Restore delete's log-and-continue contract on failed vGPU release

Delete goes back to the pre-refactor behavior: a failed vGPU release is
logged and teardown continues, instead of failing the delete and
retaining the instance. The release-side guards still never destroy a
device they cannot prove is unowned, so continuing only tolerates a
leaked slot until startup reconciliation recovers it - the same
tradeoff the mdev path always made. The restart-policy block before
teardown stays: a delete can still fail earlier when the hypervisor
cannot be confirmed dead.

Author: yummybomb

lib/instances/delete.go

@@ -86,9 +86,9 @@ func (m *manager) deleteInstanceWithOptions(
 	}
 
 	// 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.
+	// partway (e.g. the hypervisor cannot be confirmed dead) 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)
 	}
@@ -141,21 +141,21 @@ 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.
+	// or volume teardown. Release failure is logged and the delete continues,
+	// matching the pre-refactor contract: the VMM is already confirmed dead,
+	// the guards inside the release never destroy a device they cannot prove
+	// is unowned, and a skipped release is recovered by startup
+	// reconciliation.
 	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 {
+		// Log error but continue with cleanup.
+		log.WarnContext(ctx, "failed to destroy vGPU, continuing with cleanup", "instance_id", id, "uuid", stored.GPUMdevUUID, "error", err)
+	} else 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)
+			log.WarnContext(ctx, "failed to save metadata after vGPU release", "instance_id", id, "error", err)
 		}
 	}
 

lib/instances/lifecycle_noop_test.go

@@ -149,7 +149,7 @@ func TestLifecycleNoopStandbyWithOptionsStillRejectsStandbyInstance(t *testing.T
 	assertNoLifecycleEvent(t, events)
 }
 
-func TestDeleteRetainsMetadataWhenVGPUReleaseFails(t *testing.T) {
+func TestDeleteContinuesWhenVGPUReleaseFails(t *testing.T) {
 	m, id := newLifecycleNoopManagerWithInstance(t, StateStopped, time.Now().UTC())
 	meta, err := m.loadMetadata(id)
 	require.NoError(t, err)
@@ -158,32 +158,13 @@ func TestDeleteRetainsMetadataWhenVGPUReleaseFails(t *testing.T) {
 	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)
+	// A failed release is logged and the delete continues, matching the
+	// pre-refactor contract; the leaked assignment is recovered by startup
+	// reconciliation.
+	require.NoError(t, m.DeleteInstance(context.Background(), id))
 
-	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")
+	_, err = m.loadMetadata(id)
+	require.Error(t, err, "instance data must be deleted despite the failed release")
 }
 
 func TestDeletePersistsVGPUReleaseBeforeTeardown(t *testing.T) {
@@ -214,7 +195,7 @@ func TestDeletePersistsVGPUReleaseBeforeTeardown(t *testing.T) {
 	assert.Equal(t, restartpolicy.BlockedReasonManualStop, persisted.RestartStatus.BlockedReason)
 }
 
-func TestDeleteReleasesVGPUBeforeTeardown(t *testing.T) {
+func TestDeleteContinuesTeardownAfterFailedVGPURelease(t *testing.T) {
 	m, id := newLifecycleNoopManagerWithInstance(t, StateStopped, time.Now().UTC())
 	deviceManager := &recordingDeviceManager{}
 	m.deviceManager = deviceManager
@@ -226,15 +207,13 @@ func TestDeleteReleasesVGPUBeforeTeardown(t *testing.T) {
 	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)
+	// The failed release must not block the rest of the teardown: devices
+	// are detached and the instance is fully deleted.
+	require.NoError(t, m.DeleteInstance(context.Background(), id))
+	assert.Equal(t, []string{"dev-1"}, deviceManager.detached)
 
-	stored, err := m.loadMetadata(id)
-	require.NoError(t, err)
-	assert.Equal(t, devices.VGPUFramework("future-framework"), stored.GPUFramework)
+	_, err = m.loadMetadata(id)
+	require.Error(t, err, "instance data must be deleted despite the failed release")
 }
 
 // A stale release during start must be persisted immediately: if start fails