diff --git a/controllers/clusterpolicy_controller.go b/controllers/clusterpolicy_controller.go index 10f13e9a12..98ea9b29b8 100644 --- a/controllers/clusterpolicy_controller.go +++ b/controllers/clusterpolicy_controller.go @@ -19,7 +19,9 @@ package controllers import ( "context" "fmt" + "strings" + "github.com/NVIDIA/k8s-operator-libs/pkg/upgrade" "github.com/go-logr/logr" appsv1 "k8s.io/api/apps/v1" @@ -146,6 +148,7 @@ func (r *ClusterPolicyReconciler) Reconcile(ctx context.Context, req ctrl.Reques clusterPolicyCtrl.operatorMetrics.reconciliationTotal.Inc() overallStatus := gpuv1.Ready statesNotReady := []string{} + notReadyReasons := []string{} for { status, statusError := clusterPolicyCtrl.step() if statusError != nil { @@ -171,18 +174,42 @@ func (r *ClusterPolicyReconciler) Reconcile(ctx context.Context, req ctrl.Reques } } - // if any state is not ready, requeue for reconcile after 5 seconds + if clusterPolicyCtrl.singleton.Spec.Driver.UseNvidiaDriverCRDType() { + upgradeIncomplete, err := r.nvidiaDriverUpgradeIncomplete(ctx) + if err != nil { + clusterPolicyCtrl.operatorMetrics.reconciliationStatus.Set(reconciliationStatusNotReady) + clusterPolicyCtrl.operatorMetrics.reconciliationFailed.Inc() + if condErr := r.conditionUpdater.SetConditionsError(ctx, instance, conditions.ReconcileFailed, fmt.Sprintf("Failed to determine NVIDIADriver upgrade state: %s", err)); condErr != nil { + r.Log.Error(condErr, "failed to set condition") + } + return ctrl.Result{}, err + } + if upgradeIncomplete { + overallStatus = gpuv1.NotReady + notReadyReasons = append(notReadyReasons, + "NVIDIADriver upgrade has not completed", + "one or more NVIDIADriver-owned Nodes are marked pending, in-progress, or failed", + ) + } + } + + // Keep the ClusterPolicy NotReady while an NVIDIADriver upgrade is active or + // failed. Unlike operand states, upgrade-only conditions are reconciled by + // Node label watch events rather than periodic polling. if overallStatus != gpuv1.Ready { clusterPolicyCtrl.operatorMetrics.reconciliationStatus.Set(reconciliationStatusNotReady) clusterPolicyCtrl.operatorMetrics.reconciliationFailed.Inc() - err := fmt.Errorf("ClusterPolicy is not ready, states not ready: %v", statesNotReady) + err := fmt.Errorf("%s", clusterPolicyNotReadyMessage(statesNotReady, notReadyReasons)) r.Log.Error(err, "ClusterPolicy not yet ready") updateCRState(ctx, r, req.NamespacedName, gpuv1.NotReady) if condErr := r.conditionUpdater.SetConditionsError(ctx, instance, conditions.OperandNotReady, err.Error()); condErr != nil { r.Log.Error(condErr, "failed to set condition") } - return ctrl.Result{RequeueAfter: time.Second * 5}, nil + if len(statesNotReady) > 0 { + return ctrl.Result{RequeueAfter: time.Second * 5}, nil + } + return ctrl.Result{}, nil } if !clusterPolicyCtrl.hasNFDLabels { @@ -227,6 +254,40 @@ func (r *ClusterPolicyReconciler) Reconcile(ctx context.Context, req ctrl.Reques return ctrl.Result{}, nil } +// clusterPolicyNotReadyMessage formats a condition message with operand states and additional not-ready reasons. +func clusterPolicyNotReadyMessage(statesNotReady, notReadyReasons []string) string { + messageParts := []string{"ClusterPolicy is not ready"} + if len(statesNotReady) > 0 { + messageParts = append(messageParts, fmt.Sprintf("states not ready: %v", statesNotReady)) + } + messageParts = append(messageParts, notReadyReasons...) + return strings.Join(messageParts, "; ") +} + +// nvidiaDriverUpgradeIncomplete reports whether any NVIDIADriver-owned Node has a pending, active, or failed upgrade. +func (r *ClusterPolicyReconciler) nvidiaDriverUpgradeIncomplete(ctx context.Context) (bool, error) { + nodes := &corev1.NodeList{} + if err := r.List(ctx, nodes, client.HasLabels{consts.NVIDIADriverOwnerLabel}); err != nil { + return false, fmt.Errorf("failed to list nodes for NVIDIADriver upgrade state: %w", err) + } + + for _, node := range nodes.Items { + if node.Labels[upgrade.GetUpgradeSkipNodeLabelKey()] == "true" { + continue + } + if isIncompleteDriverUpgradeState(node.Labels[upgrade.GetUpgradeStateLabelKey()]) { + return true, nil + } + } + + return false, nil +} + +// isIncompleteDriverUpgradeState reports whether a Node upgrade state keeps the aggregate driver rollout incomplete. +func isIncompleteDriverUpgradeState(state string) bool { + return state != upgrade.UpgradeStateDone && state != upgrade.UpgradeStateUnknown +} + func updateCRState(ctx context.Context, r *ClusterPolicyReconciler, namespacedName types.NamespacedName, state gpuv1.State) { // Fetch latest instance and update state to avoid version mismatch instance := &gpuv1.ClusterPolicy{} @@ -303,12 +364,16 @@ func addWatchNewGPUNode(r *ClusterPolicyReconciler, c controller.Controller, mgr // 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 + modeLabelChanged || + driverOwnerLabelChanged || + driverUpgradeStateLabelChanged || + driverUpgradeSkipLabelChanged if needsUpdate { r.Log.Info("Node needs an update", @@ -318,6 +383,9 @@ func addWatchNewGPUNode(r *ClusterPolicyReconciler, c controller.Controller, mgr "gpuWorkloadConfigLabelChanged", gpuWorkloadConfigLabelChanged, "osTreeLabelChanged", osTreeLabelChanged, "modeLabelChanged", modeLabelChanged, + "driverOwnerLabelChanged", driverOwnerLabelChanged, + "driverUpgradeStateLabelChanged", driverUpgradeStateLabelChanged, + "driverUpgradeSkipLabelChanged", driverUpgradeSkipLabelChanged, ) } return needsUpdate @@ -329,12 +397,7 @@ func addWatchNewGPUNode(r *ClusterPolicyReconciler, c controller.Controller, mgr // DaemonSet. // NB: we cannot know here if the DriverToolkit is // enabled. - - labels := e.Object.GetLabels() - - _, hasOSTreeLabel := labels[nfdOSTreeVersionLabelKey] - - return hasGPULabels(labels) && hasOSTreeLabel + return shouldReconcileClusterPolicyOnNodeDeletion(e.Object.GetLabels()) }, } @@ -349,6 +412,21 @@ func addWatchNewGPUNode(r *ClusterPolicyReconciler, c controller.Controller, mgr return err } +// driverUpgradeLabelsChanged reports Node label changes that affect aggregate +// NVIDIADriver rollout status. +func driverUpgradeLabelsChanged(oldLabels, newLabels map[string]string) (bool, bool, bool) { + return oldLabels[consts.NVIDIADriverOwnerLabel] != newLabels[consts.NVIDIADriverOwnerLabel], + oldLabels[upgrade.GetUpgradeStateLabelKey()] != newLabels[upgrade.GetUpgradeStateLabelKey()], + oldLabels[upgrade.GetUpgradeSkipNodeLabelKey()] != newLabels[upgrade.GetUpgradeSkipNodeLabelKey()] +} + +// shouldReconcileClusterPolicyOnNodeDeletion reports whether deleting a Node +// can affect ClusterPolicy rendering or aggregate NVIDIADriver upgrade status. +func shouldReconcileClusterPolicyOnNodeDeletion(labels map[string]string) bool { + _, hasOSTreeLabel := labels[nfdOSTreeVersionLabelKey] + return (hasGPULabels(labels) && hasOSTreeLabel) || labels[consts.NVIDIADriverOwnerLabel] != "" +} + // SetupWithManager sets up the controller with the Manager. func (r *ClusterPolicyReconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manager) error { // Create a new controller diff --git a/controllers/clusterpolicy_controller_test.go b/controllers/clusterpolicy_controller_test.go new file mode 100644 index 0000000000..dfa6f85e13 --- /dev/null +++ b/controllers/clusterpolicy_controller_test.go @@ -0,0 +1,474 @@ +/** +# Copyright (c) NVIDIA CORPORATION. All rights reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +**/ + +package controllers + +import ( + "context" + "testing" + "time" + + "github.com/NVIDIA/k8s-operator-libs/pkg/upgrade" + "github.com/go-logr/logr" + promcli "github.com/prometheus/client_golang/prometheus" + "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/utils/ptr" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + + 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 TestIsIncompleteDriverUpgradeState(t *testing.T) { + tests := []struct { + name string + state string + expected bool + }{ + { + name: "unknown state is inactive", + state: upgrade.UpgradeStateUnknown, + expected: false, + }, + { + name: "upgrade required is pending and in progress", + state: upgrade.UpgradeStateUpgradeRequired, + expected: true, + }, + { + name: "done is inactive", + state: upgrade.UpgradeStateDone, + expected: false, + }, + { + name: "failed is incomplete", + state: upgrade.UpgradeStateFailed, + expected: true, + }, + { + name: "pod restart required is active", + state: upgrade.UpgradeStatePodRestartRequired, + expected: true, + }, + { + name: "uncordon required is active", + state: upgrade.UpgradeStateUncordonRequired, + expected: true, + }, + { + name: "unrecognized state is incomplete", + state: "new-state", + expected: true, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + require.Equal(t, tc.expected, isIncompleteDriverUpgradeState(tc.state)) + }) + } +} + +func TestClusterPolicyNotReadyMessage(t *testing.T) { + tests := []struct { + name string + statesNotReady []string + notReadyReasons []string + expected string + }{ + { + name: "driver upgrade only", + notReadyReasons: []string{ + "NVIDIADriver upgrade has not completed", + "one or more NVIDIADriver-owned Nodes are marked pending, in-progress, or failed", + }, + expected: "ClusterPolicy is not ready; NVIDIADriver upgrade has not completed; one or more NVIDIADriver-owned Nodes are marked pending, in-progress, or failed", + }, + { + name: "not ready states and driver upgrade", + statesNotReady: []string{"state-container-toolkit", "state-device-plugin"}, + notReadyReasons: []string{ + "NVIDIADriver upgrade has not completed", + "one or more NVIDIADriver-owned Nodes are marked pending, in-progress, or failed", + }, + expected: "ClusterPolicy is not ready; states not ready: [state-container-toolkit state-device-plugin]; NVIDIADriver upgrade has not completed; one or more NVIDIADriver-owned Nodes are marked pending, in-progress, or failed", + }, + { + name: "not ready states only", + statesNotReady: []string{"state-container-toolkit"}, + expected: "ClusterPolicy is not ready; states not ready: [state-container-toolkit]", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + require.Equal(t, tc.expected, clusterPolicyNotReadyMessage(tc.statesNotReady, tc.notReadyReasons)) + }) + } +} + +func TestNVIDIADriverUpgradeIncomplete(t *testing.T) { + upgradeStateLabel := upgrade.GetUpgradeStateLabelKey() + + tests := []struct { + name string + nodes []client.Object + expected bool + }{ + { + name: "active upgrade state on NVIDIADriver-owned node", + nodes: []client.Object{ + nodeWithLabels("gpu-node", map[string]string{ + gpuconsts.NVIDIADriverOwnerLabel: "default", + upgradeStateLabel: upgrade.UpgradeStatePodRestartRequired, + }), + }, + expected: true, + }, + { + name: "pending upgrade keeps rollout in progress after another node completes", + nodes: []client.Object{ + nodeWithLabels("upgraded-gpu-node", map[string]string{ + gpuconsts.NVIDIADriverOwnerLabel: "default", + upgradeStateLabel: upgrade.UpgradeStateDone, + }), + nodeWithLabels("pending-gpu-node", map[string]string{ + gpuconsts.NVIDIADriverOwnerLabel: "default", + upgradeStateLabel: upgrade.UpgradeStateUpgradeRequired, + }), + }, + expected: true, + }, + { + name: "active upgrade state on unowned node is ignored", + nodes: []client.Object{ + nodeWithLabels("gpu-node", map[string]string{ + upgradeStateLabel: upgrade.UpgradeStatePodRestartRequired, + }), + }, + expected: false, + }, + { + name: "failed upgrade state keeps rollout incomplete", + nodes: []client.Object{ + nodeWithLabels("gpu-node", map[string]string{ + gpuconsts.NVIDIADriverOwnerLabel: "default", + upgradeStateLabel: upgrade.UpgradeStateFailed, + }), + }, + expected: true, + }, + { + name: "completed upgrade state is not treated as in progress", + nodes: []client.Object{ + nodeWithLabels("gpu-node", map[string]string{ + gpuconsts.NVIDIADriverOwnerLabel: "default", + upgradeStateLabel: upgrade.UpgradeStateDone, + }), + }, + expected: false, + }, + { + name: "skipped node is excluded from the upgrade aggregate", + nodes: []client.Object{ + nodeWithLabels("skipped-gpu-node", map[string]string{ + gpuconsts.NVIDIADriverOwnerLabel: "default", + upgradeStateLabel: upgrade.UpgradeStateUpgradeRequired, + upgrade.GetUpgradeSkipNodeLabelKey(): "true", + }), + }, + expected: false, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + scheme := runtime.NewScheme() + require.NoError(t, corev1.AddToScheme(scheme)) + reconciler := &ClusterPolicyReconciler{ + Client: fake.NewClientBuilder().WithScheme(scheme).WithObjects(tc.nodes...).Build(), + } + + actual, err := reconciler.nvidiaDriverUpgradeIncomplete(context.Background()) + require.NoError(t, err) + require.Equal(t, tc.expected, actual) + }) + } +} + +func nodeWithLabels(name string, labels map[string]string) *corev1.Node { + return &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Labels: labels, + }, + } +} + +func TestDriverUpgradeLabelsChanged(t *testing.T) { + upgradeStateLabel := upgrade.GetUpgradeStateLabelKey() + + tests := []struct { + name string + oldLabels map[string]string + newLabels map[string]string + ownerChanged bool + upgradeStateChanged bool + upgradeSkipChanged bool + }{ + { + name: "driver ownership changes", + oldLabels: map[string]string{gpuconsts.NVIDIADriverOwnerLabel: "old-driver"}, + newLabels: map[string]string{gpuconsts.NVIDIADriverOwnerLabel: "new-driver"}, + ownerChanged: true, + }, + { + name: "upgrade state changes", + oldLabels: map[string]string{upgradeStateLabel: upgrade.UpgradeStateUpgradeRequired}, + newLabels: map[string]string{upgradeStateLabel: upgrade.UpgradeStateDone}, + upgradeStateChanged: true, + }, + { + name: "upgrade skip label changes", + oldLabels: map[string]string{upgrade.GetUpgradeSkipNodeLabelKey(): "false"}, + newLabels: map[string]string{upgrade.GetUpgradeSkipNodeLabelKey(): "true"}, + upgradeSkipChanged: true, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + ownerChanged, upgradeStateChanged, upgradeSkipChanged := driverUpgradeLabelsChanged(tc.oldLabels, tc.newLabels) + require.Equal(t, tc.ownerChanged, ownerChanged) + require.Equal(t, tc.upgradeStateChanged, upgradeStateChanged) + require.Equal(t, tc.upgradeSkipChanged, upgradeSkipChanged) + }) + } +} + +func TestShouldReconcileClusterPolicyOnNodeDeletion(t *testing.T) { + tests := []struct { + name string + labels map[string]string + expected bool + }{ + { + name: "NVIDIADriver-owned node", + labels: map[string]string{ + gpuconsts.NVIDIADriverOwnerLabel: "driver-a", + }, + expected: true, + }, + { + name: "unrelated node", + labels: map[string]string{ + "example.com/label": "value", + }, + expected: false, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + require.Equal(t, tc.expected, shouldReconcileClusterPolicyOnNodeDeletion(tc.labels)) + }) + } +} + +func TestClusterPolicyReconcileDriverUpgradeTransitions(t *testing.T) { + upgradeStateLabel := upgrade.GetUpgradeStateLabelKey() + cp := clusterPolicyForUpgradeTest(true) + node := nodeWithLabels("gpu-node", map[string]string{ + gpuconsts.NVIDIADriverOwnerLabel: "driver-a", + upgradeStateLabel: upgrade.UpgradeStateDone, + }) + r, c, _ := newClusterPolicyUpgradeTestReconciler(t, cp, node) + + result, 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)) + // No NFD labels are present in this focused test fixture, so Ready follows + // the existing NFD polling path. + require.Equal(t, 45*time.Second, result.RequeueAfter) + + require.NoError(t, c.Get(t.Context(), client.ObjectKeyFromObject(node), node)) + node.Labels[upgradeStateLabel] = upgrade.UpgradeStateUpgradeRequired + require.NoError(t, c.Update(t.Context(), node)) + + result, err = r.Reconcile(t.Context(), ctrl.Request{NamespacedName: client.ObjectKeyFromObject(cp)}) + require.NoError(t, err) + require.Equal(t, gpuv1.NotReady, clusterPolicyState(t, c, cp.Name)) + require.Zero(t, result.RequeueAfter) + + require.NoError(t, c.Get(t.Context(), client.ObjectKeyFromObject(node), node)) + node.Labels[upgrade.GetUpgradeSkipNodeLabelKey()] = "true" + require.NoError(t, c.Update(t.Context(), node)) + + _, 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)) + + require.NoError(t, c.Get(t.Context(), client.ObjectKeyFromObject(node), node)) + delete(node.Labels, upgrade.GetUpgradeSkipNodeLabelKey()) + require.NoError(t, c.Update(t.Context(), node)) + + result, err = r.Reconcile(t.Context(), ctrl.Request{NamespacedName: client.ObjectKeyFromObject(cp)}) + require.NoError(t, err) + require.Equal(t, gpuv1.NotReady, clusterPolicyState(t, c, cp.Name)) + require.Zero(t, result.RequeueAfter) + + require.NoError(t, c.Get(t.Context(), client.ObjectKeyFromObject(node), node)) + node.Labels[upgradeStateLabel] = upgrade.UpgradeStateDone + require.NoError(t, c.Update(t.Context(), node)) + + _, 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 TestClusterPolicyReconcileBecomesReadyAfterIncompleteNodeDeletion(t *testing.T) { + upgradeStateLabel := upgrade.GetUpgradeStateLabelKey() + cp := clusterPolicyForUpgradeTest(true) + node := nodeWithLabels("failed-gpu-node", map[string]string{ + gpuconsts.NVIDIADriverOwnerLabel: "driver-a", + upgradeStateLabel: upgrade.UpgradeStateFailed, + }) + r, c, _ := newClusterPolicyUpgradeTestReconciler(t, cp, node) + request := ctrl.Request{NamespacedName: client.ObjectKeyFromObject(cp)} + + result, err := r.Reconcile(t.Context(), request) + require.NoError(t, err) + require.Equal(t, gpuv1.NotReady, clusterPolicyState(t, c, cp.Name)) + require.Zero(t, result.RequeueAfter) + + require.NoError(t, c.Delete(t.Context(), node)) + _, err = r.Reconcile(t.Context(), request) + require.NoError(t, err) + require.Equal(t, gpuv1.Ready, clusterPolicyState(t, c, cp.Name)) +} + +func TestClusterPolicyReconcileDriverUpgradeFailureCases(t *testing.T) { + upgradeStateLabel := upgrade.GetUpgradeStateLabelKey() + + t.Run("one failed driver among multiple drivers keeps ClusterPolicy not ready", func(t *testing.T) { + cp := clusterPolicyForUpgradeTest(true) + r, c, _ := newClusterPolicyUpgradeTestReconciler(t, cp, + nodeWithLabels("completed", map[string]string{gpuconsts.NVIDIADriverOwnerLabel: "driver-a", upgradeStateLabel: upgrade.UpgradeStateDone}), + nodeWithLabels("failed", map[string]string{gpuconsts.NVIDIADriverOwnerLabel: "driver-b", upgradeStateLabel: upgrade.UpgradeStateFailed}), + ) + + result, err := r.Reconcile(t.Context(), ctrl.Request{NamespacedName: client.ObjectKeyFromObject(cp)}) + require.NoError(t, err) + require.Equal(t, gpuv1.NotReady, clusterPolicyState(t, c, cp.Name)) + require.Zero(t, result.RequeueAfter) + }) + + t.Run("legacy driver management ignores upgrade labels", func(t *testing.T) { + cp := clusterPolicyForUpgradeTest(false) + r, c, _ := newClusterPolicyUpgradeTestReconciler(t, cp, + nodeWithLabels("failed", map[string]string{gpuconsts.NVIDIADriverOwnerLabel: "driver-a", upgradeStateLabel: upgrade.UpgradeStateFailed}), + ) + + _, 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)) + }) + + t.Run("terminal failure relies on Node events instead of polling", func(t *testing.T) { + cp := clusterPolicyForUpgradeTest(true) + calls := 0 + r, _, metrics := newClusterPolicyUpgradeTestReconciler(t, cp, + nodeWithLabels("failed", map[string]string{gpuconsts.NVIDIADriverOwnerLabel: "driver-a", upgradeStateLabel: upgrade.UpgradeStateFailed}), + ) + clusterPolicyCtrl.controls = []controlFunc{{func(ClusterPolicyController) (gpuv1.State, error) { + calls++ + return gpuv1.Ready, nil + }}} + + result, err := r.Reconcile(t.Context(), ctrl.Request{NamespacedName: client.ObjectKeyFromObject(cp)}) + require.NoError(t, err) + require.Zero(t, result.RequeueAfter) + require.Equal(t, 1, calls) + require.Equal(t, 1, metrics.reconciliationFailed.(*countingCounter).increments) + }) +} + +func clusterPolicyForUpgradeTest(useNvidiaDriverCRD bool) *gpuv1.ClusterPolicy { + return &gpuv1.ClusterPolicy{ + ObjectMeta: metav1.ObjectMeta{Name: "cluster-policy"}, + Spec: gpuv1.ClusterPolicySpec{Driver: gpuv1.DriverSpec{UseNvidiaDriverCRD: ptr.To(useNvidiaDriverCRD)}}, + } +} + +func newClusterPolicyUpgradeTestReconciler(t *testing.T, cp *gpuv1.ClusterPolicy, nodes ...*corev1.Node) (*ClusterPolicyReconciler, client.Client, *OperatorMetrics) { + t.Helper() + scheme := runtime.NewScheme() + require.NoError(t, gpuv1.AddToScheme(scheme)) + require.NoError(t, corev1.AddToScheme(scheme)) + require.NoError(t, nvidiav1alpha1.AddToScheme(scheme)) + + objects := []client.Object{cp} + for _, node := range nodes { + objects = append(objects, node) + } + c := fake.NewClientBuilder().WithScheme(scheme).WithObjects(objects...).WithStatusSubresource(&gpuv1.ClusterPolicy{}).Build() + metrics := newClusterPolicyUpgradeTestMetrics() + previousController := clusterPolicyCtrl + clusterPolicyCtrl = ClusterPolicyController{ + controls: []controlFunc{{func(ClusterPolicyController) (gpuv1.State, error) { return gpuv1.Ready, nil }}}, + stateNames: []string{"test"}, + operatorMetrics: metrics, + } + t.Cleanup(func() { clusterPolicyCtrl = previousController }) + + return &ClusterPolicyReconciler{Client: c, Scheme: scheme, Log: logr.Discard(), conditionUpdater: &FakeConditionUpdater{}}, c, metrics +} + +func newClusterPolicyUpgradeTestMetrics() *OperatorMetrics { + failedCounter := &countingCounter{Counter: promcli.NewCounter(promcli.CounterOpts{})} + return &OperatorMetrics{ + gpuNodesTotal: promcli.NewGauge(promcli.GaugeOpts{}), + reconciliationLastSuccess: promcli.NewGauge(promcli.GaugeOpts{}), + reconciliationStatus: promcli.NewGauge(promcli.GaugeOpts{}), + reconciliationTotal: promcli.NewCounter(promcli.CounterOpts{}), + reconciliationFailed: failedCounter, + reconciliationHasNFDLabels: promcli.NewGauge(promcli.GaugeOpts{}), + openshiftDriverToolkitEnabled: promcli.NewGauge(promcli.GaugeOpts{}), + } +} + +type countingCounter struct { + promcli.Counter + increments int +} + +func (c *countingCounter) Inc() { + c.increments++ + c.Counter.Inc() +} + +func clusterPolicyState(t *testing.T, c client.Client, name string) gpuv1.State { + t.Helper() + cp := &gpuv1.ClusterPolicy{} + require.NoError(t, c.Get(t.Context(), client.ObjectKey{Name: name}, cp)) + return cp.Status.State +} diff --git a/controllers/upgrade_controller.go b/controllers/upgrade_controller.go index e551a90f82..9d9a65494e 100644 --- a/controllers/upgrade_controller.go +++ b/controllers/upgrade_controller.go @@ -410,11 +410,10 @@ func (r *UpgradeReconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manag return getClusterPoliciesToReconcile(ctx, mgr.GetClient()) } - // Only watch for changes to the upgrade state label - upgradeStateLabelPredicate := predicate.TypedFuncs[*corev1.Node]{ + // Watch changes that alter whether a node participates in a driver upgrade. + upgradeNodeLabelPredicate := predicate.TypedFuncs[*corev1.Node]{ UpdateFunc: func(e event.TypedUpdateEvent[*corev1.Node]) bool { - label := upgrade.GetUpgradeStateLabelKey() - return e.ObjectOld.Labels[label] != e.ObjectNew.Labels[label] + return upgradeNodeLabelsChanged(e.ObjectOld.Labels, e.ObjectNew.Labels) }, } @@ -423,7 +422,7 @@ func (r *UpgradeReconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manag mgr.GetCache(), &corev1.Node{}, handler.TypedEnqueueRequestsFromMapFunc[*corev1.Node](nodeMapFn), - upgradeStateLabelPredicate, + upgradeNodeLabelPredicate, ), ) if err != nil { @@ -491,6 +490,13 @@ func (r *UpgradeReconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manag return nil } +// upgradeNodeLabelsChanged reports Node label changes that require the upgrade +// controller to re-evaluate upgrade participation or progress. +func upgradeNodeLabelsChanged(oldLabels, newLabels map[string]string) bool { + return oldLabels[upgrade.GetUpgradeStateLabelKey()] != newLabels[upgrade.GetUpgradeStateLabelKey()] || + oldLabels[upgrade.GetUpgradeSkipNodeLabelKey()] != newLabels[upgrade.GetUpgradeSkipNodeLabelKey()] +} + func getClusterPoliciesToReconcile(ctx context.Context, k8sClient client.Client) []reconcile.Request { logger := log.FromContext(ctx) opts := []client.ListOption{} diff --git a/controllers/upgrade_controller_test.go b/controllers/upgrade_controller_test.go index 3b72da082f..47dfee2c30 100644 --- a/controllers/upgrade_controller_test.go +++ b/controllers/upgrade_controller_test.go @@ -21,6 +21,7 @@ import ( "testing" upgrade_v1alpha1 "github.com/NVIDIA/k8s-operator-libs/api/upgrade/v1alpha1" + "github.com/NVIDIA/k8s-operator-libs/pkg/upgrade" "github.com/stretchr/testify/assert" ) @@ -69,3 +70,37 @@ func TestSetDrainSpecPodSelector(t *testing.T) { }) } } + +func TestUpgradeNodeLabelsChanged(t *testing.T) { + tests := []struct { + name string + oldLabels map[string]string + newLabels map[string]string + expected bool + }{ + { + name: "upgrade state changes", + oldLabels: map[string]string{upgrade.GetUpgradeStateLabelKey(): upgrade.UpgradeStateUpgradeRequired}, + newLabels: map[string]string{upgrade.GetUpgradeStateLabelKey(): upgrade.UpgradeStateDone}, + expected: true, + }, + { + name: "skip label changes", + oldLabels: map[string]string{upgrade.GetUpgradeSkipNodeLabelKey(): "false"}, + newLabels: map[string]string{upgrade.GetUpgradeSkipNodeLabelKey(): "true"}, + expected: true, + }, + { + name: "unrelated label changes", + oldLabels: map[string]string{"example.com/label": "old"}, + newLabels: map[string]string{"example.com/label": "new"}, + expected: false, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + assert.Equal(t, tc.expected, upgradeNodeLabelsChanged(tc.oldLabels, tc.newLabels)) + }) + } +}