Merge pull request #120542 from alvaroaleman/condition-change

Apimeta Set/RemoveStatusCondition: Indicate change Kubernetes-commit: fe84992651335ba6dd66e16754e8e416e5144b59
kubernetes · Oct 3, 2023 · 839ad94 · 839ad94
2 parents a017454 + 316b59e
commit 839ad94
Show file tree

Hide file tree

Showing 2 changed files with 62 additions and 19 deletions.
diff --git a/pkg/api/meta/conditions.go b/pkg/api/meta/conditions.go
@@ -22,22 +22,23 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
-// SetStatusCondition sets the corresponding condition in conditions to newCondition.
+// SetStatusCondition sets the corresponding condition in conditions to newCondition and returns true
+// if the conditions are changed by this call.
 // conditions must be non-nil.
 //  1. if the condition of the specified type already exists (all fields of the existing condition are updated to
 //     newCondition, LastTransitionTime is set to now if the new status differs from the old status)
 //  2. if a condition of the specified type does not exist (LastTransitionTime is set to now() if unset, and newCondition is appended)
-func SetStatusCondition(conditions *[]metav1.Condition, newCondition metav1.Condition) {
+func SetStatusCondition(conditions *[]metav1.Condition, newCondition metav1.Condition) (changed bool) {
 	if conditions == nil {
-		return
+		return false
 	}
 	existingCondition := FindStatusCondition(*conditions, newCondition.Type)
 	if existingCondition == nil {
 		if newCondition.LastTransitionTime.IsZero() {
 			newCondition.LastTransitionTime = metav1.NewTime(time.Now())
 		}
 		*conditions = append(*conditions, newCondition)
-		return
+		return true
 	}
 
 	if existingCondition.Status != newCondition.Status {
@@ -47,18 +48,31 @@ func SetStatusCondition(conditions *[]metav1.Condition, newCondition metav1.Cond
 		} else {
 			existingCondition.LastTransitionTime = metav1.NewTime(time.Now())
 		}
+		changed = true
 	}
 
-	existingCondition.Reason = newCondition.Reason
-	existingCondition.Message = newCondition.Message
-	existingCondition.ObservedGeneration = newCondition.ObservedGeneration
+	if existingCondition.Reason != newCondition.Reason {
+		existingCondition.Reason = newCondition.Reason
+		changed = true
+	}
+	if existingCondition.Message != newCondition.Message {
+		existingCondition.Message = newCondition.Message
+		changed = true
+	}
+	if existingCondition.ObservedGeneration != newCondition.ObservedGeneration {
+		existingCondition.ObservedGeneration = newCondition.ObservedGeneration
+		changed = true
+	}
+
+	return changed
 }
 
-// RemoveStatusCondition removes the corresponding conditionType from conditions.
+// RemoveStatusCondition removes the corresponding conditionType from conditions if present. Returns
+// true if it was present and got removed.
 // conditions must be non-nil.
-func RemoveStatusCondition(conditions *[]metav1.Condition, conditionType string) {
+func RemoveStatusCondition(conditions *[]metav1.Condition, conditionType string) (removed bool) {
 	if conditions == nil || len(*conditions) == 0 {
-		return
+		return false
 	}
 	newConditions := make([]metav1.Condition, 0, len(*conditions)-1)
 	for _, condition := range *conditions {
@@ -67,7 +81,10 @@ func RemoveStatusCondition(conditions *[]metav1.Condition, conditionType string)
 		}
 	}
 
+	removed = len(*conditions) != len(newConditions)
 	*conditions = newConditions
+
+	return removed
 }
 
 // FindStatusCondition finds the conditionType in conditions.

diff --git a/pkg/api/meta/conditions_test.go b/pkg/api/meta/conditions_test.go
@@ -29,18 +29,20 @@ func TestSetStatusCondition(t *testing.T) {
 	oneHourAfter := time.Now().Add(1 * time.Hour)
 
 	tests := []struct {
-		name       string
-		conditions []metav1.Condition
-		toAdd      metav1.Condition
-		expected   []metav1.Condition
+		name          string
+		conditions    []metav1.Condition
+		toAdd         metav1.Condition
+		expectChanged bool
+		expected      []metav1.Condition
 	}{
 		{
 			name: "should-add",
 			conditions: []metav1.Condition{
 				{Type: "first"},
 				{Type: "third"},
 			},
-			toAdd: metav1.Condition{Type: "second", Status: metav1.ConditionTrue, LastTransitionTime: metav1.Time{Time: oneHourBefore}, Reason: "reason", Message: "message"},
+			toAdd:         metav1.Condition{Type: "second", Status: metav1.ConditionTrue, LastTransitionTime: metav1.Time{Time: oneHourBefore}, Reason: "reason", Message: "message"},
+			expectChanged: true,
 			expected: []metav1.Condition{
 				{Type: "first"},
 				{Type: "third"},
@@ -54,7 +56,8 @@ func TestSetStatusCondition(t *testing.T) {
 				{Type: "second", Status: metav1.ConditionFalse},
 				{Type: "third"},
 			},
-			toAdd: metav1.Condition{Type: "second", Status: metav1.ConditionTrue, LastTransitionTime: metav1.Time{Time: oneHourBefore}, Reason: "reason", Message: "message"},
+			toAdd:         metav1.Condition{Type: "second", Status: metav1.ConditionTrue, LastTransitionTime: metav1.Time{Time: oneHourBefore}, Reason: "reason", Message: "message"},
+			expectChanged: true,
 			expected: []metav1.Condition{
 				{Type: "first"},
 				{Type: "second", Status: metav1.ConditionTrue, LastTransitionTime: metav1.Time{Time: oneHourBefore}, Reason: "reason", Message: "message"},
@@ -68,18 +71,36 @@ func TestSetStatusCondition(t *testing.T) {
 				{Type: "second", Status: metav1.ConditionTrue, LastTransitionTime: metav1.Time{Time: oneHourBefore}},
 				{Type: "third"},
 			},
-			toAdd: metav1.Condition{Type: "second", Status: metav1.ConditionTrue, LastTransitionTime: metav1.Time{Time: oneHourAfter}, Reason: "reason", Message: "message", ObservedGeneration: 3},
+			toAdd:         metav1.Condition{Type: "second", Status: metav1.ConditionTrue, LastTransitionTime: metav1.Time{Time: oneHourAfter}, Reason: "reason", Message: "message", ObservedGeneration: 3},
+			expectChanged: true,
 			expected: []metav1.Condition{
 				{Type: "first"},
 				{Type: "second", Status: metav1.ConditionTrue, LastTransitionTime: metav1.Time{Time: oneHourBefore}, Reason: "reason", Message: "message", ObservedGeneration: 3},
 				{Type: "third"},
 			},
 		},
+		{
+			name: "nothing changes",
+			conditions: []metav1.Condition{{
+				Type:               "type",
+				Status:             metav1.ConditionTrue,
+				LastTransitionTime: metav1.Time{Time: oneHourBefore},
+			}},
+			toAdd: metav1.Condition{Type: "type", Status: metav1.ConditionTrue, LastTransitionTime: metav1.Time{Time: oneHourBefore}},
+			expected: []metav1.Condition{{
+				Type:               "type",
+				Status:             metav1.ConditionTrue,
+				LastTransitionTime: metav1.Time{Time: oneHourBefore},
+			}},
+		},
 	}
 
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
-			SetStatusCondition(&test.conditions, test.toAdd)
+			changed := SetStatusCondition(&test.conditions, test.toAdd)
+			if test.expectChanged != changed {
+				t.Errorf("expectChanged=%t != changed=%t", test.expectChanged, changed)
+			}
 			if !reflect.DeepEqual(test.conditions, test.expected) {
 				t.Error(test.conditions)
 			}
@@ -92,6 +113,7 @@ func TestRemoveStatusCondition(t *testing.T) {
 		name          string
 		conditions    []metav1.Condition
 		conditionType string
+		expectRemoval bool
 		expected      []metav1.Condition
 	}{
 		{
@@ -102,6 +124,7 @@ func TestRemoveStatusCondition(t *testing.T) {
 				{Type: "third"},
 			},
 			conditionType: "second",
+			expectRemoval: true,
 			expected: []metav1.Condition{
 				{Type: "first"},
 				{Type: "third"},
@@ -131,7 +154,10 @@ func TestRemoveStatusCondition(t *testing.T) {
 
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
-			RemoveStatusCondition(&test.conditions, test.conditionType)
+			removed := RemoveStatusCondition(&test.conditions, test.conditionType)
+			if test.expectRemoval != removed {
+				t.Errorf("expectRemoval=%t != removal=%t", test.expectRemoval, removed)
+			}
 			if !reflect.DeepEqual(test.conditions, test.expected) {
 				t.Error(test.conditions)
 			}