Skip to content

Commit

Permalink
Updated based on PR comments
Browse files Browse the repository at this point in the history
  • Loading branch information
jonathan-innis committed Nov 25, 2024
1 parent 9c8eb0d commit cc1ecca
Show file tree
Hide file tree
Showing 5 changed files with 37 additions and 40 deletions.
45 changes: 24 additions & 21 deletions status/condition_set.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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 {
Expand All @@ -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 {
Expand Down
3 changes: 1 addition & 2 deletions status/condition_set_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand Down
20 changes: 5 additions & 15 deletions status/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -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), ""),
Expand Down
8 changes: 6 additions & 2 deletions status/controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down
1 change: 1 addition & 0 deletions test/object.go
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand Down

0 comments on commit cc1ecca

Please sign in to comment.