diff --git a/lib/devices/vf_health_test.go b/lib/devices/vf_health_test.go index 7d1f8d479..02256e43d 100644 --- a/lib/devices/vf_health_test.go +++ b/lib/devices/vf_health_test.go @@ -3,10 +3,8 @@ package devices import ( "encoding/json" "errors" - "fmt" "os" "path/filepath" - "sync" "testing" "github.com/stretchr/testify/assert" @@ -328,7 +326,7 @@ func TestReportVFInitSuccessWithoutMatchingFailureClearsNothing(t *testing.T) { assert.False(t, result.Rescinded) } -func TestReportVFInitSuccessNoopFanoutDoesNotRetryFailedPersist(t *testing.T) { +func TestReportVFInitSuccessNoopDoesNotRetryFailedPersist(t *testing.T) { resetVFHealthStore(t) var syncCalls int vfHealth.mu.Lock() @@ -339,28 +337,12 @@ func TestReportVFInitSuccessNoopFanoutDoesNotRetryFailedPersist(t *testing.T) { } vfHealth.mu.Unlock() - var wg sync.WaitGroup - errs := make(chan error, 64) - for i := 0; i < 64; i++ { - wg.Add(1) - go func() { - defer wg.Done() - result, err := ReportVFInitSuccess(VFInitSuccessReport{ - VFAddress: "0000:e3:00.4", - InstanceID: "healthy-instance", - }) - if result != (VFSuccessResult{}) { - errs <- fmt.Errorf("unexpected result: %+v", result) - return - } - errs <- err - }() - } - wg.Wait() - close(errs) - for err := range errs { - require.NoError(t, err) - } + result, err := ReportVFInitSuccess(VFInitSuccessReport{ + VFAddress: "0000:e3:00.4", + InstanceID: "healthy-instance", + }) + require.NoError(t, err) + assert.Equal(t, VFSuccessResult{}, result) assert.Zero(t, syncCalls) assert.True(t, VFHealthStoreUnavailable()) diff --git a/lib/instances/process_identity.go b/lib/instances/process_identity.go index 48891092a..7bfee33c9 100644 --- a/lib/instances/process_identity.go +++ b/lib/instances/process_identity.go @@ -202,7 +202,6 @@ func classifyResolvedHypervisorOwner(socketPath string, stored, resolved int, er } // Ambiguous ownership is treated as live; this must not authorize teardown. -// The error is returned so callers can surface why teardown was suppressed. func hypervisorMayBeAlive(id HypervisorProcessIdentity, socketPath string) (bool, error) { pid, err := resolveLiveHypervisorPID(id, socketPath) return err != nil || pid > 0, err diff --git a/lib/instances/storage.go b/lib/instances/storage.go index f2112162d..085a5b6d8 100644 --- a/lib/instances/storage.go +++ b/lib/instances/storage.go @@ -193,9 +193,8 @@ func (m *manager) listMetadataFiles() ([]string, error) { } // listMetadataFilesStrict returns readable metadata paths and joins any stat -// errors other than absence. Fail-closed callers (the vGPU release claim scan -// and startup reconcile protection) use it so an unreadable instance is an -// error instead of silently missing. +// errors other than absence, so fail-closed callers treat an unreadable +// instance as an error instead of silently missing. func (m *manager) listMetadataFilesStrict() ([]string, error) { files, statErr, err := m.walkMetadataFiles() return files, errors.Join(statErr, err) diff --git a/lib/instances/vgpu_sentinel.go b/lib/instances/vgpu_sentinel.go index da709b335..44ef0afa2 100644 --- a/lib/instances/vgpu_sentinel.go +++ b/lib/instances/vgpu_sentinel.go @@ -238,24 +238,23 @@ func (c *VGPUSentinelController) recordCheck(ctx context.Context, result string) // confirmAssignment rejects a report only when the instance now holds a // different VF assignment. Released assignments remain attributable to the // assignment captured in the poll target. -func (c *VGPUSentinelController) confirmAssignment(ctx context.Context, target vgpuSentinelTarget) (bool, error) { +func (c *VGPUSentinelController) confirmAssignment(ctx context.Context, target vgpuSentinelTarget, action string) bool { current, ok, err := c.store.getVGPUSentinelTarget(ctx, target.instanceID) if err != nil { - return false, err + c.log.WarnContext(ctx, "vGPU sentinel could not confirm assignment; dropping report", + "action", action, "vf", target.vfAddress, "instance_id", target.instanceID, "error", err) + return false + } + if ok && (current.vfAddress != target.vfAddress || current.assignedAt != target.assignedAt) { + c.log.InfoContext(ctx, "vGPU sentinel skipping report: assignment changed during poll", + "action", action, "vf", target.vfAddress, "instance_id", target.instanceID) + return false } - return !ok || (current.vfAddress == target.vfAddress && current.assignedAt == target.assignedAt), nil + return true } func (c *VGPUSentinelController) handleFailure(ctx context.Context, target vgpuSentinelTarget, nvrm string) { - unchanged, err := c.confirmAssignment(ctx, target) - if err != nil { - c.log.WarnContext(ctx, "vGPU sentinel could not confirm assignment before recording an init failure", - "vf", target.vfAddress, "instance_id", target.instanceID, "error", err) - return - } - if !unchanged { - c.log.InfoContext(ctx, "vGPU sentinel skipping init failure: assignment changed during poll", - "vf", target.vfAddress, "instance_id", target.instanceID) + if !c.confirmAssignment(ctx, target, "init_failure") { return } result, err := c.reportFailure(devices.VFInitFailureReport{ @@ -292,13 +291,7 @@ func (c *VGPUSentinelController) handleFailure(ctx context.Context, target vgpuS } func (c *VGPUSentinelController) handleSuccess(ctx context.Context, target vgpuSentinelTarget) { - unchanged, err := c.confirmAssignment(ctx, target) - if err != nil { - c.log.WarnContext(ctx, "vGPU sentinel could not confirm assignment before clearing init failures", - "vf", target.vfAddress, "instance_id", target.instanceID, "error", err) - return - } - if !unchanged { + if !c.confirmAssignment(ctx, target, "init_success") { return } result, err := c.reportSuccess(devices.VFInitSuccessReport{ diff --git a/lib/instances/vgpu_sentinel_test.go b/lib/instances/vgpu_sentinel_test.go index bf049f3b2..a1851a900 100644 --- a/lib/instances/vgpu_sentinel_test.go +++ b/lib/instances/vgpu_sentinel_test.go @@ -68,14 +68,7 @@ func newTestSentinelController(t *testing.T, store *fakeSentinelStore) (*VGPUSen log: slog.New(slog.DiscardHandler), interval: time.Hour, repairHealthStore: func() error { return nil }, - // Mirrors the real store: repeated reports for the same assignment are - // deduplicated. reportFailure: func(report devices.VFInitFailureReport) (devices.VFReportResult, error) { - for _, previous := range reported { - if previous == report { - return devices.VFReportResult{Outcome: devices.VFReportUnchanged, Failures: 1, Threshold: 1}, nil - } - } reported = append(reported, report) return devices.VFReportResult{Outcome: devices.VFReportQuarantined, Failures: 1, Threshold: 1}, nil }, @@ -90,7 +83,7 @@ func newTestSentinelController(t *testing.T, store *fakeSentinelStore) (*VGPUSen return c, &reported } -func TestVGPUSentinelControllerReportsFailureOnce(t *testing.T) { +func TestVGPUSentinelControllerReportsFailure(t *testing.T) { t.Parallel() store := &fakeSentinelStore{targets: []vgpuSentinelTarget{{ @@ -99,6 +92,8 @@ func TestVGPUSentinelControllerReportsFailureOnce(t *testing.T) { assignedAt: "2026-08-20T15:00:00Z", }}} c, reported := newTestSentinelController(t, store) + var logs bytes.Buffer + c.log = slog.New(slog.NewTextHandler(&logs, nil)) ctx := context.Background() c.guestGPUInitStatus = func(context.Context, string) (guest.GPUInitState, string, error) { @@ -112,9 +107,8 @@ func TestVGPUSentinelControllerReportsFailureOnce(t *testing.T) { require.Len(t, *reported, 1) assert.Equal(t, "0000:e3:00.4", (*reported)[0].VFAddress) assert.Equal(t, "instance-1", (*reported)[0].InstanceID) - - c.pollOnce(ctx) - assert.Len(t, *reported, 1, "repeated polls of the same failed assignment must deduplicate") + assert.Contains(t, logs.String(), "quarantined wedged vGPU VF") + assert.Contains(t, logs.String(), "RmInitAdapter failed!") } func TestVGPUSentinelControllerSkipsUnreachableGuest(t *testing.T) { @@ -143,10 +137,7 @@ func TestVGPUSentinelControllerSkipsUnreachableGuest(t *testing.T) { func TestVGPUSentinelControllerRepairsHealthStoreOncePerPoll(t *testing.T) { t.Parallel() - targets := make([]vgpuSentinelTarget, vgpuSentinelMaxConcurrentPolls) - for i := range targets { - targets[i] = vgpuSentinelTarget{instanceID: fmt.Sprintf("instance-%d", i)} - } + targets := []vgpuSentinelTarget{{instanceID: "instance-1"}, {instanceID: "instance-2"}} c, _ := newTestSentinelController(t, &fakeSentinelStore{targets: targets}) c.guestGPUInitStatus = guestReportsOK var repairs int @@ -159,49 +150,6 @@ func TestVGPUSentinelControllerRepairsHealthStoreOncePerPoll(t *testing.T) { assert.Equal(t, 1, repairs) } -func TestVGPUSentinelControllerPollsTargetsConcurrently(t *testing.T) { - t.Parallel() - - store := &fakeSentinelStore{targets: []vgpuSentinelTarget{ - {instanceID: "instance-1"}, - {instanceID: "instance-2"}, - }} - c, _ := newTestSentinelController(t, store) - started := make(chan struct{}, len(store.targets)) - release := make(chan struct{}) - released := false - defer func() { - if !released { - close(release) - } - }() - c.guestGPUInitStatus = func(context.Context, string) (guest.GPUInitState, string, error) { - started <- struct{}{} - <-release - return guest.GPUInitState_GPU_INIT_STATE_UNKNOWN, "", nil - } - - done := make(chan struct{}) - go func() { - c.pollOnce(context.Background()) - close(done) - }() - for range store.targets { - select { - case <-started: - case <-time.After(time.Second): - t.Fatal("targets were not polled concurrently") - } - } - close(release) - released = true - select { - case <-done: - case <-time.After(time.Second): - t.Fatal("poll did not finish") - } -} - func TestVGPUSentinelControllerRecordsCheckResults(t *testing.T) { t.Parallel() @@ -302,8 +250,6 @@ func TestVGPUSentinelControllerRetriesFailedTallyClear(t *testing.T) { }}} c, _ := newTestSentinelController(t, store) c.guestGPUInitStatus = guestReportsOK - var logs bytes.Buffer - c.log = slog.New(slog.NewTextHandler(&logs, nil)) var reports []devices.VFInitSuccessReport c.reportSuccess = func(report devices.VFInitSuccessReport) (devices.VFSuccessResult, error) { reports = append(reports, report) @@ -315,9 +261,7 @@ func TestVGPUSentinelControllerRetriesFailedTallyClear(t *testing.T) { c.pollOnce(context.Background()) require.Len(t, reports, 1) - assert.Contains(t, logs.String(), "persist failed") - // The guest keeps reporting OK, so the next poll retries the clear. c.pollOnce(context.Background()) require.Len(t, reports, 2) assert.Equal(t, "0000:e3:00.4", reports[1].VFAddress) @@ -345,90 +289,59 @@ func TestVGPUSentinelControllerRetriesFailedQuarantine(t *testing.T) { assert.Len(t, *reported, 1) } -func TestVGPUSentinelControllerLogsNVRMMessageOnQuarantine(t *testing.T) { - t.Parallel() - - store := &fakeSentinelStore{targets: []vgpuSentinelTarget{{ - instanceID: "instance-1", - vfAddress: "0000:e3:00.4", - assignedAt: "2026-08-20T15:00:00Z", - }}} - c, _ := newTestSentinelController(t, store) - var logs bytes.Buffer - c.log = slog.New(slog.NewTextHandler(&logs, nil)) - - c.pollOnce(context.Background()) - assert.Contains(t, logs.String(), "quarantined wedged vGPU VF") - assert.Contains(t, logs.String(), "RmInitAdapter failed!") -} - -func TestVGPUSentinelControllerSkipsFailureOnChangedAssignment(t *testing.T) { +func TestVGPUSentinelControllerConfirmsAssignmentBeforeReporting(t *testing.T) { t.Parallel() - store := &fakeSentinelStore{targets: []vgpuSentinelTarget{{ - instanceID: "instance-1", - vfAddress: "0000:e3:00.5", - assignedAt: "2026-08-21T00:00:10Z", - }}} - c, reported := newTestSentinelController(t, store) - stale := vgpuSentinelTarget{ instanceID: "instance-1", vfAddress: "0000:e3:00.4", assignedAt: "2026-08-21T00:00:00Z", } - c.pollTarget(context.Background(), stale) - assert.Empty(t, *reported) - - // A released assignment (instance gone) remains attributable. - store.targets = nil - c.pollTarget(context.Background(), stale) - require.Len(t, *reported, 1) - assert.Equal(t, "0000:e3:00.4", (*reported)[0].VFAddress) -} - -func TestVGPUSentinelControllerSkipsInitOKOnChangedAssignment(t *testing.T) { - t.Parallel() - - store := &fakeSentinelStore{targets: []vgpuSentinelTarget{{ + changed := []vgpuSentinelTarget{{ instanceID: "instance-1", vfAddress: "0000:e3:00.5", assignedAt: "2026-08-21T00:00:10Z", - }}} - c, _ := newTestSentinelController(t, store) - c.guestGPUInitStatus = guestReportsOK - var cleared []devices.VFInitSuccessReport - c.reportSuccess = func(report devices.VFInitSuccessReport) (devices.VFSuccessResult, error) { - cleared = append(cleared, report) - return devices.VFSuccessResult{Cleared: 1}, nil + }} + tests := []struct { + name string + guestState func(context.Context, string) (guest.GPUInitState, string, error) + targets []vgpuSentinelTarget + wantApplied bool + }{ + {"failure skipped when the assignment changed", guestReportsFailed, changed, false}, + // A released assignment (instance gone) remains attributable. + {"failure from a released assignment is reported", guestReportsFailed, nil, true}, + {"init OK skipped when the assignment changed", guestReportsOK, changed, false}, + {"init OK from a released assignment clears", guestReportsOK, nil, true}, } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + c, reported := newTestSentinelController(t, &fakeSentinelStore{targets: tt.targets}) + c.guestGPUInitStatus = tt.guestState + var cleared []devices.VFInitSuccessReport + c.reportSuccess = func(report devices.VFInitSuccessReport) (devices.VFSuccessResult, error) { + cleared = append(cleared, report) + return devices.VFSuccessResult{Cleared: 1}, nil + } - c.pollTarget(context.Background(), vgpuSentinelTarget{ - instanceID: "instance-1", - vfAddress: "0000:e3:00.4", - assignedAt: "2026-08-21T00:00:00Z", - }) - assert.Empty(t, cleared) -} - -func TestVGPUSentinelControllerAppliesInitOKFromReleasedAssignment(t *testing.T) { - t.Parallel() + c.pollTarget(context.Background(), stale) - c, _ := newTestSentinelController(t, &fakeSentinelStore{}) - c.guestGPUInitStatus = guestReportsOK - var cleared []devices.VFInitSuccessReport - c.reportSuccess = func(report devices.VFInitSuccessReport) (devices.VFSuccessResult, error) { - cleared = append(cleared, report) - return devices.VFSuccessResult{Cleared: 1}, nil + if !tt.wantApplied { + assert.Empty(t, *reported) + assert.Empty(t, cleared) + return + } + if len(*reported) == 1 { + assert.Equal(t, stale.vfAddress, (*reported)[0].VFAddress) + assert.Equal(t, stale.assignedAt, (*reported)[0].AssignedAt) + assert.Empty(t, cleared) + return + } + require.Len(t, cleared, 1) + assert.Equal(t, stale.vfAddress, cleared[0].VFAddress) + assert.Equal(t, stale.assignedAt, cleared[0].AssignedAt) + }) } - - c.pollTarget(context.Background(), vgpuSentinelTarget{ - instanceID: "instance-1", - vfAddress: "0000:e3:00.4", - assignedAt: "2026-08-21T00:00:00Z", - }) - require.Len(t, cleared, 1) - assert.Equal(t, "2026-08-21T00:00:00Z", cleared[0].AssignedAt) } func TestGetVGPUSentinelTargetSkipsRetentionStub(t *testing.T) { diff --git a/lib/system/guest_agent/gpu_watch.go b/lib/system/guest_agent/gpu_watch.go index 811b5cef8..db0f53752 100644 --- a/lib/system/guest_agent/gpu_watch.go +++ b/lib/system/guest_agent/gpu_watch.go @@ -83,6 +83,16 @@ func (r *gpuInitReporter) state() (pb.GPUInitState, string) { } } +func (r *gpuInitReporter) reportSuccess() { + r.mu.Lock() + defer r.mu.Unlock() + if r.succeeded { + return + } + r.succeeded = true + log.Printf("[guest-agent] GPU driver initialized") +} + // GetGPUInitStatus reports the GPU driver init state to the host sentinel. // The serial console is shared with workload output, so this vsock channel is // the only signal the host trusts. @@ -95,16 +105,6 @@ func (s *guestServer) GetGPUInitStatus(context.Context, *pb.GetGPUInitStatusRequ return &pb.GetGPUInitStatusResponse{State: state, FailureMessage: msg}, nil } -func (r *gpuInitReporter) reportSuccess() { - r.mu.Lock() - defer r.mu.Unlock() - if r.succeeded { - return - } - r.succeeded = true - log.Printf("[guest-agent] GPU driver initialized") -} - func watchGPUInitFailure(reporter *gpuInitReporter) { firstOpen := true for { diff --git a/lib/system/guest_agent/gpu_watch_test.go b/lib/system/guest_agent/gpu_watch_test.go index c9af4f0cb..242838df0 100644 --- a/lib/system/guest_agent/gpu_watch_test.go +++ b/lib/system/guest_agent/gpu_watch_test.go @@ -90,27 +90,6 @@ func TestProbeGPUInitRetriesAfterAttemptTimeout(t *testing.T) { assert.Equal(t, pb.GPUInitState_GPU_INIT_STATE_OK, state) } -func TestProbeGPUInitSkipsWithoutNvidiaSMI(t *testing.T) { - buf := captureAgentLog(t) - t.Setenv("PATH", t.TempDir()) - - probeGPUInit(&gpuInitReporter{}) - - assert.Empty(t, buf.String()) -} - -func TestGPUInitReporterMakesSuccessTerminal(t *testing.T) { - captureAgentLog(t) - reporter := &gpuInitReporter{} - reporter.reportFailure("NVRM: GPU 0000:00:03.0: RmInitAdapter failed!") - reporter.reportSuccess() - reporter.reportFailure("NVRM: GPU 0000:00:03.0: RmInitAdapter failed!") - - state, msg := reporter.state() - assert.Equal(t, pb.GPUInitState_GPU_INIT_STATE_OK, state) - assert.Empty(t, msg) -} - func TestGPUInitReporterState(t *testing.T) { captureAgentLog(t) @@ -136,6 +115,11 @@ func TestGPUInitReporterState(t *testing.T) { require.NoError(t, err) assert.Equal(t, pb.GPUInitState_GPU_INIT_STATE_OK, resp.State) assert.Empty(t, resp.FailureMessage) + + reporter.reportFailure("NVRM: GPU 0000:00:03.0: RmInitAdapter failed! (0x22:0x65:884)") + state, msg := reporter.state() + assert.Equal(t, pb.GPUInitState_GPU_INIT_STATE_OK, state, "success is terminal; a later failure must not replace it") + assert.Empty(t, msg) } func TestRunGPUProbeAttemptKillsAndReapsAfterDeadline(t *testing.T) {