From 82057f869c53ebd3b53cd0aeb002e5e6e5caadbf Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Fri, 15 Aug 2025 09:28:40 +0200 Subject: [PATCH] store: Remove dynamic abstraction A while ago we added a trait in preparation for multiple backends. The current composefs branch ignores it and has a bunch of `if {} else {}`. Looking at this, what I think will work better in the end is actually a more refined version of the `if {} else {}` model instead of trying to really flesh out this trait. It's hard to say of course until we get there, but the trait approach forces a high level of abstraction vs just trying to factor out common code between two backends. Signed-off-by: Colin Walters --- crates/lib/src/status.rs | 70 ++++++++++++++++++----- crates/lib/src/store/mod.rs | 59 +------------------ crates/lib/src/store/ostree_container.rs | 72 ------------------------ 3 files changed, 58 insertions(+), 143 deletions(-) delete mode 100644 crates/lib/src/store/ostree_container.rs diff --git a/crates/lib/src/status.rs b/crates/lib/src/status.rs index 910e3f567..d8cd0669c 100644 --- a/crates/lib/src/status.rs +++ b/crates/lib/src/status.rs @@ -13,12 +13,16 @@ use ostree_ext::container as ostree_container; use ostree_ext::container_utils::ostree_booted; use ostree_ext::keyfileext::KeyFileExt; use ostree_ext::oci_spec; +use ostree_ext::oci_spec::image::Digest; +use ostree_ext::oci_spec::image::ImageConfiguration; use ostree_ext::ostree; +use ostree_ext::sysroot::SysrootLock; use crate::cli::OutputFormat; +use crate::spec::ImageStatus; use crate::spec::{BootEntry, BootOrder, Host, HostSpec, HostStatus, HostType}; use crate::spec::{ImageReference, ImageSignature}; -use crate::store::{CachedImageStatus, ContainerImageStore, Storage}; +use crate::store::{CachedImageStatus, Storage}; impl From for ImageSignature { fn from(sig: ostree_container::SignatureSource) -> Self { @@ -109,6 +113,51 @@ pub(crate) fn labels_of_config( config.config().as_ref().and_then(|c| c.labels().as_ref()) } +/// Convert between a subset of ostree-ext metadata and the exposed spec API. +fn create_imagestatus( + image: ImageReference, + manifest_digest: &Digest, + config: &ImageConfiguration, +) -> ImageStatus { + let labels = labels_of_config(config); + let timestamp = labels + .and_then(|l| { + l.get(oci_spec::image::ANNOTATION_CREATED) + .map(|s| s.as_str()) + }) + .or_else(|| config.created().as_deref()) + .and_then(bootc_utils::try_deserialize_timestamp); + + let version = ostree_container::version_for_config(config).map(ToOwned::to_owned); + let architecture = config.architecture().to_string(); + ImageStatus { + image, + version, + timestamp, + image_digest: manifest_digest.to_string(), + architecture, + } +} + +fn imagestatus( + sysroot: &SysrootLock, + deployment: &ostree::Deployment, + image: ostree_container::OstreeImageReference, +) -> Result { + let repo = &sysroot.repo(); + let imgstate = ostree_container::store::query_image_commit(repo, &deployment.csum())?; + let image = ImageReference::from(image); + let cached = imgstate + .cached_update + .map(|cached| create_imagestatus(image.clone(), &cached.manifest_digest, &cached.config)); + let imagestatus = create_imagestatus(image, &imgstate.manifest_digest, &imgstate.configuration); + + Ok(CachedImageStatus { + image: Some(imagestatus), + cached_update: cached, + }) +} + /// Given an OSTree deployment, parse out metadata into our spec. #[context("Reading deployment metadata")] fn boot_entry_from_deployment( @@ -116,7 +165,6 @@ fn boot_entry_from_deployment( deployment: &ostree::Deployment, ) -> Result { let ( - store, CachedImageStatus { image, cached_update, @@ -124,26 +172,22 @@ fn boot_entry_from_deployment( incompatible, ) = if let Some(origin) = deployment.origin().as_ref() { let incompatible = crate::utils::origin_has_rpmostree_stuff(origin); - let (store, cached_imagestatus) = if incompatible { + let cached_imagestatus = if incompatible { // If there are local changes, we can't represent it as a bootc compatible image. - (None, CachedImageStatus::default()) + CachedImageStatus::default() } else if let Some(image) = get_image_origin(origin)? { - 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)?; - - (spec, status) + imagestatus(sysroot, deployment, image)? } else { // The deployment isn't using a container image - (None, CachedImageStatus::default()) + CachedImageStatus::default() }; - (store, cached_imagestatus, incompatible) + (cached_imagestatus, incompatible) } else { // The deployment has no origin at all (this generally shouldn't happen) - (None, CachedImageStatus::default(), false) + (CachedImageStatus::default(), false) }; + let store = Some(crate::spec::Store::OstreeContainer); let r = BootEntry { image, cached_update, diff --git a/crates/lib/src/store/mod.rs b/crates/lib/src/store/mod.rs index bd3e8c5aa..e38d4f1df 100644 --- a/crates/lib/src/store/mod.rs +++ b/crates/lib/src/store/mod.rs @@ -1,5 +1,4 @@ use std::cell::OnceCell; -use std::env; use std::ops::Deref; use std::sync::Arc; @@ -7,21 +6,16 @@ use anyhow::{Context, Result}; use cap_std_ext::cap_std; use cap_std_ext::cap_std::fs::{Dir, DirBuilder, DirBuilderExt as _}; use cap_std_ext::dirext::CapStdExtDirExt; -use clap::ValueEnum; use fn_error_context::context; -use ostree_ext::container::OstreeImageReference; -use ostree_ext::keyfileext::KeyFileExt; +use composefs; use ostree_ext::sysroot::SysrootLock; -use ostree_ext::{composefs, ostree}; use rustix::fs::Mode; use crate::lsm; use crate::spec::ImageStatus; use crate::utils::deployment_fd; -mod ostree_container; - /// See https://github.com/containers/composefs-rs/issues/159 pub type ComposefsRepository = composefs::repository::Repository; @@ -50,10 +44,6 @@ pub(crate) struct Storage { /// Our runtime state run: Dir, - - /// This is a stub abstraction that tries to hide ostree - /// that we aren't really using right now - pub store: Box, } #[derive(Default)] @@ -62,21 +52,6 @@ pub(crate) struct CachedImageStatus { pub cached_update: Option, } -pub(crate) trait ContainerImageStore { - fn store(&self) -> Result>>; -} - -pub(crate) trait ContainerImageStoreImpl { - fn spec(&self) -> crate::spec::Store; - - fn imagestatus( - &self, - sysroot: &SysrootLock, - deployment: &ostree::Deployment, - image: OstreeImageReference, - ) -> Result; -} - impl Deref for Storage { type Target = SysrootLock; @@ -88,15 +63,6 @@ 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(); - tracing::warn!("Unknown BOOTC_STORAGE option {val}, falling back to {default:?}"); - default - }), - Err(_) => crate::spec::Store::default(), - }; - let store = load(store); // ostree has historically always relied on // having ostree -> sysroot/ostree as a symlink in the image to @@ -119,7 +85,6 @@ impl Storage { sysroot, run, composefs: Default::default(), - store, imgstore: Default::default(), }) } @@ -190,25 +155,3 @@ impl Storage { .map_err(Into::into) } } - -impl ContainerImageStore for ostree::Deployment { - fn store<'a>(&self) -> Result>> { - if let Some(origin) = self.origin().as_ref() { - if let Some(store) = origin.optional_string("bootc", "backend")? { - let store = - crate::spec::Store::from_str(&store, true).map_err(anyhow::Error::msg)?; - Ok(Some(load(store))) - } else { - Ok(None) - } - } else { - Ok(None) - } - } -} - -pub(crate) fn load(ty: crate::spec::Store) -> Box { - match ty { - crate::spec::Store::OstreeContainer => Box::new(ostree_container::OstreeContainerStore), - } -} diff --git a/crates/lib/src/store/ostree_container.rs b/crates/lib/src/store/ostree_container.rs deleted file mode 100644 index b2f127fdb..000000000 --- a/crates/lib/src/store/ostree_container.rs +++ /dev/null @@ -1,72 +0,0 @@ -use anyhow::Result; - -use ostree_ext::container as ostree_container; -use ostree_ext::oci_spec; -use ostree_ext::oci_spec::image::{Digest, ImageConfiguration}; -use ostree_ext::ostree; -use ostree_ext::sysroot::SysrootLock; - -use super::CachedImageStatus; -use crate::spec::{ImageReference, ImageStatus}; - -pub(super) struct OstreeContainerStore; - -impl super::ContainerImageStoreImpl for OstreeContainerStore { - fn spec(&self) -> crate::spec::Store { - crate::spec::Store::OstreeContainer - } - - fn imagestatus( - &self, - sysroot: &SysrootLock, - deployment: &ostree::Deployment, - image: ostree_container::OstreeImageReference, - ) -> Result { - let repo = &sysroot.repo(); - let image = ImageReference::from(image); - let csum = deployment.csum(); - let imgstate = ostree_container::store::query_image_commit(repo, &csum)?; - let cached = imgstate.cached_update.map(|cached| { - create_imagestatus(image.clone(), &cached.manifest_digest, &cached.config) - }); - let imagestatus = - create_imagestatus(image, &imgstate.manifest_digest, &imgstate.configuration); - - Ok(CachedImageStatus { - image: Some(imagestatus), - cached_update: cached, - }) - } -} - -/// Convert between a subset of ostree-ext metadata and the exposed spec API. -fn create_imagestatus( - image: ImageReference, - manifest_digest: &Digest, - config: &ImageConfiguration, -) -> ImageStatus { - let labels = labels_of_config(config); - let timestamp = labels - .and_then(|l| { - l.get(oci_spec::image::ANNOTATION_CREATED) - .map(|s| s.as_str()) - }) - .or_else(|| config.created().as_deref()) - .and_then(bootc_utils::try_deserialize_timestamp); - - let version = ostree_container::version_for_config(config).map(ToOwned::to_owned); - let architecture = config.architecture().to_string(); - ImageStatus { - image, - version, - timestamp, - image_digest: manifest_digest.to_string(), - architecture, - } -} - -fn labels_of_config( - config: &oci_spec::image::ImageConfiguration, -) -> Option<&std::collections::HashMap> { - config.config().as_ref().and_then(|c| c.labels().as_ref()) -}