From 670a9064646eac715f51a137ec6cffb83152c357 Mon Sep 17 00:00:00 2001 From: doge Date: Mon, 27 Jul 2026 15:43:44 +0800 Subject: [PATCH 1/6] Bound the two waits that stall the tail of a large warm-pool fill MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- sandboxd/pool/pool.go | 22 +++++++++++++++++----- sandboxd/pool/refill.go | 15 +++++++++++++-- 2 files changed, 30 insertions(+), 7 deletions(-) diff --git a/sandboxd/pool/pool.go b/sandboxd/pool/pool.go index 8f62356..0fdce1d 100644 --- a/sandboxd/pool/pool.go +++ b/sandboxd/pool/pool.go @@ -35,11 +35,23 @@ import ( ) const ( - refillInterval = 2 * time.Second - reapInterval = 5 * time.Second - buildRetryDelay = 30 * time.Second - claimProbeTimeout = 15 * time.Second - coldProbeTimeout = 90 * time.Second + refillInterval = 2 * time.Second + reapInterval = 5 * time.Second + buildRetryDelay = 30 * time.Second + claimProbeTimeout = 15 * time.Second + coldProbeTimeout = 90 * time.Second + // warmProbeTimeout bounds the readiness probe of a warm-pool refill, + // which — unlike a claim — has no caller waiting on it. A clone answers + // in about a second even with the node saturated, so one that has said + // nothing for this long is an outlier worth replacing rather than + // waiting on: the refill holds a slot in the pool's target accounting + // for its whole probe, so a long deadline here stalls the LAST few of a + // large fill (deadline + refillInterval + a fresh boot) even though the + // replacement costs about a second. Claims keep the generous deadline. + warmProbeTimeout = 5 * time.Second + // removeTimeout bounds one `cocoon vm rm`; see removeVM for why a remove + // must never outlast its caller. + removeTimeout = 10 * time.Second vsockPollInterval = 100 * time.Millisecond defaultTTL = 5 * time.Minute maxTTL = 24 * time.Hour diff --git a/sandboxd/pool/refill.go b/sandboxd/pool/refill.go index e6e356e..ab0554e 100644 --- a/sandboxd/pool/refill.go +++ b/sandboxd/pool/refill.go @@ -82,7 +82,7 @@ func (m *Manager) refillOne(ctx context.Context, p *pool, golden string) { if ctx.Err() == nil { m.refillOnce(ctx) } - sb, err = m.readyBounded(ctx, sb, time.Now().Add(claimProbeTimeout)) + sb, err = m.readyBounded(ctx, sb, time.Now().Add(warmProbeTimeout)) } keep := false var fails int @@ -414,8 +414,19 @@ func (m *Manager) runBounded(ctx context.Context, n int, f func(context.Context, // removeVM deletes a VM (on a cancellation-immune ctx, else `cocoon vm rm` // no-ops and orphans it) and reports whether it is confirmed gone; a failed // remove leaves it running, so the caller must keep its lock. +// +// The remove is cancellation-immune but NOT unbounded: `cocoon vm rm` contends +// with every concurrent create on the node's metadata store, and under a large +// fill that contention can outlast any caller. A remove that never returns pins +// whatever the caller was holding — refillOne cleans up a failed create before +// it releases the refill semaphore and decrements p.refilling, so one wedged +// remove leaves the pool permanently one short of target. Give up after +// removeTimeout instead: cocoon keeps a tombstone and its own gc resumes the +// delete, and the reaper sweeps whatever is still running. func (m *Manager) removeVM(ctx context.Context, name string) bool { - if err := m.eng.Remove(context.WithoutCancel(ctx), name); err != nil { + ctx, cancel := context.WithTimeout(context.WithoutCancel(ctx), removeTimeout) + defer cancel() + if err := m.eng.Remove(ctx, name); err != nil { log.WithFunc("pool.removeVM").Errorf(ctx, err, "remove vm %s", name) return false } From 79fe49e8a948f391dd37b41a86d3014360e2c774 Mon Sep 17 00:00:00 2001 From: doge Date: Tue, 28 Jul 2026 20:11:59 +0800 Subject: [PATCH 2/6] Verify a removal took effect before releasing the slot MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- sandboxd/pool/egress_test.go | 6 +++++ sandboxd/pool/pool.go | 22 ++++++++++++------ sandboxd/pool/pool_test.go | 44 ++++++++++++++++++++++++++++++++++++ sandboxd/pool/refill.go | 39 +++++++++++++++++++++++++++++--- 4 files changed, 101 insertions(+), 10 deletions(-) diff --git a/sandboxd/pool/egress_test.go b/sandboxd/pool/egress_test.go index ec80d64..b4b0c45 100644 --- a/sandboxd/pool/egress_test.go +++ b/sandboxd/pool/egress_test.go @@ -254,6 +254,9 @@ func TestBatchArmFailureRecordsNoUsage(t *testing.T) { func TestRollbackKeepsLockWhenRemoveFails(t *testing.T) { eng := newFakeEngine() eng.removeErrFor = "sbx-r1" + // Both VMs are running; sbx-r1's remove fails and the engine keeps listing it. + eng.vms["sbx-r1"] = "/run/sbx-r1.sock" + eng.vms["sbx-r2"] = "/run/sbx-r2.sock" m := egressManager(t, eng, config.PoolSpec{PoolKey: egKey, Egress: egPolicy}) // Both members armed before a later batch member failed; sbx-r1's remove fails. sbs := []*types.Sandbox{ @@ -287,6 +290,9 @@ func TestRollbackKeepsLockWhenRemoveFails(t *testing.T) { func TestQuarantineFailedRemoveStaysUnswept(t *testing.T) { eng := newFakeEngine() eng.removeErrFor = "sbx-q1" + // The VM is running: a failed remove only means "still here" if the engine + // still lists it, which is what removeVM now checks. + eng.vms["sbx-q1"] = "/run/sbx-q1.sock" m := egressManager(t, eng, config.PoolSpec{PoolKey: egKey, Egress: egPolicy}) sb := &types.Sandbox{ID: "sb_q1", VMName: "sbx-q1", Key: egKey, TAP: "tap-q1"} m.mu.Lock() diff --git a/sandboxd/pool/pool.go b/sandboxd/pool/pool.go index 0fdce1d..3bb0434 100644 --- a/sandboxd/pool/pool.go +++ b/sandboxd/pool/pool.go @@ -50,13 +50,21 @@ const ( // replacement costs about a second. Claims keep the generous deadline. warmProbeTimeout = 5 * time.Second // removeTimeout bounds one `cocoon vm rm`; see removeVM for why a remove - // must never outlast its caller. - removeTimeout = 10 * time.Second - vsockPollInterval = 100 * time.Millisecond - defaultTTL = 5 * time.Minute - maxTTL = 24 * time.Hour - recommitBackoff = 20 * time.Millisecond - recommitMaxBackoff = 5 * time.Second + // must never outlast its caller. It is generous because the command + // contends with every concurrent create on the node's metadata store, and + // under a large scale-down that contention is the normal case, not the + // exception — a tight bound there turns routine slowness into a killed + // remove, and a killed remove can leave the guest running. + removeTimeout = 120 * time.Second + // removeVerifyTimeout bounds the check that a failed remove really did + // leave nothing behind. Short: it is one list, and being wrong here only + // costs an extra sweep. + removeVerifyTimeout = 15 * time.Second + vsockPollInterval = 100 * time.Millisecond + defaultTTL = 5 * time.Minute + maxTTL = 24 * time.Hour + recommitBackoff = 20 * time.Millisecond + recommitMaxBackoff = 5 * time.Second // One failed boot is ordinary; only an unbroken run of failures says the // next attempt will fail too. refillFailStreak = 8 diff --git a/sandboxd/pool/pool_test.go b/sandboxd/pool/pool_test.go index 83ee48b..5750c46 100644 --- a/sandboxd/pool/pool_test.go +++ b/sandboxd/pool/pool_test.go @@ -1061,3 +1061,47 @@ func (f *fakeEngine) snapRemoved(name string) bool { defer f.mu.Unlock() return slices.Contains(f.snapRemoves, name) } + +// TestRemoveVMWillNotReportGoneWhileTheVMRuns pins the invariant a bounded +// remove has to preserve. `cocoon vm rm` contends with every concurrent create +// on the node's metadata store, so under a large scale-down it routinely runs +// long; the bound exists so one slow remove cannot pin the refill slot forever. +// But a bound that reports success on expiry is worse than no bound: the +// timeout SIGKILLs the command partway through, the guest keeps running, and +// this manager forgets it exists. The slot is released, the pool refills, and +// the node accumulates VMs nothing is accounting for — silently, and faster the +// harder you drive it. Measured once at ~570 orphans per node. +func TestRemoveVMWillNotReportGoneWhileTheVMRuns(t *testing.T) { + eng := newFakeEngine() + m := &Manager{eng: eng} + + // A remove that fails outright, on a VM the engine still lists. + eng.mu.Lock() + eng.vms["survivor"] = "/run/survivor.sock" + eng.removeErrFor = "survivor" + eng.mu.Unlock() + + if m.removeVM(context.Background(), "survivor") { + t.Fatal("a VM the engine still lists was reported gone; its slot would be " + + "released while the guest keeps running") + } + + // The same failure, but the VM really is absent: reporting it gone is right, + // and is what stops a permanently failing remove from pinning a slot. + eng.mu.Lock() + delete(eng.vms, "survivor") + eng.mu.Unlock() + if !m.removeVM(context.Background(), "survivor") { + t.Fatal("a VM the engine no longer lists must be reported gone, whatever " + + "the remove command said, or a permanently failing remove pins the slot") + } + + // The ordinary path is unchanged. + eng.mu.Lock() + eng.vms["ordinary"] = "/run/ordinary.sock" + eng.removeErrFor = "" + eng.mu.Unlock() + if !m.removeVM(context.Background(), "ordinary") || !eng.removed("ordinary") { + t.Fatal("the ordinary remove path regressed") + } +} diff --git a/sandboxd/pool/refill.go b/sandboxd/pool/refill.go index ab0554e..c9bec3e 100644 --- a/sandboxd/pool/refill.go +++ b/sandboxd/pool/refill.go @@ -424,12 +424,45 @@ func (m *Manager) runBounded(ctx context.Context, n int, f func(context.Context, // removeTimeout instead: cocoon keeps a tombstone and its own gc resumes the // delete, and the reaper sweeps whatever is still running. func (m *Manager) removeVM(ctx context.Context, name string) bool { - ctx, cancel := context.WithTimeout(context.WithoutCancel(ctx), removeTimeout) + ctx = context.WithoutCancel(ctx) + rmCtx, cancel := context.WithTimeout(ctx, removeTimeout) defer cancel() - if err := m.eng.Remove(ctx, name); err != nil { - log.WithFunc("pool.removeVM").Errorf(ctx, err, "remove vm %s", name) + if err := m.eng.Remove(rmCtx, name); err == nil { + return true + } else { + log.WithFunc("pool.removeVM").Warnf(ctx, "remove vm %s: %v; verifying", name, err) + } + + // The remove failed or ran out of time, and the difference that matters is + // whether the VM is still running — not why the command stopped. A timeout + // SIGKILLs `cocoon vm rm` partway through, which can leave the guest alive + // while this process forgets it exists: the slot is released, the pool + // refills, and the machine accumulates VMs nobody is accounting for. That + // failure is silent and compounding, so confirm before reporting gone. + return m.confirmGone(ctx, name) +} + +// confirmGone reports whether name is absent from the engine's own view. +// A VM the engine still lists is reported as present, which keeps it in this +// manager's accounting so the reaper retries it rather than leaking it. +func (m *Manager) confirmGone(ctx context.Context, name string) bool { + ctx, cancel := context.WithTimeout(ctx, removeVerifyTimeout) + defer cancel() + vms, err := m.eng.List(ctx, name) + if err != nil { + // Cannot tell. Assume it is still there: a false "gone" leaks a running + // VM, a false "still here" only costs another sweep. + log.WithFunc("pool.confirmGone").Warnf(ctx, "verify remove of %s: %v", name, err) return false } + for i := range vms { + if vms[i].Config.Name == name { + log.WithFunc("pool.confirmGone").Errorf(ctx, + fmt.Errorf("vm %s survived removal", name), + "remove did not take effect; leaving it accounted for retry") + return false + } + } return true } From 0080b6a2b42f179338e428311ccb448f4557d2fd Mon Sep 17 00:00:00 2001 From: doge Date: Tue, 28 Jul 2026 20:15:09 +0800 Subject: [PATCH 3/6] Park refill when the node itself is out of capacity MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- sandboxd/pool/pool.go | 33 ++++++++++-- sandboxd/pool/refill.go | 73 +++++++++++++++++++++++-- sandboxd/pool/refillbackoff_test.go | 6 ++- sandboxd/pool/refillkick_test.go | 83 ++++++++++++++++++++++++++++- sandboxd/server/server.go | 10 +++- 5 files changed, 192 insertions(+), 13 deletions(-) diff --git a/sandboxd/pool/pool.go b/sandboxd/pool/pool.go index 3bb0434..0b94648 100644 --- a/sandboxd/pool/pool.go +++ b/sandboxd/pool/pool.go @@ -60,11 +60,18 @@ const ( // leave nothing behind. Short: it is one list, and being wrong here only // costs an extra sweep. removeVerifyTimeout = 15 * time.Second - vsockPollInterval = 100 * time.Millisecond - defaultTTL = 5 * time.Minute - maxTTL = 24 * time.Hour - recommitBackoff = 20 * time.Millisecond - recommitMaxBackoff = 5 * time.Second + // capacityBackoff parks refill after the node reports it cannot attach + // another VM. The condition is a property of the node and clears only when + // VMs go away, so the refill ticker would otherwise retry a permanent + // failure at full concurrency forever. Matched to buildRetryDelay: long + // enough that the retry costs nothing, short enough that a drain is picked + // up promptly. + capacityBackoff = 30 * time.Second + vsockPollInterval = 100 * time.Millisecond + defaultTTL = 5 * time.Minute + maxTTL = 24 * time.Hour + recommitBackoff = 20 * time.Millisecond + recommitMaxBackoff = 5 * time.Second // One failed boot is ordinary; only an unbroken run of failures says the // next attempt will fail too. refillFailStreak = 8 @@ -154,6 +161,13 @@ type Gauges struct { Hibernated int Archived int Draining bool + // AtCapacity reports that the node refused to attach another VM and refill + // is parked. It is published so a placer can send work elsewhere instead of + // reading this node's short warm count as transient slowness. + AtCapacity bool + // AtCapacityReason carries what the node said, so the condition is + // diagnosable from the API rather than only from the node's logs. + AtCapacityReason string } type pool struct { @@ -343,6 +357,12 @@ type Manager struct { pools map[types.PoolKey]*pool claimed map[string]*types.Sandbox + // atCapacityUntil parks every pool's refill after the node reports it can + // hold no more VMs. It is node-wide rather than per-pool because the + // resource that ran out — bridge ports — is shared by all of them. + atCapacityUntil time.Time + atCapacityReason string + refillSem chan struct{} probeSem chan struct{} refillKick chan struct{} @@ -558,6 +578,9 @@ func (m *Manager) Info() ([]PoolInfo, Gauges) { } slices.SortFunc(pools, func(a, b PoolInfo) int { return strings.Compare(a.Key.Hash(), b.Key.Hash()) }) g := Gauges{Claimed: len(m.claimed), Draining: m.draining} + if now.Before(m.atCapacityUntil) { + g.AtCapacity, g.AtCapacityReason = true, m.atCapacityReason + } for _, sb := range m.claimed { if sb.HibernateSnap != "" { g.Hibernated++ diff --git a/sandboxd/pool/refill.go b/sandboxd/pool/refill.go index c9bec3e..1853b2e 100644 --- a/sandboxd/pool/refill.go +++ b/sandboxd/pool/refill.go @@ -32,6 +32,13 @@ func (m *Manager) refillOnce(ctx context.Context) { return } now := time.Now() + if now.Before(m.atCapacityUntil) { + // The node cannot attach another VM. Spawning refills anyway is what + // turned a capacity limit into an outage: each attempt still forks the + // engine, builds a netns and runs the CNI plugins before failing, and + // every one of those takes the global rtnl lock on the way through. + return + } inFlight := 0 for key, p := range m.pools { inFlight += p.refilling @@ -100,15 +107,25 @@ func (m *Manager) refillOne(ctx context.Context, p *pool, golden string) { if backedOff { fails, wait = p.refillFails, time.Until(p.nextRefill) } + // Capacity is node-wide, the streak backoff above is per-pool; both park + // refill, so record both and let the capacity episode own the log line. + entered := err != nil && m.noteCapacityLocked(err) m.mu.Unlock() if err != nil { if ctx.Err() == nil { hash := p.key.Hash() - log.WithFunc("pool.refillOne").Errorf(ctx, err, "refill %s", hash) - if backedOff { - log.WithFunc("pool.refillOne").Warnf(ctx, - "refill %s failed %d times running; pausing refill for %s", - hash, fails, wait.Round(time.Millisecond)) + if entered { + // One line per episode, not one per attempt: the storm this + // replaces wrote twenty thousand of these an hour. + log.WithFunc("pool.refillOne").Errorf(ctx, err, + "node is at capacity; parking refill for %s", capacityBackoff) + } else if !m.atCapacity() { + log.WithFunc("pool.refillOne").Errorf(ctx, err, "refill %s", hash) + if backedOff { + log.WithFunc("pool.refillOne").Warnf(ctx, + "refill %s failed %d times running; pausing refill for %s", + hash, fails, wait.Round(time.Millisecond)) + } } } return @@ -121,6 +138,52 @@ func (m *Manager) refillOne(ctx context.Context, p *pool, golden string) { } } +// noteCapacityLocked parks refill when err says the node itself is full, and +// reports whether this call is what parked it — so the condition is logged once +// per episode rather than once per failed attempt. Called with m.mu held. +func (m *Manager) noteCapacityLocked(err error) bool { + reason, full := nodeAtCapacity(err) + if !full { + return false + } + first := !time.Now().Before(m.atCapacityUntil) + m.atCapacityUntil = time.Now().Add(capacityBackoff) + m.atCapacityReason = reason + return first +} + +func (m *Manager) atCapacity() bool { + m.mu.Lock() + defer m.mu.Unlock() + return time.Now().Before(m.atCapacityUntil) +} + +// capacitySignatures are the failures that mean "this node cannot attach +// another VM", as opposed to "this VM failed to start". They are matched as +// text because the engine is a subprocess: its exit status is 1 for every +// failure and the distinction survives only in the message. +// +// "exchange full" is the kernel's EXFULL, which a Linux bridge returns for its +// 1025th port — BR_MAX_PORTS is a compile-time constant, so the condition is +// permanent until VMs are removed, and no amount of retrying clears it. +var capacitySignatures = []string{ + "exchange full", + "no space left on device", +} + +func nodeAtCapacity(err error) (string, bool) { + if err == nil { + return "", false + } + msg := strings.ToLower(err.Error()) + for _, sig := range capacitySignatures { + if strings.Contains(msg, sig) { + return sig, true + } + } + return "", false +} + func (m *Manager) buildGolden(ctx context.Context, p *pool) { logger := log.WithFunc("pool.buildGolden") hash := p.key.Hash() diff --git a/sandboxd/pool/refillbackoff_test.go b/sandboxd/pool/refillbackoff_test.go index a13b9c3..881e366 100644 --- a/sandboxd/pool/refillbackoff_test.go +++ b/sandboxd/pool/refillbackoff_test.go @@ -64,7 +64,11 @@ func TestBackoffGrowsAndIsCapped(t *testing.T) { // while a pool is backed off, the ticker spawns nothing. func TestRefillOnceHonorsTheBackoff(t *testing.T) { eng := newFakeEngine() - eng.cloneErr = errors.New("configure network: failed to connect veth to bridge cni0: exchange full") + // Deliberately not a capacitySignature: those are parked node-wide after a + // single failure (see noteCapacityLocked), which is a different mechanism + // and would stop the per-pool streak from ever reaching refillFailStreak. + // This backoff is the fallback for a failure the node cannot classify. + eng.cloneErr = errors.New("clone: guest kernel panicked before vsock came up") m := newTestManager(t, eng, config.PoolSpec{PoolKey: testKey, Warm: 50}) m.pools[testKey].goldenDir = "/goldens/x" diff --git a/sandboxd/pool/refillkick_test.go b/sandboxd/pool/refillkick_test.go index 2642748..83ba7f3 100644 --- a/sandboxd/pool/refillkick_test.go +++ b/sandboxd/pool/refillkick_test.go @@ -1,6 +1,12 @@ package pool -import "testing" +import ( + "errors" + "testing" + "time" + + "github.com/cocoonstack/sandbox/sandboxd/config" +) func TestKickRefillCoalescesAndNeverBlocks(t *testing.T) { m := &Manager{refillKick: make(chan struct{}, 1)} @@ -16,3 +22,78 @@ func TestKickRefillCoalescesAndNeverBlocks(t *testing.T) { t.Fatalf("kick after drain not delivered") } } + +// TestNodeAtCapacityParksRefillInsteadOfRetrying pins the difference between +// "this VM failed to start" and "this node cannot hold another VM". A full +// bridge does not clear on its own, so treating it as retryable spends every +// tick forking the engine, building a netns and running CNI before failing — +// each one taking the global rtnl lock. That is how a capacity limit stopped +// being a short warm pool and became a node-wide outage. +func TestNodeAtCapacityParksRefillInsteadOfRetrying(t *testing.T) { + eng := newFakeEngine() + eng.cloneErr = errors.New(`cocoon vm clone: exit status 1: Error: configure network: ` + + `cni add A/eth0: plugin type="bridge" failed (add): ` + + `failed to connect "veth3f2" to bridge cni0: exchange full`) + m := newTestManager(t, eng, config.PoolSpec{PoolKey: testKey, Warm: 4}) + m.pools[testKey].goldenDir = "/goldens/x" + + m.refillOnce(t.Context()) + waitFor(t, func() bool { + infos, _ := m.Info() + return infos[0].Refilling == 0 + }) + attempts := eng.cloneCount() + if attempts == 0 { + t.Fatal("no clone attempted, so the test proves nothing") + } + + // The node published the condition, so a placer can route around it rather + // than read the short warm count as slowness. + if _, g := m.Info(); !g.AtCapacity || g.AtCapacityReason == "" { + t.Fatalf("at-capacity not published: %+v", g) + } + + // Every later tick is a no-op until the backoff expires. Without this the + // pool is still short of target, which is exactly the condition that drove + // the storm. + for range 5 { + m.refillOnce(t.Context()) + } + if n := eng.cloneCount(); n != attempts { + t.Errorf("clones=%d after parking, want %d: a parked node must not attempt again", n, attempts) + } + + // It is a park, not a latch: once the backoff expires refill resumes, or a + // node that briefly filled would stay idle until restarted. + m.mu.Lock() + m.atCapacityUntil = time.Now().Add(-time.Second) + m.mu.Unlock() + eng.cloneErr = nil + m.refillOnce(t.Context()) + waitFor(t, func() bool { + infos, _ := m.Info() + return infos[0].Warm == 4 + }) +} + +// TestOnlyNodeWideFailuresPark guards the classifier. Parking on an ordinary +// clone failure would stall a healthy node for 30s over one bad VM. +func TestOnlyNodeWideFailuresPark(t *testing.T) { + for _, tc := range []struct { + name string + err error + want bool + }{ + {"bridge port exhaustion", errors.New(`failed to connect "veth1" to bridge cni0: exchange full`), true}, + {"disk exhaustion", errors.New("write overlay: no space left on device"), true}, + {"one VM failed to boot", errors.New("cocoon vm clone: exit status 1: Error: guest did not respond"), false}, + {"missing golden", errors.New("cocoon vm clone: exit status 1: Error: snapshot not found"), false}, + {"no error", nil, false}, + } { + t.Run(tc.name, func(t *testing.T) { + if _, got := nodeAtCapacity(tc.err); got != tc.want { + t.Errorf("nodeAtCapacity(%v) = %v, want %v", tc.err, got, tc.want) + } + }) + } +} diff --git a/sandboxd/server/server.go b/sandboxd/server/server.go index 27a71f5..1450279 100644 --- a/sandboxd/server/server.go +++ b/sandboxd/server/server.go @@ -120,6 +120,11 @@ type InfoResponse struct { Archived int `json:"archived"` Draining bool `json:"draining,omitempty"` Peers []string `json:"peers,omitempty"` + // AtCapacity reports that the node cannot attach another VM and has parked + // refill. A placer should read a short warm count on such a node as "full", + // not "still filling", and send the work somewhere else. + AtCapacity bool `json:"at_capacity,omitempty"` + AtCapacityReason string `json:"at_capacity_reason,omitempty"` } // PoolUpdateRequest is the wire body of PUT /v1/pools. It replaces the node's @@ -543,7 +548,10 @@ func (s *Server) handlePeers(w http.ResponseWriter, _ *http.Request) { func (s *Server) handleInfo(w http.ResponseWriter, _ *http.Request) { pools, g := s.mgr.Info() - resp := InfoResponse{Pools: pools, Claimed: g.Claimed, Hibernated: g.Hibernated, Archived: g.Archived, Draining: g.Draining} + resp := InfoResponse{ + Pools: pools, Claimed: g.Claimed, Hibernated: g.Hibernated, Archived: g.Archived, + Draining: g.Draining, AtCapacity: g.AtCapacity, AtCapacityReason: g.AtCapacityReason, + } if s.placer != nil { resp.Peers = s.placer.PeerAddrs() } From f89696b318f81ba409462871dd28c6063cb30cd6 Mon Sep 17 00:00:00 2001 From: CMGS Date: Wed, 29 Jul 2026 23:18:25 +0800 Subject: [PATCH 4/6] review: classify capacity at the engine boundary; verify every remove 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. --- sandboxd/engine/capacity_test.go | 28 +++++ sandboxd/engine/engine.go | 23 ++++ sandboxd/pool/archive.go | 4 +- sandboxd/pool/claim.go | 4 +- sandboxd/pool/egress.go | 7 +- sandboxd/pool/egress_test.go | 3 +- sandboxd/pool/pool.go | 45 ++----- sandboxd/pool/pool_test.go | 10 +- sandboxd/pool/refill.go | 184 ++++++++++++---------------- sandboxd/pool/refillbackoff_test.go | 6 +- sandboxd/pool/refillkick_test.go | 37 +----- sandboxd/pool/stats.go | 13 +- 12 files changed, 154 insertions(+), 210 deletions(-) create mode 100644 sandboxd/engine/capacity_test.go diff --git a/sandboxd/engine/capacity_test.go b/sandboxd/engine/capacity_test.go new file mode 100644 index 0000000..5c1be76 --- /dev/null +++ b/sandboxd/engine/capacity_test.go @@ -0,0 +1,28 @@ +package engine + +import ( + "errors" + "testing" +) + +// TestOnlyNodeWideFailuresCarryACapacitySignature guards the classifier. +// Parking on an ordinary clone failure would stall a healthy node over one bad VM. +func TestOnlyNodeWideFailuresCarryACapacitySignature(t *testing.T) { + for _, tc := range []struct { + name string + err error + want bool + }{ + {"bridge port exhaustion", errors.New(`failed to connect "veth1" to bridge cni0: exchange full`), true}, + {"disk exhaustion", errors.New("write overlay: no space left on device"), true}, + {"one VM failed to boot", errors.New("cocoon vm clone: exit status 1: Error: guest did not respond"), false}, + {"missing golden", errors.New("cocoon vm clone: exit status 1: Error: snapshot not found"), false}, + {"no error", nil, false}, + } { + t.Run(tc.name, func(t *testing.T) { + if got := CapacitySignature(tc.err) != ""; got != tc.want { + t.Errorf("CapacitySignature(%v) carries a signature: %v, want %v", tc.err, got, tc.want) + } + }) + } +} diff --git a/sandboxd/engine/engine.go b/sandboxd/engine/engine.go index 05c7e67..a3b1da5 100644 --- a/sandboxd/engine/engine.go +++ b/sandboxd/engine/engine.go @@ -47,6 +47,15 @@ const ( portForwardMax = 4096 ) +// capacitySignatures mean "this node cannot attach another VM", not "this VM +// failed"; matched as text because cocoon is a subprocess and the distinction +// survives only in its stderr. "exchange full" is EXFULL from a bridge at +// BR_MAX_PORTS — permanent until VMs are removed. +var capacitySignatures = []string{ + "exchange full", + "no space left on device", +} + // Engine runs cocoon commands on the local node. type Engine struct { bin string @@ -463,6 +472,20 @@ func readLine(conn net.Conn, max int) (string, error) { return "", fmt.Errorf("reply exceeds %d bytes", max) } +// CapacitySignature returns the node-capacity signature err carries, or "". +func CapacitySignature(err error) string { + if err == nil { + return "" + } + msg := strings.ToLower(err.Error()) + for _, sig := range capacitySignatures { + if strings.Contains(msg, sig) { + return sig + } + } + return "" +} + func tail(s string) string { s = strings.TrimSpace(s) if len(s) <= outputTail { diff --git a/sandboxd/pool/archive.go b/sandboxd/pool/archive.go index e1cc1e7..c34d919 100644 --- a/sandboxd/pool/archive.go +++ b/sandboxd/pool/archive.go @@ -141,9 +141,7 @@ func (m *Manager) archive(ctx context.Context, sb *types.Sandbox) error { m.disarmEgress(sb.ID, true) sb.Transition.Unlock() // Committed: the store ck is authoritative now; reclaim the local footprint. - if rmErr := m.eng.Remove(ctx, vmName); rmErr != nil { - log.WithFunc("pool.archive").Warnf(ctx, "remove archived VM %s: %v", vmName, rmErr) - } + m.destroy(ctx, vmName) m.dropSnap(ctx, snap) m.counters.archives.Add(1) m.recordUsage(ctx, usageEvent{Event: "archive", ID: sb.ID, Reference: ck.ID, Tenant: sb.Tenant}) diff --git a/sandboxd/pool/claim.go b/sandboxd/pool/claim.go index 5a88052..f43156d 100644 --- a/sandboxd/pool/claim.go +++ b/sandboxd/pool/claim.go @@ -129,8 +129,8 @@ func (m *Manager) releaseResolved(ctx context.Context, id string, sb *types.Sand m.purgeArchiveCk(ctx, id, ck, sb.Tenant) // archived: no local VM } var err error - if vmName != "" { - err = m.eng.Remove(ctx, vmName) + if vmName != "" && !m.removeVM(ctx, vmName) { + err = fmt.Errorf("vm %s survived removal", vmName) } m.disarmEgress(id, err == nil) m.dropSnap(ctx, snap) diff --git a/sandboxd/pool/egress.go b/sandboxd/pool/egress.go index 5753750..290f141 100644 --- a/sandboxd/pool/egress.go +++ b/sandboxd/pool/egress.go @@ -120,15 +120,14 @@ func (m *Manager) lockEgressNIC(ctx context.Context, sb *types.Sandbox) error { } func (m *Manager) tapOf(ctx context.Context, vmName string) (string, error) { - vms, err := m.eng.List(ctx, vmName) + vm, ok, err := m.findVM(ctx, vmName) if err != nil { return "", fmt.Errorf("list %s: %w", vmName, err) } - i := slices.IndexFunc(vms, func(vm types.VMRecord) bool { return vm.Config.Name == vmName }) - if i < 0 { + if !ok { return "", fmt.Errorf("no NIC config for %s", vmName) } - if tap := vms[i].TapDevice(); tap != "" { + if tap := vm.TapDevice(); tap != "" { return tap, nil } return "", fmt.Errorf("no tap for %s", vmName) diff --git a/sandboxd/pool/egress_test.go b/sandboxd/pool/egress_test.go index b4b0c45..eada552 100644 --- a/sandboxd/pool/egress_test.go +++ b/sandboxd/pool/egress_test.go @@ -290,8 +290,7 @@ func TestRollbackKeepsLockWhenRemoveFails(t *testing.T) { func TestQuarantineFailedRemoveStaysUnswept(t *testing.T) { eng := newFakeEngine() eng.removeErrFor = "sbx-q1" - // The VM is running: a failed remove only means "still here" if the engine - // still lists it, which is what removeVM now checks. + // Running: removeVM reports "still here" only while the engine lists it. eng.vms["sbx-q1"] = "/run/sbx-q1.sock" m := egressManager(t, eng, config.PoolSpec{PoolKey: egKey, Egress: egPolicy}) sb := &types.Sandbox{ID: "sb_q1", VMName: "sbx-q1", Key: egKey, TAP: "tap-q1"} diff --git a/sandboxd/pool/pool.go b/sandboxd/pool/pool.go index 0b94648..c6f3cda 100644 --- a/sandboxd/pool/pool.go +++ b/sandboxd/pool/pool.go @@ -40,33 +40,14 @@ const ( buildRetryDelay = 30 * time.Second claimProbeTimeout = 15 * time.Second coldProbeTimeout = 90 * time.Second - // warmProbeTimeout bounds the readiness probe of a warm-pool refill, - // which — unlike a claim — has no caller waiting on it. A clone answers - // in about a second even with the node saturated, so one that has said - // nothing for this long is an outlier worth replacing rather than - // waiting on: the refill holds a slot in the pool's target accounting - // for its whole probe, so a long deadline here stalls the LAST few of a - // large fill (deadline + refillInterval + a fresh boot) even though the - // replacement costs about a second. Claims keep the generous deadline. + // A refill has no caller waiting and holds a target-accounting slot for + // its whole probe; a clone answers in ~1s even saturated, so a silent one + // is replaced, not waited on. Claims keep the generous deadline. warmProbeTimeout = 5 * time.Second - // removeTimeout bounds one `cocoon vm rm`; see removeVM for why a remove - // must never outlast its caller. It is generous because the command - // contends with every concurrent create on the node's metadata store, and - // under a large scale-down that contention is the normal case, not the - // exception — a tight bound there turns routine slowness into a killed - // remove, and a killed remove can leave the guest running. - removeTimeout = 120 * time.Second - // removeVerifyTimeout bounds the check that a failed remove really did - // leave nothing behind. Short: it is one list, and being wrong here only - // costs an extra sweep. + // One list; a wrong answer only costs an extra sweep. removeVerifyTimeout = 15 * time.Second - // capacityBackoff parks refill after the node reports it cannot attach - // another VM. The condition is a property of the node and clears only when - // VMs go away, so the refill ticker would otherwise retry a permanent - // failure at full concurrency forever. Matched to buildRetryDelay: long - // enough that the retry costs nothing, short enough that a drain is picked - // up promptly. - capacityBackoff = 30 * time.Second + // A full node clears only when VMs go away; retrying sooner buys nothing. + capacityBackoff = buildRetryDelay vsockPollInterval = 100 * time.Millisecond defaultTTL = 5 * time.Minute maxTTL = 24 * time.Hour @@ -161,12 +142,9 @@ type Gauges struct { Hibernated int Archived int Draining bool - // AtCapacity reports that the node refused to attach another VM and refill - // is parked. It is published so a placer can send work elsewhere instead of - // reading this node's short warm count as transient slowness. - AtCapacity bool - // AtCapacityReason carries what the node said, so the condition is - // diagnosable from the API rather than only from the node's logs. + // AtCapacity marks refill parked because the node refused another VM, so a + // placer reads a short warm count as "full", not "still filling". + AtCapacity bool AtCapacityReason string } @@ -357,9 +335,8 @@ type Manager struct { pools map[types.PoolKey]*pool claimed map[string]*types.Sandbox - // atCapacityUntil parks every pool's refill after the node reports it can - // hold no more VMs. It is node-wide rather than per-pool because the - // resource that ran out — bridge ports — is shared by all of them. + // Node-wide, unlike the per-pool streak backoff: the resource that ran + // out — bridge ports, disk — is shared by every pool. atCapacityUntil time.Time atCapacityReason string diff --git a/sandboxd/pool/pool_test.go b/sandboxd/pool/pool_test.go index 5750c46..3cd64f6 100644 --- a/sandboxd/pool/pool_test.go +++ b/sandboxd/pool/pool_test.go @@ -1063,14 +1063,8 @@ func (f *fakeEngine) snapRemoved(name string) bool { } // TestRemoveVMWillNotReportGoneWhileTheVMRuns pins the invariant a bounded -// remove has to preserve. `cocoon vm rm` contends with every concurrent create -// on the node's metadata store, so under a large scale-down it routinely runs -// long; the bound exists so one slow remove cannot pin the refill slot forever. -// But a bound that reports success on expiry is worse than no bound: the -// timeout SIGKILLs the command partway through, the guest keeps running, and -// this manager forgets it exists. The slot is released, the pool refills, and -// the node accumulates VMs nothing is accounting for — silently, and faster the -// harder you drive it. Measured once at ~570 orphans per node. +// remove must keep: a bound that reports success on expiry silently leaks the +// still-running guest from all accounting — measured once at ~570 orphans a node. func TestRemoveVMWillNotReportGoneWhileTheVMRuns(t *testing.T) { eng := newFakeEngine() m := &Manager{eng: eng} diff --git a/sandboxd/pool/refill.go b/sandboxd/pool/refill.go index 1853b2e..debdcb0 100644 --- a/sandboxd/pool/refill.go +++ b/sandboxd/pool/refill.go @@ -13,6 +13,7 @@ import ( "github.com/projecteru2/core/log" + "github.com/cocoonstack/sandbox/sandboxd/engine" "github.com/cocoonstack/sandbox/sandboxd/types" ) @@ -32,13 +33,10 @@ func (m *Manager) refillOnce(ctx context.Context) { return } now := time.Now() - if now.Before(m.atCapacityUntil) { - // The node cannot attach another VM. Spawning refills anyway is what - // turned a capacity limit into an outage: each attempt still forks the - // engine, builds a netns and runs the CNI plugins before failing, and - // every one of those takes the global rtnl lock on the way through. - return - } + // Spawning VMs into a full node is what turned a capacity limit into an + // outage: every doomed attempt still takes the global rtnl lock. The park + // gates spawns only — sweeping removed pools is pure bookkeeping. + parked := now.Before(m.atCapacityUntil) inFlight := 0 for key, p := range m.pools { inFlight += p.refilling @@ -50,11 +48,14 @@ func (m *Manager) refillOnce(ctx context.Context) { } continue } - if p.goldenDir == "" && !p.building && now.After(p.nextBuild) { + if !parked && p.goldenDir == "" && !p.building && now.After(p.nextBuild) { p.building = true go m.buildGolden(ctx, p) } } + if parked { + return + } limit := cap(m.refillSem) + cap(m.probeSem) for spawned := true; spawned && inFlight < limit; { spawned = false @@ -94,6 +95,7 @@ func (m *Manager) refillOne(ctx context.Context, p *pool, golden string) { keep := false var fails int var wait time.Duration + capReason := engine.CapacitySignature(err) m.mu.Lock() now := time.Now() p.refilling-- @@ -102,30 +104,33 @@ func (m *Manager) refillOne(ctx context.Context, p *pool, golden string) { p.noteLead(time.Since(start)) keep = true } - // A canceled context is the daemon going down, not the pool failing. - backedOff := ctx.Err() == nil && p.noteRefillResult(now, err != nil) + // A canceled context is the daemon going down, and a capacity failure is + // the node's fault — neither says anything about this pool's health. + backedOff := false + if ctx.Err() == nil && capReason == "" { + backedOff = p.noteRefillResult(now, err != nil) + } if backedOff { fails, wait = p.refillFails, time.Until(p.nextRefill) } - // Capacity is node-wide, the streak backoff above is per-pool; both park - // refill, so record both and let the capacity episode own the log line. - entered := err != nil && m.noteCapacityLocked(err) + entered := capReason != "" && m.noteCapacityLocked(now, capReason) + parked := now.Before(m.atCapacityUntil) m.mu.Unlock() if err != nil { - if ctx.Err() == nil { + switch { + case ctx.Err() != nil: + case entered: + // One line per episode; the storm this replaces wrote 20k an hour. + log.WithFunc("pool.refillOne").Errorf(ctx, err, + "node is at capacity; parking refill for %s", capacityBackoff) + case parked: + default: hash := p.key.Hash() - if entered { - // One line per episode, not one per attempt: the storm this - // replaces wrote twenty thousand of these an hour. - log.WithFunc("pool.refillOne").Errorf(ctx, err, - "node is at capacity; parking refill for %s", capacityBackoff) - } else if !m.atCapacity() { - log.WithFunc("pool.refillOne").Errorf(ctx, err, "refill %s", hash) - if backedOff { - log.WithFunc("pool.refillOne").Warnf(ctx, - "refill %s failed %d times running; pausing refill for %s", - hash, fails, wait.Round(time.Millisecond)) - } + log.WithFunc("pool.refillOne").Errorf(ctx, err, "refill %s", hash) + if backedOff { + log.WithFunc("pool.refillOne").Warnf(ctx, + "refill %s failed %d times running; pausing refill for %s", + hash, fails, wait.Round(time.Millisecond)) } } return @@ -138,52 +143,15 @@ func (m *Manager) refillOne(ctx context.Context, p *pool, golden string) { } } -// noteCapacityLocked parks refill when err says the node itself is full, and -// reports whether this call is what parked it — so the condition is logged once -// per episode rather than once per failed attempt. Called with m.mu held. -func (m *Manager) noteCapacityLocked(err error) bool { - reason, full := nodeAtCapacity(err) - if !full { - return false - } - first := !time.Now().Before(m.atCapacityUntil) - m.atCapacityUntil = time.Now().Add(capacityBackoff) +// noteCapacityLocked parks refill node-wide for a capacity failure, reporting +// whether this call parked it so the episode is logged once. m.mu held. +func (m *Manager) noteCapacityLocked(now time.Time, reason string) bool { + first := !now.Before(m.atCapacityUntil) + m.atCapacityUntil = now.Add(capacityBackoff) m.atCapacityReason = reason return first } -func (m *Manager) atCapacity() bool { - m.mu.Lock() - defer m.mu.Unlock() - return time.Now().Before(m.atCapacityUntil) -} - -// capacitySignatures are the failures that mean "this node cannot attach -// another VM", as opposed to "this VM failed to start". They are matched as -// text because the engine is a subprocess: its exit status is 1 for every -// failure and the distinction survives only in the message. -// -// "exchange full" is the kernel's EXFULL, which a Linux bridge returns for its -// 1025th port — BR_MAX_PORTS is a compile-time constant, so the condition is -// permanent until VMs are removed, and no amount of retrying clears it. -var capacitySignatures = []string{ - "exchange full", - "no space left on device", -} - -func nodeAtCapacity(err error) (string, bool) { - if err == nil { - return "", false - } - msg := strings.ToLower(err.Error()) - for _, sig := range capacitySignatures { - if strings.Contains(msg, sig) { - return sig, true - } - } - return "", false -} - func (m *Manager) buildGolden(ctx context.Context, p *pool) { logger := log.WithFunc("pool.buildGolden") hash := p.key.Hash() @@ -203,6 +171,11 @@ func (m *Manager) buildGolden(ctx context.Context, p *pool) { p.goldenDir = final } else { p.nextBuild = time.Now().Add(buildRetryDelay) + // A cold boot hits the same bridge and disk as a refill; feed the park + // so a node failing only its builds still publishes at-capacity. + if reason := engine.CapacitySignature(err); reason != "" { + m.noteCapacityLocked(time.Now(), reason) + } } m.mu.Unlock() if err != nil { @@ -438,18 +411,30 @@ func (m *Manager) probeReady(ctx context.Context, name, sock string, timeout tim } func (m *Manager) vsockOf(ctx context.Context, name string) (string, error) { - vms, err := m.eng.List(ctx, name) + vm, ok, err := m.findVM(ctx, name) if err != nil { return "", err } - i := slices.IndexFunc(vms, func(vm types.VMRecord) bool { return vm.Config.Name == name }) - if i < 0 { + if !ok { return "", fmt.Errorf("vm %s not found after create", name) } - if vms[i].VsockSocket == "" { + if vm.VsockSocket == "" { return "", fmt.Errorf("vm %s has no vsock socket", name) } - return vms[i].VsockSocket, nil + return vm.VsockSocket, nil +} + +// findVM returns the engine's record for name, if it still lists one. +func (m *Manager) findVM(ctx context.Context, name string) (types.VMRecord, bool, error) { + vms, err := m.eng.List(ctx, name) + if err != nil { + return types.VMRecord{}, false, err + } + i := slices.IndexFunc(vms, func(vm types.VMRecord) bool { return vm.Config.Name == name }) + if i < 0 { + return types.VMRecord{}, false, nil + } + return vms[i], true, nil } // runBounded fans f over n items on the refill semaphore, so engine @@ -475,56 +460,37 @@ func (m *Manager) runBounded(ctx context.Context, n int, f func(context.Context, } // removeVM deletes a VM (on a cancellation-immune ctx, else `cocoon vm rm` -// no-ops and orphans it) and reports whether it is confirmed gone; a failed -// remove leaves it running, so the caller must keep its lock. -// -// The remove is cancellation-immune but NOT unbounded: `cocoon vm rm` contends -// with every concurrent create on the node's metadata store, and under a large -// fill that contention can outlast any caller. A remove that never returns pins -// whatever the caller was holding — refillOne cleans up a failed create before -// it releases the refill semaphore and decrements p.refilling, so one wedged -// remove leaves the pool permanently one short of target. Give up after -// removeTimeout instead: cocoon keeps a tombstone and its own gc resumes the -// delete, and the reaper sweeps whatever is still running. +// no-ops and orphans it) and reports whether it is confirmed gone; when it is +// not, the caller must keep its lock and accounting. The engine bounds the +// command, a killed or failed remove can leave the guest alive, and cocoon's +// tombstone gc plus the reaper converge whatever it left behind. func (m *Manager) removeVM(ctx context.Context, name string) bool { ctx = context.WithoutCancel(ctx) - rmCtx, cancel := context.WithTimeout(ctx, removeTimeout) - defer cancel() - if err := m.eng.Remove(rmCtx, name); err == nil { + err := m.eng.Remove(ctx, name) + if err == nil { return true - } else { - log.WithFunc("pool.removeVM").Warnf(ctx, "remove vm %s: %v; verifying", name, err) } - - // The remove failed or ran out of time, and the difference that matters is - // whether the VM is still running — not why the command stopped. A timeout - // SIGKILLs `cocoon vm rm` partway through, which can leave the guest alive - // while this process forgets it exists: the slot is released, the pool - // refills, and the machine accumulates VMs nobody is accounting for. That - // failure is silent and compounding, so confirm before reporting gone. + log.WithFunc("pool.removeVM").Warnf(ctx, "remove vm %s: %v; verifying", name, err) return m.confirmGone(ctx, name) } -// confirmGone reports whether name is absent from the engine's own view. -// A VM the engine still lists is reported as present, which keeps it in this -// manager's accounting so the reaper retries it rather than leaking it. +// confirmGone reports whether name is absent from the engine's own view; a VM +// still listed stays in this manager's accounting so the reaper retries it. func (m *Manager) confirmGone(ctx context.Context, name string) bool { ctx, cancel := context.WithTimeout(ctx, removeVerifyTimeout) defer cancel() - vms, err := m.eng.List(ctx, name) + _, present, err := m.findVM(ctx, name) if err != nil { - // Cannot tell. Assume it is still there: a false "gone" leaks a running - // VM, a false "still here" only costs another sweep. + // Cannot tell: a false "gone" leaks a running VM, a false "still + // here" only costs another sweep. log.WithFunc("pool.confirmGone").Warnf(ctx, "verify remove of %s: %v", name, err) return false } - for i := range vms { - if vms[i].Config.Name == name { - log.WithFunc("pool.confirmGone").Errorf(ctx, - fmt.Errorf("vm %s survived removal", name), - "remove did not take effect; leaving it accounted for retry") - return false - } + if present { + log.WithFunc("pool.confirmGone").Errorf(ctx, + fmt.Errorf("vm %s survived removal", name), + "remove did not take effect; leaving it accounted for retry") + return false } return true } diff --git a/sandboxd/pool/refillbackoff_test.go b/sandboxd/pool/refillbackoff_test.go index 881e366..c7ecad2 100644 --- a/sandboxd/pool/refillbackoff_test.go +++ b/sandboxd/pool/refillbackoff_test.go @@ -64,10 +64,8 @@ func TestBackoffGrowsAndIsCapped(t *testing.T) { // while a pool is backed off, the ticker spawns nothing. func TestRefillOnceHonorsTheBackoff(t *testing.T) { eng := newFakeEngine() - // Deliberately not a capacitySignature: those are parked node-wide after a - // single failure (see noteCapacityLocked), which is a different mechanism - // and would stop the per-pool streak from ever reaching refillFailStreak. - // This backoff is the fallback for a failure the node cannot classify. + // Deliberately not a capacitySignature: that parks the node after one + // failure; the streak backoff is the fallback for unclassifiable errors. eng.cloneErr = errors.New("clone: guest kernel panicked before vsock came up") m := newTestManager(t, eng, config.PoolSpec{PoolKey: testKey, Warm: 50}) m.pools[testKey].goldenDir = "/goldens/x" diff --git a/sandboxd/pool/refillkick_test.go b/sandboxd/pool/refillkick_test.go index 83ba7f3..3634362 100644 --- a/sandboxd/pool/refillkick_test.go +++ b/sandboxd/pool/refillkick_test.go @@ -24,11 +24,7 @@ func TestKickRefillCoalescesAndNeverBlocks(t *testing.T) { } // TestNodeAtCapacityParksRefillInsteadOfRetrying pins the difference between -// "this VM failed to start" and "this node cannot hold another VM". A full -// bridge does not clear on its own, so treating it as retryable spends every -// tick forking the engine, building a netns and running CNI before failing — -// each one taking the global rtnl lock. That is how a capacity limit stopped -// being a short warm pool and became a node-wide outage. +// "this VM failed to start" and "this node cannot hold another VM". func TestNodeAtCapacityParksRefillInsteadOfRetrying(t *testing.T) { eng := newFakeEngine() eng.cloneErr = errors.New(`cocoon vm clone: exit status 1: Error: configure network: ` + @@ -47,15 +43,11 @@ func TestNodeAtCapacityParksRefillInsteadOfRetrying(t *testing.T) { t.Fatal("no clone attempted, so the test proves nothing") } - // The node published the condition, so a placer can route around it rather - // than read the short warm count as slowness. if _, g := m.Info(); !g.AtCapacity || g.AtCapacityReason == "" { t.Fatalf("at-capacity not published: %+v", g) } - // Every later tick is a no-op until the backoff expires. Without this the - // pool is still short of target, which is exactly the condition that drove - // the storm. + // Every later tick is a no-op until the backoff expires. for range 5 { m.refillOnce(t.Context()) } @@ -63,8 +55,7 @@ func TestNodeAtCapacityParksRefillInsteadOfRetrying(t *testing.T) { t.Errorf("clones=%d after parking, want %d: a parked node must not attempt again", n, attempts) } - // It is a park, not a latch: once the backoff expires refill resumes, or a - // node that briefly filled would stay idle until restarted. + // A park, not a latch: once the backoff expires refill resumes. m.mu.Lock() m.atCapacityUntil = time.Now().Add(-time.Second) m.mu.Unlock() @@ -75,25 +66,3 @@ func TestNodeAtCapacityParksRefillInsteadOfRetrying(t *testing.T) { return infos[0].Warm == 4 }) } - -// TestOnlyNodeWideFailuresPark guards the classifier. Parking on an ordinary -// clone failure would stall a healthy node for 30s over one bad VM. -func TestOnlyNodeWideFailuresPark(t *testing.T) { - for _, tc := range []struct { - name string - err error - want bool - }{ - {"bridge port exhaustion", errors.New(`failed to connect "veth1" to bridge cni0: exchange full`), true}, - {"disk exhaustion", errors.New("write overlay: no space left on device"), true}, - {"one VM failed to boot", errors.New("cocoon vm clone: exit status 1: Error: guest did not respond"), false}, - {"missing golden", errors.New("cocoon vm clone: exit status 1: Error: snapshot not found"), false}, - {"no error", nil, false}, - } { - t.Run(tc.name, func(t *testing.T) { - if _, got := nodeAtCapacity(tc.err); got != tc.want { - t.Errorf("nodeAtCapacity(%v) = %v, want %v", tc.err, got, tc.want) - } - }) - } -} diff --git a/sandboxd/pool/stats.go b/sandboxd/pool/stats.go index d10a883..e5c67ff 100644 --- a/sandboxd/pool/stats.go +++ b/sandboxd/pool/stats.go @@ -4,12 +4,9 @@ import ( "context" "os" "path/filepath" - "slices" "strconv" "strings" "time" - - "github.com/cocoonstack/sandbox/sandboxd/types" ) // SandboxStats is one sandbox's resource usage. CPUCount and MemTotalBytes come @@ -63,15 +60,11 @@ func (m *Manager) vmResidentBytes(ctx context.Context, vmName string) (int64, bo if vmName == "" { return 0, false } - vms, err := m.eng.List(ctx, vmName) - if err != nil { - return 0, false - } - i := slices.IndexFunc(vms, func(vm types.VMRecord) bool { return vm.Config.Name == vmName }) - if i < 0 || vms[i].PID <= 0 { + vm, ok, err := m.findVM(ctx, vmName) + if err != nil || !ok || vm.PID <= 0 { return 0, false } - return residentBytes(vms[i].PID) + return residentBytes(vm.PID) } // residentBytes reads a process's resident page count from statm's second field. From 0fbbc2022766b4a71325fd983c5423f5e536c772 Mon Sep 17 00:00:00 2001 From: CMGS Date: Thu, 30 Jul 2026 00:02:50 +0800 Subject: [PATCH 5/6] fix(pool): retry confirmed VM removal survivors Keep verified survivors in the existing reap loop until removal is confirmed. Preserve egress cleanup state, while startup reconciliation remains the crash-recovery authority. --- sandboxd/pool/claim.go | 6 +- sandboxd/pool/pool.go | 15 +++- sandboxd/pool/pool_test.go | 38 ----------- sandboxd/pool/reconcile.go | 11 ++- sandboxd/pool/refill.go | 38 ----------- sandboxd/pool/remove.go | 101 +++++++++++++++++++++++++++ sandboxd/pool/remove_test.go | 128 +++++++++++++++++++++++++++++++++++ 7 files changed, 249 insertions(+), 88 deletions(-) create mode 100644 sandboxd/pool/remove.go create mode 100644 sandboxd/pool/remove_test.go diff --git a/sandboxd/pool/claim.go b/sandboxd/pool/claim.go index f43156d..2c47605 100644 --- a/sandboxd/pool/claim.go +++ b/sandboxd/pool/claim.go @@ -129,7 +129,7 @@ func (m *Manager) releaseResolved(ctx context.Context, id string, sb *types.Sand m.purgeArchiveCk(ctx, id, ck, sb.Tenant) // archived: no local VM } var err error - if vmName != "" && !m.removeVM(ctx, vmName) { + if vmName != "" && !m.removeOrRetry(ctx, vmName, id) { err = fmt.Errorf("vm %s survived removal", vmName) } m.disarmEgress(id, err == nil) @@ -291,7 +291,7 @@ func (m *Manager) rollbackClaim(ctx context.Context, sbs []*types.Sandbox) { m.mu.Unlock() m.recommit(ctx, rb) for _, sb := range sbs { - m.disarmEgress(sb.ID, m.removeVM(ctx, sb.VMName)) + m.disarmEgress(sb.ID, m.removeOrRetry(ctx, sb.VMName, sb.ID)) } } @@ -370,7 +370,7 @@ func (m *Manager) reapOnce(ctx context.Context) { logger.Errorf(ctx, err, "archive expired %s", v.id) } default: - m.disarmEgress(v.id, m.removeVM(ctx, v.vmName)) + m.disarmEgress(v.id, m.removeOrRetry(ctx, v.vmName, v.id)) m.dropSnap(ctx, v.snap) m.counters.reaps.Add(1) m.recordUsage(ctx, usageEvent{Event: "reap", ID: v.id, VMName: v.vmName}) diff --git a/sandboxd/pool/pool.go b/sandboxd/pool/pool.go index c6f3cda..a9d8a74 100644 --- a/sandboxd/pool/pool.go +++ b/sandboxd/pool/pool.go @@ -148,6 +148,12 @@ type Gauges struct { AtCapacityReason string } +type pendingRemoval struct { + sandboxID string + tap string + retrying bool +} + type pool struct { key types.PoolKey @@ -331,9 +337,10 @@ type Manager struct { dial egress.DialFunc sweep func(map[string]bool) error - mu sync.Mutex - pools map[types.PoolKey]*pool - claimed map[string]*types.Sandbox + mu sync.Mutex + pools map[types.PoolKey]*pool + claimed map[string]*types.Sandbox + pendingRemovals map[string]pendingRemoval // Node-wide, unlike the per-pool streak backoff: the resource that ran // out — bridge ports, disk — is shared by every pool. @@ -367,6 +374,7 @@ func NewManager(ctx context.Context, cfg *config.Config, eng Engine, secrets *eg poolStore: newPoolStore(cfg.DataDir), pools: make(map[types.PoolKey]*pool, len(cfg.Pools)), claimed: map[string]*types.Sandbox{}, + pendingRemovals: map[string]pendingRemoval{}, tenantLive: map[string]int{}, archiving: map[string]struct{}{}, pendingCks: map[string]struct{}{}, @@ -502,6 +510,7 @@ func (m *Manager) Run(ctx context.Context) { case <-m.refillKick: m.refillOnce(ctx) case <-reap.C: + m.retryRemovals(ctx) m.reapOnce(ctx) m.idleOnce(ctx) m.archiveOnce(ctx) diff --git a/sandboxd/pool/pool_test.go b/sandboxd/pool/pool_test.go index 3cd64f6..83ee48b 100644 --- a/sandboxd/pool/pool_test.go +++ b/sandboxd/pool/pool_test.go @@ -1061,41 +1061,3 @@ func (f *fakeEngine) snapRemoved(name string) bool { defer f.mu.Unlock() return slices.Contains(f.snapRemoves, name) } - -// TestRemoveVMWillNotReportGoneWhileTheVMRuns pins the invariant a bounded -// remove must keep: a bound that reports success on expiry silently leaks the -// still-running guest from all accounting — measured once at ~570 orphans a node. -func TestRemoveVMWillNotReportGoneWhileTheVMRuns(t *testing.T) { - eng := newFakeEngine() - m := &Manager{eng: eng} - - // A remove that fails outright, on a VM the engine still lists. - eng.mu.Lock() - eng.vms["survivor"] = "/run/survivor.sock" - eng.removeErrFor = "survivor" - eng.mu.Unlock() - - if m.removeVM(context.Background(), "survivor") { - t.Fatal("a VM the engine still lists was reported gone; its slot would be " + - "released while the guest keeps running") - } - - // The same failure, but the VM really is absent: reporting it gone is right, - // and is what stops a permanently failing remove from pinning a slot. - eng.mu.Lock() - delete(eng.vms, "survivor") - eng.mu.Unlock() - if !m.removeVM(context.Background(), "survivor") { - t.Fatal("a VM the engine no longer lists must be reported gone, whatever " + - "the remove command said, or a permanently failing remove pins the slot") - } - - // The ordinary path is unchanged. - eng.mu.Lock() - eng.vms["ordinary"] = "/run/ordinary.sock" - eng.removeErrFor = "" - eng.mu.Unlock() - if !m.removeVM(context.Background(), "ordinary") || !eng.removed("ordinary") { - t.Fatal("the ordinary remove path regressed") - } -} diff --git a/sandboxd/pool/reconcile.go b/sandboxd/pool/reconcile.go index a029e6e..14511d0 100644 --- a/sandboxd/pool/reconcile.go +++ b/sandboxd/pool/reconcile.go @@ -131,9 +131,11 @@ func (m *Manager) sweepStaleVMs(ctx context.Context, live map[string]types.VMRec } gone := make([]bool, len(stale)) // distinct indices: no lock under the Wait barrier m.runBounded(ctx, len(stale), func(ctx context.Context, i int) { - if m.removeVM(ctx, stale[i]) { + if m.removeOrRetry(ctx, stale[i], "") { gone[i] = true logger.Infof(ctx, "removed stale VM %s", stale[i]) + } else { + m.queueRemoval(stale[i], "", live[stale[i]].TapDevice()) } }).Wait() removed := make(map[string]bool, len(stale)) @@ -190,12 +192,9 @@ func (m *Manager) resyncEgress(ctx context.Context, live map[string]types.VMReco } } -// quarantineClaim removes a claim whose egress NIC could not be locked and drops -// it from service; it returns whether the VM is confirmed gone (a failed remove -// leaves it for the next restart's stale sweep). A never-applied lock plus a -// failed remove leaves the VM unguarded until a later remove succeeds. +// A failed remove stays out of service and queued until teardown succeeds. func (m *Manager) quarantineClaim(ctx context.Context, sb *types.Sandbox) bool { - gone := m.removeVM(ctx, sb.VMName) + gone := m.removeOrRetry(ctx, sb.VMName, sb.ID) m.mu.Lock() delete(m.claimed, sb.ID) m.tenantDelta(sb.Tenant, -1) diff --git a/sandboxd/pool/refill.go b/sandboxd/pool/refill.go index debdcb0..191d10f 100644 --- a/sandboxd/pool/refill.go +++ b/sandboxd/pool/refill.go @@ -459,44 +459,6 @@ func (m *Manager) runBounded(ctx context.Context, n int, f func(context.Context, return &wg } -// removeVM deletes a VM (on a cancellation-immune ctx, else `cocoon vm rm` -// no-ops and orphans it) and reports whether it is confirmed gone; when it is -// not, the caller must keep its lock and accounting. The engine bounds the -// command, a killed or failed remove can leave the guest alive, and cocoon's -// tombstone gc plus the reaper converge whatever it left behind. -func (m *Manager) removeVM(ctx context.Context, name string) bool { - ctx = context.WithoutCancel(ctx) - err := m.eng.Remove(ctx, name) - if err == nil { - return true - } - log.WithFunc("pool.removeVM").Warnf(ctx, "remove vm %s: %v; verifying", name, err) - return m.confirmGone(ctx, name) -} - -// confirmGone reports whether name is absent from the engine's own view; a VM -// still listed stays in this manager's accounting so the reaper retries it. -func (m *Manager) confirmGone(ctx context.Context, name string) bool { - ctx, cancel := context.WithTimeout(ctx, removeVerifyTimeout) - defer cancel() - _, present, err := m.findVM(ctx, name) - if err != nil { - // Cannot tell: a false "gone" leaks a running VM, a false "still - // here" only costs another sweep. - log.WithFunc("pool.confirmGone").Warnf(ctx, "verify remove of %s: %v", name, err) - return false - } - if present { - log.WithFunc("pool.confirmGone").Errorf(ctx, - fmt.Errorf("vm %s survived removal", name), - "remove did not take effect; leaving it accounted for retry") - return false - } - return true -} - -func (m *Manager) destroy(ctx context.Context, name string) { _ = m.removeVM(ctx, name) } - func (m *Manager) dropSnap(ctx context.Context, snap string) { if snap == "" { return diff --git a/sandboxd/pool/remove.go b/sandboxd/pool/remove.go new file mode 100644 index 0000000..b7c6731 --- /dev/null +++ b/sandboxd/pool/remove.go @@ -0,0 +1,101 @@ +package pool + +import ( + "context" + "fmt" + + "github.com/projecteru2/core/log" + + "github.com/cocoonstack/sandbox/sandboxd/netfilter" +) + +func (m *Manager) removeVM(ctx context.Context, name string) bool { + ctx = context.WithoutCancel(ctx) + err := m.eng.Remove(ctx, name) + if err == nil { + return true + } + log.WithFunc("pool.removeVM").Warnf(ctx, "remove vm %s: %v; verifying", name, err) + return m.confirmGone(ctx, name) +} + +func (m *Manager) confirmGone(ctx context.Context, name string) bool { + ctx, cancel := context.WithTimeout(ctx, removeVerifyTimeout) + defer cancel() + _, present, err := m.findVM(ctx, name) + if err != nil { + log.WithFunc("pool.confirmGone").Warnf(ctx, "verify remove of %s: %v", name, err) + return false + } + if present { + log.WithFunc("pool.confirmGone").Errorf(ctx, + fmt.Errorf("vm %s survived removal", name), + "remove did not take effect; leaving it accounted for retry") + return false + } + return true +} + +func (m *Manager) removeOrRetry(ctx context.Context, name, sandboxID string) bool { + if m.removeVM(ctx, name) { + return true + } + m.queueRemoval(name, sandboxID, "") + return false +} + +func (m *Manager) queueRemoval(name, sandboxID, tap string) { + m.mu.Lock() + pending := m.pendingRemovals[name] + if pending.sandboxID == "" { + pending.sandboxID = sandboxID + } + if pending.tap == "" { + pending.tap = tap + } + m.pendingRemovals[name] = pending + m.mu.Unlock() +} + +func (m *Manager) retryRemovals(ctx context.Context) { + m.mu.Lock() + names := make([]string, 0, len(m.pendingRemovals)) + for name, pending := range m.pendingRemovals { + if pending.retrying { + continue + } + pending.retrying = true + m.pendingRemovals[name] = pending + names = append(names, name) + } + m.mu.Unlock() + m.runBounded(ctx, len(names), func(ctx context.Context, i int) { + m.retryRemoval(ctx, names[i]) + }) +} + +func (m *Manager) retryRemoval(ctx context.Context, name string) { + removed := m.removeVM(ctx, name) + m.mu.Lock() + pending, ok := m.pendingRemovals[name] + if !ok { + m.mu.Unlock() + return + } + if removed { + delete(m.pendingRemovals, name) + } else { + pending.retrying = false + m.pendingRemovals[name] = pending + } + m.mu.Unlock() + if removed && pending.sandboxID != "" { + m.disarmEgress(pending.sandboxID, true) + } else if removed && pending.tap != "" { + _ = netfilter.Unlock(pending.tap) + } +} + +func (m *Manager) destroy(ctx context.Context, name string) { + m.removeOrRetry(ctx, name, "") +} diff --git a/sandboxd/pool/remove_test.go b/sandboxd/pool/remove_test.go new file mode 100644 index 0000000..46d3a2c --- /dev/null +++ b/sandboxd/pool/remove_test.go @@ -0,0 +1,128 @@ +package pool + +import ( + "testing" + + "github.com/cocoonstack/sandbox/sandboxd/types" +) + +func TestRemoveVMClassifiesOutcome(t *testing.T) { + eng := newFakeEngine() + m := newTestManager(t, eng) + + eng.vms["survivor"] = "/run/survivor.sock" + eng.removeErrFor = "survivor" + if m.removeVM(t.Context(), "survivor") { + t.Fatal("running VM reported gone") + } + + delete(eng.vms, "survivor") + if !m.removeOrRetry(t.Context(), "survivor", "") { + t.Fatal("absent VM reported present") + } + m.mu.Lock() + pending := len(m.pendingRemovals) + m.mu.Unlock() + if pending != 0 { + t.Fatalf("confirmed-gone VM queued for retry: %d", pending) + } + + eng.vms["ordinary"] = "/run/ordinary.sock" + eng.removeErrFor = "" + if !m.removeVM(t.Context(), "ordinary") || !eng.removed("ordinary") { + t.Fatal("ordinary removal failed") + } +} + +func TestRemovalRetryConvergesConfirmedSurvivor(t *testing.T) { + eng := newFakeEngine() + m := newTestManager(t, eng) + eng.vms["survivor"] = "/run/survivor.sock" + eng.removeErrFor = "survivor" + + m.destroy(t.Context(), "survivor") + m.mu.Lock() + _, pending := m.pendingRemovals["survivor"] + m.mu.Unlock() + if !pending { + t.Fatal("surviving VM was not retained for retry") + } + + m.retryRemovals(t.Context()) + waitFor(t, func() bool { + m.mu.Lock() + pending := m.pendingRemovals["survivor"] + m.mu.Unlock() + return !pending.retrying + }) + + eng.mu.Lock() + eng.removeErrFor = "" + eng.mu.Unlock() + m.retryRemovals(t.Context()) + waitFor(t, func() bool { + m.mu.Lock() + _, pending := m.pendingRemovals["survivor"] + m.mu.Unlock() + return !pending && eng.removed("survivor") + }) +} + +func TestRemovalRetryFinishesEgressCleanup(t *testing.T) { + eng := newFakeEngine() + m := newTestManager(t, eng) + m.lockEgress = true + eng.vms["survivor"] = "/run/survivor.sock" + eng.removeErrFor = "survivor" + m.egressTaps["sb_survivor"] = "tap-survivor" + + removed := m.removeOrRetry(t.Context(), "survivor", "sb_survivor") + m.disarmEgress("sb_survivor", removed) + if removed { + t.Fatal("surviving VM reported gone") + } + + eng.mu.Lock() + eng.removeErrFor = "" + eng.mu.Unlock() + m.retryRemovals(t.Context()) + waitFor(t, func() bool { + m.mu.Lock() + _, pending := m.pendingRemovals["survivor"] + _, locked := m.egressTaps["sb_survivor"] + m.mu.Unlock() + return !pending && !locked && eng.removed("survivor") + }) +} + +func TestStaleRemovalRetryKeepsTapForCleanup(t *testing.T) { + eng := newFakeEngine() + m := newTestManager(t, eng) + eng.vms["sbx-stale"] = "/run/stale.sock" + eng.removeErrFor = "sbx-stale" + live := map[string]types.VMRecord{ + "sbx-stale": { + Config: types.VMConfig{Name: "sbx-stale"}, + NetworkConfigs: []types.VMNetConfig{{TAP: "tap-stale"}}, + }, + } + + m.sweepStaleVMs(t.Context(), live, nil) + m.mu.Lock() + tap := m.pendingRemovals["sbx-stale"].tap + m.mu.Unlock() + if tap != "tap-stale" { + t.Fatalf("pending tap %q, want tap-stale", tap) + } + + eng.mu.Lock() + eng.removeErrFor = "" + eng.mu.Unlock() + m.retryRemovals(t.Context()) + waitFor(t, func() bool { + m.mu.Lock() + _, pending := m.pendingRemovals["sbx-stale"] + m.mu.Unlock() + return !pending && eng.removed("sbx-stale") + }) +} From 4784a12bdbc56685dead5ae6a4cf6375d01eb872 Mon Sep 17 00:00:00 2001 From: CMGS Date: Thu, 30 Jul 2026 00:26:12 +0800 Subject: [PATCH 6/6] review: dispatch-drained retry queue; one remove convention 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. --- sandboxd/pool/claim.go | 6 ++-- sandboxd/pool/pool.go | 1 - sandboxd/pool/reconcile.go | 6 ++-- sandboxd/pool/remove.go | 66 ++++++++++++++++-------------------- sandboxd/pool/remove_test.go | 64 +++++++++++++++++----------------- sandboxd/server/server.go | 5 ++- 6 files changed, 69 insertions(+), 79 deletions(-) diff --git a/sandboxd/pool/claim.go b/sandboxd/pool/claim.go index 2c47605..b628eb5 100644 --- a/sandboxd/pool/claim.go +++ b/sandboxd/pool/claim.go @@ -129,7 +129,7 @@ func (m *Manager) releaseResolved(ctx context.Context, id string, sb *types.Sand m.purgeArchiveCk(ctx, id, ck, sb.Tenant) // archived: no local VM } var err error - if vmName != "" && !m.removeOrRetry(ctx, vmName, id) { + if vmName != "" && !m.removeOrRetry(ctx, vmName, id, "") { err = fmt.Errorf("vm %s survived removal", vmName) } m.disarmEgress(id, err == nil) @@ -291,7 +291,7 @@ func (m *Manager) rollbackClaim(ctx context.Context, sbs []*types.Sandbox) { m.mu.Unlock() m.recommit(ctx, rb) for _, sb := range sbs { - m.disarmEgress(sb.ID, m.removeOrRetry(ctx, sb.VMName, sb.ID)) + m.disarmEgress(sb.ID, m.removeOrRetry(ctx, sb.VMName, sb.ID, "")) } } @@ -370,7 +370,7 @@ func (m *Manager) reapOnce(ctx context.Context) { logger.Errorf(ctx, err, "archive expired %s", v.id) } default: - m.disarmEgress(v.id, m.removeOrRetry(ctx, v.vmName, v.id)) + m.disarmEgress(v.id, m.removeOrRetry(ctx, v.vmName, v.id, "")) m.dropSnap(ctx, v.snap) m.counters.reaps.Add(1) m.recordUsage(ctx, usageEvent{Event: "reap", ID: v.id, VMName: v.vmName}) diff --git a/sandboxd/pool/pool.go b/sandboxd/pool/pool.go index a9d8a74..5a9f259 100644 --- a/sandboxd/pool/pool.go +++ b/sandboxd/pool/pool.go @@ -151,7 +151,6 @@ type Gauges struct { type pendingRemoval struct { sandboxID string tap string - retrying bool } type pool struct { diff --git a/sandboxd/pool/reconcile.go b/sandboxd/pool/reconcile.go index 14511d0..4edc817 100644 --- a/sandboxd/pool/reconcile.go +++ b/sandboxd/pool/reconcile.go @@ -131,11 +131,9 @@ func (m *Manager) sweepStaleVMs(ctx context.Context, live map[string]types.VMRec } gone := make([]bool, len(stale)) // distinct indices: no lock under the Wait barrier m.runBounded(ctx, len(stale), func(ctx context.Context, i int) { - if m.removeOrRetry(ctx, stale[i], "") { + if m.removeOrRetry(ctx, stale[i], "", live[stale[i]].TapDevice()) { gone[i] = true logger.Infof(ctx, "removed stale VM %s", stale[i]) - } else { - m.queueRemoval(stale[i], "", live[stale[i]].TapDevice()) } }).Wait() removed := make(map[string]bool, len(stale)) @@ -194,7 +192,7 @@ func (m *Manager) resyncEgress(ctx context.Context, live map[string]types.VMReco // A failed remove stays out of service and queued until teardown succeeds. func (m *Manager) quarantineClaim(ctx context.Context, sb *types.Sandbox) bool { - gone := m.removeOrRetry(ctx, sb.VMName, sb.ID) + gone := m.removeOrRetry(ctx, sb.VMName, sb.ID, "") m.mu.Lock() delete(m.claimed, sb.ID) m.tenantDelta(sb.Tenant, -1) diff --git a/sandboxd/pool/remove.go b/sandboxd/pool/remove.go index b7c6731..122ebff 100644 --- a/sandboxd/pool/remove.go +++ b/sandboxd/pool/remove.go @@ -3,6 +3,9 @@ package pool import ( "context" "fmt" + "maps" + "slices" + "sync" "github.com/projecteru2/core/log" @@ -10,6 +13,7 @@ import ( ) func (m *Manager) removeVM(ctx context.Context, name string) bool { + // Cancellation-immune: on a canceled ctx `cocoon vm rm` no-ops and orphans. ctx = context.WithoutCancel(ctx) err := m.eng.Remove(ctx, name) if err == nil { @@ -24,6 +28,7 @@ func (m *Manager) confirmGone(ctx context.Context, name string) bool { defer cancel() _, present, err := m.findVM(ctx, name) if err != nil { + // Cannot tell: a false "gone" leaks a running VM, a retry only costs a sweep. log.WithFunc("pool.confirmGone").Warnf(ctx, "verify remove of %s: %v", name, err) return false } @@ -36,66 +41,53 @@ func (m *Manager) confirmGone(ctx context.Context, name string) bool { return true } -func (m *Manager) removeOrRetry(ctx context.Context, name, sandboxID string) bool { +// removeOrRetry reports whether the VM is confirmed gone; a survivor is queued +// for the reap tick to retry, carrying what its cleanup needs (the sandbox ID +// when a claim owns the tap via egressTaps, the tap itself when none does). +func (m *Manager) removeOrRetry(ctx context.Context, name, sandboxID, tap string) bool { if m.removeVM(ctx, name) { return true } - m.queueRemoval(name, sandboxID, "") + m.queueRemoval(name, sandboxID, tap) return false } func (m *Manager) queueRemoval(name, sandboxID, tap string) { m.mu.Lock() - pending := m.pendingRemovals[name] - if pending.sandboxID == "" { - pending.sandboxID = sandboxID - } - if pending.tap == "" { - pending.tap = tap - } - m.pendingRemovals[name] = pending + m.pendingRemovals[name] = pendingRemoval{sandboxID: sandboxID, tap: tap} m.mu.Unlock() } -func (m *Manager) retryRemovals(ctx context.Context) { +// retryRemovals dispatches by emptying the queue, so absence also means "in +// flight" and the next tick cannot double-dispatch; a failed retry re-queues +// itself on completion. Callers that need completion Wait. +func (m *Manager) retryRemovals(ctx context.Context) *sync.WaitGroup { m.mu.Lock() - names := make([]string, 0, len(m.pendingRemovals)) - for name, pending := range m.pendingRemovals { - if pending.retrying { - continue - } - pending.retrying = true - m.pendingRemovals[name] = pending - names = append(names, name) + if len(m.pendingRemovals) == 0 { + m.mu.Unlock() + return new(sync.WaitGroup) } + batch := m.pendingRemovals + m.pendingRemovals = map[string]pendingRemoval{} m.mu.Unlock() - m.runBounded(ctx, len(names), func(ctx context.Context, i int) { - m.retryRemoval(ctx, names[i]) + names := slices.Collect(maps.Keys(batch)) + return m.runBounded(ctx, len(names), func(ctx context.Context, i int) { + m.retryRemoval(ctx, names[i], batch[names[i]]) }) } -func (m *Manager) retryRemoval(ctx context.Context, name string) { - removed := m.removeVM(ctx, name) - m.mu.Lock() - pending, ok := m.pendingRemovals[name] - if !ok { - m.mu.Unlock() +func (m *Manager) retryRemoval(ctx context.Context, name string, pending pendingRemoval) { + if !m.removeVM(ctx, name) { + m.queueRemoval(name, pending.sandboxID, pending.tap) return } - if removed { - delete(m.pendingRemovals, name) - } else { - pending.retrying = false - m.pendingRemovals[name] = pending - } - m.mu.Unlock() - if removed && pending.sandboxID != "" { + if pending.sandboxID != "" { m.disarmEgress(pending.sandboxID, true) - } else if removed && pending.tap != "" { + } else if pending.tap != "" { _ = netfilter.Unlock(pending.tap) } } func (m *Manager) destroy(ctx context.Context, name string) { - m.removeOrRetry(ctx, name, "") + m.removeOrRetry(ctx, name, "", "") } diff --git a/sandboxd/pool/remove_test.go b/sandboxd/pool/remove_test.go index 46d3a2c..7331896 100644 --- a/sandboxd/pool/remove_test.go +++ b/sandboxd/pool/remove_test.go @@ -17,7 +17,7 @@ func TestRemoveVMClassifiesOutcome(t *testing.T) { } delete(eng.vms, "survivor") - if !m.removeOrRetry(t.Context(), "survivor", "") { + if !m.removeOrRetry(t.Context(), "survivor", "", "") { t.Fatal("absent VM reported present") } m.mu.Lock() @@ -48,24 +48,25 @@ func TestRemovalRetryConvergesConfirmedSurvivor(t *testing.T) { t.Fatal("surviving VM was not retained for retry") } - m.retryRemovals(t.Context()) - waitFor(t, func() bool { - m.mu.Lock() - pending := m.pendingRemovals["survivor"] - m.mu.Unlock() - return !pending.retrying - }) + // A failed retry re-queues itself, so the next tick tries again. + m.retryRemovals(t.Context()).Wait() + m.mu.Lock() + _, pending = m.pendingRemovals["survivor"] + m.mu.Unlock() + if !pending { + t.Fatal("failed retry dropped the survivor from the queue") + } eng.mu.Lock() eng.removeErrFor = "" eng.mu.Unlock() - m.retryRemovals(t.Context()) - waitFor(t, func() bool { - m.mu.Lock() - _, pending := m.pendingRemovals["survivor"] - m.mu.Unlock() - return !pending && eng.removed("survivor") - }) + m.retryRemovals(t.Context()).Wait() + m.mu.Lock() + _, pending = m.pendingRemovals["survivor"] + m.mu.Unlock() + if pending || !eng.removed("survivor") { + t.Fatalf("retry did not converge: pending=%v removed=%v", pending, eng.removed("survivor")) + } } func TestRemovalRetryFinishesEgressCleanup(t *testing.T) { @@ -76,7 +77,7 @@ func TestRemovalRetryFinishesEgressCleanup(t *testing.T) { eng.removeErrFor = "survivor" m.egressTaps["sb_survivor"] = "tap-survivor" - removed := m.removeOrRetry(t.Context(), "survivor", "sb_survivor") + removed := m.removeOrRetry(t.Context(), "survivor", "sb_survivor", "") m.disarmEgress("sb_survivor", removed) if removed { t.Fatal("surviving VM reported gone") @@ -85,14 +86,15 @@ func TestRemovalRetryFinishesEgressCleanup(t *testing.T) { eng.mu.Lock() eng.removeErrFor = "" eng.mu.Unlock() - m.retryRemovals(t.Context()) - waitFor(t, func() bool { - m.mu.Lock() - _, pending := m.pendingRemovals["survivor"] - _, locked := m.egressTaps["sb_survivor"] - m.mu.Unlock() - return !pending && !locked && eng.removed("survivor") - }) + m.retryRemovals(t.Context()).Wait() + m.mu.Lock() + _, pending := m.pendingRemovals["survivor"] + _, locked := m.egressTaps["sb_survivor"] + m.mu.Unlock() + if pending || locked || !eng.removed("survivor") { + t.Fatalf("egress cleanup incomplete: pending=%v locked=%v removed=%v", + pending, locked, eng.removed("survivor")) + } } func TestStaleRemovalRetryKeepsTapForCleanup(t *testing.T) { @@ -118,11 +120,11 @@ func TestStaleRemovalRetryKeepsTapForCleanup(t *testing.T) { eng.mu.Lock() eng.removeErrFor = "" eng.mu.Unlock() - m.retryRemovals(t.Context()) - waitFor(t, func() bool { - m.mu.Lock() - _, pending := m.pendingRemovals["sbx-stale"] - m.mu.Unlock() - return !pending && eng.removed("sbx-stale") - }) + m.retryRemovals(t.Context()).Wait() + m.mu.Lock() + _, pending := m.pendingRemovals["sbx-stale"] + m.mu.Unlock() + if pending || !eng.removed("sbx-stale") { + t.Fatalf("stale retry did not converge: pending=%v removed=%v", pending, eng.removed("sbx-stale")) + } } diff --git a/sandboxd/server/server.go b/sandboxd/server/server.go index 1450279..bd81e45 100644 --- a/sandboxd/server/server.go +++ b/sandboxd/server/server.go @@ -120,9 +120,8 @@ type InfoResponse struct { Archived int `json:"archived"` Draining bool `json:"draining,omitempty"` Peers []string `json:"peers,omitempty"` - // AtCapacity reports that the node cannot attach another VM and has parked - // refill. A placer should read a short warm count on such a node as "full", - // not "still filling", and send the work somewhere else. + // AtCapacity marks refill parked because the node refused another VM, so a + // placer reads a short warm count as "full", not "still filling". AtCapacity bool `json:"at_capacity,omitempty"` AtCapacityReason string `json:"at_capacity_reason,omitempty"` }