registry pull retries - #2443
Conversation
Direct image pulls can fail when a registry transiently rejects the GetBlob request. Retry that narrowly classified failure at the whole-pull boundary with a bounded attempt count, while allowing non-registry and unrelated failures to return immediately. Related: bootc-dev#2177 Assisted-by: AI Signed-off-by: HarshwardhanPatil07 <harshpat@redhat.com>
The non-unified installation path prepared and pulled images directly, bypassing the shared retry boundary and leaving bootc install to-filesystem exposed to transient registry failures. Route that path through the retrying pull while preserving unified-storage behavior. Related: bootc-dev#2177 Assisted-by: AI Signed-off-by: HarshwardhanPatil07 <harshpat@redhat.com>
| } | ||
| } | ||
| } else { | ||
| pull( |
There was a problem hiding this comment.
Won't this skip the disk space check?
There was a problem hiding this comment.
It's also losing the separation between "prepare" and "pull" for the non-unified path.
prepare is "fetch manifest to use for change detection", "pull" = "download whole image".
There's a bigger picture issue here in that we're not doing retries for the first, but we need to in the general case as we can hit flakes there too (DNS, TCP etc).
|
|
||
| async fn retry_pull_operation<F, Fut, T>( | ||
| transport: &str, | ||
| mut operation: F, |
There was a problem hiding this comment.
Do we need this argument? It's just pull_once that's going to be called right?
| // between transient registry failures and errors such as a missing blob. | ||
| // Retry the opaque registry error at this top-level boundary, with the | ||
| // attempt limit above preventing an unbounded delay. | ||
| error.chain().any(|source| { |
There was a problem hiding this comment.
Not too sure about this. What happens if the image itself doesn't exist? Will we still keep retrying?
There was a problem hiding this comment.
Yes I think this should only use https://github.com/bootc-dev/containers-image-proxy-rs/blob/154f4868c2fbf0ea60bad91651389260e5f1e948/src/imageproxy.rs#L91
| Err(error) => return Err(error), | ||
| } | ||
| } | ||
| unreachable!("the pull attempt range is non-empty") |
There was a problem hiding this comment.
Let's have this be an error instead
| async fn retry_pull_operation<F, Fut, T>( | ||
| transport: &str, | ||
| mut operation: F, | ||
| retry_delay: Duration, |
There was a problem hiding this comment.
PULL_MAX_ATTEMPTS is used as is, why does this need to be an argument?
| // between transient registry failures and errors such as a missing blob. | ||
| // Retry the opaque registry error at this top-level boundary, with the | ||
| // attempt limit above preventing an unbounded delay. | ||
| error.chain().any(|source| { |
There was a problem hiding this comment.
Yes I think this should only use https://github.com/bootc-dev/containers-image-proxy-rs/blob/154f4868c2fbf0ea60bad91651389260e5f1e948/src/imageproxy.rs#L91
| // Match the default attempt count and delay used by the Justfile's build-fetch | ||
| // retry helper. A failed attempt has to rebuild the importer, so retries are | ||
| // intentionally made at the whole-pull boundary instead of independently for | ||
| // every layer. | ||
| const PULL_MAX_ATTEMPTS: u32 = 3; | ||
| const PULL_RETRY_DELAY: Duration = Duration::from_secs(30); |
There was a problem hiding this comment.
Conceptually I think we should be matching what e.g. podman does by default. What our build system happens to do is a different unrelated thing.
On that topic see podman-container-tools/container-libs#951
In the short term, we can just copy the same defaults and add a link to that issue as a TODO to allow having bootc be configurable in the same way.
| /// Wrapper for pulling a container image, wiring up status output. | ||
| pub(crate) async fn pull( | ||
| fn is_retryable_pull_error(transport: &str, error: &anyhow::Error) -> bool { | ||
| if transport != "registry" { |
There was a problem hiding this comment.
I think it'd be cleaner to skip the retry loop at a higher level
| } | ||
| } | ||
| } else { | ||
| pull( |
There was a problem hiding this comment.
It's also losing the separation between "prepare" and "pull" for the non-unified path.
prepare is "fetch manifest to use for change detection", "pull" = "download whole image".
There's a bigger picture issue here in that we're not doing retries for the first, but we need to in the general case as we can hit flakes there too (DNS, TCP etc).
3 participants
What
Add bounded retries for registry-backed OSTree image pulls when the containers-image proxy reports a
GetBlobrequest-initiation failure.This also routes the normal, non-unified OSTree installation path through the shared retrying pull function.
Related: #2177
Why
CI has intermittently failed during commands such as
bootc install to-filesystemwhen registries like Quay return transient errors, including502 Bad Gatewayand network timeouts.The existing CI retry wrapper covers preparatory Podman operations, but it does not cover image pulls performed internally by bootc. Consequently, one temporary registry failure can fail the entire integration job.
Retries are appropriate here because registry availability is outside bootc's control and repeating an image pull is safe.
The proxy error does not expose the underlying HTTP status. Therefore, bootc cannot distinguish a transient
502from every permanentGetBlobinitiation failure. The retry count bounds the worst-case additional delay to 60 seconds.