From fccb8fbcf9886355ba132d1e2ab3b74627c57876 Mon Sep 17 00:00:00 2001 From: Pragyan Poudyal Date: Mon, 6 Jul 2026 13:39:53 +0530 Subject: [PATCH 1/3] etc-merge: Handle unmergable paths properly We would have unmergable paths if a modified file was changed to a directory in the new etc or vice-versa. One of the major issues was that we were erroring out during "merge" which is not ideal. A new vector now keeps track of all unmergable paths and the reason they're not mergable. Ref: composefs/composefs-rs/issues/335 Signed-off-by: Pragyan Poudyal --- crates/etc-merge/src/lib.rs | 164 +++++++++++++++++++++++++++++------- 1 file changed, 134 insertions(+), 30 deletions(-) diff --git a/crates/etc-merge/src/lib.rs b/crates/etc-merge/src/lib.rs index 1ea0d1f72..6862ca5cd 100644 --- a/crates/etc-merge/src/lib.rs +++ b/crates/etc-merge/src/lib.rs @@ -78,6 +78,12 @@ fn stat_eq_ignore_mtime(this: &Stat, other: &Stat) -> bool { return true; } +#[derive(Debug)] +pub struct UnmergablePaths { + path: PathBuf, + reason: String, +} + /// Represents the differences between two directory trees. #[derive(Debug)] pub struct Diff { @@ -88,6 +94,8 @@ pub struct Diff { modified: Vec, /// Paths that exist in the pristine /etc but not in the current one removed: Vec, + /// Paths that are unmergable + unmergable_paths: Vec, } fn collect_all_files( @@ -170,6 +178,47 @@ fn get_deletions( Ok(()) } +fn check_if_mergable( + new: &Directory, + current_inode: &Inode, + current_path: &PathBuf, + diff: &mut Diff, +) { + match current_inode { + // If currently 'file' is a directory, make sure it's not a regular file + // new_etc as well, else we can't merge + Inode::Directory(..) => { + let new_dir = new.get_directory(¤t_path.as_os_str()); + + if matches!(new_dir, Err(ImageError::NotADirectory(..))) { + diff.unmergable_paths.push(UnmergablePaths { + path: current_path.clone(), + reason: format!( + "Directory '{}' now defaults to a file in new etc", + current_path.display() + ), + }); + } + } + + // If currently 'file' is not a directory, make sure it's not a directory in the + // new_etc either, else we can't merge + Inode::Leaf(..) => { + let new_dir = new.get_directory(¤t_path.as_os_str()); + + if matches!(new_dir, Ok(..)) { + diff.unmergable_paths.push(UnmergablePaths { + path: current_path.clone(), + reason: format!( + "File '{}' now defaults to a directory in new etc", + current_path.display() + ), + }); + } + } + } +} + // 1. Files in the currently booted deployment’s /etc which were modified from the default /usr/etc (of the same deployment) are retained. // // 2. Files in the currently booted deployment’s /etc which were not modified from the default /usr/etc (of the same deployment) @@ -190,6 +239,7 @@ fn get_modifications( current: &Directory, pristine_leaves: &[Leaf], current_leaves: &[Leaf], + new_leaves: &[Leaf], new: &Directory, mut current_path: PathBuf, diff: &mut Diff, @@ -216,26 +266,45 @@ fn get_modifications( &curr_dir, pristine_leaves, current_leaves, + new_leaves, new, current_path.clone(), diff, )?; - // This directory or its contents were modified/added - // Check if the new directory was deleted from new_etc - // If it was, we want to add the directory back - if new.get_directory_opt(¤t_path.as_os_str())?.is_none() { - if diff.added.len() != total_added { - diff.added.insert(total_added, current_path.clone()); - } else if diff.modified.len() != total_modified { - diff.modified.insert(total_modified, current_path.clone()); + match new.get_directory(¤t_path.as_os_str()) { + Ok(..) => { + // Directory exists in both current and new etc. + // Modifications/additions within this directory are handled recursively. + // No additional action needed here. } + + Err(ImageError::NotFound(..)) | Err(ImageError::NotADirectory(..)) => { + // This directory was deleted in new_etc + // If it was modified in the current etc, we want this back + // + // Was a directory in current etc, but is now + // a file/symlink in the new etc + if diff.added.len() != total_added { + diff.added.insert(total_added, current_path.clone()); + } else if diff.modified.len() != total_modified { + diff.modified.insert(total_added, current_path.clone()); + } + } + + Err(e) => Err(e)?, + } + + if diff.modified.len() != total_modified || diff.added.len() != total_added + { + check_if_mergable(new, inode, ¤t_path, diff); } } Err(ImageError::NotFound(..)) => { // Dir not found in original /etc, dir was added diff.added.push(current_path.clone()); + check_if_mergable(new, inode, ¤t_path, diff); // Also add every file inside that dir collect_all_files(&curr_dir, current_path.clone(), &mut diff.added); @@ -245,9 +314,10 @@ fn get_modifications( // Some directory was changed to a file/symlink // This should be counted in the diff, but we don't really merge this diff.modified.push(current_path.clone()); + check_if_mergable(new, inode, ¤t_path, diff); } - Err(e) => Err(e)?, + Err(e) => Err(e).with_context(|| format!("Opening pristine {path:?}"))?, } } @@ -255,8 +325,10 @@ fn get_modifications( Ok(old_leaf_id) => { let leaf = ¤t_leaves[leaf_id.0]; let old_leaf = &pristine_leaves[old_leaf_id.0]; + if !stat_eq_ignore_mtime(&old_leaf.stat, &leaf.stat) { diff.modified.push(current_path.clone()); + check_if_mergable(new, inode, ¤t_path, diff); current_path.pop(); continue; } @@ -266,6 +338,7 @@ fn get_modifications( if old_meta.content_hash != current_meta.content_hash { // File modified in some way diff.modified.push(current_path.clone()); + check_if_mergable(new, inode, ¤t_path, diff); } } @@ -273,12 +346,14 @@ fn get_modifications( if old_link != current_link { // Symlink modified in some way diff.modified.push(current_path.clone()); + check_if_mergable(new, inode, ¤t_path, diff); } } (Symlink(..), Regular(..)) | (Regular(..), Symlink(..)) => { // File changed to symlink or vice-versa diff.modified.push(current_path.clone()); + check_if_mergable(new, inode, ¤t_path, diff); } (a, b) => { @@ -290,14 +365,16 @@ fn get_modifications( Err(ImageError::IsADirectory(..)) => { // A directory was changed to a file diff.modified.push(current_path.clone()); + check_if_mergable(new, inode, ¤t_path, diff); } Err(ImageError::NotFound(..)) => { // File not found in original /etc, file was added diff.added.push(current_path.clone()); + check_if_mergable(new, inode, ¤t_path, diff); } - Err(e) => Err(e).context(format!("{path:?}"))?, + Err(e) => Err(e).with_context(|| format!("Opening pristine {path:?}"))?, }, } @@ -385,6 +462,7 @@ pub fn compute_diff( added: vec![], modified: vec![], removed: vec![], + unmergable_paths: vec![], }; get_modifications( @@ -392,6 +470,7 @@ pub fn compute_diff( ¤t_etc_files.root, &pristine_etc_files.leaves, ¤t_etc_files.leaves, + &new_etc_files.leaves, &new_etc_files.root, PathBuf::new(), &mut diff, @@ -422,6 +501,15 @@ pub fn print_diff(diff: &Diff, writer: &mut impl Write) { for removed in &diff.removed { let _ = writeln!(writer, "{} {removed:?}", ModificationType::Removed.red()); } + + for unmergable in &diff.unmergable_paths { + let _ = writeln!( + writer, + "{} {}", + ModificationType::Unmergable.magenta(), + unmergable.reason + ); + } } #[context("Collecting xattrs")] @@ -589,6 +677,7 @@ enum ModificationType { Added, Modified, Removed, + Unmergable, } impl std::fmt::Display for ModificationType { @@ -603,6 +692,7 @@ impl ModificationType { ModificationType::Added => "+", ModificationType::Modified => "~", ModificationType::Removed => "-", + ModificationType::Unmergable => "*", } } } @@ -613,23 +703,35 @@ fn create_dir_with_perms( stat: &Stat, new_inode: Option<&Inode>, ) -> anyhow::Result<()> { - // The new directory is not present in the new_etc, so we create it, else we only copy the - // metadata - if new_inode.is_none() { - // Here we use `create_dir_all` to create every parent as we will set the permissions later - // on. Due to the fact that we have an ordered (sorted) list of directories and directory - // entries and we have a DFS traversal, we will always have directory creation starting from - // the parent anyway. - // - // The exception being, if a directory is modified in the current_etc, and a new directory - // is added inside the modified directory, say `dir/prems` has its permissions modified and - // `dir/prems/new` is the new directory created. Since we handle added files/directories first, - // we will create the directories `perms/new` with directory `new` also getting its - // permissions set, but `perms` will not. `perms` will have its permissions set up when we - // handle the modified directories. - new_etc_fd - .create_dir_all(&dir_name) - .context(format!("Failed to create dir {dir_name:?}"))?; + match new_inode { + Some(inode) => match inode { + Inode::Directory(..) => { /* no-op */ } + + Inode::Leaf(..) => { + anyhow::bail!( + "Modified config directory {dir_name:?} newly defaults to file. Cannot merge" + ) + } + }, + + // The new directory is not present in the new_etc, so we create it, else we only copy the + // metadata + None => { + // Here we use `create_dir_all` to create every parent as we will set the permissions later + // on. Due to the fact that we have an ordered (sorted) list of directories and directory + // entries and we have a DFS traversal, we will always have directory creation starting from + // the parent anyway. + // + // The exception being, if a directory is modified in the current_etc, and a new directory + // is added inside the modified directory, say `dir/prems` has its permissions modified and + // `dir/prems/new` is the new directory created. Since we handle added files/directories first, + // we will create the directories `perms/new` with directory `new` also getting its + // permissions set, but `perms` will not. `perms` will have its permissions set up when we + // handle the modified directories. + new_etc_fd + .create_dir_all(&dir_name) + .context(format!("Failed to create dir {dir_name:?}"))?; + } } new_etc_fd @@ -730,12 +832,14 @@ fn merge_modified_files( file, current_inode.stat(current_leaves), new_inode, - )?; + ) + .context("Merging directory")?; } Inode::Leaf(leaf_id, _) => { let leaf = ¤t_leaves[leaf_id.0]; - merge_leaf(current_etc_fd, new_etc_fd, leaf, new_inode, file)? + merge_leaf(current_etc_fd, new_etc_fd, leaf, new_inode, file) + .context("Merging leaf")? } }; } @@ -755,7 +859,7 @@ fn merge_modified_files( } }, - Err(e) => Err(e)?, + Err(e) => Err(e).with_context(|| format!("Opening {file:?} in new etc"))?, }; } From 954905dc358db53c00bf0729b3ddf632f7c1b966 Mon Sep 17 00:00:00 2001 From: Pragyan Poudyal Date: Tue, 7 Jul 2026 14:42:11 +0530 Subject: [PATCH 2/3] cfs/stage: Compute etc diff before staging Before we write the staged deployment file at `/run/composefs` and before we create new boot entries, we now compute the etc diff in advance to check whethere we will face any merge conflicts while finalizing the deployment. If we find conflicts we exit with and error and detailing where conflicts were found Signed-off-by: Pragyan Poudyal --- crates/etc-merge/src/lib.rs | 28 +++++++++------ crates/lib/src/bootc_composefs/finalize.rs | 23 +++++++++---- crates/lib/src/bootc_composefs/update.rs | 40 ++++++++++++++++------ crates/lib/src/cli.rs | 3 +- crates/lib/src/store/mod.rs | 1 - 5 files changed, 66 insertions(+), 29 deletions(-) diff --git a/crates/etc-merge/src/lib.rs b/crates/etc-merge/src/lib.rs index 6862ca5cd..9858788fe 100644 --- a/crates/etc-merge/src/lib.rs +++ b/crates/etc-merge/src/lib.rs @@ -80,8 +80,8 @@ fn stat_eq_ignore_mtime(this: &Stat, other: &Stat) -> bool { #[derive(Debug)] pub struct UnmergablePaths { - path: PathBuf, - reason: String, + pub path: PathBuf, + pub reason: String, } /// Represents the differences between two directory trees. @@ -95,7 +95,7 @@ pub struct Diff { /// Paths that exist in the pristine /etc but not in the current one removed: Vec, /// Paths that are unmergable - unmergable_paths: Vec, + pub unmergable_paths: Vec, } fn collect_all_files( @@ -486,6 +486,19 @@ pub fn compute_diff( Ok(diff) } +pub fn print_unmergable_paths(diff: &Diff, writer: &mut impl Write) { + use owo_colors::OwoColorize; + + for unmergable in &diff.unmergable_paths { + let _ = writeln!( + writer, + "{} {}", + ModificationType::Unmergable.magenta(), + unmergable.reason + ); + } +} + /// Prints a colorized summary of differences to standard output. pub fn print_diff(diff: &Diff, writer: &mut impl Write) { use owo_colors::OwoColorize; @@ -502,14 +515,7 @@ pub fn print_diff(diff: &Diff, writer: &mut impl Write) { let _ = writeln!(writer, "{} {removed:?}", ModificationType::Removed.red()); } - for unmergable in &diff.unmergable_paths { - let _ = writeln!( - writer, - "{} {}", - ModificationType::Unmergable.magenta(), - unmergable.reason - ); - } + print_unmergable_paths(diff, writer); } #[context("Collecting xattrs")] diff --git a/crates/lib/src/bootc_composefs/finalize.rs b/crates/lib/src/bootc_composefs/finalize.rs index 9e0f5e3d4..23dcf8a72 100644 --- a/crates/lib/src/bootc_composefs/finalize.rs +++ b/crates/lib/src/bootc_composefs/finalize.rs @@ -14,12 +14,17 @@ use cap_std_ext::cap_std::{ambient_authority, fs::Dir}; use cap_std_ext::dirext::CapStdExtDirExt; use composefs::generic_tree::{FileSystem, Stat}; use composefs_ctl::composefs; -use etc_merge::{compute_diff, merge, print_diff, traverse_etc}; +use etc_merge::{Diff, compute_diff, merge, print_diff, traverse_etc}; use rustix::fs::fsync; use fn_error_context::context; -pub(crate) async fn get_etc_diff(storage: &Storage, booted_cfs: &BootedComposefs) -> Result<()> { +pub(crate) async fn get_etc_diff( + storage: &Storage, + booted_cfs: &BootedComposefs, + new_etc: Option<&Dir>, + print: bool, +) -> Result { let host = get_composefs_status(storage, booted_cfs).await?; let booted_composefs = host.require_composefs_booted()?; @@ -37,16 +42,22 @@ pub(crate) async fn get_etc_diff(storage: &Storage, booted_cfs: &BootedComposefs Dir::open_ambient_dir(erofs_tmp_mnt.dir.path().join("etc"), ambient_authority())?; let current_etc = Dir::open_ambient_dir("/etc", ambient_authority())?; - let (pristine_files, current_files, _) = traverse_etc(&pristine_etc, ¤t_etc, None)?; + let (pristine_files, current_files, new_etc_files) = + traverse_etc(&pristine_etc, ¤t_etc, new_etc)?; + let diff = compute_diff( &pristine_files, ¤t_files, - &FileSystem::new(Stat::uninitialized()), + &new_etc_files + .as_ref() + .unwrap_or(&FileSystem::new(Stat::uninitialized())), )?; - print_diff(&diff, &mut std::io::stdout()); + if print { + print_diff(&diff, &mut std::io::stdout()); + } - Ok(()) + Ok(diff) } pub(crate) async fn composefs_backend_finalize( diff --git a/crates/lib/src/bootc_composefs/update.rs b/crates/lib/src/bootc_composefs/update.rs index 1f48021c7..90e7af130 100644 --- a/crates/lib/src/bootc_composefs/update.rs +++ b/crates/lib/src/bootc_composefs/update.rs @@ -7,10 +7,12 @@ use composefs_ctl::composefs; use composefs_ctl::composefs_boot; use composefs_ctl::composefs_oci; use composefs_oci::image::create_filesystem; +use etc_merge::print_unmergable_paths; use fn_error_context::context; use ocidir::cap_std::ambient_authority; use ostree_ext::container::ManifestDiff; +use crate::bootc_composefs::finalize::get_etc_diff; use crate::bootc_composefs::gc::GCOpts; use crate::spec::BootloaderKind; use crate::{ @@ -288,6 +290,18 @@ pub(crate) async fn do_upgrade( .context("Failed to mount composefs image")?, )?; + // Check if three way etc merge is possible without conflicts + let new_etc = mounted_fs + .open_dir("etc") + .context("Opening deployment's etc")?; + + let diff = get_etc_diff(storage, booted_cfs, Some(&new_etc), false).await?; + + if !diff.unmergable_paths.is_empty() { + print_unmergable_paths(&diff, &mut std::io::stderr()); + anyhow::bail!("Merge conflicts found in etc"); + } + let boot_type = BootType::from(entry); let boot_digest = match boot_type { @@ -307,14 +321,16 @@ pub(crate) async fn do_upgrade( )?, }; + let staged_state = StagedDeployment { + depl_id: id.to_hex(), + finalization_locked: opts.download_only, + }; + write_composefs_state( &Utf8PathBuf::from("/sysroot"), &id, imgref, - Some(StagedDeployment { - depl_id: id.to_hex(), - finalization_locked: opts.download_only, - }), + Some(staged_state), boot_type, boot_digest, &manifest_digest, @@ -393,16 +409,20 @@ pub(crate) async fn upgrade_composefs( start_finalize_stated_svc()?; - // Make the staged deployment not download_only - let new_staged = StagedDeployment { - depl_id: staged.require_composefs()?.verity.clone(), - finalization_locked: false, - }; - let staged_depl_dir = Dir::open_ambient_dir(COMPOSEFS_TRANSIENT_STATE_DIR, ambient_authority()) .context("Opening transient state directory")?; + let current = staged_depl_dir + .read_to_string(COMPOSEFS_STAGED_DEPLOYMENT_FNAME) + .context("Reading staged file")?; + + let mut new_staged: StagedDeployment = + serde_json::from_str(¤t).context("Deserialzing staged file")?; + + // Make the staged deployment not download_only + new_staged.finalization_locked = false; + staged_depl_dir .atomic_replace_with( COMPOSEFS_STAGED_DEPLOYMENT_FNAME, diff --git a/crates/lib/src/cli.rs b/crates/lib/src/cli.rs index b70eb44ce..d66c0d246 100644 --- a/crates/lib/src/cli.rs +++ b/crates/lib/src/cli.rs @@ -2455,7 +2455,8 @@ async fn run_from_opt(opt: Opt) -> Result<()> { anyhow::bail!("ConfigDiff is only supported for composefs backend") } BootedStorageKind::Composefs(booted_cfs) => { - get_etc_diff(storage, &booted_cfs).await + get_etc_diff(storage, &booted_cfs, None, true).await?; + Ok(()) } } } diff --git a/crates/lib/src/store/mod.rs b/crates/lib/src/store/mod.rs index 78926d173..d7274d868 100644 --- a/crates/lib/src/store/mod.rs +++ b/crates/lib/src/store/mod.rs @@ -249,7 +249,6 @@ impl<'a> BootedOstree<'a> { } /// Represents a composefs-based boot environment -#[allow(dead_code)] pub struct BootedComposefs { pub repo: Arc, pub cmdline: &'static ComposefsCmdline, From df2466f6372d3bef1fb8f8d4c27d7ab8f7eae843 Mon Sep 17 00:00:00 2001 From: Pragyan Poudyal Date: Thu, 9 Jul 2026 15:03:13 +0530 Subject: [PATCH 3/3] tmt: Add tests for etc merge conflict This was predominantly AI generated with some minor tweaks from me. Makes sure that bootc upgrade/switch fails if merge conflicts are found during three way etc merge. Signed-off-by: Pragyan Poudyal --- tmt/plans/integration.fmf | 7 ++ tmt/tests/booted/test-etc-merge-conflict.nu | 78 +++++++++++++++++++++ tmt/tests/tests.fmf | 5 ++ 3 files changed, 90 insertions(+) create mode 100644 tmt/tests/booted/test-etc-merge-conflict.nu diff --git a/tmt/plans/integration.fmf b/tmt/plans/integration.fmf index 43e3f2cbc..e032999f6 100644 --- a/tmt/plans/integration.fmf +++ b/tmt/plans/integration.fmf @@ -285,4 +285,11 @@ execute: how: fmf test: - /tmt/tests/tests/test-45-composefs-corruped-state-resilience + +/plan-46-etc-merge-conflict: + summary: Verify etc merge conflicts are caught during upgrade, not finalization + discover: + how: fmf + test: + - /tmt/tests/tests/test-46-etc-merge-conflict # END GENERATED PLANS diff --git a/tmt/tests/booted/test-etc-merge-conflict.nu b/tmt/tests/booted/test-etc-merge-conflict.nu new file mode 100644 index 000000000..09dcdb8d4 --- /dev/null +++ b/tmt/tests/booted/test-etc-merge-conflict.nu @@ -0,0 +1,78 @@ +# number: 46 +# tmt: +# summary: Verify etc merge conflicts are caught during upgrade, not finalization +# duration: 15m +# +# Verifies that file-to-directory and directory-to-file type changes between +# the current /etc and the new image's /etc are detected during `bootc switch` +# (the upgrade phase) and cause it to fail immediately, rather than silently +# staging and failing during finalization at reboot. +# +# No reboot needed: the upgrade is expected to fail before staging. +use std assert +use tap.nu + +if not (tap is_composefs) { + exit 0 +} + +tap begin "etc merge conflict detection during upgrade" + +def main [] { + bootc image copy-to-storage + + # Test case 1: File in current /etc conflicts with directory in new image's /etc. + # Create a file on the live system; the new image will have a directory at + # the same path. + "local modification" | save /etc/test-etc-merge-conflict + + let td = mktemp -d + cd $td + + let dockerfile = ' +FROM localhost/bootc as base +RUN mkdir -p /etc/test-etc-merge-conflict && \ + echo "inner" > /etc/test-etc-merge-conflict/inner.conf +' + (tap make_uki_containerfile $dockerfile) | save Dockerfile + podman build -t localhost/bootc-etc-conflict . + + let result = do { bootc switch --transport containers-storage localhost/bootc-etc-conflict } | complete + print $"exit_code: ($result.exit_code)" + print $"stderr: ($result.stderr)" + + assert ($result.exit_code != 0) "bootc switch should fail: locally-added file conflicts with directory in new etc" + assert ($result.stderr | str contains "Merge conflicts found in etc") $"Expected 'Merge conflicts found in etc' in stderr, got: ($result.stderr)" + + # Verify no deployment was staged - the failure must happen during upgrade, + # not during finalization at reboot. + assert ((bootc status --json | from json | get status.staged) == null) "No deployment should be staged after merge conflict failure" + + # Test case 2: Directory in current /etc conflicts with file in new image's /etc. + # Create a directory with a file inside on the live system; the new image + # will have a regular file at the same path. + mkdir /etc/test-etc-merge-dir + "local config" | save /etc/test-etc-merge-dir/local.conf + + let td2 = mktemp -d + cd $td2 + + let dockerfile2 = ' +FROM localhost/bootc as base +RUN echo "now a file" > /etc/test-etc-merge-dir +' + (tap make_uki_containerfile $dockerfile2) | save Dockerfile + podman build -t localhost/bootc-etc-conflict-2 . + + let result2 = do { bootc switch --transport containers-storage localhost/bootc-etc-conflict-2 } | complete + print $"exit_code: ($result2.exit_code)" + print $"stderr: ($result2.stderr)" + + assert ($result2.exit_code != 0) "bootc switch should fail: locally-added directory conflicts with file in new etc" + assert ($result2.stderr | str contains "Merge conflicts found in etc") $"Expected 'Merge conflicts found in etc' in stderr, got: ($result2.stderr)" + + # Again verify nothing was staged + assert ((bootc status --json | from json | get status.staged) == null) "No deployment should be staged after directory-to-file conflict failure" + + tap ok +} diff --git a/tmt/tests/tests.fmf b/tmt/tests/tests.fmf index a8b5b91f8..0c12b5d4a 100644 --- a/tmt/tests/tests.fmf +++ b/tmt/tests/tests.fmf @@ -178,3 +178,8 @@ check: summary: Test composefs backend resilience to state corruption duration: 30m test: nu booted/test-composefs-corruped-state-resilience.nu + +/test-46-etc-merge-conflict: + summary: Verify etc merge conflicts are caught during upgrade, not finalization + duration: 15m + test: nu booted/test-etc-merge-conflict.nu