Skip to content

Fix the three refill-path stalls that cap a large warm-pool fill - #56

Merged
CMGS merged 6 commits into
mainfrom
pr1-pool-stability
Jul 29, 2026
Merged

Fix the three refill-path stalls that cap a large warm-pool fill#56
CMGS merged 6 commits into
mainfrom
pr1-pool-stability

Conversation

@doge-rgb

@doge-rgb doge-rgb commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

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 rm contends with every
concurrent 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 EXFULL for its 1025th port; BR_MAX_PORTS is a
compile-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 golden
builds — node-wide for capacityBackoff (30s), logs once per episode, and
publishes at_capacity/at_capacity_reason through /v1/info so a placer
reads 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

  • a failed remove whose VM the engine still lists is not reported gone; one
    whose VM is really absent is, and the ordinary path is unchanged
  • one capacity-classified failure parks the node and publishes the gauge; later
    ticks spawn nothing until the backoff expires; a park is not a latch
  • only node-wide signatures classify — ordinary boot failures and missing
    goldens do not
  • Back a warm pool off after an unbroken run of failed refills #55's streak test now uses an unclassifiable error, since exchange full
    parks the node after a single failure before a streak can form

go test ./... green (including -race on the pool package); golangci-lint
(pinned v2.12.2) clean on GOOS=linux and GOOS=darwin.

doge-rgb and others added 4 commits July 29, 2026 21:30
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.
@CMGS CMGS changed the title Stop a large warm-pool fill from stalling on its own removes Fix the three refill-path stalls that cap a large warm-pool fill Jul 29, 2026
CMGS added 2 commits July 30, 2026 00:02
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.
@CMGS
CMGS merged commit c8a2894 into main Jul 29, 2026
1 check passed
@CMGS
CMGS deleted the pr1-pool-stability branch July 29, 2026 16:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants