From 2869ce754bcfedd84b9ee1f6f73e76b46fb83ca5 Mon Sep 17 00:00:00 2001 From: Daniel Hast Date: Sun, 2 Aug 2026 23:17:47 -0400 Subject: [PATCH] feat: clamp xattr and bigfiles mtimes to most recent file mtime This solves a reproducibility issue with `bigfiles` and `xattr` components, which don't automatically have reproducible timestamps: Even if all the files in the component are given fixed modification timestamps, the timestamps of ancestor directories aren't clamped to match the files, which makes the layers non-reproducible unless `SOURCE_DATE_EPOCH` is set. To fix this, we clamp `bigfiles` components to the modification time of the single file, and clamp `xattr` components to the most recent modification time of a file with that xattr label. --- src/components/bigfiles.rs | 36 +++++++++++++------------- src/components/mod.rs | 10 +++----- src/components/xattr.rs | 50 +++++++++++++++++++------------------ tests/e2e/lib.sh | 33 ++++++++++++++++++++++++ tests/e2e/test-buildtime.sh | 19 +++++++++++--- 5 files changed, 96 insertions(+), 52 deletions(-) diff --git a/src/components/bigfiles.rs b/src/components/bigfiles.rs index 6a4e523..f9f30ce 100644 --- a/src/components/bigfiles.rs +++ b/src/components/bigfiles.rs @@ -3,7 +3,7 @@ use std::collections::HashMap; use anyhow::Result; use camino::{Utf8Path, Utf8PathBuf}; use cap_std_ext::cap_std::fs::Dir; -use indexmap::IndexSet; +use indexmap::IndexMap; use super::{ComponentId, ComponentInfo, ComponentsRepo, FileMap, FileType}; @@ -25,12 +25,10 @@ const REPO_NAME: &str = "bigfiles"; /// Some special handling for hardlinked files (same inode); we still want /// unclaimed files that are hardlinked to end up in the same component. pub struct BigfilesRepo { - /// Component names, indexed by ComponentId. - components: IndexSet, + /// Map from component names to modification times, indexed by ComponentId. + components: IndexMap, /// Mapping from path to ComponentId. path_to_component: HashMap, - /// Default mtime clamp for components. - default_mtime_clamp: u64, } impl BigfilesRepo { @@ -41,8 +39,8 @@ impl BigfilesRepo { // 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, default_mtime_clamp: u64) -> Option { - let mut components: IndexSet = IndexSet::new(); + pub fn load(files: &FileMap) -> Option { + let mut components: IndexMap = IndexMap::new(); let mut path_to_component: HashMap = HashMap::new(); // build inode table for hardlink handling @@ -72,7 +70,7 @@ impl BigfilesRepo { .expect("filename has no basename"); // derive component name from filename - let component_name = if components.contains(&filename) { + let component_name = if components.contains_key(&filename) { // filename already used, use full path without leading '/' path.strip_prefix("/") .expect("non-absolute file path in FileMap") @@ -83,7 +81,7 @@ impl BigfilesRepo { }; // create a component for this path - let (idx, _) = components.insert_full(component_name); + let (idx, _) = components.insert_full(component_name, file_info.mtime); tracing::trace!(path = %path, component = %components[idx], id = idx, "bigfiles component created"); let component_id = ComponentId(idx); path_to_component.insert(path.clone(), component_id); @@ -109,7 +107,6 @@ impl BigfilesRepo { Some(Self { components, path_to_component, - default_mtime_clamp, }) } } @@ -137,14 +134,15 @@ impl ComponentsRepo for BigfilesRepo { } fn component_info(&self, id: ComponentId) -> ComponentInfo<'_> { + let (name, &mtime_clamp) = 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"); ComponentInfo { - name: self - .components - .get_index(id.0) - // SAFETY: the ids we're given come from the IndexSet itself - // when we inserted the element, so it must be valid. - .expect("invalid ComponentId"), - mtime_clamp: self.default_mtime_clamp, + name, + mtime_clamp, stability: 0.0, } } @@ -223,7 +221,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, 12345).unwrap(); + let repo = BigfilesRepo::load(&files).unwrap(); // small file should not be claimed let claims = repo @@ -271,7 +269,7 @@ mod tests { create_sparse_file(rootfs, "b/foobar", 4 * 1024 * 1024); }); - let repo = BigfilesRepo::load(&files, 0).unwrap(); + let repo = BigfilesRepo::load(&files).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 284e16c..7e3e02b 100644 --- a/src/components/mod.rs +++ b/src/components/mod.rs @@ -116,9 +116,7 @@ impl ComponentsRepos { 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, default_mtime_clamp).context("loading xattrs")? - { + if let Some(repo) = xattr::XattrRepo::load(files).context("loading xattrs")? { tracing::info!(repo = "xattr", "loaded repo"); repos.push(Box::new(repo)); } @@ -137,7 +135,7 @@ impl ComponentsRepos { repos.push(Box::new(repo)); } - if let Some(repo) = bigfiles::BigfilesRepo::load(files, default_mtime_clamp) { + if let Some(repo) = bigfiles::BigfilesRepo::load(files) { tracing::info!(repo = "bigfiles", "loaded repo"); repos.push(Box::new(repo)); } @@ -412,7 +410,7 @@ mod tests { let files = crate::scan::Scanner::new(&rootfs).scan().unwrap(); - let xattr_repo = xattr::XattrRepo::load(&files, 0).unwrap().unwrap(); + let xattr_repo = xattr::XattrRepo::load(&files).unwrap().unwrap(); let packages = rpm_qa::load_from_str(RPM_FIXTURE).unwrap(); let now = std::time::SystemTime::now() .duration_since(std::time::UNIX_EPOCH) @@ -480,7 +478,7 @@ mod tests { let files = crate::scan::Scanner::new(&rootfs).scan().unwrap(); - let xattr_repo = xattr::XattrRepo::load(&files, 0).unwrap().unwrap(); + let xattr_repo = xattr::XattrRepo::load(&files).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 e7373e4..13cba38 100644 --- a/src/components/xattr.rs +++ b/src/components/xattr.rs @@ -2,7 +2,7 @@ use std::collections::HashMap; use anyhow::{Context, Result}; use camino::{Utf8Path, Utf8PathBuf}; -use indexmap::IndexSet; +use indexmap::IndexMap; use super::{ ComponentId, ComponentInfo, ComponentsRepo, FileInfo, FileMap, FileType, STABILITY_PERIOD_DAYS, @@ -19,24 +19,20 @@ const REPO_NAME: &str = "xattr"; /// Directories with this xattr apply to all files underneath unless overridden. /// Directory inheritance is pre-computed during load. pub struct XattrRepo { - /// Component names, indexed by ComponentId. - components: IndexSet, + /// Map from component names to modification times, indexed by ComponentId. + components: IndexMap, /// Mapping from path to ComponentId (pre-computed with inheritance). path_to_component: HashMap, /// Per-component stability, indexed by ComponentId. component_stability: Vec, - /// Currently, the on-disk mtime is canonical and we clamp it, but it would - /// make sense in the future to support another user xattr to specify a - /// canonical mtime for easier layer reproducibility. - default_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, default_mtime_clamp: u64) -> Result> { - let mut components: IndexSet = IndexSet::new(); + pub fn load(files: &FileMap) -> Result> { + let mut components: IndexMap = IndexMap::new(); let mut path_to_component: HashMap = HashMap::new(); // Track raw intervals during scanning to detect conflicts let mut update_intervals: Vec> = Vec::new(); @@ -63,7 +59,7 @@ impl XattrRepo { // https://github.com/indexmap-rs/indexmap/issues/355 or // https://github.com/indexmap-rs/indexmap/issues/388 let idx = components.get_index_of(name).unwrap_or_else(|| { - let idx = components.insert_full(name.clone()).0; + let idx = components.insert_full(name.clone(), file_info.mtime).0; update_intervals.push(None); tracing::trace!(path = %path, name = %name, id = idx, "xattr component created"); idx @@ -84,6 +80,12 @@ impl XattrRepo { if let Some(id) = effective_id { tracing::trace!(path = %path, component_id = id.0, "xattr assignment"); path_to_component.insert(path.clone(), id); + let (_, component_mtime) = components + .get_index_mut(id.0) + .expect("xattr component should have already been created"); + // This ultimately makes the component mtime equal to the most + // recent modification time of a file in the component. + *component_mtime = file_info.mtime.max(*component_mtime); // Check for user.update-interval xattr (not inherited from directories) if let Some(interval) = get_update_interval_xattr(file_info) @@ -153,7 +155,6 @@ impl XattrRepo { components, path_to_component, component_stability, - default_mtime_clamp, })) } } @@ -233,14 +234,15 @@ impl ComponentsRepo for XattrRepo { } fn component_info(&self, id: ComponentId) -> ComponentInfo<'_> { + let (name, &mtime_clamp) = 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"); ComponentInfo { - name: self - .components - .get_index(id.0) - // SAFETY: the ids we're given come from the IndexSet itself - // when we inserted the element, so it must be valid. - .expect("invalid ComponentId"), - mtime_clamp: self.default_mtime_clamp, + name, + mtime_clamp, stability: self.component_stability[id.0], } } @@ -313,7 +315,7 @@ mod tests { // File without xattr outside of directory - should not be claimed rootfs.write("noattr", "content").unwrap(); }); - let repo = XattrRepo::load(&files, 0).unwrap().unwrap(); + let repo = XattrRepo::load(&files).unwrap().unwrap(); // /mydir and /mydir/normal should be dircomponent assert_component(&repo, "/mydir", FileType::Directory, "dircomponent"); @@ -343,7 +345,7 @@ mod tests { set_component(rootfs, "a/b/c/d", "compD"); set_component(rootfs, "x", "compX"); }); - let repo = XattrRepo::load(&files, 0).unwrap().unwrap(); + let repo = XattrRepo::load(&files).unwrap().unwrap(); assert_component(&repo, "/a", FileType::Directory, "compA"); assert_component(&repo, "/a/other", FileType::File, "compA"); // inherits from /a @@ -364,7 +366,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, 0).unwrap().unwrap(); + let repo = XattrRepo::load(&files).unwrap().unwrap(); // Both should be claimed by mycomp assert_component(&repo, "/mydir", FileType::Directory, "mycomp"); @@ -400,7 +402,7 @@ mod tests { rootfs.write("mydir/file", "content").unwrap(); set_update_interval(rootfs, "mydir/file", "60"); }); - let result = XattrRepo::load(&files, 0); + let result = XattrRepo::load(&files); assert!(result.is_err()); let msg = format!("{:#}", result.err().unwrap()); assert!(msg.contains("conflicting"), "unexpected error: {msg}"); @@ -415,7 +417,7 @@ mod tests { rootfs.write("mydir/file", "content").unwrap(); set_update_interval(rootfs, "mydir/file", "30"); }); - let repo = XattrRepo::load(&files, 0).unwrap().unwrap(); + let repo = XattrRepo::load(&files).unwrap().unwrap(); let claims = repo.strong_claims_for_path(Utf8Path::new("/mydir"), &fi(FileType::Directory)); let expected = interval_to_stability(30); @@ -435,7 +437,7 @@ mod tests { set_component(rootfs, "app2", "comp2"); rootfs.write("app2/file", "content").unwrap(); }); - let repo = XattrRepo::load(&files, 0).unwrap().unwrap(); + let repo = XattrRepo::load(&files).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 37b69a7..7767fe5 100644 --- a/tests/e2e/lib.sh +++ b/tests/e2e/lib.sh @@ -48,6 +48,39 @@ assert_layer_count() { fi } +# Get digest of layer with specific component name in image +get_image_component_digest() { + local component="$1" + local image="$2" + local digest + digest="$(skopeo inspect -n "containers-storage:${image}" \ + | jq -r --arg component "${component}" ' + .LayersData[] + | select(.Annotations."org.chunkah.component" == $component) + | .Digest + ')" + if [[ -z "${digest}" ]]; then + echo "ERROR: Layer with component '${component}' not found in image ${image}" >&2 + exit 1 + fi + printf '%s' "${digest}" +} + +# Assert layer with given component name is unmodified between images +assert_layer_unmodified() { + local component="$1" + local image1="$2" + local image2="$3" + local digest1 digest2 + digest1=$(get_image_component_digest "${component}" "${image1}") || exit 1 + digest2=$(get_image_component_digest "${component}" "${image2}") || exit 1 + if [[ "${digest1}" != "${digest2}" ]]; then + echo "ERROR: Digests of layer with component '${component}' differ between ${image1} and ${image2}:" >&2 + printf '%s != %s\n' "${digest1}" "${digest2}" >&2 + exit 1 + fi +} + # Assert that a path exists in an image. assert_path_exists() { local image="${1}"; shift diff --git a/tests/e2e/test-buildtime.sh b/tests/e2e/test-buildtime.sh index a245212..4fc6fbb 100755 --- a/tests/e2e/test-buildtime.sh +++ b/tests/e2e/test-buildtime.sh @@ -11,6 +11,7 @@ BASE_IMAGE="quay.io/fedora/fedora-minimal:latest" ROOTFS_IMAGE="localhost/test-buildtime-rootfs:latest" CHUNKED_IMAGE="localhost/fedora-minimal-chunked:test" CHUNKED_IMAGE2="localhost/fedora-minimal-chunked:test2" +CHUNKED_IMAGE3="localhost/fedora-minimal-chunked:test3" cleanup() { cleanup_images "${CHUNKED_IMAGE}" "${CHUNKED_IMAGE2}" "${ROOTFS_IMAGE}" @@ -22,9 +23,14 @@ podman pull "${BASE_IMAGE}" # Build rootfs as a separate image so that the reproducibility check below # operates on the same rootfs both times. Without this, each buildah_build # would re-run dnf install, producing different file timestamps. +# +# Also create a large random file used to test reproducibility of bigfiles +# components. cat > Containerfile.rootfs <