install/bootupd: chroot to deployment

[jbtrystram]

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.

[jbtrystram]

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

1. A high-severity issue where mounts can be leaked if an error occurs during the setup of the bind mounts.
2. 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]

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]

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]

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.

[cgwalters]

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