fix: Allow spec changes after ProgressDeadlineExceeded

This commit addresses the reconcile loop and archival timeout issues when
ProgressDeadlineExceeded is set on a ClusterObjectSet.

Generated-By: Claude

RollingOut revisions are sorted oldest→newest, so checking the first
element (index 0) could cause unnecessary re-resolution when a newer
rolling-out revision already matches the current spec.

Now checks the last element (latest rolling-out revision) before
deciding whether to resolve a new bundle from catalog.

This prevents creating extra revisions when multiple rolling-out
revisions exist (e.g., older revision stuck, newer one rolling out).

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

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,32 @@ 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 latest 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.
+		//
+		// Note: RollingOut slice is sorted oldest→newest, so use the last element (most recent).
+		// This avoids re-resolving when a newer revision already matches the spec, even if an
+		// older revision is still stuck rolling out.
 		if len(state.revisionStates.RollingOut) > 0 {
-			installedBundleName := ""
-			if state.revisionStates.Installed != nil {
-				installedBundleName = state.revisionStates.Installed.Name
+			latestRollingOutRevision := state.revisionStates.RollingOut[len(state.revisionStates.RollingOut)-1]
+			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 latest rolling-out revision version (or no version specified),
+			// reuse the existing rolling-out revision
+			if specVersion == "" || specVersion == latestRollingOutRevision.Version {
+				installedBundleName := ""
+				if state.revisionStates.Installed != nil {
+					installedBundleName = state.revisionStates.Installed.Name
+				}
+				SetDeprecationStatus(ext, installedBundleName, nil, false)
+				state.resolvedRevisionMetadata = latestRollingOutRevision
+				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

@@ -31,7 +31,6 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/builder"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
-	"sigs.k8s.io/controller-runtime/pkg/event"
 	"sigs.k8s.io/controller-runtime/pkg/handler"
 	"sigs.k8s.io/controller-runtime/pkg/log"
 	"sigs.k8s.io/controller-runtime/pkg/predicate"
@@ -81,7 +80,10 @@ func (c *ClusterObjectSetReconciler) Reconcile(ctx context.Context, req ctrl.Req
 	reconciledRev := existingRev.DeepCopy()
 	res, reconcileErr := c.reconcile(ctx, reconciledRev)
 
-	if pd := existingRev.Spec.ProgressDeadlineMinutes; pd > 0 {
+	// Progress deadline enforcement only applies to Active revisions, not Archived ones.
+	// Archived revisions may need to retry teardown operations and should not have their
+	// Progressing condition or requeue behavior overridden by the deadline check.
+	if pd := existingRev.Spec.ProgressDeadlineMinutes; pd > 0 && reconciledRev.Spec.LifecycleState != ocv1.ClusterObjectSetLifecycleStateArchived {
 		cnd := meta.FindStatusCondition(reconciledRev.Status.Conditions, ocv1.ClusterObjectSetTypeProgressing)
 		isStillProgressing := cnd != nil && cnd.Status == metav1.ConditionTrue && cnd.Reason != ocv1.ReasonSucceeded
 		succeeded := meta.IsStatusConditionTrue(reconciledRev.Status.Conditions, ocv1.ClusterObjectSetTypeSucceeded)
@@ -344,29 +346,12 @@ type Sourcoser interface {
 }
 
 func (c *ClusterObjectSetReconciler) SetupWithManager(mgr ctrl.Manager) error {
-	skipProgressDeadlineExceededPredicate := predicate.Funcs{
-		UpdateFunc: func(e event.UpdateEvent) bool {
-			rev, ok := e.ObjectNew.(*ocv1.ClusterObjectSet)
-			if !ok {
-				return true
-			}
-			// allow deletions to happen
-			if !rev.DeletionTimestamp.IsZero() {
-				return true
-			}
-			if cnd := meta.FindStatusCondition(rev.Status.Conditions, ocv1.ClusterObjectSetTypeProgressing); cnd != nil && cnd.Status == metav1.ConditionFalse && cnd.Reason == ocv1.ReasonProgressDeadlineExceeded {
-				return false
-			}
-			return true
-		},
-	}
 	c.Clock = clock.RealClock{}
 	return ctrl.NewControllerManagedBy(mgr).
 		For(
 			&ocv1.ClusterObjectSet{},
 			builder.WithPredicates(
 				predicate.ResourceVersionChangedPredicate{},
-				skipProgressDeadlineExceededPredicate,
 			),
 		).
 		WatchesRawSource(
@@ -684,7 +669,33 @@ 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
+// 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.
+//
+// Archived revisions are exempt because they skip progress deadline enforcement entirely,
+// so their status updates won't be overridden.
+//
+// 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 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 &&
+		cos.Spec.LifecycleState != ocv1.ClusterObjectSetLifecycleStateArchived {
+		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
+	}
 	meta.SetStatusCondition(&cos.Status.Conditions, metav1.Condition{
 		Type:               ocv1.ClusterObjectSetTypeProgressing,
 		Status:             metav1.ConditionTrue,

internal/operator-controller/controllers/clusterobjectset_controller_internal_test.go

@@ -6,6 +6,7 @@ import (
 	"time"
 
 	"github.com/stretchr/testify/require"
+	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
@@ -235,3 +236,113 @@ func (m *mockTrackingCacheInternal) Watch(ctx context.Context, user client.Objec
 func (m *mockTrackingCacheInternal) Source(h handler.EventHandler, predicates ...predicate.Predicate) source.Source {
 	return nil
 }
+
+func Test_markAsProgressing_doesNotOverwriteProgressDeadlineExceeded(t *testing.T) {
+	for _, tc := range []struct {
+		name               string
+		generation         int64
+		existingConditions []metav1.Condition
+		reason             string
+		expectProgressing  metav1.ConditionStatus
+		expectReason       string
+	}{
+		{
+			name:               "sets Progressing when no condition exists",
+			generation:         1,
+			existingConditions: nil,
+			reason:             ocv1.ReasonRollingOut,
+			expectProgressing:  metav1.ConditionTrue,
+			expectReason:       ocv1.ReasonRollingOut,
+		},
+		{
+			name:       "sets Progressing when currently RollingOut",
+			generation: 1,
+			existingConditions: []metav1.Condition{
+				{
+					Type:               ocv1.ClusterObjectSetTypeProgressing,
+					Status:             metav1.ConditionTrue,
+					Reason:             ocv1.ReasonRollingOut,
+					ObservedGeneration: 1,
+				},
+			},
+			reason:            ocv1.ReasonSucceeded,
+			expectProgressing: metav1.ConditionTrue,
+			expectReason:      ocv1.ReasonSucceeded,
+		},
+		{
+			name:       "does not overwrite ProgressDeadlineExceeded with RollingOut (same generation)",
+			generation: 1,
+			existingConditions: []metav1.Condition{
+				{
+					Type:               ocv1.ClusterObjectSetTypeProgressing,
+					Status:             metav1.ConditionFalse,
+					Reason:             ocv1.ReasonProgressDeadlineExceeded,
+					ObservedGeneration: 1,
+				},
+			},
+			reason:            ocv1.ReasonRollingOut,
+			expectProgressing: metav1.ConditionFalse,
+			expectReason:      ocv1.ReasonProgressDeadlineExceeded,
+		},
+		{
+			name:       "allows Succeeded to overwrite ProgressDeadlineExceeded",
+			generation: 1,
+			existingConditions: []metav1.Condition{
+				{
+					Type:               ocv1.ClusterObjectSetTypeProgressing,
+					Status:             metav1.ConditionFalse,
+					Reason:             ocv1.ReasonProgressDeadlineExceeded,
+					ObservedGeneration: 1,
+				},
+			},
+			reason:            ocv1.ReasonSucceeded,
+			expectProgressing: metav1.ConditionTrue,
+			expectReason:      ocv1.ReasonSucceeded,
+		},
+		{
+			name:       "allows Retrying to overwrite ProgressDeadlineExceeded (same generation)",
+			generation: 1,
+			existingConditions: []metav1.Condition{
+				{
+					Type:               ocv1.ClusterObjectSetTypeProgressing,
+					Status:             metav1.ConditionFalse,
+					Reason:             ocv1.ReasonProgressDeadlineExceeded,
+					ObservedGeneration: 1,
+				},
+			},
+			reason:            ocv1.ClusterObjectSetReasonRetrying,
+			expectProgressing: metav1.ConditionTrue,
+			expectReason:      ocv1.ClusterObjectSetReasonRetrying,
+		},
+		{
+			name:       "allows RollingOut when generation changed since ProgressDeadlineExceeded",
+			generation: 2,
+			existingConditions: []metav1.Condition{
+				{
+					Type:               ocv1.ClusterObjectSetTypeProgressing,
+					Status:             metav1.ConditionFalse,
+					Reason:             ocv1.ReasonProgressDeadlineExceeded,
+					ObservedGeneration: 1,
+				},
+			},
+			reason:            ocv1.ReasonRollingOut,
+			expectProgressing: metav1.ConditionTrue,
+			expectReason:      ocv1.ReasonRollingOut,
+		},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			cos := &ocv1.ClusterObjectSet{
+				ObjectMeta: metav1.ObjectMeta{Generation: tc.generation},
+				Status: ocv1.ClusterObjectSetStatus{
+					Conditions: tc.existingConditions,
+				},
+			}
+			markAsProgressing(cos, tc.reason, "test message")
+
+			cnd := meta.FindStatusCondition(cos.Status.Conditions, ocv1.ClusterObjectSetTypeProgressing)
+			require.NotNil(t, cnd)
+			require.Equal(t, tc.expectProgressing, cnd.Status)
+			require.Equal(t, tc.expectReason, cnd.Reason)
+		})
+	}
+}