diff --git a/cmd/gpu-operator/main.go b/cmd/gpu-operator/main.go index 4e94db1f2..281e2667c 100644 --- a/cmd/gpu-operator/main.go +++ b/cmd/gpu-operator/main.go @@ -193,11 +193,12 @@ func main() { WithRestartOnlyPredicate(predicates.DriverPodRestartOnly(upgradeLogger)) if err = (&controllers.UpgradeReconciler{ - Client: mgr.GetClient(), - Log: upgradeLogger, - Scheme: mgr.GetScheme(), - StateManager: clusterUpgradeStateManager, - OperatorMetrics: operatorMetrics, + Client: mgr.GetClient(), + Log: upgradeLogger, + Scheme: mgr.GetScheme(), + StateManager: clusterUpgradeStateManager, + OperatorMetrics: operatorMetrics, + OperatorNamespace: operatorNamespace, }).SetupWithManager(ctx, mgr); err != nil { setupLog.Error(err, "unable to create controller", "controller", "Upgrade") os.Exit(1) diff --git a/controllers/gpucluster_controller_test.go b/controllers/gpucluster_controller_test.go index 7e9eb6519..97a0bf1db 100644 --- a/controllers/gpucluster_controller_test.go +++ b/controllers/gpucluster_controller_test.go @@ -71,14 +71,17 @@ func newGPUClusterReconciler(t *testing.T, objs ...client.Object) (*GPUClusterRe } // fakeStateManager returns canned SyncState results so the controller tests don't load -// real manifests. GetWatchSources is promoted from the embedded (nil) interface and is -// never called here — only SetupWithManager calls it, which these tests skip. +// real manifests. It records the last info catalog passed to SyncState so tests can +// assert on its entries. GetWatchSources is promoted from the embedded (nil) interface +// and is never called here — only SetupWithManager calls it, which these tests skip. type fakeStateManager struct { state.Manager - results state.Results + results state.Results + lastCatalog state.InfoCatalog } -func (f *fakeStateManager) SyncState(_ context.Context, _ interface{}, _ state.InfoCatalog) state.Results { +func (f *fakeStateManager) SyncState(_ context.Context, _ interface{}, catalog state.InfoCatalog) state.Results { + f.lastCatalog = catalog return f.results } diff --git a/controllers/nvidiadriver_controller.go b/controllers/nvidiadriver_controller.go index 94dfa6212..3c7d48958 100644 --- a/controllers/nvidiadriver_controller.go +++ b/controllers/nvidiadriver_controller.go @@ -64,6 +64,7 @@ type NVIDIADriverReconciler struct { //+kubebuilder:rbac:groups=nvidia.com,resources=nvidiadrivers,verbs=get;list;watch;create;update;patch;delete //+kubebuilder:rbac:groups=nvidia.com,resources=nvidiadrivers/status,verbs=get;update;patch //+kubebuilder:rbac:groups=nvidia.com,resources=nvidiadrivers/finalizers,verbs=update +//+kubebuilder:rbac:groups=nvidia.com,resources=gpuclusters,verbs=get;list;watch // Reconcile is part of the main kubernetes reconciliation loop which aims to // move the current state of the cluster closer to the desired state. @@ -98,45 +99,26 @@ func (r *NVIDIADriverReconciler) Reconcile(ctx context.Context, req ctrl.Request return reconcile.Result{}, nil } - // Get the singleton NVIDIA ClusterPolicy object in the cluster. - clusterPolicyList := &gpuv1.ClusterPolicyList{} - if err := r.List(ctx, clusterPolicyList); err != nil { - wrappedErr := fmt.Errorf("error getting ClusterPolicy list: %w", err) - logger.Error(err, "error getting ClusterPolicy list") - instance.Status.State = nvidiav1alpha1.NotReady - if condErr := r.conditionUpdater.SetConditionsError(ctx, instance, conditions.ReconcileFailed, err.Error()); condErr != nil { - logger.Error(condErr, "failed to set condition") - } - return reconcile.Result{}, wrappedErr - } - - if len(clusterPolicyList.Items) == 0 { - err := fmt.Errorf("no ClusterPolicy object found in the cluster") - logger.Error(err, "failed to get ClusterPolicy object") + // Source the cluster-wide host root from the active configuration: a ClusterPolicy takes + // precedence, otherwise the controller runs standalone against the GPUCluster. + clusterPolicy, gpuCluster, err := resolveActiveConfig(ctx, r.Client) + if err != nil { + logger.Error(err, "error resolving active cluster configuration") instance.Status.State = nvidiav1alpha1.NotReady if condErr := r.conditionUpdater.SetConditionsError(ctx, instance, conditions.ReconcileFailed, err.Error()); condErr != nil { logger.Error(condErr, "failed to set condition") } return reconcile.Result{}, err } - clusterPolicyInstance := clusterPolicyList.Items[0] // Ensure the NVIDIADriver CR has a consumer: either the ClusterPolicy delegates its // driver to the NVIDIADriver CRD, or a GPUCluster exists. GPUCluster does // not manage the driver itself — it is either preinstalled on the host (no NVIDIADriver // CR) or installed via NVIDIADriver CRs, so any CR that exists alongside one is in use. - if !clusterPolicyInstance.Spec.Driver.UseNvidiaDriverCRDType() { - gpuClusters := &nvidiav1alpha1.GPUClusterList{} - if err := r.List(ctx, gpuClusters); err != nil { - wrappedErr := fmt.Errorf("error getting GPUCluster list: %w", err) - logger.Error(err, "error getting GPUCluster list") - instance.Status.State = nvidiav1alpha1.NotReady - if condErr := r.conditionUpdater.SetConditionsError(ctx, instance, conditions.ReconcileFailed, err.Error()); condErr != nil { - logger.Error(condErr, "failed to set condition") - } - return reconcile.Result{}, wrappedErr - } - if len(gpuClusters.Items) == 0 { + var hostRoot string + switch { + case clusterPolicy != nil: + if !clusterPolicy.Spec.Driver.UseNvidiaDriverCRDType() && gpuCluster == nil { msg := "useNvidiaDriverCRD is not enabled in ClusterPolicy and no GPUCluster exists" logger.V(consts.LogLevelWarning).Info("NVIDIADriver reconciliation skipped", "reason", msg) instance.Status.State = nvidiav1alpha1.Disabled @@ -145,6 +127,17 @@ func (r *NVIDIADriverReconciler) Reconcile(ctx context.Context, req ctrl.Request } return reconcile.Result{}, nil } + hostRoot = clusterPolicy.Spec.HostPaths.RootFS + case gpuCluster != nil: + hostRoot = gpuCluster.Spec.HostPaths.RootFS + default: + err := fmt.Errorf("no ClusterPolicy or GPUCluster object found in the cluster") + logger.Error(err, "failed to get a cluster-wide configuration object") + instance.Status.State = nvidiav1alpha1.NotReady + if condErr := r.conditionUpdater.SetConditionsError(ctx, instance, conditions.ReconcileFailed, err.Error()); condErr != nil { + logger.Error(condErr, "failed to set condition") + } + return reconcile.Result{}, err } // Create a new InfoCatalog which is a generic interface for passing information to state managers @@ -153,8 +146,8 @@ func (r *NVIDIADriverReconciler) Reconcile(ctx context.Context, req ctrl.Request // Add an entry for ClusterInfo, which was collected before the NVIDIADriver controller was started infoCatalog.Add(state.InfoTypeClusterInfo, r.ClusterInfo) - // Add an entry for Clusterpolicy, which is needed to deploy the driver daemonset - infoCatalog.Add(state.InfoTypeClusterPolicyCR, clusterPolicyInstance) + // Add the host root, which is needed to deploy the driver daemonset + infoCatalog.Add(state.InfoTypeHostRoot, hostRoot) // Verify the nodeSelector configured for this NVIDIADriver instance does // not conflict with any other instances. This ensures only one driver @@ -405,6 +398,7 @@ func (r *NVIDIADriverReconciler) SetupWithManager(ctx context.Context, mgr ctrl. gpuClusterMapFn := func(ctx context.Context, _ *nvidiav1alpha1.GPUCluster) []reconcile.Request { return r.enqueueAllNVIDIADrivers(ctx) } + err = c.Watch( source.Kind( mgr.GetCache(), diff --git a/controllers/nvidiadriver_controller_test.go b/controllers/nvidiadriver_controller_test.go index dd5d2d00b..0e85823c0 100644 --- a/controllers/nvidiadriver_controller_test.go +++ b/controllers/nvidiadriver_controller_test.go @@ -240,6 +240,96 @@ func TestReconcile(t *testing.T) { } } +// TestReconcileStandalone covers the no-ClusterPolicy path: the controller falls back +// to the GPUCluster for the cluster-wide configuration, and fails early when +// neither object exists. +func TestReconcileStandalone(t *testing.T) { + scheme := runtime.NewScheme() + require.NoError(t, nvidiav1alpha1.AddToScheme(scheme)) + require.NoError(t, gpuv1.AddToScheme(scheme)) + + cp := &gpuv1.ClusterPolicy{ + ObjectMeta: metav1.ObjectMeta{Name: "cluster-policy"}, + Spec: gpuv1.ClusterPolicySpec{ + Driver: gpuv1.DriverSpec{ + UseNvidiaDriverCRD: ptr.To(true), + }, + HostPaths: gpuv1.HostPathsSpec{RootFS: "/cp-root"}, + }, + } + gpuCluster := &nvidiav1alpha1.GPUCluster{ + ObjectMeta: metav1.ObjectMeta{Name: "gpu-cluster-config"}, + Spec: nvidiav1alpha1.GPUClusterSpec{ + HostPaths: gpuv1.HostPathsSpec{RootFS: "/gpuCluster-root"}, + }, + } + + tests := []struct { + name string + objects []client.Object + expectedErr string + expectedHostRoot string + }{ + { + name: "no ClusterPolicy, GPUCluster provides the host root", + objects: []client.Object{gpuCluster}, + expectedHostRoot: "/gpuCluster-root", + }, + { + name: "ClusterPolicy preferred over GPUCluster", + objects: []client.Object{cp, gpuCluster}, + expectedHostRoot: "/cp-root", + }, + { + name: "neither ClusterPolicy nor GPUCluster", + expectedErr: "no ClusterPolicy or GPUCluster object found in the cluster", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + driver := &nvidiav1alpha1.NVIDIADriver{ + ObjectMeta: metav1.ObjectMeta{Name: "test-driver"}, + } + + c := fake.NewClientBuilder(). + WithScheme(scheme). + WithObjects(append([]client.Object{driver}, tc.objects...)...). + WithStatusSubresource(&nvidiav1alpha1.NVIDIADriver{}). + Build() + + updater := &FakeConditionUpdater{} + stateManager := &fakeStateManager{results: state.Results{Status: state.SyncStateReady}} + + reconciler := &NVIDIADriverReconciler{ + Client: c, + Scheme: scheme, + conditionUpdater: updater, + nodeSelectorValidator: &FakeNodeSelectorValidator{}, + stateManager: stateManager, + } + + req := ctrl.Request{NamespacedName: types.NamespacedName{Name: driver.Name}} + _, err := reconciler.Reconcile(context.Background(), req) + + if tc.expectedErr != "" { + require.ErrorContains(t, err, tc.expectedErr) + require.Equal(t, nvidiav1alpha1.NotReady, updater.LastErrorState) + return + } + require.NoError(t, err) + + hostRoot, ok := stateManager.lastCatalog.Get(state.InfoTypeHostRoot).(string) + require.True(t, ok, "info catalog must hold a host root string") + require.Equal(t, tc.expectedHostRoot, hostRoot) + + instance := &nvidiav1alpha1.NVIDIADriver{} + require.NoError(t, c.Get(context.Background(), types.NamespacedName{Name: driver.Name}, instance)) + require.Equal(t, nvidiav1alpha1.Ready, instance.Status.State) + }) + } +} + func TestReconcileConflictSetsNotReadyState(t *testing.T) { scheme := runtime.NewScheme() require.NoError(t, nvidiav1alpha1.AddToScheme(scheme)) diff --git a/controllers/upgrade_controller.go b/controllers/upgrade_controller.go index e551a90f8..aa1a3fab2 100644 --- a/controllers/upgrade_controller.go +++ b/controllers/upgrade_controller.go @@ -22,7 +22,6 @@ import ( "time" corev1 "k8s.io/api/core/v1" - apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/types" @@ -31,7 +30,6 @@ import ( "sigs.k8s.io/controller-runtime/pkg/controller" "sigs.k8s.io/controller-runtime/pkg/event" "sigs.k8s.io/controller-runtime/pkg/handler" - "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/predicate" "sigs.k8s.io/controller-runtime/pkg/reconcile" "sigs.k8s.io/controller-runtime/pkg/source" @@ -53,13 +51,18 @@ import ( // UpgradeReconciler reconciles Driver Daemon Sets for upgrade type UpgradeReconciler struct { client.Client - Log logr.Logger - Scheme *runtime.Scheme - StateManager upgrade.ClusterUpgradeStateManager - OperatorMetrics *OperatorMetrics + Log logr.Logger + Scheme *runtime.Scheme + StateManager upgrade.ClusterUpgradeStateManager + OperatorMetrics *OperatorMetrics + OperatorNamespace string } const ( + // upgradeControllerSingletonName is the request name every watch enqueues; the + // reconciler resolves the active configuration source itself. + upgradeControllerSingletonName = "driver-upgrade" + plannedRequeueInterval = time.Minute * 2 // DriverLabelKey indicates pod label key of the driver DriverLabelKey = "app" @@ -87,22 +90,22 @@ func (r *UpgradeReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct reqLogger := r.Log.WithValues("upgrade", req.NamespacedName) reqLogger.V(consts.LogLevelInfo).Info("Reconciling Upgrade") - // Fetch the ClusterPolicy instance - clusterPolicy := &gpuv1.ClusterPolicy{} - err := r.Get(ctx, req.NamespacedName, clusterPolicy) + // Requests carry the singleton name rather than a CR reference, so re-resolve the + // active configuration source on every reconcile. + clusterPolicy, _, err := resolveActiveConfig(ctx, r.Client) if err != nil { - reqLogger.Error(err, "Error getting ClusterPolicy object") + reqLogger.Error(err, "Error resolving the active configuration source") r.OperatorMetrics.reconciliationStatus.Set(reconciliationStatusClusterPolicyUnavailable) - if apierrors.IsNotFound(err) { - // Request object not found, could have been deleted after reconcile request. - // Owned objects are automatically garbage collected. For additional cleanup logic use finalizers. - // Return and don't requeue - return reconcile.Result{}, nil - } - // Error reading the object - requeue the request. return reconcile.Result{}, err } + if clusterPolicy == nil { + // Without a ClusterPolicy, any operator-managed driver belongs to NVIDIADriver + // CRs; with none of those either, this cleans up stale upgrade-state labels + // and no-ops. + return r.reconcileNVIDIADriverUpgrades(ctx, reqLogger) + } + if clusterPolicy.Spec.SandboxWorkloads.IsEnabled() { reqLogger.V(consts.LogLevelInfo).Info("Advanced driver upgrade policy is not supported when 'sandboxWorkloads.enabled=true'" + "in ClusterPolicy, cleaning up upgrade state and skipping reconciliation") @@ -110,9 +113,6 @@ func (r *UpgradeReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct return ctrl.Result{}, r.removeNodeUpgradeStateLabels(ctx) } - // TODO: When integrating the NVIDIA DRA Driver for GPUs, decouple - // the driver-upgrade controller from ClusterPolicy. If a ClusterPolicy - // CR does not exist, take the NVIDIADriver code path. if clusterPolicy.Spec.Driver.UseNvidiaDriverCRDType() { return r.reconcileNVIDIADriverUpgrades(ctx, reqLogger) } @@ -147,7 +147,7 @@ func (r *UpgradeReconciler) reconcileClusterPolicyDriverUpgrades(ctx context.Con driverLabel = map[string]string{driverLabelKey: driverLabelValue} reqLogger.Info("Using label selector", "key", driverLabelKey, "value", driverLabelValue) - state, err := r.StateManager.BuildState(ctx, clusterPolicyCtrl.operatorNamespace, + state, err := r.StateManager.BuildState(ctx, r.OperatorNamespace, driverLabel) if err != nil { r.Log.Error(err, "Failed to build cluster upgrade state") @@ -234,8 +234,7 @@ func (r *UpgradeReconciler) reconcileNVIDIADriverUpgrades(ctx context.Context, r // Build a cluster-wide upgrade state using only the component label so that ALL // driver pods are captured, including orphaned pods (e.g. pods left over from a // ClusterPolicy-managed DaemonSet). - // TODO: decouple the operatorNamespace field from the ClusterPolicyController object - clusterState, err := r.StateManager.BuildState(ctx, clusterPolicyCtrl.operatorNamespace, map[string]string{AppComponentLabelKey: DriverAppComponentLabelValue}) + clusterState, err := r.StateManager.BuildState(ctx, r.OperatorNamespace, map[string]string{AppComponentLabelKey: DriverAppComponentLabelValue}) if err != nil { r.Log.Error(err, "Failed to build cluster upgrade state") return ctrl.Result{}, err @@ -393,21 +392,40 @@ func (r *UpgradeReconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manag return err } - // Watch for changes to primary resource ClusterPolicy + mapToSingleton := func(_ context.Context, _ client.Object) []reconcile.Request { + return []reconcile.Request{{NamespacedName: types.NamespacedName{Name: upgradeControllerSingletonName}}} + } + + // Watch ClusterPolicy; its deletion switches reconciliation to the NVIDIADriver path. + cpMapFn := func(ctx context.Context, cp *gpuv1.ClusterPolicy) []reconcile.Request { + return mapToSingleton(ctx, cp) + } err = c.Watch(source.Kind( mgr.GetCache(), &gpuv1.ClusterPolicy{}, - &handler.TypedEnqueueRequestForObject[*gpuv1.ClusterPolicy]{}, + handler.TypedEnqueueRequestsFromMapFunc(cpMapFn), predicate.TypedGenerationChangedPredicate[*gpuv1.ClusterPolicy]{}), ) if err != nil { return err } - // Define a mapping from the Node object in the event to one or more - // ClusterPolicy objects to Reconcile - nodeMapFn := func(ctx context.Context, o *corev1.Node) []reconcile.Request { - return getClusterPoliciesToReconcile(ctx, mgr.GetClient()) + // Watch NVIDIADriver so upgradePolicy changes are reconciled even with no ClusterPolicy. + nvdMapFn := func(ctx context.Context, nd *nvidiav1alpha1.NVIDIADriver) []reconcile.Request { + return mapToSingleton(ctx, nd) + } + err = c.Watch(source.Kind( + mgr.GetCache(), + &nvidiav1alpha1.NVIDIADriver{}, + handler.TypedEnqueueRequestsFromMapFunc(nvdMapFn), + predicate.TypedGenerationChangedPredicate[*nvidiav1alpha1.NVIDIADriver]{}), + ) + if err != nil { + return err + } + + nodeMapFn := func(ctx context.Context, n *corev1.Node) []reconcile.Request { + return mapToSingleton(ctx, n) } // Only watch for changes to the upgrade state label @@ -430,9 +448,6 @@ func (r *UpgradeReconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manag return err } - // Define a mapping between the DaemonSet object in the event - // to one or more ClusterPolicy instances to reconcile. - // // For events generated by DaemonSets, ensure the object is // owned by either ClusterPolicy or NVIDIADriver. dsMapFn := func(ctx context.Context, a *appsv1.DaemonSet) []reconcile.Request { @@ -451,10 +466,10 @@ func (r *UpgradeReconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manag return nil } - return getClusterPoliciesToReconcile(ctx, mgr.GetClient()) + return mapToSingleton(ctx, a) } - // Watch for changes to NVIDIA driver daemonsets and enqueue ClusterPolicy + // Watch for changes to NVIDIA driver daemonsets // TODO: use one common label to identify all NVIDIA driver DaemonSets appLabelSelector := predicate.NewTypedPredicateFuncs(func(ds *appsv1.DaemonSet) bool { ls := metav1.LabelSelector{MatchLabels: map[string]string{DriverLabelKey: DriverLabelValue}} @@ -490,26 +505,3 @@ func (r *UpgradeReconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manag return nil } - -func getClusterPoliciesToReconcile(ctx context.Context, k8sClient client.Client) []reconcile.Request { - logger := log.FromContext(ctx) - opts := []client.ListOption{} - list := &gpuv1.ClusterPolicyList{} - - err := k8sClient.List(ctx, list, opts...) - if err != nil { - logger.Error(err, "Unable to list ClusterPolicies") - return []reconcile.Request{} - } - - cpToRec := []reconcile.Request{} - - for _, cp := range list.Items { - cpToRec = append(cpToRec, reconcile.Request{NamespacedName: types.NamespacedName{ - Name: cp.GetName(), - Namespace: cp.GetNamespace(), - }}) - } - - return cpToRec -} diff --git a/controllers/upgrade_controller_test.go b/controllers/upgrade_controller_test.go index 3b72da082..56642d8ef 100644 --- a/controllers/upgrade_controller_test.go +++ b/controllers/upgrade_controller_test.go @@ -17,11 +17,28 @@ package controllers import ( + "context" "fmt" "testing" - upgrade_v1alpha1 "github.com/NVIDIA/k8s-operator-libs/api/upgrade/v1alpha1" + "github.com/go-logr/logr" + promcli "github.com/prometheus/client_golang/prometheus" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + + upgrade_v1alpha1 "github.com/NVIDIA/k8s-operator-libs/api/upgrade/v1alpha1" + "github.com/NVIDIA/k8s-operator-libs/pkg/upgrade" + + gpuv1 "github.com/NVIDIA/gpu-operator/api/nvidia/v1" + nvidiav1alpha1 "github.com/NVIDIA/gpu-operator/api/nvidia/v1alpha1" + gpuconsts "github.com/NVIDIA/gpu-operator/internal/consts" ) func TestSetDrainSpecPodSelector(t *testing.T) { @@ -69,3 +86,227 @@ func TestSetDrainSpecPodSelector(t *testing.T) { }) } } + +// fakeUpgradeStateManager records BuildState/ApplyState calls and serves a canned state. +type fakeUpgradeStateManager struct { + state upgrade.ClusterUpgradeState + buildCalls int + buildNamespace string + buildLabels map[string]string + applyCalls int + appliedPolicies []*upgrade_v1alpha1.DriverUpgradePolicySpec +} + +func (f *fakeUpgradeStateManager) GetTotalManagedNodes(*upgrade.ClusterUpgradeState) int { return 0 } +func (f *fakeUpgradeStateManager) GetUpgradesInProgress(*upgrade.ClusterUpgradeState) int { + return 0 +} +func (f *fakeUpgradeStateManager) GetUpgradesDone(*upgrade.ClusterUpgradeState) int { return 0 } +func (f *fakeUpgradeStateManager) GetUpgradesAvailable(*upgrade.ClusterUpgradeState, int, int) int { + return 0 +} +func (f *fakeUpgradeStateManager) GetUpgradesFailed(*upgrade.ClusterUpgradeState) int { return 0 } +func (f *fakeUpgradeStateManager) GetUpgradesPending(*upgrade.ClusterUpgradeState) int { return 0 } +func (f *fakeUpgradeStateManager) IsPodDeletionEnabled() bool { return false } +func (f *fakeUpgradeStateManager) IsValidationEnabled() bool { return false } + +func (f *fakeUpgradeStateManager) WithPodDeletionEnabled(upgrade.PodDeletionFilter) upgrade.ClusterUpgradeStateManager { + return f +} + +func (f *fakeUpgradeStateManager) WithValidationEnabled(string) upgrade.ClusterUpgradeStateManager { + return f +} + +func (f *fakeUpgradeStateManager) WithRestartOnlyPredicate(upgrade.RestartOnlyPredicate) upgrade.ClusterUpgradeStateManager { + return f +} + +func (f *fakeUpgradeStateManager) BuildState(_ context.Context, namespace string, driverLabels map[string]string) (*upgrade.ClusterUpgradeState, error) { + f.buildCalls++ + f.buildNamespace = namespace + f.buildLabels = driverLabels + return &f.state, nil +} + +func (f *fakeUpgradeStateManager) ApplyState(_ context.Context, _ *upgrade.ClusterUpgradeState, upgradePolicy *upgrade_v1alpha1.DriverUpgradePolicySpec) error { + f.applyCalls++ + f.appliedPolicies = append(f.appliedPolicies, upgradePolicy) + return nil +} + +// newTestOperatorMetrics builds the gauges the upgrade reconciler touches without +// registering them; InitOperatorMetrics panics when registered twice per binary. +func newTestOperatorMetrics() *OperatorMetrics { + newGauge := func(name string) promcli.Gauge { + return promcli.NewGauge(promcli.GaugeOpts{Name: name}) + } + return &OperatorMetrics{ + reconciliationStatus: newGauge("test_reconciliation_status"), + driverAutoUpgradeEnabled: newGauge("test_driver_auto_upgrade_enabled"), + upgradesInProgress: newGauge("test_upgrades_in_progress"), + upgradesDone: newGauge("test_upgrades_done"), + upgradesAvailable: newGauge("test_upgrades_available"), + upgradesFailed: newGauge("test_upgrades_failed"), + upgradesPending: newGauge("test_upgrades_pending"), + } +} + +const testOperatorNamespace = "test-operator-namespace" + +func newTestUpgradeReconciler(t *testing.T, objs ...client.Object) (*UpgradeReconciler, *fakeUpgradeStateManager) { + t.Helper() + upgrade.SetDriverName("gpu") + + scheme := runtime.NewScheme() + require.NoError(t, gpuv1.AddToScheme(scheme)) + require.NoError(t, nvidiav1alpha1.AddToScheme(scheme)) + require.NoError(t, corev1.AddToScheme(scheme)) + + stateManager := &fakeUpgradeStateManager{state: upgrade.NewClusterUpgradeState()} + r := &UpgradeReconciler{ + Client: fake.NewClientBuilder().WithScheme(scheme).WithObjects(objs...).Build(), + Log: logr.Discard(), + Scheme: scheme, + StateManager: stateManager, + OperatorMetrics: newTestOperatorMetrics(), + OperatorNamespace: testOperatorNamespace, + } + return r, stateManager +} + +func upgradeSingletonRequest() ctrl.Request { + return ctrl.Request{NamespacedName: types.NamespacedName{Name: upgradeControllerSingletonName}} +} + +func nodeWithUpgradeState(name, owner string) *corev1.Node { + node := &corev1.Node{ObjectMeta: metav1.ObjectMeta{ + Name: name, + Labels: map[string]string{upgrade.GetUpgradeStateLabelKey(): "upgrade-required"}, + }} + if owner != "" { + node.Labels[gpuconsts.NVIDIADriverOwnerLabel] = owner + } + return node +} + +// TestUpgradeReconcileWithoutConfigSources covers the preinstalled-driver scenario: no +// ClusterPolicy and no NVIDIADriver CRs must leave the cluster untouched apart from +// clearing stale upgrade-state labels. +func TestUpgradeReconcileWithoutConfigSources(t *testing.T) { + tests := []struct { + name string + gpuCluster *nvidiav1alpha1.GPUCluster + }{ + {name: "with a GPUCluster", gpuCluster: &nvidiav1alpha1.GPUCluster{ObjectMeta: metav1.ObjectMeta{Name: "gpu-cluster"}}}, + {name: "without a GPUCluster"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + node := nodeWithUpgradeState("node-1", "") + objs := []client.Object{node} + if tt.gpuCluster != nil { + objs = append(objs, tt.gpuCluster) + } + r, stateManager := newTestUpgradeReconciler(t, objs...) + + result, err := r.Reconcile(context.Background(), upgradeSingletonRequest()) + require.NoError(t, err) + assert.Equal(t, ctrl.Result{}, result) + assert.Zero(t, stateManager.buildCalls) + assert.Zero(t, stateManager.applyCalls) + + // The stale upgrade-state label is stripped; no CR by design is not an error. + updated := &corev1.Node{} + require.NoError(t, r.Get(context.Background(), types.NamespacedName{Name: node.Name}, updated)) + assert.NotContains(t, updated.Labels, upgrade.GetUpgradeStateLabelKey()) + }) + } +} + +func TestUpgradeReconcileNVIDIADriverWithoutClusterPolicy(t *testing.T) { + nvd := &nvidiav1alpha1.NVIDIADriver{ObjectMeta: metav1.ObjectMeta{Name: "gpu-driver"}} + node := nodeWithUpgradeState("node-1", nvd.Name) + r, stateManager := newTestUpgradeReconciler(t, nvd, node) + stateManager.state.NodeStates["upgrade-required"] = []*upgrade.NodeUpgradeState{{Node: node}} + + result, err := r.Reconcile(context.Background(), upgradeSingletonRequest()) + require.NoError(t, err) + assert.Equal(t, plannedRequeueInterval, result.RequeueAfter) + + assert.Equal(t, 1, stateManager.buildCalls) + assert.Equal(t, testOperatorNamespace, stateManager.buildNamespace) + assert.Equal(t, map[string]string{AppComponentLabelKey: DriverAppComponentLabelValue}, stateManager.buildLabels) + + require.Len(t, stateManager.appliedPolicies, 1) + assert.True(t, stateManager.appliedPolicies[0].AutoUpgrade, "nil upgradePolicy must default to autoUpgrade enabled") +} + +func TestUpgradeReconcileNVIDIADriverAutoUpgradeDisabledScope(t *testing.T) { + enabled := &nvidiav1alpha1.NVIDIADriver{ObjectMeta: metav1.ObjectMeta{Name: "enabled-driver"}} + disabled := &nvidiav1alpha1.NVIDIADriver{ + ObjectMeta: metav1.ObjectMeta{Name: "disabled-driver"}, + Spec: nvidiav1alpha1.NVIDIADriverSpec{ + UpgradePolicy: &nvidiav1alpha1.DriverUpgradePolicySpec{AutoUpgrade: false}, + }, + } + enabledNode := nodeWithUpgradeState("node-enabled", enabled.Name) + disabledNode := nodeWithUpgradeState("node-disabled", disabled.Name) + r, stateManager := newTestUpgradeReconciler(t, enabled, disabled, enabledNode, disabledNode) + stateManager.state.NodeStates["upgrade-required"] = []*upgrade.NodeUpgradeState{{Node: enabledNode}, {Node: disabledNode}} + + _, err := r.Reconcile(context.Background(), upgradeSingletonRequest()) + require.NoError(t, err) + + // Cleanup is scoped to the disabled CR's nodes; the enabled CR still gets ApplyState. + assert.Equal(t, 1, stateManager.applyCalls) + updated := &corev1.Node{} + require.NoError(t, r.Get(context.Background(), types.NamespacedName{Name: disabledNode.Name}, updated)) + assert.NotContains(t, updated.Labels, upgrade.GetUpgradeStateLabelKey()) + require.NoError(t, r.Get(context.Background(), types.NamespacedName{Name: enabledNode.Name}, updated)) + assert.Contains(t, updated.Labels, upgrade.GetUpgradeStateLabelKey()) +} + +func TestUpgradeReconcileClusterPolicyPathsUnchanged(t *testing.T) { + t.Run("legacy driver builds state with the daemonset app label", func(t *testing.T) { + cp := &gpuv1.ClusterPolicy{ObjectMeta: metav1.ObjectMeta{Name: "cluster-policy"}} + cp.Spec.Driver.UpgradePolicy = &upgrade_v1alpha1.DriverUpgradePolicySpec{AutoUpgrade: true} + r, stateManager := newTestUpgradeReconciler(t, cp) + + result, err := r.Reconcile(context.Background(), upgradeSingletonRequest()) + require.NoError(t, err) + assert.Equal(t, plannedRequeueInterval, result.RequeueAfter) + assert.Equal(t, map[string]string{DriverLabelKey: DriverLabelValue}, stateManager.buildLabels) + assert.Equal(t, 1, stateManager.applyCalls) + }) + + t.Run("sandbox workloads clean up and skip", func(t *testing.T) { + cp := &gpuv1.ClusterPolicy{ObjectMeta: metav1.ObjectMeta{Name: "cluster-policy"}} + enabled := true + cp.Spec.SandboxWorkloads.Enabled = &enabled + node := nodeWithUpgradeState("node-1", "") + r, stateManager := newTestUpgradeReconciler(t, cp, node) + + result, err := r.Reconcile(context.Background(), upgradeSingletonRequest()) + require.NoError(t, err) + assert.Equal(t, ctrl.Result{}, result) + assert.Zero(t, stateManager.buildCalls) + + updated := &corev1.Node{} + require.NoError(t, r.Get(context.Background(), types.NamespacedName{Name: node.Name}, updated)) + assert.NotContains(t, updated.Labels, upgrade.GetUpgradeStateLabelKey()) + }) + + t.Run("useNvidiaDriverCRD takes the NVIDIADriver path", func(t *testing.T) { + cp := &gpuv1.ClusterPolicy{ObjectMeta: metav1.ObjectMeta{Name: "cluster-policy"}} + useCRD := true + cp.Spec.Driver.UseNvidiaDriverCRD = &useCRD + nvd := &nvidiav1alpha1.NVIDIADriver{ObjectMeta: metav1.ObjectMeta{Name: "gpu-driver"}} + r, stateManager := newTestUpgradeReconciler(t, cp, nvd) + + _, err := r.Reconcile(context.Background(), upgradeSingletonRequest()) + require.NoError(t, err) + assert.Equal(t, map[string]string{AppComponentLabelKey: DriverAppComponentLabelValue}, stateManager.buildLabels) + }) +} diff --git a/internal/state/driver.go b/internal/state/driver.go index 0488b0efb..8c84639a9 100644 --- a/internal/state/driver.go +++ b/internal/state/driver.go @@ -40,7 +40,6 @@ import ( "sigs.k8s.io/controller-runtime/pkg/predicate" "sigs.k8s.io/controller-runtime/pkg/source" - gpuv1 "github.com/NVIDIA/gpu-operator/api/nvidia/v1" nvidiav1alpha1 "github.com/NVIDIA/gpu-operator/api/nvidia/v1alpha1" "github.com/NVIDIA/gpu-operator/controllers/clusterinfo" driverconfig "github.com/NVIDIA/gpu-operator/internal/config" @@ -252,11 +251,14 @@ func (s *stateDriver) cleanupStaleDriverDaemonsets(ctx context.Context, cr *nvid func (s *stateDriver) getManifestObjects(ctx context.Context, cr *nvidiav1alpha1.NVIDIADriver, infoCatalog InfoCatalog) ([]*unstructured.Unstructured, error) { logger := log.FromContext(ctx) - info := infoCatalog.Get(InfoTypeClusterPolicyCR) + info := infoCatalog.Get(InfoTypeHostRoot) if info == nil { - return nil, fmt.Errorf("failed to get ClusterPolicy CR from info catalog") + return nil, fmt.Errorf("failed to get host root from info catalog") + } + hostRoot, ok := info.(string) + if !ok { + return nil, fmt.Errorf("host root in info catalog has unexpected type %T", info) } - clusterPolicy := info.(gpuv1.ClusterPolicy) info = infoCatalog.Get(InfoTypeClusterInfo) if info == nil { @@ -280,7 +282,7 @@ func (s *stateDriver) getManifestObjects(ctx context.Context, cr *nvidiav1alpha1 renderData := &driverRenderData{ GPUDirectRDMA: gpuDirectRDMASpec, Runtime: runtimeSpec, - HostRoot: clusterPolicy.Spec.HostPaths.RootFS, + HostRoot: hostRoot, } if len(nodePools) == 0 { diff --git a/internal/state/driver_test.go b/internal/state/driver_test.go index 572f2c7a6..e1d43b6ba 100644 --- a/internal/state/driver_test.go +++ b/internal/state/driver_test.go @@ -199,6 +199,20 @@ func TestDriverHostSysDevicesSystemVolumeUsesStableParentDirectory(t *testing.T) assert.Empty(t, hostSysDevicesSystemMount.SubPath) } +func TestDriverRenderMissingHostRoot(t *testing.T) { + state, err := NewStateDriver(nil, "", nil, manifestDir) + require.Nil(t, err) + stateDriver, ok := state.(*stateDriver) + require.True(t, ok) + + catalog := NewInfoCatalog() + catalog.Add(InfoTypeClusterInfo, testClusterInfo{}) + + _, err = stateDriver.getManifestObjects(context.Background(), &nvidiav1alpha1.NVIDIADriver{}, catalog) + require.Error(t, err, "rendering must fail when no host root is in the catalog") + require.Contains(t, err.Error(), "host root") +} + func TestDriverHostNetwork(t *testing.T) { const ( testName = "driver-hostnetwork" diff --git a/internal/state/info_source.go b/internal/state/info_source.go index 4f6409a90..a30b0b6bb 100644 --- a/internal/state/info_source.go +++ b/internal/state/info_source.go @@ -20,7 +20,7 @@ type InfoType uint const ( InfoTypeClusterInfo = iota - InfoTypeClusterPolicyCR + InfoTypeHostRoot ) func NewInfoCatalog() InfoCatalog {