mirror of
https://github.com/agent-substrate/substrate.git
synced 2026-10-02 03:24:42 +08:00
imagecache: clarify pull retry backoff comments (#1425)
## Summary Comment-only follow-ups to the review comments on #1419 (https://github.com/agent-substrate/substrate/pull/1419#pullrequestreview-comments): - **Stale identifier** ([comment](https://github.com/agent-substrate/substrate/pull/1419#discussion_r3919918156)): the retry test's doc comment still referenced `pullRetryBackoff`, which that PR split into `dedicatedRegistryBackoff` and `sharedRegistryBackoff`. It now points at the per-registry backoff picked by `retryBackoffFor`. - **Unreachable `docker.io` entry** ([comment](https://github.com/agent-substrate/substrate/pull/1419#discussion_r3919918239)): kept the entry (harmless, and a lookup by the canonical name should classify the same way), but the map comment now states plainly that production lookups only ever see `index.docker.io` because `name.ParseReference` normalizes `docker.io` and bare refs before `retryBackoffFor` runs. - **Backoff accumulation vs. deadlines** ([comment](https://github.com/agent-substrate/substrate/pull/1419#discussion_r3919918313)): confirmed the concern is bounded and documented why. The backoff is per request, so a multi-layer pull under a sustained throttle can accumulate well past one request's ~14s worst case — but every request carries the caller ctx (`remote.WithContext`), go-containerregistry's retry transport never retries a context error, so retrying overshoots a deadline by at most one backoff sleep. Production pulls run under the Run/Restore RPC ctx, which ateapi caps at `maxRPCDeadline` (10m); the router's parking budget deliberately defers to that server-side deadline. A throttled shared-registry pull therefore surfaces as the RPC's deadline error, not a hang. ## Test plan Comment-only change: `go test ./internal/imagecache/`, `go vet`, and `gofmt` all pass. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
This commit is contained in:
@@ -106,6 +106,16 @@ const (
|
||||
// including the /v2/ auth ping. The retryable status codes stay at the
|
||||
// library default, which already includes 429. Vars so tests can shrink the
|
||||
// waits.
|
||||
//
|
||||
// The backoff applies per request, so under a sustained throttle a
|
||||
// multi-layer pull can accumulate far more wall clock than one request's
|
||||
// worst case (~14s shared). The caller's ctx still bounds the total: every
|
||||
// request carries it (remote.WithContext in remoteOpts), a canceled ctx
|
||||
// fails the next attempt immediately and is never retried, so retrying
|
||||
// overshoots a deadline by at most one backoff sleep. Production pulls run
|
||||
// under the Run/Restore RPC ctx, which ateapi's resume path caps at its
|
||||
// server-wide max RPC deadline — a throttled shared-registry pull surfaces
|
||||
// as that RPC's deadline error, not a hang.
|
||||
var (
|
||||
// dedicatedRegistryBackoff covers registries where the deployment has
|
||||
// its own quota (Artifact Registry, ECR, Harbor, self-hosted, …): the
|
||||
@@ -141,9 +151,13 @@ var (
|
||||
// the small stable set of communal hosts beats guessing at every private
|
||||
// registry vendor.
|
||||
var sharedRegistries = map[string]bool{
|
||||
"registry.k8s.io": true,
|
||||
"registry.k8s.io": true,
|
||||
// Pulls only ever present index.docker.io here: name.ParseReference
|
||||
// normalizes docker.io (and bare refs like "ubuntu") to it before
|
||||
// retryBackoffFor runs. The docker.io entry is kept so a lookup by the
|
||||
// canonical name classifies the same way.
|
||||
"docker.io": true,
|
||||
"index.docker.io": true, // name.ParseReference normalizes docker.io to this
|
||||
"index.docker.io": true,
|
||||
"registry-1.docker.io": true,
|
||||
"quay.io": true,
|
||||
"ghcr.io": true,
|
||||
|
||||
@@ -119,9 +119,10 @@ func TestRetryBackoffFor(t *testing.T) {
|
||||
}
|
||||
|
||||
// TestEnsureImage_RetriesRateLimit proves a pull survives transient 429s.
|
||||
// go-containerregistry's retry transport, configured with pullRetryBackoff,
|
||||
// covers every request including the /v2/ auth ping — the first request a
|
||||
// throttling registry rejects (as registry.k8s.io does per source IP).
|
||||
// go-containerregistry's retry transport, configured with the per-registry
|
||||
// backoff picked by retryBackoffFor, covers every request including the /v2/
|
||||
// auth ping — the first request a throttling registry rejects (as
|
||||
// registry.k8s.io does per source IP).
|
||||
func TestEnsureImage_RetriesRateLimit(t *testing.T) {
|
||||
origBackoff := dedicatedRegistryBackoff
|
||||
dedicatedRegistryBackoff = remote.Backoff{Duration: time.Millisecond, Factor: 2.0, Jitter: 0.1, Steps: 4}
|
||||
|
||||
Reference in New Issue
Block a user