diff --git a/api/v1/clusterobjectdeployment_types.go b/api/v1/clusterobjectdeployment_types.go index fac421ffdf..2032128410 100644 --- a/api/v1/clusterobjectdeployment_types.go +++ b/api/v1/clusterobjectdeployment_types.go @@ -40,6 +40,22 @@ const ( // ClusterObjectDeploymentSpec defines the mutable desired state of a deployment. type ClusterObjectDeploymentSpec struct { + // revisionHistoryLimit is the maximum number of archived (superseded) + // ClusterObjectSet revisions to retain, so that rollout history can be + // inspected and earlier revisions rolled back to. Active revisions are never + // removed and do not count against this limit. When the number of archived + // revisions exceeds this value, the oldest archived revisions are deleted + // until the limit is met. + // + // A value of 0 retains no archived revisions. The minimum is 0, the maximum + // is 50, and the default is 5. + // + // +kubebuilder:validation:Minimum=0 + // +kubebuilder:validation:Maximum=50 + // +kubebuilder:default=5 + // +optional + RevisionHistoryLimit *int32 `json:"revisionHistoryLimit,omitempty"` + // template is the desired content of newly created ClusterObjectSets. // Changes to template create a new revision; existing revisions remain immutable. // +required diff --git a/api/v1/validation_test.go b/api/v1/validation_test.go index 1e7ce6e034..4ba26340c7 100644 --- a/api/v1/validation_test.go +++ b/api/v1/validation_test.go @@ -2,12 +2,14 @@ package v1 import ( "fmt" + "strings" "testing" "time" "github.com/stretchr/testify/require" "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/utils/ptr" "sigs.k8s.io/controller-runtime/pkg/client" ) @@ -183,6 +185,59 @@ func TestValidate(t *testing.T) { } } +// newRevisionHistoryLimitCOD returns a minimal valid ClusterObjectDeployment with +// the given revisionHistoryLimit (nil to leave it unset). +func newRevisionHistoryLimitCOD(name string, limit *int32) *ClusterObjectDeployment { + return &ClusterObjectDeployment{ + ObjectMeta: metav1.ObjectMeta{Name: name}, + Spec: ClusterObjectDeploymentSpec{ + RevisionHistoryLimit: limit, + Template: ClusterObjectDeploymentTemplate{ + Spec: ClusterObjectDeploymentTemplateSpec{ + CollisionProtection: CollisionProtectionPrevent, + Phases: []ClusterObjectSetPhase{ + {Name: "deploy", Objects: []ClusterObjectSetObject{{Object: configMap()}}}, + }, + }, + }, + }, + } +} + +func TestClusterObjectDeploymentRevisionHistoryLimitBounds(t *testing.T) { + c := newClient(t) + + for name, tc := range map[string]struct { + limit *int32 + valid bool + }{ + "below minimum is rejected": {limit: ptr.To(int32(-1)), valid: false}, + "minimum of zero is valid": {limit: ptr.To(int32(0)), valid: true}, + "maximum of fifty is valid": {limit: ptr.To(int32(50)), valid: true}, + "above maximum is rejected": {limit: ptr.To(int32(51)), valid: false}, + } { + t.Run(name, func(t *testing.T) { + cod := newRevisionHistoryLimitCOD("cod-rhl-"+strings.ReplaceAll(name, " ", "-"), tc.limit) + err := c.Create(t.Context(), cod) + if tc.valid { + require.NoError(t, err) + } else { + require.True(t, errors.IsInvalid(err), "expected create to fail as invalid, but got: %v", err) + } + }) + } +} + +func TestClusterObjectDeploymentRevisionHistoryLimitDefault(t *testing.T) { + c := newClient(t) + + cod := newRevisionHistoryLimitCOD("cod-rhl-default", nil) + require.NoError(t, c.Create(t.Context(), cod)) + + require.NotNil(t, cod.Spec.RevisionHistoryLimit, "revisionHistoryLimit should be defaulted on create when unset") + require.Equal(t, int32(5), *cod.Spec.RevisionHistoryLimit, "unset revisionHistoryLimit should default to 5") +} + func TestClusterObjectSetCompletedAtImmutable(t *testing.T) { c := newClient(t) diff --git a/api/v1/zz_generated.deepcopy.go b/api/v1/zz_generated.deepcopy.go index 0f679a1384..769cd89557 100644 --- a/api/v1/zz_generated.deepcopy.go +++ b/api/v1/zz_generated.deepcopy.go @@ -485,6 +485,11 @@ func (in *ClusterObjectDeploymentList) DeepCopyObject() runtime.Object { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *ClusterObjectDeploymentSpec) DeepCopyInto(out *ClusterObjectDeploymentSpec) { *out = *in + if in.RevisionHistoryLimit != nil { + in, out := &in.RevisionHistoryLimit, &out.RevisionHistoryLimit + *out = new(int32) + **out = **in + } in.Template.DeepCopyInto(&out.Template) } diff --git a/applyconfigurations/api/v1/clusterobjectdeploymentspec.go b/applyconfigurations/api/v1/clusterobjectdeploymentspec.go index 8064d0c598..79e73bdf70 100644 --- a/applyconfigurations/api/v1/clusterobjectdeploymentspec.go +++ b/applyconfigurations/api/v1/clusterobjectdeploymentspec.go @@ -23,6 +23,16 @@ package v1 // // ClusterObjectDeploymentSpec defines the mutable desired state of a deployment. type ClusterObjectDeploymentSpecApplyConfiguration struct { + // revisionHistoryLimit is the maximum number of archived (superseded) + // ClusterObjectSet revisions to retain, so that rollout history can be + // inspected and earlier revisions rolled back to. Active revisions are never + // removed and do not count against this limit. When the number of archived + // revisions exceeds this value, the oldest archived revisions are deleted + // until the limit is met. + // + // A value of 0 retains no archived revisions. The minimum is 0, the maximum + // is 50, and the default is 5. + RevisionHistoryLimit *int32 `json:"revisionHistoryLimit,omitempty"` // template is the desired content of newly created ClusterObjectSets. // Changes to template create a new revision; existing revisions remain immutable. Template *ClusterObjectDeploymentTemplateApplyConfiguration `json:"template,omitempty"` @@ -34,6 +44,14 @@ func ClusterObjectDeploymentSpec() *ClusterObjectDeploymentSpecApplyConfiguratio return &ClusterObjectDeploymentSpecApplyConfiguration{} } +// WithRevisionHistoryLimit sets the RevisionHistoryLimit 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 RevisionHistoryLimit field is set to the value of the last call. +func (b *ClusterObjectDeploymentSpecApplyConfiguration) WithRevisionHistoryLimit(value int32) *ClusterObjectDeploymentSpecApplyConfiguration { + b.RevisionHistoryLimit = &value + return b +} + // WithTemplate sets the Template 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 Template field is set to the value of the last call. diff --git a/applyconfigurations/internal/internal.go b/applyconfigurations/internal/internal.go index 0c03ea2e92..f5bc3c15da 100644 --- a/applyconfigurations/internal/internal.go +++ b/applyconfigurations/internal/internal.go @@ -270,6 +270,10 @@ var schemaYAML = typed.YAMLObject(`types: - name: com.github.operator-framework.operator-controller.api.v1.ClusterObjectDeploymentSpec map: fields: + - name: revisionHistoryLimit + type: + scalar: numeric + default: 5 - name: template type: namedType: com.github.operator-framework.operator-controller.api.v1.ClusterObjectDeploymentTemplate diff --git a/docs/api-reference/olmv1-api-reference.md b/docs/api-reference/olmv1-api-reference.md index 553bd54f71..fa49d42ec3 100644 --- a/docs/api-reference/olmv1-api-reference.md +++ b/docs/api-reference/olmv1-api-reference.md @@ -442,6 +442,7 @@ _Appears in:_ | Field | Description | Default | Validation | | --- | --- | --- | --- | +| `revisionHistoryLimit` _integer_ | revisionHistoryLimit is the maximum number of archived (superseded)
ClusterObjectSet revisions to retain, so that rollout history can be
inspected and earlier revisions rolled back to. Active revisions are never
removed and do not count against this limit. When the number of archived
revisions exceeds this value, the oldest archived revisions are deleted
until the limit is met.
A value of 0 retains no archived revisions. The minimum is 0, the maximum
is 50, and the default is 5. | 5 | Maximum: 50
Minimum: 0
Optional: \{\}
| | `template` _[ClusterObjectDeploymentTemplate](#clusterobjectdeploymenttemplate)_ | template is the desired content of newly created ClusterObjectSets.
Changes to template create a new revision; existing revisions remain immutable. | | Required: \{\}
| diff --git a/helm/olmv1/base/object-controller/crd/experimental/olm.operatorframework.io_clusterobjectdeployments.yaml b/helm/olmv1/base/object-controller/crd/experimental/olm.operatorframework.io_clusterobjectdeployments.yaml index 6d24b1ce5f..3634c469ae 100644 --- a/helm/olmv1/base/object-controller/crd/experimental/olm.operatorframework.io_clusterobjectdeployments.yaml +++ b/helm/olmv1/base/object-controller/crd/experimental/olm.operatorframework.io_clusterobjectdeployments.yaml @@ -54,6 +54,22 @@ spec: spec: description: spec defines the desired deployment template. properties: + revisionHistoryLimit: + default: 5 + description: |- + revisionHistoryLimit is the maximum number of archived (superseded) + ClusterObjectSet revisions to retain, so that rollout history can be + inspected and earlier revisions rolled back to. Active revisions are never + removed and do not count against this limit. When the number of archived + revisions exceeds this value, the oldest archived revisions are deleted + until the limit is met. + + A value of 0 retains no archived revisions. The minimum is 0, the maximum + is 50, and the default is 5. + format: int32 + maximum: 50 + minimum: 0 + type: integer template: description: |- template is the desired content of newly created ClusterObjectSets. diff --git a/internal/object-controller/controllers/clusterobjectdeployment_controller.go b/internal/object-controller/controllers/clusterobjectdeployment_controller.go index be41b36c2c..06efa0a1a8 100644 --- a/internal/object-controller/controllers/clusterobjectdeployment_controller.go +++ b/internal/object-controller/controllers/clusterobjectdeployment_controller.go @@ -10,6 +10,7 @@ import ( "slices" "k8s.io/apimachinery/pkg/api/equality" + apierrors "k8s.io/apimachinery/pkg/api/errors" apimeta "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" @@ -27,6 +28,11 @@ import ( hashutil "github.com/operator-framework/operator-controller/internal/shared/util/hash" ) +// defaultRevisionHistoryLimit is the number of archived revisions retained when +// spec.revisionHistoryLimit is unset. It mirrors the CRD default and is applied +// defensively for callers (e.g. tests) that bypass API-server defaulting. +const defaultRevisionHistoryLimit = 5 + // ClusterObjectDeploymentReconciler reconciles ClusterObjectDeployment objects by // managing their owned ClusterObjectSet revisions. type ClusterObjectDeploymentReconciler struct { @@ -68,9 +74,8 @@ func (c *ClusterObjectDeploymentReconciler) Reconcile(ctx context.Context, req c return ctrl.Result{}, reconcileErr } -// reconcile creates a new ClusterObjectSet revision when the template has changed -// (or no active revision exists) and reflects the latest revision's rollout -// progress in the deployment's Progressing condition. Requeues are driven by the +// reconcile garbage-collects excess archived revisions and reconciles the +// deployment's rollout (see reconcileRollout). Requeues are driven by the // controller's watch on owned ClusterObjectSets. func (c *ClusterObjectDeploymentReconciler) reconcile(ctx context.Context, cod *ocv1.ClusterObjectDeployment) error { revisions, err := c.listRevisions(ctx, cod.Name) @@ -78,6 +83,21 @@ func (c *ClusterObjectDeploymentReconciler) reconcile(ctx context.Context, cod * return err } + // Garbage collection is best-effort housekeeping and must not block rollout or + // status reconciliation: a persistently failing prune (e.g. a delete denied by + // an admission policy) would otherwise indefinitely stall deployment of an + // updated template and leave status stale even with a healthy active revision. + // Preserve the error, continue reconciling, and return it joined with the + // rollout outcome so the failure is still surfaced and retried. + gcErr := c.garbageCollectArchivedRevisions(ctx, cod, revisions) + + return errors.Join(gcErr, c.reconcileRollout(ctx, cod, revisions)) +} + +// reconcileRollout creates a new ClusterObjectSet revision when the template has +// changed (or no active revision exists) and reflects the latest revision's +// rollout progress in the deployment's Available and Progressing conditions. +func (c *ClusterObjectDeploymentReconciler) reconcileRollout(ctx context.Context, cod *ocv1.ClusterObjectDeployment, revisions []ocv1.ClusterObjectSet) error { hash := templateHash(cod.Spec.Template) setAvailable(cod, revisions) @@ -113,6 +133,74 @@ func (c *ClusterObjectDeploymentReconciler) listRevisions(ctx context.Context, g return list.Items, nil } +// garbageCollectArchivedRevisions deletes the oldest archived ClusterObjectSet +// revisions in excess of spec.revisionHistoryLimit. Active revisions are never +// deleted and do not count against the limit. A nil limit falls back to +// defaultRevisionHistoryLimit; a limit of 0 retains no archived revisions. +// +// Only archived revisions that have finished tearing down are candidates for +// pruning (see archivedTeardownComplete). Deleting a revision mid-teardown would +// send it down the ClusterObjectSet controller's deletion path, which removes the +// teardown finalizer without calling RevisionEngine.Teardown and so bypasses +// ordered reverse-phase cleanup (e.g. a controller could be removed before the +// resources it finalizes have terminated). A revision still tearing down is +// skipped and pruned on a later reconcile, which the controller's watch on owned +// ClusterObjectSets triggers once teardown completes. +// +// revisions must be sorted ascending by revision number (oldest first), as +// returned by listRevisions. +// +// NOTE: The candidate revisions come from listRevisions, which selects by +// spec.group (equal to the deployment name) rather than by this deployment's +// controller owner reference. This is deliberate and consistent with the rest of +// OLMv1: sibling membership of a revision family is defined by spec.group, not by +// ownership (see createRevision's naming/grouping and the ClusterObjectSet +// controller's own sibling archival, both keyed on spec.group). A consequence is +// that a ClusterObjectSet sharing this group but created by a different owner +// would be counted toward the limit and could be deleted here. That is tolerated +// because the deployment name is the group and is treated as an exclusive +// namespace for its revisions — nothing else should create ClusterObjectSets in +// it. If siblinghood is ever redefined in terms of ownership, this function and +// listRevisions must be updated together to filter by owner reference and UID. +func (c *ClusterObjectDeploymentReconciler) garbageCollectArchivedRevisions(ctx context.Context, cod *ocv1.ClusterObjectDeployment, revisions []ocv1.ClusterObjectSet) error { + limit := defaultRevisionHistoryLimit + if cod.Spec.RevisionHistoryLimit != nil { + limit = int(*cod.Spec.RevisionHistoryLimit) + } + + archived := make([]*ocv1.ClusterObjectSet, 0, len(revisions)) + for i := range revisions { + rev := &revisions[i] + if rev.DeletionTimestamp.IsZero() && + rev.Spec.LifecycleState == ocv1.ClusterObjectSetLifecycleStateArchived && + archivedTeardownComplete(rev) { + archived = append(archived, rev) + } + } + + // Delete the oldest archived revisions beyond the limit. Attempt every + // excess revision even if one fails, so a single revision that cannot be + // deleted (e.g. a denying webhook or a conflict) does not block pruning of + // the others on every reconcile. + var errs []error + for i := 0; i < len(archived)-limit; i++ { + if err := c.Client.Delete(ctx, archived[i]); err != nil && !apierrors.IsNotFound(err) { + errs = append(errs, fmt.Errorf("deleting archived revision %q: %w", archived[i].Name, err)) + } + } + return errors.Join(errs...) +} + +// archivedTeardownComplete reports whether an archived revision has finished +// tearing down its managed objects, as signalled by its Ready condition +// (Ready=False/Archived). The ClusterObjectSet controller sets this reason only +// after RevisionEngine.Teardown completes; while teardown is still in progress it +// reports Ready=False/Archiving. +func archivedTeardownComplete(rev *ocv1.ClusterObjectSet) bool { + cond := apimeta.FindStatusCondition(rev.Status.Conditions, ocv1.ClusterObjectSetTypeReady) + return cond != nil && cond.Reason == ocv1.ClusterObjectSetReasonArchived +} + // createRevision builds and creates the next ClusterObjectSet revision owned by // the deployment. Any create error, including AlreadyExists, is returned. // diff --git a/internal/object-controller/controllers/clusterobjectdeployment_controller_test.go b/internal/object-controller/controllers/clusterobjectdeployment_controller_test.go index f8c110be5f..77e89a24bc 100644 --- a/internal/object-controller/controllers/clusterobjectdeployment_controller_test.go +++ b/internal/object-controller/controllers/clusterobjectdeployment_controller_test.go @@ -4,6 +4,7 @@ package controllers_test import ( "context" + "fmt" "testing" "github.com/stretchr/testify/require" @@ -12,6 +13,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/types" + "k8s.io/utils/ptr" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" @@ -284,6 +286,219 @@ func Test_ClusterObjectDeploymentReconciler_CreatesNewRevisionWhenLatestArchived require.Equal(t, int64(2), rev2.Spec.Revision) } +// revisionCOS builds a ClusterObjectSet revision in the test deployment's group +// with the given revision number and lifecycle state, for garbage-collection tests. +// An archived revision is given Ready=False/Archived, mirroring a revision whose +// teardown has completed — the state in which garbage collection may prune it. +func revisionCOS(revision int64, lifecycle ocv1.ClusterObjectSetLifecycleState) *ocv1.ClusterObjectSet { + cos := &ocv1.ClusterObjectSet{ + ObjectMeta: metav1.ObjectMeta{Name: fmt.Sprintf("%s-%d", clusterObjectDeploymentName, revision)}, + Spec: ocv1.ClusterObjectSetSpec{ + Group: clusterObjectDeploymentName, + Revision: revision, + CollisionProtection: ocv1.CollisionProtectionPrevent, + LifecycleState: lifecycle, + }, + } + if lifecycle == ocv1.ClusterObjectSetLifecycleStateArchived { + setCOSReady(cos, metav1.ConditionFalse, ocv1.ClusterObjectSetReasonArchived) + } + return cos +} + +// setCOSReady sets the revision's Ready condition to the given status and reason. +func setCOSReady(cos *ocv1.ClusterObjectSet, status metav1.ConditionStatus, reason string) { + apimeta.SetStatusCondition(&cos.Status.Conditions, metav1.Condition{ + Type: ocv1.ClusterObjectSetTypeReady, + Status: status, + Reason: reason, + Message: "test", + }) +} + +func Test_ClusterObjectDeploymentReconciler_GarbageCollectsArchivedRevisions(t *testing.T) { + const active = ocv1.ClusterObjectSetLifecycleStateActive + const archived = ocv1.ClusterObjectSetLifecycleStateArchived + + type rev struct { + num int64 + state ocv1.ClusterObjectSetLifecycleState + // tearingDown marks an archived revision whose teardown has not yet + // completed (Ready=False/Archiving). Such a revision must not be pruned, so + // garbage collection does not short-circuit ordered reverse-phase cleanup. + tearingDown bool + } + + for _, tc := range []struct { + name string + limit *int32 + revisions []rev + wantGone []int64 + wantKept []int64 + }{ + { + name: "retains the newest archived revisions up to the limit", + limit: ptr.To(int32(2)), + revisions: []rev{ + {num: 1, state: archived}, {num: 2, state: archived}, {num: 3, state: archived}, {num: 4, state: active}, + }, + wantGone: []int64{1}, + wantKept: []int64{2, 3, 4}, + }, + { + name: "a limit of zero deletes all archived revisions", + limit: ptr.To(int32(0)), + revisions: []rev{ + {num: 1, state: archived}, {num: 2, state: archived}, {num: 3, state: active}, + }, + wantGone: []int64{1, 2}, + wantKept: []int64{3}, + }, + { + name: "active revisions are never deleted even below the limit", + limit: ptr.To(int32(0)), + revisions: []rev{ + {num: 1, state: active}, {num: 2, state: active}, {num: 3, state: active}, + }, + wantGone: nil, + wantKept: []int64{1, 2, 3}, + }, + { + name: "a nil limit defaults to retaining five archived revisions", + limit: nil, + revisions: []rev{ + {num: 1, state: archived}, {num: 2, state: archived}, {num: 3, state: archived}, + {num: 4, state: archived}, {num: 5, state: archived}, {num: 6, state: archived}, + }, + wantGone: []int64{1}, + wantKept: []int64{2, 3, 4, 5, 6}, + }, + { + name: "an archived revision still tearing down is not pruned", + limit: ptr.To(int32(0)), + revisions: []rev{ + {num: 1, state: archived, tearingDown: true}, + {num: 2, state: archived}, + {num: 3, state: active}, + }, + // Revision 1 is still tearing down (Ready=False/Archiving), so it is + // excluded from pruning until its controller completes teardown; + // revision 2 has finished tearing down and is pruned. + wantGone: []int64{2}, + wantKept: []int64{1, 3}, + }, + } { + t.Run(tc.name, func(t *testing.T) { + cod := codFixture() + cod.Spec.RevisionHistoryLimit = tc.limit + + objs := make([]client.Object, 0, 1+len(tc.revisions)) + objs = append(objs, cod) + for _, r := range tc.revisions { + cos := revisionCOS(r.num, r.state) + if r.tearingDown { + setCOSReady(cos, metav1.ConditionFalse, ocv1.ClusterObjectSetReasonArchiving) + } + objs = append(objs, cos) + } + c := newCODClient(t, objs...) + + _, err := reconcileCOD(t, c) + require.NoError(t, err) + + for _, n := range tc.wantGone { + _, ok := getCOS(t, c, fmt.Sprintf("%s-%d", clusterObjectDeploymentName, n)) + require.Falsef(t, ok, "archived revision %d beyond the limit should be garbage collected", n) + } + for _, n := range tc.wantKept { + _, ok := getCOS(t, c, fmt.Sprintf("%s-%d", clusterObjectDeploymentName, n)) + require.Truef(t, ok, "revision %d within the limit should be retained", n) + } + }) + } +} + +func Test_ClusterObjectDeploymentReconciler_GarbageCollectContinuesAfterDeleteError(t *testing.T) { + cod := codFixture() + cod.Spec.RevisionHistoryLimit = ptr.To(int32(0)) // every archived revision is excess + + failRev := clusterObjectDeploymentName + "-1" + + base := testutil.WithIndexes(t, fake.NewClientBuilder().WithScheme(newScheme(t)), controllers.SetupIndexes). + WithStatusSubresource(&ocv1.ClusterObjectDeployment{}, &ocv1.ClusterObjectSet{}). + WithObjects( + cod, + revisionCOS(1, ocv1.ClusterObjectSetLifecycleStateArchived), + revisionCOS(2, ocv1.ClusterObjectSetLifecycleStateArchived), + revisionCOS(3, ocv1.ClusterObjectSetLifecycleStateArchived), + ). + Build() + + // Fail the delete of the oldest excess revision; the rest must still be attempted. + c := interceptor.NewClient(base, interceptor.Funcs{ + Delete: func(ctx context.Context, cl client.WithWatch, obj client.Object, opts ...client.DeleteOption) error { + if obj.GetName() == failRev { + return apierrors.NewInternalError(fmt.Errorf("boom")) + } + return cl.Delete(ctx, obj, opts...) + }, + }) + + _, err := (&controllers.ClusterObjectDeploymentReconciler{Client: c, Scheme: newScheme(t)}). + Reconcile(t.Context(), ctrl.Request{NamespacedName: types.NamespacedName{Name: clusterObjectDeploymentName}}) + require.Error(t, err, "a delete failure during garbage collection must surface as a reconcile error") + + _, ok := getCOS(t, c, failRev) + require.True(t, ok, "the revision whose delete failed should remain") + _, ok = getCOS(t, c, clusterObjectDeploymentName+"-2") + require.False(t, ok, "later excess revisions must still be deleted despite an earlier delete failure") + _, ok = getCOS(t, c, clusterObjectDeploymentName+"-3") + require.False(t, ok, "later excess revisions must still be deleted despite an earlier delete failure") +} + +// Test_ClusterObjectDeploymentReconciler_RolloutProceedsDespiteGarbageCollectError +// verifies that a persistently failing archived-revision prune does not block the +// rollout: a new revision is still created, status is still reconciled, and the +// cleanup error is surfaced so the reconcile is retried. +func Test_ClusterObjectDeploymentReconciler_RolloutProceedsDespiteGarbageCollectError(t *testing.T) { + cod := codFixture() + cod.Spec.RevisionHistoryLimit = ptr.To(int32(0)) // the archived revision is excess + + failRev := clusterObjectDeploymentName + "-1" + + base := testutil.WithIndexes(t, fake.NewClientBuilder().WithScheme(newScheme(t)), controllers.SetupIndexes). + WithStatusSubresource(&ocv1.ClusterObjectDeployment{}, &ocv1.ClusterObjectSet{}). + WithObjects( + cod, + // An archived revision whose prune is denied, and no active revision, so + // the rollout must create a new one. + revisionCOS(1, ocv1.ClusterObjectSetLifecycleStateArchived), + ). + Build() + + c := interceptor.NewClient(base, interceptor.Funcs{ + Delete: func(ctx context.Context, cl client.WithWatch, obj client.Object, opts ...client.DeleteOption) error { + if obj.GetName() == failRev { + return apierrors.NewInternalError(fmt.Errorf("boom")) + } + return cl.Delete(ctx, obj, opts...) + }, + }) + + _, err := (&controllers.ClusterObjectDeploymentReconciler{Client: c, Scheme: newScheme(t)}). + Reconcile(t.Context(), ctrl.Request{NamespacedName: types.NamespacedName{Name: clusterObjectDeploymentName}}) + require.Error(t, err, "the garbage-collection failure must still be surfaced") + + _, ok := getCOS(t, c, failRev) + require.True(t, ok, "the archived revision whose delete failed should remain") + rev2, ok := getCOS(t, c, clusterObjectDeploymentName+"-2") + require.True(t, ok, "the rollout must create a new revision despite the garbage-collection failure") + require.Equal(t, int64(2), rev2.Spec.Revision) + + require.NotNil(t, available(t, c), "the Available condition must be reconciled despite the garbage-collection failure") + require.NotNil(t, progressing(t, c), "the Progressing condition must be reconciled despite the garbage-collection failure") +} + func Test_ClusterObjectDeploymentReconciler_DeletionIsNoop(t *testing.T) { cod := codFixture() now := metav1.Now() diff --git a/manifests/experimental-e2e.yaml b/manifests/experimental-e2e.yaml index 661c4bd1be..b8a342a229 100644 --- a/manifests/experimental-e2e.yaml +++ b/manifests/experimental-e2e.yaml @@ -1465,6 +1465,22 @@ spec: spec: description: spec defines the desired deployment template. properties: + revisionHistoryLimit: + default: 5 + description: |- + revisionHistoryLimit is the maximum number of archived (superseded) + ClusterObjectSet revisions to retain, so that rollout history can be + inspected and earlier revisions rolled back to. Active revisions are never + removed and do not count against this limit. When the number of archived + revisions exceeds this value, the oldest archived revisions are deleted + until the limit is met. + + A value of 0 retains no archived revisions. The minimum is 0, the maximum + is 50, and the default is 5. + format: int32 + maximum: 50 + minimum: 0 + type: integer template: description: |- template is the desired content of newly created ClusterObjectSets. diff --git a/manifests/experimental.yaml b/manifests/experimental.yaml index bc970c43f4..f561c1ce72 100644 --- a/manifests/experimental.yaml +++ b/manifests/experimental.yaml @@ -1426,6 +1426,22 @@ spec: spec: description: spec defines the desired deployment template. properties: + revisionHistoryLimit: + default: 5 + description: |- + revisionHistoryLimit is the maximum number of archived (superseded) + ClusterObjectSet revisions to retain, so that rollout history can be + inspected and earlier revisions rolled back to. Active revisions are never + removed and do not count against this limit. When the number of archived + revisions exceeds this value, the oldest archived revisions are deleted + until the limit is met. + + A value of 0 retains no archived revisions. The minimum is 0, the maximum + is 50, and the default is 5. + format: int32 + maximum: 50 + minimum: 0 + type: integer template: description: |- template is the desired content of newly created ClusterObjectSets. diff --git a/test/e2e/features/cod_revision_history.feature b/test/e2e/features/cod_revision_history.feature new file mode 100644 index 0000000000..0466bb6c4e --- /dev/null +++ b/test/e2e/features/cod_revision_history.feature @@ -0,0 +1,93 @@ +@BoxcutterRuntime +Feature: ClusterObjectDeployment revision history limit + + As an OLM user applying a ClusterObjectDeployment, I want archived (superseded) + ClusterObjectSet revisions beyond spec.revisionHistoryLimit to be garbage + collected, so that rollout history is bounded while recent revisions remain + available for inspection and rollback. + + Background: + Given OLM is available + + Scenario: Archived revisions beyond the revisionHistoryLimit are garbage collected + Given namespace "${TEST_NAMESPACE}" is available + When resource is applied + """ + apiVersion: olm.operatorframework.io/v1 + kind: ClusterObjectDeployment + metadata: + name: ${NAME} + spec: + revisionHistoryLimit: 1 + template: + spec: + collisionProtection: Prevent + phases: + - name: resources + objects: + - object: + apiVersion: v1 + kind: ConfigMap + metadata: + name: cod-cm + namespace: ${TEST_NAMESPACE} + data: + k: v1 + """ + Then ClusterObjectSet "${NAME}-1" reports Ready as True with Reason AllObjectsReady + # Second revision supersedes and archives the first. + When resource is applied + """ + apiVersion: olm.operatorframework.io/v1 + kind: ClusterObjectDeployment + metadata: + name: ${NAME} + spec: + revisionHistoryLimit: 1 + template: + spec: + collisionProtection: Prevent + phases: + - name: resources + objects: + - object: + apiVersion: v1 + kind: ConfigMap + metadata: + name: cod-cm + namespace: ${TEST_NAMESPACE} + data: + k: v2 + """ + Then ClusterObjectSet "${NAME}-2" reports Ready as True with Reason AllObjectsReady + And ClusterObjectSet "${NAME}-1" is archived + # Third revision supersedes and archives the second. With revisionHistoryLimit=1 + # there are now two archived revisions (1 and 2), so the oldest (1) is pruned + # while the newest archived revision (2) is retained. + When resource is applied + """ + apiVersion: olm.operatorframework.io/v1 + kind: ClusterObjectDeployment + metadata: + name: ${NAME} + spec: + revisionHistoryLimit: 1 + template: + spec: + collisionProtection: Prevent + phases: + - name: resources + objects: + - object: + apiVersion: v1 + kind: ConfigMap + metadata: + name: cod-cm + namespace: ${TEST_NAMESPACE} + data: + k: v3 + """ + Then ClusterObjectSet "${NAME}-3" reports Ready as True with Reason AllObjectsReady + And ClusterObjectSet "${NAME}-2" is archived + And ClusterObjectSet "${NAME}-1" is eventually deleted + And ClusterObjectDeployment "${NAME}" reports Available as True with Reason Available diff --git a/test/e2e/steps/steps.go b/test/e2e/steps/steps.go index e3b23f5571..4c10162cfc 100644 --- a/test/e2e/steps/steps.go +++ b/test/e2e/steps/steps.go @@ -135,6 +135,7 @@ func RegisterSteps(sc *godog.ScenarioContext) { sc.Step(`^(?i)ClusterObjectSet "([^"]+)" reconciliation is triggered$`, TriggerClusterObjectSetReconciliation) sc.Step(`^(?i)ClusterObjectSet "([^"]+)" has observed phase "([^"]+)" with a non-empty digest$`, ClusterObjectSetHasObservedPhase) sc.Step(`^(?i)ClusterObjectSet "([^"]+)" is archived$`, ClusterObjectSetIsArchived) + sc.Step(`^(?i)ClusterObjectSet "([^"]+)" is eventually deleted$`, ClusterObjectSetIsEventuallyDeleted) sc.Step(`^(?i)ClusterObjectSet "([^"]+)" reports phase "([^"]+)" object counts: total=(\d+), present=(\d+), synced=(\d+), ready=(\d+)$`, ClusterObjectSetPhaseHasObjectCounts) sc.Step(`^(?i)ClusterObjectSet "([^"]+)" reports aggregate object counts: total=(\d+), present=(\d+), synced=(\d+), ready=(\d+)$`, ClusterObjectSetHasAggregateObjectCounts) sc.Step(`^(?i)ClusterObjectSet "([^"]+)" contains annotation "([^"]+)" with value$`, ClusterObjectSetHasAnnotationWithValue) @@ -1051,6 +1052,21 @@ func ClusterObjectSetIsArchived(ctx context.Context, revisionName string) error return waitForCondition(ctx, "clusterobjectset", substituteScenarioVars(revisionName, scenarioCtx(ctx)), "Ready", "False", ptr.To("Archived"), nil) } +// ClusterObjectSetIsEventuallyDeleted waits for the named ClusterObjectSet to be +// removed from the cluster, e.g. garbage collected once it is archived beyond the +// owning ClusterObjectDeployment's revisionHistoryLimit. Polls with timeout. +func ClusterObjectSetIsEventuallyDeleted(ctx context.Context, revisionName string) error { + revisionName = substituteScenarioVars(strings.TrimSpace(revisionName), scenarioCtx(ctx)) + waitFor(ctx, func() bool { + out, err := k8sClient(ctx, "get", "clusterobjectset", revisionName, "--ignore-not-found", "-o", "json") + if err != nil { + return false + } + return strings.TrimSpace(out) == "" + }) + return nil +} + // ClusterObjectSetPhaseHasObjectCounts waits for a specific phase of the named ClusterObjectSet to // report exact object counts for total, present, synced, and available. Polls with timeout. func ClusterObjectSetPhaseHasObjectCounts(ctx context.Context, cosName, phaseName string, total, present, synced, ready int64) error {