Merge pull request #11342 from fabriziopandini/truncate-lastTransitionTime-v1beta2-conditions

🌱 Truncate lastTransitionTime for v1beta2 conditions

Author: k8s-ci-robot

util/conditions/v1beta2/patch.go

@@ -148,7 +148,7 @@ func (p Patch) Apply(latest Setter, opts ...PatchApplyOption) error {
 		case AddConditionPatch:
 			// If the condition is owned, always keep the after value.
 			if applyOpt.forceOverwrite || applyOpt.isOwnedConditionType(conditionPatch.After.Type) {
-				meta.SetStatusCondition(&latestConditions, *conditionPatch.After)
+				setStatusCondition(&latestConditions, *conditionPatch.After)
 				continue
 			}
 
@@ -163,12 +163,12 @@ func (p Patch) Apply(latest Setter, opts ...PatchApplyOption) error {
 				continue
 			}
 			// If the condition does not exists on the latest, add the new after condition.
-			meta.SetStatusCondition(&latestConditions, *conditionPatch.After)
+			setStatusCondition(&latestConditions, *conditionPatch.After)
 
 		case ChangeConditionPatch:
 			// If the conditions is owned, always keep the after value.
 			if applyOpt.forceOverwrite || applyOpt.isOwnedConditionType(conditionPatch.After.Type) {
-				meta.SetStatusCondition(&latestConditions, *conditionPatch.After)
+				setStatusCondition(&latestConditions, *conditionPatch.After)
 				continue
 			}
 
@@ -190,7 +190,7 @@ func (p Patch) Apply(latest Setter, opts ...PatchApplyOption) error {
 				continue
 			}
 			// Otherwise apply the new after condition.
-			meta.SetStatusCondition(&latestConditions, *conditionPatch.After)
+			setStatusCondition(&latestConditions, *conditionPatch.After)
 
 		case RemoveConditionPatch:
 			// If latestConditions is nil or empty, nothing to remove.

util/conditions/v1beta2/patch_test.go

@@ -18,6 +18,7 @@ package v1beta2
 
 import (
 	"testing"
+	"time"
 
 	. "github.com/onsi/gomega"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -26,8 +27,9 @@ import (
 )
 
 func TestNewPatch(t *testing.T) {
-	fooTrue := metav1.Condition{Type: "foo", Status: metav1.ConditionTrue}
-	fooFalse := metav1.Condition{Type: "foo", Status: metav1.ConditionFalse}
+	now := metav1.Now()
+	fooTrue := metav1.Condition{Type: "foo", Status: metav1.ConditionTrue, LastTransitionTime: now}
+	fooFalse := metav1.Condition{Type: "foo", Status: metav1.ConditionFalse, LastTransitionTime: now}
 
 	tests := []struct {
 		name    string
@@ -133,6 +135,12 @@ func TestApply(t *testing.T) {
 	fooFalse := metav1.Condition{Type: "foo", Status: metav1.ConditionFalse, LastTransitionTime: now}
 	fooFalse2 := metav1.Condition{Type: "foo", Status: metav1.ConditionFalse, Reason: "Something else", LastTransitionTime: now}
 
+	addMilliseconds := func(c metav1.Condition) metav1.Condition {
+		c1 := c.DeepCopy()
+		c1.LastTransitionTime.Time = c1.LastTransitionTime.Add(10 * time.Millisecond)
+		return *c1
+	}
+
 	tests := []struct {
 		name    string
 		before  Setter
@@ -169,7 +177,7 @@ func TestApply(t *testing.T) {
 		{
 			name:    "Add: When a condition does not exists, it should add",
 			before:  objectWithConditions(),
-			after:   objectWithConditions(fooTrue),
+			after:   objectWithConditions(addMilliseconds(fooTrue)), // this will force the test to fail if an AddConditionPatch operation doesn't drop milliseconds
 			latest:  objectWithConditions(),
 			want:    []metav1.Condition{fooTrue},
 			wantErr: false,
@@ -193,7 +201,7 @@ func TestApply(t *testing.T) {
 		{
 			name:    "Add: When a condition already exists but with conflicts, it should not error if force override is set",
 			before:  objectWithConditions(),
-			after:   objectWithConditions(fooTrue),
+			after:   objectWithConditions(addMilliseconds(fooTrue)), // this will force the test to fail if an AddConditionPatch operation doesn't drop milliseconds
 			latest:  objectWithConditions(fooFalse),
 			options: []PatchApplyOption{ForceOverwrite(true)},
 			want:    []metav1.Condition{fooTrue}, // after condition should be kept in case of error
@@ -202,7 +210,7 @@ func TestApply(t *testing.T) {
 		{
 			name:    "Add: When a condition already exists but with conflicts, it should not error if the condition is owned",
 			before:  objectWithConditions(),
-			after:   objectWithConditions(fooTrue),
+			after:   objectWithConditions(addMilliseconds(fooTrue)), // this will force the test to fail if an AddConditionPatch operation doesn't drop milliseconds
 			latest:  objectWithConditions(fooFalse),
 			options: []PatchApplyOption{OwnedConditionTypes{"foo"}},
 			want:    []metav1.Condition{fooTrue}, // after condition should be kept in case of error
@@ -253,7 +261,7 @@ func TestApply(t *testing.T) {
 		{
 			name:    "Change: When a condition exists without conflicts, it should change",
 			before:  objectWithConditions(fooTrue),
-			after:   objectWithConditions(fooFalse),
+			after:   objectWithConditions(addMilliseconds(fooFalse)), // this will force the test to fail if an ChangeConditionPatch operation doesn't drop milliseconds
 			latest:  objectWithConditions(fooTrue),
 			want:    []metav1.Condition{fooFalse},
 			wantErr: false,
@@ -277,7 +285,7 @@ func TestApply(t *testing.T) {
 		{
 			name:    "Change: When a condition exists with conflicts but there is no agreement on the final state, it should not error if force override is set",
 			before:  objectWithConditions(fooFalse),
-			after:   objectWithConditions(fooFalse2),
+			after:   objectWithConditions(addMilliseconds(fooFalse2)), // this will force the test to fail if an ChangeConditionPatch operation doesn't drop milliseconds
 			latest:  objectWithConditions(fooTrue),
 			options: []PatchApplyOption{ForceOverwrite(true)},
 			want:    []metav1.Condition{fooFalse2},
@@ -286,7 +294,7 @@ func TestApply(t *testing.T) {
 		{
 			name:    "Change: When a condition exists with conflicts but there is no agreement on the final state, it should not error if the condition is owned",
 			before:  objectWithConditions(fooFalse),
-			after:   objectWithConditions(fooFalse2),
+			after:   objectWithConditions(addMilliseconds(fooFalse2)), // this will force the test to fail if an ChangeConditionPatch operation doesn't drop milliseconds
 			latest:  objectWithConditions(fooTrue),
 			options: []PatchApplyOption{OwnedConditionTypes{"foo"}},
 			want:    []metav1.Condition{fooFalse2},

util/conditions/v1beta2/setter.go

@@ -18,6 +18,7 @@ package v1beta2
 
 import (
 	"sort"
+	"time"
 
 	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -85,7 +86,7 @@ func Set(targetObj Setter, condition metav1.Condition, opts ...SetOption) {
 	}
 
 	conditions := targetObj.GetV1Beta2Conditions()
-	if changed := meta.SetStatusCondition(&conditions, condition); !changed {
+	if changed := setStatusCondition(&conditions, condition); !changed {
 		return
 	}
 
@@ -98,6 +99,17 @@ func Set(targetObj Setter, condition metav1.Condition, opts ...SetOption) {
 	targetObj.SetV1Beta2Conditions(conditions)
 }
 
+func setStatusCondition(conditions *[]metav1.Condition, condition metav1.Condition) bool {
+	// Truncate last transition time to seconds.
+	// This prevents inconsistencies from what we have in objects in memory and what Marshal/Unmarshal
+	// will do while the data is sent to/read from the API server.
+	if condition.LastTransitionTime.IsZero() {
+		condition.LastTransitionTime = metav1.Now()
+	}
+	condition.LastTransitionTime.Time = condition.LastTransitionTime.Truncate(1 * time.Second)
+	return meta.SetStatusCondition(conditions, condition)
+}
+
 // Delete deletes the condition with the given type.
 func Delete(to Setter, conditionType string) {
 	if to == nil {

util/conditions/v1beta2/setter_test.go

@@ -18,6 +18,7 @@ package v1beta2
 
 import (
 	"testing"
+	"time"
 
 	"github.com/google/go-cmp/cmp"
 	. "github.com/onsi/gomega"
@@ -176,6 +177,42 @@ func TestSet(t *testing.T) {
 		expected := []metav1.Condition{condition}
 		g.Expect(foo.Status.Conditions).To(Equal(expected), cmp.Diff(foo.Status.Conditions, expected))
 	})
+
+	t.Run("Set drops milliseconds", func(t *testing.T) {
+		g := NewWithT(t)
+		foo := &builder.Phase3Obj{
+			ObjectMeta: metav1.ObjectMeta{Generation: 123},
+			Status: builder.Phase3ObjStatus{
+				Conditions: nil,
+			},
+		}
+
+		condition := metav1.Condition{
+			Type:    "fooCondition",
+			Status:  metav1.ConditionTrue,
+			Reason:  "FooReason",
+			Message: "FooMessage",
+		}
+
+		// Check LastTransitionTime after setting a condition for the first time
+		Set(foo, condition)
+		ltt1 := foo.Status.Conditions[0].LastTransitionTime.Time
+		g.Expect(ltt1).To(Equal(ltt1.Truncate(1*time.Second)), cmp.Diff(ltt1, ltt1.Truncate(1*time.Second)))
+
+		// Check LastTransitionTime after changing an existing condition
+		condition.Status = metav1.ConditionFalse     // this will force set to change the LastTransitionTime
+		condition.LastTransitionTime = metav1.Time{} // this will force set to compute a new LastTransitionTime
+		Set(foo, condition)
+		ltt2 := foo.Status.Conditions[0].LastTransitionTime.Time
+		g.Expect(ltt2).To(Equal(ltt2.Truncate(1*time.Second)), cmp.Diff(ltt2, ltt2.Truncate(1*time.Second)))
+
+		// Check LastTransitionTime after setting a Time with milliseconds
+		condition.Status = metav1.ConditionTrue     // this will force set to change the LastTransitionTime
+		condition.LastTransitionTime = metav1.Now() // this will force set to not default LastTransitionTime
+		Set(foo, condition)
+		ltt3 := foo.Status.Conditions[0].LastTransitionTime.Time
+		g.Expect(ltt3).To(Equal(ltt3.Truncate(1*time.Second)), cmp.Diff(ltt3, ltt3.Truncate(1*time.Second)))
+	})
 }
 
 func TestDelete(t *testing.T) {