fix(runtime,cacheprune,imagegc): bound teardownContainer's exit wait; make cache clear --all force eviction - #198
Open
luthermonson wants to merge 2 commits into
Open
fix(runtime,cacheprune,imagegc): bound teardownContainer's exit wait; make cache clear --all force eviction#198luthermonson wants to merge 2 commits into
luthermonson wants to merge 2 commits into
Conversation
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.
… force eviction 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The review-clean half of #195, split out so it can land while the buildkit work keeps baking.
Why the split: #195 bundled four fixes. Three adversarial review rounds each found a daemon-wedging defect, and every single finding was in the buildkit
Rebuild/serverCore.close()/buildCorearea. These two fixes reviewed clean in all three rounds and are unrelated to that machinery. #195 stays open with the buildkit cluster, the Linux-VM start ladder, the scheduleratomic.Pointer, and the HCN endpoint sweep (which round 3 flagged as running while jobs are live).1.
teardownContainer's post-kill exit wait was unboundedpkg/runtime/runtime.go— the startup-path twin of the bug #190 fixed inDestroy. After killing a task it did a bare<-exitCh; a dead containerd shim never delivers that event, so startupCleanOrphans— and the periodic dead-container reaper — would hang forever. Now uses the existingwaitTaskExit(ctx, exitCh, destroyKillWait)from #190 and warns on timeout. No bare receive remains inpkg/runtime.2.
cache clear containerd --allwas silently ignoredPrunedropped theallflag at thepruneContainerdhop, so an operator had no way to force eviction when the watermarks weren't tripped — the collector correctly evicted nothing and the CLI's promise that "clear means clear" was false. (Diagnosed during a Windows-node investigation where--allmeasurably did nothing.)allnow reaches a newCollector.CollectAll→ purePlanForced.Every protection still holds in the forced path — this was checked explicitly in review:
PlanForcedcalls the samefilterProtectedwith the same protected set, so live-container image refs, pinned runner images, andLiveJobPrefixes(BuildKit job records) remain absolute vetoes; aRunningContainerserror still aborts the pass rather than proceeding against an empty live set. A forced pass also skips the exhausted-backoff bookkeeping so one operator--allcan't silence the automatic collector on a filling node — and clears a previously-armed backoff when it actually reclaimed, since the "nothing left to evict" premise is then falsified.No CLI change was needed:
pruneViaDaemonalready sentAll: trueand the gRPC layer already forwarded it.pkg/scheduleris untouched.Testing
TestPlanForced(LRU order, running-container veto, pinned veto, all-protected-evicts-nothing, sizeless records) plusTestPlanForced_BeatsWatermarkPolicy, which asserts the premise —PlanEvictionreally plans nothing under no pressure — before asserting the fix. Newcollector_test.gocovers theCollectAll → PlanForcedwiring, theRunningContainers-error abort under force, and the backoff not-armed/cleared/left-alone cases.cacheprunetests now distinguish the two passes by name rather than asserting a count that couldn't tell threaded from dropped.go vetclean;pkg/runtime,pkg/imagegc,pkg/cacheprune,pkg/dindall green.git diff origin/main --name-onlytouches only those three packages — nothing frompkg/buildkit,pkg/scheduler,pkg/vm, orcmd/ephemerd.Two notes for the reviewer:
Collectorgains four nil-defaulted overridable seams that exist purely soCollectAll's control flow is reachable without a live containerd — separable from the functional fix if you'd rather they not land (cost:collector_test.go). And the backoff-clearing condition also clears when a forced pass evicted nothing but the disk happens to be under the watermarks — arguably correct since the premise is falsified either way, and pinned by test, but it wasn't called out in the original commit.