fix: ResourceGroup unnecessary timestamp updates (#1570)

The ResourceGroup controller has been updating status conditions
every time it reconciles, even when nothing changed but the
LastTransitionTime.

This change fixes that problem by only updating the conditions if
another condition field changed. This prevents the pre-existing
semantic DeepEquals from always failing.

Author: karlkfi

pkg/resourcegroup/controllers/resourcegroup/condition.go

@@ -42,6 +42,32 @@ func newStalledCondition(status v1alpha1.ConditionStatus, reason, message string
 	}
 }
 
+// updateCondition modifies and returns a list of conditions to update the
+// specified condition. This avoids updating the LastTransitionTime when there's
+// no other change.
+func updateCondition(conditions []v1alpha1.Condition, newCondition v1alpha1.Condition) []v1alpha1.Condition {
+	for i, condition := range conditions {
+		if condition.Type == newCondition.Type {
+			if !isConditionEqual(condition, newCondition) {
+				conditions[i] = newCondition
+			}
+			// assume no duplicate condition types
+			return conditions
+		}
+	}
+	// if not found, add it
+	conditions = append(conditions, newCondition)
+	return conditions
+}
+
+// isConditionEqual returns true if a == b, ignoring the LastTransitionTime.
+func isConditionEqual(a, b v1alpha1.Condition) bool {
+	return a.Type == b.Type &&
+		a.Status == b.Status &&
+		a.Reason == b.Reason &&
+		a.Message == b.Message
+}
+
 // adjustConditionOrder adjusts the order of the conditions to make sure that
 // the first condition in the slice is Reconciling;
 // the second condition in the slice is Stalled;

pkg/resourcegroup/controllers/resourcegroup/resourcegroup_controller.go

@@ -166,15 +166,15 @@ func (r *reconciler) updateStatusKptGroup(ctx context.Context, resgroup *v1alpha
 }
 
 func (r *reconciler) startReconcilingStatus(status v1alpha1.ResourceGroupStatus) v1alpha1.ResourceGroupStatus {
-	newStatus := v1alpha1.ResourceGroupStatus{
-		ObservedGeneration: status.ObservedGeneration,
-		ResourceStatuses:   status.ResourceStatuses,
-		SubgroupStatuses:   status.SubgroupStatuses,
-		Conditions: []v1alpha1.Condition{
-			newReconcilingCondition(v1alpha1.TrueConditionStatus, StartReconciling, startReconcilingMsg),
-			newStalledCondition(v1alpha1.FalseConditionStatus, "", ""),
-		},
-	}
+	// Preserve existing status and only update fields that need to change.
+	// This avoids repeatedly updating the status just to update a timestamp.
+	// It also prevents fighting with other controllers updating the status.
+	newStatus := *status.DeepCopy()
+	// Update conditions, if they've changed
+	newStatus.Conditions = updateCondition(newStatus.Conditions,
+		newReconcilingCondition(v1alpha1.TrueConditionStatus, StartReconciling, startReconcilingMsg))
+	newStatus.Conditions = updateCondition(newStatus.Conditions,
+		newStalledCondition(v1alpha1.FalseConditionStatus, "", ""))
 	return newStatus
 }
 
@@ -186,32 +186,33 @@ func (r *reconciler) endReconcilingStatus(
 	status v1alpha1.ResourceGroupStatus,
 	generation int64,
 ) v1alpha1.ResourceGroupStatus {
-	// reset newStatus to make sure the former setting of newStatus does not carry over
-	newStatus := v1alpha1.ResourceGroupStatus{}
+	// Preserve existing status and only update fields that need to change.
+	// This avoids repeatedly updating the status just to update a timestamp.
+	// It also prevents fighting with other controllers updating the status.
+	newStatus := *status.DeepCopy()
 	startTime := time.Now()
 	reconcileTimeout := getReconcileTimeOut(len(spec.Subgroups) + len(spec.Resources))
 
 	finish := make(chan struct{})
 	go func() {
+		defer close(finish)
 		newStatus.ResourceStatuses = r.computeResourceStatuses(ctx, id, status, spec.Resources, namespacedName)
 		newStatus.SubgroupStatuses = r.computeSubGroupStatuses(ctx, id, status, spec.Subgroups, namespacedName)
-		close(finish)
 	}()
 	select {
 	case <-finish:
 		newStatus.ObservedGeneration = generation
-		newStatus.Conditions = []v1alpha1.Condition{
-			newReconcilingCondition(v1alpha1.FalseConditionStatus, FinishReconciling, finishReconcilingMsg),
-			aggregateResourceStatuses(newStatus.ResourceStatuses),
-		}
+		// Update conditions, if they've changed
+		newStatus.Conditions = updateCondition(newStatus.Conditions,
+			newReconcilingCondition(v1alpha1.FalseConditionStatus, FinishReconciling, finishReconcilingMsg))
+		newStatus.Conditions = updateCondition(newStatus.Conditions,
+			aggregateResourceStatuses(newStatus.ResourceStatuses))
 	case <-time.After(reconcileTimeout):
-		newStatus.ObservedGeneration = status.ObservedGeneration
-		newStatus.ResourceStatuses = status.ResourceStatuses
-		newStatus.SubgroupStatuses = status.SubgroupStatuses
-		newStatus.Conditions = []v1alpha1.Condition{
-			newReconcilingCondition(v1alpha1.FalseConditionStatus, ExceedTimeout, exceedTimeoutMsg),
-			newStalledCondition(v1alpha1.TrueConditionStatus, ExceedTimeout, exceedTimeoutMsg),
-		}
+		// Update conditions, if they've changed
+		newStatus.Conditions = updateCondition(newStatus.Conditions,
+			newReconcilingCondition(v1alpha1.FalseConditionStatus, ExceedTimeout, exceedTimeoutMsg))
+		newStatus.Conditions = updateCondition(newStatus.Conditions,
+			newStalledCondition(v1alpha1.TrueConditionStatus, ExceedTimeout, exceedTimeoutMsg))
 	}
 
 	metrics.RecordReconcileDuration(ctx, newStatus.Conditions[1].Reason, startTime)

pkg/resourcegroup/controllers/resourcegroup/resourcegroup_controller_test.go

@@ -128,6 +128,7 @@ func TestReconcile(t *testing.T) {
 	resgroupKpt = waitForResourceGroupStatus(t, ctx, c, rgKey, 1, 0, expectedStatus)
 
 	// Push an event to the channel, which will cause trigger a reconciliation for resgroup
+	t.Log("Sending event to controller")
 	channelKpt <- event.GenericEvent{Object: resgroupKpt}
 
 	// Verify that the reconciliation modifies the ResourceGroupStatus field correctly
@@ -171,6 +172,7 @@ func TestReconcile(t *testing.T) {
 	expectedStatus.ObservedGeneration = 2
 	resgroupKpt = waitForResourceGroupStatus(t, ctx, c, rgKey, 2, 2, expectedStatus)
 
+	t.Log("Sending event to controller")
 	channelKpt <- event.GenericEvent{Object: resgroupKpt}
 
 	// Verify that the reconciliation modifies the ResourceGroupStatus field correctly
@@ -235,6 +237,7 @@ func TestReconcile(t *testing.T) {
 	require.NoError(t, err)
 	require.Equal(t, corev1.NamespaceActive, updatedNS.Status.Phase)
 
+	t.Log("Sending event to controller")
 	channelKpt <- event.GenericEvent{Object: resgroupKpt}
 
 	// Verify that the reconciliation modifies the ResourceGroupStatus field correctly
@@ -282,6 +285,7 @@ func TestReconcile(t *testing.T) {
 	expectedStatus.ObservedGeneration = 3
 	resgroupKpt = waitForResourceGroupStatus(t, ctx, c, rgKey, 3, 1, expectedStatus)
 
+	t.Log("Sending event to controller")
 	channelKpt <- event.GenericEvent{Object: resgroupKpt}
 
 	// Verify that the reconciliation modifies the ResourceGroupStatus field correctly