From b640387e3fbf5ada74a09799ee881d30b97120d7 Mon Sep 17 00:00:00 2001 From: Daniel Franz Date: Wed, 10 Sep 2025 22:28:36 +0900 Subject: [PATCH] CRE Previous Limit Sets a limit to the number of previous ClusterExtensionRevisions we keep in the cluster, and trims the list of previous revisions when creating new revisions to stay at the limit. The limit has been set to 5 for now. Any revisions beyond this limit will be removed from the cluster. Signed-off-by: Daniel Franz --- .../operator-controller/applier/boxcutter.go | 38 +++- .../applier/boxcutter_test.go | 213 ++++++++++++++++++ 2 files changed, 243 insertions(+), 8 deletions(-) diff --git a/internal/operator-controller/applier/boxcutter.go b/internal/operator-controller/applier/boxcutter.go index 74f5574eea..ef9e7a4723 100644 --- a/internal/operator-controller/applier/boxcutter.go +++ b/internal/operator-controller/applier/boxcutter.go @@ -16,6 +16,7 @@ import ( "github.com/davecgh/go-spew/spew" "helm.sh/helm/v3/pkg/release" "helm.sh/helm/v3/pkg/storage/driver" + apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" @@ -35,7 +36,8 @@ import ( ) const ( - RevisionHashAnnotation = "olm.operatorframework.io/hash" + RevisionHashAnnotation = "olm.operatorframework.io/hash" + ClusterExtensionRevisionPreviousLimit = 5 ) type ClusterExtensionRevisionGenerator interface { @@ -285,11 +287,9 @@ func (bc *Boxcutter) apply(ctx context.Context, contentFS fs.FS, ext *ocv1.Clust } newRevision.Annotations[RevisionHashAnnotation] = desiredHash newRevision.Spec.Revision = revisionNumber - for _, prevRevision := range prevRevisions { - newRevision.Spec.Previous = append(newRevision.Spec.Previous, ocv1.ClusterExtensionRevisionPrevious{ - Name: prevRevision.Name, - UID: prevRevision.UID, - }) + + if err = bc.setPreviousRevisions(ctx, newRevision, prevRevisions); err != nil { + return false, "", fmt.Errorf("garbage collecting old Revisions: %w", err) } if err := controllerutil.SetControllerReference(ext, newRevision, bc.Scheme); err != nil { @@ -301,8 +301,6 @@ func (bc *Boxcutter) apply(ctx context.Context, contentFS fs.FS, ext *ocv1.Clust currentRevision = newRevision } - // TODO: Delete archived previous revisions over a certain revision limit - progressingCondition := meta.FindStatusCondition(currentRevision.Status.Conditions, ocv1.TypeProgressing) availableCondition := meta.FindStatusCondition(currentRevision.Status.Conditions, ocv1.ClusterExtensionRevisionTypeAvailable) succeededCondition := meta.FindStatusCondition(currentRevision.Status.Conditions, ocv1.ClusterExtensionRevisionTypeSucceeded) @@ -319,6 +317,30 @@ func (bc *Boxcutter) apply(ctx context.Context, contentFS fs.FS, ext *ocv1.Clust return true, "", nil } +// setPreviousRevisions populates spec.previous of latestRevision, trimming the list of previous _archived_ revisions down to +// ClusterExtensionRevisionPreviousLimit or to the first _active_ revision and deletes trimmed revisions from the cluster. +// NOTE: revisionList must be sorted in chronographical order, from oldest to latest. +func (bc *Boxcutter) setPreviousRevisions(ctx context.Context, latestRevision *ocv1.ClusterExtensionRevision, revisionList []ocv1.ClusterExtensionRevision) error { + trimmedPrevious := make([]ocv1.ClusterExtensionRevisionPrevious, 0) + for index, r := range revisionList { + if index < len(revisionList)-ClusterExtensionRevisionPreviousLimit && r.Spec.LifecycleState == ocv1.ClusterExtensionRevisionLifecycleStateArchived { + // Delete oldest CREs from the cluster and list to reach ClusterExtensionRevisionPreviousLimit or latest active revision + if err := bc.Client.Delete(ctx, &ocv1.ClusterExtensionRevision{ + ObjectMeta: metav1.ObjectMeta{ + Name: r.Name, + }, + }); err != nil && !apierrors.IsNotFound(err) { + return fmt.Errorf("deleting previous archived Revision: %w", err) + } + } else { + // All revisions within the limit or still active are preserved + trimmedPrevious = append(trimmedPrevious, ocv1.ClusterExtensionRevisionPrevious{Name: r.Name, UID: r.GetUID()}) + } + } + latestRevision.Spec.Previous = trimmedPrevious + return nil +} + // getExistingRevisions returns the list of ClusterExtensionRevisions for a ClusterExtension with name extName in revision order (oldest to newest) func (bc *Boxcutter) getExistingRevisions(ctx context.Context, extName string) ([]ocv1.ClusterExtensionRevision, error) { existingRevisionList := &ocv1.ClusterExtensionRevisionList{} diff --git a/internal/operator-controller/applier/boxcutter_test.go b/internal/operator-controller/applier/boxcutter_test.go index 92d2ea2ffc..0b3b1ea700 100644 --- a/internal/operator-controller/applier/boxcutter_test.go +++ b/internal/operator-controller/applier/boxcutter_test.go @@ -15,6 +15,7 @@ import ( "helm.sh/helm/v3/pkg/storage/driver" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" @@ -567,6 +568,218 @@ func TestBoxcutter_Apply(t *testing.T) { assert.Empty(t, revList.Items) }, }, + { + name: "sixth revision", + mockBuilder: &mockBundleRevisionBuilder{ + makeRevisionFunc: func(bundleFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionAnnotations map[string]string) (*ocv1.ClusterExtensionRevision, error) { + return &ocv1.ClusterExtensionRevision{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: revisionAnnotations, + Labels: map[string]string{ + controllers.ClusterExtensionRevisionOwnerLabel: ext.Name, + }, + }, + Spec: ocv1.ClusterExtensionRevisionSpec{}, + }, nil + }, + }, + existingObjs: []client.Object{ + &ocv1.ClusterExtensionRevision{ + ObjectMeta: metav1.ObjectMeta{ + Name: "rev-1", + Labels: map[string]string{ + controllers.ClusterExtensionRevisionOwnerLabel: ext.Name, + }, + }, + Spec: ocv1.ClusterExtensionRevisionSpec{ + LifecycleState: ocv1.ClusterExtensionRevisionLifecycleStateArchived, + }, + }, + &ocv1.ClusterExtensionRevision{ + ObjectMeta: metav1.ObjectMeta{ + Name: "rev-2", + Labels: map[string]string{ + controllers.ClusterExtensionRevisionOwnerLabel: ext.Name, + }, + }, + Spec: ocv1.ClusterExtensionRevisionSpec{ + LifecycleState: ocv1.ClusterExtensionRevisionLifecycleStateArchived, + }, + }, + &ocv1.ClusterExtensionRevision{ + ObjectMeta: metav1.ObjectMeta{ + Name: "rev-3", + Labels: map[string]string{ + controllers.ClusterExtensionRevisionOwnerLabel: ext.Name, + }, + }, + Spec: ocv1.ClusterExtensionRevisionSpec{ + LifecycleState: ocv1.ClusterExtensionRevisionLifecycleStateArchived, + }, + }, + &ocv1.ClusterExtensionRevision{ + ObjectMeta: metav1.ObjectMeta{ + Name: "rev-4", + Labels: map[string]string{ + controllers.ClusterExtensionRevisionOwnerLabel: ext.Name, + }, + }, + Spec: ocv1.ClusterExtensionRevisionSpec{ + LifecycleState: ocv1.ClusterExtensionRevisionLifecycleStateArchived, + }, + }, + &ocv1.ClusterExtensionRevision{ + ObjectMeta: metav1.ObjectMeta{ + Name: "rev-5", + Labels: map[string]string{ + controllers.ClusterExtensionRevisionOwnerLabel: ext.Name, + }, + }, + Spec: ocv1.ClusterExtensionRevisionSpec{ + LifecycleState: ocv1.ClusterExtensionRevisionLifecycleStateArchived, + }, + }, + &ocv1.ClusterExtensionRevision{ + ObjectMeta: metav1.ObjectMeta{ + Name: "rev-6", + Labels: map[string]string{ + controllers.ClusterExtensionRevisionOwnerLabel: ext.Name, + }, + }, + Spec: ocv1.ClusterExtensionRevisionSpec{ + LifecycleState: ocv1.ClusterExtensionRevisionLifecycleStateArchived, + }, + }, + }, + validate: func(t *testing.T, c client.Client) { + rev1 := &ocv1.ClusterExtensionRevision{} + err := c.Get(t.Context(), client.ObjectKey{Name: "rev-1"}, rev1) + require.Error(t, err) + assert.True(t, apierrors.IsNotFound(err)) + + latest := &ocv1.ClusterExtensionRevision{} + err = c.Get(t.Context(), client.ObjectKey{Name: "test-ext-1"}, latest) + require.NoError(t, err) + assert.Len(t, latest.Spec.Previous, applier.ClusterExtensionRevisionPreviousLimit) + }, + }, + { + name: "len([]revisions) > limit but contains active revisions with index beyond limit", + mockBuilder: &mockBundleRevisionBuilder{ + makeRevisionFunc: func(bundleFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionAnnotations map[string]string) (*ocv1.ClusterExtensionRevision, error) { + return &ocv1.ClusterExtensionRevision{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: revisionAnnotations, + Labels: map[string]string{ + controllers.ClusterExtensionRevisionOwnerLabel: ext.Name, + }, + }, + Spec: ocv1.ClusterExtensionRevisionSpec{}, + }, nil + }, + }, + existingObjs: []client.Object{ + &ocv1.ClusterExtensionRevision{ + ObjectMeta: metav1.ObjectMeta{ + Name: "rev-1", + Labels: map[string]string{ + controllers.ClusterExtensionRevisionOwnerLabel: ext.Name, + }, + }, + Spec: ocv1.ClusterExtensionRevisionSpec{ + LifecycleState: ocv1.ClusterExtensionRevisionLifecycleStateArchived, + }, + }, + &ocv1.ClusterExtensionRevision{ + ObjectMeta: metav1.ObjectMeta{ + Name: "rev-2", + Labels: map[string]string{ + controllers.ClusterExtensionRevisionOwnerLabel: ext.Name, + }, + }, + Spec: ocv1.ClusterExtensionRevisionSpec{ + // index beyond the retention limit but active; should be preserved + LifecycleState: ocv1.ClusterExtensionRevisionLifecycleStateActive, + }, + }, + &ocv1.ClusterExtensionRevision{ + ObjectMeta: metav1.ObjectMeta{ + Name: "rev-3", + Labels: map[string]string{ + controllers.ClusterExtensionRevisionOwnerLabel: ext.Name, + }, + }, + Spec: ocv1.ClusterExtensionRevisionSpec{ + LifecycleState: ocv1.ClusterExtensionRevisionLifecycleStateActive, + }, + }, + &ocv1.ClusterExtensionRevision{ + ObjectMeta: metav1.ObjectMeta{ + Name: "rev-4", + Labels: map[string]string{ + controllers.ClusterExtensionRevisionOwnerLabel: ext.Name, + }, + }, + Spec: ocv1.ClusterExtensionRevisionSpec{ + // archived but should be preserved since it is within the limit + LifecycleState: ocv1.ClusterExtensionRevisionLifecycleStateArchived, + }, + }, + &ocv1.ClusterExtensionRevision{ + ObjectMeta: metav1.ObjectMeta{ + Name: "rev-5", + Labels: map[string]string{ + controllers.ClusterExtensionRevisionOwnerLabel: ext.Name, + }, + }, + Spec: ocv1.ClusterExtensionRevisionSpec{ + LifecycleState: ocv1.ClusterExtensionRevisionLifecycleStateActive, + }, + }, + &ocv1.ClusterExtensionRevision{ + ObjectMeta: metav1.ObjectMeta{ + Name: "rev-6", + Labels: map[string]string{ + controllers.ClusterExtensionRevisionOwnerLabel: ext.Name, + }, + }, + Spec: ocv1.ClusterExtensionRevisionSpec{ + LifecycleState: ocv1.ClusterExtensionRevisionLifecycleStateActive, + }, + }, + &ocv1.ClusterExtensionRevision{ + ObjectMeta: metav1.ObjectMeta{ + Name: "rev-7", + Labels: map[string]string{ + controllers.ClusterExtensionRevisionOwnerLabel: ext.Name, + }, + }, + Spec: ocv1.ClusterExtensionRevisionSpec{ + LifecycleState: ocv1.ClusterExtensionRevisionLifecycleStateActive, + }, + }, + }, + validate: func(t *testing.T, c client.Client) { + rev1 := &ocv1.ClusterExtensionRevision{} + err := c.Get(t.Context(), client.ObjectKey{Name: "rev-1"}, rev1) + require.Error(t, err) + assert.True(t, apierrors.IsNotFound(err)) + + rev2 := &ocv1.ClusterExtensionRevision{} + err = c.Get(t.Context(), client.ObjectKey{Name: "rev-2"}, rev2) + require.NoError(t, err) + + rev4 := &ocv1.ClusterExtensionRevision{} + err = c.Get(t.Context(), client.ObjectKey{Name: "rev-4"}, rev4) + require.NoError(t, err) + + latest := &ocv1.ClusterExtensionRevision{} + err = c.Get(t.Context(), client.ObjectKey{Name: "test-ext-1"}, latest) + require.NoError(t, err) + assert.Len(t, latest.Spec.Previous, 6) + assert.Contains(t, latest.Spec.Previous, ocv1.ClusterExtensionRevisionPrevious{Name: "rev-4"}) + }, + }, } for _, tc := range testCases {