From 3c60f3518219b58dd20440b74d11c10a0dddcc9b Mon Sep 17 00:00:00 2001 From: Balazs Gibizer Date: Mon, 6 Mar 2023 12:37:35 +0100 Subject: [PATCH] Mirror order conditions as False, Unknown, True This is a breaking change on Conditions.Mirror(). Originally this call ordered conditions as False, True, Unknown then picked the first of that list to reflect. However all the use cases (we see) assume that if there is reasourceA that depends on the status of resourceB then resourceA reconciler calls: resourceB.Status.Mirror(ResourceBSpecificSubConditionOfA) and expects that the call returns the most sever non True condition if any or return a True condition otherwise. So the original behavior that returned True even if there was Unknown condition was unexpected. This patch changes the order of the status to False, Unknown, True. But keeps the original logic that if the ReadyCondition is True then the rest of the conditions are ignored even if there are False conditions in the list. --- modules/common/condition/funcs.go | 27 ++++++++++---- modules/common/condition/funcs_test.go | 50 ++++++++++++++++++++------ 2 files changed, 61 insertions(+), 16 deletions(-) diff --git a/modules/common/condition/funcs.go b/modules/common/condition/funcs.go index 71db2fe1..13f34f2b 100644 --- a/modules/common/condition/funcs.go +++ b/modules/common/condition/funcs.go @@ -295,9 +295,15 @@ func GetHigherPrioCondition(cond1, cond2 *Condition) *Condition { } // Mirror - mirrors Status, Message, Reason and Severity from the latest condition -// of a sorted conditionGroup list into a target condition of type t. -// The conditionGroup entries are split by Status with the order False, True, Unknown. +// of a sorted conditionGroup list into a target condition of type t. If the +// top level ReadyCondition is True then it is assumed that there are no False +// or important Uknown conditions present in the list as ReadyCondition is expected +// to be an aggregated status condition. +// If ReadyCondition is not True then the conditionGroup entries are split by +// Status with the order False, Unknown, True. // If Status=False its again split into Severity with the order Error, Warning, Info. +// So Mirror either reflects the ReadyCondition=True or reflects the latest most +// sever False or Uknown condition. func (conditions *Conditions) Mirror(t Type) *Condition { if conditions == nil || len(*conditions) == 0 { @@ -344,8 +350,17 @@ func (conditions *Conditions) Mirror(t Type) *Condition { break } - mirrorCondition = UnknownCondition(t, c.Reason, c.Message) - mirrorCondition.LastTransitionTime = c.LastTransitionTime + if c.Status == corev1.ConditionUnknown { + mirrorCondition = UnknownCondition(t, c.Reason, c.Message) + mirrorCondition.LastTransitionTime = c.LastTransitionTime + break + } + + // The only valid values for Status is True, False, Unknown are handled + // above so if we reach here then we have an invalid status condition. + // This should never happen. + panic(fmt.Sprintf("Condition %v has invalid status value '%s'. The only valid values are True, False, Unknown", c, c.Status)) + } return mirrorCondition @@ -392,9 +407,9 @@ func groupOrder(c Condition) int { case SeverityInfo: return 2 } - case corev1.ConditionTrue: - return 3 case corev1.ConditionUnknown: + return 3 + case corev1.ConditionTrue: return 4 } diff --git a/modules/common/condition/funcs_test.go b/modules/common/condition/funcs_test.go index 2512c406..9ed2e0bf 100644 --- a/modules/common/condition/funcs_test.go +++ b/modules/common/condition/funcs_test.go @@ -386,11 +386,11 @@ func TestMirror(t *testing.T) { conditions.Set(trueA) g.Expect(conditions).To(haveSameConditionsOf(CreateList(unknownReady, trueA))) targetCondition = conditions.Mirror("targetConditon") - // expect to be mirrored trueA - g.Expect(targetCondition.Status).To(BeIdenticalTo(trueA.Status)) - g.Expect(targetCondition.Severity).To(BeIdenticalTo(trueA.Severity)) - g.Expect(targetCondition.Reason).To(BeIdenticalTo(trueA.Reason)) - g.Expect(targetCondition.Message).To(BeIdenticalTo(trueA.Message)) + // expect to be mirrored unknownReady + g.Expect(targetCondition.Status).To(BeIdenticalTo(unknownReady.Status)) + g.Expect(targetCondition.Severity).To(BeIdenticalTo(unknownReady.Severity)) + g.Expect(targetCondition.Reason).To(BeIdenticalTo(unknownReady.Reason)) + g.Expect(targetCondition.Message).To(BeIdenticalTo(unknownReady.Message)) conditions.Set(falseB) g.Expect(conditions).To(haveSameConditionsOf(CreateList(unknownReady, trueA, falseB))) @@ -410,10 +410,13 @@ func TestMirror(t *testing.T) { g.Expect(targetCondition.Reason).To(BeIdenticalTo(falseBError.Reason)) g.Expect(targetCondition.Message).To(BeIdenticalTo(falseBError.Message)) - // mark all non true conditions to be true + // mark ReadyCondition to true + // We expect that ReadyCondition True means the overall status of the + // resource is ready that condition is mirrored even if there are other + // conditions in non True state. conditions.MarkTrue(ReadyCondition, ReadyMessage) - conditions.MarkTrue(trueB.Type, trueB.Message) - g.Expect(conditions).To(haveSameConditionsOf(CreateList(trueReady, trueA, trueB))) + conditions.Set(unknownA) + g.Expect(conditions).To(haveSameConditionsOf(CreateList(trueReady, unknownA, falseBError))) targetCondition = conditions.Mirror("targetConditon") // expect to be mirrored trueReady g.Expect(targetCondition.Status).To(BeIdenticalTo(trueReady.Status)) @@ -422,6 +425,29 @@ func TestMirror(t *testing.T) { g.Expect(targetCondition.Message).To(BeIdenticalTo(trueReady.Message)) } +func TestMirrorInvalidStatus(t *testing.T) { + g := NewWithT(t) + + conditions := Conditions{} + invalidStatusA := Condition{ + Type: "a", + Status: "FooBar", + Reason: "", + Severity: SeverityNone, + Message: "", + } + conditions.Init(&Conditions{invalidStatusA}) + // NOTE(gibi): we always have a ReadyCondition with Unknown status added + // automatically so it is picked up by Mirror before reaching to the + // condition with FooBar status as FooBar status handled as the lowest prio + // as it is not matching of any expectes Status value. So to trigger the + // error case we need to remove the ReadyCondition explicitly. + conditions.Remove(ReadyCondition) + + g.Expect(func() { conditions.Mirror("targetConditon") }).To( + PanicWith(MatchRegexp(`Condition \{a FooBar .*\} has invalid status value 'FooBar'.`))) +} + func TestIsError(t *testing.T) { g := NewWithT(t) @@ -444,11 +470,15 @@ func TestGetHigherPrioCondition(t *testing.T) { c = GetHigherPrioCondition(nil, unknownA) g.Expect(hasSameState(c, unknownA)).To(BeTrue()) - // Status True has higher prio then Status Unknown + // Status Unknown has higher prio then Status True c = GetHigherPrioCondition(unknownA, trueA) - g.Expect(hasSameState(c, trueA)).To(BeTrue()) + g.Expect(hasSameState(c, unknownA)).To(BeTrue()) // Status False has higher prio then Status Unknown + c = GetHigherPrioCondition(falseA, unknownA) + g.Expect(hasSameState(c, falseA)).To(BeTrue()) + + // Status False has higher prio then Status True c = GetHigherPrioCondition(falseA, trueA) g.Expect(hasSameState(c, falseA)).To(BeTrue())