From 09e6c8a14c2dd955924e7534727974d1e89643e5 Mon Sep 17 00:00:00 2001 From: fabriziopandini Date: Fri, 25 Oct 2024 17:29:48 +0200 Subject: [PATCH] Truncate lastTransitionTime for v1beta2 conditions --- util/conditions/v1beta2/patch.go | 8 +++--- util/conditions/v1beta2/patch_test.go | 24 +++++++++++------ util/conditions/v1beta2/setter.go | 14 +++++++++- util/conditions/v1beta2/setter_test.go | 37 ++++++++++++++++++++++++++ 4 files changed, 70 insertions(+), 13 deletions(-) diff --git a/util/conditions/v1beta2/patch.go b/util/conditions/v1beta2/patch.go index 99f17a52eb33..4c94286b8f91 100644 --- a/util/conditions/v1beta2/patch.go +++ b/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. diff --git a/util/conditions/v1beta2/patch_test.go b/util/conditions/v1beta2/patch_test.go index d0b71458afea..b2e34f9f1757 100644 --- a/util/conditions/v1beta2/patch_test.go +++ b/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}, diff --git a/util/conditions/v1beta2/setter.go b/util/conditions/v1beta2/setter.go index c41b98b12332..75b1042b94e7 100644 --- a/util/conditions/v1beta2/setter.go +++ b/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 { diff --git a/util/conditions/v1beta2/setter_test.go b/util/conditions/v1beta2/setter_test.go index 33f6786944b9..34266ff05b9a 100644 --- a/util/conditions/v1beta2/setter_test.go +++ b/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) {