From 9ba5716af8d2dafa8fd552b72946d9c83d059881 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 | 55 +++++++++++++++++++++++--- 2 files changed, 70 insertions(+), 12 deletions(-) diff --git a/modules/common/condition/funcs.go b/modules/common/condition/funcs.go index 71db2fe1b..13f34f2bd 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 2512c4066..de4095b74 100644 --- a/modules/common/condition/funcs_test.go +++ b/modules/common/condition/funcs_test.go @@ -387,10 +387,10 @@ func TestMirror(t *testing.T) { 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)) + 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))) @@ -422,6 +422,45 @@ func TestMirror(t *testing.T) { g.Expect(targetCondition.Message).To(BeIdenticalTo(trueReady.Message)) } +func TestMirrorReadyTrueAndFalse(t *testing.T) { + g := NewWithT(t) + + conditions := Conditions{} + // 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.Init(&Conditions{*trueReady, *falseB, *unknownA}) + + targetCondition := conditions.Mirror("targetConditon") + + g.Expect(targetCondition.Status).To(BeIdenticalTo(trueReady.Status)) + g.Expect(targetCondition.Severity).To(BeIdenticalTo(trueReady.Severity)) + g.Expect(targetCondition.Reason).To(BeIdenticalTo(trueReady.Reason)) + 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 added automatically so it + // is picked up by Mirror before reaching to the condition with FooBar + // status. 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 +483,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())