diff --git a/api/nvidia/v1/clusterpolicy_types.go b/api/nvidia/v1/clusterpolicy_types.go index 16d95220c..c330de8c6 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 93b15c255..5c0e8c7a6 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) { @@ -86,3 +89,82 @@ 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()) + }) + } +} + +// 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/api/nvidia/v1/zz_generated.deepcopy.go b/api/nvidia/v1/zz_generated.deepcopy.go index 9e936de60..e23b75c9d 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 e8d0be746..66f609a24 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 f7430a177..6f9054412 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 e8d0be746..66f609a24 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 f7430a177..6f9054412 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 c1bf59e4c..910c51aec 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" @@ -37,6 +38,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" @@ -50,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" ) @@ -117,6 +120,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" @@ -331,6 +337,146 @@ var SubscriptionPathMap = map[string](MountPathToVolumeSource){ type controlFunc []func(n ClusterPolicyController) (gpuv1.State, 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 nil + } + return err + } + if ownership.IsManaged(found, n.singleton, dcgmExporterServiceAccountMarker) { + return nil + } + return ownership.ConflictError("ClusterPolicy", obj.Name, obj.Namespace) +} + +// dcgmExporterServiceAccountRenamed reports whether the exporter is configured to use a +// ServiceAccount other than the operator default. +func dcgmExporterServiceAccountRenamed(config *gpuv1.ClusterPolicySpec) bool { + return dcgmExporterServiceAccountName(config) != DCGMExporterDefaultServiceAccountName +} + +// 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 !ownership.IsManaged(sa, n.singleton, dcgmExporterServiceAccountMarker) { + return nil + } + 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) +} + +// 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 + } + logger := n.logger.WithValues("Namespace", n.operatorNamespace) + 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 +// 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 + } + } + if err := n.deleteOwnedDCGMExporterServiceAccounts(ctx, keep, logger); err != nil { + logger.Info("Couldn't delete", "Error", err) + return gpuv1.NotReady, 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), + dcgmExporterServiceAccountMarker.Selector()); err != nil { + return err + } + for i := range list.Items { + sa := &list.Items[i] + 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) + 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 @@ -338,10 +484,25 @@ func ServiceAccount(n ClusterPolicyController) (gpuv1.State, error) { obj := n.resources[state].ServiceAccount.DeepCopy() obj.Namespace = n.operatorNamespace + // 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) + dcgmExporterServiceAccountMarker.Apply(obj) + } + // 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 isDCGMExporter { + return n.disableDCGMExporterServiceAccount(ctx, obj, unmanaged, logger) + } err := n.client.Delete(ctx, obj) if err != nil && !apierrors.IsNotFound(err) { logger.Info("Couldn't delete", "Error", err) @@ -350,19 +511,59 @@ 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 + } + // 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.releaseDCGMExporterServiceAccount(ctx, found, logger); err != nil { + return gpuv1.NotReady, err + } + return gpuv1.Ready, nil + } + + // 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 + } + } + 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("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") } + + // 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 } @@ -435,6 +636,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 +667,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 +682,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 +782,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 +2037,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 +5101,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 cdf20701c..2a6ce6191 100644 --- a/controllers/object_controls_test.go +++ b/controllers/object_controls_test.go @@ -47,6 +47,8 @@ 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" gpuv1 "github.com/NVIDIA/gpu-operator/api/nvidia/v1" @@ -2532,3 +2534,790 @@ 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" + driverName = "nvidia-driver" + ) + + testScheme := runtime.NewScheme() + require.NoError(t, corev1.AddToScheme(testScheme)) + require.NoError(t, gpuv1.AddToScheme(testScheme)) + + newClusterPolicy := func() *gpuv1.ClusterPolicy { + return &gpuv1.ClusterPolicy{ObjectMeta: metav1.ObjectMeta{Name: "cluster-policy", UID: "cp-uid"}} + } + + serviceAccount := func(name string) *corev1.ServiceAccount { + 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) + dcgmExporterServiceAccountMarker.Apply(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)) + return sa + } + + 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 + } + + testCases := map[string]struct { + serviceAccount *gpuv1.DCGMExporterServiceAccountConfig + exporterState *bool + stateName string + // 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) + }{ + "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)) + require.True(t, dcgmExporterServiceAccountMarker.Matches(sa.Labels), + "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) { + sa, ok := getServiceAccount(t, k8s, customName) + require.True(t, ok) + 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") + }, + }, + "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") + }, + }, + "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, + 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") + }, + }, + "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 + // 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), + 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) + }, + }, + "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. + 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) + }, + }, + } + + 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 + + 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 + if stateName == "" { + stateName = "state-dcgm-exporter" + } + if stateName == "state-driver" { + cp.Spec.Driver.Enabled = new(false) + } + + 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) + if tc.expectedError { + require.Error(t, err) + } else { + require.NoError(t, err) + } + require.Equal(t, tc.expectedState, state) + tc.assert(t, k8s, cp) + }) + } +} + +// 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 +// 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" + customA = "metrics-a" + customB = "metrics-b" + driverName = "nvidia-driver" + ) + + 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 + stateName string + // 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 + // 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}, + expectKept: []string{DCGMExporterDefaultServiceAccountName}, + }, + "renaming reclaims the superseded operator-owned default": { + 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: 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: customA}, + unowned: []string{DCGMExporterDefaultServiceAccountName}, + expectKept: []string{DCGMExporterDefaultServiceAccountName}, + }, + "bringing the default name keeps that ServiceAccount": { + serviceAccount: &gpuv1.DCGMExporterServiceAccountConfig{ + Name: DCGMExporterDefaultServiceAccountName, Create: new(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() sweeps the component's ServiceAccounts on the disabled path. + serviceAccount: &gpuv1.DCGMExporterServiceAccountConfig{Name: customA}, + exporterState: new(false), + owned: []string{DCGMExporterDefaultServiceAccountName, customA}, + expectKept: []string{DCGMExporterDefaultServiceAccountName, customA}, + }, + "another state reclaims nothing": { + serviceAccount: &gpuv1.DCGMExporterServiceAccountConfig{Name: customA}, + stateName: "state-driver", + 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 { + 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 + + var objects []client.Object + for _, saName := range tc.owned { + sa := &corev1.ServiceAccount{ObjectMeta: metav1.ObjectMeta{Name: saName, Namespace: testNamespace}} + dcgmExporterServiceAccountMarker.Apply(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}, + }) + } + 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) + } + + 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 == "" { + stateName = "state-dcgm-exporter" + } + + n := ClusterPolicyController{ + client: k8s, + ctx: context.Background(), + singleton: cp, + scheme: testScheme, + operatorNamespace: testNamespace, + stateNames: []string{stateName}, + idx: 0, + logger: ctrl.Log.WithName("test"), + } + + require.NoError(t, n.cleanupSupersededDCGMExporterServiceAccount(context.Background())) + + 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) + } + }) + } +} + +// 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)) + + spec := gpuv1.ClusterPolicySpec{ + DCGMExporter: gpuv1.DCGMExporterSpec{ + EnablePodLabels: new(true), + ServiceAccount: &gpuv1.DCGMExporterServiceAccountConfig{Name: customSA}, + }, + } + + 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) + }, + }, + "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) + }, + }, + } + + 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"), + } + + state, err := tc.control(n) + require.NoError(t, err) + require.Equal(t, gpuv1.Ready, state) + tc.assert(t, k8s) + }) + } +} + +// 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, appsv1.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} + + // 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}, + } + dcgmExporterServiceAccountMarker.Apply(superseded) + require.NoError(t, controllerutil.SetControllerReference(cp, superseded, testScheme)) + // 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)) + 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 65ea23ce8..70ef05754 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,6 +990,18 @@ 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 converged { + if err := n.cleanupSupersededDCGMExporterServiceAccount(n.ctx); err != nil { + return gpuv1.NotReady, err + } } // move to next state diff --git a/controllers/transforms_test.go b/controllers/transforms_test.go index 8931298a4..72c0cbb26 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 e8d0be746..66f609a24 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 f7430a177..6f9054412 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 e156f5b7a..e40227787 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/templates/gpucluster.yaml b/deployments/gpu-operator/templates/gpucluster.yaml index 51a96ab96..af457d001 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/deployments/gpu-operator/values.yaml b/deployments/gpu-operator/values.yaml index f6222b0d5..e849a7789 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/ownership/serviceaccount.go b/internal/ownership/serviceaccount.go new file mode 100644 index 000000000..9e71e725b --- /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/configurable_state.go b/internal/state/configurable_state.go index 83892666f..33c9588ff 100644 --- a/internal/state/configurable_state.go +++ b/internal/state/configurable_state.go @@ -49,6 +49,25 @@ 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. 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 + // 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) @@ -65,10 +84,32 @@ 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) } - return s.syncObjects(ctx, cr, objs) + if s.preSync != nil { + if err := s.preSync(ctx, s, cr); err != nil { + return SyncStateNotReady, err + } + } + + 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 686274f75..51b1c35e6 100644 --- a/internal/state/dcgm_exporter.go +++ b/internal/state/dcgm_exporter.go @@ -18,16 +18,23 @@ 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/apis/meta/v1/unstructured" "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" nvidiav1alpha1 "github.com/NVIDIA/gpu-operator/api/nvidia/v1alpha1" "github.com/NVIDIA/gpu-operator/internal/consts" + "github.com/NVIDIA/gpu-operator/internal/ownership" ) const ( @@ -43,8 +50,24 @@ 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" + + // 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, @@ -56,6 +79,7 @@ func NewStateDCGMExporter( if err != nil { return nil, err } + skel.adoptionGuard = guardDCGMExporterServiceAccountAdoption return &configurableState{ stateSkel: skel, isEnabled: func(cr *nvidiav1alpha1.GPUCluster) bool { @@ -67,6 +91,9 @@ func NewStateDCGMExporter( }, imageEnvName: dcgmExporterImageEnvName, buildRenderData: buildDCGMExporterRenderData, + preSync: checkDCGMExporterServiceAccount, + postSync: reclaimSupersededDCGMExporterServiceAccounts, + preDelete: releaseDCGMExporterServiceAccountOnDelete, }, nil } @@ -140,9 +167,173 @@ 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 } +// 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) + + if !spec.IsServiceAccountCreateEnabled() { + // The manifests omit the ServiceAccount entirely, so a missing one would leave + // the DaemonSet pending without any signal. + 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", + name, s.namespace) + } + return err + } + // 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 { + 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. + // 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 && !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) + } + + return nil +} + +// dcgmExporterServiceAccountTakeoverError is the error returned when the configured +// ServiceAccount exists but belongs to somebody else. +func dcgmExporterServiceAccountTakeoverError(name, namespace string) error { + return ownership.ConflictError("GPUCluster", 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 + } + // 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. 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()) +} + +// 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 +// 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 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 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 !ownership.IsManaged(sa, cr, dcgmExporterServiceAccountMarker) { + return nil + } + 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) + 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{} + err := s.client.Get(ctx, types.NamespacedName{Namespace: s.namespace, Name: name}, sa) + return sa, err +} + +// 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), + dcgmExporterServiceAccountMarker.Selector()); err != nil { + return err + } + for i := range list.Items { + sa := &list.Items[i] + if sa.Name == keep || !ownership.IsManaged(sa, cr, dcgmExporterServiceAccountMarker) { + 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 +} + // 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 6b228808d..85ba46e3d 100644 --- a/internal/state/dcgm_exporter_test.go +++ b/internal/state/dcgm_exporter_test.go @@ -22,16 +22,22 @@ 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" "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" nvidiav1alpha1 "github.com/NVIDIA/gpu-operator/api/nvidia/v1alpha1" + "github.com/NVIDIA/gpu-operator/internal/consts" ) const dcgmExporterManifestDir = "../../manifests/state-dcgm-exporter" @@ -58,6 +64,31 @@ func newTestDCGMExporterState(t *testing.T, serviceMonitorCRD bool) *configurabl return s.(*configurableState) } +// newTestDCGMExporterStateWithObjects builds the state with a client that already holds +// 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() @@ -235,3 +266,668 @@ 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 +} + +// 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, + }, + } + + 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")) + }) + } +} + +// 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), + }}, + }, + } +} + +// 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"}, + } +} + +// 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. +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 { + 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 + // 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": { + 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)} + }, + }, + "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 { + return []client.Object{unownedServiceAccount(customName)} + }, + 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}, + 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}, + }, + } + + 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) + // 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) + + // 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) + } + }) + } +} + +// 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 ( + 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. + deleted []string + kept []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: customA}, + existing: func(cr *nvidiav1alpha1.GPUCluster) []client.Object { + return []client.Object{ + ownedServiceAccount(cr, dcgmExporterDefaultServiceAccountName), + ownedServiceAccount(cr, customA), + } + }, + deleted: []string{dcgmExporterDefaultServiceAccountName}, + kept: []string{customA}, + }, + "renaming keeps a previous ServiceAccount the operator does not own": { + 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 { + return []client.Object{ + ownedServiceAccount(cr, dcgmExporterDefaultServiceAccountName), + unownedServiceAccount(byoName), + } + }, + deleted: []string{dcgmExporterDefaultServiceAccountName}, + kept: []string{byoName}, + }, + "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, byoName)} + }, + kept: []string{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)} + }, + kept: []string{driverName, customA}, + }, + } + + 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, reclaimSupersededDCGMExporterServiceAccounts(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) + } + }) + } +} + +// 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{}) + + // 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{{ + 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 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 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 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 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. 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 the DCGM Exporter of this GPUCluster", + }, + } + + 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) + }) + } +} + +// 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"}, + }) + + testScheme := runtime.NewScheme() + require.NoError(t, corev1.AddToScheme(testScheme)) + require.NoError(t, nvidiav1alpha1.AddToScheme(testScheme)) + + // The ServiceAccount appeared after the preSync check accepted the configuration. + existing := unownedServiceAccount("metrics-identity") + k8sClient := fake.NewClientBuilder().WithScheme(testScheme).WithObjects(existing).Build() + + skel := &stateSkel{ + name: "state-dcgm-exporter", + namespace: "test-operator", + client: k8sClient, + scheme: testScheme, + adoptionGuard: guardDCGMExporterServiceAccountAdoption, + } + + desired := &unstructured.Unstructured{} + desired.SetAPIVersion("v1") + desired.SetKind("ServiceAccount") + desired.SetName("metrics-identity") + desired.SetNamespace("test-operator") + + err := skel.createOrUpdateObjs(ctx, cr, func(*unstructured.Unstructured) error { return nil }, + []*unstructured.Unstructured{desired}) + 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") + require.NoError(t, err) + 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 preSync -- which the disabled path +// never reaches. +func TestDCGMExporterServiceAccountReleasedOnDelete(t *testing.T) { + ctx := context.Background() + 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; 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)}, + 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}, + }, + "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}, + }, + "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)} + }, + stillManaged: []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 { + requireReleased(t, ctx, s, cr, saName) + } + 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") + } + }) + } +} + +// 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) + + 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") +} diff --git a/internal/state/driver.go b/internal/state/driver.go index bf7d0b527..48947f438 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 21c37b86c..542dd1148 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 { diff --git a/internal/state/types.go b/internal/state/types.go index 8c7c9237b..57a3631aa 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 2bd03f52f..62334e4eb 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 | quote }} namespace: {{ .Namespace }} +{{- end }} diff --git a/manifests/state-dcgm-exporter/0300_rolebinding.yaml b/manifests/state-dcgm-exporter/0300_rolebinding.yaml index 5bbb009da..de076f969 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 | quote }} namespace: {{ .Namespace }} diff --git a/manifests/state-dcgm-exporter/0310_clusterrolebinding.yaml b/manifests/state-dcgm-exporter/0310_clusterrolebinding.yaml index 401695f2d..14b4da5d5 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 | quote }} namespace: {{ .Namespace }} diff --git a/manifests/state-dcgm-exporter/0450_scc.openshift.yaml b/manifests/state-dcgm-exporter/0450_scc.openshift.yaml index e45262ff4..9ef041fa4 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 dcf9b4203..05d255aa2 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 | 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.