From 6e74fea063b998ea076061a30dfc82b238ec745d Mon Sep 17 00:00:00 2001 From: "Per G. da Silva" Date: Mon, 12 Oct 2026 16:37:52 +0200 Subject: [PATCH] =?UTF-8?q?=E2=9C=A8=20report=20superseded=20ClusterObject?= =?UTF-8?q?Set=20as=20Ready=3DFalse/Superseded?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When one or more objects have been adopted by a newer sibling revision (machinery.ActionProgressed), report Superseded instead of ObjectsNotReady. boxcutter emits ActionProgressed only for a newer sibling (a foreign owner is a collision), so no revision comparison is needed. A partial hand-off counts, and the progress deadline still takes precedence (ProgressDeadlineExceeded > Superseded > ObjectsNotReady). ClusterExtension Progressing maps Superseded to RollingOut and determineFailureReason treats it as non-error (Absent). 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 | 1 + ...peratorframework.io_clusterobjectsets.yaml | 1 + .../clusterobjectset_controller.go | 46 ++++-- .../clusterobjectset_controller_test.go | 132 ++++++++++++++++++ .../controllers/common_controller_test.go | 11 ++ manifests/experimental-e2e.yaml | 1 + manifests/experimental.yaml | 1 + 9 files changed, 187 insertions(+), 9 deletions(-) diff --git a/api/v1/clusterobjectset_types.go b/api/v1/clusterobjectset_types.go index 4a22ad15a2..5f54a1c02d 100644 --- a/api/v1/clusterobjectset_types.go +++ b/api/v1/clusterobjectset_types.go @@ -32,6 +32,7 @@ const ( ClusterObjectSetReasonArchived = "Archived" ClusterObjectSetReasonArchiving = "Archiving" ClusterObjectSetReasonObjectsNotReady = "ObjectsNotReady" + ClusterObjectSetReasonSuperseded = "Superseded" ClusterObjectSetReasonObjectCollision = "ObjectCollision" ClusterObjectSetReasonRetryableError = "RetryableError" ClusterObjectSetReasonValidationFailure = "ValidationFailure" @@ -501,6 +502,7 @@ type ClusterObjectSetStatus struct { // the first reconciliation has evaluated the revision. // - True with reason AllObjectsReady: the revision has rolled out and all objects are at the desired state. // - False with reason ObjectsNotReady: one or more objects are still being applied or have not yet passed their readiness probes during rollout. + // - False with reason Superseded: one or more objects have been adopted by a newer revision of the object set. // - 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. diff --git a/applyconfigurations/api/v1/clusterobjectsetstatus.go b/applyconfigurations/api/v1/clusterobjectsetstatus.go index 16eb7a7f84..ed8ed67aef 100644 --- a/applyconfigurations/api/v1/clusterobjectsetstatus.go +++ b/applyconfigurations/api/v1/clusterobjectsetstatus.go @@ -41,6 +41,7 @@ type ClusterObjectSetStatusApplyConfiguration struct { // the first reconciliation has evaluated the revision. // - True with reason AllObjectsReady: the revision has rolled out and all objects are at the desired state. // - False with reason ObjectsNotReady: one or more objects are still being applied or have not yet passed their readiness probes during rollout. + // - False with reason Superseded: one or more objects have been adopted by a newer revision of the object set. // - 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. diff --git a/docs/draft/concepts/clusterobjectsets.md b/docs/draft/concepts/clusterobjectsets.md index 8c48744509..ae380a6ac6 100644 --- a/docs/draft/concepts/clusterobjectsets.md +++ b/docs/draft/concepts/clusterobjectsets.md @@ -218,6 +218,7 @@ Indicates whether all objects in the revision have been successfully rolled out | --- | --- | --- | | True | `AllObjectsReady` | All objects are at the desired state | | 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 | | Unknown | `RetryableError` | Error prevented probe observation | 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 31c3a9ec7f..9fb96efbb3 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 @@ -560,6 +560,7 @@ spec: the first reconciliation has evaluated the revision. - True with reason AllObjectsReady: the revision has rolled out and all objects are at the desired state. - False with reason ObjectsNotReady: one or more objects are still being applied or have not yet passed their readiness probes during rollout. + - False with reason Superseded: one or more objects have been adopted by a newer revision of the object set. - 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. diff --git a/internal/object-controller/controllers/clusterobjectset_controller.go b/internal/object-controller/controllers/clusterobjectset_controller.go index 4949f8d715..f4f2880ddb 100644 --- a/internal/object-controller/controllers/clusterobjectset_controller.go +++ b/internal/object-controller/controllers/clusterobjectset_controller.go @@ -48,10 +48,11 @@ const ( clusterObjectSetTeardownFinalizer = "olm.operatorframework.io/teardown" // validationRetryInterval is how often the reconciler re-checks a revision that is - // Ready=False/ValidationFailure due to referenced-content problems (preflight validation, - // a mutable referenced Secret, a changed digest, or an invalid referenced manifest). - // Referenced Secrets are not watched and the phase/ref spec is immutable, so a periodic - // retry lets the revision recover once the Secret is replaced, without a controller restart. + // Ready=False/ValidationFailure. 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 + // replaced or the cluster state catches up) without a controller restart. validationRetryInterval = 10 * time.Second ) @@ -230,8 +231,9 @@ func (c *ClusterObjectSetReconciler) reconcile(ctx context.Context, cos *ocv1.Cl return ctrl.Result{}, fmt.Errorf("revision reconcile: %v", err) } - // Retry failing preflight checks with a flat 10s retry. - // TODO: report status, backoff? + // Report revision-level preflight validation failures and re-check on a flat timer: + // the dry-run can be rejected for reasons that clear without a spec change (e.g. RBAC + // 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)) @@ -294,8 +296,15 @@ func (c *ClusterObjectSetReconciler) reconcile(ctx context.Context, cos *ocv1.Cl } } + // A revision whose objects have all been adopted by a newer sibling revision can + // report IsComplete() (progressed objects count as complete), but it is superseded, + // not successfully rolled out: skip the completion branch so it does not archive + // previous revisions, report AllObjectsReady, or record a first completion + // (completedAt) that would identify it as the installed revision. + superseded := hasSupersededObject(rres) + //nolint:nestif - if rres.IsComplete() { + if rres.IsComplete() && !superseded { // Archive previous revisions previous, err := c.listPreviousRevisions(ctx, cos) if err != nil { @@ -357,9 +366,13 @@ func (c *ClusterObjectSetReconciler) reconcile(ctx context.Context, cos *ocv1.Cl if len(notReadyMsgs) > 0 { l.V(1).Info("objects not yet ready", "count", len(notReadyMsgs), "details", notReadyMsgs) } - if isDeadlineExceeded { + // Precedence: ProgressDeadlineExceeded > Superseded > ObjectsNotReady. + switch { + case isDeadlineExceeded: setReadyWithDeadline(cos, metav1.ConditionFalse, ocv1.ClusterObjectSetReasonObjectsNotReady, genericNotReadyMsg, true) - } else { + case superseded: + markAsNotReady(cos, ocv1.ClusterObjectSetReasonSuperseded, "Object(s) have been adopted by a newer revision of the object set.") + default: notReadyMsg := genericNotReadyMsg if len(notReadyMsgs) > 0 { notReadyMsg = fmt.Sprintf("%d object(s) are not yet ready.", len(notReadyMsgs)) @@ -780,6 +793,21 @@ func markAsNotReady(cos *ocv1.ClusterObjectSet, reason, message string) bool { }) } +// hasSupersededObject reports whether any object in the revision has been adopted +// by a newer sibling revision (machinery.ActionProgressed). boxcutter only emits +// ActionProgressed for a newer sibling (a foreign owner is a collision), so no +// revision comparison is needed here. +func hasSupersededObject(rres machinery.RevisionResult) bool { + for _, pres := range rres.GetPhases() { + for _, ores := range pres.GetObjects() { + if ores.Action() == machinery.ActionProgressed { + return true + } + } + } + return false +} + func markAsArchiving(cos *ocv1.ClusterObjectSet) { const msg = "The object set is being archived; removing owned objects." markAsNotReady(cos, ocv1.ClusterObjectSetReasonArchiving, msg) diff --git a/internal/object-controller/controllers/clusterobjectset_controller_test.go b/internal/object-controller/controllers/clusterobjectset_controller_test.go index d5165bdfee..34c532ce2d 100644 --- a/internal/object-controller/controllers/clusterobjectset_controller_test.go +++ b/internal/object-controller/controllers/clusterobjectset_controller_test.go @@ -213,6 +213,97 @@ func Test_ClusterObjectSetReconciler_Reconcile_RevisionReconciliation(t *testing require.Equal(t, ocv1.ClusterObjectSetReasonObjectsNotReady, cond.Reason) }, }, + { + // A fully handed-off revision can report rres.IsComplete()==true because + // progressed objects count as complete (boxcutter). It must still report + // Superseded, not AllObjectsReady, and must NOT stamp completedAt — otherwise + // the superseded revision could be mis-identified as the installed one. + name: "complete revision with a progressed object reports Superseded, not AllObjectsReady", + reconcilingRevisionName: clusterObjectSetName, + revisionResult: newMockRevisionResult(mockCtrl, revisionResultConfig{ + isComplete: true, + phases: []machinery.PhaseResult{ + newMockPhaseResult(mockCtrl, phaseResultConfig{ + name: "somephase", + isComplete: true, + objects: []machinery.ObjectResult{ + newMockObjectResult(mockCtrl, objectResultConfig{ + action: machinery.ActionProgressed, + complete: true, + object: func() client.Object { + obj := &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: "cm", Namespace: "ns"}} + obj.SetGroupVersionKind(corev1.SchemeGroupVersion.WithKind("ConfigMap")) + return obj + }(), + }), + }, + }), + }, + }), + existingObjs: func() []client.Object { + ext := newTestClusterExtension() + return []client.Object{ext, newTestClusterObjectSet(t, clusterObjectSetName, ext, testScheme)} + }, + validate: func(t *testing.T, c client.Client) { + rev := &ocv1.ClusterObjectSet{} + require.NoError(t, c.Get(t.Context(), client.ObjectKey{Name: clusterObjectSetName}, rev)) + cond := meta.FindStatusCondition(rev.Status.Conditions, ocv1.ClusterObjectSetTypeReady) + require.NotNil(t, cond) + require.Equal(t, metav1.ConditionFalse, cond.Status) + require.Equal(t, ocv1.ClusterObjectSetReasonSuperseded, cond.Reason) + require.True(t, rev.Status.CompletedAt.IsZero(), "a superseded revision must not stamp completedAt") + }, + }, + { + // When one or more objects have been adopted by a newer sibling revision + // (machinery.ActionProgressed), the revision reports Superseded, which takes + // precedence over ObjectsNotReady even when other objects are still not ready. + name: "set Ready:False:Superseded when an object has progressed to a newer revision", + reconcilingRevisionName: clusterObjectSetName, + revisionResult: newMockRevisionResult(mockCtrl, revisionResultConfig{ + inTransition: true, + isComplete: false, + phases: []machinery.PhaseResult{ + newMockPhaseResult(mockCtrl, phaseResultConfig{ + name: "somephase", + isComplete: false, + objects: []machinery.ObjectResult{ + newMockObjectResult(mockCtrl, objectResultConfig{ + action: machinery.ActionProgressed, + object: func() client.Object { + obj := &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: "cm", Namespace: "ns"}} + obj.SetGroupVersionKind(corev1.SchemeGroupVersion.WithKind("ConfigMap")) + return obj + }(), + }), + newMockObjectResult(mockCtrl, objectResultConfig{ + object: func() client.Object { + obj := &corev1.Service{ObjectMeta: metav1.ObjectMeta{Name: "svc", Namespace: "ns"}} + obj.SetGroupVersionKind(corev1.SchemeGroupVersion.WithKind("Service")) + return obj + }(), + probes: machinerytypes.ProbeResultContainer{ + boxcutter.ProgressProbeType: {Status: machinerytypes.ProbeStatusFalse, Messages: []string{"not ready"}}, + }, + }), + }, + }), + }, + }), + existingObjs: func() []client.Object { + ext := newTestClusterExtension() + return []client.Object{ext, newTestClusterObjectSet(t, clusterObjectSetName, ext, testScheme)} + }, + validate: func(t *testing.T, c client.Client) { + rev := &ocv1.ClusterObjectSet{} + require.NoError(t, c.Get(t.Context(), client.ObjectKey{Name: clusterObjectSetName}, rev)) + cond := meta.FindStatusCondition(rev.Status.Conditions, ocv1.ClusterObjectSetTypeReady) + require.NotNil(t, cond) + require.Equal(t, metav1.ConditionFalse, cond.Status) + require.Equal(t, ocv1.ClusterObjectSetReasonSuperseded, cond.Reason) + require.Equal(t, "Object(s) have been adopted by a newer revision of the object set.", cond.Message) + }, + }, { name: "set Ready:False:ObjectsNotReady condition when probe failures are detected and revision is in transition", reconcilingRevisionName: clusterObjectSetName, @@ -1438,6 +1529,47 @@ func Test_ClusterObjectSetReconciler_Reconcile_ProgressDeadline(t *testing.T) { require.Equal(t, "Revision has not rolled out for 1 minute(s). Last status: Objects are not yet ready.", cnd.Message) }, }, + { + // A progressed (superseded) object AND an exceeded progress deadline must + // set Ready=False/ProgressDeadlineExceeded (deadline wins over Superseded). + name: "Ready set to false/ProgressDeadlineExceeded when deadline is exceeded with a superseded object (deadline wins over Superseded)", + existingObjs: func() []client.Object { + ext := newTestClusterExtension() + rev1 := newTestClusterObjectSet(t, clusterObjectSetName, ext, testScheme) + rev1.Spec.ProgressDeadlineMinutes = 1 + rev1.CreationTimestamp = metav1.NewTime(time.Date(2022, 1, 1, 0, 0, 0, 0, time.UTC)) + return []client.Object{rev1, ext} + }, + clock: clocktesting.NewFakeClock(time.Date(2022, 1, 1, 0, 1, 1, 0, time.UTC)), + revisionResult: newMockRevisionResult(mockCtrl, revisionResultConfig{ + inTransition: true, + isComplete: false, + phases: []machinery.PhaseResult{ + newMockPhaseResult(mockCtrl, phaseResultConfig{ + name: "somephase", + isComplete: false, + objects: []machinery.ObjectResult{ + newMockObjectResult(mockCtrl, objectResultConfig{ + action: machinery.ActionProgressed, + object: func() client.Object { + obj := &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: "cm", Namespace: "ns"}} + obj.SetGroupVersionKind(corev1.SchemeGroupVersion.WithKind("ConfigMap")) + return obj + }(), + }), + }, + }), + }, + }), + validate: func(t *testing.T, c client.Client) { + rev := &ocv1.ClusterObjectSet{} + require.NoError(t, c.Get(t.Context(), client.ObjectKey{Name: clusterObjectSetName}, rev)) + cnd := meta.FindStatusCondition(rev.Status.Conditions, ocv1.ClusterObjectSetTypeReady) + require.NotNil(t, cnd) + require.Equal(t, metav1.ConditionFalse, cnd.Status) + require.Equal(t, ocv1.ReasonProgressDeadlineExceeded, cnd.Reason) + }, + }, { name: "requeue after progressDeadline time for final progression deadline check", existingObjs: func() []client.Object { diff --git a/internal/operator-controller/controllers/common_controller_test.go b/internal/operator-controller/controllers/common_controller_test.go index a98e5f3d51..0263975e7f 100644 --- a/internal/operator-controller/controllers/common_controller_test.go +++ b/internal/operator-controller/controllers/common_controller_test.go @@ -480,6 +480,11 @@ func TestDetermineFailureReason(t *testing.T) { rolling: []*RevisionMetadata{{Conditions: availCond(metav1.ConditionFalse, ocv1.ClusterObjectSetReasonValidationFailure)}}, expected: ocv1.ReasonFailed, }, + { + name: "latest superseded -> Absent", + rolling: []*RevisionMetadata{{Conditions: availCond(metav1.ConditionFalse, ocv1.ClusterObjectSetReasonSuperseded)}}, + expected: ocv1.ReasonAbsent, + }, { name: "only the latest revision matters", rolling: []*RevisionMetadata{ @@ -520,6 +525,12 @@ func TestProgressingFromReady(t *testing.T) { completed: false, expected: metav1.Condition{Type: ocv1.TypeProgressing, Status: metav1.ConditionTrue, Reason: ocv1.ReasonRollingOut, Message: "Objects are not yet ready."}, }, + { + name: "superseded maps to RollingOut", + ready: &metav1.Condition{Type: ocv1.ClusterObjectSetTypeReady, Status: metav1.ConditionFalse, Reason: ocv1.ClusterObjectSetReasonSuperseded, Message: "Object(s) have been adopted by a newer revision of the object set."}, + completed: false, + expected: metav1.Condition{Type: ocv1.TypeProgressing, Status: metav1.ConditionTrue, Reason: ocv1.ReasonRollingOut, Message: "Object(s) have been adopted by a newer revision of the object set."}, + }, { name: "archiving maps to RollingOut", ready: &metav1.Condition{Type: ocv1.ClusterObjectSetTypeReady, Status: metav1.ConditionFalse, Reason: ocv1.ClusterObjectSetReasonArchiving, Message: "The object set is being archived; removing owned objects."}, diff --git a/manifests/experimental-e2e.yaml b/manifests/experimental-e2e.yaml index 3e876e7182..1ab1aa4887 100644 --- a/manifests/experimental-e2e.yaml +++ b/manifests/experimental-e2e.yaml @@ -1971,6 +1971,7 @@ spec: the first reconciliation has evaluated the revision. - True with reason AllObjectsReady: the revision has rolled out and all objects are at the desired state. - False with reason ObjectsNotReady: one or more objects are still being applied or have not yet passed their readiness probes during rollout. + - False with reason Superseded: one or more objects have been adopted by a newer revision of the object set. - 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. diff --git a/manifests/experimental.yaml b/manifests/experimental.yaml index 8891ae4962..ffa7dab444 100644 --- a/manifests/experimental.yaml +++ b/manifests/experimental.yaml @@ -1932,6 +1932,7 @@ spec: the first reconciliation has evaluated the revision. - True with reason AllObjectsReady: the revision has rolled out and all objects are at the desired state. - False with reason ObjectsNotReady: one or more objects are still being applied or have not yet passed their readiness probes during rollout. + - False with reason Superseded: one or more objects have been adopted by a newer revision of the object set. - 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.