From 4f1637e59c48528f7a5c1a3116054dbc96216f8c Mon Sep 17 00:00:00 2001 From: fabriziopandini Date: Tue, 11 Jun 2024 16:41:00 +0200 Subject: [PATCH] Rename helpers --- docs/proposals/20200506-conditions.md | 7 +-- util/conditions/getter_test.go | 64 +++++++++++++-------------- util/conditions/merge_strategies.go | 1 + util/conditions/merge_test.go | 6 ++- util/conditions/setter.go | 32 +++++++------- util/conditions/setter_test.go | 8 ++-- 6 files changed, 61 insertions(+), 57 deletions(-) diff --git a/docs/proposals/20200506-conditions.md b/docs/proposals/20200506-conditions.md index 507b39b4d77b..07f2f51f4060 100644 --- a/docs/proposals/20200506-conditions.md +++ b/docs/proposals/20200506-conditions.md @@ -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` + positive polarity or when `Status=True` + negative polarity; -severity 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: @@ -473,7 +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 allowing conditions with negative polarity goes into this direction + - 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 diff --git a/util/conditions/getter_test.go b/util/conditions/getter_test.go index 3ffd56cb5e44..7b3e0e807d1c 100644 --- a/util/conditions/getter_test.go +++ b/util/conditions/getter_test.go @@ -33,12 +33,12 @@ var ( falseWarning1 = FalseCondition("falseWarning1", "reason falseWarning1", clusterv1.ConditionSeverityWarning, "message falseWarning1") falseError1 = FalseCondition("falseError1", "reason falseError1", clusterv1.ConditionSeverityError, "message falseError1") - negativePolarityConditions = sets.New("positive-false1", "negative-unknown1", "negative-trueInfo1", "negative-trueWarning1", "negative-trueError1") - positiveFalse1 = PositiveFalseCondition("positive-false1") - negativeUnknown1 = UnknownCondition("negative-unknown1", "reason negative-unknown1", "message negative-unknown1") - negativeTrueInfo1 = NegativeTrueCondition("negative-trueInfo1", "reason negative-trueInfo1", clusterv1.ConditionSeverityInfo, "message negative-trueInfo1") - negativeTrueWarning1 = NegativeTrueCondition("negative-trueWarning1", "reason negative-trueWarning1", clusterv1.ConditionSeverityWarning, "message negative-trueWarning1") - negativeTrueError1 = NegativeTrueCondition("negative-trueError1", "reason negative-trueError1", clusterv1.ConditionSeverityError, "message negative-trueError1") + 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) { @@ -58,51 +58,51 @@ func TestGetAndHas(t *testing.T) { func TestIsMethods(t *testing.T) { g := NewWithT(t) - obj := getterWithConditions(nil1, true1, unknown1, falseInfo1, falseWarning1, falseError1, positiveFalse1, negativeUnknown1, negativeTrueInfo1, negativeTrueWarning1, negativeTrueError1) + 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, "positive-false1")).To(BeFalse()) - g.Expect(IsTrue(obj, "negative-trueInfo1")).To(BeTrue()) - g.Expect(IsTrue(obj, "negative-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, "positive-false1")).To(BeTrue()) - g.Expect(IsFalse(obj, "negative-trueInfo1")).To(BeFalse()) - g.Expect(IsFalse(obj, "negative-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, "positive-false1")).To(BeFalse()) - g.Expect(IsUnknown(obj, "negative-trueInfo1")).To(BeFalse()) - g.Expect(IsUnknown(obj, "negative-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, "negative-trueInfo1")).To(Equal("reason negative-trueInfo1")) + 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, "negative-trueInfo1")).To(Equal("message negative-trueInfo1")) + 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") g.Expect(severity).To(Equal(&expectedSeverity)) - severity = GetSeverity(obj, "negative-trueInfo1") + severity = GetSeverity(obj, "trueInfo1-negative-polarity") g.Expect(severity).To(Equal(&expectedSeverity)) // test GetLastTransitionTime @@ -153,8 +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") - negativeFoo := PositiveFalseCondition("negative-foo") - negativeBar := NegativeTrueCondition("negative-bar", "reason negative-falseInfo1", clusterv1.ConditionSeverityInfo, "message negative-falseInfo1") + 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 { @@ -175,15 +175,15 @@ func TestSummary(t *testing.T) { }, { name: "Returns ready condition with the summary of existing conditions with negative polarity (with default options)", - from: getterWithConditions(negativeFoo, negativeBar), - options: []MergeOption{WithNegativePolarityConditions("negative-foo", "negative-bar")}, - want: FalseCondition(clusterv1.ReadyCondition, "reason negative-falseInfo1", clusterv1.ConditionSeverityInfo, "message negative-falseInfo1"), + 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, negativeFoo, negativeBar), - options: []MergeOption{WithNegativePolarityConditions("negative-foo", "negative-bar")}, - want: FalseCondition(clusterv1.ReadyCondition, "reason falseInfo1", clusterv1.ConditionSeverityInfo, "message falseInfo1"), // bar take precedence on negativeBar because it is listed first + 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)", @@ -223,15 +223,15 @@ func TestSummary(t *testing.T) { }, { name: "Returns ready condition with the summary of selected conditions with negative polarity (using WithConditions options)", - from: getterWithConditions(negativeFoo, negativeBar), - options: []MergeOption{WithConditions("negative-foo"), WithNegativePolarityConditions("negative-foo", "negative-bar")}, // negative-bar should be ignored + 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, negativeFoo, negativeBar), - options: []MergeOption{WithConditions("foo", "negative-foo", "negative-bar"), WithNegativePolarityConditions("negative-foo", "negative-bar")}, // bar should be ignored - want: FalseCondition(clusterv1.ReadyCondition, "reason negative-falseInfo1", clusterv1.ConditionSeverityInfo, "message negative-falseInfo1"), + 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)", diff --git a/util/conditions/merge_strategies.go b/util/conditions/merge_strategies.go index c346435b9a25..cda31159f604 100644 --- a/util/conditions/merge_strategies.go +++ b/util/conditions/merge_strategies.go @@ -52,6 +52,7 @@ 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 diff --git a/util/conditions/merge_test.go b/util/conditions/merge_test.go index 31a216aad68a..36ebecaac614 100644 --- a/util/conditions/merge_test.go +++ b/util/conditions/merge_test.go @@ -86,10 +86,12 @@ func TestNewConditionsGroup(t *testing.T) { t.Run("Negative polarity", func(t *testing.T) { g := NewWithT(t) - conditions := []*clusterv1.Condition{nil1, positiveFalse1, positiveFalse1, negativeTrueInfo1, negativeTrueWarning1, negativeTrueWarning1, negativeTrueError1, negativeUnknown1} + 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)) @@ -143,7 +145,7 @@ func TestNewConditionsGroup(t *testing.T) { t.Run("Mixed polarity", func(t *testing.T) { g := NewWithT(t) - conditions := []*clusterv1.Condition{nil1, true1, true1, falseInfo1, falseWarning1, falseWarning1, falseError1, unknown1, positiveFalse1, positiveFalse1, negativeTrueInfo1, negativeTrueWarning1, negativeTrueWarning1, negativeTrueError1, negativeUnknown1} + conditions := []*clusterv1.Condition{nil1, true1, true1, falseInfo1, falseWarning1, falseWarning1, falseError1, unknown1, false1WithNegativePolarity, false1WithNegativePolarity, trueInfo1WithNegativePolarity, trueWarning1WithNegativePolarity, trueWarning1WithNegativePolarity, trueError1WithNegativePolarity, unknown1WithNegativePolarity} got := getConditionGroups(conditionsWithSource(&clusterv1.Cluster{}, conditions...)) diff --git a/util/conditions/setter.go b/util/conditions/setter.go index 83145cd62138..c387f0fc410c 100644 --- a/util/conditions/setter.go +++ b/util/conditions/setter.go @@ -85,30 +85,30 @@ func TrueCondition(t clusterv1.ConditionType) *clusterv1.Condition { } } -// 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 { +// 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.ConditionFalse, + Status: corev1.ConditionTrue, Reason: reason, Severity: severity, Message: fmt.Sprintf(messageFormat, messageArgs...), } } -// NegativeTrueCondition returns a negative polarity condition with Status=True and the given type (Status=True has a negative meaning). -func NegativeTrueCondition(t clusterv1.ConditionType, reason string, severity clusterv1.ConditionSeverity, messageFormat string, messageArgs ...interface{}) *clusterv1.Condition { +// 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{ Type: t, - Status: corev1.ConditionTrue, + Status: corev1.ConditionFalse, Reason: reason, Severity: severity, Message: fmt.Sprintf(messageFormat, messageArgs...), } } -// PositiveFalseCondition returns a negative polarity condition with Status=false and the given type (Status=False has a positive meaning). -func PositiveFalseCondition(t clusterv1.ConditionType) *clusterv1.Condition { +// 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, @@ -130,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...)) @@ -140,14 +145,9 @@ func MarkFalse(to Setter, t clusterv1.ConditionType, reason string, severity clu Set(to, FalseCondition(t, reason, severity, messageFormat, messageArgs...)) } -// MarkNegativeTrue sets Status=True for the negative polarity condition with the given type (Status=True has a negative meaning). -func MarkNegativeTrue(to Setter, t clusterv1.ConditionType, reason string, severity clusterv1.ConditionSeverity, messageFormat string, messageArgs ...interface{}) { - Set(to, NegativeTrueCondition(t, reason, severity, messageFormat, messageArgs...)) -} - -// MarkPositiveFalse sets Status=False for the negative polarity condition with the given type (Status=False has a positive meaning). -func MarkPositiveFalse(to Setter, t clusterv1.ConditionType) { - Set(to, PositiveFalseCondition(t)) +// 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 diff --git a/util/conditions/setter_test.go b/util/conditions/setter_test.go index 0bbc837e6642..57c3a770e629 100644 --- a/util/conditions/setter_test.go +++ b/util/conditions/setter_test.go @@ -224,15 +224,15 @@ func TestMarkMethods(t *testing.T) { Message: "messageBaz", })) - // test MarkPositiveFalse - MarkPositiveFalse(cluster, "conditionFoo") + // test MarkFalseWithNegativePolarity + MarkFalseWithNegativePolarity(cluster, "conditionFoo") g.Expect(Get(cluster, "conditionFoo")).To(HaveSameStateOf(&clusterv1.Condition{ Type: "conditionFoo", Status: corev1.ConditionFalse, })) - // test MarkNegativeTrue - MarkNegativeTrue(cluster, "conditionBar", "reasonBar", clusterv1.ConditionSeverityError, "messageBar") + // test MarkTrueWithNegativePolarity + MarkTrueWithNegativePolarity(cluster, "conditionBar", "reasonBar", clusterv1.ConditionSeverityError, "messageBar") g.Expect(Get(cluster, "conditionBar")).To(HaveSameStateOf(&clusterv1.Condition{ Type: "conditionBar", Status: corev1.ConditionTrue,