From 9193fc55e26ae813b2a29dbdb33d7338383a5947 Mon Sep 17 00:00:00 2001 From: Fabricio Aguiar Date: Wed, 21 Oct 2026 16:40:39 +0100 Subject: [PATCH 1/2] feat(object-controller): persist phase completion milestones Record immutable first-completion timestamps for ClusterObjectSet phases and retain them across retries and health regressions. Exclude downstream phases that are still paused and objects handed to a newer revision. These durable milestones allow OPRUN-4791 to measure rollout inactivity. Cover ordered completion and timestamp immutability with controller and API admission tests. Signed-off-by: Fabricio Aguiar rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED --- api/v1/clusterobjectset_types.go | 14 +++- api/v1/validation_test.go | 67 +++++++++++++++++ .../clusterobjectset_controller.go | 22 ++++++ .../clusterobjectset_controller_test.go | 74 +++++++++++++++++++ 4 files changed, 176 insertions(+), 1 deletion(-) diff --git a/api/v1/clusterobjectset_types.go b/api/v1/clusterobjectset_types.go index 377aa81633..364fa3258b 100644 --- a/api/v1/clusterobjectset_types.go +++ b/api/v1/clusterobjectset_types.go @@ -506,6 +506,7 @@ const ( // update, which would otherwise allow the timestamp to be cleared and re-set. // // +kubebuilder:validation:XValidation:rule="!has(oldSelf.completedAt) || has(self.completedAt)",message="completedAt cannot be removed once set" +// +kubebuilder:validation:XValidation:rule="!has(oldSelf.observedPhases) || oldSelf.observedPhases.all(p, !has(p.completedAt) || (has(self.observedPhases) && self.observedPhases.exists(q, q.name == p.name)))",message="completed phases cannot be removed from observedPhases" type ClusterObjectSetStatus struct { // conditions is an optional list of status conditions describing the state of the // ClusterObjectSet. @@ -560,7 +561,9 @@ type ClusterObjectSetStatus struct { CompletedAt metav1.Time `json:"completedAt,omitempty,omitzero"` } -// ObservedPhase records the observed content digest of a resolved phase. +// ObservedPhase records the content and rollout progress of a resolved phase. +// +// +kubebuilder:validation:XValidation:rule="!has(oldSelf.completedAt) || has(self.completedAt)",message="completedAt cannot be removed once set" type ObservedPhase struct { // name is the phase name matching a phase in spec.phases. // @@ -585,6 +588,15 @@ type ObservedPhase struct { // fields are reported (including zeros). // +optional ObjectCounts ObjectCounts `json:"objectCounts,omitzero"` + + // completedAt is the timestamp at which this phase was first observed to have + // successfully reconciled and passed its probes after all preceding phases + // completed. It is set once and is immutable thereafter, even if the phase + // later becomes unhealthy. ClusterObjectDeployment uses it as a progress + // milestone for its inactivity deadline. + // +optional + // +kubebuilder:validation:XValidation:rule="self == oldSelf || oldSelf == null",message="completedAt is immutable once set" + CompletedAt metav1.Time `json:"completedAt,omitempty,omitzero"` } // ObjectCounts reports the number of objects in a ClusterObjectSet or phase by state. diff --git a/api/v1/validation_test.go b/api/v1/validation_test.go index 1e7ce6e034..cb5fbfaf47 100644 --- a/api/v1/validation_test.go +++ b/api/v1/validation_test.go @@ -6,6 +6,7 @@ import ( "time" "github.com/stretchr/testify/require" + apiequality "k8s.io/apimachinery/pkg/api/equality" "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "sigs.k8s.io/controller-runtime/pkg/client" @@ -240,6 +241,72 @@ func TestClusterObjectSetCompletedAtCannotBeRemoved(t *testing.T) { require.True(t, errors.IsInvalid(err), "expected removal to fail as invalid, but got: %v", err) } +func TestClusterObjectSetPhaseCompletedAt(t *testing.T) { + c := newClient(t) + firstReady := metav1.NewTime(time.Date(2026, 10, 21, 12, 0, 0, 0, time.UTC)) + + for i, tc := range []struct { + name string + oldObservedPhases []ObservedPhase + newObservedPhases []ObservedPhase + wantErr bool + }{ + { + name: "set the first completion timestamp", + oldObservedPhases: []ObservedPhase{{Name: "deploy", Digest: "sha256:abc123"}}, + newObservedPhases: []ObservedPhase{{Name: "deploy", Digest: "sha256:abc123", CompletedAt: firstReady}}, + }, + { + name: "reapply the same completion timestamp", + oldObservedPhases: []ObservedPhase{{Name: "deploy", Digest: "sha256:abc123", CompletedAt: firstReady}}, + newObservedPhases: []ObservedPhase{{Name: "deploy", Digest: "sha256:abc123", CompletedAt: firstReady}}, + }, + { + name: "reject changing the completion timestamp", + oldObservedPhases: []ObservedPhase{{Name: "deploy", Digest: "sha256:abc123", CompletedAt: firstReady}}, + newObservedPhases: []ObservedPhase{{Name: "deploy", Digest: "sha256:abc123", CompletedAt: metav1.NewTime(firstReady.Add(time.Minute))}}, + wantErr: true, + }, + { + name: "reject removing the completion timestamp", + oldObservedPhases: []ObservedPhase{{Name: "deploy", Digest: "sha256:abc123", CompletedAt: firstReady}}, + newObservedPhases: []ObservedPhase{{Name: "deploy", Digest: "sha256:abc123"}}, + wantErr: true, + }, + { + name: "reject removing a completed phase", + oldObservedPhases: []ObservedPhase{{Name: "deploy", Digest: "sha256:abc123", CompletedAt: firstReady}}, + newObservedPhases: nil, + wantErr: true, + }, + } { + t.Run(tc.name, func(t *testing.T) { + cos := &ClusterObjectSet{ + ObjectMeta: metav1.ObjectMeta{Name: fmt.Sprintf("cos-phase-completedat-%d", i)}, + Spec: ClusterObjectSetSpec{ + Group: "test-group", + Revision: 1, + CollisionProtection: CollisionProtectionPrevent, + LifecycleState: ClusterObjectSetLifecycleStateActive, + }, + } + require.NoError(t, c.Create(t.Context(), cos)) + cos.Status.ObservedPhases = tc.oldObservedPhases + require.NoError(t, c.Status().Update(t.Context(), cos)) + + cos.Status.ObservedPhases = tc.newObservedPhases + err := c.Status().Update(t.Context(), cos) + if tc.wantErr { + require.True(t, errors.IsInvalid(err), "expected update to fail as invalid, but got: %v", err) + return + } + require.NoError(t, err) + require.NoError(t, c.Get(t.Context(), client.ObjectKeyFromObject(cos), cos)) + require.True(t, apiequality.Semantic.DeepEqual(tc.newObservedPhases, cos.Status.ObservedPhases), "phase status must survive storage") + }) + } +} + func TestClusterObjectSetDigestImmutable(t *testing.T) { c := newClient(t) diff --git a/internal/object-controller/controllers/clusterobjectset_controller.go b/internal/object-controller/controllers/clusterobjectset_controller.go index 6354f35abe..12bfaffd28 100644 --- a/internal/object-controller/controllers/clusterobjectset_controller.go +++ b/internal/object-controller/controllers/clusterobjectset_controller.go @@ -11,6 +11,7 @@ import ( "errors" "fmt" "io" + "slices" "strings" "time" @@ -245,6 +246,7 @@ func (c *ClusterObjectSetReconciler) reconcile(ctx context.Context, cos *ocv1.Cl // Set object counts status.FromReconcile(cos, rres) + c.recordPhaseCompletions(cos, rres) // 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 @@ -795,6 +797,26 @@ func markAsNotReady(cos *ocv1.ClusterObjectSet, reason, message string) bool { }) } +// recordPhaseCompletions records each phase's first successful rollout. Read-only +// downstream phases and phases superseded by a newer revision are not milestones. +// Existing timestamps survive retries, drift, and controller restarts. +func (c *ClusterObjectSetReconciler) recordPhaseCompletions(cos *ocv1.ClusterObjectSet, result machinery.RevisionResult) { + now := metav1.NewTime(c.Clock.Now()) + for _, phase := range result.GetPhases() { + if !phase.IsComplete() || slices.ContainsFunc(phase.GetObjects(), func(obj machinery.ObjectResult) bool { + return obj.Action() == machinery.ActionProgressed + }) { + break + } + for i := range cos.Status.ObservedPhases { + observed := &cos.Status.ObservedPhases[i] + if observed.Name == phase.GetName() && observed.CompletedAt.IsZero() { + observed.CompletedAt = now + } + } + } +} + // 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 diff --git a/internal/object-controller/controllers/clusterobjectset_controller_test.go b/internal/object-controller/controllers/clusterobjectset_controller_test.go index f5e830526a..4eb1ef44a4 100644 --- a/internal/object-controller/controllers/clusterobjectset_controller_test.go +++ b/internal/object-controller/controllers/clusterobjectset_controller_test.go @@ -763,6 +763,80 @@ func Test_ClusterObjectSetReconciler_Reconcile_CompletedAt(t *testing.T) { } } +func Test_ClusterObjectSetReconciler_Reconcile_PhaseCompletedAt(t *testing.T) { + firstReady := metav1.NewTime(time.Date(2026, 10, 21, 12, 0, 0, 0, time.UTC)) + now := metav1.NewTime(firstReady.Add(time.Minute)) + for _, tc := range []struct { + name string + firstComplete bool + secondComplete bool + superseded bool + existing metav1.Time + wantFirst metav1.Time + wantSecond metav1.Time + }{ + {name: "records progress before the revision completes", firstComplete: true, wantFirst: now}, + {name: "does not count read-only downstream phases", secondComplete: true}, + {name: "preserves an earlier milestone when the next phase completes", firstComplete: true, secondComplete: true, existing: firstReady, wantFirst: firstReady, wantSecond: now}, + {name: "preserves completion after drift", existing: firstReady, wantFirst: firstReady}, + {name: "a partially superseded phase is not progress", firstComplete: true, secondComplete: true, superseded: true}, + } { + t.Run(tc.name, func(t *testing.T) { + mockCtrl := gomock.NewController(t) + testScheme := newScheme(t) + ext := newTestClusterExtension() + rev := newTestClusterObjectSet(t, clusterObjectSetName, ext, testScheme) + rev.Finalizers = []string{"olm.operatorframework.io/teardown"} + second := rev.Spec.Phases[0] + second.Name = "later" + rev.Spec.Phases = append(rev.Spec.Phases, second) + c := testutil.WithIndexes(t, fake.NewClientBuilder().WithScheme(testScheme), controllers.SetupIndexes). + WithStatusSubresource(&ocv1.ClusterObjectSet{}). + WithObjects(ext, rev). + Build() + var objects []machinery.ObjectResult + if tc.superseded { + objects = []machinery.ObjectResult{ + newMockObjectResult(mockCtrl, objectResultConfig{action: machinery.ActionProgressed, complete: true}), + newMockObjectResult(mockCtrl, objectResultConfig{action: machinery.ActionIdle, complete: true}), + } + } + phases := []machinery.PhaseResult{ + newMockPhaseResult(mockCtrl, phaseResultConfig{name: "everything", isComplete: tc.firstComplete, objects: objects}), + newMockPhaseResult(mockCtrl, phaseResultConfig{name: "later", isComplete: tc.secondComplete}), + } + result := newMockRevisionResult(mockCtrl, revisionResultConfig{phases: phases}) + nextResult := newMockRevisionResult(mockCtrl, revisionResultConfig{phases: []machinery.PhaseResult{ + newMockPhaseResult(mockCtrl, phaseResultConfig{name: "everything", isComplete: !tc.existing.IsZero()}), + newMockPhaseResult(mockCtrl, phaseResultConfig{name: "later"}), + }}) + engine := newMockRevisionEngineWithReconcile(mockCtrl, + func(context.Context, machinerytypes.Revision, ...machinerytypes.RevisionReconcileOption) (machinery.RevisionResult, error) { + current := nextResult + nextResult = result + return current, nil + }, nil) + clk := clocktesting.NewFakeClock(firstReady.Time) + reconciler := &controllers.ClusterObjectSetReconciler{ + Client: c, Clock: clk, + RevisionEngineFactory: newMockRevisionEngineFactoryWithEngine(mockCtrl, engine, nil), + TrackingCache: newMockTrackingCache(mockCtrl, c, nil), + } + req := ctrl.Request{NamespacedName: client.ObjectKeyFromObject(rev)} + _, err := reconciler.Reconcile(t.Context(), req) + require.NoError(t, err) + require.NoError(t, c.Get(t.Context(), req.NamespacedName, rev)) + clk.SetTime(now.Time) + _, err = reconciler.Reconcile(t.Context(), req) + require.NoError(t, err) + require.NoError(t, c.Get(t.Context(), req.NamespacedName, rev)) + require.Len(t, rev.Status.ObservedPhases, 2) + require.True(t, tc.wantFirst.Equal(&rev.Status.ObservedPhases[0].CompletedAt), "first phase: %v", rev.Status.ObservedPhases[0].CompletedAt) + require.True(t, tc.wantSecond.Equal(&rev.Status.ObservedPhases[1].CompletedAt), "second phase: %v", rev.Status.ObservedPhases[1].CompletedAt) + }) + } +} + func Test_ClusterObjectSetReconciler_Reconcile_ValidationError_Retries(t *testing.T) { const ( clusterExtensionName = "test-ext" From 431be2828e72cdea3a2b924f5f386ccee43fc159 Mon Sep 17 00:00:00 2001 From: Fabricio Aguiar Date: Wed, 21 Oct 2026 16:40:40 +0100 Subject: [PATCH 2/2] chore(api): regenerate phase completion artifacts Regenerate apply configurations, DeepCopy methods, CRDs, and experimental manifests for the immutable phase completion milestones. Signed-off-by: Fabricio Aguiar rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED --- api/v1/zz_generated.deepcopy.go | 5 +++- applyconfigurations/api/v1/observedphase.go | 20 +++++++++++++++- applyconfigurations/internal/internal.go | 3 +++ ...peratorframework.io_clusterobjectsets.yaml | 23 +++++++++++++++++-- manifests/experimental-e2e.yaml | 23 +++++++++++++++++-- manifests/experimental.yaml | 23 +++++++++++++++++-- 6 files changed, 89 insertions(+), 8 deletions(-) diff --git a/api/v1/zz_generated.deepcopy.go b/api/v1/zz_generated.deepcopy.go index 0f679a1384..0bcbe66cfe 100644 --- a/api/v1/zz_generated.deepcopy.go +++ b/api/v1/zz_generated.deepcopy.go @@ -706,7 +706,9 @@ func (in *ClusterObjectSetStatus) DeepCopyInto(out *ClusterObjectSetStatus) { if in.ObservedPhases != nil { in, out := &in.ObservedPhases, &out.ObservedPhases *out = make([]ObservedPhase, len(*in)) - copy(*out, *in) + for i := range *in { + (*in)[i].DeepCopyInto(&(*out)[i]) + } } in.CompletedAt.DeepCopyInto(&out.CompletedAt) } @@ -837,6 +839,7 @@ func (in *ObjectSourceRef) DeepCopy() *ObjectSourceRef { func (in *ObservedPhase) DeepCopyInto(out *ObservedPhase) { *out = *in out.ObjectCounts = in.ObjectCounts + in.CompletedAt.DeepCopyInto(&out.CompletedAt) } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ObservedPhase. diff --git a/applyconfigurations/api/v1/observedphase.go b/applyconfigurations/api/v1/observedphase.go index 2e97961e79..2c4b46ef7f 100644 --- a/applyconfigurations/api/v1/observedphase.go +++ b/applyconfigurations/api/v1/observedphase.go @@ -18,10 +18,14 @@ limitations under the License. package v1 +import ( + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + // ObservedPhaseApplyConfiguration represents a declarative configuration of the ObservedPhase type for use // with apply. // -// ObservedPhase records the observed content digest of a resolved phase. +// ObservedPhase records the content and rollout progress of a resolved phase. type ObservedPhaseApplyConfiguration struct { // name is the phase name matching a phase in spec.phases. Name *string `json:"name,omitempty"` @@ -32,6 +36,12 @@ type ObservedPhaseApplyConfiguration struct { // It is omitted until counts have been computed; when present, all count // fields are reported (including zeros). ObjectCounts *ObjectCountsApplyConfiguration `json:"objectCounts,omitempty"` + // completedAt is the timestamp at which this phase was first observed to have + // successfully reconciled and passed its probes after all preceding phases + // completed. It is set once and is immutable thereafter, even if the phase + // later becomes unhealthy. ClusterObjectDeployment uses it as a progress + // milestone for its inactivity deadline. + CompletedAt *metav1.Time `json:"completedAt,omitempty"` } // ObservedPhaseApplyConfiguration constructs a declarative configuration of the ObservedPhase type for use with @@ -63,3 +73,11 @@ func (b *ObservedPhaseApplyConfiguration) WithObjectCounts(value *ObjectCountsAp b.ObjectCounts = value return b } + +// WithCompletedAt sets the CompletedAt field in the declarative configuration to the given value +// and returns the receiver, so that objects can be built by chaining "With" function invocations. +// If called multiple times, the CompletedAt field is set to the value of the last call. +func (b *ObservedPhaseApplyConfiguration) WithCompletedAt(value metav1.Time) *ObservedPhaseApplyConfiguration { + b.CompletedAt = &value + return b +} diff --git a/applyconfigurations/internal/internal.go b/applyconfigurations/internal/internal.go index 0c03ea2e92..39ca00fdd0 100644 --- a/applyconfigurations/internal/internal.go +++ b/applyconfigurations/internal/internal.go @@ -494,6 +494,9 @@ var schemaYAML = typed.YAMLObject(`types: - name: com.github.operator-framework.operator-controller.api.v1.ObservedPhase map: fields: + - name: completedAt + type: + namedType: io.k8s.apimachinery.pkg.apis.meta.v1.Time - name: digest type: scalar: string 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 2c8bd4ab83..d4836ec5d7 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 @@ -722,9 +722,21 @@ spec: different content. Each entry covers all fully-resolved object manifests within a phase, making it source-agnostic. items: - description: ObservedPhase records the observed content digest of - a resolved phase. + description: ObservedPhase records the content and rollout progress + of a resolved phase. properties: + completedAt: + description: |- + completedAt is the timestamp at which this phase was first observed to have + successfully reconciled and passed its probes after all preceding phases + completed. It is set once and is immutable thereafter, even if the phase + later becomes unhealthy. ClusterObjectDeployment uses it as a progress + milestone for its inactivity deadline. + format: date-time + type: string + x-kubernetes-validations: + - message: completedAt is immutable once set + rule: self == oldSelf || oldSelf == null digest: description: |- digest is the digest of the phase's resolved object content @@ -795,6 +807,9 @@ spec: - digest - name type: object + x-kubernetes-validations: + - message: completedAt cannot be removed once set + rule: '!has(oldSelf.completedAt) || has(self.completedAt)' maxItems: 20 type: array x-kubernetes-list-map-keys: @@ -804,6 +819,10 @@ spec: x-kubernetes-validations: - message: completedAt cannot be removed once set rule: '!has(oldSelf.completedAt) || has(self.completedAt)' + - message: completed phases cannot be removed from observedPhases + rule: '!has(oldSelf.observedPhases) || oldSelf.observedPhases.all(p, + !has(p.completedAt) || (has(self.observedPhases) && self.observedPhases.exists(q, + q.name == p.name)))' required: - spec type: object diff --git a/manifests/experimental-e2e.yaml b/manifests/experimental-e2e.yaml index d79111109d..c2297c20af 100644 --- a/manifests/experimental-e2e.yaml +++ b/manifests/experimental-e2e.yaml @@ -2693,9 +2693,21 @@ spec: different content. Each entry covers all fully-resolved object manifests within a phase, making it source-agnostic. items: - description: ObservedPhase records the observed content digest of - a resolved phase. + description: ObservedPhase records the content and rollout progress + of a resolved phase. properties: + completedAt: + description: |- + completedAt is the timestamp at which this phase was first observed to have + successfully reconciled and passed its probes after all preceding phases + completed. It is set once and is immutable thereafter, even if the phase + later becomes unhealthy. ClusterObjectDeployment uses it as a progress + milestone for its inactivity deadline. + format: date-time + type: string + x-kubernetes-validations: + - message: completedAt is immutable once set + rule: self == oldSelf || oldSelf == null digest: description: |- digest is the digest of the phase's resolved object content @@ -2766,6 +2778,9 @@ spec: - digest - name type: object + x-kubernetes-validations: + - message: completedAt cannot be removed once set + rule: '!has(oldSelf.completedAt) || has(self.completedAt)' maxItems: 20 type: array x-kubernetes-list-map-keys: @@ -2775,6 +2790,10 @@ spec: x-kubernetes-validations: - message: completedAt cannot be removed once set rule: '!has(oldSelf.completedAt) || has(self.completedAt)' + - message: completed phases cannot be removed from observedPhases + rule: '!has(oldSelf.observedPhases) || oldSelf.observedPhases.all(p, + !has(p.completedAt) || (has(self.observedPhases) && self.observedPhases.exists(q, + q.name == p.name)))' required: - spec type: object diff --git a/manifests/experimental.yaml b/manifests/experimental.yaml index 31f9c4232e..cdd36f147c 100644 --- a/manifests/experimental.yaml +++ b/manifests/experimental.yaml @@ -2654,9 +2654,21 @@ spec: different content. Each entry covers all fully-resolved object manifests within a phase, making it source-agnostic. items: - description: ObservedPhase records the observed content digest of - a resolved phase. + description: ObservedPhase records the content and rollout progress + of a resolved phase. properties: + completedAt: + description: |- + completedAt is the timestamp at which this phase was first observed to have + successfully reconciled and passed its probes after all preceding phases + completed. It is set once and is immutable thereafter, even if the phase + later becomes unhealthy. ClusterObjectDeployment uses it as a progress + milestone for its inactivity deadline. + format: date-time + type: string + x-kubernetes-validations: + - message: completedAt is immutable once set + rule: self == oldSelf || oldSelf == null digest: description: |- digest is the digest of the phase's resolved object content @@ -2727,6 +2739,9 @@ spec: - digest - name type: object + x-kubernetes-validations: + - message: completedAt cannot be removed once set + rule: '!has(oldSelf.completedAt) || has(self.completedAt)' maxItems: 20 type: array x-kubernetes-list-map-keys: @@ -2736,6 +2751,10 @@ spec: x-kubernetes-validations: - message: completedAt cannot be removed once set rule: '!has(oldSelf.completedAt) || has(self.completedAt)' + - message: completed phases cannot be removed from observedPhases + rule: '!has(oldSelf.observedPhases) || oldSelf.observedPhases.all(p, + !has(p.completedAt) || (has(self.observedPhases) && self.observedPhases.exists(q, + q.name == p.name)))' required: - spec type: object