fix: Allow Retrying status while preventing RollingOut loop

Generated-by: Claude

Author: camilamacedo86

internal/operator-controller/controllers/clusterextension_controller_test.go

@@ -785,7 +785,9 @@ func TestClusterExtensionBoxcutterApplierFailsDoesNotLeakDeprecationErrors(t *te
 		// the same inputs the runtime would see.
 		d.RevisionStatesGetter = &MockRevisionStatesGetter{
 			RevisionStates: &controllers.RevisionStates{
-				RollingOut: []*controllers.RevisionMetadata{{}},
+				RollingOut: []*controllers.RevisionMetadata{{
+					BundleMetadata: ocv1.BundleMetadata{Version: "1.0.0"},
+				}},
 			},
 		}
 		d.Resolver = resolve.Func(func(ctx context.Context, ext *ocv1.ClusterExtension, installedBundle *ocv1.BundleMetadata) (*declcfg.Bundle, *bundle.VersionRelease, *declcfg.Deprecation, error) {

internal/operator-controller/controllers/clusterextension_reconcile_steps.go

@@ -140,15 +140,28 @@ func ResolveBundle(r resolve.Resolver, c client.Client) ReconcileStepFunc {
 	return func(ctx context.Context, state *reconcileState, ext *ocv1.ClusterExtension) (*ctrl.Result, error) {
 		l := log.FromContext(ctx)
 
-		// If already rolling out, use existing revision and set deprecation to Unknown (no catalog check)
+		// If already rolling out, check if the rolling-out revision matches the current spec.
+		// If spec has changed (e.g., version updated), we need to resolve a new bundle instead of
+		// reusing the rolling-out revision.
 		if len(state.revisionStates.RollingOut) > 0 {
-			installedBundleName := ""
-			if state.revisionStates.Installed != nil {
-				installedBundleName = state.revisionStates.Installed.Name
+			rollingOutRevision := state.revisionStates.RollingOut[0]
+			specVersion := ""
+			if ext.Spec.Source.Catalog != nil {
+				specVersion = ext.Spec.Source.Catalog.Version
 			}
-			SetDeprecationStatus(ext, installedBundleName, nil, false)
-			state.resolvedRevisionMetadata = state.revisionStates.RollingOut[0]
-			return nil, nil
+
+			// If spec version matches rolling-out revision version (or no version specified),
+			// reuse the existing rolling-out revision
+			if specVersion == "" || specVersion == rollingOutRevision.Version {
+				installedBundleName := ""
+				if state.revisionStates.Installed != nil {
+					installedBundleName = state.revisionStates.Installed.Name
+				}
+				SetDeprecationStatus(ext, installedBundleName, nil, false)
+				state.resolvedRevisionMetadata = rollingOutRevision
+				return nil, nil
+			}
+			// Spec version changed - fall through to resolve new bundle from catalog
 		}
 
 		// Resolve a new bundle from the catalog

internal/operator-controller/controllers/clusterobjectset_controller.go

@@ -672,27 +672,27 @@ func setRetryingConditions(cos *ocv1.ClusterObjectSet, message string) {
 // markAsProgressing sets the Progressing condition to True with the given reason.
 //
 // Once ProgressDeadlineExceeded has been set for the current generation, this function
-// keeps that condition sticky for non-terminal progressing reasons that would otherwise
-// cause the reconciler to oscillate between Progressing=True and
-// ProgressDeadlineExceeded=False for the same generation.
+// blocks only the RollingOut reason to prevent a reconcile loop for Active revisions.
+// The loop would occur when reconcile() sees in-transition objects and sets
+// Progressing=True/RollingOut, then the progress deadline check immediately resets it
+// to ProgressDeadlineExceeded, causing a status-only update that triggers another reconcile.
 //
-// In particular, both RollingOut and Retrying are blocked for Active revisions.
-// Allowing those reasons to overwrite ProgressDeadlineExceeded can trigger unnecessary
-// status-only reconciles and interfere with retry/requeue behavior due to the progress
-// deadline check resetting the condition.
+// Archived revisions are exempt because they skip progress deadline enforcement entirely,
+// so their status updates won't be overridden.
 //
-// Archived revisions are exempt from this guard because the progress deadline check
-// does not apply to them, allowing archival teardown retries to show Retrying status.
+// Other reasons like Succeeded and Retrying are always allowed because:
+// - Succeeded represents terminal success (rollout eventually completed)
+// - Retrying represents transient errors that need to be visible in status
 //
-// If the generation changed (spec update) since ProgressDeadlineExceeded was recorded,
-// the guard allows the update through so the condition reflects the new generation.
+// If the generation changed since ProgressDeadlineExceeded was recorded, the guard
+// allows all updates through so the condition reflects the new generation.
 func markAsProgressing(cos *ocv1.ClusterObjectSet, reason, message string) {
 	if cnd := meta.FindStatusCondition(cos.Status.Conditions, ocv1.ClusterObjectSetTypeProgressing); cnd != nil &&
 		cnd.Status == metav1.ConditionFalse && cnd.Reason == ocv1.ReasonProgressDeadlineExceeded &&
 		cnd.ObservedGeneration == cos.Generation &&
-		(reason == ocv1.ReasonRollingOut || reason == ocv1.ClusterObjectSetReasonRetrying) &&
+		reason == ocv1.ReasonRollingOut &&
 		cos.Spec.LifecycleState != ocv1.ClusterObjectSetLifecycleStateArchived {
-		log.Log.V(1).Info("skipping markAsProgressing: ProgressDeadlineExceeded is sticky for non-terminal progressing reasons",
+		log.Log.V(1).Info("skipping markAsProgressing: ProgressDeadlineExceeded is sticky for RollingOut on Active revisions",
 			"requestedReason", reason, "revision", cos.Name, "lifecycleState", cos.Spec.LifecycleState)
 		return
 	}

internal/operator-controller/controllers/clusterobjectset_controller_internal_test.go

@@ -300,7 +300,7 @@ func Test_markAsProgressing_doesNotOverwriteProgressDeadlineExceeded(t *testing.
 			expectReason:      ocv1.ReasonSucceeded,
 		},
 		{
-			name:       "does not overwrite ProgressDeadlineExceeded with Retrying (same generation)",
+			name:       "allows Retrying to overwrite ProgressDeadlineExceeded (same generation)",
 			generation: 1,
 			existingConditions: []metav1.Condition{
 				{
@@ -311,8 +311,8 @@ func Test_markAsProgressing_doesNotOverwriteProgressDeadlineExceeded(t *testing.
 				},
 			},
 			reason:            ocv1.ClusterObjectSetReasonRetrying,
-			expectProgressing: metav1.ConditionFalse,
-			expectReason:      ocv1.ReasonProgressDeadlineExceeded,
+			expectProgressing: metav1.ConditionTrue,
+			expectReason:      ocv1.ClusterObjectSetReasonRetrying,
 		},
 		{
 			name:       "allows RollingOut when generation changed since ProgressDeadlineExceeded",

internal/operator-controller/controllers/clusterobjectset_controller_test.go

@@ -1083,6 +1083,8 @@ func Test_ClusterObjectSetReconciler_Reconcile_ArchivalAfterProgressDeadlineExce
 
 	ext := newTestClusterExtension()
 	rev1 := newTestClusterObjectSet(t, clusterObjectSetName, ext, testScheme)
+	rev1.Spec.ProgressDeadlineMinutes = 1
+	rev1.CreationTimestamp = metav1.NewTime(time.Now().Add(-2 * time.Minute))
 	rev1.Finalizers = []string{"olm.operatorframework.io/teardown"}
 	meta.SetStatusCondition(&rev1.Status.Conditions, metav1.Condition{
 		Type:               ocv1.ClusterObjectSetTypeProgressing,