s3: expose retry/backoff constants via environment variables by alliasgher · Pull Request #296 · longhorn/backupstore

alliasgher · 2026-04-23T19:04:44Z

Which issue(s) this PR fixes:

What this PR does / why we need it:

The S3 backupstore service hardcodes three retry-related constants — AWSRetryMaxAttempts (5), AWSRetryMaximumAttempts (10), and AWSRetryMaximumBackoff (300s). Operators running against S3-compatible endpoints with different latency or reliability characteristics have no way to tune them without recompiling.

This PR adds three optional environment-variable overrides, following the existing AWS_ENDPOINTS / VIRTUAL_HOSTED_STYLE env-var pattern already used by this file:

Env var	Type	Overrides
`AWS_RETRY_MAX_ATTEMPTS`	positive integer	`AWSRetryMaxAttempts`
`AWS_RETRY_MAXIMUM_ATTEMPTS`	positive integer	`AWSRetryMaximumAttempts`
`AWS_RETRY_MAXIMUM_BACKOFF`	Go duration (`60s`, `5m`)	`AWSRetryMaximumBackoff`

Empty, missing, or malformed values silently fall back to the existing defaults, so no behaviour change for users that don't set them. Unit tests for each helper cover unset / valid / zero / negative / malformed cases.

Special notes for your reviewer:

I kept the existing exported constants unchanged so other callers (and the longhorn/longhorn integration) keep compiling.
The new helpers are lowercase / package-local since they only apply to this file's retry plumbing.
Once this lands I'm happy to open a companion PR against longhorn/longhorn exposing these as global settings / backup-target URL parameters per the issue description; figured it was cleaner to land the library-side primitive first.

Additional documentation or context

[IMPROVEMENT] expose the hardcode parameters for users in backupstore longhorn#12155

derekbit · 2026-08-16T14:23:19Z

@mantissahz Please review the PR.

Copilot

Pull request overview

Adds environment-based S3 retry tuning as the backupstore-side foundation for longhorn/longhorn#12155.

Changes:

Adds retry-attempt and backoff environment overrides.
Preserves defaults for invalid or missing values.
Adds parsing-focused unit tests.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
`s3/s3_service.go`	Defines and applies S3 retry overrides.
`s3/s3_service_retry_test.go`	Tests override parsing and fallback behavior.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

alliasgher · 2026-08-18T00:15:16Z

+			t.Setenv(EnvAWSRetryMaximumAttempts, tc.env)
+			if got := retryMaximumAttempts(); got != tc.want {
+				t.Fatalf("retryMaximumAttempts() = %d, want %d", got, tc.want)


Added in 21c2977 as TestRetryMaximumAttempts_AppliedToClient, covering both the backoff and non-backoff branches.

alliasgher · 2026-08-18T00:15:15Z

 		if retryBackoff {
 			o.Retryer = retry.NewStandard(func(so *retry.StandardOptions) {
-				so.MaxAttempts = AWSRetryMaximumAttempts
-				so.MaxBackoff = AWSRetryMaximumBackoff
+				so.MaxAttempts = retryMaximumAttempts()
+				so.MaxBackoff = retryMaximumBackoff()


Confirmed and fixed in 21c2977. Reproduced with the vendored SDK: effective MaxAttempts was 5 with AWS_RETRY_MAXIMUM_ATTEMPTS=20, and 20 once o.RetryMaxAttempts is cleared.

mantissahz

LGTM
Hi @alliasgher,
Could you check if the Copilot's comment is valid and helpful?

alliasgher · 2026-08-18T00:15:14Z

@mantissahz It is valid, and it was a real bug. Fixed in 21c2977.

I verified the mechanism rather than taking it on trust. finalizeRetryMaxAttempts runs after the option callback (api_client.go:230) and wraps the retryer when o.RetryMaxAttempts != 0, which it is, because newInstance passes config.WithRetryMaxAttempts and resolveAWSRetryMaxAttempts copies it onto the options.

Reproduced with the same SDK versions this repo vendors (s3 v1.97.3, config v1.32.13):

AWS_RETRY_MAX_ATTEMPTS=5, AWS_RETRY_MAXIMUM_ATTEMPTS=20 -> effective 5
same, with o.RetryMaxAttempts cleared                   -> effective 20

So AWS_RETRY_MAXIMUM_ATTEMPTS was a no-op above 5. Clearing the field inside the retryBackoff branch fixes it; the non-backoff path is untouched and still honours AWS_RETRY_MAX_ATTEMPTS.

Copilot's first comment was right too: the existing tests only cover the env parsing helpers, so they passed either way. Added one asserting client.Options().Retryer.MaxAttempts() for both branches. Same harness confirms it is not vacuous: 5 before the fix, 20 after.

One caveat: util uses Linux-only unix constants without build tags, so go test ./s3/ cannot run on macOS. I cross-compiled the test binary for linux/amd64 and verified the mechanism standalone, but CI will be the first real run of the new test.

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

mantissahz · 2026-08-18T11:51:41Z

+	EnvAWSRetryMaxAttempts = "AWS_RETRY_MAX_ATTEMPTS"
+	// EnvAWSRetryMaximumAttempts overrides AWSRetryMaximumAttempts when set to a positive integer.
+	EnvAWSRetryMaximumAttempts = "AWS_RETRY_MAXIMUM_ATTEMPTS"
+	// EnvAWSRetryMaximumBackoff overrides AWSRetryMaximumBackoff when set to a Go duration string (e.g. "60s", "5m").
+	EnvAWSRetryMaximumBackoff = "AWS_RETRY_MAXIMUM_BACKOFF"


Hi @alliasgher,
Could you try to add the new environment variables into this function setupS3Credential in the util/credential.go as AWSSecretKey?
Then we can allow users to set up the parameters in the Secret and pass them here.

Done in b9449f8. Added the three names to types next to the other credential keys, copied them in getS3CredentialFromEnvVars and restored them in setupS3Credential, so they can be set through the Secret like AWS_ENDPOINTS and VIRTUAL_HOSTED_STYLE. The s3 package now points at the shared constants so the names are defined once.

Correct, and fixed in b9449f8 along the lines you describe: shared constants in types/types.go, copied and restored in both S3 credential helpers. Added TestS3CredentialRoundTripCarriesRetrySettings, which fails without the change with all three keys empty on both sides of the round trip.

The S3 backupstore service currently hardcodes three retry-related constants (`AWSRetryMaxAttempts`, `AWSRetryMaximumAttempts`, `AWSRetryMaximumBackoff`) so operators running against S3-compatible endpoints with different latency/reliability characteristics have no way to tune them without recompiling the binary. Add three optional environment-variable overrides, following the existing `AWS_ENDPOINTS` / `VIRTUAL_HOSTED_STYLE` env-var pattern: * `AWS_RETRY_MAX_ATTEMPTS` — integer, overrides AWSRetryMaxAttempts * `AWS_RETRY_MAXIMUM_ATTEMPTS` — integer, overrides AWSRetryMaximumAttempts * `AWS_RETRY_MAXIMUM_BACKOFF` — Go duration (e.g. "60s", "5m"), overrides AWSRetryMaximumBackoff Empty / missing / malformed values silently fall back to the existing defaults, so there's no behaviour change for users that don't set them. Refs longhorn/longhorn#12155 Signed-off-by: Ali <alliasgher123@gmail.com> Signed-off-by: Ali <ali@kscope.ai>

NewFromConfig calls finalizeRetryMaxAttempts after the option callback. When o.RetryMaxAttempts is nonzero, which it is because newInstance passes config.WithRetryMaxAttempts, it wraps the retryer in retry.AddWithMaxAttempts. That capped the custom retryer at AWS_RETRY_MAX_ATTEMPTS, so AWS_RETRY_MAXIMUM_ATTEMPTS had no effect above 5. Clear o.RetryMaxAttempts when installing the custom retryer. The non-backoff path is unchanged and still honours AWS_RETRY_MAX_ATTEMPTS. The existing tests only cover the env parsing helpers and passed either way, so add one that asserts Retryer.MaxAttempts() on the constructed client. Signed-off-by: alliasgher <alliasgher123@gmail.com>

The retry variables are read by newInstance in whichever process performs the S3 operation, but they are configured in the calling process. The credential map is what bridges the two, and it copies AWS_ENDPOINTS, AWS_CERT, the proxies and VIRTUAL_HOSTED_STYLE but not the new keys, so they were dropped and newInstance always saw the defaults. Add the three names to types alongside the other credential keys, copy them in getS3CredentialFromEnvVars and restore them in setupS3Credential. The s3 package now references the shared constants so the names are defined once. This also lets the values be supplied through the Secret, like the existing S3 credential fields. Signed-off-by: alliasgher <alliasgher123@gmail.com>

…urable-s3-retry # Conflicts: # s3/s3_service.go # util/credential.go

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

mantissahz · 2026-08-24T01:24:12Z

+			// NewFromConfig runs finalizeRetryMaxAttempts after this callback, which
+			// wraps the retryer above in retry.AddWithMaxAttempts(o.RetryMaxAttempts)
+			// and would cap it at AWS_RETRY_MAX_ATTEMPTS. Clear it so the retryer's
+			// own AWS_RETRY_MAXIMUM_ATTEMPTS stays effective.
+			o.RetryMaxAttempts = 0


Hi @alliasgher, please check this comment.

codecov · 2026-08-23T16:19:52Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 10.63%. Comparing base (ab5179e) to head (9df154d).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #296      +/-   ##
==========================================
+ Coverage    8.49%   10.63%   +2.14%     
==========================================
  Files          24       24              
  Lines        2131     2153      +22     
==========================================
+ Hits          181      229      +48     
+ Misses       1924     1892      -32     
- Partials       26       32       +6

Flag	Coverage Δ
unittests	`10.63% <100.00%> (+2.14%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

alliasgher mentioned this pull request Apr 23, 2026

[IMPROVEMENT] expose the hardcode parameters for users in backupstore longhorn/longhorn#12155

Open

derekbit requested a review from mantissahz August 16, 2026 14:23

derekbit requested a balanced review from Copilot August 16, 2026 14:23

Copilot started reviewing on behalf of derekbit August 16, 2026 14:24 View session

Copilot AI reviewed Aug 16, 2026

View reviewed changes

mantissahz previously approved these changes Aug 17, 2026

View reviewed changes

derekbit assigned alliasgher Aug 18, 2026

alliasgher dismissed mantissahz’s stale review via 21c2977 August 18, 2026 00:15

mantissahz requested a balanced review from Copilot August 18, 2026 08:44

Copilot started reviewing on behalf of mantissahz August 18, 2026 08:45 View session

Copilot AI reviewed Aug 18, 2026

View reviewed changes

alliasgher added 2 commits August 19, 2026 00:33

derekbit force-pushed the feat/12155-configurable-s3-retry branch from 21c2977 to 7fd2623 Compare August 18, 2026 16:33

alliasgher added 2 commits August 23, 2026 16:35

Merge remote-tracking branch 'upstream/master' into feat/12155-config…

9df154d

…urable-s3-retry # Conflicts: # s3/s3_service.go # util/credential.go

derekbit requested review from mantissahz and a balanced review from Copilot August 23, 2026 16:09

Copilot started reviewing on behalf of derekbit August 23, 2026 16:10 View session

Copilot AI reviewed Aug 23, 2026

View reviewed changes

Navigation Menu

Uh oh!

s3: expose retry/backoff constants via environment variables - #296

s3: expose retry/backoff constants via environment variables#296
alliasgher wants to merge 4 commits into
longhorn:masterfrom
alliasgher:feat/12155-configurable-s3-retry

alliasgher commented Apr 23, 2026

Uh oh!

derekbit commented Aug 16, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

alliasgher Aug 18, 2026

Uh oh!

alliasgher Aug 18, 2026

Uh oh!

mantissahz left a comment

Uh oh!

alliasgher commented Aug 18, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

mantissahz Aug 18, 2026

Uh oh!

alliasgher Aug 23, 2026

Uh oh!

alliasgher Aug 23, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

mantissahz Aug 24, 2026

Uh oh!

codecov Bot commented Aug 23, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

alliasgher commented Apr 23, 2026

Which issue(s) this PR fixes:

What this PR does / why we need it:

Special notes for your reviewer:

Additional documentation or context

Uh oh!

derekbit commented Aug 16, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

alliasgher Aug 18, 2026

Choose a reason for hiding this comment

Uh oh!

alliasgher Aug 18, 2026

Choose a reason for hiding this comment

Uh oh!

mantissahz left a comment

Choose a reason for hiding this comment

Uh oh!

alliasgher commented Aug 18, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

mantissahz Aug 18, 2026

Choose a reason for hiding this comment

Uh oh!

alliasgher Aug 23, 2026

Choose a reason for hiding this comment

Uh oh!

alliasgher Aug 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

mantissahz Aug 24, 2026

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Aug 23, 2026

Codecov Report

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants