diff --git a/api/nvidia/v1alpha1/gpucluster_types.go b/api/nvidia/v1alpha1/gpucluster_types.go index 1d63db509d..5b6836213e 100644 --- a/api/nvidia/v1alpha1/gpucluster_types.go +++ b/api/nvidia/v1alpha1/gpucluster_types.go @@ -26,11 +26,6 @@ const ( GPUClusterCRDName = "GPUCluster" ) -const ( - // Ignored marks a duplicate GPUCluster that the singleton controller does not reconcile. - Ignored State = "ignored" -) - // NOTE: json tags are required. Any new fields you add must have json tags for the fields to be serialized. // GPUClusterSpec defines the desired state of GPUCluster, the DRA-based @@ -170,7 +165,7 @@ type HostPathsSpec struct { // GPUClusterStatus defines the observed state of GPUCluster type GPUClusterStatus struct { - // +kubebuilder:validation:Enum=ignored;ready;notReady;disabled + // +kubebuilder:validation:Enum=ready;notReady;disabled // State indicates the status of the GPUCluster instance State State `json:"state"` // Namespace indicates the namespace in which the operator and operands are installed @@ -186,6 +181,7 @@ type GPUClusterStatus struct { //+kubebuilder:resource:scope=Cluster,shortName={"gc"} //+kubebuilder:printcolumn:name="Status",type=string,JSONPath=`.status.state`,priority=0 //+kubebuilder:printcolumn:name="Age",type=date,JSONPath=`.metadata.creationTimestamp`,priority=0 +//+kubebuilder:validation:XValidation:rule="self.metadata.name == 'gpu-cluster'",message="GPUCluster is a singleton, metadata.name must be 'gpu-cluster'" // GPUCluster is the Schema for the gpuclusters API type GPUCluster struct { diff --git a/bundle/manifests/nvidia.com_gpuclusters.yaml b/bundle/manifests/nvidia.com_gpuclusters.yaml index 66b59a6364..4612cd0356 100644 --- a/bundle/manifests/nvidia.com_gpuclusters.yaml +++ b/bundle/manifests/nvidia.com_gpuclusters.yaml @@ -1027,7 +1027,6 @@ spec: state: description: State indicates the status of the GPUCluster instance enum: - - ignored - ready - notReady - disabled @@ -1036,6 +1035,9 @@ spec: - state type: object type: object + x-kubernetes-validations: + - message: GPUCluster is a singleton, metadata.name must be 'gpu-cluster' + rule: self.metadata.name == 'gpu-cluster' served: true storage: true subresources: diff --git a/config/crd/bases/nvidia.com_gpuclusters.yaml b/config/crd/bases/nvidia.com_gpuclusters.yaml index 66b59a6364..4612cd0356 100644 --- a/config/crd/bases/nvidia.com_gpuclusters.yaml +++ b/config/crd/bases/nvidia.com_gpuclusters.yaml @@ -1027,7 +1027,6 @@ spec: state: description: State indicates the status of the GPUCluster instance enum: - - ignored - ready - notReady - disabled @@ -1036,6 +1035,9 @@ spec: - state type: object type: object + x-kubernetes-validations: + - message: GPUCluster is a singleton, metadata.name must be 'gpu-cluster' + rule: self.metadata.name == 'gpu-cluster' served: true storage: true subresources: diff --git a/config/samples/nvidia_v1alpha1_gpucluster.yaml b/config/samples/nvidia_v1alpha1_gpucluster.yaml index 6749b7959b..fc99088dd2 100644 --- a/config/samples/nvidia_v1alpha1_gpucluster.yaml +++ b/config/samples/nvidia_v1alpha1_gpucluster.yaml @@ -1,7 +1,7 @@ apiVersion: nvidia.com/v1alpha1 kind: GPUCluster metadata: - name: gpucluster-sample + name: gpu-cluster spec: draDriver: repository: registry.k8s.io/dra-driver-nvidia diff --git a/controllers/active_config.go b/controllers/active_config.go index 3ae1bb9b8f..4489df933f 100644 --- a/controllers/active_config.go +++ b/controllers/active_config.go @@ -27,17 +27,39 @@ import ( "github.com/NVIDIA/gpu-operator/internal/consts" ) -// TODO: with multiple CRs of a kind, the tie-breaker is list order, which is not -// guaranteed. Resolve the singleton the reconcilers actually selected instead. -func resolveActiveConfig(ctx context.Context, c client.Client) (*gpuv1.ClusterPolicy, *nvidiav1alpha1.GPUCluster, error) { - var clusterPolicy *gpuv1.ClusterPolicy +// getSingletonClusterPolicy returns the ClusterPolicy treated as the cluster-wide +// singleton: oldest creationTimestamp first, lowest name as the tie-breaker. This +// selection is a heuristic: ideally the operator would hold a reference to the +// singleton ClusterPolicy globally throughout its lifetime, and until it does, +// picking the oldest is the next-best approach. Unlike a first-reconciled-wins claim +// held in controller memory, the result is stable across operator restarts and +// derivable by every controller. Returns nil for an empty list. +// GPUCluster needs no selection: its CRD pins metadata.name, enforcing the singleton +// at admission. +func getSingletonClusterPolicy(items []gpuv1.ClusterPolicy) *gpuv1.ClusterPolicy { + var active *gpuv1.ClusterPolicy + for i := range items { + candidate := &items[i] + if active == nil { + active = candidate + continue + } + candidateCreated, activeCreated := candidate.CreationTimestamp, active.CreationTimestamp + if candidateCreated.Before(&activeCreated) || + (candidateCreated.Equal(&activeCreated) && candidate.Name < active.Name) { + active = candidate + } + } + return active +} + +// resolveActiveConfig returns the active ClusterPolicy, selected with the same +// getSingletonClusterPolicy rule the ClusterPolicy controller uses, and the GPUCluster. +func resolveActiveConfig(ctx context.Context, c client.Reader) (*gpuv1.ClusterPolicy, *nvidiav1alpha1.GPUCluster, error) { clusterPolicies := &gpuv1.ClusterPolicyList{} if err := c.List(ctx, clusterPolicies); err != nil { return nil, nil, fmt.Errorf("failed to list ClusterPolicy: %w", err) } - if len(clusterPolicies.Items) > 0 { - clusterPolicy = &clusterPolicies.Items[0] - } var gpuCluster *nvidiav1alpha1.GPUCluster gpuClusters := &nvidiav1alpha1.GPUClusterList{} @@ -48,7 +70,7 @@ func resolveActiveConfig(ctx context.Context, c client.Client) (*gpuv1.ClusterPo gpuCluster = &gpuClusters.Items[0] } - return clusterPolicy, gpuCluster, nil + return getSingletonClusterPolicy(clusterPolicies.Items), gpuCluster, nil } // resolveDefaultMode returns the nvidia.com/gpu-operator.resource-allocation.mode value for a GPU node that diff --git a/controllers/active_config_test.go b/controllers/active_config_test.go index 35f706d66c..19946fde93 100644 --- a/controllers/active_config_test.go +++ b/controllers/active_config_test.go @@ -20,6 +20,7 @@ import ( "context" "errors" "testing" + "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -76,6 +77,21 @@ func TestResolveActiveConfig(t *testing.T) { assert.Equal(t, "cluster-config", gc.Name) }) + t.Run("oldest ClusterPolicy is the singleton, name breaks ties", func(t *testing.T) { + oldTime := metav1.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC) + zOlder := &gpuv1.ClusterPolicy{ObjectMeta: metav1.ObjectMeta{Name: "z-older", CreationTimestamp: oldTime}} + bOlder := &gpuv1.ClusterPolicy{ObjectMeta: metav1.ObjectMeta{Name: "b-older", CreationTimestamp: oldTime}} + aNewer := &gpuv1.ClusterPolicy{ObjectMeta: metav1.ObjectMeta{ + Name: "a-newer", CreationTimestamp: metav1.Date(2026, 1, 2, 0, 0, 0, 0, time.UTC)}} + c := fake.NewClientBuilder().WithScheme(scheme). + WithObjects(zOlder, bOlder, aNewer).Build() + + cp, _, err := resolveActiveConfig(context.Background(), c) + require.NoError(t, err) + require.NotNil(t, cp) + assert.Equal(t, "b-older", cp.Name) + }) + t.Run("neither CR present returns all nil", func(t *testing.T) { c := fake.NewClientBuilder().WithScheme(scheme).Build() diff --git a/controllers/clusterpolicy_controller.go b/controllers/clusterpolicy_controller.go index 98ea9b29b8..b89cbe3354 100644 --- a/controllers/clusterpolicy_controller.go +++ b/controllers/clusterpolicy_controller.go @@ -117,9 +117,14 @@ func (r *ClusterPolicyReconciler) Reconcile(ctx context.Context, req ctrl.Reques return reconcile.Result{}, err } - // TODO: Handle deletion of the main ClusterPolicy and cycle to the next one. - // We already have a main Clusterpolicy - if clusterPolicyCtrl.singleton != nil && clusterPolicyCtrl.singleton.Name != instance.Name { + // The ClusterPolicy with the oldest creationTimestamp (name as tie-breaker) is + // treated as the singleton, so the choice is stable across operator restarts and + // derivable by other controllers (see getSingletonClusterPolicy). + clusterPolicies := &gpuv1.ClusterPolicyList{} + if err := r.List(ctx, clusterPolicies); err != nil { + return ctrl.Result{}, fmt.Errorf("error listing ClusterPolicy objects: %w", err) + } + if active := getSingletonClusterPolicy(clusterPolicies.Items); active != nil && active.Name != instance.Name { instance.SetStatus(gpuv1.Ignored, clusterPolicyCtrl.operatorNamespace) // do not change `clusterPolicyCtrl.operatorMetrics.reconciliationStatus` here, // spurious reconciliation diff --git a/controllers/clusterpolicy_controller_test.go b/controllers/clusterpolicy_controller_test.go index dfa6f85e13..d50f3e4e90 100644 --- a/controllers/clusterpolicy_controller_test.go +++ b/controllers/clusterpolicy_controller_test.go @@ -419,6 +419,31 @@ func clusterPolicyForUpgradeTest(useNvidiaDriverCRD bool) *gpuv1.ClusterPolicy { } } +// The ClusterPolicy with the oldest creationTimestamp is the singleton regardless of +// reconcile order; any other instance is skipped without running any states. +func TestClusterPolicyReconcileSkipsNonSingleton(t *testing.T) { + older := clusterPolicyForUpgradeTest(true) + older.Name = "older" + older.CreationTimestamp = metav1.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC) + r, c, _ := newClusterPolicyUpgradeTestReconciler(t, older) + + newer := clusterPolicyForUpgradeTest(true) + newer.Name = "newer" + newer.CreationTimestamp = metav1.Date(2026, 1, 2, 0, 0, 0, 0, time.UTC) + require.NoError(t, c.Create(t.Context(), newer)) + + // The newer instance reconciles first but is not the singleton: no error, no + // requeue, and no state is persisted since its states never run. + result, err := r.Reconcile(t.Context(), ctrl.Request{NamespacedName: client.ObjectKeyFromObject(newer)}) + require.NoError(t, err) + require.Zero(t, result) + require.Empty(t, clusterPolicyState(t, c, newer.Name)) + + _, err = r.Reconcile(t.Context(), ctrl.Request{NamespacedName: client.ObjectKeyFromObject(older)}) + require.NoError(t, err) + require.Equal(t, gpuv1.Ready, clusterPolicyState(t, c, older.Name)) +} + func newClusterPolicyUpgradeTestReconciler(t *testing.T, cp *gpuv1.ClusterPolicy, nodes ...*corev1.Node) (*ClusterPolicyReconciler, client.Client, *OperatorMetrics) { t.Helper() scheme := runtime.NewScheme() diff --git a/controllers/gpucluster_controller.go b/controllers/gpucluster_controller.go index 0c14736107..d63df64b51 100644 --- a/controllers/gpucluster_controller.go +++ b/controllers/gpucluster_controller.go @@ -66,10 +66,6 @@ type GPUClusterReconciler struct { stateManager state.Manager conditionUpdater conditions.Updater recorder events.EventRecorder - - // singleton is the GPUCluster that owns reconciliation; the first instance to - // reconcile claims it (first-wins), mirroring ClusterPolicy. - singleton *nvidiav1alpha1.GPUCluster } //+kubebuilder:rbac:groups=nvidia.com,resources=gpuclusters,verbs=get;list;watch;create;update;patch;delete @@ -108,18 +104,8 @@ func (r *GPUClusterReconciler) Reconcile(ctx context.Context, req ctrl.Request) // stack): every operand DaemonSet of both stacks gates on the per-node // nvidia.com/gpu-operator.resource-allocation.mode label, so each node is served by exactly one stack. - // Singleton, first-wins (mirroring ClusterPolicy): the first instance to reconcile - // claims ownership; any other instance is marked Ignored and skipped. The owner is - // held in memory, so the choice resets on operator restart. - if r.singleton != nil && r.singleton.Name != instance.Name { - logger.V(consts.LogLevelWarning).Info("Multiple GPUCluster instances found, ignoring this one", - "name", instance.Name, "owner", r.singleton.Name) - if err := r.updateCRStatus(ctx, instance, nvidiav1alpha1.Ignored); err != nil { - return ctrl.Result{}, err - } - return ctrl.Result{}, nil - } - r.singleton = instance + // No singleton claim is needed: the CRD's CEL rule pins metadata.name, so at most + // one GPUCluster can exist. // DRA requires all driver management through NVIDIADriver CRs: surface an unmet // prerequisite on this CR's status and hold off deploying operands until it is met. @@ -185,12 +171,10 @@ func (r *GPUClusterReconciler) validatePrerequisites(ctx context.Context) (strin if err := r.List(ctx, clusterPolicies); err != nil { return "", fmt.Errorf("error listing ClusterPolicy objects: %w", err) } - // TODO: check only the active singleton ClusterPolicy once the singleton - // selection is resolvable across controllers (see resolveActiveConfig). - for _, clusterPolicy := range clusterPolicies.Items { - if !clusterPolicy.Spec.Driver.UseNvidiaDriverCRDType() { - return fmt.Sprintf("ClusterPolicy %s does not have driver.useNvidiaDriverCRD enabled; migrate driver management to NVIDIADriver CRs before enabling DRA", clusterPolicy.Name), nil - } + // Only the active singleton ClusterPolicy matters here: an Ignored instance + // deploys nothing, so it cannot own driver daemonsets. + if active := getSingletonClusterPolicy(clusterPolicies.Items); active != nil && !active.Spec.Driver.UseNvidiaDriverCRDType() { + return fmt.Sprintf("ClusterPolicy %s does not have driver.useNvidiaDriverCRD enabled; migrate driver management to NVIDIADriver CRs before enabling DRA", active.Name), nil } return "", nil } diff --git a/controllers/gpucluster_controller_test.go b/controllers/gpucluster_controller_test.go index 2351c79ff3..cfd82ad36b 100644 --- a/controllers/gpucluster_controller_test.go +++ b/controllers/gpucluster_controller_test.go @@ -267,37 +267,6 @@ func TestGPUClusterClusterPolicyDriverPrerequisite(t *testing.T) { require.Equal(t, nvidiav1alpha1.Ready, gccState(t, c, cfg.Name)) } -// First-reconciled wins (mirroring ClusterPolicy): whichever instance reconciles first -// claims ownership, regardless of name or creationTimestamp. -func TestGPUClusterSingleton(t *testing.T) { - first := &nvidiav1alpha1.GPUCluster{ObjectMeta: metav1.ObjectMeta{Name: "first"}} - second := &nvidiav1alpha1.GPUCluster{ObjectMeta: metav1.ObjectMeta{Name: "second"}} - r, c := newGPUClusterReconciler(t, first, second) - - gccReconcile(t, r, first.Name) - require.Equal(t, nvidiav1alpha1.Ready, gccState(t, c, first.Name)) - - gccReconcile(t, r, second.Name) - require.Equal(t, nvidiav1alpha1.Ignored, gccState(t, c, second.Name)) -} - -// Matching ClusterPolicy, an ignored duplicate carries no status condition. -func TestGPUClusterDuplicateNoCondition(t *testing.T) { - owner := &nvidiav1alpha1.GPUCluster{ObjectMeta: metav1.ObjectMeta{Name: "owner"}} - duplicate := &nvidiav1alpha1.GPUCluster{ObjectMeta: metav1.ObjectMeta{Name: "duplicate"}} - r, c := newGPUClusterReconciler(t, owner, duplicate) - r.conditionUpdater = conditions.NewGPUClusterUpdater(c) - - gccReconcile(t, r, owner.Name) // owner reconciles first, claiming ownership - gccReconcile(t, r, duplicate.Name) // duplicate is ignored - - instance := &nvidiav1alpha1.GPUCluster{} - require.NoError(t, c.Get(t.Context(), types.NamespacedName{Name: duplicate.Name}, instance)) - require.Equal(t, nvidiav1alpha1.Ignored, instance.Status.State) - require.Nil(t, meta.FindStatusCondition(instance.Status.Conditions, conditions.Error)) - require.Nil(t, meta.FindStatusCondition(instance.Status.Conditions, conditions.Ready)) -} - func TestEnqueueAllGPUClusters(t *testing.T) { r, _ := newGPUClusterReconciler(t, &nvidiav1alpha1.GPUCluster{ObjectMeta: metav1.ObjectMeta{Name: "config-a"}}, diff --git a/deployments/gpu-operator/crds/nvidia.com_gpuclusters.yaml b/deployments/gpu-operator/crds/nvidia.com_gpuclusters.yaml index 66b59a6364..4612cd0356 100644 --- a/deployments/gpu-operator/crds/nvidia.com_gpuclusters.yaml +++ b/deployments/gpu-operator/crds/nvidia.com_gpuclusters.yaml @@ -1027,7 +1027,6 @@ spec: state: description: State indicates the status of the GPUCluster instance enum: - - ignored - ready - notReady - disabled @@ -1036,6 +1035,9 @@ spec: - state type: object type: object + x-kubernetes-validations: + - message: GPUCluster is a singleton, metadata.name must be 'gpu-cluster' + rule: self.metadata.name == 'gpu-cluster' served: true storage: true subresources: