From 91c22376917f4296da3122f2694163cb8bc64229 Mon Sep 17 00:00:00 2001 From: Aleksei Sviridkin Date: Fri, 18 Sep 2026 13:53:05 +0300 Subject: [PATCH] fix(roles): ask the driver before writing a vGPU profile 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 --- README.md | 2 +- .../cozystack-nvidia-vgpu-restore.sh.j2 | 52 +++++---- tests/test-nvidia-vgpu-host-stages.yml | 103 ++++++++++++++---- 3 files changed, 110 insertions(+), 47 deletions(-) diff --git a/README.md b/README.md index 2d9c2a1..01c33aa 100644 --- a/README.md +++ b/README.md @@ -206,7 +206,7 @@ A reboot on such a host loses more than this role restores. These are the three Where gpu-operator manages the vGPU Manager, that container's entrypoint enables the virtual functions itself and its driver root lives under `/run/nvidia/driver`, so the host has no `sriov-manage`. The unit carries `ConditionPathExists` on the host's `sriov-manage` and systemd reports it skipped. The same covers a host with no NVIDIA GPU and a host running a plain compute driver. If a host-installed driver and an operator-managed driver root are both present, ownership is undecidable, and the unit declines every stage and logs why. -Those are skips, and they exit zero. The unit fails instead when work the operator named did not happen, and the journal names which one. The exception is the per-PF `vgpu_profile` shorthand, which the role writes to every function the card exposes: functions past the card's instance limit reject it, and that is logged and skipped rather than failed, as described below. Common causes of a failure are a declared address that is not present on this host, virtual functions that could not be created, a function named in `vgpu_profiles` that would not take its profile, and MIG mode that could not be set. Treat that as examples rather than the whole set: every failing path logs its own line, so read the journal instead of matching a failure against this paragraph. The declared-address case is the one to watch when copying the example below, since an address left unedited names a card that does not exist. Declaring nothing at all is neither a skip nor a failure: with an empty device list the unit exits zero before looking at the hardware. +Those are skips, and they exit zero. The unit fails instead when work the operator named did not happen, and the journal names which one. The exception is the per-PF `vgpu_profile` shorthand, which the role writes to every function the card exposes: functions past the card's instance limit reject it, and that is logged and skipped rather than failed, as described below. The other exception is a card whose driver does not report `Host VGPU Mode: SR-IOV`. The virtual-function and profile stages ask the driver first and skip such a card whatever was declared for it: no virtual functions are created, no profile is written to any function the card already exposes, and the mode each stage saw is logged. Common causes of a failure are a declared address that is not present on this host, virtual functions that could not be created, a function named in `vgpu_profiles` that would not take its profile, and MIG mode that could not be set. Treat that as examples rather than the whole set: every failing path logs its own line, so read the journal instead of matching a failure against this paragraph. The declared-address case is the one to watch when copying the example below, since an address left unedited names a card that does not exist. Declaring nothing at all is neither a skip nor a failure: with an empty device list the unit exits zero before looking at the hardware. This covers the host-installed, ansible-managed path only. For clusters where the operator manages the driver, a DaemonSet reconciling per-VF profiles from a ConfigMap is the right mechanism and this role is not a substitute for one. diff --git a/roles/nvidia_vgpu_host/templates/cozystack-nvidia-vgpu-restore.sh.j2 b/roles/nvidia_vgpu_host/templates/cozystack-nvidia-vgpu-restore.sh.j2 index f5c7312..7bc243b 100644 --- a/roles/nvidia_vgpu_host/templates/cozystack-nvidia-vgpu-restore.sh.j2 +++ b/roles/nvidia_vgpu_host/templates/cozystack-nvidia-vgpu-restore.sh.j2 @@ -88,11 +88,6 @@ declare -A VF_OVERRIDE_COUNT_DECLARED=( ) declare -A VF_OVERRIDE_COUNT=() -# GPUs the virtual-function stage declined because the driver does not -# report SR-IOV mode. The profile stage stays quiet about those rather -# than failing the unit for a card it was already told to leave alone. -declare -A VF_STAGE_DECLINED=() - # Every printf tail below carries `|| true`. A failed write to the # journal would otherwise return non-zero from the function, and under # errexit a bare `log ...` call would end the run at the exact moment it @@ -207,6 +202,27 @@ host_vgpu_mode() { | tr -d '\r' } +# Asks the driver to state its own operating mode rather than derive +# SR-IOV capability from the PCI capability bits, and reports whether the +# card must be left alone. Hardware exists that advertises virtual +# functions in sysfs yet drives vGPU through the legacy mdev path and +# never creates them. Enabling functions on such a card can unbind a +# physical function carrying live vGPUs, and writing a profile to one can +# disturb the vGPUs that path already placed there. Both stages that +# touch SR-IOV state ask for themselves, so the answer does not depend on +# which flag the operator declared or on the other stage having run. +# Returns 0 when the card is refused, which is a valid outcome and never +# sets FAILED, and non-zero when the stage may proceed. Guard every call: +# a bare one would end the whole run under errexit on the card the stage +# was cleared to act on. +refuse_unless_sriov() { + local stage="$1" declared="$2" index="$3" mode + mode="$(trim "$(host_vgpu_mode "$index")")" + [ "$mode" = "$SRIOV_MODE" ] && return 1 + log "${stage}: ${declared} declined, host vGPU mode is not SR-IOV or was unrecognised; observed mode: '${mode}'. NVML defines only 'SR-IOV' and 'Non SR-IOV'; an empty value means the driver reported no such field" + return 0 +} + {% if not (mig_devices or sriov_devices or profile_all) %} # Nothing was declared, so there is no work and no reason to look at the # hardware at all. Rendered before the preconditions on purpose: the @@ -300,21 +316,13 @@ ensure_mig_mode() { # ---- STAGE 2: SR-IOV virtual functions ---- ensure_vfs() { - local declared="$1" index bus sysfs canonical mode numvfs output attempt + local declared="$1" index bus sysfs canonical numvfs output attempt if ! read -r index bus < <(resolve_gpu "$declared"); then log "VF: ${declared} is not present on this host, so its virtual functions were not restored" FAILED=1 return 0 fi - # Ask the driver to state its own operating mode rather than derive - # SR-IOV capability from the PCI capability bits. Hardware exists that - # advertises virtual functions in sysfs yet drives vGPU through the - # legacy mdev path and never creates them, and enabling functions on - # such a card can unbind a physical function carrying live vGPUs. - mode="$(trim "$(host_vgpu_mode "$index")")" - if [ "$mode" != "$SRIOV_MODE" ]; then - log "VF: ${declared} declined, host vGPU mode is not SR-IOV or was unrecognised; observed mode: '${mode}'. NVML defines only 'SR-IOV' and 'Non SR-IOV'; an empty value means the driver reported no such field" - VF_STAGE_DECLINED["$(normalise_bdf "$declared")"]=1 + if refuse_unless_sriov "VF" "$declared" "$index"; then return 0 fi if ! sysfs="$(sysfs_for_bdf "$bus")"; then @@ -369,7 +377,7 @@ ensure_vfs() { # request it. ensure_vf_profiles() { local declared="$1" fallback="$2" - local bus sysfs prefix virtfn vf_path vf_bdf profile current node + local index bus sysfs prefix virtfn vf_path vf_bdf profile current node local -i explicit=0 local -i seen=0 applied_overrides=0 declared_overrides=0 # Each card waits on its own budget. Sharing one across cards let the @@ -377,11 +385,14 @@ ensure_vf_profiles() { # named by hand on a later card failed for a node that was seconds from # appearing. PROFILE_WAIT_DEADLINE=0 - if ! read -r _ bus < <(resolve_gpu "$declared"); then + if ! read -r index bus < <(resolve_gpu "$declared"); then log "profile: ${declared} is not present on this host, so no vGPU profile was written" FAILED=1 return 0 fi + if refuse_unless_sriov "profile" "$declared" "$index"; then + return 0 + fi if ! sysfs="$(sysfs_for_bdf "$bus")"; then log "profile: no sysfs entry for ${declared} (bus ${bus}), so no vGPU profile was written" FAILED=1 @@ -458,13 +469,6 @@ ensure_vf_profiles() { done shopt -u nullglob if [ "$seen" -eq 0 ]; then - if [ -n "${VF_STAGE_DECLINED[$prefix]:-}" ]; then - # Already reported once by the virtual-function stage. Failing here - # too would give an operator on a legacy mdev card a red unit at - # every boot for a card the script was correct to leave alone. - log "profile: ${declared} has no virtual functions because its virtual-function stage declined it; nothing to write" - return 0 - fi log "profile: ${declared} has no virtual functions, so no vGPU profile could be written" FAILED=1 return 0 diff --git a/tests/test-nvidia-vgpu-host-stages.yml b/tests/test-nvidia-vgpu-host-stages.yml index 3f5c23a..8ab8581 100644 --- a/tests/test-nvidia-vgpu-host-stages.yml +++ b/tests/test-nvidia-vgpu-host-stages.yml @@ -503,30 +503,27 @@ report success. This is where an unedited example address lands. rc={{ _absent_card.rc }}, stdout={{ _absent_card.stdout }} - # The branch that keeps the profile stage quiet about a card the - # virtual-function stage already declined. It only runs when the card - # has no functions, which is the real state of a declined card, so the - # fixture has to strip the ones it was given. - - name: Strip the second domain's function - ansible.builtin.file: - path: "{{ _pci }}/0001:41:00.0/virtfn0" - state: absent - + # A card whose driver does not report SR-IOV keeps its functions here + # rather than being stripped of them, because the functions are the + # outcome under test: hardware that advertises them in sysfs while + # driving vGPU through the legacy mdev path is what the mode check + # exists for, and neither stage may write to one. + # # This play accumulates fixture state, so set every input this case - # depends on rather than inheriting it. An earlier case left the card - # reporting functions, which makes the stage report the work already - # done, and another left the mode as SR-IOV, which makes it succeed. - # Either one stops the branch under test from being entered. + # depends on rather than inheriting it. An earlier case left the mode + # as SR-IOV, which makes the card pass the check instead of being + # declined, and another left a profile on the function, which makes an + # unwritten node indistinguishable from a written one. - name: Set the inputs this case depends on ansible.builtin.copy: dest: "{{ item.path }}" content: "{{ item.content }}\n" mode: "0644" loop: - - {path: "{{ _pci }}/0001:41:00.0/sriov_numvfs", content: "0"} + - {path: "{{ _pci }}/0001:41:00.4/nvidia/current_vgpu_type", content: "0"} - {path: "{{ _tmp }}/host_vgpu_mode", content: "Non SR-IOV"} loop_control: - label: "{{ item.path | basename }}" + label: "{{ item.path }}" - name: Render a declined card that also declares a profile ansible.builtin.include_role: @@ -546,18 +543,80 @@ changed_when: false failed_when: false - - name: Assert a declined card does not fail the unit for its profile + - name: Read back the declined card's profile node + ansible.builtin.slurp: + src: "{{ _pci }}/0001:41:00.4/nvidia/current_vgpu_type" + register: _declined_node + + - name: Assert a declined card keeps its functions untouched ansible.builtin.assert: that: + - "'observed mode: ' + \"'Non SR-IOV'\" in _declined_profile.stdout" + - (_declined_node.content | b64decode).strip() == '0' - _declined_profile.rc == 0 - - "'virtual-function stage declined it' in _declined_profile.stdout" + - "'profile: 0001:41:00.0 declined' in _declined_profile.stdout" fail_msg: >- - A card the virtual-function stage declined has no functions to - write a profile to, and saying so twice must not fail the unit. - Otherwise a legacy mdev card is red at every boot for a card the - script was right to leave alone. rc={{ _declined_profile.rc }}, + The driver does not report SR-IOV for this card, so both stages + must refuse it; a write to a function it exposes can disturb + vGPUs the legacy mdev path already placed there. Refusing is + not a failure either, or such a card is red at every boot for + work the script was right not to do. + rc={{ _declined_profile.rc }}, + node={{ _declined_node.content | b64decode }}, stdout={{ _declined_profile.stdout }} + # The same card and the same driver mode, declared without sriov. The + # virtual-function stage is not rendered for such a card at all, so + # the profile stage is the only stage that runs and nothing else can + # have asked the driver anything on its behalf. + - name: Set the inputs the profile-only case depends on + ansible.builtin.copy: + dest: "{{ item.path }}" + content: "{{ item.content }}\n" + mode: "0644" + loop: + - {path: "{{ _pci }}/0001:41:00.4/nvidia/current_vgpu_type", content: "0"} + - {path: "{{ _tmp }}/host_vgpu_mode", content: "Non SR-IOV"} + loop_control: + label: "{{ item.path }}" + + - name: Render a card that declares a profile without sriov + ansible.builtin.include_role: + name: cozystack.installer.nvidia_vgpu_host + vars: + cozystack_nvidia_vgpu_devices: + - address: "0001:41:00.0" + vgpu_profile: 1155 + + - name: Run against a card that declares a profile without sriov + ansible.builtin.command: + cmd: "{{ _script }}" + environment: + PATH: "{{ _bin }}:{{ ansible_env.PATH }}" + register: _profile_only + changed_when: false + failed_when: false + + - name: Read back the profile-only card's node + ansible.builtin.slurp: + src: "{{ _pci }}/0001:41:00.4/nvidia/current_vgpu_type" + register: _profile_only_node + + - name: Assert the profile stage asks the driver on its own + ansible.builtin.assert: + that: + - (_profile_only_node.content | b64decode).strip() == '0' + - _profile_only.rc == 0 + - "'profile: 0001:41:00.0 declined' in _profile_only.stdout" + fail_msg: >- + A card whose driver does not report SR-IOV must be left alone + whichever flag the operator declared. Without sriov nothing + else looks at this card's mode, and writing current_vgpu_type + can disturb the vGPUs the legacy mdev path already placed on + it. rc={{ _profile_only.rc }}, + node={{ _profile_only_node.content | b64decode }}, + stdout={{ _profile_only.stdout }} + # Two declared cards where the second one's profile node is still # being populated when its turn comes. The profile stage's wait is one # budget for the whole stage, so a first card that spends it leaves @@ -598,7 +657,7 @@ path: "{{ _pci }}/0001:41:00.4/nvidia/current_vgpu_type" state: absent - - name: Give the second card back the function the previous case stripped + - name: Give the second card the function this case needs ansible.builtin.file: src: "{{ _pci }}/0001:41:00.4" dest: "{{ _pci }}/0001:41:00.0/virtfn0"