Skip to content

Commit

Permalink
Add severity handling to ConditionSet.
Browse files Browse the repository at this point in the history 
This adds a Severity field to our common Condition type, as outlined [here](knative/serving#2394 (comment)).

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`).
  • Loading branch information
@mattmoor
mattmoor committed Nov 7, 2018
1 parent d0878e8 commit 341c4ff
Show file tree
Hide file tree
Showing 3 changed files with 115 additions and 49 deletions.
58 changes: 40 additions & 18 deletions apis/duck/v1alpha1/condition_set.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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),
})
}

Expand All @@ -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.
Expand All @@ -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),
})
}
}
Expand All @@ -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),
})
}
}
Expand All @@ -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),
})
}
}
Expand Down
85 changes: 55 additions & 30 deletions apis/duck/v1alpha1/condition_set_impl_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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)
}
})
Expand Down Expand Up @@ -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)
}
})
Expand Down Expand Up @@ -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)
}
})
Expand Down Expand Up @@ -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)
}
})
Expand Down Expand Up @@ -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)
}
})
Expand Down Expand Up @@ -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)
}
})
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)
}
})
Expand All @@ -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)
}
})
Expand Down Expand Up @@ -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)
}
})
Expand Down Expand Up @@ -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)
}
})
Expand Down Expand Up @@ -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)
}
})
Expand Down Expand Up @@ -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)
}
})
Expand Down Expand Up @@ -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,
},
}}

Expand Down
21 changes: 20 additions & 1 deletion apis/duck/v1alpha1/conditions_types.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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).
Expand Down Expand Up @@ -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)

Expand Down

0 comments on commit 341c4ff

Please sign in to comment.