Add configurable ServiceAccount for the DCGM Exporter

[100-JM]

Description

Closes #2729.

The DCGM Exporter ServiceAccount name is hardcoded to nvidia-dcgm-exporter across the assets, the RBAC bindings, the OpenShift SCC and the DaemonSet, so platforms that bind an identity (IRSA, Workload Identity, PKI) to a specific ServiceAccount name cannot use the operator-managed exporter — they have to run a standalone exporter, vendor-patch the operator, or fight reconciliation with an admission mutator.

This adds dcgmExporter.serviceAccount with the {name, create} shape agreed in the issue:

configuration	behaviour
unset	identical to today
{name: X}	the operator creates and manages ServiceAccount X; every exporter operand references it
{name: X, create: false}	X must already exist in the operator namespace; the operator only references it
{create: false} without a name	rejected by CRD validation

A user-provided ServiceAccount is treated as unmanaged: it is never created, adopted, mutated or deleted, it is left without an owner reference, it survives disabling the exporter, and a missing one is reported as NotReady instead of silently leaving the DaemonSet pending. A ServiceAccount is only deleted when it carries a ClusterPolicy owner reference, so one a user had already provisioned under the same name is left alone.

Scope follows the issue discussion:

• SCC — the SCC name and the openshift.io/scc annotation on the DaemonSet stay tied to the asset name; only the users entry follows the resolved ServiceAccount.
• RBAC — only the exporter's own subject is rewritten, so the Prometheus subject in 0500_prom_rolebinding_openshift.yaml is preserved.
• GPUCluster — DCGMExporterSpec is shared, so the DRA manifests honour the same field, defaulting to nvidia-dcgm-exporter-dra.
• Helm — the block is emitted only when the user sets it.
• ServiceAccount annotations are intentionally out of scope: IRSA / Workload Identity users own their ServiceAccount through create: false.

DCGM Exporter is the only component touched.

Checklist

• No secrets, sensitive information, or unrelated changes
• Lint checks passing (make lint)
• Generated assets in-sync (make validate-generated-assets)
• Go mod artifacts in-sync (make validate-modules)
• Test cases are added for new code paths

Testing

make unit-test passes (no failures) and make lint, make fmt, make license-check, make validate-generated-assets, make validate-modules are clean.

New unit tests:

• api/nvidia/v1: TestDCGMExporterServiceAccount — name/create resolution including the unset default.
• controllers: TestDCGMExporterServiceAccountReconcile — the default is created and owned; a configured name is created instead of the default; create: false reports NotReady when the ServiceAccount is missing, references an existing one without adopting it, and keeps it when the exporter is disabled; a ServiceAccount without a ClusterPolicy owner reference is not deleted; one with it is.
• controllers: TestDCGMExporterRBACSubjects — RoleBinding / ClusterRoleBinding subjects and the SCC users entry follow the configured ServiceAccount while the object names stay unchanged, and the Prometheus subject is untouched.
• controllers: TestTransformDCGMExporterServiceAccount — the DaemonSet reference, and that an unset or default configuration does not change the asset value.

Not tested on a live cluster: I do not have an OpenShift cluster for the SCC path, so that path is covered by unit tests only.

🤖 Generated with Claude Code

https://claude.ai/code/session_01UiFVTVAyEMmn68Xvo9LSTf

[copy-pr-bot]

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

[coderabbitai]

📝 Walkthrough

Walkthrough

The change adds optional DCGM Exporter ServiceAccount configuration with a custom name and creation flag. Validation requires a name for externally managed accounts. The controller resolves the configured or default name, manages only operator-owned accounts, validates missing external accounts, and updates RBAC, SCC, and DaemonSet references. Helm values and ClusterPolicy templates expose the configuration. Rendered manifests conditionally create the ServiceAccount and use the configured name.

Priority: ➖ Normal

Severity of issue fixed: Medium

Merge Risk: 🔵 Low · up to c76cf

ServiceAccount names such as true or 123 can cause the DCGM exporter DaemonSet to be rejected. Quote the rendered value before merge.

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Note

Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.

🟡 Other comments (2)

api/nvidia/v1/clusterpolicy_types_test.go-113-117 (1)

113-117: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win

Add a CRD validation regression test for dcgmExporter.serviceAccount: {create: false} without name.

The helper tests do not exercise the generated ClusterPolicy CRD. The XValidation rule is the only safeguard before reconciliation; without it, the helper defaults the missing name to nvidia-dcgm-exporter and selects unmanaged reconciliation. Add a test that asserts the generated CRD rejects this configuration.

manifests/state-dcgm-exporter/0700_daemonset.yaml-43-43 (1)

43-43: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win

Quote ServiceAccountName in this manifest.

When the configured string is true or 123, the unquoted template output is decoded into the unstructured object as a YAML boolean or number. Kubernetes can then reject spec.template.spec.serviceAccountName, which requires a string.

-      serviceAccountName: {{ .ServiceAccountName }}
+      serviceAccountName: {{ .ServiceAccountName | quote }}

🧹 Nitpick comments (1)

controllers/object_controls_test.go (1)

2670-2684: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add one non-DCGM state case.

Every subtest sets stateNames to state-dcgm-exporter, so the new isDCGMExporter guard is never exercised in the false direction. If that guard is later widened, the ownership check would start skipping deletion for driver, toolkit and device-plugin ServiceAccounts on cleanup, and no test here would fail. A subtest with stateNames: []string{"state-driver"} and a disabled driver that asserts the unowned ServiceAccount is deleted pins the current behavior.

As per path instructions: "Flag ... new operand or CRD behaviour that ships with no regression test."

Source: Path instructions

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: QUIET

Plan: Enterprise

Run ID: d440e324-e2a5-4b14-a69e-3c8eb81a44f5

📥 Commits

Reviewing files that changed from the base of the PR and between 3904817 and c76cf0d.

⛔ Files ignored due to path filters (7)

• api/nvidia/v1/zz_generated.deepcopy.go is excluded by !**/zz_generated.*.go
• bundle/manifests/nvidia.com_clusterpolicies.yaml is excluded by !bundle/manifests/nvidia.com_*.yaml
• bundle/manifests/nvidia.com_gpuclusters.yaml is excluded by !bundle/manifests/nvidia.com_*.yaml
• config/crd/bases/nvidia.com_clusterpolicies.yaml is excluded by !config/crd/bases/**
• config/crd/bases/nvidia.com_gpuclusters.yaml is excluded by !config/crd/bases/**
• deployments/gpu-operator/crds/nvidia.com_clusterpolicies.yaml is excluded by !deployments/gpu-operator/crds/**
• deployments/gpu-operator/crds/nvidia.com_gpuclusters.yaml is excluded by !deployments/gpu-operator/crds/**

📒 Files selected for processing (14)

• api/nvidia/v1/clusterpolicy_types.go
• api/nvidia/v1/clusterpolicy_types_test.go
• controllers/object_controls.go
• controllers/object_controls_test.go
• controllers/transforms_test.go
• deployments/gpu-operator/templates/clusterpolicy.yaml
• deployments/gpu-operator/values.yaml
• internal/state/dcgm_exporter.go
• internal/state/types.go
• manifests/state-dcgm-exporter/0100_serviceaccount.yaml
• manifests/state-dcgm-exporter/0300_rolebinding.yaml
• manifests/state-dcgm-exporter/0310_clusterrolebinding.yaml
• manifests/state-dcgm-exporter/0450_scc.openshift.yaml
• manifests/state-dcgm-exporter/0700_daemonset.yaml

Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review.

[100-JM]

Thanks — all three points addressed in dd3685a.

Quote the templated ServiceAccount name. Applied to 0700_daemonset.yaml, and to the three other places the DRA manifests interpolate the user-supplied name (0100_serviceaccount.yaml, 0300_rolebinding.yaml, 0310_clusterrolebinding.yaml) so a name such as true or 123 cannot be decoded as a boolean or a number anywhere. The SCC entry is left as is: system:serviceaccount:<ns>:<name> is a plain scalar regardless of the name.

CRD validation regression test. Added TestDCGMExporterServiceAccountCRDValidation, which reads the generated ClusterPolicy and GPUCluster CRDs and asserts the serviceAccount schema still carries exactly one x-kubernetes-validations entry with the expected message and a rule referencing both self.create and self.name. The repository has no CEL evaluator dependency, so the test pins that the marker survives regeneration rather than re-evaluating the expression itself; adding cel-go only for this felt out of scope for this PR, but I'm happy to if you would rather see the rule exercised against real inputs.

Non-DCGM state case. Added a subtest that runs ServiceAccount() with stateNames: []string{"state-driver"} and a disabled driver, asserting an unowned ServiceAccount is still deleted. That pins the ownership check to the DCGM Exporter and would fail if the guard were widened.

make unit-test, lint, fmt, license-check, validate-generated-assets and validate-modules are clean.

[arushigarg1031]

1. Helm GPUCluster does not pass through the field
File: deployments/gpu-operator/templates/gpucluster.yaml

dcgmExporter.serviceAccount is wired in deployments/gpu-operator/templates/clusterpolicy.yaml but not here (after the service / serviceMonitor block, ~L121). A Helm GPUCluster install cannot set the field. Add the same {{- if .Values.dcgmExporter.serviceAccount }} passthrough. Values are already documented in deployments/gpu-operator/values.yaml.

2. create: false does not surface NotReady when the SA is missing
Files: internal/state/dcgm_exporter.go, internal/state/configurable_state.go

manifests/state-dcgm-exporter/0100_serviceaccount.yaml skips the SA when CreateServiceAccount is false, but configurable_state.go Sync() never checks that the user-provided SA exists. ClusterPolicy does this in controllers/object_controls.go (ServiceAccount()). Before syncObjects, if create: false, Get the SA and return SyncStateNotReady when it is missing.

3. GPUCluster create: true can adopt a pre-existing SA
File: internal/state/state_skel.go

createOrUpdateObjs() always SetControllerReference then updates on AlreadyExists. That adopts a pre-existing SA and GC will delete it with the GPUCluster. ClusterPolicy skips update on AlreadyExists in controllers/object_controls.go. For the DCGM exporter SA, do not set ownerRef / update if the object already exists and is not owned by this CR.

4. Renaming does not delete the previous operator-owned SA
Files: controllers/object_controls.go, internal/state/dcgm_exporter.go

ServiceAccount() only reconciles the new name. Switching nvidia-dcgm-exporter → metrics-identity leaves the old owned SA. On disable it also only deletes the current name. After resolving the new name, delete the previous default/configured SA only if it has a ClusterPolicy/GPUCluster ownerRef. Same cleanup needed on the DRA path (stale objects are not removed in state_skel.go syncObjects).

5. No GPUCluster tests for the new field
Files: internal/state/dcgm_exporter_test.go, internal/state/gpucluster_render_test.go

ClusterPolicy coverage in controllers/object_controls_test.go / controllers/transforms_test.go is good. Add GPUCluster cases for a custom name and create: false (SA omitted, DaemonSet/RBAC/SCC still reference it). Update goldens under internal/state/testdata/golden/ if the default render changes.

[100-JM]

Thanks — this was a real gap: the first commit wired the field on the ClusterPolicy path and left the DRA path able to render the reference without honouring the contract behind it. All five points are addressed in ba6cc85.

1. Helm GPUCluster passthrough. Added after the serviceMonitor block in templates/gpucluster.yaml, same {{- if }} guard as clusterpolicy.yaml. Verified with helm template --set dcgmExporter.serviceAccount.name=... --set dcgmExporter.serviceAccount.create=false.

2. create: false now reports NotReady. Added an optional preSync hook to configurableState, run after the manifests render and before they are applied, and implemented it for the exporter. A missing user-provided ServiceAccount returns an error naming it, so Sync() reports SyncStateNotReady instead of applying a DaemonSet that cannot schedule.

3. No adoption on the DRA path. Rather than special-casing a kind inside createOrUpdateObjs(), the same preSync refuses a configured name that already exists and is not controlled by the CR, pointing the user at create: false. The default name stays tolerant on purpose: an upgrade whose ServiceAccount lost its owner reference has to keep converging. Note that the create: false case never reaches createOrUpdateObjs() at all, since the manifest omits the object.

4. Renaming reclaims the previous ServiceAccount. Both paths now delete the operator-owned default once a different name takes over, and only when it carries the CR's owner reference — a ServiceAccount the user had provisioned under that name is left alone. Renaming between two custom names is not tracked, since the previous name is not recorded anywhere; that limitation is called out in the helper's comment. If you would rather see it handled, recording the applied name in status would be the way, and I am happy to do it in a follow-up.

5. GPUCluster tests. TestDCGMExporterDefaultServiceAccount, TestDCGMExporterCustomServiceAccountName (ServiceAccount, DaemonSet and both RBAC subjects follow the name while the binding objects keep theirs), TestDCGMExporterUserProvidedServiceAccount (no ServiceAccount rendered, everything still references it), and TestDCGMExporterServiceAccountPreSync covering each branch above. The default render is unchanged, so no golden files needed updating.

make unit-test (0 failures), lint, fmt, license-check, validate-generated-assets and validate-modules are clean.

[arushigarg1031]

Thanks — @100-JM covers the previous notes (gpucluster.yaml, preSync NotReady, no adopt of a custom name, reclaim of the default SA, GPUCluster tests).
One leftover: {name: X, create: false} still does not reclaim the operator-owned default.

• controllers/object_controls.go (ServiceAccount(), the unmanaged early return)
• internal/state/dcgm_exporter.go (checkDCGMExporterServiceAccount(), the create=false branch)
  That is the BYO/IRSA path (default install → existing SA). After the Get succeeds, call deleteOwnedServiceAccount for the default name, same as create: true. Tests in controllers/object_controls_test.go and internal/state/dcgm_exporter_test.go.

[100-JM]

Good catch — that is the transition the feature exists for, and both create: false branches returned as soon as the Get succeeded. Fixed in 754d887 on the ClusterPolicy path (ServiceAccount()) and the DRA path (checkDCGMExporterServiceAccount()), under the same ownership rule as the rename case.

One guard added on top of your note: the reclaim is skipped when the user brings the default name itself ({name: nvidia-dcgm-exporter, create: false}, or nvidia-dcgm-exporter-dra on the DRA path). Without it the operator would delete the very ServiceAccount it was just told to reference, because that object can still carry an owner reference from the earlier managed install.

Tests on both paths:

• controllers: handing over to a user-provided ServiceAccount reclaims the owned default and bringing the default name keeps that ServiceAccount.
• internal/state: create=false reclaims the operator-owned default and create=false with the default name keeps that ServiceAccount.

make unit-test (0 failures), lint, fmt, license-check, validate-generated-assets and validate-modules are clean.