From 79ed0bea61b666dbe512011d03b6b5b7ada56cca Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Thu, 18 Jul 2024 16:32:41 -0400 Subject: [PATCH 1/5] boundimage: More struct definition to top I think it's generally best to have type definitions come before the code that references them. Signed-off-by: Colin Walters --- lib/src/boundimage.rs | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/lib/src/boundimage.rs b/lib/src/boundimage.rs index 24119b95..89ba491b 100644 --- a/lib/src/boundimage.rs +++ b/lib/src/boundimage.rs @@ -18,6 +18,17 @@ use ostree_ext::sysroot::SysrootLock; /// symbolic links to `.container` or `.image` files. const BOUND_IMAGE_DIR: &str = "usr/lib/bootc-experimental/bound-images.d"; +/// A subset of data parsed from a `.image` or `.container` file with +/// the minimal information necessary to fetch the image. +/// +/// In the future this may be extended to include e.g. certificates or +/// other pull options. +#[derive(PartialEq, Eq)] +struct BoundImage { + image: String, + auth_file: Option, +} + /// Given a deployment, pull all container images it references. pub(crate) fn pull_bound_images(sysroot: &SysrootLock, deployment: &Deployment) -> Result<()> { let deployment_root = &crate::utils::deployment_fd(sysroot, deployment)?; @@ -117,17 +128,6 @@ fn pull_images(_deployment_root: &Dir, bound_images: Vec) -> Result< Ok(()) } -/// A subset of data parsed from a `.image` or `.container` file with -/// the minimal information necessary to fetch the image. -/// -/// In the future this may be extended to include e.g. certificates or -/// other pull options. -#[derive(PartialEq, Eq)] -struct BoundImage { - image: String, - auth_file: Option, -} - impl BoundImage { fn new(image: String, auth_file: Option) -> Result { let image = parse_spec_value(&image).context("Invalid image value")?; From fcc20051494a7ee2a16ad581d274bbe968862a07 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Thu, 18 Jul 2024 16:35:38 -0400 Subject: [PATCH 2/5] boundimage: Drop const parameter We were always passing this single constant value to the function; let's just reference it inside the function and simplify callers. Signed-off-by: Colin Walters --- lib/src/boundimage.rs | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/lib/src/boundimage.rs b/lib/src/boundimage.rs index 89ba491b..7718679b 100644 --- a/lib/src/boundimage.rs +++ b/lib/src/boundimage.rs @@ -32,12 +32,13 @@ struct BoundImage { /// Given a deployment, pull all container images it references. pub(crate) fn pull_bound_images(sysroot: &SysrootLock, deployment: &Deployment) -> Result<()> { let deployment_root = &crate::utils::deployment_fd(sysroot, deployment)?; - let bound_images = parse_spec_dir(deployment_root, BOUND_IMAGE_DIR)?; + let bound_images = parse_spec_dir(deployment_root)?; pull_images(deployment_root, bound_images) } #[context("parse bound image spec dir")] -fn parse_spec_dir(root: &Dir, spec_dir: &str) -> Result> { +fn parse_spec_dir(root: &Dir) -> Result> { + let spec_dir = BOUND_IMAGE_DIR; let Some(bound_images_dir) = root.open_dir_optional(spec_dir)? else { tracing::debug!("Missing {spec_dir}"); return Ok(Default::default()); @@ -178,12 +179,12 @@ mod tests { // Empty dir should return an empty vector let td = &cap_std_ext::cap_tempfile::TempDir::new(cap_std::ambient_authority())?; - let images = parse_spec_dir(td, &BOUND_IMAGE_DIR).unwrap(); + let images = parse_spec_dir(td).unwrap(); assert_eq!(images.len(), 0); td.create_dir_all(BOUND_IMAGE_DIR).unwrap(); td.create_dir_all(CONTAINER_IMAGE_DIR).unwrap(); - let images = parse_spec_dir(td, &BOUND_IMAGE_DIR).unwrap(); + let images = parse_spec_dir(td).unwrap(); assert_eq!(images.len(), 0); // Should return BoundImages @@ -215,7 +216,7 @@ mod tests { ) .unwrap(); - let mut images = parse_spec_dir(td, &BOUND_IMAGE_DIR).unwrap(); + let mut images = parse_spec_dir(td).unwrap(); images.sort_by(|a, b| a.image.as_str().cmp(&b.image.as_str())); assert_eq!(images.len(), 2); assert_eq!(images[0].image, "quay.io/bar/bar:latest"); @@ -224,13 +225,13 @@ mod tests { // Invalid symlink should return an error td.symlink("./blah", format!("{BOUND_IMAGE_DIR}/blah.image")) .unwrap(); - assert!(parse_spec_dir(td, &BOUND_IMAGE_DIR).is_err()); + assert!(parse_spec_dir(td).is_err()); // Invalid image contents should return an error td.write("error.image", "[Image]\n").unwrap(); td.symlink_contents("/error.image", format!("{BOUND_IMAGE_DIR}/error.image")) .unwrap(); - assert!(parse_spec_dir(td, &BOUND_IMAGE_DIR).is_err()); + assert!(parse_spec_dir(td).is_err()); Ok(()) } From 5c5d201a2f709a952e0134ae2e50ebc87803f1f5 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Thu, 18 Jul 2024 16:40:59 -0400 Subject: [PATCH 3/5] boundimage: Rename helper function I think this name is a bit clearer. Signed-off-by: Colin Walters --- lib/src/boundimage.rs | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/lib/src/boundimage.rs b/lib/src/boundimage.rs index 7718679b..ae362a40 100644 --- a/lib/src/boundimage.rs +++ b/lib/src/boundimage.rs @@ -32,12 +32,12 @@ struct BoundImage { /// Given a deployment, pull all container images it references. pub(crate) fn pull_bound_images(sysroot: &SysrootLock, deployment: &Deployment) -> Result<()> { let deployment_root = &crate::utils::deployment_fd(sysroot, deployment)?; - let bound_images = parse_spec_dir(deployment_root)?; + let bound_images = query_bound_images(deployment_root)?; pull_images(deployment_root, bound_images) } -#[context("parse bound image spec dir")] -fn parse_spec_dir(root: &Dir) -> Result> { +#[context("Querying bound images")] +fn query_bound_images(root: &Dir) -> Result> { let spec_dir = BOUND_IMAGE_DIR; let Some(bound_images_dir) = root.open_dir_optional(spec_dir)? else { tracing::debug!("Missing {spec_dir}"); @@ -179,12 +179,12 @@ mod tests { // Empty dir should return an empty vector let td = &cap_std_ext::cap_tempfile::TempDir::new(cap_std::ambient_authority())?; - let images = parse_spec_dir(td).unwrap(); + let images = query_bound_images(td).unwrap(); assert_eq!(images.len(), 0); td.create_dir_all(BOUND_IMAGE_DIR).unwrap(); td.create_dir_all(CONTAINER_IMAGE_DIR).unwrap(); - let images = parse_spec_dir(td).unwrap(); + let images = query_bound_images(td).unwrap(); assert_eq!(images.len(), 0); // Should return BoundImages @@ -216,7 +216,7 @@ mod tests { ) .unwrap(); - let mut images = parse_spec_dir(td).unwrap(); + let mut images = query_bound_images(td).unwrap(); images.sort_by(|a, b| a.image.as_str().cmp(&b.image.as_str())); assert_eq!(images.len(), 2); assert_eq!(images[0].image, "quay.io/bar/bar:latest"); @@ -225,13 +225,13 @@ mod tests { // Invalid symlink should return an error td.symlink("./blah", format!("{BOUND_IMAGE_DIR}/blah.image")) .unwrap(); - assert!(parse_spec_dir(td).is_err()); + assert!(query_bound_images(td).is_err()); // Invalid image contents should return an error td.write("error.image", "[Image]\n").unwrap(); td.symlink_contents("/error.image", format!("{BOUND_IMAGE_DIR}/error.image")) .unwrap(); - assert!(parse_spec_dir(td).is_err()); + assert!(query_bound_images(td).is_err()); Ok(()) } From f791531df66f696d61ccc2af70695996df8fe39c Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Thu, 18 Jul 2024 16:43:04 -0400 Subject: [PATCH 4/5] boundimage: Make helpers `pub(crate)` Prep for using them in the install path. Signed-off-by: Colin Walters --- lib/src/boundimage.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/src/boundimage.rs b/lib/src/boundimage.rs index ae362a40..3c46923f 100644 --- a/lib/src/boundimage.rs +++ b/lib/src/boundimage.rs @@ -24,7 +24,7 @@ const BOUND_IMAGE_DIR: &str = "usr/lib/bootc-experimental/bound-images.d"; /// In the future this may be extended to include e.g. certificates or /// other pull options. #[derive(PartialEq, Eq)] -struct BoundImage { +pub(crate) struct BoundImage { image: String, auth_file: Option, } @@ -37,7 +37,7 @@ pub(crate) fn pull_bound_images(sysroot: &SysrootLock, deployment: &Deployment) } #[context("Querying bound images")] -fn query_bound_images(root: &Dir) -> Result> { +pub(crate) fn query_bound_images(root: &Dir) -> Result> { let spec_dir = BOUND_IMAGE_DIR; let Some(bound_images_dir) = root.open_dir_optional(spec_dir)? else { tracing::debug!("Missing {spec_dir}"); @@ -113,7 +113,7 @@ fn parse_container_file(file_contents: &tini::Ini) -> Result { } #[context("pull bound images")] -fn pull_images(_deployment_root: &Dir, bound_images: Vec) -> Result<()> { +pub(crate) fn pull_images(_deployment_root: &Dir, bound_images: Vec) -> Result<()> { tracing::debug!("Pulling bound images: {}", bound_images.len()); //TODO: do this in parallel for bound_image in bound_images { From d4acdce83fd9c2b7b65edfeeed67d39d56b447f4 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Wed, 17 Jul 2024 20:09:09 -0400 Subject: [PATCH 5/5] install: Pull bound images by default Unless overridden by a CLI option, in which case they will likely be pulled on firstboot. Signed-off-by: Colin Walters --- lib/src/install.rs | 91 +++++++++++++++++++++++++++++++++++++--------- 1 file changed, 73 insertions(+), 18 deletions(-) diff --git a/lib/src/install.rs b/lib/src/install.rs index ec8cc4a2..6a83ae70 100644 --- a/lib/src/install.rs +++ b/lib/src/install.rs @@ -35,6 +35,7 @@ use ostree_ext::container as ostree_container; use ostree_ext::oci_spec; use ostree_ext::ostree; use ostree_ext::prelude::Cast; +use ostree_ext::sysroot::SysrootLock; use rustix::fs::{FileTypeExt, MetadataExt as _}; use serde::{Deserialize, Serialize}; @@ -160,6 +161,11 @@ pub(crate) struct InstallConfigOpts { #[clap(long)] #[serde(default)] pub(crate) generic_image: bool, + + /// Do not pull any "logically bound" images at install time. + #[clap(long, hide = true)] + #[serde(default)] + pub(crate) skip_bound_images: bool, } #[derive(Debug, Clone, clap::Parser, Serialize, Deserialize, PartialEq, Eq)] @@ -1219,23 +1225,21 @@ async fn prepare_install( Ok(state) } -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()); - } - - // We verify this upfront because it's currently required by bootupd - let boot_uuid = rootfs - .get_boot_uuid()? - .or(rootfs.rootfs_uuid.as_deref()) - .ok_or_else(|| anyhow!("No uuid for boot/root"))?; - tracing::debug!("boot uuid={boot_uuid}"); - - // Initialize the ostree sysroot (repo, stateroot, etc.) - let sysroot = initialize_ostree_root(state, rootfs).await?; +/// Given a baseline root filesystem with an ostree sysroot initialized: +/// - install the container to that root +/// - install the bootloader +/// - Other post operations, such as pulling bound images +async fn install_with_sysroot( + state: &State, + rootfs: &RootSetup, + sysroot: &ostree::Sysroot, + boot_uuid: &str, +) -> Result<()> { + let sysroot = SysrootLock::new_from_sysroot(&sysroot).await?; // And actually set up the container in that root, returning a deployment and // the aleph state (see below). let (deployment, aleph) = install_container(state, rootfs, &sysroot).await?; + let stateroot = deployment.osname(); // Write the aleph data that captures the system state at the time of provisioning for aid in future debugging. rootfs .rootfs_fd @@ -1244,6 +1248,7 @@ async fn install_to_filesystem_impl(state: &State, rootfs: &mut RootSetup) -> Re anyhow::Ok(()) }) .context("Writing aleph version")?; + if cfg!(target_arch = "s390x") { // TODO: Integrate s390x support into install_via_bootupd crate::bootloader::install_via_zipl(&rootfs.device_info, boot_uuid)?; @@ -1254,12 +1259,62 @@ async fn install_to_filesystem_impl(state: &State, rootfs: &mut RootSetup) -> Re &state.config_opts, )?; } + tracing::debug!("Installed bootloader"); - // After this point, we need to drop all open references to the filesystem - drop(deployment); - drop(sysroot); + tracing::debug!("Perfoming post-deployment operations"); + let deployment_root = crate::utils::deployment_fd(&sysroot, &deployment)?; + let bound_images = if state.config_opts.skip_bound_images { + Vec::new() + } else { + crate::boundimage::query_bound_images(&deployment_root)? + }; + if !bound_images.is_empty() { + // TODO: We only do this dance to initialize `/var` at install time if + // there are bound images today; it minimizes side effects. + // However going forward we really do need to handle a separate /var partition... + // and to do that we may in the general case need to run the `var.mount` + // target from the new root. + let varpath = format!("ostree/deploy/{stateroot}/var"); + let var = rootfs + .rootfs_fd + .open_dir(&varpath) + .with_context(|| format!("Opening {varpath}"))?; + Task::new("Mounting deployment /var", "mount") + .args(["--bind", ".", "/var"]) + .cwd(&var)? + .run()?; + // podman needs this + Task::new("Initializing /var/tmp", "systemd-tmpfiles") + .args(["--create", "--boot", "--prefix=/var/tmp"]) + .verbose() + .run()?; + crate::boundimage::pull_images(&deployment_root, bound_images)?; + } - tracing::debug!("Installed bootloader"); + Ok(()) +} + +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()); + } + // Drop exclusive ownership since we're done with mutation + let rootfs = &*rootfs; + + // We verify this upfront because it's currently required by bootupd + let boot_uuid = rootfs + .get_boot_uuid()? + .or(rootfs.rootfs_uuid.as_deref()) + .ok_or_else(|| anyhow!("No uuid for boot/root"))?; + tracing::debug!("boot uuid={boot_uuid}"); + + // Initialize the ostree sysroot (repo, stateroot, etc.) + { + let sysroot = initialize_ostree_root(state, rootfs).await?; + install_with_sysroot(state, rootfs, &sysroot, &boot_uuid).await?; + // We must drop the sysroot here in order to close any open file + // descriptors. + } // Finalize mounted filesystems if !rootfs.skip_finalize {