kubernetes: jitter dial backoff so builders do not retry in lockstep

[1991santhu]

The problem

calculateBackoff is a pure function of the attempt number:

return min(time.Duration(1<<uint(attempt))*baseDelay, maxDelay)

so every builder retrying the same condition waits for exactly 500ms, 1s, 2s, 4s, 8s.

That matters specifically because of the condition this retry exists to handle. From the comment on tryWithBackoff:

// This handles the race condition where Kubernetes marks nodes as "Ready" before their
// Certificate Signing Requests (CSRs) are approved, causing transient TLS errors.

CSR approval lag is a cluster-wide, time-correlated event rather than a per-pod accident. When a node group comes up, every build scheduled onto those nodes hits the transient TLS error at roughly the same moment — and then retries at identical instants. In CI, where parallel builds are the normal case, that concentrates retries on the API server precisely while it is still working through the approval backlog.

The backoff limits how often each builder retries. It does not stop builders retrying together.

The change

Equal jitter — retain half the computed interval as a floor, randomise the remainder:

d := min(time.Duration(1<<uint(attempt))*baseDelay, maxDelay)
half := d / 2

return half + time.Duration(rand.Int63n(int64(d-half)+1))

Half is kept rather than using full jitter so a retry is never issued immediately after a TLS failure. The result never exceeds maxDelay, so no builder waits longer than it does today — the change only removes the alignment.

Testing

go test -count=2 ./driver/kubernetes/... passes, go vet clean.

The package had no test file, so this adds one covering: the result stays within [d/2, d] for attempts 0–5 across 500 samples each; a large attempt count stays capped at maxDelay; and successive calls vary, which fails if the implementation regresses to deterministic.

Note

math/rand rather than crypto/rand — this is load spreading, not a security boundary. Happy to change if the project prefers otherwise. Context for the original retry: #2668.

[1991santhu]

Corrected the direction of the jitter in my own patch.

It was equal jitter, [d/2, d]. At attempt 0 that lets a retry fire at 250ms when baseDelay is 500ms, so it hits the API server sooner than configured while it's still working through the CSR backlog. It adds now, [d, 2d], bounded by the headroom to maxDelay. With maxRetries=5 the delays used are 500ms through 4s, so the 10s cap never comes into play.

I ran the numbers rather than just asserting the problem. 100 builders hitting the same CSR lag: without jitter all 100 dial inside the same 100ms window on every attempt, with it about 28 on the first retry and 7 by the fourth. Simulated arrival times, not something I've measured on a real cluster.

go test -count=2 -run TestCalculateBackoff ./driver/kubernetes/ passes.

[crazy-max]

The direction makes sense to me.

One small wording nit: this is additive jitter, [d, 2d] capped by maxDelay, not equal jitter anymore. That behavior is fine if preserving the current exponential delay as the floor is intentional, but the description/comment should call it that directly.

Also, please squash the commits before merge.

[crazy-max]

The new math/rand call should use the same gosec suppression pattern as podchooser.RandomPodChooser, since retry jitter doesn't need crypto randomness:

return d + time.Duration(rand.Int63n(int64(extra)+1)) // #nosec G404 -- no strong randomness required for retry jitter

[1991santhu]

Thanks, all three done.

Switched to the same pattern as podchooser.RandomPodChooser:

return d + time.Duration(rand.Int63n(int64(extra)+1)) // #nosec G404 -- no strong randomness required for retry jitter

You're right about the wording. The doc comment said "backoff and jitter" without saying which kind, which is worse than useless here since the whole point of the change is that the jitter is additive rather than centred. It now says so directly: additive jitter drawing from [d, 2d] capped by maxDelay, with the exponential value as the floor rather than the midpoint. That is intentional, for the reason the rest of the comment gives, so a retry can never fire sooner than the schedule alone would have.

Squashed to one commit.

Verified with golangci-lint v2.8.0 built from source against Go 1.26, matching how the lint job builds it: 0 issues. go test -count=2 -run TestCalculateBackoff ./driver/kubernetes/ passes.

[crazy-max]

@chagui if you can take a look 🙏

[chagui]

Left some suggestions.
I could not find any occurence of the problem in our environment but the PR is defensively reasonable 👍

[chagui]

Nit: at attempt 3, d is 4s, so the additive-jitter range is [4s, 8s], four seconds wide not two. I think this comment is left over from the earlier equal-jitter implementation. The test itself looks correct.

Suggested change

	// attempt 3 is two seconds wide, so one distinct value across 500 draws
	// attempt 3 is four seconds wide, so one distinct value across 500 draws

[chagui]

suggestion: took me some time to understand the function, I think we could simplify the code and use more explicit names to improve readability:

Suggested change

	d := min(time.Duration(1<<uint(attempt))*baseDelay, maxDelay)
	extra := d
	if headroom := maxDelay - d; headroom < extra {
	extra = headroom
	}
	if extra <= 0 {
	return d
	}
	return d + time.Duration(rand.Int63n(int64(extra)+1)) // #nosec G404 -- no strong randomness required for retry jitter
	delay := min(time.Duration(1<<uint(attempt))*baseDelay, maxDelay)
	jitterRange := min(delay, maxDelay-delay) // never exceed maxDelay
	if jitterRange <= 0 {
	return delay
	}
	// Floored at delay so jitter never issues a retry sooner than
	// plain exponential backoff would have.
	return delay + time.Duration(rand.Int63n(int64(jitterRange)+1)) // #nosec G404 -- no strong randomness required for retry jitter

[chagui]

duplicates tryWithBackoff (few lines above: https://github.com/1991santhu/buildx/blob/fix%2Fkubernetes-dial-backoff-jitter/driver/kubernetes/driver.go#L316-L318) docs, attaching the TLS/CSR narrative to calculateBackoff doesn't really make sense here because calculateBackoff only needs to justify its own local choice (floor vs. midpoint).

[chagui]

suggestion: doc comment is much longer than this package's convention and duplicates the caller's documentation. I'd leave the rationale behind the retries to tryWithBackoff and focus on calculateBackoff contract and non-obvious implementation choice (floor vs. midpoint).

See http://go.dev/doc/comment

Doc comments should not explain internal details such as the algorithm used in the current implementation. Those are best left to comments inside the function body.

Suggested change

	// calculateBackoff calculates the delay for the given attempt with exponential
	// backoff and additive jitter, drawing from [d, 2d] capped by maxDelay, where d
	// is the exponential value for the attempt. The exponential value is the floor
	// rather than the midpoint, so a retry is never issued sooner than the schedule
	// would have on its own.
	// calculateBackoff returns a randomized exponential backoff delay for attempt,
	// never exceeding maxDelay.

(and remove everything below this to keep it to 2 lines)