From 1dc6c8ecae9bf157cd78ba06a9da573b6362ef29 Mon Sep 17 00:00:00 2001 From: yummybomb <19238148+yummybomb@users.noreply.github.com> Date: Thu, 17 Sep 2026 15:18:34 +0000 Subject: [PATCH 1/3] Wait for exiting VMM tasks and retain failed vGPU cleanup --- lib/instances/delete.go | 3 +- lib/instances/lifecycle_noop_test.go | 73 ++++++++++++++------ lib/instances/process_exit_linux_test.go | 86 ++++++++++++++++++++++++ lib/instances/process_identity.go | 15 ++++- 4 files changed, 154 insertions(+), 23 deletions(-) create mode 100644 lib/instances/process_exit_linux_test.go diff --git a/lib/instances/delete.go b/lib/instances/delete.go index d59475a0f..36db53385 100644 --- a/lib/instances/delete.go +++ b/lib/instances/delete.go @@ -150,7 +150,8 @@ func (m *manager) deleteInstanceWithOptions( log.InfoContext(ctx, "destroying vGPU", "instance_id", id, "uuid", stored.GPUMdevUUID) } if err := m.releaseStoredVGPUPersisted(ctx, meta); err != nil { - log.WarnContext(ctx, "failed to destroy vGPU, continuing with cleanup; the next allocation repairs the VF before reuse", "instance_id", id, "uuid", stored.GPUMdevUUID, "error", err) + log.WarnContext(ctx, "failed to destroy vGPU; retaining instance metadata for retry", "instance_id", id, "uuid", stored.GPUMdevUUID, "error", err) + return fmt.Errorf("release vGPU: %w", err) } // 6. Release network allocation diff --git a/lib/instances/lifecycle_noop_test.go b/lib/instances/lifecycle_noop_test.go index 4a0f76dcf..53f353371 100644 --- a/lib/instances/lifecycle_noop_test.go +++ b/lib/instances/lifecycle_noop_test.go @@ -3,9 +3,11 @@ package instances import ( "context" "errors" + "fmt" "os" "path/filepath" "sync" + "syscall" "testing" "time" @@ -177,25 +179,58 @@ func TestDeletePersistsVGPUReleaseBeforeTeardown(t *testing.T) { assert.Equal(t, restartpolicy.BlockedReasonManualStop, persisted.RestartStatus.BlockedReason) } -func TestDeleteContinuesTeardownAfterFailedVGPURelease(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)) - - // 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) - - _, err = m.loadMetadata(id) - require.Error(t, err, "instance data must be deleted despite the failed release") +func TestDeleteRetainsFailedVGPUReleaseForRetry(t *testing.T) { + for name, releaseErr := range map[string]error{"scan failure": syscall.EBADF, "driver busy": syscall.EPERM} { + for _, reconcile := range []bool{false, true} { + t.Run(fmt.Sprintf("%s/reconcile=%t", name, reconcile), func(t *testing.T) { + m, id := newLifecycleNoopManagerWithInstance(t, StateStopped, time.Now().UTC()) + deviceManager := &recordingDeviceManager{} + m.deviceManager = deviceManager + m.destroyVGPU = func(context.Context, devices.VGPUAssignment) error { return releaseErr } + meta, err := m.loadMetadata(id) + require.NoError(t, err) + meta.RestartPolicy = &restartpolicy.Policy{Policy: restartpolicy.PolicyAlways} + meta.GPUProfile = "NVIDIA L40S-2Q" + meta.GPUFramework = devices.VGPUFrameworkVendorVFIO + meta.GPUDevicePath = "/sys/bus/pci/devices/0000:82:00.4" + meta.Devices = []string{"dev-1"} + require.NoError(t, m.saveMetadata(meta)) + + require.ErrorIs(t, m.DeleteInstance(t.Context(), id), releaseErr) + assert.Empty(t, deviceManager.detached) + retained, err := m.loadMetadata(id) + require.NoError(t, err) + assert.Equal(t, meta.GPUDevicePath, retained.GPUDevicePath) + assert.Equal(t, meta.GPUFramework, retained.GPUFramework) + assert.Equal(t, restartpolicy.BlockedReasonManualStop, retained.RestartStatus.BlockedReason) + + if reconcile { + // A new manager must be able to recover the claim from disk. + restarted := &manager{ + paths: m.paths, + destroyVGPU: m.destroyVGPU, + reconcileVGPUDevices: func(context.Context, map[string]struct{}) error { return nil }, + } + restarted.ReconcileVGPUs(t.Context()) + retained, err = restarted.loadMetadata(id) + require.NoError(t, err) + assert.Equal(t, meta.GPUDevicePath, retained.GPUDevicePath) + restarted.destroyVGPU = func(context.Context, devices.VGPUAssignment) error { return nil } + restarted.ReconcileVGPUs(t.Context()) + retained, err = restarted.loadMetadata(id) + require.NoError(t, err) + assert.Empty(t, retained.GPUDevicePath) + assert.Equal(t, restartpolicy.BlockedReasonManualStop, retained.RestartStatus.BlockedReason) + } else { + m.destroyVGPU = func(context.Context, devices.VGPUAssignment) error { return nil } + } + require.NoError(t, m.DeleteInstance(t.Context(), id)) + assert.Equal(t, []string{"dev-1"}, deviceManager.detached) + _, err = m.loadMetadata(id) + require.ErrorIs(t, err, ErrNotFound) + }) + } + } } // A stale release during start must be persisted immediately: if start fails diff --git a/lib/instances/process_exit_linux_test.go b/lib/instances/process_exit_linux_test.go new file mode 100644 index 000000000..62f958266 --- /dev/null +++ b/lib/instances/process_exit_linux_test.go @@ -0,0 +1,86 @@ +//go:build linux + +package instances + +import ( + "bufio" + "fmt" + "os" + "os/exec" + "runtime" + "strconv" + "syscall" + "testing" + "time" + + "github.com/stretchr/testify/require" +) + +func init() { + if os.Getenv("HYPEMAN_TEST_EXIT_LEADER") != "1" { + return + } + // init runs on the startup thread. Exit only that thread, leaving a worker + // alive to model a zombie QEMU leader with a vhost task still closing VFIO. + runtime.LockOSThread() + go func() { + fmt.Fprintln(os.Stdout, "ready") + _, _ = os.Stdin.Read(make([]byte, 1)) + os.Exit(0) + }() + syscall.Syscall(syscall.SYS_EXIT, 0, 0, 0) +} + +func TestProcessExitWaitsForZombieLeadersTasks(t *testing.T) { + child := exec.Command(os.Args[0], "-test.run=^$") + child.Env = append(os.Environ(), "HYPEMAN_TEST_EXIT_LEADER=1") + stdin, err := child.StdinPipe() + require.NoError(t, err) + stdout, err := child.StdoutPipe() + require.NoError(t, err) + require.NoError(t, child.Start()) + t.Cleanup(func() { + _ = stdin.Close() + _ = child.Process.Kill() + _ = child.Wait() + }) + _, err = bufio.NewReader(stdout).ReadString('\n') + require.NoError(t, err) + pid := child.Process.Pid + require.Eventually(t, func() bool { + state, err := readLinuxProcessState(pid) + return err == nil && state == "Z" + }, 5*time.Second, 10*time.Millisecond) + + require.True(t, ProcessExists(pid), "a zombie leader does not imply its tasks have exited") + require.False(t, WaitForProcessExit(pid, 50*time.Millisecond)) + // A sibling process gets ECHILD from wait4, as the API does after restart. + observer := exec.Command(os.Args[0], "-test.run=^TestProcessExitObserver$") + observer.Env = append(os.Environ(), "HYPEMAN_TEST_WAIT_PID="+strconv.Itoa(pid)) + output, err := observer.CombinedOutput() + require.NoError(t, err, "%s", output) + + require.NoError(t, stdin.Close()) + require.True(t, WaitForProcessExit(pid, 5*time.Second)) + require.False(t, ProcessExists(pid)) +} + +func TestProcessExitObserver(t *testing.T) { + value := os.Getenv("HYPEMAN_TEST_WAIT_PID") + if value == "" { + return + } + pid, err := strconv.Atoi(value) + require.NoError(t, err) + var status syscall.WaitStatus + _, err = syscall.Wait4(pid, &status, syscall.WNOHANG, nil) + require.ErrorIs(t, err, syscall.ECHILD) + require.False(t, WaitForProcessExit(pid, 50*time.Millisecond)) + identity := HypervisorProcessIdentity{} + identity.Set(pid) + resolved, err := resolveLiveHypervisorPID(identity, "") + require.NoError(t, err) + require.Equal(t, pid, resolved) + m := &manager{} + require.True(t, m.vgpuHypervisorMayBeAlive(t.Context(), &StoredMetadata{HypervisorProcessIdentity: identity})) +} diff --git a/lib/instances/process_identity.go b/lib/instances/process_identity.go index 936adf1e7..6d8cf2305 100644 --- a/lib/instances/process_identity.go +++ b/lib/instances/process_identity.go @@ -136,7 +136,7 @@ func resolveLiveHypervisorPID(id HypervisorProcessIdentity, socketPath string) ( if ProcessExists(*id.HypervisorPID) { stored = *id.HypervisorPID } else { - // ProcessExists treats zombies as dead, so a direct-child VMM that + // ProcessExists treats fully exited zombies as dead, so a direct-child VMM that // exited on its own never reaches the Wait4 in WaitForProcessExit // and would sit unreaped. Reap it here: WNOHANG leaves a live child // untouched, and a recycled or non-child PID fails with ECHILD. @@ -201,7 +201,7 @@ func classifyResolvedHypervisorOwner(socketPath string, stored, resolved int, er return 0, fmt.Errorf("cannot confirm ownership of socket %s: %w", socketPath, err) } -// ProcessExists reports whether pid belongs to a live, non-zombie process. +// ProcessExists includes exiting processes whose non-leader tasks still hold resources. func ProcessExists(pid int) bool { if pid <= 0 { return false @@ -217,7 +217,16 @@ func ProcessExists(pid int) bool { if err != nil { return true } - return state != "Z" + if state != "Z" { + return true + } + // The leader can be a zombie while a vhost task is still releasing VFIO. + // Treat the whole group as alive until only the zombie leader remains. + tasks, err := os.ReadDir(filepath.Join("/proc", strconv.Itoa(pid), "task")) + if err != nil { + return !os.IsNotExist(err) + } + return len(tasks) > 1 } func readLinuxProcessState(pid int) (string, error) { From 3e738240729d33a2d945a8e79f9267a9e5ee5f17 Mon Sep 17 00:00:00 2001 From: yummybomb <19238148+yummybomb@users.noreply.github.com> Date: Thu, 17 Sep 2026 16:53:46 +0000 Subject: [PATCH 2/3] Drop redundant errno subtests and fix stale ProcessExists comments --- lib/instances/delete.go | 2 +- lib/instances/lifecycle_noop_test.go | 94 ++++++++++---------- lib/instances/process_identity.go | 3 +- lib/instances/process_identity_linux_test.go | 4 +- 4 files changed, 51 insertions(+), 52 deletions(-) diff --git a/lib/instances/delete.go b/lib/instances/delete.go index 36db53385..e1e0bb012 100644 --- a/lib/instances/delete.go +++ b/lib/instances/delete.go @@ -150,7 +150,7 @@ func (m *manager) deleteInstanceWithOptions( log.InfoContext(ctx, "destroying vGPU", "instance_id", id, "uuid", stored.GPUMdevUUID) } if err := m.releaseStoredVGPUPersisted(ctx, meta); err != nil { - log.WarnContext(ctx, "failed to destroy vGPU; retaining instance metadata for retry", "instance_id", id, "uuid", stored.GPUMdevUUID, "error", err) + log.ErrorContext(ctx, "failed to destroy vGPU; retaining instance metadata for retry", "instance_id", id, "uuid", stored.GPUMdevUUID, "error", err) return fmt.Errorf("release vGPU: %w", err) } diff --git a/lib/instances/lifecycle_noop_test.go b/lib/instances/lifecycle_noop_test.go index 53f353371..375dab05a 100644 --- a/lib/instances/lifecycle_noop_test.go +++ b/lib/instances/lifecycle_noop_test.go @@ -7,7 +7,6 @@ import ( "os" "path/filepath" "sync" - "syscall" "testing" "time" @@ -180,56 +179,55 @@ func TestDeletePersistsVGPUReleaseBeforeTeardown(t *testing.T) { } func TestDeleteRetainsFailedVGPUReleaseForRetry(t *testing.T) { - for name, releaseErr := range map[string]error{"scan failure": syscall.EBADF, "driver busy": syscall.EPERM} { - for _, reconcile := range []bool{false, true} { - t.Run(fmt.Sprintf("%s/reconcile=%t", name, reconcile), func(t *testing.T) { - m, id := newLifecycleNoopManagerWithInstance(t, StateStopped, time.Now().UTC()) - deviceManager := &recordingDeviceManager{} - m.deviceManager = deviceManager - m.destroyVGPU = func(context.Context, devices.VGPUAssignment) error { return releaseErr } - meta, err := m.loadMetadata(id) - require.NoError(t, err) - meta.RestartPolicy = &restartpolicy.Policy{Policy: restartpolicy.PolicyAlways} - meta.GPUProfile = "NVIDIA L40S-2Q" - meta.GPUFramework = devices.VGPUFrameworkVendorVFIO - meta.GPUDevicePath = "/sys/bus/pci/devices/0000:82:00.4" - meta.Devices = []string{"dev-1"} - require.NoError(t, m.saveMetadata(meta)) - - require.ErrorIs(t, m.DeleteInstance(t.Context(), id), releaseErr) - assert.Empty(t, deviceManager.detached) - retained, err := m.loadMetadata(id) + releaseErr := errors.New("reset failed") + for _, reconcile := range []bool{false, true} { + t.Run(fmt.Sprintf("reconcile=%t", reconcile), func(t *testing.T) { + m, id := newLifecycleNoopManagerWithInstance(t, StateStopped, time.Now().UTC()) + deviceManager := &recordingDeviceManager{} + m.deviceManager = deviceManager + m.destroyVGPU = func(context.Context, devices.VGPUAssignment) error { return releaseErr } + meta, err := m.loadMetadata(id) + require.NoError(t, err) + meta.RestartPolicy = &restartpolicy.Policy{Policy: restartpolicy.PolicyAlways} + meta.GPUProfile = "NVIDIA L40S-2Q" + meta.GPUFramework = devices.VGPUFrameworkVendorVFIO + meta.GPUDevicePath = "/sys/bus/pci/devices/0000:82:00.4" + meta.Devices = []string{"dev-1"} + require.NoError(t, m.saveMetadata(meta)) + + require.ErrorIs(t, m.DeleteInstance(t.Context(), id), releaseErr) + assert.Empty(t, deviceManager.detached) + retained, err := m.loadMetadata(id) + require.NoError(t, err) + assert.Equal(t, meta.GPUDevicePath, retained.GPUDevicePath) + assert.Equal(t, meta.GPUFramework, retained.GPUFramework) + assert.Equal(t, restartpolicy.BlockedReasonManualStop, retained.RestartStatus.BlockedReason) + + if reconcile { + // A new manager must be able to recover the claim from disk. + restarted := &manager{ + paths: m.paths, + destroyVGPU: m.destroyVGPU, + reconcileVGPUDevices: func(context.Context, map[string]struct{}) error { return nil }, + } + restarted.ReconcileVGPUs(t.Context()) + retained, err = restarted.loadMetadata(id) require.NoError(t, err) assert.Equal(t, meta.GPUDevicePath, retained.GPUDevicePath) - assert.Equal(t, meta.GPUFramework, retained.GPUFramework) + restarted.destroyVGPU = func(context.Context, devices.VGPUAssignment) error { return nil } + restarted.ReconcileVGPUs(t.Context()) + retained, err = restarted.loadMetadata(id) + require.NoError(t, err) + assert.Empty(t, retained.GPUDevicePath) assert.Equal(t, restartpolicy.BlockedReasonManualStop, retained.RestartStatus.BlockedReason) - - if reconcile { - // A new manager must be able to recover the claim from disk. - restarted := &manager{ - paths: m.paths, - destroyVGPU: m.destroyVGPU, - reconcileVGPUDevices: func(context.Context, map[string]struct{}) error { return nil }, - } - restarted.ReconcileVGPUs(t.Context()) - retained, err = restarted.loadMetadata(id) - require.NoError(t, err) - assert.Equal(t, meta.GPUDevicePath, retained.GPUDevicePath) - restarted.destroyVGPU = func(context.Context, devices.VGPUAssignment) error { return nil } - restarted.ReconcileVGPUs(t.Context()) - retained, err = restarted.loadMetadata(id) - require.NoError(t, err) - assert.Empty(t, retained.GPUDevicePath) - assert.Equal(t, restartpolicy.BlockedReasonManualStop, retained.RestartStatus.BlockedReason) - } else { - m.destroyVGPU = func(context.Context, devices.VGPUAssignment) error { return nil } - } - require.NoError(t, m.DeleteInstance(t.Context(), id)) - assert.Equal(t, []string{"dev-1"}, deviceManager.detached) - _, err = m.loadMetadata(id) - require.ErrorIs(t, err, ErrNotFound) - }) - } + } else { + m.destroyVGPU = func(context.Context, devices.VGPUAssignment) error { return nil } + } + require.NoError(t, m.DeleteInstance(t.Context(), id)) + assert.Equal(t, []string{"dev-1"}, deviceManager.detached) + _, err = m.loadMetadata(id) + require.ErrorIs(t, err, ErrNotFound) + }) } } diff --git a/lib/instances/process_identity.go b/lib/instances/process_identity.go index 6d8cf2305..3c2feba2f 100644 --- a/lib/instances/process_identity.go +++ b/lib/instances/process_identity.go @@ -201,7 +201,8 @@ func classifyResolvedHypervisorOwner(socketPath string, stored, resolved int, er return 0, fmt.Errorf("cannot confirm ownership of socket %s: %w", socketPath, err) } -// ProcessExists includes exiting processes whose non-leader tasks still hold resources. +// ProcessExists reports whether pid belongs to a live process. A zombie +// leader still counts as live while other tasks in its thread group remain. func ProcessExists(pid int) bool { if pid <= 0 { return false diff --git a/lib/instances/process_identity_linux_test.go b/lib/instances/process_identity_linux_test.go index eec3a0acf..35c70822d 100644 --- a/lib/instances/process_identity_linux_test.go +++ b/lib/instances/process_identity_linux_test.go @@ -44,8 +44,8 @@ func TestResolveLiveHypervisorPIDWithoutStoredPID(t *testing.T) { // TestResolveLiveHypervisorPIDReapsZombieChild guards against leaking one // zombie per direct-child VMM that exits on its own: ProcessExists treats -// zombies as dead, so the confirmed-gone paths in stop, delete, and standby -// never reach the Wait4 in WaitForProcessExit. +// fully exited zombies as dead, so the confirmed-gone paths in stop, delete, +// and standby never reach the Wait4 in WaitForProcessExit. func TestResolveLiveHypervisorPIDReapsZombieChild(t *testing.T) { child := exec.Command("true") require.NoError(t, child.Start()) From 5036425e1b6e97266937bad29ae5b0bdbb2f654e Mon Sep 17 00:00:00 2001 From: yummybomb <19238148+yummybomb@users.noreply.github.com> Date: Thu, 17 Sep 2026 17:10:25 +0000 Subject: [PATCH 3/3] Name the re-exec observer as a helper and explain the retried delete --- lib/instances/lifecycle_noop_test.go | 2 ++ lib/instances/process_exit_linux_test.go | 4 ++-- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/lib/instances/lifecycle_noop_test.go b/lib/instances/lifecycle_noop_test.go index 375dab05a..cb11eefae 100644 --- a/lib/instances/lifecycle_noop_test.go +++ b/lib/instances/lifecycle_noop_test.go @@ -223,6 +223,8 @@ func TestDeleteRetainsFailedVGPUReleaseForRetry(t *testing.T) { } else { m.destroyVGPU = func(context.Context, devices.VGPUAssignment) error { return nil } } + // In the reconcile case m.destroyVGPU still fails; the retried delete + // succeeds because the claim was already cleared on disk. require.NoError(t, m.DeleteInstance(t.Context(), id)) assert.Equal(t, []string{"dev-1"}, deviceManager.detached) _, err = m.loadMetadata(id) diff --git a/lib/instances/process_exit_linux_test.go b/lib/instances/process_exit_linux_test.go index 62f958266..0b075a6a1 100644 --- a/lib/instances/process_exit_linux_test.go +++ b/lib/instances/process_exit_linux_test.go @@ -55,7 +55,7 @@ func TestProcessExitWaitsForZombieLeadersTasks(t *testing.T) { require.True(t, ProcessExists(pid), "a zombie leader does not imply its tasks have exited") require.False(t, WaitForProcessExit(pid, 50*time.Millisecond)) // A sibling process gets ECHILD from wait4, as the API does after restart. - observer := exec.Command(os.Args[0], "-test.run=^TestProcessExitObserver$") + observer := exec.Command(os.Args[0], "-test.run=^TestProcessExitObserverHelper$") observer.Env = append(os.Environ(), "HYPEMAN_TEST_WAIT_PID="+strconv.Itoa(pid)) output, err := observer.CombinedOutput() require.NoError(t, err, "%s", output) @@ -65,7 +65,7 @@ func TestProcessExitWaitsForZombieLeadersTasks(t *testing.T) { require.False(t, ProcessExists(pid)) } -func TestProcessExitObserver(t *testing.T) { +func TestProcessExitObserverHelper(t *testing.T) { value := os.Getenv("HYPEMAN_TEST_WAIT_PID") if value == "" { return