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) + } +} 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) + } } } }