registry pull retries

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::<ostree_ext::containers_image_proxy::Error>(),
+            Some(
+                ostree_ext::containers_image_proxy::Error::RequestInitiationFailure {
+                    method,
+                    ..
+                }
+            ) if method.as_ref() == "GetBlob"
+        )
+    })
+}
+
+async fn retry_pull_operation<F, Fut, T>(
+    transport: &str,
+    mut operation: F,
+    retry_delay: Duration,
+) -> Result<T>
+where
+    F: FnMut() -> Fut,
+    Fut: std::future::Future<Output = Result<T>>,
+{
+    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<Box<ImageState>> {
+    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();

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);