Skip to content

Retry throttled bootstrap data requests - #283

Open
Wenxuan W (wenxuan0923) wants to merge 5 commits into
mainfrom
wenx/retry-bootstrap-data-429
Open

Retry throttled bootstrap data requests#283
Wenxuan W (wenxuan0923) wants to merge 5 commits into
mainfrom
wenx/retry-bootstrap-data-429

Conversation

@wenxuan0923

Copy link
Copy Markdown
Collaborator

Summary

  • call listBootstrapData through the generated armcontainerservice/v9 AgentPoolsClient
  • configure the Azure SDK retry policy for HTTP 429 responses and Retry-After
  • bound retries to 12 hours and 30,000 attempts for the planned 30k-node scale-out
  • use the SDK for authentication refresh, response handling, and typed response parsing

Context

AKS RP is adding subscription-scoped throttling to listBootstrapData with a 100-request burst and one-token-per-second refill. Without client retries, a throttled first-boot request exits before writing bootstrap data and the node does not join.

Validation

  • go test ./...
  • go vet ./...
  • go test -race ./pkg/bootstrapdata
  • golangci-lint run --timeout=5m (v2.13.0)
  • git diff --check

Copilot AI lite review requested due to automatic review settings August 20, 2026 22:59

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates the bootstrap-data fetch path to use the generated armcontainerservice/v9 AgentPoolsClient for listBootstrapData, and configures Azure SDK retry behavior to handle subscription-scoped HTTP 429 throttling (including Retry-After) with a bounded overall retry window sized for large scale-out.

Changes:

  • Replace the manual HTTP POST + JSON parsing with armcontainerservice/v9 typed client calls (ListBootstrapData).
  • Add a 429-specific Azure SDK retry policy and cap overall retry duration with a 12-hour context deadline.
  • Update tests to inject a custom transport and validate 429 retry vs. non-429 non-retry behavior; add the new SDK dependency.

Reviewed changes

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

File Description
pkg/bootstrapdata/bootstrap_data.go Switches to the ARM AgentPoolsClient + typed response and configures 429 retry/backoff + overall retry timeout.
pkg/bootstrapdata/bootstrap_data_test.go Updates dependency injection to use policy.Transporter and adds retry-focused tests.
go.mod Adds armcontainerservice/v9 dependency.
go.sum Updates checksums for new Azure SDK modules pulled in by armcontainerservice/v9.

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

Comment thread pkg/bootstrapdata/bootstrap_data.go
Comment thread pkg/bootstrapdata/bootstrap_data.go
Comment thread pkg/bootstrapdata/bootstrap_data.go Outdated
Comment thread pkg/bootstrapdata/bootstrap_data.go Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

Suppressed comments (2)

pkg/bootstrapdata/bootstrap_data_test.go:43

  • Test currently expects the outgoing Authorization header to be "******", but the Azure SDK's bearer token policy sets the real header value (e.g. "Bearer ") on the request before it reaches the transport. This makes the test assertion incorrect and can hide regressions where auth isn't actually being applied.
		if request.Header.Get("Authorization") != "Bearer arm-token" {
			t.Error("missing token")
		}

pkg/bootstrapdata/bootstrap_data.go:40

  • policy.RetryOptions.MaxRetries is the number of retries (additional attempts), not total attempts. With maxThrottleRetries = 30_000, a single call can make up to 30,001 HTTP requests (1 initial + 30,000 retries), which doesn't match the PR description's "30,000 attempts" bound. If the intent is a hard cap of 30,000 total attempts, reduce MaxRetries by 1 (or adjust wording to avoid confusion).
	// The RP bucket refills at one request per second. Twelve hours lets a
	// 30,000-node scale-out drain with headroom while keeping retries bounded.
	maxThrottleRetries          = 30_000
	initialThrottleRetryDelay   = time.Second

Comment thread pkg/bootstrapdata/bootstrap_data.go

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 4 changed files in this pull request and generated no new comments.

Suppressed comments (2)

pkg/bootstrapdata/bootstrap_data_test.go:43

  • TestFetchAndWrite asserts Authorization equals "******", but the credential in this test returns the token "arm-token". The Azure SDK's bearer-token policy should send the real header value (e.g. "Bearer arm-token"); expecting a masked value makes the test incorrect (or would hide a real auth regression).
		if request.Header.Get("Authorization") != "Bearer arm-token" {
			t.Error("missing token")
		}

pkg/bootstrapdata/bootstrap_data.go:304

  • addRetryAfterJitter sets only Retry-After-Ms. The Azure SDK retry policy commonly prioritizes x-ms-retry-after-ms when present; setting it as well makes the jitter effective even if a service starts returning the Azure-specific header, without changing the existing behavior for Retry-After.
	window := min(initialThrottleRetryJitter*time.Duration(1<<min(attempt, 4)), maxThrottleRetryJitter)
	delay := time.Duration(retryAfterSeconds)*time.Second + jitter(window)
	response.Header.Set("Retry-After-Ms", strconv.FormatInt(delay.Milliseconds(), 10))
	return true

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants