From a1a81027448ccf7b7b0047baac4258f8ad4dbc14 Mon Sep 17 00:00:00 2001 From: jbtrystram Date: Fri, 28 Nov 2025 18:08:34 +0100 Subject: [PATCH 1/8] install/bootupd: chroot to deployment When `--src-imgref` is passed, the deployed systemd does not match the running environnement. In this case, let's chroot into the deployment before calling bootupd. This makes sure we are using the binaries shipped in the image (and relevant config files such as grub fragements). We could do that in all cases but i kept it behind the `--src-imgref` option since when using the target container as the buildroot it will have no impact, and we expect this scenario to be the most common. In CoreOS we have a specific test that checks if the bootloader was installed with the `grub2-install` of the image. Fixes https://github.com/bootc-dev/bootc/issues/1559 Also see https://github.com/bootc-dev/bootc/issues/1455 Signed-off-by: jbtrystram --- crates/lib/src/bootloader.rs | 51 +++++++++++++++++++++++++++++++----- 1 file changed, 45 insertions(+), 6 deletions(-) diff --git a/crates/lib/src/bootloader.rs b/crates/lib/src/bootloader.rs index 6d27497d7..a688be26d 100644 --- a/crates/lib/src/bootloader.rs +++ b/crates/lib/src/bootloader.rs @@ -9,6 +9,7 @@ use fn_error_context::context; use bootc_blockdev::{Partition, PartitionTable}; use bootc_mount as mount; +use rustix::mount::UnmountFlags; use crate::bootc_composefs::boot::{mount_esp, SecurebootKeys}; use crate::{discoverable_partition_specification, utils}; @@ -54,22 +55,60 @@ pub(crate) fn install_via_bootupd( // bootc defaults to only targeting the platform boot method. let bootupd_opts = (!configopts.generic_image).then_some(["--update-firmware", "--auto"]); - let abs_deployment_path = deployment_path.map(|v| rootfs.join(v)); - let src_root_arg = if let Some(p) = abs_deployment_path.as_deref() { - vec!["--src-root", p.as_str()] + let abs_deployment_path = deployment_path.map(|deploy| rootfs.join(deploy)); + // When not running inside the target container (through `--src-imgref`) we chroot + // into the deployment before running bootupd. This makes sure we use binaries + // from the target image rather than the buildroot + let bind_mount_dirs = ["/dev", "/run", "/proc", "/sys"]; + let chroot_args = if let Some(target_root) = abs_deployment_path.as_deref() { + tracing::debug!("Setting up bind-mounts before chrooting to the target deployment"); + for src in bind_mount_dirs { + let dest = target_root + // joining an absolute path + // makes it replace self, so we strip the prefix + .join_os(src.strip_prefix("/").unwrap()); + tracing::debug!("bind mounting {}", dest.display()); + rustix::mount::mount_bind_recursive(src, dest)?; + } + // Append the `bootupctl` command, it will be passed as + // an argument to chroot + vec![target_root.as_str(), "bootupctl"] } else { vec![] }; + let devpath = device.path(); println!("Installing bootloader via bootupd"); - Command::new("bootupctl") + let mut bootupctl = if abs_deployment_path.is_some() { + Command::new("chroot") + } else { + Command::new("bootupctl") + }; + let install_result = bootupctl + .args(chroot_args) .args(["backend", "install", "--write-uuid"]) .args(verbose) .args(bootupd_opts.iter().copied().flatten()) - .args(src_root_arg) .args(["--device", devpath.as_str(), rootfs.as_str()]) .log_debug() - .run_inherited_with_cmd_context() + .run_inherited_with_cmd_context(); + + // Clean up the mounts after ourselves + if let Some(target_root) = abs_deployment_path { + let mut unmount_res = Ok(()); + for dir in bind_mount_dirs { + let mount = target_root + .join(dir.strip_prefix("/").unwrap()) + .into_std_path_buf(); + if let Err(e) = rustix::mount::unmount(&mount, UnmountFlags::DETACH) { + tracing::warn!("Error unmounting {}: {e}", mount.display()); + unmount_res = Err(e.into()); + } + } + install_result.and(unmount_res) + } else { + install_result + } } #[context("Installing bootloader")] From 73abdc68891a81297a14ecd74a87b71b6960ae9e Mon Sep 17 00:00:00 2001 From: jbtrystram Date: Thu, 11 Dec 2025 10:50:09 +0100 Subject: [PATCH 2/8] install/bootloader when chrooting for bootupctl do try to pass the target rootfs path --- crates/lib/src/bootloader.rs | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/crates/lib/src/bootloader.rs b/crates/lib/src/bootloader.rs index a688be26d..583613170 100644 --- a/crates/lib/src/bootloader.rs +++ b/crates/lib/src/bootloader.rs @@ -59,6 +59,13 @@ pub(crate) fn install_via_bootupd( // When not running inside the target container (through `--src-imgref`) we chroot // into the deployment before running bootupd. This makes sure we use binaries // from the target image rather than the buildroot + // But then `/target` (or wherever the user mounted the target FS) is not available, + // but since bootupd use that just to find the underlying device, + // we can use the deployement path just fine. + // Another way of doing this would be to enforce having the target + // rootfs mounted under `/run` so we'd get access to it as part of + // the standard bind-mounts below. + let chroot_root: Option<&str>; let bind_mount_dirs = ["/dev", "/run", "/proc", "/sys"]; let chroot_args = if let Some(target_root) = abs_deployment_path.as_deref() { tracing::debug!("Setting up bind-mounts before chrooting to the target deployment"); @@ -70,10 +77,12 @@ pub(crate) fn install_via_bootupd( tracing::debug!("bind mounting {}", dest.display()); rustix::mount::mount_bind_recursive(src, dest)?; } + chroot_root = Some("/"); // Append the `bootupctl` command, it will be passed as // an argument to chroot vec![target_root.as_str(), "bootupctl"] } else { + chroot_root = None; vec![] }; @@ -89,7 +98,8 @@ pub(crate) fn install_via_bootupd( .args(["backend", "install", "--write-uuid"]) .args(verbose) .args(bootupd_opts.iter().copied().flatten()) - .args(["--device", devpath.as_str(), rootfs.as_str()]) + .args(["--device", devpath.as_str()]) + .args(chroot_root.or(Some(rootfs.as_str()))) .log_debug() .run_inherited_with_cmd_context(); From 0a6fa9268e1234a5a8f378c8e2091c01d5b55b5a Mon Sep 17 00:00:00 2001 From: jbtrystram Date: Thu, 11 Dec 2025 11:38:20 +0100 Subject: [PATCH 3/8] add debug output to bootupctl for install tests --- crates/tests-integration/src/install.rs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/crates/tests-integration/src/install.rs b/crates/tests-integration/src/install.rs index 8487c0354..b3d2e4bf3 100644 --- a/crates/tests-integration/src/install.rs +++ b/crates/tests-integration/src/install.rs @@ -9,7 +9,14 @@ use fn_error_context::context; use libtest_mimic::Trial; use xshell::{cmd, Shell}; -pub(crate) const BASE_ARGS: &[&str] = &["podman", "run", "--rm", "--privileged", "--pid=host"]; +pub(crate) const BASE_ARGS: &[&str] = &[ + "podman", + "run", + "--rm", + "--privileged", + "--pid=host", + "--env BOOTC_BOOTLOADER_DEBUG=true", +]; // Arbitrary const NON_DEFAULT_STATEROOT: &str = "foo"; From 858bcec019e801e38070915212e05a90f47a93dc Mon Sep 17 00:00:00 2001 From: jbtrystram Date: Thu, 11 Dec 2025 12:30:29 +0100 Subject: [PATCH 4/8] install/bootloader bind-mount rootfs to /run before chrooting --- crates/lib/src/bootloader.rs | 25 ++++++++++++++----------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/crates/lib/src/bootloader.rs b/crates/lib/src/bootloader.rs index 583613170..565c459cf 100644 --- a/crates/lib/src/bootloader.rs +++ b/crates/lib/src/bootloader.rs @@ -10,6 +10,7 @@ use fn_error_context::context; use bootc_blockdev::{Partition, PartitionTable}; use bootc_mount as mount; use rustix::mount::UnmountFlags; +use rustix::path::Arg; use crate::bootc_composefs::boot::{mount_esp, SecurebootKeys}; use crate::{discoverable_partition_specification, utils}; @@ -59,13 +60,18 @@ pub(crate) fn install_via_bootupd( // When not running inside the target container (through `--src-imgref`) we chroot // into the deployment before running bootupd. This makes sure we use binaries // from the target image rather than the buildroot - // But then `/target` (or wherever the user mounted the target FS) is not available, - // but since bootupd use that just to find the underlying device, - // we can use the deployement path just fine. - // Another way of doing this would be to enforce having the target - // rootfs mounted under `/run` so we'd get access to it as part of - // the standard bind-mounts below. - let chroot_root: Option<&str>; + // In some cases (e.g. --write-uuid), bootupd needs to find the underlying device + // for /boot. But since we don't control where the destination rootfs is mounted + // let's bind mount it to a temp mountpoint under /run + // so it gets carried over in the chroot. + + let rootfs_mount = if rootfs.starts_with("/run") { + rootfs.to_path_buf().into_std_path_buf() + } else { + let rootfs_mount = tempfile::tempdir_in("/run")?.keep(); + rustix::mount::mount_bind_recursive(rootfs.as_std_path(), &rootfs_mount)?; + rootfs_mount + }; let bind_mount_dirs = ["/dev", "/run", "/proc", "/sys"]; let chroot_args = if let Some(target_root) = abs_deployment_path.as_deref() { tracing::debug!("Setting up bind-mounts before chrooting to the target deployment"); @@ -77,12 +83,10 @@ pub(crate) fn install_via_bootupd( tracing::debug!("bind mounting {}", dest.display()); rustix::mount::mount_bind_recursive(src, dest)?; } - chroot_root = Some("/"); // Append the `bootupctl` command, it will be passed as // an argument to chroot vec![target_root.as_str(), "bootupctl"] } else { - chroot_root = None; vec![] }; @@ -98,8 +102,7 @@ pub(crate) fn install_via_bootupd( .args(["backend", "install", "--write-uuid"]) .args(verbose) .args(bootupd_opts.iter().copied().flatten()) - .args(["--device", devpath.as_str()]) - .args(chroot_root.or(Some(rootfs.as_str()))) + .args(["--device", devpath.as_str(), rootfs_mount.as_str()?]) .log_debug() .run_inherited_with_cmd_context(); From 7d2f8ff6a2533b1491e27ddd0370e5f84c685de5 Mon Sep 17 00:00:00 2001 From: jbtrystram Date: Thu, 11 Dec 2025 13:59:48 +0100 Subject: [PATCH 5/8] fixup! add debug output to bootupctl for install tests --- crates/tests-integration/src/install.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/crates/tests-integration/src/install.rs b/crates/tests-integration/src/install.rs index b3d2e4bf3..63a9a154a 100644 --- a/crates/tests-integration/src/install.rs +++ b/crates/tests-integration/src/install.rs @@ -15,7 +15,8 @@ pub(crate) const BASE_ARGS: &[&str] = &[ "--rm", "--privileged", "--pid=host", - "--env BOOTC_BOOTLOADER_DEBUG=true", + "--env", + "BOOTC_BOOTLOADER_DEBUG=true", ]; // Arbitrary From 2924cc669578c88e7f59ca11d4b4746048ba9116 Mon Sep 17 00:00:00 2001 From: jbtrystram Date: Thu, 11 Dec 2025 15:21:06 +0100 Subject: [PATCH 6/8] fixup! install/bootloader bind-mount rootfs to /run before chrooting --- crates/lib/src/bootloader.rs | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/crates/lib/src/bootloader.rs b/crates/lib/src/bootloader.rs index 565c459cf..8b13d6b56 100644 --- a/crates/lib/src/bootloader.rs +++ b/crates/lib/src/bootloader.rs @@ -108,20 +108,18 @@ pub(crate) fn install_via_bootupd( // Clean up the mounts after ourselves if let Some(target_root) = abs_deployment_path { - let mut unmount_res = Ok(()); for dir in bind_mount_dirs { let mount = target_root .join(dir.strip_prefix("/").unwrap()) .into_std_path_buf(); if let Err(e) = rustix::mount::unmount(&mount, UnmountFlags::DETACH) { + // let's not propagate the error up because in some cases we can't unmount + // e.g. when running `to-existing-root` tracing::warn!("Error unmounting {}: {e}", mount.display()); - unmount_res = Err(e.into()); } } - install_result.and(unmount_res) - } else { - install_result } + install_result } #[context("Installing bootloader")] From 7a14a4bf27f2a1c5482d1fe42c693a5be0e6d8f9 Mon Sep 17 00:00:00 2001 From: jbtrystram Date: Wed, 17 Dec 2025 21:32:21 +0100 Subject: [PATCH 7/8] insert a reasonnable default PATH into the chroot --- crates/lib/src/bootloader.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/crates/lib/src/bootloader.rs b/crates/lib/src/bootloader.rs index 8b13d6b56..2445bc2fb 100644 --- a/crates/lib/src/bootloader.rs +++ b/crates/lib/src/bootloader.rs @@ -99,6 +99,10 @@ pub(crate) fn install_via_bootupd( }; let install_result = bootupctl .args(chroot_args) + .env( + "PATH", + "/bin:/usr/bin:/sbin:/usr/sbin:/usr/local/bin:/usr/local/sbin", + ) .args(["backend", "install", "--write-uuid"]) .args(verbose) .args(bootupd_opts.iter().copied().flatten()) From eafdea019b54222e8b26a9aa9626b700a98d69b8 Mon Sep 17 00:00:00 2001 From: jbtrystram Date: Thu, 18 Dec 2025 13:17:49 +0100 Subject: [PATCH 8/8] DNM: add more debug --- crates/tests-integration/src/install.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/crates/tests-integration/src/install.rs b/crates/tests-integration/src/install.rs index 63a9a154a..3264bf658 100644 --- a/crates/tests-integration/src/install.rs +++ b/crates/tests-integration/src/install.rs @@ -17,6 +17,8 @@ pub(crate) const BASE_ARGS: &[&str] = &[ "--pid=host", "--env", "BOOTC_BOOTLOADER_DEBUG=true", + "--env", + "RUST_LOG=debug", ]; // Arbitrary