Complete the DCGM Exporter ServiceAccount support on the GPUCluster path

Review feedback: the first commit wired the field on the ClusterPolicy path only,
leaving the DRA path able to render the reference without honouring the contract
behind it.

- pass the field through deployments/gpu-operator/templates/gpucluster.yaml, so a
  Helm GPUCluster install can set it;
- add a preSync hook to configurableState and use it for the exporter, so
  create: false reports NotReady when the ServiceAccount is missing rather than
  leaving the DaemonSet pending on an object the manifests deliberately omit;
- refuse to take over a ServiceAccount that already exists under a configured name
  and is not owned by the CR. createOrUpdateObjs() would otherwise adopt it on the
  DRA path and hand it to garbage collection with the GPUCluster. The default name
  stays tolerant so an upgrade that lost the owner reference keeps converging;
- reclaim the operator-owned default ServiceAccount once a different name takes
  over, on both paths. Renaming between two custom names is not tracked, so only
  the default is reclaimed;
- cover the GPUCluster path: the rendered ServiceAccount, DaemonSet and RBAC
  subjects for a custom name, the omitted ServiceAccount for create: false, and
  each preSync branch.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UiFVTVAyEMmn68Xvo9LSTf
Signed-off-by: 백지명 <wlaud9805@gmail.com>

Author: 100-JM

controllers/object_controls.go

@@ -28,6 +28,7 @@ import (
 	"strconv"
 	"strings"
 
+	"github.com/go-logr/logr"
 	apiconfigv1 "github.com/openshift/api/config/v1"
 	apiimagev1 "github.com/openshift/api/image/v1"
 	secv1 "github.com/openshift/api/security/v1"
@@ -350,6 +351,28 @@ func (n ClusterPolicyController) isServiceAccountOwned(ctx context.Context, obj
 	return metav1.IsControlledBy(found, n.singleton), nil
 }
 
+// deleteOwnedServiceAccount removes a ServiceAccount left behind by a previous
+// configuration, but only when this ClusterPolicy owns it: an object the user
+// provisioned under the same name is left alone.
+func (n ClusterPolicyController) deleteOwnedServiceAccount(ctx context.Context, name string, logger logr.Logger) error {
+	found := &corev1.ServiceAccount{}
+	err := n.client.Get(ctx, types.NamespacedName{Namespace: n.operatorNamespace, Name: name}, found)
+	if err != nil {
+		if apierrors.IsNotFound(err) {
+			return nil
+		}
+		return err
+	}
+	if !metav1.IsControlledBy(found, n.singleton) {
+		return nil
+	}
+	logger.V(1).Info("Removing the superseded dcgm-exporter ServiceAccount", "Name", name)
+	if err := n.client.Delete(ctx, found); err != nil && !apierrors.IsNotFound(err) {
+		return err
+	}
+	return nil
+}
+
 func ServiceAccount(n ClusterPolicyController) (gpuv1.State, error) {
 	ctx := n.ctx
 	state := n.idx
@@ -406,18 +429,41 @@ func ServiceAccount(n ClusterPolicyController) (gpuv1.State, error) {
 		return gpuv1.Ready, nil
 	}
 
+	isRenamed := isDCGMExporter && obj.Name != DCGMExporterDefaultServiceAccountName
+	if isRenamed {
+		// Only a name the user chose can collide with an unrelated object; the default is
+		// left tolerant so an upgrade that lost the owner reference keeps converging.
+		owned, err := n.isServiceAccountOwned(ctx, obj)
+		if err != nil {
+			return gpuv1.NotReady, err
+		}
+		if !owned {
+			err := fmt.Errorf("ServiceAccount %q already exists in namespace %q and is not managed by this ClusterPolicy; "+
+				"set dcgmExporter.serviceAccount.create to false to reference it", obj.Name, obj.Namespace)
+			logger.Error(err, "Refusing to take over an existing ServiceAccount")
+			return gpuv1.NotReady, err
+		}
+	}
+
 	if err := controllerutil.SetControllerReference(n.singleton, obj, n.scheme); err != nil {
 		return gpuv1.NotReady, err
 	}
 
 	if err := n.client.Create(ctx, obj); err != nil {
-		if apierrors.IsAlreadyExists(err) {
-			logger.Info("Found Resource, skipping update")
-			return gpuv1.Ready, nil
+		if !apierrors.IsAlreadyExists(err) {
+			logger.Info("Couldn't create", "Error", err)
+			return gpuv1.NotReady, err
 		}
+		logger.Info("Found Resource, skipping update")
+	}
 
-		logger.Info("Couldn't create", "Error", err)
-		return gpuv1.NotReady, err
+	if isRenamed {
+		// A previous configuration may have left the operator-owned default behind.
+		// Renaming between two custom names is not tracked, so only the default is
+		// reclaimed here.
+		if err := n.deleteOwnedServiceAccount(ctx, DCGMExporterDefaultServiceAccountName, logger); err != nil {
+			return gpuv1.NotReady, err
+		}
 	}
 	return gpuv1.Ready, nil
 }

controllers/object_controls_test.go

@@ -2683,6 +2683,56 @@ func TestDCGMExporterServiceAccountReconcile(t *testing.T) {
 		require.True(t, ok, "only a ServiceAccount owned by the ClusterPolicy may be deleted")
 	})
 
+	t.Run("a configured name refuses to take over an unowned ServiceAccount", func(t *testing.T) {
+		k8s := fake.NewClientBuilder().WithScheme(testScheme).
+			WithObjects(serviceAccount("metrics-identity")).Build()
+		cp := clusterPolicy()
+		cp.Spec.DCGMExporter.ServiceAccount = &gpuv1.DCGMExporterServiceAccountConfig{Name: "metrics-identity"}
+		n := newController(k8s, cp)
+
+		state, err := ServiceAccount(n)
+		require.Error(t, err)
+		require.Equal(t, gpuv1.NotReady, state)
+
+		sa, ok := getServiceAccount(t, k8s, "metrics-identity")
+		require.True(t, ok)
+		require.Empty(t, sa.OwnerReferences, "an existing ServiceAccount must not be adopted")
+	})
+
+	t.Run("renaming reclaims the superseded operator-owned default", func(t *testing.T) {
+		cp := clusterPolicy()
+		previous := serviceAccount(DCGMExporterDefaultServiceAccountName)
+		require.NoError(t, controllerutil.SetControllerReference(cp, previous, testScheme))
+
+		k8s := fake.NewClientBuilder().WithScheme(testScheme).WithObjects(previous).Build()
+		cp.Spec.DCGMExporter.ServiceAccount = &gpuv1.DCGMExporterServiceAccountConfig{Name: "metrics-identity"}
+		n := newController(k8s, cp)
+
+		state, err := ServiceAccount(n)
+		require.NoError(t, err)
+		require.Equal(t, gpuv1.Ready, state)
+
+		_, ok := getServiceAccount(t, k8s, "metrics-identity")
+		require.True(t, ok)
+		_, ok = getServiceAccount(t, k8s, DCGMExporterDefaultServiceAccountName)
+		require.False(t, ok, "the superseded default must be removed")
+	})
+
+	t.Run("renaming keeps a previous ServiceAccount the operator does not own", func(t *testing.T) {
+		k8s := fake.NewClientBuilder().WithScheme(testScheme).
+			WithObjects(serviceAccount(DCGMExporterDefaultServiceAccountName)).Build()
+		cp := clusterPolicy()
+		cp.Spec.DCGMExporter.ServiceAccount = &gpuv1.DCGMExporterServiceAccountConfig{Name: "metrics-identity"}
+		n := newController(k8s, cp)
+
+		state, err := ServiceAccount(n)
+		require.NoError(t, err)
+		require.Equal(t, gpuv1.Ready, state)
+
+		_, ok := getServiceAccount(t, k8s, DCGMExporterDefaultServiceAccountName)
+		require.True(t, ok, "only an owned ServiceAccount may be reclaimed")
+	})
+
 	t.Run("a non-DCGM state deletes its ServiceAccount regardless of ownership", func(t *testing.T) {
 		// The ownership check is scoped to the DCGM Exporter; every other state keeps
 		// the previous unconditional cleanup on disable.

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 }}

internal/state/configurable_state.go

@@ -49,6 +49,12 @@ type configurableState struct {
 	// image path and DRA apiVersion. It receives ctx and the skeleton so operands that
 	// need the client or logging (e.g. dcgm-exporter's ServiceMonitor CRD probe) can use them.
 	buildRenderData func(ctx context.Context, s *configurableState, cr *nvidiav1alpha1.GPUCluster, imagePath, apiVersion, openshiftVersion string) (any, error)
+
+	// preSync runs after the manifests render but before they are applied, for operands
+	// that depend on cluster state the templates cannot express. Returning an error marks
+	// the state NotReady, so it is the place to surface a misconfiguration instead of
+	// applying objects that cannot converge.
+	preSync func(ctx context.Context, s *configurableState, cr *nvidiav1alpha1.GPUCluster) error
 }
 
 var _ State = (*configurableState)(nil)
@@ -68,6 +74,12 @@ func (s *configurableState) Sync(ctx context.Context, customResource any, infoCa
 		return s.handleStateObjectsDeletion(ctx)
 	}
 
+	if s.preSync != nil {
+		if err := s.preSync(ctx, s, cr); err != nil {
+			return SyncStateNotReady, err
+		}
+	}
+
 	return s.syncObjects(ctx, cr, objs)
 }
 

internal/state/dcgm_exporter.go

@@ -18,11 +18,16 @@ package state
 
 import (
 	"context"
+	"fmt"
 	"path/filepath"
 	"strings"
 
+	corev1 "k8s.io/api/core/v1"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
+	"k8s.io/apimachinery/pkg/types"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/log"
 
@@ -71,6 +76,7 @@ func NewStateDCGMExporter(
 		},
 		imageEnvName:    dcgmExporterImageEnvName,
 		buildRenderData: buildDCGMExporterRenderData,
+		preSync:         checkDCGMExporterServiceAccount,
 	}, nil
 }
 
@@ -149,6 +155,78 @@ func buildDCGMExporterRenderData(ctx context.Context, s *configurableState, cr *
 	}, nil
 }
 
+// checkDCGMExporterServiceAccount reconciles the parts of the ServiceAccount contract the
+// manifests cannot express: a ServiceAccount the user brings has to already exist, one the
+// operator would manage must not be an existing object owned by somebody else, and the
+// operator-owned default is removed once a different name takes over.
+func checkDCGMExporterServiceAccount(ctx context.Context, s *configurableState, cr *nvidiav1alpha1.GPUCluster) error {
+	spec := cr.Spec.DCGMExporter
+	name := spec.GetServiceAccountName(dcgmExporterDefaultServiceAccountName)
+
+	if !spec.IsServiceAccountCreateEnabled() {
+		// The manifests omit the ServiceAccount entirely, so a missing one would leave
+		// the DaemonSet pending without any signal.
+		if _, err := s.getServiceAccount(ctx, name); err != nil {
+			if apierrors.IsNotFound(err) {
+				return fmt.Errorf(
+					"ServiceAccount %q configured with create=false does not exist in namespace %q",
+					name, s.namespace)
+			}
+			return err
+		}
+		return nil
+	}
+
+	if name == dcgmExporterDefaultServiceAccountName {
+		return nil
+	}
+
+	// Adopting an object the operator did not create would hand it to garbage collection
+	// on CR deletion, so a name that is already taken has to be opted into explicitly.
+	existing, err := s.getServiceAccount(ctx, name)
+	if err != nil && !apierrors.IsNotFound(err) {
+		return err
+	}
+	if err == nil && !metav1.IsControlledBy(existing, cr) {
+		return fmt.Errorf(
+			"ServiceAccount %q already exists in namespace %q and is not managed by this GPUCluster; "+
+				"set dcgmExporter.serviceAccount.create to false to reference it",
+			name, s.namespace)
+	}
+
+	return s.deleteOwnedServiceAccount(ctx, cr, dcgmExporterDefaultServiceAccountName)
+}
+
+// getServiceAccount reads a ServiceAccount from the operand namespace.
+func (s *configurableState) getServiceAccount(ctx context.Context, name string) (*corev1.ServiceAccount, error) {
+	sa := &corev1.ServiceAccount{}
+	err := s.client.Get(ctx, types.NamespacedName{Namespace: s.namespace, Name: name}, sa)
+	return sa, err
+}
+
+// deleteOwnedServiceAccount removes a ServiceAccount left behind by a previous
+// configuration, but only when this CR owns it: an object the user provisioned under the
+// same name is left alone. Renaming from one custom name to another is not tracked, so
+// only the operator default is reclaimed here.
+func (s *configurableState) deleteOwnedServiceAccount(ctx context.Context, cr *nvidiav1alpha1.GPUCluster, name string) error {
+	sa, err := s.getServiceAccount(ctx, name)
+	if err != nil {
+		if apierrors.IsNotFound(err) {
+			return nil
+		}
+		return err
+	}
+	if !metav1.IsControlledBy(sa, cr) {
+		return nil
+	}
+	log.FromContext(ctx).V(consts.LogLevelInfo).Info(
+		"Removing the superseded dcgm-exporter ServiceAccount", "Name", name)
+	if err := s.client.Delete(ctx, sa); err != nil && !apierrors.IsNotFound(err) {
+		return err
+	}
+	return nil
+}
+
 // serviceMonitorCRDServed reports whether the cluster serves the monitoring.coreos.com
 // ServiceMonitor kind (i.e. the Prometheus Operator CRDs are installed).
 func serviceMonitorCRDServed(k8sClient client.Client) bool {