install/bootupd: chroot to deployment #1816

jbtrystram · 2025-11-29T12:31:40Z

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 #1559 Also see #1455

gemini-code-assist

Code Review

This pull request introduces a chroot into the deployment before calling bootupd when --src-imgref is used. This is a sensible approach to ensure that the binaries and configuration files from the target image are used for bootloader installation. The implementation correctly sets up bind mounts for necessary pseudo-filesystems. However, I've identified a potential resource leak where these bind mounts would not be cleaned up if the bootupd command fails. My review includes a suggestion to refactor the cleanup logic to ensure it runs regardless of the command's success or failure, preventing leaked mounts.

crates/lib/src/bootloader.rs

jbtrystram · 2025-11-30T14:38:08Z

/gemini review

gemini-code-assist

Code Review

This pull request introduces a chroot into the deployment before calling bootupd when a source image reference is provided. This is a good change to ensure the correct binaries and configuration from the target image are used.

My review has identified two issues with the resource management of the bind mounts:

A high-severity issue where mounts can be leaked if an error occurs during the setup of the bind mounts.
A medium-severity issue in the cleanup logic where only the last error from multiple unmount failures would be reported.

I've provided detailed comments and a suggestion for one of the issues. Addressing these will make the implementation more robust.

gemini-code-assist · 2025-11-30T14:40:42Z

crates/lib/src/bootloader.rs

+        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)?;
+        }


If mount_bind_recursive fails for any of the directories (e.g., the second one), the function will return early due to the ? operator. This will leak any mounts that were successfully created in previous iterations of the loop, as the cleanup code at the end of the function will not be executed.

To fix this, you should ensure that cleanup is always performed. A common pattern in Rust is to use a guard struct with a Drop implementation. However, since you want to propagate unmount errors, a Drop guard (which cannot return errors) might not be what you want.

An alternative is to restructure the code to ensure the cleanup block is always reached. For example, you could wrap the mounting and command execution in a closure that returns a Result, and then perform cleanup regardless of whether it succeeded or failed. This would involve tracking which directories were successfully mounted.

Example structure:

let mut mounted_dirs = Vec::new(); let result = (|| { // Mount loop, add to mounted_dirs on success for src in bind_mount_dirs { // ... mount logic ... mounted_dirs.push(src); } // Execute command // ... Ok(()) })(); // Cleanup loop over mounted_dirs // ... // Return combined result

This is a significant refactoring, but it's crucial for resource safety.

gemini-code-assist · 2025-11-30T14:40:42Z

crates/lib/src/bootloader.rs

+            if let Err(e) = rustix::mount::unmount(&mount, UnmountFlags::DETACH) {
+                tracing::warn!("Error unmounting {}: {e}", mount.display());
+                unmount_res = Err(e.into());
+            }


In the case of multiple unmount failures, this logic will overwrite previous errors, and only the last error will be propagated. To ensure the first error is preserved and reported, you should only set unmount_res if it's currently Ok.

if let Err(e) = rustix::mount::unmount(&mount, UnmountFlags::DETACH) { tracing::warn!("Error unmounting {}: {e}", mount.display()); if unmount_res.is_ok() { unmount_res = Err(e.into()); } }

cgwalters · 2025-12-01T19:58:04Z

crates/lib/src/bootloader.rs

-    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


There's other threads were we talked about offering a bootc install mount as a general ability to mount a deployment outside of booting it; were we to do that it would make a lot of sense for this code to use it.

In ostree we resisted doing that for a long time but eventually did just internally for selinux, see https://github.com/ostreedev/ostree/blob/c6f0b5b2bc26b22fbceee0dc28a0f31349c28d41/src/libostree/ostree-sysroot-deploy.c#L3308

On that topic, it'd be a lot cleaner even here to use a more proper containerization than just setting up the mounts. It's a bit tricky though because we actually do need to e.g. pass through all of /dev and /sys here (i.e. --privileged in docker/podman terms) in order to update the ESP if desired.

I haven't looked at which of bwrap/{runc,crun}/nspawn/podman would make the most sense for this use case.

On that topic, it'd be a lot cleaner even here to use a more proper containerization than just setting up the mounts. It's a bit tricky though because we actually do need to e.g. pass through all of /dev and /sys here (i.e. --privileged in docker/podman terms) in order to update the ESP if desired.

I haven't looked at which of bwrap/{runc,crun}/nspawn/podman would make the most sense for this use case.

I am not sure of what you mean with this comment. Do you want to block this change until there are more proper containerization helpers in bootc, or are you just making a note that this should be revisited later on ?

We had a live chat about this and agreed to merge as is and file a tracker followup issue for improving the mount setup.

cgwalters · 2025-12-01T21:06:21Z

crates/lib/src/bootloader.rs

-        .run_inherited_with_cmd_context()
+        .run_inherited_with_cmd_context();
+
+    // Clean up the mounts after ourselves


We could entirely avoid the need to clean up by using the new mount API to get file descriptors instead, and then use https://docs.rs/cap-std-ext/latest/cap_std_ext/cmdext/trait.CapStdExtCommandExt.html#tymethod.cwd_dir with chroot . or so

cgwalters · 2025-12-05T15:58:01Z

OK there's some legit failures here like content: error: boot data installation failed: installing component EFI: Listing partitions of /dev/loop0: No such file or directory (os error 2).

cgwalters

Marking as requested changes due to failing CI

cgwalters

Marking as requested changes due to failing CI

crates/lib/src/bootloader.rs

cgwalters · 2025-12-16T14:37:38Z

crates/lib/src/bootloader.rs

+        }
+        // Append the `bootupctl` command, it will be passed as
+        // an argument to chroot
+        vec![target_root.as_str(), "bootupctl"]


Can't we just pass / here instead of target_root?

we would then need to have another bind mount rootfs/boot into deployment_path/boot I think.

Ok so i ended up doing just that and now more the tests are passing.

crates/lib/src/bootloader.rs

cgwalters · 2025-12-16T14:41:04Z

crates/lib/src/bootloader.rs

+    // 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") {


Thinking about this it may be a lot simpler to just setup the API filesystems by default in the install flow.

(and per discussion mount e.g. /boot in the target rootfs)

cgwalters · 2025-12-16T14:57:35Z

Note that it's only Fedora variants that are failing; should reproduce locally via e.g. just base=quay.io/fedora/fedora-bootc:42 test-tmt install-unified

jbtrystram · 2025-12-17T13:14:19Z

Note that it's only Fedora variants that are failing; should reproduce locally via e.g. just base=quay.io/fedora/fedora-bootc:42 test-tmt install-unified

Yes, i can reproduce that locally ! Ok so I dug deeper and I think I figured out something :
These tmt test run the install process through a systemd transient unit with MountFlags=Slave, which cause the /dev/loop device to not get mounted inside the chroot target.

I am not sure what happens because removing the MountFlags does not fix it, but removing the systemd-run wrapper does help :

truncate -s 10G disk.img
systemd-run  -qdPG -- /bin/sh -c $"./bootc install to-disk --disable-selinux --via-loopback --filesystem xfs  --source-imgref docker://quay.io/centos-bootc/centos-bootc:stream10 ./disk.img"
....
Installing bootloader via bootupd
error: boot data installation failed: installing component EFI: Listing partitions of /dev/loop0: No such file or directory (os error 2)

Without the systemd wrapper :

truncate -s 10G disk.img
./bootc install to-disk --disable-selinux --via-loopback --filesystem xfs  --source-imgref docker://quay.io/centos-bootc/centos-bootc:stream10 ./disk.img
.....
Bootloader: grub
Installing bootloader via bootupd
Added 01_users.cfg
Added 10_blscfg.cfg
Added 14_menu_show_once.cfg
Added 30_uefi-firmware.cfg
Added 41_custom.cfg
Installed: grub.cfg
Installed: bootuuid.cfg
Installed: "centos/grub.cfg"
Installed: "centos/bootuuid.cfg"

In the second case the chroot works.

Another thing to note, and i cannot figure this out yet : the error finding the /dev/loop0 device is yielded from bootc code and not bootupd.
Ah, nevermind : it's pulled in bootupd : https://github.com/coreos/bootupd/blob/78dc9baea7e8418e040ba0217caba63b25dcfb75/src/blockdev.rs#L39-L53

jbtrystram · 2025-12-17T14:32:01Z

Thinking more about this : in both cases the mounting and formating of the block device (disk.img) through the loopback device works. So it's systemd not letting us bind mount inside the chroot ?

jbtrystram · 2025-12-17T20:08:44Z

Ok so after doing a bunch of testing around systemd-run it's not the cause.
I ended up changing the bootloader.rs code to simply do : chroot <deployment_path> /usr/sbin/sfdisk -J /dev/loop0 and it works !? But it does not work without specifying sfdisk full path. So maybe it's down to a $PATH issue ?

I will try to add the full path but that requires a rebuild of bootupd as well, so that's for tomorrow.

edit: ok I did ask bootc to run which sfdisk in the chroot and sure enough: no sfdisk in (/usr/local/bin:/usr/bin)

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 bootc-dev#1559 Also see bootc-dev#1455 Signed-off-by: jbtrystram <[email protected]>

Signed-off-by: jbtrystram <[email protected]>

jbtrystram · 2026-01-07T18:14:26Z

/gemini review

gemini-code-assist

Code Review

This pull request introduces a chroot mechanism when installing the bootloader with bootupd via --src-imgref. This ensures that the binaries and configuration from the target image are used. The changes look mostly correct, but I've found a few issues.

There is a critical bug in how the rootfs_mount path is determined, which will break the non-chroot installation path. I've also found a high-severity issue where an unmount operation is performed unconditionally while the corresponding mount is conditional, which could lead to errors.

Additionally, I've left some comments regarding leftover commented-out code, a WIP comment, and a suggestion to make the code more robust by avoiding .unwrap().

Once these issues are addressed, the changes should be good to go.

gemini-code-assist · 2026-01-07T18:18:01Z

crates/lib/src/bootloader.rs

+    let rootfs_mount = if rootfs.starts_with("/run") {
+        rootfs.as_str()
+    } else {
+        "/"
+    };


The logic for determining rootfs_mount is incorrect and will cause failures in some scenarios.

Non-chroot case: If abs_deployment_path is None and rootfs does not start with /run (e.g., /tmp/target), rootfs_mount incorrectly becomes /. The bootupctl command will then fail because it's pointed at the wrong root filesystem. It should be rootfs.as_str().

Chroot case: If abs_deployment_path is Some and rootfs starts with /run, rootfs_mount becomes the path to rootfs. However, when chrooted, bootupctl should operate on / as its root filesystem.

This logic should be dependent on whether a chroot is being performed. I recommend refactoring this to set rootfs_mount to / in the chroot case and rootfs.as_str() otherwise.

gemini-code-assist · 2026-01-07T18:18:01Z

crates/lib/src/bootloader.rs

+        if let Err(e) = rustix::mount::unmount(
+            target_root.join("boot").into_std_path_buf(),
+            UnmountFlags::DETACH,
+        ) {
+            tracing::warn!("Error unmounting target/boot: {e}");
+        }


The unmount of target_root.join("boot") is performed unconditionally, but the corresponding mount only happens if !rootfs.starts_with("/run"). This can lead to attempts to unmount a path that was not mounted by this function, resulting in unnecessary warnings or errors. The unmount operation should be wrapped in the same condition as the mount operation.

if !rootfs.starts_with("/run") { if let Err(e) = rustix::mount::unmount( target_root.join("boot").into_std_path_buf(), UnmountFlags::DETACH, ) { tracing::warn!("Error unmounting target/boot: {e}"); } }

crates/lib/src/bootloader.rs

gemini-code-assist · 2026-01-07T18:18:01Z

crates/lib/src/bootloader.rs

+            let dest = target_root
+                // joining an absolute path
+                // makes it replace self, so we strip the prefix
+                .join_os(src.strip_prefix("/").unwrap());


Using .unwrap() on the result of strip_prefix could cause a panic if a path without a leading / is ever added to bind_mount_dirs. While it's safe with the current hardcoded values, it would be more robust to handle the None case, for example by using expect() with a clear error message or by restructuring the code to avoid the possibility of a panic. This also applies to the similar usage on line 186.

gemini-code-assist · 2026-01-07T18:18:01Z

crates/lib/src/bootloader.rs

+            tracing::debug!("bind mounting {}", dest.display());
+            rustix::mount::mount_bind_recursive(src, dest)?;
+        }
+        // WIP : let's try to bind-mount /target/boot into the deployment as well rather than bind-mounting the whole thing??


This WIP comment should be removed before merging.

bootc-bot bot requested a review from jmarrero November 29, 2025 12:31

jbtrystram mentioned this pull request Nov 29, 2025

osbuild: use bootc install to deploy the container coreos/coreos-assembler#4224

Open

gemini-code-assist bot reviewed Nov 29, 2025

View reviewed changes

crates/lib/src/bootloader.rs Outdated Show resolved Hide resolved

jbtrystram force-pushed the install-chroot-bootupd branch from 2e335d8 to 3b92a48 Compare November 30, 2025 14:37

jbtrystram force-pushed the install-chroot-bootupd branch 2 times, most recently from 0b51f0e to 7d79124 Compare November 30, 2025 14:40

gemini-code-assist bot reviewed Nov 30, 2025

View reviewed changes

cgwalters reviewed Dec 1, 2025

View reviewed changes

cgwalters requested changes Dec 5, 2025

View reviewed changes

github-actions bot added area/install Issues related to `bootc install` area/ostree Issues related to ostree labels Dec 11, 2025

jbtrystram force-pushed the install-chroot-bootupd branch 2 times, most recently from 023a4c6 to d636cb1 Compare December 15, 2025 14:54

cgwalters requested changes Dec 16, 2025

View reviewed changes

jbtrystram force-pushed the install-chroot-bootupd branch from e225788 to 7a14a4b Compare December 17, 2025 21:14

jbtrystram force-pushed the install-chroot-bootupd branch from 77cd963 to 421e427 Compare January 7, 2026 12:45

jbtrystram added 3 commits January 7, 2026 13:45

DNM: add more debug output to tests

350e537

Signed-off-by: jbtrystram <[email protected]>

insert a reasonnable default PATH into the chroot

1a35bb5

Signed-off-by: jbtrystram <[email protected]>

jbtrystram force-pushed the install-chroot-bootupd branch 2 times, most recently from e6bc7e1 to f7891ca Compare January 7, 2026 13:28

test: just bind-mount target/boot in deploymnt/boot

9e2fbc4

jbtrystram force-pushed the install-chroot-bootupd branch from f7891ca to 9e2fbc4 Compare January 7, 2026 14:12

gemini-code-assist bot reviewed Jan 7, 2026

View reviewed changes

install/bootupd: chroot to deployment #1816

Are you sure you want to change the base?

install/bootupd: chroot to deployment #1816

Uh oh!

Conversation

jbtrystram commented Nov 29, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

jbtrystram commented Nov 30, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Nov 30, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Nov 30, 2025

Choose a reason for hiding this comment

Uh oh!

cgwalters Dec 1, 2025

Choose a reason for hiding this comment

Uh oh!

jbtrystram Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

cgwalters Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

cgwalters Dec 1, 2025

Choose a reason for hiding this comment

Uh oh!

cgwalters commented Dec 5, 2025

Uh oh!

cgwalters left a comment

Choose a reason for hiding this comment

Uh oh!

cgwalters left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cgwalters Dec 16, 2025

Choose a reason for hiding this comment

Uh oh!

jbtrystram Jan 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jbtrystram Jan 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cgwalters Dec 16, 2025

Choose a reason for hiding this comment

Uh oh!

cgwalters commented Dec 16, 2025

Uh oh!

jbtrystram commented Dec 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jbtrystram commented Dec 17, 2025

Uh oh!

jbtrystram commented Dec 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jbtrystram commented Jan 7, 2026

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Jan 7, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Jan 7, 2026

jbtrystram Jan 7, 2026 •

edited

Loading

jbtrystram Jan 7, 2026 •

edited

Loading

jbtrystram commented Dec 17, 2025 •

edited

Loading

jbtrystram commented Dec 17, 2025 •

edited

Loading