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.

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>

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

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 2 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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants