diff --git a/spinifex/daemon/vm_adapters.go b/spinifex/daemon/vm_adapters.go index b95a792db..e9f41cb42 100644 --- a/spinifex/daemon/vm_adapters.go +++ b/spinifex/daemon/vm_adapters.go @@ -10,6 +10,7 @@ import ( "time" "github.com/mulgadc/spinifex/spinifex/awserrors" + "github.com/mulgadc/spinifex/spinifex/gpu" handlers_ec2_placementgroup "github.com/mulgadc/spinifex/spinifex/handlers/ec2/placementgroup" "github.com/mulgadc/spinifex/spinifex/network/topology" "github.com/mulgadc/spinifex/spinifex/tags" @@ -880,11 +881,21 @@ func (a *instanceCleanerAdapter) RemoveFromSpotRequest(instance *vm.VM) error { // ReleaseGPU unbinds the instance's GPU from vfio-pci and rebinds to its // original host driver. No-op for instances without a GPU allocation or // when GPU passthrough is disabled. +// +// Holding no device is success, not failure: teardown re-drives this until it +// reports done, and a stop that already released leaves nothing for the later +// terminate to do. Returning an error there wedges the record forever. func (a *instanceCleanerAdapter) ReleaseGPU(instance *vm.VM) error { if a.d.gpuManager == nil || len(instance.GPUAttachments) == 0 { return nil } - if err := a.d.gpuManager.Release(instance.ID); err != nil { + err := a.d.gpuManager.Release(instance.ID) + switch { + case errors.Is(err, gpu.ErrNoGPUClaimed): + slog.Info("GPU already released, nothing to do", + "gpus", instance.GPUAttachments, "instanceId", instance.ID) + return nil + case err != nil: slog.Error("Failed to release GPU on stop, device may need manual rebind", "gpus", instance.GPUAttachments, "instanceId", instance.ID, "err", err) return err diff --git a/spinifex/daemon/vm_adapters_test.go b/spinifex/daemon/vm_adapters_test.go index 80156dbb2..f70ad6aba 100644 --- a/spinifex/daemon/vm_adapters_test.go +++ b/spinifex/daemon/vm_adapters_test.go @@ -368,15 +368,22 @@ func TestReleaseGPU_NoAddresses_NoOp(t *testing.T) { a.ReleaseGPU(instance) } -// ReleaseGPU logs a warning when the manager returns an error (instance not claimed). -func TestReleaseGPU_ManagerError_LogsWarning(t *testing.T) { +// TestReleaseGPU_AlreadyReleased_IsSuccess pins the fix for a teardown record +// that could never complete. A stop releases the GPU, so the later terminate +// finds no claim; reporting that as failure left the gpu teardown mark stuck +// short of done, so the record was never purged and the GC re-drove it every +// two minutes indefinitely, failing identically each time. +func TestReleaseGPU_AlreadyReleased_IsSuccess(t *testing.T) { mgr := gpu.NewManager(nil) d := &Daemon{gpuManager: mgr} a := newInstanceCleanerAdapter(d) - // GPU address set but no claim registered — Release returns an error. + // GPU address set but no claim registered — the shape a re-driven release + // sees once the work is already done. instance := &vm.VM{ID: "i-unclaimed", GPUAttachments: []gpu.GPUAttachment{{PCIAddress: "0000:03:00.0"}}} - // Must not panic; the error is logged as a warning. - a.ReleaseGPU(instance) + + require.NoError(t, a.ReleaseGPU(instance), "an already-released GPU must not fail teardown") + // Idempotent under repetition, which is what the reaper does to it. + require.NoError(t, a.ReleaseGPU(instance)) } // --- RemoveFromSpotRequest --- diff --git a/spinifex/gpu/manager.go b/spinifex/gpu/manager.go index b33a32c58..f44006585 100644 --- a/spinifex/gpu/manager.go +++ b/spinifex/gpu/manager.go @@ -8,6 +8,11 @@ import ( "sync" ) +// ErrNoGPUClaimed reports that the instance held no device when Release ran. +// Teardown re-drives releases and should read this as success, since the state +// it wants already holds; a claim/release pair that hits it is out of step. +var ErrNoGPUClaimed = errors.New("no GPU claimed by instance") + type gpuEntry struct { Device GPUDevice MIGInstance *MIGInstance // nil = whole-GPU passthrough; non-nil = MIG slice @@ -213,6 +218,10 @@ func (m *Manager) Claim(instanceID, profileName string) (*GPUDevice, *MIGInstanc // (already vfio-pci before the daemon) are left bound; Claim-bound devices are // unbound and rebind to their original driver (failure marks them unavailable). // For dynamically carved MIG GPUs, the last release destroys all slices. +// +// Holding nothing yields ErrNoGPUClaimed, which is a distinct condition rather +// than a fault: it says the desired state already holds. Callers that re-drive +// releases decide for themselves whether that counts as success. func (m *Manager) Release(instanceID string) error { m.mu.Lock() defer m.mu.Unlock() @@ -269,7 +278,7 @@ func (m *Manager) Release(instanceID string) error { } if !released { - return fmt.Errorf("no GPU claimed by instance %s", instanceID) + return fmt.Errorf("%w %s", ErrNoGPUClaimed, instanceID) } // For dynamically carved MIG GPUs: destroy all slices when the last instance diff --git a/spinifex/gpu/manager_mig_test.go b/spinifex/gpu/manager_mig_test.go index 6e06b682d..a8c00804e 100644 --- a/spinifex/gpu/manager_mig_test.go +++ b/spinifex/gpu/manager_mig_test.go @@ -153,6 +153,7 @@ func TestMIGRelease_UnknownInstance_ReturnsError(t *testing.T) { m := NewManager(nil) err := m.Release("i-never-claimed") require.Error(t, err) + require.ErrorIs(t, err, ErrNoGPUClaimed) } // --- MarkMIGFailed --- diff --git a/spinifex/gpu/manager_test.go b/spinifex/gpu/manager_test.go index a52ca5d0c..552b408d6 100644 --- a/spinifex/gpu/manager_test.go +++ b/spinifex/gpu/manager_test.go @@ -1,8 +1,10 @@ package gpu import ( + "errors" "os" "path/filepath" + "strings" "testing" ) @@ -151,7 +153,15 @@ func TestManagerRelease_UnknownInstance(t *testing.T) { err := m.Release("i-does-not-exist") if err == nil { - t.Error("want error releasing unknown instance, got nil") + t.Fatal("want error releasing unknown instance, got nil") + } + // Matchable rather than a bare message: teardown re-drives releases and has + // to tell "already released" from a rebind failure without reading text. + if !errors.Is(err, ErrNoGPUClaimed) { + t.Errorf("want ErrNoGPUClaimed, got %v", err) + } + if !strings.Contains(err.Error(), "i-does-not-exist") { + t.Errorf("want the instance ID in the message, got %q", err) } } @@ -181,6 +191,12 @@ func TestManagerRelease_FailureMarksUnavailable(t *testing.T) { if err == nil { t.Error("want error when vfio-pci unbind fails, got nil") } + // Must stay distinct from the benign already-released case, which callers + // are entitled to ignore. This one clears Available, the health signal the + // Bedrock capacity gate reads. + if errors.Is(err, ErrNoGPUClaimed) { + t.Errorf("a rebind failure must not report as ErrNoGPUClaimed, got %v", err) + } // Pool entry must be marked unavailable so the broken GPU isn't re-offered. if m.Available() != 0 {