oci: Retry transient registry failures when pulling - #1
cgwalters-bot wants to merge 5 commits into
Conversation
39c9788 to
4e85e06
Compare
| /// error, so this is necessarily a heuristic. It is modelled on | ||
| /// `IsErrorRetryable()` in containers/common `pkg/retry`. Truncated or | ||
| /// corrupted blob transfers are matched separately by [`is_truncated_blob`]. | ||
| const TRANSIENT_ERROR_PATTERNS: &[&str] = &[ |
There was a problem hiding this comment.
This is too ugly, we need to reuse the logic from the proxy, is that not possible?
There was a problem hiding this comment.
Yes. Since 1.19, skopeo tags every failed reply with c/common's IsErrorRetryable() verdict (error_code: "retryable"), but containers-image-proxy-rs was dropping it. cgwalters-forge/containers-image-proxy-rs#2 exposes it as Error::is_retryable(), and retry.rs now relies on that alone, so the error-string patterns, the jitter and the delay cap are all gone. That leaves podman's 1s/2s/4s loop, which is short, and neither proxy-rs nor composefs-rs had one to reuse. This needs a proxy-rs release. Until then, the new second commit pulls proxy-rs in from git; the rebased and squashed branch is at 9dbdcf0.
Generated-by: https://github.com/cgwalters/#llms
4e85e06 to
9dbdcf0
Compare
cgwalters-bot
left a comment
There was a problem hiding this comment.
Review guide for head 9dbdcf02cd: where to look closely and what is safe to skim. It is advice from the bot's reviewer and doesn't replace reading the diff; the review app walks it.
Retries transient registry failures during docker:// pulls. Retries are per proxy operation (OpenImage, manifest, config, each layer and delta blob) on podman's default schedule: 3 retries after 1s, 2s and 4s. The only classifier is containers-image-proxy's new Error::is_retryable() from forge proxy-rs#2, so no error-text matching is left. With skopeo older than 1.19, nothing is retried. The [patch.crates-io] commit is temporary and must become a 0.11.x version bump once proxy-rs is released. The main risk is the new integration test. It needs skopeo 1.19 or later but only checks that skopeo exists. The upstream Integration job runs the unprivileged tests on the ubuntu-24.04 host, whose skopeo (from plucky) is 1.13, so the test will fail there. Also look at the layer import refactor. The proxy driver is now always awaited, and nothing is registered until the proxy has verified the blob. Digest mismatches are no longer retried, which matches podman.
Hotspots
- look-closely · api —
Cargo.toml:60-63(a85d634): Temporary git patch pointing at the forge fork. Before merge, drop it for a containers-image-proxy 0.11.x version bump in both the composefs-oci and integration-tests Cargo.toml. crates.io 0.11.0 has no is_retryable(). - risky · test-gap —
crates/composefs-integration-tests/src/tests/registry_retry.rs:275-279(9dbdcf0): This only checks that skopeo exists, but the test needs skopeo 1.19 or later. The upstream Integration job runs unprivileged tests on the ubuntu-24.04 host with skopeo 1.13 from plucky, so the first case will fail. Skip when skopeo --version is below 1.19. - look-closely · logic —
crates/composefs-oci/src/retry.rs:122-157(8565d72): The retry loop itself: the attempt count, the backoff and the give-up context. max_retries=3 means 4 attempts. - look-closely · error-handling —
crates/composefs-oci/src/retry.rs:82-111(8565d72): ProxyAndImportError, plus its special case in is_transient, is the only machinery beyond the loop. Wrapping the proxy error with the import error as anyhow context would keep the proxy error in the chain and remove the need for this type. - note · logic —
crates/composefs-oci/src/skopeo.rs:102-111(8565d72): Only Transport::Registry retries; local transports get RetryPolicy::none(). - look-closely · logic —
crates/composefs-oci/src/skopeo.rs:531-568(8565d72): Per-layer retry. Started is sent again for each attempt, and fetch() must start a fresh GetBlob every time. The semaphore permit stays held during backoff sleeps. - look-closely · logic —
crates/composefs-oci/src/skopeo.rs:626-676(8565d72): Behavior change from main: the driver is now awaited even when the import failed. The reader is dropped first, so skopeo gets EPIPE and FinishPipe returns instead of hanging. The layer is only registered after the driver reports success. - note · api —
crates/composefs-oci/src/lib.rs:349-353(8565d72): New pub field on PullOptions, which is not non_exhaustive, so struct literals without ..Default::default() break. pull_image() also retries by default now. - note · logic —
crates/composefs-ctl/src/lib.rs:169-177(c2afac4): When stderr is not a tty, every progress message now goes to it. This is intended, so that retry warnings reach CI logs.
Safe to skim
crates/composefs-oci/src/retry.rs:159-374: Table-driven unit testscrates/composefs-oci/src/skopeo.rs:798-1094: Unit tests with a scripted fake fetchercrates/composefs-integration-tests/src/tests/registry_retry.rs:1-270: Minimal flaky HTTP registry fixturecrates/composefs-integration-tests/src/tests/zstd_chunked.rs: have_skopeo() moved to main.rscrates/composefs-oci/Cargo.toml: Adds the tokio time feature
9dbdcf0 to
39b7496
Compare
Avoid indexing arrays when parsing the split cmdline, instead use CStr::from_bytes_until_nul() and iterators. Signed-off-by: Alexander Larsson <alexl@redhat.com>
indicatif hides progress bars when stderr is not a terminal, and MultiProgress::println() then silently discards the message. So status messages from a pull never show up in CI logs, which is where the upcoming retry warnings for flaky registries matter most. Note this means scripted (non-tty) callers now also see messages like "Fetching config ..." on stderr. Generated-by: AI Signed-off-by: Colin Walters <walters@verbum.org>
Prep for retrying pulls, which needs the proxy's classification of retryable errors. This is only until a containers-image-proxy release includes it; then it becomes a plain version bump. Generated-by: AI Signed-off-by: Colin Walters <walters@verbum.org>
Pulls through skopeo have no retries at all, so a single 503 or dropped connection from e.g. quay.io fails the whole pull; this is a regular source of CI flakes for bootc. The proxy doesn't retry for us: in podman the retry loop lives in the caller (c/common pkg/retry), so do the same here. Which errors are worth retrying is the proxy's call: since skopeo 1.19 it classifies each failure with c/common's IsErrorRetryable(), the heuristic podman's retry loop uses, and containers-image-proxy exposes that as Error::is_retryable(). With an older skopeo nothing is retried. Only errors from the proxy are considered; a local failure to import verified data is never retried. Retries happen per proxy operation (opening the image, fetching the manifest, the config, and each layer or delta blob), so a failure in one layer doesn't refetch the others. The schedule is podman's default: 3 retries after 1s, 2s and 4s, or a fixed delay like its --retry-delay; PullOptions::retry tunes or disables it. Only registry (docker://) pulls retry. Note pull_image() now retries with the default policy too. A layer is still only registered once the proxy has verified the whole blob, and each attempt starts a fresh request. We now also wait for the proxy's verdict when the import itself failed, since a broken transfer is then usually the root cause and decides whether to retry; the import error is attached to it as context. Since each attempt restarts the transfer, a layer's Started event is sent again per attempt, and cfsctl replaces that layer's progress bar. Retries are reported as progress messages, which is how cfsctl shows them, and only logged at debug level so they don't appear twice. Generated-by: AI Closes: composefs#348 Signed-off-by: Colin Walters <walters@verbum.org>
Expose the retry count for registry pulls, so it can be raised for flaky environments or set to 0 to fail fast. The varlink API keeps the default policy for now. The integration test serves a local OCI layout from a minimal in-process registry that answers the first manifest request with a 503 and hangs up on the first request for each blob (skopeo retries neither itself), and checks that the pull recovers and yields the pinned image ID, while --retry 0 fails. This goes through the real skopeo proxy, so it also checks that skopeo classifies both failures as retryable. That classification needs skopeo 1.19, so the test is skipped with older ones (e.g. Ubuntu 24.04 has 1.13); the skopeo version check is now shared by all integration tests. Generated-by: AI Signed-off-by: Colin Walters <walters@verbum.org>
39b7496 to
773334d
Compare
|
Signed off 4 commit(s) with |
773334d to
a594bfe
Compare
|
Opened upstream as composefs#407 (draft). Closing this review draft. |
Pulls through skopeo have no retries at all, so a single 503 or dropped connection from e.g. quay.io fails the whole pull. That's a regular source of CI flakes for bootc (bootc-dev/bootc#2177). The proxy doesn't retry for us: in podman the retry loop lives in the caller (c/common
pkg/retry), so this adds one here.The proxy decides which errors are worth retrying. Since skopeo 1.19 it tags each failed reply with c/common's
IsErrorRetryable()verdict, the same check podman's retry loop uses, and cgwalters-forge/containers-image-proxy-rs#2 exposes that asError::is_retryable(). composefs-rs does no error-text matching of its own. With an older skopeo nothing gets retried, same as today. Only proxy errors count, so a local failure to import verified data is never retried.Retries happen per proxy operation: opening the image, fetching the manifest, the config, and each layer or delta blob. That way a failed layer doesn't refetch the others. The schedule is podman's default, 3 retries after 1s, 2s and 4s, and a fixed delay can be set like
--retry-delay. Onlydocker://pulls retry.PullOptionsgains a publicretry: RetryPolicyfield to tune or disable it. SincePullOptionsisn't#[non_exhaustive], code that builds it with a struct literal and no..Default::default()needs updating.cfsctl oci pull --retry Nexposes the count (--retry 0fails fast); the varlink API keeps the default. Note thatpull_image()now retries by default too. Each attempt restarts the blob transfer, so a layer'sStartedprogress event is sent again per attempt, and cfsctl replaces that layer's bar.The first commit makes cfsctl print progress messages when stderr isn't a terminal. indicatif drops them otherwise, so retry warnings would never show up in CI logs. It could land on its own.
Before merging: the second commit points
containers-image-proxyat the forge branch through[patch.crates-io]. The plan is to land cgwalters-forge/containers-image-proxy-rs#2 and release it as 0.11.1 first, then replace the[patch]with a version bump. The retry loop stays in composefs-rs for now; bootc already has its own, and switching bootc's classifier tois_retryable()is a separate follow-up (bootc-dev/bootc#2466).The new integration test serves an OCI layout from a small in-process registry. The registry answers the first manifest request with a 503 and hangs up on the first request for each blob. The test checks that
cfsctl oci pull docker://...recovers and yields the pinned image ID, and that--retry 0fails. It goes through the real skopeo, so it also checks that skopeo classifies both failures as retryable. That classification needs skopeo 1.19, so the test parsesskopeo --versionand skips with older versions (upstream's ubuntu-24.04 runners have 1.13.3). The skopeo check is now one shared helper in the integration tests'main.rs; before,old_format.rshad its own copy.Testing ran on a 16-core devspace at head 39b7496, with proxy-rs pulled from the forge branch. Two containers were used:
quay.io/fedora/fedora:latest(rustc 1.98, skopeo 1.22.3):just fmt-check,just clippyandjust check-feature-comboswere clean, andcargo test -p composefs-oci -p composefs-ctlpassed (174 composefs-oci tests, including the retry andfetch_layerunit tests).just test-integrationpassed 112, includingtest_pull_retries_transient_registry_errorsand the newtest_parse_skopeo_version. It failedtest_ostree_pull_local_all_modesandtest_varlink_open_repository_invalid_spec, and main fails the same two in that container.ubuntu:24.04(skopeo 1.13.3, rust stable), as in upstream's smoke job: the retry test reportsskopeo (1, 13, 3) does not classify retryable errors (needs (1, 19, 0)), skipping registry retry testand passes, andtest_parse_skopeo_versionpasses. I stopped the full unprivileged suite there after it sat for 30 minutes with idlecfsctlprocesses. I didn't dig into that, and main wasn't run there for comparison.The fs-verity-dependent unit tests,
just check-fuzzand the privileged VM tests weren't run for this revision. The earlier revision passedcheck-fuzz.Closes composefs#348
Generated-by: https://github.com/cgwalters/#llms
Review draft in cgwalters-forge, not upstream yet. This section is removed when the PR is opened upstream.
composefs/composefs-rs, basemainPVTI_lAHOAQ_SPs4Bj2Gizg8PhYkTo review:
/promoteon a line of its own, to open it upstream, ready for review. Either covers only the commits pushed so far.Signed-off-by: Colin Walters <walters@verbum.org>to the commits lacking it (the bot's and yours; anyone else's only if you ask), with you as committer./draftline (in the same comment or before) to open it upstream as a draft (/readyundoes that).