Make the revision controller report degraded on error

This makes the error handling slightly easier, keeps the retry on error
logic, leaves the requeue on self-modification we rely upon.

This doesn't introduce VAP to prevent writes to "immutable" configmaps
and secrets, but I'm open to doing that to prevent stale caches from
creating unstable and inconsistent revisions.

Author: deads2k

pkg/operator/revisioncontroller/revision_controller.go

@@ -7,10 +7,8 @@ import (
 	"strings"
 	"time"
 
-	operatorv1 "github.com/openshift/api/operator/v1"
 	applyoperatorv1 "github.com/openshift/client-go/operator/applyconfigurations/operator/v1"
 	"github.com/openshift/library-go/pkg/controller/factory"
-	"github.com/openshift/library-go/pkg/operator/condition"
 	"github.com/openshift/library-go/pkg/operator/events"
 	"github.com/openshift/library-go/pkg/operator/management"
 	"github.com/openshift/library-go/pkg/operator/resource/resourceapply"
@@ -90,26 +88,28 @@ func NewRevisionController(
 			kubeInformersForTargetNamespace.Core().V1().Secrets().Informer(),
 		).
 		WithSync(c.sync).
+		WithSyncDegradedOnError(operatorClient).
 		ResyncEvery(1*time.Minute).
 		ToController(
-			"RevisionController", // don't change what is passed here unless you also remove the old FooDegraded condition
+			"RevisionController",
 			eventRecorder,
 		)
 }
 
 // createRevisionIfNeeded takes care of creating content for the static pods to use.
 // returns whether or not requeue and if an error happened when updating status.  Normally it updates status itself.
-func (c RevisionController) createRevisionIfNeeded(ctx context.Context, recorder events.Recorder, currentLastAvailableRevision int32) (wroteStatus bool, requeue bool, err error) {
+func (c RevisionController) createRevisionIfNeeded(ctx context.Context, recorder events.Recorder, currentLastAvailableRevision int32) error {
 	isLatestRevisionCurrent, requiredIsNotFound, reason := c.isLatestRevisionCurrent(ctx, currentLastAvailableRevision)
 
 	// check to make sure that the latestRevision has the exact content we expect.  No mutation here, so we start creating the next Revision only when it is required
 	if isLatestRevisionCurrent {
 		klog.V(4).Infof("Returning early, %d triggered and up to date", currentLastAvailableRevision)
-		return false, false, nil
+		return nil
 	}
 
 	nextRevision := currentLastAvailableRevision + 1
 	var createdNewRevision bool
+	var err error
 	// check to make sure no new revision is created when a required object is missing
 	if requiredIsNotFound {
 		err = fmt.Errorf("%v", reason)
@@ -119,41 +119,24 @@ func (c RevisionController) createRevisionIfNeeded(ctx context.Context, recorder
 	}
 
 	if err != nil {
-		status := applyoperatorv1.OperatorStatus().
-			WithConditions(applyoperatorv1.OperatorCondition().
-				WithType(condition.RevisionControllerDegradedConditionType).
-				WithStatus(operatorv1.ConditionTrue).
-				WithReason("ContentCreationError").
-				WithMessage(err.Error()),
-			).
-			WithLatestAvailableRevision(currentLastAvailableRevision)
-		updateError := c.operatorClient.ApplyOperatorStatus(ctx, c.controllerInstanceName, status)
-		if updateError != nil {
-			recorder.Warningf("RevisionCreateFailed", "Failed to create revision %d: %v", nextRevision, err.Error())
-			return true, true, updateError
-		}
-		return true, true, nil
+		return err
 	}
 
 	if !createdNewRevision {
 		klog.V(4).Infof("Revision %v not created", nextRevision)
-		return false, false, nil
+		return nil
 	}
 
 	recorder.Eventf("RevisionTriggered", "new revision %d triggered by %q", nextRevision, reason)
 
 	status := applyoperatorv1.OperatorStatus().
-		WithConditions(applyoperatorv1.OperatorCondition().
-			WithType(condition.RevisionControllerDegradedConditionType).
-			WithStatus(operatorv1.ConditionFalse),
-		).
 		WithLatestAvailableRevision(nextRevision)
 	updateError := c.operatorClient.ApplyOperatorStatus(ctx, c.controllerInstanceName, status)
 	if updateError != nil {
-		return true, true, updateError
+		return updateError
 	}
 
-	return true, false, nil
+	return nil
 }
 
 func nameFor(name string, revision int32) string {
@@ -394,47 +377,23 @@ func (c RevisionController) sync(ctx context.Context, syncCtx factory.SyncContex
 	if latestObservedRevision != 0 && operatorStatus.LatestAvailableRevision != latestObservedRevision {
 		// Then make sure that revision number is what's in the operator status
 		status := applyoperatorv1.OperatorStatus().
-			WithConditions(applyoperatorv1.OperatorCondition().
-				WithType(condition.RevisionControllerDegradedConditionType).
-				WithStatus(operatorv1.ConditionFalse),
-			).
 			WithLatestAvailableRevision(latestObservedRevision)
 		err := c.operatorClient.ApplyOperatorStatus(ctx, c.controllerInstanceName, status)
 		if err != nil {
 			return err
 		}
-		// regardless of whether we made a change, requeue to rerun the sync with updated status
-		return factory.SyntheticRequeueError
+		// we will be called again because the update will self-feed the informer
+		return nil
 	}
 
 	if shouldCreateNewRevision, err := c.revisionPrecondition(ctx); err != nil || !shouldCreateNewRevision {
 		return err
 	}
 
-	wroteStatus, requeue, syncErr := c.createRevisionIfNeeded(ctx, syncCtx.Recorder(), operatorStatus.LatestAvailableRevision)
-	switch {
-	case requeue && syncErr == nil:
-		return factory.SyntheticRequeueError
-	case syncErr != nil:
-		// this is updated in status inside of createRevisionIfNeeded
+	syncErr := c.createRevisionIfNeeded(ctx, syncCtx.Recorder(), operatorStatus.LatestAvailableRevision)
+	if syncErr != nil {
 		return syncErr
-	case wroteStatus:
-		return syncErr
-	}
-
-	// update failing condition
-	status := applyoperatorv1.OperatorStatus().
-		WithConditions(applyoperatorv1.OperatorCondition().
-			WithType(condition.RevisionControllerDegradedConditionType).
-			WithStatus(operatorv1.ConditionFalse),
-		).
-		WithLatestAvailableRevision(operatorStatus.LatestAvailableRevision)
-	updateError := c.operatorClient.ApplyOperatorStatus(ctx, c.controllerInstanceName, status)
-	if updateError != nil {
-		if err == nil {
-			return updateError
-		}
 	}
 
-	return err
+	return nil
 }

pkg/operator/revisioncontroller/revision_controller_test.go

@@ -220,17 +220,8 @@ func TestRevisionController(t *testing.T) {
 			),
 			testConfigs:     []RevisionResource{{Name: "test-config"}},
 			testSecrets:     []RevisionResource{{Name: "test-secret"}},
-			expectSyncError: "synthetic requeue request",
+			expectSyncError: `configmaps "test-config" not found`,
 			validateStatus: func(t *testing.T, status *operatorv1.StaticPodOperatorStatus) {
-				if status.Conditions[0].Type != "RevisionControllerDegraded" {
-					t.Errorf("expected status condition to be 'RevisionControllerFailing', got %v", status.Conditions[0].Type)
-				}
-				if status.Conditions[0].Reason != "ContentCreationError" {
-					t.Errorf("expected status condition reason to be 'ContentCreationError', got %v", status.Conditions[0].Reason)
-				}
-				if !strings.Contains(status.Conditions[0].Message, `configmaps "test-config" not found`) {
-					t.Errorf("expected status to be 'configmaps test-config not found', got: %s", status.Conditions[0].Message)
-				}
 			},
 			validateActions: func(t *testing.T, actions []clienttesting.Action, kclient *fake.Clientset) {
 				createdObjects := filterCreateActions(actions)