Conversation
The DCGM Exporter ServiceAccount name is hardcoded to nvidia-dcgm-exporter
across the assets, the RBAC bindings, the OpenShift SCC and the DaemonSet.
Platforms that bind an identity (IRSA, Workload Identity, PKI) to a specific
ServiceAccount name therefore cannot use the operator-managed exporter and have
to run a standalone one, vendor-patch the operator, or fight reconciliation with
an admission mutator.
Add DCGMExporterSpec.serviceAccount with a {name, create} shape:
- unset keeps the current behaviour;
- name selects the ServiceAccount every exporter operand references;
- create: false binds to a ServiceAccount that already exists in the operator
namespace.
A user-provided ServiceAccount is never created, adopted, mutated or deleted: it
is left without an owner reference, it survives disabling the exporter, and a
missing one is surfaced as NotReady rather than leaving the DaemonSet pending.
A ServiceAccount is only deleted when it carries a ClusterPolicy owner
reference, so one provisioned by the user under the same name is left alone.
The SCC name and the openshift.io/scc annotation stay tied to the asset; only
its users entry follows the resolved ServiceAccount. On the RBAC bindings only
the exporter subject is rewritten, so the Prometheus subject is preserved.
GPUCluster embeds the same spec, so the DRA manifests honour it as well,
defaulting to nvidia-dcgm-exporter-dra.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UiFVTVAyEMmn68Xvo9LSTf
Signed-off-by: 백지명 <wlaud9805@gmail.com>
📝 WalkthroughWalkthroughThe 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 ServiceAccount names such as Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 winAdd a CRD validation regression test for
dcgmExporter.serviceAccount: {create: false}withoutname.The helper tests do not exercise the generated
ClusterPolicyCRD. The XValidation rule is the only safeguard before reconciliation; without it, the helper defaults the missing name tonvidia-dcgm-exporterand 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 winQuote
ServiceAccountNamein this manifest.When the configured string is
trueor123, the unquoted template output is decoded into the unstructured object as a YAML boolean or number. Kubernetes can then rejectspec.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 winAdd one non-DCGM state case.
Every subtest sets
stateNamestostate-dcgm-exporter, so the newisDCGMExporterguard 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 withstateNames: []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
⛔ Files ignored due to path filters (7)
api/nvidia/v1/zz_generated.deepcopy.gois excluded by!**/zz_generated.*.gobundle/manifests/nvidia.com_clusterpolicies.yamlis excluded by!bundle/manifests/nvidia.com_*.yamlbundle/manifests/nvidia.com_gpuclusters.yamlis excluded by!bundle/manifests/nvidia.com_*.yamlconfig/crd/bases/nvidia.com_clusterpolicies.yamlis excluded by!config/crd/bases/**config/crd/bases/nvidia.com_gpuclusters.yamlis excluded by!config/crd/bases/**deployments/gpu-operator/crds/nvidia.com_clusterpolicies.yamlis excluded by!deployments/gpu-operator/crds/**deployments/gpu-operator/crds/nvidia.com_gpuclusters.yamlis excluded by!deployments/gpu-operator/crds/**
📒 Files selected for processing (14)
api/nvidia/v1/clusterpolicy_types.goapi/nvidia/v1/clusterpolicy_types_test.gocontrollers/object_controls.gocontrollers/object_controls_test.gocontrollers/transforms_test.godeployments/gpu-operator/templates/clusterpolicy.yamldeployments/gpu-operator/values.yamlinternal/state/dcgm_exporter.gointernal/state/types.gomanifests/state-dcgm-exporter/0100_serviceaccount.yamlmanifests/state-dcgm-exporter/0300_rolebinding.yamlmanifests/state-dcgm-exporter/0310_clusterrolebinding.yamlmanifests/state-dcgm-exporter/0450_scc.openshift.yamlmanifests/state-dcgm-exporter/0700_daemonset.yaml
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review.
- quote the templated ServiceAccount name in the DRA manifests so a name that YAML would otherwise decode as a boolean or a number stays a string; - pin the CEL rule that rejects create: false without a name with a test over the generated ClusterPolicy and GPUCluster CRDs, since the helpers cannot catch that combination on their own; - cover a non-DCGM state in the ServiceAccount cleanup test so the ownership check staying scoped to the DCGM Exporter is exercised in both directions. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UiFVTVAyEMmn68Xvo9LSTf Signed-off-by: 백지명 <wlaud9805@gmail.com>
|
Thanks — all three points addressed in dd3685a. Quote the templated ServiceAccount name. Applied to CRD validation regression test. Added Non-DCGM state case. Added a subtest that runs
|
|
1. Helm GPUCluster does not pass through the field 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 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 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 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 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. |
Review feedback: the first commit wired the field on the ClusterPolicy path only, leaving the DRA path able to render the reference without honouring the contract behind it. - pass the field through deployments/gpu-operator/templates/gpucluster.yaml, so a Helm GPUCluster install can set it; - add a preSync hook to configurableState and use it for the exporter, so create: false reports NotReady when the ServiceAccount is missing rather than leaving the DaemonSet pending on an object the manifests deliberately omit; - refuse to take over a ServiceAccount that already exists under a configured name and is not owned by the CR. createOrUpdateObjs() would otherwise adopt it on the DRA path and hand it to garbage collection with the GPUCluster. The default name stays tolerant so an upgrade that lost the owner reference keeps converging; - reclaim the operator-owned default ServiceAccount once a different name takes over, on both paths. Renaming between two custom names is not tracked, so only the default is reclaimed; - cover the GPUCluster path: the rendered ServiceAccount, DaemonSet and RBAC subjects for a custom name, the omitted ServiceAccount for create: false, and each preSync branch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UiFVTVAyEMmn68Xvo9LSTf Signed-off-by: 백지명 <wlaud9805@gmail.com>
b864a60 to
ba6cc85
Compare
|
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 2. 3. No adoption on the DRA path. Rather than special-casing a kind inside 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.
|
|
Thanks — @100-JM covers the previous notes (
|
The create=false branch returned as soon as the user-provided ServiceAccount was found, so the default install path -- the operator creates nvidia-dcgm-exporter, the user then switches to their own ServiceAccount for IRSA or Workload Identity -- left the superseded default behind on both the ClusterPolicy and the DRA path. Reclaim it there as well, under the same ownership rule as the rename case, and skip the reclaim when the user brings the default name itself: that object is the one now being referenced. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UiFVTVAyEMmn68Xvo9LSTf Signed-off-by: 백지명 <wlaud9805@gmail.com>
|
Good catch — that is the transition the feature exists for, and both One guard added on top of your note: the reclaim is skipped when the user brings the default name itself ( Tests on both paths:
|
kvalliyurnatt
left a comment
kvalliyurnatt
left a comment
There was a problem hiding this comment.
Thanks for your contribution, left a few comments.
| if unmanaged { | ||
| // Surface the misconfiguration here rather than leaving the DaemonSet | ||
| // pending on a ServiceAccount that does not exist. | ||
| found := &corev1.ServiceAccount{} |
There was a problem hiding this comment.
nit: can we break the service account deletion and is renamed into separate functions
There was a problem hiding this comment.
Done in cec36fa — dcgmExporterServiceAccountRenamed(config) is now its own predicate, and the deletion lives in cleanupSupersededDCGMExporterServiceAccount() / releaseServiceAccountOwnership() rather than inline in ServiceAccount().
| return found, true | ||
| } | ||
|
|
||
| t.Run("default configuration creates the default ServiceAccount", func(t *testing.T) { |
There was a problem hiding this comment.
nit: could we make it a table driven test
There was a problem hiding this comment.
Done in cec36fa. TestDCGMExporterServiceAccountReconcile is table-driven now, and the deferred-cleanup cases moved to TestDCGMExporterSupersededServiceAccountCleanup, which exercises cleanupSupersededDCGMExporterServiceAccount directly.
| }, | ||
| } | ||
|
|
||
| t.Run("RoleBinding subject follows the configured ServiceAccount", func(t *testing.T) { |
There was a problem hiding this comment.
nit: can we make it a table driven test
There was a problem hiding this comment.
Done in cec36fa — TestDCGMExporterRBACSubjects is table-driven, one case per object (RoleBinding, ClusterRoleBinding, SCC).
| } | ||
| } | ||
|
|
||
| t.Run("create=false requires the ServiceAccount to exist", func(t *testing.T) { |
There was a problem hiding this comment.
nit: could this be rewritten as table driven ?
There was a problem hiding this comment.
Done in cec36fa. The preSync test is now TestDCGMExporterServiceAccountValidation (table-driven, validation only), and the behaviour that moved out of it is covered by TestDCGMExporterServiceAccountOwnershipReconcile. The three rendering tests were folded into a table-driven TestDCGMExporterServiceAccountRendering too, and the new guard has TestDCGMExporterServiceAccountAdoptionGuard / ...GuardWiring.
| return gpuv1.Disabled, nil | ||
| } | ||
|
|
||
| if unmanaged { |
There was a problem hiding this comment.
Here should we ensure that the new service account is not owned by the clusterPolicy ? else it will be garbage collected with the deletion of the Cluster policy
There was a problem hiding this comment.
Fixed in cec36fa. The create=false branch no longer deletes the ServiceAccount — deleting an object the user asked us to reference was wrong regardless. It now calls releaseServiceAccountOwnership(), which strips this ClusterPolicy's owner reference and updates the object, so it survives ClusterPolicy deletion.
| spec := cr.Spec.DCGMExporter | ||
| name := spec.GetServiceAccountName(dcgmExporterDefaultServiceAccountName) | ||
|
|
||
| if !spec.IsServiceAccountCreateEnabled() { |
There was a problem hiding this comment.
The same-name create: true → create: false transition leaves both the GPUCluster owner reference and the state label on the ServiceAccount. Similar to the Cluster Policy we need to ensure that the service account is no longer owned by. the GPUCluster
There was a problem hiding this comment.
Fixed in cec36fa. reconcileDCGMExporterServiceAccountOwnership (the new postSync hook) calls releaseServiceAccount() for the create=false case, which removes both this GPUCluster's controller reference and consts.StateLabel before updating the object — otherwise it is either garbage-collected with the CR or swept by the state cleanup. It is a no-op when neither is present, so a ServiceAccount the operator never owned is not written to.
Covered by TestDCGMExporterServiceAccountOwnershipReconcile (create=false releases a ServiceAccount the operator used to own).
| // Handing the exporter over to a user-provided ServiceAccount supersedes the one a | ||
| // default install created. Skipped when the user brings the default name itself, | ||
| // since that is the object now being referenced. | ||
| if obj.Name != DCGMExporterDefaultServiceAccountName { |
There was a problem hiding this comment.
This deletes the currently active ServiceAccount before the RoleBindings, SCC, and DaemonSet have been updated to reference the replacement. If a later control fails, the deployed DaemonSet remains configured with a deleted account. Please defer cleanup until the replacement resources have been reconciled successfully.
There was a problem hiding this comment.
Fixed in cec36fa. The reclaim moved out of ServiceAccount() into cleanupSupersededDCGMExporterServiceAccount(), which the state manager calls in step() only after the control loop returned Ready — so the RoleBindings, SCC and DaemonSet already reference the replacement by then. If any control fails, the previous ServiceAccount is still there and the deployed DaemonSet keeps working.
It also re-checks the state name, isStateEnabled and that the name actually changed, so it is a no-op for every other state and for the default configuration.
| if name == dcgmExporterDefaultServiceAccountName { | ||
| return nil | ||
| } | ||
| return s.deleteOwnedServiceAccount(ctx, cr, dcgmExporterDefaultServiceAccountName) |
There was a problem hiding this comment.
This cleanup runs from preSync, before syncObjects creates the replacement account and updates RBAC and the DaemonSet. Any subsequent API failure leaves the existing workload referencing a deleted ServiceAccount. Please perform this deletion only after the replacement objects have converged.
There was a problem hiding this comment.
Fixed in cec36fa, the same way as the ClusterPolicy path. configurableState gained a postSync hook that runs after syncObjects reports SyncStateReady, and the reclaim moved from checkDCGMExporterServiceAccount (preSync) into reconcileDCGMExporterServiceAccountOwnership (postSync). preSync is validation only now — it rejects a configuration the manifests cannot express and never touches cluster state.
| // Adopting an object the operator did not create would hand it to garbage collection | ||
| // on CR deletion, so a name that is already taken has to be opted into explicitly. | ||
| existing, err := s.getServiceAccount(ctx, name) | ||
| if err != nil && !apierrors.IsNotFound(err) { |
There was a problem hiding this comment.
There is a race between this ownership check and createObj: if another actor creates this ServiceAccount in between, the AlreadyExists path in createOrUpdateObjs unconditionally updates it with the GPUCluster owner reference. Please revalidate ownership after AlreadyExists before merging/updating the object.
There was a problem hiding this comment.
Good catch — fixed in cec36fa by revalidating on the AlreadyExists path, as you suggested.
stateSkel gained an optional adoptionGuard func(owner metav1.Object, current *unstructured.Unstructured) error. createOrUpdateObjs calls it right after the AlreadyExists + getObj, before mergeObjects/updateObj, so nothing is written when it returns an error. NewStateDCGMExporter sets it to guardDCGMExporterServiceAccountAdoption, which refuses a ServiceAccount this CR does not control, but keeps the operator default adoptable when it carries no owner references at all (upgrade from a release that did not set them). Every other state leaves the field nil and is unaffected.
TestDCGMExporterServiceAccountAdoptionGuardWiring drives the real createOrUpdateObjs against a ServiceAccount that appeared after the preSync check and asserts the object is left untouched.
Address the review on the configurable DCGM Exporter ServiceAccount.
Reclaiming the ServiceAccount a previous configuration superseded ran while
the state was still being applied: for the ClusterPolicy path inside
ServiceAccount(), which is the first control of the state, and for the DRA
path in the preSync hook. Both delete an object the RoleBindings, the SCC and
the DaemonSet still reference at that point, so a failure later in the same
sync leaves the operands pointing at a ServiceAccount that no longer exists.
The reclaim now runs once every control of the state reported Ready:
* ClusterPolicy: cleanupSupersededDCGMExporterServiceAccount(), called from
the state manager after the control loop.
* GPUCluster: reconcileDCGMExporterServiceAccountOwnership(), a new postSync
hook on configurableState that runs after syncObjects succeeded.
Handing a ServiceAccount over to the user with create=false no longer deletes
it either. The object stays, and the operator instead drops its controller
reference (and, on the DRA path, the state label) so it is neither
garbage-collected with the CR nor swept by the state cleanup.
Finally, checking ownership in preSync leaves a window: a ServiceAccount
created between that check and the create call would be adopted through the
AlreadyExists path, taking over an object the operator does not own. stateSkel
gained an optional adoptionGuard that runs on that path;
guardDCGMExporterServiceAccountAdoption vetoes the takeover, while keeping the
operator default adoptable when it carries no owner references (an upgrade
from a release that did not set them).
The tests for all of this are now table-driven.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UiFVTVAyEMmn68Xvo9LSTf
Signed-off-by: 백지명 <wlaud9805@gmail.com>
|
@kvalliyurnatt thanks for the thorough review — all nine comments are addressed in cec36fa, and I replied inline on each thread. Summary of the three substantive ones:
All tests flagged as nits are table-driven now, and the new behaviours have their own coverage. Local verification: PTAL when you get a chance. |
| return syncState, err | ||
| } | ||
|
|
||
| if s.postSync != nil { |
There was a problem hiding this comment.
The disabled path above returns before this ownership reconciliation runs. If one update disables the exporter and changes its managed ServiceAccount to create: false, handleStateObjectsDeletion still sees the state label and deletes the ServiceAccount. Please release its owner reference and state label before generic state cleanup, with a regression test for this combined transition.
There was a problem hiding this comment.
Fixed in e373618. configurableState gained a preDelete hook that runs before handleStateObjectsDeletion, and NewStateDCGMExporter registers releaseDCGMExporterServiceAccountOnDelete on it. The generic cleanup matches on consts.StateLabel alone, so a ServiceAccount taken over with create: false has to lose both the owner reference and that label before it runs — postSync was the wrong place because the deletion path returns before it.
The hook is a no-op for a nil spec and for a managed ServiceAccount, which is still meant to go with the state.
Two tests: TestDCGMExporterServiceAccountReleasedOnDelete covers the hook directly (released / never-owned / still-managed), and TestDCGMExporterSyncReleasesBeforeDeletion drives the real Sync() on a disabled exporter with create: false and asserts the ServiceAccount survives with neither the owner reference nor the state label. The second one fails if the hook call is removed from Sync().
|
|
||
| // Check if state is disabled and cleanup resource if exists | ||
| if !n.isStateEnabled(n.stateNames[n.idx]) { | ||
| if unmanaged { |
There was a problem hiding this comment.
This returns before releaseServiceAccountOwnership runs. If an operator-managed ServiceAccount transitions to create: false while the exporter is disabled, it retains the ClusterPolicy owner reference and can later be garbage-collected with the ClusterPolicy. Please release ownership before returning Disabled.
There was a problem hiding this comment.
Fixed in e373618. The unmanaged branch now reads the ServiceAccount and calls releaseServiceAccountOwnership before returning Disabled, so the transition to create: false drops this ClusterPolicy's owner reference even when the exporter is switched off in the same update. A ServiceAccount that is already gone is tolerated (IsNotFound → Disabled).
Covered by TestDCGMExporterServiceAccountReconcile/disabling_the_exporter_releases_a_ServiceAccount_the_operator_used_to_own, which seeds an operator-owned ServiceAccount, applies create: false plus enabled: false in one spec, and asserts the object survives without the owner reference. I verified it fails when the release is removed.
| // Objects a previous configuration superseded are reclaimed only once every control | ||
| // of this state converged: deleting them earlier would leave the operands that still | ||
| // reference them pointing at objects that no longer exist. | ||
| if result == gpuv1.Ready { |
There was a problem hiding this comment.
This condition prevents cleanup under the default exporter configuration. The optional read-pods ClusterRole and ClusterRoleBinding return Disabled when pod metadata enrichment is off, so result is not Ready even after all required operands converge. Consequently, the superseded default ServiceAccount is never removed. Please track convergence independently and treat intentionally disabled controls as converged.
There was a problem hiding this comment.
You are right, and this was the worst of the three — the reclaim was effectively dead code in the default configuration. rbacGates["nvidia-dcgm-exporter-read-pods"] is false unless pod metadata enrichment is on, so ClusterRole() and ClusterRoleBinding() return Disabled, the loop assigns that to result, and the result == Ready gate never opened.
Fixed in e373618 by tracking convergence separately, as you suggested:
if stat != gpuv1.Ready {
result = stat
}
if stat == gpuv1.NotReady {
converged = false
}Disabled means intentionally off, so it no longer holds the reclaim back; only a control that is still coming up does. TestDCGMExporterCleanupSurvivesDisabledControls drives step() with fake controls and pins one case per reported state — all-ready, ready+disabled, ready+not-ready, and disabled+not-ready together — so the distinction cannot quietly collapse again.
…t were missed
Address the second review round. All three gaps share a shape: a transition
that leaves the operand namespace in a state no control on that path fixes.
The deferred reclaim was unreachable under the default configuration. step()
gated it on the state's reported result, but a control that is intentionally
off reports Disabled, and with pod metadata enrichment disabled -- the default
-- the optional read-pods ClusterRole and ClusterRoleBinding always do. The
result was never Ready, so the superseded default ServiceAccount was never
removed. Convergence is now tracked apart from the reported state: only a
control that is still coming up (NotReady) holds the reclaim back.
Disabling the exporter and handing its ServiceAccount to the user in the same
update dropped the release on both paths.
* ClusterPolicy: ServiceAccount() returned Disabled before
releaseServiceAccountOwnership() could run, so the object kept this
ClusterPolicy's owner reference and was garbage-collected with it.
* GPUCluster: Sync() takes the deletion path when the state renders no
objects, which returns before postSync. The generic cleanup matches on the
state label alone, and a ServiceAccount taken over with create=false still
carries that label from when the operator managed it, so disabling the
exporter deleted an object the operator no longer owned. configurableState
gained a preDelete hook that runs before that cleanup.
Each fix has a regression test for the combined transition, and the step()
convergence rule is pinned per reported state, including the disabled-plus-
not-ready case.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: 백지명 <wlaud9805@gmail.com>
|
@kvalliyurnatt all three are fixed in The common thread in your review was transitions that no control on the taken path repairs, and the read-pods one was the most damaging: with pod metadata enrichment off — the default — The other two are the same combined transition on the two paths — handing the ServiceAccount to the user with Four regression tests, all mutation-checked — I reverted each fix in turn and confirmed the matching test fails:
Local verification: PTAL. |
|
@kvalliyurnatt a gentle nudge on this one — all three comments from your second round are fixed in Recap of what changed since your review:
Four regression tests, each mutation-checked (I reverted the fix and confirmed the matching test fails). Nothing is outstanding on my side — happy to rebase or split anything out if that makes the review easier. |
| } | ||
|
|
||
| syncState, err := s.syncObjects(ctx, cr, objs) | ||
| if err != nil || syncState != SyncStateReady { |
There was a problem hiding this comment.
Here postSync does not run until the DaemonSet is Ready. During a managed → create: false handoff without a change in a ServiceAccount name, the serviceAccount retains ownership until the update is complete, if for some reason the update is stuck and then the GPU cluster is deleted then user created service account will be Garbage collected, I think we should release ownership immediately and not wait for postsync to do it
There was a problem hiding this comment.
Moved the release into preSync (checkDCGMExporterServiceAccount), which already Gets the ServiceAccount for the create=false existence check, so it is handed back before syncObjects runs — whether or not the DaemonSet ever becomes Ready. postSync now only reclaims superseded ServiceAccounts, which is the part that genuinely has to wait for convergence because it deletes.
TestDCGMExporterSyncReleasesBeforeOperandsConverge drives Sync() with the DaemonSet pinned short of Ready (status subresource in the fake client) and checks the owner reference and state label are gone while the superseded default is still in place. In e3a00ba.
| // releaseServiceAccount drops this GPUCluster's controller reference and the state label | ||
| // from a ServiceAccount the user now owns. | ||
| func (s *configurableState) releaseServiceAccount(ctx context.Context, cr *nvidiav1alpha1.GPUCluster, sa *corev1.ServiceAccount) error { | ||
| changed := false |
There was a problem hiding this comment.
This releases any ServiceAccount controlled by the GPUCluster, even when it belongs to another operand. For example, configuring an existing DCGM or validator ServiceAccount with create: false removes that operand’s owner reference and state label. It also removes the state label from an otherwise unowned account. Could we only release metadata when the account is identified as previously managed by the DCGM Exporter?
There was a problem hiding this comment.
releaseServiceAccount now returns early unless labels["nvidia.com/gpu-operator.state"] == "state-dcgm-exporter". Every object syncObjects creates already carries that label with its own state's name, so it identifies "previously managed by this state" without introducing a new marker; another state's ServiceAccount — or an unowned one the user happens to reference — keeps its owner reference and label untouched.
Covered by the "leaves another state's ServiceAccount to that state" cases in the preSync and preDelete tests. In e3a00ba.
| // the state converged, so a failure part-way through reconciliation never leaves the | ||
| // DaemonSet referencing a ServiceAccount that has already been deleted. | ||
| func (n ClusterPolicyController) cleanupSupersededDCGMExporterServiceAccount(ctx context.Context) error { | ||
| if n.stateNames[n.idx] != "state-dcgm-exporter" || !n.isStateEnabled(n.stateNames[n.idx]) { |
There was a problem hiding this comment.
This enabled-state guard leaves the previous managed default behind when one update both disables the exporter and selects a differently named/BYO ServiceAccount. ServiceAccount() targets only the new name, and this cleanup then exits without deleting the old default. Could superseded-account cleanup also run after the disabled controls converge?
There was a problem hiding this comment.
The disabled branch no longer targets the configured name. disableDCGMExporterServiceAccount lists every ServiceAccount carrying the exporter label that this ClusterPolicy controls and deletes them all — the previous default included — with a user-provided one released first and excluded from the sweep. Since the state is off nothing references them any more, so there is nothing to wait for; the deferred cleanup in step() remains for the enabled path only.
Tests: "disabling the exporter after a rename deletes every ServiceAccount it created" and "disabling the exporter while handing over deletes the owned default and releases the user's ServiceAccount". In e3a00ba.
| // ServiceAccount the user has taken over. Without it the object stays garbage-collected | ||
| // together with the ClusterPolicy even though the operator no longer manages it. | ||
| func (n ClusterPolicyController) releaseServiceAccountOwnership(ctx context.Context, sa *corev1.ServiceAccount, logger logr.Logger) error { | ||
| if !metav1.IsControlledBy(sa, n.singleton) { |
There was a problem hiding this comment.
Similar comment here as well https://github.com/NVIDIA/gpu-operator/pull/2870/changes#r4007468891
There was a problem hiding this comment.
Same fix on this path in e3a00ba: releaseDCGMExporterServiceAccount requires app=nvidia-dcgm-exporter in addition to the controller reference. The asset has carried that label since before this PR, so ServiceAccounts created by an older operator qualify on upgrade; ServiceAccount() now also stamps it explicitly on what it creates rather than trusting the asset. Test: "create=false leaves another component's ServiceAccount to that component".
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| syncState, err := s.syncObjects(ctx, cr, objs) | ||
| if err != nil || syncState != SyncStateReady { | ||
| return syncState, err | ||
| } | ||
|
|
||
| if s.postSync != nil { | ||
| if err := s.postSync(ctx, s, cr); err != nil { | ||
| return SyncStateNotReady, err |
There was a problem hiding this comment.
🔴 User ServiceAccount deleted with GPUCluster
After switching to create=false, postSync delays ownership release until every manifest becomes Ready. An unready DaemonSet blocks release, so GPUCluster deletion garbage-collects the user’s ServiceAccount.
Learn more
A managed ServiceAccount carries a controller owner reference to the GPUCluster. Switching that same name to create=false transfers lifecycle responsibility to the user, so the owner reference must be removed. The release currently occurs in reconcileDCGMExporterServiceAccountOwnership, which is registered as postSync. This hook only runs after syncObjects reports the DaemonSet Ready. If readiness never arrives, the owner reference remains indefinitely, including through the GPUCluster finalizer path.
Example: GPUCluster cluster creates ServiceAccount metrics. The user changes metrics to create: false, but the exporter DaemonSet remains unready because its image cannot start. Deleting cluster then removes its finalizer and Kubernetes deletes metrics through its stale owner reference, although the user expected it to survive.
Recommended fix: Release the configured ServiceAccount before readiness gating once create=false has been validated, or explicitly release it during GPUCluster finalization. Preserve the existing rule that only this GPUCluster's owner reference and state label are removed.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Fixed in e3a00ba by releasing in preSync rather than during finalization — see the thread on line 100. With the hand-off no longer gated on readiness, the only window left is a spec change followed by a GPUCluster delete before any reconcile ran, which the controller has no opportunity to act on either way, so I did not add DCGM-specific logic to reconcileDelete.
| logger := n.logger.WithValues("ServiceAccount", DCGMExporterDefaultServiceAccountName, "Namespace", n.operatorNamespace) | ||
| return n.deleteOwnedServiceAccount(ctx, DCGMExporterDefaultServiceAccountName, logger) |
There was a problem hiding this comment.
🟡 Custom ServiceAccount renames leak identities
After changing managed ServiceAccount A to B, cleanup only checks the hard-coded default. The GPUCluster ownership reconcile repeats this restriction. Custom A remains owned and stale until the parent resource is deleted.
Learn more
The operator can create any configured ServiceAccount name, but cleanup has no record of the previously configured custom name. It therefore deletes nvidia-dcgm-exporter only. An old custom ServiceAccount keeps its owner reference even after all exporter operands move to the new name. Repeated renames accumulate unused identities for the lifetime of the ClusterPolicy or GPUCluster.
Example: Configure {name: metrics-a} and let the operator create it. Then change the configuration to {name: metrics-b}. The operands and RBAC move to metrics-b, but metrics-a remains in the namespace; changing back to the default leaves both custom accounts behind.
Recommended fix: Discover and delete superseded ServiceAccounts owned by the current CR, rather than checking only the default name. Add a component-specific label to managed ServiceAccounts and list by that label, then retain only the currently configured name. Verify ownership before every deletion.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Both paths now discover superseded ServiceAccounts by label — app=nvidia-dcgm-exporter on the ClusterPolicy path, the state label on the GPUCluster path — and delete every one the CR controls except the configured name, instead of checking the default name. The renamed short-circuit is gone as well, so switching back to the default reclaims the custom ones left behind.
Tests in both packages: "renaming from one custom name to another reclaims the previous one" and "switching back to the default reclaims every custom ServiceAccount left behind". In e3a00ba.
| // TestDCGMExporterServiceAccountReconcile covers the ServiceAccount lifecycle for the | ||
| // DCGM Exporter: the operator honours a configured name and, when the ServiceAccount is | ||
| // supplied by the user, only references it -- it is never created, adopted or deleted. | ||
|
|
||
| // TestDCGMExporterServiceAccountReconcile covers the ServiceAccount lifecycle for the | ||
| // DCGM Exporter: the operator honours a configured name, and a ServiceAccount supplied by | ||
| // the user is only referenced -- never created, adopted, mutated or deleted, and never | ||
| // left carrying a ClusterPolicy owner reference that would garbage-collect it. |
There was a problem hiding this comment.
🔍 Duplicate generated test comment
Two consecutive comments describe TestDCGMExporterServiceAccountReconcile. Remove the stale first version to keep the change scoped and comments purposeful.
Was this helpful? React with 👍 or 👎 to provide feedback.
The hand-off and the clean-up of the exporter's ServiceAccount relied on two things: the hard-coded default name, and the CR's controller reference. Neither identifies "a ServiceAccount this component created", which is what every remaining review finding came down to. - Releasing a ServiceAccount the user took over with create=false waited for postSync, i.e. for the DaemonSet to become Ready. One that never does would keep the owner reference on the user's object for good, and deleting the GPUCluster would garbage-collect it. The release now runs in preSync, before the operands sync; only the reclaim still waits for convergence, since that one deletes. - The release matched on ownership alone, but the CR controls every operand's ServiceAccount. Pointing create=false at the driver's would strip its owner reference and state label. Both paths now release only a ServiceAccount carrying the exporter's own label. - Superseded ServiceAccounts were reclaimed by the default name only, so renaming from one custom name to another, or back to the default, left the previous ones owned and stale. Both paths now list by label and delete every one this CR controls except the configured name. - On the ClusterPolicy path the disabled branch targeted the configured name only, so disabling the exporter and renaming in one update left the previous default behind. It now sweeps every ServiceAccount the exporter created, handing a user-provided one back first. The labels already exist on everything the operator created before this change -- the state label on the GPUCluster path, app=nvidia-dcgm-exporter from the asset on the ClusterPolicy path -- so upgraded clusters are covered; ServiceAccount() stamps the latter explicitly rather than trusting the asset. Signed-off-by: 백지명 <wlaud9805@gmail.com>
|
e3a00ba addresses the four open threads plus the two Devin findings. They shared one root cause — nothing identified "a ServiceAccount this component created", only the hard-coded default name and the CR's controller reference — so the fix is the same on both paths: identify the exporter's ServiceAccounts by label (the existing state label for GPUCluster,
Both labels predate this PR on everything the operator created, so upgraded clusters are covered. Unit tests across the module and |
3 participants
Description
Closes #2729.
The DCGM Exporter ServiceAccount name is hardcoded to
nvidia-dcgm-exporteracross 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.serviceAccountwith the{name, create}shape agreed in the issue:{name: X}X; every exporter operand references it{name: X, create: false}Xmust already exist in the operator namespace; the operator only references it{create: false}without a nameA 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
NotReadyinstead 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:
openshift.io/sccannotation on the DaemonSet stay tied to the asset name; only theusersentry follows the resolved ServiceAccount.0500_prom_rolebinding_openshift.yamlis preserved.DCGMExporterSpecis shared, so the DRA manifests honour the same field, defaulting tonvidia-dcgm-exporter-dra.create: false.DCGM Exporter is the only component touched.
Checklist
make lint)make validate-generated-assets)make validate-modules)Testing
make unit-testpasses (no failures) andmake lint,make fmt,make license-check,make validate-generated-assets,make validate-modulesare 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: falsereportsNotReadywhen 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 SCCusersentry 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