controller: sched: update status only if needed

So far updating the scheduler status was done in every reconcilation iteration
even if the spec fields reflected the same values.

This commit saves a copy of the initial resource status and uses it as
a reference to identify if the resource requires a status update or not
and act accordingly.

Signed-off-by: Shereen Haj <[email protected]>
(cherry picked from commit 558833b)
(cherry picked from commit 032e581)

Author: shajmakh

controllers/numaresourcesscheduler_controller.go

@@ -112,9 +112,11 @@ func (r *NUMAResourcesSchedulerReconciler) Reconcile(ctx context.Context, req ct
 		return ctrl.Result{}, err
 	}
 
+	initialStatus := *instance.Status.DeepCopy()
+
 	if req.Name != objectnames.DefaultNUMAResourcesSchedulerCrName {
 		message := fmt.Sprintf("incorrect NUMAResourcesScheduler resource name: %s", instance.Name)
-		return ctrl.Result{}, r.updateStatus(ctx, instance, status.ConditionDegraded, status.ConditionTypeIncorrectNUMAResourcesSchedulerResourceName, message)
+		return ctrl.Result{}, r.updateStatus(ctx, initialStatus, instance, status.ConditionDegraded, status.ConditionTypeIncorrectNUMAResourcesSchedulerResourceName, message)
 	}
 
 	if annotations.IsPauseReconciliationEnabled(instance.Annotations) {
@@ -123,7 +125,7 @@ func (r *NUMAResourcesSchedulerReconciler) Reconcile(ctx context.Context, req ct
 	}
 
 	result, condition, err := r.reconcileResource(ctx, instance)
-	if err := r.updateStatus(ctx, instance, condition, status.ReasonFromError(err), status.MessageFromError(err)); err != nil {
+	if err := r.updateStatus(ctx, initialStatus, instance, condition, status.ReasonFromError(err), status.MessageFromError(err)); err != nil {
 		klog.InfoS("Failed to update numaresourcesscheduler status", "Desired condition", condition, "error", err)
 	}
 
@@ -287,8 +289,15 @@ func platformNormalize(spec *nropv1.NUMAResourcesSchedulerSpec, platInfo Platfor
 		klog.V(4).InfoS("SchedulerInformer default is overridden", "Platform", platInfo.Platform, "PlatformVersion", platInfo.Version.String(), "SchedulerInformer", &spec.SchedulerInformer)
 	}
 }
-func (r *NUMAResourcesSchedulerReconciler) updateStatus(ctx context.Context, sched *nropv1.NUMAResourcesScheduler, condition string, reason string, message string) error {
-	sched.Status.Conditions, _ = status.UpdateConditions(sched.Status.Conditions, condition, reason, message)
+func (r *NUMAResourcesSchedulerReconciler) updateStatus(ctx context.Context, initialStatus nropv1.NUMAResourcesSchedulerStatus, sched *nropv1.NUMAResourcesScheduler, condition string, reason string, message string) error {
+	updatedStatus := *sched.Status.DeepCopy()
+
+	updatedStatus.Conditions, _ = status.UpdateConditions(sched.Status.Conditions, condition, reason, message)
+	if !status.NUMAResourcesSchedulerNeedsUpdate(initialStatus, updatedStatus) {
+		return nil
+	}
+
+	sched.Status.Conditions = updatedStatus.Conditions
 	if err := r.Client.Status().Update(ctx, sched); err != nil {
 		return fmt.Errorf("could not update status for object %s: %w", client.ObjectKeyFromObject(sched), err)
 	}

controllers/numaresourcesscheduler_controller_test.go

@@ -825,8 +825,6 @@ var _ = ginkgo.Describe("Test scheduler spec platformNormalize", func() {
 		})
 
 		ginkgo.It("should not override default informer if kubelet is not fixed - version is less than first supported (zstream)", func() {
-			// this is only for testing purposes as there is plan to backport the fix to older minor versions
-			// will need to remove this test if the fix is supported starting the first zstream of the release
 			v, _ := platform.ParseVersion("4.18.8")
 			spec := nropv1.NUMAResourcesSchedulerSpec{}
 			platformNormalize(&spec, PlatformInfo{Platform: platform.OpenShift, Version: v})

pkg/status/status.go

@@ -18,6 +18,7 @@ package status
 
 import (
 	"errors"
+	"reflect"
 	"time"
 
 	"github.com/google/go-cmp/cmp"
@@ -59,6 +60,16 @@ func IsUpdatedNUMAResourcesOperator(oldStatus, newStatus *nropv1.NUMAResourcesOp
 	return !cmp.Equal(newStatus, oldStatus, options...)
 }
 
+func NUMAResourcesSchedulerNeedsUpdate(oldStatus, newStatus nropv1.NUMAResourcesSchedulerStatus) bool {
+	os := oldStatus.DeepCopy()
+	ns := newStatus.DeepCopy()
+
+	resetIncomparableConditionFields(os.Conditions)
+	resetIncomparableConditionFields(ns.Conditions)
+
+	return !reflect.DeepEqual(os, ns)
+}
+
 // UpdateConditions compute new conditions based on arguments, and then compare with given current conditions.
 // Returns the conditions to use, either current or newly computed, and a boolean flag which is `true` if conditions need
 // update - so if they are updated since the current conditions.

pkg/status/status_test.go

@@ -25,6 +25,8 @@ import (
 
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/client-go/kubernetes/scheme"
+	"k8s.io/utils/ptr"
+
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
 
@@ -201,6 +203,58 @@ func TestIsUpdatedNUMAResourcesOperator(t *testing.T) {
 	}
 }
 
+func TestNUMAResourcesSchedulerNeedsUpdate(t *testing.T) {
+	type testCase struct {
+		name            string
+		oldStatus       nropv1.NUMAResourcesSchedulerStatus
+		updaterFunc     func(*nropv1.NUMAResourcesSchedulerStatus)
+		expectedUpdated bool
+	}
+	testCases := []testCase{
+		{
+			name:            "empty status, no change",
+			oldStatus:       nropv1.NUMAResourcesSchedulerStatus{},
+			updaterFunc:     func(st *nropv1.NUMAResourcesSchedulerStatus) {},
+			expectedUpdated: false,
+		},
+		{
+			name: "status, conditions, updated only time",
+			oldStatus: nropv1.NUMAResourcesSchedulerStatus{
+				Conditions: NewConditions(ConditionAvailable, "test all good", "testing info"),
+			},
+			updaterFunc: func(st *nropv1.NUMAResourcesSchedulerStatus) {
+				time.Sleep(42 * time.Millisecond) // make sure the timestamp changed
+				st.Conditions = NewConditions(ConditionAvailable, "test all good", "testing info")
+			},
+			expectedUpdated: false,
+		},
+		{
+			name: "status, conditions, updated only time, other fields changed",
+			oldStatus: nropv1.NUMAResourcesSchedulerStatus{
+				Conditions: NewConditions(ConditionAvailable, "test all good", "testing info"),
+			},
+			updaterFunc: func(st *nropv1.NUMAResourcesSchedulerStatus) {
+				time.Sleep(42 * time.Millisecond) // make sure the timestamp changed
+				st.Conditions = NewConditions(ConditionAvailable, "test all good", "testing info")
+				st.CacheResyncPeriod = ptr.To(metav1.Duration{Duration: 42 * time.Second})
+			},
+			expectedUpdated: true,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			oldStatus := *tc.oldStatus.DeepCopy()
+			newStatus := *tc.oldStatus.DeepCopy()
+			tc.updaterFunc(&newStatus)
+			got := NUMAResourcesSchedulerNeedsUpdate(oldStatus, newStatus)
+			if got != tc.expectedUpdated {
+				t.Errorf("isUpdated %v expected %v", got, tc.expectedUpdated)
+			}
+		})
+	}
+}
+
 func TestReasonFromError(t *testing.T) {
 	type testCase struct {
 		name     string