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.

crates/lib/src/bootloader.rs

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: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)

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.

crates/lib/src/bootloader.rs

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.

The install-outside-container and install-unified-flag tests were failing because they pulled centos-bootc:stream10 which has a newer bootupd with EFI files at /usr/lib/efi/, while the running Fedora image has the old layout at /usr/lib/bootupd/updates/EFI/. Use the booted image instead to ensure bootupd versions match. The original code is commented out for easy reversion once PR bootc-dev#1816 lands to properly handle cross-version installs. Assisted-by: OpenCode (Claude Sonnet 4) Signed-off-by: Colin Walters <[email protected]>

When `--src-imgref` is passed, the deployed systemd does not match the running environnement. In this case, let's run bootupd from inside the deployment. This makes sure we are using the binaries shipped in the image (and relevant config files such as grub fragements). We use bwrap to set up the chroot for a easier handling of the API filesystems. 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 Assisted-by: OpenCode (Opus 4.5) Signed-off-by: jbtrystram <[email protected]>

jbtrystram · 2026-01-28T14:48:15Z

Alright so I finally figured out a working solution ! For posterity i'll leave a small writeup :

I figured out why the CI was failing in my previous manual chroot + bind-mount attempts : the recursive bind-mount of /dev/ inside the chroot was messing up with the host's /dev/pts.
This lead the install to-exising-root tests to be unable to allocate a PTY to answer the prompts.

The chroot was technically working, but leaving the host in a broken state, requiring a reboot.
I tried to fix /dev/pts manually but then hit another issue with /dev/shm and realized I was probably re-inventing the wheel.

I tried systemd-nspawn to handle all the API filesystems better than me manually, which seemed to work just fine in my initial testing, until i figured out the base bootc images don't ship the systemd-container rpm.

I then tried podman run --rootfs but containers inside containers are weird and require a bunch more mounts in the initial podman invocation. I thought that would lead to a terrible UX.

bwrap was the easy answer in the end.

The bwrap code was added in the util crate in the hope that it get reused with the composefs backend to achieve the same effect for composefs + systemd boot. This however requires mounting the EROFS first in a temporary directory.

cgwalters · 2026-01-28T17:50:56Z

crates/utils/src/bwrap.rs

+#[derive(Debug, Default)]
+pub struct BwrapCmd<'a> {
+    /// The target directory to use as root for the container
+    chroot_path: &'a str,


Fine as is but I would like to add an option that takes a Dir instead.

Fixed in the last push

cgwalters · 2026-01-28T17:52:58Z

crates/lib/src/bootloader.rs

+        for partition in &device.partitions {
+            cmd = cmd.bind_device(&partition.node);
+        }
+        // // TODO : is it needed ?


In the general case - maybe, and especially when talking about cross-distro.

Especially when run outside of a container in the general case we could get all sorts of stuff leaking into our environment - like LD_LIBRARY_PATH or LD_PRELOAD and that could easily break because now the environment is different from the target root.

But in practice, we can just let it be inherited for now.

So this is a leftover from the manual chroot, but I noticed that bwrap does set this up for us so I commented it out.

You're right about other things that could leak in though

crates/utils/src/bwrap.rs

cgwalters · 2026-01-28T18:11:18Z

crates/utils/src/bwrap.rs

+        }
+
+        // Add device bind mounts
+        for device in self.devices {


What would actually probably work most reliably in general is for us to pass the block device as a file descriptor.

IIUC that would require bootupd to accept the device as a FD rather than a path. so we can't change that here for now, correct ?

Oh, or we can pass /proc/ns/fd/{rawFd} I guess ?

cgwalters · 2026-01-28T18:30:00Z

There was one failure in fedora-43/ostree which might have been related to this; I restarted it. Clearly one thing that would help again is for us to lock the blockdev and pass it down as a fd and not a path through the stack.

jbtrystram · 2026-01-28T20:01:34Z

There was one failure in fedora-43/ostree which might have been related to this; I restarted it. Clearly one thing that would help again is for us to lock the blockdev and pass it down as a fd and not a path through the stack.

I think that's because of the commented out $PATH. I'm testing locally to verify.

jbtrystram · 2026-01-28T20:56:47Z

There was one failure in fedora-43/ostree which might have been related to this; I restarted it. Clearly one thing that would help again is for us to lock the blockdev and pass it down as a fd and not a path through the stack.

I think that's because of the commented out $PATH. I'm testing locally to verify.

Confirmed. I'll push a fix that also address some of the review comments :)

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

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

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

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

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

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

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

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

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 4 times, most recently from f7891ca to 9e2fbc4 Compare January 7, 2026 14:12

gemini-code-assist bot reviewed Jan 7, 2026

View reviewed changes

jbtrystram force-pushed the install-chroot-bootupd branch 2 times, most recently from 0866667 to 93aaa72 Compare January 9, 2026 13:29

HuijingHei mentioned this pull request Jan 15, 2026

Add an argument boot-uuid to bootupctl install coreos/bootupd#1051

Open

cgwalters mentioned this pull request Jan 16, 2026

build-sys: Rework sealing to be one build step #1898

Merged

jbtrystram force-pushed the install-chroot-bootupd branch from 035ccd8 to 4344dc3 Compare January 19, 2026 16:32

This was referenced Jan 20, 2026

build-sys: Enable CentOS Stream compose repos to avoid version skew #1926

Merged

efi: transfer usr/lib/ostree-boot to usr/lib/efi coreos/bootupd#995

Merged

jbtrystram force-pushed the install-chroot-bootupd branch from 4344dc3 to 6f94015 Compare January 23, 2026 13:22

HuijingHei mentioned this pull request Jan 26, 2026

Support bootupctl backend generate-metadata --format=2 coreos/bootupd#1057

Open

jbtrystram force-pushed the install-chroot-bootupd branch from 6f94015 to 38d8381 Compare January 28, 2026 14:48

jbtrystram requested a review from cgwalters January 28, 2026 14:50

cgwalters previously approved these changes Jan 28, 2026

View reviewed changes

cgwalters enabled auto-merge (rebase) January 28, 2026 18:29

auto-merge was automatically disabled January 29, 2026 13:01
Head branch was pushed to by a user without write access

jbtrystram dismissed cgwalters’s stale review via 62964bb January 29, 2026 13:01

address review comments

caa758b

jbtrystram force-pushed the install-chroot-bootupd branch from 62964bb to caa758b Compare January 29, 2026 14:19

install/bootupd: chroot to deployment #1816

Are you sure you want to change the base?

install/bootupd: chroot to deployment #1816

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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gemini-code-assist bot Jan 7, 2026

Choose a reason for hiding this comment

Uh oh!

jbtrystram commented Jan 28, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jbtrystram Jan 28, 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 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

jbtrystram commented Dec 17, 2025 •

edited

Loading

jbtrystram commented Dec 17, 2025 •

edited

Loading

jbtrystram Jan 28, 2026 •

edited

Loading

jbtrystram Jan 29, 2026 •

edited

Loading