validate-image-cache: drive eviction through Store.EvictUnused - #837
Open
igooch wants to merge 5 commits into
Open
validate-image-cache: drive eviction through Store.EvictUnused#837igooch wants to merge 5 commits into
igooch wants to merge 5 commits into
Conversation
igooch
force-pushed
the
imagecache-validate-tool
branch
2 times, most recently
from
August 11, 2026 15:59
cccc8e7 to
b0ead0b
Compare
igooch
added a commit
to igooch/substrate
that referenced
this pull request
Aug 11, 2026
…gent-substrate#837 @ b0ead0b) into the POC Brings the POC to parity with both open PRs in one merge (the tool branch stacks on the loop branch): ErrIncompleteEnumeration sentinel with engine-side gate logs dropped (single ERROR per gated pass, at the caller), classifyGCPass + imageCacheGC receiver with snapshotted flags, immediate first pass, honest CacheSize skip, negative-min-age rejection, cleaned-path BasePath check, and the tool driving eviction through the engine with WithActorsDir rooting live actors.
igooch
force-pushed
the
imagecache-validate-tool
branch
from
August 11, 2026 16:29
b0ead0b to
df0b81d
Compare
The loop that turns the eviction engine on. Every --image-cache-gc-period (default 5m; 0 disables), a serialized, panic-recovered pass measures the cache volume with statfs and the pool's recorded size, computes a byte target - down to --image-cache-low-percent when volume usage crossed --image-cache-high-percent, and/or down to --image-cache-max-bytes - capped at the pool's own size (the cache is one tenant of a shared volume; an uncapped target would evict everything chasing pressure it did not cause), and hands it to Store.EvictUnused. - Gated passes (unreadable records, garbled diffIDs, unenumerable root set) are handled before the shortfall accounting: zero stats plus an error means nothing was attempted, logged ERROR "pass skipped", never counted as a shortfall. - Genuine shortfall warns with backoff, then a periodic reminder: on a volume under foreign pressure, shortfall is the steady state and must not ERROR every tick. - atelet passes WithActorsDir(ateompath.ActorsDir) so the root set sees placed actors; ateompath gains the ActorsDir constant. - --image-cache-gc-dry-run computes and logs every decision, deleting nothing: the production soak mechanism. - A cache dir outside BasePath logs a warning (its watermarks would measure a different volume than actor state). - README: the GC section's "loop lands next" intro replaced with the loop and flag documentation. Defaults ship enabled (5m / 85% / 80%), matching kubelet so operator intuition transfers; dry-run is the opt-out soak. Target math and flag validation covered in imagegc_test.go.
Review fixes on the loop: - The engine exports ErrIncompleteEnumeration, wrapped into all four gate returns (EvictUnused and RecoverOrphans), and the loop detects a gated pass with errors.Is instead of inferring it from zero stats - the inference was correct today but nothing in the contract promised it, and a future error path before the candidate loop would have silently reclassified real failures as skips. - classifyGCPass extracts the skipped/shortfall/complete/quiet decision as a pure function with a table test, including the cases the old shape could not test: gated-vs-per-item errors, per-item errors with and without shortfall. - A CacheSize failure now skips the pass and says so; the old path set cacheSize=0, which the target cap silently drove to zero while the log claimed "watermark target only". - The first pass runs immediately instead of one full period after boot: a node starting under disk pressure should not wait 5 minutes (startup recovery reclaims debris, not pressure). - Stale comment dropped from the quiet branch (it cited an orphan branch and gauges that do not exist here); long doc comments on imageCacheGCTarget and the out-of-BasePath warning tightened.
- The engine's gates no longer log: a gated pass produced two ERROR lines per tick (engine + loop), and the loop's line carries the full attrs plus the wrapped error. New logs the startup scan's gate once, at ERROR (was WARN over a second engine ERROR). - Loop state moves onto an imageCacheGC receiver with flag values snapshotted at construction: runPass reads no globals (testable without flag juggling), and the observability phase adds its instruments as fields instead of more out-params. The pure decision functions stay free functions. - validateImageCacheGCFlags rejects a negative --image-cache-min-age (it inverts the veto: the cutoff lands in the future) and cleans the cache-dir path before the BasePath prefix check. - Comments: the ~1%-early watermark floor; why the pass runs even at target 0 (gate corruption should surface before disk pressure does). - Flag validation test: table with t.Cleanup and the legal boundary high=100/low=0.
Deletes the evictIfLow prototype - a pre-engine placeholder that removed the oldest layer trees by mtime with none of the engine's protections (no refcounts, no two-phase rename, no restore protocol), then dropped every manifest record to stay tidy. The tool now asks the engine to reclaim the free-space shortfall, so validation runs get the same semantics production will: record-driven refcounting, min-age (--evict-idle maps to WithMinAge), two-phase retirement, and proper record deletion. Also adds --evict-all: run one free-everything pass and exit, without a refs file or registry auth. This is the operator-facing home for "flush the cache now" (the MaxInt64 path), deliberately a one-shot tool command rather than a daemon flag. With no actors dir configured the tool's root set is empty by design - the corpus exercises refcounts, min-age, and restore-on-keep, not bundle-spec rooting, which the e2e suite owns.
Review: with no actors dir configured, --evict-all pointed at a live node's cache would retire lowerdirs under active mounts - a long-running actor's record is cold, so min-age is no backstop; the bundle spec is the only protection, and the tool was not reading it. Both constructors now pass WithActorsDir(ateompath.ActorsDir): on a live node placed actors root their images; on a validation host the dir does not exist, which InUse treats as a legitimate empty root set. Also per review: the package doc carries the live-node framing, and evictIfLow documents why the tool skips the daemon loop's pool-size cap (a dedicated validation disk just runs out of candidates). The branch now stacks on imagecache-gc-loop for ateompath.ActorsDir.
igooch
force-pushed
the
imagecache-validate-tool
branch
from
August 11, 2026 17:28
df0b81d to
bce9eab
Compare
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
1 participant
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.
Stacked on #836 (needs
ateompath.ActorsDir) — the first commits here are #836's; review the last two.Phase 2 of #463, following #735:
validate-image-cachenow drives eviction throughStore.EvictUnused, deleting its pre-engine prototype.What
evictIfLowdeleted. The prototype removed the oldest layer trees by mtime with none of the engine's protections — no refcounts, no two-phase rename, no restore protocol — then dropped every manifest record to stay tidy. The tool now asks the engine to reclaim the free-space shortfall below--min-free-gb, so corpus runs exercise exactly the semantics production nodes get.--evict-idlemaps to the store's min-age (WithMinAge) instead of a hand-rolled mtime check; its small-disk caveat moved into the flag description.--evict-alladded: one free-everything pass (theMaxInt64target) and exit — no refs file or registry auth needed. This is the operator-facing "flush the cache now", deliberately a one-shot tool command rather than a daemon flag; it also reclaims crash-debris orphans on the way, since it goes throughNew.WithActorsDir(ateompath.ActorsDir)(review fix): on a live node, placed actors' bundle specs root their images, so--evict-allis a safe flush; on a validation host the dir doesn't exist, whichInUsetreats as a legitimate empty root set. Corpus runs therefore exercise refcounts, min-age, and restore-on-keep, not bundle-spec rooting (the e2e suite owns that).evictIfLowdoesn't cap the target at pool size — on a dedicated validation disk an oversized target just runs out of candidates (documented at the call).Testing
--evict-allsmoke-tested against a scratch cache (engine pass visible in the tool's output, empty-pool no-op correct).