fix(installplans): GC older installplans

Author: ecordell

pkg/controller/operators/catalog/operator.go

@@ -6,6 +6,7 @@ import (
 	"errors"
 	"fmt"
 	"reflect"
+	"sort"
 	"strings"
 	"sync"
 	"time"
@@ -22,6 +23,7 @@ import (
 	extinf "k8s.io/apiextensions-apiserver/pkg/client/informers/externalversions"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	utilclock "k8s.io/apimachinery/pkg/util/clock"
 	utilerrors "k8s.io/apimachinery/pkg/util/errors"
@@ -67,6 +69,7 @@ const (
 	roleKind               = "Role"
 	roleBindingKind        = "RoleBinding"
 	generatedByKey         = "olm.generated-by"
+	maxInstallPlanCount    = 5
 )
 
 // Operator represents a Kubernetes operator that executes InstallPlans by
@@ -1180,6 +1183,64 @@ func (o *Operator) unpackBundles(plan *v1alpha1.InstallPlan) (bool, *v1alpha1.In
 	return unpacked, out, nil
 }
 
+func (o *Operator) gcInstallPlans(log logrus.FieldLogger, namespace string) {
+	ips, err := o.lister.OperatorsV1alpha1().InstallPlanLister().InstallPlans(namespace).List(labels.Everything())
+	if err != nil {
+		log.Warn("unable to list installplans for GC")
+	}
+
+	if len(ips) <= maxInstallPlanCount {
+		return
+	}
+
+	byGen := map[int][]*v1alpha1.InstallPlan{}
+	for _, ip := range ips {
+		gen, ok := byGen[ip.Spec.Generation]
+		if !ok {
+			gen = make([]*v1alpha1.InstallPlan, 0)
+		}
+		byGen[ip.Spec.Generation] = append(gen, ip)
+	}
+
+	gens := make([]int, 0)
+	for i := range byGen {
+		gens = append(gens, i)
+	}
+
+	sort.Ints(gens)
+
+	toDelete := make([]*v1alpha1.InstallPlan, 0)
+
+	for _, i := range gens {
+		g := byGen[i]
+
+		if len(ips)-len(toDelete) <= maxInstallPlanCount {
+			break
+		}
+
+		// if removing all installplans at this generation doesn't dip below the max, safe to delete all of them
+		if len(ips)-len(toDelete)-len(g) >= maxInstallPlanCount {
+			toDelete = append(toDelete, g...)
+			continue
+		}
+
+		// CreationTimestamp sorting shouldn't ever be hit unless there is a bug that causes installplans to be
+		// generated without bumping the generation. It is here as a safeguard only.
+
+		// sort by creation time
+		sort.Slice(g, func(i, j int) bool {
+			return g[i].CreationTimestamp.Before(&g[j].CreationTimestamp)
+		})
+		toDelete = append(toDelete, g[:len(ips)-len(toDelete)-maxInstallPlanCount]...)
+	}
+
+	for _, i := range toDelete {
+		if err := o.client.OperatorsV1alpha1().InstallPlans(namespace).Delete(context.TODO(), i.GetName(), metav1.DeleteOptions{}); err != nil {
+			log.WithField("deleting", i.GetName()).WithError(err).Warn("error GCing old installplan")
+		}
+	}
+}
+
 func (o *Operator) syncInstallPlans(obj interface{}) (syncError error) {
 	plan, ok := obj.(*v1alpha1.InstallPlan)
 	if !ok {
@@ -1196,6 +1257,8 @@ func (o *Operator) syncInstallPlans(obj interface{}) (syncError error) {
 
 	logger.Info("syncing")
 
+	o.gcInstallPlans(logger, plan.GetNamespace())
+
 	if len(plan.Status.Plan) == 0 && len(plan.Status.BundleLookups) == 0 {
 		logger.Info("skip processing installplan without status - subscription sync responsible for initial status")
 		return

pkg/controller/operators/catalog/operator_test.go

@@ -6,8 +6,11 @@ import (
 	"errors"
 	"fmt"
 	"io/ioutil"
+	"math/rand"
+	"reflect"
 	"strings"
 	"testing"
+	"testing/quick"
 	"time"
 
 	"github.com/sirupsen/logrus"
@@ -28,6 +31,7 @@ import (
 	"k8s.io/apimachinery/pkg/types"
 	utilclock "k8s.io/apimachinery/pkg/util/clock"
 	utilyaml "k8s.io/apimachinery/pkg/util/yaml"
+	"k8s.io/apiserver/pkg/storage/names"
 	fakedynamic "k8s.io/client-go/dynamic/fake"
 	"k8s.io/client-go/informers"
 	k8sfake "k8s.io/client-go/kubernetes/fake"
@@ -168,7 +172,7 @@ func TestSyncInstallPlanUnhappy(t *testing.T) {
 					},
 				},
 			),
-			err:  fmt.Errorf("attenuated service account query failed - no operator group found that is managing this namespace"),
+			err: fmt.Errorf("attenuated service account query failed - no operator group found that is managing this namespace"),
 		},
 	}
 
@@ -186,6 +190,70 @@ func TestSyncInstallPlanUnhappy(t *testing.T) {
 	}
 }
 
+type ipSet []runtime.Object
+
+func (ipSet) Generate(rand *rand.Rand, size int) reflect.Value {
+	ips := []runtime.Object{}
+
+	// each i is the generation value
+	for i := 0; i < rand.Intn(size)+1; i++ {
+
+		// generate a few at each generation to account for bugs that don't increment the generation
+		for j := 0; j < rand.Intn(3); j++ {
+			ips = append(ips, &v1alpha1.InstallPlan{
+				ObjectMeta: metav1.ObjectMeta{
+					Namespace: "ns",
+					Name:      names.SimpleNameGenerator.GenerateName(fmt.Sprintf("%d", i)),
+				},
+				Spec: v1alpha1.InstallPlanSpec{
+					Generation: i,
+				},
+			})
+		}
+	}
+	return reflect.ValueOf(ipSet(ips))
+}
+
+func TestGC(t *testing.T) {
+	f := func(ips ipSet) bool {
+		if len(ips) == 0 {
+			return true
+		}
+		ctx, cancel := context.WithCancel(context.TODO())
+		defer cancel()
+
+		var maxGen int64 = 0
+		for _, i := range ips {
+			if g := i.(*v1alpha1.InstallPlan).Generation; g > maxGen {
+				maxGen = g
+			}
+		}
+		op, err := NewFakeOperator(ctx, "ns", []string{"ns"}, withClientObjs([]runtime.Object(ips)...))
+		require.NoError(t, err)
+
+		err = op.syncInstallPlans(ips[0])
+		require.NoError(t, err)
+
+		out, err := op.client.OperatorsV1alpha1().InstallPlans("ns").List(ctx, metav1.ListOptions{})
+		require.NoError(t, err)
+
+		keptMax := false
+		for _, o := range out.Items {
+			if o.Generation == maxGen {
+				keptMax = true
+				break
+			}
+		}
+		require.True(t, keptMax)
+
+		if len(ips) < maxInstallPlanCount {
+			return len(out.Items) == len(ips)
+		}
+		return len(out.Items) == maxInstallPlanCount
+	}
+	require.NoError(t, quick.Check(f, nil))
+}
+
 func TestExecutePlan(t *testing.T) {
 	namespace := "ns"