mirror of
https://github.com/mulgadc/spinifex.git
synced 2026-10-15 17:11:30 +00:00
fix(daemon,vm): gate GPU admission on cpu/mem + release leaked GPU slot on crash (#513)
* fix(daemon): gate GPU instance admission on cpu/mem and GPU-slot availability canAllocateLocked early-returned count for whole-GPU (non-MIG) types, bypassing the vCPU/memory admission calc and the live MemAvailable gate. canAllocateCount already had a requiresGPU/availGPU branch, but every production caller passed requiresGPU=false, making it dead code. GPU instance types (whole-GPU and MIG) now flow through the same cpu/mem + live-mem gate as every other type, with GPU-slot count wired in as an additional hard constraint from gpuManager.Available(). This also gives MIG a real up-front GPU-slice gate instead of only failing late at Claim. * fix(vm): release leaked GPU pool slot on crash-to-error and add reconciler backstop HandleCrash freed vCPU/memory via deallocateResources but never called InstanceCleaner.ReleaseGPU, since it bypasses the stop/terminate cleanup chain that normally carries that call. A successful restart reuses the still-set GPUAttachments, so that path is untouched, but when restarts are exhausted, the instance type is unknown, there isn't enough capacity to restart, or the relaunch itself fails, the instance settles permanently in StateError still holding its GPU pool entry with nothing to reclaim it. Release the GPU and clear GPUAttachments on each of those terminal paths. Add a GPU-pool reconciler mirroring the existing ENI reconciler, wired into the daemon's reconcile loop, that periodically frees any pool entry whose instance is absent or has settled into a non-running state, as a backstop for a node-down mid-cascade or any other gap in the synchronous release.
This commit is contained in:
@@ -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)
|
||||
}
|
||||
|
||||
@@ -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 ---
|
||||
|
||||
@@ -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)]
|
||||
}
|
||||
@@ -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())
|
||||
}
|
||||
@@ -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) {
|
||||
|
||||
@@ -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 })
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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}
|
||||
|
||||
Reference in New Issue
Block a user