diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 36e269ce15..5ba04583b8 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -302,9 +302,6 @@ jobs: seal_state: ["sealed", "unsealed"] exclude: - # https://github.com/bootc-dev/bootc/issues/1812 - - test_os: centos-9 - variant: composefs - seal_state: "sealed" boot_type: bls - seal_state: "sealed" diff --git a/Makefile b/Makefile index ff96de6923..5577e9b0a5 100644 --- a/Makefile +++ b/Makefile @@ -29,7 +29,15 @@ prefix ?= /usr # We may in the future also want to include Fedora+derivatives as # the code is really tiny. # (Note we should also make installation of the units conditional on the rhsm feature) -CARGO_FEATURES_DEFAULT ?= $(shell . /usr/lib/os-release; if echo "$$ID_LIKE" |grep -qF rhel; then echo rhsm; fi) +# +# Enable the rhel9 feature on RHEL/CentOS Stream 9, which runs kernel 5.14. +# That kernel cannot mount an erofs image directly from a file descriptor; +# composefs-ctl's rhel9 feature activates a loopback-device fallback instead. +CARGO_FEATURES_DEFAULT ?= $(shell . /usr/lib/os-release; \ + features=""; \ + if echo "$$ID_LIKE" | grep -qF rhel; then features="$$features rhsm"; fi; \ + if echo "$$ID_LIKE" | grep -qF rhel && [ "$$VERSION_ID" = "9" ]; then features="$$features rhel9"; fi; \ + echo $$features) # You can set this to override all cargo features, including the defaults CARGO_FEATURES ?= $(CARGO_FEATURES_DEFAULT) diff --git a/contrib/packaging/bootc.spec b/contrib/packaging/bootc.spec index 7ce0f7fd2b..1865a4a7e0 100644 --- a/contrib/packaging/bootc.spec +++ b/contrib/packaging/bootc.spec @@ -12,6 +12,14 @@ %bcond_with rhsm %endif +# kernel 5.14 (RHEL/CentOS 9) cannot mount an erofs image directly from a file +# descriptor; composefs-ctl's rhel9 feature enables a loopback-device fallback. +%if 0%{?rhel} == 9 + %bcond_without rhel9 +%else + %bcond_with rhel9 +%endif + %global rust_minor %(rustc --version | cut -f2 -d" " | cut -f2 -d".") # https://github.com/bootc-dev/bootc/issues/1640 @@ -132,13 +140,16 @@ make manpages # Build all binaries %if 0%{?container_build} # Container build: use cargo directly with cached dependencies to avoid RPM macro overhead -cargo build -j%{_smp_build_ncpus} --release %{?with_rhsm:--features rhsm} --bins +cargo build -j%{_smp_build_ncpus} --release %{?with_rhsm:--features rhsm} %{?with_rhel9:--features rhel9} --bins %else # Non-container build: use RPM macros for proper dependency tracking %if %new_cargo_macros - %cargo_build %{?with_rhsm:-f rhsm} -- --bins + # Note: %%cargo_build's own -f option only accepts a single value, so a + # second -f would silently clobber the first; pass extra features as + # plain --features args after -- instead, which cargo unions correctly. + %cargo_build -- %{?with_rhsm:--features rhsm} %{?with_rhel9:--features rhel9} --bins %else - %cargo_build %{?with_rhsm:--features rhsm} -- --bins + %cargo_build %{?with_rhsm:--features rhsm} %{?with_rhel9:--features rhel9} -- --bins %endif %endif @@ -152,7 +163,7 @@ sed -i -e '/https:\/\//d' cargo-vendor.txt %install # Pass CARGO_FEATURES explicitly to prevent auto-detection rebuild in install environment -%make_install INSTALL="install -p -c" CARGO_FEATURES="%{?with_rhsm:rhsm}" +%make_install INSTALL="install -p -c" CARGO_FEATURES="%{?with_rhsm:rhsm} %{?with_rhel9:rhel9}" %if %{with ostree_ext} make install-ostree-hooks DESTDIR=%{?buildroot} %endif diff --git a/crates/lib/src/cli.rs b/crates/lib/src/cli.rs index 86b4eb9998..b87cc49796 100644 --- a/crates/lib/src/cli.rs +++ b/crates/lib/src/cli.rs @@ -1210,7 +1210,7 @@ struct ApplyFromDownloadedOpts { apply: bool, } -fn apply_from_downloaded_ostree( +async fn apply_from_downloaded_ostree( storage: &Storage, booted_ostree: &BootedOstree<'_>, host: &crate::spec::Host, @@ -1222,6 +1222,7 @@ fn apply_from_downloaded_ostree( .ok_or_else(|| anyhow::anyhow!("No staged deployment found"))?; if staged_deployment.is_finalization_locked() { + crate::boundimage::pull_bound_images(storage, &staged_deployment).await?; ostree.change_finalization(&staged_deployment)?; println!("Staged deployment will now be applied on reboot"); } else { @@ -1298,7 +1299,8 @@ async fn upgrade( soft_reboot: opts.soft_reboot, apply: opts.apply, }, - ); + ) + .await; } // Ensure the bootc storage directory is initialized; the --check path @@ -1479,7 +1481,8 @@ async fn switch_ostree( soft_reboot: opts.soft_reboot, apply: opts.apply, }, - ); + ) + .await; } let target = imgref_for_switch(&opts)?; diff --git a/crates/lib/src/deploy.rs b/crates/lib/src/deploy.rs index b361a3b79f..3346c6c972 100644 --- a/crates/lib/src/deploy.rs +++ b/crates/lib/src/deploy.rs @@ -46,7 +46,7 @@ use std::collections::HashSet; use std::io::{BufRead, Write}; -use std::os::fd::AsFd; +use std::os::fd::{AsFd, AsRawFd}; use std::process::Command; use anyhow::{Context, Result, anyhow}; @@ -1007,6 +1007,38 @@ impl MergeState { } } +/// Pull the bound images referenced by an imported commit before staging it. +#[context("Pulling bound images for ostree commit {commit}")] +async fn pull_bound_images_for_commit(sysroot: &Storage, commit: &str) -> Result<()> { + let repo = sysroot.get_ostree()?.repo(); + let repo_dir = Dir::reopen_dir(&repo.dfd_borrow())?; + let repo_tmp = repo_dir + .open_dir("tmp") + .context("Opening ostree repo tmp/")?; + let td = cap_std_ext::cap_tempfile::TempDir::new_in(&repo_tmp)?; + let checkout_mode = if repo.mode() == ostree::RepoMode::Bare { + ostree::RepoCheckoutMode::None + } else { + ostree::RepoCheckoutMode::User + }; + let checkout_opts = ostree::RepoCheckoutAtOptions { + mode: checkout_mode, + ..Default::default() + }; + let root_name = "root"; + repo.checkout_at( + Some(&checkout_opts), + td.as_raw_fd(), + root_name, + commit, + gio::Cancellable::NONE, + ) + .context("Checking out imported commit")?; + let root = td.open_dir(root_name)?; + let bound_images = crate::boundimage::query_bound_images(&root)?; + crate::boundimage::pull_images(sysroot, bound_images).await +} + /// Stage (queue deployment of) a fetched container image. #[context("Staging")] pub(crate) async fn stage( @@ -1054,9 +1086,9 @@ pub(crate) async fn stage( subtask.completed = true; subtasks.push(subtask.clone()); - subtask.subtask = "deploying".into(); - subtask.id = "deploying".into(); - subtask.description = "Deploying Image".into(); + subtask.subtask = "bound_images".into(); + subtask.id = "bound_images".into(); + subtask.description = "Pulling Bound Images".into(); subtask.completed = false; prog.send(Event::ProgressSteps { task: "staging".into(), @@ -1072,15 +1104,13 @@ pub(crate) async fn stage( .collect(), }) .await; - let origin = origin_from_imageref(spec.image)?; - let deployment = - crate::deploy::deploy(sysroot, from, image, &origin, lock_finalization).await?; + pull_bound_images_for_commit(sysroot, &image.ostree_commit).await?; subtask.completed = true; subtasks.push(subtask.clone()); - subtask.subtask = "bound_images".into(); - subtask.id = "bound_images".into(); - subtask.description = "Pulling Bound Images".into(); + subtask.subtask = "deploying".into(); + subtask.id = "deploying".into(); + subtask.description = "Deploying Image".into(); subtask.completed = false; prog.send(Event::ProgressSteps { task: "staging".into(), @@ -1096,7 +1126,8 @@ pub(crate) async fn stage( .collect(), }) .await; - crate::boundimage::pull_bound_images(sysroot, &deployment).await?; + let origin = origin_from_imageref(spec.image)?; + crate::deploy::deploy(sysroot, from, image, &origin, lock_finalization).await?; subtask.completed = true; subtasks.push(subtask.clone()); diff --git a/crates/utils/Cargo.toml b/crates/utils/Cargo.toml index 5377678310..b0fed6ec44 100644 --- a/crates/utils/Cargo.toml +++ b/crates/utils/Cargo.toml @@ -17,7 +17,6 @@ rustix = { workspace = true } serde = { workspace = true, features = ["derive"] } serde_json = { workspace = true } shlex = { workspace = true } -tempfile = { workspace = true } tokio = { workspace = true, features = ["process", "rt", "macros"] } tracing = { workspace = true } tracing-subscriber = { workspace = true } @@ -26,6 +25,7 @@ tracing-journald = { workspace = true } [dev-dependencies] similar-asserts = { workspace = true } static_assertions = { workspace = true } +tempfile = { workspace = true } [lints] workspace = true diff --git a/crates/utils/src/command.rs b/crates/utils/src/command.rs index 0a9d759c11..afd565deeb 100644 --- a/crates/utils/src/command.rs +++ b/crates/utils/src/command.rs @@ -9,6 +9,16 @@ use std::{ use anyhow::{Context, Result}; +/// Create a seekable, filesystem-independent file for command output. +fn command_output_file() -> Result { + // bootc's command helpers run from a systemd generator. Generators on + // systemd 252 and older may see a read-only /tmp (253+ provides a private + // writable /tmp), so output capture must not rely on filesystem temp files. + rustix::fs::memfd_create("bootc-command-output", rustix::fs::MemfdFlags::CLOEXEC) + .map(std::fs::File::from) + .context("create memfd for command output") +} + /// Helpers intended for [`std::process::Command`]. pub trait CommandRunExt { /// Log (at debug level) the full child commandline. @@ -139,7 +149,7 @@ impl CommandRunExt for Command { /// Synchronously execute the child, and return an error if the child exited unsuccessfully. fn run_capture_stderr(&mut self) -> Result<()> { - let stderr = tempfile::tempfile()?; + let stderr = command_output_file()?; self.stderr(stderr.try_clone()?); tracing::trace!("exec: {self:?}"); self.status()?.check_status_with_stderr(stderr) @@ -168,7 +178,7 @@ impl CommandRunExt for Command { } fn run_get_output(&mut self) -> Result> { - let mut stdout = tempfile::tempfile()?; + let mut stdout = command_output_file()?; self.stdout(stdout.try_clone()?); self.run_capture_stderr()?; stdout.seek(std::io::SeekFrom::Start(0)).context("seek")?; @@ -220,7 +230,7 @@ pub trait AsyncCommandRunExt { impl AsyncCommandRunExt for tokio::process::Command { async fn run(&mut self) -> Result<()> { - let stderr = tempfile::tempfile()?; + let stderr = command_output_file()?; self.stderr(stderr.try_clone()?); self.status().await?.check_status_with_stderr(stderr) } @@ -283,6 +293,22 @@ mod tests { ); } + #[test] + fn command_output_file_is_a_memfd() { + use std::os::fd::AsRawFd; + + let file = command_output_file().unwrap(); + // An unprivileged test cannot reliably make /tmp read-only, so verify + // directly that the capture backing file is a memfd instead. + let fd_path = format!("/proc/self/fd/{}", file.as_raw_fd()); + let target = std::fs::read_link(fd_path).unwrap(); + assert!( + target + .to_string_lossy() + .contains("memfd:bootc-command-output") + ); + } + #[test] fn exit_status_check_status() { use std::process::Command; @@ -307,14 +333,14 @@ mod tests { // Test successful exit status let mut success_status = Command::new("true").status().unwrap(); - let temp_stderr = tempfile::tempfile().unwrap(); + let temp_stderr = command_output_file().unwrap(); success_status .check_status_with_stderr(temp_stderr) .unwrap(); // Test failed exit status with stderr content let mut fail_status = Command::new("false").status().unwrap(); - let mut temp_stderr = tempfile::tempfile().unwrap(); + let mut temp_stderr = command_output_file().unwrap(); write!(temp_stderr, "test error message").unwrap(); let e = fail_status .check_status_with_stderr(temp_stderr) @@ -351,6 +377,13 @@ mod tests { let (success, fail) = tokio::join!(success.run(), fail.run(),); success.unwrap(); assert!(fail.is_err()); + + let error = AsyncCommand::new("/bin/sh") + .args(["-c", "echo expected-async-error 1>&2; exit 1"]) + .run() + .await + .unwrap_err(); + assert!(error.to_string().contains("expected-async-error")); } #[test] diff --git a/tmt/tests/booted/test-image-pushpull-upgrade.nu b/tmt/tests/booted/test-image-pushpull-upgrade.nu index 708b868ecf..237cb3c3f1 100644 --- a/tmt/tests/booted/test-image-pushpull-upgrade.nu +++ b/tmt/tests/booted/test-image-pushpull-upgrade.nu @@ -130,9 +130,12 @@ def sanity_check_switch_progress_json [data] { assert equal $deploy.steps 3 assert equal $deploy.stepsTotal 3 let deploy_tasks = $deploy.subtasks + # Bound images are now pulled before staging (see deploy::stage), so + # the "bound_images" subtask now comes before "deploying" instead of + # after it. assert equal ($deploy_tasks | length) 5 let deploy_names = $deploy_tasks | get subtask - assert equal $deploy_names ["merging", "deploying", "bound_images", "cleanup", "cleanup"] + assert equal $deploy_names ["merging", "bound_images", "deploying", "cleanup", "cleanup"] } # The second boot; verify we're in the derived image diff --git a/tmt/tests/booted/test-logically-bound-switch.nu b/tmt/tests/booted/test-logically-bound-switch.nu index 298d7ff86a..c756ca9d60 100644 --- a/tmt/tests/booted/test-logically-bound-switch.nu +++ b/tmt/tests/booted/test-logically-bound-switch.nu @@ -102,6 +102,18 @@ def first_boot [] { }] let image_name = "localhost/bootc-bound" + let unavailable_images = [{ + "bound": true, + "image": "invalid.invalid/bootc-bound-image-does-not-exist:latest", + "name": "unavailable" + }] + build_image $image_name $unavailable_images [] + let failed_switch = do { bootc switch --transport containers-storage $image_name } | complete + assert ($failed_switch.exit_code != 0) "switch should fail when a bound image cannot be pulled" + assert ((bootc status --json | from json | get status.staged) == null) "no deployment should be staged after a bound image pull failure" + + # Rebuilding the tag verifies that a failed pull leaves no partial staged + # deployment which would prevent a successful retry. build_image $image_name $images $containers bootc switch --transport containers-storage $image_name verify_images $images $containers