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/4] 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/4] 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/4] 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/4] 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{