make no matching conditions result in unknown, not success

Author: deads2k

pkg/operator/status/condition.go

@@ -11,48 +11,78 @@ import (
 
 // conditionMergeState indicates whether you want to merge all Falses or merge all Trues.  For instance, Failures merge
 // on true, but Available merges on false.  Thing of it like an anti-default.
-func unionCondition(conditionType string, conditionMergeState operatorv1.ConditionStatus, allConditions ...operatorv1.OperatorCondition) configv1.ClusterOperatorStatusCondition {
-	var interestingConditions []operatorv1.OperatorCondition
+func unionCondition(conditionType string, defaultConditionState operatorv1.ConditionStatus, allConditions ...operatorv1.OperatorCondition) configv1.ClusterOperatorStatusCondition {
+	var oppositeConditionStatus operatorv1.ConditionStatus
+	if defaultConditionState == operatorv1.ConditionTrue {
+		oppositeConditionStatus = operatorv1.ConditionFalse
+	} else {
+		oppositeConditionStatus = operatorv1.ConditionTrue
+	}
+
+	interestingConditions := []operatorv1.OperatorCondition{}
+	badConditions := []operatorv1.OperatorCondition{}
 	for _, condition := range allConditions {
-		if strings.HasSuffix(condition.Type, conditionType) && condition.Status == conditionMergeState {
+		if strings.HasSuffix(condition.Type, conditionType) {
 			interestingConditions = append(interestingConditions, condition)
+
+			if condition.Status == oppositeConditionStatus {
+				badConditions = append(badConditions, condition)
+			}
 		}
 	}
 
 	unionedCondition := operatorv1.OperatorCondition{Type: conditionType, Status: operatorv1.ConditionUnknown}
-	if len(interestingConditions) > 0 {
-		unionedCondition.Status = conditionMergeState
-		var messages []string
-		latestTransitionTime := metav1.Time{}
-		for _, condition := range interestingConditions {
-			if latestTransitionTime.Before(&condition.LastTransitionTime) {
-				latestTransitionTime = condition.LastTransitionTime
-			}
+	if len(interestingConditions) == 0 {
+		unionedCondition.Status = operatorv1.ConditionUnknown
+		unionedCondition.Reason = "NoData"
+		return OperatorConditionToClusterOperatorCondition(unionedCondition)
+	}
 
-			if len(condition.Message) == 0 {
-				continue
-			}
-			for _, message := range strings.Split(condition.Message, "\n") {
-				messages = append(messages, fmt.Sprintf("%s: %s", condition.Type, message))
-			}
+	if len(badConditions) == 0 {
+		unionedCondition.Status = defaultConditionState
+		unionedCondition.Message = unionMessage(interestingConditions)
+		unionedCondition.Reason = "AsExpected"
+		unionedCondition.LastTransitionTime = latestTransitionTime(interestingConditions)
+
+		return OperatorConditionToClusterOperatorCondition(unionedCondition)
+	}
+
+	// at this point we have bad conditions
+	unionedCondition.Status = oppositeConditionStatus
+	unionedCondition.Message = unionMessage(badConditions)
+	unionedCondition.Reason = unionReason(badConditions)
+	unionedCondition.LastTransitionTime = latestTransitionTime(badConditions)
+
+	return OperatorConditionToClusterOperatorCondition(unionedCondition)
+}
+
+func latestTransitionTime(conditions []operatorv1.OperatorCondition) metav1.Time {
+	latestTransitionTime := metav1.Time{}
+	for _, condition := range conditions {
+		if latestTransitionTime.Before(&condition.LastTransitionTime) {
+			latestTransitionTime = condition.LastTransitionTime
 		}
-		if len(messages) > 0 {
-			unionedCondition.Message = strings.Join(messages, "\n")
+	}
+	return latestTransitionTime
+}
+
+func unionMessage(conditions []operatorv1.OperatorCondition) string {
+	messages := []string{}
+	for _, condition := range conditions {
+		if len(condition.Message) == 0 {
+			continue
 		}
-		if len(interestingConditions) == 1 {
-			unionedCondition.Reason = interestingConditions[0].Type
-		} else {
-			unionedCondition.Reason = "MultipleConditionsMatching"
+		for _, message := range strings.Split(condition.Message, "\n") {
+			messages = append(messages, fmt.Sprintf("%s: %s", condition.Type, message))
 		}
-		unionedCondition.LastTransitionTime = latestTransitionTime
+	}
+	return strings.Join(messages, "\n")
+}
 
+func unionReason(conditions []operatorv1.OperatorCondition) string {
+	if len(conditions) == 1 {
+		return conditions[0].Type
 	} else {
-		if conditionMergeState == operatorv1.ConditionTrue {
-			unionedCondition.Status = operatorv1.ConditionFalse
-		} else {
-			unionedCondition.Status = operatorv1.ConditionTrue
-		}
+		return "MultipleConditionsMatching"
 	}
-
-	return OperatorConditionToClusterOperatorCondition(unionedCondition)
 }

pkg/operator/status/status_controller.go

@@ -132,10 +132,10 @@ func (c StatusSyncer) sync() error {
 	}
 
 	clusterOperatorObj.Status.RelatedObjects = c.relatedObjects
-	configv1helpers.SetStatusCondition(&clusterOperatorObj.Status.Conditions, unionCondition("Failing", operatorv1.ConditionTrue, currentDetailedStatus.Conditions...))
-	configv1helpers.SetStatusCondition(&clusterOperatorObj.Status.Conditions, unionCondition("Progressing", operatorv1.ConditionTrue, currentDetailedStatus.Conditions...))
-	configv1helpers.SetStatusCondition(&clusterOperatorObj.Status.Conditions, unionCondition("Available", operatorv1.ConditionFalse, currentDetailedStatus.Conditions...))
-	configv1helpers.SetStatusCondition(&clusterOperatorObj.Status.Conditions, unionCondition("Upgradeable", operatorv1.ConditionFalse, currentDetailedStatus.Conditions...))
+	configv1helpers.SetStatusCondition(&clusterOperatorObj.Status.Conditions, unionCondition("Failing", operatorv1.ConditionFalse, currentDetailedStatus.Conditions...))
+	configv1helpers.SetStatusCondition(&clusterOperatorObj.Status.Conditions, unionCondition("Progressing", operatorv1.ConditionFalse, currentDetailedStatus.Conditions...))
+	configv1helpers.SetStatusCondition(&clusterOperatorObj.Status.Conditions, unionCondition("Available", operatorv1.ConditionTrue, currentDetailedStatus.Conditions...))
+	configv1helpers.SetStatusCondition(&clusterOperatorObj.Status.Conditions, unionCondition("Upgradeable", operatorv1.ConditionTrue, currentDetailedStatus.Conditions...))
 
 	// TODO work out removal.  We don't always know the existing value, so removing early seems like a bad idea.  Perhaps a remove flag.
 	versions := c.versionGetter.GetVersions()

pkg/operator/status/status_controller_test.go

@@ -1,7 +1,6 @@
 package status
 
 import (
-	"fmt"
 	"reflect"
 	"strings"
 	"testing"
@@ -20,25 +19,36 @@ import (
 func TestFailing(t *testing.T) {
 
 	testCases := []struct {
+		name                  string
 		conditions            []operatorv1.OperatorCondition
 		expectedFailingStatus configv1.ConditionStatus
 		expectedMessages      []string
 		expectedReason        string
 	}{
 		{
+			name:                  "no data",
+			conditions:            []operatorv1.OperatorCondition{},
+			expectedFailingStatus: configv1.ConditionUnknown,
+			expectedReason:        "NoData",
+		},
+		{
+			name: "one failing false",
 			conditions: []operatorv1.OperatorCondition{
 				{Type: "TypeAFailing", Status: operatorv1.ConditionFalse},
 			},
 			expectedFailingStatus: configv1.ConditionFalse,
+			expectedReason:        "AsExpected",
 		},
 		{
+			name: "one failing true",
 			conditions: []operatorv1.OperatorCondition{
 				{Type: "TypeAFailing", Status: operatorv1.ConditionTrue},
 			},
 			expectedFailingStatus: configv1.ConditionTrue,
 			expectedReason:        "TypeAFailing",
 		},
 		{
+			name: "two present, one failing",
 			conditions: []operatorv1.OperatorCondition{
 				{Type: "TypeAFailing", Status: operatorv1.ConditionTrue, Message: "a message from type a"},
 				{Type: "TypeBFailing", Status: operatorv1.ConditionFalse},
@@ -50,6 +60,7 @@ func TestFailing(t *testing.T) {
 			},
 		},
 		{
+			name: "two present, second one failing",
 			conditions: []operatorv1.OperatorCondition{
 				{Type: "TypeAFailing", Status: operatorv1.ConditionFalse},
 				{Type: "TypeBFailing", Status: operatorv1.ConditionTrue, Message: "a message from type b"},
@@ -61,6 +72,7 @@ func TestFailing(t *testing.T) {
 			},
 		},
 		{
+			name: "many present, some failing",
 			conditions: []operatorv1.OperatorCondition{
 				{Type: "TypeAFailing", Status: operatorv1.ConditionFalse},
 				{Type: "TypeBFailing", Status: operatorv1.ConditionTrue, Message: "a message from type b\nanother message from type b"},
@@ -76,8 +88,8 @@ func TestFailing(t *testing.T) {
 			},
 		},
 	}
-	for name, tc := range testCases {
-		t.Run(fmt.Sprintf("%05d", name), func(t *testing.T) {
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
 			clusterOperatorClient := fake.NewSimpleClientset(&configv1.ClusterOperator{
 				ObjectMeta: metav1.ObjectMeta{Name: "OPERATOR_NAME", ResourceVersion: "12"},
 			})