install: Add support for pulling LBIs during install

[omertuc]

Partially solves #846

This adds a new --bound-images option to bootc install which will allow the user to choose how they want to handle the retrieval of LBIs into the target's container storage.

The existing behavior, which will stay the default, is --bound-images stored which will resolve the LBIs and verify they exist in the source's container storage before copying them into the target's container storage.

The new behavior is --bound-images pull which will skip the resolution step and directly pull the LBIs into the target's container storage.

The older --skip-bound-images option (previously hidden) is now removed and replaced with the new (but still hidden) --bound-images skip option.

[cgwalters]

Thanks for working on this!

[omertuc]

On another note, I'll work on the second part of #846 (bootc image list) in a separate PR

EDIT: #871

[cgwalters]

Looking good to me!

I guess the last step is probably a test? This one should also be pretty easy to do with our existing "install inside GHA runner" case where we just verify that the LBI isn't pulled right?

[cgwalters]

BTW will trade my review for a review here

[omertuc]

I guess the last step is probably a test? This one should also be pretty easy to do with our existing "install inside GHA runner" case where we just verify that the LBI isn't pulled right?

Yep, once you've reviewed the new interface I'll add a test

[cgwalters]

Interface LGTM!

[cgwalters]

Needs a cargo fmt

[cgwalters]

This one just needs a test right?

[omertuc]

Yep

[omertuc]

I guess the last step is probably a test? This one should also be pretty easy to do with our existing "install inside GHA runner" case where we just verify that the LBI isn't pulled right?

Coming again to this, I'm not so sure I understand what you meant. The GHA integration tests make use of a Containerfile (hack/Containerfile) that currently doesn't have LBIs.

I thought instead we could test it by simply dogfooding in the existing logically bound install tmt plan, i.e.:

diff --git a/xtask/src/xtask.rs b/xtask/src/xtask.rs
index 7257511..f069935 100644
--- a/xtask/src/xtask.rs
+++ b/xtask/src/xtask.rs
@@ -11,11 +11,6 @@ use fn_error_context::context;
 use xshell::{cmd, Shell};
 
 const NAME: &str = "bootc";
-const TEST_IMAGES: &[&str] = &[
-    "quay.io/curl/curl-base:latest",
-    "quay.io/curl/curl:latest",
-    "registry.access.redhat.com/ubi9/podman:latest",
-];
 
 fn main() {
     if let Err(e) = try_main() {
@@ -202,9 +197,6 @@ fn test_tmt(sh: &Shell) -> Result<()> {
 
     cmd!(sh, "cargo run -p tests-integration run-vm prepare-tmt").run()?;
 
-    // pull some small images that are used for LBI installation tests
-    cmd!(sh, "podman pull {TEST_IMAGES...}").run()?;
-
     for (_prio, name) in tests {
         // cc https://pagure.io/testcloud/pull-request/174
         cmd!(sh, "rm -vf /var/tmp/tmt/testcloud/images/disk.qcow2").run()?;

Then simply* build the LBI tmt tests qcow2 image with the new --bound-images=pull flag

*While writing this comment, I started digging into what it would take:

• The plan uses how: bootc
• how: bootc delegates to a tmt plugin
• The tmt plugin uses bib
• bib uses osbuild/images
• osbuild/images uses osbuild/osbuild
• Finally osbuild/osbuild calls bootc

Quite a rabbit hole... I knew nothing about all this osbuild stuff or how bib only recently started using bootc and used to rely on other ways to unpack the image

Anyway so to get osbuild to --bound-images=pull we'd have to add this as a configuration option throughout the entire call stack mentioned above? Or maybe osbuild should always default to doing that?

Or I could test this some way that's not dogfooding

[cgwalters]

Quite a rabbit hole... I knew nothing about all this osbuild stuff or how bib only recently started using bootc and used to rely on other ways to unpack the image

Yeah...

One angle in this for me is that there's too many installers and I'm trying hard to centralize things in a very opinionated bootc install to-filesystem that handles lots of things from kernel arguments to the bootloader.

Basically 95% of "files" on the system for the base OS should be owned by bootc because it's also the thing responsible for updating them.

Anyway so to get osbuild to --bound-images=pull we'd have to add this as a configuration option throughout the entire call stack mentioned above? Or maybe osbuild should always default to doing that?

This is a complex topic. I don't think we need to plumb through --pull to bib yet although it would be convenient sometimes.

What we don't have right now is install tests that run through TMT. We probably should - we could do this via an "alongside" install for a package-based cloud image or so. Though we still have this overall issue in getting the container image into the VM in scenarios like that...which is the same thing needed for virt-manager/virt-manager#739 (basically allow the VM to fetch the container via virtiofs or so from the host).

Currently though we do have a suite of install tests that basically mutate a GHA runner, that's what this guy is:

bootc/.github/workflows/ci.yml

Line 87 in c4f3e33

sudo bootc-integration-tests install-alongside localhost/bootc

The advantage of this test is in CI it's cheap and reliable, but the downside is we can't actually reboot after install, and we don't have a single handy script to spawn a temporary VM to run them.

So...what we could do in that suite is add a container build that does use LBIs in that path (sharing with the existing TMT test).

[ckyrouac]

What we don't have right now is install tests that run through TMT.

The bootc plugin should enable these. We already have one tmt based install test here. Seems like the issue here is with propagating install flags through the plugin. I think this would be pretty straightforward if the tmt plugin switches to calling bootc to do the install instead of image builder. Then the tmt plugin could add a field to define arbitrary bootc install flags. The only reason I chose to use image builder in the tmt plugin is because that's what the customer docs tend to tell users to do.

[cgwalters]

Seems like the issue here is with propagating install flags through the plugin.

Those tests are good but I think we are talking about something different here. Remember that in the primary initial target for the --pull code here we'll be using bootc from outside the container image, where we won't have the base image or the LBIs.

That testing flow could actually look closer to what we're doing in the GHA flow I linked above, just pretending that the image we built there is actually fetched from a registry i.e. we do bootc install --source-imageref containers-storage:.... (Note to make this work we'd actually need to build a bootc binary for ubuntu which is going to have its own trickiness unfortunately...it's tempting to add a mode where we statically link ostree)

But in the short term I think what we could do with the TMT tests is spawn a non-bootc VM and test via install to-existing-root to test pulling LBIs that way.

Or of course, we could try standing up a CI path that injects bootc-under-test into an Anaconda ISO because that's our target for this for real.

[ckyrouac]

Remember that in the primary initial target for the --pull code here we'll be using bootc from outside the container image

Writing down what was discussed in real time. I think there will still be value in testing this in the containerized install path via the existing tmt framework. For the bootc install outside of container path, we could add another option to the bootc tmt plugin to enable this. Something like install_method: <container|host>. The tmt bootc plugin would need to require bootc be installed on the host.

[cgwalters]

I rebased this on main, OK if I push it?

From 7b50408786ecf129383c258b98de9a2b3969b62c Mon Sep 17 00:00:00 2001
From: Omer Tuchfeld <omer@tuchfeld.dev>
Date: Thu, 31 Oct 2024 13:49:25 +0100
Subject: [PATCH] install: Add support for pulling LBIs during install

Partially solves #846

This adds a new `--bound-images` option to `bootc install` which will
allow the user to choose how they want to handle the retrieval of LBIs
into the target's container storage.

The existing behavior, which will stay the default, is `--bound-images
stored` which will resolve the LBIs and verify they exist in the
source's container storage before copying them into the target's
container storage.

The new behavior is `--bound-images pull` which will skip the resolution
step and directly pull the LBIs into the target's container storage.

The older `--skip-bound-images` option (previously hidden) is now
removed and replaced with the new (but still hidden) `--bound-images
skip` option.

Signed-off-by: Omer Tuchfeld <omer@tuchfeld.dev>
Signed-off-by: Colin Walters <walters@verbum.org>
---
 lib/src/boundimage.rs |   6 ++-
 lib/src/cli.rs        |   5 ++
 lib/src/install.rs    | 116 +++++++++++++++++++++++++++++++-----------
 3 files changed, 95 insertions(+), 32 deletions(-)

diff --git a/lib/src/boundimage.rs b/lib/src/boundimage.rs
index f5b552c3..bc78d538 100644
--- a/lib/src/boundimage.rs
+++ b/lib/src/boundimage.rs
@@ -108,6 +108,7 @@ pub(crate) fn query_bound_images(root: &Dir) -> Result<Vec<BoundImage>> {
 
 #[cfg(feature = "install")]
 impl ResolvedBoundImage {
+    #[context("resolving bound image {}", src.image)]
     pub(crate) async fn from_image(src: &BoundImage) -> Result<Self> {
         let proxy = containers_image_proxy::ImageProxy::new().await?;
         let img = proxy
@@ -148,7 +149,10 @@ fn parse_container_file(file_contents: &tini::Ini) -> Result<BoundImage> {
 }
 
 #[context("Pulling bound images")]
-pub(crate) async fn pull_images(sysroot: &Storage, bound_images: Vec<BoundImage>) -> Result<()> {
+pub(crate) async fn pull_images(
+    sysroot: &Storage,
+    bound_images: Vec<crate::boundimage::BoundImage>,
+) -> Result<()> {
     tracing::debug!("Pulling bound images: {}", bound_images.len());
     // Yes, the usage of NonZeroUsize here is...maybe odd looking, but I find
     // it an elegant way to divide (empty vector, non empty vector) since
diff --git a/lib/src/cli.rs b/lib/src/cli.rs
index 1a9d47de..9f19a6a6 100644
--- a/lib/src/cli.rs
+++ b/lib/src/cli.rs
@@ -1039,6 +1039,11 @@ fn test_parse_install_args() {
     };
     assert!(o.target_opts.target_no_signature_verification);
     assert_eq!(o.filesystem_opts.root_path.as_str(), "/target");
+    // Ensure we default to old bound images behavior
+    assert_eq!(
+        o.config_opts.bound_images,
+        crate::install::BoundImagesOpt::Stored
+    );
 }
 
 #[test]
diff --git a/lib/src/install.rs b/lib/src/install.rs
index 1893c0a7..b3911562 100644
--- a/lib/src/install.rs
+++ b/lib/src/install.rs
@@ -21,7 +21,6 @@ use std::sync::Arc;
 use std::time::Duration;
 
 use anyhow::{anyhow, Context, Result};
-use anyhow::{ensure, Ok};
 use bootc_utils::CommandRunExt;
 use camino::Utf8Path;
 use camino::Utf8PathBuf;
@@ -44,6 +43,7 @@ use rustix::fs::{FileTypeExt, MetadataExt as _};
 use serde::{Deserialize, Serialize};
 
 use self::baseline::InstallBlockDeviceOpts;
+use crate::boundimage::{BoundImage, ResolvedBoundImage};
 use crate::containerenv::ContainerExecutionInfo;
 use crate::lsm;
 use crate::mount::Filesystem;
@@ -131,6 +131,27 @@ pub(crate) struct InstallSourceOpts {
     pub(crate) source_imgref: Option<String>,
 }
 
+#[derive(ValueEnum, Debug, Copy, Clone, PartialEq, Eq, Serialize, Deserialize, Default)]
+#[serde(rename_all = "kebab-case")]
+pub(crate) enum BoundImagesOpt {
+    /// Bound images must exist in the source's root container storage (default)
+    #[default]
+    Stored,
+    #[clap(hide = true)]
+    /// Do not resolve any "logically bound" images at install time.
+    Skip,
+    // TODO: Once we implement https://github.com/containers/bootc/issues/863 update this comment
+    // to mention source's root container storage being used as lookaside cache
+    /// Bound images will be pulled and stored directly in the target's bootc container storage
+    Pull,
+}
+
+impl std::fmt::Display for BoundImagesOpt {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        self.to_possible_value().unwrap().get_name().fmt(f)
+    }
+}
+
 #[derive(clap::Args, Debug, Clone, Serialize, Deserialize, PartialEq, Eq)]
 pub(crate) struct InstallConfigOpts {
     /// Disable SELinux in the target (installed) system.
@@ -166,10 +187,11 @@ pub(crate) struct InstallConfigOpts {
     #[serde(default)]
     pub(crate) generic_image: bool,
 
-    /// Do not pull any "logically bound" images at install time.
-    #[clap(long, hide = true)]
+    /// How should logically bound images be retrieved.
+    #[clap(long)]
     #[serde(default)]
-    pub(crate) skip_bound_images: bool,
+    #[arg(default_value_t)]
+    pub(crate) bound_images: BoundImagesOpt,
 
     /// The stateroot name to use. Defaults to `default`.
     #[clap(long)]
@@ -630,7 +652,7 @@ async fn initialize_ostree_root(state: &State, root_setup: &RootSetup) -> Result
     sysroot.load(cancellable)?;
 
     let stateroot_exists = rootfs_dir.try_exists(format!("ostree/deploy/{stateroot}"))?;
-    ensure!(
+    anyhow::ensure!(
         !stateroot_exists,
         "Cannot redeploy over extant stateroot {stateroot}"
     );
@@ -1318,7 +1340,7 @@ async fn install_with_sysroot(
     rootfs: &RootSetup,
     sysroot: &Storage,
     boot_uuid: &str,
-    bound_images: &[crate::boundimage::ResolvedBoundImage],
+    bound_images: BoundImages,
     has_ostree: bool,
 ) -> Result<()> {
     // And actually set up the container in that root, returning a deployment and
@@ -1346,18 +1368,66 @@ async fn install_with_sysroot(
     tracing::debug!("Installed bootloader");
 
     tracing::debug!("Perfoming post-deployment operations");
-    // Note that we *always* initialize this container storage, even
-    // if there are no bound images today.
+
+    // Note that we *always* initialize this container storage, even if there are no bound images
+    // today.
     let imgstore = sysroot.get_ensure_imgstore()?;
-    // Now copy each bound image from the host's container storage into the target.
-    for image in bound_images {
-        let image = image.image.as_str();
-        imgstore.pull_from_host_storage(image).await?;
+
+    match bound_images {
+        BoundImages::Skip => {}
+        BoundImages::Resolved(resolved_bound_images) => {
+            // Now copy each bound image from the host's container storage into the target.
+            for image in resolved_bound_images {
+                let image = image.image.as_str();
+                imgstore.pull_from_host_storage(image).await?;
+            }
+        }
+        BoundImages::Unresolved(bound_images) => {
+            crate::boundimage::pull_images(sysroot, bound_images)
+                .await
+                .context("pulling bound images")?;
+        }
     }
 
     Ok(())
 }
 
+enum BoundImages {
+    Skip,
+    Resolved(Vec<ResolvedBoundImage>),
+    Unresolved(Vec<BoundImage>),
+}
+
+impl BoundImages {
+    async fn from_state(state: &State) -> Result<Self> {
+        let bound_images = match state.config_opts.bound_images {
+            BoundImagesOpt::Skip => BoundImages::Skip,
+            others => {
+                let queried_images = crate::boundimage::query_bound_images(&state.container_root)?;
+                match others {
+                    BoundImagesOpt::Stored => {
+                        // Verify each bound image is present in the container storage
+                        let mut r = Vec::with_capacity(queried_images.len());
+                        for image in queried_images {
+                            let resolved = ResolvedBoundImage::from_image(&image).await?;
+                            tracing::debug!("Resolved {}: {}", resolved.image, resolved.digest);
+                            r.push(resolved)
+                        }
+                        BoundImages::Resolved(r)
+                    }
+                    BoundImagesOpt::Pull => {
+                        // No need to resolve the images, we will pull them into the target later
+                        BoundImages::Unresolved(queried_images)
+                    }
+                    BoundImagesOpt::Skip => anyhow::bail!("unreachable error"),
+                }
+            }
+        };
+
+        Ok(bound_images)
+    }
+}
+
 async fn install_to_filesystem_impl(state: &State, rootfs: &mut RootSetup) -> Result<()> {
     if matches!(state.selinux_state, SELinuxFinalState::ForceTargetDisabled) {
         rootfs.kargs.push("selinux=0".to_string());
@@ -1385,28 +1455,12 @@ async fn install_to_filesystem_impl(state: &State, rootfs: &mut RootSetup) -> Re
     tracing::debug!("boot uuid={boot_uuid}");
 
     // If we're doing an alongside install, then the /dev bootupd sees needs to be the host's.
-    ensure!(
+    anyhow::ensure!(
         crate::mount::is_same_as_host(Utf8Path::new("/dev"))?,
         "Missing /dev mount to host /dev"
     );
 
-    let bound_images = if state.config_opts.skip_bound_images {
-        Vec::new()
-    } else {
-        crate::boundimage::query_bound_images(&state.container_root)?
-    };
-    tracing::debug!("bound images={bound_images:?}");
-
-    // Verify each bound image is present in the container storage
-    let bound_images = {
-        let mut r = Vec::with_capacity(bound_images.len());
-        for image in bound_images {
-            let resolved = crate::boundimage::ResolvedBoundImage::from_image(&image).await?;
-            tracing::debug!("Resolved {}: {}", resolved.image, resolved.digest);
-            r.push(resolved)
-        }
-        r
-    };
+    let bound_images = BoundImages::from_state(state).await?;
 
     // Initialize the ostree sysroot (repo, stateroot, etc.)
     {
@@ -1416,7 +1470,7 @@ async fn install_to_filesystem_impl(state: &State, rootfs: &mut RootSetup) -> Re
             rootfs,
             &sysroot,
             &boot_uuid,
-            &bound_images,
+            bound_images,
             has_ostree,
         )
         .await?;
-- 
2.47.0