diff --git a/apis/duck/v1alpha1/condition_set.go b/apis/duck/v1alpha1/condition_set.go index 30b76a5f66..e330970bd0 100644 --- a/apis/duck/v1alpha1/condition_set.go +++ b/apis/duck/v1alpha1/condition_set.go @@ -62,7 +62,7 @@ type ConditionManager interface { SetCondition(new Condition) // MarkTrue sets the status of t to true, and then marks the happy condition to - // true if all other dependents are also true. + // true if all dependents are true. MarkTrue(t ConditionType) // MarkUnknown sets the status of t to Unknown and also sets the happy condition @@ -82,12 +82,14 @@ type ConditionManager interface { // NewLivingConditionSet returns a ConditionSet to hold the conditions for the // living resource. ConditionReady is used as the happy condition. +// The set of condition types provided are those of the terminal subconditions. func NewLivingConditionSet(d ...ConditionType) ConditionSet { return newConditionSet(ConditionReady, d...) } // NewBatchConditionSet returns a ConditionSet to hold the conditions for the // batch resource. ConditionSucceeded is used as the happy condition. +// The set of condition types provided are those of the terminal subconditions. func NewBatchConditionSet(d ...ConditionType) ConditionSet { return newConditionSet(ConditionSucceeded, d...) } @@ -209,13 +211,30 @@ func (r conditionsImpl) SetCondition(new Condition) { r.accessor.SetConditions(conditions) } +func (r conditionsImpl) isTerminal(t ConditionType) bool { + for _, cond := range append(r.dependents, r.happy) { + if cond == t { + return true + } + } + return false +} + +func (r conditionsImpl) severity(t ConditionType) ConditionSeverity { + if r.isTerminal(t) { + return ConditionSeverityError + } + return ConditionSeverityInfo +} + // MarkTrue sets the status of t to true, and then marks the happy condition to // true if all other dependents are also true. func (r conditionsImpl) MarkTrue(t ConditionType) { // set the specified condition r.SetCondition(Condition{ - Type: t, - Status: corev1.ConditionTrue, + Type: t, + Status: corev1.ConditionTrue, + Severity: r.severity(t), }) // check the dependents. @@ -229,8 +248,9 @@ func (r conditionsImpl) MarkTrue(t ConditionType) { // set the happy condition r.SetCondition(Condition{ - Type: r.happy, - Status: corev1.ConditionTrue, + Type: r.happy, + Status: corev1.ConditionTrue, + Severity: r.severity(r.happy), }) } @@ -239,13 +259,15 @@ func (r conditionsImpl) MarkTrue(t ConditionType) { func (r conditionsImpl) MarkUnknown(t ConditionType, reason, messageFormat string, messageA ...interface{}) { // set the specified condition r.SetCondition(Condition{ - Type: t, - Status: corev1.ConditionUnknown, - Reason: reason, - Message: fmt.Sprintf(messageFormat, messageA...), + Type: t, + Status: corev1.ConditionUnknown, + Reason: reason, + Message: fmt.Sprintf(messageFormat, messageA...), + Severity: r.severity(t), }) // check the dependents. + isDependent := false for _, cond := range r.dependents { c := r.GetCondition(cond) // Failed conditions trump Unknown conditions @@ -257,28 +279,39 @@ func (r conditionsImpl) MarkUnknown(t ConditionType, reason, messageFormat strin } return } + if cond == t { + isDependent = true + } } - // set the happy condition - r.SetCondition(Condition{ - Type: r.happy, - Status: corev1.ConditionUnknown, - Reason: reason, - Message: fmt.Sprintf(messageFormat, messageA...), - }) + if isDependent { + // set the happy condition, if it is one of our dependent subconditions. + r.SetCondition(Condition{ + Type: r.happy, + Status: corev1.ConditionUnknown, + Reason: reason, + Message: fmt.Sprintf(messageFormat, messageA...), + Severity: r.severity(r.happy), + }) + } } // MarkFalse sets the status of t and the happy condition to False. func (r conditionsImpl) MarkFalse(t ConditionType, reason, messageFormat string, messageA ...interface{}) { - for _, t := range []ConditionType{ - t, - r.happy, - } { + types := []ConditionType{t} + for _, cond := range r.dependents { + if cond == t { + types = append(types, r.happy) + } + } + + for _, t := range types { r.SetCondition(Condition{ - Type: t, - Status: corev1.ConditionFalse, - Reason: reason, - Message: fmt.Sprintf(messageFormat, messageA...), + Type: t, + Status: corev1.ConditionFalse, + Reason: reason, + Message: fmt.Sprintf(messageFormat, messageA...), + Severity: r.severity(t), }) } } @@ -295,8 +328,9 @@ func (r conditionsImpl) InitializeConditions() { func (r conditionsImpl) InitializeCondition(t ConditionType) { if c := r.GetCondition(t); c == nil { r.SetCondition(Condition{ - Type: t, - Status: corev1.ConditionUnknown, + Type: t, + Status: corev1.ConditionUnknown, + Severity: r.severity(t), }) } } diff --git a/apis/duck/v1alpha1/condition_set_impl_test.go b/apis/duck/v1alpha1/condition_set_impl_test.go index 5dcb8a3008..735f3429ca 100644 --- a/apis/duck/v1alpha1/condition_set_impl_test.go +++ b/apis/duck/v1alpha1/condition_set_impl_test.go @@ -51,6 +51,10 @@ type TestFatFingers struct { Conditionals Conditions } +var ( + ignoreFields = cmpopts.IgnoreFields(Condition{}, "LastTransitionTime", "Severity") +) + func TestGetCondition(t *testing.T) { condSet := NewLivingConditionSet() cases := []struct { @@ -86,8 +90,7 @@ func TestGetCondition(t *testing.T) { for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { e, a := tc.expect, condSet.Manage(tc.status).GetCondition(tc.get) - ignoreArguments := cmpopts.IgnoreFields(Condition{}, "LastTransitionTime") - if diff := cmp.Diff(e, a, ignoreArguments); diff != "" { + if diff := cmp.Diff(e, a, ignoreFields); diff != "" { t.Errorf("%s (-want, +got) = %v", tc.name, diff) } }) @@ -129,8 +132,7 @@ func TestGetConditionReflection(t *testing.T) { for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { e, a := tc.expect, condSet.Manage(tc.status).GetCondition(tc.get) - ignoreArguments := cmpopts.IgnoreFields(Condition{}, "LastTransitionTime") - if diff := cmp.Diff(e, a, ignoreArguments); diff != "" { + if diff := cmp.Diff(e, a, ignoreFields); diff != "" { t.Errorf("%s (-want, +got) = %v", tc.name, diff) } }) @@ -169,8 +171,7 @@ func TestGetConditionFatFingers(t *testing.T) { for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { e, a := tc.expect, condSet.Manage(tc.status).GetCondition(tc.get) - ignoreArguments := cmpopts.IgnoreFields(Condition{}, "LastTransitionTime") - if diff := cmp.Diff(e, a, ignoreArguments); diff != "" { + if diff := cmp.Diff(e, a, ignoreFields); diff != "" { t.Errorf("%s (-want, +got) = %v", tc.name, diff) } }) @@ -222,8 +223,7 @@ func TestSetCondition(t *testing.T) { t.Run(tc.name, func(t *testing.T) { condSet.Manage(tc.status).SetCondition(tc.set) e, a := tc.expect, condSet.Manage(tc.status).GetCondition(tc.set.Type) - ignoreArguments := cmpopts.IgnoreFields(Condition{}, "LastTransitionTime") - if diff := cmp.Diff(e, a, ignoreArguments); diff != "" { + if diff := cmp.Diff(e, a, ignoreFields); diff != "" { t.Errorf("%s (-want, +got) = %v", tc.name, diff) } }) @@ -275,8 +275,7 @@ func TestSetConditionReflection(t *testing.T) { t.Run(tc.name, func(t *testing.T) { condSet.Manage(tc.status).SetCondition(tc.set) e, a := tc.expect, condSet.Manage(tc.status).GetCondition(tc.set.Type) - ignoreArguments := cmpopts.IgnoreFields(Condition{}, "LastTransitionTime") - if diff := cmp.Diff(e, a, ignoreArguments); diff != "" { + if diff := cmp.Diff(e, a, ignoreFields); diff != "" { t.Errorf("%s (-want, +got) = %v", tc.name, diff) } }) @@ -341,8 +340,7 @@ func TestSetConditionFatFingers(t *testing.T) { t.Run(tc.name, func(t *testing.T) { condSet.Manage(tc.status).SetCondition(tc.set) e, a := tc.expect, condSet.Manage(tc.status).GetCondition(tc.set.Type) - ignoreArguments := cmpopts.IgnoreFields(Condition{}, "LastTransitionTime") - if diff := cmp.Diff(e, a, ignoreArguments); diff != "" { + if diff := cmp.Diff(e, a, ignoreFields); diff != "" { t.Errorf("%s (-want, +got) = %v", tc.name, diff) } }) @@ -770,6 +768,36 @@ func TestResourceConditions(t *testing.T) { } } +func TestConditionSeverity(t *testing.T) { + condSet := NewLivingConditionSet("Foo") + status := &TestStatusReflection{} + + // Add a new condition. + condSet.Manage(status).InitializeConditions() + + if got, want := len(status.Conditions), 2; got != want { + t.Errorf("Unexpected number of conditions: %d, wanted %d", got, want) + } + + condSet.Manage(status).MarkFalse("Bar", "", "") + + if got, want := len(status.Conditions), 3; got != want { + t.Errorf("Unexpected number of conditions: %d, wanted %d", got, want) + } + + if got, want := condSet.Manage(status).GetCondition("Ready").Severity, ConditionSeverityError; got != want { + t.Errorf("GetCondition(%q).Severity = %v, wanted %v", "Ready", got, want) + } + + if got, want := condSet.Manage(status).GetCondition("Foo").Severity, ConditionSeverityError; got != want { + t.Errorf("GetCondition(%q).Severity = %v, wanted %v", "Foo", got, want) + } + + if got, want := condSet.Manage(status).GetCondition("Bar").Severity, ConditionSeverityInfo; got != want { + t.Errorf("GetCondition(%q).Severity = %v, wanted %v", "Bar", got, want) + } +} + // getTypes is a small helped to strip out the used ConditionTypes from Conditions func getTypes(conds Conditions) []ConditionType { var types []ConditionType @@ -805,8 +833,7 @@ func doTestMarkTrueAccessor(t *testing.T, cases []ConditionMarkTrueTest) { } e, a := expected, condSet.Manage(status).GetCondition(tc.mark) - ignoreArguments := cmpopts.IgnoreFields(Condition{}, "LastTransitionTime") - if diff := cmp.Diff(e, a, ignoreArguments); diff != "" { + if diff := cmp.Diff(e, a, ignoreFields); diff != "" { t.Errorf("%s (-want, +got) = %v", tc.name, diff) } }) @@ -832,8 +859,7 @@ func doTestMarkTrueReflection(t *testing.T, cases []ConditionMarkTrueTest) { } e, a := expected, condSet.Manage(status).GetCondition(tc.mark) - ignoreArguments := cmpopts.IgnoreFields(Condition{}, "LastTransitionTime") - if diff := cmp.Diff(e, a, ignoreArguments); diff != "" { + if diff := cmp.Diff(e, a, ignoreFields); diff != "" { t.Errorf("%s (-want, +got) = %v", tc.name, diff) } }) @@ -945,8 +971,7 @@ func doTestMarkFalseAccessor(t *testing.T, cases []ConditionMarkFalseTest) { } e, a := expected, condSet.Manage(status).GetCondition(tc.mark) - ignoreArguments := cmpopts.IgnoreFields(Condition{}, "LastTransitionTime") - if diff := cmp.Diff(e, a, ignoreArguments); diff != "" { + if diff := cmp.Diff(e, a, ignoreFields); diff != "" { t.Errorf("%s (-want, +got) = %v", tc.name, diff) } }) @@ -975,8 +1000,7 @@ func doTestMarkFalseReflection(t *testing.T, cases []ConditionMarkFalseTest) { } e, a := expected, condSet.Manage(status).GetCondition(tc.mark) - ignoreArguments := cmpopts.IgnoreFields(Condition{}, "LastTransitionTime") - if diff := cmp.Diff(e, a, ignoreArguments); diff != "" { + if diff := cmp.Diff(e, a, ignoreFields); diff != "" { t.Errorf("%s (-want, +got) = %v", tc.name, diff) } }) @@ -1093,8 +1117,7 @@ func doTestMarkUnknownAccessor(t *testing.T, cases []ConditionMarkUnknownTest) { } e, a := expected, condSet.Manage(status).GetCondition(tc.mark) - ignoreArguments := cmpopts.IgnoreFields(Condition{}, "LastTransitionTime") - if diff := cmp.Diff(e, a, ignoreArguments); diff != "" { + if diff := cmp.Diff(e, a, ignoreFields); diff != "" { t.Errorf("%s (-want, +got) = %v", tc.name, diff) } }) @@ -1127,8 +1150,7 @@ func doTestMarkUnknownReflection(t *testing.T, cases []ConditionMarkUnknownTest) } e, a := expected, condSet.Manage(status).GetCondition(tc.mark) - ignoreArguments := cmpopts.IgnoreFields(Condition{}, "LastTransitionTime") - if diff := cmp.Diff(e, a, ignoreArguments); diff != "" { + if diff := cmp.Diff(e, a, ignoreFields); diff != "" { t.Errorf("%s (-want, +got) = %v", tc.name, diff) } }) @@ -1231,18 +1253,21 @@ func TestInitializeConditions(t *testing.T) { }{{ name: "initialized", condition: &Condition{ - Type: ConditionReady, - Status: corev1.ConditionUnknown, + Type: ConditionReady, + Status: corev1.ConditionUnknown, + Severity: ConditionSeverityError, }, }, { name: "already initialized", conditions: Conditions{{ - Type: ConditionReady, - Status: corev1.ConditionFalse, + Type: ConditionReady, + Status: corev1.ConditionFalse, + Severity: ConditionSeverityError, }}, condition: &Condition{ - Type: ConditionReady, - Status: corev1.ConditionFalse, + Type: ConditionReady, + Status: corev1.ConditionFalse, + Severity: ConditionSeverityError, }, }} diff --git a/apis/duck/v1alpha1/condition_set_test.go b/apis/duck/v1alpha1/condition_set_test.go index caa324c5f2..2bf1c404d1 100644 --- a/apis/duck/v1alpha1/condition_set_test.go +++ b/apis/duck/v1alpha1/condition_set_test.go @@ -18,6 +18,8 @@ package v1alpha1 import ( "testing" + + corev1 "k8s.io/api/core/v1" ) func TestNewLivingConditionSet(t *testing.T) { @@ -85,3 +87,33 @@ func TestNewBatchConditionSet(t *testing.T) { }) } } + +func TestNonTerminalCondition(t *testing.T) { + set := NewLivingConditionSet("Foo") + status := &KResourceStatus{} + + manager := set.Manage(status) + manager.InitializeConditions() + + if got, want := len(status.Conditions), 2; got != want { + t.Errorf("InitializeConditions() = %v, wanted %v", got, want) + } + + // Setting the other "terminal" condition makes Ready true. + manager.MarkTrue("Foo") + if got, want := manager.GetCondition("Ready").Status, corev1.ConditionTrue; got != want { + t.Errorf("MarkTrue(Foo) = %v, wanted %v", got, want) + } + + // Setting a "non-terminal" condition, doesn't change Ready. + manager.MarkUnknown("Bar", "", "") + if got, want := manager.GetCondition("Ready").Status, corev1.ConditionTrue; got != want { + t.Errorf("MarkUnknown(Foo) = %v, wanted %v", got, want) + } + + // Setting a "non-terminal" condition, doesn't change Ready. + manager.MarkFalse("Bar", "", "") + if got, want := manager.GetCondition("Ready").Status, corev1.ConditionTrue; got != want { + t.Errorf("MarkFalse(Foo) = %v, wanted %v", got, want) + } +} diff --git a/apis/duck/v1alpha1/conditions_types.go b/apis/duck/v1alpha1/conditions_types.go index 44b9992105..0dbad3397e 100644 --- a/apis/duck/v1alpha1/conditions_types.go +++ b/apis/duck/v1alpha1/conditions_types.go @@ -42,6 +42,21 @@ const ( ConditionSucceeded ConditionType = "Succeeded" ) +// ConditionSeverity expresses the severity of a Condition Type failing. +type ConditionSeverity string + +const ( + // ConditionSeverityError specifies that a failure of a condition type + // should be viewed as an error. + ConditionSeverityError ConditionSeverity = "Error" + // ConditionSeverityWarning specifies that a failure of a condition type + // should be viewed as a warning, but that things could still work. + ConditionSeverityWarning ConditionSeverity = "Warning" + // ConditionSeverityInfo specifies that a failure of a condition type + // should be viewed as purely informational, and that things could still work. + ConditionSeverityInfo ConditionSeverity = "Info" +) + // Conditions defines a readiness condition for a Knative resource. // See: https://github.com/kubernetes/community/blob/master/contributors/devel/api-conventions.md#typical-status-properties // +k8s:deepcopy-gen=true @@ -54,6 +69,11 @@ type Condition struct { // +required Status corev1.ConditionStatus `json:"status" description:"status of the condition, one of True, False, Unknown"` + // Severity with which to treat failures of this type of condition. + // When this is not specified, it defaults to Error. + // +optional + Severity ConditionSeverity `json:"severity,omitempty" description:"how to interpret failures of this condition, one of Error, Warning, Info"` + // LastTransitionTime is the last time the condition transitioned from one status to another. // We use VolatileTime in place of metav1.Time to exclude this from creating equality.Semantic // differences (all other things held constant). @@ -93,7 +113,6 @@ func (c *Condition) IsUnknown() bool { return c.Status == corev1.ConditionUnknown } - // Conditions is an Implementable "duck type". var _ duck.Implementable = (*Conditions)(nil)