fix(roles): ask the driver before writing a vGPU profile - #76
Conversation
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe restore script now uses one helper to check host VGPU mode before VF and profile stages. Non-SR-IOV cards are logged and skipped successfully. Tests cover declined cards and profile-only declarations, and the README documents the behavior. ChangesSR-IOV mode handling
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant VFProfileStage
participant refuse_unless_sriov
participant NVIDIADriver
VFProfileStage->>refuse_unless_sriov: validate declared card mode
refuse_unless_sriov->>NVIDIADriver: query host_vgpu_mode
NVIDIADriver-->>refuse_unless_sriov: return observed mode
refuse_unless_sriov-->>VFProfileStage: allow SR-IOV or decline stage
Merge Risk: ⚪ Minimal · up to Non-SR-IOV cards are skipped without changing virtual functions or profiles, while supported cards continue through the existing stages. No actionable merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
IvanHunters
left a comment
IvanHunters
left a comment
There was a problem hiding this comment.
LGTM.
This fixes the real cause, not a symptom. On the base revision only the virtual-function stage asked the driver for Host VGPU Mode; the profile stage leaned on the cross-stage VF_STAGE_DECLINED flag instead. That flag was never set for a card declared with vgpu_profile alone, and it was only read when the card had zero functions. So current_vgpu_type could still land on a legacy-mdev card by two routes: a profile-only declaration the VF stage never renders for, and an sriov card that already exposes functions. Folding the mode check into one refuse_unless_sriov helper that each stage calls for itself, and deleting the flag, drops the ordering dependency rather than papering over it.
What I checked:
- The helper only reads (
nvidia-smi -q,log,return), so re-runs don't flip state. The profile write path (current == profileshort-circuit) is untouched, and thechanged=0second run holds. - Both call sites wrap the helper in an
if, so the "may proceed"return 1works and errexit can't abort on a card the stage was cleared to act on. This also quietly closes a latent base-revision bug: the baremode="$(...)"assignment could kill the whole run on ahead -n 1SIGPIPE underpipefail. indexis now read fromresolve_gpuand declaredlocalinensure_vf_profiles, so nothing leaks and the mode query hits the right-iindex in both stages.- Nothing changes for non-vGPU or undeclared hosts. The preconditions are untouched and such a host exits before any stage runs. No new default, no secret.
VF_STAGE_DECLINEDis gone with no dangling reference. The two tests keep the card's functions in place and assertcurrent_vgpu_typestays0, with the decline logged by the profile stage itself. That's a real mutation: on the base revision the node would take the declared profile and the assertion would fail. README matches the code.
Checked against current main (703e634): the two commits on main since this branch forked are renovate dependency bumps, disjoint from the nvidia_vgpu_host role, so nothing that landed on main touches this path.
[NIT] (non-blocking, out of scope): the MIG stage ensure_mig_mode still doesn't gate on Host VGPU Mode. That's pre-existing, MIG and mdev-vGPU are mutually exclusive, and enabling MIG needs a reset the stage already refuses. Not a reason to hold this change.
A card whose driver does not report Host VGPU Mode SR-IOV runs vGPU through the legacy mdev path, and writing current_vgpu_type on one of its functions can disturb the vGPUs that path already placed there. Only the virtual-function stage asked the driver for that mode, so a card reached through a profile declaration alone, or one the function stage had already refused, had its profile written anyway. Both stages now ask for themselves and leave the card alone unless it reports SR-IOV. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la>
ccbdcd8 to
91c2237
Compare
2 participants
Summary
The boot unit wrote
current_vgpu_typeon cards whose driver does not reportHost VGPU Mode: SR-IOV. A card that reportsNon SR-IOVruns vGPU through the legacy mdev path, and a write to one of its functions can disturb the vGPUs that path already placed there. Only the virtual-function stage asked the driver for that mode. The profile stage did not, so it wrote to a card the function stage had refused, and to a card declared with a profile alone, which that stage never sees at all.Changes
SR-IOV. They call one helper, so the check and the line it logs exist once, and the flag that carried the function stage's decision into the profile stage is gone.sriovand a profile, and a card declared with a profile and nosriov. Each keeps the functions it exposes, and each asserts the profile node is unchanged after the run.Test plan
ansible-lintpassesansible-test sanitypassesNo vGPU hardware here, so the stages ran against the fake
nvidia-smi,sriov-manageand PCI tree undertests/, on Ubuntu 24.04 with systemd.Summary by CodeRabbit
Bug Fixes
Documentation
Tests