set status for other failure modes during ClusterExtensionRevision reconciliation

Signed-off-by: Joe Lanford <[email protected]>

Author: joelanford

internal/operator-controller/applier/boxcutter.go

@@ -5,6 +5,7 @@ import (
 	"context"
 	"crypto/sha256"
 	"encoding/hex"
+	"errors"
 	"fmt"
 	"hash"
 	"io/fs"
@@ -204,11 +205,18 @@ func (bc *Boxcutter) apply(ctx context.Context, contentFS fs.FS, ext *ocv1.Clust
 	// TODO: Delete archived previous revisions over a certain revision limit
 
 	// TODO: Define constants for the ClusterExtensionRevision condition types.
-	installedCondition := meta.FindStatusCondition(currentRevision.Status.Conditions, "Succeeded")
-	if installedCondition == nil {
+	progressingCondition := meta.FindStatusCondition(currentRevision.Status.Conditions, "Progressing")
+	availableCondition := meta.FindStatusCondition(currentRevision.Status.Conditions, "Available")
+	succeededCondition := meta.FindStatusCondition(currentRevision.Status.Conditions, "Succeeded")
+
+	if progressingCondition == nil && availableCondition == nil && succeededCondition == nil {
 		return false, "New revision created", nil
-	} else if installedCondition.Status != metav1.ConditionTrue {
-		return false, installedCondition.Message, nil
+	} else if progressingCondition != nil && progressingCondition.Status == metav1.ConditionTrue {
+		return false, progressingCondition.Message, nil
+	} else if availableCondition != nil && availableCondition.Status != metav1.ConditionTrue {
+		return false, "", errors.New(availableCondition.Message)
+	} else if succeededCondition != nil && succeededCondition.Status != metav1.ConditionTrue {
+		return false, succeededCondition.Message, nil
 	}
 	return true, "", nil
 }
@@ -240,6 +248,9 @@ func computeSHA256Hash(obj any) string {
 func deepHashObject(hasher hash.Hash, objectToWrite any) {
 	hasher.Reset()
 
+	// TODO: change this out to `json.Marshal`. Pretty sure we found issues in the past where
+	//   spew would produce different output when internal structures changed without the
+	//   external public API changing.
 	printer := spew.ConfigState{
 		Indent:         " ",
 		SortKeys:       true,

internal/operator-controller/controllers/clusterextensionrevision_controller.go

@@ -5,6 +5,7 @@ package controllers
 import (
 	"context"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"strings"
 	"time"
@@ -115,20 +116,58 @@ func (c *ClusterExtensionRevisionReconciler) reconcile(ctx context.Context, rev
 	}
 	rres, err := c.RevisionEngine.Reconcile(ctx, *revision, opts...)
 	if err != nil {
-		return ctrl.Result{}, fmt.Errorf("revision reconcile: %w", err)
+		meta.SetStatusCondition(&rev.Status.Conditions, metav1.Condition{
+			Type:               "Available",
+			Status:             metav1.ConditionFalse,
+			Reason:             "ReconcileFailure",
+			Message:            err.Error(),
+			ObservedGeneration: rev.Generation,
+		})
+		return ctrl.Result{}, fmt.Errorf("revision reconcile: %w", errors.Join(err, c.Client.Status().Update(ctx, rev)))
 	}
 	l.Info("reconcile report", "report", rres.String())
 
 	// Retry failing preflight checks with a flat 10s retry.
 	// TODO: report status, backoff?
 	if verr := rres.GetValidationError(); verr != nil {
 		l.Info("preflight error, retrying after 10s", "err", verr.String())
-		return ctrl.Result{RequeueAfter: 10 * time.Second}, nil
+		meta.SetStatusCondition(&rev.Status.Conditions, metav1.Condition{
+			Type:               "Available",
+			Status:             metav1.ConditionFalse,
+			Reason:             "RevisionValidationFailure",
+			Message:            fmt.Sprintf("revision validation error: %s", verr),
+			ObservedGeneration: rev.Generation,
+		})
+		return ctrl.Result{RequeueAfter: 10 * time.Second}, c.Client.Status().Update(ctx, rev)
 	}
-	for _, pres := range rres.GetPhases() {
+	for i, pres := range rres.GetPhases() {
 		if verr := pres.GetValidationError(); verr != nil {
 			l.Info("preflight error, retrying after 10s", "err", verr.String())
-			return ctrl.Result{RequeueAfter: 10 * time.Second}, nil
+			meta.SetStatusCondition(&rev.Status.Conditions, metav1.Condition{
+				Type:               "Available",
+				Status:             metav1.ConditionFalse,
+				Reason:             "PhaseValidationError",
+				Message:            fmt.Sprintf("phase %d validation error: %s", i, verr),
+				ObservedGeneration: rev.Generation,
+			})
+			return ctrl.Result{RequeueAfter: 10 * time.Second}, c.Client.Status().Update(ctx, rev)
+		}
+		var collidingObjs []string
+		for _, ores := range pres.GetObjects() {
+			if ores.Action() == machinery.ActionCollision {
+				collidingObjs = append(collidingObjs, ores.String())
+			}
+		}
+		if len(collidingObjs) > 0 {
+			l.Info("object collision error, retrying after 10s", "collisions", collidingObjs)
+			meta.SetStatusCondition(&rev.Status.Conditions, metav1.Condition{
+				Type:               "Available",
+				Status:             metav1.ConditionFalse,
+				Reason:             "ObjectCollisions",
+				Message:            fmt.Sprintf("revision object collisions in phase %d\n%s", i, strings.Join(collidingObjs, "\n\n")),
+				ObservedGeneration: rev.Generation,
+			})
+			return ctrl.Result{RequeueAfter: 10 * time.Second}, c.Client.Status().Update(ctx, rev)
 		}
 	}
 
@@ -201,14 +240,14 @@ func (c *ClusterExtensionRevisionReconciler) reconcile(ctx context.Context, rev
 	}
 	if rres.InTransistion() {
 		meta.SetStatusCondition(&rev.Status.Conditions, metav1.Condition{
-			Type:               "InTransition",
+			Type:               "Progressing",
 			Status:             metav1.ConditionTrue,
-			Reason:             "InTransition",
+			Reason:             "Progressing",
 			Message:            "Rollout in progress.",
 			ObservedGeneration: rev.Generation,
 		})
 	} else {
-		meta.RemoveStatusCondition(&rev.Status.Conditions, "InTransition")
+		meta.RemoveStatusCondition(&rev.Status.Conditions, "Progressing")
 	}
 
 	return ctrl.Result{}, c.Client.Status().Update(ctx, rev)

internal/operator-controller/controllers/clusterextensionrevision_controller_test.go

@@ -171,7 +171,7 @@ func Test_ClusterExtensionRevisionReconciler_Reconcile_RevisionProgression(t *te
 			},
 		},
 		{
-			name: "set InTransition:True:InTransition condition while revision is transitioning",
+			name: "set Progressing:True:Progressing condition while revision is transitioning",
 			revisionResult: mockRevisionResult{
 				inTransition: true,
 			},
@@ -187,16 +187,16 @@ func Test_ClusterExtensionRevisionReconciler_Reconcile_RevisionProgression(t *te
 					Name: clusterExtensionRevisionName,
 				}, rev)
 				require.NoError(t, err)
-				cond := meta.FindStatusCondition(rev.Status.Conditions, "InTransition")
+				cond := meta.FindStatusCondition(rev.Status.Conditions, "Progressing")
 				require.NotNil(t, cond)
 				require.Equal(t, metav1.ConditionTrue, cond.Status)
-				require.Equal(t, "InTransition", cond.Reason)
+				require.Equal(t, "Progressing", cond.Reason)
 				require.Equal(t, "Rollout in progress.", cond.Message)
 				require.Equal(t, int64(1), cond.ObservedGeneration)
 			},
 		},
 		{
-			name: "remove InTransition condition once transition rollout is finished",
+			name: "remove Progressing condition once transition rollout is finished",
 			revisionResult: mockRevisionResult{
 				inTransition: false,
 			},
@@ -205,9 +205,9 @@ func Test_ClusterExtensionRevisionReconciler_Reconcile_RevisionProgression(t *te
 				rev1 := newTestClusterExtensionRevision(clusterExtensionRevisionName)
 				require.NoError(t, controllerutil.SetControllerReference(ext, rev1, testScheme))
 				meta.SetStatusCondition(&rev1.Status.Conditions, metav1.Condition{
-					Type:               "InTransition",
+					Type:               "Progressing",
 					Status:             metav1.ConditionTrue,
-					Reason:             "InTransition",
+					Reason:             "Progressing",
 					Message:            "some message",
 					ObservedGeneration: 1,
 				})
@@ -219,7 +219,7 @@ func Test_ClusterExtensionRevisionReconciler_Reconcile_RevisionProgression(t *te
 					Name: clusterExtensionRevisionName,
 				}, rev)
 				require.NoError(t, err)
-				cond := meta.FindStatusCondition(rev.Status.Conditions, "InTransition")
+				cond := meta.FindStatusCondition(rev.Status.Conditions, "Progressing")
 				require.Nil(t, cond)
 			},
 		},

test/e2e/cluster_extension_install_test.go

@@ -940,14 +940,12 @@ func TestClusterExtensionRecoversFromNoNamespaceWhenFailureFixed(t *testing.T) {
 		require.Equal(ct, ocv1.ReasonRetrying, cond.Reason)
 	}, pollDuration, pollInterval)
 
-	t.Log("By eventually failing to install the package successfully due to no namespace")
+	t.Log("By eventually reporting Installed != True")
 	require.EventuallyWithT(t, func(ct *assert.CollectT) {
 		require.NoError(ct, c.Get(context.Background(), types.NamespacedName{Name: clusterExtension.Name}, clusterExtension))
 		cond := apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1.TypeInstalled)
 		require.NotNil(ct, cond)
-		require.Equal(ct, metav1.ConditionUnknown, cond.Status)
-		require.Equal(ct, ocv1.ReasonFailed, cond.Reason)
-		require.Contains(ct, cond.Message, fmt.Sprintf("service account %q not found in namespace %q: unable to authenticate with the Kubernetes cluster.", clusterExtension.Name, clusterExtension.Name))
+		require.NotEqual(ct, metav1.ConditionTrue, cond.Status)
 	}, pollDuration, pollInterval)
 
 	t.Log("By creating the Namespace and ServiceAccount")
@@ -1066,7 +1064,10 @@ func TestClusterExtensionRecoversFromExistingDeploymentWhenFailureFixed(t *testi
 		cond := apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1.TypeInstalled)
 		require.NotNil(ct, cond)
 		require.Equal(ct, metav1.ConditionFalse, cond.Status)
-		require.Equal(ct, ocv1.ReasonFailed, cond.Reason)
+		// TODO: We probably _should_ be testing the reason here, but helm and boxcutter applier have different reasons.
+		//   Maybe we change helm to use "Absent" rather than "Failed" since the Progressing condition already captures
+		//   the failure?
+		//require.Equal(ct, ocv1.ReasonFailed, cond.Reason)
 		require.Contains(ct, cond.Message, "No bundle installed")
 	}, pollDuration, pollInterval)