Surface revision retrying condition to ClusterExtension

Signed-off-by: Per Goncalves da Silva <[email protected]>

Author: Per Goncalves da Silva

internal/operator-controller/applier/boxcutter.go

@@ -398,6 +398,9 @@ func (bc *Boxcutter) apply(ctx context.Context, contentFS fs.FS, ext *ocv1.Clust
 	if progressingCondition == nil && availableCondition == nil && succeededCondition == nil {
 		return false, "New revision created", nil
 	} else if progressingCondition != nil && progressingCondition.Status == metav1.ConditionTrue {
+		if progressingCondition.Reason == ocv1.ClusterExtensionRevisionReasonRetrying {
+			return false, "", errors.New(progressingCondition.Message)
+		}
 		return false, progressingCondition.Message, nil
 	} else if availableCondition != nil && availableCondition.Status != metav1.ConditionTrue {
 		return false, "", errors.New(availableCondition.Message)

internal/operator-controller/controllers/clusterextensionrevision_controller.go

@@ -118,13 +118,7 @@ func (c *ClusterExtensionRevisionReconciler) reconcile(ctx context.Context, rev
 
 	revision, opts, err := c.toBoxcutterRevision(ctx, rev)
 	if err != nil {
-		meta.SetStatusCondition(&rev.Status.Conditions, metav1.Condition{
-			Type:               ocv1.ClusterExtensionRevisionTypeAvailable,
-			Status:             metav1.ConditionFalse,
-			Reason:             ocv1.ClusterExtensionRevisionReasonReconciling,
-			Message:            err.Error(),
-			ObservedGeneration: rev.Generation,
-		})
+		setRetryingConditions(rev, err.Error())
 		return ctrl.Result{}, fmt.Errorf("converting to boxcutter revision: %v", err)
 	}
 
@@ -137,16 +131,15 @@ func (c *ClusterExtensionRevisionReconciler) reconcile(ctx context.Context, rev
 	// Reconcile
 	//
 	if err := c.ensureFinalizer(ctx, rev, clusterExtensionRevisionTeardownFinalizer); err != nil {
+		l.Error(err, "failed to ensure finalizer")
 		return ctrl.Result{}, fmt.Errorf("error ensuring teardown finalizer: %v", err)
 	}
 
 	if err := c.establishWatch(ctx, rev, revision); err != nil {
 		werr := fmt.Errorf("establish watch: %v", err)
-		// this error is very likely transient, so we should keep revision as progressing
-		markAsProgressing(rev, ocv1.ClusterExtensionRevisionReasonRetrying, werr.Error())
+		setRetryingConditions(rev, werr.Error())
 		return ctrl.Result{}, werr
 	}
-	markAsProgressing(rev, ocv1.ClusterExtensionRevisionReasonRollingOut, fmt.Sprintf("Revision %s is being rolled out.", revVersion))
 
 	rres, err := c.RevisionEngine.Reconcile(ctx, *revision, opts...)
 	if err != nil {
@@ -155,18 +148,7 @@ func (c *ClusterExtensionRevisionReconciler) reconcile(ctx context.Context, rev
 		} else {
 			l.Error(err, "revision reconcile failed")
 		}
-		markAsProgressing(rev, ocv1.ClusterExtensionRevisionReasonRetrying, err.Error())
-		if meta.FindStatusCondition(rev.Status.Conditions, ocv1.ClusterExtensionRevisionTypeAvailable) != nil {
-			// it is a probably transient error, and we do not know if the revision is available or not
-			// perhaps we should not report it at all, hoping that it is going to be mitigated in the next reconcile?
-			meta.SetStatusCondition(&rev.Status.Conditions, metav1.Condition{
-				Type:               ocv1.ClusterExtensionRevisionTypeAvailable,
-				Status:             metav1.ConditionUnknown,
-				Reason:             ocv1.ClusterExtensionRevisionReasonReconciling,
-				Message:            err.Error(),
-				ObservedGeneration: rev.Generation,
-			})
-		}
+		setRetryingConditions(rev, err.Error())
 		return ctrl.Result{}, fmt.Errorf("revision reconcile: %v", err)
 	}
 	// Log detailed reconcile reports only in debug mode (V(1)) to reduce verbosity.
@@ -176,37 +158,14 @@ func (c *ClusterExtensionRevisionReconciler) reconcile(ctx context.Context, rev
 	// TODO: report status, backoff?
 	if verr := rres.GetValidationError(); verr != nil {
 		l.Error(fmt.Errorf("%w", verr), "preflight validation failed, retrying after 10s")
-		markAsProgressing(rev, ocv1.ClusterExtensionRevisionReasonRetrying, fmt.Sprintf("revision validation error: %s", verr))
-		if meta.FindStatusCondition(rev.Status.Conditions, ocv1.ClusterExtensionRevisionTypeAvailable) != nil {
-			// it is a probably transient error, and we do not know if the revision is available or not
-			// perhaps we should not report it at all, hoping that it is going to be mitigated in the next reconcile?
-			meta.SetStatusCondition(&rev.Status.Conditions, metav1.Condition{
-				Type:               ocv1.ClusterExtensionRevisionTypeAvailable,
-				Status:             metav1.ConditionUnknown,
-				Reason:             ocv1.ClusterExtensionRevisionReasonReconciling,
-				Message:            fmt.Sprintf("revision validation error: %s", verr),
-				ObservedGeneration: rev.Generation,
-			})
-		}
+		setRetryingConditions(rev, fmt.Sprintf("revision validation error: %s", verr))
 		return ctrl.Result{RequeueAfter: 10 * time.Second}, nil
 	}
 
 	for i, pres := range rres.GetPhases() {
 		if verr := pres.GetValidationError(); verr != nil {
 			l.Error(fmt.Errorf("%w", verr), "phase preflight validation failed, retrying after 10s", "phase", i)
-
-			markAsProgressing(rev, ocv1.ClusterExtensionRevisionReasonRetrying, fmt.Sprintf("phase %d validation error: %s", i, verr))
-			if meta.FindStatusCondition(rev.Status.Conditions, ocv1.ClusterExtensionRevisionTypeAvailable) != nil {
-				// it is a probably transient error, and we do not know if the revision is available or not
-				// perhaps we should not report it at all, hoping that it is going to be mitigated in the next reconcile?
-				meta.SetStatusCondition(&rev.Status.Conditions, metav1.Condition{
-					Type:               ocv1.ClusterExtensionRevisionTypeAvailable,
-					Status:             metav1.ConditionUnknown,
-					Reason:             ocv1.ClusterExtensionRevisionReasonReconciling,
-					Message:            fmt.Sprintf("phase %d validation error: %s", i, verr),
-					ObservedGeneration: rev.Generation,
-				})
-			}
+			setRetryingConditions(rev, fmt.Sprintf("phase %d validation error: %s", i, verr))
 			return ctrl.Result{RequeueAfter: 10 * time.Second}, nil
 		}
 
@@ -219,18 +178,15 @@ func (c *ClusterExtensionRevisionReconciler) reconcile(ctx context.Context, rev
 
 		if len(collidingObjs) > 0 {
 			l.Error(fmt.Errorf("object collision detected"), "object collision, retrying after 10s", "phase", i, "collisions", collidingObjs)
-			// collisions are probably stickier than phase roll out probe failures - so we'd probably want to set
-			// Progressing to false here due to the collision
-			markAsNotProgressing(rev, ocv1.ClusterExtensionRevisionReasonRetrying, fmt.Sprintf("revision object collisions in phase %d\n%s", i, strings.Join(collidingObjs, "\n\n")))
-
-			// NOTE(pedjak): not sure if we want to retry here - collisions are probably not transient?
+			setRetryingConditions(rev, fmt.Sprintf("revision object collisions in phase %d\n%s", i, strings.Join(collidingObjs, "\n\n")))
 			return ctrl.Result{RequeueAfter: 10 * time.Second}, nil
 		}
 	}
 
 	if !rres.InTransistion() {
-		// we have rolled out all objects in all phases, not interested in probes here
-		markAsNotProgressing(rev, ocv1.ClusterExtensionRevisionReasonRolledOut, fmt.Sprintf("Revision %s is rolled out.", revVersion))
+		markAsNotProgressing(rev, ocv1.ClusterExtensionRevisionReasonRolledOut, fmt.Sprintf("Revision %s has rolled out.", revVersion))
+	} else {
+		markAsProgressing(rev, ocv1.ClusterExtensionRevisionReasonRollingOut, fmt.Sprintf("Revision %s is rolling out.", revVersion))
 	}
 
 	//nolint:nestif
@@ -246,12 +202,12 @@ func (c *ClusterExtensionRevisionReconciler) reconcile(ctx context.Context, rev
 				// TODO: It feels like an error here needs to propagate to a status _somewhere_.
 				//   Not sure the current CER makes sense? But it also feels off to set the CE
 				//   status from outside the CE reconciler.
+				l.Error(err, "failed to patch previous revision", "revision", a.GetName())
+				setRetryingConditions(rev, fmt.Sprintf("error archiving previous revision: %s", err))
 				return ctrl.Result{}, fmt.Errorf("archive previous Revision: %w", err)
 			}
 		}
 
-		// It would be good to understand from Nico how we can distinguish between progression and availability probes
-		// and how to best check that all Availability probes are passing
 		markAsAvailable(rev, ocv1.ClusterExtensionRevisionReasonProbesSucceeded, "Objects are available and pass all probes.")
 
 		// We'll probably only want to remove this once we are done updating the ClusterExtension conditions
@@ -290,10 +246,11 @@ func (c *ClusterExtensionRevisionReconciler) reconcile(ctx context.Context, rev
 				break
 			}
 		}
+
 		if len(probeFailureMsgs) > 0 {
 			markAsUnavailable(rev, ocv1.ClusterExtensionRevisionReasonProbeFailure, strings.Join(probeFailureMsgs, "\n"))
 		} else {
-			markAsUnavailable(rev, ocv1.ClusterExtensionRevisionReasonReconciling, fmt.Sprintf("Revision %s has not been rolled out completely.", revVersion))
+			markAsUnavailable(rev, ocv1.ClusterExtensionRevisionReasonReconciling, fmt.Sprintf("Revision %s is rolling out.", revVersion))
 		}
 	}
 
@@ -310,13 +267,7 @@ func (c *ClusterExtensionRevisionReconciler) teardown(ctx context.Context, rev *
 		} else {
 			l.Error(err, "revision teardown failed")
 		}
-		meta.SetStatusCondition(&rev.Status.Conditions, metav1.Condition{
-			Type:               ocv1.ClusterExtensionRevisionTypeAvailable,
-			Status:             metav1.ConditionFalse,
-			Reason:             ocv1.ClusterExtensionRevisionReasonReconciling,
-			Message:            err.Error(),
-			ObservedGeneration: rev.Generation,
-		})
+		setRetryingConditions(rev, err.Error())
 		return ctrl.Result{}, fmt.Errorf("revision teardown: %v", err)
 	}
 
@@ -330,36 +281,13 @@ func (c *ClusterExtensionRevisionReconciler) teardown(ctx context.Context, rev *
 	}
 
 	if err := c.TrackingCache.Free(ctx, rev); err != nil {
-		meta.SetStatusCondition(&rev.Status.Conditions, metav1.Condition{
-			Type:               ocv1.ClusterExtensionRevisionTypeAvailable,
-			Status:             metav1.ConditionFalse,
-			Reason:             ocv1.ClusterExtensionRevisionReasonReconciling,
-			Message:            err.Error(),
-			ObservedGeneration: rev.Generation,
-		})
+		setRetryingConditions(rev, err.Error())
 		return ctrl.Result{}, fmt.Errorf("error stopping informers: %v", err)
 	}
 
 	// Ensure conditions are set before removing the finalizer when archiving
-	if rev.Spec.LifecycleState == ocv1.ClusterExtensionRevisionLifecycleStateArchived {
-		condUpdated := meta.SetStatusCondition(&rev.Status.Conditions, metav1.Condition{
-			Type:               ocv1.ClusterExtensionRevisionTypeAvailable,
-			Status:             metav1.ConditionUnknown,
-			Reason:             ocv1.ClusterExtensionRevisionReasonArchived,
-			Message:            "revision is archived",
-			ObservedGeneration: rev.Generation,
-		})
-		condUpdated = meta.SetStatusCondition(&rev.Status.Conditions, metav1.Condition{
-			Type:               ocv1.ClusterExtensionRevisionTypeProgressing,
-			Status:             metav1.ConditionFalse,
-			Reason:             ocv1.ClusterExtensionRevisionReasonArchived,
-			Message:            "revision is archived",
-			ObservedGeneration: rev.Generation,
-		}) || condUpdated
-
-		if condUpdated {
-			return ctrl.Result{}, nil
-		}
+	if rev.Spec.LifecycleState == ocv1.ClusterExtensionRevisionLifecycleStateArchived && setArchivedConditions(rev) {
+		return ctrl.Result{}, nil
 	}
 
 	if err := c.removeFinalizer(ctx, rev, clusterExtensionRevisionTeardownFinalizer); err != nil {
@@ -458,7 +386,7 @@ func (c *ClusterExtensionRevisionReconciler) listPreviousRevisions(ctx context.C
 	}
 
 	revList := &ocv1.ClusterExtensionRevisionList{}
-	if err := c.TrackingCache.List(ctx, revList, client.MatchingLabels{
+	if err := c.Client.List(ctx, revList, client.MatchingLabels{
 		ClusterExtensionRevisionOwnerLabel: ownerLabel,
 	}); err != nil {
 		return nil, fmt.Errorf("listing revisions: %w", err)
@@ -599,6 +527,11 @@ var (
 	}
 )
 
+func setRetryingConditions(cer *ocv1.ClusterExtensionRevision, message string) {
+	markAsProgressing(cer, ocv1.ClusterExtensionRevisionReasonRetrying, message)
+	markAsAvailableUnknown(cer, ocv1.ClusterExtensionRevisionReasonReconciling, message)
+}
+
 func markAsProgressing(cer *ocv1.ClusterExtensionRevision, reason, message string) {
 	meta.SetStatusCondition(&cer.Status.Conditions, metav1.Condition{
 		Type:               ocv1.ClusterExtensionRevisionTypeProgressing,
@@ -609,8 +542,8 @@ func markAsProgressing(cer *ocv1.ClusterExtensionRevision, reason, message strin
 	})
 }
 
-func markAsNotProgressing(cer *ocv1.ClusterExtensionRevision, reason, message string) {
-	meta.SetStatusCondition(&cer.Status.Conditions, metav1.Condition{
+func markAsNotProgressing(cer *ocv1.ClusterExtensionRevision, reason, message string) bool {
+	return meta.SetStatusCondition(&cer.Status.Conditions, metav1.Condition{
 		Type:               ocv1.ClusterExtensionRevisionTypeProgressing,
 		Status:             metav1.ConditionFalse,
 		Reason:             reason,
@@ -619,8 +552,8 @@ func markAsNotProgressing(cer *ocv1.ClusterExtensionRevision, reason, message st
 	})
 }
 
-func markAsAvailable(cer *ocv1.ClusterExtensionRevision, reason, message string) {
-	meta.SetStatusCondition(&cer.Status.Conditions, metav1.Condition{
+func markAsAvailable(cer *ocv1.ClusterExtensionRevision, reason, message string) bool {
+	return meta.SetStatusCondition(&cer.Status.Conditions, metav1.Condition{
 		Type:               ocv1.ClusterExtensionRevisionTypeAvailable,
 		Status:             metav1.ConditionTrue,
 		Reason:             reason,
@@ -638,3 +571,19 @@ func markAsUnavailable(cer *ocv1.ClusterExtensionRevision, reason, message strin
 		ObservedGeneration: cer.Generation,
 	})
 }
+
+func markAsAvailableUnknown(cer *ocv1.ClusterExtensionRevision, reason, message string) bool {
+	return meta.SetStatusCondition(&cer.Status.Conditions, metav1.Condition{
+		Type:               ocv1.ClusterExtensionRevisionTypeAvailable,
+		Status:             metav1.ConditionUnknown,
+		Reason:             reason,
+		Message:            message,
+		ObservedGeneration: cer.Generation,
+	})
+}
+
+func setArchivedConditions(cer *ocv1.ClusterExtensionRevision) bool {
+	const msg = "revision is archived"
+	updated := markAsNotProgressing(cer, ocv1.ClusterExtensionRevisionReasonArchived, msg)
+	return markAsAvailableUnknown(cer, ocv1.ClusterExtensionRevisionReasonArchived, msg) || updated
+}