Skip to content

Commit

Permalink
Update the ConditionSet to support non-terminal conditions. (#161)
Browse files Browse the repository at this point in the history 
* 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: knative/serving#2394

* Add severity handling to ConditionSet.

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 @knative-prow-robot
mattmoor authored and knative-prow-robot committed Nov 8, 2018
1 parent 7e60946 commit 4b704fa
Show file tree
Hide file tree
Showing 4 changed files with 167 additions and 57 deletions.
86 changes: 60 additions & 26 deletions apis/duck/v1alpha1/condition_set.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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...)
}
Expand Down Expand Up @@ -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.
Expand All @@ -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),
})
}

Expand All @@ -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
Expand All @@ -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),
})
}
}
Expand All @@ -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),
})
}
}
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
32 changes: 32 additions & 0 deletions apis/duck/v1alpha1/condition_set_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ package v1alpha1

import (
"testing"

corev1 "k8s.io/api/core/v1"
)

func TestNewLivingConditionSet(t *testing.T) {
Expand Down Expand Up @@ -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)
}
}
Loading

0 comments on commit 4b704fa

Please sign in to comment.