revision_controller: update last revision only when a revision is com…

…pletely rendered

Signed-off-by: David Eads <deads@redhat.com>

pkg/operator/revisioncontroller/revision_controller.go

@@ -89,12 +89,14 @@ func (c RevisionController) createRevisionIfNeeded(ctx context.Context, recorder
 
 	// 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", latestAvailableRevision)
 		return false, nil
 	}
 
 	nextRevision := latestAvailableRevision + 1
-	recorder.Eventf("RevisionTriggered", "new revision %d triggered by %q", nextRevision, reason)
-	if err := c.createNewRevision(ctx, recorder, nextRevision, reason); err != nil {
+	recorder.Eventf("StartingNewRevision", "new revision %d triggered by %q", nextRevision, reason)
+	createdNewRevision, err := c.createNewRevision(ctx, recorder, nextRevision, reason)
+	if err != nil {
 		cond := operatorv1.OperatorCondition{
 			Type:    "RevisionControllerDegraded",
 			Status:  operatorv1.ConditionTrue,
@@ -108,6 +110,12 @@ func (c RevisionController) createRevisionIfNeeded(ctx context.Context, recorder
 		return true, nil
 	}
 
+	if !createdNewRevision {
+		klog.V(4).Infof("Revision %v not created", nextRevision)
+		return false, nil
+	}
+	recorder.Eventf("RevisionTriggered", "new revision %d triggered by %q", nextRevision, reason)
+
 	cond := operatorv1.OperatorCondition{
 		Type:   "RevisionControllerDegraded",
 		Status: operatorv1.ConditionFalse,
@@ -208,50 +216,74 @@ func (c RevisionController) isLatestRevisionCurrent(ctx context.Context, revisio
 	return true, ""
 }
 
-func (c RevisionController) createNewRevision(ctx context.Context, recorder events.Recorder, revision int32, reason string) error {
+// returns true if we created a revision
+func (c RevisionController) createNewRevision(ctx context.Context, recorder events.Recorder, revision int32, reason string) (bool, error) {
 	// Create a new InProgress status configmap
-	statusConfigMap := &corev1.ConfigMap{
+	desiredStatusConfigMap := &corev1.ConfigMap{
 		ObjectMeta: metav1.ObjectMeta{
 			Namespace: c.targetNamespace,
 			Name:      nameFor("revision-status", revision),
+			Annotations: map[string]string{
+				"operator.openshift.io/revision-ready": "false",
+			},
 		},
 		Data: map[string]string{
 			"revision": fmt.Sprintf("%d", revision),
 			"reason":   reason,
 		},
 	}
-	statusConfigMap, _, err := resourceapply.ApplyConfigMap(ctx, c.configMapGetter, recorder, statusConfigMap)
-	if err != nil {
-		return err
+	createdStatus, err := c.configMapGetter.ConfigMaps(desiredStatusConfigMap.Namespace).Create(ctx, desiredStatusConfigMap, metav1.CreateOptions{})
+	switch {
+	case apierrors.IsAlreadyExists(err):
+		if createdStatus == nil || len(createdStatus.UID) == 0 {
+			createdStatus, err = c.configMapGetter.ConfigMaps(desiredStatusConfigMap.Namespace).Get(ctx, desiredStatusConfigMap.Name, metav1.GetOptions{})
+			if err != nil {
+				return false, err
+			}
+		}
+		// take a live GET here to get current status to check the annotation
+		if createdStatus.Annotations["operator.openshift.io/revision-ready"] == "true" {
+			// no work to do because our cache is out of date and when we're updated, we will be able to see the result
+			klog.Infof("down the branch indicating that our cache was out of date and we're trying to recreate a revision.")
+			return false, nil
+		}
+		// update the sync and continue
+	case err != nil:
+		return false, err
 	}
 
 	ownerRefs := []metav1.OwnerReference{{
 		APIVersion: "v1",
 		Kind:       "ConfigMap",
-		Name:       statusConfigMap.Name,
-		UID:        statusConfigMap.UID,
+		Name:       createdStatus.Name,
+		UID:        createdStatus.UID,
 	}}
 
 	for _, cm := range c.configMaps {
 		obj, _, err := resourceapply.SyncConfigMap(ctx, c.configMapGetter, recorder, c.targetNamespace, cm.Name, c.targetNamespace, nameFor(cm.Name, revision), ownerRefs)
 		if err != nil {
-			return err
+			return false, err
 		}
 		if obj == nil && !cm.Optional {
-			return apierrors.NewNotFound(corev1.Resource("configmaps"), cm.Name)
+			return false, apierrors.NewNotFound(corev1.Resource("configmaps"), cm.Name)
 		}
 	}
 	for _, s := range c.secrets {
 		obj, _, err := resourceapply.SyncSecret(ctx, c.secretGetter, recorder, c.targetNamespace, s.Name, c.targetNamespace, nameFor(s.Name, revision), ownerRefs)
 		if err != nil {
-			return err
+			return false, err
 		}
 		if obj == nil && !s.Optional {
-			return apierrors.NewNotFound(corev1.Resource("secrets"), s.Name)
+			return false, apierrors.NewNotFound(corev1.Resource("secrets"), s.Name)
 		}
 	}
 
-	return nil
+	createdStatus.Annotations["operator.openshift.io/revision-ready"] = "true"
+	if _, err := c.configMapGetter.ConfigMaps(createdStatus.Namespace).Update(ctx, createdStatus, metav1.UpdateOptions{}); err != nil {
+		return false, err
+	}
+
+	return true, nil
 }
 
 // getLatestAvailableRevision returns the latest known revision to the operator

pkg/operator/revisioncontroller/revision_controller_test.go

@@ -2,6 +2,7 @@ package revisioncontroller
 
 import (
 	"context"
+	"reflect"
 	"strings"
 	"testing"
 	"time"
@@ -27,6 +28,11 @@ func filterCreateActions(actions []clienttesting.Action) []runtime.Object {
 		if !isCreate {
 			continue
 		}
+		// Filter out Update actions as both clienttesting.CreateAction
+		// and clienttesting.UpdateAction implement the same interface
+		if reflect.TypeOf(a) == reflect.TypeOf(clienttesting.UpdateActionImpl{}) {
+			continue
+		}
 		_, isEvent := createAction.GetObject().(*v1.Event)
 		if isEvent {
 			continue
@@ -101,7 +107,7 @@ func TestRevisionController(t *testing.T) {
 			},
 			validateActions: func(t *testing.T, actions []clienttesting.Action, kclient *fake.Clientset) {
 				updatedObjects := filterUpdateActions(actions)
-				if len(updatedObjects) != 3 {
+				if len(updatedObjects) != 4 {
 					t.Errorf("expected 4 updated objects, but got %v", len(updatedObjects))
 				}
 				_, err := kclient.CoreV1().ConfigMaps(targetNamespace).Get(context.TODO(), "revision-status-2", metav1.GetOptions{})