diff --git a/lib/devices/mdev_darwin.go b/lib/devices/mdev_darwin.go index 8ec67db4..1427a509 100644 --- a/lib/devices/mdev_darwin.go +++ b/lib/devices/mdev_darwin.go @@ -52,9 +52,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 } diff --git a/lib/devices/types.go b/lib/devices/types.go index fd717d83..809d669f 100644 --- a/lib/devices/types.go +++ b/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 diff --git a/lib/devices/vgpu_linux.go b/lib/devices/vgpu_linux.go index 429e9988..eaf210b4 100644 --- a/lib/devices/vgpu_linux.go +++ b/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) } diff --git a/lib/instances/create.go b/lib/instances/create.go index 6bdf620e..98410afe 100644 --- a/lib/instances/create.go +++ b/lib/instances/create.go @@ -297,7 +297,12 @@ func (m *manager) createInstance( // Add vGPU cleanup to stack cu.Add(func() { log.DebugContext(ctx, "destroying vGPU on cleanup", "instance_id", id, "uuid", gpuDevice.MdevUUID) - 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, "uuid", gpuDevice.MdevUUID, "error", err) } }) diff --git a/lib/instances/delete.go b/lib/instances/delete.go index 586e9ad8..067a76b9 100644 --- a/lib/instances/delete.go +++ b/lib/instances/delete.go @@ -85,6 +85,21 @@ func (m *manager) deleteInstanceWithOptions( guest.CloseConn(dialer.Key()) } + // 3b. Block the restart policy before any teardown. If the delete fails + // partway (e.g. a failed vGPU release) the metadata is retained with the + // VMM already stopped, and without this marker the restart policy + // controller would start the instance again. + if err := m.markRestartManualStopLocked(ctx, id); err != nil { + return fmt.Errorf("block restart policy before delete: %w", err) + } + // markRestartManualStopLocked persists through a separate metadata load. + // Reload it so later saves in this delete do not overwrite the block. + meta, err = m.loadMetadata(id) + if err != nil { + return fmt.Errorf("reload metadata after blocking restart policy: %w", err) + } + stored = &meta.StoredMetadata + // 4. If active, try graceful guest shutdown before force kill. gracefulShutdown := false if !options.skipGracefulShutdown && (inst.State == StateRunning || inst.State == StateInitializing) { @@ -125,6 +140,25 @@ func (m *manager) deleteInstanceWithOptions( } m.closeFirecrackerUFFDSession(ctx, stored) + // 5b. Release the vGPU assignment if present, before any network, device, + // or volume teardown. A failed release retains the instance metadata; the + // VMM has already been stopped, but its attachments are intact and the + // restart policy is blocked, so a retried delete is safe. + hadVGPUAssignment := storedVGPUDevicePath(stored) != "" + if hadVGPUAssignment { + log.InfoContext(ctx, "destroying vGPU", "instance_id", id, "uuid", stored.GPUMdevUUID) + } + if err := releaseStoredVGPU(ctx, stored); err != nil { + log.ErrorContext(ctx, "failed to destroy vGPU; retaining instance metadata", "instance_id", id, "uuid", stored.GPUMdevUUID, "error", err) + return fmt.Errorf("destroy vGPU: %w", err) + } + if hadVGPUAssignment { + if err := m.saveMetadata(meta); err != nil { + log.ErrorContext(ctx, "failed to save metadata after vGPU release", "instance_id", id, "error", err) + return fmt.Errorf("save metadata after vGPU release: %w", err) + } + } + // 6. Release network allocation if inst.NetworkEnabled { m.unregisterEgressProxyInstance(ctx, id) @@ -170,15 +204,6 @@ func (m *manager) deleteInstanceWithOptions( } } - // 7c. Release the vGPU assignment if present. - if storedVGPUDevicePath(stored) != "" { - log.InfoContext(ctx, "destroying vGPU", "instance_id", id, "uuid", stored.GPUMdevUUID) - if err := releaseStoredVGPU(ctx, stored); err != nil { - // Log error but continue with cleanup - log.WarnContext(ctx, "failed to destroy vGPU, continuing with cleanup", "instance_id", id, "uuid", stored.GPUMdevUUID, "error", err) - } - } - // 8. Delete all instance data log.DebugContext(ctx, "deleting instance data", "instance_id", id) _, dataSpanEnd := m.startLifecycleStep(ctx, "delete_instance_data", diff --git a/lib/instances/fork.go b/lib/instances/fork.go index ea6d3a4c..7354b3ed 100644 --- a/lib/instances/fork.go +++ b/lib/instances/fork.go @@ -298,6 +298,11 @@ func (m *manager) forkInstanceFromStoppedOrStandby(ctx context.Context, id strin // phase (Standby for snapshot forks, Stopped for stopped forks) will be // recorded by the appropriate operation when the fork is acted on. forkMeta.Phases.Reset() + // A vGPU assignment is never shared with a fork: normally stop already + // released it, and an assignment retained by a failed release must stay + // with the source so only one instance retries it. The fork acquires its + // own vGPU on start from GPUProfile. + clearStoredVGPUDevice(&forkMeta) switch source.State { case StateStandby: forkMeta.Phases.Record(phasetracking.PhaseStandby, now) diff --git a/lib/instances/fork_test.go b/lib/instances/fork_test.go index 2763eab4..d9cace3f 100644 --- a/lib/instances/fork_test.go +++ b/lib/instances/fork_test.go @@ -17,6 +17,7 @@ import ( "time" "github.com/kernel/hypeman/lib/autostandby" + "github.com/kernel/hypeman/lib/devices" "github.com/kernel/hypeman/lib/guest" "github.com/kernel/hypeman/lib/healthcheck" "github.com/kernel/hypeman/lib/hypervisor" @@ -29,6 +30,39 @@ import ( "github.com/stretchr/testify/require" ) +func TestForkInstanceClearsVGPUAssignment(t *testing.T) { + manager, _ := setupTestManager(t) + ctx := context.Background() + hvType := hypervisor.Type("fork-vgpu-test") + hypervisor.RegisterCapabilities(hvType, hypervisor.Capabilities{SupportsConcurrentForkPrepare: true}) + manager.vmStarters[hvType] = concurrentForkPrepareTestStarter{} + + sourceID := "fork-vgpu-source" + createStoppedSnapshotSourceFixture(t, manager, sourceID, sourceID, hvType) + + // A retained assignment (release failed during stop) must stay with the + // source; the fork keeps only the profile and acquires its own vGPU on + // start. + meta, err := manager.loadMetadata(sourceID) + require.NoError(t, err) + meta.GPUProfile = "NVIDIA L40S-2Q" + meta.GPUFramework = devices.VGPUFramework("future-framework") + meta.GPUDevicePath = "/sys/bus/pci/devices/0000:82:00.4" + meta.GPUMdevUUID = "retained-uuid" + require.NoError(t, manager.saveMetadata(meta)) + + forked, err := manager.ForkInstance(ctx, sourceID, ForkInstanceRequest{Name: "fork-vgpu-copy"}) + require.NoError(t, err) + assert.Equal(t, "NVIDIA L40S-2Q", forked.GPUProfile) + assert.Equal(t, devices.VGPUFrameworkNone, forked.GPUFramework) + assert.Empty(t, forked.GPUDevicePath) + assert.Empty(t, forked.GPUMdevUUID) + + source, err := manager.loadMetadata(sourceID) + require.NoError(t, err) + assert.Equal(t, "/sys/bus/pci/devices/0000:82:00.4", source.GPUDevicePath) +} + func TestForkInstance_VZStoppedSourceSupported(t *testing.T) { t.Parallel() manager, _ := setupTestManager(t) diff --git a/lib/instances/lifecycle_noop_test.go b/lib/instances/lifecycle_noop_test.go index 5ca7515f..f65694a1 100644 --- a/lib/instances/lifecycle_noop_test.go +++ b/lib/instances/lifecycle_noop_test.go @@ -9,8 +9,10 @@ import ( "testing" "time" + "github.com/kernel/hypeman/lib/devices" "github.com/kernel/hypeman/lib/hypervisor" "github.com/kernel/hypeman/lib/paths" + restartpolicy "github.com/kernel/hypeman/lib/restart-policy" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -147,6 +149,178 @@ func TestLifecycleNoopStandbyWithOptionsStillRejectsStandbyInstance(t *testing.T assertNoLifecycleEvent(t, events) } +func TestDeleteRetainsMetadataWhenVGPUReleaseFails(t *testing.T) { + m, id := newLifecycleNoopManagerWithInstance(t, StateStopped, time.Now().UTC()) + meta, err := m.loadMetadata(id) + require.NoError(t, err) + meta.GPUProfile = "NVIDIA L40S-2Q" + meta.GPUFramework = devices.VGPUFramework("future-framework") + meta.GPUDevicePath = "/sys/bus/pci/devices/0000:82:00.4" + require.NoError(t, m.saveMetadata(meta)) + + err = m.DeleteInstance(context.Background(), id) + require.Error(t, err) + + stored, err := m.loadMetadata(id) + require.NoError(t, err) + assert.Equal(t, devices.VGPUFramework("future-framework"), stored.GPUFramework) + assert.Equal(t, "/sys/bus/pci/devices/0000:82:00.4", stored.GPUDevicePath) +} + +func TestDeleteBlocksRestartPolicyWhenVGPUReleaseFails(t *testing.T) { + m, id := newLifecycleNoopManagerWithInstance(t, StateStopped, time.Now().UTC()) + meta, err := m.loadMetadata(id) + require.NoError(t, err) + meta.RestartPolicy = &restartpolicy.Policy{Policy: restartpolicy.PolicyAlways} + meta.GPUProfile = "NVIDIA L40S-2Q" + meta.GPUFramework = devices.VGPUFramework("future-framework") + meta.GPUDevicePath = "/sys/bus/pci/devices/0000:82:00.4" + require.NoError(t, m.saveMetadata(meta)) + + err = m.DeleteInstance(context.Background(), id) + require.Error(t, err) + + stored, err := m.loadMetadata(id) + require.NoError(t, err) + assert.Equal(t, restartpolicy.BlockedReasonManualStop, stored.RestartStatus.BlockedReason, + "a failed delete must not leave the instance restartable") +} + +func TestDeletePersistsVGPUReleaseBeforeTeardown(t *testing.T) { + m, id := newLifecycleNoopManagerWithInstance(t, StateStopped, time.Now().UTC()) + var persisted *metadata + deviceManager := &recordingDeviceManager{ + onMarkDetached: func() { + var err error + persisted, err = m.loadMetadata(id) + require.NoError(t, err) + }, + } + m.deviceManager = deviceManager + meta, err := m.loadMetadata(id) + require.NoError(t, err) + meta.RestartPolicy = &restartpolicy.Policy{Policy: restartpolicy.PolicyAlways} + meta.GPUProfile = "NVIDIA L40S-2Q" + meta.GPUDevicePath = "/sys/bus/mdev/devices/test-mdev" + meta.GPUMdevUUID = "test-mdev" + meta.Devices = []string{"dev-1"} + require.NoError(t, m.saveMetadata(meta)) + + require.NoError(t, m.DeleteInstance(context.Background(), id)) + require.NotNil(t, persisted) + assert.Empty(t, persisted.GPUDevicePath) + assert.Empty(t, persisted.GPUMdevUUID) + assert.Equal(t, "NVIDIA L40S-2Q", persisted.GPUProfile) + assert.Equal(t, restartpolicy.BlockedReasonManualStop, persisted.RestartStatus.BlockedReason) +} + +func TestDeleteReleasesVGPUBeforeTeardown(t *testing.T) { + m, id := newLifecycleNoopManagerWithInstance(t, StateStopped, time.Now().UTC()) + deviceManager := &recordingDeviceManager{} + m.deviceManager = deviceManager + meta, err := m.loadMetadata(id) + require.NoError(t, err) + meta.GPUProfile = "NVIDIA L40S-2Q" + meta.GPUFramework = devices.VGPUFramework("future-framework") + meta.GPUDevicePath = "/sys/bus/pci/devices/0000:82:00.4" + meta.Devices = []string{"dev-1"} + require.NoError(t, m.saveMetadata(meta)) + + err = m.DeleteInstance(context.Background(), id) + require.Error(t, err) + assert.ErrorContains(t, err, "destroy vGPU") + assert.Empty(t, deviceManager.detached) + assert.Empty(t, deviceManager.unbound) + + stored, err := m.loadMetadata(id) + require.NoError(t, err) + assert.Equal(t, devices.VGPUFramework("future-framework"), stored.GPUFramework) +} + +// A stale release during start must be persisted immediately: if start fails +// later (here at vGPU recreation on a host without VFs), the on-disk metadata +// must no longer point at the already-released device. +func TestStartPersistsStaleVGPUReleaseImmediately(t *testing.T) { + m, id := newLifecycleNoopManagerWithInstance(t, StateStopped, time.Now().UTC()) + m.imageManager = readyFixtureImageManager{name: "test-image"} + meta, err := m.loadMetadata(id) + require.NoError(t, err) + meta.GPUProfile = "NVIDIA L40S-2Q" + meta.HypervisorType = hypervisor.TypeQEMU + meta.GPUFramework = devices.VGPUFrameworkNone + meta.GPUDevicePath = "/sys/bus/pci/devices/0000:82:00.4" + require.NoError(t, m.saveMetadata(meta)) + + _, err = m.StartInstance(context.Background(), id, StartInstanceRequest{}) + require.Error(t, err) + + stored, err := m.loadMetadata(id) + require.NoError(t, err) + assert.Empty(t, stored.GPUDevicePath, "released assignment should be persisted despite the failed start") + assert.Equal(t, "NVIDIA L40S-2Q", stored.GPUProfile, "profile is kept for the next start") +} + +func TestStopStoppedInstanceReleasesRetainedVGPU(t *testing.T) { + m, id := newLifecycleNoopManagerWithInstance(t, StateStopped, time.Now().UTC()) + meta, err := m.loadMetadata(id) + require.NoError(t, err) + meta.GPUProfile = "NVIDIA L40S-2Q" + meta.GPUFramework = devices.VGPUFrameworkNone + meta.GPUDevicePath = "/sys/bus/pci/devices/0000:82:00.4" + require.NoError(t, m.saveMetadata(meta)) + + 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) + assert.Empty(t, stored.GPUDevicePath) +} + +func TestStopStoppedInstanceVGPUReleaseFailureRemainsNoop(t *testing.T) { + m, id := newLifecycleNoopManagerWithInstance(t, StateStopped, time.Now().UTC()) + meta, err := m.loadMetadata(id) + require.NoError(t, err) + meta.GPUProfile = "NVIDIA L40S-2Q" + meta.GPUFramework = devices.VGPUFramework("future-framework") + meta.GPUDevicePath = "/sys/bus/pci/devices/0000:82:00.4" + require.NoError(t, m.saveMetadata(meta)) + + 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) + assert.Equal(t, devices.VGPUFramework("future-framework"), stored.GPUFramework) + assert.Equal(t, "/sys/bus/pci/devices/0000:82:00.4", stored.GPUDevicePath) +} + +// recordingDeviceManager is a devices.Manager stub that records passthrough +// teardown calls. Only the methods delete exercises are implemented. +type recordingDeviceManager struct { + devices.Manager + detached []string + unbound []string + onMarkDetached func() +} + +func (m *recordingDeviceManager) MarkDetached(ctx context.Context, deviceID string) error { + m.detached = append(m.detached, deviceID) + if m.onMarkDetached != nil { + m.onMarkDetached() + } + return nil +} + +func (m *recordingDeviceManager) UnbindFromVFIO(ctx context.Context, id string) error { + m.unbound = append(m.unbound, id) + return nil +} + func newLifecycleNoopManagerWithInstance(t *testing.T, state State, now time.Time) (*manager, string) { t.Helper() diff --git a/lib/instances/manager.go b/lib/instances/manager.go index 8e8e25f3..85f75975 100644 --- a/lib/instances/manager.go +++ b/lib/instances/manager.go @@ -621,6 +621,12 @@ func (m *manager) StopInstance(ctx context.Context, id string) (*Instance, error if err := m.markRestartManualStopLocked(ctx, id); err != nil { return nil, err } + // 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. 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 diff --git a/lib/instances/snapshot.go b/lib/instances/snapshot.go index 71bc562d..57dcd9e1 100644 --- a/lib/instances/snapshot.go +++ b/lib/instances/snapshot.go @@ -305,6 +305,12 @@ func (m *manager) restoreSnapshot(ctx context.Context, id string, snapshotID str restored.StoppedAt = nil restored.ExitCode = nil restored.ExitMessage = "" + // vGPU assignments are live host state, not snapshot payload: keep the + // instance's current assignment (possibly retained from a failed release) + // instead of resurrecting the one embedded in the snapshot. + restored.GPUFramework = sourceMeta.GPUFramework + restored.GPUDevicePath = sourceMeta.GPUDevicePath + restored.GPUMdevUUID = sourceMeta.GPUMdevUUID restored.HypervisorType = targetHypervisor restored.HypervisorVersion = targetHypervisorVersion restored.SocketPath = m.paths.InstanceSocket(id, starter.SocketName()) @@ -433,6 +439,7 @@ func (m *manager) forkSnapshot(ctx context.Context, snapshotID string, req ForkS forkMeta.ExitCode = nil forkMeta.ExitMessage = "" forkMeta.RestartStatus = restartpolicy.Status{} + clearStoredVGPUDevice(&forkMeta) forkMeta.FirecrackerUFFDSessionID = "" forkMeta.FirecrackerUFFDPagerVersion = "" forkMeta.FirecrackerUseUFFDOnNextRestore = useFirecrackerUFFDOnNextRestore(targetHypervisor, rec.Snapshot.Kind == SnapshotKindStandby, targetState) diff --git a/lib/instances/snapshot_test.go b/lib/instances/snapshot_test.go index ff3a7e8f..c2bef2ef 100644 --- a/lib/instances/snapshot_test.go +++ b/lib/instances/snapshot_test.go @@ -8,6 +8,7 @@ import ( "testing" "time" + "github.com/kernel/hypeman/lib/devices" "github.com/kernel/hypeman/lib/hypervisor" "github.com/kernel/hypeman/lib/images" snapshotstore "github.com/kernel/hypeman/lib/snapshot" @@ -15,6 +16,118 @@ import ( "github.com/stretchr/testify/require" ) +func TestForkSnapshotClearsVGPUAssignment(t *testing.T) { + mgr, _ := setupTestManager(t) + ctx := context.Background() + + sourceID := "snapshot-vgpu-source" + createStoppedSnapshotSourceFixture(t, mgr, sourceID, sourceID, mgr.defaultHypervisor) + + meta, err := mgr.loadMetadata(sourceID) + require.NoError(t, err) + meta.GPUProfile = "NVIDIA L40S-2Q" + meta.GPUFramework = devices.VGPUFramework("future-framework") + meta.GPUDevicePath = "/sys/bus/pci/devices/0000:82:00.4" + meta.GPUMdevUUID = "retained-uuid" + require.NoError(t, mgr.saveMetadata(meta)) + + snapshot, err := mgr.CreateSnapshot(ctx, sourceID, CreateSnapshotRequest{ + Kind: SnapshotKindStopped, + Name: "snapshot-vgpu", + }) + require.NoError(t, err) + + forked, err := mgr.ForkSnapshot(ctx, snapshot.Id, ForkSnapshotRequest{ + Name: "snapshot-vgpu-fork", + TargetState: StateStopped, + }) + require.NoError(t, err) + assert.Equal(t, "NVIDIA L40S-2Q", forked.GPUProfile) + assert.Equal(t, devices.VGPUFrameworkNone, forked.GPUFramework) + assert.Empty(t, forked.GPUDevicePath) + assert.Empty(t, forked.GPUMdevUUID) + + source, err := mgr.loadMetadata(sourceID) + require.NoError(t, err) + assert.Equal(t, "/sys/bus/pci/devices/0000:82:00.4", source.GPUDevicePath) +} + +func TestRestoreSnapshotDoesNotResurrectStaleVGPUAssignment(t *testing.T) { + mgr, _ := setupTestManager(t) + ctx := context.Background() + + sourceID := "snapshot-vgpu-restore-stale" + createStoppedSnapshotSourceFixture(t, mgr, sourceID, sourceID, mgr.defaultHypervisor) + + meta, err := mgr.loadMetadata(sourceID) + require.NoError(t, err) + meta.GPUProfile = "NVIDIA L40S-2Q" + meta.GPUFramework = devices.VGPUFramework("future-framework") + meta.GPUDevicePath = "/sys/bus/pci/devices/0000:82:00.4" + meta.GPUMdevUUID = "retained-uuid" + require.NoError(t, mgr.saveMetadata(meta)) + + snapshot, err := mgr.CreateSnapshot(ctx, sourceID, CreateSnapshotRequest{ + Kind: SnapshotKindStopped, + Name: "snapshot-vgpu-restore-stale", + }) + require.NoError(t, err) + + // The retained assignment is released successfully after the snapshot + // was taken; a restore must not resurrect the snapshot's embedded copy. + meta, err = mgr.loadMetadata(sourceID) + require.NoError(t, err) + clearStoredVGPUDevice(&meta.StoredMetadata) + require.NoError(t, mgr.saveMetadata(meta)) + + _, err = mgr.RestoreSnapshot(ctx, sourceID, snapshot.Id, RestoreSnapshotRequest{ + TargetState: StateStopped, + TargetHypervisor: mgr.defaultHypervisor, + }) + require.NoError(t, err) + + restored, err := mgr.loadMetadata(sourceID) + require.NoError(t, err) + assert.Equal(t, devices.VGPUFrameworkNone, restored.GPUFramework) + assert.Empty(t, restored.GPUDevicePath) + assert.Empty(t, restored.GPUMdevUUID) +} + +func TestRestoreSnapshotKeepsCurrentVGPUAssignment(t *testing.T) { + mgr, _ := setupTestManager(t) + ctx := context.Background() + + sourceID := "snapshot-vgpu-restore-retained" + createStoppedSnapshotSourceFixture(t, mgr, sourceID, sourceID, mgr.defaultHypervisor) + + snapshot, err := mgr.CreateSnapshot(ctx, sourceID, CreateSnapshotRequest{ + Kind: SnapshotKindStopped, + Name: "snapshot-vgpu-restore-retained", + }) + require.NoError(t, err) + + // An assignment retained after the snapshot was taken (e.g. from a + // failed release on stop) must survive the restore for the next retry. + meta, err := mgr.loadMetadata(sourceID) + require.NoError(t, err) + meta.GPUFramework = devices.VGPUFramework("future-framework") + meta.GPUDevicePath = "/sys/bus/pci/devices/0000:82:00.4" + meta.GPUMdevUUID = "retained-uuid" + require.NoError(t, mgr.saveMetadata(meta)) + + _, err = mgr.RestoreSnapshot(ctx, sourceID, snapshot.Id, RestoreSnapshotRequest{ + TargetState: StateStopped, + TargetHypervisor: mgr.defaultHypervisor, + }) + require.NoError(t, err) + + restored, err := mgr.loadMetadata(sourceID) + require.NoError(t, err) + assert.Equal(t, devices.VGPUFramework("future-framework"), restored.GPUFramework) + assert.Equal(t, "/sys/bus/pci/devices/0000:82:00.4", restored.GPUDevicePath) + assert.Equal(t, "retained-uuid", restored.GPUMdevUUID) +} + func TestStoppedSnapshotLifecycleAndForkAfterSourceDeletion(t *testing.T) { t.Parallel() mgr, _ := setupTestManager(t) diff --git a/lib/instances/start.go b/lib/instances/start.go index e66db89a..7e7855ea 100644 --- a/lib/instances/start.go +++ b/lib/instances/start.go @@ -49,6 +49,21 @@ func (m *manager) startInstance( return nil, fmt.Errorf("%w: cannot start from state %s, must be Stopped", ErrInvalidState, inst.State) } + // Release any assignment retained by an earlier failed release and + // persist the cleared fields immediately, so a failure later in start + // cannot leave on-disk metadata pointing at a device that is already + // gone (matching releaseRetainedVGPULocked). + if storedVGPUDevicePath(stored) != "" { + if err := releaseStoredVGPU(ctx, stored); err != nil { + log.ErrorContext(ctx, "failed to release stale vGPU before start", "instance_id", id, "error", err) + return nil, fmt.Errorf("release stale vGPU before start: %w", err) + } + if err := m.saveMetadata(meta); err != nil { + log.ErrorContext(ctx, "failed to save metadata after stale vGPU release", "instance_id", id, "error", err) + return nil, fmt.Errorf("save metadata after stale vGPU release: %w", err) + } + } + // 2a. Clear stale exit info from previous run and apply command overrides stored.ExitCode = nil stored.ExitMessage = "" @@ -158,7 +173,12 @@ func (m *manager) startInstance( // Add vGPU cleanup to stack cu.Add(func() { log.DebugContext(ctx, "destroying vGPU on cleanup", "instance_id", id, "uuid", device.MdevUUID) - 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, "uuid", device.MdevUUID, "error", err) } }) diff --git a/lib/instances/stop.go b/lib/instances/stop.go index ec03a3fc..162391db 100644 --- a/lib/instances/stop.go +++ b/lib/instances/stop.go @@ -267,8 +267,7 @@ func (m *manager) stopInstance( log.InfoContext(ctx, "destroying vGPU on stop", "instance_id", id, "uuid", stored.GPUMdevUUID) if err := releaseStoredVGPU(ctx, stored); err != nil { // Log error but continue - vGPU cleanup is best-effort - log.WarnContext(ctx, "failed to destroy vGPU on stop", "instance_id", id, "uuid", stored.GPUMdevUUID, "error", err) - clearStoredVGPUDevice(stored) + log.WarnContext(ctx, "failed to destroy vGPU on stop; retaining assignment metadata", "instance_id", id, "uuid", stored.GPUMdevUUID, "error", err) } } diff --git a/lib/instances/vgpu.go b/lib/instances/vgpu.go index c2294ac5..cffe2ac1 100644 --- a/lib/instances/vgpu.go +++ b/lib/instances/vgpu.go @@ -5,6 +5,7 @@ import ( "path/filepath" "github.com/kernel/hypeman/lib/devices" + "github.com/kernel/hypeman/lib/logger" ) func setStoredVGPUDevice(stored *StoredMetadata, device *devices.VGPUDevice) { @@ -22,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 } } @@ -30,6 +36,30 @@ func releaseStoredVGPU(ctx context.Context, stored *StoredMetadata) error { return nil } +// 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, 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 { + log.WarnContext(ctx, "failed to load metadata for retained vGPU release", "instance_id", id, "error", err) + return + } + stored := &meta.StoredMetadata + if storedVGPUDevicePath(stored) == "" { + return + } + if err := releaseStoredVGPU(ctx, stored); err != nil { + 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) + } +} + func storedVGPUDevicePath(stored *StoredMetadata) string { if stored.GPUDevicePath != "" { return stored.GPUDevicePath