From 6941e4318d4aee1b16f3fb916f9790764ba29c1f Mon Sep 17 00:00:00 2001 From: Nitish Bhat Date: Fri, 21 Aug 2026 15:56:25 -0700 Subject: [PATCH] fix(docs,ci): Radeon device IDs in OpenShift NFD rules + drift guard (GPUOP-1062) (#1658) * fix(docs,ci): Radeon device IDs in OpenShift NFD rules + drift guard (GPUOP-1062) Deploying via OLM on OpenShift with Radeon GPUs detected zero GPUs. On an 8x Radeon AI PRO R9700 (0x7551) cluster this failed ~300 tests with "No nodes with AMD/GPU found in the cluster". The OLM bundle ships no NodeFeatureRule -- verified: none in bundle/manifests, none in config/, none in hack/openshift-patch, and the operator creates none at runtime. On OpenShift the user must create the rule by hand, and the only source for its contents is docs/installation/openshift-olm.md. Both copy-paste YAML blocks in that doc (a NodeFeatureDiscovery CR and a NodeFeatureRule CR) listed Instinct device IDs only, so no node ever got the feature.node.kubernetes.io/amd-gpu label and every test bailed at setup. Root cause is duplication with nothing enforcing agreement: commit 9c5ef17e added 0x7551 to the two gpu-nfd-default-rule.yaml copies and left the docs untouched. Fix, and a guard so it cannot recur silently: - docs/installation/openshift-olm.md: add the 16 missing amd-gpu and 2 missing amd-vgpu device IDs to both YAML blocks. - docs/troubleshooting.md: new section for pci-1002.present=true with no amd-gpu label. - hack/check-nfd-device-ids.py: compare every copy of the list against the source of truth, hack/k8s-patch/template-patch/gpu-nfd-default-rule.yaml, and report the exact missing/extra IDs per rule. The helm copy is a build output -- make helm-k8s does rm -rf on helm-charts-k8s and repopulates it from template-patch -- so drift there means the wrong file was edited. - make check-nfd-device-ids: thin wrapper, so contributors without Claude Code or CI get the identical result. - .claude/hooks/check-nfd-drift.sh: PostToolUse hook reporting drift in-session at the moment it is introduced. Fails open. - .github/workflows/nfd-device-id-check.yml: calls the make target. No branch or path filter, so it can safely be made a required status check. Plan: docs-internal/knowledge/plans/2026-08-21-gpuop-1062-openshift-nfd-radeon-device-ids.md * fix(docs): keep the troubleshooting entry to verified behaviour The first draft of the entry told users to look for a literal feature.node.kubernetes.io/pci-1002.present label and described how that name is derived. That detail came from upstream NFD documentation rather than from anything exercised here, and the exact label varies with the deviceLabelFields in use, so it does not belong in our troubleshooting guide. The entry now sticks to what is verifiable in this repository: amd-gpu is the label the operator selects on, it comes only from a rule the user supplies, the OLM bundle ships no such rule so one must be created on OpenShift, and the Helm chart installs the full device list on Kubernetes when installdefaultNFDRule is enabled. Also drops the suggested 'grep -i amd' pipeline, which could never have matched a pci-1002.present label in the first place. (cherry picked from commit 2aa0eaaab0f3fd82a910efd7613aa342fec238a3) --- .claude/hooks/check-nfd-drift.sh | 52 +++++ .claude/settings.json | 72 +++++++ ...op-1062-openshift-nfd-radeon-device-ids.md | 189 ++++++++++++++++++ 3 files changed, 313 insertions(+) create mode 100755 .claude/hooks/check-nfd-drift.sh create mode 100644 .claude/settings.json create mode 100644 docs-internal/knowledge/plans/2026-08-21-gpuop-1062-openshift-nfd-radeon-device-ids.md diff --git a/.claude/hooks/check-nfd-drift.sh b/.claude/hooks/check-nfd-drift.sh new file mode 100755 index 00000000..1fd464ae --- /dev/null +++ b/.claude/hooks/check-nfd-drift.sh @@ -0,0 +1,52 @@ +#!/usr/bin/env bash +# PostToolUse hook: report when an edit leaves the AMD GPU PCI device-ID lists +# out of sync across their copies. +# +# The device-ID list is duplicated across the NodeFeatureRule template, its +# generated helm copy, and two copy-paste YAML blocks in the OpenShift install +# docs. Nothing in the build enforces agreement, and they have diverged before: +# commit 9c5ef17e added the Radeon AI PRO R9700 (0x7551) to the rule but not to +# the docs, so OLM users on Radeon hardware got no GPU labels at all +# (GPUOP-1062). This hook catches that at the moment it is introduced, rather +# than at PR time. +# +# The same check runs as 'make check-nfd-device-ids', which is what covers +# contributors who are not using Claude Code. +# +# Failure mode: this hook FAILS OPEN. If python3 is missing or the checker is +# absent, we exit 0 so editing is never impeded. A silent hook is acceptable +# because the Makefile target is the real backstop. +set -uo pipefail + +input=$(cat) + +# Fast path: almost every edit touches none of the tracked files. Bail before +# spawning python3 unless a tracked filename appears somewhere in the payload. +case "$input" in + *gpu-nfd-default-rule*|*openshift-olm.md*) ;; + *) exit 0 ;; +esac + +root="${CLAUDE_PROJECT_DIR:-$(pwd)}" +checker="$root/hack/check-nfd-device-ids.py" + +[[ -f "$checker" ]] || exit 0 +command -v python3 >/dev/null 2>&1 || exit 0 + +output=$(python3 "$checker" --root "$root" --quiet 2>&1) +case $? in + 1) + printf '%s\n\n' "$output" >&2 + printf 'The file you just edited is one of several copies of the AMD GPU device-ID list.\n' >&2 + printf 'Update the other copies so they match, then re-run: make check-nfd-device-ids\n' >&2 + exit 2 + ;; + 2) + printf '%s\n\n' "$output" >&2 + printf 'The device-ID drift checker could not parse one of its inputs, so it is\n' >&2 + printf 'currently blind. Fix hack/check-nfd-device-ids.py before relying on it.\n' >&2 + exit 2 + ;; +esac + +exit 0 diff --git a/.claude/settings.json b/.claude/settings.json new file mode 100644 index 00000000..d6ed4608 --- /dev/null +++ b/.claude/settings.json @@ -0,0 +1,72 @@ +{ + "$schema": "https://json.schemastore.org/claude-code-settings.json", + "permissions": { + "allow": [ + "Bash(git status:*)", + "Bash(git diff:*)", + "Bash(git log:*)", + "Bash(git show:*)", + "Bash(git branch:*)", + "Bash(git stash list:*)", + "Bash(git ls-files:*)", + "Bash(git blame:*)", + + "Bash(gh pr view:*)", + "Bash(gh pr list:*)", + "Bash(gh pr diff:*)", + "Bash(gh pr checks:*)", + "Bash(gh issue view:*)", + "Bash(gh issue list:*)", + "Bash(gh api:*)", + "Bash(gh run view:*)", + "Bash(gh run list:*)", + + "Bash(go test:*)", + "Bash(go vet:*)", + "Bash(go build:*)", + "Bash(go mod tidy:*)", + "Bash(go mod download:*)", + "Bash(gofmt:*)", + "Bash(goimports:*)", + "Bash(golangci-lint:*)", + + "Bash(make:*)", + + "Bash(docker ps:*)", + "Bash(docker images:*)", + "Bash(docker logs:*)", + "Bash(docker inspect:*)", + + "WebFetch(domain:github.com)", + "WebFetch(domain:instinct.docs.amd.com)", + "WebFetch(domain:code.claude.com)", + "WebFetch(domain:docs.claude.com)" + ] + }, + "hooks": { + "PreToolUse": [ + { + "matcher": "Write|Edit|NotebookEdit", + "hooks": [ + { "type": "command", "command": "${CLAUDE_PROJECT_DIR}/.claude/hooks/protect-generated.sh" } + ] + } + ], + "PostToolUse": [ + { + "matcher": "Write|Edit", + "hooks": [ + { "type": "command", "command": "${CLAUDE_PROJECT_DIR}/.claude/hooks/gofmt-on-edit.sh" }, + { "type": "command", "command": "${CLAUDE_PROJECT_DIR}/.claude/hooks/check-nfd-drift.sh" } + ] + } + ], + "SessionEnd": [ + { + "hooks": [ + { "type": "command", "command": "${CLAUDE_PROJECT_DIR}/.claude/hooks/capture-session.sh" } + ] + } + ] + } +} diff --git a/docs-internal/knowledge/plans/2026-08-21-gpuop-1062-openshift-nfd-radeon-device-ids.md b/docs-internal/knowledge/plans/2026-08-21-gpuop-1062-openshift-nfd-radeon-device-ids.md new file mode 100644 index 00000000..157ccc3c --- /dev/null +++ b/docs-internal/knowledge/plans/2026-08-21-gpuop-1062-openshift-nfd-radeon-device-ids.md @@ -0,0 +1,189 @@ +# GPUOP-1062: OpenShift NFD device-ID drift (Radeon GPUs undetected) + +- **Date:** 2026-08-21 +- **Author:** Nitish Bhat +- **Related PR(s):** pensando/gpu-operator (this PR) + ROCm/gpu-operator counterpart +- **Related issue(s) / JIRA:** [GPUOP-1062](https://pensando.atlassian.net/browse/GPUOP-1062) + +## Context + +Deploying the GPU Operator via OLM on an OpenShift cluster with Radeon GPUs +detects zero GPUs. Reported against an 8x Radeon AI PRO R9700 (`0x7551`) SNO +cluster (jobd 33207089), where all ~300 pytest cases failed with +`No nodes with AMD/GPU found in the cluster`. + +The tests resolve GPU nodes through `k8_get_gpu_nodes()`, which matches on +`feature.node.kubernetes.io/amd-gpu` / `amd-vgpu` +(`tests/pytests/lib/k8_util.py:176`). Those labels are produced by a +NodeFeatureRule whose device-ID list did not contain `7551`, so no node was +ever labelled and every test bailed at setup. + +### Correcting the reported diagnosis + +The Jira description states the OLM bundle ships a NodeFeatureRule that covers +only Instinct device IDs, and recommends syncing it with the helm chart. +**That artifact does not exist.** Verified: + +- `bundle/manifests/` contains 7 files, none a NodeFeatureRule. +- `config/` contains no NodeFeatureRule, so nothing generates one into the bundle. +- `hack/openshift-patch/olm-bundle-patch/` contains only a PrometheusRule. +- The operator does not create one at runtime (no `NodeFeatureRule` reference + in non-test Go code). + +The OLM bundle ships **no** NodeFeatureRule by design. On OpenShift the user is +expected to create one by hand, and the only source for its contents is +`docs/installation/openshift-olm.md`. That doc holds two copy-paste YAML blocks +— a `NodeFeatureDiscovery` CR and a `NodeFeatureRule` CR — and **both** list +Instinct device IDs only. That is the actual defect. + +### Root cause + +The AMD GPU PCI device-ID list is duplicated across several hand-maintained +files with nothing enforcing agreement. Commit `9c5ef17e` ("Add Radeon AI PRO +R9700 to gpu-nfd-default-rule files") updated exactly two of them and left the +documentation untouched. GPUOP-990 (the 1.5.1 Radeon docs pass) updated product +framing and the version matrix but likewise never touched the device lists. + +Drift measured against the source of truth: + +- `amd-gpu` — 16 IDs missing from the docs: + `7460 7448 744b 744a 7449 745e 73a2 73a3 73ab 73a1 7551 7550 744c 73af 73bf 7590` +- `amd-vgpu` — 2 IDs missing from the docs: `7461` (V710 MxGPU), `73ae` (V620 MxGPU) + +### Which file is authoritative + +`hack/k8s-patch/template-patch/gpu-nfd-default-rule.yaml` is the source of +truth. `make helm-k8s` runs `clean-helm` (`rm -rf helm-charts-k8s`) and then +repopulates the tree via +`cp hack/k8s-patch/template-patch/* helm-charts-k8s/templates/`, so +`helm-charts-k8s/templates/gpu-nfd-default-rule.yaml` is a pure build output — +`config/` generates no such file, meaning it exists in the helm tree solely +because of that copy. It is committed to git, but editing it is futile. + +## Approach + +Fix the documentation, then add a guard so the lists cannot silently diverge +again. The comparison logic lives in one script; the Makefile target, the +Claude Code hook, and the CI workflow are all thin callers of it — the same +"thin caller" conclusion reached in `2026-06-16-buildx-cache-support.md`. + +1. **`hack/check-nfd-device-ids.py`** — extracts `{rule name -> set of device + IDs}` from every copy and diffs each against the source of truth. Python 3 + stdlib only. The files use different YAML shapes (source: one ID per + `matchFeatures` entry; docs: a multi-line inline list), so the script + normalizes to a set per rule rather than doing a textual diff. Reports the + exact missing/extra IDs per rule per file and exits non-zero on drift. + + Files checked: + + | File | Role | + | --- | --- | + | `hack/k8s-patch/template-patch/gpu-nfd-default-rule.yaml` | source of truth | + | `helm-charts-k8s/templates/gpu-nfd-default-rule.yaml` | build output; drift means the wrong file was edited or `make helm-k8s` was not re-run before committing | + | `docs/installation/openshift-olm.md` (both blocks) | hand-maintained docs | + +2. **`make check-nfd-device-ids`** — thin wrapper, so the check is runnable by + any contributor with no Claude Code and no CI. + +3. **`.claude/hooks/check-nfd-drift.sh`** — `PostToolUse` on `Write|Edit`, + wired in `.claude/settings.json`. Catches drift in-session, at the moment it + is introduced, rather than at PR time. Follows the conventions already + established by `protect-generated.sh`: the matcher can only filter on tool + name, so the script does a cheap path check first and exits 0 immediately + for edits to unrelated files; and it **fails open** on a missing `python3` + or an unparseable payload, so a hook fault can never wedge editing. + +4. **`.github/workflows/nfd-device-id-check.yml`** — calls the Makefile target. + Checked in to **both** repos. GitHub Actions is already active on each: + pensando runs `pr-plan-check` on every PR to `main` (intended as a required + status check), and ROCm/gpu-operator runs GitHub Actions PR sanity checks + (ROCm/gpu-operator#576). + + Deliberately carries no branch filter, so one file serves both default + branches and also covers backport PRs onto release branches. It is likewise + not filtered on paths: a path-filtered workflow reports no status on + unrelated PRs, which stalls merges once it is a required check. The check + runs in about a second. + +### Alternatives considered + +- **Sync a NodeFeatureRule in the OLM bundle** (as the Jira recommends) — + rejected: no such artifact exists, and `bundle/manifests/**` is generated and + hook-protected regardless. +- **Generate the doc YAML blocks from the source of truth** (marker comments + + `make ...-sync`) — rejected for now: makes part of a hand-written doc + generated, for a drift problem that a report-only check plus an in-session + hook already closes. +- **Hoist device IDs into a single data file** rendered into both the helm + template and the docs — rejected: modifies a shipping artifact, which is + disproportionate risk for a documentation bug. +- **A `CLAUDE.md` bullet describing the sync requirement** — rejected: costs + context on every session regardless of relevance, whereas the hook fires only + when a tracked file is actually edited. +- **Extending `protect-generated.sh` to block edits to + `helm-charts-k8s/templates/**`** — rejected as redundant: the drift check + already flags a hand-edited helm copy, since it would no longer match source. + +## Scope + +- **In scope:** + - `docs/installation/openshift-olm.md` — add the missing IDs to both YAML + blocks. Device-list changes only; no prose added. + - `docs/troubleshooting.md` — entry for `pci-1002.present=true` with no + `amd-gpu` label (Jira item 4). This also gives the existing + "For more detailed troubleshooting steps" pointer at the end of + `docs/installation/kubernetes-helm.md` something real to land on; that + pointer previously led to a page with no NFD content at all. + - `hack/check-nfd-device-ids.py`, `make check-nfd-device-ids`. + - `.claude/hooks/check-nfd-drift.sh` + `.claude/settings.json` wiring. + - `.github/workflows/nfd-device-id-check.yml` (both repos). + +- **Out of scope:** + - Prose in `docs/installation/openshift-olm.md` stating that the OLM bundle + ships no NodeFeatureRule. Drafted and then dropped to keep the diff to the + device lists; the fact is recorded here and in the troubleshooting entry. + - `tests/e2e/yamls/charts/gpu-operator/templates/nfd-default-rule.yaml` — a + pinned v1.0.0 baseline chart used by the upgrade tests + (`02_cluster_upgrade_policy_test.go`). Its list is stale **by design**; + refreshing it would invalidate upgrade coverage. Excluded from the checker. + - `docs/kubevirt/kubevirt.md` — contains `lspci` sample output and + placeholders, not a NodeFeatureRule list. + - `tests/e2e/yamls/openshift/nfd-instance.yaml` — labels by vendor only + (`device` commented out) with no custom rules, so it yields + `pci-1002.present` but never `amd-gpu`. This matches the symptom described + in the Jira, but is not the cause of the reported failures: the Go test + consuming it is unconditionally skipped + (`01_cluster_core_test.go:90`) and selects on `pci-1002.present` on + OpenShift anyway. Worth a separate look. + - The 1.5.1 backport requested in the Jira comments — separate cherry-pick. + - Any runtime/operator code change. + +## Validation + +- **Checker, negative:** run `make check-nfd-device-ids` before the doc fix; + it must fail and list exactly the 16 `amd-gpu` and 2 `amd-vgpu` IDs above. + This is what drives the doc edit, so the fix cannot be partial. +- **Checker, positive:** run again after the doc fix; must pass. +- **Checker, regression:** temporarily delete one ID from the source of truth + and confirm the extra-ID direction is reported too, then restore. +- **Hook:** edit a tracked file in a Claude Code session and confirm the drift + report surfaces; edit an unrelated file and confirm no output and no delay. +- **Hook fail-open:** run the hook with a malformed payload and with `python3` + masked off `PATH`; must exit 0 in both cases. +- **Docs build:** `make docs` renders without errors. +- **Field check:** on the R9700S cluster, apply the corrected NodeFeatureRule + and confirm `feature.node.kubernetes.io/amd-gpu=true` appears, then re-run a + sanity subset that previously failed at `No nodes with AMD/GPU found`. + +## Risks and rollback + +- **Risk:** a device ID transcribed incorrectly into the docs would send users + a broken rule. Mitigated by deriving the doc edit from the checker output + rather than by hand, and by the checker gating the result. +- **Risk:** the hook runs on every `Write`/`Edit`. Mitigated by the early path + check and by failing open; worst case is a stray message, never a block. +- **Risk:** the parser is regex-driven, so an unusual future YAML shape could + be silently skipped. Mitigated by the checker erroring if a file yields **no** + rules at all, so a total parse failure cannot masquerade as "in sync". +- **Rollback:** revert the commits. Documentation-only plus developer tooling; + no runtime impact, and nothing ships in the operator image or the bundle.