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..235f13e61 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" @@ -117,6 +119,9 @@ const ( DCGMRemoteEngineEnvName = "DCGM_REMOTE_HOSTENGINE_INFO" // DCGMDefaultPort indicates default port bound to DCGM host engine DCGMDefaultPort = 5555 + // DCGMExporterDefaultServiceAccountName is the ServiceAccount the DCGM Exporter + // operands reference unless the user configures a different one. + DCGMExporterDefaultServiceAccountName = "nvidia-dcgm-exporter" // DCGMExporterConfigMapDataEnvName is the env name specifying the namespace:name // ConfigMap with custom metrics DCGMExporterConfigMapDataEnvName = "DCGM_EXPORTER_CONFIGMAP_DATA" @@ -332,16 +337,120 @@ var SubscriptionPathMap = map[string](MountPathToVolumeSource){ type controlFunc []func(n ClusterPolicyController) (gpuv1.State, error) // ServiceAccount creates ServiceAccount resource +// isServiceAccountOwned reports whether the ServiceAccount exists and is controlled +// by the ClusterPolicy being reconciled. A missing ServiceAccount counts as owned so +// that callers fall through to a delete that is a no-op. +func (n ClusterPolicyController) isServiceAccountOwned(ctx context.Context, obj *corev1.ServiceAccount) (bool, error) { + found := &corev1.ServiceAccount{} + if err := n.client.Get(ctx, types.NamespacedName{Namespace: obj.Namespace, Name: obj.Name}, found); err != nil { + if apierrors.IsNotFound(err) { + return true, nil + } + return false, err + } + return metav1.IsControlledBy(found, n.singleton), nil +} + +// dcgmExporterServiceAccountRenamed reports whether the exporter is configured to use a +// ServiceAccount other than the operator default, i.e. whether a previously created +// default ServiceAccount may have been superseded. +func dcgmExporterServiceAccountRenamed(config *gpuv1.ClusterPolicySpec) bool { + return dcgmExporterServiceAccountName(config) != DCGMExporterDefaultServiceAccountName +} + +// releaseServiceAccountOwnership drops this ClusterPolicy's controller reference from a +// ServiceAccount the user has taken over. Without it the object stays garbage-collected +// together with the ClusterPolicy even though the operator no longer manages it. +func (n ClusterPolicyController) releaseServiceAccountOwnership(ctx context.Context, sa *corev1.ServiceAccount, logger logr.Logger) error { + if !metav1.IsControlledBy(sa, n.singleton) { + return nil + } + refs := make([]metav1.OwnerReference, 0, len(sa.OwnerReferences)) + for _, ref := range sa.OwnerReferences { + if ref.UID == n.singleton.GetUID() { + continue + } + refs = append(refs, ref) + } + sa.OwnerReferences = refs + logger.V(1).Info("Releasing ownership of a user-provided ServiceAccount", "Name", sa.Name) + return n.client.Update(ctx, sa) +} + +// cleanupSupersededDCGMExporterServiceAccount removes the operator-created default +// ServiceAccount once a different one has taken over. It runs only after every control of +// the state converged, so a failure part-way through reconciliation never leaves the +// DaemonSet referencing a ServiceAccount that has already been deleted. +func (n ClusterPolicyController) cleanupSupersededDCGMExporterServiceAccount(ctx context.Context) error { + if n.stateNames[n.idx] != "state-dcgm-exporter" || !n.isStateEnabled(n.stateNames[n.idx]) { + return nil + } + if !dcgmExporterServiceAccountRenamed(&n.singleton.Spec) { + // The configured ServiceAccount is the default one, so there is nothing it + // could have superseded. + return nil + } + logger := n.logger.WithValues("ServiceAccount", DCGMExporterDefaultServiceAccountName, "Namespace", n.operatorNamespace) + return n.deleteOwnedServiceAccount(ctx, DCGMExporterDefaultServiceAccountName, logger) +} + +// deleteOwnedServiceAccount removes a ServiceAccount left behind by a previous +// configuration, but only when this ClusterPolicy owns it: an object the user +// provisioned under the same name is left alone. +func (n ClusterPolicyController) deleteOwnedServiceAccount(ctx context.Context, name string, logger logr.Logger) error { + found := &corev1.ServiceAccount{} + err := n.client.Get(ctx, types.NamespacedName{Namespace: n.operatorNamespace, Name: name}, found) + if err != nil { + if apierrors.IsNotFound(err) { + return nil + } + return err + } + if !metav1.IsControlledBy(found, n.singleton) { + return nil + } + logger.V(1).Info("Removing the superseded dcgm-exporter ServiceAccount", "Name", name) + if err := n.client.Delete(ctx, found); err != nil && !apierrors.IsNotFound(err) { + return err + } + return nil +} + func ServiceAccount(n ClusterPolicyController) (gpuv1.State, error) { ctx := n.ctx state := n.idx obj := n.resources[state].ServiceAccount.DeepCopy() obj.Namespace = n.operatorNamespace + // The DCGM Exporter ServiceAccount name is user-configurable. + isDCGMExporter := n.stateNames[state] == "state-dcgm-exporter" + if isDCGMExporter { + obj.Name = dcgmExporterServiceAccountName(&n.singleton.Spec) + } + // A ServiceAccount the user brings is only referenced, never managed: the + // operator must not create, adopt, mutate or delete it. + unmanaged := isDCGMExporter && !n.singleton.Spec.DCGMExporter.IsServiceAccountCreateEnabled() + logger := n.logger.WithValues("ServiceAccount", obj.Name, "Namespace", obj.Namespace) // Check if state is disabled and cleanup resource if exists if !n.isStateEnabled(n.stateNames[n.idx]) { + if unmanaged { + return gpuv1.Disabled, nil + } + if isDCGMExporter { + // A ServiceAccount that carries no ClusterPolicy owner reference was not + // created by this operator -- for instance one the user had already + // provisioned under the configured name -- so it is left untouched. + owned, err := n.isServiceAccountOwned(ctx, obj) + if err != nil { + return gpuv1.NotReady, err + } + if !owned { + logger.V(1).Info("ServiceAccount is not owned by the ClusterPolicy, skipping deletion") + return gpuv1.Disabled, nil + } + } err := n.client.Delete(ctx, obj) if err != nil && !apierrors.IsNotFound(err) { logger.Info("Couldn't delete", "Error", err) @@ -350,19 +459,55 @@ 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.releaseServiceAccountOwnership(ctx, found, logger); err != nil { + return gpuv1.NotReady, err + } + return gpuv1.Ready, nil + } + + if isDCGMExporter && dcgmExporterServiceAccountRenamed(&n.singleton.Spec) { + // Only a name the user chose can collide with an unrelated object; the default is + // left tolerant so an upgrade that lost the owner reference keeps converging. + owned, err := n.isServiceAccountOwned(ctx, obj) + if err != nil { + return gpuv1.NotReady, err + } + if !owned { + err := fmt.Errorf("ServiceAccount %q already exists in namespace %q and is not managed by this ClusterPolicy; "+ + "set dcgmExporter.serviceAccount.create to false to reference it", obj.Name, obj.Namespace) + 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 + logger.Info("Found Resource, skipping update") } + + // Reclaiming the superseded default ServiceAccount is deferred to + // cleanupSupersededDCGMExporterServiceAccount, which runs once every control of this + // state has converged. return gpuv1.Ready, nil } @@ -435,6 +580,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 +611,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 +626,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 +726,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 +1981,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 +5045,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..72df13061 100644 --- a/controllers/object_controls_test.go +++ b/controllers/object_controls_test.go @@ -47,6 +47,7 @@ import ( ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "sigs.k8s.io/controller-runtime/pkg/log/zap" gpuv1 "github.com/NVIDIA/gpu-operator/api/nvidia/v1" @@ -2532,3 +2533,419 @@ func TestDriverPrecompiledLibModulesSuse(t *testing.T) { }) } } + +// TestDCGMExporterServiceAccountReconcile covers the ServiceAccount lifecycle for the +// DCGM Exporter: the operator honours a configured name and, when the ServiceAccount is +// supplied by the user, only references it -- it is never created, adopted or deleted. + +// TestDCGMExporterServiceAccountReconcile covers the ServiceAccount lifecycle for the +// DCGM Exporter: the operator honours a configured name, and a ServiceAccount supplied by +// the user is only referenced -- never created, adopted, mutated or deleted, and never +// left carrying a ClusterPolicy owner reference that would garbage-collect it. +func TestDCGMExporterServiceAccountReconcile(t *testing.T) { + const ( + testNamespace = "test-namespace" + byoName = "byo-metrics-identity" + customName = "metrics-identity" + ) + + testScheme := runtime.NewScheme() + require.NoError(t, corev1.AddToScheme(testScheme)) + require.NoError(t, gpuv1.AddToScheme(testScheme)) + + 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 := 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; ownedExisting are seeded with a ClusterPolicy + // controller reference. + existing []string + ownedExisting []string + expectedState gpuv1.State + expectedError bool + // assert runs after ServiceAccount(); present reports whether each name exists. + assert func(t *testing.T, k8s client.Client, cp *gpuv1.ClusterPolicy) + }{ + "default configuration creates the default ServiceAccount": { + expectedState: gpuv1.Ready, + assert: func(t *testing.T, k8s client.Client, cp *gpuv1.ClusterPolicy) { + sa, ok := getServiceAccount(t, k8s, DCGMExporterDefaultServiceAccountName) + require.True(t, ok) + require.True(t, metav1.IsControlledBy(sa, cp)) + }, + }, + "configured name creates that ServiceAccount instead of the default": { + serviceAccount: &gpuv1.DCGMExporterServiceAccountConfig{Name: customName}, + expectedState: gpuv1.Ready, + assert: func(t *testing.T, k8s client.Client, cp *gpuv1.ClusterPolicy) { + _, ok := getServiceAccount(t, k8s, customName) + require.True(t, ok) + _, ok = getServiceAccount(t, k8s, DCGMExporterDefaultServiceAccountName) + require.False(t, ok, "the default ServiceAccount must not be created as well") + }, + }, + "a configured name refuses to take over an unowned ServiceAccount": { + serviceAccount: &gpuv1.DCGMExporterServiceAccountConfig{Name: customName}, + existing: []string{customName}, + expectedState: gpuv1.NotReady, + expectedError: true, + assert: func(t *testing.T, k8s client.Client, cp *gpuv1.ClusterPolicy) { + sa, ok := getServiceAccount(t, k8s, customName) + require.True(t, ok) + require.Empty(t, sa.OwnerReferences, "an existing ServiceAccount must not be adopted") + }, + }, + "create=false reports NotReady when the ServiceAccount is missing": { + serviceAccount: &gpuv1.DCGMExporterServiceAccountConfig{Name: byoName, Create: new(false)}, + expectedState: gpuv1.NotReady, + expectedError: true, + assert: func(t *testing.T, k8s client.Client, cp *gpuv1.ClusterPolicy) { + _, ok := getServiceAccount(t, k8s, byoName) + require.False(t, ok, "a user-provided ServiceAccount must never be created by the operator") + }, + }, + "create=false references an existing ServiceAccount without adopting it": { + serviceAccount: &gpuv1.DCGMExporterServiceAccountConfig{Name: byoName, Create: new(false)}, + existing: []string{byoName}, + expectedState: gpuv1.Ready, + assert: func(t *testing.T, k8s client.Client, cp *gpuv1.ClusterPolicy) { + sa, ok := getServiceAccount(t, k8s, byoName) + require.True(t, ok) + require.Empty(t, sa.OwnerReferences, "the operator must not take ownership of a user-provided ServiceAccount") + }, + }, + "create=false releases ownership of a ServiceAccount the operator had created": { + serviceAccount: &gpuv1.DCGMExporterServiceAccountConfig{ + Name: DCGMExporterDefaultServiceAccountName, Create: new(false), + }, + ownedExisting: []string{DCGMExporterDefaultServiceAccountName}, + expectedState: gpuv1.Ready, + assert: func(t *testing.T, k8s client.Client, cp *gpuv1.ClusterPolicy) { + sa, ok := getServiceAccount(t, k8s, DCGMExporterDefaultServiceAccountName) + require.True(t, ok, "the referenced ServiceAccount must not be deleted") + require.False(t, metav1.IsControlledBy(sa, cp), + "the owner reference has to go, otherwise the user's ServiceAccount is garbage-collected with the ClusterPolicy") + }, + }, + "disabling the exporter keeps a user-provided ServiceAccount": { + serviceAccount: &gpuv1.DCGMExporterServiceAccountConfig{Name: byoName, Create: new(false)}, + exporterState: new(false), + existing: []string{byoName}, + expectedState: gpuv1.Disabled, + assert: func(t *testing.T, k8s client.Client, cp *gpuv1.ClusterPolicy) { + _, ok := getServiceAccount(t, k8s, byoName) + require.True(t, ok, "a user-provided ServiceAccount must survive disabling the exporter") + }, + }, + "disabling the exporter keeps a ServiceAccount the operator does not own": { + exporterState: new(false), + existing: []string{DCGMExporterDefaultServiceAccountName}, + expectedState: gpuv1.Disabled, + assert: func(t *testing.T, k8s client.Client, cp *gpuv1.ClusterPolicy) { + _, ok := getServiceAccount(t, k8s, DCGMExporterDefaultServiceAccountName) + require.True(t, ok, "only a ServiceAccount owned by the ClusterPolicy may be deleted") + }, + }, + "disabling the exporter deletes the ServiceAccount the operator owns": { + exporterState: new(false), + ownedExisting: []string{DCGMExporterDefaultServiceAccountName}, + expectedState: gpuv1.Disabled, + assert: func(t *testing.T, k8s client.Client, cp *gpuv1.ClusterPolicy) { + _, ok := getServiceAccount(t, k8s, DCGMExporterDefaultServiceAccountName) + require.False(t, ok) + }, + }, + "a non-DCGM state deletes its ServiceAccount regardless of ownership": { + // The ownership check is scoped to the DCGM Exporter; every other state keeps + // the previous unconditional cleanup on disable. + stateName: "state-driver", + existing: []string{DCGMExporterDefaultServiceAccountName}, + expectedState: gpuv1.Disabled, + assert: func(t *testing.T, k8s client.Client, cp *gpuv1.ClusterPolicy) { + _, ok := getServiceAccount(t, k8s, DCGMExporterDefaultServiceAccountName) + require.False(t, ok) + }, + }, + } + + 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)) + for _, saName := range tc.existing { + objects = append(objects, serviceAccount(saName)) + } + for _, saName := range tc.ownedExisting { + objects = append(objects, ownedServiceAccount(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) + }) + } +} + +// TestDCGMExporterSupersededServiceAccountCleanup covers the deferred reclaim of the +// operator default. It runs only after every control of the state converged, so the +// RoleBindings, SCC and DaemonSet already reference the replacement by then. +func TestDCGMExporterSupersededServiceAccountCleanup(t *testing.T) { + const ( + testNamespace = "test-namespace" + customName = "metrics-identity" + ) + + testScheme := runtime.NewScheme() + require.NoError(t, corev1.AddToScheme(testScheme)) + require.NoError(t, gpuv1.AddToScheme(testScheme)) + + testCases := map[string]struct { + serviceAccount *gpuv1.DCGMExporterServiceAccountConfig + exporterState *bool + stateName string + defaultOwned bool + expectDeleted bool + }{ + "renaming reclaims the superseded operator-owned default": { + serviceAccount: &gpuv1.DCGMExporterServiceAccountConfig{Name: customName}, + defaultOwned: true, + expectDeleted: true, + }, + "handing over to a user-provided ServiceAccount reclaims the owned default": { + serviceAccount: &gpuv1.DCGMExporterServiceAccountConfig{Name: customName, Create: new(false)}, + defaultOwned: true, + expectDeleted: true, + }, + "a previous default the operator does not own is left alone": { + serviceAccount: &gpuv1.DCGMExporterServiceAccountConfig{Name: customName}, + defaultOwned: false, + expectDeleted: false, + }, + "the default name supersedes nothing": { + defaultOwned: true, + expectDeleted: false, + }, + "bringing the default name keeps that ServiceAccount": { + serviceAccount: &gpuv1.DCGMExporterServiceAccountConfig{ + Name: DCGMExporterDefaultServiceAccountName, Create: new(false), + }, + defaultOwned: true, + expectDeleted: false, + }, + "a disabled exporter reclaims nothing here": { + serviceAccount: &gpuv1.DCGMExporterServiceAccountConfig{Name: customName}, + exporterState: new(false), + defaultOwned: true, + expectDeleted: false, + }, + "another state reclaims nothing": { + serviceAccount: &gpuv1.DCGMExporterServiceAccountConfig{Name: customName}, + stateName: "state-driver", + defaultOwned: true, + expectDeleted: false, + }, + } + + 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 + + previous := &corev1.ServiceAccount{ + ObjectMeta: metav1.ObjectMeta{Name: DCGMExporterDefaultServiceAccountName, Namespace: testNamespace}, + } + if tc.defaultOwned { + require.NoError(t, controllerutil.SetControllerReference(cp, previous, testScheme)) + } + + k8s := fake.NewClientBuilder().WithScheme(testScheme).WithObjects(previous).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())) + + found := &corev1.ServiceAccount{} + err := k8s.Get(context.Background(), + types.NamespacedName{Namespace: testNamespace, Name: DCGMExporterDefaultServiceAccountName}, found) + if tc.expectDeleted { + require.True(t, apierrors.IsNotFound(err), "the superseded default must be reclaimed") + } else { + require.NoError(t, err, "this ServiceAccount must not be reclaimed") + } + }) + } +} + +// TestDCGMExporterRBACSubjects verifies that the RBAC bindings and the OpenShift SCC +// 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) + }) + } +} diff --git a/controllers/state_manager.go b/controllers/state_manager.go index 65ea23ce8..95b91dd98 100644 --- a/controllers/state_manager.go +++ b/controllers/state_manager.go @@ -987,6 +987,15 @@ func (n *ClusterPolicyController) step() (gpuv1.State, error) { } } + // Objects a previous configuration superseded are reclaimed only once every control + // of this state converged: deleting them earlier would leave the operands that still + // reference them pointing at objects that no longer exist. + if result == gpuv1.Ready { + if err := n.cleanupSupersededDCGMExporterServiceAccount(n.ctx); err != nil { + return gpuv1.NotReady, err + } + } + // move to next state n.idx++ diff --git a/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/state/configurable_state.go b/internal/state/configurable_state.go index 83892666f..0a6e7e9b0 100644 --- a/internal/state/configurable_state.go +++ b/internal/state/configurable_state.go @@ -49,6 +49,17 @@ type configurableState struct { // image path and DRA apiVersion. It receives ctx and the skeleton so operands that // need the client or logging (e.g. dcgm-exporter's ServiceMonitor CRD probe) can use them. buildRenderData func(ctx context.Context, s *configurableState, cr *nvidiav1alpha1.GPUCluster, imagePath, apiVersion, openshiftVersion string) (any, error) + + // preSync runs after the manifests render but before they are applied, for operands + // that depend on cluster state the templates cannot express. Returning an error marks + // the state NotReady, so it is the place to surface a misconfiguration instead of + // applying objects that cannot converge. + preSync func(ctx context.Context, s *configurableState, cr *nvidiav1alpha1.GPUCluster) error + + // postSync runs after the manifests converged. Reclaiming objects a previous + // configuration superseded belongs here rather than in preSync: deleting them before + // the replacements exist would leave the operands referencing objects that are gone. + postSync func(ctx context.Context, s *configurableState, cr *nvidiav1alpha1.GPUCluster) error } var _ State = (*configurableState)(nil) @@ -68,7 +79,24 @@ func (s *configurableState) Sync(ctx context.Context, customResource any, infoCa 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..06b2a3b56 100644 --- a/internal/state/dcgm_exporter.go +++ b/internal/state/dcgm_exporter.go @@ -18,11 +18,17 @@ 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" @@ -43,6 +49,10 @@ const ( dcgmExporterCustomCollectors = "/etc/dcgm-exporter/dcgm-metrics.csv" dcgmExporterDefaultKubeletRootDir = "/var/lib/kubelet" dcgmExporterDefaultJobMappingDir = "/var/lib/dcgm-exporter/job-mapping" + + // dcgmExporterDefaultServiceAccountName is the ServiceAccount the DRA operands + // reference unless the user configures a different one. + dcgmExporterDefaultServiceAccountName = "nvidia-dcgm-exporter-dra" ) func NewStateDCGMExporter( @@ -56,6 +66,7 @@ func NewStateDCGMExporter( if err != nil { return nil, err } + skel.adoptionGuard = guardDCGMExporterServiceAccountAdoption return &configurableState{ stateSkel: skel, isEnabled: func(cr *nvidiav1alpha1.GPUCluster) bool { @@ -67,6 +78,8 @@ func NewStateDCGMExporter( }, imageEnvName: dcgmExporterImageEnvName, buildRenderData: buildDCGMExporterRenderData, + preSync: checkDCGMExporterServiceAccount, + postSync: reconcileDCGMExporterServiceAccountOwnership, }, nil } @@ -140,9 +153,168 @@ 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 reconciles the parts of the ServiceAccount contract the +// manifests cannot express: a ServiceAccount the user brings has to already exist, one the +// operator would manage must not be an existing object owned by somebody else, and the +// operator-owned default is removed once a different name takes over. +func checkDCGMExporterServiceAccount(ctx context.Context, s *configurableState, cr *nvidiav1alpha1.GPUCluster) error { + spec := cr.Spec.DCGMExporter + name := spec.GetServiceAccountName(dcgmExporterDefaultServiceAccountName) + + if !spec.IsServiceAccountCreateEnabled() { + // The manifests omit the ServiceAccount entirely, so a missing one would leave + // the DaemonSet pending without any signal. + if _, err := s.getServiceAccount(ctx, name); err != nil { + if apierrors.IsNotFound(err) { + return fmt.Errorf( + "ServiceAccount %q configured with create=false does not exist in namespace %q", + name, s.namespace) + } + return err + } + return nil + } + + if name == dcgmExporterDefaultServiceAccountName { + return nil + } + + // Adopting an object the operator did not create would hand it to garbage collection + // on CR deletion, so a name that is already taken has to be opted into explicitly. + // guardDCGMExporterServiceAccountAdoption re-checks this after the create call, for + // an object that appears in between. + existing, err := s.getServiceAccount(ctx, name) + if err != nil && !apierrors.IsNotFound(err) { + return err + } + if err == nil && !metav1.IsControlledBy(existing, cr) { + return dcgmExporterServiceAccountTakeoverError(name, s.namespace) + } + + return nil +} + +// dcgmExporterServiceAccountTakeoverError is the error returned when the configured +// ServiceAccount exists but belongs to somebody else. +func dcgmExporterServiceAccountTakeoverError(name, namespace string) error { + return fmt.Errorf( + "ServiceAccount %q already exists in namespace %q and is not managed by this GPUCluster; "+ + "set dcgmExporter.serviceAccount.create to false to reference it", + name, namespace) +} + +// guardDCGMExporterServiceAccountAdoption stops createOrUpdateObjs from taking over a +// ServiceAccount that appeared between the preSync ownership check and the create call. +// Without it the AlreadyExists path would stamp this CR's controller reference onto an +// object somebody else owns, handing it to garbage collection with the GPUCluster. +func guardDCGMExporterServiceAccountAdoption(owner metav1.Object, current *unstructured.Unstructured) error { + if current.GetKind() != "ServiceAccount" { + return nil + } + for _, ref := range current.GetOwnerReferences() { + if ref.Controller != nil && *ref.Controller && ref.UID == owner.GetUID() { + return nil + } + } + if current.GetName() == dcgmExporterDefaultServiceAccountName && len(current.GetOwnerReferences()) == 0 { + // The operator default may predate owner references (upgrade from an older + // release), so it stays adoptable. + return nil + } + return dcgmExporterServiceAccountTakeoverError(current.GetName(), current.GetNamespace()) +} + +// reconcileDCGMExporterServiceAccountOwnership runs once the manifests converged. It +// reclaims the operator default a different ServiceAccount superseded, and releases +// ownership of a ServiceAccount the user took over with create=false. +func reconcileDCGMExporterServiceAccountOwnership(ctx context.Context, s *configurableState, cr *nvidiav1alpha1.GPUCluster) error { + spec := cr.Spec.DCGMExporter + name := spec.GetServiceAccountName(dcgmExporterDefaultServiceAccountName) + + if !spec.IsServiceAccountCreateEnabled() { + // The same object may have been operator-managed before create was set to false. + // Both the controller reference and the state label have to go, otherwise it is + // garbage-collected with the GPUCluster or swept by the state cleanup. + sa, err := s.getServiceAccount(ctx, name) + if err != nil { + if apierrors.IsNotFound(err) { + return nil + } + return err + } + if err := s.releaseServiceAccount(ctx, cr, sa); err != nil { + return err + } + } + + if name == dcgmExporterDefaultServiceAccountName { + return nil + } + return s.deleteOwnedServiceAccount(ctx, cr, dcgmExporterDefaultServiceAccountName) +} + +// releaseServiceAccount drops this GPUCluster's controller reference and the state label +// from a ServiceAccount the user now owns. +func (s *configurableState) releaseServiceAccount(ctx context.Context, cr *nvidiav1alpha1.GPUCluster, sa *corev1.ServiceAccount) error { + changed := false + if metav1.IsControlledBy(sa, cr) { + refs := make([]metav1.OwnerReference, 0, len(sa.OwnerReferences)) + for _, ref := range sa.OwnerReferences { + if ref.UID == cr.GetUID() { + continue + } + refs = append(refs, ref) + } + sa.OwnerReferences = refs + changed = true + } + if _, ok := sa.Labels[consts.StateLabel]; ok { + delete(sa.Labels, consts.StateLabel) + changed = true + } + if !changed { + return nil + } + log.FromContext(ctx).V(consts.LogLevelInfo).Info( + "Releasing ownership of a user-provided dcgm-exporter ServiceAccount", "Name", sa.Name) + return s.client.Update(ctx, sa) +} + +// getServiceAccount reads a ServiceAccount from the operand namespace. +func (s *configurableState) getServiceAccount(ctx context.Context, name string) (*corev1.ServiceAccount, error) { + sa := &corev1.ServiceAccount{} + err := s.client.Get(ctx, types.NamespacedName{Namespace: s.namespace, Name: name}, sa) + return sa, err +} + +// deleteOwnedServiceAccount removes a ServiceAccount left behind by a previous +// configuration, but only when this CR owns it: an object the user provisioned under the +// same name is left alone. Renaming from one custom name to another is not tracked, so +// only the operator default is reclaimed here. +func (s *configurableState) deleteOwnedServiceAccount(ctx context.Context, cr *nvidiav1alpha1.GPUCluster, name string) error { + sa, err := s.getServiceAccount(ctx, name) + if err != nil { + if apierrors.IsNotFound(err) { + return nil + } + return err + } + if !metav1.IsControlledBy(sa, cr) { + return nil + } + log.FromContext(ctx).V(consts.LogLevelInfo).Info( + "Removing the superseded dcgm-exporter ServiceAccount", "Name", name) + if err := s.client.Delete(ctx, sa); err != nil && !apierrors.IsNotFound(err) { + return err + } + return nil +} + // serviceMonitorCRDServed reports whether the cluster serves the monitoring.coreos.com // ServiceMonitor kind (i.e. the Prometheus Operator CRDs are installed). func serviceMonitorCRDServed(k8sClient client.Client) bool { diff --git a/internal/state/dcgm_exporter_test.go b/internal/state/dcgm_exporter_test.go index 6b228808d..11a6a2127 100644 --- a/internal/state/dcgm_exporter_test.go +++ b/internal/state/dcgm_exporter_test.go @@ -23,15 +23,19 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/utils/ptr" + "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" nvidiav1 "github.com/NVIDIA/gpu-operator/api/nvidia/v1" nvidiav1alpha1 "github.com/NVIDIA/gpu-operator/api/nvidia/v1alpha1" + "github.com/NVIDIA/gpu-operator/internal/consts" ) const dcgmExporterManifestDir = "../../manifests/state-dcgm-exporter" @@ -59,6 +63,26 @@ func newTestDCGMExporterState(t *testing.T, serviceMonitorCRD bool) *configurabl } // exporterCR returns a sample CR with dcgm-exporter enabled and the given exporter spec. +// newTestDCGMExporterStateWithObjects builds the state with a client that already holds +// the given objects, for the ServiceAccount checks that read cluster state. +func newTestDCGMExporterStateWithObjects(t *testing.T, objs ...client.Object) *configurableState { + t.Helper() + t.Setenv("DCGM_EXPORTER_IMAGE", "nvcr.io/nvidia/k8s/dcgm-exporter:test") + + testScheme := runtime.NewScheme() + require.NoError(t, corev1.AddToScheme(testScheme)) + require.NoError(t, nvidiav1alpha1.AddToScheme(testScheme)) + + k8sClient := fake.NewClientBuilder(). + WithScheme(testScheme). + WithRESTMapper(restMapperWithServiceMonitor(false)). + WithObjects(objs...). + Build() + s, err := NewStateDCGMExporter(k8sClient, "test-operator", testScheme, dcgmExporterManifestDir) + require.NoError(t, err) + return s.(*configurableState) +} + func exporterCR(spec *nvidiav1.DCGMExporterSpec) *nvidiav1alpha1.GPUCluster { cr := sampleGPUCluster() cr.Spec.DCGMExporter = spec @@ -235,3 +259,405 @@ 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"}, + } +} + +// TestDCGMExporterServiceAccountValidation covers the preSync hook, which only rejects a +// configuration the manifests cannot express. It never mutates cluster state -- reclaiming +// what a previous configuration left behind happens in the postSync hook, once the +// operands stopped referencing it. +func TestDCGMExporterServiceAccountValidation(t *testing.T) { + ctx := context.Background() + const ( + customName = "metrics-identity" + byoName = "byo-sa" + ) + + testCases := map[string]struct { + serviceAccount *nvidiav1.DCGMExporterServiceAccountConfig + existing func(cr *nvidiav1alpha1.GPUCluster) []client.Object + // expectedError is a substring of the error the hook must return; empty accepts. + expectedError string + }{ + "the default configuration needs nothing to exist": {}, + "create=false requires the ServiceAccount to exist": { + serviceAccount: &nvidiav1.DCGMExporterServiceAccountConfig{Name: byoName, Create: new(false)}, + expectedError: byoName, + }, + "create=false accepts an existing ServiceAccount": { + serviceAccount: &nvidiav1.DCGMExporterServiceAccountConfig{Name: byoName, Create: new(false)}, + existing: func(*nvidiav1alpha1.GPUCluster) []client.Object { + return []client.Object{unownedServiceAccount(byoName)} + }, + }, + "a configured name refuses to take over an unowned ServiceAccount": { + serviceAccount: &nvidiav1.DCGMExporterServiceAccountConfig{Name: customName}, + existing: func(*nvidiav1alpha1.GPUCluster) []client.Object { + return []client.Object{unownedServiceAccount(customName)} + }, + expectedError: "not managed by this GPUCluster", + }, + "a configured name accepts the ServiceAccount it already owns": { + serviceAccount: &nvidiav1.DCGMExporterServiceAccountConfig{Name: customName}, + existing: func(cr *nvidiav1alpha1.GPUCluster) []client.Object { + return []client.Object{ownedServiceAccount(cr, customName)} + }, + }, + "renaming leaves the superseded default for the postSync reclaim": { + serviceAccount: &nvidiav1.DCGMExporterServiceAccountConfig{Name: customName}, + existing: func(cr *nvidiav1alpha1.GPUCluster) []client.Object { + return []client.Object{ownedServiceAccount(cr, dcgmExporterDefaultServiceAccountName)} + }, + }, + } + + for name, tc := range testCases { + t.Run(name, func(t *testing.T) { + cr := exporterCR(&nvidiav1.DCGMExporterSpec{ServiceAccount: tc.serviceAccount}) + var objs []client.Object + if tc.existing != nil { + objs = tc.existing(cr) + } + s := newTestDCGMExporterStateWithObjects(t, objs...) + + err := checkDCGMExporterServiceAccount(ctx, s, cr) + if tc.expectedError != "" { + require.ErrorContains(t, err, tc.expectedError) + return + } + require.NoError(t, err) + + // Validation only: whatever was there stays there. + for _, obj := range objs { + _, getErr := s.getServiceAccount(ctx, obj.GetName()) + require.NoError(t, getErr, "the preSync hook must not remove %q", obj.GetName()) + } + }) + } +} + +// TestDCGMExporterServiceAccountOwnershipReconcile covers the postSync hook: it runs after +// the manifests converged, so the operands already reference the new ServiceAccount and +// the superseded one can be reclaimed. +func TestDCGMExporterServiceAccountOwnershipReconcile(t *testing.T) { + ctx := context.Background() + const ( + customName = "metrics-identity" + byoName = "byo-sa" + ) + + testCases := map[string]struct { + serviceAccount *nvidiav1.DCGMExporterServiceAccountConfig + existing func(cr *nvidiav1alpha1.GPUCluster) []client.Object + // deleted names the ServiceAccounts that must be gone afterwards, kept those that + // must survive, and released those that must survive without operator ownership. + deleted []string + kept []string + released []string + }{ + "the default configuration reclaims nothing": { + existing: func(cr *nvidiav1alpha1.GPUCluster) []client.Object { + return []client.Object{ownedServiceAccount(cr, dcgmExporterDefaultServiceAccountName)} + }, + kept: []string{dcgmExporterDefaultServiceAccountName}, + }, + "renaming reclaims the superseded operator-owned default": { + serviceAccount: &nvidiav1.DCGMExporterServiceAccountConfig{Name: customName}, + existing: func(cr *nvidiav1alpha1.GPUCluster) []client.Object { + return []client.Object{ + ownedServiceAccount(cr, dcgmExporterDefaultServiceAccountName), + ownedServiceAccount(cr, customName), + } + }, + deleted: []string{dcgmExporterDefaultServiceAccountName}, + kept: []string{customName}, + }, + "renaming keeps a previous ServiceAccount the operator does not own": { + serviceAccount: &nvidiav1.DCGMExporterServiceAccountConfig{Name: customName}, + existing: func(*nvidiav1alpha1.GPUCluster) []client.Object { + return []client.Object{unownedServiceAccount(dcgmExporterDefaultServiceAccountName)} + }, + kept: []string{dcgmExporterDefaultServiceAccountName}, + }, + "create=false reclaims the operator-owned default": { + serviceAccount: &nvidiav1.DCGMExporterServiceAccountConfig{Name: byoName, Create: new(false)}, + existing: func(cr *nvidiav1alpha1.GPUCluster) []client.Object { + return []client.Object{ + ownedServiceAccount(cr, dcgmExporterDefaultServiceAccountName), + unownedServiceAccount(byoName), + } + }, + deleted: []string{dcgmExporterDefaultServiceAccountName}, + kept: []string{byoName}, + }, + "create=false releases a ServiceAccount the operator used to own": { + serviceAccount: &nvidiav1.DCGMExporterServiceAccountConfig{ + Name: dcgmExporterDefaultServiceAccountName, Create: new(false), + }, + existing: func(cr *nvidiav1alpha1.GPUCluster) []client.Object { + return []client.Object{ownedServiceAccount(cr, dcgmExporterDefaultServiceAccountName)} + }, + released: []string{dcgmExporterDefaultServiceAccountName}, + }, + "create=false on a ServiceAccount that was never owned changes nothing": { + serviceAccount: &nvidiav1.DCGMExporterServiceAccountConfig{Name: byoName, Create: new(false)}, + existing: func(*nvidiav1alpha1.GPUCluster) []client.Object { + return []client.Object{unownedServiceAccount(byoName)} + }, + released: []string{byoName}, + }, + } + + for name, tc := range testCases { + t.Run(name, func(t *testing.T) { + cr := exporterCR(&nvidiav1.DCGMExporterSpec{ServiceAccount: tc.serviceAccount}) + s := newTestDCGMExporterStateWithObjects(t, tc.existing(cr)...) + + require.NoError(t, reconcileDCGMExporterServiceAccountOwnership(ctx, s, cr)) + + for _, saName := range tc.deleted { + _, err := s.getServiceAccount(ctx, saName) + require.True(t, apierrors.IsNotFound(err), "%q must be reclaimed", saName) + } + for _, saName := range tc.kept { + _, err := s.getServiceAccount(ctx, saName) + require.NoError(t, err, "%q must not be reclaimed", saName) + } + for _, saName := range tc.released { + sa, err := s.getServiceAccount(ctx, saName) + require.NoError(t, err, "a user-provided ServiceAccount must never be deleted") + assert.False(t, metav1.IsControlledBy(sa, cr), + "the owner reference has to go, otherwise the user's ServiceAccount is garbage-collected with the GPUCluster") + assert.NotContains(t, sa.Labels, consts.StateLabel, + "the state label has to go, otherwise the state cleanup sweeps the user's ServiceAccount") + } + }) + } +} + +// 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 := func(kind, name string, refs []metav1.OwnerReference) *unstructured.Unstructured { + obj := &unstructured.Unstructured{} + obj.SetKind(kind) + obj.SetName(name) + obj.SetNamespace("test-operator") + obj.SetOwnerReferences(refs) + return obj + } + ourRef := []metav1.OwnerReference{{ + APIVersion: nvidiav1alpha1.SchemeGroupVersion.String(), + Kind: "GPUCluster", + Name: cr.Name, + UID: cr.UID, + Controller: new(true), + }} + foreignRef := []metav1.OwnerReference{{ + APIVersion: "apps/v1", + Kind: "Deployment", + Name: "somebody-else", + UID: "other-uid", + Controller: new(true), + }} + + testCases := map[string]struct { + current *unstructured.Unstructured + expectedError string + }{ + "another kind is not this guard's business": { + current: current("ConfigMap", "metrics-identity", foreignRef), + }, + "a ServiceAccount this CR already controls is ours to update": { + current: current("ServiceAccount", "metrics-identity", ourRef), + }, + "a ServiceAccount somebody else controls is refused": { + current: current("ServiceAccount", "metrics-identity", foreignRef), + expectedError: "not managed by this GPUCluster", + }, + "an unowned ServiceAccount under a configured name is refused": { + current: current("ServiceAccount", "metrics-identity", nil), + expectedError: "not managed by this GPUCluster", + }, + "the operator default without owner references stays adoptable": { + // It predates owner references, i.e. an upgrade from an older release. + current: current("ServiceAccount", dcgmExporterDefaultServiceAccountName, nil), + }, + "the operator default somebody else controls is refused": { + current: current("ServiceAccount", dcgmExporterDefaultServiceAccountName, foreignRef), + expectedError: "not managed by this GPUCluster", + }, + } + + 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 this GPUCluster") + + // The object the guard refused must be left exactly as it was found. + found, err := (&configurableState{stateSkel: *skel}).getServiceAccount(ctx, "metrics-identity") + require.NoError(t, err) + assert.Empty(t, found.OwnerReferences) + assert.NotContains(t, found.Labels, consts.StateLabel) +} diff --git a/internal/state/driver.go b/internal/state/driver.go index 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.