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

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.