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"
@@ -105,20 +106,58 @@ func (c *ClusterExtensionRevisionReconciler) reconcile(ctx context.Context, ce *
 	}
 	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)
 		}
 	}
 
@@ -191,14 +230,14 @@ func (c *ClusterExtensionRevisionReconciler) reconcile(ctx context.Context, ce *
 	}
 	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)

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")
@@ -1057,7 +1055,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)