From bffb3bb564b8caad9710ad223c31666861285749 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Thu, 1 Aug 2024 09:24:03 -0400 Subject: [PATCH] Open/create imagestore as needed The previous change in https://github.com/containers/bootc/pull/724 broke for two important scenarios: - Installing with Anaconda - Upgrades from previous states Closes: https://github.com/containers/bootc/issues/747 Signed-off-by: Colin Walters --- Cargo.lock | 7 +++++++ lib/Cargo.toml | 1 + lib/src/boundimage.rs | 9 +++++++-- lib/src/cli.rs | 16 ++++++++++------ lib/src/deploy.rs | 5 ++++- lib/src/image.rs | 2 +- lib/src/imgstorage.rs | 14 ++++++++++++++ lib/src/install.rs | 5 ++++- lib/src/status.rs | 2 +- lib/src/store/mod.rs | 22 ++++++++++++++++------ 10 files changed, 65 insertions(+), 18 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index f93fd9373..860e60745 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -193,6 +193,7 @@ dependencies = [ "serde_json", "serde_yaml", "similar-asserts", + "static_assertions", "tempfile", "tini", "tokio", @@ -1846,6 +1847,12 @@ dependencies = [ "windows-sys 0.48.0", ] +[[package]] +name = "static_assertions" +version = "1.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a2eb9349b6444b326872e140eb1cf5e7c522154d69e7a0ffb0fb81c06b37543f" + [[package]] name = "strsim" version = "0.10.0" diff --git a/lib/Cargo.toml b/lib/Cargo.toml index c2639b64a..c888a853b 100644 --- a/lib/Cargo.toml +++ b/lib/Cargo.toml @@ -50,6 +50,7 @@ tini = "1.3.0" [dev-dependencies] indoc = "2.0.5" similar-asserts = { version = "1.5.0" } +static_assertions = "1.1.0" [features] default = ["install"] diff --git a/lib/src/boundimage.rs b/lib/src/boundimage.rs index 71df834b8..166b733a4 100644 --- a/lib/src/boundimage.rs +++ b/lib/src/boundimage.rs @@ -145,13 +145,18 @@ fn parse_container_file(file_contents: &tini::Ini) -> Result { #[context("Pulling bound images")] pub(crate) async fn pull_images(sysroot: &Storage, bound_images: Vec) -> Result<()> { tracing::debug!("Pulling bound images: {}", bound_images.len()); + // Only initialize the image storage if we have images to pull + let imgstore = if !bound_images.is_empty() { + sysroot.get_ensure_imgstore()? + } else { + return Ok(()); + }; //TODO: do this in parallel for bound_image in bound_images { let image = &bound_image.image; let desc = format!("Updating bound image: {image}"); crate::utils::async_task_with_spinner(&desc, async move { - sysroot - .imgstore + imgstore .pull(&bound_image.image, PullMode::IfNotExists) .await }) diff --git a/lib/src/cli.rs b/lib/src/cli.rs index 4732c9051..6d0c2fe1a 100644 --- a/lib/src/cli.rs +++ b/lib/src/cli.rs @@ -839,22 +839,26 @@ async fn run_from_opt(opt: Opt) -> Result<()> { } ImageOpts::PullFromDefaultStorage { image } => { let sysroot = get_storage().await?; - sysroot.imgstore.pull_from_host_storage(&image).await + sysroot + .get_ensure_imgstore()? + .pull_from_host_storage(&image) + .await } ImageOpts::Cmd(opt) => { - let sysroot = get_storage().await?; + let storage = get_storage().await?; + let imgstore = storage.get_ensure_imgstore()?; match opt { ImageCmdOpts::List { args } => { - crate::image::imgcmd_entrypoint(&sysroot.imgstore, "list", &args).await + crate::image::imgcmd_entrypoint(imgstore, "list", &args).await } ImageCmdOpts::Build { args } => { - crate::image::imgcmd_entrypoint(&sysroot.imgstore, "build", &args).await + crate::image::imgcmd_entrypoint(imgstore, "build", &args).await } ImageCmdOpts::Pull { args } => { - crate::image::imgcmd_entrypoint(&sysroot.imgstore, "pull", &args).await + crate::image::imgcmd_entrypoint(imgstore, "pull", &args).await } ImageCmdOpts::Push { args } => { - crate::image::imgcmd_entrypoint(&sysroot.imgstore, "push", &args).await + crate::image::imgcmd_entrypoint(imgstore, "push", &args).await } } } diff --git a/lib/src/deploy.rs b/lib/src/deploy.rs index 836f3de1e..8404eba98 100644 --- a/lib/src/deploy.rs +++ b/lib/src/deploy.rs @@ -280,7 +280,10 @@ pub(crate) async fn prune_container_store(sysroot: &Storage) -> Result<()> { } // Convert to a hashset of just the image names let image_names = HashSet::from_iter(all_bound_images.iter().map(|img| img.image.as_str())); - let pruned = sysroot.imgstore.prune_except_roots(&image_names).await?; + let pruned = sysroot + .get_ensure_imgstore()? + .prune_except_roots(&image_names) + .await?; tracing::debug!("Pruned images: {}", pruned.len()); Ok(()) } diff --git a/lib/src/image.rs b/lib/src/image.rs index 5fd84b4f3..7e5908d2e 100644 --- a/lib/src/image.rs +++ b/lib/src/image.rs @@ -25,7 +25,7 @@ pub(crate) async fn list_entrypoint() -> Result<()> { println!(); println!("# Logically bound images"); - let mut listcmd = sysroot.imgstore.new_image_cmd()?; + let mut listcmd = sysroot.get_ensure_imgstore()?.new_image_cmd()?; listcmd.arg("list"); listcmd.run()?; diff --git a/lib/src/imgstorage.rs b/lib/src/imgstorage.rs index 705a8a996..1f7899c9c 100644 --- a/lib/src/imgstorage.rs +++ b/lib/src/imgstorage.rs @@ -49,6 +49,11 @@ pub(crate) struct Storage { #[allow(dead_code)] /// Our runtime state run: Dir, + /// Disallow using this across multiple threads concurrently; while we + /// have internal locking in podman, in the future we may change how + /// things work here. And we don't have a use case right now for + /// concurrent operations. + _unsync: std::cell::Cell<()>, } #[derive(Debug, PartialEq, Eq)] @@ -160,12 +165,14 @@ impl Storage { sysroot .rename(&tmp, sysroot, subpath) .context("Renaming tmpdir")?; + tracing::debug!("Created image store"); } Self::open(sysroot, run) } #[context("Opening imgstorage")] pub(crate) fn open(sysroot: &Dir, run: &Dir) -> Result { + tracing::trace!("Opening container image store"); Self::init_globals()?; let storage_root = sysroot .open_dir(SUBPATH) @@ -178,6 +185,7 @@ impl Storage { sysroot: sysroot.try_clone()?, storage_root, run, + _unsync: Default::default(), }) } @@ -292,3 +300,9 @@ impl Storage { Ok(()) } } + +#[cfg(test)] +mod tests { + use super::*; + static_assertions::assert_not_impl_any!(Storage: Sync); +} diff --git a/lib/src/install.rs b/lib/src/install.rs index 95eb6d8ec..456363c69 100644 --- a/lib/src/install.rs +++ b/lib/src/install.rs @@ -1289,10 +1289,13 @@ 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. + 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(); - sysroot.imgstore.pull_from_host_storage(image).await?; + imgstore.pull_from_host_storage(image).await?; } Ok(()) diff --git a/lib/src/status.rs b/lib/src/status.rs index 34377e8e4..d9f5d2d62 100644 --- a/lib/src/status.rs +++ b/lib/src/status.rs @@ -136,7 +136,7 @@ fn boot_entry_from_deployment( let store = deployment.store()?; let store = store.as_ref().unwrap_or(&sysroot.store); let spec = Some(store.spec()); - let status = store.imagestatus(sysroot, deployment, image)?; + let status = store.imagestatus(&*sysroot, deployment, image)?; (spec, status) } else { diff --git a/lib/src/store/mod.rs b/lib/src/store/mod.rs index b501347a1..56e142e8b 100644 --- a/lib/src/store/mod.rs +++ b/lib/src/store/mod.rs @@ -1,3 +1,4 @@ +use std::cell::OnceCell; use std::env; use std::ops::Deref; @@ -16,8 +17,8 @@ mod ostree_container; pub(crate) struct Storage { pub sysroot: SysrootLock, - #[allow(dead_code)] - pub imgstore: crate::imgstorage::Storage, + run: Dir, + imgstore: OnceCell, pub store: Box, } @@ -52,6 +53,7 @@ impl Deref for Storage { impl Storage { pub fn new(sysroot: SysrootLock, run: &Dir) -> Result { + let run = run.try_clone()?; let store = match env::var("BOOTC_STORAGE") { Ok(val) => crate::spec::Store::from_str(&val, true).unwrap_or_else(|_| { let default = crate::spec::Store::default(); @@ -61,17 +63,25 @@ impl Storage { Err(_) => crate::spec::Store::default(), }; - let sysroot_dir = Dir::reopen_dir(&crate::utils::sysroot_fd(&sysroot))?; - let imgstore = crate::imgstorage::Storage::open(&sysroot_dir, run)?; - let store = load(store); Ok(Self { sysroot, + run, store, - imgstore, + imgstore: Default::default(), }) } + + /// Access the image storage; will automatically initialize it if necessary. + pub(crate) fn get_ensure_imgstore(&self) -> Result<&crate::imgstorage::Storage> { + if let Some(imgstore) = self.imgstore.get() { + return Ok(imgstore); + } + let sysroot_dir = Dir::reopen_dir(&crate::utils::sysroot_fd(&self.sysroot))?; + let imgstore = crate::imgstorage::Storage::create(&sysroot_dir, &self.run)?; + Ok(self.imgstore.get_or_init(|| imgstore)) + } } impl ContainerImageStore for ostree::Deployment {