Skip to content

fix: make too many retries situations more explicit

Pawel Rozlachrequested to merge
prozlach/gcs_retries_explicit into master

What does this MR do?

Inspired by https://gitlab.com/gitlab-org/container-registry/-/jobs/11176795385

=== RUN   TestURLFor_Download
=== PAUSE TestURLFor_Download
=== CONT  TestURLFor_Download
    middleware_integration_test.go:240: 
        	Error Trace:	/builds/gitlab-org/container-[MASKED]/[MASKED]/storage/driver/middleware/googlecdn/middleware_integration_test.go:240
        	Error:      	Received unexpected error:
        	            	gcs: Post "https://storage.googleapis.com/upload/storage/v1/b/container-[MASKED]-cdn-bucket/o?alt=json&name=tmp%2FTestURLFor_Download3417259802%2F001%2Ffoo%2Fbar&prettyPrint=false&projection=full&uploadType=multipart": oauth2: cannot fetch token: Post "https://oauth2.googleapis.com/token": http2: client conn could not be established
        	Test:       	TestURLFor_Download

We need a solid confirmation in cases like the one above that the number of retries was exhausted so that we know that there is no problem with the retry-ing code itself. The fix is simple - we just wrap the error emitted by the retry function with the info that number of retries was exhausted.

Author checklist

  • Assign one of conventional-commit prefixes to the MR.
    • fix: Indicates a bug fix, triggers a patch release.
    • feat: Signals the introduction of a new feature, triggers a minor release.
    • perf: Focuses on performance improvements that don't introduce new features or fix bugs, triggers a patch release.
    • docs: Updates or changes to documentation. Does not trigger a release.
    • style: Changes that do not affect the code's functionality. Does not trigger a release.
    • refactor: Modifications to the code that do not fix bugs or add features but improve code structure or readability. Does not trigger a release.
    • test: Changes related to adding or modifying tests. Does not trigger a release.
    • chore: Routine tasks that don't affect the application, such as updating build processes, package manager configs, etc. Does not trigger a release.
    • build: Changes that affect the build system or external dependencies. May trigger a release.
    • ci: Modifications to continuous integration configuration files and scripts. Does not trigger a release.
    • revert: Reverts a previous commit. It could result in a patch, minor, or major release.
  • Feature flags
    • This change does not require a feature flag
    • Added feature flag: ( Add the Feature flag tracking issue link here )
  • Unit-tests
    • Unit-tests are not required
    • I added unit tests
  • Documentation:
  • database changes including schema/background migrations:
    • Change does not introduce database changes
    • MR includes DB chagnes
      • Do not include code that depends on the schema migrations in the same commit. Split the MR into two or more.
      • Do not include code that depends on background migrations in the same release.
      • Manually run up and down migrations in a postgres.ai production database clone and post a screenshot of the result here.
      • If adding new schema migrations make sure the REGISTRY_SELF_MANAGED_RELEASE_VERSION CI variable in migrate.yml is pointing to the latest GitLab self-managed released registry version. Find the correct registry version here. Make sure to select the branch of the latest GitLab release.
      • If adding new queries, extract a query plan from postgres.ai and post the link here. If changing existing queries, also extract a query plan for the current version for comparison.
        • I do not have access to postgres.ai and have made a comment on this MR asking for these to be run on my behalf.
      • If adding new background migration, follow the guide for performance testing new background migrations and add a report/summary to the MR with your analysis.
  • Ensured this change is safe to deploy to individual stages in the same environment (cny -> prod). State-related changes can be troublesome due to having parts of the fleet processing (possibly related) requests in different ways.
  • If the change contains a breaking change, apply the breaking change label.
  • If the change is considered high risk, apply the label high-risk-change
  • Changes cannot be rolled back
    • Change can be safelly rolled back
    • Change can't be safelly rolled back
      • Apply the label cannot-rollback.
      • Add a section to the MR description that includes the following details:
        • The reasoning behind why a release containing the presented MR can not be rolled back (e.g. schema migrations or changes to the FS structure)
        • Detailed steps to revert/disable a feature introduced by the same change where a migration cannot be rolled back. (note: ideally MRs containing schema migrations should not contain feature changes.)
        • Ensure this MR does not add code that depends on these changes that cannot be rolled back.
Documentation/resources

Code review guidelines

Go Style guidelines

Reviewer checklist

  • Ensure the commit and MR tittle are still accurate.
  • If the change contains a breaking change, verify the breaking change label.
  • If the change is considered high risk, verify the label high-risk-change
  • Identify if the change can be rolled back safely. (note: all other reasons for not being able to rollback will be sufficiently captured by major version changes).

Merge request reports