From 3c8a2587a423717b28b0ad2eb6982de6de9cd294 Mon Sep 17 00:00:00 2001 From: Ivy Date: Mon, 10 Aug 2026 14:45:36 -0700 Subject: [PATCH 01/10] validate-image-cache: drive eviction through Store.EvictUnused 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. --- tools/validate-image-cache/main.go | 99 ++++++++++++++---------------- 1 file changed, 47 insertions(+), 52 deletions(-) diff --git a/tools/validate-image-cache/main.go b/tools/validate-image-cache/main.go index 3800611d9..948c9f705 100644 --- a/tools/validate-image-cache/main.go +++ b/tools/validate-image-cache/main.go @@ -26,9 +26,13 @@ // // gcloud artifacts docker images list REPO --format="value[separator='@'](package,version)" // -// Disk is bounded by evicting the oldest cached layers when the cache -// volume's free space drops below --min-free-gb. Only layers idle for more -// than 30 minutes are evicted, so in-flight images are never raced. +// Disk is bounded by the cache's own eviction engine: when the cache +// volume's free space drops below --min-free-gb, the tool asks +// Store.EvictUnused to reclaim the difference — LRU by image last-use, +// with in-flight pulls protected by record refcounts and layers younger +// than --evict-idle never touched. --evict-all instead empties everything +// evictable and exits (operator use: flush a cache without deleting the +// directory). package main import ( @@ -38,10 +42,9 @@ import ( "flag" "fmt" "log" + "math" "math/rand" "os" - "path/filepath" - "sort" "strconv" "strings" "sync" @@ -61,8 +64,9 @@ var ( outCSV = flag.String("out", "validate-results.csv", "Results CSV path") parallel = flag.Int("parallel", 3, "Images validated concurrently (each pulls up to 4 layers in parallel)") timeout = flag.Duration("timeout", 20*time.Minute, "Per-image timeout") - minFreeGB = flag.Uint64("min-free-gb", 150, "Evict oldest idle cached layers when the cache volume has less free space than this") - evictIdle = flag.Duration("evict-idle", 10*time.Minute, "Only evict layers idle for at least this long (must exceed the time any in-flight image needs a just-unpacked layer; small disks + high throughput need small values)") + minFreeGB = flag.Uint64("min-free-gb", 150, "Ask the eviction engine to reclaim disk when the cache volume has less free space than this") + evictIdle = flag.Duration("evict-idle", 10*time.Minute, "Eviction min-age: layers and records younger than this are never evicted. NOTE: if the corpus unpacks faster than this window elapses on a small disk, nothing is evictable while the disk fills — size it well below disk-fill time") + evictAll = flag.Bool("evict-all", false, "Evict everything evictable from the cache and exit (no refs file needed)") platform = flag.String("platform", "linux/amd64", "Image platform to pull") ) @@ -76,12 +80,28 @@ type result struct { func main() { flag.Parse() - if *refsFile == "" || *cacheDir == "" { + if *cacheDir == "" || (*refsFile == "" && !*evictAll) { flag.Usage() os.Exit(2) } ctx := context.Background() + if *evictAll { + // Flush mode: no refs, no registry auth. New also reclaims any + // crash-debris orphans before the pass. + store, err := imagecache.New(*cacheDir, imagecache.WithMinAge(*evictIdle)) + if err != nil { + log.Fatalf("opening cache: %v", err) + } + stats, err := store.EvictUnused(ctx, math.MaxInt64, false) + if err != nil { + log.Fatalf("evict-all: %v", err) + } + log.Printf("evict-all: %d images / %d layers evicted, %.1f GB credited (free now %.0f GB)", + stats.EvictedImages, stats.EvictedLayers, float64(stats.FreedBytes)/1e9, float64(freeBytes(*cacheDir))/1e9) + return + } + refs, err := loadRefs(*refsFile) if err != nil { log.Fatalf("loading refs: %v", err) @@ -105,6 +125,7 @@ func main() { store, err := imagecache.New(*cacheDir, imagecache.WithAuthenticator(auth), imagecache.WithPlatform(v1.Platform{OS: osName, Architecture: arch}), + imagecache.WithMinAge(*evictIdle), ) if err != nil { log.Fatalf("opening cache: %v", err) @@ -132,7 +153,7 @@ func main() { wg.Go(func() { defer func() { <-sem }() - evictIfLow(*cacheDir, *minFreeGB*1e9) + evictIfLow(ctx, store, *cacheDir, *minFreeGB*1e9) r := validateOne(ctx, store, ref, *timeout) @@ -203,57 +224,31 @@ func shortRef(ref string) string { return ref } -// evictIfLow deletes the oldest cached layer trees until the cache volume -// has at least minFree bytes available. Layers touched within the last -// --evict-idle are skipped so an in-flight image's freshly unpacked layers -// are not raced away mid-validation. NOTE: if the corpus unpacks faster -// than the idle window elapses on a small disk, nothing is evictable while -// the disk fills — size --evict-idle well below disk-fill time. -var evictMu sync.Mutex +// evictIfLow asks the cache's eviction engine to reclaim the free-space +// shortfall when the cache volume drops below minFree. The engine's +// protections all apply — record refcounts keep in-flight images' layers +// alive, --evict-idle is its min-age, deletion is two-phase — so nothing +// here can race a running validation. With no actors dir configured the +// root set is empty by design: nothing in a validation cache is mounted. +var evictMu sync.Mutex // one attempt per low-water episode; queued workers re-check and return -func evictIfLow(cacheRoot string, minFree uint64) { +func evictIfLow(ctx context.Context, store *imagecache.Store, cacheRoot string, minFree uint64) { evictMu.Lock() defer evictMu.Unlock() - if freeBytes(cacheRoot) >= minFree { + free := freeBytes(cacheRoot) + if free >= minFree { return } - layersDir := filepath.Join(cacheRoot, "layers", "sha256") - entries, err := os.ReadDir(layersDir) + stats, err := store.EvictUnused(ctx, int64(minFree-free), false) if err != nil { - return - } - type aged struct { - path string - mod time.Time + // Per-item failures or a gated pass: either way validation goes on; + // the next low-water check retries. + log.Printf("eviction pass reported errors (continuing): %v", err) } - var candidates []aged - for _, e := range entries { - info, err := e.Info() - if err != nil || time.Since(info.ModTime()) < *evictIdle { - continue - } - candidates = append(candidates, aged{filepath.Join(layersDir, e.Name()), info.ModTime()}) - } - sort.Slice(candidates, func(i, j int) bool { return candidates[i].mod.Before(candidates[j].mod) }) - - evicted := 0 - for _, c := range candidates { - if freeBytes(cacheRoot) >= minFree { - break - } - if err := imagecache.RemoveAllWritable(c.path); err == nil { - evicted++ - } - } - // Records referencing evicted layers re-pull only the missing layers, so - // stale manifests are harmless; drop them anyway to keep the dir tidy. - if evicted > 0 { - manifests, _ := filepath.Glob(filepath.Join(cacheRoot, "manifests", "sha256", "*.json")) - for _, m := range manifests { - _ = os.Remove(m) - } - log.Printf("evicted %d idle layers to reclaim disk (free now %.0f GB)", evicted, float64(freeBytes(cacheRoot))/1e9) + if stats.EvictedImages > 0 || stats.EvictedLayers > 0 { + log.Printf("evicted %d images / %d layers, %.1f GB credited (free now %.0f GB)", + stats.EvictedImages, stats.EvictedLayers, float64(stats.FreedBytes)/1e9, float64(freeBytes(cacheRoot))/1e9) } } From 7231c723db42e8fdc517c45ba2589869660ff25f Mon Sep 17 00:00:00 2001 From: Ivy Date: Mon, 10 Aug 2026 15:06:44 -0700 Subject: [PATCH 02/10] validate-image-cache: root placed actors during eviction 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. --- tools/validate-image-cache/main.go | 33 +++++++++++++++++------------- 1 file changed, 19 insertions(+), 14 deletions(-) diff --git a/tools/validate-image-cache/main.go b/tools/validate-image-cache/main.go index 948c9f705..f51269c65 100644 --- a/tools/validate-image-cache/main.go +++ b/tools/validate-image-cache/main.go @@ -26,13 +26,11 @@ // // gcloud artifacts docker images list REPO --format="value[separator='@'](package,version)" // -// Disk is bounded by the cache's own eviction engine: when the cache -// volume's free space drops below --min-free-gb, the tool asks -// Store.EvictUnused to reclaim the difference — LRU by image last-use, -// with in-flight pulls protected by record refcounts and layers younger -// than --evict-idle never touched. --evict-all instead empties everything -// evictable and exits (operator use: flush a cache without deleting the -// directory). +// Disk is bounded by the cache's own eviction engine: below +// --min-free-gb free space, the tool asks Store.EvictUnused to reclaim +// the shortfall. --evict-all instead empties everything evictable and +// exits — safe on a live node, because placed actors' bundle specs root +// their images (the root set is scanned from ateompath.ActorsDir). package main import ( @@ -50,6 +48,7 @@ import ( "sync" "time" + "github.com/agent-substrate/substrate/internal/ateompath" "github.com/agent-substrate/substrate/internal/imagecache" v1 "github.com/google/go-containerregistry/pkg/v1" googlecontainerauth "github.com/google/go-containerregistry/pkg/v1/google" @@ -89,7 +88,13 @@ func main() { if *evictAll { // Flush mode: no refs, no registry auth. New also reclaims any // crash-debris orphans before the pass. - store, err := imagecache.New(*cacheDir, imagecache.WithMinAge(*evictIdle)) + // WithActorsDir makes a flush on a live node safe: placed actors' + // bundle specs root their images. On a validation host the dir + // doesn't exist, which InUse treats as an empty root set. + store, err := imagecache.New(*cacheDir, + imagecache.WithMinAge(*evictIdle), + imagecache.WithActorsDir(ateompath.ActorsDir), + ) if err != nil { log.Fatalf("opening cache: %v", err) } @@ -126,6 +131,7 @@ func main() { imagecache.WithAuthenticator(auth), imagecache.WithPlatform(v1.Platform{OS: osName, Architecture: arch}), imagecache.WithMinAge(*evictIdle), + imagecache.WithActorsDir(ateompath.ActorsDir), ) if err != nil { log.Fatalf("opening cache: %v", err) @@ -224,12 +230,11 @@ func shortRef(ref string) string { return ref } -// evictIfLow asks the cache's eviction engine to reclaim the free-space -// shortfall when the cache volume drops below minFree. The engine's -// protections all apply — record refcounts keep in-flight images' layers -// alive, --evict-idle is its min-age, deletion is two-phase — so nothing -// here can race a running validation. With no actors dir configured the -// root set is empty by design: nothing in a validation cache is mounted. +// evictIfLow asks the eviction engine to reclaim the free-space +// shortfall below minFree. All engine protections apply, so in-flight +// validations are never raced. Unlike atelet's loop, the target is not +// capped at the pool size: on a dedicated validation disk an oversized +// target just runs out of candidates. var evictMu sync.Mutex // one attempt per low-water episode; queued workers re-check and return func evictIfLow(ctx context.Context, store *imagecache.Store, cacheRoot string, minFree uint64) { From 22580f5050292b40a6b10d9725a1498fe8fa3ad1 Mon Sep 17 00:00:00 2001 From: Ivy Date: Tue, 11 Aug 2026 15:17:03 -0700 Subject: [PATCH 03/10] atelet: address second-round GC loop review - Reject a negative --image-cache-gc-period (it silently disabled the loop at the launch-site guard). - Extract noteOutcome from runPass and unit-test the shortfall backoff cadence: warn x3, reminder every 12th, streak preserved across gated passes, reset re-arms the warnings. - Demote the engine's per-pass Info line to Debug so a no-target loop pass is actually silent; the driving caller owns the pass-level line. - Single-line sentinel wraps: fmt.Errorf("%w: %w") instead of errors.Join at the four enumeration gates. - imageCacheDirOutsideBasePath helper: filepath.Abs before the prefix check, table-tested (the warn branch was never exercised). - Flag help: period=0 leaves startup orphan recovery running; min-age governs the startup scan too. Run's doc states the root-context convention instead of implying a shutdown stop that never fires. - README: caveats on the total-size cap (evict-everything under sustained foreign pressure; retention floor as the extension) and the raw-capacity watermark reading ~5 points above df on ext4. --- cmd/atelet/imagegc.go | 39 ++++++++--- cmd/atelet/imagegc_test.go | 124 +++++++++++++++++++++++++++++++--- internal/imagecache/README.md | 15 +++- internal/imagecache/gc.go | 12 ++-- 4 files changed, 165 insertions(+), 25 deletions(-) diff --git a/cmd/atelet/imagegc.go b/cmd/atelet/imagegc.go index 1bc7e1dac..a5fca50fe 100644 --- a/cmd/atelet/imagegc.go +++ b/cmd/atelet/imagegc.go @@ -42,11 +42,11 @@ import ( ) var ( - imageCacheGCPeriod = pflag.Duration("image-cache-gc-period", 5*time.Minute, "How often to run the image cache eviction pass. 0 disables eviction entirely.") + imageCacheGCPeriod = pflag.Duration("image-cache-gc-period", 5*time.Minute, "How often to run the image cache eviction pass. 0 disables the periodic pass (startup orphan recovery still runs at every atelet start).") imageCacheHighPct = pflag.Int("image-cache-high-percent", 85, "Cache-volume usage percentage above which eviction starts.") imageCacheLowPct = pflag.Int("image-cache-low-percent", 80, "Cache-volume usage percentage eviction frees down to. Must be lower than --image-cache-high-percent.") imageCacheMaxBytes = pflag.Int64("image-cache-max-bytes", 0, "Absolute cap on the summed size of cached layers, evicted down to independently of the volume watermarks. 0 means no cap.") - imageCacheMinAge = pflag.Duration("image-cache-min-age", 2*time.Minute, "Layers and image records younger than this are never evicted (protects images pulled but not yet mounted).") + imageCacheMinAge = pflag.Duration("image-cache-min-age", 2*time.Minute, "Layers and image records younger than this are never evicted (protects images pulled but not yet mounted). Governs startup orphan recovery too, so it is live even with the periodic pass disabled.") imageCacheGCDryRun = pflag.Bool("image-cache-gc-dry-run", false, "Compute and log eviction decisions without deleting anything.") ) @@ -60,6 +60,11 @@ const ( ) func validateImageCacheGCFlags() error { + if *imageCacheGCPeriod < 0 { + // A negative period would silently disable the loop (it fails the + // > 0 guard at the launch site, which also protects the ticker). + return fmt.Errorf("--image-cache-gc-period %v must be >= 0", *imageCacheGCPeriod) + } if *imageCacheHighPct < 1 || *imageCacheHighPct > 100 { return fmt.Errorf("--image-cache-high-percent %d out of range [1,100]", *imageCacheHighPct) } @@ -71,11 +76,7 @@ func validateImageCacheGCFlags() error { // future), making just-pulled layers evictable. return fmt.Errorf("--image-cache-min-age %v must be >= 0", *imageCacheMinAge) } - // Outside BasePath the watermarks measure a different volume than - // actor state, so the pass would chase pressure the cache doesn't - // contribute to. Warn, don't fail: a separate cache volume is - // legitimate (recommended for IOPS) — it just wants its own numbers. - if !strings.HasPrefix(filepath.Clean(*imageCacheDir), ateompath.BasePath+string(os.PathSeparator)) { + if imageCacheDirOutsideBasePath(*imageCacheDir) { slog.Warn("Image cache dir is outside the ateom base path; its volume watermarks are measured separately from actor state", slog.String("image_cache_dir", *imageCacheDir), slog.String("actors_dir", ateompath.ActorsDir)) @@ -83,6 +84,18 @@ func validateImageCacheGCFlags() error { return nil } +// imageCacheDirOutsideBasePath reports whether the cache dir is outside +// the ateom base path — the watermarks then measure a different volume +// than actor state. Warn-worthy, not an error: a separate cache volume is +// legitimate (recommended for IOPS). +func imageCacheDirOutsideBasePath(dir string) bool { + abs, err := filepath.Abs(dir) + if err != nil { + abs = filepath.Clean(dir) + } + return !strings.HasPrefix(abs, ateompath.BasePath+string(os.PathSeparator)) +} + // imageCacheGCTarget computes the bytes a pass should free: the larger // of the watermark shortfall (kubelet's formula — usage at highPct frees // down to lowPct) and the pool's overage past maxBytes, but never more @@ -151,6 +164,10 @@ func newImageCacheGC(store *imagecache.Store, cacheDir string) *imageCacheGC { // Run executes eviction passes on the configured period until ctx is // done. Passes are strictly serialized: a slow pass delays the next tick // rather than overlapping it. +// +// atelet passes its root context (the StartMetricsServer convention), so +// the loop dies with the process; a pass cut off there leaves only .rm-* +// dirs for the startup sweep. Cancellation is honored for tests. func (g *imageCacheGC) Run(ctx context.Context) { // First pass immediately: a node booting under disk pressure must not // wait a full period (startup recovery reclaims debris, not pressure). @@ -221,7 +238,13 @@ func (g *imageCacheGC) runPass(ctx context.Context) { slog.Bool("dry_run", g.dryRun), slog.Duration("took", time.Since(tStart)), } - outcome := classifyGCPass(err, target, stats.FreedBytes) + g.noteOutcome(ctx, classifyGCPass(err, target, stats.FreedBytes), err, attrs) +} + +// noteOutcome logs one finished pass and advances the shortfall backoff. +// The counter survives a gated pass (which says nothing about whether the +// cache can meet a target) and resets when a target is met or absent. +func (g *imageCacheGC) noteOutcome(ctx context.Context, outcome gcPassOutcome, err error, attrs []any) { if outcome == gcPassSkipped { slog.ErrorContext(ctx, "Image cache GC pass skipped", append(attrs, slog.Any("err", err))...) return diff --git a/cmd/atelet/imagegc_test.go b/cmd/atelet/imagegc_test.go index 86e418012..95941fda7 100644 --- a/cmd/atelet/imagegc_test.go +++ b/cmd/atelet/imagegc_test.go @@ -15,11 +15,17 @@ package main import ( + "bytes" + "context" "errors" "fmt" + "log/slog" + "path/filepath" + "strings" "testing" "time" + "github.com/agent-substrate/substrate/internal/ateompath" "github.com/agent-substrate/substrate/internal/imagecache" ) @@ -112,38 +118,134 @@ func TestImageCacheGCTarget(t *testing.T) { } func TestValidateImageCacheGCFlags(t *testing.T) { - setFlags := func(high, low int, minAge time.Duration) { + setFlags := func(period time.Duration, high, low int, minAge time.Duration) { + *imageCacheGCPeriod = period *imageCacheHighPct = high *imageCacheLowPct = low *imageCacheMinAge = minAge } - t.Cleanup(func() { setFlags(85, 80, 2*time.Minute) }) + t.Cleanup(func() { setFlags(5*time.Minute, 85, 80, 2*time.Minute) }) cases := []struct { name string + period time.Duration high, low int minAge time.Duration wantErr bool }{ - {"defaults", 85, 80, 2 * time.Minute, false}, - {"boundary high=100 low=0", 100, 0, 0, false}, - {"high over 100", 101, 80, 0, true}, - {"low equals high", 85, 85, 0, true}, - {"low above high", 85, 90, 0, true}, - {"negative low", 85, -1, 0, true}, - {"negative min-age inverts the veto", 85, 80, -time.Second, true}, + {"defaults", 5 * time.Minute, 85, 80, 2 * time.Minute, false}, + {"boundary high=100 low=0", 5 * time.Minute, 100, 0, 0, false}, + {"zero period disables the periodic pass", 0, 85, 80, 0, false}, + {"negative period would silently disable the loop", -5 * time.Minute, 85, 80, 0, true}, + {"high over 100", 5 * time.Minute, 101, 80, 0, true}, + {"low equals high", 5 * time.Minute, 85, 85, 0, true}, + {"low above high", 5 * time.Minute, 85, 90, 0, true}, + {"negative low", 5 * time.Minute, 85, -1, 0, true}, + {"negative min-age inverts the veto", 5 * time.Minute, 85, 80, -time.Second, true}, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { - setFlags(tc.high, tc.low, tc.minAge) + setFlags(tc.period, tc.high, tc.low, tc.minAge) err := validateImageCacheGCFlags() if (err != nil) != tc.wantErr { - t.Errorf("high=%d low=%d minAge=%v: err=%v, wantErr=%v", tc.high, tc.low, tc.minAge, err, tc.wantErr) + t.Errorf("period=%v high=%d low=%d minAge=%v: err=%v, wantErr=%v", tc.period, tc.high, tc.low, tc.minAge, err, tc.wantErr) } }) } } +func TestImageCacheDirOutsideBasePath(t *testing.T) { + cases := []struct { + name string + dir string + want bool + }{ + {"inside", filepath.Join(ateompath.BasePath, "image-cache"), false}, + {"inside with doubled separator", ateompath.BasePath + "//image-cache", false}, + {"inside via dot-dot", ateompath.BasePath + "/x/../image-cache", false}, + {"base path itself is not inside", ateompath.BasePath, true}, + {"sibling with the base path as name prefix", ateompath.BasePath + "-other/image-cache", true}, + {"outside", "/var/lib/elsewhere/image-cache", true}, + {"dot-dot escaping the base path", ateompath.BasePath + "/../elsewhere/image-cache", true}, + {"relative resolves against the cwd, not the base path", "image-cache", true}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := imageCacheDirOutsideBasePath(tc.dir); got != tc.want { + t.Errorf("imageCacheDirOutsideBasePath(%q) = %v, want %v", tc.dir, got, tc.want) + } + }) + } +} + +// TestNoteOutcomeShortfallBackoff drives the shortfall cadence end to end: +// warn on the first shortfallWarnLimit consecutive shortfalls, then only +// every shortfallReminderEvery-th, streak preserved across a gated pass, +// reset (re-arming the warnings) on a met or absent target. +func TestNoteOutcomeShortfallBackoff(t *testing.T) { + var buf bytes.Buffer + prev := slog.Default() + slog.SetDefault(slog.New(slog.NewTextHandler(&buf, nil))) + t.Cleanup(func() { slog.SetDefault(prev) }) + + ctx := context.Background() + g := &imageCacheGC{} + logCount := func(msg string) int { return strings.Count(buf.String(), msg) } + + for range shortfallWarnLimit { + g.noteOutcome(ctx, gcPassShortfall, nil, nil) + } + if got := logCount("could not reach target"); got != shortfallWarnLimit { + t.Errorf("initial warns = %d, want %d", got, shortfallWarnLimit) + } + + buf.Reset() + for g.consecutiveShortfalls < 2*shortfallReminderEvery { + g.noteOutcome(ctx, gcPassShortfall, nil, nil) + } + if got := logCount("still short of target"); got != 2 { + t.Errorf("reminders through streak %d = %d, want 2", g.consecutiveShortfalls, got) + } + if got := logCount("could not reach target"); got != 0 { + t.Errorf("warns past the limit = %d, want 0", got) + } + + streak := g.consecutiveShortfalls + buf.Reset() + g.noteOutcome(ctx, gcPassSkipped, errors.New("gated"), nil) + if g.consecutiveShortfalls != streak { + t.Errorf("streak after gated pass = %d, want %d (preserved)", g.consecutiveShortfalls, streak) + } + if got := logCount("Image cache GC pass skipped"); got != 1 { + t.Errorf("skip logs = %d, want 1", got) + } + + buf.Reset() + g.noteOutcome(ctx, gcPassComplete, errors.New("one dir failed"), nil) + if g.consecutiveShortfalls != 0 { + t.Errorf("streak after complete pass = %d, want 0", g.consecutiveShortfalls) + } + if got := logCount("pass complete"); got != 1 { + t.Errorf("complete logs = %d, want 1", got) + } + if got := logCount("finished with errors"); got != 1 { + t.Errorf("per-item error warns = %d, want 1", got) + } + + buf.Reset() + g.noteOutcome(ctx, gcPassShortfall, nil, nil) + if got := logCount("could not reach target"); got != 1 { + t.Errorf("warns after reset = %d, want 1 (re-armed)", got) + } + + g.consecutiveShortfalls = shortfallWarnLimit + 1 + buf.Reset() + g.noteOutcome(ctx, gcPassQuiet, nil, nil) + if g.consecutiveShortfalls != 0 || buf.Len() != 0 { + t.Errorf("quiet pass: streak=%d buf=%q, want silent reset", g.consecutiveShortfalls, buf.String()) + } +} + func TestClassifyGCPass(t *testing.T) { gated := fmt.Errorf("pass gated: %w", imagecache.ErrIncompleteEnumeration) perItem := errors.New("while removing retired layer: permission denied") diff --git a/internal/imagecache/README.md b/internal/imagecache/README.md index 428edeb97..621ee2752 100644 --- a/internal/imagecache/README.md +++ b/internal/imagecache/README.md @@ -167,7 +167,8 @@ it — called from the checkpoint cleanup path in ateom-gvisor and ## Garbage collection `gc.go` holds the eviction engine; atelet drives it as a periodic pass -(`--image-cache-gc-period`, default 5m; `0` disables it). Each tick +(`--image-cache-gc-period`, default 5m; `0` disables the periodic pass, +but startup orphan recovery still runs at every atelet start). Each tick measures the cache volume with `statfs` and the pool's own size from the per-layer `size` files, then computes a byte target: free down to `--image-cache-low-percent` when volume usage reaches @@ -178,6 +179,18 @@ to fix disk pressure it didn't cause. `--image-cache-gc-dry-run` computes and logs every decision while mutating nothing: the recommended way to soak the policy on a live fleet. +Two caveats on those numbers. The cap is the pool's *total* size, not +the evictable subset (rooted and fresh layers can never be freed), so +under **sustained** foreign pressure the target stays unreachable and +every pass evicts everything unrooted and older than min-age — hit rate +goes to zero until the pressure clears. The "could not reach target" +WARNs are the signal; if this bites in practice, a retention floor +(never evict below N bytes) is the intended extension. And usage is +computed against the volume's raw capacity — kubelet's formula, so +operator intuition transfers — which counts ext4's ~5% reserved blocks +as used: eviction starts about five points below the configured +percentage as `df` reports it. + **One pass** (`Store.EvictUnused(ctx, targetBytes, dryRun)`): 1. **Root set** (`Store.InUse`): scan every bundle's diff --git a/internal/imagecache/gc.go b/internal/imagecache/gc.go index cddd3624a..b7a8d478a 100644 --- a/internal/imagecache/gc.go +++ b/internal/imagecache/gc.go @@ -251,7 +251,7 @@ func (s *Store) EvictUnused(ctx context.Context, targetBytes int64, dryRun bool) // partial scan would retire layers a running actor still mounts. // Not logged here — the caller logs the gated pass once, with its // own context (see ErrIncompleteEnumeration). - return stats, errors.Join(ErrIncompleteEnumeration, rootsErr) + return stats, fmt.Errorf("%w: %w", ErrIncompleteEnumeration, rootsErr) } cutoff := time.Now().Add(-s.minAge) @@ -261,11 +261,13 @@ func (s *Store) EvictUnused(ctx context.Context, targetBytes int64, dryRun bool) // be retired while that record still names it. Fail the whole pass // toward retention; the error names the records to repair or // delete, and every later pass retries. - return stats, errors.Join(ErrIncompleteEnumeration, listErr) + return stats, fmt.Errorf("%w: %w", ErrIncompleteEnumeration, listErr) } stats.Candidates = len(candidates) - slog.InfoContext(ctx, "Image cache eviction pass", + // Debug: the driving caller logs the pass outcome once, and a + // no-target pass should be silent. + slog.DebugContext(ctx, "Image cache eviction pass", slog.Int64("target_bytes", targetBytes), slog.Bool("dry_run", dryRun), slog.Int("rooted_images", stats.RootedImages), @@ -489,12 +491,12 @@ func (s *Store) RecoverOrphans(ctx context.Context) (EvictStats, error) { roots, rootsErr := s.InUse() if rootsErr != nil { - return stats, errors.Join(ErrIncompleteEnumeration, rootsErr) + return stats, fmt.Errorf("%w: %w", ErrIncompleteEnumeration, rootsErr) } cutoff := time.Now().Add(-s.minAge) _, refcount, complete, listErr := s.listEviction(roots, cutoff, &stats) if !complete { - return stats, errors.Join(ErrIncompleteEnumeration, listErr) + return stats, fmt.Errorf("%w: %w", ErrIncompleteEnumeration, listErr) } retired, errs := s.sweepOrphanLayers(ctx, roots, refcount, cutoff, false, &stats) From abd7fd30ddf4a68a225c510443546f234a1c4bd8 Mon Sep 17 00:00:00 2001 From: Ivy Date: Tue, 11 Aug 2026 15:41:07 -0700 Subject: [PATCH 04/10] validate-image-cache: gate-aware errors, fruitless-pass cooldown, honest live-node docs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Split gated vs per-item errors in both eviction paths on the ErrIncompleteEnumeration sentinel: evict-all no longer dies statless after a pass that ran (per-item errors print stats, then exit 1), and evictIfLow no longer calls a did-nothing gated pass "continuing". - 30s cooldown after a pass that freed nothing, so workers stop running a full root-set scan + record enumeration each when nothing is evictable yet. - Note that the statfs shortfall and FreedBytes are different estimates. - Package doc: bundle-spec rooting protects mounted images, but the engine's locks are per-process — stop atelet for a clean flush. - --evict-all documents min-age survival; --evict-all with --refs-file is a usage error; shared store options in one newStore helper. --- tools/validate-image-cache/main.go | 82 ++++++++++++++++++++++-------- 1 file changed, 60 insertions(+), 22 deletions(-) diff --git a/tools/validate-image-cache/main.go b/tools/validate-image-cache/main.go index f51269c65..29581d8a8 100644 --- a/tools/validate-image-cache/main.go +++ b/tools/validate-image-cache/main.go @@ -29,14 +29,18 @@ // Disk is bounded by the cache's own eviction engine: below // --min-free-gb free space, the tool asks Store.EvictUnused to reclaim // the shortfall. --evict-all instead empties everything evictable and -// exits — safe on a live node, because placed actors' bundle specs root -// their images (the root set is scanned from ateompath.ActorsDir). +// exits. Bundle-spec rooting (scanned from ateompath.ActorsDir) protects +// placed actors' mounted images, but the engine's locks are per-process: +// a run concurrent with a live atelet is two unsynchronized GC passes +// over one pool, and New's orphan scan assumes no pull is in flight. +// Stop atelet first for a clean flush. package main import ( "bufio" "context" "encoding/csv" + "errors" "flag" "fmt" "log" @@ -65,10 +69,20 @@ var ( timeout = flag.Duration("timeout", 20*time.Minute, "Per-image timeout") minFreeGB = flag.Uint64("min-free-gb", 150, "Ask the eviction engine to reclaim disk when the cache volume has less free space than this") evictIdle = flag.Duration("evict-idle", 10*time.Minute, "Eviction min-age: layers and records younger than this are never evicted. NOTE: if the corpus unpacks faster than this window elapses on a small disk, nothing is evictable while the disk fills — size it well below disk-fill time") - evictAll = flag.Bool("evict-all", false, "Evict everything evictable from the cache and exit (no refs file needed)") + evictAll = flag.Bool("evict-all", false, "Evict everything evictable from the cache and exit (no refs file needed). Rooted images and anything younger than --evict-idle survive") platform = flag.String("platform", "linux/amd64", "Image platform to pull") ) +// newStore opens the cache with the options both modes share: min-age +// from --evict-idle, and bundle-spec rooting from the node's actors dir +// (absent on a validation host, which InUse treats as an empty root set). +func newStore(extra ...imagecache.Option) (*imagecache.Store, error) { + return imagecache.New(*cacheDir, append([]imagecache.Option{ + imagecache.WithMinAge(*evictIdle), + imagecache.WithActorsDir(ateompath.ActorsDir), + }, extra...)...) +} + type result struct { ref string digest string @@ -79,7 +93,8 @@ type result struct { func main() { flag.Parse() - if *cacheDir == "" || (*refsFile == "" && !*evictAll) { + if *cacheDir == "" || (*refsFile == "" && !*evictAll) || (*refsFile != "" && *evictAll) { + fmt.Fprintln(os.Stderr, "need --cache-dir plus exactly one of --refs-file or --evict-all") flag.Usage() os.Exit(2) } @@ -87,23 +102,26 @@ func main() { if *evictAll { // Flush mode: no refs, no registry auth. New also reclaims any - // crash-debris orphans before the pass. - // WithActorsDir makes a flush on a live node safe: placed actors' - // bundle specs root their images. On a validation host the dir - // doesn't exist, which InUse treats as an empty root set. - store, err := imagecache.New(*cacheDir, - imagecache.WithMinAge(*evictIdle), - imagecache.WithActorsDir(ateompath.ActorsDir), - ) + // crash-debris orphans before the pass. See the package doc for + // why a live atelet should be stopped first. + store, err := newStore() if err != nil { log.Fatalf("opening cache: %v", err) } stats, err := store.EvictUnused(ctx, math.MaxInt64, false) + if errors.Is(err, imagecache.ErrIncompleteEnumeration) { + // Gated: nothing was attempted; the error names the file to + // repair or delete. + log.Fatalf("evict-all did nothing: %v", err) + } if err != nil { - log.Fatalf("evict-all: %v", err) + log.Printf("evict-all finished with errors: %v", err) } log.Printf("evict-all: %d images / %d layers evicted, %.1f GB credited (free now %.0f GB)", stats.EvictedImages, stats.EvictedLayers, float64(stats.FreedBytes)/1e9, float64(freeBytes(*cacheDir))/1e9) + if err != nil { + os.Exit(1) + } return } @@ -127,11 +145,9 @@ func main() { log.Fatalf("creating GCP authenticator (need application-default credentials): %v", err) } - store, err := imagecache.New(*cacheDir, + store, err := newStore( imagecache.WithAuthenticator(auth), imagecache.WithPlatform(v1.Platform{OS: osName, Architecture: arch}), - imagecache.WithMinAge(*evictIdle), - imagecache.WithActorsDir(ateompath.ActorsDir), ) if err != nil { log.Fatalf("opening cache: %v", err) @@ -234,8 +250,20 @@ func shortRef(ref string) string { // shortfall below minFree. All engine protections apply, so in-flight // validations are never raced. Unlike atelet's loop, the target is not // capped at the pool size: on a dedicated validation disk an oversized -// target just runs out of candidates. -var evictMu sync.Mutex // one attempt per low-water episode; queued workers re-check and return +// target just runs out of candidates. The statfs shortfall and the +// engine's FreedBytes are different estimates (disk blocks vs recorded +// sizes), so a "met" target can leave free space still short; the next +// worker's re-check converges. +var ( + evictMu sync.Mutex // one attempt per low-water episode; queued workers re-check and return + // lastFruitless backs off when a pass freed nothing (typically: + // everything is younger than --evict-idle). Without it, every queued + // worker would run a full root-set scan and record enumeration ahead + // of its validation until free space moved. + lastFruitless time.Time +) + +const fruitlessCooldown = 30 * time.Second func evictIfLow(ctx context.Context, store *imagecache.Store, cacheRoot string, minFree uint64) { evictMu.Lock() @@ -245,16 +273,26 @@ func evictIfLow(ctx context.Context, store *imagecache.Store, cacheRoot string, if free >= minFree { return } + if time.Since(lastFruitless) < fruitlessCooldown { + return + } stats, err := store.EvictUnused(ctx, int64(minFree-free), false) - if err != nil { - // Per-item failures or a gated pass: either way validation goes on; - // the next low-water check retries. - log.Printf("eviction pass reported errors (continuing): %v", err) + switch { + case errors.Is(err, imagecache.ErrIncompleteEnumeration): + // Gated: nothing was attempted; the error names the file to repair. + // Validation itself goes on — it only needed the disk space. + log.Printf("eviction pass skipped, nothing attempted: %v", err) + case err != nil: + // Per-item failures on a pass that ran; each retries next pass. + log.Printf("eviction pass finished with errors: %v", err) } if stats.EvictedImages > 0 || stats.EvictedLayers > 0 { log.Printf("evicted %d images / %d layers, %.1f GB credited (free now %.0f GB)", stats.EvictedImages, stats.EvictedLayers, float64(stats.FreedBytes)/1e9, float64(freeBytes(cacheRoot))/1e9) } + if stats.FreedBytes == 0 { + lastFruitless = time.Now() + } } func freeBytes(path string) uint64 { From 0fbea649e67b85ae7527b7a5a68f9f7069cb00c1 Mon Sep 17 00:00:00 2001 From: Ivy Date: Tue, 11 Aug 2026 15:41:07 -0700 Subject: [PATCH 05/10] atelet: cover runPass skip and recovery paths behind a store seam gcStore (CacheSize + EvictUnused) is the two-method slice of Store the loop uses; the constructor still takes *imagecache.Store. Tests cover the statfs-failure and CacheSize-failure skips, target/dry-run plumb-through, and panic recovery. --- cmd/atelet/imagegc.go | 9 +++++- cmd/atelet/imagegc_test.go | 64 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 72 insertions(+), 1 deletion(-) diff --git a/cmd/atelet/imagegc.go b/cmd/atelet/imagegc.go index a5fca50fe..4eb8b63f3 100644 --- a/cmd/atelet/imagegc.go +++ b/cmd/atelet/imagegc.go @@ -134,11 +134,18 @@ func imageCacheGCTarget(capacity, available uint64, cacheSize, maxBytes int64, h return target } +// gcStore is what the loop needs from *imagecache.Store — a seam so +// runPass's skip and recovery paths are testable without a real pool. +type gcStore interface { + CacheSize() (int64, error) + EvictUnused(ctx context.Context, targetBytes int64, dryRun bool) (imagecache.EvictStats, error) +} + // imageCacheGC is the loop's state: configuration snapshotted from the // flags at construction (the pass logic never reads globals, so it is // testable without flag juggling) plus the shortfall-backoff counter. type imageCacheGC struct { - store *imagecache.Store + store gcStore cacheDir string period time.Duration highPct int diff --git a/cmd/atelet/imagegc_test.go b/cmd/atelet/imagegc_test.go index 95941fda7..9f45ee38d 100644 --- a/cmd/atelet/imagegc_test.go +++ b/cmd/atelet/imagegc_test.go @@ -178,6 +178,70 @@ func TestImageCacheDirOutsideBasePath(t *testing.T) { } } +type fakeGCStore struct { + size int64 + sizeErr error + sizeCalls int + evictCalls int + gotTarget int64 + gotDryRun bool + evictErr error + stats imagecache.EvictStats + panicOnEvict bool +} + +func (f *fakeGCStore) CacheSize() (int64, error) { + f.sizeCalls++ + return f.size, f.sizeErr +} + +func (f *fakeGCStore) EvictUnused(_ context.Context, target int64, dryRun bool) (imagecache.EvictStats, error) { + f.evictCalls++ + f.gotTarget = target + f.gotDryRun = dryRun + if f.panicOnEvict { + panic("boom") + } + return f.stats, f.evictErr +} + +func TestRunPassSkipsOnStatfsFailure(t *testing.T) { + fake := &fakeGCStore{} + g := &imageCacheGC{store: fake, cacheDir: filepath.Join(t.TempDir(), "missing"), highPct: 85, lowPct: 80} + g.runPass(context.Background()) + if fake.sizeCalls != 0 || fake.evictCalls != 0 { + t.Errorf("statfs failure: sizeCalls=%d evictCalls=%d, want 0/0", fake.sizeCalls, fake.evictCalls) + } +} + +func TestRunPassSkipsOnCacheSizeFailure(t *testing.T) { + fake := &fakeGCStore{sizeErr: errors.New("unreadable size file")} + g := &imageCacheGC{store: fake, cacheDir: t.TempDir(), highPct: 85, lowPct: 80} + g.runPass(context.Background()) + if fake.evictCalls != 0 { + t.Errorf("CacheSize failure: evictCalls=%d, want 0", fake.evictCalls) + } +} + +func TestRunPassEvictsAndPassesDryRun(t *testing.T) { + // high=100 keeps the host volume's real usage out of the target, so + // the max-bytes overage (100-1=99) is the whole target. + fake := &fakeGCStore{size: 100, stats: imagecache.EvictStats{FreedBytes: 99}} + g := &imageCacheGC{store: fake, cacheDir: t.TempDir(), highPct: 100, lowPct: 0, maxBytes: 1, dryRun: true} + g.runPass(context.Background()) + if fake.evictCalls != 1 || fake.gotTarget != 99 || !fake.gotDryRun { + t.Errorf("evictCalls=%d target=%d dryRun=%v, want 1/99/true", fake.evictCalls, fake.gotTarget, fake.gotDryRun) + } + if g.consecutiveShortfalls != 0 { + t.Errorf("consecutiveShortfalls=%d after met target, want 0", g.consecutiveShortfalls) + } +} + +func TestRunPassRecoversPanic(t *testing.T) { + g := &imageCacheGC{store: &fakeGCStore{panicOnEvict: true}, cacheDir: t.TempDir(), highPct: 100, lowPct: 0} + g.runPass(context.Background()) // must not propagate the panic +} + // TestNoteOutcomeShortfallBackoff drives the shortfall cadence end to end: // warn on the first shortfallWarnLimit consecutive shortfalls, then only // every shortfallReminderEvery-th, streak preserved across a gated pass, From 81c67c0de1bf7030aa7cb8ef2f1ab0ddff0efe98 Mon Sep 17 00:00:00 2001 From: Ivy Date: Tue, 11 Aug 2026 15:50:46 -0700 Subject: [PATCH 06/10] validate-image-cache, atelet: third-round review polish - Package doc: run as the cache/actors-dir owner (an unreadable actors dir gates every pass); gated-pass comments say unreadable-or-corrupt. - lastFruitless comment covers the gated-pass case it also backs off. - runPass target test tolerates a near-full host volume (>= 99, not ==) and pre-seeds consecutiveShortfalls so the reset assertion is real. - listEviction doc states the invariant the %w gates rely on: complete is false only alongside a non-nil err. --- cmd/atelet/imagegc_test.go | 12 +++++++----- internal/imagecache/gc.go | 9 +++++---- tools/validate-image-cache/main.go | 16 +++++++++------- 3 files changed, 21 insertions(+), 16 deletions(-) diff --git a/cmd/atelet/imagegc_test.go b/cmd/atelet/imagegc_test.go index 9f45ee38d..4ee763038 100644 --- a/cmd/atelet/imagegc_test.go +++ b/cmd/atelet/imagegc_test.go @@ -224,13 +224,15 @@ func TestRunPassSkipsOnCacheSizeFailure(t *testing.T) { } func TestRunPassEvictsAndPassesDryRun(t *testing.T) { - // high=100 keeps the host volume's real usage out of the target, so - // the max-bytes overage (100-1=99) is the whole target. - fake := &fakeGCStore{size: 100, stats: imagecache.EvictStats{FreedBytes: 99}} + // high=100 sidelines the watermark on any volume with >=1% free, so + // the max-bytes overage (99) is the target; a near-full host volume + // can lift it to the cacheSize cap, hence >= not ==. + fake := &fakeGCStore{size: 100, stats: imagecache.EvictStats{FreedBytes: 100}} g := &imageCacheGC{store: fake, cacheDir: t.TempDir(), highPct: 100, lowPct: 0, maxBytes: 1, dryRun: true} + g.consecutiveShortfalls = 5 // a met target must reset it g.runPass(context.Background()) - if fake.evictCalls != 1 || fake.gotTarget != 99 || !fake.gotDryRun { - t.Errorf("evictCalls=%d target=%d dryRun=%v, want 1/99/true", fake.evictCalls, fake.gotTarget, fake.gotDryRun) + if fake.evictCalls != 1 || fake.gotTarget < 99 || !fake.gotDryRun { + t.Errorf("evictCalls=%d target=%d dryRun=%v, want 1/>=99/true", fake.evictCalls, fake.gotTarget, fake.gotDryRun) } if g.consecutiveShortfalls != 0 { t.Errorf("consecutiveShortfalls=%d after met target, want 0", g.consecutiveShortfalls) diff --git a/internal/imagecache/gc.go b/internal/imagecache/gc.go index b7a8d478a..8a27804fb 100644 --- a/internal/imagecache/gc.go +++ b/internal/imagecache/gc.go @@ -601,10 +601,11 @@ func (s *Store) dryRunRetire(hex string, cutoff time.Time) (int64, retireStatus) // their references are what keep shared layers alive — counting // listing-stage vetoes into stats. // -// complete reports whether every record was read and decoded. Refcounts -// from a partial listing understate references — a layer shared with an -// unread record looks unreferenced — so both callers gate on it and do -// nothing with a partial listing. +// complete reports whether every record was read and decoded, and is +// false only alongside a non-nil err (the gates wrap that err with %w, +// which a nil would garble). Refcounts from a partial listing understate +// references — a layer shared with an unread record looks unreferenced — +// so both callers gate on it and do nothing with a partial listing. func (s *Store) listEviction(roots RootSet, cutoff time.Time, stats *EvictStats) (cands []evictionCandidate, refcount map[string]int, complete bool, err error) { refcount = map[string]int{} entries, err := os.ReadDir(s.manifestsDir()) diff --git a/tools/validate-image-cache/main.go b/tools/validate-image-cache/main.go index 29581d8a8..a3a7e5b0f 100644 --- a/tools/validate-image-cache/main.go +++ b/tools/validate-image-cache/main.go @@ -33,7 +33,8 @@ // placed actors' mounted images, but the engine's locks are per-process: // a run concurrent with a live atelet is two unsynchronized GC passes // over one pool, and New's orphan scan assumes no pull is in flight. -// Stop atelet first for a clean flush. +// Stop atelet first for a clean flush, and run as the user that owns +// the cache and actors dirs — an unreadable actors dir gates every pass. package main import ( @@ -110,13 +111,14 @@ func main() { } stats, err := store.EvictUnused(ctx, math.MaxInt64, false) if errors.Is(err, imagecache.ErrIncompleteEnumeration) { - // Gated: nothing was attempted; the error names the file to - // repair or delete. + // Gated: nothing was attempted; the error names the unreadable + // or corrupt file. log.Fatalf("evict-all did nothing: %v", err) } if err != nil { log.Printf("evict-all finished with errors: %v", err) } + // Print the summary even on partial failure; err decides the exit code. log.Printf("evict-all: %d images / %d layers evicted, %.1f GB credited (free now %.0f GB)", stats.EvictedImages, stats.EvictedLayers, float64(stats.FreedBytes)/1e9, float64(freeBytes(*cacheDir))/1e9) if err != nil { @@ -256,10 +258,10 @@ func shortRef(ref string) string { // worker's re-check converges. var ( evictMu sync.Mutex // one attempt per low-water episode; queued workers re-check and return - // lastFruitless backs off when a pass freed nothing (typically: - // everything is younger than --evict-idle). Without it, every queued - // worker would run a full root-set scan and record enumeration ahead - // of its validation until free space moved. + // lastFruitless backs off when a pass freed nothing — everything + // younger than --evict-idle, or a gated pass. Without it every queued + // worker would run a full engine pass ahead of its validation until + // free space moved. lastFruitless time.Time ) From 24323e77c316742ae7af85e763d55f2ab48018bf Mon Sep 17 00:00:00 2001 From: Ivy Date: Tue, 11 Aug 2026 16:32:16 -0700 Subject: [PATCH 07/10] validate-image-cache, atelet: fourth-round review fixes - Live-node guidance corrected: atelet runs for the node lifetime, so "stop atelet first" is gone. The docs and a runtime warning now state the real contract: the pool cannot be corrupted (record-first pulls, two-phase retirement); worst case a concurrent actor start fails once and heals on re-pull. - Fruitless-pass predicate includes EvictedLayers: a retired layer with an unreadable size file credits zero bytes, and bytes alone would call that productive pass fruitless. - Run covered: immediate first pass then ticks, via the gcStore seam. --- cmd/atelet/imagegc_test.go | 11 +++++++++++ tools/validate-image-cache/main.go | 25 ++++++++++++++++--------- 2 files changed, 27 insertions(+), 9 deletions(-) diff --git a/cmd/atelet/imagegc_test.go b/cmd/atelet/imagegc_test.go index 4ee763038..cbea350b7 100644 --- a/cmd/atelet/imagegc_test.go +++ b/cmd/atelet/imagegc_test.go @@ -239,6 +239,17 @@ func TestRunPassEvictsAndPassesDryRun(t *testing.T) { } } +func TestRunImmediateFirstPassThenTicks(t *testing.T) { + fake := &fakeGCStore{} + g := &imageCacheGC{store: fake, cacheDir: t.TempDir(), highPct: 100, lowPct: 0, period: 10 * time.Millisecond} + ctx, cancel := context.WithTimeout(context.Background(), 150*time.Millisecond) + defer cancel() + g.Run(ctx) + if fake.evictCalls < 2 { + t.Errorf("evictCalls=%d, want >=2 (immediate first pass plus ticks)", fake.evictCalls) + } +} + func TestRunPassRecoversPanic(t *testing.T) { g := &imageCacheGC{store: &fakeGCStore{panicOnEvict: true}, cacheDir: t.TempDir(), highPct: 100, lowPct: 0} g.runPass(context.Background()) // must not propagate the panic diff --git a/tools/validate-image-cache/main.go b/tools/validate-image-cache/main.go index a3a7e5b0f..dd7b75fff 100644 --- a/tools/validate-image-cache/main.go +++ b/tools/validate-image-cache/main.go @@ -30,11 +30,13 @@ // --min-free-gb free space, the tool asks Store.EvictUnused to reclaim // the shortfall. --evict-all instead empties everything evictable and // exits. Bundle-spec rooting (scanned from ateompath.ActorsDir) protects -// placed actors' mounted images, but the engine's locks are per-process: -// a run concurrent with a live atelet is two unsynchronized GC passes -// over one pool, and New's orphan scan assumes no pull is in flight. -// Stop atelet first for a clean flush, and run as the user that owns -// the cache and actors dirs — an unreadable actors dir gates every pass. +// placed actors' mounted images. The engine's locks are per-process, so a +// run beside a live atelet is unsynchronized with its GC and pulls — the +// pool cannot be corrupted (record-first pulls, two-phase retirement), +// but a layer idle past --evict-idle can be retired just as a new pull +// cache-hits it: that actor start fails once and heals on re-pull. Run +// as the user that owns the cache and actors dirs — an unreadable actors +// dir gates every pass. package main import ( @@ -70,7 +72,7 @@ var ( timeout = flag.Duration("timeout", 20*time.Minute, "Per-image timeout") minFreeGB = flag.Uint64("min-free-gb", 150, "Ask the eviction engine to reclaim disk when the cache volume has less free space than this") evictIdle = flag.Duration("evict-idle", 10*time.Minute, "Eviction min-age: layers and records younger than this are never evicted. NOTE: if the corpus unpacks faster than this window elapses on a small disk, nothing is evictable while the disk fills — size it well below disk-fill time") - evictAll = flag.Bool("evict-all", false, "Evict everything evictable from the cache and exit (no refs file needed). Rooted images and anything younger than --evict-idle survive") + evictAll = flag.Bool("evict-all", false, "Evict everything evictable from the cache and exit (no refs file needed). Rooted images and anything younger than --evict-idle survive. Safe alongside a running atelet: worst case a concurrent actor start fails once and heals on retry") platform = flag.String("platform", "linux/amd64", "Image platform to pull") ) @@ -103,8 +105,11 @@ func main() { if *evictAll { // Flush mode: no refs, no registry auth. New also reclaims any - // crash-debris orphans before the pass. See the package doc for - // why a live atelet should be stopped first. + // crash-debris orphans before the pass. + if _, err := os.Stat(ateompath.ActorsDir); err == nil { + // An actors dir is a decent proxy for a live node. + log.Printf("live node: this run is not synchronized with atelet's GC; a concurrent actor start may fail once and heal on retry") + } store, err := newStore() if err != nil { log.Fatalf("opening cache: %v", err) @@ -292,7 +297,9 @@ func evictIfLow(ctx context.Context, store *imagecache.Store, cacheRoot string, log.Printf("evicted %d images / %d layers, %.1f GB credited (free now %.0f GB)", stats.EvictedImages, stats.EvictedLayers, float64(stats.FreedBytes)/1e9, float64(freeBytes(cacheRoot))/1e9) } - if stats.FreedBytes == 0 { + // Layers retired with an unreadable size file credit zero bytes, so + // bytes alone would call a productive pass fruitless. + if stats.EvictedLayers == 0 && stats.FreedBytes == 0 { lastFruitless = time.Now() } } From 33bfc9f3d14068edf49c3e6462c022ad883c61a4 Mon Sep 17 00:00:00 2001 From: Ivy Date: Tue, 11 Aug 2026 17:28:50 -0700 Subject: [PATCH 08/10] validate-image-cache, atelet: fifth-round review fixes and doc audit - Drop the bounded-failure guarantee from --evict-all's contract: the per-process locks can't bound the concurrent case (an already-mounted actor takes EIO, not a retryable start failure). The package doc states both outcomes and the intended end state (an atelet-owned flush); the live-node warning is one generic, non-derivable fact. - Floor --evict-idle at 1m when an actors dir exists: min-age is the only protection that applies across processes, so on a live node it must not be tuned away. Validation hosts keep full freedom. - Split the Run test: immediate-first-pass pinned deterministically (hour period, pre-cancelled context, exactly one call), ticking covered separately. - Doc audit: --refs-file is not required with --evict-all; the tool README's flag table gains --evict-all and sheds the deleted prototype's vocabulary; gate comments say "path" (dirs gate too); the engine's platform-option comment describes callers generically. --- cmd/atelet/imagegc_test.go | 17 ++++++++++++-- internal/imagecache/imagecache.go | 5 ++-- tools/validate-image-cache/README.md | 13 ++++++----- tools/validate-image-cache/main.go | 34 ++++++++++++++++++++-------- 4 files changed, 48 insertions(+), 21 deletions(-) diff --git a/cmd/atelet/imagegc_test.go b/cmd/atelet/imagegc_test.go index cbea350b7..99df8d3a3 100644 --- a/cmd/atelet/imagegc_test.go +++ b/cmd/atelet/imagegc_test.go @@ -239,14 +239,27 @@ func TestRunPassEvictsAndPassesDryRun(t *testing.T) { } } -func TestRunImmediateFirstPassThenTicks(t *testing.T) { +func TestRunFirstPassIsImmediate(t *testing.T) { + // A cancelled context and an hour-long period: the single call can + // only be the immediate first pass, never a tick. + fake := &fakeGCStore{} + g := &imageCacheGC{store: fake, cacheDir: t.TempDir(), highPct: 100, lowPct: 0, period: time.Hour} + ctx, cancel := context.WithCancel(context.Background()) + cancel() + g.Run(ctx) + if fake.evictCalls != 1 { + t.Errorf("evictCalls=%d, want exactly 1 (the immediate first pass)", fake.evictCalls) + } +} + +func TestRunTicks(t *testing.T) { fake := &fakeGCStore{} g := &imageCacheGC{store: fake, cacheDir: t.TempDir(), highPct: 100, lowPct: 0, period: 10 * time.Millisecond} ctx, cancel := context.WithTimeout(context.Background(), 150*time.Millisecond) defer cancel() g.Run(ctx) if fake.evictCalls < 2 { - t.Errorf("evictCalls=%d, want >=2 (immediate first pass plus ticks)", fake.evictCalls) + t.Errorf("evictCalls=%d, want >=2 (first pass plus at least one tick)", fake.evictCalls) } } diff --git a/internal/imagecache/imagecache.go b/internal/imagecache/imagecache.go index da6fb04e4..115f526d7 100644 --- a/internal/imagecache/imagecache.go +++ b/internal/imagecache/imagecache.go @@ -105,9 +105,8 @@ type Store struct { localhostRegistryReplacement string - // platform overrides the default pull platform (linux/GOARCH). Used by - // validation tooling that runs on a different architecture than the - // nodes it validates for. + // platform overrides the default pull platform (linux/GOARCH), for + // callers pulling on a different architecture than the images' target. platform *v1.Platform // actorsDir is scanned by InUse for bundle overlay specs; empty disables diff --git a/tools/validate-image-cache/README.md b/tools/validate-image-cache/README.md index 54f28f81d..18881ebf2 100644 --- a/tools/validate-image-cache/README.md +++ b/tools/validate-image-cache/README.md @@ -23,9 +23,9 @@ whiteouts, oversized layer counts, ...). `domain:google.com` grant) is not necessarily readable by a *service account* — validate with the identity that production will use. - Disk: unpacked layers are 2–3× their compressed size. The tool bounds - usage by evicting idle cached layers when the cache volume's free space - drops below `--min-free-gb`, but give it room to breathe (tens of GB - minimum). + usage by asking the cache's own eviction engine to reclaim space when the + cache volume's free space drops below `--min-free-gb`, but give it room + to breathe (tens of GB minimum). - Runs anywhere Go runs, including macOS (unpack is pure file I/O). Caveat: a case-insensitive filesystem (default macOS APFS) unpacks case-colliding paths slightly differently than Linux — silently, not as an error. Linux @@ -68,15 +68,16 @@ go run ./tools/validate-image-cache \ | Flag | Default | Meaning | |---|---|---| -| `--refs-file` | (required) | file with one image ref per line | +| `--refs-file` | (required unless `--evict-all`) | file with one image ref per line | | `--cache-dir` | (required) | cache root; reused (and cache-hit) across runs | | `--sample` | 0 (= all) | validate a random sample of N refs | | `--seed` | 1 | sampling seed; same seed + file ⇒ same sample | | `--out` | `validate-results.csv` | results CSV, written incrementally | | `--parallel` | 3 | images validated concurrently (each pulls up to 4 layers in parallel) | | `--timeout` | 20m | per-image timeout | -| `--min-free-gb` | 150 | evict oldest idle layers below this free-space floor | -| `--evict-idle` | 10m | only evict layers idle at least this long; must be far below disk-fill time on small disks | +| `--min-free-gb` | 150 | reclaim the shortfall below this free-space floor via the eviction engine | +| `--evict-idle` | 10m | eviction min-age: layers and records younger than this are never evicted (minimum 1m on a node with an actors dir); must be far below disk-fill time on small disks | +| `--evict-all` | false | evict everything evictable and exit (mutually exclusive with `--refs-file`); rooted images and anything younger than `--evict-idle` survive | | `--platform` | `linux/amd64` | image platform to pull | ## Output and rerunning diff --git a/tools/validate-image-cache/main.go b/tools/validate-image-cache/main.go index dd7b75fff..096fcc3a9 100644 --- a/tools/validate-image-cache/main.go +++ b/tools/validate-image-cache/main.go @@ -33,10 +33,14 @@ // placed actors' mounted images. The engine's locks are per-process, so a // run beside a live atelet is unsynchronized with its GC and pulls — the // pool cannot be corrupted (record-first pulls, two-phase retirement), -// but a layer idle past --evict-idle can be retired just as a new pull -// cache-hits it: that actor start fails once and heals on re-pull. Run -// as the user that owns the cache and actors dirs — an unreadable actors -// dir gates every pass. +// but a layer idle past --evict-idle can be retired just as atelet +// reuses it: a not-yet-mounted actor start fails once and heals on +// re-pull; an already-mounted actor can take EIO from a removed +// lowerdir. min-age is the only cross-process guard, hence the floor on +// --evict-idle when an actors dir exists. (The end state is an +// atelet-owned flush — RPC or trigger file — not a second process in +// the pool.) Run as the user that owns the cache and actors dirs — an +// unreadable actors dir gates every pass. package main import ( @@ -63,7 +67,7 @@ import ( ) var ( - refsFile = flag.String("refs-file", "", "File with one image ref per line (required)") + refsFile = flag.String("refs-file", "", "File with one image ref per line (required unless --evict-all)") sample = flag.Int("sample", 0, "Validate a random sample of N refs (0 = all)") seed = flag.Int64("seed", 1, "Seed for reproducible sampling") cacheDir = flag.String("cache-dir", "", "Cache root (required); reused across runs") @@ -71,8 +75,8 @@ var ( parallel = flag.Int("parallel", 3, "Images validated concurrently (each pulls up to 4 layers in parallel)") timeout = flag.Duration("timeout", 20*time.Minute, "Per-image timeout") minFreeGB = flag.Uint64("min-free-gb", 150, "Ask the eviction engine to reclaim disk when the cache volume has less free space than this") - evictIdle = flag.Duration("evict-idle", 10*time.Minute, "Eviction min-age: layers and records younger than this are never evicted. NOTE: if the corpus unpacks faster than this window elapses on a small disk, nothing is evictable while the disk fills — size it well below disk-fill time") - evictAll = flag.Bool("evict-all", false, "Evict everything evictable from the cache and exit (no refs file needed). Rooted images and anything younger than --evict-idle survive. Safe alongside a running atelet: worst case a concurrent actor start fails once and heals on retry") + evictIdle = flag.Duration("evict-idle", 10*time.Minute, "Eviction min-age: layers and records younger than this are never evicted. Minimum 1m on a node with an actors dir. NOTE: if the corpus unpacks faster than this window elapses on a small disk, nothing is evictable while the disk fills — size it well below disk-fill time") + evictAll = flag.Bool("evict-all", false, "Evict everything evictable from the cache and exit (no refs file needed). Rooted images and anything younger than --evict-idle survive") platform = flag.String("platform", "linux/amd64", "Image platform to pull") ) @@ -101,6 +105,16 @@ func main() { flag.Usage() os.Exit(2) } + // min-age is the only protection that works across processes (the + // engine's locks are per-process), so on a live node it must not be + // tuned away. Validation hosts (no actors dir) keep full freedom. + const liveNodeIdleFloor = time.Minute + if *evictIdle < liveNodeIdleFloor { + if _, err := os.Stat(ateompath.ActorsDir); err == nil { + log.Fatalf("--evict-idle=%v is below %v with %s present: min-age is the only protection that applies across processes using the cache", + *evictIdle, liveNodeIdleFloor, ateompath.ActorsDir) + } + } ctx := context.Background() if *evictAll { @@ -108,7 +122,7 @@ func main() { // crash-debris orphans before the pass. if _, err := os.Stat(ateompath.ActorsDir); err == nil { // An actors dir is a decent proxy for a live node. - log.Printf("live node: this run is not synchronized with atelet's GC; a concurrent actor start may fail once and heal on retry") + log.Printf("WARNING: %s exists — this looks like a live node, and this run is not synchronized with other processes using the cache", ateompath.ActorsDir) } store, err := newStore() if err != nil { @@ -117,7 +131,7 @@ func main() { stats, err := store.EvictUnused(ctx, math.MaxInt64, false) if errors.Is(err, imagecache.ErrIncompleteEnumeration) { // Gated: nothing was attempted; the error names the unreadable - // or corrupt file. + // or corrupt path. log.Fatalf("evict-all did nothing: %v", err) } if err != nil { @@ -286,7 +300,7 @@ func evictIfLow(ctx context.Context, store *imagecache.Store, cacheRoot string, stats, err := store.EvictUnused(ctx, int64(minFree-free), false) switch { case errors.Is(err, imagecache.ErrIncompleteEnumeration): - // Gated: nothing was attempted; the error names the file to repair. + // Gated: nothing was attempted; the error names the path to repair. // Validation itself goes on — it only needed the disk space. log.Printf("eviction pass skipped, nothing attempted: %v", err) case err != nil: From 8974ffcc868022bf02b988255268613336424c74 Mon Sep 17 00:00:00 2001 From: Ivy Date: Wed, 12 Aug 2026 08:41:52 -0700 Subject: [PATCH 09/10] validate-image-cache: harden live-node guards; address review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Reject --evict-idle < 0 everywhere (mirrors atelet): a negative min-age puts the cutoff in the future, making even in-flight pulls' layers evictable — spurious FAILs in the tool's own CSV. - Fix the inverted severity between the live-node checks: --evict-all with an actors dir present now refuses without --force (it was the higher-risk operation yet only warned, while the idle floor fataled). - looksLikeLiveNode() treats anything but ENOENT as a node, so an EACCES actors dir no longer bypasses the floor or the force gate. - Refs mode logs the same live-node caveat as flush mode: evictIfLow runs the same engine beside whatever else uses the pool. - --evict-all rejects explicitly-set refs-mode flags (flag.Visit). - Fruitless-pass predicate uses the same counters the log line gates on; freeGB names the statfs sentinel instead of printing ~18e9 GB. --- tools/validate-image-cache/README.md | 1 + tools/validate-image-cache/main.go | 69 +++++++++++++++++++++------- 2 files changed, 54 insertions(+), 16 deletions(-) diff --git a/tools/validate-image-cache/README.md b/tools/validate-image-cache/README.md index 18881ebf2..a9f702054 100644 --- a/tools/validate-image-cache/README.md +++ b/tools/validate-image-cache/README.md @@ -78,6 +78,7 @@ go run ./tools/validate-image-cache \ | `--min-free-gb` | 150 | reclaim the shortfall below this free-space floor via the eviction engine | | `--evict-idle` | 10m | eviction min-age: layers and records younger than this are never evicted (minimum 1m on a node with an actors dir); must be far below disk-fill time on small disks | | `--evict-all` | false | evict everything evictable and exit (mutually exclusive with `--refs-file`); rooted images and anything younger than `--evict-idle` survive | +| `--force` | false | allow `--evict-all` on a node with an actors dir | | `--platform` | `linux/amd64` | image platform to pull | ## Output and rerunning diff --git a/tools/validate-image-cache/main.go b/tools/validate-image-cache/main.go index 096fcc3a9..336739568 100644 --- a/tools/validate-image-cache/main.go +++ b/tools/validate-image-cache/main.go @@ -76,10 +76,19 @@ var ( timeout = flag.Duration("timeout", 20*time.Minute, "Per-image timeout") minFreeGB = flag.Uint64("min-free-gb", 150, "Ask the eviction engine to reclaim disk when the cache volume has less free space than this") evictIdle = flag.Duration("evict-idle", 10*time.Minute, "Eviction min-age: layers and records younger than this are never evicted. Minimum 1m on a node with an actors dir. NOTE: if the corpus unpacks faster than this window elapses on a small disk, nothing is evictable while the disk fills — size it well below disk-fill time") - evictAll = flag.Bool("evict-all", false, "Evict everything evictable from the cache and exit (no refs file needed). Rooted images and anything younger than --evict-idle survive") + evictAll = flag.Bool("evict-all", false, "Evict everything evictable from the cache and exit (no refs file needed). Rooted images and anything younger than --evict-idle survive. Requires --force on a node with an actors dir") + force = flag.Bool("force", false, "Allow --evict-all on a node with an actors dir") platform = flag.String("platform", "linux/amd64", "Image platform to pull") ) +// looksLikeLiveNode reports whether the node's actors dir exists — the +// same authority the eviction root set is scanned from. Anything but +// ENOENT counts: an unreadable actors dir is still a node. +func looksLikeLiveNode() bool { + _, err := os.Stat(ateompath.ActorsDir) + return err == nil || !errors.Is(err, os.ErrNotExist) +} + // newStore opens the cache with the options both modes share: min-age // from --evict-idle, and bundle-spec rooting from the node's actors dir // (absent on a validation host, which InUse treats as an empty root set). @@ -105,24 +114,36 @@ func main() { flag.Usage() os.Exit(2) } + if *evictIdle < 0 { + // A negative min-age inverts the veto (the cutoff lands in the + // future), making even in-flight pulls' layers evictable. + log.Fatalf("--evict-idle %v must be >= 0", *evictIdle) + } // min-age is the only protection that works across processes (the // engine's locks are per-process), so on a live node it must not be // tuned away. Validation hosts (no actors dir) keep full freedom. const liveNodeIdleFloor = time.Minute - if *evictIdle < liveNodeIdleFloor { - if _, err := os.Stat(ateompath.ActorsDir); err == nil { - log.Fatalf("--evict-idle=%v is below %v with %s present: min-age is the only protection that applies across processes using the cache", - *evictIdle, liveNodeIdleFloor, ateompath.ActorsDir) - } + if *evictIdle < liveNodeIdleFloor && looksLikeLiveNode() { + log.Fatalf("--evict-idle=%v is below %v with %s present: min-age is the only protection that applies across processes using the cache", + *evictIdle, liveNodeIdleFloor, ateompath.ActorsDir) } ctx := context.Background() if *evictAll { // Flush mode: no refs, no registry auth. New also reclaims any // crash-debris orphans before the pass. - if _, err := os.Stat(ateompath.ActorsDir); err == nil { - // An actors dir is a decent proxy for a live node. - log.Printf("WARNING: %s exists — this looks like a live node, and this run is not synchronized with other processes using the cache", ateompath.ActorsDir) + var stray []string + flag.Visit(func(f *flag.Flag) { + switch f.Name { + case "min-free-gb", "sample", "seed", "out", "parallel", "timeout", "platform": + stray = append(stray, "--"+f.Name) + } + }) + if len(stray) > 0 { + log.Fatalf("%s: only valid with --refs-file", strings.Join(stray, ", ")) + } + if looksLikeLiveNode() && !*force { + log.Fatalf("%s exists — this looks like a live node, and this run is not synchronized with other processes using the cache; re-run with --force to proceed", ateompath.ActorsDir) } store, err := newStore() if err != nil { @@ -138,14 +159,20 @@ func main() { log.Printf("evict-all finished with errors: %v", err) } // Print the summary even on partial failure; err decides the exit code. - log.Printf("evict-all: %d images / %d layers evicted, %.1f GB credited (free now %.0f GB)", - stats.EvictedImages, stats.EvictedLayers, float64(stats.FreedBytes)/1e9, float64(freeBytes(*cacheDir))/1e9) + log.Printf("evict-all: %d images / %d layers evicted, %.1f GB credited (free now %s)", + stats.EvictedImages, stats.EvictedLayers, float64(stats.FreedBytes)/1e9, freeGB(*cacheDir)) if err != nil { os.Exit(1) } return } + if looksLikeLiveNode() { + // Same caveat as flush mode: evictIfLow runs the same engine + // beside whatever else uses this pool. + log.Printf("WARNING: %s exists — this looks like a live node, and evictions during this run are not synchronized with other processes using the cache", ateompath.ActorsDir) + } + refs, err := loadRefs(*refsFile) if err != nil { log.Fatalf("loading refs: %v", err) @@ -308,12 +335,12 @@ func evictIfLow(ctx context.Context, store *imagecache.Store, cacheRoot string, log.Printf("eviction pass finished with errors: %v", err) } if stats.EvictedImages > 0 || stats.EvictedLayers > 0 { - log.Printf("evicted %d images / %d layers, %.1f GB credited (free now %.0f GB)", - stats.EvictedImages, stats.EvictedLayers, float64(stats.FreedBytes)/1e9, float64(freeBytes(cacheRoot))/1e9) + log.Printf("evicted %d images / %d layers, %.1f GB credited (free now %s)", + stats.EvictedImages, stats.EvictedLayers, float64(stats.FreedBytes)/1e9, freeGB(cacheRoot)) } - // Layers retired with an unreadable size file credit zero bytes, so - // bytes alone would call a productive pass fruitless. - if stats.EvictedLayers == 0 && stats.FreedBytes == 0 { + // The same counters the log line above gates on — not bytes, which + // credit zero for a layer retired with an unreadable size file. + if stats.EvictedImages == 0 && stats.EvictedLayers == 0 { lastFruitless = time.Now() } } @@ -325,3 +352,13 @@ func freeBytes(path string) uint64 { } return st.Bavail * uint64(st.Bsize) } + +// freeGB renders free space for logs, naming the statfs-failure sentinel +// instead of printing it as ~18 billion GB. +func freeGB(path string) string { + free := freeBytes(path) + if free == ^uint64(0) { + return "unknown" + } + return fmt.Sprintf("%.0f GB", float64(free)/1e9) +} From b0351cd0c10f7120691ae174d6750f2afef2f90d Mon Sep 17 00:00:00 2001 From: Ivy Date: Wed, 12 Aug 2026 10:04:29 -0700 Subject: [PATCH 10/10] validate-image-cache: uniform live-node force gate; testable flag matrix - Refs mode was evict-all on a loop behind a warning: below --min-free-gb (default 150, larger than most node volumes) every worker slot triggers an uncapped eviction pass. Both modes now require --force on a node with an actors dir; the warning is gone. - Flag validation extracted into runConfig.validate() and table-tested (14 cases): mode XOR, negative idle, live-node floor vs force ordering, stray flags, --min-free-gb overflow. Usage-class errors carry an errUsage sentinel. - Stray-flag check inverted to an allowlist so future flags fail closed; looksLikeLiveNode parameterized (single stat, testable); freeGB sentinel branch tested. - README: Live nodes section (force requirement, floor, both failure outcomes, refs mode flushes repeatedly); "in-flight images are not raced" scoped to this process. --- tools/validate-image-cache/README.md | 22 ++++- tools/validate-image-cache/main.go | 119 ++++++++++++++++-------- tools/validate-image-cache/main_test.go | 96 +++++++++++++++++++ 3 files changed, 191 insertions(+), 46 deletions(-) create mode 100644 tools/validate-image-cache/main_test.go diff --git a/tools/validate-image-cache/README.md b/tools/validate-image-cache/README.md index a9f702054..9d93e792a 100644 --- a/tools/validate-image-cache/README.md +++ b/tools/validate-image-cache/README.md @@ -78,7 +78,7 @@ go run ./tools/validate-image-cache \ | `--min-free-gb` | 150 | reclaim the shortfall below this free-space floor via the eviction engine | | `--evict-idle` | 10m | eviction min-age: layers and records younger than this are never evicted (minimum 1m on a node with an actors dir); must be far below disk-fill time on small disks | | `--evict-all` | false | evict everything evictable and exit (mutually exclusive with `--refs-file`); rooted images and anything younger than `--evict-idle` survive | -| `--force` | false | allow `--evict-all` on a node with an actors dir | +| `--force` | false | allow eviction on a node with an actors dir (required for both modes there) | | `--platform` | `linux/amd64` | image platform to pull | ## Output and rerunning @@ -91,10 +91,22 @@ non-zero if any image failed. Reruns are cheap by design: completed layers stay in the cache (and even an interrupted pull keeps every layer that finished), so re-running the same sample — or just the failed refs — mostly re-validates from local disk. -Eviction only removes layers idle for at least `--evict-idle`, so in-flight -images are not raced; on a small disk with high throughput, set it well -below the time the corpus needs to fill the disk, or nothing will be -evictable while it fills. +Eviction never removes layers or records younger than `--evict-idle`, so +this process's own in-flight images are not raced; on a small disk with +high throughput, set it well below the time the corpus needs to fill the +disk, or nothing will be evictable while it fills. + +## Live nodes + +On a node with an actors dir, **both modes require `--force`** and +`--evict-idle` is floored at 1m. The engine's locks are per-process, so a +run there is not synchronized with the node agent's own GC and pulls. +Bundle-spec rooting protects placed actors' images and min-age protects +fresh pulls, but a layer that ages past `--evict-idle` and is reused +mid-pass can be evicted out from under an actor: a not-yet-mounted start +fails once and heals on re-pull; an already-mounted actor can take I/O +errors from a removed lowerdir. Note that refs mode evicts too — below +`--min-free-gb` (default 150) it flushes repeatedly, not once. ## Scaling out diff --git a/tools/validate-image-cache/main.go b/tools/validate-image-cache/main.go index 336739568..2178fd035 100644 --- a/tools/validate-image-cache/main.go +++ b/tools/validate-image-cache/main.go @@ -77,18 +77,77 @@ var ( minFreeGB = flag.Uint64("min-free-gb", 150, "Ask the eviction engine to reclaim disk when the cache volume has less free space than this") evictIdle = flag.Duration("evict-idle", 10*time.Minute, "Eviction min-age: layers and records younger than this are never evicted. Minimum 1m on a node with an actors dir. NOTE: if the corpus unpacks faster than this window elapses on a small disk, nothing is evictable while the disk fills — size it well below disk-fill time") evictAll = flag.Bool("evict-all", false, "Evict everything evictable from the cache and exit (no refs file needed). Rooted images and anything younger than --evict-idle survive. Requires --force on a node with an actors dir") - force = flag.Bool("force", false, "Allow --evict-all on a node with an actors dir") + force = flag.Bool("force", false, "Allow eviction on a node with an actors dir") platform = flag.String("platform", "linux/amd64", "Image platform to pull") ) -// looksLikeLiveNode reports whether the node's actors dir exists — the -// same authority the eviction root set is scanned from. Anything but -// ENOENT counts: an unreadable actors dir is still a node. -func looksLikeLiveNode() bool { - _, err := os.Stat(ateompath.ActorsDir) +// looksLikeLiveNode reports whether dir — the node's actors dir, the +// same authority the eviction root set is scanned from — exists. +// Anything but ENOENT counts: an unreadable actors dir is still a node. +func looksLikeLiveNode(dir string) bool { + _, err := os.Stat(dir) return err == nil || !errors.Is(err, os.ErrNotExist) } +// liveNodeIdleFloor: min-age is the only protection that works across +// processes (the engine's locks are per-process), so on a live node it +// must not be tuned away. Validation hosts keep full freedom. +const liveNodeIdleFloor = time.Minute + +// errUsage marks validation failures that should print flag usage. +var errUsage = errors.New("usage") + +// runConfig is the flag state validate checks — a plain struct so the +// combination matrix is table-testable. +type runConfig struct { + cacheDir, refsFile string + evictAll, force bool + evictIdle time.Duration + minFreeGB uint64 + live bool + setFlags []string // flag names given explicitly +} + +func (c runConfig) validate() error { + if c.cacheDir == "" || (c.refsFile == "" && !c.evictAll) || (c.refsFile != "" && c.evictAll) { + return fmt.Errorf("%w: need --cache-dir plus exactly one of --refs-file or --evict-all", errUsage) + } + if c.evictIdle < 0 { + // A negative min-age inverts the veto (the cutoff lands in the + // future), making even in-flight pulls' layers evictable. + return fmt.Errorf("--evict-idle %v must be >= 0", c.evictIdle) + } + if c.minFreeGB > math.MaxUint64/uint64(1e9) { + return fmt.Errorf("--min-free-gb %d overflows a byte count", c.minFreeGB) + } + if c.evictAll { + var stray []string + for _, name := range c.setFlags { + switch name { + case "cache-dir", "evict-idle", "evict-all", "force": + // The whole flush-mode surface; new flags fail closed. + default: + stray = append(stray, "--"+name) + } + } + if len(stray) > 0 { + return fmt.Errorf("%w: %s: only valid with --refs-file", errUsage, strings.Join(stray, ", ")) + } + } + if c.live { + if c.evictIdle < liveNodeIdleFloor { + return fmt.Errorf("--evict-idle=%v is below %v with %s present: min-age is the only protection that applies across processes using the cache", + c.evictIdle, liveNodeIdleFloor, ateompath.ActorsDir) + } + // Both modes evict here (evictIfLow is a low-water flush on a + // loop), unsynchronized with every other user of the pool. + if !c.force { + return fmt.Errorf("%s exists — this looks like a live node, and evictions are not synchronized with other processes using the cache; re-run with --force to proceed", ateompath.ActorsDir) + } + } + return nil +} + // newStore opens the cache with the options both modes share: min-age // from --evict-idle, and bundle-spec rooting from the node's actors dir // (absent on a validation host, which InUse treats as an empty root set). @@ -109,42 +168,26 @@ type result struct { func main() { flag.Parse() - if *cacheDir == "" || (*refsFile == "" && !*evictAll) || (*refsFile != "" && *evictAll) { - fmt.Fprintln(os.Stderr, "need --cache-dir plus exactly one of --refs-file or --evict-all") - flag.Usage() - os.Exit(2) + cfg := runConfig{ + cacheDir: *cacheDir, refsFile: *refsFile, + evictAll: *evictAll, force: *force, + evictIdle: *evictIdle, minFreeGB: *minFreeGB, + live: looksLikeLiveNode(ateompath.ActorsDir), } - if *evictIdle < 0 { - // A negative min-age inverts the veto (the cutoff lands in the - // future), making even in-flight pulls' layers evictable. - log.Fatalf("--evict-idle %v must be >= 0", *evictIdle) - } - // min-age is the only protection that works across processes (the - // engine's locks are per-process), so on a live node it must not be - // tuned away. Validation hosts (no actors dir) keep full freedom. - const liveNodeIdleFloor = time.Minute - if *evictIdle < liveNodeIdleFloor && looksLikeLiveNode() { - log.Fatalf("--evict-idle=%v is below %v with %s present: min-age is the only protection that applies across processes using the cache", - *evictIdle, liveNodeIdleFloor, ateompath.ActorsDir) + flag.Visit(func(f *flag.Flag) { cfg.setFlags = append(cfg.setFlags, f.Name) }) + if err := cfg.validate(); err != nil { + if errors.Is(err, errUsage) { + fmt.Fprintln(os.Stderr, err) + flag.Usage() + os.Exit(2) + } + log.Fatal(err) } ctx := context.Background() if *evictAll { // Flush mode: no refs, no registry auth. New also reclaims any // crash-debris orphans before the pass. - var stray []string - flag.Visit(func(f *flag.Flag) { - switch f.Name { - case "min-free-gb", "sample", "seed", "out", "parallel", "timeout", "platform": - stray = append(stray, "--"+f.Name) - } - }) - if len(stray) > 0 { - log.Fatalf("%s: only valid with --refs-file", strings.Join(stray, ", ")) - } - if looksLikeLiveNode() && !*force { - log.Fatalf("%s exists — this looks like a live node, and this run is not synchronized with other processes using the cache; re-run with --force to proceed", ateompath.ActorsDir) - } store, err := newStore() if err != nil { log.Fatalf("opening cache: %v", err) @@ -167,12 +210,6 @@ func main() { return } - if looksLikeLiveNode() { - // Same caveat as flush mode: evictIfLow runs the same engine - // beside whatever else uses this pool. - log.Printf("WARNING: %s exists — this looks like a live node, and evictions during this run are not synchronized with other processes using the cache", ateompath.ActorsDir) - } - refs, err := loadRefs(*refsFile) if err != nil { log.Fatalf("loading refs: %v", err) diff --git a/tools/validate-image-cache/main_test.go b/tools/validate-image-cache/main_test.go new file mode 100644 index 000000000..ce71a8ae7 --- /dev/null +++ b/tools/validate-image-cache/main_test.go @@ -0,0 +1,96 @@ +// Copyright 2026 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package main + +import ( + "errors" + "math" + "path/filepath" + "strings" + "testing" + "time" +) + +func TestRunConfigValidate(t *testing.T) { + valid := runConfig{cacheDir: "/c", refsFile: "r.txt", evictIdle: 10 * time.Minute, minFreeGB: 150} + cases := []struct { + name string + mutate func(*runConfig) + wantErr string // substring of the error; "" means valid + usage bool // wraps errUsage + }{ + {"refs mode defaults", func(c *runConfig) {}, "", false}, + {"missing cache-dir", func(c *runConfig) { c.cacheDir = "" }, "exactly one", true}, + {"neither mode", func(c *runConfig) { c.refsFile = "" }, "exactly one", true}, + {"both modes", func(c *runConfig) { c.evictAll = true }, "exactly one", true}, + {"negative idle rejected everywhere", func(c *runConfig) { c.evictIdle = -time.Hour }, "must be >= 0", false}, + {"low idle fine on a validation host", func(c *runConfig) { c.evictIdle = 10 * time.Second }, "", false}, + {"low idle floored on a live node", func(c *runConfig) { c.evictIdle = 10 * time.Second; c.live = true; c.force = true }, "is below", false}, + {"live node refuses without force", func(c *runConfig) { c.live = true }, "--force", false}, + {"live node with force", func(c *runConfig) { c.live = true; c.force = true }, "", false}, + {"live flush refuses without force", func(c *runConfig) { c.refsFile = ""; c.evictAll = true; c.live = true }, "--force", false}, + {"flush with a refs-mode flag", func(c *runConfig) { + c.refsFile = "" + c.evictAll = true + c.setFlags = []string{"cache-dir", "evict-all", "parallel"} + }, "only valid with --refs-file", true}, + {"flush with its own flags", func(c *runConfig) { + c.refsFile = "" + c.evictAll = true + c.setFlags = []string{"cache-dir", "evict-idle", "evict-all", "force"} + }, "", false}, + {"future flags fail closed", func(c *runConfig) { + c.refsFile = "" + c.evictAll = true + c.setFlags = []string{"some-new-flag"} + }, "only valid with --refs-file", true}, + {"min-free-gb overflow", func(c *runConfig) { c.minFreeGB = math.MaxUint64 }, "overflows", false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + c := valid + tc.mutate(&c) + err := c.validate() + if tc.wantErr == "" { + if err != nil { + t.Fatalf("validate() = %v, want nil", err) + } + return + } + if err == nil || !strings.Contains(err.Error(), tc.wantErr) { + t.Fatalf("validate() = %v, want error containing %q", err, tc.wantErr) + } + if got := errors.Is(err, errUsage); got != tc.usage { + t.Errorf("errors.Is(err, errUsage) = %v, want %v", got, tc.usage) + } + }) + } +} + +func TestLooksLikeLiveNode(t *testing.T) { + dir := t.TempDir() + if !looksLikeLiveNode(dir) { + t.Error("existing dir: want true") + } + if looksLikeLiveNode(filepath.Join(dir, "missing")) { + t.Error("missing dir: want false") + } +} + +func TestFreeGBNamesTheSentinel(t *testing.T) { + if got := freeGB(filepath.Join(t.TempDir(), "missing")); got != "unknown" { + t.Errorf("freeGB(missing path) = %q, want \"unknown\"", got) + } +}