Open/create imagestore as needed

The previous change in #724 broke for two important scenarios: - Installing with Anaconda - Upgrades from previous states Closes: #747 Signed-off-by: Colin Walters <[email protected]>
containers · Aug 1, 2024 · bffb3bb · bffb3bb
1 parent ee11428
commit bffb3bb
Show file tree

Hide file tree

Showing 10 changed files with 65 additions and 18 deletions.
diff --git a/Cargo.lock b/Cargo.lock
diff --git 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
@@ -145,13 +145,18 @@ 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<()> {
     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
@@ -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
@@ -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
@@ -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
@@ -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<Self> {
+        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
@@ -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
@@ -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
@@ -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<crate::imgstorage::Storage>,
     pub store: Box<dyn ContainerImageStoreImpl>,
 }
 
@@ -52,6 +53,7 @@ impl Deref for Storage {
 
 impl Storage {
     pub fn new(sysroot: SysrootLock, run: &Dir) -> Result<Self> {
+        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 {