diff --git a/internal/operator-controller/applier/boxcutter.go b/internal/operator-controller/applier/boxcutter.go index 74f5574ee..ef9e7a472 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 92d2ea2ff..0b3b1ea70 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 {