⚠️ Remove Succeeded condition type from ClusterObjectSet

Replace the ClusterObjectSet Succeeded condition type with the
status.completedAt field as the signal that a revision has rolled out.
The ClusterExtension status mapping and the progress deadline check now
key off completedAt instead of the Succeeded condition, and the
Helm-to-boxcutter migrator records completedAt on migrated revisions.

Since ClusterObjectSet is experimental and does not guarantee upgrade
safety, this is a clean cut with no backward-compatibility fallback.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Per G. da Silva <pegoncal@redhat.com>

Author: Per G. da Silva

api/v1/clusterobjectset_types.go

@@ -28,7 +28,6 @@ const (
 	// Condition Types
 	ClusterObjectSetTypeAvailable   = "Available"
 	ClusterObjectSetTypeProgressing = "Progressing"
-	ClusterObjectSetTypeSucceeded   = "Succeeded"
 
 	// Condition Reasons
 	ClusterObjectSetReasonArchived        = "Archived"
@@ -510,9 +509,6 @@ type ClusterObjectSetStatus struct {
 	//   - When status is Unknown and reason is Archived, the ClusterObjectSet has been archived and its objects have been torn down.
 	//   - When status is Unknown and reason is Migrated, the ClusterObjectSet was migrated from an existing release and object status probe results have not yet been observed.
 	//
-	// The Succeeded condition represents whether the revision has successfully completed its rollout:
-	//   - When status is True and reason is Succeeded, the ClusterObjectSet has successfully completed its rollout. This condition is set once and persists even if the revision later becomes unavailable.
-	//
 	// +listType=map
 	// +listMapKey=type
 	// +optional

applyconfigurations/api/v1/clusterobjectsetstatus.go

@@ -570,9 +570,6 @@ spec:
                     - When status is Unknown and reason is Reconciling, the ClusterObjectSet has encountered an error that prevented it from observing the probes.
                     - When status is Unknown and reason is Archived, the ClusterObjectSet has been archived and its objects have been torn down.
                     - When status is Unknown and reason is Migrated, the ClusterObjectSet was migrated from an existing release and object status probe results have not yet been observed.
-
-                  The Succeeded condition represents whether the revision has successfully completed its rollout:
-                    - When status is True and reason is Succeeded, the ClusterObjectSet has successfully completed its rollout. This condition is set once and persists even if the revision later becomes unavailable.
                 items:
                   description: Condition contains details for one aspect of the current
                     state of this API Resource.

helm/olmv1/base/operator-controller/crd/experimental/olm.operatorframework.io_clusterobjectsets.yaml

@@ -252,20 +252,11 @@ func (c *ClusterObjectSetReconciler) reconcile(ctx context.Context, cos *ocv1.Cl
 
 		// Record the timestamp of the first time the revision was observed to be
 		// ready. This is set once and never changes for subsequent reconciliations.
+		// It also serves as the signal that the revision has completed its rollout,
+		// which the ClusterExtension controller uses to determine the installed revision.
 		if cos.Status.CompletedAt.IsZero() {
 			cos.Status.CompletedAt = metav1.NewTime(c.Clock.Now())
 		}
-
-		// 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.
-		meta.SetStatusCondition(&cos.Status.Conditions, metav1.Condition{
-			Type:               ocv1.ClusterObjectSetTypeSucceeded,
-			Status:             metav1.ConditionTrue,
-			Reason:             ocv1.ReasonSucceeded,
-			Message:            "Revision succeeded rolling out.",
-			ObservedGeneration: cos.Generation,
-		})
 	} else {
 		var probeFailureMsgs []string
 		for _, pres := range rres.GetPhases() {

internal/object-controller/controllers/clusterobjectset_controller.go

@@ -398,7 +398,7 @@ func Test_ClusterObjectSetReconciler_Reconcile_RevisionReconciliation(t *testing
 			},
 		},
 		{
-			name: "set Available:True:ProbesSucceeded and Succeeded:True:Succeeded conditions on successful revision rollout",
+			name: "set Available:True:ProbesSucceeded condition and completedAt on successful revision rollout",
 			revisionResult: newMockRevisionResult(mockCtrl, revisionResultConfig{
 				isComplete: true,
 			}),
@@ -428,12 +428,7 @@ func Test_ClusterObjectSetReconciler_Reconcile_RevisionReconciliation(t *testing
 				require.Equal(t, "Revision 1.0.0 has rolled out.", cond.Message)
 				require.Equal(t, int64(1), cond.ObservedGeneration)
 
-				cond = meta.FindStatusCondition(rev.Status.Conditions, ocv1.ClusterObjectSetTypeSucceeded)
-				require.NotNil(t, cond)
-				require.Equal(t, metav1.ConditionTrue, cond.Status)
-				require.Equal(t, ocv1.ReasonSucceeded, cond.Reason)
-				require.Equal(t, "Revision succeeded rolling out.", cond.Message)
-				require.Equal(t, int64(1), cond.ObservedGeneration)
+				require.False(t, rev.Status.CompletedAt.IsZero(), "completedAt should be set on successful rollout")
 			},
 		},
 		{
@@ -1137,12 +1132,7 @@ func Test_ClusterObjectSetReconciler_Reconcile_ProgressDeadline(t *testing.T) {
 					Reason:             ocv1.ReasonSucceeded,
 					ObservedGeneration: rev1.Generation,
 				})
-				meta.SetStatusCondition(&rev1.Status.Conditions, metav1.Condition{
-					Type:               ocv1.ClusterObjectSetTypeSucceeded,
-					Status:             metav1.ConditionTrue,
-					Reason:             ocv1.ReasonSucceeded,
-					ObservedGeneration: rev1.Generation,
-				})
+				rev1.Status.CompletedAt = metav1.Now()
 				return []client.Object{rev1, ext}
 			},
 			revisionResult: newMockRevisionResult(mockCtrl, revisionResultConfig{

internal/object-controller/controllers/clusterobjectset_controller_test.go

@@ -7,7 +7,6 @@ import (
 	"sync"
 	"time"
 
-	"k8s.io/apimachinery/pkg/api/meta"
 	"k8s.io/client-go/util/workqueue"
 	"k8s.io/utils/clock"
 	ctrl "sigs.k8s.io/controller-runtime"
@@ -78,25 +77,26 @@ func (r *deadlineAwareRateLimiter) NumRequeues(item ctrl.Request) int {
 // expires. A negative duration means the deadline has already passed.
 //
 // It derives the deadline from spec and metadata only, with one exception:
-// it checks the Succeeded status condition so that a revision recovering
-// from drift is not penalised by the original deadline.
+// it checks status.completedAt so that a revision recovering from drift is not
+// penalised by the original deadline.
 //
-// Succeeded is a latch: there is no way to deduce from current cluster state
-// alone that a COS succeeded in the past. If Succeeded is removed or set to
-// False, this function will return a deadline and the reconciler will set
-// ProgressDeadlineExceeded even though the revision previously succeeded.
+// completedAt is a latch: there is no way to deduce from current cluster state
+// alone that a COS became ready in the past. It is set once and never cleared,
+// so once observed ready a revision is never subject to the deadline again.
 //
 // Returns (0, false) when there is no active deadline:
 //   - progressDeadlineMinutes is 0
-//   - the revision has already succeeded
+//   - the revision has already been observed ready (completedAt set)
 //   - the revision is archived (deadline is irrelevant)
 //   - the revision is being deleted
 func durationUntilDeadline(clk clock.Clock, cos *ocv1.ClusterObjectSet) (time.Duration, bool) {
 	pd := cos.Spec.ProgressDeadlineMinutes
 	if pd <= 0 {
 		return 0, false
 	}
-	if meta.IsStatusConditionTrue(cos.Status.Conditions, ocv1.ClusterObjectSetTypeSucceeded) {
+	// Once the revision has been observed ready (completedAt set), the deadline
+	// no longer applies.
+	if !cos.Status.CompletedAt.IsZero() {
 		return 0, false
 	}
 	if cos.Spec.LifecycleState == ocv1.ClusterObjectSetLifecycleStateArchived {

internal/object-controller/controllers/progress_deadline.go

@@ -38,15 +38,12 @@ func TestDurationUntilDeadline(t *testing.T) {
 			expectHasDeadline: false,
 		},
 		{
-			name: "Succeeded is true — no deadline",
+			name: "completedAt is set — no deadline",
 			cos: ocv1.ClusterObjectSet{
 				ObjectMeta: metav1.ObjectMeta{CreationTimestamp: metav1.NewTime(creation)},
 				Spec:       ocv1.ClusterObjectSetSpec{ProgressDeadlineMinutes: 1, LifecycleState: ocv1.ClusterObjectSetLifecycleStateActive},
 				Status: ocv1.ClusterObjectSetStatus{
-					Conditions: []metav1.Condition{{
-						Type:   ocv1.ClusterObjectSetTypeSucceeded,
-						Status: metav1.ConditionTrue,
-					}},
+					CompletedAt: metav1.NewTime(creation),
 				},
 			},
 			expectDuration:    0,

internal/object-controller/controllers/progress_deadline_test.go

@@ -17,7 +17,6 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apiextensions-apiserver/pkg/apis/apiextensions"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
-	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime"
@@ -376,8 +375,8 @@ func (m *BoxcutterStorageMigrator) ensureMigratedRevisionStatus(ctx context.Cont
 		if revisions[i].Spec.Revision != 1 {
 			continue
 		}
-		// Skip if already succeeded - status is already set correctly.
-		if meta.IsStatusConditionTrue(revisions[i].Status.Conditions, ocv1.ClusterObjectSetTypeSucceeded) {
+		// Skip if already completed - status is already set correctly.
+		if !revisions[i].Status.CompletedAt.IsZero() {
 			return nil
 		}
 		// Ensure revision 1 status is set correctly, including for previously migrated
@@ -416,8 +415,8 @@ func (m *BoxcutterStorageMigrator) findLatestDeployedRelease(ac helmclient.Actio
 	return latestDeployed, nil
 }
 
-// ensureRevisionStatus ensures the revision has the Succeeded status condition set.
-// Returns nil if the status is already set or after successfully setting it.
+// ensureRevisionStatus ensures the revision has completedAt set, marking it as
+// installed. Returns nil if the status is already set or after successfully setting it.
 // Only sets status on revisions that were actually migrated from Helm (marked with MigratedFromHelmKey label).
 func (m *BoxcutterStorageMigrator) ensureRevisionStatus(ctx context.Context, name string) error {
 	rev := &ocv1.ClusterObjectSet{}
@@ -426,25 +425,22 @@ func (m *BoxcutterStorageMigrator) ensureRevisionStatus(ctx context.Context, nam
 	}
 
 	// Only set status if this revision was actually migrated from Helm.
-	// This prevents us from incorrectly marking normal Boxcutter revision 1 as succeeded
+	// This prevents us from incorrectly marking normal Boxcutter revision 1 as completed
 	// when it's still in progress.
 	if rev.Labels[labels.MigratedFromHelmKey] != "true" {
 		return nil
 	}
 
-	// Check if status is already set to Succeeded=True
-	if meta.IsStatusConditionTrue(rev.Status.Conditions, ocv1.ClusterObjectSetTypeSucceeded) {
+	// Check if completedAt is already set.
+	if !rev.Status.CompletedAt.IsZero() {
 		return nil
 	}
 
-	// Set the Succeeded status condition
-	meta.SetStatusCondition(&rev.Status.Conditions, metav1.Condition{
-		Type:               ocv1.ClusterObjectSetTypeSucceeded,
-		Status:             metav1.ConditionTrue,
-		Reason:             ocv1.ReasonSucceeded,
-		Message:            "Revision succeeded - migrated from Helm release",
-		ObservedGeneration: rev.GetGeneration(),
-	})
+	// Since we're migrating from a successfully deployed Helm release, the revision
+	// represents a working installation. Record completedAt so the ClusterExtension
+	// controller treats it as installed. The original ready time is not recoverable,
+	// so we use the migration time.
+	rev.Status.CompletedAt = metav1.Now()
 
 	if err := m.Client.Status().Update(ctx, rev); err != nil {
 		return fmt.Errorf("updating migrated revision status: %w", err)

internal/operator-controller/applier/boxcutter.go

@@ -17,7 +17,6 @@ import (
 	appsv1 "k8s.io/api/apps/v1"
 	corev1 "k8s.io/api/core/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
-	apimeta "k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime"
@@ -1196,18 +1195,13 @@ func TestBoxcutterStorageMigrator(t *testing.T) {
 		err := sm.Migrate(t.Context(), ext, map[string]string{"my-label": "my-value"})
 		require.NoError(t, err)
 
-		// Verify the migrated revision has Succeeded=True status with Succeeded reason and a migration message
+		// Verify the migrated revision has completedAt set, marking it as installed
 		require.NotNil(t, updatedObj, "Updated object should not be nil")
 
 		rev, ok := updatedObj.(*ocv1.ClusterObjectSet)
 		require.True(t, ok, "Updated object should be a ClusterObjectSet")
 
-		succeededCond := apimeta.FindStatusCondition(rev.Status.Conditions, ocv1.ClusterObjectSetTypeSucceeded)
-		require.NotNil(t, succeededCond, "Succeeded condition should be set")
-		assert.Equal(t, metav1.ConditionTrue, succeededCond.Status, "Succeeded condition should be True")
-		assert.Equal(t, ocv1.ReasonSucceeded, succeededCond.Reason, "Reason should be Succeeded")
-		assert.Equal(t, "Revision succeeded - migrated from Helm release", succeededCond.Message, "Message should indicate Helm migration")
-		assert.Equal(t, int64(1), succeededCond.ObservedGeneration, "ObservedGeneration should match revision generation")
+		assert.False(t, rev.Status.CompletedAt.IsZero(), "completedAt should be set on migrated revision")
 	})
 
 	t.Run("does not create revision when revisions exist", func(t *testing.T) {
@@ -1245,13 +1239,7 @@ func TestBoxcutterStorageMigrator(t *testing.T) {
 				Revision: 1, // Migration creates revision 1
 			},
 			Status: ocv1.ClusterObjectSetStatus{
-				Conditions: []metav1.Condition{
-					{
-						Type:   ocv1.ClusterObjectSetTypeSucceeded,
-						Status: metav1.ConditionTrue,
-						Reason: ocv1.ReasonSucceeded,
-					},
-				},
+				CompletedAt: metav1.Now(),
 			},
 		}
 
@@ -1334,13 +1322,10 @@ func TestBoxcutterStorageMigrator(t *testing.T) {
 		rev, ok := updatedObj.(*ocv1.ClusterObjectSet)
 		require.True(t, ok, "Updated object should be a ClusterObjectSet")
 
-		succeededCond := apimeta.FindStatusCondition(rev.Status.Conditions, ocv1.ClusterObjectSetTypeSucceeded)
-		require.NotNil(t, succeededCond, "Succeeded condition should be set")
-		assert.Equal(t, metav1.ConditionTrue, succeededCond.Status, "Succeeded condition should be True")
-		assert.Equal(t, ocv1.ReasonSucceeded, succeededCond.Reason, "Reason should be Succeeded")
+		assert.False(t, rev.Status.CompletedAt.IsZero(), "completedAt should be set")
 	})
 
-	t.Run("updates status from False to True for migrated revision", func(t *testing.T) {
+	t.Run("sets completedAt for migrated revision that has not completed", func(t *testing.T) {
 		testScheme := runtime.NewScheme()
 		require.NoError(t, ocv1.AddToScheme(testScheme))
 
@@ -1364,8 +1349,8 @@ func TestBoxcutterStorageMigrator(t *testing.T) {
 			FieldOwner:         "test-owner",
 		}
 
-		// Migrated revision with Succeeded=False (e.g., from a previous failed status update attempt)
-		// This simulates a revision whose Succeeded condition should be corrected from False to True during migration.
+		// Migrated revision without completedAt (e.g., from a previous failed status update attempt).
+		// This simulates a revision whose completedAt should be set during migration.
 		existingRev := ocv1.ClusterObjectSet{
 			ObjectMeta: metav1.ObjectMeta{
 				Name:       "test-revision",
@@ -1377,15 +1362,7 @@ func TestBoxcutterStorageMigrator(t *testing.T) {
 			Spec: ocv1.ClusterObjectSetSpec{
 				Revision: 1,
 			},
-			Status: ocv1.ClusterObjectSetStatus{
-				Conditions: []metav1.Condition{
-					{
-						Type:   ocv1.ClusterObjectSetTypeSucceeded,
-						Status: metav1.ConditionFalse, // Important: False, not missing
-						Reason: "InProgress",
-					},
-				},
-			},
+			// completedAt is not set - simulating a revision that was migrated but never marked completed.
 		}
 
 		mockClient.EXPECT().List(gomock.Any(), gomock.Any(), gomock.Any()).
@@ -1412,16 +1389,13 @@ func TestBoxcutterStorageMigrator(t *testing.T) {
 		err := sm.Migrate(t.Context(), ext, map[string]string{"my-label": "my-value"})
 		require.NoError(t, err)
 
-		// Verify the status was updated from False to True
+		// Verify completedAt was set
 		require.NotNil(t, updatedObj, "Updated object should not be nil")
 
 		rev, ok := updatedObj.(*ocv1.ClusterObjectSet)
 		require.True(t, ok, "Updated object should be a ClusterObjectSet")
 
-		succeededCond := apimeta.FindStatusCondition(rev.Status.Conditions, ocv1.ClusterObjectSetTypeSucceeded)
-		require.NotNil(t, succeededCond, "Succeeded condition should be set")
-		assert.Equal(t, metav1.ConditionTrue, succeededCond.Status, "Succeeded condition should be updated to True")
-		assert.Equal(t, ocv1.ReasonSucceeded, succeededCond.Reason, "Reason should be Succeeded")
+		assert.False(t, rev.Status.CompletedAt.IsZero(), "completedAt should be set during migration")
 	})
 
 	t.Run("does not set status on non-migrated revision 1", func(t *testing.T) {
@@ -1569,15 +1543,13 @@ func TestBoxcutterStorageMigrator(t *testing.T) {
 		err := sm.Migrate(t.Context(), ext, map[string]string{"my-label": "my-value"})
 		require.NoError(t, err)
 
-		// Verify the migrated revision has Succeeded=True status
+		// Verify the migrated revision has completedAt set
 		require.NotNil(t, updatedObj, "Updated object should not be nil")
 
 		rev, ok := updatedObj.(*ocv1.ClusterObjectSet)
 		require.True(t, ok, "Updated object should be a ClusterObjectSet")
 
-		succeededCond := apimeta.FindStatusCondition(rev.Status.Conditions, ocv1.ClusterObjectSetTypeSucceeded)
-		require.NotNil(t, succeededCond, "Succeeded condition should be set")
-		assert.Equal(t, metav1.ConditionTrue, succeededCond.Status, "Succeeded condition should be True")
+		assert.False(t, rev.Status.CompletedAt.IsZero(), "completedAt should be set on migrated revision")
 	})
 
 	t.Run("does not create revision when helm release is not deployed and no deployed history", func(t *testing.T) {

internal/operator-controller/applier/boxcutter_test.go

@@ -76,7 +76,9 @@ func (d *BoxcutterRevisionStatesGetter) GetRevisionStates(ctx context.Context, e
 			rm.Release = &releaseValue
 		}
 
-		if apimeta.IsStatusConditionTrue(rev.Status.Conditions, ocv1.ClusterObjectSetTypeSucceeded) {
+		// A revision is considered installed once it has been observed ready at
+		// least once, recorded by status.completedAt.
+		if !rev.Status.CompletedAt.IsZero() {
 			rs.Installed = rm
 		} else {
 			rs.RollingOut = append(rs.RollingOut, rm)