From d0878e83aa1d5911c49c9c30df152b861fb16703 Mon Sep 17 00:00:00 2001 From: Matt Moore Date: Fri, 2 Nov 2018 22:48:38 +0000 Subject: [PATCH 1/2] Update the ConditionSet to support non-terminal conditions. This change updates the `ConditionSet` datatype to treat the set of conditions that it is initially supplied with as the set of "terminal" conditions, where as before: 1. If all `True`, results in `Ready=True` 2. If any `False`, results in `Ready=False` 3. Otherwise, `Ready=Unknown` However, in additional to this initial "terminal" set, other conditions may be set that are "non-terminal" and have no influence over the "happy" condition (e.g. `Ready`). Related to: https://github.com/knative/serving/issues/2394 --- apis/duck/v1alpha1/condition_set.go | 36 ++++++++++++++++-------- apis/duck/v1alpha1/condition_set_test.go | 32 +++++++++++++++++++++ 2 files changed, 56 insertions(+), 12 deletions(-) diff --git a/apis/duck/v1alpha1/condition_set.go b/apis/duck/v1alpha1/condition_set.go index 30b76a5f66..f3a91df1eb 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...) } @@ -246,6 +248,7 @@ func (r conditionsImpl) MarkUnknown(t ConditionType, reason, messageFormat strin }) // check the dependents. + isDependent := false for _, cond := range r.dependents { c := r.GetCondition(cond) // Failed conditions trump Unknown conditions @@ -257,23 +260,32 @@ 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...), + }) + } } // 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, 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) + } +} From 341c4ffd356db2c8260963e564997d32420a2b9d Mon Sep 17 00:00:00 2001 From: Matt Moore Date: Wed, 7 Nov 2018 23:48:21 +0000 Subject: [PATCH 2/2] Add severity handling to ConditionSet. This adds a Severity field to our common Condition type, as outlined [here](https://github.com/knative/serving/issues/2394#issuecomment-436806801). The only way Severity is populated in this change is: * Terminal conditions (passed at initial construction) always result in `severity: Error`, * non-Terminal conditions (added outside initial set) always result in `severity: Info`. We should think about how we want to update the `Condition{Set,Manager}` interfaces to surface more control (e.g. `severity: Warning`). --- apis/duck/v1alpha1/condition_set.go | 58 +++++++++---- apis/duck/v1alpha1/condition_set_impl_test.go | 85 ++++++++++++------- apis/duck/v1alpha1/conditions_types.go | 21 ++++- 3 files changed, 115 insertions(+), 49 deletions(-) diff --git a/apis/duck/v1alpha1/condition_set.go b/apis/duck/v1alpha1/condition_set.go index f3a91df1eb..e330970bd0 100644 --- a/apis/duck/v1alpha1/condition_set.go +++ b/apis/duck/v1alpha1/condition_set.go @@ -211,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. @@ -231,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), }) } @@ -241,10 +259,11 @@ 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. @@ -268,10 +287,11 @@ func (r conditionsImpl) MarkUnknown(t ConditionType, reason, messageFormat strin 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...), + Type: r.happy, + Status: corev1.ConditionUnknown, + Reason: reason, + Message: fmt.Sprintf(messageFormat, messageA...), + Severity: r.severity(r.happy), }) } } @@ -287,10 +307,11 @@ func (r conditionsImpl) MarkFalse(t ConditionType, reason, messageFormat string, 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), }) } } @@ -307,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/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)