-
Notifications
You must be signed in to change notification settings - Fork 150
store: Add support for composefs-rs #1471
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,33 +1,58 @@ | ||
| use std::cell::OnceCell; | ||
| use std::env; | ||
| use std::ops::Deref; | ||
| use std::sync::Arc; | ||
|
|
||
| use anyhow::{Context, Result}; | ||
| use cap_std_ext::cap_std; | ||
| use cap_std_ext::cap_std::fs::Dir; | ||
| 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 ostree_ext::ostree; | ||
| 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<composefs::fsverity::Sha512HashValue>; | ||
|
|
||
| /// Path to the physical root | ||
| pub const SYSROOT: &str = "sysroot"; | ||
|
|
||
| /// The toplevel composefs directory path | ||
| pub const COMPOSEFS: &str = "composefs"; | ||
| pub const COMPOSEFS_MODE: Mode = Mode::from_raw_mode(0o700); | ||
|
|
||
| /// The path to the bootc root directory, relative to the physical | ||
| /// system root | ||
| pub(crate) const BOOTC_ROOT: &str = "ostree/bootc"; | ||
|
|
||
| pub(crate) struct Storage { | ||
| /// Directory holding the physical root | ||
| pub physical_root: Dir, | ||
|
|
||
| /// The OSTree storage | ||
| pub sysroot: SysrootLock, | ||
| run: Dir, | ||
| /// The composefs storage | ||
| pub composefs: OnceCell<Arc<ComposefsRepository>>, | ||
| /// The containers-image storage used foR LBIs | ||
| imgstore: OnceCell<crate::imgstorage::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<dyn ContainerImageStoreImpl>, | ||
| } | ||
|
|
||
|
|
@@ -71,12 +96,29 @@ impl Storage { | |
| }), | ||
| 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 | ||
| // make it so that code doesn't need to distinguish between booted | ||
| // vs offline target. The ostree code all just looks at the ostree/ | ||
| // directory, and will follow the link in the booted case. | ||
| // | ||
| // For composefs we aren't going to do a similar thing, so here | ||
| // we need to explicitly distinguish the two and the storage | ||
| // here hence holds a reference to the physical root. | ||
| let ostree_sysroot_dir = crate::utils::sysroot_dir(&sysroot)?; | ||
| let physical_root = if sysroot.is_booted() { | ||
| ostree_sysroot_dir.open_dir(SYSROOT)? | ||
| } else { | ||
| ostree_sysroot_dir | ||
| }; | ||
|
|
||
| Ok(Self { | ||
| physical_root, | ||
| sysroot, | ||
| run, | ||
| composefs: Default::default(), | ||
| store, | ||
| imgstore: Default::default(), | ||
| }) | ||
|
|
@@ -111,6 +153,31 @@ impl Storage { | |
| Ok(self.imgstore.get_or_init(|| imgstore)) | ||
| } | ||
|
|
||
| pub(crate) fn get_ensure_composefs(&self) -> Result<Arc<ComposefsRepository>> { | ||
| if let Some(composefs) = self.composefs.get() { | ||
| return Ok(Arc::clone(composefs)); | ||
| } | ||
|
|
||
| let mut db = DirBuilder::new(); | ||
| db.mode(COMPOSEFS_MODE.as_raw_mode()); | ||
| self.physical_root.ensure_dir_with(COMPOSEFS, &db)?; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we not care about the returned bool for anything here? We just rely on the fact that if there's an error, we're unwraping it and we will return the error. The returned bool isn't really useful for us in this context I guess?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, it just says whether or not the directory existed already which is information we don't need.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, that makes sense. It would never make it to the next line if there was an error returned from ensure_dir_with(). There's no logical need to store and reference the returned value. |
||
|
|
||
| let mut composefs = | ||
| ComposefsRepository::open_path(&self.physical_root.open_dir(COMPOSEFS)?, ".")?; | ||
|
|
||
| // Bootstrap verity off of the ostree state. In practice this means disabled by | ||
| // default right now. | ||
| let ostree_repo = &self.sysroot.repo(); | ||
| let ostree_verity = ostree_ext::fsverity::is_verity_enabled(ostree_repo)?; | ||
| if !ostree_verity.enabled { | ||
| tracing::debug!("Setting insecure mode for composefs repo"); | ||
| composefs.set_insecure(true); | ||
| } | ||
| let composefs = Arc::new(composefs); | ||
| let r = Arc::clone(self.composefs.get_or_init(|| composefs)); | ||
| Ok(r) | ||
|
Comment on lines
+177
to
+178
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I noted your response to Gemini below, you mentioned you only expect this to be called once and don't necessarily require thread safety here. I'm just curious about your use of Arc over rc in that case? Since Arc is slightly more computationally expensive, I'm curious to understand your thought process. There's every possibility that my lack of Rust experience is causing me to overlook something. In which case, consider this a question for my own educational purposes rather than a functional critique of your PR per se. :)
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The underlying API wants Arc https://github.com/containers/composefs-rs/blob/126751dee8a71aa54c9666e69645d2fd1eccb176/crates/composefs/src/repository.rs#L112 Ultimately we definitely don't care about the refcounting overhead here, and in general Arc is needed for things like tokio with work stealing.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes that's fine! Thanks for looking at the PR!
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ahh, that makes sense then. Thanks for the clarification. |
||
| } | ||
cgwalters marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| /// Update the mtime on the storage root directory | ||
| #[context("Updating storage root mtime")] | ||
| pub(crate) fn update_mtime(&self) -> Result<()> { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| use std assert | ||
| use tap.nu | ||
|
|
||
| tap begin "composefs integration smoke test" | ||
|
|
||
| bootc internals test-composefs | ||
|
|
||
| tap ok |
Uh oh!
There was an error while loading. Please reload this page.