From 75f5b12fccbd74179b1cc500349161eb059368ff Mon Sep 17 00:00:00 2001 From: Ashkan Nami Date: Sun, 21 Jun 2026 10:56:51 +0000 Subject: [PATCH 1/2] feat(agent): release a stopped server's host capacity and allow reschedule A stopped server used to keep its full slot: the host runtime counted its CPU/memory toward capacity, and the scheduler kept its reservation, so a stopped server tied up resources it was not using and was pinned to its original host. Free that capacity and let a stopped server reschedule onto any host when it next starts: - Host runtime no longer counts non-running VMs (checkCapacity/fitsLocked); Start re-checks capacity since a resumed VM's slot may have been taken. - New `evict` op across the agent contract (Runtime -> firecracker/fake -> link -> hub -> Commander -> RemoteProvisioner -> Provisioner). Firecracker's Deprovision and Evict share teardown(deleteWorld): Evict drops the host-local VM + disk but keeps the durable world snapshot; Deprovision also deletes it. - reconciler.stop: snapshot world -> evict VM -> scheduler.Release -> mark stopped. MarkStopped now clears host_id, so UsedCapacity stops counting the server and its next start re-runs placement (world restored from the durable store, keyed by server id). Tradeoff: on a host with world persistence but no durable store, the world is host-local, so a reschedule to a different host starts from an empty world. Documented inline; capacity is freed on stop regardless. --- docs/PLAN.md | 13 ++-- internal/agent/agent_test.go | 51 +++++++++++--- internal/agent/firecracker/runtime.go | 50 ++++++++++---- .../agent/firecracker/worldsnapshot_test.go | 66 +++++++++++++++++++ internal/agent/link.go | 3 + internal/agent/runtime.go | 56 +++++++++++++--- internal/agentlink/hub.go | 9 ++- internal/model/game_server.go | 5 +- internal/provisioner/provisioner.go | 11 +++- internal/provisioner/remote.go | 11 ++++ internal/provisioner/remote_test.go | 17 +++++ internal/reconciler/reconciler.go | 27 ++++++-- internal/repository/game_server.go | 17 +++-- 13 files changed, 285 insertions(+), 51 deletions(-) diff --git a/docs/PLAN.md b/docs/PLAN.md index 342ea2e..d5c89e2 100644 --- a/docs/PLAN.md +++ b/docs/PLAN.md @@ -236,11 +236,14 @@ it is feature-complete for IPv4 TCP/UDP.)* - **Retry/backoff:** today a failed reconcile sets `status=error` and just relies on the 2s tick; replace with `attempts` + `next_attempt_at` (exponential backoff) and have `ListReconcilable` respect it. -- **Host-failure reschedule:** the host reaper marks a host `down` but **nothing - reschedules its servers** — `host_id` is only cleared on delete. Add: on - sustained host-down, clear `host_id`/`vm_id` and re-place, **with fencing** - (generation token / ensure the old VM is gone) to avoid split-brain. P5's - store-mediated reschedule is the data half; this is the control half. +- **Host-failure reschedule:** a *stop* now releases a server's host (clears + `host_id`, returns the reservation) so its next *start* re-places it on any + host — voluntary reschedule works. But the host reaper marks a dead host + `down` while **nothing reschedules the servers still desired-running on it**: + that involuntary path is the gap. Add: on sustained host-down, clear + `host_id`/`vm_id` and re-place, **with fencing** (generation token / ensure the + old VM is gone) to avoid split-brain. P5's store-mediated reschedule is the + data half; this is the control half. - **Draining:** `model.HostDraining` exists but is never entered or honored — wire a drain that blocks new placement and migrates servers off. - Optional leader election (advisory lock/lease) for multiple control-plane diff --git a/internal/agent/agent_test.go b/internal/agent/agent_test.go index 46a7098..adb0514 100644 --- a/internal/agent/agent_test.go +++ b/internal/agent/agent_test.go @@ -47,9 +47,39 @@ func TestFakeRuntimeLifecycle(t *testing.T) { assertState(t, rt, vm.ID, StateMissing) } +// TestFakeRuntimeEvict verifies Evict tears a VM down like Deprovision for the +// fake (which holds no durable world), freeing its capacity and host port. +func TestFakeRuntimeEvict(t *testing.T) { + ctx := context.Background() + rt := NewFakeRuntime("10.0.0.7", WithCapacity(4, 8192)) + + vm, err := rt.Provision(ctx, VMSpec{ServerID: "a", CPUs: 4, MemoryMB: 8192}) + if err != nil { + t.Fatalf("provision: %v", err) + } + if err := rt.Evict(ctx, vm.ID); err != nil { + t.Fatalf("evict: %v", err) + } + assertState(t, rt, vm.ID, StateMissing) + + // Capacity and the host port are freed: a fresh VM fits and reuses the port. + next, err := rt.Provision(ctx, VMSpec{ServerID: "b", CPUs: 4, MemoryMB: 8192}) + if err != nil { + t.Fatalf("provision after evict should fit: %v", err) + } + if next.Port != vm.Port { + t.Errorf("port after evict = %d, want freed port %d reused", next.Port, vm.Port) + } + // Evicting an unknown VM is a no-op. + if err := rt.Evict(ctx, "ghost"); err != nil { + t.Errorf("evict unknown = %v, want nil (idempotent)", err) + } +} + // TestFakeRuntimeRefusesOvercommit verifies a capacity-bounded host agent will // not boot a VM that would exceed its total: two 4/8192 servers cannot both run -// on a single 4/8192 host. Deprovisioning the first frees the slot for another. +// on a single 4/8192 host. Stopping the first frees its slot for another, and +// restarting it back over that taken slot is itself refused. func TestFakeRuntimeRefusesOvercommit(t *testing.T) { ctx := context.Background() rt := NewFakeRuntime("10.0.0.7", WithCapacity(4, 8192)) @@ -61,19 +91,24 @@ func TestFakeRuntimeRefusesOvercommit(t *testing.T) { if _, err := rt.Provision(ctx, VMSpec{ServerID: "b", CPUs: 4, MemoryMB: 8192}); !errors.Is(err, ErrInsufficientCapacity) { t.Fatalf("second provision err = %v, want ErrInsufficientCapacity", err) } - // A stopped VM keeps its slot, so it still cannot fit a second. + // A stopped VM frees its slot, so a second now fits. if err := rt.Stop(ctx, first.ID); err != nil { t.Fatalf("stop: %v", err) } - if _, err := rt.Provision(ctx, VMSpec{ServerID: "b", CPUs: 4, MemoryMB: 8192}); !errors.Is(err, ErrInsufficientCapacity) { - t.Fatalf("provision over a stopped VM err = %v, want ErrInsufficientCapacity", err) + second, err := rt.Provision(ctx, VMSpec{ServerID: "b", CPUs: 4, MemoryMB: 8192}) + if err != nil { + t.Fatalf("provision over a stopped VM should fit: %v", err) + } + // Restarting the first back over the now-taken slot is refused. + if _, err := rt.Start(ctx, first.ID); !errors.Is(err, ErrInsufficientCapacity) { + t.Fatalf("restart over a taken slot err = %v, want ErrInsufficientCapacity", err) } - // Deprovisioning frees the capacity. - if err := rt.Deprovision(ctx, first.ID); err != nil { + // Freeing the second's slot lets the first restart. + if err := rt.Deprovision(ctx, second.ID); err != nil { t.Fatalf("deprovision: %v", err) } - if _, err := rt.Provision(ctx, VMSpec{ServerID: "b", CPUs: 4, MemoryMB: 8192}); err != nil { - t.Fatalf("provision after deprovision should fit: %v", err) + if _, err := rt.Start(ctx, first.ID); err != nil { + t.Fatalf("restart after freeing capacity should fit: %v", err) } } diff --git a/internal/agent/firecracker/runtime.go b/internal/agent/firecracker/runtime.go index ac63752..91970f7 100644 --- a/internal/agent/firecracker/runtime.go +++ b/internal/agent/firecracker/runtime.go @@ -298,15 +298,19 @@ func (r *Runtime) Provision(ctx context.Context, spec agent.VMSpec) (*agent.VM, } // checkCapacity reports whether a VM needing cpus/memMB fits the host's -// remaining capacity, returning agent.ErrInsufficientCapacity if not. A live VM -// holds its slot whether running or stopped, so every tracked VM counts. A zero -// configured total leaves that dimension unconstrained. +// remaining capacity, returning agent.ErrInsufficientCapacity if not. Only a +// running VM holds its slot — a stopped VM's process is gone, freeing its +// cpu/memory for other work, at the cost that restarting it may then fail. A +// zero configured total leaves that dimension unconstrained. func (r *Runtime) checkCapacity(cpus, memMB int) error { r.mu.Lock() defer r.mu.Unlock() var usedCPUs, usedMem int for _, m := range r.vms { + if !m.running() { + continue + } usedCPUs += m.vcpus usedMem += m.memoryMB } @@ -331,6 +335,12 @@ func (r *Runtime) Start(ctx context.Context, vmID string) (*agent.VM, error) { if m.running() { return r.vmView(m), nil } + // A stopped VM gave up its capacity slot, so restarting it must clear the + // host backstop again — the cpu/memory it needs may have been handed to + // another VM while it was down. + if err := r.checkCapacity(m.vcpus, m.memoryMB); err != nil { + return nil, err + } if err := m.boot(ctx); err != nil { return nil, fmt.Errorf("firecracker: restart vm: %w", err) } @@ -360,8 +370,25 @@ func (r *Runtime) Stop(ctx context.Context, vmID string) error { return nil } -// Deprovision force-stops a VM and removes its working directory (idempotent). +// Evict force-stops a VM and removes its host-local footprint — working dir and +// local world disk — but keeps the durable world snapshot, so the server can be +// rescheduled onto another host and restore its world from the store there. It +// backs releasing a stopped server from its host (idempotent). +func (r *Runtime) Evict(_ context.Context, vmID string) error { + return r.teardown(vmID, false) +} + +// Deprovision force-stops a VM and removes it entirely, including its durable +// world snapshot — it is a server delete (idempotent). func (r *Runtime) Deprovision(_ context.Context, vmID string) error { + return r.teardown(vmID, true) +} + +// teardown force-stops a VM and removes its host-local footprint. When +// deleteWorld is set it also deletes the durable world snapshot (a server +// delete); otherwise the durable copy is preserved so the world can be restored +// after a reschedule. An unknown VM is a no-op. +func (r *Runtime) teardown(vmID string, deleteWorld bool) error { r.mu.Lock() m, ok := r.vms[vmID] if ok { @@ -383,18 +410,17 @@ func (r *Runtime) Deprovision(_ context.Context, vmID string) error { } _ = deleteTAP(m.tapName) } - // Destroy the world disk along with the VM. This is the point P5b will - // snapshot-then-upload before removing, so a deprovision becomes a safe - // teardown rather than data loss; for now destroy means destroy. Removing - // the keyed parent dir (DataDir/) takes the disk with it. + // Remove the host-local world disk. Removing the keyed parent dir + // (DataDir/) takes the disk with it. The durable copy in the store is + // the system of record across hosts; we delete it only on a true server + // delete, keeping it for a reschedule. if m.worldDisk != "" { if err := os.RemoveAll(filepath.Dir(m.worldDisk)); err != nil { return fmt.Errorf("firecracker: remove world disk: %w", err) } - // Delete the durable copy too: deprovision is a server delete, so the - // world is meant to be gone. Best-effort — an orphaned blob is harmless - // (a later GC can sweep it) and must not block teardown. - if r.store != nil { + if deleteWorld && r.store != nil { + // Best-effort — an orphaned blob is harmless (a later GC can sweep + // it) and must not block teardown. _ = r.store.Delete(context.Background(), m.worldKey) } } diff --git a/internal/agent/firecracker/worldsnapshot_test.go b/internal/agent/firecracker/worldsnapshot_test.go index 0ac5189..3c3e2ad 100644 --- a/internal/agent/firecracker/worldsnapshot_test.go +++ b/internal/agent/firecracker/worldsnapshot_test.go @@ -137,3 +137,69 @@ func TestPrepareWorldDiskFreshWhenStoreEmpty(t *testing.T) { t.Fatal("expected fresh-format fallback to invoke mkfs and fail") } } + +// TestEvictPreservesDurableWorld verifies the teardown split: Evict removes a +// VM's host-local footprint but keeps the durable world snapshot (so the server +// can restore it after a reschedule), while Deprovision deletes the durable copy +// too (a true server delete). +func TestEvictPreservesDurableWorld(t *testing.T) { + ctx := context.Background() + store, err := storage.NewDirStore(t.TempDir()) + if err != nil { + t.Fatal(err) + } + rt := newTestRuntime(t) + rt.store = store + + // seed builds a durable snapshot for key and a tracked machine with a local + // world disk, returning the machine. + seed := func(id, key string) *machine { + if err := store.Put(ctx, key, bytes.NewReader([]byte("world-"+key))); err != nil { + t.Fatalf("seed durable %s: %v", key, err) + } + base := t.TempDir() + vmDir := filepath.Join(base, id) + worldDir := filepath.Join(base, "worlds", key) + for _, d := range []string{vmDir, worldDir} { + if err := os.MkdirAll(d, 0o750); err != nil { + t.Fatal(err) + } + } + disk := filepath.Join(worldDir, "world.ext4") + if err := os.WriteFile(disk, []byte("local-"+key), 0o640); err != nil { + t.Fatal(err) + } + m := &machine{id: id, serverID: key, dir: vmDir, worldDisk: disk, worldKey: key} + rt.vms[id] = m + return m + } + + gone := func(path string) bool { + _, err := os.Stat(path) + return os.IsNotExist(err) + } + + // Evict: durable world survives, host-local footprint is gone. + mE := seed("vm-evict", "srv-evict") + if err := rt.Evict(ctx, mE.id); err != nil { + t.Fatalf("evict: %v", err) + } + if ok, _ := store.Exists(ctx, "srv-evict"); !ok { + t.Error("evict deleted the durable world; want preserved") + } + if !gone(mE.dir) || !gone(filepath.Dir(mE.worldDisk)) { + t.Error("evict left host-local footprint behind") + } + if _, ok := rt.vms[mE.id]; ok { + t.Error("evict left the machine tracked") + } + + // Deprovision: durable world is deleted too. + mD := seed("vm-deprov", "srv-deprov") + if err := rt.Deprovision(ctx, mD.id); err != nil { + t.Fatalf("deprovision: %v", err) + } + if ok, _ := store.Exists(ctx, "srv-deprov"); ok { + t.Error("deprovision kept the durable world; want deleted") + } +} diff --git a/internal/agent/link.go b/internal/agent/link.go index fe00df3..4e0abf8 100644 --- a/internal/agent/link.go +++ b/internal/agent/link.go @@ -21,6 +21,7 @@ const ( OpStart = "start" OpStop = "stop" OpSnapshot = "snapshot" + OpEvict = "evict" OpDeprovision = "deprovision" OpStatus = "status" ) @@ -168,6 +169,8 @@ func execOp(ctx context.Context, rt Runtime, cmd *pb.Command) (payload []byte, e return nil, errString(rt.Stop(ctx, vmRef(cmd))) case OpSnapshot: return nil, errString(rt.Snapshot(ctx, vmRef(cmd))) + case OpEvict: + return nil, errString(rt.Evict(ctx, vmRef(cmd))) case OpDeprovision: return nil, errString(rt.Deprovision(ctx, vmRef(cmd))) case OpStatus: diff --git a/internal/agent/runtime.go b/internal/agent/runtime.go index 595534a..f72590a 100644 --- a/internal/agent/runtime.go +++ b/internal/agent/runtime.go @@ -87,7 +87,15 @@ type Runtime interface { Start(ctx context.Context, vmID string) (*VM, error) // Stop halts a VM without destroying it. Idempotent (missing VM is success). Stop(ctx context.Context, vmID string) error - // Deprovision destroys a VM. Idempotent (missing VM is success). + // Evict destroys a VM and its host-local footprint (working dir, local world + // disk) while preserving the durable world snapshot, so the server can be + // rescheduled onto another host and restore its world there. It is the + // teardown half of releasing a stopped server from its host; Deprovision is + // the stronger form that also deletes the durable world. Idempotent (missing + // VM is success). + Evict(ctx context.Context, vmID string) error + // Deprovision destroys a VM, including its durable world. Idempotent (missing + // VM is success). Deprovision(ctx context.Context, vmID string) error // Status reports a VM's observed state, returning StateMissing for an // unknown id rather than an error. @@ -117,9 +125,10 @@ type FakeRuntime struct { usedPorts map[int]struct{} // host ports currently bound by a live VM } -// fakeVM is a simulated VM plus the host resources it holds. A provisioned VM -// occupies its cpu/memory until it is deprovisioned, even while stopped — -// mirroring a real host, where a halted VM keeps its slot. +// fakeVM is a simulated VM plus the host resources it holds. A running VM +// occupies its cpu/memory; stopping it frees that slot back to the host — +// mirroring a real host that reclaims a halted VM's resources — so only running +// VMs count against capacity. type fakeVM struct { vm *VM cpus int @@ -187,11 +196,16 @@ func (r *FakeRuntime) allocatePortLocked() int { } // fitsLocked reports whether a VM needing cpus/memMB fits the host's remaining -// capacity, returning ErrInsufficientCapacity if not. A zero total leaves that +// capacity, returning ErrInsufficientCapacity if not. Only running VMs hold a +// slot — a stopped VM frees its cpu/memory so the host can pack other work into +// it, at the cost that restarting it may then fail. A zero total leaves that // dimension unconstrained. Caller holds r.mu. func (r *FakeRuntime) fitsLocked(cpus, memMB int) error { var usedCPUs, usedMem int for _, fv := range r.vms { + if fv.vm.State != StateRunning { + continue + } usedCPUs += fv.cpus usedMem += fv.memMB } @@ -204,7 +218,10 @@ func (r *FakeRuntime) fitsLocked(cpus, memMB int) error { return nil } -// Start boots an existing VM back to running. +// Start boots an existing VM back to running, refusing to overcommit. Because a +// stopped VM gave up its slot, the cpu/memory it needs may have been handed to +// another VM while it was down, so a restart can fail with +// ErrInsufficientCapacity. Restarting an already-running VM is a no-op. func (r *FakeRuntime) Start(_ context.Context, vmID string) (*VM, error) { r.mu.Lock() defer r.mu.Unlock() @@ -213,7 +230,12 @@ func (r *FakeRuntime) Start(_ context.Context, vmID string) (*VM, error) { if !ok { return nil, ErrVMNotFound } - fv.vm.State = StateRunning + if fv.vm.State != StateRunning { + if err := r.fitsLocked(fv.cpus, fv.memMB); err != nil { + return nil, err + } + fv.vm.State = StateRunning + } return clone(fv.vm), nil } @@ -228,17 +250,35 @@ func (r *FakeRuntime) Stop(_ context.Context, vmID string) error { return nil } +// Evict destroys a VM, freeing its capacity and host port. The fake keeps no +// durable world, so it is indistinguishable from Deprovision here; the +// distinction (preserving the durable snapshot) only matters for the real +// runtime. Unknown VM is a no-op. +func (r *FakeRuntime) Evict(_ context.Context, vmID string) error { + r.mu.Lock() + defer r.mu.Unlock() + + r.removeLocked(vmID) + return nil +} + // Deprovision destroys a VM, freeing its capacity and host port. Unknown VM is // a no-op. func (r *FakeRuntime) Deprovision(_ context.Context, vmID string) error { r.mu.Lock() defer r.mu.Unlock() + r.removeLocked(vmID) + return nil +} + +// removeLocked drops a VM from the inventory and releases its host port. Caller +// holds r.mu; an unknown VM is a no-op. +func (r *FakeRuntime) removeLocked(vmID string) { if fv, ok := r.vms[vmID]; ok { delete(r.usedPorts, fv.vm.Port) delete(r.vms, vmID) } - return nil } // Status reports a VM's state, or a missing VM for an unknown id. diff --git a/internal/agentlink/hub.go b/internal/agentlink/hub.go index 3b72b84..d42a693 100644 --- a/internal/agentlink/hub.go +++ b/internal/agentlink/hub.go @@ -222,7 +222,14 @@ func (h *Hub) Snapshot(ctx context.Context, hostID, vmID string) error { return err } -// Deprovision asks the host's agent to destroy a VM. +// Evict asks the host's agent to tear down a VM while preserving its durable +// world, releasing the host so the server can be rescheduled elsewhere. +func (h *Hub) Evict(ctx context.Context, hostID, vmID string) error { + _, err := h.call(ctx, hostID, agent.OpEvict, agent.VMRef{VMID: vmID}) + return err +} + +// Deprovision asks the host's agent to destroy a VM, including its durable world. func (h *Hub) Deprovision(ctx context.Context, hostID, vmID string) error { _, err := h.call(ctx, hostID, agent.OpDeprovision, agent.VMRef{VMID: vmID}) return err diff --git a/internal/model/game_server.go b/internal/model/game_server.go index 81db154..2d52d8a 100644 --- a/internal/model/game_server.go +++ b/internal/model/game_server.go @@ -54,8 +54,9 @@ type GameServer struct { Env map[string]string `json:"env,omitempty"` // HostID is the fleet host the scheduler placed this server on (P2). It is - // set before provisioning and persists across stop/start (the VM stays put); - // it is cleared only on delete. Nil until placed. + // set before provisioning and cleared on stop and on delete — a stopped + // server holds no host, so its next start re-runs placement and may land on a + // different host. Nil until placed. HostID *string `json:"host_id,omitempty"` // Runtime details, populated once provisioned. diff --git a/internal/provisioner/provisioner.go b/internal/provisioner/provisioner.go index 8d54d9c..ecdc418 100644 --- a/internal/provisioner/provisioner.go +++ b/internal/provisioner/provisioner.go @@ -43,7 +43,12 @@ type Provisioner interface { Start(ctx context.Context, s *model.GameServer) (*Instance, error) // Stop halts the backing VM without destroying it. It must be idempotent. Stop(ctx context.Context, s *model.GameServer) error - // Deprovision tears down the backing VM. It must be idempotent. + // Evict releases the backing VM from its host while preserving the durable + // world, so the server can be rescheduled onto another host on its next + // start. It must be idempotent. + Evict(ctx context.Context, s *model.GameServer) error + // Deprovision tears down the backing VM, including its durable world. It must + // be idempotent. Deprovision(ctx context.Context, s *model.GameServer) error // Status reports the observed state of the backing VM. Status(ctx context.Context, s *model.GameServer) (State, error) @@ -91,6 +96,10 @@ func (f Fake) Start(ctx context.Context, s *model.GameServer) (*Instance, error) // Stop is a no-op for the fake backend; the VM is considered halted but kept. func (Fake) Stop(_ context.Context, _ *model.GameServer) error { return nil } +// Evict is a no-op for the fake backend; there is no host-local footprint to +// release. +func (Fake) Evict(_ context.Context, _ *model.GameServer) error { return nil } + // Deprovision is a no-op for the fake backend. func (Fake) Deprovision(_ context.Context, _ *model.GameServer) error { return nil } diff --git a/internal/provisioner/remote.go b/internal/provisioner/remote.go index 82f35fb..ce2f4ad 100644 --- a/internal/provisioner/remote.go +++ b/internal/provisioner/remote.go @@ -24,6 +24,7 @@ type Commander interface { Start(ctx context.Context, hostID, vmID string) (*agent.VM, error) Stop(ctx context.Context, hostID, vmID string) error Snapshot(ctx context.Context, hostID, vmID string) error + Evict(ctx context.Context, hostID, vmID string) error Deprovision(ctx context.Context, hostID, vmID string) error Status(ctx context.Context, hostID, vmID string) (*agent.VM, error) } @@ -101,6 +102,16 @@ func (p *RemoteProvisioner) Stop(ctx context.Context, s *model.GameServer) error return p.cmd.Stop(ctx, hostID, *s.VMID) } +// Evict releases the VM from its host while preserving the durable world, so the +// server can be rescheduled elsewhere on its next start (idempotent). A server +// with no backing VM has nothing to evict. +func (p *RemoteProvisioner) Evict(ctx context.Context, s *model.GameServer) error { + if s.HostID == nil || *s.HostID == "" || s.VMID == nil || *s.VMID == "" { + return nil + } + return p.cmd.Evict(ctx, *s.HostID, *s.VMID) +} + // Deprovision tears down the VM on its host (idempotent). A server that was // never placed or provisioned has nothing to tear down. func (p *RemoteProvisioner) Deprovision(ctx context.Context, s *model.GameServer) error { diff --git a/internal/provisioner/remote_test.go b/internal/provisioner/remote_test.go index 2d8c97b..4998232 100644 --- a/internal/provisioner/remote_test.go +++ b/internal/provisioner/remote_test.go @@ -26,6 +26,9 @@ func (c fakeCommander) Stop(ctx context.Context, _, vmID string) error { func (c fakeCommander) Snapshot(ctx context.Context, _, vmID string) error { return c.rt.Snapshot(ctx, vmID) } +func (c fakeCommander) Evict(ctx context.Context, _, vmID string) error { + return c.rt.Evict(ctx, vmID) +} func (c fakeCommander) Deprovision(ctx context.Context, _, vmID string) error { return c.rt.Deprovision(ctx, vmID) } @@ -57,6 +60,11 @@ func (c errCommander) Snapshot(context.Context, string, string) error { c.t.Fatal("Snapshot called, want no-op") return nil } +func (c errCommander) Evict(context.Context, string, string) error { + c.t.Helper() + c.t.Fatal("Evict called, want no-op") + return nil +} func (c errCommander) Deprovision(context.Context, string, string) error { c.t.Helper() c.t.Fatal("Deprovision called, want no-op") @@ -106,6 +114,12 @@ func TestRemoteProvisionerLifecycle(t *testing.T) { } assertRemoteState(t, p, s, StateRunning) + // Evict releases the VM from its host; the agent reports it gone. + if err := p.Evict(ctx, s); err != nil { + t.Fatalf("evict: %v", err) + } + assertRemoteState(t, p, s, StateMissing) + if err := p.Deprovision(ctx, s); err != nil { t.Fatalf("deprovision: %v", err) } @@ -126,6 +140,9 @@ func TestRemoteProvisionerUnplaced(t *testing.T) { if err := p.Deprovision(ctx, &model.GameServer{ID: "x"}); err != nil { t.Errorf("deprovision unplaced = %v, want nil", err) } + if err := p.Evict(ctx, &model.GameServer{ID: "x"}); err != nil { + t.Errorf("evict unplaced = %v, want nil", err) + } if err := p.Stop(ctx, &model.GameServer{ID: "x", HostID: ptr("h")}); err != nil { t.Errorf("stop with no vm = %v, want nil", err) } diff --git a/internal/reconciler/reconciler.go b/internal/reconciler/reconciler.go index 0c4d16f..1d79f85 100644 --- a/internal/reconciler/reconciler.go +++ b/internal/reconciler/reconciler.go @@ -115,12 +115,13 @@ func (r *Reconciler) start(ctx context.Context, s *model.GameServer) error { if s.Status == model.StatusRunning { return nil } - // A server that already has a VM was provisioned before and merely stopped; - // resume it rather than creating a fresh one. + // A server that still has a VM (e.g. an interrupted provision, retried before + // it was torn down) is resumed in place rather than rebuilt from scratch. provisioned := s.VMID != nil && *s.VMID != "" - // Place a not-yet-provisioned, unassigned server on a host before booting a - // new VM. A resumed VM already lives on its host; an already-assigned server - // keeps its reservation across a failed attempt, so neither re-schedules. + // Place an unassigned server on a host before booting a VM. A stopped server + // was unplaced on stop, so it re-schedules here and may land on a new host + // (its world rides the durable store). A mid-flight server that still has a + // VM or a host assignment keeps it across a retry, so neither re-schedules. if !provisioned && s.HostID == nil { if err := r.place(ctx, s); err != nil { return err @@ -181,11 +182,23 @@ func (r *Reconciler) stop(ctx context.Context, s *model.GameServer) error { if err := r.servers.MarkStatus(ctx, s.ID, model.StatusStopping, ""); err != nil { return err } - // Halt the VM but keep it; the world and runtime details survive for a later - // start. Destruction only happens on delete. + // Halt the VM, snapshotting its world to the durable store. if err := r.prov.Stop(ctx, s); err != nil { return err } + // Release the host: evict the VM's host-local footprint (keeping the durable + // world) and return its reservation, so a stopped server ties up no capacity + // and is free to reschedule onto any host when it next starts. The world + // rides the durable store, keyed by server id, so the restart restores it + // wherever it lands. NOTE: on a host with world persistence but no durable + // store, the world is host-local only, so a reschedule to a different host + // starts from an empty world — an accepted tradeoff for freeing capacity. + if err := r.prov.Evict(ctx, s); err != nil { + return err + } + if s.HostID != nil { + _ = r.sched.Release(ctx, *s.HostID, s.CPUs, s.MemoryMB) + } r.log.Info("server stopped", zap.String("id", s.ID)) return r.servers.MarkStopped(ctx, s.ID) } diff --git a/internal/repository/game_server.go b/internal/repository/game_server.go index 2e516e8..7f5372e 100644 --- a/internal/repository/game_server.go +++ b/internal/repository/game_server.go @@ -173,11 +173,11 @@ func (r *GameServerRepository) SetDesiredState(ctx context.Context, id, desired } // UsedCapacity returns the total cpu and memory currently committed to a host: -// the sum over every live (non-deleted) server assigned to it. A server keeps -// its reservation while stopped (the VM stays put), so this counts all assigned -// servers regardless of status. It lets the control plane rebuild a host's -// allocatable capacity from the durable record after a restart, instead of -// resetting it to total and forgetting in-flight placements. +// the sum over every live (non-deleted) server assigned to it. A stopped server +// has its host_id cleared (see MarkStopped), so it drops out of this sum and +// frees its reservation — only placed servers count. It lets the control plane +// rebuild a host's allocatable capacity from the durable record after a restart, +// instead of resetting it to total and forgetting in-flight placements. func (r *GameServerRepository) UsedCapacity(ctx context.Context, hostID string) (cpus, memoryMB int, err error) { if hostID == "" { return 0, 0, nil @@ -219,12 +219,15 @@ func (r *GameServerRepository) MarkRunning(ctx context.Context, id, vmID, host s return err } -// MarkStopped records a stopped server with its runtime details cleared. +// MarkStopped records a stopped server with its runtime details and host +// assignment cleared. Clearing host_id releases the server's place in the fleet: +// it no longer counts against its old host's capacity (see UsedCapacity), and +// its next start re-runs placement, so a stopped server can resume on any host. func (r *GameServerRepository) MarkStopped(ctx context.Context, id string) error { _, err := r.pool.Exec(ctx, ` UPDATE game_servers SET status = 'stopped', vm_id = NULL, host = NULL, port = NULL, - status_message = NULL, updated_at = now() + host_id = NULL, status_message = NULL, updated_at = now() WHERE id = $1`, id) return err From 64d191a6226ce8a528ab5ffb67d2ff58a5de6094 Mon Sep 17 00:00:00 2001 From: Ashkan Nami Date: Sun, 21 Jun 2026 11:01:31 +0000 Subject: [PATCH 2/2] test(e2e): assert a valid pool port, not exactly 25565 TestGameServerLifecycle asserted the running server's port was exactly 25565, which only holds when it is the first VM on the shared suite host. The fake runtime hands out the lowest free host port at or above the standard Minecraft port, so when other tests' VMs hold the low ports this server gets the next one up (e.g. 25570) and the assertion flakes depending on suite ordering. Assert it is a valid pool port (>= 25565) instead. --- test/e2e/servers_test.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/test/e2e/servers_test.go b/test/e2e/servers_test.go index 51b4fa6..f0d8efd 100644 --- a/test/e2e/servers_test.go +++ b/test/e2e/servers_test.go @@ -174,8 +174,12 @@ func TestGameServerLifecycle(t *testing.T) { if running["host"] == nil || running["port"] == nil || running["vm_id"] == nil { t.Errorf("running server missing runtime details: %v", running) } - if running["port"].(float64) != 25565 { - t.Errorf("port = %v, want 25565", running["port"]) + // The fake runtime hands each VM the lowest free host port at or above the + // standard Minecraft port, so a server sharing the suite's host gets 25565 or + // the next free port up — assert it is a valid pool port, not an exact value + // that depends on how many other VMs are live when this test runs. + if p, ok := running["port"].(float64); !ok || p < 25565 { + t.Errorf("port = %v, want a host port >= 25565", running["port"]) } // Stop.