From dbe0d73ed9678d3fcb7d8510c688bef378060256 Mon Sep 17 00:00:00 2001 From: Luther Monson Date: Sun, 6 Sep 2026 09:19:31 -0700 Subject: [PATCH 1/2] fix(runtime): bound teardownContainer's post-kill exit wait MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit teardownContainer had its own unbounded `<-exitCh` — the exact bug #190 fixed in Destroy, still present in the per-container teardown shared by startup CleanOrphans and the #187 periodic reaper. A dead containerd shim never delivers the exit event, so the wait never returns. That is worse here than it was in Destroy: CleanOrphans runs BEFORE the scheduler accepts jobs, so a single leftover container with a dead shim hung daemon startup indefinitely and the node never came back — no scheduler, no webhook receiver, no jobs, and nothing in the log after the orphan sweep line. Only a manual containerd cleanup got the host back. Now uses the waitTaskExit helper and the destroyKillWait bound already on main from #190, and logs a warning when the bound is hit instead of blocking forever. The periodic reaper is unaffected either way: it only ever passes provably-dead tasks, so the Running branch is a no-op for it. --- pkg/runtime/runtime.go | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/pkg/runtime/runtime.go b/pkg/runtime/runtime.go index 43ca865..a860683 100644 --- a/pkg/runtime/runtime.go +++ b/pkg/runtime/runtime.go @@ -498,7 +498,20 @@ func (r *Runtime) teardownContainer(ctx context.Context, c client.Container, log } exitCh, err := task.Wait(ctx) if err == nil { - <-exitCh + // Bounded, for the same reason destroySteps' wait is + // (#190): a dead containerd shim never delivers the + // exit event, and this used to be a bare <-exitCh. + // That is far worse here than in Destroy — the + // startup CleanOrphans caller runs BEFORE the + // scheduler accepts jobs, so one leftover container + // with a dead shim hung daemon startup indefinitely + // and the node never came back. The periodic reaper + // only ever passes provably-dead tasks, so this + // branch is a no-op for it either way. + if !waitTaskExit(ctx, exitCh, destroyKillWait) { + log.Warn("killed orphan task never reported exit; proceeding with teardown", + "waited", destroyKillWait) + } } } } From f524985e854eec2b80afdd465a443411c04a719c Mon Sep 17 00:00:00 2001 From: Luther Monson Date: Sun, 6 Sep 2026 09:19:47 -0700 Subject: [PATCH 2/2] fix(cacheprune,imagegc): make `cache clear containerd --all` actually force eviction MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit pruneContainerd took no `all` parameter, so the flag was dropped for the containerd target: the CLI sends All=true, the daemon ran the ordinary watermark-driven pass, correctly evicted nothing on a node whose thresholds were not tripped, and reported success with 0 records removed. An operator chasing disk on a Windows node had no override and no hint the flag was being ignored — the command looked broken rather than declined. The flag is now threaded through to a new imagegc.PlanForced / Collector.CollectAll, which evicts every unprotected record regardless of disk pressure. It forces the POLICY, not the safety: images backing running containers and the node's pinned runner images stay an absolute veto, exactly as in the automatic pass. A forced pass also ignores the exhausted-backoff suppression — that failsafe exists to stop the daemon's own timer spinning on a store it cannot shrink, and a human typing --all is not the daemon's timer. Routing goes through a pure containerdPass() so it is table-testable without a live containerd, and the disabled-collector error names the pass it would have run: that string is the only externally visible evidence that the flag reached this function at all, which is precisely the hop where it used to vanish. Three follow-ups on the same path: - A forced pass correctly never ARMS the exhausted backoff, but it never CLEARED one already armed. The backoff's premise is "everything evictable is gone and we are still over the line"; an operator running --all evicts a SUPERSET of what the automatic pass may touch, so a forced pass that frees records or gets back under the watermarks has falsified it. Leaving the suppression in place meant --all freed the disk and the automatic collector stayed muted for up to 30 more minutes anyway, on a premise a human had just disproved. - The shared eviction log line printed bytes_to_free_gib from plan.BytesToFree, which PlanForced never sets. On a forced pass it always read "0", i.e. "nothing to reclaim", on exactly the pass an operator runs when they believe there is. It is now logged only for the watermark pass, the only one with a byte budget. - PlanEviction was computed and then discarded whenever force was set — a full protected-filter and LRU sort of every candidate on the node, thrown away on the one code path an operator is watching. Now if/else. Tests: PlanForced's protections and its precedence over the watermark policy; the --all routing through Prune -> pruneContainerd (asserting on the pass name, since a nil ImageGC returns at the disabled guard and the old "one result came back" assertion passed identically whether the flag was threaded or hard-coded). Collector.CollectAll had zero coverage despite being the operator's disk-recovery command, because every entry point needs a live *containerd.Client; the four containerd-facing calls collect() makes are now overridable seams (nil = use the package function, so the production path is unchanged) and the pass's control flow is covered: the CollectAll -> PlanForced wiring on an unpressured disk, the pinned/live-image veto holding under force, the abort when RunningContainers fails, the backoff not-armed / cleared / left-alone cases, and that a forced pass passes no stop function. --- pkg/cacheprune/cacheprune.go | 56 ++++++- pkg/cacheprune/cacheprune_test.go | 81 +++++++++ pkg/imagegc/collector.go | 167 ++++++++++++++++--- pkg/imagegc/collector_test.go | 262 ++++++++++++++++++++++++++++++ pkg/imagegc/plan.go | 39 +++++ pkg/imagegc/plan_test.go | 96 +++++++++++ 6 files changed, 674 insertions(+), 27 deletions(-) create mode 100644 pkg/imagegc/collector_test.go diff --git a/pkg/cacheprune/cacheprune.go b/pkg/cacheprune/cacheprune.go index 5a3ff80..bf1cd7a 100644 --- a/pkg/cacheprune/cacheprune.go +++ b/pkg/cacheprune/cacheprune.go @@ -141,7 +141,7 @@ func (p *Pruner) Prune(ctx context.Context, targets []string, all bool) []Result case TargetBuildKit: results = append(results, p.pruneBuildKit(ctx, all)) case TargetContainerd: - results = append(results, p.pruneContainerd(ctx)) + results = append(results, p.pruneContainerd(ctx, all)) } } return results @@ -199,17 +199,63 @@ func (p *Pruner) pruneBuildKit(ctx context.Context, all bool) Result { return res } -// pruneContainerd runs one image-collector pass. Under no disk pressure +// collectPass names which image-collector entry point a containerd prune +// request maps to. Split out as a pure function so the routing is +// table-testable without a live containerd — the bug it guards against is +// exactly that the request's `all` flag never reached the collector at all. +type collectPass string + +const ( + // passWatermark is the ordinary policy pass: evict only what disk + // pressure justifies. + passWatermark collectPass = "watermark" + // passForced is the operator override: evict every unprotected record. + passForced collectPass = "forced" +) + +func containerdPass(all bool) collectPass { + if all { + return passForced + } + return passWatermark +} + +// pruneContainerd runs one image-collector pass. +// +// all=false runs the ordinary watermark-driven pass. Under no disk pressure // that is a no-op by design: the collector protects images referenced by // running containers and the node's pinned runner images, and evicting a // warm image store on demand would only cost a re-pull. -func (p *Pruner) pruneContainerd(ctx context.Context) Result { +// +// all=true is the operator override. It used to be SILENTLY DROPPED — this +// function took no all parameter at all, so `ephemerd cache clear containerd +// --all` ran the same watermark pass, correctly evicted nothing on a node +// whose thresholds were not tripped, and reported success with zero records +// removed. An operator chasing disk on a Windows node had no way to force +// eviction and no way to tell the flag was being ignored. It now forces the +// policy open (CollectAll) while keeping the safety: live-container images +// and pinned runner images are still an absolute veto. +func (p *Pruner) pruneContainerd(ctx context.Context, all bool) Result { res := Result{Name: TargetContainerd} + pass := containerdPass(all) if p.ImageGC == nil { - res.Err = fmt.Errorf("image gc is disabled on this node") + // Naming the pass we WOULD have run is not decoration: it is the + // only externally visible evidence that the request's `all` flag + // reached this function at all, which is exactly the hop where it + // used to be dropped silently. It also tells an operator whose + // `--all` did nothing that the flag was understood and the + // collector is simply not configured on this node. + res.Err = fmt.Errorf("image gc is disabled on this node (requested pass: %s)", pass) return res } - gcRes, err := p.ImageGC.Collect(ctx) + var gcRes imagegc.Result + var err error + switch pass { + case passForced: + gcRes, err = p.ImageGC.CollectAll(ctx) + default: + gcRes, err = p.ImageGC.Collect(ctx) + } if err != nil { res.Err = err return res diff --git a/pkg/cacheprune/cacheprune_test.go b/pkg/cacheprune/cacheprune_test.go index ff11fdf..2a8f2cb 100644 --- a/pkg/cacheprune/cacheprune_test.go +++ b/pkg/cacheprune/cacheprune_test.go @@ -1,7 +1,9 @@ package cacheprune import ( + "context" "reflect" + "strings" "testing" ) @@ -73,3 +75,82 @@ func TestResolveTargets(t *testing.T) { func TestPruner_ImplementsInterface(t *testing.T) { var _ Interface = (*Pruner)(nil) } + +// TestContainerdPass pins the fix for a documented paper cut: pruneContainerd +// used to take no `all` parameter at all, so `ephemerd cache clear containerd +// --all` was accepted, silently downgraded to the ordinary watermark pass, +// and reported "0 records removed" on a node whose thresholds were not +// tripped. The operator had no override and no hint the flag was ignored. +func TestContainerdPass(t *testing.T) { + if got := containerdPass(false); got != passWatermark { + t.Errorf("containerdPass(false) = %q, want %q — a plain prune must stay policy-driven", got, passWatermark) + } + if got := containerdPass(true); got != passForced { + t.Errorf("containerdPass(true) = %q, want %q — --all must reach the collector", got, passForced) + } +} + +// TestPruneContainerd_DisabledIsUnchangedByAll: --all forces the eviction +// POLICY open, it does not conjure a collector. A node without image GC must +// report the same clear reason either way (the requested pass is appended for +// diagnosis; the reason itself does not change). +func TestPruneContainerd_DisabledIsUnchangedByAll(t *testing.T) { + p := &Pruner{} + for _, all := range []bool{false, true} { + res := p.pruneContainerd(context.Background(), all) + if res.Name != TargetContainerd { + t.Errorf("all=%v: Name = %q, want %q", all, res.Name, TargetContainerd) + } + if res.Err == nil { + t.Fatalf("all=%v: expected an error when image gc is disabled", all) + } + if !strings.Contains(res.Err.Error(), "image gc is disabled") { + t.Errorf("all=%v: err = %v, want it to name the disabled collector", all, res.Err) + } + } +} + +// TestPrune_ContainerdTargetThreadsAll walks the PUBLIC entry point to prove +// the flag survives the Prune -> pruneContainerd hop — the hop where it used +// to be dropped silently, so `cache clear containerd --all` ran a watermark +// pass, evicted nothing, and reported success. +// +// It asserts on the pass NAME carried in the disabled-collector error, which +// is the only externally visible evidence of what the flag became. Asserting +// only "one result came back" (what this test used to do) proves nothing: a +// nil ImageGC returns at the disabled guard, so the previous version of this +// test passed identically whether `all` was threaded or hard-coded to false. +func TestPrune_ContainerdTargetThreadsAll(t *testing.T) { + p := &Pruner{} + for _, tc := range []struct { + all bool + want collectPass + }{ + {false, passWatermark}, + {true, passForced}, + } { + results := p.Prune(context.Background(), []string{TargetContainerd}, tc.all) + if len(results) != 1 || results[0].Name != TargetContainerd { + t.Fatalf("all=%v: results = %+v, want one containerd result", tc.all, results) + } + if results[0].Err == nil { + t.Fatalf("all=%v: expected the disabled-collector error", tc.all) + } + if got := results[0].Err.Error(); !strings.Contains(got, string(tc.want)) { + t.Errorf("all=%v: err = %q, want it to name pass %q — the flag did not survive Prune -> pruneContainerd", tc.all, got, tc.want) + } + } +} + +// TestPrune_ContainerdTargetThreadsAll_DistinguishesThePasses guards the +// assertion above against a degenerate pass naming (e.g. both constants +// becoming the same string), which would make it pass vacuously. +func TestPrune_ContainerdTargetThreadsAll_DistinguishesThePasses(t *testing.T) { + if passWatermark == passForced { + t.Fatal("passWatermark and passForced are identical; the threading assertions above cannot distinguish them") + } + if strings.Contains(string(passForced), string(passWatermark)) || + strings.Contains(string(passWatermark), string(passForced)) { + t.Fatalf("pass names %q and %q are substrings of one another; a Contains-based assertion would be ambiguous", passWatermark, passForced) + } +} diff --git a/pkg/imagegc/collector.go b/pkg/imagegc/collector.go index 4486a3b..9c8e25d 100644 --- a/pkg/imagegc/collector.go +++ b/pkg/imagegc/collector.go @@ -82,6 +82,49 @@ type Collector struct { // now is time.Now, swappable in tests. now func() time.Time + + // The four containerd-facing calls collect() makes, as overridable + // seams. Nil means "use the package function", so the production path + // is unchanged and New never has to set them. + // + // They exist because collect()'s CONTROL FLOW — which planner a forced + // pass picks, that a RunningContainers error aborts before anything is + // deleted, whether the exhausted-backoff is armed or cleared — is the + // part that has been wrong, and none of it was reachable in a test: + // every entry point needs a live *containerd.Client, so CollectAll had + // zero coverage while being the operator's disk-recovery command. + resolveNamespacesFn func(ctx context.Context, c *client.Client, exact, prefixes []string) ([]string, error) + runningContainersFn func(ctx context.Context, c *client.Client, log *slog.Logger) (ids, refs map[string]struct{}, err error) + listCandidatesFn func(ctx context.Context, c *client.Client, nss []string, log *slog.Logger) ([]Candidate, error) + evictFn func(ctx context.Context, c *client.Client, cands []Candidate, synchronous bool, log *slog.Logger, stop func() bool) int +} + +func (c *Collector) resolveNamespaces(ctx context.Context, exact, prefixes []string) ([]string, error) { + if c.resolveNamespacesFn != nil { + return c.resolveNamespacesFn(ctx, c.cfg.Client, exact, prefixes) + } + return ResolveNamespaces(ctx, c.cfg.Client, exact, prefixes) +} + +func (c *Collector) runningContainers(ctx context.Context, log *slog.Logger) (ids, refs map[string]struct{}, err error) { + if c.runningContainersFn != nil { + return c.runningContainersFn(ctx, c.cfg.Client, log) + } + return RunningContainers(ctx, c.cfg.Client, log) +} + +func (c *Collector) listCandidates(ctx context.Context, nss []string, log *slog.Logger) ([]Candidate, error) { + if c.listCandidatesFn != nil { + return c.listCandidatesFn(ctx, c.cfg.Client, nss, log) + } + return ListCandidates(ctx, c.cfg.Client, nss, log) +} + +func (c *Collector) evict(ctx context.Context, cands []Candidate, synchronous bool, log *slog.Logger, stop func() bool) int { + if c.evictFn != nil { + return c.evictFn(ctx, c.cfg.Client, cands, synchronous, log, stop) + } + return Evict(ctx, c.cfg.Client, cands, synchronous, log, stop) } // New builds a Collector. Returns nil when no mechanism is configured, so @@ -121,6 +164,9 @@ type Result struct { // Skipped reports that the pass did not run because the failsafe // backoff was still in effect. Skipped bool + // Forced reports that the pass ignored the watermarks because an + // operator asked for a full eviction. + Forced bool } // EnsureHeadroom runs a collection pass if — and only if — the disk is @@ -170,6 +216,26 @@ func (c *Collector) EnsureHeadroom(ctx context.Context) { // further passes for ExhaustedBackoff so the daemon does not spin listing // and re-planning a store it cannot shrink. func (c *Collector) Collect(ctx context.Context) (Result, error) { + return c.collect(ctx, false) +} + +// CollectAll runs one pass that evicts EVERY unprotected image record, +// whether or not the disk is over a watermark. It is the operator override +// behind `ephemerd cache clear containerd --all`. +// +// It forces the POLICY, not the safety: images backing running containers +// and the node's pinned runner images are protected here exactly as they are +// in the automatic pass (see PlanForced). It also ignores the exhausted +// backoff — that failsafe exists to stop the daemon spinning on its own +// timer, and a human typing --all is not the daemon's timer. +// +// Cost is a cold image store: the next job on this node re-pulls everything. +// That is the operator's stated intent when they pass --all. +func (c *Collector) CollectAll(ctx context.Context) (Result, error) { + return c.collect(ctx, true) +} + +func (c *Collector) collect(ctx context.Context, force bool) (Result, error) { if c == nil { return Result{}, nil } @@ -178,6 +244,7 @@ func (c *Collector) Collect(ctx context.Context) (Result, error) { log := c.cfg.Log var res Result + res.Forced = force before, err := diskspace.Check(c.cfg.Path) if err != nil { @@ -186,18 +253,19 @@ func (c *Collector) Collect(ctx context.Context) (Result, error) { res.Before, res.After = before, before res.Pressure = Evaluate(before, c.cfg.Thresholds) - if res.Pressure.Over && c.now().Before(c.exhaustedUntil) { + if !force && res.Pressure.Over && c.now().Before(c.exhaustedUntil) { res.Skipped = true log.Debug("image gc suppressed; previous pass exhausted every evictable image", "until", c.exhaustedUntil) return res, nil } // Nothing to do: not under pressure and no age backstop configured. - if !res.Pressure.Over && c.cfg.MaxAge <= 0 { + // A forced pass always has something to do. + if !force && !res.Pressure.Over && c.cfg.MaxAge <= 0 { return res, nil } - nss, err := ResolveNamespaces(ctx, c.cfg.Client, c.cfg.Namespaces, c.cfg.NamespacePrefixes) + nss, err := c.resolveNamespaces(ctx, c.cfg.Namespaces, c.cfg.NamespacePrefixes) if err != nil { return res, err } @@ -205,7 +273,7 @@ func (c *Collector) Collect(ctx context.Context) (Result, error) { return res, nil } - liveIDs, running, err := RunningContainers(ctx, c.cfg.Client, log) + liveIDs, running, err := c.runningContainers(ctx, log) if err != nil { // Without the live set we cannot guarantee we won't evict an // image a running job needs. Abort rather than risk it. @@ -213,7 +281,7 @@ func (c *Collector) Collect(ctx context.Context) (Result, error) { } protected := ProtectedSet(c.cfg.PinnedImages, running) - cands, err := ListCandidates(ctx, c.cfg.Client, nss, log) + cands, err := c.listCandidates(ctx, nss, log) if err != nil { return res, err } @@ -236,47 +304,72 @@ func (c *Collector) Collect(ctx context.Context) (Result, error) { cutoff := c.now().UTC().Add(-c.cfg.MaxAge) aged := PlanByAge(cands, protected, cutoff) if len(aged) > 0 { - res.AgeEvicted = Evict(ctx, c.cfg.Client, aged, false, log, nil) + res.AgeEvicted = c.evict(ctx, aged, false, log, nil) log.Info("image gc: age backstop evicted records", "count", res.AgeEvicted, "max_age", c.cfg.MaxAge) cands = removeEvicted(cands, aged) } } - if !res.Pressure.Over { + if !force && !res.Pressure.Over { res.After, _ = diskspace.Check(c.cfg.Path) return res, nil } - plan := PlanEviction(cands, protected, before, c.cfg.Thresholds) + // A forced pass takes everything unprotected instead of stopping at the + // low watermark — that is the whole point of the operator override. The + // two are exclusive: computing PlanEviction unconditionally and then + // throwing it away cost a full protected-filter and LRU sort of every + // candidate on the node, on the one code path an operator is watching. + var plan Plan + if force { + plan = PlanForced(cands, protected) + } else { + plan = PlanEviction(cands, protected, before, c.cfg.Thresholds) + } res.Protected = plan.Protected - log.Info("image gc: disk over high watermark", + attrs := []any{ "path", c.cfg.Path, + "forced", force, "used_percent", round1(before.UsedPercent()), "free_gib", round1(diskspace.GiB(before.FreeBytes)), "total_gib", round1(diskspace.GiB(before.TotalBytes)), "reason", plan.ReasonString(), - "bytes_to_free_gib", round1(diskspace.GiB(plan.BytesToFree)), "candidates", len(cands), "planned", len(plan.Evict), - "protected", plan.Protected) + "protected", plan.Protected, + } + if !force { + // Only the watermark pass has a byte budget. PlanForced leaves + // BytesToFree zero by construction (it has no target — it takes + // everything unprotected), so logging it on a forced pass printed a + // constant "bytes_to_free_gib=0" that read as "nothing to reclaim" + // on exactly the pass an operator runs when they believe there is. + attrs = append(attrs, "bytes_to_free_gib", round1(diskspace.GiB(plan.BytesToFree))) + } + log.Info("image gc: evicting image records", attrs...) // Stop as soon as a real reading clears both watermarks; the planned - // sizes are only a budgeting hint. + // sizes are only a budgeting hint. A forced pass has no stopping point + // short of the whole plan, so it passes no stop function. cleared := false - res.Evicted = Evict(ctx, c.cfg.Client, plan.Evict, true, log, func() bool { - u, err := diskspace.Check(c.cfg.Path) - if err != nil { + var stop func() bool + if !force { + stop = func() bool { + u, err := diskspace.Check(c.cfg.Path) + if err != nil { + return false + } + res.After = u + if !Evaluate(u, c.cfg.Thresholds).Over { + cleared = true + return true + } return false } - res.After = u - if !Evaluate(u, c.cfg.Thresholds).Over { - cleared = true - return true - } - return false - }) + } + res.Evicted = c.evict(ctx, plan.Evict, true, log, stop) after, err := diskspace.Check(c.cfg.Path) if err == nil { @@ -284,6 +377,36 @@ func (c *Collector) Collect(ctx context.Context) (Result, error) { cleared = !Evaluate(after, c.cfg.Thresholds).Over } + // A forced pass was never chasing a watermark, so it must not arm the + // exhausted-backoff failsafe: doing so would let one operator-run + // `cache clear --all` silence the AUTOMATIC collector for the next 30 + // minutes on a node that is genuinely filling up. + // + // The mirror image matters just as much and used to be missing: a forced + // pass that actually reclaimed something must CLEAR a backoff that is + // already armed. The backoff's premise is "everything evictable is gone + // and we are still over the line". An operator running --all evicts a + // SUPERSET of what the automatic pass was allowed to touch, so a pass + // that frees records or gets the disk back under the watermarks has + // falsified that premise — and leaving the suppression in place kept the + // automatic collector idle for up to 30 more minutes on a node the + // operator had just fixed. + if force { + reclaimed := res.Evicted+res.AgeEvicted > 0 + if !c.exhaustedUntil.IsZero() && (reclaimed || cleared) { + c.exhaustedUntil = time.Time{} + log.Info("image gc: forced pass reclaimed space; clearing the exhausted-backoff suppression", + "evicted", res.Evicted, "age_evicted", res.AgeEvicted, "under_watermarks", cleared) + } + log.Info("image gc: forced pass complete", + "evicted", res.Evicted, + "protected", res.Protected, + "used_percent", round1(res.After.UsedPercent()), + "free_gib", round1(diskspace.GiB(res.After.FreeBytes)), + "reclaimed_gib", round1(diskspace.GiB(saturatingSub(res.After.FreeBytes, before.FreeBytes)))) + return res, nil + } + switch { case cleared: log.Info("image gc: complete", diff --git a/pkg/imagegc/collector_test.go b/pkg/imagegc/collector_test.go new file mode 100644 index 0000000..5aeb9cb --- /dev/null +++ b/pkg/imagegc/collector_test.go @@ -0,0 +1,262 @@ +package imagegc + +import ( + "context" + "errors" + "io" + "log/slog" + "math" + "testing" + "time" + + "github.com/containerd/containerd/v2/client" +) + +func quietLog() *slog.Logger { return slog.New(slog.NewTextHandler(io.Discard, nil)) } + +// testCollector builds a Collector with the four containerd-facing calls +// stubbed. New() cannot be used here — it requires a live *containerd.Client +// — which is precisely why CollectAll had no coverage at all despite being +// the operator's disk-recovery command. +// +// Thresholds default to "always over" (an impossible MinFreeBytes) so the +// watermark pass has something to do without needing a full disk; individual +// tests override. +func testCollector(t *testing.T, cands []Candidate) *Collector { + t.Helper() + c := &Collector{ + cfg: Config{ + Path: t.TempDir(), + Thresholds: Thresholds{MinFreeBytes: math.MaxInt64}, + ExhaustedBackoff: DefaultExhaustedBackoff, + Log: quietLog(), + }, + now: time.Now, + resolveNamespacesFn: func(context.Context, *client.Client, []string, []string) ([]string, error) { + return []string{"ephemerd"}, nil + }, + runningContainersFn: func(context.Context, *client.Client, *slog.Logger) (map[string]struct{}, map[string]struct{}, error) { + return map[string]struct{}{}, map[string]struct{}{}, nil + }, + listCandidatesFn: func(context.Context, *client.Client, []string, *slog.Logger) ([]Candidate, error) { + return cands, nil + }, + } + return c +} + +func candidate(ns, name string, size int64, age time.Duration) Candidate { + return Candidate{ + Namespace: ns, + Name: name, + SizeBytes: size, + LastAccessed: time.Now().UTC().Add(-age), + } +} + +// TestCollectAll_ForcesThePlannerOpen pins the CollectAll -> collect(force) +// -> PlanForced wiring. +// +// PlanEviction is watermark-driven: on a node whose thresholds are NOT +// tripped it correctly plans nothing. That is right for the timer and wrong +// for `ephemerd cache clear containerd --all`, and getting it wrong is the +// bug this whole path exists to fix — the operator saw "0 records removed" +// and concluded the command was broken. So the assertion is specifically +// that a forced pass evicts everything unprotected on a disk that is under +// no pressure at all. +func TestCollectAll_ForcesThePlannerOpen(t *testing.T) { + cands := []Candidate{ + candidate("ephemerd", "docker.io/library/alpine:3", 100, time.Hour), + candidate("ephemerd", "docker.io/library/golang:1", 200, 2*time.Hour), + candidate("ephemerd", "ghcr.io/pinned/runner:v1", 300, 3*time.Hour), + } + c := testCollector(t, cands) + // No pressure whatsoever: a plain Collect must plan nothing. + c.cfg.Thresholds = Thresholds{HighUsedPercent: 100.1} // unreachable: UsedPercent() never exceeds 100, so "not over" is deterministic + c.cfg.PinnedImages = []string{"ghcr.io/pinned/runner:v1"} + + var evicted []Candidate + c.evictFn = func(_ context.Context, _ *client.Client, cs []Candidate, _ bool, _ *slog.Logger, _ func() bool) int { + evicted = append(evicted, cs...) + return len(cs) + } + + res, err := c.CollectAll(context.Background()) + if err != nil { + t.Fatalf("CollectAll: %v", err) + } + if !res.Forced { + t.Error("Result.Forced = false on a CollectAll pass") + } + if res.Evicted != 2 { + t.Errorf("evicted %d records, want 2 — a forced pass must ignore the watermarks, which is the entire point of --all", res.Evicted) + } + if res.Protected != 1 { + t.Errorf("protected = %d, want 1 — --all forces the POLICY open, never the safety", res.Protected) + } + for _, e := range evicted { + if e.Name == "ghcr.io/pinned/runner:v1" { + t.Fatal("a pinned runner image was evicted by --all; the protected set is an absolute veto") + } + } + + // Control: the same collector under the same (absent) pressure must + // plan nothing on the automatic path. Without this the assertion above + // would also pass if force were being ignored and the disk happened to + // look full. + c2 := testCollector(t, cands) + c2.cfg.Thresholds = Thresholds{HighUsedPercent: 100.1} // unreachable: UsedPercent() never exceeds 100, so "not over" is deterministic + c2.evictFn = func(context.Context, *client.Client, []Candidate, bool, *slog.Logger, func() bool) int { + t.Fatal("the watermark pass evicted something on an unpressured disk") + return 0 + } + if _, err := c2.Collect(context.Background()); err != nil { + t.Fatalf("Collect: %v", err) + } +} + +// TestCollectAll_AbortsWhenTheLiveSetIsUnknown: without the running-container +// set there is no way to guarantee we do not evict an image a job in flight +// needs. Forcing the policy open must not force this safety open too — the +// pass has to abort before anything is deleted. +func TestCollectAll_AbortsWhenTheLiveSetIsUnknown(t *testing.T) { + boom := errors.New("containerd: connection refused") + c := testCollector(t, []Candidate{candidate("ephemerd", "alpine:3", 100, time.Hour)}) + c.runningContainersFn = func(context.Context, *client.Client, *slog.Logger) (map[string]struct{}, map[string]struct{}, error) { + return nil, nil, boom + } + c.evictFn = func(context.Context, *client.Client, []Candidate, bool, *slog.Logger, func() bool) int { + t.Fatal("records were evicted after the live-container lookup failed; a running job's image could have gone with them") + return 0 + } + + _, err := c.CollectAll(context.Background()) + if !errors.Is(err, boom) { + t.Fatalf("CollectAll err = %v, want the containerd error so the operator sees why nothing happened", err) + } +} + +// TestCollectAll_DoesNotArmTheExhaustedBackoff: the failsafe exists to stop +// the DAEMON'S OWN TIMER spinning on a store it cannot shrink. A human typing +// --all is not the daemon's timer, and arming it here would silence the +// automatic collector for 30 minutes on a node that is genuinely filling up. +func TestCollectAll_DoesNotArmTheExhaustedBackoff(t *testing.T) { + c := testCollector(t, nil) // nothing evictable, and always "over" + c.evictFn = func(context.Context, *client.Client, []Candidate, bool, *slog.Logger, func() bool) int { return 0 } + + res, err := c.CollectAll(context.Background()) + if err != nil { + t.Fatalf("CollectAll: %v", err) + } + if res.Exhausted { + t.Error("Result.Exhausted set by a forced pass; it was never chasing a watermark") + } + if !c.exhaustedUntil.IsZero() { + t.Fatalf("forced pass armed the backoff until %v — one operator `cache clear --all` would mute the automatic collector", c.exhaustedUntil) + } +} + +// TestCollectAll_ClearsAnArmedExhaustedBackoff is the mirror image, and the +// half that was missing. +// +// The backoff's premise is "everything evictable is gone and we are still +// over the line". An operator running --all evicts a SUPERSET of what the +// automatic pass is allowed to touch, so a forced pass that actually frees +// records has falsified that premise. Leaving the suppression armed meant +// --all freed the disk and the automatic collector stayed muted for up to 30 +// more minutes anyway — on a false premise, right after a human fixed the +// node. +func TestCollectAll_ClearsAnArmedExhaustedBackoff(t *testing.T) { + c := testCollector(t, []Candidate{candidate("ephemerd", "alpine:3", 100, time.Hour)}) + armedUntil := time.Now().Add(29 * time.Minute) + c.exhaustedUntil = armedUntil + c.evictFn = func(_ context.Context, _ *client.Client, cs []Candidate, _ bool, _ *slog.Logger, _ func() bool) int { + return len(cs) + } + + if _, err := c.CollectAll(context.Background()); err != nil { + t.Fatalf("CollectAll: %v", err) + } + if !c.exhaustedUntil.IsZero() { + t.Fatalf("backoff still armed until %v after a forced pass reclaimed records; the automatic collector stays suppressed on a premise the operator just falsified", c.exhaustedUntil) + } + + // A forced pass that reclaimed NOTHING has falsified nothing, so it must + // leave an armed backoff alone (and still must not arm one). + c2 := testCollector(t, nil) + c2.exhaustedUntil = armedUntil + c2.evictFn = func(context.Context, *client.Client, []Candidate, bool, *slog.Logger, func() bool) int { return 0 } + if _, err := c2.CollectAll(context.Background()); err != nil { + t.Fatalf("CollectAll: %v", err) + } + if !c2.exhaustedUntil.Equal(armedUntil) { + t.Errorf("exhaustedUntil = %v, want it untouched at %v — a pass that freed nothing disproves nothing", c2.exhaustedUntil, armedUntil) + } +} + +// TestCollectAll_IgnoresTheBackoffSuppression: the automatic pass skips while +// the failsafe is armed. An operator override must run anyway, or `--all` is +// unusable for up to 30 minutes after exactly the event that makes someone +// want to run it. +func TestCollectAll_IgnoresTheBackoffSuppression(t *testing.T) { + c := testCollector(t, []Candidate{candidate("ephemerd", "alpine:3", 100, time.Hour)}) + c.exhaustedUntil = time.Now().Add(29 * time.Minute) + c.evictFn = func(_ context.Context, _ *client.Client, cs []Candidate, _ bool, _ *slog.Logger, _ func() bool) int { + return len(cs) + } + + res, err := c.CollectAll(context.Background()) + if err != nil { + t.Fatalf("CollectAll: %v", err) + } + if res.Skipped { + t.Fatal("forced pass was skipped by the exhausted backoff; --all must not be gated on the daemon's own failsafe") + } + if res.Evicted != 1 { + t.Errorf("evicted = %d, want 1", res.Evicted) + } + + // The automatic pass, by contrast, must still be suppressed. + c2 := testCollector(t, []Candidate{candidate("ephemerd", "alpine:3", 100, time.Hour)}) + c2.exhaustedUntil = time.Now().Add(29 * time.Minute) + res2, err := c2.Collect(context.Background()) + if err != nil { + t.Fatalf("Collect: %v", err) + } + if !res2.Skipped { + t.Error("automatic pass was not suppressed by an armed backoff") + } +} + +// TestCollectAll_NoStopFunction: a forced pass has no stopping point short of +// the whole plan. The watermark pass passes a stop closure that halts the +// moment a real reading clears; passing one on a forced pass would make --all +// stop early and leave the operator with a partial clear they did not ask +// for. +func TestCollectAll_NoStopFunction(t *testing.T) { + c := testCollector(t, []Candidate{candidate("ephemerd", "alpine:3", 100, time.Hour)}) + var gotStop bool + c.evictFn = func(_ context.Context, _ *client.Client, cs []Candidate, _ bool, _ *slog.Logger, stop func() bool) int { + gotStop = stop != nil + return len(cs) + } + if _, err := c.CollectAll(context.Background()); err != nil { + t.Fatalf("CollectAll: %v", err) + } + if gotStop { + t.Error("a forced pass supplied a stop function; --all must run the whole plan") + } +} + +// TestNilCollectorCollectAll: callers invoke the collector unconditionally, +// and a node without image GC configured has a nil one. +func TestNilCollectorCollectAll(t *testing.T) { + var c *Collector + res, err := c.CollectAll(context.Background()) + if err != nil { + t.Fatalf("nil CollectAll: %v", err) + } + if res.Evicted != 0 { + t.Errorf("nil collector evicted %d", res.Evicted) + } +} diff --git a/pkg/imagegc/plan.go b/pkg/imagegc/plan.go index 58c77df..44a915d 100644 --- a/pkg/imagegc/plan.go +++ b/pkg/imagegc/plan.go @@ -203,6 +203,45 @@ func PlanEviction(candidates []Candidate, protected map[string]struct{}, usage d return p } +// PlanForced selects EVERY unprotected candidate, least-recently-used +// first, regardless of disk pressure. It is the operator override behind +// `ephemerd cache clear containerd --all`. Pure. +// +// WHY THIS IS SEPARATE FROM PlanEviction. PlanEviction is watermark-driven +// by design: on a node whose thresholds are not tripped it correctly returns +// an empty plan, which is right for the timer and wrong for a human who has +// just decided this node's image store must go. Before this existed the +// `--all` flag was accepted, silently dropped for the containerd target, and +// reported "0 records removed" — so the operator concluded the command was +// broken rather than that the policy had declined (observed while chasing +// disk on a Windows node). +// +// The protected set is still an ABSOLUTE veto here, exactly as in +// PlanEviction: --all forces the POLICY open, never the safety. An image +// backing a running container or listed as a pinned runner image is never +// selected, because evicting one breaks a job in flight or costs every +// subsequent job a re-pull — neither is something an operator asking to +// reclaim cache is asking for. +func PlanForced(candidates []Candidate, protected map[string]struct{}) Plan { + var p Plan + p.Over = true + p.Reasons = []string{ReasonForced} + + eligible := filterProtected(candidates, protected, &p.Protected) + sortLRU(eligible) + p.Evict = eligible + for _, c := range eligible { + if c.SizeBytes > 0 { + p.PlannedBytes += uint64(c.SizeBytes) + } + } + return p +} + +// ReasonForced is the Pressure reason PlanForced reports: the pass ran +// because an operator asked for it, not because a watermark tripped. +const ReasonForced = "forced" + // PlanByAge selects every candidate last accessed before cutoff, // least-recently-used first, skipping the protected set. It is the optional // age backstop (`cache_max_age`), not the primary mechanism: age alone diff --git a/pkg/imagegc/plan_test.go b/pkg/imagegc/plan_test.go index e4c20f8..58cfe81 100644 --- a/pkg/imagegc/plan_test.go +++ b/pkg/imagegc/plan_test.go @@ -418,3 +418,99 @@ func TestPressure_ReasonString(t *testing.T) { t.Errorf("ReasonString() on empty = %q, want empty", got) } } + +// TestPlanForced covers the operator override behind `cache clear containerd +// --all`. Two properties matter, and they pull in opposite directions: +// forcing must beat the watermark policy (the whole complaint was that +// --all evicted nothing on a node under no pressure), and it must NOT beat +// the protected set (evicting a running job's image or a pinned runner image +// is a broken job or a re-pull on every subsequent one). +func TestPlanForced(t *testing.T) { + tests := []struct { + name string + candidates []Candidate + protected map[string]struct{} + wantEvict []string + wantProt int + }{ + { + name: "no pressure is irrelevant: everything unprotected goes", + candidates: []Candidate{cand("a", 1, 5), cand("b", 9, 5), cand("c", 3, 5)}, + protected: set(), + // LRU first: oldest access leads. + wantEvict: []string{"b", "c", "a"}, + }, + { + name: "a running job's image is still an absolute veto", + candidates: []Candidate{cand("busy", 30, 20), cand("cold", 1, 1)}, + protected: set("busy"), + wantEvict: []string{"cold"}, + wantProt: 1, + }, + { + name: "pinned runner images are still an absolute veto", + candidates: []Candidate{cand("runner:latest", 40, 8), cand("scratch", 2, 1)}, + protected: set("runner:latest"), + wantEvict: []string{"scratch"}, + wantProt: 1, + }, + { + // The failure mode this must NOT have: --all wiping a node whose + // every image is live. + name: "everything protected evicts nothing", + candidates: []Candidate{cand("a", 5, 1), cand("b", 6, 1)}, + protected: set("a", "b"), + wantEvict: nil, + wantProt: 2, + }, + { + name: "empty store is a no-op, not an error", + candidates: nil, + protected: set(), + wantEvict: nil, + }, + { + // Unknown size (non-positive) must stay eligible, matching + // PlanEviction — a record with no size is not invisible. + name: "unknown-size records are still evicted", + candidates: []Candidate{ + {Namespace: "ephemerd", Name: "sizeless", LastAccessed: time.Time{}}, + }, + protected: set(), + wantEvict: []string{"sizeless"}, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + p := PlanForced(tc.candidates, tc.protected) + if !p.Over { + t.Error("Plan.Over = false; a forced pass is always 'over' so callers treat it as a real pass") + } + if p.ReasonString() != ReasonForced { + t.Errorf("reason = %q, want %q", p.ReasonString(), ReasonForced) + } + if got := names(p.Evict); !reflect.DeepEqual(got, tc.wantEvict) { + t.Errorf("evict = %v, want %v", got, tc.wantEvict) + } + if p.Protected != tc.wantProt { + t.Errorf("protected = %d, want %d", p.Protected, tc.wantProt) + } + }) + } +} + +// TestPlanForced_BeatsWatermarkPolicy is the regression proper: on a node +// with plenty of free space PlanEviction correctly plans nothing, and that +// emptiness is what an operator's --all used to inherit. +func TestPlanForced_BeatsWatermarkPolicy(t *testing.T) { + cands := []Candidate{cand("a", 10, 5), cand("b", 2, 5)} + idle := usage(500, 400) // 20% used — nowhere near the 85% high mark + + if plan := PlanEviction(cands, set(), idle, defaults); len(plan.Evict) != 0 { + t.Fatalf("PlanEviction planned %v under no pressure; test premise is wrong", names(plan.Evict)) + } + if got := names(PlanForced(cands, set()).Evict); !reflect.DeepEqual(got, []string{"a", "b"}) { + t.Errorf("PlanForced = %v, want both records evicted regardless of pressure", got) + } +}