Quarantine unhealthy vGPU VFs via a persisted health store - #462
Conversation
182853a to
34d27c2
Compare
-->
✱ stlc build✅ go code · compare
✅ python code · compare
✅ typescript code · compare
Diagnostics: ❗ 0 new / 1 total error, 💡 0 new / 5 total note
Build metadata
This comment is auto-generated by stlc and is kept up to date as you push. |
|
Cleanup pass (deslop / test dedup), no behavior changes:
The persist-failure/rollback/retry test family was left intact — each pins a distinct branch fixed by Verified: |
94f8496 to
5a8fcae
Compare
5104254 to
bf0fd62
Compare
296b483 to
ddb1b28
Compare
|
Follow-up pass on the review nits, no behavior change to placement or quarantine semantics:
Verified: |
46155c9 to
cc6b625
Compare
|
made some changes since pr 321 changed signfiicantly from the last review will request a re review |
sjmiller609
left a comment
There was a problem hiding this comment.
re-reviewed. adjustments look good
2ca3cb5 to
c1cb504
Compare
Add a VF health store persisted at <data-dir>/gpu/vf-health.json: init failures reported against a VF are tallied per instance assignment, and once failures accumulate from gpu.vf_quarantine_threshold distinct assignments (default 2) the VF is quarantined. Quarantined VFs are excluded from placement and advertised profile availability, cards with quarantined VFs are deprioritized, and selection among equivalent free VFs is randomized. An exact-assignment success report clears the match and older tallies and rescinds that assignment's quarantine. An unreadable or invalid state file fails closed: mutations are refused, placement and advertised availability are disabled, and loads are retried after repair. Writes fsync before and after the rename. GET /resources reports allocatable_slots and quarantined_slots, and GPU admission gates on the allocatable count. GPU.md documents the store semantics, draining the parent GPU, the SR-IOV recovery cycle, and clearing quarantine state. (cherry picked from commit f0b0e11)
A failed state write previously rolled memory back and left the store reporting healthy, so a VF whose threshold-crossing failure could not be persisted stayed allocatable. Latch write failures and refuse placement until a later write succeeds; re-reported markers retry the write. Also make acknowledged reports crash-durable (fsync the parent when the state dir is first created, treat directory sync failures as persist failures instead of logging success), and re-evaluate persisted tallies against the configured threshold at load and on threshold changes so a lowered gpu.vf_quarantine_threshold applies to existing failures. (cherry picked from commit def61de)
(cherry picked from commit 4527e64)
(cherry picked from commit 54f03c4)
(cherry picked from commit 294405a)
Fold the below-threshold placement assertion into TestVGPUAvailability and the repaired-state recovery assertion into TestVGPUAvailabilityFailsWhenStoreUnavailable, exercising both through the public API. Drop TestReportVFInitFailureRespectsConfiguredThreshold and TestCheckedAddressesFailsClosedOnUnloadedState, whose remaining coverage is subsumed by the threshold re-evaluation and invalid-record tests. (cherry picked from commit bf0fd62)
Placement for vendor VFIO vGPUs lives in the instance manager's claim path, so quarantine exclusion moves there: selectVendorVFIOVF drops quarantined VFs from the candidate set, counts them against their parent GPU so placement drifts away from cards carrying a wedged VF, and picks uniformly at random among equally ranked clean VFs instead of always taking the lowest PCI address. Claims fail closed while the VF health store is unavailable. The quarantine set is read under the allocation lock but mutated under the devices lock, so configure re-checks it under that lock before touching the VF. This replaces the selection-time lock coupling the original design had. Each persisted claim now records GPUClaimedAt. The health store keys failure and success reports on (instance, assignment), and this field is the assignment identity the detection path will report.
…ormat Load the VF health store from main instead of as a side effect of devices.NewManager, so the lifecycle is visible next to the threshold config and tests can point the store at a path directly. Add FormatVFAssignedAt so failure and success reports agree on the AssignedAt key. Drop the two exported getters only tests used, log a single warning when the store is unavailable in /resources, and document why the address pattern and parent-dir sync look the way they do.
A failed write latched the store closed, and only a guest report could retry it. Closed placement produces no new reports, so a transient disk error wedged vGPU placement until restart. Retry the write when placement or /resources reads the quarantine set.
GetVGPUAvailability now returns a snapshot that ListGPUProfilesWithVFs consumes, so a /resources read touches the VF health store once instead of twice. SetVFQuarantineThreshold and the load path return the persist error from threshold re-evaluation instead of logging it, so InitVFHealth reports the failure and startup logs it. Document why re-evaluation keeps the in-memory quarantine on a failed write, align the rescind rule comments with the code, and note why the claim time is set outside setStoredVGPUDevice.
Passing the threshold to InitVFHealth removes the ordering dependency between configuring it and loading persisted tallies. The load error for a duplicate failure now names both halves of the assignment key.
persistenced holds the GPU open like DCGM does, and the runbook dropped it when the quiesce step was expanded.
GetFullStatus dropped the GPU status error, so a broken VF health state was indistinguishable from a full host in /resources. The response now carries placement_disabled_reason when allocatable_slots is 0 for that reason, and the hypeman_resources_gpu_slots gauge exports allocatable and quarantined counts alongside total and used.
The GPU status provider walks sysfs and may retry a failed VF health write, which fsyncs. Fetch it before the manager lock so a slow disk cannot stall CPU or memory admission.
getVGPUStatus already knows why allocatable capacity could not be determined, so record the reason on the status there instead of copying the struct in GetFullStatus. The error still reaches admission. Drop the warning on every status read; the reason is visible in /resources and the admission error, and a broken store would otherwise log on every metrics tick.
kind=allocatable reads 0 both when the host is full and when the VF health store is unavailable. hypeman_resources_gpu_placement_disabled separates the two so the runbook condition is alertable from metrics alone.
Callers no longer hand back a VGPUAvailability just so the function can read an unexported field from it. The set is exported on the snapshot and the profile listing takes it explicitly.
Production only sets the threshold through InitVFHealth. The tests keep a helper that changes it on the loaded store and re-evaluates tallies.
selectVendorVFIOVF took six positional arguments, two of them nilable, and claimVGPU repeated the full list at each of its three call sites. The host state it chooses from now travels as one value that the retry paths update in place.
c1cb504 to
9aea9c4
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 9aea9c4. Configure here.
A success matching an older assignment on a quarantined VF returned without changing the record, which contradicts the documented semantics: the match and every older tally are cleared, and only the rescind is withheld unless the match is the newest failure.

Summary
Bottom half of the wedged-VF work (detection lands separately on top, in #435). This PR adds the quarantine machinery: a persisted VF health store plus the placement, admission, and API changes that act on it. Nothing in this PR reports failures automatically yet — the store is fed by the guest-side detection in #435, and by operators editing the state file per the runbook.
<data-dir>/gpu/vf-health.jsonper instance assignment; a VF is quarantined once failures accumulate fromgpu.vf_quarantine_thresholddistinct assignments (default 2). An exact-assignment success report clears the match and every older tally. It rescinds the quarantine only when the matched failure is the most recent one recorded, i.e. the report that crossed the threshold.GET /resourcesreportsallocatable_slotsandquarantined_slots; GPU admission gates on the allocatable count. While the health store is unavailable,allocatable_slotsis 0 andplacement_disabled_reasoncarries the load or write error, so a broken state file is distinguishable from a full host. Thehypeman_resources_gpu_slotsgauge gainskind=allocatableandkind=quarantined, andhypeman_resources_gpu_placement_disabledis 1 while the store is unavailable so the condition is alertable from metrics alone.GPUClaimedAt. The health store keys reports on (instance, assignment), and this is the assignment identity Detect wedged vGPU VFs from the guest and report to the health store #435 will report.devices.InitVFHealth(path, threshold)takes the configured threshold directly, so loaded tallies are always evaluated against it without an ordering dependency on a separate setter.Rebuilt on the claim-first allocator
This branch was rebuilt from
mainafter #321 merged as the claim-first rewrite. The health store, config, paths, resources, API, and docs commits are the originally reviewed ones, replayed. Placement is the part that changed:selectVendorVFIOVFinlib/instances/vgpu.go, which is where placement lives onmain. The devices-layerselectLeastLoadedVFandreconcilehunks from the earlier revision are gone with the code they patched.configurere-checks quarantine under that lock before writing to the VF, so a VF quarantined between selection and configuration fails the claim instead of being handed to a guest.configuregained tests for the quarantine and store-unavailable refusals.Safety and failure handling
lib/devices/GPU.mddocuments the store semantics, draining the parent GPU, the SR-IOV recovery cycle (including quiescingnvidia-persistencedalongside DCGM), and clearing quarantine state.Testing
Passed locally:
The full
lib/instancesandcmd/api/apisuites were not run here; tests that boot real VM images or pull images need a host this environment does not provide.lib/oapi/oapi.gowas regenerated withmake oapi-generate, not merged.Note
Medium Risk
Changes vGPU placement, admission, and fail-closed behavior when VF health state is corrupt or unwritable; incorrect quarantine or store outages can block all new vGPU allocations until recovery.
Overview
Adds persisted VF health (
<data-dir>/gpu/vf-health.json) and the plumbing to act on it when guest driver init failures stack up. Configgpu.vf_quarantine_threshold(default 2) controls how many distinct instance assignments must fail before a VF is quarantined; tallies are re-evaluated on load when the threshold changes.Placement and capacity: Quarantined VFs are dropped from vendor-VFIO profile counts and from
selectVendorVFIOVF(GPUs with any quarantined VF sort behind clean cards; ties among equivalent free VFs are randomized).configurere-checks quarantine under the devices lock. Vendor VFIO claims persistGPUClaimedAtas the assignment key for future failure/success reports (fed by a follow-up PR or manual state edits).API and admission:
GET /resourcesGPU status gainsallocatable_slots,quarantined_slots, and optionalplacement_disabled_reason. GPU admission and pending reservations gate on allocatable slots, nottotal_slots - used_slots. Invalid or unwritable health state fails closed (no new vGPU placement until load/write recovers).Ops: Startup calls
devices.InitVFHealth; metrics add allocatable/quarantined slot kinds andhypeman_resources_gpu_placement_disabled. Docs and example config describe the store and runbook.Reviewed by Cursor Bugbot for commit 7cc1a45. Bugbot is set up for automated code reviews on this repo. Configure here.