From 09d89cfe94b3e0fe5120d7b42521f3a4dd0715f6 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Tue, 18 Nov 2025 21:09:39 -0500 Subject: [PATCH] install: Detect bootloader from target image instead of host Fixes a regression where bootupd detection was happening before the container was deployed, causing bootc to incorrectly check the host system instead of the target container image. This led to false negatives when the container had bootupd but the host didn't. The fix moves bootloader detection into a new PostFetchState that's created after the deployment is available, ensuring we check the actual target filesystem. Fixes: #1778 Assisted-by: Claude Code (Sonnet 4.5) Signed-off-by: Colin Walters --- crates/lib/src/bootc_composefs/boot.rs | 44 ++++++++++------ crates/lib/src/bootloader.rs | 8 ++- crates/lib/src/install.rs | 52 +++++++++++++------ .../booted/test-install-outside-container.nu | 26 +++++++++- 4 files changed, 91 insertions(+), 39 deletions(-) diff --git a/crates/lib/src/bootc_composefs/boot.rs b/crates/lib/src/bootc_composefs/boot.rs index 08c19d0ee..5cc475b75 100644 --- a/crates/lib/src/bootc_composefs/boot.rs +++ b/crates/lib/src/bootc_composefs/boot.rs @@ -33,7 +33,6 @@ use rustix::{mount::MountFlags, path::Arg}; use schemars::JsonSchema; use serde::{Deserialize, Serialize}; -use crate::bootc_composefs::status::get_sorted_grub_uki_boot_entries; use crate::composefs_consts::{TYPE1_ENT_PATH, TYPE1_ENT_PATH_STAGED}; use crate::parsers::bls_config::{BLSConfig, BLSConfigType}; use crate::parsers::grub_menuconfig::MenuEntry; @@ -46,6 +45,7 @@ use crate::{ bootc_composefs::state::{get_booted_bls, write_composefs_state}, bootloader::esp_in, }; +use crate::{bootc_composefs::status::get_sorted_grub_uki_boot_entries, install::PostFetchState}; use crate::{ composefs_consts::{ BOOT_LOADER_ENTRIES, COMPOSEFS_CMDLINE, ORIGIN_KEY_BOOT, ORIGIN_KEY_BOOT_DIGEST, @@ -77,7 +77,14 @@ pub(crate) const SYSTEMD_UKI_DIR: &str = "EFI/Linux/bootc"; pub(crate) enum BootSetupType<'a> { /// For initial setup, i.e. install to-disk - Setup((&'a RootSetup, &'a State, &'a ComposefsFilesystem)), + Setup( + ( + &'a RootSetup, + &'a State, + &'a PostFetchState, + &'a ComposefsFilesystem, + ), + ), /// For `bootc upgrade` Upgrade((&'a Storage, &'a ComposefsFilesystem, &'a Host)), } @@ -378,7 +385,7 @@ pub(crate) fn setup_composefs_bls_boot( let id_hex = id.to_hex(); let (root_path, esp_device, cmdline_refs, fs, bootloader) = match setup_type { - BootSetupType::Setup((root_setup, state, fs)) => { + BootSetupType::Setup((root_setup, state, postfetch, fs)) => { // root_setup.kargs has [root=UUID=, "rw"] let mut cmdline_options = Cmdline::new(); @@ -400,7 +407,7 @@ pub(crate) fn setup_composefs_bls_boot( esp_part.node.clone(), cmdline_options, fs, - state.detected_bootloader.clone(), + postfetch.detected_bootloader.clone(), ) } @@ -854,7 +861,7 @@ pub(crate) fn setup_composefs_uki_boot( entries: Vec>, ) -> Result<()> { let (root_path, esp_device, bootloader, is_insecure_from_opts, uki_addons) = match setup_type { - BootSetupType::Setup((root_setup, state, ..)) => { + BootSetupType::Setup((root_setup, state, postfetch, ..)) => { state.require_no_kargs_for_uki()?; let esp_part = esp_in(&root_setup.device_info)?; @@ -862,7 +869,7 @@ pub(crate) fn setup_composefs_uki_boot( ( root_setup.physical_root_path.clone(), esp_part.node.clone(), - state.detected_bootloader.clone(), + postfetch.detected_bootloader.clone(), state.composefs_options.insecure, state.composefs_options.uki_addon.as_ref(), ) @@ -964,6 +971,18 @@ pub(crate) fn setup_composefs_boot( state: &State, image_id: &str, ) -> Result<()> { + let repo = open_composefs_repo(&root_setup.physical_root)?; + let mut fs = create_composefs_filesystem(&repo, image_id, None)?; + let entries = fs.transform_for_boot(&repo)?; + let id = fs.commit_image(&repo, None)?; + let mounted_fs = Dir::reopen_dir( + &repo + .mount(&id.to_hex()) + .context("Failed to mount composefs image")?, + )?; + + let postfetch = PostFetchState::new(state, &mounted_fs)?; + let boot_uuid = root_setup .get_boot_uuid()? .or(root_setup.rootfs_uuid.as_deref()) @@ -972,7 +991,7 @@ pub(crate) fn setup_composefs_boot( if cfg!(target_arch = "s390x") { // TODO: Integrate s390x support into install_via_bootupd crate::bootloader::install_via_zipl(&root_setup.device_info, boot_uuid)?; - } else if state.detected_bootloader == Bootloader::Grub { + } else if postfetch.detected_bootloader == Bootloader::Grub { crate::bootloader::install_via_bootupd( &root_setup.device_info, &root_setup.physical_root_path, @@ -988,13 +1007,6 @@ pub(crate) fn setup_composefs_boot( )?; } - let repo = open_composefs_repo(&root_setup.physical_root)?; - - let mut fs = create_composefs_filesystem(&repo, image_id, None)?; - - let entries = fs.transform_for_boot(&repo)?; - let id = fs.commit_image(&repo, None)?; - let Some(entry) = entries.iter().next() else { anyhow::bail!("No boot entries!"); }; @@ -1005,7 +1017,7 @@ pub(crate) fn setup_composefs_boot( match boot_type { BootType::Bls => { let digest = setup_composefs_bls_boot( - BootSetupType::Setup((&root_setup, &state, &fs)), + BootSetupType::Setup((&root_setup, &state, &postfetch, &fs)), repo, &id, entry, @@ -1014,7 +1026,7 @@ pub(crate) fn setup_composefs_boot( boot_digest = Some(digest); } BootType::Uki => setup_composefs_uki_boot( - BootSetupType::Setup((&root_setup, &state, &fs)), + BootSetupType::Setup((&root_setup, &state, &postfetch, &fs)), repo, &id, entries, diff --git a/crates/lib/src/bootloader.rs b/crates/lib/src/bootloader.rs index fb9a81521..6126cef9e 100644 --- a/crates/lib/src/bootloader.rs +++ b/crates/lib/src/bootloader.rs @@ -3,6 +3,7 @@ use std::process::Command; use anyhow::{anyhow, bail, Context, Result}; use bootc_utils::CommandRunExt; use camino::Utf8Path; +use cap_std_ext::cap_std::fs::Dir; use fn_error_context::context; use bootc_blockdev::{Partition, PartitionTable}; @@ -28,15 +29,12 @@ pub(crate) fn esp_in(device: &PartitionTable) -> Result<&Partition> { /// Determine if the invoking environment contains bootupd, and if there are bootupd-based /// updates in the target root. #[context("Querying for bootupd")] -#[allow(dead_code)] -pub(crate) fn supports_bootupd(deployment_path: Option<&str>) -> Result { +pub(crate) fn supports_bootupd(root: &Dir) -> Result { if !utils::have_executable("bootupctl")? { tracing::trace!("No bootupctl binary found"); return Ok(false); }; - let deployment_path = Utf8Path::new(deployment_path.unwrap_or("/")); - let updates = deployment_path.join(BOOTUPD_UPDATES); - let r = updates.try_exists()?; + let r = root.try_exists(BOOTUPD_UPDATES)?; tracing::trace!("bootupd updates: {r}"); Ok(r) } diff --git a/crates/lib/src/install.rs b/crates/lib/src/install.rs index 622b8d5e3..12e65c599 100644 --- a/crates/lib/src/install.rs +++ b/crates/lib/src/install.rs @@ -476,7 +476,11 @@ pub(crate) struct State { // If Some, then --composefs_native is passed pub(crate) composefs_options: InstallComposefsOpts, +} +// Shared read-only global state +#[derive(Debug)] +pub(crate) struct PostFetchState { /// Detected bootloader type for the target system pub(crate) detected_bootloader: crate::spec::Bootloader, } @@ -1453,21 +1457,6 @@ async fn prepare_install( .map(|p| std::fs::read_to_string(p).with_context(|| format!("Reading {p}"))) .transpose()?; - // Determine bootloader type for the target system - // Priority: user-specified > bootupd availability > systemd-boot fallback - let detected_bootloader = { - if let Some(bootloader) = composefs_options.bootloader.clone() { - bootloader - } else { - if crate::bootloader::supports_bootupd(None)? { - crate::spec::Bootloader::Grub - } else { - crate::spec::Bootloader::Systemd - } - } - }; - println!("Bootloader: {detected_bootloader}"); - // Create our global (read-only) state which gets wrapped in an Arc // so we can pass it to worker threads too. Right now this just // combines our command line options along with some bind mounts from the host. @@ -1483,13 +1472,35 @@ async fn prepare_install( tempdir, host_is_container, composefs_required, - detected_bootloader, composefs_options, }); Ok(state) } +impl PostFetchState { + pub(crate) fn new(state: &State, d: &Dir) -> Result { + // Determine bootloader type for the target system + // Priority: user-specified > bootupd availability > systemd-boot fallback + let detected_bootloader = { + if let Some(bootloader) = state.composefs_options.bootloader.clone() { + bootloader + } else { + if crate::bootloader::supports_bootupd(d)? { + crate::spec::Bootloader::Grub + } else { + crate::spec::Bootloader::Systemd + } + } + }; + println!("Bootloader: {detected_bootloader}"); + let r = Self { + detected_bootloader, + }; + Ok(r) + } +} + /// Given a baseline root filesystem with an ostree sysroot initialized: /// - install the container to that root /// - install the bootloader @@ -1513,11 +1524,17 @@ async fn install_with_sysroot( let deployment_path = ostree.deployment_dirpath(&deployment); + let deployment_dir = rootfs + .physical_root + .open_dir(&deployment_path) + .context("Opening deployment dir")?; + let postfetch = PostFetchState::new(state, &deployment_dir)?; + if cfg!(target_arch = "s390x") { // TODO: Integrate s390x support into install_via_bootupd crate::bootloader::install_via_zipl(&rootfs.device_info, boot_uuid)?; } else { - match state.detected_bootloader { + match postfetch.detected_bootloader { Bootloader::Grub => { crate::bootloader::install_via_bootupd( &rootfs.device_info, @@ -1660,6 +1677,7 @@ async fn install_to_filesystem_impl( let (id, verity) = initialize_composefs_repository(state, rootfs).await?; tracing::info!("id: {}, verity: {}", hex::encode(id), verity.to_hex()); + setup_composefs_boot(rootfs, state, &hex::encode(id))?; } else { ostree_install(state, rootfs, cleanup).await?; diff --git a/tmt/tests/booted/test-install-outside-container.nu b/tmt/tests/booted/test-install-outside-container.nu index 5f07ff977..5341e8c42 100644 --- a/tmt/tests/booted/test-install-outside-container.nu +++ b/tmt/tests/booted/test-install-outside-container.nu @@ -1,14 +1,38 @@ use std assert use tap.nu +# In this test we install a generic image mainly because it keeps +# this test in theory independent of starting from a bootc host, +# but also because it's useful to test "skew" between the bootc binary +# doing the install and the target image. +let target_image = "docker://quay.io/centos-bootc/centos-bootc:stream10" + # setup filesystem mkdir /var/mnt -truncate -s 100M disk.img +truncate -s 10G disk.img mkfs.ext4 disk.img mount -o loop disk.img /var/mnt # attempt to install to filesystem without specifying a source-imgref let result = bootc install to-filesystem /var/mnt e>| find "--source-imgref must be defined" assert not equal $result null +umount /var/mnt + +# Mask off the bootupd state to reproduce https://github.com/bootc-dev/bootc/issues/1778 +# Also it turns out that installation outside of containers dies due to `error: Multiple commit objects found` +# so we mask off /sysroot/ostree +# And using systemd-run here breaks our install_t so we disable SELinux enforcement +setenforce 0 +systemd-run -p MountFlags=slave -qdPG -- /bin/sh -c $" +set -xeuo pipefail +if test -d /sysroot/ostree; then mount --bind /usr/share/empty /sysroot/ostree; fi +mkdir -p /tmp/ovl/{upper,work} +mount -t overlay -olowerdir=/usr,workdir=/tmp/ovl/work,upperdir=/tmp/ovl/upper overlay /usr +# Note we do keep the other bootupd state +rm -vrf /usr/lib/bootupd/updates +# Another bootc install bug, we should not look at this in outside-of-container flows +rm -vrf /usr/lib/bootc/bound-images.d +bootc install to-disk --disable-selinux --via-loopback --filesystem xfs --source-imgref ($target_image) ./disk.img +" tap ok