diff --git a/spinifex/daemon/daemon.go b/spinifex/daemon/daemon.go index 28554f7cb..3e3fba6f9 100644 --- a/spinifex/daemon/daemon.go +++ b/spinifex/daemon/daemon.go @@ -1625,6 +1625,9 @@ func (d *Daemon) startCluster() error { if eniRec := d.newENIReconciler(); eniRec != nil { reapers = append(reapers, eniRec) } + if gpuRec := d.newGPUPoolReconciler(); gpuRec != nil { + reapers = append(reapers, gpuRec) + } if d.volumeService != nil { reapers = append(reapers, d.volumeService.NewVolumeLeakReaper(d.leakedVolumeInstances)) } @@ -2254,18 +2257,26 @@ func (rm *ResourceManager) canAllocate(instanceType *ec2.InstanceTypeInfo, count // canAllocateLocked is the lock-free body of canAllocate. Caller must already // hold rm.mu for read or write. Extracted so allocate can re-check capacity // while holding the write lock without dropping it. +// +// GPU instance types (whole-GPU and MIG alike) are gated on BOTH the cpu/mem +// calc AND GPU-slot availability. cpu/mem still applies to whole-GPU types — +// the guest still consumes real host vCPU/RAM even though a GPU backs it — +// and GPU-slot count is an additional hard constraint layered on top, so +// admission never advertises more instances than there are GPU slots to +// back them. func (rm *ResourceManager) canAllocateLocked(instanceType *ec2.InstanceTypeInfo, count int) int { - // Whole-GPU passthrough: gpuManager.Claim is the sole gate — one GPU per - // slot, so host CPU/memory is never the binding constraint. - // MIG types fall through to the normal check: one physical GPU backs many - // concurrent slices, each consuming real host CPU and memory, so those - // resources must be tracked and enforced like any non-GPU instance type. instanceTypeName := "" if instanceType.InstanceType != nil { instanceTypeName = *instanceType.InstanceType } - if instancetypes.IsGPUType(instanceType) && !instancetypes.IsMIGType(instanceTypeName) { - return count + + requiresGPU := instancetypes.IsGPUType(instanceType) + availGPU := 0 + if requiresGPU && rm.gpuManager != nil { + gpusNeeded := instancetypes.GPUCountForType(instanceTypeName) + if gpusNeeded > 0 { + availGPU = rm.gpuManager.Available() / gpusNeeded + } } n := canAllocateCount( @@ -2274,7 +2285,7 @@ func (rm *ResourceManager) canAllocateLocked(instanceType *ec2.InstanceTypeInfo, instanceTypeVCPUs(instanceType), rm.instanceMemChargeMiB(instanceType), count, - 0, false, + availGPU, requiresGPU, ) return rm.liveMemGate(n, instanceType) } diff --git a/spinifex/daemon/daemon_test.go b/spinifex/daemon/daemon_test.go index 5807cb0b6..df00259c4 100644 --- a/spinifex/daemon/daemon_test.go +++ b/spinifex/daemon/daemon_test.go @@ -2986,16 +2986,47 @@ func TestGetAvailableInstanceTypeInfos_GPUType_ShowCapacity(t *testing.T) { assert.Equal(t, 2, count12xl, "4 GPUs → 2 slots for g7e.12xlarge") } -// canAllocate for GPU types returns count without CPU/memory gating. -func TestCanAllocate_GPUType_BypassesCPUMemory(t *testing.T) { +// canAllocate for GPU types is gated by CPU/memory like any other instance +// type. Regression for mulga-jeo02: the whole-GPU early return used to +// bypass this check entirely, letting a launch proceed on a host with no +// memory headroom. +func TestCanAllocate_GPUType_GatedByCPUMemory(t *testing.T) { rm := &ResourceManager{ hostVCPU: 4, // deliberately tiny hostMemGB: 8.0, // deliberately tiny instanceTypes: map[string]*ec2.InstanceTypeInfo{}, + gpuManager: gpu.NewManager([]gpu.GPUDevice{makeGPUDevice()}), } gpuType := makeGPUInstanceType("g7e.4xlarge", 16, 128*1024) - assert.Equal(t, 1, rm.canAllocate(gpuType, 1), - "GPU type must be allocatable even when CPU/memory would normally block it") + assert.Equal(t, 0, rm.canAllocate(gpuType, 1), + "GPU type must be blocked by cpu/mem exhaustion like any other instance type") +} + +// canAllocate for GPU types succeeds once both cpu/mem headroom and a free +// GPU slot exist. +func TestCanAllocate_GPUType_SucceedsWithRoomAndSlot(t *testing.T) { + rm := &ResourceManager{ + hostVCPU: 32, + hostMemGB: 256.0, + instanceTypes: map[string]*ec2.InstanceTypeInfo{}, + gpuManager: gpu.NewManager([]gpu.GPUDevice{makeGPUDevice()}), + } + gpuType := makeGPUInstanceType("g7e.4xlarge", 16, 128*1024) + assert.Equal(t, 1, rm.canAllocate(gpuType, 1)) +} + +// canAllocate for GPU types is refused up front when the GPU pool has no +// free slot, even with abundant cpu/mem headroom. +func TestCanAllocate_GPUType_RefusedWhenGPUPoolExhausted(t *testing.T) { + rm := &ResourceManager{ + hostVCPU: 32, + hostMemGB: 256.0, + instanceTypes: map[string]*ec2.InstanceTypeInfo{}, + gpuManager: gpu.NewManager(nil), + } + gpuType := makeGPUInstanceType("g7e.4xlarge", 16, 128*1024) + assert.Equal(t, 0, rm.canAllocate(gpuType, 1), + "no GPU in the pool must refuse admission regardless of cpu/mem headroom") } // --- GetSupportedInstanceTypeInfos --- diff --git a/spinifex/daemon/gpu_pool_reconciler.go b/spinifex/daemon/gpu_pool_reconciler.go new file mode 100644 index 000000000..833dddd50 --- /dev/null +++ b/spinifex/daemon/gpu_pool_reconciler.go @@ -0,0 +1,85 @@ +package daemon + +import ( + "context" + "log/slog" + + "github.com/mulgadc/spinifex/spinifex/vm" +) + +// gpuPoolLiveStates are the instance states that legitimately still hold a +// GPU pool entry: mid-launch, running, or mid-teardown (stopCleanup / +// terminateCleanup release the slot before the instance settles into +// Stopped/Terminated). Anything else — Stopped, Error, Terminated, or the +// instance being entirely absent from the running map — means the slot +// should already have been released and was not. +var gpuPoolLiveStates = map[vm.InstanceState]bool{ + vm.StateProvisioning: true, + vm.StatePending: true, + vm.StateRunning: true, + vm.StateStopping: true, + vm.StateShuttingDown: true, +} + +// gpuPoolReconciler frees GPU pool entries whose owning instance is absent +// or has settled in a terminal, non-running state. It is the backstop for +// the synchronous release in vm/crash_recovery.go: a node-down mid-cascade, +// or any other gap that skips the synchronous release, would otherwise leak +// the slot forever, since nothing else reclaims it. Node-local vm.Reaper run +// by the GarbageCollector backstop, mirroring eniReconciler. +type gpuPoolReconciler struct { + d *Daemon +} + +var _ vm.Reaper = (*gpuPoolReconciler)(nil) + +// newGPUPoolReconciler builds the reconciler over the daemon. Returns nil +// when the VM manager is absent so the caller can skip wiring it. The GPU +// manager itself is read live from d.gpuManager on every sweep (not +// captured here) since SIGHUP can replace it at runtime. +func (d *Daemon) newGPUPoolReconciler() *gpuPoolReconciler { + if d.vmMgr == nil { + return nil + } + return &gpuPoolReconciler{d: d} +} + +func (r *gpuPoolReconciler) Class() string { return "gpu-pool" } +func (r *gpuPoolReconciler) Scope() vm.ReaperScope { return vm.ScopeNodeLocal } + +// Sweep walks the GPU pool and releases any claimed entry whose instance is +// absent from the running map or has settled in a non-running state. +func (r *gpuPoolReconciler) Sweep(ctx context.Context) (int, error) { + mgr := r.d.gpuManager + if mgr == nil { + return 0, nil + } + + reaped := 0 + for _, entry := range mgr.Snapshot() { + if ctx.Err() != nil { + return reaped, ctx.Err() + } + if entry.InstanceID == "" || r.instanceHoldsSlot(entry.InstanceID) { + continue + } + slog.Warn("gpu-reconciler: releasing orphaned GPU pool entry", + "instanceId", entry.InstanceID, "pci", entry.Device.PCIAddress) + if err := mgr.Release(entry.InstanceID); err != nil { + slog.Warn("gpu-reconciler: release failed", "instanceId", entry.InstanceID, "err", err) + continue + } + reaped++ + } + return reaped, nil +} + +// instanceHoldsSlot reports whether instanceID names a VM in a state that +// legitimately still holds its GPU pool entry. +func (r *gpuPoolReconciler) instanceHoldsSlot(instanceID string) bool { + instance, ok := r.d.vmMgr.Get(instanceID) + if !ok { + return false + } + return gpuPoolLiveStates[r.d.vmMgr.Status(instance)] +} diff --git a/spinifex/daemon/gpu_pool_reconciler_test.go b/spinifex/daemon/gpu_pool_reconciler_test.go new file mode 100644 index 000000000..c735fb644 --- /dev/null +++ b/spinifex/daemon/gpu_pool_reconciler_test.go @@ -0,0 +1,103 @@ +package daemon + +import ( + "context" + "os" + "path/filepath" + "testing" + + "github.com/mulgadc/spinifex/spinifex/gpu" + "github.com/mulgadc/spinifex/spinifex/vm" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// claimMIGSliceForReconcilerTest seeds a one-slice MIG pool and claims it for +// instanceID. Uses the MIG path (not whole-GPU) so the test never touches +// real VFIO/sysfs state — MIG release is pure in-memory bookkeeping. +func claimMIGSliceForReconcilerTest(t *testing.T, instanceID string) *gpu.Manager { + t.Helper() + mdevPath := filepath.Join(t.TempDir(), "mdev0") + require.NoError(t, os.WriteFile(mdevPath, []byte("x"), 0o600)) + + mgr := gpu.NewManager(nil) + mgr.AddMIGInstances(gpu.GPUDevice{PCIAddress: "0000:03:00.0"}, []gpu.MIGInstance{ + {Profile: gpu.MIGProfile{Name: "1g.10gb"}, MdevPath: mdevPath}, + }) + _, _, err := mgr.Claim(instanceID, "1g.10gb") + require.NoError(t, err) + require.Equal(t, 0, mgr.Available(), "GPU slot must be claimed before the sweep") + return mgr +} + +// TestGPUPoolReconciler_FreesOrphanedSlot is the regression test for +// mulga-keptb: a GPU pool entry whose InstanceID names an instance absent +// from the running map entirely must be released. +func TestGPUPoolReconciler_FreesOrphanedSlot(t *testing.T) { + mgr := claimMIGSliceForReconcilerTest(t, "i-orphan") + d := &Daemon{vmMgr: vm.NewManager(), gpuManager: mgr} + r := d.newGPUPoolReconciler() + require.NotNil(t, r) + + reaped, err := r.Sweep(context.Background()) + + require.NoError(t, err) + assert.Equal(t, 1, reaped) + assert.Equal(t, 1, mgr.Available(), "orphaned slot must return to the pool") +} + +// TestGPUPoolReconciler_LeavesRunningInstanceAlone verifies the reconciler +// does not touch a slot whose instance is actively running. +func TestGPUPoolReconciler_LeavesRunningInstanceAlone(t *testing.T) { + mgr := claimMIGSliceForReconcilerTest(t, "i-running") + vmMgr := vm.NewManager() + vmMgr.Insert(&vm.VM{ID: "i-running", Status: vm.StateRunning}) + d := &Daemon{vmMgr: vmMgr, gpuManager: mgr} + r := d.newGPUPoolReconciler() + require.NotNil(t, r) + + reaped, err := r.Sweep(context.Background()) + + require.NoError(t, err) + assert.Equal(t, 0, reaped) + assert.Equal(t, 0, mgr.Available(), "a running instance's GPU slot must not be reclaimed") +} + +// TestGPUPoolReconciler_FreesSlotForTerminalInstance covers the backstop case +// for a stuck StateError instance still holding a GPU slot — the same +// scenario the synchronous crash-recovery release targets directly, here +// caught by the reconciler as a second line of defense. +func TestGPUPoolReconciler_FreesSlotForTerminalInstance(t *testing.T) { + mgr := claimMIGSliceForReconcilerTest(t, "i-error") + vmMgr := vm.NewManager() + vmMgr.Insert(&vm.VM{ID: "i-error", Status: vm.StateError}) + d := &Daemon{vmMgr: vmMgr, gpuManager: mgr} + r := d.newGPUPoolReconciler() + require.NotNil(t, r) + + reaped, err := r.Sweep(context.Background()) + + require.NoError(t, err) + assert.Equal(t, 1, reaped) + assert.Equal(t, 1, mgr.Available()) +} + +// TestGPUPoolReconciler_NoGPUManager_NoOp verifies the reconciler is a +// harmless no-op when GPU passthrough is disabled. +func TestGPUPoolReconciler_NoGPUManager_NoOp(t *testing.T) { + d := &Daemon{vmMgr: vm.NewManager()} + r := d.newGPUPoolReconciler() + require.NotNil(t, r) + + reaped, err := r.Sweep(context.Background()) + + require.NoError(t, err) + assert.Equal(t, 0, reaped) +} + +// TestNewGPUPoolReconciler_NilWithoutVMManager verifies the constructor +// refuses to build a reconciler with no VM manager to query. +func TestNewGPUPoolReconciler_NilWithoutVMManager(t *testing.T) { + d := &Daemon{} + assert.Nil(t, d.newGPUPoolReconciler()) +} diff --git a/spinifex/daemon/resource_test.go b/spinifex/daemon/resource_test.go index ff0580a0a..b48dadec0 100644 --- a/spinifex/daemon/resource_test.go +++ b/spinifex/daemon/resource_test.go @@ -5,6 +5,7 @@ import ( "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/ec2" + "github.com/mulgadc/spinifex/spinifex/gpu" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -386,12 +387,15 @@ func TestCanAllocateLocked_MIGvsWholeGPU(t *testing.T) { TotalGpuMemoryInMiB: aws.Int64(10240), } - // Host with 8 schedulable vCPUs and 32 GiB schedulable memory (after reserve). + // Host with 8 schedulable vCPUs and 32 GiB schedulable memory (after + // reserve), and a GPU pool with 5 free slots — plenty for the GPU-slot + // gate so these assertions exercise the cpu/mem gate in isolation. rm := &ResourceManager{ hostVCPU: 10, hostMemGB: 34.0, reservedVCPU: 2, reservedMem: 2.0, + gpuManager: gpu.NewManager(make([]gpu.GPUDevice, 5)), } migType := &ec2.InstanceTypeInfo{ @@ -402,8 +406,8 @@ func TestCanAllocateLocked_MIGvsWholeGPU(t *testing.T) { } wholeGPUType := &ec2.InstanceTypeInfo{ InstanceType: aws.String("g7e.4xlarge"), - VCpuInfo: &ec2.VCpuInfo{DefaultVCpus: aws.Int64(16)}, - MemoryInfo: &ec2.MemoryInfo{SizeInMiB: aws.Int64(128 * 1024)}, + VCpuInfo: &ec2.VCpuInfo{DefaultVCpus: aws.Int64(4)}, + MemoryInfo: &ec2.MemoryInfo{SizeInMiB: aws.Int64(16 * 1024)}, GpuInfo: gpuInfo, } @@ -415,8 +419,70 @@ func TestCanAllocateLocked_MIGvsWholeGPU(t *testing.T) { rm.allocatedMem = 32.0 assert.Equal(t, 0, rm.canAllocateLocked(migType, 10), "MIG: second slice rejected when resources exhausted") - // Whole-GPU: resource check bypassed — count returned regardless of headroom. - assert.Equal(t, 10, rm.canAllocateLocked(wholeGPUType, 10), "whole-GPU: bypasses CPU/mem check") + // Whole-GPU: regression for mulga-jeo02. With cpu/mem already exhausted by + // the MIG slice above, a whole-GPU instance must also be refused instead + // of bypassing the check. + assert.Equal(t, 0, rm.canAllocateLocked(wholeGPUType, 10), "whole-GPU: no longer bypasses the cpu/mem gate") +} + +// TestCanAllocateLocked_WholeGPU_MemoryAndSlotGates is the regression test +// for mulga-jeo02: a whole-GPU launch must be refused (allocateForLaunch maps +// this to InsufficientInstanceCapacity) when the node lacks the instance's +// memory, must succeed when memory is available, and must be refused up +// front — not just late at gpuManager.Claim — when the GPU pool is exhausted. +func TestCanAllocateLocked_WholeGPU_MemoryAndSlotGates(t *testing.T) { + gpuInfo := &ec2.GpuInfo{ + Gpus: []*ec2.GpuDeviceInfo{{ + Count: aws.Int64(1), + Manufacturer: aws.String("NVIDIA"), + Name: aws.String("A10G"), + MemoryInfo: &ec2.GpuDeviceMemoryInfo{SizeInMiB: aws.Int64(24576)}, + }}, + TotalGpuMemoryInMiB: aws.Int64(24576), + } + wholeGPUType := &ec2.InstanceTypeInfo{ + InstanceType: aws.String("g5.xlarge"), + VCpuInfo: &ec2.VCpuInfo{DefaultVCpus: aws.Int64(4)}, + MemoryInfo: &ec2.MemoryInfo{SizeInMiB: aws.Int64(16 * 1024)}, + GpuInfo: gpuInfo, + } + + // 8 schedulable vCPUs → 6 remaining after reserve; 32 GiB → 30 GiB remaining. + newRM := func(freeGPUSlots int) *ResourceManager { + return &ResourceManager{ + hostVCPU: 8, + hostMemGB: 32.0, + reservedVCPU: 2, + reservedMem: 2.0, + gpuManager: gpu.NewManager(make([]gpu.GPUDevice, freeGPUSlots)), + } + } + + t.Run("refused when host memory insufficient", func(t *testing.T) { + rm := newRM(1) + rm.allocatedMem = 25.0 // 30 GiB schedulable - 25 allocated = 5 GiB left, short of the 16 GiB guest + got := rm.canAllocateLocked(wholeGPUType, 1) + assert.Equal(t, 0, got, "whole-GPU must be refused on memory exhaustion, not bypass the cpu/mem gate") + _, err := allocateForLaunch(got, 1, 1) + assert.ErrorIs(t, err, errInsufficientCapacity, "RunInstances maps this to InsufficientInstanceCapacity") + }) + + t.Run("succeeds when memory and a GPU slot are both available", func(t *testing.T) { + rm := newRM(1) + got := rm.canAllocateLocked(wholeGPUType, 1) + assert.Equal(t, 1, got) + n, err := allocateForLaunch(got, 1, 1) + require.NoError(t, err) + assert.Equal(t, 1, n) + }) + + t.Run("refused up front when the GPU pool is exhausted despite cpu/mem headroom", func(t *testing.T) { + rm := newRM(0) + got := rm.canAllocateLocked(wholeGPUType, 1) + assert.Equal(t, 0, got, "GPU-slot exhaustion must refuse up front, not defer to a late Claim failure") + _, err := allocateForLaunch(got, 1, 1) + assert.ErrorIs(t, err, errInsufficientCapacity) + }) } func TestResolveHostReserve(t *testing.T) { diff --git a/spinifex/vm/crash_recovery.go b/spinifex/vm/crash_recovery.go index 4612b8bb8..7866c23ec 100644 --- a/spinifex/vm/crash_recovery.go +++ b/spinifex/vm/crash_recovery.go @@ -183,6 +183,7 @@ func (m *Manager) MaybeRestart(instance *VM) { "crashes", crashCount, "window", RestartWindow, "max", MaxRestartsInWindow) + m.releaseGPUOnTerminalCrash(instance) return } @@ -190,6 +191,7 @@ func (m *Manager) MaybeRestart(instance *VM) { if _, ok := m.deps.InstanceTypes.Resolve(instance.InstanceType); !ok { slog.Error("Unknown instance type, cannot restart", "instance", instance.ID, "type", instance.InstanceType) + m.releaseGPUOnTerminalCrash(instance) return } } @@ -198,6 +200,7 @@ func (m *Manager) MaybeRestart(instance *VM) { if m.deps.Resources.CanAllocate(instance.InstanceType, 1) < 1 { slog.Error("Insufficient resources to restart instance", "instance", instance.ID, "type", instance.InstanceType) + m.releaseGPUOnTerminalCrash(instance) return } } @@ -280,5 +283,30 @@ func (m *Manager) RestartCrashedInstance(instance *VM) { "instance", instance.ID, "err", err) } } + // The relaunch attempt failed, so the instance settles in StateError + // for good — release the GPU slot it still holds from before the + // crash rather than leaving it stuck until the daemon restarts. + m.releaseGPUOnTerminalCrash(instance) + } +} + +// releaseGPUOnTerminalCrash frees a crashed instance's GPU pool slot once it +// is known no restart will follow (restarts exhausted, unknown instance +// type, insufficient resources to restart, or a failed relaunch attempt). +// HandleCrash's own deallocateResources call only returns vCPU/memory to the +// ResourceManager; it never touches the GPU pool, since a successful restart +// reuses the instance's still-set GPUAttachments without reclaiming them. +// Called only from paths that leave the instance permanently in StateError, +// so releasing here cannot race a restart that still needs the slot. +func (m *Manager) releaseGPUOnTerminalCrash(instance *VM) { + if m.deps.InstanceCleaner == nil { + return + } + if err := m.deps.InstanceCleaner.ReleaseGPU(instance); err != nil { + slog.Warn("Failed to release GPU for crashed instance settling in error state", + "instance", instance.ID, "err", err) + } + if len(instance.GPUAttachments) > 0 { + m.UpdateState(instance.ID, func(v *VM) { v.GPUAttachments = nil }) } } diff --git a/spinifex/vm/crash_recovery_test.go b/spinifex/vm/crash_recovery_test.go index 93964a064..66bccf96e 100644 --- a/spinifex/vm/crash_recovery_test.go +++ b/spinifex/vm/crash_recovery_test.go @@ -11,10 +11,55 @@ import ( "time" "github.com/aws/aws-sdk-go/service/ec2" + "github.com/mulgadc/spinifex/spinifex/gpu" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) +// gpuBackedCleaner is a minimal InstanceCleaner whose ReleaseGPU calls a real +// gpu.Manager (every other method is a no-op), mirroring the production +// instanceCleanerAdapter closely enough that crash-recovery GPU-release +// tests can assert against gpuManager.Available() directly instead of just +// recording that ReleaseGPU was called. +type gpuBackedCleaner struct { + mgr *gpu.Manager +} + +func (c *gpuBackedCleaner) DeleteVolumes(*VM) error { return nil } +func (c *gpuBackedCleaner) CleanupMgmtNetwork(*VM) {} +func (c *gpuBackedCleaner) ReleasePublicIP(*VM) error { return nil } +func (c *gpuBackedCleaner) DetachAndDeleteENI(*VM) error { return nil } +func (c *gpuBackedCleaner) RemoveFromPlacementGroup(*VM) error { return nil } +func (c *gpuBackedCleaner) RemoveFromSpotRequest(*VM) error { return nil } + +func (c *gpuBackedCleaner) ReleaseGPU(v *VM) error { + if c.mgr == nil || len(v.GPUAttachments) == 0 { + return nil + } + return c.mgr.Release(v.ID) +} + +var _ InstanceCleaner = (*gpuBackedCleaner)(nil) + +// claimMIGSliceForTest seeds a one-slice MIG pool and claims it for +// instanceID, returning the manager and the mdev path now attached. Uses the +// MIG path (not whole-GPU) so the test never touches real VFIO/sysfs state — +// MIG release is pure in-memory bookkeeping. +func claimMIGSliceForTest(t *testing.T, instanceID string) (*gpu.Manager, string) { + t.Helper() + mdevPath := filepath.Join(t.TempDir(), "mdev0") + require.NoError(t, os.WriteFile(mdevPath, []byte("x"), 0o600)) + + mgr := gpu.NewManager(nil) + mgr.AddMIGInstances(gpu.GPUDevice{PCIAddress: "0000:03:00.0"}, []gpu.MIGInstance{ + {Profile: gpu.MIGProfile{Name: "1g.10gb"}, MdevPath: mdevPath}, + }) + _, _, err := mgr.Claim(instanceID, "1g.10gb") + require.NoError(t, err) + require.Equal(t, 0, mgr.Available(), "GPU slot must be claimed before the crash") + return mgr, mdevPath +} + func TestClassifyCrashReason_CleanExit(t *testing.T) { assert.Equal(t, "clean-exit", ClassifyCrashReason(nil)) } @@ -277,6 +322,41 @@ func TestMaybeRestart_ExceedsMaxInWindow(t *testing.T) { "RestartCount must not increment when exceeded") } +// TestMaybeRestart_ExceedsMaxInWindow_ReleasesGPU is the regression test for +// mulga-keptb: a GPU instance that crashes past restart-exhaustion must +// release its GPU pool slot instead of holding it forever in StateError, +// since no restart will ever be scheduled to reclaim it. +func TestMaybeRestart_ExceedsMaxInWindow_ReleasesGPU(t *testing.T) { + m, _, _, _ := crashTestManager(t) + mgr, mdevPath := claimMIGSliceForTest(t, "i-gpu-exhausted") + m.SetDeps(Deps{ + NodeID: "test-node", + Resources: newFakeResourceController(), + InstanceTypes: fakeInstanceTypeResolver{"t3.micro": {VCPUs: 1, MemoryMiB: 1024, Architecture: "x86_64"}}, + InstanceCleaner: &gpuBackedCleaner{mgr: mgr}, + }) + + instance := &VM{ + ID: "i-gpu-exhausted", + Status: StateError, + InstanceType: "t3.micro", + GPUAttachments: []gpu.GPUAttachment{{MdevPath: mdevPath}}, + Health: InstanceHealthState{ + CrashCount: MaxRestartsInWindow + 1, + FirstCrashTime: time.Now(), + RestartCount: MaxRestartsInWindow, + }, + } + m.Insert(instance) + + m.MaybeRestart(instance) + + assert.Equal(t, 1, mgr.Available(), + "GPU slot must return to the pool once restarts are exhausted") + assert.Empty(t, instance.GPUAttachments, + "GPUAttachments must be cleared once the slot is released") +} + func TestMaybeRestart_ResetsAfterWindow(t *testing.T) { m, _, _, _ := crashTestManager(t) @@ -534,6 +614,41 @@ func TestRestartCrashedInstance_RunFailureRollback(t *testing.T) { "instance must end in StateError after Run-failure rollback") } +// TestRestartCrashedInstance_RunFailureReleasesGPU is the regression test for +// mulga-keptb: when the relaunch attempt itself fails, the instance settles +// back in StateError for good — the GPU pool slot it still held from before +// the crash must be released, not left claimed forever. +func TestRestartCrashedInstance_RunFailureReleasesGPU(t *testing.T) { + m, rc, rt, _ := crashTestManager(t) + mgr, mdevPath := claimMIGSliceForTest(t, "i-rollback-gpu") + + mounter := &fakeVolumeMounter{mountErr: errors.New("mount failed during restart")} + m.SetDeps(Deps{ + NodeID: "test-node", + Resources: rc, + VolumeMounter: mounter, + InstanceTypes: fakeInstanceTypeResolver{"t3.micro": {VCPUs: 1, MemoryMiB: 1024, Architecture: "x86_64"}}, + TransitionState: rt.apply, + InstanceCleaner: &gpuBackedCleaner{mgr: mgr}, + }) + + instance := &VM{ + ID: "i-rollback-gpu", + Status: StateError, + InstanceType: "t3.micro", + GPUAttachments: []gpu.GPUAttachment{{MdevPath: mdevPath}}, + } + m.Insert(instance) + + m.RestartCrashedInstance(instance) + + assert.Equal(t, StateError, m.Status(instance)) + assert.Equal(t, 1, mgr.Available(), + "GPU slot must return to the pool when the relaunch attempt fails") + assert.Empty(t, instance.GPUAttachments, + "GPUAttachments must be cleared once the slot is released") +} + func TestRecordQMPFailure_StampsImpairedAtThreshold(t *testing.T) { m := NewManager() instance := &VM{ID: "i-qmp", Status: StateRunning}