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"), diff --git a/crates/lib/src/bootc_composefs/status.rs b/crates/lib/src/bootc_composefs/status.rs index bd8b7a19c..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; @@ -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> { @@ -797,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 @@ -932,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() @@ -942,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 @@ -956,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; @@ -979,6 +1026,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 +1268,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(()) + } } 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 }