WIP Introduce Progressing condition

pedjak · Per Goncalves da Silva · commit 10d56d57652e · 2025-11-11T19:46:54.000+01:00
diff --git a/api/v1/clusterextensionrevision_types.go b/api/v1/clusterextensionrevision_types.go
@@ -41,7 +41,9 @@ const (
 	ClusterExtensionRevisionReasonIncomplete                = "Incomplete"
 	ClusterExtensionRevisionReasonProgressing               = "Progressing"
 	ClusterExtensionRevisionReasonArchived                  = "Archived"
-	ClusterExtensionRevisionReasonRolloutInProgress         = "RolloutInProgress"
+	ClusterExtensionRevisionReasonRolloutInProgress         = "RollingOut"
+	ClusterExtensionRevisionReasonRolloutError              = "RolloutError"
+	ClusterExtensionRevisionReasonRolledOut                 = "RolledOut"
 )
 
 // ClusterExtensionRevisionSpec defines the desired state of ClusterExtensionRevision.
@@ -152,6 +154,7 @@ type ClusterExtensionRevisionStatus struct {
 
 // ClusterExtensionRevision is the Schema for the clusterextensionrevisions API
 // +kubebuilder:printcolumn:name="Available",type=string,JSONPath=`.status.conditions[?(@.type=='Available')].status`
+// +kubebuilder:printcolumn:name="Progressing",type=string,JSONPath=`.status.conditions[?(@.type=='Progressing')].status`
 // +kubebuilder:printcolumn:name=Age,type=date,JSONPath=`.metadata.creationTimestamp`
 type ClusterExtensionRevision struct {
 	metav1.TypeMeta   `json:",inline"`
diff --git a/helm/olmv1/base/operator-controller/crd/experimental/olm.operatorframework.io_clusterextensionrevisions.yaml b/helm/olmv1/base/operator-controller/crd/experimental/olm.operatorframework.io_clusterextensionrevisions.yaml
@@ -19,6 +19,9 @@ spec:
     - jsonPath: .status.conditions[?(@.type=='Available')].status
       name: Available
       type: string
+    - jsonPath: .status.conditions[?(@.type=='Progressing')].status
+      name: Progressing
+      type: string
     - jsonPath: .metadata.creationTimestamp
       name: Age
       type: date
diff --git a/internal/operator-controller/controllers/clusterextensionrevision_controller.go b/internal/operator-controller/controllers/clusterextensionrevision_controller.go
@@ -126,25 +126,29 @@ func (c *ClusterExtensionRevisionReconciler) reconcile(ctx context.Context, rev
 	// Reconcile
 	//
 	if err := c.ensureFinalizer(ctx, rev, clusterExtensionRevisionTeardownFinalizer); err != nil {
-		meta.SetStatusCondition(&rev.Status.Conditions, metav1.Condition{
-			Type:               ocv1.ClusterExtensionRevisionTypeAvailable,
-			Status:             metav1.ConditionFalse,
-			Reason:             ocv1.ClusterExtensionRevisionReasonReconcileFailure,
-			Message:            err.Error(),
-			ObservedGeneration: rev.Generation,
-		})
 		return ctrl.Result{}, fmt.Errorf("error ensuring teardown finalizer: %v", err)
 	}
 
-	if err := c.establishWatch(ctx, rev, revision); err != nil {
+	inRollout := meta.FindStatusCondition(rev.Status.Conditions, ocv1.ClusterExtensionRevisionTypeAvailable) == nil
+	if inRollout {
+		if err := c.establishWatch(ctx, rev, revision); err != nil {
+			werr := fmt.Errorf("establish watch: %v", err)
+			meta.SetStatusCondition(&rev.Status.Conditions, metav1.Condition{
+				Type:               ocv1.ClusterExtensionRevisionTypeProgressing,
+				Status:             metav1.ConditionTrue,
+				Reason:             ocv1.ClusterExtensionRevisionReasonReconcileFailure,
+				Message:            werr.Error(),
+				ObservedGeneration: rev.Generation,
+			})
+			return ctrl.Result{}, werr
+		}
 		meta.SetStatusCondition(&rev.Status.Conditions, metav1.Condition{
-			Type:               ocv1.ClusterExtensionRevisionTypeAvailable,
-			Status:             metav1.ConditionFalse,
-			Reason:             ocv1.ClusterExtensionRevisionReasonReconcileFailure,
-			Message:            err.Error(),
+			Type:               ocv1.ClusterExtensionRevisionTypeProgressing,
+			Status:             metav1.ConditionTrue,
+			Reason:             ocv1.ClusterExtensionRevisionReasonRolloutInProgress,
+			Message:            "Revision is being rolled out.",
 			ObservedGeneration: rev.Generation,
 		})
-		return ctrl.Result{}, fmt.Errorf("establish watch: %v", err)
 	}
 
 	rres, err := c.RevisionEngine.Reconcile(ctx, *revision, opts...)
@@ -154,13 +158,24 @@ func (c *ClusterExtensionRevisionReconciler) reconcile(ctx context.Context, rev
 		} else {
 			l.Error(err, "revision reconcile failed")
 		}
-		meta.SetStatusCondition(&rev.Status.Conditions, metav1.Condition{
-			Type:               ocv1.ClusterExtensionRevisionTypeAvailable,
-			Status:             metav1.ConditionFalse,
-			Reason:             ocv1.ClusterExtensionRevisionReasonReconcileFailure,
-			Message:            err.Error(),
-			ObservedGeneration: rev.Generation,
-		})
+		if inRollout {
+			meta.SetStatusCondition(&rev.Status.Conditions, metav1.Condition{
+				Type:               ocv1.ClusterExtensionRevisionTypeProgressing,
+				Status:             metav1.ConditionTrue,
+				Reason:             ocv1.ClusterExtensionRevisionReasonRolloutError,
+				Message:            err.Error(),
+				ObservedGeneration: rev.Generation,
+			})
+		} else {
+			// it is a probably transient error, and we do not know if the revision is available or not
+			meta.SetStatusCondition(&rev.Status.Conditions, metav1.Condition{
+				Type:               ocv1.ClusterExtensionRevisionTypeAvailable,
+				Status:             metav1.ConditionUnknown,
+				Reason:             ocv1.ClusterExtensionRevisionReasonReconcileFailure,
+				Message:            err.Error(),
+				ObservedGeneration: rev.Generation,
+			})
+		}
 		return ctrl.Result{}, fmt.Errorf("revision reconcile: %v", err)
 	}
 	// Log detailed reconcile reports only in debug mode (V(1)) to reduce verbosity.
@@ -171,32 +186,51 @@ func (c *ClusterExtensionRevisionReconciler) reconcile(ctx context.Context, rev
 	if verr := rres.GetValidationError(); verr != nil {
 		l.Error(fmt.Errorf("%w", verr), "preflight validation failed, retrying after 10s")
 
-		// if we take Availability as strictly being "all availability probes pass"
-		// we should probably be at an Unknown state (or not posted it at all here)
-		// and post this up in the Progressing condition as False with a failed rollout reason / message
-		meta.SetStatusCondition(&rev.Status.Conditions, metav1.Condition{
-			Type:               ocv1.ClusterExtensionRevisionTypeAvailable,
-			Status:             metav1.ConditionFalse,
-			Reason:             ocv1.ClusterExtensionRevisionReasonRevisionValidationFailure,
-			Message:            fmt.Sprintf("revision validation error: %s", verr),
-			ObservedGeneration: rev.Generation,
-		})
+		if inRollout {
+			// given that we retry, we are not going to keep Progressing condition True
+			meta.SetStatusCondition(&rev.Status.Conditions, metav1.Condition{
+				Type:               ocv1.ClusterExtensionRevisionTypeProgressing,
+				Status:             metav1.ConditionTrue,
+				Reason:             ocv1.ClusterExtensionRevisionReasonRevisionValidationFailure,
+				Message:            fmt.Sprintf("revision validation error: %s", verr),
+				ObservedGeneration: rev.Generation,
+			})
+		} else {
+			// it is a probably transient error, and we do not know if the revision is available or not
+			meta.SetStatusCondition(&rev.Status.Conditions, metav1.Condition{
+				Type:               ocv1.ClusterExtensionRevisionTypeAvailable,
+				Status:             metav1.ConditionUnknown,
+				Reason:             ocv1.ClusterExtensionRevisionReasonReconcileFailure,
+				Message:            fmt.Sprintf("revision validation error: %s", verr),
+				ObservedGeneration: rev.Generation,
+			})
+		}
 		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)
 
-			// we probably want to either eat this error and leave Progressing as True with a rolling out reason and implement timeout
-			// or surface this error through a Degraded-type condition that can flap as roll out continues
-			meta.SetStatusCondition(&rev.Status.Conditions, metav1.Condition{
-				Type:               ocv1.ClusterExtensionRevisionTypeAvailable,
-				Status:             metav1.ConditionFalse,
-				Reason:             ocv1.ClusterExtensionRevisionReasonPhaseValidationError,
-				Message:            fmt.Sprintf("phase %d validation error: %s", i, verr),
-				ObservedGeneration: rev.Generation,
-			})
+			if inRollout {
+				// given that we retry, we are not going to keep Progressing condition True
+				meta.SetStatusCondition(&rev.Status.Conditions, metav1.Condition{
+					Type:               ocv1.ClusterExtensionRevisionTypeProgressing,
+					Status:             metav1.ConditionTrue,
+					Reason:             ocv1.ClusterExtensionRevisionReasonPhaseValidationError,
+					Message:            fmt.Sprintf("phase %d validation error: %s", i, verr),
+					ObservedGeneration: rev.Generation,
+				})
+			} else {
+				// it is a probably transient error, and we do not know if the revision is available or not
+				meta.SetStatusCondition(&rev.Status.Conditions, metav1.Condition{
+					Type:               ocv1.ClusterExtensionRevisionTypeAvailable,
+					Status:             metav1.ConditionUnknown,
+					Reason:             ocv1.ClusterExtensionRevisionReasonPhaseValidationError,
+					Message:            fmt.Sprintf("phase %d validation error: %s", i, verr),
+					ObservedGeneration: rev.Generation,
+				})
+			}
 			return ctrl.Result{RequeueAfter: 10 * time.Second}, nil
 		}
 
@@ -211,19 +245,32 @@ func (c *ClusterExtensionRevisionReconciler) reconcile(ctx context.Context, rev
 			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
-			meta.SetStatusCondition(&rev.Status.Conditions, metav1.Condition{
-				Type:               ocv1.ClusterExtensionRevisionTypeAvailable,
-				Status:             metav1.ConditionFalse,
-				Reason:             ocv1.ClusterExtensionRevisionReasonObjectCollisions,
-				Message:            fmt.Sprintf("revision object collisions in phase %d\n%s", i, strings.Join(collidingObjs, "\n\n")),
-				ObservedGeneration: rev.Generation,
-			})
-
-			// idk if we want to return yet or check the Availability probes on a best-effort basis
-			return ctrl.Result{RequeueAfter: 10 * time.Second}, nil
+			if inRollout {
+				meta.SetStatusCondition(&rev.Status.Conditions, metav1.Condition{
+					Type:               ocv1.ClusterExtensionRevisionTypeProgressing,
+					Status:             metav1.ConditionFalse,
+					Reason:             ocv1.ClusterExtensionRevisionReasonObjectCollisions,
+					Message:            fmt.Sprintf("revision object collisions in phase %d\n%s", i, strings.Join(collidingObjs, "\n\n")),
+					ObservedGeneration: rev.Generation,
+				})
+
+				// not sure if we want to retry here - collisions are probably not transient?
+				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
+		meta.SetStatusCondition(&rev.Status.Conditions, metav1.Condition{
+			Type:               ocv1.ClusterExtensionRevisionTypeProgressing,
+			Status:             metav1.ConditionFalse,
+			Reason:             ocv1.ClusterExtensionRevisionReasonRolledOut,
+			Message:            "Revision is rolled out.",
+			ObservedGeneration: rev.Generation,
+		})
+	}
+
 	//nolint:nestif
 	if rres.IsComplete() {
 		// Archive other revisions.
@@ -237,30 +284,26 @@ func (c *ClusterExtensionRevisionReconciler) reconcile(ctx context.Context, rev
 			}
 		}
 
-		// Report status.
-		// We probably want to set Progressing to False here with a rollout success reason and message
 		// 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
 		meta.SetStatusCondition(&rev.Status.Conditions, metav1.Condition{
 			Type:               ocv1.ClusterExtensionRevisionTypeAvailable,
 			Status:             metav1.ConditionTrue,
 			Reason:             ocv1.ClusterExtensionRevisionReasonAvailable,
-			Message:            "Object is available and passes all probes.",
+			Message:            "Objects are available and passes all probes.",
 			ObservedGeneration: rev.Generation,
 		})
 
 		// We'll probably only want to remove this once we are done updating the ClusterExtension conditions
 		// as its one of the interfaces between the revision and the extension. If we still have the Succeeded for now
 		// that's fine.
-		if !meta.IsStatusConditionTrue(rev.Status.Conditions, ocv1.ClusterExtensionRevisionTypeSucceeded) {
-			meta.SetStatusCondition(&rev.Status.Conditions, metav1.Condition{
-				Type:               ocv1.ClusterExtensionRevisionTypeSucceeded,
-				Status:             metav1.ConditionTrue,
-				Reason:             ocv1.ClusterExtensionRevisionReasonRolloutSuccess,
-				Message:            "Revision succeeded rolling out.",
-				ObservedGeneration: rev.Generation,
-			})
-		}
+		meta.SetStatusCondition(&rev.Status.Conditions, metav1.Condition{
+			Type:               ocv1.ClusterExtensionRevisionTypeSucceeded,
+			Status:             metav1.ConditionTrue,
+			Reason:             ocv1.ClusterExtensionRevisionReasonRolloutSuccess,
+			Message:            "Revision succeeded rolling out.",
+			ObservedGeneration: rev.Generation,
+		})
 	} else {
 		var probeFailureMsgs []string
 		for _, pres := range rres.GetPhases() {
@@ -296,7 +339,6 @@ func (c *ClusterExtensionRevisionReconciler) reconcile(ctx context.Context, rev
 				ObservedGeneration: rev.Generation,
 			})
 		} else {
-			// either to nothing or set the Progressing condition to True with rolling out reason / message
 			meta.SetStatusCondition(&rev.Status.Conditions, metav1.Condition{
 				Type:               ocv1.ClusterExtensionRevisionTypeAvailable,
 				Status:             metav1.ConditionFalse,
@@ -306,19 +348,6 @@ func (c *ClusterExtensionRevisionReconciler) reconcile(ctx context.Context, rev
 			})
 		}
 	}
-	if rres.InTransistion() {
-		// seems to be the right thing XD
-		meta.SetStatusCondition(&rev.Status.Conditions, metav1.Condition{
-			Type:               ocv1.TypeProgressing,
-			Status:             metav1.ConditionTrue,
-			Reason:             ocv1.ClusterExtensionRevisionReasonProgressing,
-			Message:            "Rollout in progress.",
-			ObservedGeneration: rev.Generation,
-		})
-	} else {
-		// we may not want to remove it but rather set it to False?
-		meta.RemoveStatusCondition(&rev.Status.Conditions, ocv1.TypeProgressing)
-	}
 
 	return ctrl.Result{}, nil
 }
diff --git a/manifests/experimental-e2e.yaml b/manifests/experimental-e2e.yaml
@@ -610,6 +610,9 @@ spec:
     - jsonPath: .status.conditions[?(@.type=='Available')].status
       name: Available
       type: string
+    - jsonPath: .status.conditions[?(@.type=='Progressing')].status
+      name: Progressing
+      type: string
     - jsonPath: .metadata.creationTimestamp
       name: Age
       type: date
diff --git a/manifests/experimental.yaml b/manifests/experimental.yaml
@@ -575,6 +575,9 @@ spec:
     - jsonPath: .status.conditions[?(@.type=='Available')].status
       name: Available
       type: string
+    - jsonPath: .status.conditions[?(@.type=='Progressing')].status
+      name: Progressing
+      type: string
     - jsonPath: .metadata.creationTimestamp
       name: Age
       type: date
