pkg/cvo/internal: Do not block on Degraded=True ClusterOperator

We have blocking on this condition since 545c342 (api: make status
substruct on operatorstatus, 2018-10-15, #31) when it was Failing.
We'd softened our install-time handling to act this way back in
b0b4902 (clusteroperator: Don't block on failing during
initialization, 2019-03-11, #136), motivated by install speed [1].
And a degraded operator may slow dependent components in their own
transitions.  But as long as the operator/operand are available at
all, it should not block depndent components from transitioning, so
this commit removes the Degraded=True block from the remaining modes.

We still have the warning ClusterOperatorDegraded alerting admins
when an operator goes Degraded=True for a while, we will just no
longer block updates at that point.  We won't block ReconcilingMode
manifest application either, but since that's already flattened and
permuted, and ClusterOperator tend to be towards the end of their
TaskNode, the impact on ReconcilingMode is minimal (except that we
will no longer go Failing=True in ClusterVersion when the only issue
is some Degraded=True ClusterOperator).

[1]: #136 (comment)

Author: wking

pkg/cvo/internal/operatorstatus.go

@@ -232,20 +232,15 @@ func checkOperatorHealth(ctx context.Context, client ClusterOperatorsGetter, exp
 		}
 	}
 
+	var degradedError error
 	if degraded {
 		if degradedCondition != nil && len(degradedCondition.Message) > 0 {
 			nestedMessage = fmt.Errorf("cluster operator %s is %s=%s: %s, %s", actual.Name, degradedCondition.Type, degradedCondition.Status, degradedCondition.Reason, degradedCondition.Message)
 		}
-		var updateEffect payload.UpdateEffectType
 
-		if mode == resourcebuilder.InitializingMode {
-			updateEffect = payload.UpdateEffectReport
-		} else {
-			updateEffect = payload.UpdateEffectFailAfterInterval
-		}
-		return &payload.UpdateError{
+		degradedError = &payload.UpdateError{
 			Nested:              nestedMessage,
-			UpdateEffect:        updateEffect,
+			UpdateEffect:        payload.UpdateEffectReport,
 			Reason:              "ClusterOperatorDegraded",
 			PluralReason:        "ClusterOperatorsDegraded",
 			Message:             fmt.Sprintf("Cluster operator %s is degraded", actual.Name),
@@ -256,10 +251,19 @@ func checkOperatorHealth(ctx context.Context, client ClusterOperatorsGetter, exp
 
 	// during initialization we allow undone versions
 	if len(undone) > 0 && mode != resourcebuilder.InitializingMode {
-		nestedMessage = fmt.Errorf("cluster operator %s is available and not degraded but has not finished updating to target version", actual.Name)
+		var updateEffect payload.UpdateEffectType
+
+		if degradedError == nil {
+			nestedMessage = fmt.Errorf("cluster operator %s is available and not degraded but has not finished updating to target version", actual.Name)
+			updateEffect = payload.UpdateEffectNone // block on versions, but nothing to go Failing=True over
+		} else {
+			nestedMessage = fmt.Errorf("cluster operator %s is available but %s=%s and has not finished updating to target version", actual.Name, degradedCondition.Type, degradedCondition.Status)
+			updateEffect = payload.UpdateEffectFail // block on versions, go Failing=True on Degraded=True
+		}
+
 		return &payload.UpdateError{
 			Nested:              nestedMessage,
-			UpdateEffect:        payload.UpdateEffectNone,
+			UpdateEffect:        updateEffect,
 			Reason:              "ClusterOperatorUpdating",
 			PluralReason:        "ClusterOperatorsUpdating",
 			Message:             fmt.Sprintf("Cluster operator %s is updating versions", actual.Name),
@@ -268,5 +272,5 @@ func checkOperatorHealth(ctx context.Context, client ClusterOperatorsGetter, exp
 		}
 	}
 
-	return nil
+	return degradedError
 }

pkg/cvo/internal/operatorstatus_test.go

@@ -432,7 +432,7 @@ func Test_checkOperatorHealth(t *testing.T) {
 		},
 		expErr: &payload.UpdateError{
 			Nested:              fmt.Errorf("cluster operator test-co is Degraded=True: RandomReason, random error"),
-			UpdateEffect:        payload.UpdateEffectFailAfterInterval,
+			UpdateEffect:        payload.UpdateEffectReport,
 			Reason:              "ClusterOperatorDegraded",
 			PluralReason:        "ClusterOperatorsDegraded",
 			Message:             "Cluster operator test-co is degraded",
@@ -504,7 +504,7 @@ func Test_checkOperatorHealth(t *testing.T) {
 		},
 		expErr: &payload.UpdateError{
 			Nested:              fmt.Errorf("cluster operator test-co is Degraded=True: RandomReason, random error"),
-			UpdateEffect:        payload.UpdateEffectFailAfterInterval,
+			UpdateEffect:        payload.UpdateEffectReport,
 			Reason:              "ClusterOperatorDegraded",
 			PluralReason:        "ClusterOperatorsDegraded",
 			Message:             "Cluster operator test-co is degraded",
@@ -573,7 +573,7 @@ func Test_checkOperatorHealth(t *testing.T) {
 		},
 		expErr: &payload.UpdateError{
 			Nested:              fmt.Errorf("cluster operator test-co: available=true, progressing=true, degraded=true, undone="),
-			UpdateEffect:        payload.UpdateEffectFailAfterInterval,
+			UpdateEffect:        payload.UpdateEffectReport,
 			Reason:              "ClusterOperatorDegraded",
 			PluralReason:        "ClusterOperatorsDegraded",
 			Message:             "Cluster operator test-co is degraded",

pkg/cvo/status.go

@@ -552,23 +552,6 @@ func convertErrorToProgressing(now time.Time, statusFailure error) (reason strin
 		return uErr.Reason, fmt.Sprintf("waiting on %s", uErr.Name), true
 	case payload.UpdateEffectFail:
 		return "", "", false
-	case payload.UpdateEffectFailAfterInterval:
-		var exceeded []string
-		threshold := now.Add(-(40 * time.Minute))
-		names := uErr.Names
-		if len(names) == 0 {
-			names = []string{uErr.Name}
-		}
-		for _, name := range names {
-			if payload.COUpdateStartTimesGet(name).Before(threshold) {
-				exceeded = append(exceeded, name)
-			}
-		}
-		if len(exceeded) > 0 {
-			return uErr.Reason, fmt.Sprintf("wait has exceeded 40 minutes for these operators: %s", strings.Join(exceeded, ", ")), false
-		} else {
-			return uErr.Reason, fmt.Sprintf("waiting up to 40 minutes on %s", uErr.Name), true
-		}
 	}
 	return "", "", false
 }

pkg/cvo/status_test.go

@@ -439,7 +439,7 @@ func TestUpdateClusterVersionStatus_FilteringMultipleErrorsForFailingCondition(t
 			},
 		},
 		{
-			name: "MultipleErrors of UpdateEffectFail and UpdateEffectFailAfterInterval",
+			name: "MultipleErrors of UpdateEffectFail and UpdateEffectReport",
 			args: args{
 				syncWorkerStatus: &SyncWorkerStatus{
 					Failure: &payload.UpdateError{
@@ -450,14 +450,14 @@ func TestUpdateClusterVersionStatus_FilteringMultipleErrorsForFailingCondition(t
 								Message:      "Cluster operator A is not available",
 							},
 							&payload.UpdateError{
-								UpdateEffect: payload.UpdateEffectFailAfterInterval,
+								UpdateEffect: payload.UpdateEffectReport,
 								Reason:       "ClusterOperatorDegraded",
 								Message:      "Cluster operator B is degraded",
 							},
 						}),
 						UpdateEffect: payload.UpdateEffectFail,
-						Reason:       "MultipleErrors",
-						Message:      "Multiple errors are preventing progress:\n* Cluster operator A is not available\n* Cluster operator B is degraded",
+						Reason:       "ClusterOperatorNotAvailable",
+						Message:      "Cluster operator A is not available",
 					},
 				},
 			},
@@ -541,7 +541,7 @@ func TestUpdateClusterVersionStatus_FilteringMultipleErrorsForFailingCondition(t
 			},
 		},
 		{
-			name: "MultipleErrors of UpdateEffectFail, UpdateEffectFailAfterInterval, and UpdateEffectNone",
+			name: "MultipleErrors of UpdateEffectFail, UpdateEffectReport, and UpdateEffectNone",
 			args: args{
 				syncWorkerStatus: &SyncWorkerStatus{
 					Failure: &payload.UpdateError{
@@ -557,14 +557,14 @@ func TestUpdateClusterVersionStatus_FilteringMultipleErrorsForFailingCondition(t
 								Message:      "Cluster operator B is updating versions",
 							},
 							&payload.UpdateError{
-								UpdateEffect: payload.UpdateEffectFailAfterInterval,
+								UpdateEffect: payload.UpdateEffectReport,
 								Reason:       "ClusterOperatorDegraded",
 								Message:      "Cluster operator C is degraded",
 							},
 						}),
 						UpdateEffect: payload.UpdateEffectFail,
 						Reason:       "MultipleErrors",
-						Message:      "Multiple errors are preventing progress:\n* Cluster operator A is not available\n* Cluster operator B is updating versions\n* Cluster operator C is degraded",
+						Message:      "Multiple errors are preventing progress:\n* Cluster operator A is not available\n* Cluster operator B is updating versions",
 					},
 				},
 			},
@@ -664,10 +664,6 @@ func Test_filterOutUpdateErrors(t *testing.T) {
 						Name:         "Report",
 						UpdateEffect: payload.UpdateEffectReport,
 					},
-					&payload.UpdateError{
-						Name:         "Fail After Interval",
-						UpdateEffect: payload.UpdateEffectFailAfterInterval,
-					},
 				},
 				updateEffectType: payload.UpdateEffectNone,
 			},
@@ -680,19 +676,15 @@ func Test_filterOutUpdateErrors(t *testing.T) {
 					Name:         "Report",
 					UpdateEffect: payload.UpdateEffectReport,
 				},
-				&payload.UpdateError{
-					Name:         "Fail After Interval",
-					UpdateEffect: payload.UpdateEffectFailAfterInterval,
-				},
 			},
 		},
 		{
 			name: "errors contain update errors of the specified value UpdateEffectNone",
 			args: args{
 				errs: []error{
 					&payload.UpdateError{
-						Name:         "Fail After Interval",
-						UpdateEffect: payload.UpdateEffectFailAfterInterval,
+						Name:         "Fail",
+						UpdateEffect: payload.UpdateEffectFail,
 					},
 					&payload.UpdateError{
 						Name:         "None #1",
@@ -711,8 +703,8 @@ func Test_filterOutUpdateErrors(t *testing.T) {
 			},
 			want: []error{
 				&payload.UpdateError{
-					Name:         "Fail After Interval",
-					UpdateEffect: payload.UpdateEffectFailAfterInterval,
+					Name:         "Fail",
+					UpdateEffect: payload.UpdateEffectFail,
 				},
 				&payload.UpdateError{
 					Name:         "Report",
@@ -725,8 +717,8 @@ func Test_filterOutUpdateErrors(t *testing.T) {
 			args: args{
 				errs: []error{
 					&payload.UpdateError{
-						Name:         "Fail After Interval",
-						UpdateEffect: payload.UpdateEffectFailAfterInterval,
+						Name:         "Fail",
+						UpdateEffect: payload.UpdateEffectFail,
 					},
 					&payload.UpdateError{
 						Name:         "None #1",
@@ -745,8 +737,8 @@ func Test_filterOutUpdateErrors(t *testing.T) {
 			},
 			want: []error{
 				&payload.UpdateError{
-					Name:         "Fail After Interval",
-					UpdateEffect: payload.UpdateEffectFailAfterInterval,
+					Name:         "Fail",
+					UpdateEffect: payload.UpdateEffectFail,
 				},
 				&payload.UpdateError{
 					Name:         "None #1",

pkg/cvo/sync_worker.go

@@ -1264,10 +1264,6 @@ func condenseClusterOperators(errs []error) []error {
 				if updateEffect == payload.UpdateEffectReport {
 					updateEffect = payload.UpdateEffectNone
 				}
-			case payload.UpdateEffectFailAfterInterval:
-				if updateEffect != payload.UpdateEffectFail {
-					updateEffect = payload.UpdateEffectFailAfterInterval
-				}
 			case payload.UpdateEffectFail:
 				updateEffect = payload.UpdateEffectFail
 			}

pkg/cvo/sync_worker_test.go

@@ -256,8 +256,8 @@ func Test_condenseClusterOperators(t *testing.T) {
 		Name:                "test-co-B",
 		Task:                task("test-co-B", configv1.GroupVersion.WithKind("ClusterOperator")),
 	}
-	coCDegradedFailAfterInterval := &payload.UpdateError{
-		UpdateEffect:        payload.UpdateEffectFailAfterInterval,
+	coCDegraded := &payload.UpdateError{
+		UpdateEffect:        payload.UpdateEffectReport,
 		Reason:              "ClusterOperatorDegraded",
 		PluralReason:        "ClusterOperatorsDegraded",
 		Message:             "Cluster operator test-co-C is degraded",
@@ -318,10 +318,9 @@ func Test_condenseClusterOperators(t *testing.T) {
 		input: []error{
 			coBUpdating,
 			coAUpdating,
-			coCDegradedFailAfterInterval,
+			coCDegraded,
 		},
 		expected: []error{
-			coCDegradedFailAfterInterval, // Degraded sorts before Updating
 			&payload.UpdateError{
 				Nested:              errors.NewAggregate([]error{coBUpdating, coAUpdating}),
 				UpdateEffect:        payload.UpdateEffectNone,
@@ -332,17 +331,18 @@ func Test_condenseClusterOperators(t *testing.T) {
 				Name:                "test-co-A, test-co-B",
 				Names:               []string{"test-co-A", "test-co-B"},
 			},
+			coCDegraded,
 		},
 	}, {
 		name: "there ClusterOperator with the same reason but different effects",
 		input: []error{
 			coBDegradedFail,
 			coADegradedNone,
-			coCDegradedFailAfterInterval,
+			coCDegraded,
 		},
 		expected: []error{
 			&payload.UpdateError{
-				Nested:              errors.NewAggregate([]error{coBDegradedFail, coADegradedNone, coCDegradedFailAfterInterval}),
+				Nested:              errors.NewAggregate([]error{coBDegradedFail, coADegradedNone, coCDegraded}),
 				UpdateEffect:        payload.UpdateEffectFail,
 				Reason:              "ClusterOperatorsDegraded",
 				PluralReason:        "ClusterOperatorsDegraded",
@@ -353,15 +353,15 @@ func Test_condenseClusterOperators(t *testing.T) {
 			},
 		},
 	}, {
-		name: "to ClusterOperator with the same reason but None and FailAfterInterval effects",
+		name: "to ClusterOperator with the same reason but None and Report effects",
 		input: []error{
 			coADegradedNone,
-			coCDegradedFailAfterInterval,
+			coCDegraded,
 		},
 		expected: []error{
 			&payload.UpdateError{
-				Nested:              errors.NewAggregate([]error{coADegradedNone, coCDegradedFailAfterInterval}),
-				UpdateEffect:        payload.UpdateEffectFailAfterInterval,
+				Nested:              errors.NewAggregate([]error{coADegradedNone, coCDegraded}),
+				UpdateEffect:        payload.UpdateEffectFail,
 				Reason:              "ClusterOperatorsDegraded",
 				PluralReason:        "ClusterOperatorsDegraded",
 				Message:             "Cluster operators test-co-A, test-co-C are degraded",

pkg/payload/task.go

@@ -161,10 +161,6 @@ const (
 
 	// UpdateEffectFail defines an error as indicating the update is failing.
 	UpdateEffectFail UpdateEffectType = "Fail"
-
-	// UpdateEffectFailAfterInterval defines an error as one which indicates the update
-	// is failing if the error continues for a defined interval.
-	UpdateEffectFailAfterInterval UpdateEffectType = "FailAfterInterval"
 )
 
 // UpdateError is a wrapper for errors that occur during a payload sync.