diff --git a/api/v1/clusterobjectset_types.go b/api/v1/clusterobjectset_types.go index 4c8028a07a..d68ad9efca 100644 --- a/api/v1/clusterobjectset_types.go +++ b/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. diff --git a/applyconfigurations/api/v1/clusterobjectsetstatus.go b/applyconfigurations/api/v1/clusterobjectsetstatus.go index 906da18177..56e66972a6 100644 --- a/applyconfigurations/api/v1/clusterobjectsetstatus.go +++ b/applyconfigurations/api/v1/clusterobjectsetstatus.go @@ -39,6 +39,7 @@ type ClusterObjectSetStatusApplyConfiguration 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. diff --git a/docs/draft/concepts/clusterobjectsets.md b/docs/draft/concepts/clusterobjectsets.md index b2c9828069..96c9a67147 100644 --- a/docs/draft/concepts/clusterobjectsets.md +++ b/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 diff --git a/helm/olmv1/base/object-controller/crd/experimental/olm.operatorframework.io_clusterobjectsets.yaml b/helm/olmv1/base/object-controller/crd/experimental/olm.operatorframework.io_clusterobjectsets.yaml index dec8cd06eb..b8625ab136 100644 --- a/helm/olmv1/base/object-controller/crd/experimental/olm.operatorframework.io_clusterobjectsets.yaml +++ b/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. diff --git a/internal/object-controller/controllers/clusterobjectset_controller.go b/internal/object-controller/controllers/clusterobjectset_controller.go index 9a066a6b84..83755bbb6e 100644 --- a/internal/object-controller/controllers/clusterobjectset_controller.go +++ b/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 diff --git a/internal/object-controller/controllers/clusterobjectset_controller_test.go b/internal/object-controller/controllers/clusterobjectset_controller_test.go index fabb88b1b6..aa3efa03b9 100644 --- a/internal/object-controller/controllers/clusterobjectset_controller_test.go +++ b/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{ diff --git a/manifests/experimental-e2e.yaml b/manifests/experimental-e2e.yaml index 062ec31cb7..af5be46e93 100644 --- a/manifests/experimental-e2e.yaml +++ b/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. diff --git a/manifests/experimental.yaml b/manifests/experimental.yaml index cd9328ce7d..d1664fc010 100644 --- a/manifests/experimental.yaml +++ b/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.