diff --git a/controllers/active_config.go b/controllers/active_config.go index 4489df933f..3a89b7b31c 100644 --- a/controllers/active_config.go +++ b/controllers/active_config.go @@ -24,7 +24,6 @@ import ( gpuv1 "github.com/NVIDIA/gpu-operator/api/nvidia/v1" nvidiav1alpha1 "github.com/NVIDIA/gpu-operator/api/nvidia/v1alpha1" - "github.com/NVIDIA/gpu-operator/internal/consts" ) // getSingletonClusterPolicy returns the ClusterPolicy treated as the cluster-wide @@ -72,25 +71,3 @@ func resolveActiveConfig(ctx context.Context, c client.Reader) (*gpuv1.ClusterPo return getSingletonClusterPolicy(clusterPolicies.Items), gpuCluster, nil } - -// resolveDefaultMode returns the nvidia.com/gpu-operator.resource-allocation.mode value for a GPU node that -// does not have one yet. When exactly one configuration CR exists its stack wins; -// envDefaultMode (the validated DEFAULT_GPU_ALLOCATION_MODE operator environment variable) -// is consulted only when both CRs exist, defaulting to device-plugin when unset. Nodes -// already labeled are never touched, so changing DEFAULT_GPU_ALLOCATION_MODE only affects -// nodes labeled afterward. -func resolveDefaultMode(clusterPolicyExists, gpuClusterExists bool, envDefaultMode consts.GPUAllocationMode) consts.GPUAllocationMode { - switch { - case clusterPolicyExists && gpuClusterExists: - if envDefaultMode == consts.GPUAllocationModeDRA { - return consts.GPUAllocationModeDRA - } - return consts.GPUAllocationModeDevicePlugin - case gpuClusterExists: - return consts.GPUAllocationModeDRA - case clusterPolicyExists: - return consts.GPUAllocationModeDevicePlugin - default: - return "" - } -} diff --git a/controllers/active_config_test.go b/controllers/active_config_test.go index 19946fde93..71b3b1567d 100644 --- a/controllers/active_config_test.go +++ b/controllers/active_config_test.go @@ -32,7 +32,6 @@ import ( gpuv1 "github.com/NVIDIA/gpu-operator/api/nvidia/v1" nvidiav1alpha1 "github.com/NVIDIA/gpu-operator/api/nvidia/v1alpha1" - "github.com/NVIDIA/gpu-operator/internal/consts" ) func TestResolveActiveConfig(t *testing.T) { @@ -137,26 +136,3 @@ func TestResolveActiveConfig(t *testing.T) { assert.Nil(t, gc) }) } - -func TestResolveDefaultMode(t *testing.T) { - testCases := []struct { - description string - clusterPolicyExists bool - gpuClusterExists bool - envDefaultMode consts.GPUAllocationMode - expected consts.GPUAllocationMode - }{ - {"both CRs, DEFAULT_GPU_ALLOCATION_MODE=dra", true, true, consts.GPUAllocationModeDRA, consts.GPUAllocationModeDRA}, - {"both CRs, DEFAULT_GPU_ALLOCATION_MODE=device-plugin", true, true, consts.GPUAllocationModeDevicePlugin, consts.GPUAllocationModeDevicePlugin}, - {"both CRs, DEFAULT_GPU_ALLOCATION_MODE unset defaults to device-plugin", true, true, "", consts.GPUAllocationModeDevicePlugin}, - {"only ClusterPolicy ignores DEFAULT_GPU_ALLOCATION_MODE", true, false, consts.GPUAllocationModeDRA, consts.GPUAllocationModeDevicePlugin}, - {"only GPUCluster ignores DEFAULT_GPU_ALLOCATION_MODE", false, true, consts.GPUAllocationModeDevicePlugin, consts.GPUAllocationModeDRA}, - {"neither CR resolves to no mode", false, false, consts.GPUAllocationModeDRA, ""}, - } - for _, tc := range testCases { - t.Run(tc.description, func(t *testing.T) { - mode := resolveDefaultMode(tc.clusterPolicyExists, tc.gpuClusterExists, tc.envDefaultMode) - assert.Equal(t, tc.expected, mode) - }) - } -} diff --git a/controllers/clusterpolicy_controller.go b/controllers/clusterpolicy_controller.go index b89cbe3354..b6fde47516 100644 --- a/controllers/clusterpolicy_controller.go +++ b/controllers/clusterpolicy_controller.go @@ -131,8 +131,25 @@ func (r *ClusterPolicyReconciler) Reconcile(ctx context.Context, req ctrl.Reques return ctrl.Result{}, nil } + // TODO: remove the below code block once both ClusterPolicy and GPUCluster can co-exist + gpuClusters := &nvidiav1alpha1.GPUClusterList{} + if err := r.List(ctx, gpuClusters); err != nil { + return ctrl.Result{}, fmt.Errorf("failed to list GPUCluster objects: %w", err) + } + if len(gpuClusters.Items) > 0 { + err := fmt.Errorf("conflicting GPUCluster resource %q detected; ClusterPolicy and GPUCluster cannot co-exist", gpuClusters.Items[0].Name) + r.Log.Error(err, "only one CR may be present at a time") + updateCRState(ctx, r, req.NamespacedName, gpuv1.NotReady) + if condErr := r.conditionUpdater.SetConditionsError(ctx, instance, conditions.ReconcileFailed, err.Error()); condErr != nil { + r.Log.Error(condErr, "failed to set condition") + } + clusterPolicyCtrl.operatorMetrics.reconciliationStatus.Set(reconciliationStatusClusterPolicyUnavailable) + return ctrl.Result{}, err + } + if err := clusterPolicyCtrl.init(ctx, r, instance); err != nil { r.Log.Error(err, "unable to initialize ClusterPolicy controller") + updateCRState(ctx, r, req.NamespacedName, gpuv1.NotReady) if condErr := r.conditionUpdater.SetConditionsError(ctx, instance, conditions.ReconcileFailed, err.Error()); condErr != nil { r.Log.Error(condErr, "failed to set condition") } @@ -366,16 +383,12 @@ func addWatchNewGPUNode(r *ClusterPolicyReconciler, c controller.Controller, mgr newOSTreeLabel := newLabels[nfdOSTreeVersionLabelKey] osTreeLabelChanged := oldOSTreeLabel != newOSTreeLabel - // The resource-allocation mode label gates rendering of the mode nodeSelector - // on operand DaemonSets, so re-render when it lands or changes. - modeLabelChanged := oldLabels[consts.GPUAllocationModeLabelKey] != newLabels[consts.GPUAllocationModeLabelKey] driverOwnerLabelChanged, driverUpgradeStateLabelChanged, driverUpgradeSkipLabelChanged := driverUpgradeLabelsChanged(oldLabels, newLabels) needsUpdate := gpuCommonLabelAdded || commonOperandsLabelChanged || gpuWorkloadConfigLabelChanged || osTreeLabelChanged || - modeLabelChanged || driverOwnerLabelChanged || driverUpgradeStateLabelChanged || driverUpgradeSkipLabelChanged @@ -387,7 +400,6 @@ func addWatchNewGPUNode(r *ClusterPolicyReconciler, c controller.Controller, mgr "commonOperandsLabelChanged", commonOperandsLabelChanged, "gpuWorkloadConfigLabelChanged", gpuWorkloadConfigLabelChanged, "osTreeLabelChanged", osTreeLabelChanged, - "modeLabelChanged", modeLabelChanged, "driverOwnerLabelChanged", driverOwnerLabelChanged, "driverUpgradeStateLabelChanged", driverUpgradeStateLabelChanged, "driverUpgradeSkipLabelChanged", driverUpgradeSkipLabelChanged, diff --git a/controllers/clusterpolicy_controller_test.go b/controllers/clusterpolicy_controller_test.go index d50f3e4e90..da81bcfc61 100644 --- a/controllers/clusterpolicy_controller_test.go +++ b/controllers/clusterpolicy_controller_test.go @@ -444,6 +444,24 @@ func TestClusterPolicyReconcileSkipsNonSingleton(t *testing.T) { require.Equal(t, gpuv1.Ready, clusterPolicyState(t, c, older.Name)) } +func TestClusterPolicyBlockedByGPUCluster(t *testing.T) { + cp := clusterPolicyForUpgradeTest(true) + r, c, _ := newClusterPolicyUpgradeTestReconciler(t, cp) + + gc := &nvidiav1alpha1.GPUCluster{ObjectMeta: metav1.ObjectMeta{Name: "config"}} + require.NoError(t, c.Create(t.Context(), gc)) + + _, err := r.Reconcile(t.Context(), ctrl.Request{NamespacedName: client.ObjectKeyFromObject(cp)}) + require.Equal(t, gpuv1.NotReady, clusterPolicyState(t, c, cp.Name)) + require.ErrorContains(t, err, "ClusterPolicy and GPUCluster cannot co-exist") + + // Deleting the GPUCluster instance unblocks the next reconcile + require.NoError(t, c.Delete(t.Context(), gc)) + _, err = r.Reconcile(t.Context(), ctrl.Request{NamespacedName: client.ObjectKeyFromObject(cp)}) + require.NoError(t, err) + require.Equal(t, gpuv1.Ready, clusterPolicyState(t, c, cp.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 d63df64b51..36da35d6d4 100644 --- a/controllers/gpucluster_controller.go +++ b/controllers/gpucluster_controller.go @@ -100,15 +100,6 @@ func (r *GPUClusterReconciler) Reconcile(ctx context.Context, req ctrl.Request) return ctrl.Result{}, fmt.Errorf("error adding finalizer to GPUCluster %s: %w", req.NamespacedName, err) } - // GPUCluster (DRA stack) may coexist with a ClusterPolicy (device-plugin - // 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. - - // 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. if msg, err := r.validatePrerequisites(ctx); err != nil { return ctrl.Result{}, err } else if msg != "" { @@ -171,10 +162,9 @@ 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) } - // 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 + // TODO: relax this prerequisite once ClusterPolicy and GPUCluster can co-exist + if clusterPolicy := getSingletonClusterPolicy(clusterPolicies.Items); clusterPolicy != nil { + return fmt.Sprintf("A ClusterPolicy CR %q exists; a ClusterPolicy CR and GPUCluster CR may not exist at the same time", clusterPolicy.Name), nil } return "", nil } diff --git a/controllers/gpucluster_controller_test.go b/controllers/gpucluster_controller_test.go index cfd82ad36b..c80b5ac13c 100644 --- a/controllers/gpucluster_controller_test.go +++ b/controllers/gpucluster_controller_test.go @@ -20,13 +20,11 @@ import ( "context" "sort" "testing" - "time" "github.com/stretchr/testify/require" appsv1 "k8s.io/api/apps/v1" 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/runtime" "k8s.io/apimachinery/pkg/types" @@ -38,7 +36,6 @@ import ( gpuv1 "github.com/NVIDIA/gpu-operator/api/nvidia/v1" nvidiav1alpha1 "github.com/NVIDIA/gpu-operator/api/nvidia/v1alpha1" - "github.com/NVIDIA/gpu-operator/internal/conditions" "github.com/NVIDIA/gpu-operator/internal/state" ) @@ -217,52 +214,17 @@ func TestGPUClusterTeardownDrainsClaimConsumersFirst(t *testing.T) { require.NoError(t, c.Get(t.Context(), types.NamespacedName{Name: plugin.Name, Namespace: "test-namespace"}, ds)) } -// A ClusterPolicy in the cluster does not disable the GPUCluster, provided it -// delegates driver management to NVIDIADriver CRs: the two stacks coexist, with -// per-node ownership decided by the nvidia.com/gpu-operator.resource-allocation.mode label. +// A ClusterPolicy and GPUCluster CR cannot co-exist func TestGPUClusterCoexistsWithClusterPolicy(t *testing.T) { - cfg := &nvidiav1alpha1.GPUCluster{ObjectMeta: metav1.ObjectMeta{Name: "config"}} - cp := &gpuv1.ClusterPolicy{ - ObjectMeta: metav1.ObjectMeta{Name: "cluster-policy"}, - Spec: gpuv1.ClusterPolicySpec{ - Driver: gpuv1.DriverSpec{UseNvidiaDriverCRD: ptr.To(true)}, - }, - } - r, c := newGPUClusterReconciler(t, cfg, cp) - - gccReconcile(t, r, cfg.Name) - - require.Equal(t, nvidiav1alpha1.Ready, gccState(t, c, cfg.Name)) -} - -// A ClusterPolicy that manages its own driver (useNvidiaDriverCRD=false) is an invalid -// companion for DRA: the GPUCluster reports the unmet prerequisite and deploys nothing. -func TestGPUClusterClusterPolicyDriverPrerequisite(t *testing.T) { cfg := &nvidiav1alpha1.GPUCluster{ObjectMeta: metav1.ObjectMeta{Name: "config"}} cp := &gpuv1.ClusterPolicy{ObjectMeta: metav1.ObjectMeta{Name: "cluster-policy"}} r, c := newGPUClusterReconciler(t, cfg, cp) - r.conditionUpdater = conditions.NewGPUClusterUpdater(c) - - res, err := r.Reconcile(t.Context(), gccRequest(cfg.Name)) - require.NoError(t, err) - require.Equal(t, time.Minute, res.RequeueAfter) + gccReconcile(t, r, cfg.Name) require.Equal(t, nvidiav1alpha1.NotReady, gccState(t, c, cfg.Name)) - require.Nil(t, r.stateManager.(*fakeStateManager).lastCatalog, "operands must not be synced") - - instance := &nvidiav1alpha1.GPUCluster{} - require.NoError(t, c.Get(t.Context(), types.NamespacedName{Name: cfg.Name}, instance)) - cond := meta.FindStatusCondition(instance.Status.Conditions, conditions.Error) - require.NotNil(t, cond) - require.Equal(t, conditions.PrerequisiteNotMet, cond.Reason) - require.Contains(t, cond.Message, "useNvidiaDriverCRD") - - // Toggling the flag to true clears the prerequisite on the next reconcile. - updated := &gpuv1.ClusterPolicy{} - require.NoError(t, c.Get(t.Context(), types.NamespacedName{Name: cp.Name}, updated)) - updated.Spec.Driver.UseNvidiaDriverCRD = ptr.To(true) - require.NoError(t, c.Update(t.Context(), updated)) + // Deleting the ClusterPolicy instance satisfies the prerequisites on the next reconcile + require.NoError(t, c.Delete(t.Context(), cp)) gccReconcile(t, r, cfg.Name) require.Equal(t, nvidiav1alpha1.Ready, gccState(t, c, cfg.Name)) } diff --git a/controllers/nodelabeling_controller.go b/controllers/nodelabeling_controller.go index 63bd1d624a..00e13d3fcd 100644 --- a/controllers/nodelabeling_controller.go +++ b/controllers/nodelabeling_controller.go @@ -19,7 +19,6 @@ package controllers import ( "context" "fmt" - "os" "time" "github.com/NVIDIA/k8s-operator-libs/pkg/upgrade" @@ -60,16 +59,14 @@ type NodeLabelingReconciler struct { } // nodeLabelingController holds per-reconcile state so that helper methods don't need to -// re-receive that state as arguments. clusterPolicy drives the device-plugin stack and -// gpuCluster the DRA stack; the two may coexist, with each node served by exactly -// one stack according to its nvidia.com/gpu-operator.resource-allocation.mode label. defaultMode is the mode -// applied to GPU nodes that do not have the label yet. +// re-receive that state as arguments. Exactly one of clusterPolicy (device-plugin stack) +// or gpuCluster (DRA stack) is non-nil per reconcile; Reconcile returns an error if both +// exist simultaneously. type nodeLabelingController struct { client client.Client namespace string clusterPolicy *gpuv1.ClusterPolicy gpuCluster *nvidiav1alpha1.GPUCluster - defaultMode consts.GPUAllocationMode logger logr.Logger // draPluginRemovalDeferred records that gpu.deploy.dra-driver removal was skipped on @@ -93,8 +90,6 @@ type nodeLabelUpdateReasons struct { gpuCommonLabelOutdated bool gpuCommonLabelChanged bool commonOperandsLabelChanged bool - modeLabelMissing bool - modeLabelChanged bool gpuWorkloadConfigChanged bool migCapableLabelChanged bool osTreeLabelChanged bool @@ -107,8 +102,6 @@ func (r nodeLabelUpdateReasons) needsUpdate() bool { r.gpuCommonLabelOutdated || r.gpuCommonLabelChanged || r.commonOperandsLabelChanged || - r.modeLabelMissing || - r.modeLabelChanged || r.gpuWorkloadConfigChanged || r.migCapableLabelChanged || r.osTreeLabelChanged || @@ -125,8 +118,6 @@ func getNodeLabelUpdateReasons(oldLabels, newLabels map[string]string) nodeLabel gpuCommonLabelOutdated: !hasGPULabels(newLabels) && hasCommonGPULabel(newLabels), gpuCommonLabelChanged: oldLabels[commonGPULabelKey] != newLabels[commonGPULabelKey], commonOperandsLabelChanged: hasOperandsDisabled(oldLabels) != hasOperandsDisabled(newLabels), - modeLabelMissing: hasCommonGPULabel(newLabels) && newLabels[consts.GPUAllocationModeLabelKey] == "", - modeLabelChanged: oldLabels[consts.GPUAllocationModeLabelKey] != newLabels[consts.GPUAllocationModeLabelKey], gpuWorkloadConfigChanged: oldGPUWorkloadConfig != newGPUWorkloadConfig, migCapableLabelChanged: hasMIGCapableGPU(oldLabels) != hasMIGCapableGPU(newLabels), osTreeLabelChanged: oldLabels[nfdOSTreeVersionLabelKey] != newLabels[nfdOSTreeVersionLabelKey], @@ -140,8 +131,6 @@ func getNodeLabelUpdateReasons(oldLabels, newLabels map[string]string) nodeLabel func (r *NodeLabelingReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { r.Log.Info("Reconciling node labels") - // The ClusterPolicy (device-plugin stack) and GPUCluster (DRA stack) CRs may - // coexist; neither existing means there is nothing to label. clusterPolicy, gpuCluster, err := resolveActiveConfig(ctx, r.Client) if err != nil { return reconcile.Result{}, err @@ -150,14 +139,9 @@ func (r *NodeLabelingReconciler) Reconcile(ctx context.Context, req ctrl.Request r.Log.Info("No ClusterPolicy or GPUCluster CR exists, skipping node labeling") return reconcile.Result{}, nil } - - envDefaultMode, err := defaultModeFromEnv() - if err != nil { - return reconcile.Result{}, err - } - if clusterPolicy != nil && gpuCluster != nil && envDefaultMode == "" { - r.Log.Info("WARNING: both ClusterPolicy and GPUCluster exist but DEFAULT_GPU_ALLOCATION_MODE is unset; " + - "defaulting new GPU nodes to the device-plugin stack") + // TODO: allow both CRs to co-exist + if clusterPolicy != nil && gpuCluster != nil { + return reconcile.Result{}, fmt.Errorf("both ClusterPolicy and GPUCluster CRs exist; only one may be present at a time") } nlc := &nodeLabelingController{ @@ -165,7 +149,6 @@ func (r *NodeLabelingReconciler) Reconcile(ctx context.Context, req ctrl.Request namespace: r.Namespace, clusterPolicy: clusterPolicy, gpuCluster: gpuCluster, - defaultMode: resolveDefaultMode(clusterPolicy != nil, gpuCluster != nil, envDefaultMode), logger: r.Log, } @@ -192,9 +175,7 @@ func (r *NodeLabelingReconciler) Reconcile(ctx context.Context, req ctrl.Request usesNvidiaDriverCRD := nlc.gpuCluster != nil || (nlc.clusterPolicy != nil && nlc.clusterPolicy.Spec.Driver.UseNvidiaDriverCRDType()) if usesNvidiaDriverCRD { - classicClusterPolicyDriver := nlc.clusterPolicy != nil && - !nlc.clusterPolicy.Spec.Driver.UseNvidiaDriverCRDType() - if _, err := nvidiadriverutil.AssignOwners(ctx, r.Client, classicClusterPolicyDriver); err != nil { + if _, err := nvidiadriverutil.AssignOwners(ctx, r.Client); err != nil { return reconcile.Result{}, fmt.Errorf("failed to assign NVIDIADriver owners to nodes: %w", err) } if err := nlc.labelNodesWithOrphanedDriverPods(ctx); err != nil { @@ -215,21 +196,6 @@ func (r *NodeLabelingReconciler) Reconcile(ctx context.Context, req ctrl.Request return reconcile.Result{}, nil } -// defaultModeFromEnv reads and validates the DEFAULT_GPU_ALLOCATION_MODE operator -// environment variable. Unset yields the empty mode (resolveDefaultMode then falls back -// to device-plugin); a set-but-invalid value is an error. -func defaultModeFromEnv() (consts.GPUAllocationMode, error) { - raw := os.Getenv(consts.DefaultGPUAllocationModeEnvName) - switch mode := consts.GPUAllocationMode(raw); mode { - case "", consts.GPUAllocationModeDevicePlugin, consts.GPUAllocationModeDRA: - return mode, nil - default: - return "", fmt.Errorf("invalid %s environment variable: %q is not one of %q or %q", - consts.DefaultGPUAllocationModeEnvName, raw, - consts.GPUAllocationModeDevicePlugin, consts.GPUAllocationModeDRA) - } -} - // labelGPUNodes reconciles GPU-related labels and reports which node labels were patched. func (nlc *nodeLabelingController) labelGPUNodes(ctx context.Context) (gpuNodeLabelsUpdateResult, error) { result := gpuNodeLabelsUpdateResult{} @@ -242,7 +208,6 @@ func (nlc *nodeLabelingController) labelGPUNodes(ctx context.Context) (gpuNodeLa original := node.DeepCopy() labels := node.GetLabels() gpuDiscoveryStateChanged := false - modeLabelModified := false stateLabelsModified := false if nlc.reconcileCommonGPULabel(labels, node.Name) { @@ -250,17 +215,12 @@ func (nlc *nodeLabelingController) labelGPUNodes(ctx context.Context) (gpuNodeLa gpuDiscoveryStateChanged = true } - if nlc.reconcileModeLabel(labels, node.Name) { - node.SetLabels(labels) - modeLabelModified = true - } - if nlc.updateGPUStateLabels(ctx, labels, node.Name) { node.SetLabels(labels) stateLabelsModified = true } - modified := gpuDiscoveryStateChanged || modeLabelModified || stateLabelsModified + modified := gpuDiscoveryStateChanged || stateLabelsModified if modified { if err := nlc.client.Patch(ctx, &node, client.MergeFrom(original)); err != nil { return result, fmt.Errorf("unable to label node %s: %w", node.Name, err) @@ -290,52 +250,22 @@ func (nlc *nodeLabelingController) reconcileCommonGPULabel(labels map[string]str return false } -// reconcileModeLabel writes nvidia.com/gpu-operator.resource-allocation.mode on GPU nodes that do not have it -// yet. An existing value is never overwritten (or removed), whether set by a previous -// reconcile or manually by a user: changing the cluster configuration or DEFAULT_GPU_ALLOCATION_MODE -// must not migrate nodes that are already serving GPUs through one stack. Returns true if -// labels were modified. -func (nlc *nodeLabelingController) reconcileModeLabel(labels map[string]string, nodeName string) bool { - if !hasCommonGPULabel(labels) { - return false - } - if _, ok := labels[consts.GPUAllocationModeLabelKey]; ok { - return false - } - nlc.logger.Info("Setting GPU Operator mode label", "NodeName", nodeName, - "Label", consts.GPUAllocationModeLabelKey, "Value", nlc.defaultMode) - labels[consts.GPUAllocationModeLabelKey] = string(nlc.defaultMode) - return true -} - // updateGPUStateLabels syncs nvidia.com/gpu.deploy.* labels and sets the MIG config label when -// appropriate. Which label set is applied follows the node's nvidia.com/gpu-operator.resource-allocation.mode -// label; deploy labels exclusive to the other stack are swept away, while shared and -// unrecognized deploy labels are left alone. If the node does not have the common GPU -// label, all state labels are removed. Returns true if labels were modified. +// appropriate. Which label set is applied follows which CR is active (gpuCluster → DRA stack, +// clusterPolicy → device-plugin stack); deploy labels exclusive to the other stack are swept +// away, while shared and unrecognized deploy labels are left alone. If the node does not have +// the common GPU label, all state labels are removed. Returns true if labels were modified. func (nlc *nodeLabelingController) updateGPUStateLabels(ctx context.Context, labels map[string]string, nodeName string) bool { if !hasCommonGPULabel(labels) { return removeAllGPUStateLabels(labels) } - switch consts.GPUAllocationMode(labels[consts.GPUAllocationModeLabelKey]) { - case consts.GPUAllocationModeDRA: - if nlc.gpuCluster == nil { - return false - } + if nlc.gpuCluster != nil { // Sweep only the device-plugin stack's exclusive keys so k8s-driver-manager // pause state on the DRA stack's own keys survives. sweptPreviousStack := nlc.removeLabelsFromNode(labels, devicePluginOnlyStateLabelKeys(), nodeName) appliedStackLabels := updateGPUClusterStateLabels(labels) return sweptPreviousStack || appliedStackLabels - case consts.GPUAllocationModeDevicePlugin: - if nlc.clusterPolicy == nil { - return false - } - default: - // Unlabeled (or unrecognized mode): apply no deploy labels, which keeps the node - // empty of operands until a mode is set. - return false } cp := nlc.clusterPolicy @@ -766,8 +696,6 @@ func (r *NodeLabelingReconciler) SetupWithManager(ctx context.Context, mgr ctrl. "gpuCommonLabelOutdated", reasons.gpuCommonLabelOutdated, "gpuCommonLabelChanged", reasons.gpuCommonLabelChanged, "commonOperandsLabelChanged", reasons.commonOperandsLabelChanged, - "modeLabelMissing", reasons.modeLabelMissing, - "modeLabelChanged", reasons.modeLabelChanged, "gpuWorkloadConfigLabelChanged", reasons.gpuWorkloadConfigChanged, "migCapableLabelChanged", reasons.migCapableLabelChanged, "osTreeLabelChanged", reasons.osTreeLabelChanged, diff --git a/controllers/nodelabeling_controller_test.go b/controllers/nodelabeling_controller_test.go index 8ec98d6d21..affe51fe5a 100644 --- a/controllers/nodelabeling_controller_test.go +++ b/controllers/nodelabeling_controller_test.go @@ -561,69 +561,15 @@ func TestUpdateGPUStateLabels(t *testing.T) { clusterPolicy: tc.clusterPolicy, logger: logr.Discard(), } - // The ClusterPolicy workload-config logic only applies to nodes owned by the - // device-plugin stack, so GPU nodes carry the corresponding mode label. labels := mergeLabels(tc.initialLabels) expectedLabels := mergeLabels(tc.expectedLabels) - if hasCommonGPULabel(labels) { - labels[consts.GPUAllocationModeLabelKey] = string(consts.GPUAllocationModeDevicePlugin) - expectedLabels[consts.GPUAllocationModeLabelKey] = string(consts.GPUAllocationModeDevicePlugin) - } nlc.updateGPUStateLabels(context.Background(), labels, "test-node") assert.Equal(t, expectedLabels, labels) }) } } -func TestReconcileModeLabel(t *testing.T) { - tests := []struct { - name string - defaultMode consts.GPUAllocationMode - initialLabels map[string]string - expectedMode string - expectModified bool - }{ - { - name: "unlabeled GPU node gets the default mode", - defaultMode: consts.GPUAllocationModeDRA, - initialLabels: map[string]string{commonGPULabelKey: commonGPULabelValue}, - expectedMode: string(consts.GPUAllocationModeDRA), - expectModified: true, - }, - { - name: "pre-labeled node is never overwritten", - defaultMode: consts.GPUAllocationModeDRA, - initialLabels: map[string]string{ - commonGPULabelKey: commonGPULabelValue, - consts.GPUAllocationModeLabelKey: string(consts.GPUAllocationModeDevicePlugin), - }, - expectedMode: string(consts.GPUAllocationModeDevicePlugin), - expectModified: false, - }, - { - name: "non-GPU node is not labeled", - defaultMode: consts.GPUAllocationModeDevicePlugin, - initialLabels: map[string]string{"kubernetes.io/hostname": "plain"}, - expectedMode: "", - expectModified: false, - }, - } - - for _, tc := range tests { - t.Run(tc.name, func(t *testing.T) { - nlc := &nodeLabelingController{ - defaultMode: tc.defaultMode, - logger: logr.Discard(), - } - labels := mergeLabels(tc.initialLabels) - modified := nlc.reconcileModeLabel(labels, "test-node") - assert.Equal(t, tc.expectModified, modified) - assert.Equal(t, tc.expectedMode, labels[consts.GPUAllocationModeLabelKey]) - }) - } -} - -func TestUpdateGPUStateLabelsPerMode(t *testing.T) { +func TestUpdateGPUStateLabelsDispatch(t *testing.T) { clusterPolicy := &gpuv1.ClusterPolicy{} gpuCluster := &nvidiav1alpha1.GPUCluster{} @@ -631,47 +577,24 @@ func TestUpdateGPUStateLabelsPerMode(t *testing.T) { name string clusterPolicy *gpuv1.ClusterPolicy gpuCluster *nvidiav1alpha1.GPUCluster - mode string + initialLabels map[string]string expectedLabels map[string]string }{ { - name: "dra node gets the DRA deploy labels only", - clusterPolicy: clusterPolicy, + name: "GPUCluster gets the DRA deploy labels", gpuCluster: gpuCluster, - mode: string(consts.GPUAllocationModeDRA), + initialLabels: map[string]string{commonGPULabelKey: commonGPULabelValue}, expectedLabels: mergeLabels(gpuClusterStateLabels), }, { - name: "device-plugin node gets the ClusterPolicy deploy labels only", + name: "ClusterPolicy gets the device-plugin deploy labels", clusterPolicy: clusterPolicy, - gpuCluster: gpuCluster, - mode: string(consts.GPUAllocationModeDevicePlugin), + initialLabels: map[string]string{commonGPULabelKey: commonGPULabelValue}, expectedLabels: mergeLabels(gpuStateLabels[gpuWorkloadConfigContainer]), }, { - name: "unlabeled node gets no deploy labels", - clusterPolicy: clusterPolicy, - gpuCluster: gpuCluster, - mode: "", - expectedLabels: map[string]string{}, - }, - { - name: "unrecognized mode gets no deploy labels", - clusterPolicy: clusterPolicy, - gpuCluster: gpuCluster, - mode: "bogus", - expectedLabels: map[string]string{}, - }, - { - name: "dra node without a GPUCluster gets no deploy labels", - clusterPolicy: clusterPolicy, - mode: string(consts.GPUAllocationModeDRA), - expectedLabels: map[string]string{}, - }, - { - name: "device-plugin node without a ClusterPolicy gets no deploy labels", - gpuCluster: gpuCluster, - mode: string(consts.GPUAllocationModeDevicePlugin), + name: "no labels added without common GPU label present", + initialLabels: map[string]string{}, expectedLabels: map[string]string{}, }, } @@ -684,10 +607,7 @@ func TestUpdateGPUStateLabelsPerMode(t *testing.T) { gpuCluster: tc.gpuCluster, logger: logr.Discard(), } - labels := map[string]string{commonGPULabelKey: commonGPULabelValue} - if tc.mode != "" { - labels[consts.GPUAllocationModeLabelKey] = tc.mode - } + labels := tc.initialLabels expected := mergeLabels(labels, tc.expectedLabels) nlc.updateGPUStateLabels(context.Background(), labels, "test-node") assert.Equal(t, expected, labels) @@ -698,14 +618,7 @@ func TestUpdateGPUStateLabelsPerMode(t *testing.T) { func TestUpdateGPUStateLabelsModeSweep(t *testing.T) { clusterPolicy := &gpuv1.ClusterPolicy{} gpuCluster := &nvidiav1alpha1.GPUCluster{} - draBase := map[string]string{ - commonGPULabelKey: commonGPULabelValue, - consts.GPUAllocationModeLabelKey: string(consts.GPUAllocationModeDRA), - } - devicePluginBase := map[string]string{ - commonGPULabelKey: commonGPULabelValue, - consts.GPUAllocationModeLabelKey: string(consts.GPUAllocationModeDevicePlugin), - } + gpuBase := map[string]string{commonGPULabelKey: commonGPULabelValue} tests := []struct { name string @@ -715,71 +628,53 @@ func TestUpdateGPUStateLabelsModeSweep(t *testing.T) { expectedLabels map[string]string }{ { - name: "dra node sweeps container-config leftovers, keeps shared keys", - clusterPolicy: clusterPolicy, + name: "GPUCluster sweeps container-config leftovers, keeps shared keys", gpuCluster: gpuCluster, - initialLabels: mergeLabels(draBase, gpuStateLabels[gpuWorkloadConfigContainer]), - expectedLabels: mergeLabels(draBase, gpuClusterStateLabels), + initialLabels: mergeLabels(gpuBase, gpuStateLabels[gpuWorkloadConfigContainer]), + expectedLabels: mergeLabels(gpuBase, gpuClusterStateLabels), }, { - name: "dra node sweeps vm-passthrough leftovers", - clusterPolicy: clusterPolicy, + name: "GPUCluster sweeps vm-passthrough leftovers", gpuCluster: gpuCluster, - initialLabels: mergeLabels(draBase, gpuStateLabels[gpuWorkloadConfigVMPassthrough]), - expectedLabels: mergeLabels(draBase, gpuClusterStateLabels), + initialLabels: mergeLabels(gpuBase, gpuStateLabels[gpuWorkloadConfigVMPassthrough]), + expectedLabels: mergeLabels(gpuBase, gpuClusterStateLabels), }, { - name: "dra node sweeps vm-vgpu leftovers but keeps the vgpu-manager driver gate", - clusterPolicy: clusterPolicy, + name: "GPUCluster sweeps vm-vgpu leftovers but keeps the vgpu-manager driver gate", gpuCluster: gpuCluster, - initialLabels: mergeLabels(draBase, gpuStateLabels[gpuWorkloadConfigVMVgpu]), - expectedLabels: mergeLabels(draBase, gpuClusterStateLabels, + initialLabels: mergeLabels(gpuBase, gpuStateLabels[gpuWorkloadConfigVMVgpu]), + expectedLabels: mergeLabels(gpuBase, gpuClusterStateLabels, map[string]string{vgpuManagerDeployLabelKey: "true"}), }, { - name: "device-plugin node sweeps DRA leftovers", + name: "ClusterPolicy sweeps DRA leftovers", clusterPolicy: clusterPolicy, - gpuCluster: gpuCluster, - initialLabels: mergeLabels(devicePluginBase, gpuClusterStateLabels), - expectedLabels: mergeLabels(devicePluginBase, gpuStateLabels[gpuWorkloadConfigContainer]), + initialLabels: mergeLabels(gpuBase, gpuClusterStateLabels), + expectedLabels: mergeLabels(gpuBase, gpuStateLabels[gpuWorkloadConfigContainer]), }, { - name: "sweep never touches values of the node's own stack keys", - clusterPolicy: clusterPolicy, - gpuCluster: gpuCluster, - initialLabels: mergeLabels(draBase, + name: "sweep never touches values of the node's own stack keys", + gpuCluster: gpuCluster, + initialLabels: mergeLabels(gpuBase, map[string]string{draDriverDeployLabelKey: "paused-for-driver-upgrade"}), - expectedLabels: mergeLabels(draBase, gpuClusterStateLabels, + expectedLabels: mergeLabels(gpuBase, gpuClusterStateLabels, map[string]string{draDriverDeployLabelKey: "paused-for-driver-upgrade"}), }, { - name: "sweep removes only other-stack keys, sparing unrecognized keys and the operands kill switch", - clusterPolicy: clusterPolicy, - gpuCluster: gpuCluster, - initialLabels: mergeLabels(draBase, map[string]string{ + name: "sweep removes only other-stack keys, sparing unrecognized keys and the operands kill switch", + gpuCluster: gpuCluster, + initialLabels: mergeLabels(gpuBase, map[string]string{ "nvidia.com/gpu.deploy.nvsm": "true", migManagerLabelKey: "true", commonOperandsLabelKey: "false", migConfigLabelKey: migConfigDisabledValue, }), - expectedLabels: mergeLabels(draBase, gpuClusterStateLabels, map[string]string{ + expectedLabels: mergeLabels(gpuBase, gpuClusterStateLabels, map[string]string{ "nvidia.com/gpu.deploy.nvsm": "true", commonOperandsLabelKey: "false", migConfigLabelKey: migConfigDisabledValue, }), }, - { - name: "dra node without a GPUCluster sweeps nothing", - clusterPolicy: clusterPolicy, - initialLabels: mergeLabels(draBase, gpuStateLabels[gpuWorkloadConfigContainer]), - expectedLabels: mergeLabels(draBase, gpuStateLabels[gpuWorkloadConfigContainer]), - }, - { - name: "device-plugin node without a ClusterPolicy sweeps nothing", - gpuCluster: gpuCluster, - initialLabels: mergeLabels(devicePluginBase, gpuClusterStateLabels), - expectedLabels: mergeLabels(devicePluginBase, gpuClusterStateLabels), - }, } for _, tc := range tests { @@ -828,18 +723,17 @@ func TestDeferDRAPluginRemoval(t *testing.T) { Status: corev1.PodStatus{Phase: corev1.PodRunning}, } + // flippedNodeLabels simulates a node that was on the DRA stack and is being switched + // to the device-plugin stack (GPUCluster deleted, ClusterPolicy now active). The node + // still carries DRA state labels from the previous stack. flippedNodeLabels := func() map[string]string { - return mergeLabels(map[string]string{ - commonGPULabelKey: commonGPULabelValue, - consts.GPUAllocationModeLabelKey: string(consts.GPUAllocationModeDevicePlugin), - }, gpuClusterStateLabels) + return mergeLabels(map[string]string{commonGPULabelKey: commonGPULabelValue}, gpuClusterStateLabels) } t.Run("claim pod on node defers plugin label removal", func(t *testing.T) { nlc := &nodeLabelingController{ client: fake.NewClientBuilder().WithScheme(scheme).WithIndex(&corev1.Pod{}, podNodeNameIndexKey, podNodeNameIndexer).WithObjects(gpuClaim.DeepCopy(), claimPod.DeepCopy()).Build(), clusterPolicy: &gpuv1.ClusterPolicy{}, - gpuCluster: &nvidiav1alpha1.GPUCluster{}, logger: logr.Discard(), } labels := flippedNodeLabels() @@ -861,7 +755,6 @@ func TestDeferDRAPluginRemoval(t *testing.T) { nlc := &nodeLabelingController{ client: fake.NewClientBuilder().WithScheme(scheme).WithIndex(&corev1.Pod{}, podNodeNameIndexKey, podNodeNameIndexer).WithObjects(adminClaim, adminPod).Build(), clusterPolicy: &gpuv1.ClusterPolicy{}, - gpuCluster: &nvidiav1alpha1.GPUCluster{}, logger: logr.Discard(), } labels := flippedNodeLabels() @@ -874,7 +767,6 @@ func TestDeferDRAPluginRemoval(t *testing.T) { nlc := &nodeLabelingController{ client: fake.NewClientBuilder().WithScheme(scheme).WithIndex(&corev1.Pod{}, podNodeNameIndexKey, podNodeNameIndexer).Build(), clusterPolicy: &gpuv1.ClusterPolicy{}, - gpuCluster: &nvidiav1alpha1.GPUCluster{}, logger: logr.Discard(), } labels := flippedNodeLabels() @@ -891,7 +783,6 @@ func TestDeferDRAPluginRemoval(t *testing.T) { nlc := &nodeLabelingController{ client: fake.NewClientBuilder().WithScheme(scheme).WithIndex(&corev1.Pod{}, podNodeNameIndexKey, podNodeNameIndexer).WithObjects(gpuClaim.DeepCopy(), terminating).Build(), clusterPolicy: &gpuv1.ClusterPolicy{}, - gpuCluster: &nvidiav1alpha1.GPUCluster{}, logger: logr.Discard(), } labels := flippedNodeLabels() @@ -913,6 +804,8 @@ func TestModeSweepDeleteSets(t *testing.T) { assert.ElementsMatch(t, []string{ migManagerLabelKey, gfdDeployLabelKey, + dcgmDeployLabelKey, + dcgmExporterDeployLabelKey, kataDevicePluginDeployLabelKey, kubevirtDevicePluginDeployLabelKey, "nvidia.com/gpu.deploy.client", @@ -1211,12 +1104,12 @@ func TestUpdateGPUClusterStateLabels(t *testing.T) { name: "GPU node gets the DRA operand deploy labels", initialLabels: map[string]string{commonGPULabelKey: commonGPULabelValue}, expectedLabels: map[string]string{ - commonGPULabelKey: commonGPULabelValue, - driverDeployLabelKey: "true", - draDriverDeployLabelKey: "true", - draValidatorDeployLabelKey: "true", - dcgmDeployLabelKey: "true", - dcgmExporterDeployLabelKey: "true", + commonGPULabelKey: commonGPULabelValue, + driverDeployLabelKey: "true", + draDriverDeployLabelKey: "true", + draValidatorDeployLabelKey: "true", + draDCGMDeployLabelKey: "true", + draDCGMExporterDeployLabelKey: "true", }, expectModified: true, }, @@ -1227,50 +1120,50 @@ func TestUpdateGPUClusterStateLabels(t *testing.T) { driverDeployLabelKey: "true", }, expectedLabels: map[string]string{ - commonGPULabelKey: commonGPULabelValue, - driverDeployLabelKey: "true", - draDriverDeployLabelKey: "true", - draValidatorDeployLabelKey: "true", - dcgmDeployLabelKey: "true", - dcgmExporterDeployLabelKey: "true", + commonGPULabelKey: commonGPULabelValue, + driverDeployLabelKey: "true", + draDriverDeployLabelKey: "true", + draValidatorDeployLabelKey: "true", + draDCGMDeployLabelKey: "true", + draDCGMExporterDeployLabelKey: "true", }, expectModified: true, }, { name: "paused deploy labels are honored, not overwritten", initialLabels: map[string]string{ - commonGPULabelKey: commonGPULabelValue, - driverDeployLabelKey: "false", - draDriverDeployLabelKey: "false", - draValidatorDeployLabelKey: "false", - dcgmDeployLabelKey: "false", - dcgmExporterDeployLabelKey: "false", + commonGPULabelKey: commonGPULabelValue, + driverDeployLabelKey: "false", + draDriverDeployLabelKey: "false", + draValidatorDeployLabelKey: "false", + draDCGMDeployLabelKey: "false", + draDCGMExporterDeployLabelKey: "false", }, expectedLabels: map[string]string{ - commonGPULabelKey: commonGPULabelValue, - driverDeployLabelKey: "false", - draDriverDeployLabelKey: "false", - draValidatorDeployLabelKey: "false", - dcgmDeployLabelKey: "false", - dcgmExporterDeployLabelKey: "false", + commonGPULabelKey: commonGPULabelValue, + driverDeployLabelKey: "false", + draDriverDeployLabelKey: "false", + draValidatorDeployLabelKey: "false", + draDCGMDeployLabelKey: "false", + draDCGMExporterDeployLabelKey: "false", }, expectModified: false, }, { name: "empty deploy labels are treated as absent and set", initialLabels: map[string]string{ - commonGPULabelKey: commonGPULabelValue, - driverDeployLabelKey: "", - dcgmDeployLabelKey: "", - dcgmExporterDeployLabelKey: "paused-for-driver-upgrade", + commonGPULabelKey: commonGPULabelValue, + driverDeployLabelKey: "", + draDCGMDeployLabelKey: "", + draDCGMExporterDeployLabelKey: "paused-for-driver-upgrade", }, expectedLabels: map[string]string{ - commonGPULabelKey: commonGPULabelValue, - driverDeployLabelKey: "true", - draDriverDeployLabelKey: "true", - draValidatorDeployLabelKey: "true", - dcgmDeployLabelKey: "true", - dcgmExporterDeployLabelKey: "paused-for-driver-upgrade", + commonGPULabelKey: commonGPULabelValue, + driverDeployLabelKey: "true", + draDriverDeployLabelKey: "true", + draValidatorDeployLabelKey: "true", + draDCGMDeployLabelKey: "true", + draDCGMExporterDeployLabelKey: "paused-for-driver-upgrade", }, expectModified: true, }, @@ -1302,7 +1195,7 @@ func TestReconcileGPUClusterNodeLabels(t *testing.T) { }} } - t.Run("GPUCluster present and no ClusterPolicy labels the GPU node", func(t *testing.T) { + t.Run("GPUCluster present and no ClusterPolicy labels the GPU node with DRA stack", func(t *testing.T) { gc := &nvidiav1alpha1.GPUCluster{ObjectMeta: metav1.ObjectMeta{Name: "config"}} r, c := newReconciler(gc, gpuNode()) @@ -1312,98 +1205,44 @@ func TestReconcileGPUClusterNodeLabels(t *testing.T) { node := &corev1.Node{} require.NoError(t, c.Get(context.Background(), types.NamespacedName{Name: "gpu-node"}, node)) assert.Equal(t, commonGPULabelValue, node.Labels[commonGPULabelKey]) - assert.Equal(t, string(consts.GPUAllocationModeDRA), node.Labels[consts.GPUAllocationModeLabelKey]) assert.Equal(t, "true", node.Labels[driverDeployLabelKey]) assert.Equal(t, "true", node.Labels[draDriverDeployLabelKey]) - assert.Equal(t, "true", node.Labels[dcgmDeployLabelKey]) - assert.Equal(t, "true", node.Labels[dcgmExporterDeployLabelKey]) + assert.Equal(t, "true", node.Labels[draDCGMDeployLabelKey]) + assert.Equal(t, "true", node.Labels[draDCGMExporterDeployLabelKey]) }) - t.Run("no ClusterPolicy and no GPUCluster leaves the node untouched", func(t *testing.T) { - r, c := newReconciler(gpuNode()) + t.Run("ClusterPolicy present and no GPUCluster labels the GPU node with device-plugin stack", func(t *testing.T) { + cp := &gpuv1.ClusterPolicy{ObjectMeta: metav1.ObjectMeta{Name: "cluster-policy"}} + r, c := newReconciler(cp, gpuNode()) _, err := r.Reconcile(context.Background(), reconcile.Request{}) require.NoError(t, err) node := &corev1.Node{} require.NoError(t, c.Get(context.Background(), types.NamespacedName{Name: "gpu-node"}, node)) - assert.NotContains(t, node.Labels, commonGPULabelKey) - assert.NotContains(t, node.Labels, consts.GPUAllocationModeLabelKey) - assert.NotContains(t, node.Labels, draDriverDeployLabelKey) - }) - - getNode := func(t *testing.T, c client.Client) *corev1.Node { - t.Helper() - node := &corev1.Node{} - require.NoError(t, c.Get(context.Background(), types.NamespacedName{Name: "gpu-node"}, node)) - return node - } - clusterPolicy := func() *gpuv1.ClusterPolicy { - return &gpuv1.ClusterPolicy{ObjectMeta: metav1.ObjectMeta{Name: "cluster-policy"}} - } - gpuCluster := func() *nvidiav1alpha1.GPUCluster { - return &nvidiav1alpha1.GPUCluster{ObjectMeta: metav1.ObjectMeta{Name: "config"}} - } - - t.Run("both CRs label a new GPU node with DEFAULT_GPU_ALLOCATION_MODE", func(t *testing.T) { - t.Setenv(consts.DefaultGPUAllocationModeEnvName, string(consts.GPUAllocationModeDRA)) - r, c := newReconciler(clusterPolicy(), gpuCluster(), gpuNode()) - - _, err := r.Reconcile(context.Background(), reconcile.Request{}) - require.NoError(t, err) - - node := getNode(t, c) - assert.Equal(t, string(consts.GPUAllocationModeDRA), node.Labels[consts.GPUAllocationModeLabelKey]) - assert.Equal(t, "true", node.Labels[draDriverDeployLabelKey]) - assert.NotContains(t, node.Labels, "nvidia.com/gpu.deploy.container-toolkit") - }) - - t.Run("both CRs and unset DEFAULT_GPU_ALLOCATION_MODE default a new GPU node to device-plugin", func(t *testing.T) { - r, c := newReconciler(clusterPolicy(), gpuCluster(), gpuNode()) - - _, err := r.Reconcile(context.Background(), reconcile.Request{}) - require.NoError(t, err) - - node := getNode(t, c) - assert.Equal(t, string(consts.GPUAllocationModeDevicePlugin), node.Labels[consts.GPUAllocationModeLabelKey]) + assert.Equal(t, commonGPULabelValue, node.Labels[commonGPULabelKey]) assert.Equal(t, "true", node.Labels["nvidia.com/gpu.deploy.container-toolkit"]) assert.NotContains(t, node.Labels, draDriverDeployLabelKey) }) - t.Run("an invalid DEFAULT_GPU_ALLOCATION_MODE fails reconciliation and labels nothing", func(t *testing.T) { - t.Setenv(consts.DefaultGPUAllocationModeEnvName, "bogus") - r, c := newReconciler(clusterPolicy(), gpuCluster(), gpuNode()) - - _, err := r.Reconcile(context.Background(), reconcile.Request{}) - require.ErrorContains(t, err, `invalid DEFAULT_GPU_ALLOCATION_MODE environment variable: "bogus"`) - - node := getNode(t, c) - assert.NotContains(t, node.Labels, consts.GPUAllocationModeLabelKey) - }) - - t.Run("a single CR wins over a contrary DEFAULT_GPU_ALLOCATION_MODE", func(t *testing.T) { - t.Setenv(consts.DefaultGPUAllocationModeEnvName, string(consts.GPUAllocationModeDevicePlugin)) - r, c := newReconciler(gpuCluster(), gpuNode()) + t.Run("no ClusterPolicy and no GPUCluster leaves the node untouched", func(t *testing.T) { + r, c := newReconciler(gpuNode()) _, err := r.Reconcile(context.Background(), reconcile.Request{}) require.NoError(t, err) - node := getNode(t, c) - assert.Equal(t, string(consts.GPUAllocationModeDRA), node.Labels[consts.GPUAllocationModeLabelKey]) + node := &corev1.Node{} + require.NoError(t, c.Get(context.Background(), types.NamespacedName{Name: "gpu-node"}, node)) + assert.NotContains(t, node.Labels, commonGPULabelKey) + assert.NotContains(t, node.Labels, draDriverDeployLabelKey) }) - t.Run("a pre-labeled node keeps its mode and its stack's deploy labels", func(t *testing.T) { - t.Setenv(consts.DefaultGPUAllocationModeEnvName, string(consts.GPUAllocationModeDRA)) - node := gpuNode() - node.Labels[consts.GPUAllocationModeLabelKey] = string(consts.GPUAllocationModeDevicePlugin) - r, c := newReconciler(clusterPolicy(), gpuCluster(), node) + t.Run("both CRs present returns an error", func(t *testing.T) { + cp := &gpuv1.ClusterPolicy{ObjectMeta: metav1.ObjectMeta{Name: "cluster-policy"}} + gc := &nvidiav1alpha1.GPUCluster{ObjectMeta: metav1.ObjectMeta{Name: "config"}} + r, _ := newReconciler(cp, gc, gpuNode()) _, err := r.Reconcile(context.Background(), reconcile.Request{}) - require.NoError(t, err) - - got := getNode(t, c) - assert.Equal(t, string(consts.GPUAllocationModeDevicePlugin), got.Labels[consts.GPUAllocationModeLabelKey]) - assert.Equal(t, "true", got.Labels["nvidia.com/gpu.deploy.device-plugin"]) - assert.NotContains(t, got.Labels, draDriverDeployLabelKey) + require.ErrorContains(t, err, "both ClusterPolicy and GPUCluster CRs exist") }) } diff --git a/controllers/object_controls.go b/controllers/object_controls.go index e35c073f41..9130e60360 100644 --- a/controllers/object_controls.go +++ b/controllers/object_controls.go @@ -717,8 +717,6 @@ func preprocessService(obj *corev1.Service, n ClusterPolicyController) error { func preProcessDaemonSet(obj *appsv1.DaemonSet, n ClusterPolicyController) error { logger := n.logger.WithValues("Daemonset", obj.Name) - applyModeSelector(obj, n) - transformations := map[string]func(*appsv1.DaemonSet, *gpuv1.ClusterPolicySpec, ClusterPolicyController) error{ "nvidia-driver-daemonset": TransformDriver, "nvidia-vgpu-manager-daemonset": TransformVGPUManager, @@ -771,25 +769,6 @@ func preProcessDaemonSet(obj *appsv1.DaemonSet, n ClusterPolicyController) error return nil } -// applyModeSelector adds the nvidia.com/gpu-operator.resource-allocation.mode nodeSelector to a -// ClusterPolicy operand DaemonSet, restricting it to device-plugin-stack nodes. The selector is -// rendered only once a GPUCluster CR exists (before that there is no DRA stack to fence operands -// off from, and ClusterPolicy operands schedule on their gpu.deploy.* labels alone) AND every GPU -// node already carries the mode label. The second condition keeps the selector from de-scheduling -// operand pods on nodes the NodeLabelingReconciler has not labeled yet (e.g. a GPUCluster created -// in the same upgrade that introduced the label); it holds the selector back cluster-wide until -// labeling converges, which is safe: unlabeled nodes carry no DRA deploy labels, so both stacks -// stay correctly routed by the deploy labels alone in the interim. -func applyModeSelector(obj *appsv1.DaemonSet, n ClusterPolicyController) { - if !n.gpuClusterExists || !n.allGPUNodesModeLabeled { - return - } - if obj.Spec.Template.Spec.NodeSelector == nil { - obj.Spec.Template.Spec.NodeSelector = map[string]string{} - } - obj.Spec.Template.Spec.NodeSelector[consts.GPUAllocationModeLabelKey] = string(consts.GPUAllocationModeDevicePlugin) -} - // applyCommonDaemonsetMetadata adds additional labels and annotations to the daemonset podSpec if there are any specified // by the user in the podSpec. func applyCommonDaemonsetMetadata(obj *appsv1.DaemonSet, dsSpec *gpuv1.DaemonsetsSpec) { diff --git a/controllers/object_controls_test.go b/controllers/object_controls_test.go index 549aa78a61..d84917c0b5 100644 --- a/controllers/object_controls_test.go +++ b/controllers/object_controls_test.go @@ -1664,84 +1664,6 @@ func TestDCGMExporter(t *testing.T) { } } -// TestApplyModeSelector verifies the render gate for the resource-allocation mode -// nodeSelector: injected only when a GPUCluster exists AND every GPU node already carries -// the mode label. It drives preProcessDaemonSet with a DaemonSet that has no per-operand -// transformation (nvidia-kata-manager) to prove the injection covers that path too. -func TestApplyModeSelector(t *testing.T) { - testCases := []struct { - description string - gpuClusterExists bool - allGPUNodesModeLabeled bool - expectSelector bool - }{ - {"no GPUCluster", false, true, false}, - {"GPUCluster but unlabeled node", true, false, false}, - {"GPUCluster and all nodes labeled", true, true, true}, - } - - for _, tc := range testCases { - t.Run(tc.description, func(t *testing.T) { - n := clusterPolicyController - n.gpuClusterExists = tc.gpuClusterExists - n.allGPUNodesModeLabeled = tc.allGPUNodesModeLabeled - - ds := &appsv1.DaemonSet{ObjectMeta: metav1.ObjectMeta{Name: "nvidia-kata-manager"}} - require.NoError(t, preProcessDaemonSet(ds, n)) - - if tc.expectSelector { - require.Equal(t, "device-plugin", - ds.Spec.Template.Spec.NodeSelector["nvidia.com/gpu-operator.resource-allocation.mode"]) - } else { - require.NotContains(t, ds.Spec.Template.Spec.NodeSelector, - "nvidia.com/gpu-operator.resource-allocation.mode") - } - }) - } -} - -// TestDiscoverGPUNodesModeLabelGate verifies discoverGPUNodes reports whether every GPU node -// carries the resource-allocation mode label; non-GPU nodes are ignored. -func TestDiscoverGPUNodesModeLabelGate(t *testing.T) { - newNode := func(name string, labels map[string]string) *corev1.Node { - return &corev1.Node{ObjectMeta: metav1.ObjectMeta{Name: name, Labels: labels}} - } - gpuLabeled := map[string]string{ - commonGPULabelKey: "true", - "nvidia.com/gpu-operator.resource-allocation.mode": "device-plugin", - } - gpuUnlabeled := map[string]string{commonGPULabelKey: "true"} - nonGPUUnlabeled := map[string]string{"kubernetes.io/os": "linux"} - - testCases := []struct { - description string - nodes []*corev1.Node - expected bool - }{ - {"all GPU nodes labeled", []*corev1.Node{newNode("a", gpuLabeled), newNode("b", gpuLabeled)}, true}, - {"one GPU node unlabeled", []*corev1.Node{newNode("a", gpuLabeled), newNode("b", gpuUnlabeled)}, false}, - {"non-GPU node without label ignored", []*corev1.Node{newNode("a", gpuLabeled), newNode("b", nonGPUUnlabeled)}, true}, - } - - for _, tc := range testCases { - t.Run(tc.description, func(t *testing.T) { - cl := fake.NewClientBuilder().WithScheme(scheme.Scheme).Build() - for _, node := range tc.nodes { - require.NoError(t, cl.Create(context.Background(), node)) - } - n := ClusterPolicyController{ - ctx: context.Background(), - client: cl, - logger: clusterPolicyController.logger, - operatorMetrics: clusterPolicyController.operatorMetrics, - } - _, _, err := n.discoverGPUNodes() - require.NoError(t, err) - require.Equal(t, tc.expected, n.allGPUNodesModeLabeled) - }) - } -} - func TestGetSanitizedKernelVersion(t *testing.T) { tests := []struct { input string diff --git a/controllers/state_manager.go b/controllers/state_manager.go index 9fd8e71f19..d49c311967 100644 --- a/controllers/state_manager.go +++ b/controllers/state_manager.go @@ -35,8 +35,6 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client/config" gpuv1 "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 ( @@ -79,6 +77,8 @@ const ( driverDeployLabelKey = "nvidia.com/gpu.deploy.driver" draDriverDeployLabelKey = "nvidia.com/gpu.deploy.dra-driver" draValidatorDeployLabelKey = "nvidia.com/gpu.deploy.dra-validator" + draDCGMDeployLabelKey = "nvidia.com/gpu.deploy.dcgm-dra" + draDCGMExporterDeployLabelKey = "nvidia.com/gpu.deploy.dcgm-exporter-dra" gfdDeployLabelKey = "nvidia.com/gpu.deploy.gpu-feature-discovery" dcgmDeployLabelKey = "nvidia.com/gpu.deploy.dcgm" dcgmExporterDeployLabelKey = "nvidia.com/gpu.deploy.dcgm-exporter" @@ -129,11 +129,11 @@ var gpuStateLabels = map[string]map[string]string{ // GPUCluster operands gate their nodeSelectors on, analogous to gpuStateLabels for // the ClusterPolicy stack. var gpuClusterStateLabels = map[string]string{ - driverDeployLabelKey: "true", - draDriverDeployLabelKey: "true", - draValidatorDeployLabelKey: "true", - dcgmDeployLabelKey: "true", - dcgmExporterDeployLabelKey: "true", + driverDeployLabelKey: "true", + draDriverDeployLabelKey: "true", + draValidatorDeployLabelKey: "true", + draDCGMDeployLabelKey: "true", + draDCGMExporterDeployLabelKey: "true", } // clusterPolicyStateLabelKeys returns every deploy-label key the ClusterPolicy @@ -225,11 +225,6 @@ type ClusterPolicyController struct { hasGPUNodes bool hasNFDLabels bool sandboxEnabled bool - - // gpuClusterExists and allGPUNodesModeLabeled gate rendering of the resource-allocation - // mode nodeSelector on operand DaemonSets; see applyModeSelector. - gpuClusterExists bool - allGPUNodesModeLabeled bool } func addState(n *ClusterPolicyController, path string) { @@ -526,8 +521,7 @@ func (w *gpuWorkloadConfiguration) removeGPUStateLabels(labels map[string]string } // discoverGPUNodes reads all cluster nodes and returns whether any NFD labels are present -// and how many GPU nodes (with nvidia.com/gpu.present=true) exist. It also records in -// n.allGPUNodesModeLabeled whether every GPU node carries the resource-allocation mode label. +// and how many GPU nodes (with nvidia.com/gpu.present=true) exist. // Node label writes are handled by NodeLabelingReconciler. func (n *ClusterPolicyController) discoverGPUNodes() (bool, int, error) { ctx := n.ctx @@ -538,7 +532,6 @@ func (n *ClusterPolicyController) discoverGPUNodes() (bool, int, error) { clusterHasNFDLabels := false gpuNodesTotal := 0 - n.allGPUNodesModeLabeled = true for _, node := range list.Items { labels := node.GetLabels() if !clusterHasNFDLabels { @@ -548,9 +541,6 @@ func (n *ClusterPolicyController) discoverGPUNodes() (bool, int, error) { continue } gpuNodesTotal++ - if labels[consts.GPUAllocationModeLabelKey] == "" { - n.allGPUNodesModeLabeled = false - } if n.ocpDriverToolkit.requested { rhcosVersion, ok := labels[nfdOSTreeVersionLabelKey] if ok { @@ -879,12 +869,6 @@ func (n *ClusterPolicyController) init(ctx context.Context, reconciler *ClusterP n.hasGPUNodes = gpuNodeCount != 0 n.hasNFDLabels = hasNFDLabels - gpuClusters := &nvidiav1alpha1.GPUClusterList{} - if err := n.client.List(ctx, gpuClusters); err != nil { - return fmt.Errorf("unable to list GPUClusters: %w", err) - } - n.gpuClusterExists = len(gpuClusters.Items) > 0 - if n.hasGPUNodes { gpuNodeOSRelease, gpuNodeOSTag, err := n.getGPUNodeOSInfo() if err != nil { diff --git a/controllers/state_manager_test.go b/controllers/state_manager_test.go index eb56ed3d0a..73f4b7579e 100644 --- a/controllers/state_manager_test.go +++ b/controllers/state_manager_test.go @@ -30,7 +30,6 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client/fake" gpuv1 "github.com/NVIDIA/gpu-operator/api/nvidia/v1" - "github.com/NVIDIA/gpu-operator/internal/consts" ) func TestGetGPUNodeOSInfo(t *testing.T) { @@ -521,19 +520,6 @@ func TestRemoveAllGPUStateLabels(t *testing.T) { require.False(t, modified) require.Equal(t, map[string]string{"kubernetes.io/hostname": "plain"}, labels) }) - // The mode label is sticky: operand rendering gates on all GPU nodes carrying it, so - // state-label cleanup must never strip it. - t.Run("preserves the resource-allocation mode label", func(t *testing.T) { - labels := map[string]string{ - consts.GPUAllocationModeLabelKey: string(consts.GPUAllocationModeDevicePlugin), - driverDeployLabelKey: "true", - } - modified := removeAllGPUStateLabels(labels) - require.True(t, modified) - require.Equal(t, map[string]string{ - consts.GPUAllocationModeLabelKey: string(consts.GPUAllocationModeDevicePlugin), - }, labels) - }) } func TestIsStateEnabled_SandboxAndKataDevicePlugin(t *testing.T) { diff --git a/deployments/gpu-operator/templates/clusterpolicy.yaml b/deployments/gpu-operator/templates/clusterpolicy.yaml index af7f098e75..e156f5b7a9 100644 --- a/deployments/gpu-operator/templates/clusterpolicy.yaml +++ b/deployments/gpu-operator/templates/clusterpolicy.yaml @@ -1,5 +1,3 @@ -{{- /* ClusterPolicy (device-plugin stack) and GPUCluster (DRA stack) may coexist; per-node - ownership is decided by the nvidia.com/gpu-operator.resource-allocation.mode label. */ -}} {{- if .Values.clusterPolicy.deployCR }} apiVersion: nvidia.com/v1 kind: ClusterPolicy diff --git a/deployments/gpu-operator/templates/validations.yaml b/deployments/gpu-operator/templates/validations.yaml index 72b583a059..bb4677ebbe 100644 --- a/deployments/gpu-operator/templates/validations.yaml +++ b/deployments/gpu-operator/templates/validations.yaml @@ -1,3 +1,11 @@ +{{- if and .Values.clusterPolicy.deployCR .Values.gpuCluster.deployCR }} +{{ fail "clusterPolicy.deployCR and gpuCluster.deployCR cannot both be true; only one CR can exist" }} +{{- end }} + +{{- if and .Values.gpuCluster.deployCR (and .Values.driver.enabled (not .Values.driver.nvidiaDriverCRD.enabled)) }} +{{ fail "the NVIDIADriver CRD must be enabled when deploying a GPUCluster CR, set driver.nvidiaDriverCRD.enabled=true" }} +{{- end }} + {{- if and (eq .Values.cdi.enabled false) (eq .Values.cdi.nriPluginEnabled true) }} {{ fail "the NRI Plugin cannot be enabled when CDI is disabled" }} {{- end }} diff --git a/deployments/gpu-operator/values.yaml b/deployments/gpu-operator/values.yaml index a197ee757b..933632ddaf 100644 --- a/deployments/gpu-operator/values.yaml +++ b/deployments/gpu-operator/values.yaml @@ -76,9 +76,6 @@ operator: #version: "" imagePullPolicy: IfNotPresent imagePullSecrets: [] - # Additional environment variables for the operator container, e.g. DEFAULT_GPU_ALLOCATION_MODE - # ("device-plugin" or "dra"): the stack assigned to new GPU nodes when both a - # ClusterPolicy and a GPUCluster exist. env: [] priorityClassName: system-node-critical runtimeClass: nvidia @@ -563,26 +560,24 @@ ccManager: resources: {} hostNetwork: false -# clusterPolicy controls whether the chart deploys the ClusterPolicy CR, which -# manages the classic device-plugin GPU enablement stack. Disable it to run a -# GPUCluster-only (DRA) installation. +# The ClusterPolicy CR stores the desired state of the GPU +# enablement stack based on the Kubernetes Device Plugin framework. clusterPolicy: deployCR: true -# gpuCluster deploys the DRA-based GPU enablement stack (the NVIDIA DRA -# driver for GPUs) via a GPUCluster CR. This is an alpha feature -# (nvidia.com/v1alpha1) and is disabled by default. +# The GPUCluster CR stores the desired state of the GPU +# enablement stack based on Dynamic Resource Allocation (DRA). +# This is an experimental feature and is disabled by default. # -# The GPUCluster and ClusterPolicy CRs may coexist: each GPU node is served by -# exactly one stack according to its nvidia.com/gpu-operator.resource-allocation.mode -# label. Requires an NVIDIA driver (>= 580) with CDI, host-installed or via -# driver.enabled / an NVIDIADriver CR; GPUCluster waits for driver readiness -# before deploying. +# To enable the GPUCluster (DRA) enablement stack, set +# gpuCluster.deployCR=true and clusterPolicy.deployCR=false +# It is an invalid configuration for both CRs to exist. gpuCluster: deployCR: false -# draDriver configures the NVIDIA DRA driver operand managed by the GPUCluster -# CR (rendered only when gpuCluster.deployCR is true). +# draDriver configures the NVIDIA DRA driver for GPUs which +# is managed by the GPUCluster CR (rendered only when +# gpuCluster.deployCR=true). draDriver: repository: registry.k8s.io/dra-driver-nvidia image: dra-driver-nvidia-gpu diff --git a/internal/consts/consts.go b/internal/consts/consts.go index 107453dc9a..f28c507942 100644 --- a/internal/consts/consts.go +++ b/internal/consts/consts.go @@ -70,31 +70,6 @@ const ( // NVIDIADriverOwnerLabel is an operator-managed node label used to route each GPU node to one NVIDIADriver. NVIDIADriverOwnerLabel = "nvidia.com/gpu-operator.driver.owner" - // GPUAllocationModeLabelKey is a node label selecting which stack serves the node's GPUs: - // the device plugin (ClusterPolicy) or the DRA driver (GPUCluster). Once both stacks can - // coexist (a GPUCluster exists) and every GPU node carries the label, operand DaemonSets - // except the DRA kubelet-plugin carry it as a nodeSelector entry alongside their - // gpu.deploy. selector, so a node only ever runs operands of the stack it is - // labeled for; rendering the selector is deferred until then so that introducing it never - // de-schedules operands from nodes not yet labeled. The kubelet-plugin gates only on - // gpu.deploy.dra-driver, which the node-labeling controller removes last — after every - // claim-holding pod is gone — so claims can still be unprepared during a mode flip. The - // node-labeling controller writes the mode label once per GPU node and never overwrites - // an existing value. - GPUAllocationModeLabelKey = "nvidia.com/gpu-operator.resource-allocation.mode" - // GPUAllocationModeDevicePlugin selects the device-plugin (ClusterPolicy) stack for a node. - GPUAllocationModeDevicePlugin GPUAllocationMode = "device-plugin" - // GPUAllocationModeDRA selects the DRA (GPUCluster) stack for a node. - GPUAllocationModeDRA GPUAllocationMode = "dra" - // DefaultGPUAllocationModeEnvName is the operator environment variable holding the mode - // applied to GPU nodes that do not have the mode label yet, consulted when both a - // ClusterPolicy and a GPUCluster exist. It never overrides an existing label. - DefaultGPUAllocationModeEnvName = "DEFAULT_GPU_ALLOCATION_MODE" - // MinimumGDSVersionForOpenRM indicates the minimum GDS version that is supported only with OpenRM driver MinimumGDSVersionForOpenRM = "v2.17.5" ) - -// GPUAllocationMode is the value set of the GPUAllocationModeLabelKey node label and the -// DEFAULT_GPU_ALLOCATION_MODE environment variable, selecting which stack serves a node's GPUs. -type GPUAllocationMode string diff --git a/internal/nvidiadriver/nvidiadriver.go b/internal/nvidiadriver/nvidiadriver.go index f6df6c5778..06fa8ab6cf 100644 --- a/internal/nvidiadriver/nvidiadriver.go +++ b/internal/nvidiadriver/nvidiadriver.go @@ -35,11 +35,8 @@ func nodeMatchesSelector(nodeLabels map[string]string, selector map[string]strin // AssignOwners labels GPU nodes with the NVIDIADriver that should manage their driver pods. // Non-default NVIDIADrivers take precedence over the default fallback, and conflicts fail closed before -// node owner labels are changed. When classicClusterPolicyDriver is true (a ClusterPolicy manages -// its own driver rather than delegating to NVIDIADriver CRs), device-plugin nodes get no owner: -// their driver comes from the ClusterPolicy DaemonSet, and assigning an owner would land a second -// driver DaemonSet on them. On success, it returns true when any node owner label was changed. -func AssignOwners(ctx context.Context, c client.Client, classicClusterPolicyDriver bool) (bool, error) { +// node owner labels are changed. On success, it returns true when any node owner label was changed. +func AssignOwners(ctx context.Context, c client.Client) (bool, error) { drivers := &nvidiav1alpha1.NVIDIADriverList{} if err := c.List(ctx, drivers); err != nil { return false, fmt.Errorf("failed to list NVIDIADriver CRs: %w", err) @@ -64,11 +61,6 @@ func AssignOwners(ctx context.Context, c client.Client, classicClusterPolicyDriv desiredOwnersByNode := map[string]string{} for _, node := range nodes.Items { - if classicClusterPolicyDriver && - node.Labels[consts.GPUAllocationModeLabelKey] == string(consts.GPUAllocationModeDevicePlugin) { - desiredOwnersByNode[node.Name] = "" - continue - } desiredOwner, err := desiredOwnerForNode(&node, nonDefaultDrivers, defaultOwner) if err != nil { return false, err diff --git a/internal/nvidiadriver/nvidiadriver_errors_test.go b/internal/nvidiadriver/nvidiadriver_errors_test.go index e6ecbcc788..98f6f11751 100644 --- a/internal/nvidiadriver/nvidiadriver_errors_test.go +++ b/internal/nvidiadriver/nvidiadriver_errors_test.go @@ -62,7 +62,7 @@ func TestAssignOwnersReturnsErrorWhenDriverListFails(t *testing.T) { }). Build() - changed, err := AssignOwners(context.Background(), c, false) + changed, err := AssignOwners(context.Background(), c) require.Error(t, err) require.False(t, changed) require.Contains(t, err.Error(), "failed to list NVIDIADriver CRs") @@ -88,7 +88,7 @@ func TestAssignOwnersReturnsErrorWhenNodeListFails(t *testing.T) { }). Build() - changed, err := AssignOwners(context.Background(), c, false) + changed, err := AssignOwners(context.Background(), c) require.Error(t, err) require.False(t, changed) require.Contains(t, err.Error(), "failed to list GPU nodes") @@ -112,7 +112,7 @@ func TestAssignOwnersReturnsErrorWhenOwnerLabelUpdateFails(t *testing.T) { }). Build() - changed, err := AssignOwners(context.Background(), c, false) + changed, err := AssignOwners(context.Background(), c) require.Error(t, err) require.False(t, changed) require.Contains(t, err.Error(), "failed to update NVIDIADriver owner label for node \"gpu-node\"") @@ -134,7 +134,7 @@ func TestAssignOwnersReturnsErrorWhenOwnerLabelRemovalFails(t *testing.T) { }). Build() - changed, err := AssignOwners(context.Background(), c, false) + changed, err := AssignOwners(context.Background(), c) require.Error(t, err) require.False(t, changed) require.Contains(t, err.Error(), "failed to remove NVIDIADriver owner label for node \"gpu-node\"") diff --git a/internal/nvidiadriver/nvidiadriver_test.go b/internal/nvidiadriver/nvidiadriver_test.go index c56eb72bc2..8e7c50eda2 100644 --- a/internal/nvidiadriver/nvidiadriver_test.go +++ b/internal/nvidiadriver/nvidiadriver_test.go @@ -123,43 +123,6 @@ func TestNodeMatchesSelector(t *testing.T) { } } -func TestAssignOwnersSkipsDevicePluginNodesUnderClassicClusterPolicyDriver(t *testing.T) { - scheme := runtime.NewScheme() - require.NoError(t, nvidiav1alpha1.AddToScheme(scheme)) - require.NoError(t, corev1.AddToScheme(scheme)) - - defaultDriver := &nvidiav1alpha1.NVIDIADriver{ - ObjectMeta: metav1.ObjectMeta{Name: consts.DefaultNVIDIADriverName}, - Spec: nvidiav1alpha1.NVIDIADriverSpec{Default: true}, - } - draNode := &corev1.Node{ObjectMeta: metav1.ObjectMeta{ - Name: "dra-node", - Labels: map[string]string{ - consts.GPUPresentLabel: "true", - consts.GPUAllocationModeLabelKey: string(consts.GPUAllocationModeDRA), - }, - }} - devicePluginNode := &corev1.Node{ObjectMeta: metav1.ObjectMeta{ - Name: "device-plugin-node", - Labels: map[string]string{ - consts.GPUPresentLabel: "true", - consts.GPUAllocationModeLabelKey: string(consts.GPUAllocationModeDevicePlugin), - consts.NVIDIADriverOwnerLabel: consts.DefaultNVIDIADriverName, - }, - }} - - k8sClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(defaultDriver, draNode, devicePluginNode).Build() - - changed, err := AssignOwners(context.Background(), k8sClient, true) - require.NoError(t, err) - require.True(t, changed) - - require.NoError(t, k8sClient.Get(context.Background(), client.ObjectKey{Name: "dra-node"}, draNode)) - require.NoError(t, k8sClient.Get(context.Background(), client.ObjectKey{Name: "device-plugin-node"}, devicePluginNode)) - require.Equal(t, consts.DefaultNVIDIADriverName, draNode.Labels[consts.NVIDIADriverOwnerLabel]) - require.NotContains(t, devicePluginNode.Labels, consts.NVIDIADriverOwnerLabel) -} - func TestAssignNVIDIADriverOwnersGivesSpecificDriversPrecedence(t *testing.T) { scheme := runtime.NewScheme() require.NoError(t, nvidiav1alpha1.AddToScheme(scheme)) @@ -186,7 +149,7 @@ func TestAssignNVIDIADriverOwnersGivesSpecificDriversPrecedence(t *testing.T) { k8sClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(defaultDriver, specificDriver, defaultNode, specificNode).Build() - changed, err := AssignOwners(context.Background(), k8sClient, false) + changed, err := AssignOwners(context.Background(), k8sClient) require.NoError(t, err) require.True(t, changed) @@ -218,7 +181,7 @@ func TestAssignNVIDIADriverOwnersAllowsMissingDefaultDriver(t *testing.T) { k8sClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(specificDriver, unmatchedNode, specificNode).Build() - changed, err := AssignOwners(context.Background(), k8sClient, false) + changed, err := AssignOwners(context.Background(), k8sClient) require.NoError(t, err) require.True(t, changed) @@ -255,7 +218,7 @@ func TestAssignNVIDIADriverOwnersIgnoresDeletingDrivers(t *testing.T) { k8sClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(deletingDriver, node).Build() - changed, err := AssignOwners(context.Background(), k8sClient, false) + changed, err := AssignOwners(context.Background(), k8sClient) require.NoError(t, err) require.True(t, changed) @@ -279,7 +242,7 @@ func TestAssignNVIDIADriverOwnersUsesDefaultDriverWithArbitraryName(t *testing.T k8sClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(defaultDriver, node).Build() - changed, err := AssignOwners(context.Background(), k8sClient, false) + changed, err := AssignOwners(context.Background(), k8sClient) require.NoError(t, err) require.True(t, changed) @@ -320,7 +283,7 @@ func TestAssignNVIDIADriverOwnersReturnsFalseWhenOwnersAreCurrent(t *testing.T) k8sClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(defaultDriver, specificDriver, defaultNode, specificNode).Build() - changed, err := AssignOwners(context.Background(), k8sClient, false) + changed, err := AssignOwners(context.Background(), k8sClient) require.NoError(t, err) require.False(t, changed) } @@ -345,7 +308,7 @@ func TestAssignNVIDIADriverOwnersErrorsOnMultipleDefaultDrivers(t *testing.T) { k8sClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(defaultDriverA, defaultDriverB, node).Build() - changed, err := AssignOwners(context.Background(), k8sClient, false) + changed, err := AssignOwners(context.Background(), k8sClient) require.Error(t, err) require.False(t, changed) require.Contains(t, err.Error(), "multiple default NVIDIADrivers found") @@ -375,7 +338,7 @@ func TestAssignNVIDIADriverOwnersRejectsReservedOwnerLabelSelector(t *testing.T) k8sClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(driver, node).Build() - changed, err := AssignOwners(context.Background(), k8sClient, false) + changed, err := AssignOwners(context.Background(), k8sClient) require.Error(t, err) require.False(t, changed) require.Contains(t, err.Error(), "reserved label") @@ -418,7 +381,7 @@ func TestAssignNVIDIADriverOwnersRejectsDefaultDriverNodeSelector(t *testing.T) k8sClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(defaultDriver, specificDriver, defaultNode, unmatchedNode, specificNode).Build() - changed, err := AssignOwners(context.Background(), k8sClient, false) + changed, err := AssignOwners(context.Background(), k8sClient) require.Error(t, err) require.False(t, changed) require.Contains(t, err.Error(), "default NVIDIADriver") @@ -464,7 +427,7 @@ func TestAssignNVIDIADriverOwnersDoesNotFallbackToDefaultOnUserDriverConflict(t k8sClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(defaultDriver, driverA, driverB, conflictedNode).Build() - changed, err := AssignOwners(context.Background(), k8sClient, false) + changed, err := AssignOwners(context.Background(), k8sClient) require.Error(t, err) require.False(t, changed) require.Contains(t, err.Error(), "multiple NVIDIADrivers match the same node") @@ -510,7 +473,7 @@ func TestAssignNVIDIADriverOwnersDoesNotChangeOwnersWhenAnyUserDriverConflicts(t k8sClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(defaultDriver, goldDriver, silverDriver, goldNode, defaultNode).Build() - changed, err := AssignOwners(context.Background(), k8sClient, false) + changed, err := AssignOwners(context.Background(), k8sClient) require.Error(t, err) require.False(t, changed) require.Contains(t, err.Error(), "multiple NVIDIADrivers match the same node") diff --git a/internal/state/dcgm_exporter_test.go b/internal/state/dcgm_exporter_test.go index e24dac06d7..a404eb0732 100644 --- a/internal/state/dcgm_exporter_test.go +++ b/internal/state/dcgm_exporter_test.go @@ -88,9 +88,7 @@ func TestDCGMExporterEnabledByDefault(t *testing.T) { ds := findDaemonSet(t, objs) podSpec := ds.Spec.Template.Spec - assert.Equal(t, "true", podSpec.NodeSelector["nvidia.com/gpu.deploy.dcgm-exporter"]) - // The mode gate keeps the DRA-stack exporter off device-plugin nodes. - assert.Equal(t, "dra", podSpec.NodeSelector["nvidia.com/gpu-operator.resource-allocation.mode"]) + assert.Equal(t, "true", podSpec.NodeSelector["nvidia.com/gpu.deploy.dcgm-exporter-dra"]) require.NotNil(t, podSpec.AutomountServiceAccountToken) assert.False(t, *podSpec.AutomountServiceAccountToken) diff --git a/internal/state/dcgm_test.go b/internal/state/dcgm_test.go index f9d046fadc..30c39ed5ec 100644 --- a/internal/state/dcgm_test.go +++ b/internal/state/dcgm_test.go @@ -124,7 +124,7 @@ func TestDCGMEnabled(t *testing.T) { ds := findDaemonSet(t, objs) podSpec := ds.Spec.Template.Spec - assert.Equal(t, "true", podSpec.NodeSelector["nvidia.com/gpu.deploy.dcgm"]) + assert.Equal(t, "true", podSpec.NodeSelector["nvidia.com/gpu.deploy.dcgm-dra"]) require.Len(t, podSpec.Containers, 1) ctr := podSpec.Containers[0] assert.Equal(t, "nvidia-dcgm-ctr", ctr.Name) diff --git a/internal/state/testdata/golden/gpucluster-dcgm-exporter-embedded.yaml b/internal/state/testdata/golden/gpucluster-dcgm-exporter-embedded.yaml index 9b32029fef..1120ea22ea 100644 --- a/internal/state/testdata/golden/gpucluster-dcgm-exporter-embedded.yaml +++ b/internal/state/testdata/golden/gpucluster-dcgm-exporter-embedded.yaml @@ -122,8 +122,7 @@ spec: name: pod-gpu-resources readOnly: true nodeSelector: - nvidia.com/gpu-operator.resource-allocation.mode: dra - nvidia.com/gpu.deploy.dcgm-exporter: "true" + nvidia.com/gpu.deploy.dcgm-exporter-dra: "true" priorityClassName: system-node-critical resourceClaims: - name: admin-gpus diff --git a/internal/state/testdata/golden/gpucluster-dcgm-exporter-remote-engine.yaml b/internal/state/testdata/golden/gpucluster-dcgm-exporter-remote-engine.yaml index cc93b3ca86..104ef9a2c7 100644 --- a/internal/state/testdata/golden/gpucluster-dcgm-exporter-remote-engine.yaml +++ b/internal/state/testdata/golden/gpucluster-dcgm-exporter-remote-engine.yaml @@ -124,8 +124,7 @@ spec: name: pod-gpu-resources readOnly: true nodeSelector: - nvidia.com/gpu-operator.resource-allocation.mode: dra - nvidia.com/gpu.deploy.dcgm-exporter: "true" + nvidia.com/gpu.deploy.dcgm-exporter-dra: "true" priorityClassName: system-node-critical resourceClaims: - name: admin-gpus diff --git a/internal/state/testdata/golden/gpucluster-dcgm.yaml b/internal/state/testdata/golden/gpucluster-dcgm.yaml index c5694d1ff7..f60d64b712 100644 --- a/internal/state/testdata/golden/gpucluster-dcgm.yaml +++ b/internal/state/testdata/golden/gpucluster-dcgm.yaml @@ -84,8 +84,7 @@ spec: securityContext: privileged: true nodeSelector: - nvidia.com/gpu-operator.resource-allocation.mode: dra - nvidia.com/gpu.deploy.dcgm: "true" + nvidia.com/gpu.deploy.dcgm-dra: "true" priorityClassName: system-node-critical resourceClaims: - name: admin-gpus diff --git a/internal/state/testdata/golden/gpucluster-dra-validation.yaml b/internal/state/testdata/golden/gpucluster-dra-validation.yaml index a82abc8f91..07f9ec34e2 100644 --- a/internal/state/testdata/golden/gpucluster-dra-validation.yaml +++ b/internal/state/testdata/golden/gpucluster-dra-validation.yaml @@ -72,7 +72,6 @@ spec: claims: - name: validation-gpu nodeSelector: - nvidia.com/gpu-operator.resource-allocation.mode: dra nvidia.com/gpu.deploy.dra-validator: "true" priorityClassName: system-node-critical resourceClaims: diff --git a/manifests/state-dcgm-exporter/0700_daemonset.yaml b/manifests/state-dcgm-exporter/0700_daemonset.yaml index 22aed627fd..242bac4a62 100644 --- a/manifests/state-dcgm-exporter/0700_daemonset.yaml +++ b/manifests/state-dcgm-exporter/0700_daemonset.yaml @@ -45,8 +45,7 @@ spec: # 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. nodeSelector: - nvidia.com/gpu.deploy.dcgm-exporter: "true" - nvidia.com/gpu-operator.resource-allocation.mode: "dra" + nvidia.com/gpu.deploy.dcgm-exporter-dra: "true" {{- if .DCGMExporter.Spec.ImagePullSecrets }} imagePullSecrets: {{- range .DCGMExporter.Spec.ImagePullSecrets }} diff --git a/manifests/state-dcgm/0500_daemonset.yaml b/manifests/state-dcgm/0500_daemonset.yaml index bccd805a28..1cc511a813 100644 --- a/manifests/state-dcgm/0500_daemonset.yaml +++ b/manifests/state-dcgm/0500_daemonset.yaml @@ -41,8 +41,7 @@ spec: # Gate scheduling on the per-component deploy label so the k8s-driver-manager # can pause it to drain DCGM off a node during a driver reload. nodeSelector: - nvidia.com/gpu.deploy.dcgm: "true" - nvidia.com/gpu-operator.resource-allocation.mode: "dra" + nvidia.com/gpu.deploy.dcgm-dra: "true" {{- if .DCGM.Spec.ImagePullSecrets }} imagePullSecrets: {{- range .DCGM.Spec.ImagePullSecrets }} diff --git a/manifests/state-dra-validation/0500_daemonset.yaml b/manifests/state-dra-validation/0500_daemonset.yaml index 1c78e6ebfc..5c8e2a4ca2 100644 --- a/manifests/state-dra-validation/0500_daemonset.yaml +++ b/manifests/state-dra-validation/0500_daemonset.yaml @@ -62,7 +62,6 @@ spec: # pause it to drain (and force a re-validate of) the validator during a driver reload. nodeSelector: nvidia.com/gpu.deploy.dra-validator: "true" - nvidia.com/gpu-operator.resource-allocation.mode: "dra" {{- if .Validator.Spec.ImagePullSecrets }} imagePullSecrets: {{- range .Validator.Spec.ImagePullSecrets }}