From 355ab4da23c9d2cbf19e8d2fbe302947d75c1d1c Mon Sep 17 00:00:00 2001 From: "Per G. da Silva" Date: Mon, 12 Oct 2026 17:06:33 +0200 Subject: [PATCH] =?UTF-8?q?=E2=9A=A0=20remove=20ClusterObjectSet=20Ready?= =?UTF-8?q?=20RetryableError=20reason?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Transient errors now leave the Ready condition unchanged and return an error for rate-limited backoff requeue, instead of overwriting Ready with RetryableError. On a first reconcile the condition may remain absent (read as Unknown). The ClusterExtension no longer maps transient COS errors to Installed=Failed or Progressing=Retrying; it goes neutral. Remove the RetryableError reason constant and the setRetryableErrorConditions helper. determineFailureReason now treats only ObjectCollision, PreconditionsNotMet, and InvalidSpecification as installation failures. 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 +- docs/draft/concepts/large-bundle-support.md | 4 +- ...peratorframework.io_clusterobjectsets.yaml | 1 - .../clusterobjectset_controller.go | 26 ++-- .../clusterobjectset_controller_test.go | 111 ++++++++---------- .../controllers/common_controller.go | 25 ++-- .../controllers/common_controller_test.go | 30 +++-- manifests/experimental-e2e.yaml | 1 - manifests/experimental.yaml | 1 - 11 files changed, 94 insertions(+), 111 deletions(-) diff --git a/api/v1/clusterobjectset_types.go b/api/v1/clusterobjectset_types.go index f89d2bb5aa..4c8028a07a 100644 --- a/api/v1/clusterobjectset_types.go +++ b/api/v1/clusterobjectset_types.go @@ -34,7 +34,6 @@ const ( ClusterObjectSetReasonObjectsNotReady = "ObjectsNotReady" ClusterObjectSetReasonSuperseded = "Superseded" ClusterObjectSetReasonObjectCollision = "ObjectCollision" - ClusterObjectSetReasonRetryableError = "RetryableError" ClusterObjectSetReasonPreconditionsNotMet = "PreconditionsNotMet" ClusterObjectSetReasonInvalidSpecification = "InvalidSpecification" ClusterObjectSetReasonAllObjectsReady = "AllObjectsReady" @@ -508,7 +507,6 @@ type ClusterObjectSetStatus struct { // - False with reason PreconditionsNotMet: a precondition for applying the revision is not yet satisfied (a referenced Secret is mutable or has invalid or changed content, or a preflight dry-run was rejected, e.g. an immutable field, a conflicting object, or RBAC/dependency CRDs not yet propagated); the revision keeps retrying and recovers once the precondition is met. // - False with reason InvalidSpecification: the revision spec is invalid (a malformed revision/phase object or progression probe) and requires a spec change to proceed. // - False with reason ProgressDeadlineExceeded: the revision did not roll out within spec.progressDeadlineMinutes. - // - False with reason RetryableError: the revision encountered a transient error that may resolve on a subsequent reconciliation. // - False with reason Archiving: the revision is being archived and its owned objects are still being torn down. // - False with reason Archived: the revision has been archived and its objects have been torn down. // diff --git a/applyconfigurations/api/v1/clusterobjectsetstatus.go b/applyconfigurations/api/v1/clusterobjectsetstatus.go index f57fa46bd2..906da18177 100644 --- a/applyconfigurations/api/v1/clusterobjectsetstatus.go +++ b/applyconfigurations/api/v1/clusterobjectsetstatus.go @@ -46,7 +46,6 @@ type ClusterObjectSetStatusApplyConfiguration struct { // - False with reason PreconditionsNotMet: a precondition for applying the revision is not yet satisfied (a referenced Secret is mutable or has invalid or changed content, or a preflight dry-run was rejected, e.g. an immutable field, a conflicting object, or RBAC/dependency CRDs not yet propagated); the revision keeps retrying and recovers once the precondition is met. // - False with reason InvalidSpecification: the revision spec is invalid (a malformed revision/phase object or progression probe) and requires a spec change to proceed. // - False with reason ProgressDeadlineExceeded: the revision did not roll out within spec.progressDeadlineMinutes. - // - False with reason RetryableError: the revision encountered a transient error that may resolve on a subsequent reconciliation. // - False with reason Archiving: the revision is being archived and its owned objects are still being torn down. // - False with reason Archived: the revision has been archived and its objects have been torn down. // diff --git a/docs/draft/concepts/clusterobjectsets.md b/docs/draft/concepts/clusterobjectsets.md index dd0e666f8a..b2c9828069 100644 --- a/docs/draft/concepts/clusterobjectsets.md +++ b/docs/draft/concepts/clusterobjectsets.md @@ -222,11 +222,12 @@ Indicates whether all objects in the revision have been successfully rolled out | False | `ObjectCollision` | One or more objects are controlled by another owner, or are unowned and collision protection prevents adoption | | False | `PreconditionsNotMet` | A precondition for applying the revision is not yet satisfied (a referenced Secret is mutable or has invalid or changed content, or a preflight dry-run was rejected, e.g. an immutable field, a conflicting object, or RBAC/dependency CRDs not yet propagated); the revision keeps retrying and recovers once the precondition is met | | False | `InvalidSpecification` | The revision spec is invalid (a malformed revision/phase object or progression probe); requires a spec change | -| Unknown | `RetryableError` | Error prevented probe observation | | False | `Archiving` | The revision is being archived; owned objects are still being torn down | | False | `Archived` | Objects torn down after archival | | 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`). + ### Succeeded A terminal condition set once the rollout completes. It persists even if the revision later becomes unavailable, marking that this version was successfully deployed at least once. diff --git a/docs/draft/concepts/large-bundle-support.md b/docs/draft/concepts/large-bundle-support.md index 3bbf238c99..a407258549 100644 --- a/docs/draft/concepts/large-bundle-support.md +++ b/docs/draft/concepts/large-bundle-support.md @@ -416,8 +416,8 @@ The reconciler distinguishes two failure modes: - A referenced **Secret that does not exist yet** — for example, informer cache lag after creation, or a partially completed creation sequence — is treated as a - transient error. The ClusterObjectSet reports `Ready=False, Reason=RetryableError` - and the controller retries with backoff. + transient error. The ClusterObjectSet leaves its `Ready` condition unchanged and the + controller returns the error so it retries with backoff. - **Invalid referenced content** — a missing key, a malformed manifest, or content that has changed since first rollout — is an unmet precondition. The ClusterObjectSet reports `Ready=False, Reason=PreconditionsNotMet`. Because referenced Secrets are not 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 9f3004e66c..dec8cd06eb 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 @@ -565,7 +565,6 @@ spec: - False with reason PreconditionsNotMet: a precondition for applying the revision is not yet satisfied (a referenced Secret is mutable or has invalid or changed content, or a preflight dry-run was rejected, e.g. an immutable field, a conflicting object, or RBAC/dependency CRDs not yet propagated); the revision keeps retrying and recovers once the precondition is met. - False with reason InvalidSpecification: the revision spec is invalid (a malformed revision/phase object or progression probe) and requires a spec change to proceed. - False with reason ProgressDeadlineExceeded: the revision did not roll out within spec.progressDeadlineMinutes. - - False with reason RetryableError: the revision encountered a transient error that may resolve on a subsequent reconciliation. - False with reason Archiving: the revision is being archived and its owned objects are still being torn down. - False with reason Archived: the revision has been archived and its objects have been torn down. diff --git a/internal/object-controller/controllers/clusterobjectset_controller.go b/internal/object-controller/controllers/clusterobjectset_controller.go index 0ced85436b..9a066a6b84 100644 --- a/internal/object-controller/controllers/clusterobjectset_controller.go +++ b/internal/object-controller/controllers/clusterobjectset_controller.go @@ -150,10 +150,9 @@ func (c *ClusterObjectSetReconciler) reconcile(ctx context.Context, cos *ocv1.Cl markAsNotReady(cos, ocv1.ClusterObjectSetReasonPreconditionsNotMet, err.Error()) return ctrl.Result{RequeueAfter: validationRetryInterval}, nil } - // A Secret read error is transient; surface it as retryable and requeue with backoff. - werr := fmt.Errorf("verifying referenced Secrets: %v", err) - setRetryableErrorConditions(cos, werr.Error(), isDeadlineExceeded) - return ctrl.Result{}, werr + // A Secret read error is transient; leave the Ready condition unchanged and + // return the error for rate-limited backoff. + return ctrl.Result{}, fmt.Errorf("verifying referenced Secrets: %v", err) } phases, currentPhases, opts, err := c.buildBoxcutterPhases(ctx, cos) @@ -175,7 +174,6 @@ 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 } - setRetryableErrorConditions(cos, err.Error(), isDeadlineExceeded) return ctrl.Result{}, fmt.Errorf("converting to boxcutter revision: %v", err) } @@ -192,7 +190,6 @@ func (c *ClusterObjectSetReconciler) reconcile(ctx context.Context, cos *ocv1.Cl revisionEngine, err := c.RevisionEngineFactory.CreateRevisionEngine(ctx, cos) if err != nil { - setRetryableErrorConditions(cos, err.Error(), isDeadlineExceeded) return ctrl.Result{}, fmt.Errorf("failed to create revision engine: %v", err) } @@ -206,7 +203,6 @@ func (c *ClusterObjectSetReconciler) reconcile(ctx context.Context, cos *ocv1.Cl if cos.Spec.LifecycleState == ocv1.ClusterObjectSetLifecycleStateArchived { if err := c.TrackingCache.Free(ctx, cos); err != nil { - markAsNotReady(cos, ocv1.ClusterObjectSetReasonRetryableError, err.Error()) return ctrl.Result{}, fmt.Errorf("error stopping informers: %v", err) } return c.archive(ctx, revisionEngine, cos, revision) @@ -217,9 +213,7 @@ func (c *ClusterObjectSetReconciler) reconcile(ctx context.Context, cos *ocv1.Cl } if err := c.establishWatch(ctx, cos, revision); err != nil { - werr := fmt.Errorf("establish watch: %v", err) - setRetryableErrorConditions(cos, werr.Error(), isDeadlineExceeded) - return ctrl.Result{}, werr + return ctrl.Result{}, fmt.Errorf("establish watch: %v", err) } rres, err := revisionEngine.Reconcile(ctx, revision, opts...) @@ -228,7 +222,6 @@ func (c *ClusterObjectSetReconciler) reconcile(ctx context.Context, cos *ocv1.Cl // Log detailed reconcile reports only in debug mode (V(1)) to reduce verbosity. l.V(1).Info("reconcile report", "report", rres.String()) } - setRetryableErrorConditions(cos, err.Error(), isDeadlineExceeded) return ctrl.Result{}, fmt.Errorf("revision reconcile: %v", err) } @@ -393,7 +386,6 @@ func (c *ClusterObjectSetReconciler) reconcile(ctx context.Context, cos *ocv1.Cl func (c *ClusterObjectSetReconciler) delete(ctx context.Context, cos *ocv1.ClusterObjectSet) (ctrl.Result, error) { if err := c.TrackingCache.Free(ctx, cos); err != nil { - markAsNotReady(cos, ocv1.ClusterObjectSetReasonRetryableError, err.Error()) return ctrl.Result{}, fmt.Errorf("error stopping informers: %v", err) } if err := c.removeFinalizer(ctx, cos, clusterObjectSetTeardownFinalizer); err != nil { @@ -405,9 +397,9 @@ func (c *ClusterObjectSetReconciler) delete(ctx context.Context, cos *ocv1.Clust func (c *ClusterObjectSetReconciler) archive(ctx context.Context, revisionEngine RevisionEngine, cos *ocv1.ClusterObjectSet, revision boxcutter.RevisionBuilder) (ctrl.Result, error) { tdres, err := revisionEngine.Teardown(ctx, revision, machinerytypes.WithObserveAfterIncomplete{}) if err != nil { - err = fmt.Errorf("error archiving revision: %v", err) - setRetryableErrorConditions(cos, err.Error(), false) - return ctrl.Result{}, err + // Transient teardown error: leave the Ready condition unchanged and return + // the error for rate-limited backoff. + return ctrl.Result{}, fmt.Errorf("error archiving revision: %v", err) } // Set object counts status.FromTeardown(cos, tdres) @@ -775,10 +767,6 @@ func setReadyWithDeadline(cos *ocv1.ClusterObjectSet, status metav1.ConditionSta }) } -func setRetryableErrorConditions(cos *ocv1.ClusterObjectSet, message string, isDeadlineExceeded bool) { - setReadyWithDeadline(cos, metav1.ConditionFalse, ocv1.ClusterObjectSetReasonRetryableError, message, isDeadlineExceeded) -} - func markAsReady(cos *ocv1.ClusterObjectSet, reason, message string) bool { return meta.SetStatusCondition(&cos.Status.Conditions, metav1.Condition{ Type: ocv1.ClusterObjectSetTypeReady, diff --git a/internal/object-controller/controllers/clusterobjectset_controller_test.go b/internal/object-controller/controllers/clusterobjectset_controller_test.go index b3e1c83729..fabb88b1b6 100644 --- a/internal/object-controller/controllers/clusterobjectset_controller_test.go +++ b/internal/object-controller/controllers/clusterobjectset_controller_test.go @@ -69,7 +69,10 @@ func Test_ClusterObjectSetReconciler_Reconcile_RevisionReconciliation(t *testing }, }, { - name: "Ready condition is set to False/RetryableError on error when not previously set", + // 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", reconcilingRevisionName: clusterObjectSetName, revisionResult: newMockRevisionResult(mockCtrl, revisionResultConfig{}), revisionReconcileErr: errors.New("some error"), @@ -84,16 +87,12 @@ func Test_ClusterObjectSetReconciler_Reconcile_RevisionReconciliation(t *testing Name: clusterObjectSetName, }, rev) require.NoError(t, err) - cond := meta.FindStatusCondition(rev.Status.Conditions, ocv1.ClusterObjectSetTypeReady) - require.NotNil(t, cond) - require.Equal(t, metav1.ConditionFalse, cond.Status) - require.Equal(t, ocv1.ClusterObjectSetReasonRetryableError, cond.Reason) - require.Equal(t, "some error", cond.Message) - require.Equal(t, int64(1), cond.ObservedGeneration) + require.Nil(t, meta.FindStatusCondition(rev.Status.Conditions, ocv1.ClusterObjectSetTypeReady)) }, }, { - name: "Ready condition is set to False/RetryableError on error when previously set", + // A transient error must not overwrite an existing Ready condition. + name: "Ready condition is left unchanged on transient error when previously set", reconcilingRevisionName: clusterObjectSetName, revisionResult: newMockRevisionResult(mockCtrl, revisionResultConfig{}), revisionReconcileErr: errors.New("some error"), @@ -117,16 +116,16 @@ func Test_ClusterObjectSetReconciler_Reconcile_RevisionReconciliation(t *testing require.NoError(t, err) cond := meta.FindStatusCondition(rev.Status.Conditions, ocv1.ClusterObjectSetTypeReady) require.NotNil(t, cond) - require.Equal(t, metav1.ConditionFalse, cond.Status) - require.Equal(t, ocv1.ClusterObjectSetReasonRetryableError, cond.Reason) - require.Equal(t, "some error", cond.Message) - require.Equal(t, int64(1), cond.ObservedGeneration) + require.Equal(t, metav1.ConditionTrue, cond.Status) + require.Equal(t, ocv1.ClusterObjectSetReasonAllObjectsReady, cond.Reason) + require.Equal(t, "Revision 1 is rolled out.", cond.Message) }, }, { // A revision whose FIRST reconcile fails at engine creation (factory error) - // must set Ready=False/RetryableError even though Ready did not previously exist. - name: "Ready condition is set to False/RetryableError on factory error when not previously set", + // 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", reconcilingRevisionName: clusterObjectSetName, factoryErr: errors.New("failed to create revision engine"), existingObjs: func() []client.Object { @@ -140,10 +139,7 @@ func Test_ClusterObjectSetReconciler_Reconcile_RevisionReconciliation(t *testing Name: clusterObjectSetName, }, rev) require.NoError(t, err) - cond := meta.FindStatusCondition(rev.Status.Conditions, ocv1.ClusterObjectSetTypeReady) - require.NotNil(t, cond) - require.Equal(t, metav1.ConditionFalse, cond.Status) - require.Equal(t, ocv1.ClusterObjectSetReasonRetryableError, cond.Reason) + require.Nil(t, meta.FindStatusCondition(rev.Status.Conditions, ocv1.ClusterObjectSetTypeReady)) require.Nil(t, meta.FindStatusCondition(rev.Status.Conditions, "Progressing")) }, }, @@ -902,12 +898,12 @@ func Test_ClusterObjectSetReconciler_Reconcile_VerifyReferencedSecrets(t *testin wantReason string }{ { - // A transient, non-NotFound read error is retryable, not a validation failure. - name: "read error is retryable", - secrets: []client.Object{secret("secret-b", true)}, - failGet: "secret-a", - wantErr: true, - wantReason: ocv1.ClusterObjectSetReasonRetryableError, + // A transient, non-NotFound read error leaves the Ready condition untouched + // (absent here) and returns the error for backoff; it is not a validation failure. + name: "read error leaves Ready untouched", + secrets: []client.Object{secret("secret-b", true)}, + failGet: "secret-a", + wantErr: true, }, { name: "mutable Secret is an unmet precondition", @@ -960,18 +956,20 @@ func Test_ClusterObjectSetReconciler_Reconcile_VerifyReferencedSecrets(t *testin NamespacedName: types.NamespacedName{Name: clusterObjectSetName}, }) + updated := &ocv1.ClusterObjectSet{} if tc.wantErr { - // A read error is returned so controller-runtime requeues with backoff. + // A read error is returned so controller-runtime requeues with backoff, and + // the Ready condition is left untouched (absent on this first reconcile). require.Error(t, err) require.Equal(t, ctrl.Result{}, result) - } else { - // A confirmed mutable Secret is a validation failure that keeps retrying so the - // revision can recover once the immutable Secret is replaced. - require.NoError(t, err) - require.Equal(t, ctrl.Result{RequeueAfter: 10 * time.Second}, result) + require.NoError(t, testClient.Get(t.Context(), client.ObjectKey{Name: clusterObjectSetName}, updated)) + require.Nil(t, meta.FindStatusCondition(updated.Status.Conditions, ocv1.ClusterObjectSetTypeReady)) + return } - - updated := &ocv1.ClusterObjectSet{} + // A confirmed mutable Secret is a validation failure that keeps retrying so the + // revision can recover once the immutable Secret is replaced. + require.NoError(t, err) + require.Equal(t, ctrl.Result{RequeueAfter: 10 * time.Second}, result) require.NoError(t, testClient.Get(t.Context(), client.ObjectKey{Name: clusterObjectSetName}, updated)) cond := meta.FindStatusCondition(updated.Status.Conditions, ocv1.ClusterObjectSetTypeReady) require.NotNil(t, cond) @@ -1082,10 +1080,9 @@ func Test_ClusterObjectSetReconciler_Reconcile_BuildPhasesErrorClassification(t wantReason: ocv1.ClusterObjectSetReasonInvalidSpecification, }, { - name: "sibling revision list error is retryable", - listFails: true, - wantErr: true, - wantReason: ocv1.ClusterObjectSetReasonRetryableError, + name: "sibling revision list error leaves Ready untouched", + listFails: true, + wantErr: true, }, } { t.Run(tc.name, func(t *testing.T) { @@ -1122,14 +1119,16 @@ 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. + // Transient errors are returned so controller-runtime requeues with backoff, + // and the Ready condition is left untouched (absent on this first reconcile). require.Error(t, err) - } else { - require.NoError(t, err) + require.NoError(t, testClient.Get(t.Context(), client.ObjectKey{Name: clusterObjectSetName}, updated)) + require.Nil(t, meta.FindStatusCondition(updated.Status.Conditions, ocv1.ClusterObjectSetTypeReady)) + return } - - updated := &ocv1.ClusterObjectSet{} + require.NoError(t, err) require.NoError(t, testClient.Get(t.Context(), client.ObjectKey{Name: clusterObjectSetName}, updated)) cond := meta.FindStatusCondition(updated.Status.Conditions, ocv1.ClusterObjectSetTypeReady) require.NotNil(t, cond) @@ -1183,7 +1182,7 @@ func Test_ClusterObjectSetReconciler_Reconcile_ArchivalAndDeletion(t *testing.T) }, }, { - name: "set Ready:False:RetryableError when tracking cache free fails during deletion", + name: "Ready left untouched when tracking cache free fails during deletion", revisionResult: newMockRevisionResult(mockCtrl, revisionResultConfig{}), existingObjs: func() []client.Object { ext := newTestClusterExtension() @@ -1204,11 +1203,9 @@ func Test_ClusterObjectSetReconciler_Reconcile_ArchivalAndDeletion(t *testing.T) Name: clusterObjectSetName, }, rev) require.NoError(t, err) - cond := meta.FindStatusCondition(rev.Status.Conditions, ocv1.ClusterObjectSetTypeReady) - require.NotNil(t, cond) - require.Equal(t, metav1.ConditionFalse, cond.Status) - require.Equal(t, ocv1.ClusterObjectSetReasonRetryableError, cond.Reason) - require.Contains(t, cond.Message, "tracking cache free failed") + // A transient error during deletion is returned for backoff and does not + // write the Ready condition. + require.Nil(t, meta.FindStatusCondition(rev.Status.Conditions, ocv1.ClusterObjectSetTypeReady)) }, revisionEngineTeardownFn: func(ctrl *gomock.Controller) func(context.Context, machinerytypes.Revision, ...machinerytypes.RevisionTeardownOption) (machinery.RevisionTeardownResult, error) { return nil @@ -1284,7 +1281,7 @@ func Test_ClusterObjectSetReconciler_Reconcile_ArchivalAndDeletion(t *testing.T) }, }, { - name: "return error and set retrying conditions when archived revision teardown fails", + name: "return error and leave Ready untouched when archived revision teardown fails", revisionResult: newMockRevisionResult(mockCtrl, revisionResultConfig{}), existingObjs: func() []client.Object { ext := newTestClusterExtension() @@ -1307,18 +1304,16 @@ func Test_ClusterObjectSetReconciler_Reconcile_ArchivalAndDeletion(t *testing.T) Name: clusterObjectSetName, }, rev) require.NoError(t, err) - cond := meta.FindStatusCondition(rev.Status.Conditions, ocv1.ClusterObjectSetTypeReady) - require.NotNil(t, cond) - require.Equal(t, metav1.ConditionFalse, cond.Status) - require.Equal(t, ocv1.ClusterObjectSetReasonRetryableError, cond.Reason) - require.Contains(t, cond.Message, "teardown failed: connection refused") + // A teardown error is transient: the error is returned for backoff and the + // Ready condition is left untouched (absent here). + require.Nil(t, meta.FindStatusCondition(rev.Status.Conditions, ocv1.ClusterObjectSetTypeReady)) // Finalizer should still be present require.Contains(t, rev.Finalizers, "olm.operatorframework.io/teardown") }, }, { - name: "return error and set retrying conditions when factory fails to create engine during archived teardown", + name: "return error and leave Ready untouched when factory fails to create engine during archived teardown", revisionResult: newMockRevisionResult(mockCtrl, revisionResultConfig{}), existingObjs: func() []client.Object { ext := newTestClusterExtension() @@ -1340,11 +1335,9 @@ func Test_ClusterObjectSetReconciler_Reconcile_ArchivalAndDeletion(t *testing.T) Name: clusterObjectSetName, }, rev) require.NoError(t, err) - cond := meta.FindStatusCondition(rev.Status.Conditions, ocv1.ClusterObjectSetTypeReady) - require.NotNil(t, cond) - require.Equal(t, metav1.ConditionFalse, cond.Status) - require.Equal(t, ocv1.ClusterObjectSetReasonRetryableError, cond.Reason) - require.Contains(t, cond.Message, "token getter failed") + // A factory error is transient: the error is returned for backoff and the + // Ready condition is left untouched (absent here). + require.Nil(t, meta.FindStatusCondition(rev.Status.Conditions, ocv1.ClusterObjectSetTypeReady)) // Finalizer should still be present require.Contains(t, rev.Finalizers, "olm.operatorframework.io/teardown") diff --git a/internal/operator-controller/controllers/common_controller.go b/internal/operator-controller/controllers/common_controller.go index a5377cdd2e..327cc60275 100644 --- a/internal/operator-controller/controllers/common_controller.go +++ b/internal/operator-controller/controllers/common_controller.go @@ -144,7 +144,7 @@ func setProgressingFromReady(ext *ocv1.ClusterExtension, readyCond *metav1.Condi // // Returns Failed when: // - No rolling revisions exist (nothing to install) -// - The latest rolling revision has Ready condition with Reason: RetryableError (indicates an error occurred) +// - The latest rolling revision has Ready condition with Reason: ObjectCollision, PreconditionsNotMet, or InvalidSpecification (an error occurred) // // Returns Absent when: // - Rolling revisions exist with the latest not reporting an error reason on Ready (healthy phased rollout in progress) @@ -152,8 +152,9 @@ func setProgressingFromReady(ext *ocv1.ClusterExtension, readyCond *metav1.Condi // Rationale: // - Failed: Semantically indicates an error prevented installation // - Absent: Semantically indicates "not there yet" (neutral state, e.g., during healthy rollout) -// - RetryableError (transient), PreconditionsNotMet (a pre-apply precondition not yet met), or -// InvalidSpecification (an invalid revision spec) on Ready indicates an error +// - ObjectCollision (a foreign owner), PreconditionsNotMet (a pre-apply precondition not yet met), +// or InvalidSpecification (an invalid revision spec) on Ready indicates an error +// - A transient error leaves the Ready condition unchanged (or absent), so it maps to Absent, not Failed // - Other Ready reasons indicate healthy progress or terminal states handled elsewhere // - Only the LATEST revision matters - old errors superseded by newer healthy revisions should not cause Failed // @@ -166,12 +167,10 @@ func determineFailureReason(rollingRevisions []*RevisionMetadata) string { // Latest revision is the last element (sorted ascending by Spec.Revision). latestRevision := rollingRevisions[len(rollingRevisions)-1] readyCond := apimeta.FindStatusCondition(latestRevision.Conditions, ocv1.ClusterObjectSetTypeReady) - // RetryableError (transient error), ObjectCollision (an object is owned by another - // revision/controller), PreconditionsNotMet (a pre-apply precondition not yet met), or - // InvalidSpecification (an invalid revision spec) on the Ready condition all indicate an - // error prevented installation. - if readyCond != nil && (readyCond.Reason == ocv1.ClusterObjectSetReasonRetryableError || - readyCond.Reason == ocv1.ClusterObjectSetReasonObjectCollision || + // ObjectCollision (an object is owned by another revision/controller), + // PreconditionsNotMet (a pre-apply precondition not yet met), or InvalidSpecification + // (an invalid revision spec) on the Ready condition all indicate an error prevented installation. + if readyCond != nil && (readyCond.Reason == ocv1.ClusterObjectSetReasonObjectCollision || readyCond.Reason == ocv1.ClusterObjectSetReasonPreconditionsNotMet || readyCond.Reason == ocv1.ClusterObjectSetReasonInvalidSpecification) { return ocv1.ReasonFailed @@ -280,10 +279,10 @@ func progressingFromReady(ready *metav1.Condition, completed bool) metav1.Condit case ocv1.ReasonProgressDeadlineExceeded: cond.Status = metav1.ConditionFalse cond.Reason = ocv1.ReasonProgressDeadlineExceeded - case ocv1.ClusterObjectSetReasonRetryableError, ocv1.ClusterObjectSetReasonObjectCollision, ocv1.ClusterObjectSetReasonPreconditionsNotMet: - // A transient error (RetryableError), a foreign-owned object (ObjectCollision), or an - // unmet pre-apply precondition (PreconditionsNotMet) all prevent the rollout from - // proceeding, but the revision keeps retrying and recovers once the obstruction clears. + case ocv1.ClusterObjectSetReasonObjectCollision, ocv1.ClusterObjectSetReasonPreconditionsNotMet: + // A foreign-owned object (ObjectCollision) or an unmet pre-apply precondition + // (PreconditionsNotMet) prevents the rollout from proceeding, but the revision keeps + // retrying and recovers once the obstruction clears. cond.Reason = ocv1.ReasonRetrying default: // Any other (non-error) reason on a not-yet-complete revision is rolling out. diff --git a/internal/operator-controller/controllers/common_controller_test.go b/internal/operator-controller/controllers/common_controller_test.go index 494f72e044..3882d38e6c 100644 --- a/internal/operator-controller/controllers/common_controller_test.go +++ b/internal/operator-controller/controllers/common_controller_test.go @@ -317,7 +317,7 @@ func TestSetInstalledStatusFromRevisionStates_ConfigValidationError(t *testing.T }, }, { - name: "rolling revision with error (RetryableError) - uses Failed", + name: "rolling revision with error (ObjectCollision) - uses Failed", revisionStates: &RevisionStates{ Installed: nil, RollingOut: []*RevisionMetadata{ @@ -327,7 +327,7 @@ func TestSetInstalledStatusFromRevisionStates_ConfigValidationError(t *testing.T { Type: ocv1.ClusterObjectSetTypeReady, Status: metav1.ConditionUnknown, - Reason: ocv1.ClusterObjectSetReasonRetryableError, + Reason: ocv1.ClusterObjectSetReasonObjectCollision, Message: "some error occurred", }, }, @@ -341,7 +341,7 @@ func TestSetInstalledStatusFromRevisionStates_ConfigValidationError(t *testing.T }, }, { - name: "multiple rolling revisions with one RetryableError - uses Failed", + name: "multiple rolling revisions with one ObjectCollision - uses Failed", revisionStates: &RevisionStates{ Installed: nil, RollingOut: []*RevisionMetadata{ @@ -362,7 +362,7 @@ func TestSetInstalledStatusFromRevisionStates_ConfigValidationError(t *testing.T { Type: ocv1.ClusterObjectSetTypeReady, Status: metav1.ConditionUnknown, - Reason: ocv1.ClusterObjectSetReasonRetryableError, + Reason: ocv1.ClusterObjectSetReasonObjectCollision, Message: "validation error occurred", }, }, @@ -400,7 +400,7 @@ func TestSetInstalledStatusFromRevisionStates_ConfigValidationError(t *testing.T }, }, { - name: "old revision with RetryableError superseded by latest healthy - uses Absent", + name: "old revision with ObjectCollision superseded by latest healthy - uses Absent", revisionStates: &RevisionStates{ Installed: nil, RollingOut: []*RevisionMetadata{ @@ -410,7 +410,7 @@ func TestSetInstalledStatusFromRevisionStates_ConfigValidationError(t *testing.T { Type: ocv1.ClusterObjectSetTypeReady, Status: metav1.ConditionUnknown, - Reason: ocv1.ClusterObjectSetReasonRetryableError, + Reason: ocv1.ClusterObjectSetReasonObjectCollision, Message: "old error that was superseded", }, }, @@ -466,8 +466,16 @@ func TestDetermineFailureReason(t *testing.T) { }{ {name: "no rolling revisions -> Failed", rolling: nil, expected: ocv1.ReasonFailed}, { - name: "latest retryable error -> Failed", - rolling: []*RevisionMetadata{{Conditions: availCond(metav1.ConditionUnknown, ocv1.ClusterObjectSetReasonRetryableError)}}, + // First-reconcile transient error: the latest revision has no Ready condition + // yet. It must map to Absent, not Failed, so a transient COS error never + // surfaces as ClusterExtension Installed=Failed. + name: "latest with no Ready condition -> Absent", + rolling: []*RevisionMetadata{{Conditions: nil}}, + expected: ocv1.ReasonAbsent, + }, + { + name: "latest object collision -> Failed", + rolling: []*RevisionMetadata{{Conditions: availCond(metav1.ConditionUnknown, ocv1.ClusterObjectSetReasonObjectCollision)}}, expected: ocv1.ReasonFailed, }, { @@ -493,7 +501,7 @@ func TestDetermineFailureReason(t *testing.T) { { name: "only the latest revision matters", rolling: []*RevisionMetadata{ - {Conditions: availCond(metav1.ConditionUnknown, ocv1.ClusterObjectSetReasonRetryableError)}, + {Conditions: availCond(metav1.ConditionUnknown, ocv1.ClusterObjectSetReasonObjectCollision)}, {Conditions: availCond(metav1.ConditionFalse, ocv1.ReasonRollingOut)}, }, expected: ocv1.ReasonAbsent, @@ -543,8 +551,8 @@ func TestProgressingFromReady(t *testing.T) { expected: metav1.Condition{Type: ocv1.TypeProgressing, Status: metav1.ConditionTrue, Reason: ocv1.ReasonRollingOut, Message: "The object set is being archived; removing owned objects."}, }, { - name: "retryable error maps to Retrying", - ready: &metav1.Condition{Type: ocv1.ClusterObjectSetTypeReady, Status: metav1.ConditionUnknown, Reason: ocv1.ClusterObjectSetReasonRetryableError, Message: "boom"}, + name: "object collision maps to Retrying", + ready: &metav1.Condition{Type: ocv1.ClusterObjectSetTypeReady, Status: metav1.ConditionUnknown, Reason: ocv1.ClusterObjectSetReasonObjectCollision, Message: "boom"}, completed: false, expected: metav1.Condition{Type: ocv1.TypeProgressing, Status: metav1.ConditionTrue, Reason: ocv1.ReasonRetrying, Message: "boom"}, }, diff --git a/manifests/experimental-e2e.yaml b/manifests/experimental-e2e.yaml index 4cb587958a..062ec31cb7 100644 --- a/manifests/experimental-e2e.yaml +++ b/manifests/experimental-e2e.yaml @@ -1976,7 +1976,6 @@ spec: - False with reason PreconditionsNotMet: a precondition for applying the revision is not yet satisfied (a referenced Secret is mutable or has invalid or changed content, or a preflight dry-run was rejected, e.g. an immutable field, a conflicting object, or RBAC/dependency CRDs not yet propagated); the revision keeps retrying and recovers once the precondition is met. - False with reason InvalidSpecification: the revision spec is invalid (a malformed revision/phase object or progression probe) and requires a spec change to proceed. - False with reason ProgressDeadlineExceeded: the revision did not roll out within spec.progressDeadlineMinutes. - - False with reason RetryableError: the revision encountered a transient error that may resolve on a subsequent reconciliation. - False with reason Archiving: the revision is being archived and its owned objects are still being torn down. - False with reason Archived: the revision has been archived and its objects have been torn down. diff --git a/manifests/experimental.yaml b/manifests/experimental.yaml index e9ddd942cd..cd9328ce7d 100644 --- a/manifests/experimental.yaml +++ b/manifests/experimental.yaml @@ -1937,7 +1937,6 @@ spec: - False with reason PreconditionsNotMet: a precondition for applying the revision is not yet satisfied (a referenced Secret is mutable or has invalid or changed content, or a preflight dry-run was rejected, e.g. an immutable field, a conflicting object, or RBAC/dependency CRDs not yet propagated); the revision keeps retrying and recovers once the precondition is met. - False with reason InvalidSpecification: the revision spec is invalid (a malformed revision/phase object or progression probe) and requires a spec change to proceed. - False with reason ProgressDeadlineExceeded: the revision did not roll out within spec.progressDeadlineMinutes. - - False with reason RetryableError: the revision encountered a transient error that may resolve on a subsequent reconciliation. - False with reason Archiving: the revision is being archived and its owned objects are still being torn down. - False with reason Archived: the revision has been archived and its objects have been torn down.