fix(roles): ask the driver before writing a vGPU profile

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.
 

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,19 +377,22 @@ 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
   # first card that had to wait spend all of it, after which a function
   # 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

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"