From cc1ecca0b24a41708444e6ca5732b45448330a7b Mon Sep 17 00:00:00 2001 From: Jonathan Innis Date: Sun, 24 Nov 2024 21:23:15 -0800 Subject: [PATCH] Updated based on PR comments --- status/condition_set.go | 45 +++++++++++++++++++----------------- status/condition_set_test.go | 3 +-- status/controller.go | 20 ++++------------ status/controller_test.go | 8 +++++-- test/object.go | 1 + 5 files changed, 37 insertions(+), 40 deletions(-) diff --git a/status/condition_set.go b/status/condition_set.go index 717d045..43a6433 100644 --- a/status/condition_set.go +++ b/status/condition_set.go @@ -57,13 +57,7 @@ func (r ConditionTypes) For(object Object) ConditionSet { // Set the root condition first to get consistent timing for LastTransitionTime for _, t := range append([]string{r.root}, r.dependents...) { if cs.Get(t) == nil { - cs.Set(Condition{ - Type: t, - Status: metav1.ConditionUnknown, - Reason: "AwaitingReconciliation", - Message: "object is awaiting reconciliation", - LastTransitionTime: object.GetCreationTimestamp(), - }) + cs.SetUnknown(t) } } return cs @@ -106,32 +100,41 @@ func (c ConditionSet) IsTrue(conditionTypes ...string) bool { return true } -func (c ConditionSet) IsNormalCondition(t string) bool { +func (c ConditionSet) IsDependentCondition(t string) bool { return t == c.root || lo.Contains(c.dependents, t) } // Set sets or updates the Condition on Conditions for Condition.Type. // If there is an update, Conditions are stored back sorted. func (c ConditionSet) Set(condition Condition) (modified bool) { - conditionType := condition.Type var conditions []Condition - // If we get passed a LastTransitionTime, we should respect it - if condition.LastTransitionTime.IsZero() { - condition.LastTransitionTime = metav1.Now() - } + var foundCondition bool + condition.ObservedGeneration = c.object.GetGeneration() for _, cond := range c.object.GetConditions() { - if cond.Type != conditionType { + if cond.Type != condition.Type { conditions = append(conditions, cond) } else { - if condition.Status == cond.Status && !cond.LastTransitionTime.IsZero() { + foundCondition = true + if condition.Status == cond.Status { condition.LastTransitionTime = cond.LastTransitionTime + } else { + condition.LastTransitionTime = metav1.Now() } if reflect.DeepEqual(condition, cond) { return false } } } + if !foundCondition { + // Dependent conditions should always be set, so if it's not found, that means + // that we are initializing the condition type, and it's last "transition" was object creation + if c.IsDependentCondition(condition.Type) { + condition.LastTransitionTime = c.object.GetCreationTimestamp() + } else { + condition.LastTransitionTime = metav1.Now() + } + } conditions = append(conditions, condition) // Sorted for convenience of the consumer, i.e. kubectl. sort.SliceStable(conditions, func(i, j int) bool { @@ -144,21 +147,21 @@ func (c ConditionSet) Set(condition Condition) (modified bool) { c.object.SetConditions(conditions) // Recompute the root condition after setting any other condition - c.recomputeRootCondition(conditionType) + c.recomputeRootCondition(condition.Type) return true } -// Clear removes the abnormal condition that matches the ConditionType -// Not implemented for normal conditions +// Clear removes the independent condition that matches the ConditionType +// Not implemented for dependent conditions func (c ConditionSet) Clear(t string) error { var conditions []Condition if c.object == nil { return nil } - // Normal conditions are not handled as they can't be nil - if c.IsNormalCondition(t) { - return fmt.Errorf("clearing normal conditions not implemented") + // Dependent conditions are not handled as they can't be nil + if c.IsDependentCondition(t) { + return fmt.Errorf("clearing dependent conditions not implemented") } cond := c.Get(t) if cond == nil { diff --git a/status/condition_set_test.go b/status/condition_set_test.go index 63a0819..18f790f 100644 --- a/status/condition_set_test.go +++ b/status/condition_set_test.go @@ -13,8 +13,7 @@ import ( var _ = Describe("Conditions", func() { It("should correctly toggle conditions", func() { - testObject := test.CustomObject{} - testObject.Generation = 1 + testObject := test.Object(&test.CustomObject{}) // Conditions should be initialized conditions := testObject.StatusConditions() Expect(conditions.Get(test.ConditionTypeFoo).GetStatus()).To(Equal(metav1.ConditionUnknown)) diff --git a/status/controller.go b/status/controller.go index 8d2a8cd..26b537b 100644 --- a/status/controller.go +++ b/status/controller.go @@ -225,27 +225,17 @@ func (c *Controller[T]) reconcile(ctx context.Context, req reconcile.Request, o MetricLabelConditionStatus: string(condition.Status), pmetrics.LabelReason: condition.Reason, }) - var duration time.Duration - var status metav1.ConditionStatus + // If the previous condition was nil, we can't know how long the condition was in an "Unknown" state if observedCondition == nil { - // We only treat the CreationTimestamp as the LastTransitionTime for normal conditions - // We can't know the LastTransitionTime if the condition is absent for abnormal conditions - if !o.StatusConditions().IsNormalCondition(condition.Type) { - continue - } - duration = condition.LastTransitionTime.Time.Sub(o.GetCreationTimestamp().Time) - status = metav1.ConditionUnknown - } else { - duration = condition.LastTransitionTime.Time.Sub(observedCondition.LastTransitionTime.Time) - status = observedCondition.Status + continue } - c.observeHistogram(c.ConditionDuration, ConditionDuration, duration.Seconds(), map[string]string{ + c.observeHistogram(c.ConditionDuration, ConditionDuration, condition.LastTransitionTime.Time.Sub(observedCondition.LastTransitionTime.Time).Seconds(), map[string]string{ pmetrics.LabelType: condition.Type, - MetricLabelConditionStatus: string(status), + MetricLabelConditionStatus: string(observedCondition.Status), }) c.eventRecorder.Event(o, v1.EventTypeNormal, condition.Type, fmt.Sprintf("Status condition transitioned, Type: %s, Status: %s -> %s, Reason: %s%s", condition.Type, - status, + observedCondition.Status, condition.Status, condition.Reason, lo.Ternary(condition.Message != "", fmt.Sprintf(", Message: %s", condition.Message), ""), diff --git a/status/controller_test.go b/status/controller_test.go index 7259ab6..3bae97b 100644 --- a/status/controller_test.go +++ b/status/controller_test.go @@ -525,10 +525,14 @@ var _ = Describe("Controller", func() { Expect(testObject.StatusConditions().Get(status.ConditionReady).LastTransitionTime.Time).To(Equal(testObject.GetCreationTimestamp().Time)) }) It("should consider status conditions that aren't set as unknown", func() { + // This mimics an object creation testObject := test.Object(&test.CustomObject{}) - testObject.StatusConditions().SetTrue(test.ConditionTypeFoo) // initialize all conditions, set Foo to true + ExpectApplied(ctx, kubeClient, testObject) + ExpectReconciled(ctx, controller, testObject) - // conditions not set + // Then the status conditions gets initialized and a condition is set to True + testObject.StatusConditions().SetTrue(test.ConditionTypeFoo) + testObject.SetCreationTimestamp(metav1.Time{Time: time.Now().Add(time.Hour)}) ExpectApplied(ctx, kubeClient, testObject) ExpectReconciled(ctx, controller, testObject) diff --git a/test/object.go b/test/object.go index 8fe9087..0af6471 100644 --- a/test/object.go +++ b/test/object.go @@ -29,6 +29,7 @@ func Object[T client.Object](base T, overrides ...T) T { dest := reflect.New(reflect.TypeOf(base).Elem()).Interface().(T) dest.SetName(RandomName()) dest.SetNamespace(Namespace.Name) + dest.SetGeneration(1) dest.SetLabels(lo.Assign(dest.GetLabels(), map[string]string{DiscoveryLabel: dest.GetName()})) for _, src := range append([]T{base}, overrides...) { lo.Must0(mergo.Merge(dest, src, mergo.WithOverride))