Skip to content

Commit

Permalink
Merge pull request #11342 from fabriziopandini/truncate-lastTransitio…
Browse files Browse the repository at this point in the history 
…nTime-v1beta2-conditions

🌱 Truncate lastTransitionTime for v1beta2 conditions
  • Loading branch information
@k8s-ci-robot
k8s-ci-robot authored Oct 29, 2024
2 parents 419a9a6 + 09e6c8a commit d1ca160
Show file tree
Hide file tree
Showing 4 changed files with 70 additions and 13 deletions.
8 changes: 4 additions & 4 deletions util/conditions/v1beta2/patch.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand All @@ -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
}

Expand All @@ -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.
Expand Down
24 changes: 16 additions & 8 deletions util/conditions/v1beta2/patch_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package v1beta2

import (
"testing"
"time"

. "github.com/onsi/gomega"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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,
Expand All @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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,
Expand All @@ -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},
Expand All @@ -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},
Expand Down
14 changes: 13 additions & 1 deletion util/conditions/v1beta2/setter.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package v1beta2

import (
"sort"
"time"

"k8s.io/apimachinery/pkg/api/meta"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
Expand Down Expand Up @@ -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
}

Expand All @@ -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 {
Expand Down
37 changes: 37 additions & 0 deletions util/conditions/v1beta2/setter_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package v1beta2

import (
"testing"
"time"

"github.com/google/go-cmp/cmp"
. "github.com/onsi/gomega"
Expand Down Expand Up @@ -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) {
Expand Down

0 comments on commit d1ca160

Please sign in to comment.