fix(stamps): hash overlay contents for verbatim overlays too - #210
Draft
nicksinas wants to merge 2 commits into
Draft
fix(stamps): hash overlay contents for verbatim overlays too#210nicksinas wants to merge 2 commits into
nicksinas wants to merge 2 commits into
Conversation
An overlay is applied to the sysroot by `cp`, not RPM, so its file
contents must be folded into the install stamp — otherwise editing an
overlay file leaves the stamp current, `avocado build` proceeds, and the
stale sysroot ships the old file with no warning (ENG-2440).
The content digest already existed but was gated to overlays that opt
into preprocessing; verbatim overlays hashed only the config value. Drop
the gate so content is always hashed (raw bytes for verbatim, post-{{ }}
for preprocessed), and route the overlay dir through the shared
parse_overlay_config so a bare-string overlay hashes the right tree.
There was a problem hiding this comment.
Pull request overview
This PR fixes install-stamp under-invalidation for overlays by ensuring overlay file contents are always included in the stamp input hash (not only when preprocess: is enabled). This prevents avocado build from silently producing images with stale overlay content after an overlay/ file edit.
Changes:
- Always fold
overlay_content_digestinto stamp hashes for overlays (verbatim overlays hash raw bytes; preprocessed overlays hash rendered content). - Centralize overlay config parsing (
overlay:string vs mapping) via a sharedparse_overlay_config. - Update/add tests to assert verbatim and bare-string overlays invalidate hashes when files change.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/utils/stamps.rs |
Always folds overlay tree digest into stamp inputs; adds/updates tests for verbatim + bare-string overlays. |
src/utils/overlay_preprocess.rs |
Makes overlay content digest apply to verbatim overlays too; adds shared overlay parser; updates tests. |
src/commands/rootfs/install.rs |
Uses the shared parse_overlay_config instead of a duplicated local parser. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+195
to
+202
| /// Compute a deterministic digest of the overlay tree, without materializing it | ||
| /// to disk. Preprocessing is applied per `spec`: a verbatim overlay | ||
| /// ([`PreprocessSpec::None`]) hashes raw file bytes, an enabled one hashes the | ||
| /// post-`{{ }}` content (so a changed template value — e.g. a new claim token — | ||
| /// also moves the digest). Returns `None` only when the overlay dir does not | ||
| /// exist. Folded into the rootfs/initramfs/ext build input hashes so any edit | ||
| /// to an overlay file forces a rebuild (ENG-2440); only the SHA-256 is retained | ||
| /// — never the plaintext. |
Now that the content digest runs for verbatim overlays too, routing every path through rel_str made a non-UTF-8 filename hard-fail the build — a regression, since `cp -a` copies such a name fine and it built before. Hash the path from raw bytes (rel_bytes/os_bytes, lossless) so a verbatim overlay digests without demanding UTF-8. rel_str is now reached only by the preprocessing paths (glob matching + staging), which genuinely need UTF-8 — so its "scope/disable preprocess" error message is accurate again (addresses the PR review). For UTF-8 names the digest bytes are unchanged, so existing stamps don't invalidate.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes ENG-2440.
Problem
Edit a file under your
overlay/dir, runavocado build, deploy — and the built image still contains the old file. No warning, no error.avocado installfirst fixes it, which is why the habitualavocado install && avocado buildmasks it.Root cause
overlay:is applied to the sysroot bycp(in install), not by RPM. The install stamp is a prerequisite gate, not a work-skipping cache:avocado buildonly validates that the stamp exists/isn't stale, it never re-runs install. So an under-invalidating stamp doesn't cause redundant work — it causes silently wrong output.The overlay content digest (sorted path + mode + content-sha256) already existed in
overlay_preprocess::overlay_content_digest, butfold_overlay_content_hashgated it behindPreprocessSpec::is_enabled()— so a verbatim overlay (the common case, nopreprocess:) hashed only the config value (dir name + mode), never file contents. Editing an overlay file left the stamp current.Fix
is_enabled()gates inoverlay_content_digestandfold_overlay_content_hashso the content digest is always folded in — raw bytes for verbatim overlays (the existingprocess_file_bytesreturns raw when nothing is selected for preprocessing), post-{{ }}content for preprocessed ones. All three call sites (rootfs, initramfs, ext-build) already route throughfold_overlay_content_hash, so all are fixed at once.parse_overlay_configintooverlay_preprocess.rsas the single shared parser, so a bare-string overlay (overlay: mydir) hashes the right tree instead of the"overlay"default.Now editing an overlay file makes the stamp stale and
avocado buildtells the user to re-run install — exactly as it already does for apost_installedit.Note on the sibling ticket
Even once the file reaches the image,
AVOCADO_OS_BUILD_IDwas blind to it (acpnever touches the rpmdb), so the OTA gate no-ops — same class as ENG-2437. That's fixed generally in ENG-2441 (build id from the assembled work tree), not here.Tests
rootfs_verbatim_overlay_ignores_file_contents→…_hashes_file_contents; the "disabled yields no digest" assertion → verbatim digest tracks raw bytes).cargo fmt/clippyclean on the changed files.Out of scope (noted in the ticket)
overlay/doesn't remove it from the persistent sysroot (needs a clean or opaque mode). No hashing scheme fixes this.compute_runtime_build_input_hashis write-only (deploy validates existence only) — separate decision.