Keep stop's no-op contract on failed retained vGPU release and pass assignments to DestroyVGPU as a struct

Author: yummybomb

lib/devices/mdev_darwin.go

@@ -55,9 +55,9 @@ func IsMdevInUse(mdevUUID string) bool {
 	return false
 }
 
-func DestroyVGPU(ctx context.Context, framework VGPUFramework, devicePath, mdevUUID string) error {
-	if framework != VGPUFrameworkNone && framework != VGPUFrameworkMdev {
-		return fmt.Errorf("unknown vGPU framework %q", framework)
+func DestroyVGPU(ctx context.Context, assignment VGPUAssignment) error {
+	if assignment.Framework != VGPUFrameworkNone && assignment.Framework != VGPUFrameworkMdev {
+		return fmt.Errorf("unknown vGPU framework %q", assignment.Framework)
 	}
 	return nil
 }

lib/devices/types.go

@@ -81,6 +81,13 @@ type VirtualFunction struct {
 	Allocated  bool   `json:"allocated"`   // true if a vGPU is assigned to this VF
 }
 
+// VGPUAssignment identifies an existing vGPU assignment to release.
+type VGPUAssignment struct {
+	Framework  VGPUFramework
+	DevicePath string
+	MdevUUID   string
+}
+
 type VGPUDevice struct {
 	Framework   VGPUFramework
 	VFAddress   string

lib/devices/vgpu_linux.go

@@ -23,15 +23,16 @@ func CreateVGPU(ctx context.Context, profileName, instanceID string) (*VGPUDevic
 	}, nil
 }
 
-func DestroyVGPU(ctx context.Context, framework VGPUFramework, devicePath, mdevUUID string) error {
-	if framework != VGPUFrameworkNone && framework != VGPUFrameworkMdev {
-		return fmt.Errorf("unknown vGPU framework %q", framework)
+func DestroyVGPU(ctx context.Context, assignment VGPUAssignment) error {
+	if assignment.Framework != VGPUFrameworkNone && assignment.Framework != VGPUFrameworkMdev {
+		return fmt.Errorf("unknown vGPU framework %q", assignment.Framework)
 	}
+	mdevUUID := assignment.MdevUUID
 	if mdevUUID == "" {
-		if devicePath == "" {
+		if assignment.DevicePath == "" {
 			return nil
 		}
-		mdevUUID = filepath.Base(devicePath)
+		mdevUUID = filepath.Base(assignment.DevicePath)
 	}
 	return DestroyMdev(ctx, mdevUUID)
 }

lib/instances/create.go

@@ -287,7 +287,12 @@ func (m *manager) createInstance(
 
 		// Add vGPU cleanup to stack
 		cu.Add(func() {
-			if err := devices.DestroyVGPU(ctx, gpuDevice.Framework, gpuDevice.SysfsPath, gpuDevice.MdevUUID); err != nil {
+			assignment := devices.VGPUAssignment{
+				Framework:  gpuDevice.Framework,
+				DevicePath: gpuDevice.SysfsPath,
+				MdevUUID:   gpuDevice.MdevUUID,
+			}
+			if err := devices.DestroyVGPU(ctx, assignment); err != nil {
 				log.WarnContext(ctx, "failed to destroy vGPU on cleanup", "instance_id", id, "error", err)
 			}
 		})

lib/instances/lifecycle_noop_test.go

@@ -208,7 +208,7 @@ func TestStopStoppedInstanceReleasesRetainedVGPU(t *testing.T) {
 	assert.Empty(t, stored.GPUDevicePath)
 }
 
-func TestStopStoppedInstanceVGPUReleaseFailureReturnsError(t *testing.T) {
+func TestStopStoppedInstanceVGPUReleaseFailureRemainsNoop(t *testing.T) {
 	m, id := newLifecycleNoopManagerWithInstance(t, StateStopped, time.Now().UTC())
 	meta, err := m.loadMetadata(id)
 	require.NoError(t, err)
@@ -217,9 +217,10 @@ func TestStopStoppedInstanceVGPUReleaseFailureReturnsError(t *testing.T) {
 	meta.GPUDevicePath = "/sys/bus/pci/devices/0000:82:00.4"
 	require.NoError(t, m.saveMetadata(meta))
 
-	_, err = m.StopInstance(context.Background(), id)
-	require.Error(t, err)
-	assert.ErrorContains(t, err, "destroy vGPU")
+	inst, err := m.StopInstance(context.Background(), id)
+	require.NoError(t, err)
+	require.NotNil(t, inst)
+	assert.Equal(t, StateStopped, inst.State)
 
 	stored, err := m.loadMetadata(id)
 	require.NoError(t, err)

lib/instances/manager.go

@@ -623,10 +623,10 @@ func (m *manager) StopInstance(ctx context.Context, id string) (*Instance, error
 		}
 		// A stopped instance can retain a vGPU assignment when the release
 		// failed during the original stop. Retry it here so the vGPU slot is
-		// not held until the next start, delete, or hypeman restart.
-		if err := m.releaseRetainedVGPULocked(ctx, id); err != nil {
-			return nil, err
-		}
+		// not held until the next start, delete, or hypeman restart. A failed
+		// retry only logs, keeping stop's no-op contract for already-stopped
+		// instances.
+		m.releaseRetainedVGPULocked(ctx, id)
 		updated, err := m.currentInstanceWithoutHydration(ctx, id)
 		if err != nil {
 			return nil, err

lib/instances/start.go

@@ -160,7 +160,12 @@ func (m *manager) startInstance(
 		setStoredVGPUDevice(stored, device)
 		// Add vGPU cleanup to stack
 		cu.Add(func() {
-			if err := devices.DestroyVGPU(ctx, device.Framework, device.SysfsPath, device.MdevUUID); err != nil {
+			assignment := devices.VGPUAssignment{
+				Framework:  device.Framework,
+				DevicePath: device.SysfsPath,
+				MdevUUID:   device.MdevUUID,
+			}
+			if err := devices.DestroyVGPU(ctx, assignment); err != nil {
 				log.WarnContext(ctx, "failed to destroy vGPU on cleanup", "instance_id", id, "error", err)
 			}
 		})

lib/instances/vgpu.go

@@ -2,7 +2,6 @@ package instances
 
 import (
 	"context"
-	"fmt"
 	"path/filepath"
 
 	"github.com/kernel/hypeman/lib/devices"
@@ -24,7 +23,12 @@ func clearStoredVGPUDevice(stored *StoredMetadata) {
 func releaseStoredVGPU(ctx context.Context, stored *StoredMetadata) error {
 	path := storedVGPUDevicePath(stored)
 	if path != "" {
-		if err := devices.DestroyVGPU(ctx, stored.GPUFramework, path, stored.GPUMdevUUID); err != nil {
+		assignment := devices.VGPUAssignment{
+			Framework:  stored.GPUFramework,
+			DevicePath: path,
+			MdevUUID:   stored.GPUMdevUUID,
+		}
+		if err := devices.DestroyVGPU(ctx, assignment); err != nil {
 			return err
 		}
 	}
@@ -34,21 +38,26 @@ func releaseStoredVGPU(ctx context.Context, stored *StoredMetadata) error {
 
 // releaseRetainedVGPULocked releases a vGPU assignment retained on a stopped
 // instance after a failed release during the original stop. It is a no-op
-// when no assignment is retained. The caller must hold the instance lock.
-func (m *manager) releaseRetainedVGPULocked(ctx context.Context, id string) error {
+// when no assignment is retained, and a failed retry only logs so the
+// metadata stays for the next retry. The caller must hold the instance lock.
+func (m *manager) releaseRetainedVGPULocked(ctx context.Context, id string) {
+	log := logger.FromContext(ctx)
 	meta, err := m.loadMetadata(id)
 	if err != nil {
-		return err
+		log.WarnContext(ctx, "failed to load metadata for retained vGPU release", "instance_id", id, "error", err)
+		return
 	}
 	stored := &meta.StoredMetadata
 	if storedVGPUDevicePath(stored) == "" {
-		return nil
+		return
 	}
 	if err := releaseStoredVGPU(ctx, stored); err != nil {
-		logger.FromContext(ctx).ErrorContext(ctx, "failed to destroy retained vGPU; retaining assignment metadata", "instance_id", id, "error", err)
-		return fmt.Errorf("destroy vGPU: %w", err)
+		log.WarnContext(ctx, "failed to destroy retained vGPU; retaining assignment metadata", "instance_id", id, "error", err)
+		return
+	}
+	if err := m.saveMetadata(meta); err != nil {
+		log.WarnContext(ctx, "failed to save metadata after retained vGPU release", "instance_id", id, "error", err)
 	}
-	return m.saveMetadata(meta)
 }
 
 func storedVGPUDevicePath(stored *StoredMetadata) string {