From 019750efcd2cbe1e9f8c9332ea4d8745274be4be Mon Sep 17 00:00:00 2001 From: "Per G. da Silva" Date: Thu, 15 Oct 2026 16:59:52 +0200 Subject: [PATCH] =?UTF-8?q?=E2=9A=A0=20split=20ClusterObjectSet=20Ready=20?= =?UTF-8?q?ValidationFailure=20reason?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace the overloaded ValidationFailure reason with two reasons that distinguish a retryable obstruction from a spec-edit-required error: - PreconditionsNotMet: a pre-apply precondition is not yet satisfied (a missing/mutable/changed referenced Secret, or a preflight dry-run rejected by an immutable field, a conflicting object, or RBAC / dependency CRDs not yet propagated). The reconciler requeues on a timer and the revision recovers once the precondition is met. - InvalidSpecification: the revision spec is invalid (a malformed revision/phase object or progression probe). The reconciler does not requeue; a spec change is required. ClusterExtension reconstruction maps PreconditionsNotMet to Progressing=True/Retrying and InvalidSpecification to Progressing=False/Blocked, so a recoverable obstruction and a spec-edit-required error are no longer conflated under one reason. Each reason carries a curated, action-oriented condition message (what failed and how to resolve it); the raw error is logged at V(1). These messages also surface on the ClusterExtension Progressing condition. Experimental API only. Co-Authored-By: Claude Opus 4.8 Signed-off-by: Per G. da Silva --- api/v1/clusterobjectset_types.go | 20 ++++++----- .../api/v1/clusterobjectsetstatus.go | 3 +- docs/draft/concepts/clusterobjectsets.md | 3 +- docs/draft/concepts/large-bundle-support.md | 8 ++--- ...peratorframework.io_clusterobjectsets.yaml | 3 +- .../clusterobjectset_controller.go | 36 +++++++++---------- ...usterobjectset_controller_internal_test.go | 2 +- .../clusterobjectset_controller_test.go | 22 ++++++------ .../controllers/resolve_ref_test.go | 12 +++---- .../controllers/common_controller.go | 22 ++++++++---- .../controllers/common_controller_test.go | 19 +++++++--- manifests/experimental-e2e.yaml | 3 +- manifests/experimental.yaml | 3 +- test/e2e/features/revision.feature | 10 +++--- 14 files changed, 97 insertions(+), 69 deletions(-) diff --git a/api/v1/clusterobjectset_types.go b/api/v1/clusterobjectset_types.go index d3ee70d0c7..f89d2bb5aa 100644 --- a/api/v1/clusterobjectset_types.go +++ b/api/v1/clusterobjectset_types.go @@ -29,14 +29,15 @@ const ( ClusterObjectSetTypeReady = "Ready" // Condition Reasons - ClusterObjectSetReasonArchived = "Archived" - ClusterObjectSetReasonArchiving = "Archiving" - ClusterObjectSetReasonObjectsNotReady = "ObjectsNotReady" - ClusterObjectSetReasonSuperseded = "Superseded" - ClusterObjectSetReasonObjectCollision = "ObjectCollision" - ClusterObjectSetReasonRetryableError = "RetryableError" - ClusterObjectSetReasonValidationFailure = "ValidationFailure" - ClusterObjectSetReasonAllObjectsReady = "AllObjectsReady" + ClusterObjectSetReasonArchived = "Archived" + ClusterObjectSetReasonArchiving = "Archiving" + ClusterObjectSetReasonObjectsNotReady = "ObjectsNotReady" + ClusterObjectSetReasonSuperseded = "Superseded" + ClusterObjectSetReasonObjectCollision = "ObjectCollision" + ClusterObjectSetReasonRetryableError = "RetryableError" + ClusterObjectSetReasonPreconditionsNotMet = "PreconditionsNotMet" + ClusterObjectSetReasonInvalidSpecification = "InvalidSpecification" + ClusterObjectSetReasonAllObjectsReady = "AllObjectsReady" ) // ClusterObjectSetSpec defines the desired state of ClusterObjectSet. @@ -504,7 +505,8 @@ type ClusterObjectSetStatus struct { // - 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. // - 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 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. diff --git a/applyconfigurations/api/v1/clusterobjectsetstatus.go b/applyconfigurations/api/v1/clusterobjectsetstatus.go index b9573d88d9..f57fa46bd2 100644 --- a/applyconfigurations/api/v1/clusterobjectsetstatus.go +++ b/applyconfigurations/api/v1/clusterobjectsetstatus.go @@ -43,7 +43,8 @@ type ClusterObjectSetStatusApplyConfiguration struct { // - 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. // - 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 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. diff --git a/docs/draft/concepts/clusterobjectsets.md b/docs/draft/concepts/clusterobjectsets.md index ae380a6ac6..dd0e666f8a 100644 --- a/docs/draft/concepts/clusterobjectsets.md +++ b/docs/draft/concepts/clusterobjectsets.md @@ -220,7 +220,8 @@ Indicates whether all objects in the revision have been successfully rolled out | False | `ObjectsNotReady` | One or more objects are still being applied or have not yet passed their readiness probes during rollout | | 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 | +| 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 | diff --git a/docs/draft/concepts/large-bundle-support.md b/docs/draft/concepts/large-bundle-support.md index 554e2a5b8f..3bbf238c99 100644 --- a/docs/draft/concepts/large-bundle-support.md +++ b/docs/draft/concepts/large-bundle-support.md @@ -150,7 +150,7 @@ Recommended conventions: 2. **Immutability**: Secrets must set `immutable: true`. The reconciler verifies that all referenced Secrets have `immutable: true` set before proceeding. Mutable referenced Secrets are rejected and the ClusterObjectSet reports - `Ready=False, Reason=ValidationFailure`. Additionally, the reconciler records + `Ready=False, Reason=PreconditionsNotMet`. Additionally, the reconciler records content hashes of the resolved phases on first successful reconciliation and reports the same condition if the content changes (e.g., if a Secret is deleted and recreated with the same name but different data). @@ -397,7 +397,7 @@ referenced Secrets have `immutable: true` set. After successfully building the phases (resolving all refs), the reconciler computes a per-phase content digest and compares it against the digests recorded in `.status.observedPhases` (if present). If any phase's content has changed, the ClusterObjectSet reports -`Ready=False, Reason=ValidationFailure`. On first successful build, phase +`Ready=False, Reason=PreconditionsNotMet`. On first successful build, phase content digests are persisted to status for future comparisons. When processing a COS phase: @@ -419,8 +419,8 @@ The reconciler distinguishes two failure modes: transient error. The ClusterObjectSet reports `Ready=False, Reason=RetryableError` and the controller 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 + that has changed since first rollout — is an unmet precondition. The ClusterObjectSet + reports `Ready=False, Reason=PreconditionsNotMet`. Because referenced Secrets are not watched and the `ref` is immutable, the reconciler retries this case periodically so the revision recovers automatically once the referenced Secret is replaced with corrected content. Referenced Secrets are immutable, so correcting the content means 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 4434b754cc..9f3004e66c 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 @@ -562,7 +562,8 @@ spec: - 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. - 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 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. diff --git a/internal/object-controller/controllers/clusterobjectset_controller.go b/internal/object-controller/controllers/clusterobjectset_controller.go index 2370b2fecb..0ced85436b 100644 --- a/internal/object-controller/controllers/clusterobjectset_controller.go +++ b/internal/object-controller/controllers/clusterobjectset_controller.go @@ -49,7 +49,7 @@ const ( clusterObjectSetTeardownFinalizer = "olm.operatorframework.io/teardown" // validationRetryInterval is how often the reconciler re-checks a revision that is - // Ready=False/ValidationFailure. These failures stem from inputs that are not watched + // Ready=False/PreconditionsNotMet. These failures stem from inputs that are not watched // — a mutable or changed referenced Secret, an invalid referenced manifest, or a // preflight dry-run rejected until RBAC or a dependency CRD propagates — and the ref // spec is immutable, so a periodic retry lets the revision recover (once the Secret is @@ -145,9 +145,9 @@ func (c *ClusterObjectSetReconciler) reconcile(ctx context.Context, cos *ocv1.Cl if errors.As(err, &mutErr) { // A confirmed mutable Secret requires user action (replacing the immutable Secret), // which is not watched, so keep retrying so the revision can recover. - // ValidationFailure takes precedence over ProgressDeadlineExceeded: it is more actionable. + // PreconditionsNotMet takes precedence over ProgressDeadlineExceeded: it is more actionable. l.Error(err, "referenced Secret is mutable, marking revision not ready") - markAsNotReady(cos, ocv1.ClusterObjectSetReasonValidationFailure, err.Error()) + 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. @@ -164,7 +164,7 @@ func (c *ClusterObjectSetReconciler) reconcile(ctx context.Context, cos *ocv1.Cl // is not watched and the ref is immutable, so keep retrying so the revision can // recover once the Secret is replaced with valid content. l.Error(err, "invalid referenced content, marking revision not ready") - markAsNotReady(cos, ocv1.ClusterObjectSetReasonValidationFailure, err.Error()) + markAsNotReady(cos, ocv1.ClusterObjectSetReasonPreconditionsNotMet, fmt.Sprintf("Referenced content is invalid: %v. Fix the Secret content.", err)) return ctrl.Result{RequeueAfter: validationRetryInterval}, nil } var cfgErr *configValidationError @@ -172,7 +172,7 @@ func (c *ClusterObjectSetReconciler) reconcile(ctx context.Context, cos *ocv1.Cl // Invalid revision spec; a spec edit (or a new revision) re-triggers reconciliation, // so no periodic retry is needed. l.Error(err, "invalid configuration, marking revision not ready") - markAsNotReady(cos, ocv1.ClusterObjectSetReasonValidationFailure, err.Error()) + markAsNotReady(cos, ocv1.ClusterObjectSetReasonInvalidSpecification, fmt.Sprintf("The object set spec is invalid: %v.", err)) return ctrl.Result{}, nil } setRetryableErrorConditions(cos, err.Error(), isDeadlineExceeded) @@ -186,7 +186,7 @@ func (c *ClusterObjectSetReconciler) reconcile(ctx context.Context, cos *ocv1.Cl // Secrets are not watched, so keep retrying so the revision can recover once the // original content is restored. l.Error(err, "resolved phases content changed, marking revision not ready") - markAsNotReady(cos, ocv1.ClusterObjectSetReasonValidationFailure, err.Error()) + markAsNotReady(cos, ocv1.ClusterObjectSetReasonPreconditionsNotMet, fmt.Sprintf("Referenced Secret content no longer matches the original rollout: %v. Restore the original content.", err)) return ctrl.Result{RequeueAfter: validationRetryInterval}, nil } @@ -240,7 +240,7 @@ func (c *ClusterObjectSetReconciler) reconcile(ctx context.Context, cos *ocv1.Cl // or a dependency CRD still propagating), and those inputs are not watched. if verr := rres.GetValidationError(); verr != nil { l.Error(fmt.Errorf("%w", verr), "preflight validation failed, retrying after 10s") - markAsNotReady(cos, ocv1.ClusterObjectSetReasonValidationFailure, fmt.Sprintf("revision validation error: %s", verr)) + markAsNotReady(cos, ocv1.ClusterObjectSetReasonPreconditionsNotMet, fmt.Sprintf("A pre-apply check rejected the revision: %v.", verr)) return ctrl.Result{RequeueAfter: validationRetryInterval}, nil } @@ -263,7 +263,7 @@ func (c *ClusterObjectSetReconciler) reconcile(ctx context.Context, cos *ocv1.Cl continue } l.Error(fmt.Errorf("%w", verr), "phase preflight validation failed, retrying after 10s", "phase", pres.GetName()) - markAsNotReady(cos, ocv1.ClusterObjectSetReasonValidationFailure, fmt.Sprintf("phase %q validation error: %s", pres.GetName(), verr)) + markAsNotReady(cos, ocv1.ClusterObjectSetReasonPreconditionsNotMet, fmt.Sprintf("A pre-apply check rejected phase %q: %v.", pres.GetName(), verr)) return ctrl.Result{RequeueAfter: validationRetryInterval}, nil } @@ -564,14 +564,14 @@ func (c *ClusterObjectSetReconciler) listOtherActiveRevisions( // configValidationError marks an error caused by invalid user-provided configuration in the // revision spec (a malformed revision/phase object or a bad progression probe). The spec is -// either immutable or watched, so the reconciler reports it as Ready=False/ValidationFailure +// either immutable or watched, so the reconciler reports it as Ready=False/InvalidSpecification // without a periodic retry: a spec edit re-triggers reconciliation. type configValidationError struct{ error } // referencedContentError marks a validation failure caused by the content of a referenced // Secret (a missing key or a malformed manifest). Referenced Secrets are not watched and the // phase/ref spec is immutable, so the failure can only clear when the Secret is replaced. -// The reconciler reports it as Ready=False/ValidationFailure but keeps retrying periodically +// The reconciler reports it as Ready=False/PreconditionsNotMet but keeps retrying periodically // so the revision can recover without a controller restart. type referencedContentError struct{ error } @@ -614,7 +614,7 @@ func (c *ClusterObjectSetReconciler) buildBoxcutterPhases(ctx context.Context, c } obj = resolved default: - return nil, nil, nil, &configValidationError{fmt.Errorf("object in phase %q has neither object nor ref", specPhase.Name)} + return nil, nil, nil, &configValidationError{fmt.Errorf("an object in phase %q has neither an inline object nor a Secret reference", specPhase.Name)} } objs = append(objs, obj) @@ -724,7 +724,7 @@ func buildProgressionProbes(progressionProbes []ocv1.ProgressionProbe) (probing. fieldValueProbe := probing.FieldValueProbe(probe.FieldValue) assertions = append(assertions, &fieldValueProbe) default: - return nil, &configValidationError{fmt.Errorf("unknown progressionProbe assertion probe type: %s", probe.Type)} + return nil, &configValidationError{fmt.Errorf("unknown progression probe assertion type %q", probe.Type)} } } @@ -740,14 +740,14 @@ func buildProgressionProbes(progressionProbes []ocv1.ProgressionProbe) (probing. case ocv1.SelectorTypeLabel: selector, err := metav1.LabelSelectorAsSelector(&progressionProbe.Selector.Label) if err != nil { - return nil, &configValidationError{fmt.Errorf("invalid label selector in progressionProbe (%v): %w", progressionProbe.Selector.Label, err)} + return nil, &configValidationError{fmt.Errorf("a progression probe has an invalid label selector (%v): %w", progressionProbe.Selector.Label, err)} } selectorProbe = &probing.LabelSelector{ Selector: selector, Prober: assertions, } default: - return nil, &configValidationError{fmt.Errorf("unknown progressionProbe selector type: %s", progressionProbe.Selector.Type)} + return nil, &configValidationError{fmt.Errorf("unknown progression probe selector type %q", progressionProbe.Selector.Type)} } userProbes = append(userProbes, &probing.ObservedGenerationProbe{ Prober: selectorProbe, @@ -868,9 +868,9 @@ func verifyObservedPhases(stored, current []ocv1.ObservedPhase) error { } // mutableSecretError is returned by verifyReferencedSecretsImmutable when one or -// more referenced Secrets are confirmed to be mutable. It distinguishes a -// (non-retryable) validation failure from a transient Secret read error, so the -// caller can reserve ValidationFailure for the former and retry the latter. +// more referenced Secrets are confirmed to be mutable. It distinguishes a confirmed +// precondition failure (reported as PreconditionsNotMet and retried on a timer) from +// a transient Secret read error (surfaced as a plain error for backoff requeue). type mutableSecretError struct { msg string } @@ -929,7 +929,7 @@ func (c *ClusterObjectSetReconciler) verifyReferencedSecretsImmutable(ctx contex if len(mutableSecrets) > 0 { return &mutableSecretError{msg: fmt.Sprintf( - "the following secrets are not immutable (referenced secrets must have immutable set to true): %s", + "Referenced Secret(s) must be immutable but are not: %s. Recreate them with immutable set to true.", strings.Join(mutableSecrets, ", "))} } diff --git a/internal/object-controller/controllers/clusterobjectset_controller_internal_test.go b/internal/object-controller/controllers/clusterobjectset_controller_internal_test.go index 4c6d5595d1..eda064db36 100644 --- a/internal/object-controller/controllers/clusterobjectset_controller_internal_test.go +++ b/internal/object-controller/controllers/clusterobjectset_controller_internal_test.go @@ -468,7 +468,7 @@ func TestVerifyReferencedSecretsImmutable(t *testing.T) { err := reconciler.verifyReferencedSecretsImmutable(t.Context(), cos) require.Error(t, err) - assert.Contains(t, err.Error(), "not immutable") + assert.Contains(t, err.Error(), "must be immutable but are not") }) t.Run("skips phases with inline objects only", func(t *testing.T) { diff --git a/internal/object-controller/controllers/clusterobjectset_controller_test.go b/internal/object-controller/controllers/clusterobjectset_controller_test.go index 34c532ce2d..b3e1c83729 100644 --- a/internal/object-controller/controllers/clusterobjectset_controller_test.go +++ b/internal/object-controller/controllers/clusterobjectset_controller_test.go @@ -208,8 +208,8 @@ func Test_ClusterObjectSetReconciler_Reconcile_RevisionReconciliation(t *testing cond := meta.FindStatusCondition(rev.Status.Conditions, ocv1.ClusterObjectSetTypeReady) require.NotNil(t, cond) require.Equal(t, metav1.ConditionFalse, cond.Status) - require.NotEqual(t, ocv1.ClusterObjectSetReasonValidationFailure, cond.Reason, - "a paused downstream phase's validation error must not be reported as a terminal validation failure") + require.NotEqual(t, ocv1.ClusterObjectSetReasonPreconditionsNotMet, cond.Reason, + "a paused downstream phase's validation error must not be reported as an unmet-preconditions failure") require.Equal(t, ocv1.ClusterObjectSetReasonObjectsNotReady, cond.Reason) }, }, @@ -867,13 +867,13 @@ func Test_ClusterObjectSetReconciler_Reconcile_ValidationError_Retries(t *testin }, result) require.NoError(t, err) - // preflight validation failures surface as Ready=False/ValidationFailure. + // preflight validation failures surface as Ready=False/PreconditionsNotMet. updated := &ocv1.ClusterObjectSet{} require.NoError(t, testClient.Get(t.Context(), client.ObjectKey{Name: clusterObjectSetName}, updated)) cond := meta.FindStatusCondition(updated.Status.Conditions, ocv1.ClusterObjectSetTypeReady) require.NotNil(t, cond) require.Equal(t, metav1.ConditionFalse, cond.Status) - require.Equal(t, ocv1.ClusterObjectSetReasonValidationFailure, cond.Reason) + require.Equal(t, ocv1.ClusterObjectSetReasonPreconditionsNotMet, cond.Reason) }) } } @@ -910,10 +910,10 @@ func Test_ClusterObjectSetReconciler_Reconcile_VerifyReferencedSecrets(t *testin wantReason: ocv1.ClusterObjectSetReasonRetryableError, }, { - name: "mutable Secret is a validation failure", + name: "mutable Secret is an unmet precondition", secrets: []client.Object{secret("secret-a", true), secret("secret-b", false)}, wantErr: false, - wantReason: ocv1.ClusterObjectSetReasonValidationFailure, + wantReason: ocv1.ClusterObjectSetReasonPreconditionsNotMet, }, { // A confirmed mutable Secret takes precedence over a concurrent read error. @@ -921,7 +921,7 @@ func Test_ClusterObjectSetReconciler_Reconcile_VerifyReferencedSecrets(t *testin secrets: []client.Object{secret("secret-b", false)}, failGet: "secret-a", wantErr: false, - wantReason: ocv1.ClusterObjectSetReasonValidationFailure, + wantReason: ocv1.ClusterObjectSetReasonPreconditionsNotMet, }, } { t.Run(tc.name, func(t *testing.T) { @@ -1022,7 +1022,7 @@ func Test_ClusterObjectSetReconciler_Reconcile_ReferencedContentValidationRecove } req := ctrl.Request{NamespacedName: types.NamespacedName{Name: clusterObjectSetName}} - // Malformed referenced content is a validation failure that keeps retrying so the + // Malformed referenced content is an unmet precondition that keeps retrying so the // revision can recover once the Secret is replaced. result, err := r.Reconcile(t.Context(), req) require.NoError(t, err) @@ -1033,7 +1033,7 @@ func Test_ClusterObjectSetReconciler_Reconcile_ReferencedContentValidationRecove cond := meta.FindStatusCondition(rev.Status.Conditions, ocv1.ClusterObjectSetTypeReady) require.NotNil(t, cond) require.Equal(t, metav1.ConditionFalse, cond.Status) - require.Equal(t, ocv1.ClusterObjectSetReasonValidationFailure, cond.Reason) + require.Equal(t, ocv1.ClusterObjectSetReasonPreconditionsNotMet, cond.Reason) // No phase digest is recorded while the content is invalid, so recovery is unobstructed. require.Empty(t, rev.Status.ObservedPhases) @@ -1072,14 +1072,14 @@ func Test_ClusterObjectSetReconciler_Reconcile_BuildPhasesErrorClassification(t wantReason string }{ { - name: "invalid progression probe is a validation failure", + name: "invalid progression probe is an invalid specification", setupCOS: func(cos *ocv1.ClusterObjectSet) { cos.Spec.ProgressionProbes = []ocv1.ProgressionProbe{{ Selector: ocv1.ObjectSelector{Type: "Bogus"}, }} }, wantErr: false, - wantReason: ocv1.ClusterObjectSetReasonValidationFailure, + wantReason: ocv1.ClusterObjectSetReasonInvalidSpecification, }, { name: "sibling revision list error is retryable", diff --git a/internal/object-controller/controllers/resolve_ref_test.go b/internal/object-controller/controllers/resolve_ref_test.go index eba5c81a8b..1391d12dfe 100644 --- a/internal/object-controller/controllers/resolve_ref_test.go +++ b/internal/object-controller/controllers/resolve_ref_test.go @@ -219,8 +219,8 @@ func TestResolveObjectRef_KeyNotFound(t *testing.T) { Clock: clocktesting.NewFakeClock(metav1.Now().Time), } - // A missing key in an (immutable) referenced Secret is invalid configuration that will - // not resolve on retry: no error is returned, and Ready reports ValidationFailure. + // A missing key in an (immutable) referenced Secret is a referenced-content problem: + // no error is returned, and Ready reports PreconditionsNotMet (retried until the Secret is fixed). _, err := reconciler.Reconcile(context.Background(), ctrl.Request{ NamespacedName: types.NamespacedName{Name: cos.Name}, }) @@ -231,7 +231,7 @@ func TestResolveObjectRef_KeyNotFound(t *testing.T) { cond := meta.FindStatusCondition(updated.Status.Conditions, ocv1.ClusterObjectSetTypeReady) require.NotNil(t, cond) assert.Equal(t, metav1.ConditionFalse, cond.Status) - assert.Equal(t, ocv1.ClusterObjectSetReasonValidationFailure, cond.Reason) + assert.Equal(t, ocv1.ClusterObjectSetReasonPreconditionsNotMet, cond.Reason) assert.Contains(t, cond.Message, "key") } @@ -269,8 +269,8 @@ func TestResolveObjectRef_InvalidJSON(t *testing.T) { Clock: clocktesting.NewFakeClock(metav1.Now().Time), } - // A malformed referenced manifest is invalid configuration that will not resolve on - // retry: no error is returned, and Ready reports ValidationFailure. + // A malformed referenced manifest is a referenced-content problem: no error is returned, + // and Ready reports PreconditionsNotMet (retried until the Secret content is fixed). _, err := reconciler.Reconcile(context.Background(), ctrl.Request{ NamespacedName: types.NamespacedName{Name: cos.Name}, }) @@ -281,7 +281,7 @@ func TestResolveObjectRef_InvalidJSON(t *testing.T) { cond := meta.FindStatusCondition(updated.Status.Conditions, ocv1.ClusterObjectSetTypeReady) require.NotNil(t, cond) assert.Equal(t, metav1.ConditionFalse, cond.Status) - assert.Equal(t, ocv1.ClusterObjectSetReasonValidationFailure, cond.Reason) + assert.Equal(t, ocv1.ClusterObjectSetReasonPreconditionsNotMet, cond.Reason) assert.Contains(t, cond.Message, "unmarshal") } diff --git a/internal/operator-controller/controllers/common_controller.go b/internal/operator-controller/controllers/common_controller.go index c6cd38c761..a5377cdd2e 100644 --- a/internal/operator-controller/controllers/common_controller.go +++ b/internal/operator-controller/controllers/common_controller.go @@ -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 +// - RetryableError (transient), PreconditionsNotMet (a pre-apply precondition not yet met), or +// InvalidSpecification (an invalid revision spec) on Ready indicates an error // - 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,14 @@ 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. + // 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 || readyCond.Reason == ocv1.ClusterObjectSetReasonValidationFailure) { + readyCond.Reason == ocv1.ClusterObjectSetReasonObjectCollision || + readyCond.Reason == ocv1.ClusterObjectSetReasonPreconditionsNotMet || + readyCond.Reason == ocv1.ClusterObjectSetReasonInvalidSpecification) { return ocv1.ReasonFailed } @@ -267,13 +272,18 @@ func progressingFromReady(ready *metav1.Condition, completed bool) metav1.Condit } cond.Message = ready.Message switch ready.Reason { - case ocv1.ClusterObjectSetReasonValidationFailure: + case ocv1.ClusterObjectSetReasonInvalidSpecification: + // An invalid revision spec needs a spec change to proceed; the revision does not + // retry on its own, so report it as a terminal Progressing=False/Blocked. cond.Status = metav1.ConditionFalse cond.Reason = ocv1.ReasonBlocked case ocv1.ReasonProgressDeadlineExceeded: cond.Status = metav1.ConditionFalse cond.Reason = ocv1.ReasonProgressDeadlineExceeded - case ocv1.ClusterObjectSetReasonRetryableError, ocv1.ClusterObjectSetReasonObjectCollision: + 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. 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..494f72e044 100644 --- a/internal/operator-controller/controllers/common_controller_test.go +++ b/internal/operator-controller/controllers/common_controller_test.go @@ -476,8 +476,13 @@ func TestDetermineFailureReason(t *testing.T) { expected: ocv1.ReasonAbsent, }, { - name: "latest validation failure -> Failed", - rolling: []*RevisionMetadata{{Conditions: availCond(metav1.ConditionFalse, ocv1.ClusterObjectSetReasonValidationFailure)}}, + name: "latest preconditions not met -> Failed", + rolling: []*RevisionMetadata{{Conditions: availCond(metav1.ConditionFalse, ocv1.ClusterObjectSetReasonPreconditionsNotMet)}}, + expected: ocv1.ReasonFailed, + }, + { + name: "latest invalid specification -> Failed", + rolling: []*RevisionMetadata{{Conditions: availCond(metav1.ConditionFalse, ocv1.ClusterObjectSetReasonInvalidSpecification)}}, expected: ocv1.ReasonFailed, }, { @@ -544,11 +549,17 @@ func TestProgressingFromReady(t *testing.T) { expected: metav1.Condition{Type: ocv1.TypeProgressing, Status: metav1.ConditionTrue, Reason: ocv1.ReasonRetrying, Message: "boom"}, }, { - name: "validation failure maps to Blocked", - ready: &metav1.Condition{Type: ocv1.ClusterObjectSetTypeReady, Status: metav1.ConditionFalse, Reason: ocv1.ClusterObjectSetReasonValidationFailure, Message: "manual fix needed"}, + name: "invalid specification maps to Blocked", + ready: &metav1.Condition{Type: ocv1.ClusterObjectSetTypeReady, Status: metav1.ConditionFalse, Reason: ocv1.ClusterObjectSetReasonInvalidSpecification, Message: "manual fix needed"}, completed: false, expected: metav1.Condition{Type: ocv1.TypeProgressing, Status: metav1.ConditionFalse, Reason: ocv1.ReasonBlocked, Message: "manual fix needed"}, }, + { + name: "preconditions not met maps to Retrying", + ready: &metav1.Condition{Type: ocv1.ClusterObjectSetTypeReady, Status: metav1.ConditionFalse, Reason: ocv1.ClusterObjectSetReasonPreconditionsNotMet, Message: "waiting on a precondition"}, + completed: false, + expected: metav1.Condition{Type: ocv1.TypeProgressing, Status: metav1.ConditionTrue, Reason: ocv1.ReasonRetrying, Message: "waiting on a precondition"}, + }, { name: "deadline exceeded maps to ProgressDeadlineExceeded", ready: &metav1.Condition{Type: ocv1.ClusterObjectSetTypeReady, Status: metav1.ConditionFalse, Reason: ocv1.ReasonProgressDeadlineExceeded, Message: "too slow"}, diff --git a/manifests/experimental-e2e.yaml b/manifests/experimental-e2e.yaml index b0b7e99f30..4cb587958a 100644 --- a/manifests/experimental-e2e.yaml +++ b/manifests/experimental-e2e.yaml @@ -1973,7 +1973,8 @@ spec: - 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. - 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 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. diff --git a/manifests/experimental.yaml b/manifests/experimental.yaml index 70f956f538..e9ddd942cd 100644 --- a/manifests/experimental.yaml +++ b/manifests/experimental.yaml @@ -1934,7 +1934,8 @@ spec: - 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. - 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 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. diff --git a/test/e2e/features/revision.feature b/test/e2e/features/revision.feature index 78cd131fb7..74dbb72861 100644 --- a/test/e2e/features/revision.feature +++ b/test/e2e/features/revision.feature @@ -423,7 +423,7 @@ Feature: Install ClusterObjectSet And resource "deployment/test-httpd" is installed And ClusterObjectSet "${COS_NAME}" has observed phase "resources" with a non-empty digest - Scenario: ClusterObjectSet reports ValidationFailure when referenced Secret is mutable + Scenario: ClusterObjectSet reports PreconditionsNotMet when referenced Secret is mutable Given namespace "${TEST_NAMESPACE}" is available And resource is applied """ @@ -465,12 +465,12 @@ Feature: Install ClusterObjectSet key: configmap revision: 1 """ - Then ClusterObjectSet "${COS_NAME}" reports Ready as False with Reason ValidationFailure and Message: + Then ClusterObjectSet "${COS_NAME}" reports Ready as False with Reason PreconditionsNotMet and Message: """ - the following secrets are not immutable (referenced secrets must have immutable set to true): ${TEST_NAMESPACE}/${COS_NAME}-mutable-secret + Referenced Secret(s) must be immutable but are not: ${TEST_NAMESPACE}/${COS_NAME}-mutable-secret. Recreate them with immutable set to true. """ - Scenario: ClusterObjectSet reports ValidationFailure when referenced Secret content changes + Scenario: ClusterObjectSet reports PreconditionsNotMet when referenced Secret content changes Given namespace "${TEST_NAMESPACE}" is available When resource is applied """ @@ -541,7 +541,7 @@ Feature: Install ClusterObjectSet } """ And ClusterObjectSet "${COS_NAME}" reconciliation is triggered - Then ClusterObjectSet "${COS_NAME}" reports Ready as False with Reason ValidationFailure and Message includes: + Then ClusterObjectSet "${COS_NAME}" reports Ready as False with Reason PreconditionsNotMet and Message includes: """ resolved content of 1 phase(s) has changed since first rollout: "resources" """