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}
| // 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.
There was a problem hiding this comment.
Removed the duplicate test comment in f0de2d2 while rechecking the earlier review 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 |
There was a problem hiding this comment.
Thanks for working through the earlier feedback. I still see three correctness gaps, called out inline.
The first two come from the same underlying problem: both paths use the parent CR owner reference to decide whether a ServiceAccount belongs to the exporter, but every operand has that same owner. Also I think It would help to share the small ownership policy pieces—a dedicated exporter marker, the managed-account predicate, owner-reference removal, and the conflict error.
| if err != nil { | ||
| return gpuv1.NotReady, err | ||
| } | ||
| if !owned { |
There was a problem hiding this comment.
This only tells us that the ClusterPolicy owns the ServiceAccount. Since it also owns the ServiceAccounts for the other operands, a name such as nvidia-driver passes this check and the exporter ends up using the driver's identity. There's also a race below: if an unrelated account appears between this Get and Create, AlreadyExists is treated as success without rechecking it. Can we require the exporter marker here and fetch/revalidate the object on AlreadyExists?
There was a problem hiding this comment.
Both fixed in 815a325.
The check now requires the exporter marker (app=nvidia-dcgm-exporter) alongside the controller reference, so nvidia-driver is refused instead of handing the exporter another operand's identity. And AlreadyExists no longer passes unexamined — the same check runs again after the create call, so an account that appeared in between is rejected rather than used.
Covered by "a configured name refuses another operand's ServiceAccount" and TestDCGMExporterServiceAccountCreateRace, which reproduces the window with a fake-client interceptor (first Get reports the name free, the object is present by the time Create runs) and asserts the intruder keeps neither our owner reference nor our marker.
| if err != nil && !apierrors.IsNotFound(err) { | ||
| return err | ||
| } | ||
| if err == nil && !metav1.IsControlledBy(existing, cr) { |
There was a problem hiding this comment.
Being controlled by this GPUCluster doesn't mean the account belongs to the exporter. A custom name can point at a sibling account such as nvidia-dcgm-dra or nvidia-dra-validator, and createOrUpdateObjs will then stamp the exporter state label onto it. Can we require StateLabel == state-dcgm-exporter both here and in the adoption guard?
There was a problem hiding this comment.
Fixed in 815a325 — StateLabel == state-dcgm-exporter is now required in both places, via the shared ownership.IsManaged predicate.
Worth spelling out why this one was worse than its ClusterPolicy twin: adoption did not just reference the sibling account, it went on to relabel it. createOrUpdateObjs calls addStateSpecificLabels before the update, so nvidia-dcgm-dra would have come back carrying state-dcgm-exporter and left its own state's cleanup unable to find it.
The default-name tolerance below is kept as is: an account with no owner reference at all cannot be a sibling operand's, so it does not reopen this case.
Tests: "a configured name refuses a sibling operand's ServiceAccount" in the preSync table, plus "a sibling operand's ServiceAccount is refused" and "a ServiceAccount this CR controls but no state claims is refused" in the adoption-guard table.
| // 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 converged { |
There was a problem hiding this comment.
I don't think converged is enough to make this deletion safe. When there are no detected GPU nodes, DaemonSet() returns Ready without updating an existing DaemonSet. During a ServiceAccount rename, that lets us delete the old account while the live DaemonSet still references it. Can we either verify the deployed DaemonSet uses the configured account before cleanup, or have the skipped DaemonSet report that it hasn't converged?
There was a problem hiding this comment.
Right — converged only says every control returned, not that the operands moved. Took the first of your two options in 815a325: the reclaim now reads the deployed DaemonSet and only proceeds when spec.template.spec.serviceAccountName matches the configured account. A DaemonSet that is absent means nothing references an account, so that case reclaims freely; anything else defers to the next reconcile.
I did not make the skipped DaemonSet report not-converged because DaemonSet() is shared by every operand, so returning something other than Ready on a GPU-less cluster would change ClusterPolicy status well beyond this PR. Happy to switch if you would rather have it at that level.
Tests: "a DaemonSet still on the previous account defers the reclaim" and "no deployed DaemonSet reclaims freely".
…lone Both controllers set their own controller reference on every operand object they create, so ownership answers "does this CR own it", never "which operand created it". Where the name is user-configurable that distinction decides whether an account is ours to create, adopt, release or delete. - A configured name pointing at a sibling operand's account passed the takeover check on both paths: nvidia-driver on the ClusterPolicy path left the exporter running with the driver's identity, and nvidia-dcgm-dra on the GPUCluster path was adopted and relabelled into this state. Both now require the operand's marker alongside the controller reference, in the pre-create check and in the adoption guard. - The ClusterPolicy path treated AlreadyExists as success, so an account appearing between the check and the create was used without being looked at. It is now revalidated before the control reports Ready. - The superseded-account reclaim ran on convergence alone, but DaemonSet() reports Ready without touching the live object when no GPU node is detected, so a rename could delete an account the deployed DaemonSet still referenced. The reclaim now confirms the deployed DaemonSet uses the configured account, and defers otherwise. The ownership policy the two paths were duplicating -- the marker, the managed-account predicate, owner-reference removal and the conflict error -- now lives in internal/ownership. Each path supplies its own marker: the app label on the ClusterPolicy side, the state label on the GPUCluster side. Both are labels the operator already applied before this change, so accounts created by an earlier release are still recognized on upgrade. Signed-off-by: 백지명 <wlaud9805@gmail.com>
|
815a325 addresses all three, and pulls the shared ownership policy into You named the root cause exactly: both controllers set their own controller reference on every operand object, so ownership answers "does this CR own it", never "which operand created it". Everywhere the ServiceAccount name is user-configurable, that second question is the one that matters. The package now holds the marker, the managed-account predicate, owner-reference removal and the conflict error; each path supplies its own marker — the On top of the three findings I swept the rest of the change for the same two shapes:
Full unit suite, |
There was a problem hiding this comment.
🔍 Generated output provenance
The PR changes generated deepcopy and CRD assets. Confirm make generate, make manifests, and make sync-crds produced these exact outputs.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Confirmed. Ran make generate manifests sync-crds on 815a325 and the working tree came back clean, so the committed deepcopy and CRD assets reproduce exactly from the API types in this PR.
| preSync: checkDCGMExporterServiceAccount, | ||
| postSync: reclaimSupersededDCGMExporterServiceAccounts, | ||
| preDelete: releaseDCGMExporterServiceAccountOnDelete, |
There was a problem hiding this comment.
🔴 Immediate deletion removes handed-off account
Deleting a GPUCluster after setting create=false can bypass preDelete and leave the ServiceAccount owned. Garbage collection then deletes the user-taken-over account.
Learn more
The hand-off runs only from the DCGM exporter state's Sync method through preSync or preDelete. GPUCluster deletion bypasses state synchronization and goes directly through reconcileDelete, which removes the finalizer after draining DaemonSets. If the ServiceAccount still has the GPUCluster controller reference, Kubernetes deletes it when the CR disappears. The ClusterPolicy path has the same race because it has no finalizer to complete a pending hand-off before deletion.
Example: An operator-managed metrics-sa exists. A user updates the CR to {name: metrics-sa, create: false} and immediately deletes the CR before its next reconciliation. The owner reference remains, so metrics-sa is garbage-collected instead of surviving as user-provided.
Recommended fix: Complete ServiceAccount hand-off from the CR deletion paths before releasing their finalizers or owner objects. GPUCluster can invoke state-specific release logic from reconcileDelete; ClusterPolicy needs equivalent deletion protection, likely a finalizer, for the same transition.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Agreed that this window is real, and that it is distinct from the readiness one — but I would rather not close it in this PR.
What is left needs the user to change the spec and delete the CR before any reconcile observes the change. Closing it means teaching the deletion paths about this operand: reconcileDelete would have to read spec.dcgmExporter.serviceAccount and release before dropping the finalizer, and ClusterPolicy has no finalizer at all, so it would need a new one. A finalizer on ClusterPolicy is a much larger change than a DCGM Exporter ServiceAccount feature should carry — it affects upgrades and introduces a way for deletion to get stuck — and fixing only the GPUCluster side would leave the two paths behaving differently, which is what most of this PR has been spent removing.
So I have left it as is deliberately rather than by oversight. @kvalliyurnatt, happy to follow up in a separate issue covering both controllers, or to take the GPUCluster half here if you would rather not leave the gap open.
| if !dcgmExporterServiceAccountMarker.Matches(sa.Labels) { | ||
| return nil | ||
| } | ||
| ownership.ReleaseOwner(sa, cr.GetUID()) | ||
| delete(sa.Labels, consts.StateLabel) |
There was a problem hiding this comment.
🔴 Self-labelled user accounts are deleted
When the exporter is disabled with create=false, releaseServiceAccount preserves a user-owned account carrying the state label. Generic cleanup then deletes every object with that label.
Learn more
The disabled path invokes releaseDCGMExporterServiceAccountOnDelete before deleteStateRelatedObjects. That cleanup selects objects only by the state label and does not check ownership. Returning here therefore leaves this ServiceAccount in the cleanup selection, even though the new ownership check correctly identifies it as user-owned.
Example: A user creates byo-sa with nvidia.com/gpu.deploy.state=state-dcgm-exporter, configures create: false, and disables DCGM Exporter. The release hook preserves the label, then state cleanup deletes byo-sa; the account was expected to survive unchanged.
Recommended fix: Separate hand-off during normal reconciliation from protection during state deletion. The pre-delete path must prevent generic cleanup from deleting the configured unmanaged ServiceAccount without mutating user-owned metadata, such as by excluding that object from deletion based on ownership. Add a full Sync regression test for a disabled exporter and a self-labelled user account.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Correct, and thanks for catching it — fixed in 58170be with the suggested gate.
This was the one site still on the marker alone. The marker answers "an exporter account", not "an account this CR owns", so releasing on it edited an object the operator never created; ownership.IsManaged was already doing both halves everywhere else and now does here too.
Added "create=false leaves an account the user labelled themselves alone" to the preDelete table, asserting the user's label survives untouched.
There was a problem hiding this comment.
The new IsManaged check prevents preDelete from editing this account, but I think the original deletion bug is still there. Sync immediately calls handleStateObjectsDeletion, which lists objects by the state label alone and deletes this same ServiceAccount. The new test only calls the hook directly, so it does not exercise that cleanup. Could we add a full disabled-Sync test with selfLabelledServiceAccount and exclude the configured unmanaged account from generic deletion?
There was a problem hiding this comment.
@kvalliyurnatt You are right — my earlier reply addressed the metadata mutation but did not fix the subsequent deletion. Fixed in b890f3b.
The disabled-Sync fixture also used a REST mapper that reported the cleanup kinds as unserved, so the sweep could be skipped. It now serves ServiceAccount, ConfigMap, and DaemonSet. Full Sync tests preserve the self-labelled BYO account and another self-labelled user account without changing their metadata, while removing the previous managed account and the ConfigMap. A second Sync reaches Ignore, so preserved accounts do not keep cleanup pending.
An exporter-only deletion filter excludes the configured create=false account even if the cache still contains its old owner metadata. Other ServiceAccount candidates must have both the exporter marker and this CR's controller reference. The existing cleanup selection for other operands is retained. The new full-Sync regression fails on the previous code and passes with the fix.
releaseServiceAccount gated on the marker alone, so an account the user labelled themselves would have that label stripped and the object updated, although the operator never created it. The unmanaged contract says a ServiceAccount the user brings is only referenced. The marker answers "an exporter account", not "an account this CR owns"; both halves are needed, which is what ownership.IsManaged already does for the reclaim path. This was the one site left on the marker alone. Signed-off-by: 백지명 <wlaud9805@gmail.com>
| // to be revalidated: AlreadyExists must not silently hand the exporter an | ||
| // account it does not manage. | ||
| if guardTakeover { | ||
| if err := n.checkDCGMExporterServiceAccountAvailable(ctx, obj); err != nil { |
There was a problem hiding this comment.
This recheck still goes through mgr.GetClient(), so it can read a stale cache immediately after Create returned AlreadyExists. If the cache still says NotFound, checkDCGMExporterServiceAccountAvailable returns nil and we accept an account we haven't actually validated. Could this treat NotFound on this second check as NotReady and retry?
There was a problem hiding this comment.
Fixed in b890f3b. The ownership check now preserves NotFound. Only the initial pre-create check tolerates it; after AlreadyExists, a cache miss returns NotReady with the error so reconciliation retries instead of accepting an unvalidated account.
The table-driven create-race test now covers both an intruder visible on the second read and a cache still returning NotFound on that read. The stale-second-read case fails against the previous implementation.
| continue | ||
| } | ||
| logger.V(1).Info("Removing a dcgm-exporter ServiceAccount the operator no longer uses", "Name", sa.Name) | ||
| if err := n.client.Delete(ctx, sa); err != nil && !apierrors.IsNotFound(err) { |
There was a problem hiding this comment.
We know the object returned by List was managed, but client.Delete doesn't include its UID automatically. If that ServiceAccount is deleted and recreated under the same name before this call, this can delete the new user-owned account. Can we add a UID precondition from the listed object and leave the replacement alone on conflict?
There was a problem hiding this comment.
Fixed in b890f3b. Cleanup now passes the observed UID and resourceVersion as delete preconditions through the shared ownership.DeleteObserved helper. UID protects a same-name replacement; resourceVersion also protects an ownership handoff on the same object after it was listed.
NotFound and Conflict leave the object alone; other errors are returned. Regression tests replace the account between List and Delete in both rename cleanup and disabled cleanup and assert that the new user-owned account survives. The fake client does not enforce UID preconditions, so the interceptor models the API server's UID conflict response.
| } | ||
| log.FromContext(ctx).V(consts.LogLevelInfo).Info( | ||
| "Removing a dcgm-exporter ServiceAccount superseded by the configured one", "Name", sa.Name) | ||
| if err := s.client.Delete(ctx, sa); err != nil && !apierrors.IsNotFound(err) { |
There was a problem hiding this comment.
Same replacement race here: the ownership check applies to the listed UID, while this delete is sent by name with no UID precondition. A newly recreated user-owned account can be removed. Can we pass the observed UID as a delete precondition and treat a conflict as the object having been replaced?
There was a problem hiding this comment.
Fixed in b890f3b using the same ownership.DeleteObserved helper as the ClusterPolicy path: UID and resourceVersion preconditions, with NotFound/Conflict tolerated and other errors propagated.
The protection also covers the generic disabled-state sweep. Regression tests exercise both the post-convergence reclaim and disabled Sync with a same-name replacement appearing immediately before deletion, and verify the replacement survives.
Signed-off-by: 백지명 <wlaud9805@gmail.com>
|
Pushed b890f3b to address the latest four review comments. The fixes cover stale-cache validation after AlreadyExists, conditional deletion in both controller paths, and protection of unmanaged ServiceAccounts in the full disabled-state cleanup. Validation:
For the previously discussed window where a user sets create=false and immediately deletes the CR before reconciliation observes the handoff, I propose tracking it in a separate issue. Closing it consistently requires a ClusterPolicy finalizer and corresponding GPUCluster deletion-path handling, including upgrade and deletion-failure behavior. That lifecycle change remains outside this commit; the current handoff still takes effect during reconciliation. Please let me know if you would prefer that work to block this PR. |
| // Surface the misconfiguration here rather than leaving the DaemonSet | ||
| // pending on a ServiceAccount that does not exist. | ||
| found := &corev1.ServiceAccount{} | ||
| if err := n.client.Get(ctx, types.NamespacedName{Namespace: obj.Namespace, Name: obj.Name}, found); err != nil { |
There was a problem hiding this comment.
This detects a missing BYO account only when something else already triggered ClusterPolicy reconciliation. Once the policy is Ready, the controller does not watch ServiceAccounts and the Ready path has no periodic requeue. Deleting this account can therefore leave status Ready indefinitely while replacement exporter pods fail to start.
Could we add a ServiceAccount watch that maps changes in the operator namespace back to the active ClusterPolicy, or periodically validate Ready policies? The polling version is as small as:
return ctrl.Result{RequeueAfter: time.Minute}, nilThere was a problem hiding this comment.
Fixed in 535bc71 using the periodic revalidation option: the normal Ready path now returns RequeueAfter: time.Minute, matching the existing GPUCluster cadence. The existing NFD-missing polling and error paths are unchanged.
TestClusterPolicyRechecksReadyExporterServiceAccount runs the real Reconcile path through Ready with a scheduled follow-up, deletes the BYO account, verifies NotReady on the next reconciliation, restores it, and verifies Ready plus another scheduled follow-up. The restored account remains unowned. This detects deletion on the periodic check rather than immediately through a new watch.
| // to be revalidated: AlreadyExists must not silently hand the exporter an | ||
| // account it does not manage. | ||
| if guardTakeover { | ||
| if err := n.checkDCGMExporterServiceAccountOwnership(ctx, obj); err != nil { |
There was a problem hiding this comment.
This second Get is still cache-backed. If the previously managed account is deleted and an unowned account is recreated under the same name while the cache still holds the old object, both checks accept the old ownership but Create sees the replacement. We then continue and bind RBAC/SCC to an account we did not validate.
Could we branch on the first cached result: create only after NotFound, and for an existing managed account use its resource version as an optimistic precondition? That replaces the current always-Create API call without adding an uncached read.
base := found.DeepCopy()
if err := n.client.Patch(ctx, found,
client.MergeFromWithOptions(base, client.MergeFromWithOptimisticLock{}),
); err != nil {
return gpuv1.NotReady, err
}If the live account was replaced, the patch conflicts and reconciliation retries. The current test covers stale NotFound, but not a stale positive managed object.
There was a problem hiding this comment.
Fixed in 535bc71 as suggested. The first cached Get now selects the path: NotFound allows Create, while an existing managed account is validated with a resourceVersion-only MergeFromWithOptimisticLock patch. AlreadyExists from Create is returned for retry instead of triggering another cached ownership check. Later RBAC/SCC/DaemonSet controls cannot run when validation fails.
Tests cover a stale positive cache containing the previous managed UID, a same-UID ownership change, and a validation write failure. They assert that subsequent controls do not run and that the live account's metadata and imagePullSecrets remain untouched. I also added coverage for the analogous GPUCluster path, whose existing versioned Update rejects the stale account.
Beyond fake-client tests, I ran an isolated local Kubernetes 1.35 API server check: both stale-positive scenarios return Conflict, while the unchanged-account validation succeeds without changing its resourceVersion.
| func DeleteObserved(ctx context.Context, c client.Client, obj client.Object) error { | ||
| uid, version := obj.GetUID(), obj.GetResourceVersion() | ||
| err := c.Delete(ctx, obj, client.Preconditions{UID: &uid, ResourceVersion: &version}) | ||
| if apierrors.IsNotFound(err) || apierrors.IsConflict(err) { |
There was a problem hiding this comment.
Treating every conflict as success can abandon cleanup. A same-UID metadata update also changes resourceVersion, so the delete conflicts even though the account is still managed. ClusterPolicy can then report Ready with the obsolete account left behind and no guaranteed follow-up reconciliation.
Could we only swallow NotFound and return conflicts so the caller retries? A replacement remains safe: the next reconciliation sees the new UID/ownership and skips it.
if apierrors.IsNotFound(err) {
return nil
}
return errThere was a problem hiding this comment.
Fixed in 535bc71. DeleteObserved now ignores only NotFound; Conflict is returned so the caller schedules another reconciliation. UID and resourceVersion preconditions remain in place.
The tests now follow the retry through to its outcome: a same-name user-owned replacement survives and is skipped on the next pass; a still-managed account with a benign metadata change conflicts once and is then removed after its current version is observed. Both ClusterPolicy cleanup and GPUCluster disabled Sync are covered. A local Kubernetes API-server check also verified actual version/UID conflicts and the successful version-refreshed delete.
Validation for this commit: make unit-test, make fmt, make license-check, and make validate-generated-assets passed; Linux-target make lint reports 0 issues. No live GPU/OpenShift E2E test was performed. The previously separated immediate-CR-deletion/finalizer issue remains outside this change.
Signed-off-by: 백지명 <wlaud9805@gmail.com>
Signed-off-by: 백지명 <wlaud9805@gmail.com>
|
@kvalliyurnatt I completed another pass over the review history and both reconciliation paths. The earlier requests are addressed in 535bc71; f0de2d2 adds one further cleanup safeguard found during that audit. A GPUCluster rename could still reclaim the old ServiceAccount after observing either the previous ready DaemonSet from cache or the new template with readiness counters from its previous generation. Cleanup now checks the deployed identity, observed generation, and rollout completion in both controllers. The full-Sync regression fails in both cases before the fix and verifies that cleanup finishes once the new generation is ready. The duplicate test comment is also removed, and rendering tests cover Validation of the final changes:
I updated the PR description to reflect the final behavior and verification. The immediate-CR-deletion/finalizer window remains as described in the earlier thread; separating that follow-up is still subject to your agreement. No live GPU/OpenShift E2E test was run. These are local validation results; upstream CI success has not been verified for this head. Could you take another look when convenient? |
|
@100-JM As the PR has progressed I see it has gotten quite complex with so many different edge cases that we keep finding, I want to take a step back and see if we can revisit the design and see how we can make it simpler. I will leave a more detailed comment with a possible potential design, one thing I was thinking was following how we allow custom configmaps in the device plugin https://github.com/NVIDIA/k8s-device-plugin/blob/main/README.md#passing-configuration-to-the-plugin-via-a-configmap but I don't have a fully formed design yet, I would like to spend some time coming up with one before we make further changes. If you have any suggestions as well please feel free to share. |
This branch has not been deployed
3 participants
Description
Closes #2729.
Platforms that bind IRSA, Workload Identity, or PKI to a specific ServiceAccount currently cannot select that identity for the operator-managed DCGM Exporter. This adds
dcgmExporter.serviceAccountto ClusterPolicy, GPUCluster, and their Helm templates.nvidia-dcgm-exporterfor ClusterPolicy,nvidia-dcgm-exporter-drafor GPUCluster).{name: X}X; refuse a custom name already held by an unrelated account.{name: X, create: false}{create: false}without a nonempty nameThe DaemonSet and exporter RBAC subjects follow the configured name. The SCC name and
openshift.io/sccannotation remain unchanged; only its ServiceAccount user changes. Unrelated subjects, including Prometheus, remain unchanged. Cross-namespace identities and operator-managed ServiceAccount annotations are outside this change.Externally owned accounts retain their metadata and ownership. When an account previously managed by this exporter is switched to
create: false, reconciliation releases this CR's owner reference (and the GPUCluster state label) before applying or removing operands. Identifying such an account requires both the exporter marker and this CR's controller reference, so sibling operands' accounts remain untouched.Custom managed accounts are validated against the API server's resource version before later operands can use them. Superseded accounts are reclaimed only after the deployed DaemonSet references the configured identity and its latest generation has rolled out. Deletion uses UID and resourceVersion preconditions; conflicts trigger retries that preserve replacements and complete cleanup of accounts still managed by the exporter. Ready ClusterPolicies are revalidated every minute, matching GPUCluster's existing interval.
Validation
make unit-test,make fmt,make license-check, Linux-targetmake lint,make validate-modules, andmake validate-generated-assets.go test -racefor the changed lifecycle paths and Linux-targetmake build.helm lint,make validate-csv, andmake validate-helm-values.create: falsewith missing/empty names and preservation of numeric/boolean-looking string names.Remaining scope decision
Changing a managed account to
create: falseand immediately deleting its CR before reconciliation can still leave an owner reference for garbage collection. A follow-up covering both CR deletion paths/finalizers has been proposed in the review; maintainer agreement is still needed. This PR does not claim to close that window.No live GPU or OpenShift E2E test has been run; SCC behavior is covered by unit tests.