From c5cf0aaff8fb4ddb1da91bcf40b71e52170b0418 Mon Sep 17 00:00:00 2001 From: "Per G. da Silva" Date: Fri, 16 Oct 2026 13:49:14 +0200 Subject: [PATCH] =?UTF-8?q?=E2=9C=A8=20initialize=20ClusterObjectSet=20Rea?= =?UTF-8?q?dy=3DUnknown/Pending;=20classify=20digest=20errors?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-ups to review comments on #3012: - Initialize the Ready condition to Unknown/Pending when it is absent, so a freshly created active revision always reports an explicit Ready rather than leaving it absent when an early transient error returns before the first evaluation. Done in the outer Reconcile (after any in-reconcile metadata patches that refresh the object), only when Ready is absent (so it never overwrites an observed state), and skipped for deleting or archived revisions. Adds the Pending reason. - Classify computePhaseDigest's json.Marshal failure as configValidationError (InvalidSpecification) so no deterministic failure reaches the transient fallthrough, and document the invariant that anything reaching the buildBoxcutterPhases fallthrough is a transient infrastructure error. Updates the CRD doc-string, concept doc, generated CRD/manifests/applyconfig, and unit tests. Co-Authored-By: Claude Opus 4.8 Signed-off-by: Per G. da Silva --- api/v1/clusterobjectset_types.go | 2 + .../api/v1/clusterobjectsetstatus.go | 1 + docs/draft/concepts/clusterobjectsets.md | 3 +- ...peratorframework.io_clusterobjectsets.yaml | 1 + .../clusterobjectset_controller.go | 37 +++++- .../clusterobjectset_controller_test.go | 111 +++++++++++++++--- manifests/experimental-e2e.yaml | 1 + manifests/experimental.yaml | 1 + 8 files changed, 141 insertions(+), 16 deletions(-) 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.