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"