Skip to content

Update the ConditionSet to support non-terminal conditions. #161

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
knative-prow-robot merged 2 commits into from
Nov 8, 2018
Merged

Update the ConditionSet to support non-terminal conditions. #161

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d0878e8
Update the ConditionSet to support non-terminal conditions.
mattmoor Nov 2, 2018
341c4ff
Add severity handling to ConditionSet.
mattmoor Nov 7, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
86 changes: 60 additions & 26 deletions apis/duck/v1alpha1/condition_set.go
View file
Open in desktop
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{
Copy link
Copy Markdown
Contributor

@n3wscott n3wscott Nov 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was so close!

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)
Copy link
Copy Markdown
Contributor

@n3wscott n3wscott Nov 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch!

}
}

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
Open in desktop
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
Open in desktop
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