From c060006e8b1d47876dd48cb722f7e40743cb9d3d Mon Sep 17 00:00:00 2001 From: Pragyan Poudyal Date: Mon, 6 Jul 2026 13:39:53 +0530 Subject: [PATCH 1/5] 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 | 212 +++++++++++++++++++++++++----------- 1 file changed, 146 insertions(+), 66 deletions(-) diff --git a/crates/etc-merge/src/lib.rs b/crates/etc-merge/src/lib.rs index 0b78a0af5..1b11c835f 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,55 @@ 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(..)) => { + // This directory was deleted in new_etc + // If it was modified in the current etc, we want this back + 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(ImageError::NotADirectory(..)) => { + // Was a directory in current etc, but is now + // a file/symlink in the new etc + // + // We will fail on merging this if this was modified + // but add it in the modified list anyway + if diff.modified.len() != total_modified + || diff.added.len() != total_added + { + 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 +324,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 +335,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 +348,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 +356,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 +375,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 +472,7 @@ pub fn compute_diff( added: vec![], modified: vec![], removed: vec![], + unmergable_paths: vec![], }; get_modifications( @@ -392,6 +480,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 +511,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 +687,7 @@ enum ModificationType { Added, Modified, Removed, + Unmergable, } impl std::fmt::Display for ModificationType { @@ -603,6 +702,7 @@ impl ModificationType { ModificationType::Added => "+", ModificationType::Modified => "~", ModificationType::Removed => "-", + ModificationType::Unmergable => "*", } } } @@ -613,23 +713,37 @@ 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:?}"))?; + println!("dirname: {dir_name:?}, new_inode: {new_inode:?}"); + + 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 +844,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 +871,7 @@ fn merge_modified_files( } }, - Err(e) => Err(e)?, + Err(e) => Err(e).with_context(|| format!("Opening {file:?} in new etc"))?, }; } @@ -1130,40 +1246,4 @@ mod tests { Ok(()) } - - #[test] - fn file_to_dir() -> anyhow::Result<()> { - let tempdir = cap_std_ext::cap_tempfile::tempdir(cap_std::ambient_authority())?; - - tempdir.create_dir("pristine_etc")?; - tempdir.create_dir("current_etc")?; - tempdir.create_dir("new_etc")?; - - let p = tempdir.open_dir("pristine_etc")?; - let c = tempdir.open_dir("current_etc")?; - let n = tempdir.open_dir("new_etc")?; - - p.write("file-to-dir", "some text")?; - c.write("file-to-dir", "some text 1")?; - - n.create_dir_all("file-to-dir")?; - - let (pristine_etc_files, current_etc_files, new_etc_files) = - traverse_etc(&p, &c, Some(&n))?; - let diff = compute_diff( - &pristine_etc_files, - ¤t_etc_files, - &new_etc_files.as_ref().unwrap(), - )?; - - let merge_res = merge(&c, ¤t_etc_files, &n, &new_etc_files.unwrap(), &diff); - - assert!(merge_res.is_err()); - assert_eq!( - merge_res.unwrap_err().root_cause().to_string(), - "Modified config file \"file-to-dir\" newly defaults to directory. Cannot merge" - ); - - Ok(()) - } } From 2a630e7ec0a847a766576d64511cd9e9f60fda2b Mon Sep 17 00:00:00 2001 From: Pragyan Poudyal Date: Tue, 7 Jul 2026 11:24:40 +0530 Subject: [PATCH 2/5] etc-merge: Introduce MergeStrategy Currently we have a hard error whenever we find a merge conflict in etc. Introduce MergeStrategy enum with Fail, Skip and Replace variants which fail, skip copying and replace file/directory in the new etc respectively upon encountering a merge conflict. The deafult, while finalizing, is to still fail; next step would be making this configurable Signed-off-by: Pragyan Poudyal --- Cargo.lock | 1 + crates/etc-merge/Cargo.toml | 1 + crates/etc-merge/src/lib.rs | 388 +++++++++++++++++++-- crates/lib/src/bootc_composefs/finalize.rs | 13 +- crates/lib/src/cli.rs | 11 +- 5 files changed, 370 insertions(+), 44 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 1741e7d90..5c5b3457a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1422,6 +1422,7 @@ dependencies = [ "anstream", "anyhow", "cap-std-ext 5.1.2", + "clap", "composefs-ctl", "fn-error-context", "hex", diff --git a/crates/etc-merge/Cargo.toml b/crates/etc-merge/Cargo.toml index ce698250e..6cca52b00 100644 --- a/crates/etc-merge/Cargo.toml +++ b/crates/etc-merge/Cargo.toml @@ -16,6 +16,7 @@ composefs-ctl = { workspace = true } fn-error-context = { workspace = true } owo-colors = { workspace = true } anstream = { workspace = true } +clap = { workspace = true } [lints] workspace = true diff --git a/crates/etc-merge/src/lib.rs b/crates/etc-merge/src/lib.rs index 1b11c835f..8ed555121 100644 --- a/crates/etc-merge/src/lib.rs +++ b/crates/etc-merge/src/lib.rs @@ -5,6 +5,7 @@ use fn_error_context::context; use std::collections::BTreeMap; use std::ffi::OsStr; +use std::fmt::Display; use std::io::BufReader; use std::io::Write; use std::os::fd::{AsFd, AsRawFd}; @@ -15,6 +16,7 @@ use anyhow::Context; use cap_std_ext::cap_std; use cap_std_ext::cap_std::fs::{Dir as CapStdDir, MetadataExt, Permissions, PermissionsExt}; use cap_std_ext::dirext::CapStdExtDirExt; +use clap::ValueEnum; use composefs::fsverity::{FsVerityHashValue, Sha256HashValue, Sha512HashValue}; use composefs::generic_tree::{Directory, FileSystem, Inode, Leaf, LeafContent, LeafId, Stat}; use composefs::tree::ImageError; @@ -23,6 +25,28 @@ use rustix::fs::{ AtFlags, Gid, Uid, XattrFlags, lgetxattr, llistxattr, lsetxattr, readlinkat, symlinkat, }; +#[derive(Debug, Clone, Copy, ValueEnum, PartialEq, Eq, Default)] +#[clap(rename_all = "lowercase")] +pub enum MergeStrategy { + /// Fail if there is a merge conflict + #[default] + Fail, + /// Keep the new deployment's default, drop the user's conflicting modification + Skip, + /// Replace the new deployment's entry with the user's version + Replace, +} + +impl Display for MergeStrategy { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + MergeStrategy::Fail => f.write_str("fail"), + MergeStrategy::Skip => f.write_str("skip"), + MergeStrategy::Replace => f.write_str("replace"), + } + } +} + /// Metadata associated with a file, directory, or symlink entry. #[derive(Debug)] pub struct CustomMetadata { @@ -279,9 +303,12 @@ fn get_modifications( // No additional action needed here. } - Err(ImageError::NotFound(..)) => { + 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 { @@ -289,19 +316,6 @@ fn get_modifications( } } - Err(ImageError::NotADirectory(..)) => { - // Was a directory in current etc, but is now - // a file/symlink in the new etc - // - // We will fail on merging this if this was modified - // but add it in the modified list anyway - if diff.modified.len() != total_modified - || diff.added.len() != total_added - { - diff.modified.insert(total_added, current_path.clone()); - } - } - Err(e) => Err(e)?, } @@ -712,18 +726,34 @@ fn create_dir_with_perms( dir_name: &PathBuf, stat: &Stat, new_inode: Option<&Inode>, + merge_strategy: MergeStrategy, ) -> anyhow::Result<()> { - println!("dirname: {dir_name:?}, new_inode: {new_inode:?}"); - 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" - ) - } + Inode::Leaf(..) => match merge_strategy { + MergeStrategy::Fail => { + anyhow::bail!( + "Modified config directory {dir_name:?} newly defaults to file. Cannot merge" + ) + } + MergeStrategy::Skip => { + tracing::info!( + "Modified config directory {dir_name:?} newly defaults to file, skipping" + ); + return Ok(()); + } + MergeStrategy::Replace => { + new_etc_fd + .remove_all_optional(&dir_name) + .with_context(|| format!("Removing {dir_name:?} for replace"))?; + + new_etc_fd + .create_dir_all(&dir_name) + .context(format!("Failed to create dir {dir_name:?}"))?; + } + }, }, // The new directory is not present in the new_etc, so we create it, else we only copy the @@ -770,6 +800,7 @@ fn merge_leaf( leaf: &Leaf, new_inode: Option<&Inode>, file: &PathBuf, + merge_strategy: MergeStrategy, ) -> anyhow::Result<()> { let symlink = match &leaf.content { LeafContent::Regular(..) => None, @@ -782,7 +813,22 @@ fn merge_leaf( }; if matches!(new_inode, Some(Inode::Directory(..))) { - anyhow::bail!("Modified config file {file:?} newly defaults to directory. Cannot merge") + match merge_strategy { + MergeStrategy::Fail => { + anyhow::bail!( + "Modified config file {file:?} newly defaults to directory. Cannot merge" + ) + } + MergeStrategy::Skip => { + tracing::info!( + "Modified config file {file:?} newly defaults to directory, skipping" + ); + return Ok(()); + } + MergeStrategy::Replace => { + // Fall through remove_all_optional below handles directory removal + } + } }; // If a new file with the same path exists, we delete it @@ -819,9 +865,18 @@ fn merge_modified_files( current_etc_dirtree: &Directory, current_leaves: &[Leaf], new_etc_fd: &CapStdDir, - new_etc_dirtree: &Directory, + new_etc_dirtree: &mut Directory, + merge_strategy: MergeStrategy, ) -> anyhow::Result<()> { + let mut skipped_dirs: Vec = Vec::new(); + for file in files { + if merge_strategy == MergeStrategy::Skip && skipped_dirs.iter().any(|d| file.starts_with(d)) + { + tracing::info!("Skipping {file:?}: parent directory was skipped"); + continue; + } + let (dir, filename) = current_etc_dirtree .split(OsStr::new(&file)) .context("Getting directory and file")?; @@ -831,27 +886,64 @@ fn merge_modified_files( .ok_or_else(|| anyhow::anyhow!("{filename:?} not found"))?; // This will error out if some directory in a chain does not exist - let res = new_etc_dirtree.split(OsStr::new(&file)); + let new_etc_res = new_etc_dirtree.split(OsStr::new(&file)); + + let mut update_new_etc_tree_with: Option> = None; - match res { + match new_etc_res { Ok((new_dir, filename)) => { let new_inode = new_dir.lookup(filename); match current_inode { Inode::Directory(..) => { + let is_type_conflict = matches!(new_inode, Some(Inode::Leaf(..))); + + if is_type_conflict && merge_strategy == MergeStrategy::Skip { + tracing::info!( + "Modified config directory {file:?} newly defaults to file, skipping" + ); + skipped_dirs.push(file.clone()); + continue; + } + create_dir_with_perms( new_etc_fd, file, current_inode.stat(current_leaves), new_inode, + merge_strategy, ) .context("Merging directory")?; + + if is_type_conflict && merge_strategy == MergeStrategy::Replace { + // If in new etc this was a leaf, we'd have replaced it with a + // directory + update_new_etc_tree_with = + Some(Inode::Directory(Box::new(Directory::new(Stat::default())))); + } } Inode::Leaf(leaf_id, _) => { + let is_type_conflict = matches!(new_inode, Some(Inode::Directory(..))); + let leaf = ¤t_leaves[leaf_id.0]; - merge_leaf(current_etc_fd, new_etc_fd, leaf, new_inode, file) - .context("Merging leaf")? + merge_leaf( + current_etc_fd, + new_etc_fd, + leaf, + new_inode, + file, + merge_strategy, + ) + .context("Merging leaf")?; + + // Not strictly necessary but here anyway for consistency + // a leaf has no children in the diff, so no subsequent split() + // will try to touch it + if is_type_conflict && merge_strategy == MergeStrategy::Replace { + update_new_etc_tree_with = + Some(Inode::Leaf(LeafId(leaf_id.0), std::marker::PhantomData)); + } } }; } @@ -863,16 +955,39 @@ fn merge_modified_files( file, current_inode.stat(current_leaves), None, + merge_strategy, )?, Inode::Leaf(leaf_id, _) => { let leaf = ¤t_leaves[leaf_id.0]; - merge_leaf(current_etc_fd, new_etc_fd, leaf, None, file)?; + merge_leaf(current_etc_fd, new_etc_fd, leaf, None, file, merge_strategy)?; } }, Err(e) => Err(e).with_context(|| format!("Opening {file:?} in new etc"))?, }; + + // After Replace, update the in-memory tree to reflect the type change. + // Required for the directory case so children can find the new directory via split(). + if let Some(new_inode) = update_new_etc_tree_with { + let file_name = file + .file_name() + .ok_or_else(|| anyhow::anyhow!("No filename for {file:?}"))?; + + let parent_path = file + .parent() + .ok_or_else(|| anyhow::anyhow!("No parent for {file:?}"))?; + + let parent_dir = if parent_path.as_os_str().is_empty() { + &mut *new_etc_dirtree + } else { + new_etc_dirtree + .get_directory_mut(parent_path.as_os_str()) + .with_context(|| format!("Getting parent dir for {file:?}"))? + }; + + parent_dir.insert(file_name, new_inode); + } } Ok(()) @@ -886,16 +1001,30 @@ pub fn merge( current_etc_fd: &CapStdDir, current_etc_dirtree: &FileSystem, new_etc_fd: &CapStdDir, - new_etc_dirtree: &FileSystem, + new_etc_dirtree: &mut FileSystem, diff: &Diff, + merge_strategy: MergeStrategy, ) -> anyhow::Result<()> { + if merge_strategy == MergeStrategy::Fail && !diff.unmergable_paths.is_empty() { + anyhow::bail!( + "Cannot merge:\n{}", + diff.unmergable_paths + .iter() + .enumerate() + .map(|(i, r)| format!("{}. {}", i + 1, r.reason)) + .collect::>() + .join("\n") + ); + } + merge_modified_files( &diff.added, current_etc_fd, ¤t_etc_dirtree.root, ¤t_etc_dirtree.leaves, new_etc_fd, - &new_etc_dirtree.root, + &mut new_etc_dirtree.root, + merge_strategy, ) .context("Merging added files")?; @@ -905,7 +1034,8 @@ pub fn merge( ¤t_etc_dirtree.root, ¤t_etc_dirtree.leaves, new_etc_fd, - &new_etc_dirtree.root, + &mut new_etc_dirtree.root, + merge_strategy, ) .context("Merging modified files")?; @@ -992,7 +1122,7 @@ mod tests { c.remove_file(deleted_files[0])?; c.remove_file(deleted_files[1])?; - let (pristine_etc_files, current_etc_files, new_etc_files) = + let (pristine_etc_files, current_etc_files, mut new_etc_files) = traverse_etc(&p, &c, Some(&n))?; let res = compute_diff( @@ -1005,8 +1135,9 @@ mod tests { &c, ¤t_etc_files, &n, - new_etc_files.as_ref().unwrap(), + new_etc_files.as_mut().unwrap(), &res, + MergeStrategy::Fail, ) .expect("Merge failed"); @@ -1187,14 +1318,21 @@ mod tests { n.create_dir_all("dir/perms")?; n.write("dir/perms/some-file", "Some-file")?; - let (pristine_etc_files, current_etc_files, new_etc_files) = + let (pristine_etc_files, current_etc_files, mut new_etc_files) = traverse_etc(&p, &c, Some(&n))?; let diff = compute_diff( &pristine_etc_files, ¤t_etc_files, - &new_etc_files.as_ref().unwrap(), + new_etc_files.as_ref().unwrap(), + )?; + merge( + &c, + ¤t_etc_files, + &n, + new_etc_files.as_mut().unwrap(), + &diff, + MergeStrategy::Fail, )?; - merge(&c, ¤t_etc_files, &n, &new_etc_files.unwrap(), &diff)?; assert!(files_eq(&c, &n, "new_file.txt")?); assert!(files_eq(&c, &n, "a/new_file.txt")?); @@ -1246,4 +1384,182 @@ mod tests { Ok(()) } + + #[test] + fn file_to_dir_skip() -> anyhow::Result<()> { + let tempdir = cap_std_ext::cap_tempfile::tempdir(cap_std::ambient_authority())?; + + tempdir.create_dir("pristine_etc")?; + tempdir.create_dir("current_etc")?; + tempdir.create_dir("new_etc")?; + + let p = tempdir.open_dir("pristine_etc")?; + let c = tempdir.open_dir("current_etc")?; + let n = tempdir.open_dir("new_etc")?; + + p.write("file-to-dir", "some text")?; + c.write("file-to-dir", "modified text")?; + n.create_dir_all("file-to-dir")?; + + let (pristine_etc_files, current_etc_files, mut new_etc_files) = + traverse_etc(&p, &c, Some(&n))?; + let diff = compute_diff( + &pristine_etc_files, + ¤t_etc_files, + new_etc_files.as_ref().unwrap(), + )?; + + assert!(!diff.unmergable_paths.is_empty()); + + // Fail strategy should error + let merge_res = merge( + &c, + ¤t_etc_files, + &n, + new_etc_files.as_mut().unwrap(), + &diff, + MergeStrategy::Fail, + ); + assert!(merge_res.is_err()); + + // Skip strategy should succeed, keeping the new default (directory) + merge( + &c, + ¤t_etc_files, + &n, + new_etc_files.as_mut().unwrap(), + &diff, + MergeStrategy::Skip, + )?; + assert!(n.metadata("file-to-dir")?.is_dir()); + + Ok(()) + } + + #[test] + fn file_to_dir_replace() -> anyhow::Result<()> { + let tempdir = cap_std_ext::cap_tempfile::tempdir(cap_std::ambient_authority())?; + + tempdir.create_dir("pristine_etc")?; + tempdir.create_dir("current_etc")?; + tempdir.create_dir("new_etc")?; + + let p = tempdir.open_dir("pristine_etc")?; + let c = tempdir.open_dir("current_etc")?; + let n = tempdir.open_dir("new_etc")?; + + p.write("file-to-dir", "some text")?; + c.write("file-to-dir", "modified text")?; + n.create_dir_all("file-to-dir")?; + + let (pristine_etc_files, current_etc_files, mut new_etc_files) = + traverse_etc(&p, &c, Some(&n))?; + let diff = compute_diff( + &pristine_etc_files, + ¤t_etc_files, + new_etc_files.as_ref().unwrap(), + )?; + + // Replace strategy should succeed, user's file wins + merge( + &c, + ¤t_etc_files, + &n, + new_etc_files.as_mut().unwrap(), + &diff, + MergeStrategy::Replace, + )?; + assert!(n.metadata("file-to-dir")?.is_file()); + assert_eq!(n.read_to_string("file-to-dir")?, "modified text"); + + Ok(()) + } + + #[test] + fn dir_to_file_skip() -> anyhow::Result<()> { + let tempdir = cap_std_ext::cap_tempfile::tempdir(cap_std::ambient_authority())?; + + tempdir.create_dir("pristine_etc")?; + tempdir.create_dir("current_etc")?; + tempdir.create_dir("new_etc")?; + + let p = tempdir.open_dir("pristine_etc")?; + let c = tempdir.open_dir("current_etc")?; + let n = tempdir.open_dir("new_etc")?; + + // Directory in pristine and current (with modified child), file in new + p.create_dir("dir-to-file")?; + p.write("dir-to-file/child.txt", "pristine content")?; + c.create_dir("dir-to-file")?; + c.write("dir-to-file/child.txt", "modified content")?; + n.write("dir-to-file", "new file content")?; + + let (pristine_etc_files, current_etc_files, mut new_etc_files) = + traverse_etc(&p, &c, Some(&n))?; + let diff = compute_diff( + &pristine_etc_files, + ¤t_etc_files, + new_etc_files.as_ref().unwrap(), + )?; + + assert!(!diff.unmergable_paths.is_empty()); + + // Skip: new default (file) preserved, user's directory dropped + merge( + &c, + ¤t_etc_files, + &n, + new_etc_files.as_mut().unwrap(), + &diff, + MergeStrategy::Skip, + )?; + assert!(n.metadata("dir-to-file")?.is_file()); + assert_eq!(n.read_to_string("dir-to-file")?, "new file content"); + + Ok(()) + } + + #[test] + fn dir_to_file_replace() -> anyhow::Result<()> { + let tempdir = cap_std_ext::cap_tempfile::tempdir(cap_std::ambient_authority())?; + + tempdir.create_dir("pristine_etc")?; + tempdir.create_dir("current_etc")?; + tempdir.create_dir("new_etc")?; + + let p = tempdir.open_dir("pristine_etc")?; + let c = tempdir.open_dir("current_etc")?; + let n = tempdir.open_dir("new_etc")?; + + p.create_dir("dir-to-file")?; + p.write("dir-to-file/child.txt", "pristine content")?; + c.create_dir("dir-to-file")?; + c.write("dir-to-file/child.txt", "modified content")?; + n.write("dir-to-file", "new file content")?; + + let (pristine_etc_files, current_etc_files, mut new_etc_files) = + traverse_etc(&p, &c, Some(&n))?; + let diff = compute_diff( + &pristine_etc_files, + ¤t_etc_files, + new_etc_files.as_ref().unwrap(), + )?; + + // Replace: user's directory wins + merge( + &c, + ¤t_etc_files, + &n, + new_etc_files.as_mut().unwrap(), + &diff, + MergeStrategy::Replace, + )?; + assert!(n.metadata("dir-to-file")?.is_dir()); + assert_eq!( + n.read_to_string("dir-to-file/child.txt")?, + "modified content" + ); + + Ok(()) + } } diff --git a/crates/lib/src/bootc_composefs/finalize.rs b/crates/lib/src/bootc_composefs/finalize.rs index 9e0f5e3d4..93fe7b996 100644 --- a/crates/lib/src/bootc_composefs/finalize.rs +++ b/crates/lib/src/bootc_composefs/finalize.rs @@ -14,7 +14,7 @@ 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::{MergeStrategy, compute_diff, merge, print_diff, traverse_etc}; use rustix::fs::fsync; use fn_error_context::context; @@ -113,11 +113,18 @@ pub(crate) async fn composefs_backend_finalize( let (pristine_files, current_files, new_files) = traverse_etc(&pristine_etc, ¤t_etc, Some(&new_etc))?; - let new_files = + let mut new_files = new_files.ok_or_else(|| anyhow::anyhow!("Failed to get dirtree for new etc"))?; let diff = compute_diff(&pristine_files, ¤t_files, &new_files)?; - merge(¤t_etc, ¤t_files, &new_etc, &new_files, &diff)?; + merge( + ¤t_etc, + ¤t_files, + &new_etc, + &mut new_files, + &diff, + MergeStrategy::Fail, + )?; // Remove /etc/.updated from the new deployment so that ConditionNeedsUpdate=|/etc // services (systemd-sysusers, systemd-tmpfiles) run on the first boot, mirroring diff --git a/crates/lib/src/cli.rs b/crates/lib/src/cli.rs index 26a4ba8f3..029fc9f5e 100644 --- a/crates/lib/src/cli.rs +++ b/crates/lib/src/cli.rs @@ -24,7 +24,7 @@ use composefs_ctl::composefs_boot; use composefs_ctl::composefs_oci; use composefs_boot::BootOps as _; -use etc_merge::{compute_diff, print_diff}; +use etc_merge::{MergeStrategy, compute_diff, print_diff}; use fn_error_context::context; use indoc::indoc; use ostree::gio; @@ -717,6 +717,8 @@ pub(crate) enum InternalsOpts { /// Whether to perform the three way merge or not #[clap(long)] merge: bool, + #[clap(long, requires = "merge", default_value_t = MergeStrategy::Fail)] + strategy: MergeStrategy, }, #[cfg(feature = "docgen")] /// Dump CLI structure as JSON for documentation generation @@ -2202,6 +2204,7 @@ async fn run_from_opt(opt: Opt) -> Result<()> { current_etc, new_etc, merge, + strategy: merge_strategy, } => { let pristine_etc = Dir::open_ambient_dir(pristine_etc, cap_std::ambient_authority())?; @@ -2211,15 +2214,13 @@ async fn run_from_opt(opt: Opt) -> Result<()> { let (p, c, n) = etc_merge::traverse_etc(&pristine_etc, ¤t_etc, Some(&new_etc))?; - let n = n - .as_ref() - .ok_or_else(|| anyhow::anyhow!("Failed to get new directory tree"))?; + let mut n = n.ok_or_else(|| anyhow::anyhow!("Failed to get new directory tree"))?; let diff = compute_diff(&p, &c, &n)?; print_diff(&diff, &mut std::io::stdout()); if merge { - etc_merge::merge(¤t_etc, &c, &new_etc, &n, &diff)?; + etc_merge::merge(¤t_etc, &c, &new_etc, &mut n, &diff, merge_strategy)?; } Ok(()) From 6e8199bedc436a34e3eb4f6f2c2795993c0a2f1b Mon Sep 17 00:00:00 2001 From: Pragyan Poudyal Date: Tue, 7 Jul 2026 14:41:08 +0530 Subject: [PATCH 3/5] upgrade/switch: Accept `merge_strategy` cli param Signed-off-by: Pragyan Poudyal --- crates/lib/src/cli.rs | 13 +++++++++++++ docs/src/man/bootc-switch.8.md | 11 +++++++++++ docs/src/man/bootc-upgrade.8.md | 11 +++++++++++ 3 files changed, 35 insertions(+) diff --git a/crates/lib/src/cli.rs b/crates/lib/src/cli.rs index 029fc9f5e..cd02f266e 100644 --- a/crates/lib/src/cli.rs +++ b/crates/lib/src/cli.rs @@ -83,6 +83,13 @@ impl TryFrom for ProgressWriter { } } +#[derive(Debug, Parser, PartialEq, Eq)] +pub(crate) struct EtcMergeStrategyOpts { + /// The merge strategy to use when conflicts are found while performing three way etc merge + #[clap(long, default_value_t = MergeStrategy::Fail)] + pub(crate) merge_strategy: MergeStrategy, +} + /// Perform an upgrade operation #[derive(Debug, Parser, PartialEq, Eq)] pub(crate) struct UpgradeOpts { @@ -133,6 +140,9 @@ pub(crate) struct UpgradeOpts { #[clap(flatten)] pub(crate) progress: ProgressOptions, + + #[clap(flatten)] + pub(crate) merge_strategy: EtcMergeStrategyOpts, } /// Perform an switch operation @@ -194,6 +204,9 @@ pub(crate) struct SwitchOpts { #[clap(flatten)] pub(crate) progress: ProgressOptions, + + #[clap(flatten)] + pub(crate) merge_strategy: EtcMergeStrategyOpts, } /// Options controlling rollback diff --git a/docs/src/man/bootc-switch.8.md b/docs/src/man/bootc-switch.8.md index 116f98553..a8990d7bc 100644 --- a/docs/src/man/bootc-switch.8.md +++ b/docs/src/man/bootc-switch.8.md @@ -73,6 +73,17 @@ Soft reboot allows faster system restart by avoiding full hardware reboot when p Retain reference to currently booted image +**--merge-strategy**=*MERGE_STRATEGY* + + The merge strategy to use when conflicts are found while performing three way etc merge + + Possible values: + - fail + - skip + - replace + + Default: fail + # EXAMPLES diff --git a/docs/src/man/bootc-upgrade.8.md b/docs/src/man/bootc-upgrade.8.md index b1b3f3b69..49e7d5df3 100644 --- a/docs/src/man/bootc-upgrade.8.md +++ b/docs/src/man/bootc-upgrade.8.md @@ -73,6 +73,17 @@ Soft reboot allows faster system restart by avoiding full hardware reboot when p Upgrade to a different tag of the currently booted image +**--merge-strategy**=*MERGE_STRATEGY* + + The merge strategy to use when conflicts are found while performing three way etc merge + + Possible values: + - fail + - skip + - replace + + Default: fail + # EXAMPLES From 3219e8f23daeb6ee1bf76edfb72194f0fd58a21a Mon Sep 17 00:00:00 2001 From: Pragyan Poudyal Date: Tue, 7 Jul 2026 14:42:11 +0530 Subject: [PATCH 4/5] cfs/stage: Compute etc diff while staging Before we write the staged deployment file at `/run/composefs` we now compute the etc diff in advance to check whethere we will face any merge conflicts while finalizing the deployment. We accept the merge strategy either from the Cli or from config file at `/usr/lib/bootc/finalize.toml`. If we find conflicts and merge strategy is to Fail, we fail early on upgrade/switch and not while finalizing the deployment. Write the merge strategy to the staged file to use during finalization Signed-off-by: Pragyan Poudyal --- Cargo.lock | 1 + crates/etc-merge/Cargo.toml | 1 + crates/etc-merge/src/lib.rs | 19 ++++++- crates/lib/src/bootc_composefs/finalize.rs | 49 ++++++++++++++--- crates/lib/src/bootc_composefs/state.rs | 64 ++++++++++++++++++++-- crates/lib/src/bootc_composefs/status.rs | 17 +++++- crates/lib/src/bootc_composefs/switch.rs | 1 + crates/lib/src/bootc_composefs/update.rs | 37 +++++++++---- crates/lib/src/cli.rs | 3 +- crates/lib/src/composefs_consts.rs | 7 +++ crates/lib/src/store/mod.rs | 1 - 11 files changed, 171 insertions(+), 29 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 5c5b3457a..cda51d834 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1429,6 +1429,7 @@ dependencies = [ "openssl", "owo-colors", "rustix", + "serde", "tracing", ] diff --git a/crates/etc-merge/Cargo.toml b/crates/etc-merge/Cargo.toml index 6cca52b00..f66090362 100644 --- a/crates/etc-merge/Cargo.toml +++ b/crates/etc-merge/Cargo.toml @@ -17,6 +17,7 @@ fn-error-context = { workspace = true } owo-colors = { workspace = true } anstream = { workspace = true } clap = { workspace = true } +serde = { workspace = true } [lints] workspace = true diff --git a/crates/etc-merge/src/lib.rs b/crates/etc-merge/src/lib.rs index 8ed555121..4ab1cce78 100644 --- a/crates/etc-merge/src/lib.rs +++ b/crates/etc-merge/src/lib.rs @@ -3,6 +3,7 @@ #![allow(dead_code)] use fn_error_context::context; +use serde::{Deserialize, Serialize}; use std::collections::BTreeMap; use std::ffi::OsStr; use std::fmt::Display; @@ -25,8 +26,9 @@ use rustix::fs::{ AtFlags, Gid, Uid, XattrFlags, lgetxattr, llistxattr, lsetxattr, readlinkat, symlinkat, }; -#[derive(Debug, Clone, Copy, ValueEnum, PartialEq, Eq, Default)] +#[derive(Debug, Clone, Copy, ValueEnum, PartialEq, Eq, Default, Serialize, Deserialize)] #[clap(rename_all = "lowercase")] +#[serde(rename_all = "lowercase")] pub enum MergeStrategy { /// Fail if there is a merge conflict #[default] @@ -47,6 +49,19 @@ impl Display for MergeStrategy { } } +impl TryFrom<&str> for MergeStrategy { + type Error = anyhow::Error; + + fn try_from(value: &str) -> Result { + match value { + "fail" => Ok(Self::Fail), + "skip" => Ok(Self::Skip), + "replace" => Ok(Self::Replace), + s => anyhow::bail!("Unknown strategy {s}"), + } + } +} + /// Metadata associated with a file, directory, or symlink entry. #[derive(Debug)] pub struct CustomMetadata { @@ -119,7 +134,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( diff --git a/crates/lib/src/bootc_composefs/finalize.rs b/crates/lib/src/bootc_composefs/finalize.rs index 93fe7b996..1527f77d8 100644 --- a/crates/lib/src/bootc_composefs/finalize.rs +++ b/crates/lib/src/bootc_composefs/finalize.rs @@ -3,8 +3,10 @@ use std::path::Path; use crate::bootc_composefs::boot::BootType; use crate::bootc_composefs::gc::{GCOpts, composefs_gc}; use crate::bootc_composefs::rollback::{rename_exchange_bls_entries, rename_exchange_user_cfg}; -use crate::bootc_composefs::status::get_composefs_status; -use crate::composefs_consts::STATE_DIR_ABS; +use crate::bootc_composefs::status::{StagedDeployment, get_composefs_status}; +use crate::composefs_consts::{ + COMPOSEFS_STAGED_DEPLOYMENT_FNAME, COMPOSEFS_TRANSIENT_STATE_DIR, STATE_DIR_ABS, +}; use crate::spec::BootloaderKind; use crate::store::{BootedComposefs, Storage}; use anyhow::{Context, Result}; @@ -14,12 +16,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::{MergeStrategy, 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, + against_deployment: Option<&str>, + print: bool, +) -> Result { let host = get_composefs_status(storage, booted_cfs).await?; let booted_composefs = host.require_composefs_booted()?; @@ -37,16 +44,30 @@ 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 new_etc = match against_deployment { + Some(verity) => { + let new_etc_path = Path::new(STATE_DIR_ABS).join(&verity).join("etc"); + Some(Dir::open_ambient_dir(&new_etc_path, ambient_authority())?) + } + None => None, + }; + + let (pristine_files, current_files, new_etc_files) = + traverse_etc(&pristine_etc, ¤t_etc, new_etc.as_ref())?; + 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( @@ -99,6 +120,16 @@ pub(crate) async fn composefs_backend_finalize( let erofs_tmp_mnt = TempMount::mount_fd(&composefs_fd)?; + 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 staged_depl_file: StagedDeployment = + serde_json::from_str(¤t).context("Deserialzing staged file")?; + // Perform the /etc merge let pristine_etc = Dir::open_ambient_dir(erofs_tmp_mnt.dir.path().join("etc"), ambient_authority())?; @@ -123,7 +154,7 @@ pub(crate) async fn composefs_backend_finalize( &new_etc, &mut new_files, &diff, - MergeStrategy::Fail, + staged_depl_file.merge_strategy, )?; // Remove /etc/.updated from the new deployment so that ConditionNeedsUpdate=|/etc diff --git a/crates/lib/src/bootc_composefs/state.rs b/crates/lib/src/bootc_composefs/state.rs index 019662559..b36424ee8 100644 --- a/crates/lib/src/bootc_composefs/state.rs +++ b/crates/lib/src/bootc_composefs/state.rs @@ -1,4 +1,4 @@ -use std::io::Write; +use std::io::{Read, Write}; use std::os::unix::fs::symlink; use std::path::Path; use std::{fs::create_dir_all, process::Command}; @@ -15,6 +15,7 @@ use cap_std_ext::cap_std::fs::{Dir, Permissions, PermissionsExt}; use cap_std_ext::dirext::CapStdExtDirExt; use composefs::fsverity::{FsVerityHashValue, Sha512HashValue}; use composefs_ctl::composefs; +use etc_merge::MergeStrategy; use fn_error_context::context; use ostree_ext::container::deploy::ORIGIN_CONTAINER; @@ -24,11 +25,14 @@ use rustix::{ mount::MountAttrFlags, path::Arg, }; +use serde::{Deserialize, Serialize}; use crate::bootc_composefs::boot::BootType; +use crate::bootc_composefs::finalize::get_etc_diff; use crate::bootc_composefs::status::{ - ComposefsCmdline, StagedDeployment, get_sorted_type1_boot_entries, + ComposefsCmdline, StagedDeploymentState, get_sorted_type1_boot_entries, }; +use crate::composefs_consts::FINALIZE_CONF_FILE; use crate::parsers::bls_config::{BLSConfigType, EFIKey}; use crate::store::{BootedComposefs, Storage}; use crate::{ @@ -43,6 +47,12 @@ use crate::{ utils::path_relative_to, }; +#[derive(Debug, Serialize, Deserialize)] +struct FinalizeConfFile { + /// Merge strategy for etc + merge_strategy: Option, +} + /// Read and parse the `.origin` INI file for a deployment. /// /// Returns `None` if the state directory or origin file doesn't exist @@ -245,7 +255,7 @@ pub(crate) async fn write_composefs_state( root_path: &Utf8PathBuf, deployment_id: &Sha512HashValue, target_imgref: &ImageReference, - staged: Option, + staged: Option>, boot_type: BootType, boot_digest: String, manifest_digest: &str, @@ -307,7 +317,50 @@ pub(crate) async fn write_composefs_state( ) .context("Failed to write to .origin file")?; - if let Some(staged) = staged { + if let Some(mut staged_opts) = staged { + let diff = get_etc_diff( + staged_opts.storage, + staged_opts.booted_cfs, + Some(&deployment_id.to_hex()), + false, + ) + .await?; + + if !diff.unmergable_paths.is_empty() + && staged_opts.staged_depl.merge_strategy == MergeStrategy::Fail + { + let error = anyhow::anyhow!( + "Merge conflicts found in etc and current strategy is either to fail or is not configured. {}", + "Hint: use --merge-strategy or config file /usr/lib/bootc/finalize.toml to specify the strategy" + ); + + // The CLI default is Fail; check if the image ships a different default + let file = staged_opts + .mounted_staged_depl + .open_optional(FINALIZE_CONF_FILE) + .context("Opening finalize.toml")?; + + let Some(mut file) = file else { + return Err(error); + }; + + let mut s = String::new(); + file.read_to_string(&mut s)?; + let conf: FinalizeConfFile = + toml::from_str(&s).with_context(|| format!("Parsing {FINALIZE_CONF_FILE}"))?; + + let Some(strategy) = conf.merge_strategy else { + return Err(error); + }; + + if strategy == MergeStrategy::Fail { + return Err(error); + } + + // Store this in finalized file + staged_opts.staged_depl.merge_strategy = strategy; + } + std::fs::create_dir_all(COMPOSEFS_TRANSIENT_STATE_DIR) .with_context(|| format!("Creating {COMPOSEFS_TRANSIENT_STATE_DIR}"))?; @@ -318,7 +371,8 @@ pub(crate) async fn write_composefs_state( staged_depl_dir .atomic_write( COMPOSEFS_STAGED_DEPLOYMENT_FNAME, - staged + staged_opts + .staged_depl .to_canon_json_vec() .context("Failed to serialize staged deployment JSON")?, ) diff --git a/crates/lib/src/bootc_composefs/status.rs b/crates/lib/src/bootc_composefs/status.rs index c8e7851c3..b83cc91e3 100644 --- a/crates/lib/src/bootc_composefs/status.rs +++ b/crates/lib/src/bootc_composefs/status.rs @@ -6,6 +6,7 @@ use bootc_mount::inspect_filesystem; use composefs_ctl::composefs::fsverity::Sha512HashValue; use composefs_ctl::composefs_oci; use composefs_oci::OciImage; +use etc_merge::MergeStrategy; use fn_error_context::context; use serde::{Deserialize, Serialize}; @@ -26,7 +27,7 @@ use crate::{ grub_menuconfig::{MenuEntry, parse_grub_menuentry_file}, }, spec::{BootEntry, BootOrder, BootloaderKind, Host, HostSpec, ImageStatus}, - store::Storage, + store::{BootedComposefs, Storage}, utils::{EfiError, read_uefi_var}, }; @@ -127,6 +128,20 @@ pub(crate) struct StagedDeployment { /// Whether to finalize this staged deployment on reboot or not /// This also maps to `download_only` field in `BootEntry` pub(crate) finalization_locked: bool, + /// The merge strategy configured for three way etc merge + pub(crate) merge_strategy: MergeStrategy, +} + +/// The state needed to write `/run/composefs/staged-deployment` +pub(crate) struct StagedDeploymentState<'a> { + /// The JSON that's serialized into `/run/composefs/staged-deployment` + pub(crate) staged_depl: StagedDeployment, + /// Storage object + pub(crate) storage: &'a Storage, + /// The current booted composefs info + pub(crate) booted_cfs: &'a BootedComposefs, + /// Mounted staged EROFS fd + pub(crate) mounted_staged_depl: &'a Dir, } #[derive(Debug, PartialEq)] diff --git a/crates/lib/src/bootc_composefs/switch.rs b/crates/lib/src/bootc_composefs/switch.rs index 3f8b5af8d..bf9af5d1c 100644 --- a/crates/lib/src/bootc_composefs/switch.rs +++ b/crates/lib/src/bootc_composefs/switch.rs @@ -77,6 +77,7 @@ pub(crate) async fn switch_composefs( apply: opts.apply, download_only: false, use_unified, + merge_strategy: opts.merge_strategy.merge_strategy, }; if let Some(cfg_verity) = image { diff --git a/crates/lib/src/bootc_composefs/update.rs b/crates/lib/src/bootc_composefs/update.rs index e0e677958..3c2c9fe6e 100644 --- a/crates/lib/src/bootc_composefs/update.rs +++ b/crates/lib/src/bootc_composefs/update.rs @@ -7,11 +7,13 @@ use composefs_ctl::composefs; use composefs_ctl::composefs_boot; use composefs_ctl::composefs_oci; use composefs_oci::image::create_filesystem; +use etc_merge::MergeStrategy; use fn_error_context::context; use ocidir::cap_std::ambient_authority; use ostree_ext::container::ManifestDiff; use crate::bootc_composefs::gc::GCOpts; +use crate::bootc_composefs::status::StagedDeploymentState; use crate::spec::BootloaderKind; use crate::{ bootc_composefs::{ @@ -210,6 +212,8 @@ pub(crate) struct DoUpgradeOpts { pub(crate) download_only: bool, /// Whether to use unified storage (containers-storage + composefs). pub(crate) use_unified: bool, + /// Merge strategy to use when we have a conflict when merging etc + pub(crate) merge_strategy: MergeStrategy, } async fn apply_upgrade( @@ -307,14 +311,22 @@ pub(crate) async fn do_upgrade( )?, }; + let staged_state = StagedDeploymentState { + storage, + booted_cfs, + mounted_staged_depl: &mounted_fs, + staged_depl: StagedDeployment { + depl_id: id.to_hex(), + finalization_locked: opts.download_only, + merge_strategy: opts.merge_strategy, + }, + }; + 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, @@ -375,6 +387,7 @@ pub(crate) async fn upgrade_composefs( apply: opts.apply, download_only: opts.download_only, use_unified: false, + merge_strategy: opts.merge_strategy.merge_strategy, }; if opts.from_downloaded { @@ -393,16 +406,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 cd02f266e..e43357f72 100644 --- a/crates/lib/src/cli.rs +++ b/crates/lib/src/cli.rs @@ -2366,7 +2366,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/composefs_consts.rs b/crates/lib/src/composefs_consts.rs index 8617f1005..c72eeb3f2 100644 --- a/crates/lib/src/composefs_consts.rs +++ b/crates/lib/src/composefs_consts.rs @@ -54,3 +54,10 @@ pub(crate) const UKI_NAME_PREFIX: &str = TYPE1_BOOT_DIR_PREFIX; /// that keep the manifest, config, and layer splitstreams alive. This is /// analogous to how ostree uses `ostree/` refs. pub(crate) const BOOTC_TAG_PREFIX: &str = "localhost/bootc-"; + +/// This is the file that stores configuration for deployment finalization +/// ```toml +/// # /etc merge strategy +/// merge_strategy = "replace" +/// ``` +pub(crate) const FINALIZE_CONF_FILE: &str = "usr/lib/bootc/finalize.toml"; 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 d9dcd821de9d8a2055b66bf0a079be5f1feaa724 Mon Sep 17 00:00:00 2001 From: Pragyan Poudyal Date: Tue, 7 Jul 2026 15:10:24 +0530 Subject: [PATCH 5/5] tmt: Add test for etc-merge strategy The test was largely AI generated Signed-off-by: Pragyan Poudyal --- tmt/plans/integration.fmf | 7 ++ tmt/tests/booted/test-etc-merge-strategy.nu | 106 ++++++++++++++++++++ tmt/tests/tests.fmf | 5 + 3 files changed, 118 insertions(+) create mode 100644 tmt/tests/booted/test-etc-merge-strategy.nu diff --git a/tmt/plans/integration.fmf b/tmt/plans/integration.fmf index 43e3f2cbc..1ec117f48 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-strategy: + summary: Test etc merge strategy (skip/replace/fail) + discover: + how: fmf + test: + - /tmt/tests/tests/test-46-etc-merge-strategy # END GENERATED PLANS diff --git a/tmt/tests/booted/test-etc-merge-strategy.nu b/tmt/tests/booted/test-etc-merge-strategy.nu new file mode 100644 index 000000000..fb5157de5 --- /dev/null +++ b/tmt/tests/booted/test-etc-merge-strategy.nu @@ -0,0 +1,106 @@ +# number: 46 +# tmt: +# summary: Test etc merge strategy (skip/replace/fail) +# duration: 30m + +use std assert +use tap.nu +use bootc_testlib.nu + +if not (tap is_composefs) { + tap ok + exit 0 +} + +# Image that changes a file to a directory and a directory to a file +const DOCKERFILE_CONFLICT = ' +FROM localhost/bootc as base + +# file-to-dir: /etc/test-file-to-dir was a file in base, now becomes a directory +RUN rm -f /etc/test-file-to-dir && mkdir -p /etc/test-file-to-dir && echo "new-default" > /etc/test-file-to-dir/config + +# dir-to-file: /etc/test-dir-to-file/ was a directory in base, now becomes a file +RUN rm -rf /etc/test-dir-to-file && echo "new-default-file" > /etc/test-dir-to-file +' + +def first_boot [] { + tap begin "etc merge strategy test" + + bootc image copy-to-storage + + # Create the paths in the running pristine image so they exist in the base + # file-to-dir: starts as a file + echo "pristine-content" | save --force /etc/test-file-to-dir + + # dir-to-file: starts as a directory with a child + mkdir /etc/test-dir-to-file + echo "pristine-child" | save --force /etc/test-dir-to-file/child.conf + + # Now modify them (simulating user customization on the live system) + echo "user-modified-content" | save --force /etc/test-file-to-dir + echo "user-modified-child" | save --force /etc/test-dir-to-file/child.conf + + # Build the conflicting image + (tap make_uki_containerfile $DOCKERFILE_CONFLICT) | save --force Dockerfile + podman build -t localhost/bootc-etc-conflict . + + # Switch with skip strategy: new defaults should win + bootc switch --transport containers-storage --merge-strategy skip localhost/bootc-etc-conflict + tmt-reboot +} + +def second_boot [] { + # Verify skip: new defaults won + # file-to-dir: should now be a directory (new default won) + assert (/etc/test-file-to-dir | path type) == "dir" "skip: file-to-dir should be a directory" + assert (open /etc/test-file-to-dir/config | str trim) == "new-default" "skip: file-to-dir/config should have new default content" + + # dir-to-file: should now be a file (new default won) + assert (/etc/test-dir-to-file | path type) == "file" "skip: dir-to-file should be a file" + assert (open /etc/test-dir-to-file | str trim) == "new-default-file" "skip: dir-to-file should have new default content" + + # Re-create the original paths as files/dirs so they exist in this image's pristine etc + echo "pristine-content-2" | save --force /etc/test-file-to-dir-2 + + mkdir /etc/test-dir-to-file-2 + echo "pristine-child-2" | save --force /etc/test-dir-to-file-2/child.conf + + # User modifications + echo "user-keeps-this" | save --force /etc/test-file-to-dir-2 + echo "user-keeps-this-child" | save --force /etc/test-dir-to-file-2/child.conf + + # Build image with type conflicts on the -2 paths + let dockerfile_replace = ' +FROM localhost/bootc as base +RUN rm -f /etc/test-file-to-dir-2 && mkdir -p /etc/test-file-to-dir-2 && echo "should-be-replaced" > /etc/test-file-to-dir-2/config +RUN rm -rf /etc/test-dir-to-file-2 && echo "should-be-replaced" > /etc/test-dir-to-file-2 +' + (tap make_uki_containerfile $dockerfile_replace) | save --force Dockerfile + podman build -t localhost/bootc-etc-replace . + + # Switch with replace strategy: user modifications should win + bootc switch --transport containers-storage --merge-strategy replace localhost/bootc-etc-replace + tmt-reboot +} + +def third_boot [] { + # Verify replace: user modifications won + # file-to-dir: user had a file, should still be a file with user content + assert (/etc/test-file-to-dir-2 | path type) == "file" "replace: file-to-dir-2 should be a file (user wins)" + assert (open /etc/test-file-to-dir-2 | str trim) == "user-keeps-this" "replace: file-to-dir-2 should have user content" + + # dir-to-file: user had a directory, should still be a directory with user content + assert (/etc/test-dir-to-file-2 | path type) == "dir" "replace: dir-to-file-2 should be a directory (user wins)" + assert (open /etc/test-dir-to-file-2/child.conf | str trim) == "user-keeps-this-child" "replace: child.conf should have user content" + + tap ok +} + +def main [] { + match $env.TMT_REBOOT_COUNT? { + null | "0" => first_boot, + "1" => second_boot, + "2" => third_boot, + $o => { error make { msg: $"Invalid TMT_REBOOT_COUNT ($o)" } }, + } +} diff --git a/tmt/tests/tests.fmf b/tmt/tests/tests.fmf index a8b5b91f8..af03dfb40 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-strategy: + summary: Test etc merge strategy (skip/replace/fail) + duration: 30m + test: nu booted/test-etc-merge-strategy.nu