Initial implementation for /etc merge #1485

Johan-Liebert1 · 2025-08-05T14:21:49Z

Very initial iteration for /etc merge

gemini-code-assist

Code Review

This pull request introduces a new crate etc-merge for calculating differences between /etc directories. The initial implementation provides a compute_diff function and related helpers. My review focuses on correctness issues, such as a typo in a key data structure and an incomplete hashing function that misses file permissions. I've also pointed out an unused variable that indicates incomplete logic, and suggested improvements to test data and assertions for better maintainability.

crates/etc-merge/src/lib.rs

gemini-code-assist · 2025-08-05T14:23:14Z

crates/etc-merge/src/lib.rs

+        assert_eq!(res.added.len(), new_files.len());
+        assert!(res.added.iter().all(|file| new_files
+            .iter()
+            .find(|x| PathBuf::from(*x) == *file)
+            .is_some()));


This assertion logic is a bit complex. A simpler and more idiomatic way to compare two collections for equality regardless of order is to sort them and then compare the sorted collections. This pattern is repeated for modified and removed files as well.

Suggested change

assert_eq!(res.added.len(), new_files.len());

assert!(res.added.iter().all(|file| new_files

.iter()

.find(|x| PathBuf::from(*x) == *file)

.is_some()));

let mut res_added = res.added;

res_added.sort();

let mut expected_added: Vec<_> = new_files.iter().map(PathBuf::from).collect();

expected_added.sort();

assert_eq!(res_added, expected_added);

Johan-Liebert1 · 2025-08-05T14:32:43Z

I've tested the output of this implementation with ostree admin config-diff and it seems correct. But again, I haven't tested this exhaustively

p5 · 2025-08-05T14:35:05Z

crates/etc-merge/src/lib.rs

General question, not something specific about this PR (this looks great BTW!)

Would Bootc's ostree backend be refactored to make use of this crate in time, or is the plan for ostree to remain mostly separate? Obviously the implementation would be completely separate PR to this anyway.

This is a good question indeed. I've been really tempted to basically vendor+rewrite some of the ostree-sysroot stuff in this project. However, it has a ton of implications.

cgwalters

Thanks for working on this!

crates/etc-merge/src/lib.rs

cgwalters · 2025-08-05T22:55:16Z

crates/etc-merge/src/lib.rs

+            let mut buf = vec![0u8; entry.metadata()?.size() as usize];
+
+            let mut file = entry.open().context(format!("Opening entry {path:?}"))?;
+            file.read_exact(&mut buf)
+                .context(format!("Reading {path:?}. Buf: {buf:?}"))?;


Two key things here. First I think we should incremental hashing instead of reading the whole thing into memory and then hashing the memory buffer.

But second a nice optimization to make here is handling the case where the file has fsverity enabled. Then we can efficiently just query its fsverity hash to do a diff.

It won't be the default for files in a mutable /etc to have fsverity enabled but it makes sense to do so (dir is mutable, so the write-tmpfile-then-rename pattern continues to work) and we should make things faster for those who do use it.

crates/etc-merge/src/lib.rs

cgwalters · 2025-08-06T14:57:41Z

crates/etc-merge/src/lib.rs

+
+            path.pop();
+
+            // file has fs-verity enabled. We don't need to check the content/metadata


Note that fsverity only covers file contents, not metadata. It's totally allowed to do e.g. chmod or chown on such a file...which we probably want to have tracked as modified right?

Eh sorry...I guess maybe this fsverity suggestion is adding too much complexity at the moment for a non-default corner case.

Let's I guess just leave it as a // TODO consider also using fsverity here if available?

crates/etc-merge/src/lib.rs

Signed-off-by: Johan-Liebert1 <[email protected]>

Test for whether the file has fs-verity enabled or not, and if it does we simply check the verity. Incrementally compute hash for files rather than reading the entire file in memory. Signed-off-by: Johan-Liebert1 <[email protected]>

Signed-off-by: Johan-Liebert1 <[email protected]>

Merge added, modified, removed files from the current etc into the new etc directory, following the rules 1. If file is removed from current_etc, it will be removed from new_etc 2. If file is modified in current_etc, it will be copied to the new_etc overwriting any existing files 3. If a file is added in current_etc, then the above modification rule applies Modification includes change in content/permissions. Changed in Xattrs and/or ownership is not handled yet. Signed-off-by: Johan-Liebert1 <[email protected]>

Signed-off-by: Johan-Liebert1 <[email protected]>

Johan-Liebert1 · 2025-08-12T11:44:35Z

Hmm... tests failed because we can't chown unless we're root. And some issue with getxattr arguments

cgwalters · 2025-08-12T13:08:06Z

Hmm... tests failed because we can't chown unless we're root. And some issue with getxattr arguments

Yeah, some of this stuff really needs privileged integration tests, not unit tests.

In the short term what I think is easiest is to add an option to the core logic to ignore uid/gid + xattrs and that's what we do in unit tests.

For integration tests what we have in some other places is exposing the core functionality via bootc internals and then the tests can drive it, see e.g.

bootc/crates/lib/src/cli.rs

Line 1237 in 8ed1134

InternalsOpts::TestComposefs => {

(actually that's not so good an example because the command hardcodes the test, which bloats production binaries)
This one is maybe a bit better:

bootc/crates/lib/src/cli.rs

Line 1291 in 8ed1134

InternalsOpts::Relabel { as_path, path } => {

There's the tests-integration crate which is also intended to hold stuff like this.

Signed-off-by: Johan-Liebert1 <[email protected]>

cgwalters

Ugh I reviewed but forgot to click submit

cgwalters · 2025-08-12T12:56:53Z

Cargo.toml

-composefs = { git = "https://github.com/containers/composefs-rs", rev = "28d4721f77f973f0e394d60d6a69d9b39cb38d7f", package = "composefs", features = ["rhel9"] }
-composefs-boot = { git = "https://github.com/containers/composefs-rs", rev = "28d4721f77f973f0e394d60d6a69d9b39cb38d7f", package = "composefs-boot" }
-composefs-oci = { git = "https://github.com/containers/composefs-rs", rev = "28d4721f77f973f0e394d60d6a69d9b39cb38d7f", package = "composefs-oci" }
+composefs = { git = "https://github.com/containers/composefs-rs", rev = "8d1e570275621531d39b8f01681584bcc85ce01c", package = "composefs", features = ["rhel9"] }


In order to reduce conflicts let's do separate PRs for bumping crate dependencies.

Done in #1503

gemini-code-assist bot reviewed Aug 5, 2025

View reviewed changes

Johan-Liebert1 force-pushed the etc-merge branch from ede5599 to 37d6b19 Compare August 5, 2025 14:26

p5 reviewed Aug 5, 2025

View reviewed changes

cgwalters requested changes Aug 5, 2025

View reviewed changes

Johan-Liebert1 force-pushed the etc-merge branch from 37d6b19 to 3d6c8c3 Compare August 6, 2025 10:43

cgwalters reviewed Aug 6, 2025

View reviewed changes

cgwalters force-pushed the composefs-backend branch from 7d596fe to d9b384d Compare August 7, 2025 15:18

Johan-Liebert1 force-pushed the etc-merge branch 4 times, most recently from dc635c5 to 0faabae Compare August 11, 2025 10:22

cgwalters reviewed Aug 11, 2025

View reviewed changes

crates/etc-merge/src/lib.rs Outdated Show resolved Hide resolved

Johan-Liebert1 changed the title ~~Initial implementaion for /etc merge~~ Initial implementation for /etc merge Aug 11, 2025

Johan-Liebert1 added 8 commits August 12, 2025 16:21

Initial implementaion for /etc merge

ca2c3e8

Signed-off-by: Johan-Liebert1 <[email protected]>

etc-merge: Incremental hash computation + test verity

71a2fae

Test for whether the file has fs-verity enabled or not, and if it does we simply check the verity. Incrementally compute hash for files rather than reading the entire file in memory. Signed-off-by: Johan-Liebert1 <[email protected]>

Bump composefs-fs

a199bd0

Signed-off-by: Johan-Liebert1 <[email protected]>

etc-merge: Use generic-tree from composefs-rs

e781f44

Signed-off-by: Johan-Liebert1 <[email protected]>

Get removed files by traversing

ff83b65

Signed-off-by: Johan-Liebert1 <[email protected]>

etc-merge: Handle ownership changes

c226cf4

Signed-off-by: Johan-Liebert1 <[email protected]>

etc-merge: Handle xattrs

d527926

Signed-off-by: Johan-Liebert1 <[email protected]>

Johan-Liebert1 force-pushed the etc-merge branch 2 times, most recently from b2f05a1 to eafc75f Compare August 12, 2025 10:59

etc-merge: Ignore mtime while comparing stat

f1c87a7

Signed-off-by: Johan-Liebert1 <[email protected]>

Johan-Liebert1 force-pushed the etc-merge branch from eafc75f to f1c87a7 Compare August 12, 2025 11:00

Remove chown test

3e6d0f7

Signed-off-by: Johan-Liebert1 <[email protected]>

Johan-Liebert1 mentioned this pull request Aug 13, 2025

bootc composefs-native backend #1190

Open

12 tasks

cgwalters reviewed Aug 13, 2025

View reviewed changes

-        assert_eq!(res.added.len(), new_files.len());
-        assert!(res.added.iter().all(|file| new_files
-            .iter()
-            .find(|x| PathBuf::from(*x) == *file)
-            .is_some()));
+        let mut res_added = res.added;
+        res_added.sort();
+        let mut expected_added: Vec<_> = new_files.iter().map(PathBuf::from).collect();
+        expected_added.sort();
+        assert_eq!(res_added, expected_added);


		path.pop();

		// file has fs-verity enabled. We don't need to check the content/metadata

Initial implementation for /etc merge #1485

Are you sure you want to change the base?

Initial implementation for /etc merge #1485

Uh oh!

Conversation

Johan-Liebert1 commented Aug 5, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gemini-code-assist bot Aug 5, 2025

Choose a reason for hiding this comment

Uh oh!

Johan-Liebert1 commented Aug 5, 2025

Uh oh!

p5 Aug 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cgwalters Aug 5, 2025

Choose a reason for hiding this comment

Uh oh!

cgwalters left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

cgwalters Aug 5, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cgwalters Aug 6, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Johan-Liebert1 commented Aug 12, 2025

Uh oh!

cgwalters commented Aug 12, 2025

Uh oh!

cgwalters left a comment

Choose a reason for hiding this comment

Uh oh!

cgwalters Aug 12, 2025

Choose a reason for hiding this comment

Uh oh!

cgwalters Aug 13, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

p5 Aug 5, 2025 •

edited

Loading