✨ initialize ClusterObjectSet Ready=Unknown/Pending; classify digest errors

api/v1/clusterobjectset_types.go

@@ -29,6 +29,7 @@ const (
 	ClusterObjectSetTypeReady = "Ready"
 
 	// Condition Reasons
+	ClusterObjectSetReasonPending              = "Pending"
 	ClusterObjectSetReasonArchived             = "Archived"
 	ClusterObjectSetReasonArchiving            = "Archiving"
 	ClusterObjectSetReasonObjectsNotReady      = "ObjectsNotReady"
@@ -500,6 +501,7 @@ type ClusterObjectSetStatus struct {
 	// True means all objects are at the desired state; False means one or more
 	// objects are not at the desired state; Unknown is the initial state, before
 	// the first reconciliation has evaluated the revision.
+	//   - Unknown with reason Pending: the revision has not yet been evaluated (initial state, set when the condition is first created).
 	//   - True with reason AllObjectsReady: the revision has rolled out and all objects are at the desired state.
 	//   - False with reason ObjectsNotReady: one or more objects are still being applied or have not yet passed their readiness probes during rollout.
 	//   - False with reason Superseded: one or more objects have been adopted by a newer revision of the object set.

docs/draft/concepts/clusterobjectsets.md

@@ -224,9 +224,10 @@ Indicates whether all objects in the revision have been successfully rolled out
 | False | `InvalidSpecification` | The revision spec is invalid (a malformed revision/phase object or progression probe); requires a spec change |
 | False | `Archiving` | The revision is being archived; owned objects are still being torn down |
 | False | `Archived` | Objects torn down after archival |
+| Unknown | `Pending` | The revision has not yet been evaluated (initial state) |
 | Unknown | `Migrated` | Migrated from existing release; probes not yet observed |
 
-A transient internal error (e.g. an API read failure) leaves the `Ready` condition unchanged and is retried with backoff; it is not reported as a distinct reason, so on a first reconcile the condition may remain absent (`Unknown`).
+On a revision's first reconciliation the `Ready` condition is initialized to `Unknown`/`Pending`, so it is always present. A transient internal error (e.g. an API read failure) leaves the `Ready` condition unchanged and is retried with backoff; it is not reported as a distinct reason (while the revision is still `Pending`, it stays `Unknown`/`Pending`).
 
 ### Succeeded
 

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

@@ -558,6 +558,7 @@ spec:
                   True means all objects are at the desired state; False means one or more
                   objects are not at the desired state; Unknown is the initial state, before
                   the first reconciliation has evaluated the revision.
+                    - Unknown with reason Pending: the revision has not yet been evaluated (initial state, set when the condition is first created).
                     - True with reason AllObjectsReady: the revision has rolled out and all objects are at the desired state.
                     - False with reason ObjectsNotReady: one or more objects are still being applied or have not yet passed their readiness probes during rollout.
                     - False with reason Superseded: one or more objects have been adopted by a newer revision of the object set.

internal/object-controller/controllers/clusterobjectset_controller.go

@@ -97,6 +97,18 @@ func (c *ClusterObjectSetReconciler) Reconcile(ctx context.Context, req ctrl.Req
 	reconciledRev := existingRev.DeepCopy()
 	res, reconcileErr := c.reconcile(ctx, reconciledRev)
 
+	// Ensure an active revision always reports a Ready condition. If reconcile left it
+	// absent — e.g. an early transient error returned before the first evaluation set a
+	// concrete state — initialize it to Unknown/Pending. This runs after reconcile (past
+	// any in-reconcile metadata patches that refresh the object), only sets Ready when
+	// absent so it never overwrites a previously observed state, and is skipped for
+	// revisions that are being deleted or archived.
+	if reconciledRev.DeletionTimestamp.IsZero() &&
+		reconciledRev.Spec.LifecycleState != ocv1.ClusterObjectSetLifecycleStateArchived &&
+		meta.FindStatusCondition(reconciledRev.Status.Conditions, ocv1.ClusterObjectSetTypeReady) == nil {
+		markAsPending(reconciledRev)
+	}
+
 	// Do checks before any Update()s, as Update() may modify the resource structure!
 	updateStatus := !equality.Semantic.DeepEqual(existingRev.Status, reconciledRev.Status)
 
@@ -174,6 +186,13 @@ func (c *ClusterObjectSetReconciler) reconcile(ctx context.Context, cos *ocv1.Cl
 			markAsNotReady(cos, ocv1.ClusterObjectSetReasonInvalidSpecification, fmt.Sprintf("The object set spec is invalid: %v.", err))
 			return ctrl.Result{}, nil
 		}
+		// Invariant: the error sites in buildBoxcutterPhases classify every deterministic
+		// (spec/content) failure as a *configValidationError or *referencedContentError,
+		// handled above. Anything reaching here is therefore a transient infrastructure
+		// error (e.g. listing sibling revisions or reading a referenced Secret failed), so
+		// leave Ready unchanged and return the error for a rate-limited backoff requeue.
+		// New deterministic failure modes added to buildBoxcutterPhases must be wrapped in
+		// one of those typed errors so they are reported rather than retried forever.
 		return ctrl.Result{}, fmt.Errorf("converting to boxcutter revision: %v", err)
 	}
 
@@ -802,6 +821,19 @@ func hasSupersededObject(rres machinery.RevisionResult) bool {
 	return false
 }
 
+// markAsPending initializes Ready=Unknown/Pending. It is only used before the first
+// evaluation (when Ready is absent), so a freshly created revision always reports an
+// explicit Ready condition rather than an absent one.
+func markAsPending(cos *ocv1.ClusterObjectSet) {
+	meta.SetStatusCondition(&cos.Status.Conditions, metav1.Condition{
+		Type:               ocv1.ClusterObjectSetTypeReady,
+		Status:             metav1.ConditionUnknown,
+		Reason:             ocv1.ClusterObjectSetReasonPending,
+		Message:            "The revision has not yet been evaluated.",
+		ObservedGeneration: cos.Generation,
+	})
+}
+
 func markAsArchiving(cos *ocv1.ClusterObjectSet) {
 	const msg = "The object set is being archived; removing owned objects."
 	markAsNotReady(cos, ocv1.ClusterObjectSetReasonArchiving, msg)
@@ -823,7 +855,10 @@ func computePhaseDigest(name string, objects []client.Object) (string, error) {
 	}
 	data, err := json.Marshal(phaseMap)
 	if err != nil {
-		return "", fmt.Errorf("marshaling phase %q: %w", name, err)
+		// A non-marshalable object is invalid spec content, not a transient error;
+		// classify it so the reconciler reports InvalidSpecification instead of
+		// retrying forever.
+		return "", &configValidationError{fmt.Errorf("marshaling phase %q: %w", name, err)}
 	}
 	h := sha256.Sum256(data)
 	return fmt.Sprintf("sha256:%x", h), nil

internal/object-controller/controllers/clusterobjectset_controller_test.go

@@ -70,9 +70,10 @@ func Test_ClusterObjectSetReconciler_Reconcile_RevisionReconciliation(t *testing
 		},
 		{
 			// A transient error leaves the Ready condition unchanged and returns the
-			// error for rate-limited backoff. On a first reconcile with no prior Ready,
-			// the condition stays absent (consumers read absent as Unknown).
-			name:                    "Ready condition is left unset on transient error when not previously set",
+			// error for rate-limited backoff. On a first reconcile the transient path
+			// sets no concrete reason, so an active revision reports the initialized
+			// Ready=Unknown/Pending.
+			name:                    "Ready condition is Unknown/Pending on transient error when not previously set",
 			reconcilingRevisionName: clusterObjectSetName,
 			revisionResult:          newMockRevisionResult(mockCtrl, revisionResultConfig{}),
 			revisionReconcileErr:    errors.New("some error"),
@@ -87,7 +88,10 @@ func Test_ClusterObjectSetReconciler_Reconcile_RevisionReconciliation(t *testing
 					Name: clusterObjectSetName,
 				}, rev)
 				require.NoError(t, err)
-				require.Nil(t, meta.FindStatusCondition(rev.Status.Conditions, ocv1.ClusterObjectSetTypeReady))
+				cond := meta.FindStatusCondition(rev.Status.Conditions, ocv1.ClusterObjectSetTypeReady)
+				require.NotNil(t, cond)
+				require.Equal(t, metav1.ConditionUnknown, cond.Status)
+				require.Equal(t, ocv1.ClusterObjectSetReasonPending, cond.Reason)
 			},
 		},
 		{
@@ -123,9 +127,9 @@ func Test_ClusterObjectSetReconciler_Reconcile_RevisionReconciliation(t *testing
 		},
 		{
 			// A revision whose FIRST reconcile fails at engine creation (factory error)
-			// leaves the Ready condition unset — a transient error does not fabricate a
-			// Ready reason.
-			name:                    "Ready condition is left unset on factory error when not previously set",
+			// does not fabricate a concrete Ready reason; an active revision reports the
+			// initialized Ready=Unknown/Pending.
+			name:                    "Ready condition is Unknown/Pending on factory error when not previously set",
 			reconcilingRevisionName: clusterObjectSetName,
 			factoryErr:              errors.New("failed to create revision engine"),
 			existingObjs: func() []client.Object {
@@ -139,7 +143,10 @@ func Test_ClusterObjectSetReconciler_Reconcile_RevisionReconciliation(t *testing
 					Name: clusterObjectSetName,
 				}, rev)
 				require.NoError(t, err)
-				require.Nil(t, meta.FindStatusCondition(rev.Status.Conditions, ocv1.ClusterObjectSetTypeReady))
+				cond := meta.FindStatusCondition(rev.Status.Conditions, ocv1.ClusterObjectSetTypeReady)
+				require.NotNil(t, cond)
+				require.Equal(t, metav1.ConditionUnknown, cond.Status)
+				require.Equal(t, ocv1.ClusterObjectSetReasonPending, cond.Reason)
 				require.Nil(t, meta.FindStatusCondition(rev.Status.Conditions, "Progressing"))
 			},
 		},
@@ -958,12 +965,16 @@ func Test_ClusterObjectSetReconciler_Reconcile_VerifyReferencedSecrets(t *testin
 
 			updated := &ocv1.ClusterObjectSet{}
 			if tc.wantErr {
-				// A read error is returned so controller-runtime requeues with backoff, and
-				// the Ready condition is left untouched (absent on this first reconcile).
+				// A read error is returned so controller-runtime requeues with backoff. The
+				// read-error path itself does not set a concrete Ready reason; on a first
+				// reconcile an active revision reports the initialized Ready=Unknown/Pending.
 				require.Error(t, err)
 				require.Equal(t, ctrl.Result{}, result)
 				require.NoError(t, testClient.Get(t.Context(), client.ObjectKey{Name: clusterObjectSetName}, updated))
-				require.Nil(t, meta.FindStatusCondition(updated.Status.Conditions, ocv1.ClusterObjectSetTypeReady))
+				cond := meta.FindStatusCondition(updated.Status.Conditions, ocv1.ClusterObjectSetTypeReady)
+				require.NotNil(t, cond)
+				require.Equal(t, metav1.ConditionUnknown, cond.Status)
+				require.Equal(t, ocv1.ClusterObjectSetReasonPending, cond.Reason)
 				return
 			}
 			// A confirmed mutable Secret is a validation failure that keeps retrying so the
@@ -1121,11 +1132,15 @@ func Test_ClusterObjectSetReconciler_Reconcile_BuildPhasesErrorClassification(t
 			require.Equal(t, ctrl.Result{}, result)
 			updated := &ocv1.ClusterObjectSet{}
 			if tc.wantErr {
-				// Transient errors are returned so controller-runtime requeues with backoff,
-				// and the Ready condition is left untouched (absent on this first reconcile).
+				// Transient errors are returned so controller-runtime requeues with backoff.
+				// The transient path itself does not set a concrete Ready reason; on a first
+				// reconcile an active revision reports the initialized Ready=Unknown/Pending.
 				require.Error(t, err)
 				require.NoError(t, testClient.Get(t.Context(), client.ObjectKey{Name: clusterObjectSetName}, updated))
-				require.Nil(t, meta.FindStatusCondition(updated.Status.Conditions, ocv1.ClusterObjectSetTypeReady))
+				cond := meta.FindStatusCondition(updated.Status.Conditions, ocv1.ClusterObjectSetTypeReady)
+				require.NotNil(t, cond)
+				require.Equal(t, metav1.ConditionUnknown, cond.Status)
+				require.Equal(t, ocv1.ClusterObjectSetReasonPending, cond.Reason)
 				return
 			}
 			require.NoError(t, err)
@@ -1686,6 +1701,74 @@ func Test_ClusterObjectSetReconciler_Reconcile_ProgressDeadline(t *testing.T) {
 	}
 }
 
+func Test_ClusterObjectSetReconciler_Reconcile_InitializesReadyPending(t *testing.T) {
+	testScheme := newScheme(t)
+
+	// newListFailingClient returns a client whose ClusterObjectSetList List always fails,
+	// so reconcile returns a transient error from listSiblingRevisions before it sets any
+	// concrete Ready reason — isolating the Ready initialization behavior.
+	newListFailingClient := func(objs ...client.Object) client.Client {
+		return fake.NewClientBuilder().
+			WithScheme(testScheme).
+			WithStatusSubresource(&ocv1.ClusterObjectSet{}).
+			WithObjects(objs...).
+			WithInterceptorFuncs(interceptor.Funcs{
+				List: func(ctx context.Context, cl client.WithWatch, list client.ObjectList, opts ...client.ListOption) error {
+					if _, ok := list.(*ocv1.ClusterObjectSetList); ok {
+						return errors.New("etcdserver: request timed out")
+					}
+					return cl.List(ctx, list, opts...)
+				},
+			}).
+			Build()
+	}
+
+	reconcileReady := func(t *testing.T, c client.Client) *metav1.Condition {
+		mockCtrl := gomock.NewController(t)
+		_, err := (&controllers.ClusterObjectSetReconciler{
+			Client:                c,
+			RevisionEngineFactory: newMockRevisionEngineFactoryWithEngine(mockCtrl, newNoopMockRevisionEngine(mockCtrl), nil),
+			TrackingCache:         newMockTrackingCache(mockCtrl, c, nil),
+		}).Reconcile(t.Context(), ctrl.Request{NamespacedName: types.NamespacedName{Name: clusterObjectSetName}})
+		require.Error(t, err)
+		updated := &ocv1.ClusterObjectSet{}
+		require.NoError(t, c.Get(t.Context(), client.ObjectKey{Name: clusterObjectSetName}, updated))
+		return meta.FindStatusCondition(updated.Status.Conditions, ocv1.ClusterObjectSetTypeReady)
+	}
+
+	t.Run("initializes Ready=Unknown/Pending on a fresh active revision", func(t *testing.T) {
+		ext := newTestClusterExtension()
+		rev := newTestClusterObjectSet(t, clusterObjectSetName, ext, testScheme)
+		cond := reconcileReady(t, newListFailingClient(ext, rev))
+		require.NotNil(t, cond)
+		require.Equal(t, metav1.ConditionUnknown, cond.Status)
+		require.Equal(t, ocv1.ClusterObjectSetReasonPending, cond.Reason)
+	})
+
+	t.Run("does not overwrite an already-set Ready", func(t *testing.T) {
+		ext := newTestClusterExtension()
+		rev := newTestClusterObjectSet(t, clusterObjectSetName, ext, testScheme)
+		meta.SetStatusCondition(&rev.Status.Conditions, metav1.Condition{
+			Type:    ocv1.ClusterObjectSetTypeReady,
+			Status:  metav1.ConditionTrue,
+			Reason:  ocv1.ClusterObjectSetReasonAllObjectsReady,
+			Message: "All objects are at the desired state",
+		})
+		cond := reconcileReady(t, newListFailingClient(ext, rev))
+		require.NotNil(t, cond)
+		require.Equal(t, metav1.ConditionTrue, cond.Status, "a transient error must not flap an already-observed Ready")
+		require.Equal(t, ocv1.ClusterObjectSetReasonAllObjectsReady, cond.Reason)
+	})
+
+	t.Run("does not initialize Ready for an archived revision", func(t *testing.T) {
+		ext := newTestClusterExtension()
+		rev := newTestClusterObjectSet(t, clusterObjectSetName, ext, testScheme)
+		rev.Spec.LifecycleState = ocv1.ClusterObjectSetLifecycleStateArchived
+		cond := reconcileReady(t, newListFailingClient(ext, rev))
+		require.Nil(t, cond, "an archived revision should not report Ready=Pending")
+	})
+}
+
 func newTestClusterExtension() *ocv1.ClusterExtension {
 	return &ocv1.ClusterExtension{
 		ObjectMeta: metav1.ObjectMeta{

manifests/experimental-e2e.yaml

@@ -1969,6 +1969,7 @@ spec:
                   True means all objects are at the desired state; False means one or more
                   objects are not at the desired state; Unknown is the initial state, before
                   the first reconciliation has evaluated the revision.
+                    - Unknown with reason Pending: the revision has not yet been evaluated (initial state, set when the condition is first created).
                     - True with reason AllObjectsReady: the revision has rolled out and all objects are at the desired state.
                     - False with reason ObjectsNotReady: one or more objects are still being applied or have not yet passed their readiness probes during rollout.
                     - False with reason Superseded: one or more objects have been adopted by a newer revision of the object set.

manifests/experimental.yaml

@@ -1930,6 +1930,7 @@ spec:
                   True means all objects are at the desired state; False means one or more
                   objects are not at the desired state; Unknown is the initial state, before
                   the first reconciliation has evaluated the revision.
+                    - Unknown with reason Pending: the revision has not yet been evaluated (initial state, set when the condition is first created).
                     - True with reason AllObjectsReady: the revision has rolled out and all objects are at the desired state.
                     - False with reason ObjectsNotReady: one or more objects are still being applied or have not yet passed their readiness probes during rollout.
                     - False with reason Superseded: one or more objects have been adopted by a newer revision of the object set.