From e6e3606e712792ad8b4d3008e943cac31993be67 Mon Sep 17 00:00:00 2001 From: Jonathan Lebon Date: Wed, 30 Sep 2026 13:09:17 -0400 Subject: [PATCH] clamp: fix clamp handling for xattr/bigfiles repos Commit 2869ce7 made xattr and bigfiles components clamp their mtimes to file mtimes instead of the resolved created epoch, which fixed the reproducibility of ancestor directories when file mtimes are canonical (i.e. were explicitly set to a meaningful, fixed, value) but an epoch wasn't explicitly set. However, this breaks the case where an epoch _is_ explicitly passed. The problem is that the file mtime may be arbitrary (e.g. if it's just created with the current time at image build time, like the rpmdb), which thus renders the image non-reproducible (even though SOURCE_DATE_EPOCH has been passed!). I think what we want is actually the `min()` of the file mtime and the resolved epoch. That way, (1) if an SDE is passed in, and no mtime is explicitly set on a file created at build time, it's reasonable to expect it to be more recent than SDE and thus gets capped, but (2) if no SDE is passed, then it defaults to $now, and it's reasonable to expect any explicit mtime set on a bigfiles for reproducibility reasons to be before $now, and thus the file mtime wins. In both cases, we're basically picking the timestamp most likely to be stable. This was actually part of an earlier version of PR #161, but I mistakenly in review claimed that we could drop it: https://github.com/coreos/chunkah/pull/161#discussion_r4037740157 Fixes: #176 Assisted-by: AI --- src/components/bigfiles.rs | 25 ++++++++++++++++++++----- src/components/mod.rs | 20 +++++++++++++++----- src/components/xattr.rs | 26 ++++++++++++++++++-------- tests/e2e/lib.sh | 13 +++++++++++++ tests/e2e/test-buildtime.sh | 16 +++++++++++++--- 5 files changed, 79 insertions(+), 21 deletions(-) diff --git a/src/components/bigfiles.rs b/src/components/bigfiles.rs index f9f30ce..3c76e5b 100644 --- a/src/components/bigfiles.rs +++ b/src/components/bigfiles.rs @@ -29,17 +29,21 @@ pub struct BigfilesRepo { components: IndexMap, /// Mapping from path to ComponentId. path_to_component: HashMap, + /// Cap for component mtime clamps. + mtime_clamp: u64, } impl BigfilesRepo { /// Load bigfiles repo by scanning for files >= MIN_SIZE. /// /// Returns None if no qualifying files are found. Hardlinked files (same - /// inode, nlink > 1) are grouped into the same component. + /// inode, nlink > 1) are grouped into the same component. Component mtime + /// clamps are set to the file's mtime so that ancestor dirs are clamped to + /// a reproducible value, capped at `mtime_clamp`. // TODO: the upfront scan logic here (inode table, path_to_component map) // could be deferred to weak_claims_for_path time since it receives // &FileInfo with size/inode/nlink. - pub fn load(files: &FileMap) -> Option { + pub fn load(files: &FileMap, mtime_clamp: u64) -> Option { let mut components: IndexMap = IndexMap::new(); let mut path_to_component: HashMap = HashMap::new(); @@ -107,6 +111,7 @@ impl BigfilesRepo { Some(Self { components, path_to_component, + mtime_clamp, }) } } @@ -134,12 +139,22 @@ impl ComponentsRepo for BigfilesRepo { } fn component_info(&self, id: ComponentId) -> ComponentInfo<'_> { - let (name, &mtime_clamp) = self + let (name, &mtime) = self .components .get_index(id.0) // SAFETY: the ids we're given come from the IndexMap itself // when we inserted the element, so it must be valid. .expect("invalid ComponentId"); + // There's no canonical mtime for bigfiles (unlike e.g. the RPM build time): the file's + // mtime is the only signal, but it may be entirely arbitrary, as there's no way to know if + // the user set it explicitly for reproducibility. So we always take the min() with the + // default clamp (which comes from SOURCE_DATE_EPOCH or defaults to $now): + // - with an explicit epoch, arbitrary file mtimes (e.g. "state" files like the rpmdb) get + // clamped down, restoring reproducibility; + // - without one, the clamp defaults to $now; file mtimes are necessarily at or before $now + // (the rootfs predates chunking), so explicitly-set mtimes win, and arbitrary ones float + // but were never reproducible to begin with. + let mtime_clamp = mtime.min(self.mtime_clamp); ComponentInfo { name, mtime_clamp, @@ -221,7 +236,7 @@ mod tests { let rootfs = Dir::open_ambient_dir(tmp.path(), ambient_authority()).unwrap(); let files = crate::scan::Scanner::new(&rootfs).scan().unwrap(); - let repo = BigfilesRepo::load(&files).unwrap(); + let repo = BigfilesRepo::load(&files, 0).unwrap(); // small file should not be claimed let claims = repo @@ -269,7 +284,7 @@ mod tests { create_sparse_file(rootfs, "b/foobar", 4 * 1024 * 1024); }); - let repo = BigfilesRepo::load(&files).unwrap(); + let repo = BigfilesRepo::load(&files, 0).unwrap(); // First one uses filename, second uses full path assert_component(&repo, &rootfs, "/a/foobar", "foobar"); diff --git a/src/components/mod.rs b/src/components/mod.rs index 7e3e02b..5e4c3ba 100644 --- a/src/components/mod.rs +++ b/src/components/mod.rs @@ -35,6 +35,12 @@ pub struct ComponentsRepos { pub struct Component { /// The maximum mtime for files in this component during the build phase. /// File mtimes will be clamped to this value. + /// + /// The clamp is what makes layers reproducible: package repos use a + /// canonical mtime (e.g. the RPM build time), which anchors the whole + /// layer regardless of when it was written. Repos without a canonical + /// mtime (bigfiles, xattr) fall back to the most recent file mtime, + /// capped by the default clamp. pub mtime_clamp: u64, /// Probability that the component doesn't change over STABILITY_PERIOD_DAYS. /// Used by the packing algorithm. @@ -112,11 +118,15 @@ impl ComponentsRepos { /// The `files` map is the set of paths in the rootfs. This avoids the xattr /// repo having to walk the rootfs again. The `default_mtime_clamp` will be /// used as the mtime clamp for components that don't have a reproducible - /// clamp (e.g. xattr-claimed files, unclaimed files). + /// clamp (e.g. unclaimed files), as the stability anchor for package + /// repos, and to cap the mtime clamps of repos without canonical mtimes + /// (e.g. big files, xattr-claimed files). pub fn load(rootfs: &Dir, files: &FileMap, default_mtime_clamp: u64) -> Result { let mut repos: Vec> = Vec::new(); - if let Some(repo) = xattr::XattrRepo::load(files).context("loading xattrs")? { + if let Some(repo) = + xattr::XattrRepo::load(files, default_mtime_clamp).context("loading xattrs")? + { tracing::info!(repo = "xattr", "loaded repo"); repos.push(Box::new(repo)); } @@ -135,7 +145,7 @@ impl ComponentsRepos { repos.push(Box::new(repo)); } - if let Some(repo) = bigfiles::BigfilesRepo::load(files) { + if let Some(repo) = bigfiles::BigfilesRepo::load(files, default_mtime_clamp) { tracing::info!(repo = "bigfiles", "loaded repo"); repos.push(Box::new(repo)); } @@ -410,7 +420,7 @@ mod tests { let files = crate::scan::Scanner::new(&rootfs).scan().unwrap(); - let xattr_repo = xattr::XattrRepo::load(&files).unwrap().unwrap(); + let xattr_repo = xattr::XattrRepo::load(&files, 0).unwrap().unwrap(); let packages = rpm_qa::load_from_str(RPM_FIXTURE).unwrap(); let now = std::time::SystemTime::now() .duration_since(std::time::UNIX_EPOCH) @@ -478,7 +488,7 @@ mod tests { let files = crate::scan::Scanner::new(&rootfs).scan().unwrap(); - let xattr_repo = xattr::XattrRepo::load(&files).unwrap().unwrap(); + let xattr_repo = xattr::XattrRepo::load(&files, 0).unwrap().unwrap(); let repos: Vec> = vec![Box::new(xattr_repo)]; let loaded = ComponentsRepos { repos, diff --git a/src/components/xattr.rs b/src/components/xattr.rs index 13cba38..d16236d 100644 --- a/src/components/xattr.rs +++ b/src/components/xattr.rs @@ -25,13 +25,19 @@ pub struct XattrRepo { path_to_component: HashMap, /// Per-component stability, indexed by ComponentId. component_stability: Vec, + /// Cap for component mtime clamps. + mtime_clamp: u64, } impl XattrRepo { /// Load xattr repo by scanning rootfs for user.component xattrs. /// Pre-computes directory inheritance for all paths in `files`. /// Uses cached xattrs from FileInfo rather than reading from disk. - pub fn load(files: &FileMap) -> Result> { + /// + /// Component mtime clamps are set to the most recent file mtime in the + /// component, so that ancestor dirs are clamped to a reproducible value, + /// capped at `mtime_clamp`. + pub fn load(files: &FileMap, mtime_clamp: u64) -> Result> { let mut components: IndexMap = IndexMap::new(); let mut path_to_component: HashMap = HashMap::new(); // Track raw intervals during scanning to detect conflicts @@ -155,6 +161,7 @@ impl XattrRepo { components, path_to_component, component_stability, + mtime_clamp, })) } } @@ -234,12 +241,15 @@ impl ComponentsRepo for XattrRepo { } fn component_info(&self, id: ComponentId) -> ComponentInfo<'_> { - let (name, &mtime_clamp) = self + let (name, &mtime) = self .components .get_index(id.0) // SAFETY: the ids we're given come from the IndexMap itself // when we inserted the element, so it must be valid. .expect("invalid ComponentId"); + // Same reasoning as bigfiles: no canonical mtime for xattr components + // and file mtimes may be arbitrary, so cap with the default clamp. + let mtime_clamp = mtime.min(self.mtime_clamp); ComponentInfo { name, mtime_clamp, @@ -315,7 +325,7 @@ mod tests { // File without xattr outside of directory - should not be claimed rootfs.write("noattr", "content").unwrap(); }); - let repo = XattrRepo::load(&files).unwrap().unwrap(); + let repo = XattrRepo::load(&files, 0).unwrap().unwrap(); // /mydir and /mydir/normal should be dircomponent assert_component(&repo, "/mydir", FileType::Directory, "dircomponent"); @@ -345,7 +355,7 @@ mod tests { set_component(rootfs, "a/b/c/d", "compD"); set_component(rootfs, "x", "compX"); }); - let repo = XattrRepo::load(&files).unwrap().unwrap(); + let repo = XattrRepo::load(&files, 0).unwrap().unwrap(); assert_component(&repo, "/a", FileType::Directory, "compA"); assert_component(&repo, "/a/other", FileType::File, "compA"); // inherits from /a @@ -366,7 +376,7 @@ mod tests { // Create a symlink inside the directory - it should inherit from parent rootfs.symlink("../somewhere", "mydir/link").unwrap(); }); - let repo = XattrRepo::load(&files).unwrap().unwrap(); + let repo = XattrRepo::load(&files, 0).unwrap().unwrap(); // Both should be claimed by mycomp assert_component(&repo, "/mydir", FileType::Directory, "mycomp"); @@ -402,7 +412,7 @@ mod tests { rootfs.write("mydir/file", "content").unwrap(); set_update_interval(rootfs, "mydir/file", "60"); }); - let result = XattrRepo::load(&files); + let result = XattrRepo::load(&files, 0); assert!(result.is_err()); let msg = format!("{:#}", result.err().unwrap()); assert!(msg.contains("conflicting"), "unexpected error: {msg}"); @@ -417,7 +427,7 @@ mod tests { rootfs.write("mydir/file", "content").unwrap(); set_update_interval(rootfs, "mydir/file", "30"); }); - let repo = XattrRepo::load(&files).unwrap().unwrap(); + let repo = XattrRepo::load(&files, 0).unwrap().unwrap(); let claims = repo.strong_claims_for_path(Utf8Path::new("/mydir"), &fi(FileType::Directory)); let expected = interval_to_stability(30); @@ -437,7 +447,7 @@ mod tests { set_component(rootfs, "app2", "comp2"); rootfs.write("app2/file", "content").unwrap(); }); - let repo = XattrRepo::load(&files).unwrap().unwrap(); + let repo = XattrRepo::load(&files, 0).unwrap().unwrap(); // comp1 should have yearly stability let claims = repo.strong_claims_for_path(Utf8Path::new("/app1"), &fi(FileType::Directory)); diff --git a/tests/e2e/lib.sh b/tests/e2e/lib.sh index 7767fe5..290ccfc 100644 --- a/tests/e2e/lib.sh +++ b/tests/e2e/lib.sh @@ -91,6 +91,19 @@ assert_path_exists() { fi } +# Assert that a path in an image has the given mtime (epoch seconds). +assert_path_mtime() { + local image="${1}"; shift + local path="${1}"; shift + local expected="${1}"; shift + local actual + actual=$(podman run --rm "${image}" stat -c '%Y' "${path}") + if [[ "${actual}" != "${expected}" ]]; then + echo "ERROR: mtime of ${path} in ${image} is ${actual}, expected ${expected}" >&2 + return 1 + fi +} + # Assert that a path does not exist in an image. assert_path_not_exists() { local image="${1}"; shift diff --git a/tests/e2e/test-buildtime.sh b/tests/e2e/test-buildtime.sh index 4fc6fbb..3345361 100755 --- a/tests/e2e/test-buildtime.sh +++ b/tests/e2e/test-buildtime.sh @@ -29,14 +29,16 @@ podman pull "${BASE_IMAGE}" cat > Containerfile.rootfs < Containerfile <