From c76cf0dc22fea36b128c3e82f1eeda3b49c62725 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=EB=B0=B1=EC=A7=80=EB=AA=85?= Date: Wed, 9 Sep 2026 14:07:59 +0900 Subject: [PATCH 1/9] Add configurable ServiceAccount for the DCGM Exporter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) Claude-Session: https://claude.ai/code/session_01UiFVTVAyEMmn68Xvo9LSTf Signed-off-by: 백지명 --- api/nvidia/v1/clusterpolicy_types.go | 49 ++++ api/nvidia/v1/clusterpolicy_types_test.go | 44 +++ api/nvidia/v1/zz_generated.deepcopy.go | 25 ++ .../manifests/nvidia.com_clusterpolicies.yaml | 22 ++ bundle/manifests/nvidia.com_gpuclusters.yaml | 22 ++ .../crd/bases/nvidia.com_clusterpolicies.yaml | 22 ++ config/crd/bases/nvidia.com_gpuclusters.yaml | 22 ++ controllers/object_controls.go | 98 ++++++- controllers/object_controls_test.go | 265 ++++++++++++++++++ controllers/transforms_test.go | 51 ++++ .../crds/nvidia.com_clusterpolicies.yaml | 22 ++ .../crds/nvidia.com_gpuclusters.yaml | 22 ++ .../gpu-operator/templates/clusterpolicy.yaml | 3 + deployments/gpu-operator/values.yaml | 8 + internal/state/dcgm_exporter.go | 6 + internal/state/types.go | 4 + .../0100_serviceaccount.yaml | 4 +- .../state-dcgm-exporter/0300_rolebinding.yaml | 2 +- .../0310_clusterrolebinding.yaml | 2 +- .../0450_scc.openshift.yaml | 2 +- .../state-dcgm-exporter/0700_daemonset.yaml | 2 +- 21 files changed, 691 insertions(+), 6 deletions(-) diff --git a/api/nvidia/v1/clusterpolicy_types.go b/api/nvidia/v1/clusterpolicy_types.go index 16d95220c5..c330de8c69 100644 --- a/api/nvidia/v1/clusterpolicy_types.go +++ b/api/nvidia/v1/clusterpolicy_types.go @@ -1059,6 +1059,11 @@ type DCGMExporterSpec struct { // +operator-sdk:gen-csv:customresourcedefinitions.specDescriptors.displayName="Service configuration for NVIDIA DCGM Exporter" ServiceSpec *DCGMExporterServiceConfig `json:"service,omitempty"` + // Optional: ServiceAccount configuration for NVIDIA DCGM Exporter + // +operator-sdk:gen-csv:customresourcedefinitions.specDescriptors=true + // +operator-sdk:gen-csv:customresourcedefinitions.specDescriptors.displayName="ServiceAccount configuration for NVIDIA DCGM Exporter" + ServiceAccount *DCGMExporterServiceAccountConfig `json:"serviceAccount,omitempty"` + // HostPID allows the DCGM-Exporter daemon set to access the host's PID namespace // +kubebuilder:validation:Optional // +operator-sdk:gen-csv:customresourcedefinitions.specDescriptors=true @@ -1148,6 +1153,30 @@ type DCGMExporterServiceConfig struct { InternalTrafficPolicy *corev1.ServiceInternalTrafficPolicy `json:"internalTrafficPolicy,omitempty"` } +// DCGMExporterServiceAccountConfig defines the ServiceAccount used by the NVIDIA +// DCGM Exporter DaemonSet. +// +kubebuilder:validation:XValidation:rule="!has(self.create) || self.create || (has(self.name) && size(self.name) > 0)",message="name is required when create is false" +type DCGMExporterServiceAccountConfig struct { + // Name of the ServiceAccount used by the NVIDIA DCGM Exporter DaemonSet. + // Defaults to the operator-managed ServiceAccount when left empty. + // +kubebuilder:validation:Optional + // +operator-sdk:gen-csv:customresourcedefinitions.specDescriptors=true + // +operator-sdk:gen-csv:customresourcedefinitions.specDescriptors.displayName="ServiceAccount name for NVIDIA DCGM Exporter" + // +operator-sdk:gen-csv:customresourcedefinitions.specDescriptors.x-descriptors="urn:alm:descriptor:com.tectonic.ui:text" + Name string `json:"name,omitempty"` + + // Create indicates whether the operator manages the lifecycle of the DCGM + // Exporter ServiceAccount. Defaults to true. When set to false, a + // ServiceAccount with the configured name has to already exist in the + // operator namespace; the operator then only references it and never + // creates, adopts, mutates or deletes it. + // +kubebuilder:validation:Optional + // +operator-sdk:gen-csv:customresourcedefinitions.specDescriptors=true + // +operator-sdk:gen-csv:customresourcedefinitions.specDescriptors.displayName="Create the ServiceAccount for NVIDIA DCGM Exporter" + // +operator-sdk:gen-csv:customresourcedefinitions.specDescriptors.x-descriptors="urn:alm:descriptor:com.tectonic.ui:booleanSwitch" + Create *bool `json:"create,omitempty"` +} + // DCGMSpec defines the properties for NVIDIA DCGM deployment type DCGMSpec struct { // Enabled indicates if deployment of NVIDIA DCGM Hostengine as a separate pod is enabled. @@ -2301,6 +2330,26 @@ func (e *DCGMExporterSpec) IsKubernetesPodMetadataEnabled() bool { return e.IsPodLabelsEnabled() || e.IsPodUIDEnabled() } +// GetServiceAccountName returns the name of the ServiceAccount referenced by the +// DCGM Exporter operands, falling back to defaultName when it is not configured. +func (e *DCGMExporterSpec) GetServiceAccountName(defaultName string) string { + if e.ServiceAccount == nil || e.ServiceAccount.Name == "" { + return defaultName + } + return e.ServiceAccount.Name +} + +// IsServiceAccountCreateEnabled returns true if the operator owns the lifecycle of +// the DCGM Exporter ServiceAccount. When false the ServiceAccount is supplied by +// the user and is never created, adopted, mutated or deleted by the operator. +func (e *DCGMExporterSpec) IsServiceAccountCreateEnabled() bool { + if e.ServiceAccount == nil || e.ServiceAccount.Create == nil { + // default is true if not specified by user + return true + } + return *e.ServiceAccount.Create +} + // IsEnabled returns true if gpu-feature-discovery is enabled(default) through gpu-operator func (g *GPUFeatureDiscoverySpec) IsEnabled() bool { if g.Enabled == nil { diff --git a/api/nvidia/v1/clusterpolicy_types_test.go b/api/nvidia/v1/clusterpolicy_types_test.go index 93b15c2557..d432934fc5 100644 --- a/api/nvidia/v1/clusterpolicy_types_test.go +++ b/api/nvidia/v1/clusterpolicy_types_test.go @@ -86,3 +86,47 @@ func TestImagePath(t *testing.T) { assert.ErrorContains(t, err, "invalid nil spec") }) } + +func TestDCGMExporterServiceAccount(t *testing.T) { + const defaultName = "nvidia-dcgm-exporter" + + testCases := map[string]struct { + serviceAccount *DCGMExporterServiceAccountConfig + expectedName string + expectedCreate bool + }{ + "unset falls back to the default and is operator-managed": { + serviceAccount: nil, + expectedName: defaultName, + expectedCreate: true, + }, + "empty name falls back to the default": { + serviceAccount: &DCGMExporterServiceAccountConfig{}, + expectedName: defaultName, + expectedCreate: true, + }, + "name only stays operator-managed": { + serviceAccount: &DCGMExporterServiceAccountConfig{Name: "metrics-identity"}, + expectedName: "metrics-identity", + expectedCreate: true, + }, + "create=false marks the ServiceAccount as user-provided": { + serviceAccount: &DCGMExporterServiceAccountConfig{Name: "byo-sa", Create: new(false)}, + expectedName: "byo-sa", + expectedCreate: false, + }, + "create=true is explicit operator management": { + serviceAccount: &DCGMExporterServiceAccountConfig{Name: "managed-sa", Create: new(true)}, + expectedName: "managed-sa", + expectedCreate: true, + }, + } + + for name, tc := range testCases { + t.Run(name, func(t *testing.T) { + spec := &DCGMExporterSpec{ServiceAccount: tc.serviceAccount} + require.Equal(t, tc.expectedName, spec.GetServiceAccountName(defaultName)) + require.Equal(t, tc.expectedCreate, spec.IsServiceAccountCreateEnabled()) + }) + } +} diff --git a/api/nvidia/v1/zz_generated.deepcopy.go b/api/nvidia/v1/zz_generated.deepcopy.go index 9e936de60d..e23b75c9d2 100644 --- a/api/nvidia/v1/zz_generated.deepcopy.go +++ b/api/nvidia/v1/zz_generated.deepcopy.go @@ -344,6 +344,26 @@ func (in *DCGMExporterMetricsConfig) DeepCopy() *DCGMExporterMetricsConfig { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *DCGMExporterServiceAccountConfig) DeepCopyInto(out *DCGMExporterServiceAccountConfig) { + *out = *in + if in.Create != nil { + in, out := &in.Create, &out.Create + *out = new(bool) + **out = **in + } +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new DCGMExporterServiceAccountConfig. +func (in *DCGMExporterServiceAccountConfig) DeepCopy() *DCGMExporterServiceAccountConfig { + if in == nil { + return nil + } + out := new(DCGMExporterServiceAccountConfig) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *DCGMExporterServiceConfig) DeepCopyInto(out *DCGMExporterServiceConfig) { *out = *in @@ -414,6 +434,11 @@ func (in *DCGMExporterSpec) DeepCopyInto(out *DCGMExporterSpec) { *out = new(DCGMExporterServiceConfig) (*in).DeepCopyInto(*out) } + if in.ServiceAccount != nil { + in, out := &in.ServiceAccount, &out.ServiceAccount + *out = new(DCGMExporterServiceAccountConfig) + (*in).DeepCopyInto(*out) + } if in.HostPID != nil { in, out := &in.HostPID, &out.HostPID *out = new(bool) diff --git a/bundle/manifests/nvidia.com_clusterpolicies.yaml b/bundle/manifests/nvidia.com_clusterpolicies.yaml index e8d0be746c..66f609a24f 100644 --- a/bundle/manifests/nvidia.com_clusterpolicies.yaml +++ b/bundle/manifests/nvidia.com_clusterpolicies.yaml @@ -686,6 +686,28 @@ spec: ingress methods for a service type: string type: object + serviceAccount: + description: 'Optional: ServiceAccount configuration for NVIDIA + DCGM Exporter' + properties: + create: + description: |- + Create indicates whether the operator manages the lifecycle of the DCGM + Exporter ServiceAccount. Defaults to true. When set to false, a + ServiceAccount with the configured name has to already exist in the + operator namespace; the operator then only references it and never + creates, adopts, mutates or deletes it. + type: boolean + name: + description: |- + Name of the ServiceAccount used by the NVIDIA DCGM Exporter DaemonSet. + Defaults to the operator-managed ServiceAccount when left empty. + type: string + type: object + x-kubernetes-validations: + - message: name is required when create is false + rule: '!has(self.create) || self.create || (has(self.name) && + size(self.name) > 0)' serviceMonitor: description: 'Optional: ServiceMonitor configuration for NVIDIA DCGM Exporter' diff --git a/bundle/manifests/nvidia.com_gpuclusters.yaml b/bundle/manifests/nvidia.com_gpuclusters.yaml index f7430a1778..6f90544121 100644 --- a/bundle/manifests/nvidia.com_gpuclusters.yaml +++ b/bundle/manifests/nvidia.com_gpuclusters.yaml @@ -590,6 +590,28 @@ spec: ingress methods for a service type: string type: object + serviceAccount: + description: 'Optional: ServiceAccount configuration for NVIDIA + DCGM Exporter' + properties: + create: + description: |- + Create indicates whether the operator manages the lifecycle of the DCGM + Exporter ServiceAccount. Defaults to true. When set to false, a + ServiceAccount with the configured name has to already exist in the + operator namespace; the operator then only references it and never + creates, adopts, mutates or deletes it. + type: boolean + name: + description: |- + Name of the ServiceAccount used by the NVIDIA DCGM Exporter DaemonSet. + Defaults to the operator-managed ServiceAccount when left empty. + type: string + type: object + x-kubernetes-validations: + - message: name is required when create is false + rule: '!has(self.create) || self.create || (has(self.name) && + size(self.name) > 0)' serviceMonitor: description: 'Optional: ServiceMonitor configuration for NVIDIA DCGM Exporter' diff --git a/config/crd/bases/nvidia.com_clusterpolicies.yaml b/config/crd/bases/nvidia.com_clusterpolicies.yaml index e8d0be746c..66f609a24f 100644 --- a/config/crd/bases/nvidia.com_clusterpolicies.yaml +++ b/config/crd/bases/nvidia.com_clusterpolicies.yaml @@ -686,6 +686,28 @@ spec: ingress methods for a service type: string type: object + serviceAccount: + description: 'Optional: ServiceAccount configuration for NVIDIA + DCGM Exporter' + properties: + create: + description: |- + Create indicates whether the operator manages the lifecycle of the DCGM + Exporter ServiceAccount. Defaults to true. When set to false, a + ServiceAccount with the configured name has to already exist in the + operator namespace; the operator then only references it and never + creates, adopts, mutates or deletes it. + type: boolean + name: + description: |- + Name of the ServiceAccount used by the NVIDIA DCGM Exporter DaemonSet. + Defaults to the operator-managed ServiceAccount when left empty. + type: string + type: object + x-kubernetes-validations: + - message: name is required when create is false + rule: '!has(self.create) || self.create || (has(self.name) && + size(self.name) > 0)' serviceMonitor: description: 'Optional: ServiceMonitor configuration for NVIDIA DCGM Exporter' diff --git a/config/crd/bases/nvidia.com_gpuclusters.yaml b/config/crd/bases/nvidia.com_gpuclusters.yaml index f7430a1778..6f90544121 100644 --- a/config/crd/bases/nvidia.com_gpuclusters.yaml +++ b/config/crd/bases/nvidia.com_gpuclusters.yaml @@ -590,6 +590,28 @@ spec: ingress methods for a service type: string type: object + serviceAccount: + description: 'Optional: ServiceAccount configuration for NVIDIA + DCGM Exporter' + properties: + create: + description: |- + Create indicates whether the operator manages the lifecycle of the DCGM + Exporter ServiceAccount. Defaults to true. When set to false, a + ServiceAccount with the configured name has to already exist in the + operator namespace; the operator then only references it and never + creates, adopts, mutates or deletes it. + type: boolean + name: + description: |- + Name of the ServiceAccount used by the NVIDIA DCGM Exporter DaemonSet. + Defaults to the operator-managed ServiceAccount when left empty. + type: string + type: object + x-kubernetes-validations: + - message: name is required when create is false + rule: '!has(self.create) || self.create || (has(self.name) && + size(self.name) > 0)' serviceMonitor: description: 'Optional: ServiceMonitor configuration for NVIDIA DCGM Exporter' diff --git a/controllers/object_controls.go b/controllers/object_controls.go index c1bf59e4cf..2af65852bb 100644 --- a/controllers/object_controls.go +++ b/controllers/object_controls.go @@ -37,6 +37,7 @@ import ( corev1 "k8s.io/api/core/v1" nodev1 "k8s.io/api/node/v1" nodev1beta1 "k8s.io/api/node/v1beta1" + rbacv1 "k8s.io/api/rbac/v1" apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -117,6 +118,9 @@ const ( DCGMRemoteEngineEnvName = "DCGM_REMOTE_HOSTENGINE_INFO" // DCGMDefaultPort indicates default port bound to DCGM host engine DCGMDefaultPort = 5555 + // DCGMExporterDefaultServiceAccountName is the ServiceAccount the DCGM Exporter + // operands reference unless the user configures a different one. + DCGMExporterDefaultServiceAccountName = "nvidia-dcgm-exporter" // DCGMExporterConfigMapDataEnvName is the env name specifying the namespace:name // ConfigMap with custom metrics DCGMExporterConfigMapDataEnvName = "DCGM_EXPORTER_CONFIGMAP_DATA" @@ -332,16 +336,55 @@ var SubscriptionPathMap = map[string](MountPathToVolumeSource){ type controlFunc []func(n ClusterPolicyController) (gpuv1.State, error) // ServiceAccount creates ServiceAccount resource +// isServiceAccountOwned reports whether the ServiceAccount exists and is controlled +// by the ClusterPolicy being reconciled. A missing ServiceAccount counts as owned so +// that callers fall through to a delete that is a no-op. +func (n ClusterPolicyController) isServiceAccountOwned(ctx context.Context, obj *corev1.ServiceAccount) (bool, error) { + found := &corev1.ServiceAccount{} + if err := n.client.Get(ctx, types.NamespacedName{Namespace: obj.Namespace, Name: obj.Name}, found); err != nil { + if apierrors.IsNotFound(err) { + return true, nil + } + return false, err + } + return metav1.IsControlledBy(found, n.singleton), nil +} + func ServiceAccount(n ClusterPolicyController) (gpuv1.State, error) { ctx := n.ctx state := n.idx obj := n.resources[state].ServiceAccount.DeepCopy() obj.Namespace = n.operatorNamespace + // The DCGM Exporter ServiceAccount name is user-configurable. + isDCGMExporter := n.stateNames[state] == "state-dcgm-exporter" + if isDCGMExporter { + obj.Name = dcgmExporterServiceAccountName(&n.singleton.Spec) + } + // A ServiceAccount the user brings is only referenced, never managed: the + // operator must not create, adopt, mutate or delete it. + unmanaged := isDCGMExporter && !n.singleton.Spec.DCGMExporter.IsServiceAccountCreateEnabled() + logger := n.logger.WithValues("ServiceAccount", obj.Name, "Namespace", obj.Namespace) // Check if state is disabled and cleanup resource if exists if !n.isStateEnabled(n.stateNames[n.idx]) { + if unmanaged { + return gpuv1.Disabled, nil + } + if isDCGMExporter { + // A ServiceAccount that carries no ClusterPolicy owner reference was not + // created by this operator -- for instance one the user had already + // provisioned under the configured name -- so it is left untouched. + owned, err := n.isServiceAccountOwned(ctx, obj) + if err != nil { + return gpuv1.NotReady, err + } + if !owned { + logger.V(1).Info("ServiceAccount is not owned by the ClusterPolicy, skipping deletion") + return gpuv1.Disabled, nil + } + } err := n.client.Delete(ctx, obj) if err != nil && !apierrors.IsNotFound(err) { logger.Info("Couldn't delete", "Error", err) @@ -350,6 +393,19 @@ func ServiceAccount(n ClusterPolicyController) (gpuv1.State, error) { return gpuv1.Disabled, nil } + if unmanaged { + // 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 { + if apierrors.IsNotFound(err) { + logger.Error(err, "ServiceAccount configured with create=false does not exist") + } + return gpuv1.NotReady, err + } + return gpuv1.Ready, nil + } + if err := controllerutil.SetControllerReference(n.singleton, obj, n.scheme); err != nil { return gpuv1.NotReady, err } @@ -435,6 +491,7 @@ func RoleBinding(n ClusterPolicyController) (gpuv1.State, error) { } obj.Subjects[idx].Namespace = n.operatorNamespace } + rewriteDCGMExporterSubjects(obj.Subjects, n.stateNames[state], &n.singleton.Spec) if err := controllerutil.SetControllerReference(n.singleton, obj, n.scheme); err != nil { return gpuv1.NotReady, err @@ -465,6 +522,13 @@ var rbacGates = map[string]func(*gpuv1.ClusterPolicySpec) bool{ }, } +// dcgmExporterServiceAccountName returns the name of the ServiceAccount that every +// DCGM Exporter operand -- the DaemonSet, its RBAC bindings and the OpenShift SCC -- +// has to reference. +func dcgmExporterServiceAccountName(config *gpuv1.ClusterPolicySpec) string { + return config.DCGMExporter.GetServiceAccountName(DCGMExporterDefaultServiceAccountName) +} + func isRBACEnabled(name string, config *gpuv1.ClusterPolicySpec) bool { gate, ok := rbacGates[name] if !ok { @@ -473,6 +537,25 @@ func isRBACEnabled(name string, config *gpuv1.ClusterPolicySpec) bool { return gate(config) } +// rewriteDCGMExporterSubjects points the DCGM Exporter ServiceAccount subjects at the +// configured ServiceAccount. Only subjects naming the default ServiceAccount are +// rewritten, so unrelated subjects -- such as the Prometheus one kept in +// 0500_prom_rolebinding_openshift.yaml -- are preserved. +func rewriteDCGMExporterSubjects(subjects []rbacv1.Subject, stateName string, config *gpuv1.ClusterPolicySpec) { + if stateName != "state-dcgm-exporter" { + return + } + saName := dcgmExporterServiceAccountName(config) + if saName == DCGMExporterDefaultServiceAccountName { + return + } + for idx := range subjects { + if subjects[idx].Kind == rbacv1.ServiceAccountKind && subjects[idx].Name == DCGMExporterDefaultServiceAccountName { + subjects[idx].Name = saName + } + } +} + // ClusterRole creates ClusterRole resource func ClusterRole(n ClusterPolicyController) (gpuv1.State, error) { ctx := n.ctx @@ -554,6 +637,7 @@ func ClusterRoleBinding(n ClusterPolicyController) (gpuv1.State, error) { for idx := range obj.Subjects { obj.Subjects[idx].Namespace = n.operatorNamespace } + rewriteDCGMExporterSubjects(obj.Subjects, n.stateNames[state], &n.singleton.Spec) if err := controllerutil.SetControllerReference(n.singleton, obj, n.scheme); err != nil { return gpuv1.NotReady, err @@ -1808,6 +1892,12 @@ func TransformDCGMExporter(obj *appsv1.DaemonSet, config *gpuv1.ClusterPolicySpe addPullSecrets(&obj.Spec.Template.Spec, config.DCGMExporter.ImagePullSecrets) } + // The asset already references the default ServiceAccount, so only a + // user-configured name has to be applied here. + if saName := dcgmExporterServiceAccountName(config); saName != DCGMExporterDefaultServiceAccountName { + obj.Spec.Template.Spec.ServiceAccountName = saName + } + // merge extra annotations at the pod template level if len(config.DCGMExporter.Annotations) > 0 { addExtraAnnotations(obj, config.DCGMExporter.Annotations) @@ -4866,11 +4956,17 @@ func SecurityContextConstraints(n ClusterPolicyController) (gpuv1.State, error) return gpuv1.Disabled, nil } + // The SCC name and the openshift.io/scc annotation on the DaemonSet stay tied to + // the asset name; only the user entry follows the configured ServiceAccount. + sccServiceAccountName := obj.Name + if n.stateNames[state] == "state-dcgm-exporter" { + sccServiceAccountName = dcgmExporterServiceAccountName(&n.singleton.Spec) + } for idx := range obj.Users { if obj.Users[idx] != "FILLED BY THE OPERATOR" { continue } - obj.Users[idx] = fmt.Sprintf("system:serviceaccount:%s:%s", obj.Namespace, obj.Name) + obj.Users[idx] = fmt.Sprintf("system:serviceaccount:%s:%s", obj.Namespace, sccServiceAccountName) } if err := controllerutil.SetControllerReference(n.singleton, obj, n.scheme); err != nil { diff --git a/controllers/object_controls_test.go b/controllers/object_controls_test.go index cdf20701cf..d51e99ff84 100644 --- a/controllers/object_controls_test.go +++ b/controllers/object_controls_test.go @@ -47,6 +47,7 @@ import ( ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "sigs.k8s.io/controller-runtime/pkg/log/zap" gpuv1 "github.com/NVIDIA/gpu-operator/api/nvidia/v1" @@ -2532,3 +2533,267 @@ func TestDriverPrecompiledLibModulesSuse(t *testing.T) { }) } } + +// 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. +func TestDCGMExporterServiceAccountReconcile(t *testing.T) { + const ( + testNamespace = "test-namespace" + byoName = "byo-metrics-identity" + ) + + testScheme := runtime.NewScheme() + require.NoError(t, corev1.AddToScheme(testScheme)) + require.NoError(t, gpuv1.AddToScheme(testScheme)) + + clusterPolicy := func() *gpuv1.ClusterPolicy { + return &gpuv1.ClusterPolicy{ObjectMeta: metav1.ObjectMeta{Name: "cluster-policy", UID: "cp-uid"}} + } + + newController := func(k8s client.Client, cp *gpuv1.ClusterPolicy) ClusterPolicyController { + return ClusterPolicyController{ + client: k8s, + ctx: context.Background(), + singleton: cp, + scheme: testScheme, + operatorNamespace: testNamespace, + resources: []Resources{{ + ServiceAccount: corev1.ServiceAccount{ + ObjectMeta: metav1.ObjectMeta{Name: DCGMExporterDefaultServiceAccountName}, + }, + }}, + stateNames: []string{"state-dcgm-exporter"}, + idx: 0, + logger: ctrl.Log.WithName("test"), + } + } + + serviceAccount := func(name string) *corev1.ServiceAccount { + return &corev1.ServiceAccount{ + ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: testNamespace}, + } + } + + getServiceAccount := func(t *testing.T, k8s client.Client, name string) (*corev1.ServiceAccount, bool) { + t.Helper() + found := &corev1.ServiceAccount{} + err := k8s.Get(context.Background(), types.NamespacedName{Namespace: testNamespace, Name: name}, found) + if apierrors.IsNotFound(err) { + return nil, false + } + require.NoError(t, err) + return found, true + } + + t.Run("default configuration creates the default ServiceAccount", func(t *testing.T) { + k8s := fake.NewClientBuilder().WithScheme(testScheme).Build() + cp := clusterPolicy() + n := newController(k8s, cp) + + state, err := ServiceAccount(n) + require.NoError(t, err) + require.Equal(t, gpuv1.Ready, state) + + sa, ok := getServiceAccount(t, k8s, DCGMExporterDefaultServiceAccountName) + require.True(t, ok) + require.True(t, metav1.IsControlledBy(sa, cp)) + }) + + t.Run("configured name creates that ServiceAccount", func(t *testing.T) { + k8s := fake.NewClientBuilder().WithScheme(testScheme).Build() + cp := clusterPolicy() + cp.Spec.DCGMExporter.ServiceAccount = &gpuv1.DCGMExporterServiceAccountConfig{Name: "metrics-identity"} + n := newController(k8s, cp) + + state, err := ServiceAccount(n) + require.NoError(t, err) + require.Equal(t, gpuv1.Ready, state) + + _, ok := getServiceAccount(t, k8s, "metrics-identity") + require.True(t, ok) + _, ok = getServiceAccount(t, k8s, DCGMExporterDefaultServiceAccountName) + require.False(t, ok, "the default ServiceAccount must not be created as well") + }) + + t.Run("create=false reports NotReady when the ServiceAccount is missing", func(t *testing.T) { + k8s := fake.NewClientBuilder().WithScheme(testScheme).Build() + cp := clusterPolicy() + cp.Spec.DCGMExporter.ServiceAccount = &gpuv1.DCGMExporterServiceAccountConfig{ + Name: byoName, Create: new(false), + } + n := newController(k8s, cp) + + state, err := ServiceAccount(n) + require.Error(t, err) + require.True(t, apierrors.IsNotFound(err)) + require.Equal(t, gpuv1.NotReady, state) + + _, ok := getServiceAccount(t, k8s, byoName) + require.False(t, ok, "a user-provided ServiceAccount must never be created by the operator") + }) + + t.Run("create=false references an existing ServiceAccount without adopting it", func(t *testing.T) { + k8s := fake.NewClientBuilder().WithScheme(testScheme).WithObjects(serviceAccount(byoName)).Build() + cp := clusterPolicy() + cp.Spec.DCGMExporter.ServiceAccount = &gpuv1.DCGMExporterServiceAccountConfig{ + Name: byoName, Create: new(false), + } + n := newController(k8s, cp) + + state, err := ServiceAccount(n) + require.NoError(t, err) + require.Equal(t, gpuv1.Ready, state) + + sa, ok := getServiceAccount(t, k8s, byoName) + require.True(t, ok) + require.Empty(t, sa.OwnerReferences, "the operator must not take ownership of a user-provided ServiceAccount") + }) + + t.Run("disabling the exporter keeps a user-provided ServiceAccount", func(t *testing.T) { + k8s := fake.NewClientBuilder().WithScheme(testScheme).WithObjects(serviceAccount(byoName)).Build() + cp := clusterPolicy() + cp.Spec.DCGMExporter.Enabled = new(false) + cp.Spec.DCGMExporter.ServiceAccount = &gpuv1.DCGMExporterServiceAccountConfig{ + Name: byoName, Create: new(false), + } + n := newController(k8s, cp) + + state, err := ServiceAccount(n) + require.NoError(t, err) + require.Equal(t, gpuv1.Disabled, state) + + _, ok := getServiceAccount(t, k8s, byoName) + require.True(t, ok, "a user-provided ServiceAccount must survive disabling the exporter") + }) + + t.Run("disabling the exporter keeps a ServiceAccount the operator does not own", func(t *testing.T) { + // Same name as the operator default, but provisioned by the user beforehand. + k8s := fake.NewClientBuilder().WithScheme(testScheme). + WithObjects(serviceAccount(DCGMExporterDefaultServiceAccountName)).Build() + cp := clusterPolicy() + cp.Spec.DCGMExporter.Enabled = new(false) + n := newController(k8s, cp) + + state, err := ServiceAccount(n) + require.NoError(t, err) + require.Equal(t, gpuv1.Disabled, state) + + _, ok := getServiceAccount(t, k8s, DCGMExporterDefaultServiceAccountName) + require.True(t, ok, "only a ServiceAccount owned by the ClusterPolicy may be deleted") + }) + + t.Run("disabling the exporter deletes the ServiceAccount the operator owns", func(t *testing.T) { + cp := clusterPolicy() + owned := serviceAccount(DCGMExporterDefaultServiceAccountName) + require.NoError(t, controllerutil.SetControllerReference(cp, owned, testScheme)) + + k8s := fake.NewClientBuilder().WithScheme(testScheme).WithObjects(owned).Build() + cp.Spec.DCGMExporter.Enabled = new(false) + n := newController(k8s, cp) + + state, err := ServiceAccount(n) + require.NoError(t, err) + require.Equal(t, gpuv1.Disabled, state) + + _, ok := getServiceAccount(t, k8s, DCGMExporterDefaultServiceAccountName) + require.False(t, ok) + }) +} + +// TestDCGMExporterRBACSubjects verifies that the RBAC bindings and the OpenShift SCC +// follow the configured ServiceAccount while their own object names stay stable. +func TestDCGMExporterRBACSubjects(t *testing.T) { + const ( + testNamespace = "test-namespace" + filled = "FILLED BY THE OPERATOR" + customSA = "metrics-identity" + ) + + testScheme := runtime.NewScheme() + require.NoError(t, corev1.AddToScheme(testScheme)) + require.NoError(t, rbacv1.AddToScheme(testScheme)) + require.NoError(t, secv1.AddToScheme(testScheme)) + require.NoError(t, gpuv1.AddToScheme(testScheme)) + + newController := func(k8s client.Client, spec gpuv1.ClusterPolicySpec, res Resources) ClusterPolicyController { + return ClusterPolicyController{ + client: k8s, + ctx: context.Background(), + singleton: &gpuv1.ClusterPolicy{ObjectMeta: metav1.ObjectMeta{Name: "cluster-policy", UID: "cp-uid"}, Spec: spec}, + scheme: testScheme, + operatorNamespace: testNamespace, + resources: []Resources{res}, + stateNames: []string{"state-dcgm-exporter"}, + idx: 0, + logger: ctrl.Log.WithName("test"), + } + } + + customSpec := gpuv1.ClusterPolicySpec{ + DCGMExporter: gpuv1.DCGMExporterSpec{ + EnablePodLabels: new(true), + ServiceAccount: &gpuv1.DCGMExporterServiceAccountConfig{Name: customSA}, + }, + } + + t.Run("RoleBinding subject follows the configured ServiceAccount", func(t *testing.T) { + k8s := fake.NewClientBuilder().WithScheme(testScheme).Build() + res := Resources{RoleBinding: rbacv1.RoleBinding{ + ObjectMeta: metav1.ObjectMeta{Name: DCGMExporterDefaultServiceAccountName}, + Subjects: []rbacv1.Subject{ + {Kind: rbacv1.ServiceAccountKind, Name: DCGMExporterDefaultServiceAccountName, Namespace: filled}, + // Kept verbatim, mirroring 0500_prom_rolebinding_openshift.yaml. + {Kind: rbacv1.ServiceAccountKind, Name: "prometheus-k8s", Namespace: "openshift-monitoring"}, + }, + }} + + state, err := RoleBinding(newController(k8s, customSpec, res)) + require.NoError(t, err) + require.Equal(t, gpuv1.Ready, state) + + found := &rbacv1.RoleBinding{} + require.NoError(t, k8s.Get(context.Background(), + types.NamespacedName{Namespace: testNamespace, Name: DCGMExporterDefaultServiceAccountName}, found)) + require.Equal(t, customSA, found.Subjects[0].Name) + require.Equal(t, testNamespace, found.Subjects[0].Namespace) + require.Equal(t, "prometheus-k8s", found.Subjects[1].Name) + require.Equal(t, "openshift-monitoring", found.Subjects[1].Namespace) + }) + + t.Run("ClusterRoleBinding subject follows the configured ServiceAccount", func(t *testing.T) { + k8s := fake.NewClientBuilder().WithScheme(testScheme).Build() + res := Resources{ClusterRoleBinding: rbacv1.ClusterRoleBinding{ + ObjectMeta: metav1.ObjectMeta{Name: "nvidia-dcgm-exporter-read-pods"}, + Subjects: []rbacv1.Subject{ + {Kind: rbacv1.ServiceAccountKind, Name: DCGMExporterDefaultServiceAccountName, Namespace: filled}, + }, + }} + + state, err := ClusterRoleBinding(newController(k8s, customSpec, res)) + require.NoError(t, err) + require.Equal(t, gpuv1.Ready, state) + + found := &rbacv1.ClusterRoleBinding{} + require.NoError(t, k8s.Get(context.Background(), + types.NamespacedName{Namespace: testNamespace, Name: "nvidia-dcgm-exporter-read-pods"}, found)) + require.Equal(t, customSA, found.Subjects[0].Name) + }) + + t.Run("SCC user follows the ServiceAccount while the SCC name is unchanged", func(t *testing.T) { + k8s := fake.NewClientBuilder().WithScheme(testScheme).Build() + res := Resources{SecurityContextConstraints: secv1.SecurityContextConstraints{ + ObjectMeta: metav1.ObjectMeta{Name: DCGMExporterDefaultServiceAccountName}, + Users: []string{filled}, + }} + + state, err := SecurityContextConstraints(newController(k8s, customSpec, res)) + require.NoError(t, err) + require.Equal(t, gpuv1.Ready, state) + + found := &secv1.SecurityContextConstraints{} + require.NoError(t, k8s.Get(context.Background(), + types.NamespacedName{Namespace: testNamespace, Name: DCGMExporterDefaultServiceAccountName}, found)) + require.Equal(t, []string{fmt.Sprintf("system:serviceaccount:%s:%s", testNamespace, customSA)}, found.Users) + }) +} diff --git a/controllers/transforms_test.go b/controllers/transforms_test.go index 8931298a48..72c0cbb263 100644 --- a/controllers/transforms_test.go +++ b/controllers/transforms_test.go @@ -194,6 +194,11 @@ func (d Daemonset) WithAutomountServiceAccountToken(enabled bool) Daemonset { return d } +func (d Daemonset) WithServiceAccountName(name string) Daemonset { + d.Spec.Template.Spec.ServiceAccountName = name + return d +} + func (d Daemonset) WithVolume(volume corev1.Volume) Daemonset { d.Spec.Template.Spec.Volumes = append(d.Spec.Template.Spec.Volumes, volume) return d @@ -5078,3 +5083,49 @@ func TestHashDriverInstallConfigZeroFieldInvariant(t *testing.T) { assert.NotEqual(t, originalDigest, changedDigest, "a non-zero new field should change the digest") } + +// TestTransformDCGMExporterServiceAccount verifies that the DaemonSet references the +// configured ServiceAccount and that leaving the configuration unset does not touch +// the name carried by the asset. +func TestTransformDCGMExporterServiceAccount(t *testing.T) { + testCases := map[string]struct { + serviceAccount *gpuv1.DCGMExporterServiceAccountConfig + expectedNameChange string + }{ + "unset keeps the asset value": { + serviceAccount: nil, + expectedNameChange: "", + }, + "explicit default keeps the asset value": { + serviceAccount: &gpuv1.DCGMExporterServiceAccountConfig{Name: DCGMExporterDefaultServiceAccountName}, + expectedNameChange: "", + }, + "custom name is applied": { + serviceAccount: &gpuv1.DCGMExporterServiceAccountConfig{Name: "metrics-identity"}, + expectedNameChange: "metrics-identity", + }, + "custom name with create=false is applied": { + serviceAccount: &gpuv1.DCGMExporterServiceAccountConfig{Name: "byo-sa", Create: new(false)}, + expectedNameChange: "byo-sa", + }, + } + + for name, tc := range testCases { + t.Run(name, func(t *testing.T) { + ds := NewDaemonset().WithContainer(corev1.Container{Name: "dcgm-exporter"}) + cpSpec := &gpuv1.ClusterPolicySpec{ + DCGMExporter: gpuv1.DCGMExporterSpec{ + Repository: "nvcr.io/nvidia/k8s", + Image: "dcgm-exporter", + Version: "v1.0.0", + ServiceAccount: tc.serviceAccount, + }, + } + + err := TransformDCGMExporter(ds.DaemonSet, cpSpec, + ClusterPolicyController{runtime: gpuv1.Containerd, logger: ctrl.Log.WithName("test")}) + require.NoError(t, err) + require.Equal(t, tc.expectedNameChange, ds.Spec.Template.Spec.ServiceAccountName) + }) + } +} diff --git a/deployments/gpu-operator/crds/nvidia.com_clusterpolicies.yaml b/deployments/gpu-operator/crds/nvidia.com_clusterpolicies.yaml index e8d0be746c..66f609a24f 100644 --- a/deployments/gpu-operator/crds/nvidia.com_clusterpolicies.yaml +++ b/deployments/gpu-operator/crds/nvidia.com_clusterpolicies.yaml @@ -686,6 +686,28 @@ spec: ingress methods for a service type: string type: object + serviceAccount: + description: 'Optional: ServiceAccount configuration for NVIDIA + DCGM Exporter' + properties: + create: + description: |- + Create indicates whether the operator manages the lifecycle of the DCGM + Exporter ServiceAccount. Defaults to true. When set to false, a + ServiceAccount with the configured name has to already exist in the + operator namespace; the operator then only references it and never + creates, adopts, mutates or deletes it. + type: boolean + name: + description: |- + Name of the ServiceAccount used by the NVIDIA DCGM Exporter DaemonSet. + Defaults to the operator-managed ServiceAccount when left empty. + type: string + type: object + x-kubernetes-validations: + - message: name is required when create is false + rule: '!has(self.create) || self.create || (has(self.name) && + size(self.name) > 0)' serviceMonitor: description: 'Optional: ServiceMonitor configuration for NVIDIA DCGM Exporter' diff --git a/deployments/gpu-operator/crds/nvidia.com_gpuclusters.yaml b/deployments/gpu-operator/crds/nvidia.com_gpuclusters.yaml index f7430a1778..6f90544121 100644 --- a/deployments/gpu-operator/crds/nvidia.com_gpuclusters.yaml +++ b/deployments/gpu-operator/crds/nvidia.com_gpuclusters.yaml @@ -590,6 +590,28 @@ spec: ingress methods for a service type: string type: object + serviceAccount: + description: 'Optional: ServiceAccount configuration for NVIDIA + DCGM Exporter' + properties: + create: + description: |- + Create indicates whether the operator manages the lifecycle of the DCGM + Exporter ServiceAccount. Defaults to true. When set to false, a + ServiceAccount with the configured name has to already exist in the + operator namespace; the operator then only references it and never + creates, adopts, mutates or deletes it. + type: boolean + name: + description: |- + Name of the ServiceAccount used by the NVIDIA DCGM Exporter DaemonSet. + Defaults to the operator-managed ServiceAccount when left empty. + type: string + type: object + x-kubernetes-validations: + - message: name is required when create is false + rule: '!has(self.create) || self.create || (has(self.name) && + size(self.name) > 0)' serviceMonitor: description: 'Optional: ServiceMonitor configuration for NVIDIA DCGM Exporter' diff --git a/deployments/gpu-operator/templates/clusterpolicy.yaml b/deployments/gpu-operator/templates/clusterpolicy.yaml index e156f5b7a9..e402277872 100644 --- a/deployments/gpu-operator/templates/clusterpolicy.yaml +++ b/deployments/gpu-operator/templates/clusterpolicy.yaml @@ -568,6 +568,9 @@ spec: {{- if .Values.dcgmExporter.service }} service: {{ toYaml .Values.dcgmExporter.service | nindent 6 }} {{- end }} + {{- if .Values.dcgmExporter.serviceAccount }} + serviceAccount: {{ toYaml .Values.dcgmExporter.serviceAccount | nindent 6 }} + {{- end }} {{- if .Values.dcgmExporter.hostPID }} hostPID: {{ .Values.dcgmExporter.hostPID }} {{- end }} diff --git a/deployments/gpu-operator/values.yaml b/deployments/gpu-operator/values.yaml index f6222b0d5c..e849a7789c 100644 --- a/deployments/gpu-operator/values.yaml +++ b/deployments/gpu-operator/values.yaml @@ -324,6 +324,14 @@ dcgmExporter: # podLabelAllowlistRegex: # - "^app$" # - "^kueue\\.x-k8s\\.io/.*$" + # ServiceAccount used by the DCGM Exporter DaemonSet. Leave unset to keep the + # operator-managed "nvidia-dcgm-exporter" ServiceAccount. + # serviceAccount: + # # name of the ServiceAccount to reference + # name: nvidia-dcgm-exporter + # # set to false to bind to a ServiceAccount that already exists in the operator + # # namespace; the operator then never creates, mutates or deletes it + # create: true service: internalTrafficPolicy: Cluster serviceMonitor: diff --git a/internal/state/dcgm_exporter.go b/internal/state/dcgm_exporter.go index 686274f75c..7ac3d3da2a 100644 --- a/internal/state/dcgm_exporter.go +++ b/internal/state/dcgm_exporter.go @@ -43,6 +43,10 @@ const ( dcgmExporterCustomCollectors = "/etc/dcgm-exporter/dcgm-metrics.csv" dcgmExporterDefaultKubeletRootDir = "/var/lib/kubelet" dcgmExporterDefaultJobMappingDir = "/var/lib/dcgm-exporter/job-mapping" + + // dcgmExporterDefaultServiceAccountName is the ServiceAccount the DRA operands + // reference unless the user configures a different one. + dcgmExporterDefaultServiceAccountName = "nvidia-dcgm-exporter-dra" ) func NewStateDCGMExporter( @@ -140,6 +144,8 @@ func buildDCGMExporterRenderData(ctx context.Context, s *configurableState, cr * PodResourcesDir: filepath.Join(kubeletRootDir, "pod-resources"), ServiceType: serviceType, ServiceInternalTrafficPolicy: serviceInternalTrafficPolicy, + ServiceAccountName: spec.GetServiceAccountName(dcgmExporterDefaultServiceAccountName), + CreateServiceAccount: spec.IsServiceAccountCreateEnabled(), }, nil } diff --git a/internal/state/types.go b/internal/state/types.go index 8c7c9237b0..57a3631aa8 100644 --- a/internal/state/types.go +++ b/internal/state/types.go @@ -111,6 +111,10 @@ type dcgmExporterRenderData struct { PodResourcesDir string ServiceType string ServiceInternalTrafficPolicy string + // ServiceAccountName is the ServiceAccount the operands reference; CreateServiceAccount + // reports whether the operator owns its lifecycle (false = supplied by the user). + ServiceAccountName string + CreateServiceAccount bool } // validatorRenderData is the templating data for the DRA validator manifests. It diff --git a/manifests/state-dcgm-exporter/0100_serviceaccount.yaml b/manifests/state-dcgm-exporter/0100_serviceaccount.yaml index 2bd03f52fc..120a4dc480 100644 --- a/manifests/state-dcgm-exporter/0100_serviceaccount.yaml +++ b/manifests/state-dcgm-exporter/0100_serviceaccount.yaml @@ -1,5 +1,7 @@ +{{- if .CreateServiceAccount }} apiVersion: v1 kind: ServiceAccount metadata: - name: nvidia-dcgm-exporter-dra + name: {{ .ServiceAccountName }} namespace: {{ .Namespace }} +{{- end }} diff --git a/manifests/state-dcgm-exporter/0300_rolebinding.yaml b/manifests/state-dcgm-exporter/0300_rolebinding.yaml index 5bbb009da4..ac09e92f7e 100644 --- a/manifests/state-dcgm-exporter/0300_rolebinding.yaml +++ b/manifests/state-dcgm-exporter/0300_rolebinding.yaml @@ -9,5 +9,5 @@ roleRef: name: nvidia-dcgm-exporter-dra subjects: - kind: ServiceAccount - name: nvidia-dcgm-exporter-dra + name: {{ .ServiceAccountName }} namespace: {{ .Namespace }} diff --git a/manifests/state-dcgm-exporter/0310_clusterrolebinding.yaml b/manifests/state-dcgm-exporter/0310_clusterrolebinding.yaml index 401695f2d9..b3a8b61fff 100644 --- a/manifests/state-dcgm-exporter/0310_clusterrolebinding.yaml +++ b/manifests/state-dcgm-exporter/0310_clusterrolebinding.yaml @@ -10,5 +10,5 @@ roleRef: name: nvidia-dcgm-exporter-dra-read-pods subjects: - kind: ServiceAccount - name: nvidia-dcgm-exporter-dra + name: {{ .ServiceAccountName }} namespace: {{ .Namespace }} diff --git a/manifests/state-dcgm-exporter/0450_scc.openshift.yaml b/manifests/state-dcgm-exporter/0450_scc.openshift.yaml index e45262ff43..9ef041fa4a 100644 --- a/manifests/state-dcgm-exporter/0450_scc.openshift.yaml +++ b/manifests/state-dcgm-exporter/0450_scc.openshift.yaml @@ -33,7 +33,7 @@ seccompProfiles: supplementalGroups: type: RunAsAny users: -- system:serviceaccount:{{ .Namespace }}:nvidia-dcgm-exporter-dra +- system:serviceaccount:{{ .Namespace }}:{{ .ServiceAccountName }} volumes: - '*' {{end}} diff --git a/manifests/state-dcgm-exporter/0700_daemonset.yaml b/manifests/state-dcgm-exporter/0700_daemonset.yaml index dcf9b4203b..32c58f0da4 100644 --- a/manifests/state-dcgm-exporter/0700_daemonset.yaml +++ b/manifests/state-dcgm-exporter/0700_daemonset.yaml @@ -40,7 +40,7 @@ spec: {{- end }} spec: priorityClassName: system-node-critical - serviceAccountName: nvidia-dcgm-exporter-dra + serviceAccountName: {{ .ServiceAccountName }} automountServiceAccountToken: true # Gate scheduling on the per-component deploy label so the k8s-driver-manager # can pause it to drain dcgm-exporter off a node during a driver reload. From dd3685aca783aafa15e506ae96c990367a90c1db Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=EB=B0=B1=EC=A7=80=EB=AA=85?= Date: Wed, 9 Sep 2026 14:25:48 +0900 Subject: [PATCH 2/9] Address review feedback on the DCGM Exporter ServiceAccount MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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) Claude-Session: https://claude.ai/code/session_01UiFVTVAyEMmn68Xvo9LSTf Signed-off-by: 백지명 --- api/nvidia/v1/clusterpolicy_types_test.go | 38 +++++++++++++++++++ controllers/object_controls_test.go | 18 +++++++++ .../0100_serviceaccount.yaml | 2 +- .../state-dcgm-exporter/0300_rolebinding.yaml | 2 +- .../0310_clusterrolebinding.yaml | 2 +- .../state-dcgm-exporter/0700_daemonset.yaml | 2 +- 6 files changed, 60 insertions(+), 4 deletions(-) diff --git a/api/nvidia/v1/clusterpolicy_types_test.go b/api/nvidia/v1/clusterpolicy_types_test.go index d432934fc5..5c0e8c7a6d 100644 --- a/api/nvidia/v1/clusterpolicy_types_test.go +++ b/api/nvidia/v1/clusterpolicy_types_test.go @@ -17,10 +17,13 @@ package v1 import ( + "os" "testing" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" + "sigs.k8s.io/yaml" ) func TestImagePath(t *testing.T) { @@ -130,3 +133,38 @@ func TestDCGMExporterServiceAccount(t *testing.T) { }) } } + +// TestDCGMExporterServiceAccountCRDValidation pins the CEL rule that guards +// `serviceAccount: {create: false}` without a name. The helpers cannot catch that +// combination -- GetServiceAccountName falls back to the default and the operator +// would then treat the default ServiceAccount as user-provided -- so the generated +// CRD is the only safeguard before reconciliation. +func TestDCGMExporterServiceAccountCRDValidation(t *testing.T) { + crds := map[string]string{ + "ClusterPolicy": "../../../config/crd/bases/nvidia.com_clusterpolicies.yaml", + "GPUCluster": "../../../config/crd/bases/nvidia.com_gpuclusters.yaml", + } + + for kind, path := range crds { + t.Run(kind, func(t *testing.T) { + data, err := os.ReadFile(path) + require.NoError(t, err) + + crd := &apiextensionsv1.CustomResourceDefinition{} + require.NoError(t, yaml.Unmarshal(data, crd)) + require.NotEmpty(t, crd.Spec.Versions) + + props := crd.Spec.Versions[0].Schema.OpenAPIV3Schema.Properties["spec"]. + Properties["dcgmExporter"].Properties["serviceAccount"] + require.Contains(t, props.Properties, "name") + require.Contains(t, props.Properties, "create") + + require.Len(t, props.XValidations, 1, + "the create/name consistency rule must survive CRD regeneration") + rule := props.XValidations[0] + require.Equal(t, "name is required when create is false", rule.Message) + require.Contains(t, rule.Rule, "self.create") + require.Contains(t, rule.Rule, "self.name") + }) + } +} diff --git a/controllers/object_controls_test.go b/controllers/object_controls_test.go index d51e99ff84..95a2443655 100644 --- a/controllers/object_controls_test.go +++ b/controllers/object_controls_test.go @@ -2683,6 +2683,24 @@ func TestDCGMExporterServiceAccountReconcile(t *testing.T) { require.True(t, ok, "only a ServiceAccount owned by the ClusterPolicy may be deleted") }) + t.Run("a non-DCGM state deletes its ServiceAccount regardless of ownership", func(t *testing.T) { + // The ownership check is scoped to the DCGM Exporter; every other state keeps + // the previous unconditional cleanup on disable. + k8s := fake.NewClientBuilder().WithScheme(testScheme). + WithObjects(serviceAccount(DCGMExporterDefaultServiceAccountName)).Build() + cp := clusterPolicy() + cp.Spec.Driver.Enabled = new(false) + n := newController(k8s, cp) + n.stateNames = []string{"state-driver"} + + state, err := ServiceAccount(n) + require.NoError(t, err) + require.Equal(t, gpuv1.Disabled, state) + + _, ok := getServiceAccount(t, k8s, DCGMExporterDefaultServiceAccountName) + require.False(t, ok) + }) + t.Run("disabling the exporter deletes the ServiceAccount the operator owns", func(t *testing.T) { cp := clusterPolicy() owned := serviceAccount(DCGMExporterDefaultServiceAccountName) diff --git a/manifests/state-dcgm-exporter/0100_serviceaccount.yaml b/manifests/state-dcgm-exporter/0100_serviceaccount.yaml index 120a4dc480..62334e4ebd 100644 --- a/manifests/state-dcgm-exporter/0100_serviceaccount.yaml +++ b/manifests/state-dcgm-exporter/0100_serviceaccount.yaml @@ -2,6 +2,6 @@ apiVersion: v1 kind: ServiceAccount metadata: - name: {{ .ServiceAccountName }} + name: {{ .ServiceAccountName | quote }} namespace: {{ .Namespace }} {{- end }} diff --git a/manifests/state-dcgm-exporter/0300_rolebinding.yaml b/manifests/state-dcgm-exporter/0300_rolebinding.yaml index ac09e92f7e..de076f9691 100644 --- a/manifests/state-dcgm-exporter/0300_rolebinding.yaml +++ b/manifests/state-dcgm-exporter/0300_rolebinding.yaml @@ -9,5 +9,5 @@ roleRef: name: nvidia-dcgm-exporter-dra subjects: - kind: ServiceAccount - name: {{ .ServiceAccountName }} + name: {{ .ServiceAccountName | quote }} namespace: {{ .Namespace }} diff --git a/manifests/state-dcgm-exporter/0310_clusterrolebinding.yaml b/manifests/state-dcgm-exporter/0310_clusterrolebinding.yaml index b3a8b61fff..14b4da5d5f 100644 --- a/manifests/state-dcgm-exporter/0310_clusterrolebinding.yaml +++ b/manifests/state-dcgm-exporter/0310_clusterrolebinding.yaml @@ -10,5 +10,5 @@ roleRef: name: nvidia-dcgm-exporter-dra-read-pods subjects: - kind: ServiceAccount - name: {{ .ServiceAccountName }} + name: {{ .ServiceAccountName | quote }} namespace: {{ .Namespace }} diff --git a/manifests/state-dcgm-exporter/0700_daemonset.yaml b/manifests/state-dcgm-exporter/0700_daemonset.yaml index 32c58f0da4..05d255aa2e 100644 --- a/manifests/state-dcgm-exporter/0700_daemonset.yaml +++ b/manifests/state-dcgm-exporter/0700_daemonset.yaml @@ -40,7 +40,7 @@ spec: {{- end }} spec: priorityClassName: system-node-critical - serviceAccountName: {{ .ServiceAccountName }} + serviceAccountName: {{ .ServiceAccountName | quote }} automountServiceAccountToken: true # Gate scheduling on the per-component deploy label so the k8s-driver-manager # can pause it to drain dcgm-exporter off a node during a driver reload. From ba6cc8552eb5c56b3aab3db8dcdac073910725d6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=EB=B0=B1=EC=A7=80=EB=AA=85?= Date: Wed, 9 Sep 2026 14:40:51 +0900 Subject: [PATCH 3/9] Complete the DCGM Exporter ServiceAccount support on the GPUCluster path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) Claude-Session: https://claude.ai/code/session_01UiFVTVAyEMmn68Xvo9LSTf Signed-off-by: 백지명 --- controllers/object_controls.go | 56 +++++- controllers/object_controls_test.go | 50 +++++ .../gpu-operator/templates/gpucluster.yaml | 3 + internal/state/configurable_state.go | 12 ++ internal/state/dcgm_exporter.go | 78 ++++++++ internal/state/dcgm_exporter_test.go | 176 ++++++++++++++++++ 6 files changed, 370 insertions(+), 5 deletions(-) diff --git a/controllers/object_controls.go b/controllers/object_controls.go index 2af65852bb..bb56b68528 100644 --- a/controllers/object_controls.go +++ b/controllers/object_controls.go @@ -28,6 +28,7 @@ import ( "strconv" "strings" + "github.com/go-logr/logr" apiconfigv1 "github.com/openshift/api/config/v1" apiimagev1 "github.com/openshift/api/image/v1" secv1 "github.com/openshift/api/security/v1" @@ -350,6 +351,28 @@ func (n ClusterPolicyController) isServiceAccountOwned(ctx context.Context, obj return metav1.IsControlledBy(found, n.singleton), nil } +// deleteOwnedServiceAccount removes a ServiceAccount left behind by a previous +// configuration, but only when this ClusterPolicy owns it: an object the user +// provisioned under the same name is left alone. +func (n ClusterPolicyController) deleteOwnedServiceAccount(ctx context.Context, name string, logger logr.Logger) error { + found := &corev1.ServiceAccount{} + err := n.client.Get(ctx, types.NamespacedName{Namespace: n.operatorNamespace, Name: name}, found) + if err != nil { + if apierrors.IsNotFound(err) { + return nil + } + return err + } + if !metav1.IsControlledBy(found, n.singleton) { + return nil + } + logger.V(1).Info("Removing the superseded dcgm-exporter ServiceAccount", "Name", name) + if err := n.client.Delete(ctx, found); err != nil && !apierrors.IsNotFound(err) { + return err + } + return nil +} + func ServiceAccount(n ClusterPolicyController) (gpuv1.State, error) { ctx := n.ctx state := n.idx @@ -406,18 +429,41 @@ func ServiceAccount(n ClusterPolicyController) (gpuv1.State, error) { return gpuv1.Ready, nil } + isRenamed := isDCGMExporter && obj.Name != DCGMExporterDefaultServiceAccountName + if isRenamed { + // Only a name the user chose can collide with an unrelated object; the default is + // left tolerant so an upgrade that lost the owner reference keeps converging. + owned, err := n.isServiceAccountOwned(ctx, obj) + if err != nil { + return gpuv1.NotReady, err + } + if !owned { + err := fmt.Errorf("ServiceAccount %q already exists in namespace %q and is not managed by this ClusterPolicy; "+ + "set dcgmExporter.serviceAccount.create to false to reference it", obj.Name, obj.Namespace) + logger.Error(err, "Refusing to take over an existing ServiceAccount") + return gpuv1.NotReady, err + } + } + if err := controllerutil.SetControllerReference(n.singleton, obj, n.scheme); err != nil { return gpuv1.NotReady, err } if err := n.client.Create(ctx, obj); err != nil { - if apierrors.IsAlreadyExists(err) { - logger.Info("Found Resource, skipping update") - return gpuv1.Ready, nil + if !apierrors.IsAlreadyExists(err) { + logger.Info("Couldn't create", "Error", err) + return gpuv1.NotReady, err } + logger.Info("Found Resource, skipping update") + } - logger.Info("Couldn't create", "Error", err) - return gpuv1.NotReady, err + if isRenamed { + // A previous configuration may have left the operator-owned default behind. + // Renaming between two custom names is not tracked, so only the default is + // reclaimed here. + if err := n.deleteOwnedServiceAccount(ctx, DCGMExporterDefaultServiceAccountName, logger); err != nil { + return gpuv1.NotReady, err + } } return gpuv1.Ready, nil } diff --git a/controllers/object_controls_test.go b/controllers/object_controls_test.go index 95a2443655..fed6482555 100644 --- a/controllers/object_controls_test.go +++ b/controllers/object_controls_test.go @@ -2683,6 +2683,56 @@ func TestDCGMExporterServiceAccountReconcile(t *testing.T) { require.True(t, ok, "only a ServiceAccount owned by the ClusterPolicy may be deleted") }) + t.Run("a configured name refuses to take over an unowned ServiceAccount", func(t *testing.T) { + k8s := fake.NewClientBuilder().WithScheme(testScheme). + WithObjects(serviceAccount("metrics-identity")).Build() + cp := clusterPolicy() + cp.Spec.DCGMExporter.ServiceAccount = &gpuv1.DCGMExporterServiceAccountConfig{Name: "metrics-identity"} + n := newController(k8s, cp) + + state, err := ServiceAccount(n) + require.Error(t, err) + require.Equal(t, gpuv1.NotReady, state) + + sa, ok := getServiceAccount(t, k8s, "metrics-identity") + require.True(t, ok) + require.Empty(t, sa.OwnerReferences, "an existing ServiceAccount must not be adopted") + }) + + t.Run("renaming reclaims the superseded operator-owned default", func(t *testing.T) { + cp := clusterPolicy() + previous := serviceAccount(DCGMExporterDefaultServiceAccountName) + require.NoError(t, controllerutil.SetControllerReference(cp, previous, testScheme)) + + k8s := fake.NewClientBuilder().WithScheme(testScheme).WithObjects(previous).Build() + cp.Spec.DCGMExporter.ServiceAccount = &gpuv1.DCGMExporterServiceAccountConfig{Name: "metrics-identity"} + n := newController(k8s, cp) + + state, err := ServiceAccount(n) + require.NoError(t, err) + require.Equal(t, gpuv1.Ready, state) + + _, ok := getServiceAccount(t, k8s, "metrics-identity") + require.True(t, ok) + _, ok = getServiceAccount(t, k8s, DCGMExporterDefaultServiceAccountName) + require.False(t, ok, "the superseded default must be removed") + }) + + t.Run("renaming keeps a previous ServiceAccount the operator does not own", func(t *testing.T) { + k8s := fake.NewClientBuilder().WithScheme(testScheme). + WithObjects(serviceAccount(DCGMExporterDefaultServiceAccountName)).Build() + cp := clusterPolicy() + cp.Spec.DCGMExporter.ServiceAccount = &gpuv1.DCGMExporterServiceAccountConfig{Name: "metrics-identity"} + n := newController(k8s, cp) + + state, err := ServiceAccount(n) + require.NoError(t, err) + require.Equal(t, gpuv1.Ready, state) + + _, ok := getServiceAccount(t, k8s, DCGMExporterDefaultServiceAccountName) + require.True(t, ok, "only an owned ServiceAccount may be reclaimed") + }) + t.Run("a non-DCGM state deletes its ServiceAccount regardless of ownership", func(t *testing.T) { // The ownership check is scoped to the DCGM Exporter; every other state keeps // the previous unconditional cleanup on disable. diff --git a/deployments/gpu-operator/templates/gpucluster.yaml b/deployments/gpu-operator/templates/gpucluster.yaml index 51a96ab966..af457d0019 100644 --- a/deployments/gpu-operator/templates/gpucluster.yaml +++ b/deployments/gpu-operator/templates/gpucluster.yaml @@ -124,6 +124,9 @@ spec: {{- if .Values.dcgmExporter.serviceMonitor }} serviceMonitor: {{ toYaml .Values.dcgmExporter.serviceMonitor | nindent 6 }} {{- end }} + {{- if .Values.dcgmExporter.serviceAccount }} + serviceAccount: {{ toYaml .Values.dcgmExporter.serviceAccount | nindent 6 }} + {{- end }} {{- if and (.Values.dcgmExporter.config) (.Values.dcgmExporter.config.name) }} config: name: {{ .Values.dcgmExporter.config.name }} diff --git a/internal/state/configurable_state.go b/internal/state/configurable_state.go index 83892666f3..e07a839fa1 100644 --- a/internal/state/configurable_state.go +++ b/internal/state/configurable_state.go @@ -49,6 +49,12 @@ type configurableState struct { // image path and DRA apiVersion. It receives ctx and the skeleton so operands that // need the client or logging (e.g. dcgm-exporter's ServiceMonitor CRD probe) can use them. buildRenderData func(ctx context.Context, s *configurableState, cr *nvidiav1alpha1.GPUCluster, imagePath, apiVersion, openshiftVersion string) (any, error) + + // preSync runs after the manifests render but before they are applied, for operands + // that depend on cluster state the templates cannot express. Returning an error marks + // the state NotReady, so it is the place to surface a misconfiguration instead of + // applying objects that cannot converge. + preSync func(ctx context.Context, s *configurableState, cr *nvidiav1alpha1.GPUCluster) error } var _ State = (*configurableState)(nil) @@ -68,6 +74,12 @@ func (s *configurableState) Sync(ctx context.Context, customResource any, infoCa return s.handleStateObjectsDeletion(ctx) } + if s.preSync != nil { + if err := s.preSync(ctx, s, cr); err != nil { + return SyncStateNotReady, err + } + } + return s.syncObjects(ctx, cr, objs) } diff --git a/internal/state/dcgm_exporter.go b/internal/state/dcgm_exporter.go index 7ac3d3da2a..96b9c39615 100644 --- a/internal/state/dcgm_exporter.go +++ b/internal/state/dcgm_exporter.go @@ -18,11 +18,16 @@ package state import ( "context" + "fmt" "path/filepath" "strings" + corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/log" @@ -71,6 +76,7 @@ func NewStateDCGMExporter( }, imageEnvName: dcgmExporterImageEnvName, buildRenderData: buildDCGMExporterRenderData, + preSync: checkDCGMExporterServiceAccount, }, nil } @@ -149,6 +155,78 @@ func buildDCGMExporterRenderData(ctx context.Context, s *configurableState, cr * }, nil } +// checkDCGMExporterServiceAccount reconciles the parts of the ServiceAccount contract the +// manifests cannot express: a ServiceAccount the user brings has to already exist, one the +// operator would manage must not be an existing object owned by somebody else, and the +// operator-owned default is removed once a different name takes over. +func checkDCGMExporterServiceAccount(ctx context.Context, s *configurableState, cr *nvidiav1alpha1.GPUCluster) error { + spec := cr.Spec.DCGMExporter + name := spec.GetServiceAccountName(dcgmExporterDefaultServiceAccountName) + + if !spec.IsServiceAccountCreateEnabled() { + // The manifests omit the ServiceAccount entirely, so a missing one would leave + // the DaemonSet pending without any signal. + if _, err := s.getServiceAccount(ctx, name); err != nil { + if apierrors.IsNotFound(err) { + return fmt.Errorf( + "ServiceAccount %q configured with create=false does not exist in namespace %q", + name, s.namespace) + } + return err + } + return nil + } + + if name == dcgmExporterDefaultServiceAccountName { + return nil + } + + // 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) { + return err + } + if err == nil && !metav1.IsControlledBy(existing, cr) { + return fmt.Errorf( + "ServiceAccount %q already exists in namespace %q and is not managed by this GPUCluster; "+ + "set dcgmExporter.serviceAccount.create to false to reference it", + name, s.namespace) + } + + return s.deleteOwnedServiceAccount(ctx, cr, dcgmExporterDefaultServiceAccountName) +} + +// getServiceAccount reads a ServiceAccount from the operand namespace. +func (s *configurableState) getServiceAccount(ctx context.Context, name string) (*corev1.ServiceAccount, error) { + sa := &corev1.ServiceAccount{} + err := s.client.Get(ctx, types.NamespacedName{Namespace: s.namespace, Name: name}, sa) + return sa, err +} + +// deleteOwnedServiceAccount removes a ServiceAccount left behind by a previous +// configuration, but only when this CR owns it: an object the user provisioned under the +// same name is left alone. Renaming from one custom name to another is not tracked, so +// only the operator default is reclaimed here. +func (s *configurableState) deleteOwnedServiceAccount(ctx context.Context, cr *nvidiav1alpha1.GPUCluster, name string) error { + sa, err := s.getServiceAccount(ctx, name) + if err != nil { + if apierrors.IsNotFound(err) { + return nil + } + return err + } + if !metav1.IsControlledBy(sa, cr) { + return nil + } + log.FromContext(ctx).V(consts.LogLevelInfo).Info( + "Removing the superseded dcgm-exporter ServiceAccount", "Name", name) + if err := s.client.Delete(ctx, sa); err != nil && !apierrors.IsNotFound(err) { + return err + } + return nil +} + // serviceMonitorCRDServed reports whether the cluster serves the monitoring.coreos.com // ServiceMonitor kind (i.e. the Prometheus Operator CRDs are installed). func serviceMonitorCRDServed(k8sClient client.Client) bool { diff --git a/internal/state/dcgm_exporter_test.go b/internal/state/dcgm_exporter_test.go index 6b228808d6..1f177dfa26 100644 --- a/internal/state/dcgm_exporter_test.go +++ b/internal/state/dcgm_exporter_test.go @@ -23,11 +23,14 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/utils/ptr" + "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" nvidiav1 "github.com/NVIDIA/gpu-operator/api/nvidia/v1" @@ -59,6 +62,26 @@ func newTestDCGMExporterState(t *testing.T, serviceMonitorCRD bool) *configurabl } // exporterCR returns a sample CR with dcgm-exporter enabled and the given exporter spec. +// newTestDCGMExporterStateWithObjects builds the state with a client that already holds +// the given objects, for the ServiceAccount checks that read cluster state. +func newTestDCGMExporterStateWithObjects(t *testing.T, objs ...client.Object) *configurableState { + t.Helper() + t.Setenv("DCGM_EXPORTER_IMAGE", "nvcr.io/nvidia/k8s/dcgm-exporter:test") + + testScheme := runtime.NewScheme() + require.NoError(t, corev1.AddToScheme(testScheme)) + require.NoError(t, nvidiav1alpha1.AddToScheme(testScheme)) + + k8sClient := fake.NewClientBuilder(). + WithScheme(testScheme). + WithRESTMapper(restMapperWithServiceMonitor(false)). + WithObjects(objs...). + Build() + s, err := NewStateDCGMExporter(k8sClient, "test-operator", testScheme, dcgmExporterManifestDir) + require.NoError(t, err) + return s.(*configurableState) +} + func exporterCR(spec *nvidiav1.DCGMExporterSpec) *nvidiav1alpha1.GPUCluster { cr := sampleGPUCluster() cr.Spec.DCGMExporter = spec @@ -235,3 +258,156 @@ func TestDCGMExporterServiceType(t *testing.T) { itpValue, _, _ := unstructured.NestedString(svc.Object, "spec", "internalTrafficPolicy") assert.Equal(t, "Local", itpValue) } + +// kindNames collects the names of every rendered object of the given kind. +func kindNames(objs []*unstructured.Unstructured, kind string) []string { + var names []string + for _, o := range objs { + if o.GetKind() == kind { + names = append(names, o.GetName()) + } + } + return names +} + +// subjectNames collects the ServiceAccount subject names of a rendered RBAC binding. +func subjectNames(t *testing.T, objs []*unstructured.Unstructured, kind, name string) []string { + t.Helper() + for _, o := range objs { + if o.GetKind() != kind || o.GetName() != name { + continue + } + subjects, found, err := unstructured.NestedSlice(o.Object, "subjects") + require.NoError(t, err) + require.True(t, found) + var names []string + for _, raw := range subjects { + subject, ok := raw.(map[string]any) + require.True(t, ok) + names = append(names, subject["name"].(string)) + } + return names + } + t.Fatalf("%s %q not found in rendered objects", kind, name) + return nil +} + +func TestDCGMExporterDefaultServiceAccount(t *testing.T) { + s := newTestDCGMExporterState(t, false) + cr := exporterCR(&nvidiav1.DCGMExporterSpec{}) + + objs, err := s.getManifestObjects(context.Background(), cr, draSupportedCatalog()) + require.NoError(t, err) + + assert.Equal(t, []string{dcgmExporterDefaultServiceAccountName}, kindNames(objs, "ServiceAccount")) + assert.Equal(t, dcgmExporterDefaultServiceAccountName, + findDaemonSet(t, objs).Spec.Template.Spec.ServiceAccountName) +} + +func TestDCGMExporterCustomServiceAccountName(t *testing.T) { + s := newTestDCGMExporterState(t, false) + cr := exporterCR(&nvidiav1.DCGMExporterSpec{ + ServiceAccount: &nvidiav1.DCGMExporterServiceAccountConfig{Name: "metrics-identity"}, + }) + + objs, err := s.getManifestObjects(context.Background(), cr, draSupportedCatalog()) + require.NoError(t, err) + + // The operator still owns the ServiceAccount, only under the configured name. + assert.Equal(t, []string{"metrics-identity"}, kindNames(objs, "ServiceAccount")) + assert.Equal(t, "metrics-identity", findDaemonSet(t, objs).Spec.Template.Spec.ServiceAccountName) + assert.Equal(t, []string{"metrics-identity"}, + subjectNames(t, objs, "RoleBinding", "nvidia-dcgm-exporter-dra")) + assert.Equal(t, []string{"metrics-identity"}, + subjectNames(t, objs, "ClusterRoleBinding", "nvidia-dcgm-exporter-dra-read-pods")) + // The binding objects keep their own names. + assert.Equal(t, []string{"nvidia-dcgm-exporter-dra"}, kindNames(objs, "RoleBinding")) +} + +func TestDCGMExporterUserProvidedServiceAccount(t *testing.T) { + s := newTestDCGMExporterState(t, false) + cr := exporterCR(&nvidiav1.DCGMExporterSpec{ + ServiceAccount: &nvidiav1.DCGMExporterServiceAccountConfig{Name: "byo-sa", Create: new(false)}, + }) + + objs, err := s.getManifestObjects(context.Background(), cr, draSupportedCatalog()) + require.NoError(t, err) + + // The operator must not render a ServiceAccount it does not own, but every + // operand still has to reference it. + assert.Empty(t, kindNames(objs, "ServiceAccount")) + assert.Equal(t, "byo-sa", findDaemonSet(t, objs).Spec.Template.Spec.ServiceAccountName) + assert.Equal(t, []string{"byo-sa"}, subjectNames(t, objs, "RoleBinding", "nvidia-dcgm-exporter-dra")) + assert.Equal(t, []string{"byo-sa"}, + subjectNames(t, objs, "ClusterRoleBinding", "nvidia-dcgm-exporter-dra-read-pods")) +} + +func TestDCGMExporterServiceAccountPreSync(t *testing.T) { + ctx := context.Background() + + userServiceAccount := func(name string) *corev1.ServiceAccount { + return &corev1.ServiceAccount{ + ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: "test-operator"}, + } + } + + t.Run("create=false requires the ServiceAccount to exist", func(t *testing.T) { + s := newTestDCGMExporterState(t, false) + cr := exporterCR(&nvidiav1.DCGMExporterSpec{ + ServiceAccount: &nvidiav1.DCGMExporterServiceAccountConfig{Name: "byo-sa", Create: new(false)}, + }) + + err := checkDCGMExporterServiceAccount(ctx, s, cr) + require.ErrorContains(t, err, "byo-sa") + }) + + t.Run("create=false accepts an existing ServiceAccount", func(t *testing.T) { + s := newTestDCGMExporterStateWithObjects(t, userServiceAccount("byo-sa")) + cr := exporterCR(&nvidiav1.DCGMExporterSpec{ + ServiceAccount: &nvidiav1.DCGMExporterServiceAccountConfig{Name: "byo-sa", Create: new(false)}, + }) + + require.NoError(t, checkDCGMExporterServiceAccount(ctx, s, cr)) + }) + + t.Run("a configured name refuses to take over an unowned ServiceAccount", func(t *testing.T) { + s := newTestDCGMExporterStateWithObjects(t, userServiceAccount("metrics-identity")) + cr := exporterCR(&nvidiav1.DCGMExporterSpec{ + ServiceAccount: &nvidiav1.DCGMExporterServiceAccountConfig{Name: "metrics-identity"}, + }) + + err := checkDCGMExporterServiceAccount(ctx, s, cr) + require.ErrorContains(t, err, "not managed by this GPUCluster") + }) + + t.Run("renaming reclaims the superseded operator-owned default", func(t *testing.T) { + cr := exporterCR(&nvidiav1.DCGMExporterSpec{ + ServiceAccount: &nvidiav1.DCGMExporterServiceAccountConfig{Name: "metrics-identity"}, + }) + previous := userServiceAccount(dcgmExporterDefaultServiceAccountName) + previous.OwnerReferences = []metav1.OwnerReference{{ + APIVersion: nvidiav1alpha1.SchemeGroupVersion.String(), + Kind: "GPUCluster", + Name: cr.Name, + UID: cr.UID, + Controller: new(true), + }} + + s := newTestDCGMExporterStateWithObjects(t, previous) + require.NoError(t, checkDCGMExporterServiceAccount(ctx, s, cr)) + + _, err := s.getServiceAccount(ctx, dcgmExporterDefaultServiceAccountName) + require.True(t, apierrors.IsNotFound(err), "the superseded default must be removed") + }) + + t.Run("renaming keeps a previous ServiceAccount the operator does not own", func(t *testing.T) { + s := newTestDCGMExporterStateWithObjects(t, userServiceAccount(dcgmExporterDefaultServiceAccountName)) + cr := exporterCR(&nvidiav1.DCGMExporterSpec{ + ServiceAccount: &nvidiav1.DCGMExporterServiceAccountConfig{Name: "metrics-identity"}, + }) + + require.NoError(t, checkDCGMExporterServiceAccount(ctx, s, cr)) + _, err := s.getServiceAccount(ctx, dcgmExporterDefaultServiceAccountName) + require.NoError(t, err, "only an owned ServiceAccount may be reclaimed") + }) +} From 754d887c9bf8fab3861b65b71d8b8ea4b6d383e9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=EB=B0=B1=EC=A7=80=EB=AA=85?= Date: Wed, 9 Sep 2026 15:32:27 +0900 Subject: [PATCH 4/9] Reclaim the operator-owned default ServiceAccount on the BYO hand-off MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) Claude-Session: https://claude.ai/code/session_01UiFVTVAyEMmn68Xvo9LSTf Signed-off-by: 백지명 --- controllers/object_controls.go | 8 +++++ controllers/object_controls_test.go | 42 ++++++++++++++++++++++++++ internal/state/dcgm_exporter.go | 8 ++++- internal/state/dcgm_exporter_test.go | 44 ++++++++++++++++++++++++++++ 4 files changed, 101 insertions(+), 1 deletion(-) diff --git a/controllers/object_controls.go b/controllers/object_controls.go index bb56b68528..efe8ed2b65 100644 --- a/controllers/object_controls.go +++ b/controllers/object_controls.go @@ -426,6 +426,14 @@ func ServiceAccount(n ClusterPolicyController) (gpuv1.State, error) { } return gpuv1.NotReady, err } + // 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 { + if err := n.deleteOwnedServiceAccount(ctx, DCGMExporterDefaultServiceAccountName, logger); err != nil { + return gpuv1.NotReady, err + } + } return gpuv1.Ready, nil } diff --git a/controllers/object_controls_test.go b/controllers/object_controls_test.go index fed6482555..92b89b22a9 100644 --- a/controllers/object_controls_test.go +++ b/controllers/object_controls_test.go @@ -2650,6 +2650,48 @@ func TestDCGMExporterServiceAccountReconcile(t *testing.T) { require.Empty(t, sa.OwnerReferences, "the operator must not take ownership of a user-provided ServiceAccount") }) + t.Run("handing over to a user-provided ServiceAccount reclaims the owned default", func(t *testing.T) { + cp := clusterPolicy() + previous := serviceAccount(DCGMExporterDefaultServiceAccountName) + require.NoError(t, controllerutil.SetControllerReference(cp, previous, testScheme)) + + k8s := fake.NewClientBuilder().WithScheme(testScheme). + WithObjects(previous, serviceAccount(byoName)).Build() + cp.Spec.DCGMExporter.ServiceAccount = &gpuv1.DCGMExporterServiceAccountConfig{ + Name: byoName, Create: new(false), + } + n := newController(k8s, cp) + + state, err := ServiceAccount(n) + require.NoError(t, err) + require.Equal(t, gpuv1.Ready, state) + + _, ok := getServiceAccount(t, k8s, DCGMExporterDefaultServiceAccountName) + require.False(t, ok, "the superseded default must be removed on the BYO hand-off") + sa, ok := getServiceAccount(t, k8s, byoName) + require.True(t, ok) + require.Empty(t, sa.OwnerReferences) + }) + + t.Run("bringing the default name keeps that ServiceAccount", func(t *testing.T) { + cp := clusterPolicy() + existing := serviceAccount(DCGMExporterDefaultServiceAccountName) + require.NoError(t, controllerutil.SetControllerReference(cp, existing, testScheme)) + + k8s := fake.NewClientBuilder().WithScheme(testScheme).WithObjects(existing).Build() + cp.Spec.DCGMExporter.ServiceAccount = &gpuv1.DCGMExporterServiceAccountConfig{ + Name: DCGMExporterDefaultServiceAccountName, Create: new(false), + } + n := newController(k8s, cp) + + state, err := ServiceAccount(n) + require.NoError(t, err) + require.Equal(t, gpuv1.Ready, state) + + _, ok := getServiceAccount(t, k8s, DCGMExporterDefaultServiceAccountName) + require.True(t, ok, "the referenced ServiceAccount must not be reclaimed") + }) + t.Run("disabling the exporter keeps a user-provided ServiceAccount", func(t *testing.T) { k8s := fake.NewClientBuilder().WithScheme(testScheme).WithObjects(serviceAccount(byoName)).Build() cp := clusterPolicy() diff --git a/internal/state/dcgm_exporter.go b/internal/state/dcgm_exporter.go index 96b9c39615..86fff4a27c 100644 --- a/internal/state/dcgm_exporter.go +++ b/internal/state/dcgm_exporter.go @@ -174,7 +174,13 @@ func checkDCGMExporterServiceAccount(ctx context.Context, s *configurableState, } return err } - return nil + // 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 name == dcgmExporterDefaultServiceAccountName { + return nil + } + return s.deleteOwnedServiceAccount(ctx, cr, dcgmExporterDefaultServiceAccountName) } if name == dcgmExporterDefaultServiceAccountName { diff --git a/internal/state/dcgm_exporter_test.go b/internal/state/dcgm_exporter_test.go index 1f177dfa26..8a27c9492d 100644 --- a/internal/state/dcgm_exporter_test.go +++ b/internal/state/dcgm_exporter_test.go @@ -370,6 +370,50 @@ func TestDCGMExporterServiceAccountPreSync(t *testing.T) { require.NoError(t, checkDCGMExporterServiceAccount(ctx, s, cr)) }) + t.Run("create=false reclaims the operator-owned default", func(t *testing.T) { + cr := exporterCR(&nvidiav1.DCGMExporterSpec{ + ServiceAccount: &nvidiav1.DCGMExporterServiceAccountConfig{Name: "byo-sa", Create: new(false)}, + }) + previous := userServiceAccount(dcgmExporterDefaultServiceAccountName) + previous.OwnerReferences = []metav1.OwnerReference{{ + APIVersion: nvidiav1alpha1.SchemeGroupVersion.String(), + Kind: "GPUCluster", + Name: cr.Name, + UID: cr.UID, + Controller: new(true), + }} + + s := newTestDCGMExporterStateWithObjects(t, previous, userServiceAccount("byo-sa")) + require.NoError(t, checkDCGMExporterServiceAccount(ctx, s, cr)) + + _, err := s.getServiceAccount(ctx, dcgmExporterDefaultServiceAccountName) + require.True(t, apierrors.IsNotFound(err), "the superseded default must be removed") + _, err = s.getServiceAccount(ctx, "byo-sa") + require.NoError(t, err) + }) + + t.Run("create=false with the default name keeps that ServiceAccount", func(t *testing.T) { + cr := exporterCR(&nvidiav1.DCGMExporterSpec{ + ServiceAccount: &nvidiav1.DCGMExporterServiceAccountConfig{ + Name: dcgmExporterDefaultServiceAccountName, Create: new(false), + }, + }) + existing := userServiceAccount(dcgmExporterDefaultServiceAccountName) + existing.OwnerReferences = []metav1.OwnerReference{{ + APIVersion: nvidiav1alpha1.SchemeGroupVersion.String(), + Kind: "GPUCluster", + Name: cr.Name, + UID: cr.UID, + Controller: new(true), + }} + + s := newTestDCGMExporterStateWithObjects(t, existing) + require.NoError(t, checkDCGMExporterServiceAccount(ctx, s, cr)) + + _, err := s.getServiceAccount(ctx, dcgmExporterDefaultServiceAccountName) + require.NoError(t, err, "the referenced ServiceAccount must not be reclaimed") + }) + t.Run("a configured name refuses to take over an unowned ServiceAccount", func(t *testing.T) { s := newTestDCGMExporterStateWithObjects(t, userServiceAccount("metrics-identity")) cr := exporterCR(&nvidiav1.DCGMExporterSpec{ From cec36fa1d8a6406f89e3841a3e07fe5849b7592f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=EB=B0=B1=EC=A7=80=EB=AA=85?= Date: Fri, 11 Sep 2026 06:33:35 +0900 Subject: [PATCH 5/9] Defer the ServiceAccount reclaim until the state converged MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) Claude-Session: https://claude.ai/code/session_01UiFVTVAyEMmn68Xvo9LSTf Signed-off-by: 백지명 --- controllers/object_controls.go | 69 ++- controllers/object_controls_test.go | 642 ++++++++++++++------------- controllers/state_manager.go | 9 + internal/state/configurable_state.go | 18 +- internal/state/dcgm_exporter.go | 104 ++++- internal/state/dcgm_exporter_test.go | 482 ++++++++++++++------ internal/state/driver.go | 2 +- internal/state/state_skel.go | 18 +- 8 files changed, 875 insertions(+), 469 deletions(-) diff --git a/controllers/object_controls.go b/controllers/object_controls.go index efe8ed2b65..235f13e612 100644 --- a/controllers/object_controls.go +++ b/controllers/object_controls.go @@ -351,6 +351,49 @@ func (n ClusterPolicyController) isServiceAccountOwned(ctx context.Context, obj return metav1.IsControlledBy(found, n.singleton), nil } +// dcgmExporterServiceAccountRenamed reports whether the exporter is configured to use a +// ServiceAccount other than the operator default, i.e. whether a previously created +// default ServiceAccount may have been superseded. +func dcgmExporterServiceAccountRenamed(config *gpuv1.ClusterPolicySpec) bool { + return dcgmExporterServiceAccountName(config) != DCGMExporterDefaultServiceAccountName +} + +// releaseServiceAccountOwnership drops this ClusterPolicy's controller reference from a +// 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) { + return nil + } + refs := make([]metav1.OwnerReference, 0, len(sa.OwnerReferences)) + for _, ref := range sa.OwnerReferences { + if ref.UID == n.singleton.GetUID() { + continue + } + refs = append(refs, ref) + } + sa.OwnerReferences = refs + logger.V(1).Info("Releasing ownership of a user-provided ServiceAccount", "Name", sa.Name) + return n.client.Update(ctx, sa) +} + +// cleanupSupersededDCGMExporterServiceAccount removes the operator-created default +// ServiceAccount once a different one has taken over. It runs only after every control of +// 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]) { + return nil + } + if !dcgmExporterServiceAccountRenamed(&n.singleton.Spec) { + // The configured ServiceAccount is the default one, so there is nothing it + // could have superseded. + return nil + } + logger := n.logger.WithValues("ServiceAccount", DCGMExporterDefaultServiceAccountName, "Namespace", n.operatorNamespace) + return n.deleteOwnedServiceAccount(ctx, DCGMExporterDefaultServiceAccountName, logger) +} + // deleteOwnedServiceAccount removes a ServiceAccount left behind by a previous // configuration, but only when this ClusterPolicy owns it: an object the user // provisioned under the same name is left alone. @@ -426,19 +469,16 @@ func ServiceAccount(n ClusterPolicyController) (gpuv1.State, error) { } return gpuv1.NotReady, err } - // 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 { - if err := n.deleteOwnedServiceAccount(ctx, DCGMExporterDefaultServiceAccountName, logger); err != nil { - return gpuv1.NotReady, err - } + // The same ServiceAccount may have been operator-managed before the user set + // create=false. Leaving the controller reference in place would garbage-collect + // their object together with the ClusterPolicy. + if err := n.releaseServiceAccountOwnership(ctx, found, logger); err != nil { + return gpuv1.NotReady, err } return gpuv1.Ready, nil } - isRenamed := isDCGMExporter && obj.Name != DCGMExporterDefaultServiceAccountName - if isRenamed { + if isDCGMExporter && dcgmExporterServiceAccountRenamed(&n.singleton.Spec) { // Only a name the user chose can collide with an unrelated object; the default is // left tolerant so an upgrade that lost the owner reference keeps converging. owned, err := n.isServiceAccountOwned(ctx, obj) @@ -465,14 +505,9 @@ func ServiceAccount(n ClusterPolicyController) (gpuv1.State, error) { logger.Info("Found Resource, skipping update") } - if isRenamed { - // A previous configuration may have left the operator-owned default behind. - // Renaming between two custom names is not tracked, so only the default is - // reclaimed here. - if err := n.deleteOwnedServiceAccount(ctx, DCGMExporterDefaultServiceAccountName, logger); err != nil { - return gpuv1.NotReady, err - } - } + // Reclaiming the superseded default ServiceAccount is deferred to + // cleanupSupersededDCGMExporterServiceAccount, which runs once every control of this + // state has converged. return gpuv1.Ready, nil } diff --git a/controllers/object_controls_test.go b/controllers/object_controls_test.go index 92b89b22a9..72df13061d 100644 --- a/controllers/object_controls_test.go +++ b/controllers/object_controls_test.go @@ -2537,42 +2537,35 @@ func TestDriverPrecompiledLibModulesSuse(t *testing.T) { // 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. func TestDCGMExporterServiceAccountReconcile(t *testing.T) { const ( testNamespace = "test-namespace" byoName = "byo-metrics-identity" + customName = "metrics-identity" ) testScheme := runtime.NewScheme() require.NoError(t, corev1.AddToScheme(testScheme)) require.NoError(t, gpuv1.AddToScheme(testScheme)) - clusterPolicy := func() *gpuv1.ClusterPolicy { + newClusterPolicy := func() *gpuv1.ClusterPolicy { return &gpuv1.ClusterPolicy{ObjectMeta: metav1.ObjectMeta{Name: "cluster-policy", UID: "cp-uid"}} } - newController := func(k8s client.Client, cp *gpuv1.ClusterPolicy) ClusterPolicyController { - return ClusterPolicyController{ - client: k8s, - ctx: context.Background(), - singleton: cp, - scheme: testScheme, - operatorNamespace: testNamespace, - resources: []Resources{{ - ServiceAccount: corev1.ServiceAccount{ - ObjectMeta: metav1.ObjectMeta{Name: DCGMExporterDefaultServiceAccountName}, - }, - }}, - stateNames: []string{"state-dcgm-exporter"}, - idx: 0, - logger: ctrl.Log.WithName("test"), - } + serviceAccount := func(name string) *corev1.ServiceAccount { + return &corev1.ServiceAccount{ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: testNamespace}} } - serviceAccount := func(name string) *corev1.ServiceAccount { - return &corev1.ServiceAccount{ - ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: testNamespace}, - } + ownedServiceAccount := func(t *testing.T, cp *gpuv1.ClusterPolicy, name string) *corev1.ServiceAccount { + t.Helper() + sa := serviceAccount(name) + require.NoError(t, controllerutil.SetControllerReference(cp, sa, testScheme)) + return sa } getServiceAccount := func(t *testing.T, k8s client.Client, name string) (*corev1.ServiceAccount, bool) { @@ -2586,229 +2579,274 @@ func TestDCGMExporterServiceAccountReconcile(t *testing.T) { return found, true } - t.Run("default configuration creates the default ServiceAccount", func(t *testing.T) { - k8s := fake.NewClientBuilder().WithScheme(testScheme).Build() - cp := clusterPolicy() - n := newController(k8s, cp) - - state, err := ServiceAccount(n) - require.NoError(t, err) - require.Equal(t, gpuv1.Ready, state) - - sa, ok := getServiceAccount(t, k8s, DCGMExporterDefaultServiceAccountName) - require.True(t, ok) - require.True(t, metav1.IsControlledBy(sa, cp)) - }) + testCases := map[string]struct { + serviceAccount *gpuv1.DCGMExporterServiceAccountConfig + exporterState *bool + stateName string + // existing seeds the fake client; ownedExisting are seeded with a ClusterPolicy + // controller reference. + existing []string + ownedExisting []string + expectedState gpuv1.State + expectedError bool + // assert runs after ServiceAccount(); present reports whether each name exists. + assert func(t *testing.T, k8s client.Client, cp *gpuv1.ClusterPolicy) + }{ + "default configuration creates the default ServiceAccount": { + expectedState: gpuv1.Ready, + assert: func(t *testing.T, k8s client.Client, cp *gpuv1.ClusterPolicy) { + sa, ok := getServiceAccount(t, k8s, DCGMExporterDefaultServiceAccountName) + require.True(t, ok) + require.True(t, metav1.IsControlledBy(sa, cp)) + }, + }, + "configured name creates that ServiceAccount instead of the default": { + serviceAccount: &gpuv1.DCGMExporterServiceAccountConfig{Name: customName}, + expectedState: gpuv1.Ready, + assert: func(t *testing.T, k8s client.Client, cp *gpuv1.ClusterPolicy) { + _, ok := getServiceAccount(t, k8s, customName) + require.True(t, ok) + _, ok = getServiceAccount(t, k8s, DCGMExporterDefaultServiceAccountName) + require.False(t, ok, "the default ServiceAccount must not be created as well") + }, + }, + "a configured name refuses to take over an unowned ServiceAccount": { + serviceAccount: &gpuv1.DCGMExporterServiceAccountConfig{Name: customName}, + existing: []string{customName}, + expectedState: gpuv1.NotReady, + expectedError: true, + assert: func(t *testing.T, k8s client.Client, cp *gpuv1.ClusterPolicy) { + sa, ok := getServiceAccount(t, k8s, customName) + require.True(t, ok) + require.Empty(t, sa.OwnerReferences, "an existing ServiceAccount must not be adopted") + }, + }, + "create=false reports NotReady when the ServiceAccount is missing": { + serviceAccount: &gpuv1.DCGMExporterServiceAccountConfig{Name: byoName, Create: new(false)}, + expectedState: gpuv1.NotReady, + expectedError: true, + assert: func(t *testing.T, k8s client.Client, cp *gpuv1.ClusterPolicy) { + _, ok := getServiceAccount(t, k8s, byoName) + require.False(t, ok, "a user-provided ServiceAccount must never be created by the operator") + }, + }, + "create=false references an existing ServiceAccount without adopting it": { + serviceAccount: &gpuv1.DCGMExporterServiceAccountConfig{Name: byoName, Create: new(false)}, + existing: []string{byoName}, + expectedState: gpuv1.Ready, + assert: func(t *testing.T, k8s client.Client, cp *gpuv1.ClusterPolicy) { + sa, ok := getServiceAccount(t, k8s, byoName) + require.True(t, ok) + require.Empty(t, sa.OwnerReferences, "the operator must not take ownership of a user-provided ServiceAccount") + }, + }, + "create=false releases ownership of a ServiceAccount the operator had created": { + serviceAccount: &gpuv1.DCGMExporterServiceAccountConfig{ + Name: DCGMExporterDefaultServiceAccountName, Create: new(false), + }, + ownedExisting: []string{DCGMExporterDefaultServiceAccountName}, + expectedState: gpuv1.Ready, + assert: func(t *testing.T, k8s client.Client, cp *gpuv1.ClusterPolicy) { + sa, ok := getServiceAccount(t, k8s, DCGMExporterDefaultServiceAccountName) + require.True(t, ok, "the referenced ServiceAccount must not be deleted") + require.False(t, metav1.IsControlledBy(sa, cp), + "the owner reference has to go, otherwise the user's ServiceAccount is garbage-collected with the ClusterPolicy") + }, + }, + "disabling the exporter keeps a user-provided ServiceAccount": { + serviceAccount: &gpuv1.DCGMExporterServiceAccountConfig{Name: byoName, Create: new(false)}, + exporterState: new(false), + existing: []string{byoName}, + expectedState: gpuv1.Disabled, + assert: func(t *testing.T, k8s client.Client, cp *gpuv1.ClusterPolicy) { + _, ok := getServiceAccount(t, k8s, byoName) + require.True(t, ok, "a user-provided ServiceAccount must survive disabling the exporter") + }, + }, + "disabling the exporter keeps a ServiceAccount the operator does not own": { + exporterState: new(false), + existing: []string{DCGMExporterDefaultServiceAccountName}, + expectedState: gpuv1.Disabled, + assert: func(t *testing.T, k8s client.Client, cp *gpuv1.ClusterPolicy) { + _, ok := getServiceAccount(t, k8s, DCGMExporterDefaultServiceAccountName) + require.True(t, ok, "only a ServiceAccount owned by the ClusterPolicy may be deleted") + }, + }, + "disabling the exporter deletes the ServiceAccount the operator owns": { + exporterState: new(false), + ownedExisting: []string{DCGMExporterDefaultServiceAccountName}, + expectedState: gpuv1.Disabled, + assert: func(t *testing.T, k8s client.Client, cp *gpuv1.ClusterPolicy) { + _, ok := getServiceAccount(t, k8s, DCGMExporterDefaultServiceAccountName) + require.False(t, ok) + }, + }, + "a non-DCGM state deletes its ServiceAccount regardless of ownership": { + // The ownership check is scoped to the DCGM Exporter; every other state keeps + // the previous unconditional cleanup on disable. + stateName: "state-driver", + existing: []string{DCGMExporterDefaultServiceAccountName}, + expectedState: gpuv1.Disabled, + assert: func(t *testing.T, k8s client.Client, cp *gpuv1.ClusterPolicy) { + _, ok := getServiceAccount(t, k8s, DCGMExporterDefaultServiceAccountName) + require.False(t, ok) + }, + }, + } - t.Run("configured name creates that ServiceAccount", func(t *testing.T) { - k8s := fake.NewClientBuilder().WithScheme(testScheme).Build() - cp := clusterPolicy() - cp.Spec.DCGMExporter.ServiceAccount = &gpuv1.DCGMExporterServiceAccountConfig{Name: "metrics-identity"} - n := newController(k8s, cp) + for name, tc := range testCases { + t.Run(name, func(t *testing.T) { + cp := newClusterPolicy() + cp.Spec.DCGMExporter.ServiceAccount = tc.serviceAccount + cp.Spec.DCGMExporter.Enabled = tc.exporterState - state, err := ServiceAccount(n) - require.NoError(t, err) - require.Equal(t, gpuv1.Ready, state) - - _, ok := getServiceAccount(t, k8s, "metrics-identity") - require.True(t, ok) - _, ok = getServiceAccount(t, k8s, DCGMExporterDefaultServiceAccountName) - require.False(t, ok, "the default ServiceAccount must not be created as well") - }) - - t.Run("create=false reports NotReady when the ServiceAccount is missing", func(t *testing.T) { - k8s := fake.NewClientBuilder().WithScheme(testScheme).Build() - cp := clusterPolicy() - cp.Spec.DCGMExporter.ServiceAccount = &gpuv1.DCGMExporterServiceAccountConfig{ - Name: byoName, Create: new(false), - } - n := newController(k8s, cp) - - state, err := ServiceAccount(n) - require.Error(t, err) - require.True(t, apierrors.IsNotFound(err)) - require.Equal(t, gpuv1.NotReady, state) - - _, ok := getServiceAccount(t, k8s, byoName) - require.False(t, ok, "a user-provided ServiceAccount must never be created by the operator") - }) - - t.Run("create=false references an existing ServiceAccount without adopting it", func(t *testing.T) { - k8s := fake.NewClientBuilder().WithScheme(testScheme).WithObjects(serviceAccount(byoName)).Build() - cp := clusterPolicy() - cp.Spec.DCGMExporter.ServiceAccount = &gpuv1.DCGMExporterServiceAccountConfig{ - Name: byoName, Create: new(false), - } - n := newController(k8s, cp) + objects := make([]client.Object, 0, len(tc.existing)+len(tc.ownedExisting)) + for _, saName := range tc.existing { + objects = append(objects, serviceAccount(saName)) + } + for _, saName := range tc.ownedExisting { + objects = append(objects, ownedServiceAccount(t, cp, saName)) + } - state, err := ServiceAccount(n) - require.NoError(t, err) - require.Equal(t, gpuv1.Ready, state) - - sa, ok := getServiceAccount(t, k8s, byoName) - require.True(t, ok) - require.Empty(t, sa.OwnerReferences, "the operator must not take ownership of a user-provided ServiceAccount") - }) - - t.Run("handing over to a user-provided ServiceAccount reclaims the owned default", func(t *testing.T) { - cp := clusterPolicy() - previous := serviceAccount(DCGMExporterDefaultServiceAccountName) - require.NoError(t, controllerutil.SetControllerReference(cp, previous, testScheme)) - - k8s := fake.NewClientBuilder().WithScheme(testScheme). - WithObjects(previous, serviceAccount(byoName)).Build() - cp.Spec.DCGMExporter.ServiceAccount = &gpuv1.DCGMExporterServiceAccountConfig{ - Name: byoName, Create: new(false), - } - n := newController(k8s, cp) + k8s := fake.NewClientBuilder().WithScheme(testScheme).WithObjects(objects...).Build() + stateName := tc.stateName + if stateName == "" { + stateName = "state-dcgm-exporter" + } + if stateName == "state-driver" { + cp.Spec.Driver.Enabled = new(false) + } - state, err := ServiceAccount(n) - require.NoError(t, err) - require.Equal(t, gpuv1.Ready, state) - - _, ok := getServiceAccount(t, k8s, DCGMExporterDefaultServiceAccountName) - require.False(t, ok, "the superseded default must be removed on the BYO hand-off") - sa, ok := getServiceAccount(t, k8s, byoName) - require.True(t, ok) - require.Empty(t, sa.OwnerReferences) - }) - - t.Run("bringing the default name keeps that ServiceAccount", func(t *testing.T) { - cp := clusterPolicy() - existing := serviceAccount(DCGMExporterDefaultServiceAccountName) - require.NoError(t, controllerutil.SetControllerReference(cp, existing, testScheme)) - - k8s := fake.NewClientBuilder().WithScheme(testScheme).WithObjects(existing).Build() - cp.Spec.DCGMExporter.ServiceAccount = &gpuv1.DCGMExporterServiceAccountConfig{ - Name: DCGMExporterDefaultServiceAccountName, Create: new(false), - } - n := newController(k8s, cp) + n := ClusterPolicyController{ + client: k8s, + ctx: context.Background(), + singleton: cp, + scheme: testScheme, + operatorNamespace: testNamespace, + resources: []Resources{{ + ServiceAccount: corev1.ServiceAccount{ + ObjectMeta: metav1.ObjectMeta{Name: DCGMExporterDefaultServiceAccountName}, + }, + }}, + stateNames: []string{stateName}, + idx: 0, + logger: ctrl.Log.WithName("test"), + } - state, err := ServiceAccount(n) - require.NoError(t, err) - require.Equal(t, gpuv1.Ready, state) - - _, ok := getServiceAccount(t, k8s, DCGMExporterDefaultServiceAccountName) - require.True(t, ok, "the referenced ServiceAccount must not be reclaimed") - }) - - t.Run("disabling the exporter keeps a user-provided ServiceAccount", func(t *testing.T) { - k8s := fake.NewClientBuilder().WithScheme(testScheme).WithObjects(serviceAccount(byoName)).Build() - cp := clusterPolicy() - cp.Spec.DCGMExporter.Enabled = new(false) - cp.Spec.DCGMExporter.ServiceAccount = &gpuv1.DCGMExporterServiceAccountConfig{ - Name: byoName, Create: new(false), - } - n := newController(k8s, cp) + state, err := ServiceAccount(n) + if tc.expectedError { + require.Error(t, err) + } else { + require.NoError(t, err) + } + require.Equal(t, tc.expectedState, state) + tc.assert(t, k8s, cp) + }) + } +} - state, err := ServiceAccount(n) - require.NoError(t, err) - require.Equal(t, gpuv1.Disabled, state) +// TestDCGMExporterSupersededServiceAccountCleanup covers the deferred reclaim of the +// operator default. It runs only after every control of the state converged, so the +// RoleBindings, SCC and DaemonSet already reference the replacement by then. +func TestDCGMExporterSupersededServiceAccountCleanup(t *testing.T) { + const ( + testNamespace = "test-namespace" + customName = "metrics-identity" + ) - _, ok := getServiceAccount(t, k8s, byoName) - require.True(t, ok, "a user-provided ServiceAccount must survive disabling the exporter") - }) + testScheme := runtime.NewScheme() + require.NoError(t, corev1.AddToScheme(testScheme)) + require.NoError(t, gpuv1.AddToScheme(testScheme)) - t.Run("disabling the exporter keeps a ServiceAccount the operator does not own", func(t *testing.T) { - // Same name as the operator default, but provisioned by the user beforehand. - k8s := fake.NewClientBuilder().WithScheme(testScheme). - WithObjects(serviceAccount(DCGMExporterDefaultServiceAccountName)).Build() - cp := clusterPolicy() - cp.Spec.DCGMExporter.Enabled = new(false) - n := newController(k8s, cp) + testCases := map[string]struct { + serviceAccount *gpuv1.DCGMExporterServiceAccountConfig + exporterState *bool + stateName string + defaultOwned bool + expectDeleted bool + }{ + "renaming reclaims the superseded operator-owned default": { + serviceAccount: &gpuv1.DCGMExporterServiceAccountConfig{Name: customName}, + defaultOwned: true, + expectDeleted: true, + }, + "handing over to a user-provided ServiceAccount reclaims the owned default": { + serviceAccount: &gpuv1.DCGMExporterServiceAccountConfig{Name: customName, Create: new(false)}, + defaultOwned: true, + expectDeleted: true, + }, + "a previous default the operator does not own is left alone": { + serviceAccount: &gpuv1.DCGMExporterServiceAccountConfig{Name: customName}, + defaultOwned: false, + expectDeleted: false, + }, + "the default name supersedes nothing": { + defaultOwned: true, + expectDeleted: false, + }, + "bringing the default name keeps that ServiceAccount": { + serviceAccount: &gpuv1.DCGMExporterServiceAccountConfig{ + Name: DCGMExporterDefaultServiceAccountName, Create: new(false), + }, + defaultOwned: true, + expectDeleted: false, + }, + "a disabled exporter reclaims nothing here": { + serviceAccount: &gpuv1.DCGMExporterServiceAccountConfig{Name: customName}, + exporterState: new(false), + defaultOwned: true, + expectDeleted: false, + }, + "another state reclaims nothing": { + serviceAccount: &gpuv1.DCGMExporterServiceAccountConfig{Name: customName}, + stateName: "state-driver", + defaultOwned: true, + expectDeleted: false, + }, + } - state, err := ServiceAccount(n) - require.NoError(t, err) - require.Equal(t, gpuv1.Disabled, state) - - _, ok := getServiceAccount(t, k8s, DCGMExporterDefaultServiceAccountName) - require.True(t, ok, "only a ServiceAccount owned by the ClusterPolicy may be deleted") - }) - - t.Run("a configured name refuses to take over an unowned ServiceAccount", func(t *testing.T) { - k8s := fake.NewClientBuilder().WithScheme(testScheme). - WithObjects(serviceAccount("metrics-identity")).Build() - cp := clusterPolicy() - cp.Spec.DCGMExporter.ServiceAccount = &gpuv1.DCGMExporterServiceAccountConfig{Name: "metrics-identity"} - n := newController(k8s, cp) - - state, err := ServiceAccount(n) - require.Error(t, err) - require.Equal(t, gpuv1.NotReady, state) - - sa, ok := getServiceAccount(t, k8s, "metrics-identity") - require.True(t, ok) - require.Empty(t, sa.OwnerReferences, "an existing ServiceAccount must not be adopted") - }) - - t.Run("renaming reclaims the superseded operator-owned default", func(t *testing.T) { - cp := clusterPolicy() - previous := serviceAccount(DCGMExporterDefaultServiceAccountName) - require.NoError(t, controllerutil.SetControllerReference(cp, previous, testScheme)) - - k8s := fake.NewClientBuilder().WithScheme(testScheme).WithObjects(previous).Build() - cp.Spec.DCGMExporter.ServiceAccount = &gpuv1.DCGMExporterServiceAccountConfig{Name: "metrics-identity"} - n := newController(k8s, cp) - - state, err := ServiceAccount(n) - require.NoError(t, err) - require.Equal(t, gpuv1.Ready, state) - - _, ok := getServiceAccount(t, k8s, "metrics-identity") - require.True(t, ok) - _, ok = getServiceAccount(t, k8s, DCGMExporterDefaultServiceAccountName) - require.False(t, ok, "the superseded default must be removed") - }) - - t.Run("renaming keeps a previous ServiceAccount the operator does not own", func(t *testing.T) { - k8s := fake.NewClientBuilder().WithScheme(testScheme). - WithObjects(serviceAccount(DCGMExporterDefaultServiceAccountName)).Build() - cp := clusterPolicy() - cp.Spec.DCGMExporter.ServiceAccount = &gpuv1.DCGMExporterServiceAccountConfig{Name: "metrics-identity"} - n := newController(k8s, cp) - - state, err := ServiceAccount(n) - require.NoError(t, err) - require.Equal(t, gpuv1.Ready, state) - - _, ok := getServiceAccount(t, k8s, DCGMExporterDefaultServiceAccountName) - require.True(t, ok, "only an owned ServiceAccount may be reclaimed") - }) - - t.Run("a non-DCGM state deletes its ServiceAccount regardless of ownership", func(t *testing.T) { - // The ownership check is scoped to the DCGM Exporter; every other state keeps - // the previous unconditional cleanup on disable. - k8s := fake.NewClientBuilder().WithScheme(testScheme). - WithObjects(serviceAccount(DCGMExporterDefaultServiceAccountName)).Build() - cp := clusterPolicy() - cp.Spec.Driver.Enabled = new(false) - n := newController(k8s, cp) - n.stateNames = []string{"state-driver"} - - state, err := ServiceAccount(n) - require.NoError(t, err) - require.Equal(t, gpuv1.Disabled, state) + for name, tc := range testCases { + t.Run(name, func(t *testing.T) { + cp := &gpuv1.ClusterPolicy{ObjectMeta: metav1.ObjectMeta{Name: "cluster-policy", UID: "cp-uid"}} + cp.Spec.DCGMExporter.ServiceAccount = tc.serviceAccount + cp.Spec.DCGMExporter.Enabled = tc.exporterState - _, ok := getServiceAccount(t, k8s, DCGMExporterDefaultServiceAccountName) - require.False(t, ok) - }) + previous := &corev1.ServiceAccount{ + ObjectMeta: metav1.ObjectMeta{Name: DCGMExporterDefaultServiceAccountName, Namespace: testNamespace}, + } + if tc.defaultOwned { + require.NoError(t, controllerutil.SetControllerReference(cp, previous, testScheme)) + } - t.Run("disabling the exporter deletes the ServiceAccount the operator owns", func(t *testing.T) { - cp := clusterPolicy() - owned := serviceAccount(DCGMExporterDefaultServiceAccountName) - require.NoError(t, controllerutil.SetControllerReference(cp, owned, testScheme)) + k8s := fake.NewClientBuilder().WithScheme(testScheme).WithObjects(previous).Build() + stateName := tc.stateName + if stateName == "" { + stateName = "state-dcgm-exporter" + } - k8s := fake.NewClientBuilder().WithScheme(testScheme).WithObjects(owned).Build() - cp.Spec.DCGMExporter.Enabled = new(false) - n := newController(k8s, cp) + n := ClusterPolicyController{ + client: k8s, + ctx: context.Background(), + singleton: cp, + scheme: testScheme, + operatorNamespace: testNamespace, + stateNames: []string{stateName}, + idx: 0, + logger: ctrl.Log.WithName("test"), + } - state, err := ServiceAccount(n) - require.NoError(t, err) - require.Equal(t, gpuv1.Disabled, state) + require.NoError(t, n.cleanupSupersededDCGMExporterServiceAccount(context.Background())) - _, ok := getServiceAccount(t, k8s, DCGMExporterDefaultServiceAccountName) - require.False(t, ok) - }) + found := &corev1.ServiceAccount{} + err := k8s.Get(context.Background(), + types.NamespacedName{Namespace: testNamespace, Name: DCGMExporterDefaultServiceAccountName}, found) + if tc.expectDeleted { + require.True(t, apierrors.IsNotFound(err), "the superseded default must be reclaimed") + } else { + require.NoError(t, err, "this ServiceAccount must not be reclaimed") + } + }) + } } // TestDCGMExporterRBACSubjects verifies that the RBAC bindings and the OpenShift SCC @@ -2826,84 +2864,88 @@ func TestDCGMExporterRBACSubjects(t *testing.T) { require.NoError(t, secv1.AddToScheme(testScheme)) require.NoError(t, gpuv1.AddToScheme(testScheme)) - newController := func(k8s client.Client, spec gpuv1.ClusterPolicySpec, res Resources) ClusterPolicyController { - return ClusterPolicyController{ - client: k8s, - ctx: context.Background(), - singleton: &gpuv1.ClusterPolicy{ObjectMeta: metav1.ObjectMeta{Name: "cluster-policy", UID: "cp-uid"}, Spec: spec}, - scheme: testScheme, - operatorNamespace: testNamespace, - resources: []Resources{res}, - stateNames: []string{"state-dcgm-exporter"}, - idx: 0, - logger: ctrl.Log.WithName("test"), - } - } - - customSpec := gpuv1.ClusterPolicySpec{ + spec := gpuv1.ClusterPolicySpec{ DCGMExporter: gpuv1.DCGMExporterSpec{ EnablePodLabels: new(true), ServiceAccount: &gpuv1.DCGMExporterServiceAccountConfig{Name: customSA}, }, } - t.Run("RoleBinding subject follows the configured ServiceAccount", func(t *testing.T) { - k8s := fake.NewClientBuilder().WithScheme(testScheme).Build() - res := Resources{RoleBinding: rbacv1.RoleBinding{ - ObjectMeta: metav1.ObjectMeta{Name: DCGMExporterDefaultServiceAccountName}, - Subjects: []rbacv1.Subject{ - {Kind: rbacv1.ServiceAccountKind, Name: DCGMExporterDefaultServiceAccountName, Namespace: filled}, - // Kept verbatim, mirroring 0500_prom_rolebinding_openshift.yaml. - {Kind: rbacv1.ServiceAccountKind, Name: "prometheus-k8s", Namespace: "openshift-monitoring"}, + testCases := map[string]struct { + resources Resources + // control applies the object and returns the resulting state. + control func(ClusterPolicyController) (gpuv1.State, error) + assert func(t *testing.T, k8s client.Client) + }{ + "RoleBinding subject follows the configured ServiceAccount": { + resources: Resources{RoleBinding: rbacv1.RoleBinding{ + ObjectMeta: metav1.ObjectMeta{Name: DCGMExporterDefaultServiceAccountName}, + Subjects: []rbacv1.Subject{ + {Kind: rbacv1.ServiceAccountKind, Name: DCGMExporterDefaultServiceAccountName, Namespace: filled}, + // Kept verbatim, mirroring 0500_prom_rolebinding_openshift.yaml. + {Kind: rbacv1.ServiceAccountKind, Name: "prometheus-k8s", Namespace: "openshift-monitoring"}, + }, + }}, + control: RoleBinding, + assert: func(t *testing.T, k8s client.Client) { + found := &rbacv1.RoleBinding{} + require.NoError(t, k8s.Get(context.Background(), + types.NamespacedName{Namespace: testNamespace, Name: DCGMExporterDefaultServiceAccountName}, found)) + require.Equal(t, customSA, found.Subjects[0].Name) + require.Equal(t, testNamespace, found.Subjects[0].Namespace) + require.Equal(t, "prometheus-k8s", found.Subjects[1].Name) + require.Equal(t, "openshift-monitoring", found.Subjects[1].Namespace) }, - }} - - state, err := RoleBinding(newController(k8s, customSpec, res)) - require.NoError(t, err) - require.Equal(t, gpuv1.Ready, state) - - found := &rbacv1.RoleBinding{} - require.NoError(t, k8s.Get(context.Background(), - types.NamespacedName{Namespace: testNamespace, Name: DCGMExporterDefaultServiceAccountName}, found)) - require.Equal(t, customSA, found.Subjects[0].Name) - require.Equal(t, testNamespace, found.Subjects[0].Namespace) - require.Equal(t, "prometheus-k8s", found.Subjects[1].Name) - require.Equal(t, "openshift-monitoring", found.Subjects[1].Namespace) - }) - - t.Run("ClusterRoleBinding subject follows the configured ServiceAccount", func(t *testing.T) { - k8s := fake.NewClientBuilder().WithScheme(testScheme).Build() - res := Resources{ClusterRoleBinding: rbacv1.ClusterRoleBinding{ - ObjectMeta: metav1.ObjectMeta{Name: "nvidia-dcgm-exporter-read-pods"}, - Subjects: []rbacv1.Subject{ - {Kind: rbacv1.ServiceAccountKind, Name: DCGMExporterDefaultServiceAccountName, Namespace: filled}, + }, + "ClusterRoleBinding subject follows the configured ServiceAccount": { + resources: Resources{ClusterRoleBinding: rbacv1.ClusterRoleBinding{ + ObjectMeta: metav1.ObjectMeta{Name: "nvidia-dcgm-exporter-read-pods"}, + Subjects: []rbacv1.Subject{ + {Kind: rbacv1.ServiceAccountKind, Name: DCGMExporterDefaultServiceAccountName, Namespace: filled}, + }, + }}, + control: ClusterRoleBinding, + assert: func(t *testing.T, k8s client.Client) { + found := &rbacv1.ClusterRoleBinding{} + require.NoError(t, k8s.Get(context.Background(), + types.NamespacedName{Namespace: testNamespace, Name: "nvidia-dcgm-exporter-read-pods"}, found)) + require.Equal(t, customSA, found.Subjects[0].Name) }, - }} + }, + "SCC user follows the ServiceAccount while the SCC name is unchanged": { + resources: Resources{SecurityContextConstraints: secv1.SecurityContextConstraints{ + ObjectMeta: metav1.ObjectMeta{Name: DCGMExporterDefaultServiceAccountName}, + Users: []string{filled}, + }}, + control: SecurityContextConstraints, + assert: func(t *testing.T, k8s client.Client) { + found := &secv1.SecurityContextConstraints{} + require.NoError(t, k8s.Get(context.Background(), + types.NamespacedName{Namespace: testNamespace, Name: DCGMExporterDefaultServiceAccountName}, found)) + require.Equal(t, []string{fmt.Sprintf("system:serviceaccount:%s:%s", testNamespace, customSA)}, found.Users) + }, + }, + } - state, err := ClusterRoleBinding(newController(k8s, customSpec, res)) - require.NoError(t, err) - require.Equal(t, gpuv1.Ready, state) - - found := &rbacv1.ClusterRoleBinding{} - require.NoError(t, k8s.Get(context.Background(), - types.NamespacedName{Namespace: testNamespace, Name: "nvidia-dcgm-exporter-read-pods"}, found)) - require.Equal(t, customSA, found.Subjects[0].Name) - }) - - t.Run("SCC user follows the ServiceAccount while the SCC name is unchanged", func(t *testing.T) { - k8s := fake.NewClientBuilder().WithScheme(testScheme).Build() - res := Resources{SecurityContextConstraints: secv1.SecurityContextConstraints{ - ObjectMeta: metav1.ObjectMeta{Name: DCGMExporterDefaultServiceAccountName}, - Users: []string{filled}, - }} - - state, err := SecurityContextConstraints(newController(k8s, customSpec, res)) - require.NoError(t, err) - require.Equal(t, gpuv1.Ready, state) + for name, tc := range testCases { + t.Run(name, func(t *testing.T) { + k8s := fake.NewClientBuilder().WithScheme(testScheme).Build() + n := ClusterPolicyController{ + client: k8s, + ctx: context.Background(), + singleton: &gpuv1.ClusterPolicy{ObjectMeta: metav1.ObjectMeta{Name: "cluster-policy", UID: "cp-uid"}, Spec: spec}, + scheme: testScheme, + operatorNamespace: testNamespace, + resources: []Resources{tc.resources}, + stateNames: []string{"state-dcgm-exporter"}, + idx: 0, + logger: ctrl.Log.WithName("test"), + } - found := &secv1.SecurityContextConstraints{} - require.NoError(t, k8s.Get(context.Background(), - types.NamespacedName{Namespace: testNamespace, Name: DCGMExporterDefaultServiceAccountName}, found)) - require.Equal(t, []string{fmt.Sprintf("system:serviceaccount:%s:%s", testNamespace, customSA)}, found.Users) - }) + state, err := tc.control(n) + require.NoError(t, err) + require.Equal(t, gpuv1.Ready, state) + tc.assert(t, k8s) + }) + } } diff --git a/controllers/state_manager.go b/controllers/state_manager.go index 65ea23ce88..95b91dd989 100644 --- a/controllers/state_manager.go +++ b/controllers/state_manager.go @@ -987,6 +987,15 @@ func (n *ClusterPolicyController) step() (gpuv1.State, error) { } } + // 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 { + if err := n.cleanupSupersededDCGMExporterServiceAccount(n.ctx); err != nil { + return gpuv1.NotReady, err + } + } + // move to next state n.idx++ diff --git a/internal/state/configurable_state.go b/internal/state/configurable_state.go index e07a839fa1..0a6e7e9b04 100644 --- a/internal/state/configurable_state.go +++ b/internal/state/configurable_state.go @@ -55,6 +55,11 @@ type configurableState struct { // the state NotReady, so it is the place to surface a misconfiguration instead of // applying objects that cannot converge. preSync func(ctx context.Context, s *configurableState, cr *nvidiav1alpha1.GPUCluster) error + + // postSync runs after the manifests converged. Reclaiming objects a previous + // configuration superseded belongs here rather than in preSync: deleting them before + // the replacements exist would leave the operands referencing objects that are gone. + postSync func(ctx context.Context, s *configurableState, cr *nvidiav1alpha1.GPUCluster) error } var _ State = (*configurableState)(nil) @@ -80,7 +85,18 @@ func (s *configurableState) Sync(ctx context.Context, customResource any, infoCa } } - return s.syncObjects(ctx, cr, objs) + 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 + } + } + + return syncState, nil } func (s *configurableState) GetWatchSources(mgr ctrlManager) map[string]SyncingSource { diff --git a/internal/state/dcgm_exporter.go b/internal/state/dcgm_exporter.go index 86fff4a27c..06b2a3b56d 100644 --- a/internal/state/dcgm_exporter.go +++ b/internal/state/dcgm_exporter.go @@ -25,6 +25,7 @@ import ( corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" @@ -65,6 +66,7 @@ func NewStateDCGMExporter( if err != nil { return nil, err } + skel.adoptionGuard = guardDCGMExporterServiceAccountAdoption return &configurableState{ stateSkel: skel, isEnabled: func(cr *nvidiav1alpha1.GPUCluster) bool { @@ -77,6 +79,7 @@ func NewStateDCGMExporter( imageEnvName: dcgmExporterImageEnvName, buildRenderData: buildDCGMExporterRenderData, preSync: checkDCGMExporterServiceAccount, + postSync: reconcileDCGMExporterServiceAccountOwnership, }, nil } @@ -174,13 +177,7 @@ func checkDCGMExporterServiceAccount(ctx context.Context, s *configurableState, } return err } - // 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 name == dcgmExporterDefaultServiceAccountName { - return nil - } - return s.deleteOwnedServiceAccount(ctx, cr, dcgmExporterDefaultServiceAccountName) + return nil } if name == dcgmExporterDefaultServiceAccountName { @@ -189,20 +186,105 @@ func checkDCGMExporterServiceAccount(ctx context.Context, s *configurableState, // 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. + // guardDCGMExporterServiceAccountAdoption re-checks this after the create call, for + // an object that appears in between. existing, err := s.getServiceAccount(ctx, name) if err != nil && !apierrors.IsNotFound(err) { return err } if err == nil && !metav1.IsControlledBy(existing, cr) { - return fmt.Errorf( - "ServiceAccount %q already exists in namespace %q and is not managed by this GPUCluster; "+ - "set dcgmExporter.serviceAccount.create to false to reference it", - name, s.namespace) + return dcgmExporterServiceAccountTakeoverError(name, s.namespace) } + return nil +} + +// dcgmExporterServiceAccountTakeoverError is the error returned when the configured +// ServiceAccount exists but belongs to somebody else. +func dcgmExporterServiceAccountTakeoverError(name, namespace string) error { + return fmt.Errorf( + "ServiceAccount %q already exists in namespace %q and is not managed by this GPUCluster; "+ + "set dcgmExporter.serviceAccount.create to false to reference it", + name, namespace) +} + +// guardDCGMExporterServiceAccountAdoption stops createOrUpdateObjs from taking over a +// ServiceAccount that appeared between the preSync ownership check and the create call. +// Without it the AlreadyExists path would stamp this CR's controller reference onto an +// object somebody else owns, handing it to garbage collection with the GPUCluster. +func guardDCGMExporterServiceAccountAdoption(owner metav1.Object, current *unstructured.Unstructured) error { + if current.GetKind() != "ServiceAccount" { + return nil + } + for _, ref := range current.GetOwnerReferences() { + if ref.Controller != nil && *ref.Controller && ref.UID == owner.GetUID() { + return nil + } + } + if current.GetName() == dcgmExporterDefaultServiceAccountName && len(current.GetOwnerReferences()) == 0 { + // The operator default may predate owner references (upgrade from an older + // release), so it stays adoptable. + return nil + } + return dcgmExporterServiceAccountTakeoverError(current.GetName(), current.GetNamespace()) +} + +// reconcileDCGMExporterServiceAccountOwnership runs once the manifests converged. It +// reclaims the operator default a different ServiceAccount superseded, and releases +// ownership of a ServiceAccount the user took over with create=false. +func reconcileDCGMExporterServiceAccountOwnership(ctx context.Context, s *configurableState, cr *nvidiav1alpha1.GPUCluster) error { + spec := cr.Spec.DCGMExporter + name := spec.GetServiceAccountName(dcgmExporterDefaultServiceAccountName) + + if !spec.IsServiceAccountCreateEnabled() { + // The same object may have been operator-managed before create was set to false. + // Both the controller reference and the state label have to go, otherwise it is + // garbage-collected with the GPUCluster or swept by the state cleanup. + sa, err := s.getServiceAccount(ctx, name) + if err != nil { + if apierrors.IsNotFound(err) { + return nil + } + return err + } + if err := s.releaseServiceAccount(ctx, cr, sa); err != nil { + return err + } + } + + if name == dcgmExporterDefaultServiceAccountName { + return nil + } return s.deleteOwnedServiceAccount(ctx, cr, dcgmExporterDefaultServiceAccountName) } +// 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 + if metav1.IsControlledBy(sa, cr) { + refs := make([]metav1.OwnerReference, 0, len(sa.OwnerReferences)) + for _, ref := range sa.OwnerReferences { + if ref.UID == cr.GetUID() { + continue + } + refs = append(refs, ref) + } + sa.OwnerReferences = refs + changed = true + } + if _, ok := sa.Labels[consts.StateLabel]; ok { + delete(sa.Labels, consts.StateLabel) + changed = true + } + if !changed { + return nil + } + log.FromContext(ctx).V(consts.LogLevelInfo).Info( + "Releasing ownership of a user-provided dcgm-exporter ServiceAccount", "Name", sa.Name) + return s.client.Update(ctx, sa) +} + // getServiceAccount reads a ServiceAccount from the operand namespace. func (s *configurableState) getServiceAccount(ctx context.Context, name string) (*corev1.ServiceAccount, error) { sa := &corev1.ServiceAccount{} diff --git a/internal/state/dcgm_exporter_test.go b/internal/state/dcgm_exporter_test.go index 8a27c9492d..11a6a21278 100644 --- a/internal/state/dcgm_exporter_test.go +++ b/internal/state/dcgm_exporter_test.go @@ -35,6 +35,7 @@ import ( nvidiav1 "github.com/NVIDIA/gpu-operator/api/nvidia/v1" nvidiav1alpha1 "github.com/NVIDIA/gpu-operator/api/nvidia/v1alpha1" + "github.com/NVIDIA/gpu-operator/internal/consts" ) const dcgmExporterManifestDir = "../../manifests/state-dcgm-exporter" @@ -292,166 +293,371 @@ func subjectNames(t *testing.T, objs []*unstructured.Unstructured, kind, name st return nil } -func TestDCGMExporterDefaultServiceAccount(t *testing.T) { - s := newTestDCGMExporterState(t, false) - cr := exporterCR(&nvidiav1.DCGMExporterSpec{}) - - objs, err := s.getManifestObjects(context.Background(), cr, draSupportedCatalog()) - require.NoError(t, err) +// TestDCGMExporterServiceAccountRendering covers what the configured ServiceAccount does to +// the rendered manifests: which object is created, and which operands reference it. +func TestDCGMExporterServiceAccountRendering(t *testing.T) { + const ( + customName = "metrics-identity" + byoName = "byo-sa" + ) + + testCases := map[string]struct { + serviceAccount *nvidiav1.DCGMExporterServiceAccountConfig + // created is the ServiceAccount the operator renders, empty when it renders none. + created string + // referenced is the name every operand has to point at. + referenced string + }{ + "the default configuration creates and references the operator default": { + created: dcgmExporterDefaultServiceAccountName, + referenced: dcgmExporterDefaultServiceAccountName, + }, + "a configured name is created and referenced under that name": { + serviceAccount: &nvidiav1.DCGMExporterServiceAccountConfig{Name: customName}, + created: customName, + referenced: customName, + }, + "create=false references the ServiceAccount without rendering it": { + serviceAccount: &nvidiav1.DCGMExporterServiceAccountConfig{Name: byoName, Create: new(false)}, + created: "", + referenced: byoName, + }, + } - assert.Equal(t, []string{dcgmExporterDefaultServiceAccountName}, kindNames(objs, "ServiceAccount")) - assert.Equal(t, dcgmExporterDefaultServiceAccountName, - findDaemonSet(t, objs).Spec.Template.Spec.ServiceAccountName) + for name, tc := range testCases { + t.Run(name, func(t *testing.T) { + s := newTestDCGMExporterState(t, false) + cr := exporterCR(&nvidiav1.DCGMExporterSpec{ServiceAccount: tc.serviceAccount}) + + objs, err := s.getManifestObjects(context.Background(), cr, draSupportedCatalog()) + require.NoError(t, err) + + if tc.created == "" { + assert.Empty(t, kindNames(objs, "ServiceAccount"), + "the operator must not render a ServiceAccount it does not own") + } else { + assert.Equal(t, []string{tc.created}, kindNames(objs, "ServiceAccount")) + } + + assert.Equal(t, tc.referenced, findDaemonSet(t, objs).Spec.Template.Spec.ServiceAccountName) + assert.Equal(t, []string{tc.referenced}, + subjectNames(t, objs, "RoleBinding", "nvidia-dcgm-exporter-dra")) + assert.Equal(t, []string{tc.referenced}, + subjectNames(t, objs, "ClusterRoleBinding", "nvidia-dcgm-exporter-dra-read-pods")) + // Only the subjects follow the ServiceAccount; the binding objects keep their names. + assert.Equal(t, []string{"nvidia-dcgm-exporter-dra"}, kindNames(objs, "RoleBinding")) + }) + } } -func TestDCGMExporterCustomServiceAccountName(t *testing.T) { - s := newTestDCGMExporterState(t, false) - cr := exporterCR(&nvidiav1.DCGMExporterSpec{ - ServiceAccount: &nvidiav1.DCGMExporterServiceAccountConfig{Name: "metrics-identity"}, - }) - - objs, err := s.getManifestObjects(context.Background(), cr, draSupportedCatalog()) - require.NoError(t, err) - - // The operator still owns the ServiceAccount, only under the configured name. - assert.Equal(t, []string{"metrics-identity"}, kindNames(objs, "ServiceAccount")) - assert.Equal(t, "metrics-identity", findDaemonSet(t, objs).Spec.Template.Spec.ServiceAccountName) - assert.Equal(t, []string{"metrics-identity"}, - subjectNames(t, objs, "RoleBinding", "nvidia-dcgm-exporter-dra")) - assert.Equal(t, []string{"metrics-identity"}, - subjectNames(t, objs, "ClusterRoleBinding", "nvidia-dcgm-exporter-dra-read-pods")) - // The binding objects keep their own names. - assert.Equal(t, []string{"nvidia-dcgm-exporter-dra"}, kindNames(objs, "RoleBinding")) +// ownedServiceAccount returns a ServiceAccount in the operand namespace, controlled by cr +// and labelled as belonging to this state, the way the sync would have left it. +func ownedServiceAccount(cr *nvidiav1alpha1.GPUCluster, name string) *corev1.ServiceAccount { + return &corev1.ServiceAccount{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: "test-operator", + Labels: map[string]string{consts.StateLabel: "state-dcgm-exporter"}, + OwnerReferences: []metav1.OwnerReference{{ + APIVersion: nvidiav1alpha1.SchemeGroupVersion.String(), + Kind: "GPUCluster", + Name: cr.Name, + UID: cr.UID, + Controller: new(true), + }}, + }, + } } -func TestDCGMExporterUserProvidedServiceAccount(t *testing.T) { - s := newTestDCGMExporterState(t, false) - cr := exporterCR(&nvidiav1.DCGMExporterSpec{ - ServiceAccount: &nvidiav1.DCGMExporterServiceAccountConfig{Name: "byo-sa", Create: new(false)}, - }) - - objs, err := s.getManifestObjects(context.Background(), cr, draSupportedCatalog()) - require.NoError(t, err) - - // The operator must not render a ServiceAccount it does not own, but every - // operand still has to reference it. - assert.Empty(t, kindNames(objs, "ServiceAccount")) - assert.Equal(t, "byo-sa", findDaemonSet(t, objs).Spec.Template.Spec.ServiceAccountName) - assert.Equal(t, []string{"byo-sa"}, subjectNames(t, objs, "RoleBinding", "nvidia-dcgm-exporter-dra")) - assert.Equal(t, []string{"byo-sa"}, - subjectNames(t, objs, "ClusterRoleBinding", "nvidia-dcgm-exporter-dra-read-pods")) +// unownedServiceAccount returns a ServiceAccount in the operand namespace that the +// operator did not create. +func unownedServiceAccount(name string) *corev1.ServiceAccount { + return &corev1.ServiceAccount{ + ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: "test-operator"}, + } } -func TestDCGMExporterServiceAccountPreSync(t *testing.T) { +// TestDCGMExporterServiceAccountValidation covers the preSync hook, which only rejects a +// configuration the manifests cannot express. It never mutates cluster state -- reclaiming +// what a previous configuration left behind happens in the postSync hook, once the +// operands stopped referencing it. +func TestDCGMExporterServiceAccountValidation(t *testing.T) { ctx := context.Background() - - userServiceAccount := func(name string) *corev1.ServiceAccount { - return &corev1.ServiceAccount{ - ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: "test-operator"}, - } + const ( + customName = "metrics-identity" + byoName = "byo-sa" + ) + + testCases := map[string]struct { + serviceAccount *nvidiav1.DCGMExporterServiceAccountConfig + existing func(cr *nvidiav1alpha1.GPUCluster) []client.Object + // expectedError is a substring of the error the hook must return; empty accepts. + expectedError string + }{ + "the default configuration needs nothing to exist": {}, + "create=false requires the ServiceAccount to exist": { + serviceAccount: &nvidiav1.DCGMExporterServiceAccountConfig{Name: byoName, Create: new(false)}, + expectedError: byoName, + }, + "create=false accepts an existing ServiceAccount": { + serviceAccount: &nvidiav1.DCGMExporterServiceAccountConfig{Name: byoName, Create: new(false)}, + existing: func(*nvidiav1alpha1.GPUCluster) []client.Object { + return []client.Object{unownedServiceAccount(byoName)} + }, + }, + "a configured name refuses to take over an unowned ServiceAccount": { + serviceAccount: &nvidiav1.DCGMExporterServiceAccountConfig{Name: customName}, + existing: func(*nvidiav1alpha1.GPUCluster) []client.Object { + return []client.Object{unownedServiceAccount(customName)} + }, + expectedError: "not managed by this GPUCluster", + }, + "a configured name accepts the ServiceAccount it already owns": { + serviceAccount: &nvidiav1.DCGMExporterServiceAccountConfig{Name: customName}, + existing: func(cr *nvidiav1alpha1.GPUCluster) []client.Object { + return []client.Object{ownedServiceAccount(cr, customName)} + }, + }, + "renaming leaves the superseded default for the postSync reclaim": { + serviceAccount: &nvidiav1.DCGMExporterServiceAccountConfig{Name: customName}, + existing: func(cr *nvidiav1alpha1.GPUCluster) []client.Object { + return []client.Object{ownedServiceAccount(cr, dcgmExporterDefaultServiceAccountName)} + }, + }, } - t.Run("create=false requires the ServiceAccount to exist", func(t *testing.T) { - s := newTestDCGMExporterState(t, false) - cr := exporterCR(&nvidiav1.DCGMExporterSpec{ - ServiceAccount: &nvidiav1.DCGMExporterServiceAccountConfig{Name: "byo-sa", Create: new(false)}, + for name, tc := range testCases { + t.Run(name, func(t *testing.T) { + cr := exporterCR(&nvidiav1.DCGMExporterSpec{ServiceAccount: tc.serviceAccount}) + var objs []client.Object + if tc.existing != nil { + objs = tc.existing(cr) + } + s := newTestDCGMExporterStateWithObjects(t, objs...) + + err := checkDCGMExporterServiceAccount(ctx, s, cr) + if tc.expectedError != "" { + require.ErrorContains(t, err, tc.expectedError) + return + } + require.NoError(t, err) + + // Validation only: whatever was there stays there. + for _, obj := range objs { + _, getErr := s.getServiceAccount(ctx, obj.GetName()) + require.NoError(t, getErr, "the preSync hook must not remove %q", obj.GetName()) + } }) + } +} - err := checkDCGMExporterServiceAccount(ctx, s, cr) - require.ErrorContains(t, err, "byo-sa") - }) +// TestDCGMExporterServiceAccountOwnershipReconcile covers the postSync hook: it runs after +// the manifests converged, so the operands already reference the new ServiceAccount and +// the superseded one can be reclaimed. +func TestDCGMExporterServiceAccountOwnershipReconcile(t *testing.T) { + ctx := context.Background() + const ( + customName = "metrics-identity" + byoName = "byo-sa" + ) + + testCases := map[string]struct { + serviceAccount *nvidiav1.DCGMExporterServiceAccountConfig + existing func(cr *nvidiav1alpha1.GPUCluster) []client.Object + // deleted names the ServiceAccounts that must be gone afterwards, kept those that + // must survive, and released those that must survive without operator ownership. + deleted []string + kept []string + released []string + }{ + "the default configuration reclaims nothing": { + existing: func(cr *nvidiav1alpha1.GPUCluster) []client.Object { + return []client.Object{ownedServiceAccount(cr, dcgmExporterDefaultServiceAccountName)} + }, + kept: []string{dcgmExporterDefaultServiceAccountName}, + }, + "renaming reclaims the superseded operator-owned default": { + serviceAccount: &nvidiav1.DCGMExporterServiceAccountConfig{Name: customName}, + existing: func(cr *nvidiav1alpha1.GPUCluster) []client.Object { + return []client.Object{ + ownedServiceAccount(cr, dcgmExporterDefaultServiceAccountName), + ownedServiceAccount(cr, customName), + } + }, + deleted: []string{dcgmExporterDefaultServiceAccountName}, + kept: []string{customName}, + }, + "renaming keeps a previous ServiceAccount the operator does not own": { + serviceAccount: &nvidiav1.DCGMExporterServiceAccountConfig{Name: customName}, + existing: func(*nvidiav1alpha1.GPUCluster) []client.Object { + return []client.Object{unownedServiceAccount(dcgmExporterDefaultServiceAccountName)} + }, + kept: []string{dcgmExporterDefaultServiceAccountName}, + }, + "create=false reclaims the operator-owned default": { + serviceAccount: &nvidiav1.DCGMExporterServiceAccountConfig{Name: byoName, Create: new(false)}, + existing: func(cr *nvidiav1alpha1.GPUCluster) []client.Object { + return []client.Object{ + ownedServiceAccount(cr, dcgmExporterDefaultServiceAccountName), + unownedServiceAccount(byoName), + } + }, + deleted: []string{dcgmExporterDefaultServiceAccountName}, + kept: []string{byoName}, + }, + "create=false releases a ServiceAccount the operator used to own": { + serviceAccount: &nvidiav1.DCGMExporterServiceAccountConfig{ + Name: dcgmExporterDefaultServiceAccountName, Create: new(false), + }, + existing: func(cr *nvidiav1alpha1.GPUCluster) []client.Object { + return []client.Object{ownedServiceAccount(cr, dcgmExporterDefaultServiceAccountName)} + }, + released: []string{dcgmExporterDefaultServiceAccountName}, + }, + "create=false on a ServiceAccount that was never owned changes nothing": { + serviceAccount: &nvidiav1.DCGMExporterServiceAccountConfig{Name: byoName, Create: new(false)}, + existing: func(*nvidiav1alpha1.GPUCluster) []client.Object { + return []client.Object{unownedServiceAccount(byoName)} + }, + released: []string{byoName}, + }, + } - t.Run("create=false accepts an existing ServiceAccount", func(t *testing.T) { - s := newTestDCGMExporterStateWithObjects(t, userServiceAccount("byo-sa")) - cr := exporterCR(&nvidiav1.DCGMExporterSpec{ - ServiceAccount: &nvidiav1.DCGMExporterServiceAccountConfig{Name: "byo-sa", Create: new(false)}, + for name, tc := range testCases { + t.Run(name, func(t *testing.T) { + cr := exporterCR(&nvidiav1.DCGMExporterSpec{ServiceAccount: tc.serviceAccount}) + s := newTestDCGMExporterStateWithObjects(t, tc.existing(cr)...) + + require.NoError(t, reconcileDCGMExporterServiceAccountOwnership(ctx, s, cr)) + + for _, saName := range tc.deleted { + _, err := s.getServiceAccount(ctx, saName) + require.True(t, apierrors.IsNotFound(err), "%q must be reclaimed", saName) + } + for _, saName := range tc.kept { + _, err := s.getServiceAccount(ctx, saName) + require.NoError(t, err, "%q must not be reclaimed", saName) + } + for _, saName := range tc.released { + sa, err := s.getServiceAccount(ctx, saName) + require.NoError(t, err, "a user-provided ServiceAccount must never be deleted") + assert.False(t, metav1.IsControlledBy(sa, cr), + "the owner reference has to go, otherwise the user's ServiceAccount is garbage-collected with the GPUCluster") + assert.NotContains(t, sa.Labels, consts.StateLabel, + "the state label has to go, otherwise the state cleanup sweeps the user's ServiceAccount") + } }) + } +} - require.NoError(t, checkDCGMExporterServiceAccount(ctx, s, cr)) - }) +// TestDCGMExporterServiceAccountAdoptionGuard covers the veto the sync applies when the +// create call reports AlreadyExists: between the preSync check and that call somebody else +// may have created the ServiceAccount, and stamping our controller reference onto it would +// hand it to garbage collection. +func TestDCGMExporterServiceAccountAdoptionGuard(t *testing.T) { + cr := exporterCR(&nvidiav1.DCGMExporterSpec{}) - t.Run("create=false reclaims the operator-owned default", func(t *testing.T) { - cr := exporterCR(&nvidiav1.DCGMExporterSpec{ - ServiceAccount: &nvidiav1.DCGMExporterServiceAccountConfig{Name: "byo-sa", Create: new(false)}, - }) - previous := userServiceAccount(dcgmExporterDefaultServiceAccountName) - previous.OwnerReferences = []metav1.OwnerReference{{ - APIVersion: nvidiav1alpha1.SchemeGroupVersion.String(), - Kind: "GPUCluster", - Name: cr.Name, - UID: cr.UID, - Controller: new(true), - }} - - s := newTestDCGMExporterStateWithObjects(t, previous, userServiceAccount("byo-sa")) - require.NoError(t, checkDCGMExporterServiceAccount(ctx, s, cr)) - - _, err := s.getServiceAccount(ctx, dcgmExporterDefaultServiceAccountName) - require.True(t, apierrors.IsNotFound(err), "the superseded default must be removed") - _, err = s.getServiceAccount(ctx, "byo-sa") - require.NoError(t, err) - }) + current := func(kind, name string, refs []metav1.OwnerReference) *unstructured.Unstructured { + obj := &unstructured.Unstructured{} + obj.SetKind(kind) + obj.SetName(name) + obj.SetNamespace("test-operator") + obj.SetOwnerReferences(refs) + return obj + } + ourRef := []metav1.OwnerReference{{ + APIVersion: nvidiav1alpha1.SchemeGroupVersion.String(), + Kind: "GPUCluster", + Name: cr.Name, + UID: cr.UID, + Controller: new(true), + }} + foreignRef := []metav1.OwnerReference{{ + APIVersion: "apps/v1", + Kind: "Deployment", + Name: "somebody-else", + UID: "other-uid", + Controller: new(true), + }} + + testCases := map[string]struct { + current *unstructured.Unstructured + expectedError string + }{ + "another kind is not this guard's business": { + current: current("ConfigMap", "metrics-identity", foreignRef), + }, + "a ServiceAccount this CR already controls is ours to update": { + current: current("ServiceAccount", "metrics-identity", ourRef), + }, + "a ServiceAccount somebody else controls is refused": { + current: current("ServiceAccount", "metrics-identity", foreignRef), + expectedError: "not managed by this GPUCluster", + }, + "an unowned ServiceAccount under a configured name is refused": { + current: current("ServiceAccount", "metrics-identity", nil), + expectedError: "not managed by this GPUCluster", + }, + "the operator default without owner references stays adoptable": { + // It predates owner references, i.e. an upgrade from an older release. + current: current("ServiceAccount", dcgmExporterDefaultServiceAccountName, nil), + }, + "the operator default somebody else controls is refused": { + current: current("ServiceAccount", dcgmExporterDefaultServiceAccountName, foreignRef), + expectedError: "not managed by this GPUCluster", + }, + } - t.Run("create=false with the default name keeps that ServiceAccount", func(t *testing.T) { - cr := exporterCR(&nvidiav1.DCGMExporterSpec{ - ServiceAccount: &nvidiav1.DCGMExporterServiceAccountConfig{ - Name: dcgmExporterDefaultServiceAccountName, Create: new(false), - }, + for name, tc := range testCases { + t.Run(name, func(t *testing.T) { + err := guardDCGMExporterServiceAccountAdoption(cr, tc.current) + if tc.expectedError != "" { + require.ErrorContains(t, err, tc.expectedError) + return + } + require.NoError(t, err) }) - existing := userServiceAccount(dcgmExporterDefaultServiceAccountName) - existing.OwnerReferences = []metav1.OwnerReference{{ - APIVersion: nvidiav1alpha1.SchemeGroupVersion.String(), - Kind: "GPUCluster", - Name: cr.Name, - UID: cr.UID, - Controller: new(true), - }} - - s := newTestDCGMExporterStateWithObjects(t, existing) - require.NoError(t, checkDCGMExporterServiceAccount(ctx, s, cr)) - - _, err := s.getServiceAccount(ctx, dcgmExporterDefaultServiceAccountName) - require.NoError(t, err, "the referenced ServiceAccount must not be reclaimed") + } +} + +// TestDCGMExporterServiceAccountAdoptionGuardWiring covers the guard where it matters: the +// AlreadyExists path of the sync, which is the only place the operator would ever write an +// owner reference onto an object it did not create. +func TestDCGMExporterServiceAccountAdoptionGuardWiring(t *testing.T) { + ctx := context.Background() + cr := exporterCR(&nvidiav1.DCGMExporterSpec{ + ServiceAccount: &nvidiav1.DCGMExporterServiceAccountConfig{Name: "metrics-identity"}, }) - t.Run("a configured name refuses to take over an unowned ServiceAccount", func(t *testing.T) { - s := newTestDCGMExporterStateWithObjects(t, userServiceAccount("metrics-identity")) - cr := exporterCR(&nvidiav1.DCGMExporterSpec{ - ServiceAccount: &nvidiav1.DCGMExporterServiceAccountConfig{Name: "metrics-identity"}, - }) + testScheme := runtime.NewScheme() + require.NoError(t, corev1.AddToScheme(testScheme)) + require.NoError(t, nvidiav1alpha1.AddToScheme(testScheme)) - err := checkDCGMExporterServiceAccount(ctx, s, cr) - require.ErrorContains(t, err, "not managed by this GPUCluster") - }) + // The ServiceAccount appeared after the preSync check accepted the configuration. + existing := unownedServiceAccount("metrics-identity") + k8sClient := fake.NewClientBuilder().WithScheme(testScheme).WithObjects(existing).Build() - t.Run("renaming reclaims the superseded operator-owned default", func(t *testing.T) { - cr := exporterCR(&nvidiav1.DCGMExporterSpec{ - ServiceAccount: &nvidiav1.DCGMExporterServiceAccountConfig{Name: "metrics-identity"}, - }) - previous := userServiceAccount(dcgmExporterDefaultServiceAccountName) - previous.OwnerReferences = []metav1.OwnerReference{{ - APIVersion: nvidiav1alpha1.SchemeGroupVersion.String(), - Kind: "GPUCluster", - Name: cr.Name, - UID: cr.UID, - Controller: new(true), - }} - - s := newTestDCGMExporterStateWithObjects(t, previous) - require.NoError(t, checkDCGMExporterServiceAccount(ctx, s, cr)) - - _, err := s.getServiceAccount(ctx, dcgmExporterDefaultServiceAccountName) - require.True(t, apierrors.IsNotFound(err), "the superseded default must be removed") - }) + skel := &stateSkel{ + name: "state-dcgm-exporter", + namespace: "test-operator", + client: k8sClient, + scheme: testScheme, + adoptionGuard: guardDCGMExporterServiceAccountAdoption, + } - t.Run("renaming keeps a previous ServiceAccount the operator does not own", func(t *testing.T) { - s := newTestDCGMExporterStateWithObjects(t, userServiceAccount(dcgmExporterDefaultServiceAccountName)) - cr := exporterCR(&nvidiav1.DCGMExporterSpec{ - ServiceAccount: &nvidiav1.DCGMExporterServiceAccountConfig{Name: "metrics-identity"}, - }) + desired := &unstructured.Unstructured{} + desired.SetAPIVersion("v1") + desired.SetKind("ServiceAccount") + desired.SetName("metrics-identity") + desired.SetNamespace("test-operator") - require.NoError(t, checkDCGMExporterServiceAccount(ctx, s, cr)) - _, err := s.getServiceAccount(ctx, dcgmExporterDefaultServiceAccountName) - require.NoError(t, err, "only an owned ServiceAccount may be reclaimed") - }) + err := skel.createOrUpdateObjs(ctx, cr, func(*unstructured.Unstructured) error { return nil }, + []*unstructured.Unstructured{desired}) + require.ErrorContains(t, err, "not managed by this GPUCluster") + + // The object the guard refused must be left exactly as it was found. + found, err := (&configurableState{stateSkel: *skel}).getServiceAccount(ctx, "metrics-identity") + require.NoError(t, err) + assert.Empty(t, found.OwnerReferences) + assert.NotContains(t, found.Labels, consts.StateLabel) } diff --git a/internal/state/driver.go b/internal/state/driver.go index bf7d0b5270..48947f438f 100644 --- a/internal/state/driver.go +++ b/internal/state/driver.go @@ -148,7 +148,7 @@ func (s *stateDriver) Sync(ctx context.Context, customResource any, infoCatalog } // Create objects if they don't exist, Update objects if they do exist - err = s.createOrUpdateObjs(ctx, func(obj *unstructured.Unstructured) error { + err = s.createOrUpdateObjs(ctx, cr, func(obj *unstructured.Unstructured) error { if err := controllerutil.SetControllerReference(cr, obj, s.scheme); err != nil { return fmt.Errorf("failed to set controller reference for object: %v", err) } diff --git a/internal/state/state_skel.go b/internal/state/state_skel.go index 21c37b86c9..542dd1148c 100644 --- a/internal/state/state_skel.go +++ b/internal/state/state_skel.go @@ -49,6 +49,12 @@ type stateSkel struct { client client.Client scheme *runtime.Scheme renderer render.Renderer + + // adoptionGuard, when set, vetoes taking over an object that already exists and is + // not owned by the CR being reconciled. It runs after the create call reports + // AlreadyExists, which closes the window between an ownership check made earlier in + // the sync and the create itself. + adoptionGuard func(owner metav1.Object, current *unstructured.Unstructured) error } // Name provides the State name @@ -102,7 +108,7 @@ func (s *stateSkel) renderObjects(ctx context.Context, data any) ([]*unstructure // state. Owner references make every object (including cluster-scoped ones) garbage // collected when the owning CR is deleted. func (s *stateSkel) syncObjects(ctx context.Context, owner metav1.Object, objs []*unstructured.Unstructured) (SyncState, error) { - err := s.createOrUpdateObjs(ctx, func(obj *unstructured.Unstructured) error { + err := s.createOrUpdateObjs(ctx, owner, func(obj *unstructured.Unstructured) error { if err := controllerutil.SetControllerReference(owner, obj, s.scheme); err != nil { return fmt.Errorf("failed to set controller reference for object: %w", err) } @@ -300,6 +306,7 @@ func (s *stateSkel) updateObj(ctx context.Context, obj *unstructured.Unstructure func (s *stateSkel) createOrUpdateObjs( ctx context.Context, + owner metav1.Object, setControllerReference func(obj *unstructured.Unstructured) error, objs []*unstructured.Unstructured) error { reqLogger := log.FromContext(ctx) @@ -340,6 +347,15 @@ func (s *stateSkel) createOrUpdateObjs( return err } + // The object appeared between the checks made earlier in this sync and the + // create above, so its ownership has to be revalidated before it is merged + // into and updated with this CR's controller reference. + if s.adoptionGuard != nil { + if err := s.adoptionGuard(owner, currentObj); err != nil { + return err + } + } + if desiredObj.GetKind() == "DaemonSet" { if currentObjHash, ok := currentObj.GetAnnotations()[consts.NvidiaAnnotationHashKey]; ok { if desiredObjectHash == currentObjHash { From e37361833b0cb895ee579c92690dc2bd1e7b793e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=EB=B0=B1=EC=A7=80=EB=AA=85?= Date: Sat, 12 Sep 2026 02:56:19 +0900 Subject: [PATCH 6/9] Reclaim and release the DCGM Exporter ServiceAccount on the paths that were missed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) Signed-off-by: 백지명 --- controllers/object_controls.go | 14 ++++ controllers/object_controls_test.go | 117 +++++++++++++++++++++++++++ controllers/state_manager.go | 10 ++- internal/state/configurable_state.go | 11 +++ internal/state/dcgm_exporter.go | 23 ++++++ internal/state/dcgm_exporter_test.go | 86 ++++++++++++++++++++ 6 files changed, 260 insertions(+), 1 deletion(-) diff --git a/controllers/object_controls.go b/controllers/object_controls.go index 235f13e612..e3fbe22e77 100644 --- a/controllers/object_controls.go +++ b/controllers/object_controls.go @@ -436,6 +436,20 @@ func ServiceAccount(n ClusterPolicyController) (gpuv1.State, error) { // Check if state is disabled and cleanup resource if exists if !n.isStateEnabled(n.stateNames[n.idx]) { if unmanaged { + // The object may have been operator-managed before create was set to false. + // Disabling the exporter must still hand it back, or it keeps this + // ClusterPolicy's owner reference and is garbage-collected along with it. + found := &corev1.ServiceAccount{} + if err := n.client.Get(ctx, + types.NamespacedName{Namespace: obj.Namespace, Name: obj.Name}, found); err != nil { + if apierrors.IsNotFound(err) { + return gpuv1.Disabled, nil + } + return gpuv1.NotReady, err + } + if err := n.releaseServiceAccountOwnership(ctx, found, logger); err != nil { + return gpuv1.NotReady, err + } return gpuv1.Disabled, nil } if isDCGMExporter { diff --git a/controllers/object_controls_test.go b/controllers/object_controls_test.go index 72df13061d..8eb4c77410 100644 --- a/controllers/object_controls_test.go +++ b/controllers/object_controls_test.go @@ -2653,6 +2653,33 @@ func TestDCGMExporterServiceAccountReconcile(t *testing.T) { "the owner reference has to go, otherwise the user's ServiceAccount is garbage-collected with the ClusterPolicy") }, }, + "disabling the exporter releases a ServiceAccount the operator used to own": { + // The combined transition: the user takes the ServiceAccount over with + // create=false and disables the exporter in one update. Returning Disabled + // without releasing would leave the ClusterPolicy owner reference on it, so + // deleting the ClusterPolicy later garbage-collects the user's object. + serviceAccount: &gpuv1.DCGMExporterServiceAccountConfig{ + Name: DCGMExporterDefaultServiceAccountName, Create: new(false), + }, + exporterState: new(false), + ownedExisting: []string{DCGMExporterDefaultServiceAccountName}, + expectedState: gpuv1.Disabled, + assert: func(t *testing.T, k8s client.Client, cp *gpuv1.ClusterPolicy) { + sa, ok := getServiceAccount(t, k8s, DCGMExporterDefaultServiceAccountName) + require.True(t, ok, "a user-provided ServiceAccount must never be deleted") + require.False(t, metav1.IsControlledBy(sa, cp), + "the owner reference has to go, otherwise disabling the exporter hands the user's ServiceAccount to garbage collection") + }, + }, + "disabling the exporter tolerates a user-provided ServiceAccount that is gone": { + serviceAccount: &gpuv1.DCGMExporterServiceAccountConfig{Name: byoName, Create: new(false)}, + exporterState: new(false), + expectedState: gpuv1.Disabled, + assert: func(t *testing.T, k8s client.Client, cp *gpuv1.ClusterPolicy) { + _, ok := getServiceAccount(t, k8s, byoName) + require.False(t, ok) + }, + }, "disabling the exporter keeps a user-provided ServiceAccount": { serviceAccount: &gpuv1.DCGMExporterServiceAccountConfig{Name: byoName, Create: new(false)}, exporterState: new(false), @@ -2949,3 +2976,93 @@ func TestDCGMExporterRBACSubjects(t *testing.T) { }) } } + +// TestDCGMExporterCleanupSurvivesDisabledControls pins the convergence rule in step(): +// a control that reports Disabled is intentionally off, not unfinished, so it must not +// hold back the reclaim of the superseded ServiceAccount. Under the default exporter +// configuration the optional read-pods ClusterRole and ClusterRoleBinding always report +// Disabled, which previously made the reclaim unreachable. +func TestDCGMExporterCleanupSurvivesDisabledControls(t *testing.T) { + const ( + testNamespace = "test-namespace" + customName = "metrics-identity" + ) + + testScheme := runtime.NewScheme() + require.NoError(t, corev1.AddToScheme(testScheme)) + require.NoError(t, gpuv1.AddToScheme(testScheme)) + + testCases := map[string]struct { + // states each fake control reports, in order. + states []gpuv1.State + expectedState gpuv1.State + expectDeleted bool + }{ + "every control ready": { + states: []gpuv1.State{gpuv1.Ready, gpuv1.Ready}, + expectedState: gpuv1.Ready, + expectDeleted: true, + }, + "an optional control reporting disabled still counts as converged": { + states: []gpuv1.State{gpuv1.Ready, gpuv1.Disabled}, + expectedState: gpuv1.Disabled, + expectDeleted: true, + }, + "a control still coming up holds the reclaim back": { + states: []gpuv1.State{gpuv1.Ready, gpuv1.NotReady}, + expectedState: gpuv1.NotReady, + expectDeleted: false, + }, + "disabled and not-ready together still hold it back": { + states: []gpuv1.State{gpuv1.Disabled, gpuv1.NotReady}, + expectedState: gpuv1.NotReady, + expectDeleted: false, + }, + } + + for name, tc := range testCases { + t.Run(name, func(t *testing.T) { + cp := &gpuv1.ClusterPolicy{ObjectMeta: metav1.ObjectMeta{Name: "cluster-policy", UID: "cp-uid"}} + cp.Spec.DCGMExporter.ServiceAccount = &gpuv1.DCGMExporterServiceAccountConfig{Name: customName} + + superseded := &corev1.ServiceAccount{ + ObjectMeta: metav1.ObjectMeta{Name: DCGMExporterDefaultServiceAccountName, Namespace: testNamespace}, + } + require.NoError(t, controllerutil.SetControllerReference(cp, superseded, testScheme)) + k8s := fake.NewClientBuilder().WithScheme(testScheme).WithObjects(superseded).Build() + + // controlFunc is itself the slice of control functions for one state. + controls := make(controlFunc, 0, len(tc.states)) + for _, want := range tc.states { + controls = append(controls, func(ClusterPolicyController) (gpuv1.State, error) { + return want, nil + }) + } + + n := ClusterPolicyController{ + client: k8s, + ctx: context.Background(), + singleton: cp, + scheme: testScheme, + operatorNamespace: testNamespace, + stateNames: []string{"state-dcgm-exporter"}, + controls: []controlFunc{controls}, + idx: 0, + logger: ctrl.Log.WithName("test"), + } + + state, err := n.step() + require.NoError(t, err) + require.Equal(t, tc.expectedState, state) + + found := &corev1.ServiceAccount{} + getErr := k8s.Get(context.Background(), + types.NamespacedName{Namespace: testNamespace, Name: DCGMExporterDefaultServiceAccountName}, found) + if tc.expectDeleted { + require.True(t, apierrors.IsNotFound(getErr), "the superseded default must be reclaimed once the state converged") + } else { + require.NoError(t, getErr, "nothing may be reclaimed while a control is still coming up") + } + }) + } +} diff --git a/controllers/state_manager.go b/controllers/state_manager.go index 95b91dd989..70ef057545 100644 --- a/controllers/state_manager.go +++ b/controllers/state_manager.go @@ -975,6 +975,11 @@ func (n *ClusterPolicyController) step() (gpuv1.State, error) { return gpuv1.Disabled, nil } + // Convergence is tracked apart from the reported state: a control that reports + // Disabled is intentionally off rather than unfinished. Reusing result here would + // skip the reclaim below under the default exporter configuration, where the + // optional read-pods ClusterRole and ClusterRoleBinding always report Disabled. + converged := true for _, fs := range n.controls[n.idx] { stat, err := fs(*n) if err != nil { @@ -985,12 +990,15 @@ func (n *ClusterPolicyController) step() (gpuv1.State, error) { // mark overall status of this component as not-ready and continue with other resources, while this becomes ready result = stat } + if stat == gpuv1.NotReady { + converged = false + } } // 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 { + if converged { if err := n.cleanupSupersededDCGMExporterServiceAccount(n.ctx); err != nil { return gpuv1.NotReady, err } diff --git a/internal/state/configurable_state.go b/internal/state/configurable_state.go index 0a6e7e9b04..434d89e50b 100644 --- a/internal/state/configurable_state.go +++ b/internal/state/configurable_state.go @@ -60,6 +60,12 @@ type configurableState struct { // configuration superseded belongs here rather than in preSync: deleting them before // the replacements exist would leave the operands referencing objects that are gone. postSync func(ctx context.Context, s *configurableState, cr *nvidiav1alpha1.GPUCluster) error + + // preDelete runs before the generic cleanup removes every object carrying this + // state's label. An object the user took over still carries that label from when the + // operator managed it, so anything that must outlive the state has to be handed back + // here -- the cleanup itself only looks at the label, not at ownership. + preDelete func(ctx context.Context, s *configurableState, cr *nvidiav1alpha1.GPUCluster) error } var _ State = (*configurableState)(nil) @@ -76,6 +82,11 @@ func (s *configurableState) Sync(ctx context.Context, customResource any, infoCa } if len(objs) == 0 { + if s.preDelete != nil { + if err := s.preDelete(ctx, s, cr); err != nil { + return SyncStateNotReady, err + } + } return s.handleStateObjectsDeletion(ctx) } diff --git a/internal/state/dcgm_exporter.go b/internal/state/dcgm_exporter.go index 06b2a3b56d..715117c99e 100644 --- a/internal/state/dcgm_exporter.go +++ b/internal/state/dcgm_exporter.go @@ -80,6 +80,7 @@ func NewStateDCGMExporter( buildRenderData: buildDCGMExporterRenderData, preSync: checkDCGMExporterServiceAccount, postSync: reconcileDCGMExporterServiceAccountOwnership, + preDelete: releaseDCGMExporterServiceAccountOnDelete, }, nil } @@ -258,6 +259,28 @@ func reconcileDCGMExporterServiceAccountOwnership(ctx context.Context, s *config return s.deleteOwnedServiceAccount(ctx, cr, dcgmExporterDefaultServiceAccountName) } +// releaseDCGMExporterServiceAccountOnDelete hands a user-provided ServiceAccount back +// before the state is torn down. Disabling the exporter deletes every object carrying +// this state's label, and a ServiceAccount taken over with create=false still carries it +// from when the operator managed it -- without this, turning the exporter off would +// delete an object the operator no longer owns. +func releaseDCGMExporterServiceAccountOnDelete(ctx context.Context, s *configurableState, cr *nvidiav1alpha1.GPUCluster) error { + spec := cr.Spec.DCGMExporter + // A nil spec never named a ServiceAccount, and a managed one is meant to go with the + // state; only the user-provided case has to survive. + if spec == nil || spec.IsServiceAccountCreateEnabled() { + return nil + } + sa, err := s.getServiceAccount(ctx, spec.GetServiceAccountName(dcgmExporterDefaultServiceAccountName)) + if err != nil { + if apierrors.IsNotFound(err) { + return nil + } + return err + } + return s.releaseServiceAccount(ctx, cr, sa) +} + // 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 { diff --git a/internal/state/dcgm_exporter_test.go b/internal/state/dcgm_exporter_test.go index 11a6a21278..e440585da0 100644 --- a/internal/state/dcgm_exporter_test.go +++ b/internal/state/dcgm_exporter_test.go @@ -661,3 +661,89 @@ func TestDCGMExporterServiceAccountAdoptionGuardWiring(t *testing.T) { assert.Empty(t, found.OwnerReferences) assert.NotContains(t, found.Labels, consts.StateLabel) } + +// TestDCGMExporterServiceAccountReleasedOnDelete covers the combined transition: one +// update both hands the ServiceAccount to the user with create=false and disables the +// exporter. The generic state cleanup deletes every object carrying the state label, and +// the ServiceAccount still carries it from when the operator managed it, so ownership has +// to be released before that cleanup rather than in postSync -- which the disabled path +// never reaches. +func TestDCGMExporterServiceAccountReleasedOnDelete(t *testing.T) { + ctx := context.Background() + const byoName = "byo-sa" + + testCases := map[string]struct { + serviceAccount *nvidiav1.DCGMExporterServiceAccountConfig + existing func(cr *nvidiav1alpha1.GPUCluster) []client.Object + // released must survive with neither this CR's owner reference nor the state label. + released []string + // untouched must keep whatever the operator put on it, ready to be swept. + untouched []string + }{ + "create=false releases the ServiceAccount the operator used to own": { + serviceAccount: &nvidiav1.DCGMExporterServiceAccountConfig{Name: byoName, Create: new(false)}, + existing: func(cr *nvidiav1alpha1.GPUCluster) []client.Object { + return []client.Object{ownedServiceAccount(cr, byoName)} + }, + released: []string{byoName}, + }, + "create=false on a ServiceAccount the operator never owned changes nothing": { + serviceAccount: &nvidiav1.DCGMExporterServiceAccountConfig{Name: byoName, Create: new(false)}, + existing: func(*nvidiav1alpha1.GPUCluster) []client.Object { + return []client.Object{unownedServiceAccount(byoName)} + }, + released: []string{byoName}, + }, + "a managed ServiceAccount is left for the state cleanup to remove": { + existing: func(cr *nvidiav1alpha1.GPUCluster) []client.Object { + return []client.Object{ownedServiceAccount(cr, dcgmExporterDefaultServiceAccountName)} + }, + untouched: []string{dcgmExporterDefaultServiceAccountName}, + }, + } + + for name, tc := range testCases { + t.Run(name, func(t *testing.T) { + cr := exporterCR(&nvidiav1.DCGMExporterSpec{ServiceAccount: tc.serviceAccount}) + s := newTestDCGMExporterStateWithObjects(t, tc.existing(cr)...) + + require.NoError(t, releaseDCGMExporterServiceAccountOnDelete(ctx, s, cr)) + + for _, saName := range tc.released { + sa, err := s.getServiceAccount(ctx, saName) + require.NoError(t, err, "a user-provided ServiceAccount must never be deleted") + assert.False(t, metav1.IsControlledBy(sa, cr), + "the owner reference has to go, otherwise the user's ServiceAccount is garbage-collected with the GPUCluster") + assert.NotContains(t, sa.Labels, consts.StateLabel, + "the state label has to go, otherwise the state cleanup sweeps the user's ServiceAccount") + } + for _, saName := range tc.untouched { + sa, err := s.getServiceAccount(ctx, saName) + require.NoError(t, err) + assert.True(t, metav1.IsControlledBy(sa, cr)) + assert.Contains(t, sa.Labels, consts.StateLabel) + } + }) + } +} + +// TestDCGMExporterSyncReleasesBeforeDeletion drives Sync() on a disabled exporter and +// checks the hook actually runs on that path. +func TestDCGMExporterSyncReleasesBeforeDeletion(t *testing.T) { + ctx := context.Background() + const byoName = "byo-sa" + + cr := exporterCR(&nvidiav1.DCGMExporterSpec{ + Enabled: new(false), + ServiceAccount: &nvidiav1.DCGMExporterServiceAccountConfig{Name: byoName, Create: new(false)}, + }) + s := newTestDCGMExporterStateWithObjects(t, ownedServiceAccount(cr, byoName)) + + _, err := s.Sync(ctx, cr, draSupportedCatalog()) + require.NoError(t, err) + + sa, err := s.getServiceAccount(ctx, byoName) + require.NoError(t, err, "the user's ServiceAccount must survive disabling the exporter") + assert.False(t, metav1.IsControlledBy(sa, cr)) + assert.NotContains(t, sa.Labels, consts.StateLabel) +} From e3a00baa7f2e01391f3846ba9f04a81d83e75783 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=EB=B0=B1=EC=A7=80=EB=AA=85?= Date: Tue, 15 Sep 2026 11:14:42 +0900 Subject: [PATCH 7/9] Track the DCGM Exporter ServiceAccounts by label instead of by name MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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: 백지명 --- controllers/object_controls.go | 159 ++++++++++------- controllers/object_controls_test.go | 209 +++++++++++++++++----- internal/state/configurable_state.go | 4 +- internal/state/dcgm_exporter.go | 113 ++++++------ internal/state/dcgm_exporter_test.go | 256 ++++++++++++++++++++------- 5 files changed, 502 insertions(+), 239 deletions(-) diff --git a/controllers/object_controls.go b/controllers/object_controls.go index e3fbe22e77..f93c002270 100644 --- a/controllers/object_controls.go +++ b/controllers/object_controls.go @@ -336,10 +336,30 @@ var SubscriptionPathMap = map[string](MountPathToVolumeSource){ type controlFunc []func(n ClusterPolicyController) (gpuv1.State, error) -// ServiceAccount creates ServiceAccount resource +// dcgmExporterServiceAccountLabel is the app label every DCGM Exporter asset carries. The +// operator relies on it to find the ServiceAccounts it created for the exporter under +// whatever name a previous configuration gave them, so ServiceAccount() stamps it on each +// one it creates rather than trusting the asset. +const dcgmExporterServiceAccountLabel = "nvidia-dcgm-exporter" + +// markDCGMExporterServiceAccount labels a ServiceAccount as created for the DCGM Exporter. +func markDCGMExporterServiceAccount(sa *corev1.ServiceAccount) { + if sa.Labels == nil { + sa.Labels = map[string]string{} + } + sa.Labels["app"] = dcgmExporterServiceAccountLabel +} + +// isDCGMExporterServiceAccount reports whether the operator created the ServiceAccount for +// the DCGM Exporter. The ClusterPolicy controls the ServiceAccount of every operand, so +// ownership alone cannot tell the exporter's apart from, say, the driver's. +func isDCGMExporterServiceAccount(sa *corev1.ServiceAccount) bool { + return sa.Labels["app"] == dcgmExporterServiceAccountLabel +} + // isServiceAccountOwned reports whether the ServiceAccount exists and is controlled // by the ClusterPolicy being reconciled. A missing ServiceAccount counts as owned so -// that callers fall through to a delete that is a no-op. +// that callers fall through to a create that starts from a clean slate. func (n ClusterPolicyController) isServiceAccountOwned(ctx context.Context, obj *corev1.ServiceAccount) (bool, error) { found := &corev1.ServiceAccount{} if err := n.client.Get(ctx, types.NamespacedName{Namespace: obj.Namespace, Name: obj.Name}, found); err != nil { @@ -352,17 +372,18 @@ func (n ClusterPolicyController) isServiceAccountOwned(ctx context.Context, obj } // dcgmExporterServiceAccountRenamed reports whether the exporter is configured to use a -// ServiceAccount other than the operator default, i.e. whether a previously created -// default ServiceAccount may have been superseded. +// ServiceAccount other than the operator default. func dcgmExporterServiceAccountRenamed(config *gpuv1.ClusterPolicySpec) bool { return dcgmExporterServiceAccountName(config) != DCGMExporterDefaultServiceAccountName } -// releaseServiceAccountOwnership drops this ClusterPolicy's controller reference from a -// 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) { +// releaseDCGMExporterServiceAccount drops this ClusterPolicy's controller reference from a +// ServiceAccount the user has taken over with create=false. Without it the object stays +// garbage-collected together with the ClusterPolicy even though the operator no longer +// manages it. Only a ServiceAccount the exporter created is handed back: pointing +// create=false at another operand's ServiceAccount must leave that operand's ownership alone. +func (n ClusterPolicyController) releaseDCGMExporterServiceAccount(ctx context.Context, sa *corev1.ServiceAccount, logger logr.Logger) error { + if !isDCGMExporterServiceAccount(sa) || !metav1.IsControlledBy(sa, n.singleton) { return nil } refs := make([]metav1.OwnerReference, 0, len(sa.OwnerReferences)) @@ -377,55 +398,90 @@ func (n ClusterPolicyController) releaseServiceAccountOwnership(ctx context.Cont return n.client.Update(ctx, sa) } -// cleanupSupersededDCGMExporterServiceAccount removes the operator-created default -// ServiceAccount once a different one has taken over. It runs only after every control of -// the state converged, so a failure part-way through reconciliation never leaves the -// DaemonSet referencing a ServiceAccount that has already been deleted. +// cleanupSupersededDCGMExporterServiceAccount removes the ServiceAccounts the exporter +// created under a previous configuration once a different one has taken over. It runs only +// after every control of 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]) { + // A disabled exporter sweeps its ServiceAccounts in ServiceAccount() itself, + // where nothing references them any more. return nil } - if !dcgmExporterServiceAccountRenamed(&n.singleton.Spec) { - // The configured ServiceAccount is the default one, so there is nothing it - // could have superseded. - return nil - } - logger := n.logger.WithValues("ServiceAccount", DCGMExporterDefaultServiceAccountName, "Namespace", n.operatorNamespace) - return n.deleteOwnedServiceAccount(ctx, DCGMExporterDefaultServiceAccountName, logger) + logger := n.logger.WithValues("Namespace", n.operatorNamespace) + return n.deleteOwnedDCGMExporterServiceAccounts(ctx, dcgmExporterServiceAccountName(&n.singleton.Spec), logger) } -// deleteOwnedServiceAccount removes a ServiceAccount left behind by a previous -// configuration, but only when this ClusterPolicy owns it: an object the user -// provisioned under the same name is left alone. -func (n ClusterPolicyController) deleteOwnedServiceAccount(ctx context.Context, name string, logger logr.Logger) error { - found := &corev1.ServiceAccount{} - err := n.client.Get(ctx, types.NamespacedName{Namespace: n.operatorNamespace, Name: name}, found) - if err != nil { - if apierrors.IsNotFound(err) { - return nil +// disableDCGMExporterServiceAccount cleans up when the exporter is turned off. It sweeps +// every ServiceAccount the exporter created and this ClusterPolicy still owns -- not just +// the one currently configured -- so an update that disables the exporter and renames the +// ServiceAccount at once leaves nothing behind. A user-provided ServiceAccount (create=false) +// is handed back instead: it may have been operator-managed before, and returning Disabled +// with the owner reference in place would garbage-collect it with the ClusterPolicy. +func (n ClusterPolicyController) disableDCGMExporterServiceAccount(ctx context.Context, configured *corev1.ServiceAccount, unmanaged bool, logger logr.Logger) (gpuv1.State, error) { + keep := "" + if unmanaged { + keep = configured.Name + found := &corev1.ServiceAccount{} + err := n.client.Get(ctx, types.NamespacedName{Namespace: configured.Namespace, Name: configured.Name}, found) + switch { + case err == nil: + // Release before the sweep: once the controller reference is gone the sweep + // skips the object, whereas the other way round it would be deleted. + if err := n.releaseDCGMExporterServiceAccount(ctx, found, logger); err != nil { + return gpuv1.NotReady, err + } + case !apierrors.IsNotFound(err): + return gpuv1.NotReady, err } - return err } - if !metav1.IsControlledBy(found, n.singleton) { - return nil + if err := n.deleteOwnedDCGMExporterServiceAccounts(ctx, keep, logger); err != nil { + logger.Info("Couldn't delete", "Error", err) + return gpuv1.NotReady, err } - logger.V(1).Info("Removing the superseded dcgm-exporter ServiceAccount", "Name", name) - if err := n.client.Delete(ctx, found); err != nil && !apierrors.IsNotFound(err) { + return gpuv1.Disabled, nil +} + +// deleteOwnedDCGMExporterServiceAccounts removes every ServiceAccount the exporter created +// that this ClusterPolicy still controls, except the one named keep. Finding them by label +// rather than by name is what covers a rename from one custom name to another, or back to +// the default: no record of the previous configuration exists, but every ServiceAccount the +// operator created for the exporter carries the label. A ServiceAccount the user provisioned +// under one of those names carries no controller reference and is left alone. +func (n ClusterPolicyController) deleteOwnedDCGMExporterServiceAccounts(ctx context.Context, keep string, logger logr.Logger) error { + list := &corev1.ServiceAccountList{} + if err := n.client.List(ctx, list, + client.InNamespace(n.operatorNamespace), + client.MatchingLabels{"app": dcgmExporterServiceAccountLabel}); err != nil { return err } + for i := range list.Items { + sa := &list.Items[i] + if sa.Name == keep || !metav1.IsControlledBy(sa, n.singleton) { + 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) { + return err + } + } return nil } +// ServiceAccount creates ServiceAccount resource func ServiceAccount(n ClusterPolicyController) (gpuv1.State, error) { ctx := n.ctx state := n.idx obj := n.resources[state].ServiceAccount.DeepCopy() obj.Namespace = n.operatorNamespace - // The DCGM Exporter ServiceAccount name is user-configurable. + // The DCGM Exporter ServiceAccount name is user-configurable. The label is how the + // operator later finds the ServiceAccounts it created for the exporter, whatever they + // were named at the time. isDCGMExporter := n.stateNames[state] == "state-dcgm-exporter" if isDCGMExporter { obj.Name = dcgmExporterServiceAccountName(&n.singleton.Spec) + markDCGMExporterServiceAccount(obj) } // A ServiceAccount the user brings is only referenced, never managed: the // operator must not create, adopt, mutate or delete it. @@ -435,35 +491,8 @@ func ServiceAccount(n ClusterPolicyController) (gpuv1.State, error) { // Check if state is disabled and cleanup resource if exists if !n.isStateEnabled(n.stateNames[n.idx]) { - if unmanaged { - // The object may have been operator-managed before create was set to false. - // Disabling the exporter must still hand it back, or it keeps this - // ClusterPolicy's owner reference and is garbage-collected along with it. - found := &corev1.ServiceAccount{} - if err := n.client.Get(ctx, - types.NamespacedName{Namespace: obj.Namespace, Name: obj.Name}, found); err != nil { - if apierrors.IsNotFound(err) { - return gpuv1.Disabled, nil - } - return gpuv1.NotReady, err - } - if err := n.releaseServiceAccountOwnership(ctx, found, logger); err != nil { - return gpuv1.NotReady, err - } - return gpuv1.Disabled, nil - } if isDCGMExporter { - // A ServiceAccount that carries no ClusterPolicy owner reference was not - // created by this operator -- for instance one the user had already - // provisioned under the configured name -- so it is left untouched. - owned, err := n.isServiceAccountOwned(ctx, obj) - if err != nil { - return gpuv1.NotReady, err - } - if !owned { - logger.V(1).Info("ServiceAccount is not owned by the ClusterPolicy, skipping deletion") - return gpuv1.Disabled, nil - } + return n.disableDCGMExporterServiceAccount(ctx, obj, unmanaged, logger) } err := n.client.Delete(ctx, obj) if err != nil && !apierrors.IsNotFound(err) { @@ -486,7 +515,7 @@ func ServiceAccount(n ClusterPolicyController) (gpuv1.State, error) { // The same ServiceAccount may have been operator-managed before the user set // create=false. Leaving the controller reference in place would garbage-collect // their object together with the ClusterPolicy. - if err := n.releaseServiceAccountOwnership(ctx, found, logger); err != nil { + if err := n.releaseDCGMExporterServiceAccount(ctx, found, logger); err != nil { return gpuv1.NotReady, err } return gpuv1.Ready, nil @@ -519,7 +548,7 @@ func ServiceAccount(n ClusterPolicyController) (gpuv1.State, error) { logger.Info("Found Resource, skipping update") } - // Reclaiming the superseded default ServiceAccount is deferred to + // Reclaiming the ServiceAccounts a previous configuration left behind is deferred to // cleanupSupersededDCGMExporterServiceAccount, which runs once every control of this // state has converged. return gpuv1.Ready, nil diff --git a/controllers/object_controls_test.go b/controllers/object_controls_test.go index 8eb4c77410..92463f292c 100644 --- a/controllers/object_controls_test.go +++ b/controllers/object_controls_test.go @@ -2547,6 +2547,7 @@ func TestDCGMExporterServiceAccountReconcile(t *testing.T) { testNamespace = "test-namespace" byoName = "byo-metrics-identity" customName = "metrics-identity" + driverName = "nvidia-driver" ) testScheme := runtime.NewScheme() @@ -2561,7 +2562,19 @@ func TestDCGMExporterServiceAccountReconcile(t *testing.T) { return &corev1.ServiceAccount{ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: testNamespace}} } + // ownedServiceAccount mirrors what ServiceAccount() creates for the exporter: the + // component label and the ClusterPolicy controller reference. ownedServiceAccount := func(t *testing.T, cp *gpuv1.ClusterPolicy, name string) *corev1.ServiceAccount { + t.Helper() + sa := serviceAccount(name) + markDCGMExporterServiceAccount(sa) + require.NoError(t, controllerutil.SetControllerReference(cp, sa, testScheme)) + return sa + } + + // otherComponentServiceAccount mirrors the ServiceAccount of another operand, say the + // driver's: the ClusterPolicy controls it too, but it carries no exporter label. + otherComponentServiceAccount := func(t *testing.T, cp *gpuv1.ClusterPolicy, name string) *corev1.ServiceAccount { t.Helper() sa := serviceAccount(name) require.NoError(t, controllerutil.SetControllerReference(cp, sa, testScheme)) @@ -2583,12 +2596,13 @@ func TestDCGMExporterServiceAccountReconcile(t *testing.T) { serviceAccount *gpuv1.DCGMExporterServiceAccountConfig exporterState *bool stateName string - // existing seeds the fake client; ownedExisting are seeded with a ClusterPolicy - // controller reference. - existing []string - ownedExisting []string - expectedState gpuv1.State - expectedError bool + // existing seeds the fake client as a user would; ownedExisting as the operator + // creates them for the exporter; otherComponentExisting as another operand's. + existing []string + ownedExisting []string + otherComponentExisting []string + expectedState gpuv1.State + expectedError bool // assert runs after ServiceAccount(); present reports whether each name exists. assert func(t *testing.T, k8s client.Client, cp *gpuv1.ClusterPolicy) }{ @@ -2598,14 +2612,17 @@ func TestDCGMExporterServiceAccountReconcile(t *testing.T) { sa, ok := getServiceAccount(t, k8s, DCGMExporterDefaultServiceAccountName) require.True(t, ok) require.True(t, metav1.IsControlledBy(sa, cp)) + require.True(t, isDCGMExporterServiceAccount(sa), + "the label is how the operator finds this ServiceAccount again after a rename") }, }, "configured name creates that ServiceAccount instead of the default": { serviceAccount: &gpuv1.DCGMExporterServiceAccountConfig{Name: customName}, expectedState: gpuv1.Ready, assert: func(t *testing.T, k8s client.Client, cp *gpuv1.ClusterPolicy) { - _, ok := getServiceAccount(t, k8s, customName) + sa, ok := getServiceAccount(t, k8s, customName) require.True(t, ok) + require.True(t, isDCGMExporterServiceAccount(sa)) _, ok = getServiceAccount(t, k8s, DCGMExporterDefaultServiceAccountName) require.False(t, ok, "the default ServiceAccount must not be created as well") }, @@ -2653,6 +2670,19 @@ func TestDCGMExporterServiceAccountReconcile(t *testing.T) { "the owner reference has to go, otherwise the user's ServiceAccount is garbage-collected with the ClusterPolicy") }, }, + "create=false leaves another component's ServiceAccount to that component": { + // The ClusterPolicy controls the driver's ServiceAccount too. Referencing it is + // the user's call, but stripping its owner reference is not the exporter's. + serviceAccount: &gpuv1.DCGMExporterServiceAccountConfig{Name: driverName, Create: new(false)}, + otherComponentExisting: []string{driverName}, + expectedState: gpuv1.Ready, + assert: func(t *testing.T, k8s client.Client, cp *gpuv1.ClusterPolicy) { + sa, ok := getServiceAccount(t, k8s, driverName) + require.True(t, ok) + require.True(t, metav1.IsControlledBy(sa, cp), + "another component's ServiceAccount must keep its controller reference") + }, + }, "disabling the exporter releases a ServiceAccount the operator used to own": { // The combined transition: the user takes the ServiceAccount over with // create=false and disables the exporter in one update. Returning Disabled @@ -2708,6 +2738,43 @@ func TestDCGMExporterServiceAccountReconcile(t *testing.T) { require.False(t, ok) }, }, + "disabling the exporter after a rename deletes every ServiceAccount it created": { + // One update both turns the exporter off and picks a new name. Targeting + // the configured name alone would leave the previous default behind. + serviceAccount: &gpuv1.DCGMExporterServiceAccountConfig{Name: customName}, + exporterState: new(false), + ownedExisting: []string{DCGMExporterDefaultServiceAccountName, customName}, + expectedState: gpuv1.Disabled, + assert: func(t *testing.T, k8s client.Client, cp *gpuv1.ClusterPolicy) { + _, ok := getServiceAccount(t, k8s, DCGMExporterDefaultServiceAccountName) + require.False(t, ok, "the previous default must not outlive the exporter") + _, ok = getServiceAccount(t, k8s, customName) + require.False(t, ok) + }, + }, + "disabling the exporter while handing over deletes the owned default and releases the user's ServiceAccount": { + serviceAccount: &gpuv1.DCGMExporterServiceAccountConfig{Name: byoName, Create: new(false)}, + exporterState: new(false), + ownedExisting: []string{DCGMExporterDefaultServiceAccountName, byoName}, + expectedState: gpuv1.Disabled, + assert: func(t *testing.T, k8s client.Client, cp *gpuv1.ClusterPolicy) { + _, ok := getServiceAccount(t, k8s, DCGMExporterDefaultServiceAccountName) + require.False(t, ok, "the previous default must not outlive the exporter") + sa, ok := getServiceAccount(t, k8s, byoName) + require.True(t, ok, "a user-provided ServiceAccount must never be deleted") + require.False(t, metav1.IsControlledBy(sa, cp)) + }, + }, + "disabling the exporter leaves another component's ServiceAccount alone": { + exporterState: new(false), + otherComponentExisting: []string{driverName}, + expectedState: gpuv1.Disabled, + assert: func(t *testing.T, k8s client.Client, cp *gpuv1.ClusterPolicy) { + sa, ok := getServiceAccount(t, k8s, driverName) + require.True(t, ok) + require.True(t, metav1.IsControlledBy(sa, cp)) + }, + }, "a non-DCGM state deletes its ServiceAccount regardless of ownership": { // The ownership check is scoped to the DCGM Exporter; every other state keeps // the previous unconditional cleanup on disable. @@ -2727,13 +2794,16 @@ func TestDCGMExporterServiceAccountReconcile(t *testing.T) { cp.Spec.DCGMExporter.ServiceAccount = tc.serviceAccount cp.Spec.DCGMExporter.Enabled = tc.exporterState - objects := make([]client.Object, 0, len(tc.existing)+len(tc.ownedExisting)) + objects := make([]client.Object, 0, len(tc.existing)+len(tc.ownedExisting)+len(tc.otherComponentExisting)) for _, saName := range tc.existing { objects = append(objects, serviceAccount(saName)) } for _, saName := range tc.ownedExisting { objects = append(objects, ownedServiceAccount(t, cp, saName)) } + for _, saName := range tc.otherComponentExisting { + objects = append(objects, otherComponentServiceAccount(t, cp, saName)) + } k8s := fake.NewClientBuilder().WithScheme(testScheme).WithObjects(objects...).Build() stateName := tc.stateName @@ -2772,13 +2842,17 @@ func TestDCGMExporterServiceAccountReconcile(t *testing.T) { } } -// TestDCGMExporterSupersededServiceAccountCleanup covers the deferred reclaim of the -// operator default. It runs only after every control of the state converged, so the -// RoleBindings, SCC and DaemonSet already reference the replacement by then. +// TestDCGMExporterSupersededServiceAccountCleanup covers the deferred reclaim that runs once +// every control of the state converged, when the RoleBindings, SCC and DaemonSet already +// reference the configured ServiceAccount. Candidates are found by the exporter label +// rather than by name, so a rename from one custom name to another -- or back to the +// default -- leaves nothing behind either. func TestDCGMExporterSupersededServiceAccountCleanup(t *testing.T) { const ( testNamespace = "test-namespace" - customName = "metrics-identity" + customA = "metrics-a" + customB = "metrics-b" + driverName = "nvidia-driver" ) testScheme := runtime.NewScheme() @@ -2789,46 +2863,73 @@ func TestDCGMExporterSupersededServiceAccountCleanup(t *testing.T) { serviceAccount *gpuv1.DCGMExporterServiceAccountConfig exporterState *bool stateName string - defaultOwned bool - expectDeleted bool + // owned are seeded as the operator creates them for the exporter (label and + // controller reference), unowned as a user would, otherComponent as another + // operand's (controller reference, no exporter label). + owned []string + unowned []string + otherComponent []string + expectDeleted []string + expectKept []string }{ + "the default name supersedes nothing": { + owned: []string{DCGMExporterDefaultServiceAccountName}, + expectKept: []string{DCGMExporterDefaultServiceAccountName}, + }, "renaming reclaims the superseded operator-owned default": { - serviceAccount: &gpuv1.DCGMExporterServiceAccountConfig{Name: customName}, - defaultOwned: true, - expectDeleted: true, + serviceAccount: &gpuv1.DCGMExporterServiceAccountConfig{Name: customA}, + owned: []string{DCGMExporterDefaultServiceAccountName, customA}, + expectDeleted: []string{DCGMExporterDefaultServiceAccountName}, + expectKept: []string{customA}, }, "handing over to a user-provided ServiceAccount reclaims the owned default": { - serviceAccount: &gpuv1.DCGMExporterServiceAccountConfig{Name: customName, Create: new(false)}, - defaultOwned: true, - expectDeleted: true, + serviceAccount: &gpuv1.DCGMExporterServiceAccountConfig{Name: customA, Create: new(false)}, + owned: []string{DCGMExporterDefaultServiceAccountName}, + unowned: []string{customA}, + expectDeleted: []string{DCGMExporterDefaultServiceAccountName}, + expectKept: []string{customA}, }, "a previous default the operator does not own is left alone": { - serviceAccount: &gpuv1.DCGMExporterServiceAccountConfig{Name: customName}, - defaultOwned: false, - expectDeleted: false, - }, - "the default name supersedes nothing": { - defaultOwned: true, - expectDeleted: false, + serviceAccount: &gpuv1.DCGMExporterServiceAccountConfig{Name: customA}, + unowned: []string{DCGMExporterDefaultServiceAccountName}, + expectKept: []string{DCGMExporterDefaultServiceAccountName}, }, "bringing the default name keeps that ServiceAccount": { serviceAccount: &gpuv1.DCGMExporterServiceAccountConfig{ Name: DCGMExporterDefaultServiceAccountName, Create: new(false), }, - defaultOwned: true, - expectDeleted: false, + owned: []string{DCGMExporterDefaultServiceAccountName}, + expectKept: []string{DCGMExporterDefaultServiceAccountName}, + }, + "renaming from one custom name to another reclaims the previous one": { + serviceAccount: &gpuv1.DCGMExporterServiceAccountConfig{Name: customB}, + owned: []string{customA, customB}, + expectDeleted: []string{customA}, + expectKept: []string{customB}, + }, + "switching back to the default reclaims every custom ServiceAccount left behind": { + owned: []string{customA, customB, DCGMExporterDefaultServiceAccountName}, + expectDeleted: []string{customA, customB}, + expectKept: []string{DCGMExporterDefaultServiceAccountName}, + }, + "another component's ServiceAccount is not the exporter's to reclaim": { + serviceAccount: &gpuv1.DCGMExporterServiceAccountConfig{Name: customA}, + owned: []string{customA}, + otherComponent: []string{driverName}, + expectKept: []string{customA, driverName}, }, "a disabled exporter reclaims nothing here": { - serviceAccount: &gpuv1.DCGMExporterServiceAccountConfig{Name: customName}, + // ServiceAccount() sweeps the component's ServiceAccounts on the disabled path. + serviceAccount: &gpuv1.DCGMExporterServiceAccountConfig{Name: customA}, exporterState: new(false), - defaultOwned: true, - expectDeleted: false, + owned: []string{DCGMExporterDefaultServiceAccountName, customA}, + expectKept: []string{DCGMExporterDefaultServiceAccountName, customA}, }, "another state reclaims nothing": { - serviceAccount: &gpuv1.DCGMExporterServiceAccountConfig{Name: customName}, + serviceAccount: &gpuv1.DCGMExporterServiceAccountConfig{Name: customA}, stateName: "state-driver", - defaultOwned: true, - expectDeleted: false, + owned: []string{DCGMExporterDefaultServiceAccountName}, + expectKept: []string{DCGMExporterDefaultServiceAccountName}, }, } @@ -2838,14 +2939,25 @@ func TestDCGMExporterSupersededServiceAccountCleanup(t *testing.T) { cp.Spec.DCGMExporter.ServiceAccount = tc.serviceAccount cp.Spec.DCGMExporter.Enabled = tc.exporterState - previous := &corev1.ServiceAccount{ - ObjectMeta: metav1.ObjectMeta{Name: DCGMExporterDefaultServiceAccountName, Namespace: testNamespace}, + var objects []client.Object + for _, saName := range tc.owned { + sa := &corev1.ServiceAccount{ObjectMeta: metav1.ObjectMeta{Name: saName, Namespace: testNamespace}} + markDCGMExporterServiceAccount(sa) + require.NoError(t, controllerutil.SetControllerReference(cp, sa, testScheme)) + objects = append(objects, sa) + } + for _, saName := range tc.unowned { + objects = append(objects, &corev1.ServiceAccount{ + ObjectMeta: metav1.ObjectMeta{Name: saName, Namespace: testNamespace}, + }) } - if tc.defaultOwned { - require.NoError(t, controllerutil.SetControllerReference(cp, previous, testScheme)) + for _, saName := range tc.otherComponent { + sa := &corev1.ServiceAccount{ObjectMeta: metav1.ObjectMeta{Name: saName, Namespace: testNamespace}} + require.NoError(t, controllerutil.SetControllerReference(cp, sa, testScheme)) + objects = append(objects, sa) } - k8s := fake.NewClientBuilder().WithScheme(testScheme).WithObjects(previous).Build() + k8s := fake.NewClientBuilder().WithScheme(testScheme).WithObjects(objects...).Build() stateName := tc.stateName if stateName == "" { stateName = "state-dcgm-exporter" @@ -2864,13 +2976,13 @@ func TestDCGMExporterSupersededServiceAccountCleanup(t *testing.T) { require.NoError(t, n.cleanupSupersededDCGMExporterServiceAccount(context.Background())) - found := &corev1.ServiceAccount{} - err := k8s.Get(context.Background(), - types.NamespacedName{Namespace: testNamespace, Name: DCGMExporterDefaultServiceAccountName}, found) - if tc.expectDeleted { - require.True(t, apierrors.IsNotFound(err), "the superseded default must be reclaimed") - } else { - require.NoError(t, err, "this ServiceAccount must not be reclaimed") + for _, saName := range tc.expectDeleted { + err := k8s.Get(context.Background(), types.NamespacedName{Namespace: testNamespace, Name: saName}, &corev1.ServiceAccount{}) + require.True(t, apierrors.IsNotFound(err), "%q must be reclaimed", saName) + } + for _, saName := range tc.expectKept { + err := k8s.Get(context.Background(), types.NamespacedName{Namespace: testNamespace, Name: saName}, &corev1.ServiceAccount{}) + require.NoError(t, err, "%q must not be reclaimed", saName) } }) } @@ -3025,9 +3137,12 @@ func TestDCGMExporterCleanupSurvivesDisabledControls(t *testing.T) { cp := &gpuv1.ClusterPolicy{ObjectMeta: metav1.ObjectMeta{Name: "cluster-policy", UID: "cp-uid"}} cp.Spec.DCGMExporter.ServiceAccount = &gpuv1.DCGMExporterServiceAccountConfig{Name: customName} + // Seeded as ServiceAccount() creates it: the exporter label is what the + // reclaim finds it by. superseded := &corev1.ServiceAccount{ ObjectMeta: metav1.ObjectMeta{Name: DCGMExporterDefaultServiceAccountName, Namespace: testNamespace}, } + markDCGMExporterServiceAccount(superseded) require.NoError(t, controllerutil.SetControllerReference(cp, superseded, testScheme)) k8s := fake.NewClientBuilder().WithScheme(testScheme).WithObjects(superseded).Build() diff --git a/internal/state/configurable_state.go b/internal/state/configurable_state.go index 434d89e50b..33c9588ff0 100644 --- a/internal/state/configurable_state.go +++ b/internal/state/configurable_state.go @@ -53,7 +53,9 @@ type configurableState struct { // preSync runs after the manifests render but before they are applied, for operands // that depend on cluster state the templates cannot express. Returning an error marks // the state NotReady, so it is the place to surface a misconfiguration instead of - // applying objects that cannot converge. + // applying objects that cannot converge. It is also where an object the user took over + // is handed back: the hand-off must not wait on the operands converging, or a + // DaemonSet that never becomes Ready would hold the owner reference in place. preSync func(ctx context.Context, s *configurableState, cr *nvidiav1alpha1.GPUCluster) error // postSync runs after the manifests converged. Reclaiming objects a previous diff --git a/internal/state/dcgm_exporter.go b/internal/state/dcgm_exporter.go index 715117c99e..78ec143057 100644 --- a/internal/state/dcgm_exporter.go +++ b/internal/state/dcgm_exporter.go @@ -79,7 +79,7 @@ func NewStateDCGMExporter( imageEnvName: dcgmExporterImageEnvName, buildRenderData: buildDCGMExporterRenderData, preSync: checkDCGMExporterServiceAccount, - postSync: reconcileDCGMExporterServiceAccountOwnership, + postSync: reclaimSupersededDCGMExporterServiceAccounts, preDelete: releaseDCGMExporterServiceAccountOnDelete, }, nil } @@ -159,10 +159,10 @@ func buildDCGMExporterRenderData(ctx context.Context, s *configurableState, cr * }, nil } -// checkDCGMExporterServiceAccount reconciles the parts of the ServiceAccount contract the -// manifests cannot express: a ServiceAccount the user brings has to already exist, one the -// operator would manage must not be an existing object owned by somebody else, and the -// operator-owned default is removed once a different name takes over. +// checkDCGMExporterServiceAccount runs before the manifests are applied and reconciles the +// parts of the ServiceAccount contract the templates cannot express: a ServiceAccount the +// user brings has to already exist and is handed back if the operator used to manage it, +// and one the operator would manage must not be an existing object owned by somebody else. func checkDCGMExporterServiceAccount(ctx context.Context, s *configurableState, cr *nvidiav1alpha1.GPUCluster) error { spec := cr.Spec.DCGMExporter name := spec.GetServiceAccountName(dcgmExporterDefaultServiceAccountName) @@ -170,7 +170,8 @@ func checkDCGMExporterServiceAccount(ctx context.Context, s *configurableState, if !spec.IsServiceAccountCreateEnabled() { // The manifests omit the ServiceAccount entirely, so a missing one would leave // the DaemonSet pending without any signal. - if _, err := s.getServiceAccount(ctx, name); err != nil { + sa, err := s.getServiceAccount(ctx, name) + if err != nil { if apierrors.IsNotFound(err) { return fmt.Errorf( "ServiceAccount %q configured with create=false does not exist in namespace %q", @@ -178,7 +179,12 @@ func checkDCGMExporterServiceAccount(ctx context.Context, s *configurableState, } return err } - return nil + // The same object may have been operator-managed before create was set to false. + // It is handed back here, before the operands sync, rather than once they + // converged: a DaemonSet that never becomes Ready would otherwise keep this CR's + // controller reference on the ServiceAccount indefinitely, and deleting the + // GPUCluster would garbage-collect an object the user now owns. + return s.releaseServiceAccount(ctx, cr, sa) } if name == dcgmExporterDefaultServiceAccountName { @@ -230,33 +236,14 @@ func guardDCGMExporterServiceAccountAdoption(owner metav1.Object, current *unstr return dcgmExporterServiceAccountTakeoverError(current.GetName(), current.GetNamespace()) } -// reconcileDCGMExporterServiceAccountOwnership runs once the manifests converged. It -// reclaims the operator default a different ServiceAccount superseded, and releases -// ownership of a ServiceAccount the user took over with create=false. -func reconcileDCGMExporterServiceAccountOwnership(ctx context.Context, s *configurableState, cr *nvidiav1alpha1.GPUCluster) error { - spec := cr.Spec.DCGMExporter - name := spec.GetServiceAccountName(dcgmExporterDefaultServiceAccountName) - - if !spec.IsServiceAccountCreateEnabled() { - // The same object may have been operator-managed before create was set to false. - // Both the controller reference and the state label have to go, otherwise it is - // garbage-collected with the GPUCluster or swept by the state cleanup. - sa, err := s.getServiceAccount(ctx, name) - if err != nil { - if apierrors.IsNotFound(err) { - return nil - } - return err - } - if err := s.releaseServiceAccount(ctx, cr, sa); err != nil { - return err - } - } - - if name == dcgmExporterDefaultServiceAccountName { - return nil - } - return s.deleteOwnedServiceAccount(ctx, cr, dcgmExporterDefaultServiceAccountName) +// reclaimSupersededDCGMExporterServiceAccounts runs once the manifests converged and removes +// the ServiceAccounts this state created under a previous configuration. Waiting for +// convergence matters here, unlike for the hand-back in checkDCGMExporterServiceAccount: +// deleting a ServiceAccount the DaemonSet still referenced would leave its pods without an +// identity, whereas releasing ownership changes nothing the operands can observe. +func reclaimSupersededDCGMExporterServiceAccounts(ctx context.Context, s *configurableState, cr *nvidiav1alpha1.GPUCluster) error { + return s.deleteSupersededServiceAccounts(ctx, cr, + cr.Spec.DCGMExporter.GetServiceAccountName(dcgmExporterDefaultServiceAccountName)) } // releaseDCGMExporterServiceAccountOnDelete hands a user-provided ServiceAccount back @@ -281,10 +268,15 @@ func releaseDCGMExporterServiceAccountOnDelete(ctx context.Context, s *configura return s.releaseServiceAccount(ctx, cr, sa) } -// releaseServiceAccount drops this GPUCluster's controller reference and the state label -// from a ServiceAccount the user now owns. +// releaseServiceAccount hands a ServiceAccount the user now owns back to them by dropping +// this GPUCluster's controller reference and the state label. Only a ServiceAccount this +// state managed is touched: every operand of the GPUCluster carries its controller +// reference, so checking ownership alone would also strip the reference and label off +// another state's ServiceAccount when the user points create=false at it. func (s *configurableState) releaseServiceAccount(ctx context.Context, cr *nvidiav1alpha1.GPUCluster, sa *corev1.ServiceAccount) error { - changed := false + if sa.Labels[consts.StateLabel] != s.name { + return nil + } if metav1.IsControlledBy(sa, cr) { refs := make([]metav1.OwnerReference, 0, len(sa.OwnerReferences)) for _, ref := range sa.OwnerReferences { @@ -294,15 +286,8 @@ func (s *configurableState) releaseServiceAccount(ctx context.Context, cr *nvidi refs = append(refs, ref) } sa.OwnerReferences = refs - changed = true - } - if _, ok := sa.Labels[consts.StateLabel]; ok { - delete(sa.Labels, consts.StateLabel) - changed = true - } - if !changed { - return nil } + delete(sa.Labels, consts.StateLabel) log.FromContext(ctx).V(consts.LogLevelInfo).Info( "Releasing ownership of a user-provided dcgm-exporter ServiceAccount", "Name", sa.Name) return s.client.Update(ctx, sa) @@ -315,25 +300,29 @@ func (s *configurableState) getServiceAccount(ctx context.Context, name string) return sa, err } -// deleteOwnedServiceAccount removes a ServiceAccount left behind by a previous -// configuration, but only when this CR owns it: an object the user provisioned under the -// same name is left alone. Renaming from one custom name to another is not tracked, so -// only the operator default is reclaimed here. -func (s *configurableState) deleteOwnedServiceAccount(ctx context.Context, cr *nvidiav1alpha1.GPUCluster, name string) error { - sa, err := s.getServiceAccount(ctx, name) - if err != nil { - if apierrors.IsNotFound(err) { - return nil - } +// deleteSupersededServiceAccounts removes every ServiceAccount carrying this state's label +// that this CR controls, except the one named keep. Finding them by label rather than by +// name is what covers a rename from one custom name to another, or back to the default: no +// record of the previous configuration exists, but every ServiceAccount the state ever +// created still carries its label. A ServiceAccount the user provisioned under one of those +// names carries no controller reference from this CR and is left alone. +func (s *configurableState) deleteSupersededServiceAccounts(ctx context.Context, cr *nvidiav1alpha1.GPUCluster, keep string) error { + list := &corev1.ServiceAccountList{} + if err := s.client.List(ctx, list, + client.InNamespace(s.namespace), + client.MatchingLabels{consts.StateLabel: s.name}); err != nil { return err } - if !metav1.IsControlledBy(sa, cr) { - return nil - } - log.FromContext(ctx).V(consts.LogLevelInfo).Info( - "Removing the superseded dcgm-exporter ServiceAccount", "Name", name) - if err := s.client.Delete(ctx, sa); err != nil && !apierrors.IsNotFound(err) { - return err + for i := range list.Items { + sa := &list.Items[i] + if sa.Name == keep || !metav1.IsControlledBy(sa, cr) { + continue + } + 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) { + return err + } } return nil } diff --git a/internal/state/dcgm_exporter_test.go b/internal/state/dcgm_exporter_test.go index e440585da0..0b1accd0b1 100644 --- a/internal/state/dcgm_exporter_test.go +++ b/internal/state/dcgm_exporter_test.go @@ -22,7 +22,9 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" + rbacv1 "k8s.io/api/rbac/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -62,27 +64,32 @@ func newTestDCGMExporterState(t *testing.T, serviceMonitorCRD bool) *configurabl return s.(*configurableState) } -// exporterCR returns a sample CR with dcgm-exporter enabled and the given exporter spec. // newTestDCGMExporterStateWithObjects builds the state with a client that already holds -// the given objects, for the ServiceAccount checks that read cluster state. +// the given objects, for the ServiceAccount checks that read cluster state. The DaemonSet +// status is a subresource so a pre-seeded status survives the sync's update -- that is how +// a test keeps the operand short of Ready. func newTestDCGMExporterStateWithObjects(t *testing.T, objs ...client.Object) *configurableState { t.Helper() t.Setenv("DCGM_EXPORTER_IMAGE", "nvcr.io/nvidia/k8s/dcgm-exporter:test") testScheme := runtime.NewScheme() require.NoError(t, corev1.AddToScheme(testScheme)) + require.NoError(t, appsv1.AddToScheme(testScheme)) + require.NoError(t, rbacv1.AddToScheme(testScheme)) require.NoError(t, nvidiav1alpha1.AddToScheme(testScheme)) k8sClient := fake.NewClientBuilder(). WithScheme(testScheme). WithRESTMapper(restMapperWithServiceMonitor(false)). WithObjects(objs...). + WithStatusSubresource(&appsv1.DaemonSet{}). Build() s, err := NewStateDCGMExporter(k8sClient, "test-operator", testScheme, dcgmExporterManifestDir) require.NoError(t, err) return s.(*configurableState) } +// exporterCR returns a sample CR with dcgm-exporter enabled and the given exporter spec. func exporterCR(spec *nvidiav1.DCGMExporterSpec) *nvidiav1alpha1.GPUCluster { cr := sampleGPUCluster() cr.Spec.DCGMExporter = spec @@ -377,15 +384,49 @@ func unownedServiceAccount(name string) *corev1.ServiceAccount { } } -// TestDCGMExporterServiceAccountValidation covers the preSync hook, which only rejects a -// configuration the manifests cannot express. It never mutates cluster state -- reclaiming -// what a previous configuration left behind happens in the postSync hook, once the -// operands stopped referencing it. -func TestDCGMExporterServiceAccountValidation(t *testing.T) { +// otherStateServiceAccount returns a ServiceAccount another state of the same GPUCluster +// manages: it carries the CR's controller reference like every operand object does, but +// not this state's label. +func otherStateServiceAccount(cr *nvidiav1alpha1.GPUCluster, name string) *corev1.ServiceAccount { + sa := ownedServiceAccount(cr, name) + sa.Labels[consts.StateLabel] = "state-driver" + return sa +} + +// requireReleased asserts the ServiceAccount survived with neither this CR's controller +// reference nor the state label -- the two things that would otherwise hand a user-owned +// object to garbage collection or to the state cleanup. +func requireReleased(t *testing.T, ctx context.Context, s *configurableState, cr *nvidiav1alpha1.GPUCluster, name string) { + t.Helper() + sa, err := s.getServiceAccount(ctx, name) + require.NoError(t, err, "a user-provided ServiceAccount must never be deleted") + assert.False(t, metav1.IsControlledBy(sa, cr), + "the owner reference has to go, otherwise the user's ServiceAccount is garbage-collected with the GPUCluster") + assert.NotContains(t, sa.Labels, consts.StateLabel, + "the state label has to go, otherwise the state cleanup sweeps the user's ServiceAccount") +} + +// requireStillManaged asserts the ServiceAccount kept its controller reference and its +// state label, whichever state's that is. +func requireStillManaged(t *testing.T, ctx context.Context, s *configurableState, cr *nvidiav1alpha1.GPUCluster, name string) { + t.Helper() + sa, err := s.getServiceAccount(ctx, name) + require.NoError(t, err) + assert.True(t, metav1.IsControlledBy(sa, cr), "%q must keep its controller reference", name) + assert.Contains(t, sa.Labels, consts.StateLabel, "%q must keep its state label", name) +} + +// TestDCGMExporterServiceAccountPreSync covers the preSync hook. It rejects a configuration +// the manifests cannot express, and hands a ServiceAccount the user took over with +// create=false back to them right away -- before the operands sync, so the hand-off never +// waits on a DaemonSet becoming Ready. It deletes nothing: reclaiming what a previous +// configuration left behind happens in postSync, once the operands stopped referencing it. +func TestDCGMExporterServiceAccountPreSync(t *testing.T) { ctx := context.Background() const ( customName = "metrics-identity" byoName = "byo-sa" + driverName = "nvidia-driver" ) testCases := map[string]struct { @@ -393,6 +434,10 @@ func TestDCGMExporterServiceAccountValidation(t *testing.T) { existing func(cr *nvidiav1alpha1.GPUCluster) []client.Object // expectedError is a substring of the error the hook must return; empty accepts. expectedError string + // released must survive without this CR's controller reference or the state + // label; stillManaged must keep both. + released []string + stillManaged []string }{ "the default configuration needs nothing to exist": {}, "create=false requires the ServiceAccount to exist": { @@ -405,6 +450,31 @@ func TestDCGMExporterServiceAccountValidation(t *testing.T) { return []client.Object{unownedServiceAccount(byoName)} }, }, + "create=false releases the ServiceAccount the operator used to own": { + serviceAccount: &nvidiav1.DCGMExporterServiceAccountConfig{Name: byoName, Create: new(false)}, + existing: func(cr *nvidiav1alpha1.GPUCluster) []client.Object { + return []client.Object{ownedServiceAccount(cr, byoName)} + }, + released: []string{byoName}, + }, + "create=false releases the operator default the user took over": { + serviceAccount: &nvidiav1.DCGMExporterServiceAccountConfig{ + Name: dcgmExporterDefaultServiceAccountName, Create: new(false), + }, + existing: func(cr *nvidiav1alpha1.GPUCluster) []client.Object { + return []client.Object{ownedServiceAccount(cr, dcgmExporterDefaultServiceAccountName)} + }, + released: []string{dcgmExporterDefaultServiceAccountName}, + }, + "create=false leaves another state's ServiceAccount to that state": { + // The GPUCluster controls the driver's ServiceAccount too. Referencing it is + // the user's call, but stripping its owner reference and label is not ours. + serviceAccount: &nvidiav1.DCGMExporterServiceAccountConfig{Name: driverName, Create: new(false)}, + existing: func(cr *nvidiav1alpha1.GPUCluster) []client.Object { + return []client.Object{otherStateServiceAccount(cr, driverName)} + }, + stillManaged: []string{driverName}, + }, "a configured name refuses to take over an unowned ServiceAccount": { serviceAccount: &nvidiav1.DCGMExporterServiceAccountConfig{Name: customName}, existing: func(*nvidiav1alpha1.GPUCluster) []client.Object { @@ -417,12 +487,14 @@ func TestDCGMExporterServiceAccountValidation(t *testing.T) { existing: func(cr *nvidiav1alpha1.GPUCluster) []client.Object { return []client.Object{ownedServiceAccount(cr, customName)} }, + stillManaged: []string{customName}, }, "renaming leaves the superseded default for the postSync reclaim": { serviceAccount: &nvidiav1.DCGMExporterServiceAccountConfig{Name: customName}, existing: func(cr *nvidiav1alpha1.GPUCluster) []client.Object { return []client.Object{ownedServiceAccount(cr, dcgmExporterDefaultServiceAccountName)} }, + stillManaged: []string{dcgmExporterDefaultServiceAccountName}, }, } @@ -442,33 +514,42 @@ func TestDCGMExporterServiceAccountValidation(t *testing.T) { } require.NoError(t, err) - // Validation only: whatever was there stays there. + // Nothing is deleted on this path, whatever the outcome. for _, obj := range objs { _, getErr := s.getServiceAccount(ctx, obj.GetName()) require.NoError(t, getErr, "the preSync hook must not remove %q", obj.GetName()) } + for _, saName := range tc.released { + requireReleased(t, ctx, s, cr, saName) + } + for _, saName := range tc.stillManaged { + requireStillManaged(t, ctx, s, cr, saName) + } }) } } -// TestDCGMExporterServiceAccountOwnershipReconcile covers the postSync hook: it runs after -// the manifests converged, so the operands already reference the new ServiceAccount and -// the superseded one can be reclaimed. -func TestDCGMExporterServiceAccountOwnershipReconcile(t *testing.T) { +// TestDCGMExporterSupersededServiceAccountReclaim covers the postSync hook. It runs after +// the manifests converged, so the operands already reference the configured ServiceAccount +// and whatever this state created under a previous configuration can go. The candidates +// are found by the state label rather than by name -- that is what covers a rename from one +// custom name to another, or back to the default. +func TestDCGMExporterSupersededServiceAccountReclaim(t *testing.T) { ctx := context.Background() const ( - customName = "metrics-identity" + customA = "metrics-a" + customB = "metrics-b" byoName = "byo-sa" + driverName = "nvidia-driver" ) testCases := map[string]struct { serviceAccount *nvidiav1.DCGMExporterServiceAccountConfig existing func(cr *nvidiav1alpha1.GPUCluster) []client.Object - // deleted names the ServiceAccounts that must be gone afterwards, kept those that - // must survive, and released those that must survive without operator ownership. - deleted []string - kept []string - released []string + // deleted names the ServiceAccounts that must be gone afterwards, kept those + // that must survive. + deleted []string + kept []string }{ "the default configuration reclaims nothing": { existing: func(cr *nvidiav1alpha1.GPUCluster) []client.Object { @@ -477,23 +558,42 @@ func TestDCGMExporterServiceAccountOwnershipReconcile(t *testing.T) { kept: []string{dcgmExporterDefaultServiceAccountName}, }, "renaming reclaims the superseded operator-owned default": { - serviceAccount: &nvidiav1.DCGMExporterServiceAccountConfig{Name: customName}, + serviceAccount: &nvidiav1.DCGMExporterServiceAccountConfig{Name: customA}, existing: func(cr *nvidiav1alpha1.GPUCluster) []client.Object { return []client.Object{ ownedServiceAccount(cr, dcgmExporterDefaultServiceAccountName), - ownedServiceAccount(cr, customName), + ownedServiceAccount(cr, customA), } }, deleted: []string{dcgmExporterDefaultServiceAccountName}, - kept: []string{customName}, + kept: []string{customA}, }, "renaming keeps a previous ServiceAccount the operator does not own": { - serviceAccount: &nvidiav1.DCGMExporterServiceAccountConfig{Name: customName}, + serviceAccount: &nvidiav1.DCGMExporterServiceAccountConfig{Name: customA}, existing: func(*nvidiav1alpha1.GPUCluster) []client.Object { return []client.Object{unownedServiceAccount(dcgmExporterDefaultServiceAccountName)} }, kept: []string{dcgmExporterDefaultServiceAccountName}, }, + "renaming from one custom name to another reclaims the previous one": { + serviceAccount: &nvidiav1.DCGMExporterServiceAccountConfig{Name: customB}, + existing: func(cr *nvidiav1alpha1.GPUCluster) []client.Object { + return []client.Object{ownedServiceAccount(cr, customA), ownedServiceAccount(cr, customB)} + }, + deleted: []string{customA}, + kept: []string{customB}, + }, + "switching back to the default reclaims every custom ServiceAccount left behind": { + existing: func(cr *nvidiav1alpha1.GPUCluster) []client.Object { + return []client.Object{ + ownedServiceAccount(cr, customA), + ownedServiceAccount(cr, customB), + ownedServiceAccount(cr, dcgmExporterDefaultServiceAccountName), + } + }, + deleted: []string{customA, customB}, + kept: []string{dcgmExporterDefaultServiceAccountName}, + }, "create=false reclaims the operator-owned default": { serviceAccount: &nvidiav1.DCGMExporterServiceAccountConfig{Name: byoName, Create: new(false)}, existing: func(cr *nvidiav1alpha1.GPUCluster) []client.Object { @@ -505,21 +605,21 @@ func TestDCGMExporterServiceAccountOwnershipReconcile(t *testing.T) { deleted: []string{dcgmExporterDefaultServiceAccountName}, kept: []string{byoName}, }, - "create=false releases a ServiceAccount the operator used to own": { - serviceAccount: &nvidiav1.DCGMExporterServiceAccountConfig{ - Name: dcgmExporterDefaultServiceAccountName, Create: new(false), - }, + "the configured ServiceAccount is never a candidate, whatever it carries": { + // preSync hands it back before this hook runs; this pins the safety net for + // the case where it did not get to. + serviceAccount: &nvidiav1.DCGMExporterServiceAccountConfig{Name: byoName, Create: new(false)}, existing: func(cr *nvidiav1alpha1.GPUCluster) []client.Object { - return []client.Object{ownedServiceAccount(cr, dcgmExporterDefaultServiceAccountName)} + return []client.Object{ownedServiceAccount(cr, byoName)} }, - released: []string{dcgmExporterDefaultServiceAccountName}, + kept: []string{byoName}, }, - "create=false on a ServiceAccount that was never owned changes nothing": { - serviceAccount: &nvidiav1.DCGMExporterServiceAccountConfig{Name: byoName, Create: new(false)}, - existing: func(*nvidiav1alpha1.GPUCluster) []client.Object { - return []client.Object{unownedServiceAccount(byoName)} + "another state's ServiceAccount is not this state's to reclaim": { + serviceAccount: &nvidiav1.DCGMExporterServiceAccountConfig{Name: customA}, + existing: func(cr *nvidiav1alpha1.GPUCluster) []client.Object { + return []client.Object{otherStateServiceAccount(cr, driverName), ownedServiceAccount(cr, customA)} }, - released: []string{byoName}, + kept: []string{driverName, customA}, }, } @@ -528,7 +628,7 @@ func TestDCGMExporterServiceAccountOwnershipReconcile(t *testing.T) { cr := exporterCR(&nvidiav1.DCGMExporterSpec{ServiceAccount: tc.serviceAccount}) s := newTestDCGMExporterStateWithObjects(t, tc.existing(cr)...) - require.NoError(t, reconcileDCGMExporterServiceAccountOwnership(ctx, s, cr)) + require.NoError(t, reclaimSupersededDCGMExporterServiceAccounts(ctx, s, cr)) for _, saName := range tc.deleted { _, err := s.getServiceAccount(ctx, saName) @@ -538,14 +638,6 @@ func TestDCGMExporterServiceAccountOwnershipReconcile(t *testing.T) { _, err := s.getServiceAccount(ctx, saName) require.NoError(t, err, "%q must not be reclaimed", saName) } - for _, saName := range tc.released { - sa, err := s.getServiceAccount(ctx, saName) - require.NoError(t, err, "a user-provided ServiceAccount must never be deleted") - assert.False(t, metav1.IsControlledBy(sa, cr), - "the owner reference has to go, otherwise the user's ServiceAccount is garbage-collected with the GPUCluster") - assert.NotContains(t, sa.Labels, consts.StateLabel, - "the state label has to go, otherwise the state cleanup sweeps the user's ServiceAccount") - } }) } } @@ -666,19 +758,22 @@ func TestDCGMExporterServiceAccountAdoptionGuardWiring(t *testing.T) { // update both hands the ServiceAccount to the user with create=false and disables the // exporter. The generic state cleanup deletes every object carrying the state label, and // the ServiceAccount still carries it from when the operator managed it, so ownership has -// to be released before that cleanup rather than in postSync -- which the disabled path +// to be released before that cleanup rather than in preSync -- which the disabled path // never reaches. func TestDCGMExporterServiceAccountReleasedOnDelete(t *testing.T) { ctx := context.Background() - const byoName = "byo-sa" + const ( + byoName = "byo-sa" + driverName = "nvidia-driver" + ) testCases := map[string]struct { serviceAccount *nvidiav1.DCGMExporterServiceAccountConfig existing func(cr *nvidiav1alpha1.GPUCluster) []client.Object - // released must survive with neither this CR's owner reference nor the state label. - released []string - // untouched must keep whatever the operator put on it, ready to be swept. - untouched []string + // released must survive with neither this CR's owner reference nor the state + // label; stillManaged must keep whatever the operator put on it. + released []string + stillManaged []string }{ "create=false releases the ServiceAccount the operator used to own": { serviceAccount: &nvidiav1.DCGMExporterServiceAccountConfig{Name: byoName, Create: new(false)}, @@ -694,11 +789,18 @@ func TestDCGMExporterServiceAccountReleasedOnDelete(t *testing.T) { }, released: []string{byoName}, }, + "create=false leaves another state's ServiceAccount to that state": { + serviceAccount: &nvidiav1.DCGMExporterServiceAccountConfig{Name: driverName, Create: new(false)}, + existing: func(cr *nvidiav1alpha1.GPUCluster) []client.Object { + return []client.Object{otherStateServiceAccount(cr, driverName)} + }, + stillManaged: []string{driverName}, + }, "a managed ServiceAccount is left for the state cleanup to remove": { existing: func(cr *nvidiav1alpha1.GPUCluster) []client.Object { return []client.Object{ownedServiceAccount(cr, dcgmExporterDefaultServiceAccountName)} }, - untouched: []string{dcgmExporterDefaultServiceAccountName}, + stillManaged: []string{dcgmExporterDefaultServiceAccountName}, }, } @@ -710,18 +812,10 @@ func TestDCGMExporterServiceAccountReleasedOnDelete(t *testing.T) { require.NoError(t, releaseDCGMExporterServiceAccountOnDelete(ctx, s, cr)) for _, saName := range tc.released { - sa, err := s.getServiceAccount(ctx, saName) - require.NoError(t, err, "a user-provided ServiceAccount must never be deleted") - assert.False(t, metav1.IsControlledBy(sa, cr), - "the owner reference has to go, otherwise the user's ServiceAccount is garbage-collected with the GPUCluster") - assert.NotContains(t, sa.Labels, consts.StateLabel, - "the state label has to go, otherwise the state cleanup sweeps the user's ServiceAccount") + requireReleased(t, ctx, s, cr, saName) } - for _, saName := range tc.untouched { - sa, err := s.getServiceAccount(ctx, saName) - require.NoError(t, err) - assert.True(t, metav1.IsControlledBy(sa, cr)) - assert.Contains(t, sa.Labels, consts.StateLabel) + for _, saName := range tc.stillManaged { + requireStillManaged(t, ctx, s, cr, saName) } }) } @@ -742,8 +836,42 @@ func TestDCGMExporterSyncReleasesBeforeDeletion(t *testing.T) { _, err := s.Sync(ctx, cr, draSupportedCatalog()) require.NoError(t, err) - sa, err := s.getServiceAccount(ctx, byoName) - require.NoError(t, err, "the user's ServiceAccount must survive disabling the exporter") - assert.False(t, metav1.IsControlledBy(sa, cr)) - assert.NotContains(t, sa.Labels, consts.StateLabel) + requireReleased(t, ctx, s, cr, byoName) +} + +// TestDCGMExporterSyncReleasesBeforeOperandsConverge drives Sync() with the exporter enabled +// and its DaemonSet stuck short of Ready -- the situation in which a hand-off deferred to +// postSync would never happen. The user's ServiceAccount has to come back regardless, while +// the reclaim of the superseded default still waits for the operands to converge. +func TestDCGMExporterSyncReleasesBeforeOperandsConverge(t *testing.T) { + ctx := context.Background() + const byoName = "byo-sa" + + cr := exporterCR(&nvidiav1.DCGMExporterSpec{ + ServiceAccount: &nvidiav1.DCGMExporterServiceAccountConfig{Name: byoName, Create: new(false)}, + }) + cr.UID = "gpucluster-uid" + + // The DaemonSet controller has processed the object, but none of its pods is available. + stuck := &appsv1.DaemonSet{ + ObjectMeta: metav1.ObjectMeta{Name: "nvidia-dcgm-exporter-dra", Namespace: "test-operator", Generation: 1}, + Status: appsv1.DaemonSetStatus{ + ObservedGeneration: 1, + DesiredNumberScheduled: 1, + CurrentNumberScheduled: 1, + NumberAvailable: 0, + }, + } + s := newTestDCGMExporterStateWithObjects(t, + ownedServiceAccount(cr, byoName), + ownedServiceAccount(cr, dcgmExporterDefaultServiceAccountName), + stuck) + + syncState, err := s.Sync(ctx, cr, draSupportedCatalog()) + require.NoError(t, err) + require.Equal(t, SyncState(SyncStateNotReady), syncState, "the DaemonSet is short of Ready, so the state must not converge") + + requireReleased(t, ctx, s, cr, byoName) + _, err = s.getServiceAccount(ctx, dcgmExporterDefaultServiceAccountName) + require.NoError(t, err, "the superseded default is reclaimed only once the operands converged") } From 815a32575fee809b51ce4265d8c73174bf3c2c6f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=EB=B0=B1=EC=A7=80=EB=AA=85?= Date: Thu, 17 Sep 2026 09:24:38 +0900 Subject: [PATCH 8/9] Identify the exporter's ServiceAccounts by marker, not by ownership alone MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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: 백지명 --- controllers/object_controls.go | 115 +++++++++++--------- controllers/object_controls_test.go | 156 +++++++++++++++++++++++++-- internal/ownership/serviceaccount.go | 102 ++++++++++++++++++ internal/state/dcgm_exporter.go | 53 +++++---- internal/state/dcgm_exporter_test.go | 58 +++++++--- 5 files changed, 388 insertions(+), 96 deletions(-) create mode 100644 internal/ownership/serviceaccount.go diff --git a/controllers/object_controls.go b/controllers/object_controls.go index f93c002270..910c51aecc 100644 --- a/controllers/object_controls.go +++ b/controllers/object_controls.go @@ -52,6 +52,7 @@ import ( gpuv1 "github.com/NVIDIA/gpu-operator/api/nvidia/v1" driverconfig "github.com/NVIDIA/gpu-operator/internal/config" "github.com/NVIDIA/gpu-operator/internal/consts" + "github.com/NVIDIA/gpu-operator/internal/ownership" "github.com/NVIDIA/gpu-operator/internal/utils" ) @@ -336,39 +337,36 @@ var SubscriptionPathMap = map[string](MountPathToVolumeSource){ type controlFunc []func(n ClusterPolicyController) (gpuv1.State, error) -// dcgmExporterServiceAccountLabel is the app label every DCGM Exporter asset carries. The -// operator relies on it to find the ServiceAccounts it created for the exporter under -// whatever name a previous configuration gave them, so ServiceAccount() stamps it on each -// one it creates rather than trusting the asset. -const dcgmExporterServiceAccountLabel = "nvidia-dcgm-exporter" - -// markDCGMExporterServiceAccount labels a ServiceAccount as created for the DCGM Exporter. -func markDCGMExporterServiceAccount(sa *corev1.ServiceAccount) { - if sa.Labels == nil { - sa.Labels = map[string]string{} - } - sa.Labels["app"] = dcgmExporterServiceAccountLabel -} - -// isDCGMExporterServiceAccount reports whether the operator created the ServiceAccount for -// the DCGM Exporter. The ClusterPolicy controls the ServiceAccount of every operand, so -// ownership alone cannot tell the exporter's apart from, say, the driver's. -func isDCGMExporterServiceAccount(sa *corev1.ServiceAccount) bool { - return sa.Labels["app"] == dcgmExporterServiceAccountLabel -} - -// isServiceAccountOwned reports whether the ServiceAccount exists and is controlled -// by the ClusterPolicy being reconciled. A missing ServiceAccount counts as owned so -// that callers fall through to a create that starts from a clean slate. -func (n ClusterPolicyController) isServiceAccountOwned(ctx context.Context, obj *corev1.ServiceAccount) (bool, error) { +// dcgmExporterDaemonSetName is the DCGM Exporter DaemonSet, read to confirm which +// ServiceAccount the deployed operand actually references. +const dcgmExporterDaemonSetName = "nvidia-dcgm-exporter" + +// dcgmExporterServiceAccountMarker identifies the ServiceAccounts the operator created +// for the DCGM Exporter. It reuses the app label the asset has always carried, so +// accounts created by an earlier release are still recognized after an upgrade; +// ServiceAccount() stamps it on each one it creates rather than trusting the asset. +var dcgmExporterServiceAccountMarker = ownership.Marker{ + Key: "app", + Value: "nvidia-dcgm-exporter", +} + +// checkDCGMExporterServiceAccountAvailable reports whether the configured ServiceAccount +// may be created or adopted: it has to be absent, or an account this ClusterPolicy +// created for the exporter. Ownership alone is not enough -- the ClusterPolicy controls +// every operand's ServiceAccount, so a configured name such as nvidia-driver would pass +// and the exporter would end up running with another operand's identity. +func (n ClusterPolicyController) checkDCGMExporterServiceAccountAvailable(ctx context.Context, obj *corev1.ServiceAccount) error { found := &corev1.ServiceAccount{} if err := n.client.Get(ctx, types.NamespacedName{Namespace: obj.Namespace, Name: obj.Name}, found); err != nil { if apierrors.IsNotFound(err) { - return true, nil + return nil } - return false, err + return err + } + if ownership.IsManaged(found, n.singleton, dcgmExporterServiceAccountMarker) { + return nil } - return metav1.IsControlledBy(found, n.singleton), nil + return ownership.ConflictError("ClusterPolicy", obj.Name, obj.Namespace) } // dcgmExporterServiceAccountRenamed reports whether the exporter is configured to use a @@ -383,17 +381,10 @@ func dcgmExporterServiceAccountRenamed(config *gpuv1.ClusterPolicySpec) bool { // manages it. Only a ServiceAccount the exporter created is handed back: pointing // create=false at another operand's ServiceAccount must leave that operand's ownership alone. func (n ClusterPolicyController) releaseDCGMExporterServiceAccount(ctx context.Context, sa *corev1.ServiceAccount, logger logr.Logger) error { - if !isDCGMExporterServiceAccount(sa) || !metav1.IsControlledBy(sa, n.singleton) { + if !ownership.IsManaged(sa, n.singleton, dcgmExporterServiceAccountMarker) { return nil } - refs := make([]metav1.OwnerReference, 0, len(sa.OwnerReferences)) - for _, ref := range sa.OwnerReferences { - if ref.UID == n.singleton.GetUID() { - continue - } - refs = append(refs, ref) - } - sa.OwnerReferences = refs + ownership.ReleaseOwner(sa, n.singleton.GetUID()) logger.V(1).Info("Releasing ownership of a user-provided ServiceAccount", "Name", sa.Name) return n.client.Update(ctx, sa) } @@ -409,7 +400,25 @@ func (n ClusterPolicyController) cleanupSupersededDCGMExporterServiceAccount(ctx return nil } logger := n.logger.WithValues("Namespace", n.operatorNamespace) - return n.deleteOwnedDCGMExporterServiceAccounts(ctx, dcgmExporterServiceAccountName(&n.singleton.Spec), logger) + configured := dcgmExporterServiceAccountName(&n.singleton.Spec) + + // Convergence is not proof that the operands moved to the configured account: + // DaemonSet() reports Ready without touching the live object when no GPU node is + // detected, so a rename would delete an account the deployed DaemonSet still uses. + ds := &appsv1.DaemonSet{} + err := n.client.Get(ctx, + types.NamespacedName{Namespace: n.operatorNamespace, Name: dcgmExporterDaemonSetName}, ds) + switch { + case apierrors.IsNotFound(err): + // Nothing is deployed, so nothing references a ServiceAccount. + case err != nil: + return err + case ds.Spec.Template.Spec.ServiceAccountName != configured: + logger.V(1).Info("DCGM Exporter DaemonSet still references another ServiceAccount, deferring cleanup", + "Deployed", ds.Spec.Template.Spec.ServiceAccountName, "Configured", configured) + return nil + } + return n.deleteOwnedDCGMExporterServiceAccounts(ctx, configured, logger) } // disableDCGMExporterServiceAccount cleans up when the exporter is turned off. It sweeps @@ -452,12 +461,12 @@ func (n ClusterPolicyController) deleteOwnedDCGMExporterServiceAccounts(ctx cont list := &corev1.ServiceAccountList{} if err := n.client.List(ctx, list, client.InNamespace(n.operatorNamespace), - client.MatchingLabels{"app": dcgmExporterServiceAccountLabel}); err != nil { + dcgmExporterServiceAccountMarker.Selector()); err != nil { return err } for i := range list.Items { sa := &list.Items[i] - if sa.Name == keep || !metav1.IsControlledBy(sa, n.singleton) { + if sa.Name == keep || !ownership.IsManaged(sa, n.singleton, dcgmExporterServiceAccountMarker) { continue } logger.V(1).Info("Removing a dcgm-exporter ServiceAccount the operator no longer uses", "Name", sa.Name) @@ -481,7 +490,7 @@ func ServiceAccount(n ClusterPolicyController) (gpuv1.State, error) { isDCGMExporter := n.stateNames[state] == "state-dcgm-exporter" if isDCGMExporter { obj.Name = dcgmExporterServiceAccountName(&n.singleton.Spec) - markDCGMExporterServiceAccount(obj) + dcgmExporterServiceAccountMarker.Apply(obj) } // A ServiceAccount the user brings is only referenced, never managed: the // operator must not create, adopt, mutate or delete it. @@ -521,16 +530,11 @@ func ServiceAccount(n ClusterPolicyController) (gpuv1.State, error) { return gpuv1.Ready, nil } - if isDCGMExporter && dcgmExporterServiceAccountRenamed(&n.singleton.Spec) { - // Only a name the user chose can collide with an unrelated object; the default is - // left tolerant so an upgrade that lost the owner reference keeps converging. - owned, err := n.isServiceAccountOwned(ctx, obj) - if err != nil { - return gpuv1.NotReady, err - } - if !owned { - err := fmt.Errorf("ServiceAccount %q already exists in namespace %q and is not managed by this ClusterPolicy; "+ - "set dcgmExporter.serviceAccount.create to false to reference it", obj.Name, obj.Namespace) + // Only a name the user chose can collide with an unrelated object; the default is + // left tolerant so an upgrade that lost the owner reference keeps converging. + guardTakeover := isDCGMExporter && dcgmExporterServiceAccountRenamed(&n.singleton.Spec) + if guardTakeover { + if err := n.checkDCGMExporterServiceAccountAvailable(ctx, obj); err != nil { logger.Error(err, "Refusing to take over an existing ServiceAccount") return gpuv1.NotReady, err } @@ -545,6 +549,15 @@ func ServiceAccount(n ClusterPolicyController) (gpuv1.State, error) { logger.Info("Couldn't create", "Error", err) return gpuv1.NotReady, err } + // The object appeared between the check above and this create, so ownership has + // 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 { + logger.Error(err, "Refusing to use an existing ServiceAccount") + return gpuv1.NotReady, err + } + } logger.Info("Found Resource, skipping update") } diff --git a/controllers/object_controls_test.go b/controllers/object_controls_test.go index 92463f292c..2a6ce6191f 100644 --- a/controllers/object_controls_test.go +++ b/controllers/object_controls_test.go @@ -47,6 +47,7 @@ import ( ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "sigs.k8s.io/controller-runtime/pkg/log/zap" @@ -2567,7 +2568,7 @@ func TestDCGMExporterServiceAccountReconcile(t *testing.T) { ownedServiceAccount := func(t *testing.T, cp *gpuv1.ClusterPolicy, name string) *corev1.ServiceAccount { t.Helper() sa := serviceAccount(name) - markDCGMExporterServiceAccount(sa) + dcgmExporterServiceAccountMarker.Apply(sa) require.NoError(t, controllerutil.SetControllerReference(cp, sa, testScheme)) return sa } @@ -2612,7 +2613,7 @@ func TestDCGMExporterServiceAccountReconcile(t *testing.T) { sa, ok := getServiceAccount(t, k8s, DCGMExporterDefaultServiceAccountName) require.True(t, ok) require.True(t, metav1.IsControlledBy(sa, cp)) - require.True(t, isDCGMExporterServiceAccount(sa), + require.True(t, dcgmExporterServiceAccountMarker.Matches(sa.Labels), "the label is how the operator finds this ServiceAccount again after a rename") }, }, @@ -2622,7 +2623,7 @@ func TestDCGMExporterServiceAccountReconcile(t *testing.T) { assert: func(t *testing.T, k8s client.Client, cp *gpuv1.ClusterPolicy) { sa, ok := getServiceAccount(t, k8s, customName) require.True(t, ok) - require.True(t, isDCGMExporterServiceAccount(sa)) + require.True(t, dcgmExporterServiceAccountMarker.Matches(sa.Labels)) _, ok = getServiceAccount(t, k8s, DCGMExporterDefaultServiceAccountName) require.False(t, ok, "the default ServiceAccount must not be created as well") }, @@ -2638,6 +2639,22 @@ func TestDCGMExporterServiceAccountReconcile(t *testing.T) { require.Empty(t, sa.OwnerReferences, "an existing ServiceAccount must not be adopted") }, }, + "a configured name refuses another operand's ServiceAccount": { + // The ClusterPolicy controls nvidia-driver too, so the owner reference alone + // would pass and the exporter would run with the driver's identity. + serviceAccount: &gpuv1.DCGMExporterServiceAccountConfig{Name: driverName}, + otherComponentExisting: []string{driverName}, + expectedState: gpuv1.NotReady, + expectedError: true, + assert: func(t *testing.T, k8s client.Client, cp *gpuv1.ClusterPolicy) { + sa, ok := getServiceAccount(t, k8s, driverName) + require.True(t, ok) + require.True(t, metav1.IsControlledBy(sa, cp), + "the other operand keeps its ServiceAccount") + require.False(t, dcgmExporterServiceAccountMarker.Matches(sa.Labels), + "the exporter must not claim another operand's ServiceAccount") + }, + }, "create=false reports NotReady when the ServiceAccount is missing": { serviceAccount: &gpuv1.DCGMExporterServiceAccountConfig{Name: byoName, Create: new(false)}, expectedState: gpuv1.NotReady, @@ -2842,6 +2859,73 @@ func TestDCGMExporterServiceAccountReconcile(t *testing.T) { } } +// TestDCGMExporterServiceAccountCreateRace covers the window between the availability +// check and the create call: an unrelated ServiceAccount that appears in between makes +// Create report AlreadyExists, which must not be taken as success without revalidating +// what is actually there. +func TestDCGMExporterServiceAccountCreateRace(t *testing.T) { + const ( + testNamespace = "test-namespace" + customName = "metrics-identity" + ) + + testScheme := runtime.NewScheme() + require.NoError(t, corev1.AddToScheme(testScheme)) + require.NoError(t, gpuv1.AddToScheme(testScheme)) + + cp := &gpuv1.ClusterPolicy{ObjectMeta: metav1.ObjectMeta{Name: "cluster-policy", UID: "cp-uid"}} + cp.Spec.DCGMExporter.ServiceAccount = &gpuv1.DCGMExporterServiceAccountConfig{Name: customName} + + // Somebody else's account, already in the cluster. + intruder := &corev1.ServiceAccount{ + ObjectMeta: metav1.ObjectMeta{Name: customName, Namespace: testNamespace}, + } + + // The first Get -- the availability check -- reports the name as free; the object is + // there by the time Create runs, which is the race being reproduced. + var firstGet bool + k8s := fake.NewClientBuilder(). + WithScheme(testScheme). + WithObjects(intruder). + WithInterceptorFuncs(interceptor.Funcs{ + Get: func(ctx context.Context, c client.WithWatch, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error { + if _, ok := obj.(*corev1.ServiceAccount); ok && !firstGet { + firstGet = true + return apierrors.NewNotFound(corev1.Resource("serviceaccounts"), key.Name) + } + return c.Get(ctx, key, obj, opts...) + }, + }). + Build() + + n := ClusterPolicyController{ + client: k8s, + ctx: context.Background(), + singleton: cp, + scheme: testScheme, + operatorNamespace: testNamespace, + resources: []Resources{{ + ServiceAccount: corev1.ServiceAccount{ + ObjectMeta: metav1.ObjectMeta{Name: DCGMExporterDefaultServiceAccountName}, + }, + }}, + stateNames: []string{"state-dcgm-exporter"}, + idx: 0, + logger: ctrl.Log.WithName("test"), + } + + state, err := ServiceAccount(n) + require.Error(t, err, "AlreadyExists must not pass an account the exporter does not manage") + require.Equal(t, gpuv1.NotReady, state) + + found := &corev1.ServiceAccount{} + require.NoError(t, k8s.Get(context.Background(), + types.NamespacedName{Namespace: testNamespace, Name: customName}, found)) + require.Empty(t, found.OwnerReferences, "the intruding ServiceAccount must not be adopted") + require.False(t, dcgmExporterServiceAccountMarker.Matches(found.Labels), + "the intruding ServiceAccount must not be claimed by the exporter") +} + // TestDCGMExporterSupersededServiceAccountCleanup covers the deferred reclaim that runs once // every control of the state converged, when the RoleBindings, SCC and DaemonSet already // reference the configured ServiceAccount. Candidates are found by the exporter label @@ -2857,8 +2941,22 @@ func TestDCGMExporterSupersededServiceAccountCleanup(t *testing.T) { testScheme := runtime.NewScheme() require.NoError(t, corev1.AddToScheme(testScheme)) + require.NoError(t, appsv1.AddToScheme(testScheme)) require.NoError(t, gpuv1.AddToScheme(testScheme)) + // exporterDaemonSet is what the cleanup reads to confirm the operands moved to the + // configured account before anything is deleted. + exporterDaemonSet := func(serviceAccount string) *appsv1.DaemonSet { + return &appsv1.DaemonSet{ + ObjectMeta: metav1.ObjectMeta{Name: dcgmExporterDaemonSetName, Namespace: testNamespace}, + Spec: appsv1.DaemonSetSpec{ + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ServiceAccountName: serviceAccount}, + }, + }, + } + } + testCases := map[string]struct { serviceAccount *gpuv1.DCGMExporterServiceAccountConfig exporterState *bool @@ -2869,8 +2967,13 @@ func TestDCGMExporterSupersededServiceAccountCleanup(t *testing.T) { owned []string unowned []string otherComponent []string - expectDeleted []string - expectKept []string + // deployedServiceAccount is what the live DaemonSet references. Empty means no + // DaemonSet is deployed at all. + deployedServiceAccount string + // noDaemonSet seeds no DaemonSet even when a name would be implied. + noDaemonSet bool + expectDeleted []string + expectKept []string }{ "the default name supersedes nothing": { owned: []string{DCGMExporterDefaultServiceAccountName}, @@ -2931,6 +3034,23 @@ func TestDCGMExporterSupersededServiceAccountCleanup(t *testing.T) { owned: []string{DCGMExporterDefaultServiceAccountName}, expectKept: []string{DCGMExporterDefaultServiceAccountName}, }, + "a DaemonSet still on the previous account defers the reclaim": { + // DaemonSet() reports Ready without touching the live object when no GPU + // node is detected, so convergence alone would delete an account still in + // use. Nothing may go until the deployed DaemonSet has moved. + serviceAccount: &gpuv1.DCGMExporterServiceAccountConfig{Name: customA}, + owned: []string{DCGMExporterDefaultServiceAccountName, customA}, + deployedServiceAccount: DCGMExporterDefaultServiceAccountName, + expectKept: []string{DCGMExporterDefaultServiceAccountName, customA}, + }, + "no deployed DaemonSet reclaims freely": { + // Nothing references a ServiceAccount, so there is nothing to protect. + serviceAccount: &gpuv1.DCGMExporterServiceAccountConfig{Name: customA}, + owned: []string{DCGMExporterDefaultServiceAccountName, customA}, + noDaemonSet: true, + expectDeleted: []string{DCGMExporterDefaultServiceAccountName}, + expectKept: []string{customA}, + }, } for name, tc := range testCases { @@ -2942,7 +3062,7 @@ func TestDCGMExporterSupersededServiceAccountCleanup(t *testing.T) { var objects []client.Object for _, saName := range tc.owned { sa := &corev1.ServiceAccount{ObjectMeta: metav1.ObjectMeta{Name: saName, Namespace: testNamespace}} - markDCGMExporterServiceAccount(sa) + dcgmExporterServiceAccountMarker.Apply(sa) require.NoError(t, controllerutil.SetControllerReference(cp, sa, testScheme)) objects = append(objects, sa) } @@ -2957,6 +3077,15 @@ func TestDCGMExporterSupersededServiceAccountCleanup(t *testing.T) { objects = append(objects, sa) } + if !tc.noDaemonSet { + deployed := tc.deployedServiceAccount + if deployed == "" { + // The operands converged on the configured account. + deployed = cp.Spec.DCGMExporter.GetServiceAccountName(DCGMExporterDefaultServiceAccountName) + } + objects = append(objects, exporterDaemonSet(deployed)) + } + k8s := fake.NewClientBuilder().WithScheme(testScheme).WithObjects(objects...).Build() stateName := tc.stateName if stateName == "" { @@ -3102,6 +3231,7 @@ func TestDCGMExporterCleanupSurvivesDisabledControls(t *testing.T) { testScheme := runtime.NewScheme() require.NoError(t, corev1.AddToScheme(testScheme)) + require.NoError(t, appsv1.AddToScheme(testScheme)) require.NoError(t, gpuv1.AddToScheme(testScheme)) testCases := map[string]struct { @@ -3142,9 +3272,19 @@ func TestDCGMExporterCleanupSurvivesDisabledControls(t *testing.T) { superseded := &corev1.ServiceAccount{ ObjectMeta: metav1.ObjectMeta{Name: DCGMExporterDefaultServiceAccountName, Namespace: testNamespace}, } - markDCGMExporterServiceAccount(superseded) + dcgmExporterServiceAccountMarker.Apply(superseded) require.NoError(t, controllerutil.SetControllerReference(cp, superseded, testScheme)) - k8s := fake.NewClientBuilder().WithScheme(testScheme).WithObjects(superseded).Build() + // The deployed DaemonSet already references the configured account, so only + // the convergence rule under test decides the outcome. + deployed := &appsv1.DaemonSet{ + ObjectMeta: metav1.ObjectMeta{Name: dcgmExporterDaemonSetName, Namespace: testNamespace}, + Spec: appsv1.DaemonSetSpec{ + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ServiceAccountName: customName}, + }, + }, + } + k8s := fake.NewClientBuilder().WithScheme(testScheme).WithObjects(superseded, deployed).Build() // controlFunc is itself the slice of control functions for one state. controls := make(controlFunc, 0, len(tc.states)) diff --git a/internal/ownership/serviceaccount.go b/internal/ownership/serviceaccount.go new file mode 100644 index 0000000000..9e71e725b2 --- /dev/null +++ b/internal/ownership/serviceaccount.go @@ -0,0 +1,102 @@ +/** +# Copyright (c) NVIDIA CORPORATION. All rights reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +**/ + +// Package ownership holds the rules deciding whether an object belongs to one +// operand of the CR being reconciled. +// +// Both the ClusterPolicy and the GPUCluster controller set their own controller +// reference on every operand object they create, so ownership alone answers "does this +// CR own it", never "which operand created it". Where an object's name is +// user-configurable -- the DCGM Exporter ServiceAccount -- that distinction decides +// whether a configured name is a fresh account to create, an account to adopt, or a +// sibling operand's account that must be left alone. A Marker supplies the missing +// half. +package ownership + +import ( + "fmt" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +// Marker is the label identifying the objects one operand created. Each controller +// carries its own: the label has to be one the operator already applied before this +// policy existed, so that objects created by an earlier release are still recognized +// after an upgrade. +type Marker struct { + Key string + Value string +} + +// Matches reports whether the labels carry this marker. +func (m Marker) Matches(labels map[string]string) bool { + return labels[m.Key] == m.Value +} + +// Apply stamps the marker onto an object the operand is about to create. +func (m Marker) Apply(obj metav1.Object) { + labels := obj.GetLabels() + if labels == nil { + labels = map[string]string{} + } + labels[m.Key] = m.Value + obj.SetLabels(labels) +} + +// Selector returns the marker as a list option, for finding every object the operand +// created regardless of the name a previous configuration gave it. +func (m Marker) Selector() client.MatchingLabels { + return client.MatchingLabels{m.Key: m.Value} +} + +// IsManaged reports whether obj is an object this operand created for owner: it carries +// the operand's marker and owner is its controller. Both halves are required -- the +// marker alone would match an object another CR created for the same operand, and the +// controller reference alone would match every other operand of this CR. +func IsManaged(obj metav1.Object, owner metav1.Object, m Marker) bool { + return m.Matches(obj.GetLabels()) && metav1.IsControlledBy(obj, owner) +} + +// ReleaseOwner drops owner's reference from obj, handing the object to whoever is meant +// to own it now. Returns whether anything changed. +func ReleaseOwner(obj metav1.Object, ownerUID types.UID) bool { + refs := obj.GetOwnerReferences() + kept := make([]metav1.OwnerReference, 0, len(refs)) + for _, ref := range refs { + if ref.UID == ownerUID { + continue + } + kept = append(kept, ref) + } + if len(kept) == len(refs) { + return false + } + obj.SetOwnerReferences(kept) + return true +} + +// ConflictError reports a configured ServiceAccount name that names an object this +// operand does not manage -- an unrelated account, or one belonging to a sibling +// operand. Creating it is refused rather than silently adopting an identity the +// operand did not provision. +func ConflictError(ownerKind, name, namespace string) error { + return fmt.Errorf( + "ServiceAccount %q already exists in namespace %q and is not managed by the DCGM Exporter of this %s; "+ + "set dcgmExporter.serviceAccount.create to false to reference it", + name, namespace, ownerKind) +} diff --git a/internal/state/dcgm_exporter.go b/internal/state/dcgm_exporter.go index 78ec143057..121dff3078 100644 --- a/internal/state/dcgm_exporter.go +++ b/internal/state/dcgm_exporter.go @@ -34,6 +34,7 @@ import ( nvidiav1alpha1 "github.com/NVIDIA/gpu-operator/api/nvidia/v1alpha1" "github.com/NVIDIA/gpu-operator/internal/consts" + "github.com/NVIDIA/gpu-operator/internal/ownership" ) const ( @@ -53,8 +54,20 @@ const ( // dcgmExporterDefaultServiceAccountName is the ServiceAccount the DRA operands // reference unless the user configures a different one. dcgmExporterDefaultServiceAccountName = "nvidia-dcgm-exporter-dra" + + // dcgmExporterStateName is this state's name, and the value syncObjects writes into + // the state label of every object it applies. + dcgmExporterStateName = "state-dcgm-exporter" ) +// dcgmExporterServiceAccountMarker identifies the ServiceAccounts this state created. +// It reuses the state label rather than introducing a new one so that accounts created +// by an earlier release are still recognized after an upgrade. +var dcgmExporterServiceAccountMarker = ownership.Marker{ + Key: consts.StateLabel, + Value: dcgmExporterStateName, +} + func NewStateDCGMExporter( k8sClient client.Client, namespace string, @@ -199,7 +212,11 @@ func checkDCGMExporterServiceAccount(ctx context.Context, s *configurableState, if err != nil && !apierrors.IsNotFound(err) { return err } - if err == nil && !metav1.IsControlledBy(existing, cr) { + if err == nil && !ownership.IsManaged(existing, cr, dcgmExporterServiceAccountMarker) { + // Being controlled by this GPUCluster is not enough: it controls every operand's + // ServiceAccount, so a configured name pointing at a sibling operand's account + // (nvidia-dcgm-dra, nvidia-dra-validator) would pass. Adopting one would stamp + // this state's label onto it and hand its lifecycle to the exporter. return dcgmExporterServiceAccountTakeoverError(name, s.namespace) } @@ -209,10 +226,7 @@ func checkDCGMExporterServiceAccount(ctx context.Context, s *configurableState, // dcgmExporterServiceAccountTakeoverError is the error returned when the configured // ServiceAccount exists but belongs to somebody else. func dcgmExporterServiceAccountTakeoverError(name, namespace string) error { - return fmt.Errorf( - "ServiceAccount %q already exists in namespace %q and is not managed by this GPUCluster; "+ - "set dcgmExporter.serviceAccount.create to false to reference it", - name, namespace) + return ownership.ConflictError("GPUCluster", name, namespace) } // guardDCGMExporterServiceAccountAdoption stops createOrUpdateObjs from taking over a @@ -223,14 +237,16 @@ func guardDCGMExporterServiceAccountAdoption(owner metav1.Object, current *unstr if current.GetKind() != "ServiceAccount" { return nil } - for _, ref := range current.GetOwnerReferences() { - if ref.Controller != nil && *ref.Controller && ref.UID == owner.GetUID() { - return nil - } + // The marker is required alongside the controller reference: a sibling operand's + // ServiceAccount carries the same reference, and adopting one would relabel it into + // this state. + if ownership.IsManaged(current, owner, dcgmExporterServiceAccountMarker) { + return nil } if current.GetName() == dcgmExporterDefaultServiceAccountName && len(current.GetOwnerReferences()) == 0 { // The operator default may predate owner references (upgrade from an older - // release), so it stays adoptable. + // release), so it stays adoptable. An account with no owner reference at all + // cannot be a sibling operand's, so this does not reopen the case above. return nil } return dcgmExporterServiceAccountTakeoverError(current.GetName(), current.GetNamespace()) @@ -274,19 +290,10 @@ func releaseDCGMExporterServiceAccountOnDelete(ctx context.Context, s *configura // reference, so checking ownership alone would also strip the reference and label off // another state's ServiceAccount when the user points create=false at it. func (s *configurableState) releaseServiceAccount(ctx context.Context, cr *nvidiav1alpha1.GPUCluster, sa *corev1.ServiceAccount) error { - if sa.Labels[consts.StateLabel] != s.name { + if !dcgmExporterServiceAccountMarker.Matches(sa.Labels) { return nil } - if metav1.IsControlledBy(sa, cr) { - refs := make([]metav1.OwnerReference, 0, len(sa.OwnerReferences)) - for _, ref := range sa.OwnerReferences { - if ref.UID == cr.GetUID() { - continue - } - refs = append(refs, ref) - } - sa.OwnerReferences = refs - } + ownership.ReleaseOwner(sa, cr.GetUID()) delete(sa.Labels, consts.StateLabel) log.FromContext(ctx).V(consts.LogLevelInfo).Info( "Releasing ownership of a user-provided dcgm-exporter ServiceAccount", "Name", sa.Name) @@ -310,12 +317,12 @@ func (s *configurableState) deleteSupersededServiceAccounts(ctx context.Context, list := &corev1.ServiceAccountList{} if err := s.client.List(ctx, list, client.InNamespace(s.namespace), - client.MatchingLabels{consts.StateLabel: s.name}); err != nil { + dcgmExporterServiceAccountMarker.Selector()); err != nil { return err } for i := range list.Items { sa := &list.Items[i] - if sa.Name == keep || !metav1.IsControlledBy(sa, cr) { + if sa.Name == keep || !ownership.IsManaged(sa, cr, dcgmExporterServiceAccountMarker) { continue } log.FromContext(ctx).V(consts.LogLevelInfo).Info( diff --git a/internal/state/dcgm_exporter_test.go b/internal/state/dcgm_exporter_test.go index 0b1accd0b1..a2ea8eaa88 100644 --- a/internal/state/dcgm_exporter_test.go +++ b/internal/state/dcgm_exporter_test.go @@ -480,7 +480,17 @@ func TestDCGMExporterServiceAccountPreSync(t *testing.T) { existing: func(*nvidiav1alpha1.GPUCluster) []client.Object { return []client.Object{unownedServiceAccount(customName)} }, - expectedError: "not managed by this GPUCluster", + expectedError: "not managed by the DCGM Exporter of this GPUCluster", + }, + "a configured name refuses a sibling operand's ServiceAccount": { + // The GPUCluster controls nvidia-dcgm-dra as well, so the owner reference + // alone would pass and the sync would relabel it into this state. + serviceAccount: &nvidiav1.DCGMExporterServiceAccountConfig{Name: "nvidia-dcgm-dra"}, + existing: func(cr *nvidiav1alpha1.GPUCluster) []client.Object { + return []client.Object{otherStateServiceAccount(cr, "nvidia-dcgm-dra")} + }, + expectedError: "not managed by the DCGM Exporter of this GPUCluster", + stillManaged: []string{"nvidia-dcgm-dra"}, }, "a configured name accepts the ServiceAccount it already owns": { serviceAccount: &nvidiav1.DCGMExporterServiceAccountConfig{Name: customName}, @@ -510,6 +520,10 @@ func TestDCGMExporterServiceAccountPreSync(t *testing.T) { err := checkDCGMExporterServiceAccount(ctx, s, cr) if tc.expectedError != "" { require.ErrorContains(t, err, tc.expectedError) + // A refusal must leave the object it refused exactly as it was. + for _, saName := range tc.stillManaged { + requireStillManaged(t, ctx, s, cr, saName) + } return } require.NoError(t, err) @@ -649,12 +663,17 @@ func TestDCGMExporterSupersededServiceAccountReclaim(t *testing.T) { func TestDCGMExporterServiceAccountAdoptionGuard(t *testing.T) { cr := exporterCR(&nvidiav1.DCGMExporterSpec{}) - current := func(kind, name string, refs []metav1.OwnerReference) *unstructured.Unstructured { + // current builds the object the sync found already present. state is the value of + // the state label it carries, empty for none. + current := func(kind, name string, refs []metav1.OwnerReference, state string) *unstructured.Unstructured { obj := &unstructured.Unstructured{} obj.SetKind(kind) obj.SetName(name) obj.SetNamespace("test-operator") obj.SetOwnerReferences(refs) + if state != "" { + obj.SetLabels(map[string]string{consts.StateLabel: state}) + } return obj } ourRef := []metav1.OwnerReference{{ @@ -677,26 +696,37 @@ func TestDCGMExporterServiceAccountAdoptionGuard(t *testing.T) { expectedError string }{ "another kind is not this guard's business": { - current: current("ConfigMap", "metrics-identity", foreignRef), + current: current("ConfigMap", "metrics-identity", foreignRef, ""), + }, + "a ServiceAccount this state already manages is ours to update": { + current: current("ServiceAccount", "metrics-identity", ourRef, "state-dcgm-exporter"), + }, + "a sibling operand's ServiceAccount is refused": { + // The GPUCluster controls it too, so the owner reference alone would pass + // and the sync would relabel it into this state. + current: current("ServiceAccount", "nvidia-dcgm-dra", ourRef, "state-dcgm"), + expectedError: "not managed by the DCGM Exporter of this GPUCluster", }, - "a ServiceAccount this CR already controls is ours to update": { - current: current("ServiceAccount", "metrics-identity", ourRef), + "a ServiceAccount this CR controls but no state claims is refused": { + current: current("ServiceAccount", "metrics-identity", ourRef, ""), + expectedError: "not managed by the DCGM Exporter of this GPUCluster", }, "a ServiceAccount somebody else controls is refused": { - current: current("ServiceAccount", "metrics-identity", foreignRef), - expectedError: "not managed by this GPUCluster", + current: current("ServiceAccount", "metrics-identity", foreignRef, ""), + expectedError: "not managed by the DCGM Exporter of this GPUCluster", }, "an unowned ServiceAccount under a configured name is refused": { - current: current("ServiceAccount", "metrics-identity", nil), - expectedError: "not managed by this GPUCluster", + current: current("ServiceAccount", "metrics-identity", nil, ""), + expectedError: "not managed by the DCGM Exporter of this GPUCluster", }, "the operator default without owner references stays adoptable": { - // It predates owner references, i.e. an upgrade from an older release. - current: current("ServiceAccount", dcgmExporterDefaultServiceAccountName, nil), + // It predates owner references, i.e. an upgrade from an older release. An + // account with no owner reference cannot be a sibling operand's. + current: current("ServiceAccount", dcgmExporterDefaultServiceAccountName, nil, ""), }, "the operator default somebody else controls is refused": { - current: current("ServiceAccount", dcgmExporterDefaultServiceAccountName, foreignRef), - expectedError: "not managed by this GPUCluster", + current: current("ServiceAccount", dcgmExporterDefaultServiceAccountName, foreignRef, ""), + expectedError: "not managed by the DCGM Exporter of this GPUCluster", }, } @@ -745,7 +775,7 @@ func TestDCGMExporterServiceAccountAdoptionGuardWiring(t *testing.T) { err := skel.createOrUpdateObjs(ctx, cr, func(*unstructured.Unstructured) error { return nil }, []*unstructured.Unstructured{desired}) - require.ErrorContains(t, err, "not managed by this GPUCluster") + require.ErrorContains(t, err, "not managed by the DCGM Exporter of this GPUCluster") // The object the guard refused must be left exactly as it was found. found, err := (&configurableState{stateSkel: *skel}).getServiceAccount(ctx, "metrics-identity") From 58170bedd4668df24410be2a4fa4dbf7130f470c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=EB=B0=B1=EC=A7=80=EB=AA=85?= Date: Thu, 17 Sep 2026 09:50:13 +0900 Subject: [PATCH 9/9] Require full ownership before releasing the exporter ServiceAccount MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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: 백지명 --- internal/state/dcgm_exporter.go | 8 +++---- internal/state/dcgm_exporter_test.go | 32 +++++++++++++++++++++++++--- 2 files changed, 33 insertions(+), 7 deletions(-) diff --git a/internal/state/dcgm_exporter.go b/internal/state/dcgm_exporter.go index 121dff3078..51b1c35e62 100644 --- a/internal/state/dcgm_exporter.go +++ b/internal/state/dcgm_exporter.go @@ -286,11 +286,11 @@ func releaseDCGMExporterServiceAccountOnDelete(ctx context.Context, s *configura // releaseServiceAccount hands a ServiceAccount the user now owns back to them by dropping // this GPUCluster's controller reference and the state label. Only a ServiceAccount this -// state managed is touched: every operand of the GPUCluster carries its controller -// reference, so checking ownership alone would also strip the reference and label off -// another state's ServiceAccount when the user points create=false at it. +// state actually managed is touched, which takes both halves: the controller reference +// alone would match another state's account, and the marker alone would match an account +// the user labelled themselves -- neither is ours to mutate. func (s *configurableState) releaseServiceAccount(ctx context.Context, cr *nvidiav1alpha1.GPUCluster, sa *corev1.ServiceAccount) error { - if !dcgmExporterServiceAccountMarker.Matches(sa.Labels) { + if !ownership.IsManaged(sa, cr, dcgmExporterServiceAccountMarker) { return nil } ownership.ReleaseOwner(sa, cr.GetUID()) diff --git a/internal/state/dcgm_exporter_test.go b/internal/state/dcgm_exporter_test.go index a2ea8eaa88..85ba46e3d9 100644 --- a/internal/state/dcgm_exporter_test.go +++ b/internal/state/dcgm_exporter_test.go @@ -384,6 +384,14 @@ func unownedServiceAccount(name string) *corev1.ServiceAccount { } } +// selfLabelledServiceAccount returns a ServiceAccount the user created and labelled with +// this state's label themselves. The operator never owned it, so it is not ours to mutate. +func selfLabelledServiceAccount(name string) *corev1.ServiceAccount { + sa := unownedServiceAccount(name) + sa.Labels = map[string]string{consts.StateLabel: "state-dcgm-exporter"} + return sa +} + // otherStateServiceAccount returns a ServiceAccount another state of the same GPUCluster // manages: it carries the CR's controller reference like every operand object does, but // not this state's label. @@ -801,9 +809,11 @@ func TestDCGMExporterServiceAccountReleasedOnDelete(t *testing.T) { serviceAccount *nvidiav1.DCGMExporterServiceAccountConfig existing func(cr *nvidiav1alpha1.GPUCluster) []client.Object // released must survive with neither this CR's owner reference nor the state - // label; stillManaged must keep whatever the operator put on it. - released []string - stillManaged []string + // label; stillManaged must keep whatever the operator put on it; + // untouchedLabel must keep the label the user put on it themselves. + released []string + stillManaged []string + untouchedLabel []string }{ "create=false releases the ServiceAccount the operator used to own": { serviceAccount: &nvidiav1.DCGMExporterServiceAccountConfig{Name: byoName, Create: new(false)}, @@ -826,6 +836,16 @@ func TestDCGMExporterServiceAccountReleasedOnDelete(t *testing.T) { }, stillManaged: []string{driverName}, }, + "create=false leaves an account the user labelled themselves alone": { + // The label says "an exporter account", not "an account this CR owns". + // Stripping it off an account the operator never created would edit the + // user's object, which the unmanaged contract forbids. + serviceAccount: &nvidiav1.DCGMExporterServiceAccountConfig{Name: byoName, Create: new(false)}, + existing: func(*nvidiav1alpha1.GPUCluster) []client.Object { + return []client.Object{selfLabelledServiceAccount(byoName)} + }, + untouchedLabel: []string{byoName}, + }, "a managed ServiceAccount is left for the state cleanup to remove": { existing: func(cr *nvidiav1alpha1.GPUCluster) []client.Object { return []client.Object{ownedServiceAccount(cr, dcgmExporterDefaultServiceAccountName)} @@ -847,6 +867,12 @@ func TestDCGMExporterServiceAccountReleasedOnDelete(t *testing.T) { for _, saName := range tc.stillManaged { requireStillManaged(t, ctx, s, cr, saName) } + for _, saName := range tc.untouchedLabel { + sa, err := s.getServiceAccount(ctx, saName) + require.NoError(t, err) + assert.Contains(t, sa.Labels, consts.StateLabel, + "a label the user set on their own ServiceAccount must not be removed") + } }) } }