Skip to content

Commit 8c42457

Browse files
dtfranzdtfranz
authored
CRE Previous Limit (#2204)
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 f512e1e commit 8c42457

File tree

2 files changed

+243
-8
lines changed

2 files changed

+243
-8
lines changed

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

Lines changed: 30 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,30 @@ 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+
// NOTE: revisionList must be sorted in chronographical order, from oldest to latest.
323+
func (bc *Boxcutter) setPreviousRevisions(ctx context.Context, latestRevision *ocv1.ClusterExtensionRevision, revisionList []ocv1.ClusterExtensionRevision) error {
324+
trimmedPrevious := make([]ocv1.ClusterExtensionRevisionPrevious, 0)
325+
for index, r := range revisionList {
326+
if index < len(revisionList)-ClusterExtensionRevisionPreviousLimit && r.Spec.LifecycleState == ocv1.ClusterExtensionRevisionLifecycleStateArchived {
327+
// Delete oldest CREs from the cluster and list to reach ClusterExtensionRevisionPreviousLimit or latest active revision
328+
if err := bc.Client.Delete(ctx, &ocv1.ClusterExtensionRevision{
329+
ObjectMeta: metav1.ObjectMeta{
330+
Name: r.Name,
331+
},
332+
}); err != nil && !apierrors.IsNotFound(err) {
333+
return fmt.Errorf("deleting previous archived Revision: %w", err)
334+
}
335+
} else {
336+
// All revisions within the limit or still active are preserved
337+
trimmedPrevious = append(trimmedPrevious, ocv1.ClusterExtensionRevisionPrevious{Name: r.Name, UID: r.GetUID()})
338+
}
339+
}
340+
latestRevision.Spec.Previous = trimmedPrevious
341+
return nil
342+
}
343+
322344
// getExistingRevisions returns the list of ClusterExtensionRevisions for a ClusterExtension with name extName in revision order (oldest to newest)
323345
func (bc *Boxcutter) getExistingRevisions(ctx context.Context, extName string) ([]ocv1.ClusterExtensionRevision, error) {
324346
existingRevisionList := &ocv1.ClusterExtensionRevisionList{}

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

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

572785
for _, tc := range testCases {

0 commit comments

Comments
 (0)