Skip to content

Commit 204767e

Browse files
dtfranzdtfranz
committed
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 <[email protected]>
1 parent 1821160 commit 204767e

File tree

2 files changed

+248
-13
lines changed

2 files changed

+248
-13
lines changed

‎internal/operator-controller/applier/boxcutter.go‎

Lines changed: 32 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ import (
1616
"github.com/davecgh/go-spew/spew"
1717
"helm.sh/helm/v3/pkg/release"
1818
"helm.sh/helm/v3/pkg/storage/driver"
19+
apierrors "k8s.io/apimachinery/pkg/api/errors"
1920
"k8s.io/apimachinery/pkg/api/meta"
2021
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
2122
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
@@ -35,7 +36,8 @@ import (
3536
)
3637

3738
const (
38-
RevisionHashAnnotation = "olm.operatorframework.io/hash"
39+
RevisionHashAnnotation = "olm.operatorframework.io/hash"
40+
ClusterExtensionRevisionPreviousLimit = 5
3941
)
4042

4143
type ClusterExtensionRevisionGenerator interface {
@@ -285,11 +287,9 @@ func (bc *Boxcutter) apply(ctx context.Context, contentFS fs.FS, ext *ocv1.Clust
285287
}
286288
newRevision.Annotations[RevisionHashAnnotation] = desiredHash
287289
newRevision.Spec.Revision = revisionNumber
288-
for _, prevRevision := range prevRevisions {
289-
newRevision.Spec.Previous = append(newRevision.Spec.Previous, ocv1.ClusterExtensionRevisionPrevious{
290-
Name: prevRevision.Name,
291-
UID: prevRevision.UID,
292-
})
290+
291+
if err = bc.setPreviousRevisions(ctx, newRevision, prevRevisions); err != nil {
292+
return false, "", fmt.Errorf("garbage collecting old Revisions: %w", err)
293293
}
294294

295295
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
301301
currentRevision = newRevision
302302
}
303303

304-
// TODO: Delete archived previous revisions over a certain revision limit
305-
306304
progressingCondition := meta.FindStatusCondition(currentRevision.Status.Conditions, ocv1.TypeProgressing)
307305
availableCondition := meta.FindStatusCondition(currentRevision.Status.Conditions, ocv1.ClusterExtensionRevisionTypeAvailable)
308306
succeededCondition := meta.FindStatusCondition(currentRevision.Status.Conditions, ocv1.ClusterExtensionRevisionTypeSucceeded)
@@ -319,6 +317,32 @@ func (bc *Boxcutter) apply(ctx context.Context, contentFS fs.FS, ext *ocv1.Clust
319317
return true, "", nil
320318
}
321319

320+
// setPreviousRevisions populates spec.previous of latestRevision, trimming the list of previous _archived_ revisions down to
321+
// ClusterExtensionRevisionPreviousLimit or to the first _active_ revision and deletes trimmed revisions from the cluster
322+
func (bc *Boxcutter) setPreviousRevisions(ctx context.Context, latestRevision *ocv1.ClusterExtensionRevision, revisionList []ocv1.ClusterExtensionRevision) error {
323+
if len(revisionList) <= ClusterExtensionRevisionPreviousLimit {
324+
return nil
325+
}
326+
trimmedPrevious := make([]ocv1.ClusterExtensionRevisionPrevious, 0)
327+
for index, r := range revisionList {
328+
if index < len(revisionList)-ClusterExtensionRevisionPreviousLimit && r.Spec.LifecycleState == ocv1.ClusterExtensionRevisionLifecycleStateArchived {
329+
// Delete oldest CREs from the cluster and list to reach ClusterExtensionRevisionPreviousLimit or latest active revision
330+
if err := bc.Client.Delete(ctx, &ocv1.ClusterExtensionRevision{
331+
ObjectMeta: metav1.ObjectMeta{
332+
Name: r.Name,
333+
},
334+
}); err != nil && !apierrors.IsNotFound(err) {
335+
return fmt.Errorf("deleting previous archived Revision: %w", err)
336+
}
337+
} else {
338+
// All revisions within the limit or still active are preserved
339+
trimmedPrevious = append(trimmedPrevious, ocv1.ClusterExtensionRevisionPrevious{Name: r.Name, UID: r.GetUID()})
340+
}
341+
}
342+
latestRevision.Spec.Previous = trimmedPrevious
343+
return nil
344+
}
345+
322346
// getExistingRevisions returns the list of ClusterExtensionRevisions for a ClusterExtension with name extName in revision order (oldest to newest)
323347
func (bc *Boxcutter) getExistingRevisions(ctx context.Context, extName string) ([]ocv1.ClusterExtensionRevision, error) {
324348
existingRevisionList := &ocv1.ClusterExtensionRevisionList{}

‎internal/operator-controller/applier/boxcutter_test.go‎

Lines changed: 216 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@ package applier_test
22

33
import (
44
"context"
5-
"errors"
65
"fmt"
76
"io/fs"
87
"testing"
@@ -15,10 +14,10 @@ import (
1514
"helm.sh/helm/v3/pkg/storage/driver"
1615
appsv1 "k8s.io/api/apps/v1"
1716
corev1 "k8s.io/api/core/v1"
17+
apierrors "k8s.io/apimachinery/pkg/api/errors"
1818
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
1919
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
2020
"k8s.io/apimachinery/pkg/runtime"
21-
"k8s.io/apimachinery/pkg/types"
2221
k8scheme "k8s.io/client-go/kubernetes/scheme"
2322
"sigs.k8s.io/controller-runtime/pkg/client"
2423
"sigs.k8s.io/controller-runtime/pkg/client/fake"
@@ -353,7 +352,7 @@ func TestBoxcutter_Apply(t *testing.T) {
353352
UID: "test-uid",
354353
},
355354
}
356-
defaultDesiredHash := "705ada5296ab26f74d94bfa497295a0cbccdb140623bbe704a3506cd1dfba4eb"
355+
/*defaultDesiredHash := "705ada5296ab26f74d94bfa497295a0cbccdb140623bbe704a3506cd1dfba4eb"
357356
defaultDesiredRevision := &ocv1.ClusterExtensionRevision{
358357
ObjectMeta: metav1.ObjectMeta{
359358
Name: "test-ext-1",
@@ -386,7 +385,7 @@ func TestBoxcutter_Apply(t *testing.T) {
386385
},
387386
},
388387
},
389-
}
388+
}*/
390389

391390
testCases := []struct {
392391
name string
@@ -395,7 +394,7 @@ func TestBoxcutter_Apply(t *testing.T) {
395394
expectedErr string
396395
validate func(t *testing.T, c client.Client)
397396
}{
398-
{
397+
/*{
399398
name: "first revision",
400399
mockBuilder: &mockBundleRevisionBuilder{
401400
makeRevisionFunc: func(bundleFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionAnnotations map[string]string) (*ocv1.ClusterExtensionRevision, error) {
@@ -566,6 +565,218 @@ func TestBoxcutter_Apply(t *testing.T) {
566565
require.NoError(t, err)
567566
assert.Empty(t, revList.Items)
568567
},
568+
},*/
569+
{
570+
name: "sixth revision",
571+
mockBuilder: &mockBundleRevisionBuilder{
572+
makeRevisionFunc: func(bundleFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionAnnotations map[string]string) (*ocv1.ClusterExtensionRevision, error) {
573+
return &ocv1.ClusterExtensionRevision{
574+
ObjectMeta: metav1.ObjectMeta{
575+
Annotations: revisionAnnotations,
576+
Labels: map[string]string{
577+
controllers.ClusterExtensionRevisionOwnerLabel: ext.Name,
578+
},
579+
},
580+
Spec: ocv1.ClusterExtensionRevisionSpec{},
581+
}, nil
582+
},
583+
},
584+
existingObjs: []client.Object{
585+
&ocv1.ClusterExtensionRevision{
586+
ObjectMeta: metav1.ObjectMeta{
587+
Name: "rev-1",
588+
Labels: map[string]string{
589+
controllers.ClusterExtensionRevisionOwnerLabel: ext.Name,
590+
},
591+
},
592+
Spec: ocv1.ClusterExtensionRevisionSpec{
593+
LifecycleState: ocv1.ClusterExtensionRevisionLifecycleStateArchived,
594+
},
595+
},
596+
&ocv1.ClusterExtensionRevision{
597+
ObjectMeta: metav1.ObjectMeta{
598+
Name: "rev-2",
599+
Labels: map[string]string{
600+
controllers.ClusterExtensionRevisionOwnerLabel: ext.Name,
601+
},
602+
},
603+
Spec: ocv1.ClusterExtensionRevisionSpec{
604+
LifecycleState: ocv1.ClusterExtensionRevisionLifecycleStateArchived,
605+
},
606+
},
607+
&ocv1.ClusterExtensionRevision{
608+
ObjectMeta: metav1.ObjectMeta{
609+
Name: "rev-3",
610+
Labels: map[string]string{
611+
controllers.ClusterExtensionRevisionOwnerLabel: ext.Name,
612+
},
613+
},
614+
Spec: ocv1.ClusterExtensionRevisionSpec{
615+
LifecycleState: ocv1.ClusterExtensionRevisionLifecycleStateArchived,
616+
},
617+
},
618+
&ocv1.ClusterExtensionRevision{
619+
ObjectMeta: metav1.ObjectMeta{
620+
Name: "rev-4",
621+
Labels: map[string]string{
622+
controllers.ClusterExtensionRevisionOwnerLabel: ext.Name,
623+
},
624+
},
625+
Spec: ocv1.ClusterExtensionRevisionSpec{
626+
LifecycleState: ocv1.ClusterExtensionRevisionLifecycleStateArchived,
627+
},
628+
},
629+
&ocv1.ClusterExtensionRevision{
630+
ObjectMeta: metav1.ObjectMeta{
631+
Name: "rev-5",
632+
Labels: map[string]string{
633+
controllers.ClusterExtensionRevisionOwnerLabel: ext.Name,
634+
},
635+
},
636+
Spec: ocv1.ClusterExtensionRevisionSpec{
637+
LifecycleState: ocv1.ClusterExtensionRevisionLifecycleStateArchived,
638+
},
639+
},
640+
&ocv1.ClusterExtensionRevision{
641+
ObjectMeta: metav1.ObjectMeta{
642+
Name: "rev-6",
643+
Labels: map[string]string{
644+
controllers.ClusterExtensionRevisionOwnerLabel: ext.Name,
645+
},
646+
},
647+
Spec: ocv1.ClusterExtensionRevisionSpec{
648+
LifecycleState: ocv1.ClusterExtensionRevisionLifecycleStateArchived,
649+
},
650+
},
651+
},
652+
validate: func(t *testing.T, c client.Client) {
653+
rev1 := &ocv1.ClusterExtensionRevision{}
654+
err := c.Get(t.Context(), client.ObjectKey{Name: "rev-1"}, rev1)
655+
require.Error(t, err)
656+
assert.True(t, apierrors.IsNotFound(err))
657+
658+
latest := &ocv1.ClusterExtensionRevision{}
659+
err = c.Get(t.Context(), client.ObjectKey{Name: "test-ext-1"}, latest)
660+
require.NoError(t, err)
661+
assert.Len(t, latest.Spec.Previous, applier.ClusterExtensionRevisionPreviousLimit)
662+
},
663+
},
664+
{
665+
name: "len([]revisions) > limit but contains active revisions with index beyond limit",
666+
mockBuilder: &mockBundleRevisionBuilder{
667+
makeRevisionFunc: func(bundleFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionAnnotations map[string]string) (*ocv1.ClusterExtensionRevision, error) {
668+
return &ocv1.ClusterExtensionRevision{
669+
ObjectMeta: metav1.ObjectMeta{
670+
Annotations: revisionAnnotations,
671+
Labels: map[string]string{
672+
controllers.ClusterExtensionRevisionOwnerLabel: ext.Name,
673+
},
674+
},
675+
Spec: ocv1.ClusterExtensionRevisionSpec{},
676+
}, nil
677+
},
678+
},
679+
existingObjs: []client.Object{
680+
&ocv1.ClusterExtensionRevision{
681+
ObjectMeta: metav1.ObjectMeta{
682+
Name: "rev-1",
683+
Labels: map[string]string{
684+
controllers.ClusterExtensionRevisionOwnerLabel: ext.Name,
685+
},
686+
},
687+
Spec: ocv1.ClusterExtensionRevisionSpec{
688+
LifecycleState: ocv1.ClusterExtensionRevisionLifecycleStateArchived,
689+
},
690+
},
691+
&ocv1.ClusterExtensionRevision{
692+
ObjectMeta: metav1.ObjectMeta{
693+
Name: "rev-2",
694+
Labels: map[string]string{
695+
controllers.ClusterExtensionRevisionOwnerLabel: ext.Name,
696+
},
697+
},
698+
Spec: ocv1.ClusterExtensionRevisionSpec{
699+
// index beyond the retention limit but active; should be preserved
700+
LifecycleState: ocv1.ClusterExtensionRevisionLifecycleStateActive,
701+
},
702+
},
703+
&ocv1.ClusterExtensionRevision{
704+
ObjectMeta: metav1.ObjectMeta{
705+
Name: "rev-3",
706+
Labels: map[string]string{
707+
controllers.ClusterExtensionRevisionOwnerLabel: ext.Name,
708+
},
709+
},
710+
Spec: ocv1.ClusterExtensionRevisionSpec{
711+
LifecycleState: ocv1.ClusterExtensionRevisionLifecycleStateActive,
712+
},
713+
},
714+
&ocv1.ClusterExtensionRevision{
715+
ObjectMeta: metav1.ObjectMeta{
716+
Name: "rev-4",
717+
Labels: map[string]string{
718+
controllers.ClusterExtensionRevisionOwnerLabel: ext.Name,
719+
},
720+
},
721+
Spec: ocv1.ClusterExtensionRevisionSpec{
722+
// archived but should be preserved since it is within the limit
723+
LifecycleState: ocv1.ClusterExtensionRevisionLifecycleStateArchived,
724+
},
725+
},
726+
&ocv1.ClusterExtensionRevision{
727+
ObjectMeta: metav1.ObjectMeta{
728+
Name: "rev-5",
729+
Labels: map[string]string{
730+
controllers.ClusterExtensionRevisionOwnerLabel: ext.Name,
731+
},
732+
},
733+
Spec: ocv1.ClusterExtensionRevisionSpec{
734+
LifecycleState: ocv1.ClusterExtensionRevisionLifecycleStateActive,
735+
},
736+
},
737+
&ocv1.ClusterExtensionRevision{
738+
ObjectMeta: metav1.ObjectMeta{
739+
Name: "rev-6",
740+
Labels: map[string]string{
741+
controllers.ClusterExtensionRevisionOwnerLabel: ext.Name,
742+
},
743+
},
744+
Spec: ocv1.ClusterExtensionRevisionSpec{
745+
LifecycleState: ocv1.ClusterExtensionRevisionLifecycleStateActive,
746+
},
747+
},
748+
&ocv1.ClusterExtensionRevision{
749+
ObjectMeta: metav1.ObjectMeta{
750+
Name: "rev-7",
751+
Labels: map[string]string{
752+
controllers.ClusterExtensionRevisionOwnerLabel: ext.Name,
753+
},
754+
},
755+
Spec: ocv1.ClusterExtensionRevisionSpec{
756+
LifecycleState: ocv1.ClusterExtensionRevisionLifecycleStateActive,
757+
},
758+
},
759+
},
760+
validate: func(t *testing.T, c client.Client) {
761+
rev1 := &ocv1.ClusterExtensionRevision{}
762+
err := c.Get(t.Context(), client.ObjectKey{Name: "rev-1"}, rev1)
763+
require.Error(t, err)
764+
assert.True(t, apierrors.IsNotFound(err))
765+
766+
rev2 := &ocv1.ClusterExtensionRevision{}
767+
err = c.Get(t.Context(), client.ObjectKey{Name: "rev-2"}, rev2)
768+
require.NoError(t, err)
769+
770+
rev4 := &ocv1.ClusterExtensionRevision{}
771+
err = c.Get(t.Context(), client.ObjectKey{Name: "rev-4"}, rev4)
772+
require.NoError(t, err)
773+
774+
latest := &ocv1.ClusterExtensionRevision{}
775+
err = c.Get(t.Context(), client.ObjectKey{Name: "test-ext-1"}, latest)
776+
require.NoError(t, err)
777+
assert.Len(t, latest.Spec.Previous, 6)
778+
assert.Contains(t, latest.Spec.Previous, ocv1.ClusterExtensionRevisionPrevious{Name: "rev-4"})
779+
},
569780
},
570781
}
571782

0 commit comments

Comments
 (0)