diff --git a/api/v1/clusterobjectset_types.go b/api/v1/clusterobjectset_types.go index 5f54a1c02d..f7393da1c3 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" ClusterObjectSetReasonValidationFailure = "ValidationFailure" ClusterObjectSetReasonAllObjectsReady = "AllObjectsReady" ) @@ -506,10 +505,12 @@ type ClusterObjectSetStatus struct { // - False with reason ObjectCollision: one or more objects are controlled by another owner, or are unowned while collision protection prevents adoption, so this revision cannot take ownership of them. // - False with reason ValidationFailure: the revision failed a pre-apply check (an invalid revision/phase object or probe, preflight validation, a mutable referenced Secret, or changed referenced content) and requires manual intervention. // - 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. // + // A transient internal error leaves the Ready condition unchanged and is retried + // with backoff; it is not reported as a distinct reason. + // // Rollout completion is recorded separately by status.completedAt. // // +listType=map diff --git a/applyconfigurations/api/v1/clusterobjectsetstatus.go b/applyconfigurations/api/v1/clusterobjectsetstatus.go index ed8ed67aef..9289c630d5 100644 --- a/applyconfigurations/api/v1/clusterobjectsetstatus.go +++ b/applyconfigurations/api/v1/clusterobjectsetstatus.go @@ -45,10 +45,12 @@ type ClusterObjectSetStatusApplyConfiguration struct { // - False with reason ObjectCollision: one or more objects are controlled by another owner, or are unowned while collision protection prevents adoption, so this revision cannot take ownership of them. // - False with reason ValidationFailure: the revision failed a pre-apply check (an invalid revision/phase object or probe, preflight validation, a mutable referenced Secret, or changed referenced content) and requires manual intervention. // - 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. // + // A transient internal error leaves the Ready condition unchanged and is retried + // with backoff; it is not reported as a distinct reason. + // // Rollout completion is recorded separately by status.completedAt. Conditions []metav1.ConditionApplyConfiguration `json:"conditions,omitempty"` // observedPhases records the content hashes of resolved phases diff --git a/docs/draft/concepts/clusterobjectsets.md b/docs/draft/concepts/clusterobjectsets.md index ae380a6ac6..469b1883e6 100644 --- a/docs/draft/concepts/clusterobjectsets.md +++ b/docs/draft/concepts/clusterobjectsets.md @@ -221,11 +221,12 @@ Indicates whether all objects in the revision have been successfully rolled out | False | `Superseded` | One or more objects have been adopted by a newer revision of the object set | | False | `ObjectCollision` | One or more objects are controlled by another owner, or are unowned and collision protection prevents adoption | | False | `ValidationFailure` | A pre-apply check failed (an invalid revision/phase object or probe, preflight validation, a mutable referenced Secret, or changed referenced content); requires manual intervention | -| 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 554e2a5b8f..be8fdb6442 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 a validation failure. The ClusterObjectSet reports `Ready=False, Reason=ValidationFailure`. 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 9fb96efbb3..f38d068401 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 @@ -564,10 +564,12 @@ spec: - False with reason ObjectCollision: one or more objects are controlled by another owner, or are unowned while collision protection prevents adoption, so this revision cannot take ownership of them. - False with reason ValidationFailure: the revision failed a pre-apply check (an invalid revision/phase object or probe, preflight validation, a mutable referenced Secret, or changed referenced content) and requires manual intervention. - 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. + A transient internal error leaves the Ready condition unchanged and is retried + with backoff; it is not reported as a distinct reason. + Rollout completion is recorded separately by status.completedAt. items: description: Condition contains details for one aspect of the current diff --git a/internal/object-controller/controllers/clusterobjectset_controller.go b/internal/object-controller/controllers/clusterobjectset_controller.go index f4f2880ddb..0251f397ff 100644 --- a/internal/object-controller/controllers/clusterobjectset_controller.go +++ b/internal/object-controller/controllers/clusterobjectset_controller.go @@ -149,10 +149,9 @@ func (c *ClusterObjectSetReconciler) reconcile(ctx context.Context, cos *ocv1.Cl markAsNotReady(cos, ocv1.ClusterObjectSetReasonValidationFailure, 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) @@ -174,7 +173,6 @@ func (c *ClusterObjectSetReconciler) reconcile(ctx context.Context, cos *ocv1.Cl markAsNotReady(cos, ocv1.ClusterObjectSetReasonValidationFailure, err.Error()) return ctrl.Result{}, nil } - setRetryableErrorConditions(cos, err.Error(), isDeadlineExceeded) return ctrl.Result{}, fmt.Errorf("converting to boxcutter revision: %v", err) } @@ -191,7 +189,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) } @@ -205,7 +202,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) @@ -216,9 +212,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...) @@ -227,7 +221,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) } @@ -389,7 +382,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 { @@ -401,9 +393,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) } if tdres != nil && !tdres.IsComplete() { markAsArchiving(cos) @@ -769,10 +761,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 34c532ce2d..3038d58b81 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 a validation failure", @@ -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.ClusterObjectSetReasonValidationFailure, }, { - 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 c6cd38c761..3f547113c2 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 or ValidationFailure (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,7 +152,8 @@ 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) or ValidationFailure (pre-apply check failed) on Ready indicates an error +// - ObjectCollision (a foreign owner) or ValidationFailure (pre-apply check failed) 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 // @@ -165,10 +166,11 @@ 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) or ObjectCollision (an object is owned by another - // revision/controller) on the Ready condition both indicate an error prevented installation. - if readyCond != nil && (readyCond.Reason == ocv1.ClusterObjectSetReasonRetryableError || - readyCond.Reason == ocv1.ClusterObjectSetReasonObjectCollision || readyCond.Reason == ocv1.ClusterObjectSetReasonValidationFailure) { + // ObjectCollision (an object is owned by another revision/controller) or + // ValidationFailure (a failed pre-apply check) on the Ready condition both + // indicate an error prevented installation. + if readyCond != nil && (readyCond.Reason == ocv1.ClusterObjectSetReasonObjectCollision || + readyCond.Reason == ocv1.ClusterObjectSetReasonValidationFailure) { return ocv1.ReasonFailed } @@ -273,7 +275,7 @@ 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: + case ocv1.ClusterObjectSetReasonObjectCollision: 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 0263975e7f..e84d21be61 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, }, { @@ -488,7 +496,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, @@ -538,8 +546,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 1ab1aa4887..df8dafda52 100644 --- a/manifests/experimental-e2e.yaml +++ b/manifests/experimental-e2e.yaml @@ -1975,10 +1975,12 @@ spec: - False with reason ObjectCollision: one or more objects are controlled by another owner, or are unowned while collision protection prevents adoption, so this revision cannot take ownership of them. - False with reason ValidationFailure: the revision failed a pre-apply check (an invalid revision/phase object or probe, preflight validation, a mutable referenced Secret, or changed referenced content) and requires manual intervention. - 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. + A transient internal error leaves the Ready condition unchanged and is retried + with backoff; it is not reported as a distinct reason. + Rollout completion is recorded separately by status.completedAt. items: description: Condition contains details for one aspect of the current diff --git a/manifests/experimental.yaml b/manifests/experimental.yaml index ffa7dab444..ef7d02e5c7 100644 --- a/manifests/experimental.yaml +++ b/manifests/experimental.yaml @@ -1936,10 +1936,12 @@ spec: - False with reason ObjectCollision: one or more objects are controlled by another owner, or are unowned while collision protection prevents adoption, so this revision cannot take ownership of them. - False with reason ValidationFailure: the revision failed a pre-apply check (an invalid revision/phase object or probe, preflight validation, a mutable referenced Secret, or changed referenced content) and requires manual intervention. - 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. + A transient internal error leaves the Ready condition unchanged and is retried + with backoff; it is not reported as a distinct reason. + Rollout completion is recorded separately by status.completedAt. items: description: Condition contains details for one aspect of the current