From 29095d73e6d43890317dfdb90784228d145933d1 Mon Sep 17 00:00:00 2001 From: Pragyan Poudyal Date: Tue, 26 May 2026 10:50:38 +0530 Subject: [PATCH 1/4] cfs/rollback: Remove staged entry on rollback Similar to ostree, if we find any staged deployment while performing a rollback, we'll get rid of the staged deployment. The staged deployment still exists on disk and will be GC'd later Fixes: #2208 Signed-off-by: Pragyan Poudyal --- crates/lib/src/bootc_composefs/rollback.rs | 35 +++++++++++++++++----- 1 file changed, 27 insertions(+), 8 deletions(-) diff --git a/crates/lib/src/bootc_composefs/rollback.rs b/crates/lib/src/bootc_composefs/rollback.rs index 106919f04..2758c9bec 100644 --- a/crates/lib/src/bootc_composefs/rollback.rs +++ b/crates/lib/src/bootc_composefs/rollback.rs @@ -4,6 +4,7 @@ use anyhow::{Context, Result, anyhow}; use cap_std_ext::cap_std::fs::Dir; use cap_std_ext::dirext::CapStdExtDirExt; use fn_error_context::context; +use ocidir::cap_std::ambient_authority; use rustix::fs::{AtFlags, RenameFlags, fsync, renameat_with}; use crate::bootc_composefs::boot::{ @@ -11,8 +12,10 @@ use crate::bootc_composefs::boot::{ secondary_sort_key, type1_entry_conf_file_name, }; use crate::bootc_composefs::status::{get_composefs_status, get_sorted_type1_boot_entries}; -use crate::composefs_consts::TYPE1_ENT_PATH_STAGED; -use crate::spec::Bootloader; +use crate::composefs_consts::{ + COMPOSEFS_STAGED_DEPLOYMENT_FNAME, COMPOSEFS_TRANSIENT_STATE_DIR, TYPE1_ENT_PATH_STAGED, +}; +use crate::spec::{Bootloader, Host}; use crate::store::{BootedComposefs, Storage}; use crate::{ bootc_composefs::{boot::get_efi_uuid_source, status::get_sorted_grub_uki_boot_entries}, @@ -124,7 +127,7 @@ fn rollback_grub_uki_entries(boot_dir: &Dir) -> Result<()> { /// a. Here we assume that rollback is queued as there's no way to differentiate between this /// case and Case 1-b. This is what ostree does as well #[context("Rolling back {bootloader} entries")] -fn rollback_composefs_entries(boot_dir: &Dir, bootloader: Bootloader) -> Result<()> { +fn rollback_composefs_entries(host: &Host, boot_dir: &Dir, bootloader: Bootloader) -> Result<()> { // Get all boot entries sorted in descending order by sort-key let mut all_configs = get_sorted_type1_boot_entries(&boot_dir, false)?; @@ -143,6 +146,24 @@ fn rollback_composefs_entries(boot_dir: &Dir, bootloader: Bootloader) -> Result< // OR if rollback was queued, it would become secondary all_configs[1].sort_key = Some(secondary_sort_key(os_id)); + // Ostree will drop any staged deployment on rollback + // We follow the same approach for now + if let Some(..) = &host.status.staged { + println!("Removing currently staged deployment"); + + boot_dir + .remove_dir_all(TYPE1_ENT_PATH_STAGED) + .context("Removing staged entries")?; + + let transient_dir = + Dir::open_ambient_dir(COMPOSEFS_TRANSIENT_STATE_DIR, ambient_authority()) + .context("Opening transient dir")?; + + transient_dir + .remove_file(COMPOSEFS_STAGED_DEPLOYMENT_FNAME) + .context("Removing staged deployment file")?; + } + // Write these boot_dir .create_dir_all(TYPE1_ENT_PATH_STAGED) @@ -213,11 +234,9 @@ pub(crate) async fn composefs_rollback( let rollback_status = host .status .rollback + .as_ref() .ok_or_else(|| anyhow!("No rollback available"))?; - // TODO: Handle staged deployment - // Ostree will drop any staged deployment on rollback but will keep it if it is the first item - // in the new deployment list let Some(rollback_entry) = &rollback_status.composefs else { anyhow::bail!("Rollback deployment not a composefs deployment") }; @@ -227,7 +246,7 @@ pub(crate) async fn composefs_rollback( match &rollback_entry.bootloader { Bootloader::Grub => match rollback_entry.boot_type { BootType::Bls => { - rollback_composefs_entries(boot_dir, rollback_entry.bootloader.clone())?; + rollback_composefs_entries(&host, boot_dir, rollback_entry.bootloader.clone())?; } BootType::Uki => { rollback_grub_uki_entries(boot_dir)?; @@ -236,7 +255,7 @@ pub(crate) async fn composefs_rollback( Bootloader::Systemd => { // We use BLS entries for systemd UKI as well - rollback_composefs_entries(boot_dir, rollback_entry.bootloader.clone())?; + rollback_composefs_entries(&host, boot_dir, rollback_entry.bootloader.clone())?; } Bootloader::None => unreachable!("Checked at install time"), From 57d0036a410afc3007b28e53461612fdacaa67fd Mon Sep 17 00:00:00 2001 From: Pragyan Poudyal Date: Tue, 26 May 2026 14:42:30 +0530 Subject: [PATCH 2/4] tests: Update rollback test Add a case in the rollback test which tests rollback when there is a staged deployment present. This is a test for #2208 Signed-off-by: Pragyan Poudyal --- tmt/tests/booted/test-rollback.nu | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/tmt/tests/booted/test-rollback.nu b/tmt/tests/booted/test-rollback.nu index 0afe377ac..fae0ab0e6 100644 --- a/tmt/tests/booted/test-rollback.nu +++ b/tmt/tests/booted/test-rollback.nu @@ -103,6 +103,31 @@ def third_boot_verify [] { def fourth_boot_verify [] { back_to_first_depl Fourth + + # Stage a new deployment, then rollback -> staged deployment should be removed + let dockerfile = $" + FROM localhost/bootc as base + RUN echo 'Second Stage' > /usr/share/second-stage + " + + (tap make_uki_containerfile $dockerfile) | podman build -t localhost/second-stage . -f - + + bootc switch --transport containers-storage localhost/second-stage + + assert ( + (bootc status --json | from json).status + | get staged + | is-not-empty + ) + + bootc rollback + + assert ( + (bootc status --json | from json).status + | get staged + | is-empty + ) + tap ok } From 04d24eeacfc52ecc6afd677237ecbb26f2344d54 Mon Sep 17 00:00:00 2001 From: Pragyan Poudyal Date: Tue, 26 May 2026 15:29:56 +0530 Subject: [PATCH 3/4] cfs/status: Implement bootloader-specific sorting Update `get_sorted_type1_boot_entries_helper` to implement sorting logic based on bootloader type - systemd-boot: Sort by sort-key (using BLSConfig::cmp which handles sort-key ascending, then version descending) - GRUB: Sort by filename in descending order (ignoring sort-key fields) Unit Tests generated by ClaudeCode (Opus) Signed-off-by: Pragyan Poudyal --- crates/lib/src/bootc_composefs/status.rs | 226 ++++++++++++++++++++++- 1 file changed, 219 insertions(+), 7 deletions(-) diff --git a/crates/lib/src/bootc_composefs/status.rs b/crates/lib/src/bootc_composefs/status.rs index bd8b7a19c..f4b279dcc 100644 --- a/crates/lib/src/bootc_composefs/status.rs +++ b/crates/lib/src/bootc_composefs/status.rs @@ -243,18 +243,26 @@ fn get_sorted_grub_uki_boot_entries_helper<'a>( parse_grub_menuentry_file(str) } +/// Get sorted boot entries +/// The sort here is done in terms of what will be shown on the boot menu +/// For systemd-boot, the entries are sorted by `sort-key` +/// For grub, the entries are sorted by the filename in descending order pub(crate) fn get_sorted_type1_boot_entries( boot_dir: &Dir, ascending: bool, ) -> Result> { - get_sorted_type1_boot_entries_helper(boot_dir, ascending, false) + let bootloader = get_bootloader()?; + get_sorted_type1_boot_entries_helper(boot_dir, ascending, false, bootloader) } +/// Same as [`get_sorted_type1_boot_entries`], but returns staged entries +/// See [`get_sorted_type1_boot_entries`] for more details pub(crate) fn get_sorted_staged_type1_boot_entries( boot_dir: &Dir, ascending: bool, ) -> Result> { - get_sorted_type1_boot_entries_helper(boot_dir, ascending, true) + let bootloader = get_bootloader()?; + get_sorted_type1_boot_entries_helper(boot_dir, ascending, true, bootloader) } #[context("Getting sorted Type1 boot entries")] @@ -262,15 +270,20 @@ fn get_sorted_type1_boot_entries_helper( boot_dir: &Dir, ascending: bool, get_staged_entries: bool, + bootloader: crate::spec::Bootloader, ) -> Result> { - let mut all_configs = vec![]; + #[derive(Debug)] + struct ConfigWithFilename { + config: BLSConfig, + filename: String, + } let dir = match get_staged_entries { true => { let dir = boot_dir.open_dir_optional(TYPE1_ENT_PATH_STAGED)?; let Some(dir) = dir else { - return Ok(all_configs); + return Ok(vec![]); }; dir.read_dir(".")? @@ -279,6 +292,8 @@ fn get_sorted_type1_boot_entries_helper( false => boot_dir.read_dir(TYPE1_ENT_PATH)?, }; + let mut configs_with_filenames = vec![]; + for entry in dir { let entry = entry?; @@ -302,12 +317,31 @@ fn get_sorted_type1_boot_entries_helper( let config = parse_bls_config(&contents).context("Parsing bls config")?; - all_configs.push(config); + configs_with_filenames.push(ConfigWithFilename { + config, + filename: file_name.to_string(), + }); } - all_configs.sort_by(|a, b| if ascending { a.cmp(b) } else { b.cmp(a) }); + // Sort based on bootloader type + configs_with_filenames.sort_by(|a, b| { + let ord = match bootloader { + // For systemd-boot sort by sort-key + Bootloader::Systemd => a.config.cmp(&b.config), + // For grub, sort by filename in descending order + Bootloader::Grub => b.filename.cmp(&a.filename), + Bootloader::None => { + unreachable!("Bootloader checked during installation should not have been none") + } + }; - Ok(all_configs) + if ascending { ord } else { ord.reverse() } + }); + + Ok(configs_with_filenames + .into_iter() + .map(|c| c.config) + .collect()) } pub(crate) fn list_type1_entries(boot_dir: &Dir) -> Result> { @@ -979,6 +1013,10 @@ async fn composefs_deployment_status_from( mod tests { use cap_std_ext::{cap_std, dirext::CapStdExtDirExt}; + use crate::bootc_composefs::boot::{ + FILENAME_PRIORITY_PRIMARY, FILENAME_PRIORITY_SECONDARY, primary_sort_key, + secondary_sort_key, type1_entry_conf_file_name, + }; use crate::parsers::{bls_config::BLSConfigType, grub_menuconfig::MenuentryBody}; use super::*; @@ -1217,4 +1255,178 @@ mod tests { Ok(()) } + + #[test] + fn test_get_sorted_type1_boot_entries_helper_systemd() -> Result<()> { + let tempdir = cap_std_ext::cap_tempfile::tempdir(cap_std::ambient_authority())?; + + // Create entries with different sort-keys for systemd-boot testing + let entry1 = format!( + r#" + title Fedora Linux (1.0.0) + version 1.0.0 + sort-key {} + linux /boot/vmlinuz + initrd /boot/initramfs.img + "#, + secondary_sort_key("fedora") + ); + + let entry2 = format!( + r#" + title Fedora Linux (2.0.0) + version 2.0.0 + sort-key {} + linux /boot/vmlinuz + initrd /boot/initramfs.img + "#, + primary_sort_key("fedora") + ); + + let entry3 = format!( + r#" + title Fedora Linux (1.5.0) + version 1.5.0 + sort-key {} + linux /boot/vmlinuz + initrd /boot/initramfs.img + "#, + primary_sort_key("fedora") + ); + + tempdir.create_dir_all("loader/entries")?; + + // Use realistic filenames as used in production + let filename1 = type1_entry_conf_file_name("fedora", "1.0.0", FILENAME_PRIORITY_SECONDARY); + let filename2 = type1_entry_conf_file_name("fedora", "2.0.0", FILENAME_PRIORITY_PRIMARY); + let filename3 = type1_entry_conf_file_name("fedora", "1.5.0", FILENAME_PRIORITY_PRIMARY); + + tempdir.atomic_write(format!("loader/entries/{}", filename1), entry1)?; + tempdir.atomic_write(format!("loader/entries/{}", filename2), entry2)?; + tempdir.atomic_write(format!("loader/entries/{}", filename3), entry3)?; + + // Test systemd-boot sorting (by sort-key, then by version in descending order) + let result = get_sorted_type1_boot_entries_helper( + &tempdir, + true, + false, + crate::spec::Bootloader::Systemd, + )?; + + assert_eq!(result.len(), 3); + // With ascending=true, primary sort-key (bootc-fedora-0) should come before secondary (bootc-fedora-1) + // Within primary sort-key, version 2.0.0 should come before 1.5.0 (descending version order) + assert_eq!( + result[0].sort_key.as_ref().unwrap(), + &primary_sort_key("fedora") + ); + assert_eq!(result[0].version(), "2.0.0".into()); // Entry 2 (version 2.0.0) + assert_eq!( + result[1].sort_key.as_ref().unwrap(), + &primary_sort_key("fedora") + ); + assert_eq!(result[1].version(), "1.5.0".into()); // Entry 3 (version 1.5.0) + assert_eq!( + result[2].sort_key.as_ref().unwrap(), + &secondary_sort_key("fedora") + ); + assert_eq!(result[2].version(), "1.0.0".into()); // Entry 1 (version 1.0.0) + + // Test descending order + let result = get_sorted_type1_boot_entries_helper( + &tempdir, + false, + false, + crate::spec::Bootloader::Systemd, + )?; + + assert_eq!(result.len(), 3); + // With ascending=false, secondary sort-key should come before primary + assert_eq!( + result[0].sort_key.as_ref().unwrap(), + &secondary_sort_key("fedora") + ); + assert_eq!(result[0].version(), "1.0.0".into()); // Entry 1 + assert_eq!( + result[1].sort_key.as_ref().unwrap(), + &primary_sort_key("fedora") + ); + assert_eq!(result[1].version(), "1.5.0".into()); // Entry 3 + assert_eq!( + result[2].sort_key.as_ref().unwrap(), + &primary_sort_key("fedora") + ); + assert_eq!(result[2].version(), "2.0.0".into()); // Entry 2 + + Ok(()) + } + + #[test] + fn test_get_sorted_type1_boot_entries_helper_grub() -> Result<()> { + let tempdir = cap_std_ext::cap_tempfile::tempdir(cap_std::ambient_authority())?; + + // Create entries with sort-keys but GRUB ignores them and sorts by filename + let entry1 = format!( + r#" + title Fedora Linux (41.20251125.0) + version 41.20251125.0 + sort-key {} + linux /boot/vmlinuz + initrd /boot/initramfs.img + "#, + secondary_sort_key("fedora") + ); + + let entry2 = format!( + r#" + title Fedora Linux (42.20251125.0) + version 42.20251125.0 + sort-key {} + linux /boot/vmlinuz + initrd /boot/initramfs.img + "#, + primary_sort_key("fedora") + ); + + tempdir.create_dir_all("loader/entries")?; + + // Use realistic filenames - GRUB will sort by these, not sort-key + let filename1 = + type1_entry_conf_file_name("fedora", "41.20251125.0", FILENAME_PRIORITY_SECONDARY); + let filename2 = + type1_entry_conf_file_name("fedora", "42.20251125.0", FILENAME_PRIORITY_PRIMARY); + + tempdir.atomic_write(format!("loader/entries/{}", filename1), entry1)?; + tempdir.atomic_write(format!("loader/entries/{}", filename2), entry2)?; + + let result = get_sorted_type1_boot_entries_helper( + &tempdir, + true, + false, + crate::spec::Bootloader::Grub, + )?; + + assert_eq!(result.len(), 2); + // With ascending=true for GRUB, we reverse the default descending filename order + // Filenames: bootc_fedora-41.20251125.0-0.conf, bootc_fedora-42.20251125.0-1.conf + // Ascending filename order should be: 42-1, 41-0 + assert_eq!(result[0].version(), "42.20251125.0".into()); + assert_eq!(result[1].version(), "41.20251125.0".into()); + + // Test descending order (GRUB's default filename sorting) + let result = get_sorted_type1_boot_entries_helper( + &tempdir, + false, + false, + crate::spec::Bootloader::Grub, + )?; + + assert_eq!(result.len(), 2); + // With ascending=false for GRUB, filenames should be sorted in descending order + // Descending filename order should be: 42-1, 41-0 + assert_eq!(result[0].version(), "41.20251125.0".into()); + assert_eq!(result[1].version(), "42.20251125.0".into()); + + Ok(()) + } } From e07ffbd6c7c97c3b8511efe26fa0cf673a98df9a Mon Sep 17 00:00:00 2001 From: Pragyan Poudyal Date: Tue, 26 May 2026 15:49:06 +0530 Subject: [PATCH 4/4] cfs/rollback: Update the way we get rollback Instead of blindly selecting the "second" one in a list of sorted boot entries as the rollback and failing if there are more than one rollback candidate, sort the rollback candidates in the same order as the boot entries and take the first one as rollback. All the remaining deployments become `other_deployments`. This is especially useful if and when we implement pinned deployments for composefs Signed-off-by: Pragyan Poudyal --- crates/lib/src/bootc_composefs/status.rs | 29 +++++++++++++++++------- 1 file changed, 21 insertions(+), 8 deletions(-) diff --git a/crates/lib/src/bootc_composefs/status.rs b/crates/lib/src/bootc_composefs/status.rs index f4b279dcc..c05a6add0 100644 --- a/crates/lib/src/bootc_composefs/status.rs +++ b/crates/lib/src/bootc_composefs/status.rs @@ -1,4 +1,4 @@ -use std::{collections::HashSet, io::Read, sync::OnceLock}; +use std::{io::Read, sync::OnceLock}; use anyhow::{Context, Result}; use bootc_kernel_cmdline::utf8::Cmdline; @@ -831,7 +831,6 @@ async fn composefs_deployment_status_from( Err(e) => Err(e), }?; - // NOTE: This cannot work if we support both BLS and UKI at the same time let mut boot_type: Option = None; // Boot entries from deployments that are neither booted nor staged deployments @@ -966,6 +965,8 @@ async fn composefs_deployment_status_from( // Determine rollback deployment by matching extra deployment boot entries against entires read from /boot // This collects verity digest across bls and grub enties, we should just have one of them, but still works + // + // We want this ordered, so we have a vector here let bootloader_configured_verity = sorted_bls_config .iter() .flatten() @@ -976,9 +977,9 @@ async fn composefs_deployment_status_from( .flatten() .map(|menu| menu.get_verity()), ) - .collect::>>()?; + .collect::>>()?; - let rollback_candidates: Vec<_> = extra_deployment_boot_entries + let mut rollback_candidates: Vec<_> = extra_deployment_boot_entries .into_iter() .filter(|entry| { let verity = &entry @@ -990,10 +991,22 @@ async fn composefs_deployment_status_from( }) .collect(); - if rollback_candidates.len() > 1 { - anyhow::bail!("Multiple extra entries in /boot, could not determine rollback entry"); - } else if let Some(rollback_entry) = rollback_candidates.into_iter().next() { - host.status.rollback = Some(rollback_entry); + // We get sorted bootloader entries, so here we re-sort the rollback candidates + // wrt their positions in the sorted bootloader entries as that's what determines + // what's shown on the bootloader menu. The very next boot entry, that's not the + // default should be the rollback + rollback_candidates.sort_by_key(|ent| { + bootloader_configured_verity + .iter() + // SAFETY: ent.composefs will definitely exist + .position(|v| ent.composefs.as_ref().unwrap().verity == *v) + }); + + if !rollback_candidates.is_empty() { + let mut iter = rollback_candidates.into_iter(); + + host.status.rollback = iter.next(); + host.status.other_deployments = iter.collect(); } host.status.rollback_queued = is_rollback_queued;