Fix the three refill-path stalls that cap a large warm-pool fill - #56
Merged
Conversation
A fill would reach 97% in seconds and then crawl, sometimes settling one short of target permanently. Two unbounded waits, both on the refill path: A warm refill probed readiness with the claim deadline, 15 s. A claim deserves that patience because a caller is blocked on it; a refill has nobody waiting, and it holds a slot in the pool's target accounting for the whole probe. A clone answers in about a second even on a saturated node, so one silent for 15 s is an outlier that should be replaced, not waited on — and waiting costs the last few of a fill deadline + interval + a fresh boot to recover what a replacement gets in a second. `cocoon vm rm` ran cancellation-immune and unbounded. It contends with every concurrent create on the node's metadata store, and under a large fill that contention can outlast any caller. refillOne removes a failed create before it releases the refill semaphore and decrements p.refilling, so a single wedged remove leaves the pool one short of target forever. It now gives up after 10 s: cocoon keeps a tombstone and resumes the delete in its own gc, and the reaper sweeps anything still running. Cancellation immunity is kept — a remove that inherits a cancelled context no-ops and orphans the VM. Only the unboundedness is gone. Measured after: 10000 warm across 20 nodes, 9000 new ready in 6.7 s, no node below 237/s.
A remove that timed out was reported as gone. The timeout SIGKILLs `cocoon vm rm` partway through, so the guest can stay alive while this process forgets it exists: the slot is released, the pool refills, and the node accumulates VMs nobody accounts for. Those VMs each hold a bridge port, and a Linux bridge refuses its 1025th with EXFULL — so the leak ends as every clone on the node failing at CNI. Ask the engine whether the VM is still there and keep it accounted for when it is, or when the check itself cannot answer: a false "gone" leaks a running VM, a false "still here" costs one more sweep. The bound goes to 120s for the same reason it existed. `cocoon vm rm` contends with every concurrent create on the node's metadata store, and under a large scale-down that contention is the normal case — 10s turned routine slowness into a killed remove.
refillOne logged every failure and returned, so the 2s ticker retried at full concurrency forever. Against a permanent condition that is not a retry, it is a storm: one node wrote 19,858 refill errors in an hour, each attempt forking the engine, building a netns and running the CNI plugins before failing — all of it serialized on the global rtnl lock. CPU stayed 97% idle while load average passed 180, because load counts the uninterruptible tasks queued behind that lock. Tell the two failures apart. "This VM failed to start" stays retryable. "This node cannot attach another VM" — a bridge at BR_MAX_PORTS answering EXFULL, a full disk — parks every pool for 30s, because the resource that ran out is shared by all of them and nothing this process does will free it. One log line per episode replaces one per attempt. Publish it on /v1/info too: a placer that cannot see the difference reads a short warm count as slowness and keeps aiming at a node that is full.
engine.CapacitySignature owns the stderr matching, next to the code that formats it; pool reacts to the classification instead of re-deriving it. The park gates spawns only (sweeping removed pools is bookkeeping) and golden-build failures feed it, so a node failing only its builds still publishes at-capacity. Capacity failures no longer count toward a pool's own streak. removeVM drops its redundant timeout (engine.run already bounds every command) and the archive and release paths now go through it, so every remove is cancellation-immune and verified. findVM dedupes four List+scan sites. Comments cut to the why.
Keep verified survivors in the existing reap loop until removal is confirmed. Preserve egress cleanup state, while startup reconciliation remains the crash-recovery authority.
Dispatch empties the queue, so map absence also covers in-flight: the retrying flag, its mark/unmark dance and the unreachable presence check all go, and a failed retry re-queues itself on completion. removeOrRetry carries the tap, so the stale sweep shares the one calling convention. retryRemovals returns its WaitGroup, making the retry tests deterministic instead of polled.
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
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.
A 0→50000 fill across 20 nodes reached 97% in seconds, then crawled, and a node
would sometimes settle short of target and stay there. Three independent causes,
all on the refill path.
A warm refill probed readiness with the claim deadline (15s). A claim
deserves that patience — a caller is blocked on it. A refill has nobody waiting,
and it holds a slot in the pool's target accounting for its whole probe; a clone
answers in about a second even on a saturated node, so one silent for longer is
an outlier to replace, not wait on. Warm refills now use
warmProbeTimeout(5s); claims and forks keep the generous deadline.
A remove's word was taken at face value.
cocoon vm rmcontends with everyconcurrent create on the node's metadata store, and the engine SIGKILLs it at
its per-command bound — which can kill it partway through, with the guest still
running, or after the work was actually done. Both directions used to be
misread: a remove that failed loudly but had in fact completed left its caller
retrying (or holding an egress lock) forever; treating a killed one as done
would silently leak running VMs from all accounting (measured once at ~570
orphans on a node). A remove that reports failure is now verified against the
engine's own list — really gone releases the slot, still listed stays accounted
for the reaper to retry — and every remove path (refill cleanup, reap, archive,
release, rollback) goes through the same verified helper.
A node-wide capacity failure was retried at full concurrency forever. A
Linux bridge returns
EXFULLfor its 1025th port;BR_MAX_PORTSis acompile-time constant, so the condition clears only when VMs go away. Every
retry still forked the engine, built a netns and ran the CNI plugins before
failing, each taking the global rtnl lock — the retries cost more than the work
they replaced, wrote twenty thousand log lines an hour, and drove load past 180
on an idle-CPU box. The engine now classifies these failures at the subprocess
boundary (
engine.CapacitySignature:exchange full,no space left on device), and one classified failure parks all VM spawning — refills and goldenbuilds — node-wide for
capacityBackoff(30s), logs once per episode, andpublishes
at_capacity/at_capacity_reasonthrough/v1/infoso a placerreads a short warm count as "full" rather than "still filling". Capacity
failures do not count toward any pool's own failure streak: that backoff (#55)
stays the per-pool fallback for failures the node cannot classify.
Measured after: 10000 warm across 20 nodes, 9000 new ready in 6.7s, no node
below 237/s.
Tests
whose VM is really absent is, and the ordinary path is unchanged
ticks spawn nothing until the backoff expires; a park is not a latch
goldens do not
exchange fullparks the node after a single failure before a streak can form
go test ./...green (including-raceon the pool package); golangci-lint(pinned v2.12.2) clean on
GOOS=linuxandGOOS=darwin.