diff --git a/docs/proposals/20200506-conditions.md b/docs/proposals/20200506-conditions.md index a4774ea000da..07f2f51f4060 100644 --- a/docs/proposals/20200506-conditions.md +++ b/docs/proposals/20200506-conditions.md @@ -6,7 +6,7 @@ reviewers: - "@vincepri" - "@ncdc" creation-date: 2020-05-06 -last-updated: 2020-05-20 +last-updated: 2024-05-03 status: implementable see-also: replaces: @@ -232,7 +232,7 @@ const ( ) ``` -Condition types MUST have a consistent polarity (i.e. "True = good"); +Condition type names should make sense for humans; neither positive nor negative polarity can be recommended as a general rule. Condition types SHOULD have one of the following suffix: @@ -243,8 +243,9 @@ When the above suffix are not adequate for a specific condition type, other suff (e.g. `Completed`, `Healthy`); however, it is recommended to balance this flexibility with the objective to provide a consistent condition naming across all the Cluster API objects. -The `Severity` field MUST be set only when `Status=False` and it is designed to provide a standard classification -of possible conditions failure `Reason`. +The `Severity` field MUST be set only when `Status=False` for conditions with positive polarity, or when `Status=True` +for conditions with negative polarity; severity is designed to provide a standard classification of possible +conditions failure `Reason`. Please note that the combination of `Reason` and `Severity` gives different meaning to a condition failure allowing to detect when a long-running task is still ongoing: @@ -298,6 +299,8 @@ the following constraints/design principles MUST be applied: of the underlying `Machines.Status.Conditions[Ready]` conditions. - A corollary of the above set of constraints is that an object SHOULD NEVER be in status `Ready=True` if one of the object's conditions are `false` or if one of the object dependents is in status `Ready=False`. +- Condition that do not represent the operational state of the component, MUST not be included in the `Ready` condition + (e.g. `Paused`, which represent a state of the controller that manages the component). ##### Controller changes @@ -471,6 +474,7 @@ enhance the condition utilities to handle those situations in a generalized way. - Risk: This proposal aims to be consistent with the target state of conditions in Kubernetes, but this is still under definition (see [KEP](https://github.com/kubernetes/enhancements/pull/1624)). - Mitigation: Periodically re-evaluate this proposal vs the Kubernetes KEP. + - 2024-05-03: Change to allow conditions without positive polarity goes into this direction - Risk: Cluster API presents some specific challenges that are not common to the core Kubernetes objects. - Mitigation: To allow a minimal set of carefully evaluated differences between Cluster API and Kubernetes @@ -480,25 +484,6 @@ enhance the condition utilities to handle those situations in a generalized way. - Risk: This proposal allows for implementing conditions in incremental fashion, and this makes it complex to ensure a consistent approach across all objects. - Mitigation: Ensure all the implementations comply with the defined set of constraints/design principles. - -- Risk: Having a consistent polarity ensures a simple and clear contract with the consumers, and it allows - processing conditions in a simple and consistent way without being forced to implement specific logic - for each condition type. However, we are aware about the fact that enforcing of consistent polarity (truthy) - combined with the usage of recommended suffix for condition types can lead to verbal contortions to express - conditions, especially in case of conditions designed to signal problems or in case of conditions - that might exist or not. - - Mitigation: We are relaxing the rule about recommended suffix and allowing usage of custom suffix. - - Mitigation: We are recommending the condition adhere to the design principle to express the operational state - of the component, and this should help in avoiding conditions name to surface internal implementation details. - - Mitigation: We should recommend condition implementers to clearly document the meaning of Unknown state, because as - discussed also in the recent [Kubernetes KEP about standardizing conditions](https://github.com/kubernetes/enhancements/pull/1624#pullrequestreview-388777427), - _"Unknown" is a fact about the writer of the condition, and not a claim about the object_. - - Mitigation: We should recommend developers of code relying on conditions to treat Unknown as a separated state vs - assimilating it to True or False, because this can vary case by case and generate confusion in readers. - - As a final consideration about the risk related to using a consistent polarity, it is important to notice that a - consistent polarity ensure a clear meaning for True or o False states, which is already an improvement vs having - different interpretations for all the three possible condition states. ## Alternatives @@ -569,5 +554,6 @@ NA ## Implementation History -- [ ] 2020-04-27: Compile a Google Doc following the CAEP template -- [ ] 2020-05-06: Create CAEP PR +- [x] 2020-04-27: Compile a Google Doc following the CAEP template +- [x] 2020-05-06: Create CAEP PR +- [x] 2024-05-03: Edited allowing conditions with negative polarity diff --git a/util/conditions/getter.go b/util/conditions/getter.go index 0ff89940abaa..bfed245f812e 100644 --- a/util/conditions/getter.go +++ b/util/conditions/getter.go @@ -117,6 +117,7 @@ func GetLastTransitionTime(from Getter, t clusterv1.ConditionType) *metav1.Time // summary returns a Ready condition with the summary of all the conditions existing // on an object. If the object does not have other conditions, no summary condition is generated. +// NOTE: The resulting Ready condition will have positive polarity; the conditions we are starting from might have positive or negative polarity. func summary(from Getter, options ...MergeOption) *clusterv1.Condition { conditions := from.GetConditions() @@ -147,8 +148,18 @@ func summary(from Getter, options ...MergeOption) *clusterv1.Condition { } } + // Keep track of the polarity of the condition we are starting from. + polarity := PositivePolarity + for _, t := range mergeOpt.negativeConditionTypes { + if c.Type == t { + polarity = NegativePolarity + break + } + } + conditionsInScope = append(conditionsInScope, localizedCondition{ Condition: &c, + Polarity: polarity, Getter: from, }) } @@ -210,6 +221,7 @@ func WithFallbackValue(fallbackValue bool, reason string, severity clusterv1.Con // mirror mirrors the Ready condition from a dependent object into the target condition; // if the Ready condition does not exists in the source object, no target conditions is generated. +// NOTE: Considering that we are mirroring Ready conditions with positive polarity, also the resulting condition will have positive polarity. func mirror(from Getter, targetCondition clusterv1.ConditionType, options ...MirrorOptions) *clusterv1.Condition { mirrorOpt := &mirrorOptions{} for _, o := range options { @@ -237,6 +249,7 @@ func mirror(from Getter, targetCondition clusterv1.ConditionType, options ...Mir // Aggregates all the Ready condition from a list of dependent objects into the target object; // if the Ready condition does not exists in one of the source object, the object is excluded from // the aggregation; if none of the source object have ready condition, no target conditions is generated. +// NOTE: Considering that we are aggregating Ready conditions with positive polarity, also the resulting condition will have positive polarity. func aggregate(from []Getter, targetCondition clusterv1.ConditionType, options ...MergeOption) *clusterv1.Condition { conditionsInScope := make([]localizedCondition, 0, len(from)) for i := range from { @@ -244,6 +257,7 @@ func aggregate(from []Getter, targetCondition clusterv1.ConditionType, options . conditionsInScope = append(conditionsInScope, localizedCondition{ Condition: condition, + Polarity: PositivePolarity, Getter: from[i], }) } diff --git a/util/conditions/getter_test.go b/util/conditions/getter_test.go index 1900480863c8..7b3e0e807d1c 100644 --- a/util/conditions/getter_test.go +++ b/util/conditions/getter_test.go @@ -20,6 +20,7 @@ import ( "testing" . "github.com/onsi/gomega" + "k8s.io/apimachinery/pkg/util/sets" clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" ) @@ -31,6 +32,13 @@ var ( falseInfo1 = FalseCondition("falseInfo1", "reason falseInfo1", clusterv1.ConditionSeverityInfo, "message falseInfo1") falseWarning1 = FalseCondition("falseWarning1", "reason falseWarning1", clusterv1.ConditionSeverityWarning, "message falseWarning1") falseError1 = FalseCondition("falseError1", "reason falseError1", clusterv1.ConditionSeverityError, "message falseError1") + + negativePolarityConditions = sets.New("false1-negative-polarity", "unknown1-negative-polarity", "trueInfo1-negative-polarity", "trueWarning1-negative-polarity", "trueError1-negative-polarity") + false1WithNegativePolarity = FalseConditionWithNegativePolarity("false1-negative-polarity") + unknown1WithNegativePolarity = UnknownCondition("unknown1-negative-polarity", "reason unknown1-negative-polarity", "message unknown1-negative-polarity") + trueInfo1WithNegativePolarity = TrueConditionWithNegativePolarity("trueInfo1-negative-polarity", "reason trueInfo1-negative-polarity", clusterv1.ConditionSeverityInfo, "message trueInfo1-negative-polarity") + trueWarning1WithNegativePolarity = TrueConditionWithNegativePolarity("trueWarning1-negative-polarity", "reason trueWarning1-negative-polarity", clusterv1.ConditionSeverityWarning, "message trueWarning1-negative-polarity") + trueError1WithNegativePolarity = TrueConditionWithNegativePolarity("trueError1-negative-polarity", "reason trueError1-negative-polarity", clusterv1.ConditionSeverityError, "message trueError1-negative-polarity") ) func TestGetAndHas(t *testing.T) { @@ -50,41 +58,54 @@ func TestGetAndHas(t *testing.T) { func TestIsMethods(t *testing.T) { g := NewWithT(t) - obj := getterWithConditions(nil1, true1, unknown1, falseInfo1, falseWarning1, falseError1) + obj := getterWithConditions(nil1, true1, unknown1, falseInfo1, falseWarning1, falseError1, false1WithNegativePolarity, unknown1WithNegativePolarity, trueInfo1WithNegativePolarity, trueWarning1WithNegativePolarity, trueError1WithNegativePolarity) // test isTrue g.Expect(IsTrue(obj, "nil1")).To(BeFalse()) g.Expect(IsTrue(obj, "true1")).To(BeTrue()) g.Expect(IsTrue(obj, "falseInfo1")).To(BeFalse()) g.Expect(IsTrue(obj, "unknown1")).To(BeFalse()) + g.Expect(IsTrue(obj, "false1-negative-polarity")).To(BeFalse()) + g.Expect(IsTrue(obj, "trueInfo1-negative-polarity")).To(BeTrue()) + g.Expect(IsTrue(obj, "unknown1-negative-polarity")).To(BeFalse()) // test isFalse g.Expect(IsFalse(obj, "nil1")).To(BeFalse()) g.Expect(IsFalse(obj, "true1")).To(BeFalse()) g.Expect(IsFalse(obj, "falseInfo1")).To(BeTrue()) g.Expect(IsFalse(obj, "unknown1")).To(BeFalse()) + g.Expect(IsFalse(obj, "false1-negative-polarity")).To(BeTrue()) + g.Expect(IsFalse(obj, "trueInfo1-negative-polarity")).To(BeFalse()) + g.Expect(IsFalse(obj, "unknown1-negative-polarity")).To(BeFalse()) // test isUnknown g.Expect(IsUnknown(obj, "nil1")).To(BeTrue()) g.Expect(IsUnknown(obj, "true1")).To(BeFalse()) g.Expect(IsUnknown(obj, "falseInfo1")).To(BeFalse()) g.Expect(IsUnknown(obj, "unknown1")).To(BeTrue()) + g.Expect(IsUnknown(obj, "false1-negative-polarity")).To(BeFalse()) + g.Expect(IsUnknown(obj, "trueInfo1-negative-polarity")).To(BeFalse()) + g.Expect(IsUnknown(obj, "unknown1-negative-polarity")).To(BeTrue()) // test GetReason g.Expect(GetReason(obj, "nil1")).To(Equal("")) g.Expect(GetReason(obj, "falseInfo1")).To(Equal("reason falseInfo1")) + g.Expect(GetReason(obj, "trueInfo1-negative-polarity")).To(Equal("reason trueInfo1-negative-polarity")) // test GetMessage g.Expect(GetMessage(obj, "nil1")).To(Equal("")) g.Expect(GetMessage(obj, "falseInfo1")).To(Equal("message falseInfo1")) + g.Expect(GetMessage(obj, "trueInfo1-negative-polarity")).To(Equal("message trueInfo1-negative-polarity")) // test GetSeverity + expectedSeverity := clusterv1.ConditionSeverityInfo g.Expect(GetSeverity(obj, "nil1")).To(BeNil()) severity := GetSeverity(obj, "falseInfo1") - expectedSeverity := clusterv1.ConditionSeverityInfo + g.Expect(severity).To(Equal(&expectedSeverity)) + severity = GetSeverity(obj, "trueInfo1-negative-polarity") g.Expect(severity).To(Equal(&expectedSeverity)) - // test GetMessage + // test GetLastTransitionTime g.Expect(GetLastTransitionTime(obj, "nil1")).To(BeNil()) g.Expect(GetLastTransitionTime(obj, "falseInfo1")).ToNot(BeNil()) } @@ -132,6 +153,8 @@ func TestSummary(t *testing.T) { foo := TrueCondition("foo") bar := FalseCondition("bar", "reason falseInfo1", clusterv1.ConditionSeverityInfo, "message falseInfo1") baz := FalseCondition("baz", "reason falseInfo2", clusterv1.ConditionSeverityInfo, "message falseInfo2") + fooWithNegativePolarity := FalseConditionWithNegativePolarity("foo-negative-polarity") + barWithNegativePolarity := TrueConditionWithNegativePolarity("bar-negative-polarity", "reason trueInfo1-negative-polarity", clusterv1.ConditionSeverityInfo, "message trueInfo1-negative-polarity") existingReady := FalseCondition(clusterv1.ReadyCondition, "reason falseError1", clusterv1.ConditionSeverityError, "message falseError1") // NB. existing ready has higher priority than other conditions tests := []struct { @@ -150,6 +173,18 @@ func TestSummary(t *testing.T) { from: getterWithConditions(foo, bar), want: FalseCondition(clusterv1.ReadyCondition, "reason falseInfo1", clusterv1.ConditionSeverityInfo, "message falseInfo1"), }, + { + name: "Returns ready condition with the summary of existing conditions with negative polarity (with default options)", + from: getterWithConditions(fooWithNegativePolarity, barWithNegativePolarity), + options: []MergeOption{WithNegativePolarityConditions("foo-negative-polarity", "bar-negative-polarity")}, + want: FalseCondition(clusterv1.ReadyCondition, "reason trueInfo1-negative-polarity", clusterv1.ConditionSeverityInfo, "message trueInfo1-negative-polarity"), + }, + { + name: "Returns ready condition with the summary of existing conditions with mixed polarity (with default options)", + from: getterWithConditions(foo, bar, fooWithNegativePolarity, barWithNegativePolarity), + options: []MergeOption{WithNegativePolarityConditions("foo-negative-polarity", "bar-negative-polarity")}, + want: FalseCondition(clusterv1.ReadyCondition, "reason falseInfo1", clusterv1.ConditionSeverityInfo, "message falseInfo1"), // bar take precedence on barWithNegativePolarity because it is listed first + }, { name: "Returns ready condition with the summary of existing conditions (using WithStepCounter options)", from: getterWithConditions(foo, bar), @@ -186,6 +221,18 @@ func TestSummary(t *testing.T) { options: []MergeOption{WithConditions("foo")}, // bar should be ignored want: TrueCondition(clusterv1.ReadyCondition), }, + { + name: "Returns ready condition with the summary of selected conditions with negative polarity (using WithConditions options)", + from: getterWithConditions(fooWithNegativePolarity, barWithNegativePolarity), + options: []MergeOption{WithConditions("foo-negative-polarity"), WithNegativePolarityConditions("foo-negative-polarity", "bar-negative-polarity")}, // bar-negative-polarity should be ignored because it is not listed in WithConditions + want: TrueCondition(clusterv1.ReadyCondition), + }, + { + name: "Returns ready condition with the summary of selected conditions with mixed polarity (using WithConditions options)", + from: getterWithConditions(foo, bar, fooWithNegativePolarity, barWithNegativePolarity), + options: []MergeOption{WithConditions("foo", "foo-negative-polarity", "bar-negative-polarity"), WithNegativePolarityConditions("foo-negative-polarity", "bar-negative-polarity")}, + want: FalseCondition(clusterv1.ReadyCondition, "reason trueInfo1-negative-polarity", clusterv1.ConditionSeverityInfo, "message trueInfo1-negative-polarity"), + }, { name: "Returns ready condition with the summary of selected conditions (using WithConditions and WithStepCounter options)", from: getterWithConditions(foo, bar, baz), diff --git a/util/conditions/merge.go b/util/conditions/merge.go index d2938c06ddbb..465606d710be 100644 --- a/util/conditions/merge.go +++ b/util/conditions/merge.go @@ -24,10 +24,21 @@ import ( clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" ) +type conditionPolarity string + +const ( + // PositivePolarity describe a condition with positive polarity (Status=True good). + PositivePolarity conditionPolarity = "Positive" + + // NegativePolarity describe a condition with negative polarity (Status=False good). + NegativePolarity conditionPolarity = "Negative" +) + // localizedCondition defines a condition with the information of the object the conditions // was originated from. type localizedCondition struct { *clusterv1.Condition + Polarity conditionPolarity Getter } @@ -39,10 +50,10 @@ type localizedCondition struct { // More specifically: // 1. Conditions are grouped by status, severity // 2. The resulting condition groups are sorted according to the following priority: -// - P0 - Status=False, Severity=Error -// - P1 - Status=False, Severity=Warning -// - P2 - Status=False, Severity=Info -// - P3 - Status=True +// - P0 - Status=False - PositivePolarity | Status=True - NegativePolarity, Severity=Error +// - P1 - Status=False - PositivePolarity | Status=True - NegativePolarity, Severity=Warning +// - P2 - Status=False - PositivePolarity | Status=True - NegativePolarity, Severity=Info +// - P3 - Status=True - PositivePolarity | Status=False - NegativePolarity // - P4 - Status=Unknown // // 3. The group with highest priority is used to determine status, severity and other info of the target condition. @@ -51,6 +62,7 @@ type localizedCondition struct { // condition; in order to complete such task some trade-off should be made, because there is no a golden rule // for summarizing many Reason/Message into single Reason/Message. // mergeOptions allows the user to adapt this process to the specific needs by exposing a set of merge strategies. +// NOTE: Target condition will have positive polarity. func merge(conditions []localizedCondition, targetCondition clusterv1.ConditionType, options *mergeOptions) *clusterv1.Condition { g := getConditionGroups(conditions) if len(g) == 0 { @@ -80,8 +92,23 @@ func getConditionGroups(conditions []localizedCondition) conditionGroups { } added := false + + // Identify the groupStatus the condition belongs to. + // NOTE: status for the conditions with negative polarity is "negated" so it is possible + // to merge conditions with different polarity (conditionGroup is always using positive polarity). + groupStatus := condition.Status + if condition.Polarity == NegativePolarity { + switch groupStatus { + case corev1.ConditionFalse: + groupStatus = corev1.ConditionTrue + case corev1.ConditionTrue: + groupStatus = corev1.ConditionFalse + case corev1.ConditionUnknown: + groupStatus = corev1.ConditionUnknown + } + } for i := range groups { - if groups[i].status == condition.Status && groups[i].severity == condition.Severity { + if groups[i].status == groupStatus && groups[i].severity == condition.Severity { groups[i].conditions = append(groups[i].conditions, condition) added = true break @@ -90,7 +117,7 @@ func getConditionGroups(conditions []localizedCondition) conditionGroups { if !added { groups = append(groups, conditionGroup{ conditions: []localizedCondition{condition}, - status: condition.Status, + status: groupStatus, severity: condition.Severity, }) } @@ -141,16 +168,19 @@ func (g conditionGroups) TopGroup() *conditionGroup { } // TrueGroup returns the condition group with status True, if any. +// Note: conditionGroup is always using positive polarity; the conditions in the group might have positive or negative polarity. func (g conditionGroups) TrueGroup() *conditionGroup { return g.getByStatusAndSeverity(corev1.ConditionTrue, clusterv1.ConditionSeverityNone) } // ErrorGroup returns the condition group with status False and severity Error, if any. +// Note: conditionGroup is always using positive polarity; the conditions in the group might have positive or negative polarity. func (g conditionGroups) ErrorGroup() *conditionGroup { return g.getByStatusAndSeverity(corev1.ConditionFalse, clusterv1.ConditionSeverityError) } // WarningGroup returns the condition group with status False and severity Warning, if any. +// Note: conditionGroup is always using positive polarity; the conditions in the group might have positive or negative polarity. func (g conditionGroups) WarningGroup() *conditionGroup { return g.getByStatusAndSeverity(corev1.ConditionFalse, clusterv1.ConditionSeverityWarning) } @@ -169,6 +199,7 @@ func (g conditionGroups) getByStatusAndSeverity(status corev1.ConditionStatus, s // conditionGroup define a group of conditions with the same status and severity, // and thus with the same priority when merging into a Ready condition. +// Note: conditionGroup is always using positive polarity; the conditions in the group might have positive or negative polarity. type conditionGroup struct { status corev1.ConditionStatus severity clusterv1.ConditionSeverity @@ -177,6 +208,7 @@ type conditionGroup struct { // mergePriority provides a priority value for the status and severity tuple that identifies this // condition group. The mergePriority value allows an easier sorting of conditions groups. +// Note: conditionGroup is always using positive polarity. func (g conditionGroup) mergePriority() int { switch g.status { case corev1.ConditionFalse: diff --git a/util/conditions/merge_strategies.go b/util/conditions/merge_strategies.go index 6e582bf10d0e..cda31159f604 100644 --- a/util/conditions/merge_strategies.go +++ b/util/conditions/merge_strategies.go @@ -27,6 +27,7 @@ import ( // and more specifically for computing the target Reason and the target Message. type mergeOptions struct { conditionTypes []clusterv1.ConditionType + negativeConditionTypes []clusterv1.ConditionType addSourceRef bool addStepCounter bool addStepCounterIfOnlyConditionTypes []clusterv1.ConditionType @@ -50,6 +51,14 @@ func WithConditions(t ...clusterv1.ConditionType) MergeOption { } } +// WithNegativePolarityConditions instruct merge about which conditions should be considered having negative polarity. +// IMPORTANT: this must be a subset of WithConditions. +func WithNegativePolarityConditions(t ...clusterv1.ConditionType) MergeOption { + return func(c *mergeOptions) { + c.negativeConditionTypes = t + } +} + // WithStepCounter instructs merge to add a "x of y completed" string to the message, // where x is the number of conditions with Status=true and y is the number of conditions in scope. func WithStepCounter() MergeOption { diff --git a/util/conditions/merge_test.go b/util/conditions/merge_test.go index 8d8305190744..36ebecaac614 100644 --- a/util/conditions/merge_test.go +++ b/util/conditions/merge_test.go @@ -26,61 +26,179 @@ import ( ) func TestNewConditionsGroup(t *testing.T) { - g := NewWithT(t) + t.Run("Positive polarity", func(t *testing.T) { + g := NewWithT(t) - conditions := []*clusterv1.Condition{nil1, true1, true1, falseInfo1, falseWarning1, falseWarning1, falseError1, unknown1} + conditions := []*clusterv1.Condition{nil1, true1, true1, falseInfo1, falseWarning1, falseWarning1, falseError1, unknown1} - got := getConditionGroups(conditionsWithSource(&clusterv1.Cluster{}, conditions...)) + got := getConditionGroups(conditionsWithSource(&clusterv1.Cluster{}, conditions...)) - g.Expect(got).ToNot(BeNil()) - g.Expect(got).To(HaveLen(5)) + g.Expect(got).ToNot(BeNil()) + g.Expect(got).To(HaveLen(5)) - // The top group should be False/Error and it should have one condition - g.Expect(got.TopGroup().status).To(Equal(corev1.ConditionFalse)) - g.Expect(got.TopGroup().severity).To(Equal(clusterv1.ConditionSeverityError)) - g.Expect(got.TopGroup().conditions).To(HaveLen(1)) + // The top group should be False/Error and it should have one condition + g.Expect(got.TopGroup().status).To(Equal(corev1.ConditionFalse)) + g.Expect(got.TopGroup().severity).To(Equal(clusterv1.ConditionSeverityError)) + g.Expect(got.TopGroup().conditions).To(HaveLen(1)) - // The true group should be true and it should have two conditions - g.Expect(got.TrueGroup().status).To(Equal(corev1.ConditionTrue)) - g.Expect(got.TrueGroup().severity).To(Equal(clusterv1.ConditionSeverityNone)) - g.Expect(got.TrueGroup().conditions).To(HaveLen(2)) + // The true group should be true and it should have two conditions + g.Expect(got.TrueGroup().status).To(Equal(corev1.ConditionTrue)) + g.Expect(got.TrueGroup().severity).To(Equal(clusterv1.ConditionSeverityNone)) + g.Expect(got.TrueGroup().conditions).To(HaveLen(2)) - // The error group should be False/Error and it should have one condition - g.Expect(got.ErrorGroup().status).To(Equal(corev1.ConditionFalse)) - g.Expect(got.ErrorGroup().severity).To(Equal(clusterv1.ConditionSeverityError)) - g.Expect(got.ErrorGroup().conditions).To(HaveLen(1)) + // The error group should be False/Error and it should have one condition + g.Expect(got.ErrorGroup().status).To(Equal(corev1.ConditionFalse)) + g.Expect(got.ErrorGroup().severity).To(Equal(clusterv1.ConditionSeverityError)) + g.Expect(got.ErrorGroup().conditions).To(HaveLen(1)) - // The warning group should be False/Warning and it should have two conditions - g.Expect(got.WarningGroup().status).To(Equal(corev1.ConditionFalse)) - g.Expect(got.WarningGroup().severity).To(Equal(clusterv1.ConditionSeverityWarning)) - g.Expect(got.WarningGroup().conditions).To(HaveLen(2)) + // The warning group should be False/Warning and it should have two conditions + g.Expect(got.WarningGroup().status).To(Equal(corev1.ConditionFalse)) + g.Expect(got.WarningGroup().severity).To(Equal(clusterv1.ConditionSeverityWarning)) + g.Expect(got.WarningGroup().conditions).To(HaveLen(2)) - // got[0] should be False/Error and it should have one condition - g.Expect(got[0].status).To(Equal(corev1.ConditionFalse)) - g.Expect(got[0].severity).To(Equal(clusterv1.ConditionSeverityError)) - g.Expect(got[0].conditions).To(HaveLen(1)) + // got[0] should be False/Error and it should have one condition + g.Expect(got[0].status).To(Equal(corev1.ConditionFalse)) + g.Expect(got[0].severity).To(Equal(clusterv1.ConditionSeverityError)) + g.Expect(got[0].conditions).To(HaveLen(1)) - // got[1] should be False/Warning and it should have two conditions - g.Expect(got[1].status).To(Equal(corev1.ConditionFalse)) - g.Expect(got[1].severity).To(Equal(clusterv1.ConditionSeverityWarning)) - g.Expect(got[1].conditions).To(HaveLen(2)) + // got[1] should be False/Warning and it should have two conditions + g.Expect(got[1].status).To(Equal(corev1.ConditionFalse)) + g.Expect(got[1].severity).To(Equal(clusterv1.ConditionSeverityWarning)) + g.Expect(got[1].conditions).To(HaveLen(2)) - // got[2] should be False/Info and it should have one condition - g.Expect(got[2].status).To(Equal(corev1.ConditionFalse)) - g.Expect(got[2].severity).To(Equal(clusterv1.ConditionSeverityInfo)) - g.Expect(got[2].conditions).To(HaveLen(1)) + // got[2] should be False/Info and it should have one condition + g.Expect(got[2].status).To(Equal(corev1.ConditionFalse)) + g.Expect(got[2].severity).To(Equal(clusterv1.ConditionSeverityInfo)) + g.Expect(got[2].conditions).To(HaveLen(1)) - // got[3] should be True and it should have two conditions - g.Expect(got[3].status).To(Equal(corev1.ConditionTrue)) - g.Expect(got[3].severity).To(Equal(clusterv1.ConditionSeverityNone)) - g.Expect(got[3].conditions).To(HaveLen(2)) + // got[3] should be True and it should have two conditions + g.Expect(got[3].status).To(Equal(corev1.ConditionTrue)) + g.Expect(got[3].severity).To(Equal(clusterv1.ConditionSeverityNone)) + g.Expect(got[3].conditions).To(HaveLen(2)) - // got[4] should be Unknown and it should have one condition - g.Expect(got[4].status).To(Equal(corev1.ConditionUnknown)) - g.Expect(got[4].severity).To(Equal(clusterv1.ConditionSeverityNone)) - g.Expect(got[4].conditions).To(HaveLen(1)) + // got[4] should be Unknown and it should have one condition + g.Expect(got[4].status).To(Equal(corev1.ConditionUnknown)) + g.Expect(got[4].severity).To(Equal(clusterv1.ConditionSeverityNone)) + g.Expect(got[4].conditions).To(HaveLen(1)) - // nil conditions are ignored + // nil conditions are ignored + }) + t.Run("Negative polarity", func(t *testing.T) { + g := NewWithT(t) + + conditions := []*clusterv1.Condition{nil1, false1WithNegativePolarity, false1WithNegativePolarity, trueInfo1WithNegativePolarity, trueWarning1WithNegativePolarity, trueWarning1WithNegativePolarity, trueError1WithNegativePolarity, unknown1WithNegativePolarity} + + got := getConditionGroups(conditionsWithSource(&clusterv1.Cluster{}, conditions...)) + + // NOTE: groups always have a positive polarity + + g.Expect(got).ToNot(BeNil()) + g.Expect(got).To(HaveLen(5)) + + // The top group should be False/Error and it should have one condition + g.Expect(got.TopGroup().status).To(Equal(corev1.ConditionFalse)) + g.Expect(got.TopGroup().severity).To(Equal(clusterv1.ConditionSeverityError)) + g.Expect(got.TopGroup().conditions).To(HaveLen(1)) + + // The true group should be true and it should have two conditions + g.Expect(got.TrueGroup().status).To(Equal(corev1.ConditionTrue)) + g.Expect(got.TrueGroup().severity).To(Equal(clusterv1.ConditionSeverityNone)) + g.Expect(got.TrueGroup().conditions).To(HaveLen(2)) + + // The error group should be False/Error and it should have one condition + g.Expect(got.ErrorGroup().status).To(Equal(corev1.ConditionFalse)) + g.Expect(got.ErrorGroup().severity).To(Equal(clusterv1.ConditionSeverityError)) + g.Expect(got.ErrorGroup().conditions).To(HaveLen(1)) + + // The warning group should be False/Warning and it should have two conditions + g.Expect(got.WarningGroup().status).To(Equal(corev1.ConditionFalse)) + g.Expect(got.WarningGroup().severity).To(Equal(clusterv1.ConditionSeverityWarning)) + g.Expect(got.WarningGroup().conditions).To(HaveLen(2)) + + // got[0] should be False/Error and it should have one condition + g.Expect(got[0].status).To(Equal(corev1.ConditionFalse)) + g.Expect(got[0].severity).To(Equal(clusterv1.ConditionSeverityError)) + g.Expect(got[0].conditions).To(HaveLen(1)) + + // got[1] should be False/Warning and it should have two conditions + g.Expect(got[1].status).To(Equal(corev1.ConditionFalse)) + g.Expect(got[1].severity).To(Equal(clusterv1.ConditionSeverityWarning)) + g.Expect(got[1].conditions).To(HaveLen(2)) + + // got[2] should be False/Info and it should have one condition + g.Expect(got[2].status).To(Equal(corev1.ConditionFalse)) + g.Expect(got[2].severity).To(Equal(clusterv1.ConditionSeverityInfo)) + g.Expect(got[2].conditions).To(HaveLen(1)) + + // got[3] should be True and it should have two conditions + g.Expect(got[3].status).To(Equal(corev1.ConditionTrue)) + g.Expect(got[3].severity).To(Equal(clusterv1.ConditionSeverityNone)) + g.Expect(got[3].conditions).To(HaveLen(2)) + + // got[4] should be Unknown and it should have one condition + g.Expect(got[4].status).To(Equal(corev1.ConditionUnknown)) + g.Expect(got[4].severity).To(Equal(clusterv1.ConditionSeverityNone)) + g.Expect(got[4].conditions).To(HaveLen(1)) + + // nil conditions are ignored + }) + t.Run("Mixed polarity", func(t *testing.T) { + g := NewWithT(t) + + conditions := []*clusterv1.Condition{nil1, true1, true1, falseInfo1, falseWarning1, falseWarning1, falseError1, unknown1, false1WithNegativePolarity, false1WithNegativePolarity, trueInfo1WithNegativePolarity, trueWarning1WithNegativePolarity, trueWarning1WithNegativePolarity, trueError1WithNegativePolarity, unknown1WithNegativePolarity} + + got := getConditionGroups(conditionsWithSource(&clusterv1.Cluster{}, conditions...)) + + g.Expect(got).ToNot(BeNil()) + g.Expect(got).To(HaveLen(5)) + + // The top group should be False/Error and it should have two condition + g.Expect(got.TopGroup().status).To(Equal(corev1.ConditionFalse)) + g.Expect(got.TopGroup().severity).To(Equal(clusterv1.ConditionSeverityError)) + g.Expect(got.TopGroup().conditions).To(HaveLen(2)) + + // The true group should be true and it should have four conditions + g.Expect(got.TrueGroup().status).To(Equal(corev1.ConditionTrue)) + g.Expect(got.TrueGroup().severity).To(Equal(clusterv1.ConditionSeverityNone)) + g.Expect(got.TrueGroup().conditions).To(HaveLen(4)) + + // The error group should be False/Error and it should have two condition + g.Expect(got.ErrorGroup().status).To(Equal(corev1.ConditionFalse)) + g.Expect(got.ErrorGroup().severity).To(Equal(clusterv1.ConditionSeverityError)) + g.Expect(got.ErrorGroup().conditions).To(HaveLen(2)) + + // The warning group should be False/Warning and it should have four conditions + g.Expect(got.WarningGroup().status).To(Equal(corev1.ConditionFalse)) + g.Expect(got.WarningGroup().severity).To(Equal(clusterv1.ConditionSeverityWarning)) + g.Expect(got.WarningGroup().conditions).To(HaveLen(4)) + + // got[0] should be False/Error and it should have two condition + g.Expect(got[0].status).To(Equal(corev1.ConditionFalse)) + g.Expect(got[0].severity).To(Equal(clusterv1.ConditionSeverityError)) + g.Expect(got[0].conditions).To(HaveLen(2)) + + // got[1] should be False/Warning and it should have four conditions + g.Expect(got[1].status).To(Equal(corev1.ConditionFalse)) + g.Expect(got[1].severity).To(Equal(clusterv1.ConditionSeverityWarning)) + g.Expect(got[1].conditions).To(HaveLen(4)) + + // got[2] should be False/Info and it should have two condition + g.Expect(got[2].status).To(Equal(corev1.ConditionFalse)) + g.Expect(got[2].severity).To(Equal(clusterv1.ConditionSeverityInfo)) + g.Expect(got[2].conditions).To(HaveLen(2)) + + // got[3] should be True and it should have four conditions + g.Expect(got[3].status).To(Equal(corev1.ConditionTrue)) + g.Expect(got[3].severity).To(Equal(clusterv1.ConditionSeverityNone)) + g.Expect(got[3].conditions).To(HaveLen(4)) + + // got[4] should be Unknown and it should have two condition + g.Expect(got[4].status).To(Equal(corev1.ConditionUnknown)) + g.Expect(got[4].severity).To(Equal(clusterv1.ConditionSeverityNone)) + g.Expect(got[4].conditions).To(HaveLen(2)) + + // nil conditions are ignored + }) } func TestMergeRespectPriority(t *testing.T) { @@ -151,8 +269,14 @@ func conditionsWithSource(obj Setter, conditions ...*clusterv1.Condition) []loca ret := []localizedCondition{} for i := range conditions { + polarity := PositivePolarity + if conditions[i] != nil && negativePolarityConditions.Has(string(conditions[i].Type)) { + polarity = NegativePolarity + } + ret = append(ret, localizedCondition{ Condition: conditions[i], + Polarity: polarity, Getter: obj, }) } diff --git a/util/conditions/setter.go b/util/conditions/setter.go index c882b054d062..c387f0fc410c 100644 --- a/util/conditions/setter.go +++ b/util/conditions/setter.go @@ -85,6 +85,17 @@ func TrueCondition(t clusterv1.ConditionType) *clusterv1.Condition { } } +// TrueConditionWithNegativePolarity returns a condition with negative polarity, Status=True and the given type (Status=True has a negative meaning). +func TrueConditionWithNegativePolarity(t clusterv1.ConditionType, reason string, severity clusterv1.ConditionSeverity, messageFormat string, messageArgs ...interface{}) *clusterv1.Condition { + return &clusterv1.Condition{ + Type: t, + Status: corev1.ConditionTrue, + Reason: reason, + Severity: severity, + Message: fmt.Sprintf(messageFormat, messageArgs...), + } +} + // FalseCondition returns a condition with Status=False and the given type. func FalseCondition(t clusterv1.ConditionType, reason string, severity clusterv1.ConditionSeverity, messageFormat string, messageArgs ...interface{}) *clusterv1.Condition { return &clusterv1.Condition{ @@ -96,6 +107,14 @@ func FalseCondition(t clusterv1.ConditionType, reason string, severity clusterv1 } } +// FalseConditionWithNegativePolarity returns a condition with negative polarity, Status=false and the given type (Status=False has a positive meaning). +func FalseConditionWithNegativePolarity(t clusterv1.ConditionType) *clusterv1.Condition { + return &clusterv1.Condition{ + Type: t, + Status: corev1.ConditionFalse, + } +} + // UnknownCondition returns a condition with Status=Unknown and the given type. func UnknownCondition(t clusterv1.ConditionType, reason string, messageFormat string, messageArgs ...interface{}) *clusterv1.Condition { return &clusterv1.Condition{ @@ -111,6 +130,11 @@ func MarkTrue(to Setter, t clusterv1.ConditionType) { Set(to, TrueCondition(t)) } +// MarkTrueWithNegativePolarity sets Status=True for a condition with negative polarity and the given type (Status=True has a negative meaning). +func MarkTrueWithNegativePolarity(to Setter, t clusterv1.ConditionType, reason string, severity clusterv1.ConditionSeverity, messageFormat string, messageArgs ...interface{}) { + Set(to, TrueConditionWithNegativePolarity(t, reason, severity, messageFormat, messageArgs...)) +} + // MarkUnknown sets Status=Unknown for the condition with the given type. func MarkUnknown(to Setter, t clusterv1.ConditionType, reason, messageFormat string, messageArgs ...interface{}) { Set(to, UnknownCondition(t, reason, messageFormat, messageArgs...)) @@ -121,6 +145,11 @@ func MarkFalse(to Setter, t clusterv1.ConditionType, reason string, severity clu Set(to, FalseCondition(t, reason, severity, messageFormat, messageArgs...)) } +// MarkFalseWithNegativePolarity sets Status=False for a condition with negative polarity and the given type (Status=False has a positive meaning). +func MarkFalseWithNegativePolarity(to Setter, t clusterv1.ConditionType) { + Set(to, FalseConditionWithNegativePolarity(t)) +} + // SetSummary sets a Ready condition with the summary of all the conditions existing // on an object. If the object does not have other conditions, no summary condition is generated. func SetSummary(to Setter, options ...MergeOption) { diff --git a/util/conditions/setter_test.go b/util/conditions/setter_test.go index 60a7cba27c7e..57c3a770e629 100644 --- a/util/conditions/setter_test.go +++ b/util/conditions/setter_test.go @@ -223,6 +223,23 @@ func TestMarkMethods(t *testing.T) { Reason: "reasonBaz", Message: "messageBaz", })) + + // test MarkFalseWithNegativePolarity + MarkFalseWithNegativePolarity(cluster, "conditionFoo") + g.Expect(Get(cluster, "conditionFoo")).To(HaveSameStateOf(&clusterv1.Condition{ + Type: "conditionFoo", + Status: corev1.ConditionFalse, + })) + + // test MarkTrueWithNegativePolarity + MarkTrueWithNegativePolarity(cluster, "conditionBar", "reasonBar", clusterv1.ConditionSeverityError, "messageBar") + g.Expect(Get(cluster, "conditionBar")).To(HaveSameStateOf(&clusterv1.Condition{ + Type: "conditionBar", + Status: corev1.ConditionTrue, + Severity: clusterv1.ConditionSeverityError, + Reason: "reasonBar", + Message: "messageBar", + })) } func TestSetSummary(t *testing.T) {