diff --git a/crates/lib/src/deploy.rs b/crates/lib/src/deploy.rs index b361a3b79..2a3d300b4 100644 --- a/crates/lib/src/deploy.rs +++ b/crates/lib/src/deploy.rs @@ -48,6 +48,7 @@ use std::collections::HashSet; use std::io::{BufRead, Write}; use std::os::fd::AsFd; use std::process::Command; +use std::time::Duration; use anyhow::{Context, Result, anyhow}; use bootc_utils::skopeo_bin; @@ -76,6 +77,13 @@ use crate::utils::async_task_with_spinner; // TODO use https://github.com/ostreedev/ostree-rs-ext/pull/493/commits/afc1837ff383681b947de30c0cefc70080a4f87a const BASE_IMAGE_PREFIX: &str = "ostree/container/baseimage/bootc"; +// 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); + /// Create an ImageProxyConfig with bootc's user agent prefix set. /// /// This allows registries to distinguish "image pulls for bootc client runs" @@ -769,8 +777,59 @@ pub(crate) async fn pull_from_prepared( Ok(Box::new((*import).into())) } -/// 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" { + return false; + } + + // The legacy GetBlob proxy method does not preserve a typed distinction + // 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| { + matches!( + source.downcast_ref::(), + Some( + ostree_ext::containers_image_proxy::Error::RequestInitiationFailure { + method, + .. + } + ) if method.as_ref() == "GetBlob" + ) + }) +} + +async fn retry_pull_operation( + transport: &str, + mut operation: F, + retry_delay: Duration, +) -> Result +where + F: FnMut() -> Fut, + Fut: std::future::Future>, +{ + for attempt in 1..=PULL_MAX_ATTEMPTS { + match operation().await { + Ok(value) => return Ok(value), + Err(error) + if attempt < PULL_MAX_ATTEMPTS && is_retryable_pull_error(transport, &error) => + { + tracing::warn!( + attempt, + max_attempts = PULL_MAX_ATTEMPTS, + retry_delay_seconds = retry_delay.as_secs(), + error = %error, + "Container image pull failed; retrying" + ); + tokio::time::sleep(retry_delay).await; + } + Err(error) => return Err(error), + } + } + unreachable!("the pull attempt range is non-empty") +} + +async fn pull_once( repo: &ostree::Repo, imgref: &ImageReference, target_imgref: Option<&OstreeImageReference>, @@ -810,6 +869,32 @@ pub(crate) async fn pull( } } +/// Wrapper for pulling a container image, wiring up status output. +pub(crate) async fn pull( + repo: &ostree::Repo, + imgref: &ImageReference, + target_imgref: Option<&OstreeImageReference>, + quiet: bool, + prog: ProgressWriter, + booted_deployment: Option<&ostree::Deployment>, +) -> Result> { + retry_pull_operation( + &imgref.transport, + || { + pull_once( + repo, + imgref, + target_imgref, + quiet, + prog.clone(), + booted_deployment, + ) + }, + PULL_RETRY_DELAY, + ) + .await +} + pub(crate) async fn wipe_ostree(sysroot: Sysroot) -> Result<()> { tokio::task::spawn_blocking(move || { sysroot @@ -1403,6 +1488,99 @@ pub(crate) fn fixup_etc_fstab(root: &Dir) -> Result<()> { mod tests { use super::*; + fn get_blob_failure(message: &str) -> anyhow::Error { + let error = ostree_ext::containers_image_proxy::Error::RequestInitiationFailure { + method: "GetBlob".into(), + error: message.into(), + }; + anyhow::Error::from(error).context("Unencapsulating base") + } + + #[tokio::test] + async fn test_retry_pull_operation_succeeds() -> Result<()> { + let attempts = std::cell::Cell::new(0); + let value = retry_pull_operation( + "registry", + || { + let attempt = attempts.get() + 1; + attempts.set(attempt); + async move { + if attempt < PULL_MAX_ATTEMPTS { + Err(get_blob_failure("502 Bad Gateway")) + } else { + Ok(42) + } + } + }, + Duration::ZERO, + ) + .await?; + + assert_eq!(value, 42); + assert_eq!(attempts.get(), PULL_MAX_ATTEMPTS); + Ok(()) + } + + #[tokio::test] + async fn test_retry_pull_operation_stops_after_max_attempts() { + let attempts = std::cell::Cell::new(0); + let error = retry_pull_operation( + "registry", + || { + attempts.set(attempts.get() + 1); + async { Err::<(), _>(get_blob_failure("blob unknown")) } + }, + Duration::ZERO, + ) + .await + .unwrap_err(); + + assert_eq!(attempts.get(), PULL_MAX_ATTEMPTS); + assert_eq!( + error.root_cause().to_string(), + "failed to invoke method GetBlob: blob unknown" + ); + } + + #[tokio::test] + async fn test_retry_pull_operation_does_not_retry_other_errors() { + let attempts = std::cell::Cell::new(0); + let error = retry_pull_operation( + "registry", + || { + attempts.set(attempts.get() + 1); + async { Err::<(), _>(anyhow!("invalid image configuration")) } + }, + Duration::ZERO, + ) + .await + .unwrap_err(); + + assert_eq!(attempts.get(), 1); + assert_eq!(error.to_string(), "invalid image configuration"); + } + + #[tokio::test] + async fn test_retry_pull_operation_does_not_retry_local_storage() { + let attempts = std::cell::Cell::new(0); + let error = retry_pull_operation( + "containers-storage", + || { + attempts.set(attempts.get() + 1); + async { Err::<(), _>(get_blob_failure("local storage unavailable")) } + }, + Duration::ZERO, + ) + .await + .unwrap_err(); + + assert_eq!(attempts.get(), 1); + assert_eq!( + error.root_cause().to_string(), + "failed to invoke method GetBlob: local storage unavailable" + ); + } + #[test] fn test_new_proxy_config_user_agent() { let config = new_proxy_config(); diff --git a/crates/lib/src/install.rs b/crates/lib/src/install.rs index 3c5182fd7..f42475c1e 100644 --- a/crates/lib/src/install.rs +++ b/crates/lib/src/install.rs @@ -195,7 +195,7 @@ use crate::bootc_composefs::{ use crate::bootc_kargs::{INITRD_ARG_PREFIX, ROOTFLAGS_KEY}; use crate::boundimage::{BoundImage, ResolvedBoundImage}; use crate::containerenv::ContainerExecutionInfo; -use crate::deploy::{MergeState, PreparedPullResult, prepare_for_pull, pull_from_prepared}; +use crate::deploy::{MergeState, PreparedPullResult, pull, pull_from_prepared}; use crate::install::config::Filesystem as FilesystemEnum; use crate::lsm; use crate::progress_jsonl::ProgressWriter; @@ -1073,26 +1073,34 @@ async fn install_container( // Auto-detection (None) is only appropriate for upgrade/switch on a running system. let use_unified = state.target_opts.unified_storage_exp; - let prepared = if use_unified { + let pulled_image = if use_unified { tracing::info!("Using unified storage path for installation"); - crate::deploy::prepare_for_pull_unified( + let prepared = crate::deploy::prepare_for_pull_unified( repo, &spec_imgref, Some(&state.target_imgref), storage, None, ) - .await? - } else { - prepare_for_pull(repo, &spec_imgref, Some(&state.target_imgref), None).await? - }; - - let pulled_image = match prepared { - PreparedPullResult::AlreadyPresent(existing) => existing, - PreparedPullResult::Ready(image_meta) => { - crate::deploy::check_disk_space_ostree(repo, &image_meta, &spec_imgref)?; - pull_from_prepared(&spec_imgref, false, ProgressWriter::default(), *image_meta).await? + .await?; + match prepared { + PreparedPullResult::AlreadyPresent(existing) => existing, + PreparedPullResult::Ready(image_meta) => { + crate::deploy::check_disk_space_ostree(repo, &image_meta, &spec_imgref)?; + pull_from_prepared(&spec_imgref, false, ProgressWriter::default(), *image_meta) + .await? + } } + } else { + pull( + repo, + &spec_imgref, + Some(&state.target_imgref), + false, + ProgressWriter::default(), + None, + ) + .await? }; repo.set_disable_fsync(false);