Repository: operator-framework/operator-lifecycle-manager
Pull request #1674 (2020-07-22 21:12:45)

Bug 1859753: fix(installplans): GC older installplans
<!--

Before making a PR, please read our contributing guidelines https://github.com/operator-framework/operator-lifecycle-manager/blob/master/CONTRIBUTING.md

Note: Make sure your branch is rebased to the latest upstream master.

-->

**Description of the change:**


**Motivation for the change:**

**Reviewer Checklist**
- [ ] Implementation matches the proposed design, or proposal is updated to match implementation
- [ ] Sufficient unit test coverage 
- [ ] Sufficient end-to-end test coverage
- [ ] Docs updated or added to `/docs` 
- [ ] Commit messages sensible and descriptive


<!--

Note: If this PR is fixing an issue make sure to add a note saying:
Closes #<ISSUE_NUMBER>

-->


openshift-ci-robot: @ecordell: This pull request references [Bugzilla bug 1859753](https://bugzilla.redhat.com/show_bug.cgi?id=1859753), which is invalid:
 - expected dependent [Bugzilla bug 1859178](https://bugzilla.redhat.com/show_bug.cgi?id=1859178) to be in one of the following states: VERIFIED, RELEASE_PENDING, CLOSED (ERRATA), but it is POST instead

Comment <code>/bugzilla refresh</code> to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

<details>

In response to [this](https://github.com/operator-framework/operator-lifecycle-manager/pull/1674):

>Bug 1859753: fix(installplans): GC older installplans


Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
</details>
openshift-ci-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/operator-framework/operator-lifecycle-manager/pull/1674#" title="Author self-approved">ecordell</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=operator-framework%2Foperator-lifecycle-manager).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/operator-framework/operator-lifecycle-manager/blob/release-4.5/OWNERS)~~ [ecordell]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
openshift-ci-robot: @ecordell: The following test **failed**, say `/retest` to rerun all failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
ci/prow/unit | 81acc258a741bd375710505f87669420612765bb | [link](https://prow.ci.openshift.org/view/gs/origin-ci-test/pr-logs/pull/operator-framework_operator-lifecycle-manager/1674/pull-ci-operator-framework-operator-lifecycle-manager-release-4.5-unit/1286046562861977600) | `/test unit`

[Full PR test history](https://prow.ci.openshift.org/pr-history?org=operator-framework&repo=operator-lifecycle-manager&pr=1674). [Your PR dashboard](https://prow.ci.openshift.org/pr?query=is:pr+state:open+author:ecordell).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
njhale: checking to see if this unit test failure is consistent.

/test unit
openshift-ci-robot: @ecordell: The following test **failed**, say `/retest` to rerun all failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
ci/prow/unit | 81acc258a741bd375710505f87669420612765bb | [link](https://prow.ci.openshift.org/view/gs/origin-ci-test/pr-logs/pull/operator-framework_operator-lifecycle-manager/1674/pull-ci-operator-framework-operator-lifecycle-manager-release-4.5-unit/1286054706023501824) | `/test unit`

[Full PR test history](https://prow.ci.openshift.org/pr-history?org=operator-framework&repo=operator-lifecycle-manager&pr=1674). [Your PR dashboard](https://prow.ci.openshift.org/pr?query=is:pr+state:open+author:ecordell).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
openshift-ci-robot: @ecordell: The following tests **failed**, say `/retest` to rerun all failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
ci/prow/unit | 81acc258a741bd375710505f87669420612765bb | [link](https://prow.ci.openshift.org/view/gs/origin-ci-test/pr-logs/pull/operator-framework_operator-lifecycle-manager/1674/pull-ci-operator-framework-operator-lifecycle-manager-release-4.5-unit/1286054706023501824) | `/test unit`
ci/prow/e2e-aws-olm | 81acc258a741bd375710505f87669420612765bb | [link](https://prow.ci.openshift.org/view/gs/origin-ci-test/pr-logs/pull/operator-framework_operator-lifecycle-manager/1674/pull-ci-operator-framework-operator-lifecycle-manager-release-4.5-e2e-aws-olm/1286046562316718080) | `/test e2e-aws-olm`

[Full PR test history](https://prow.ci.openshift.org/pr-history?org=operator-framework&repo=operator-lifecycle-manager&pr=1674). [Your PR dashboard](https://prow.ci.openshift.org/pr?query=is:pr+state:open+author:ecordell).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->

Patch:
diff --git a/pkg/controller/operators/catalog/operator.go b/pkg/controller/operators/catalog/operator.go
index f57d27b58..94249fbdc 100644
--- a/pkg/controller/operators/catalog/operator.go
+++ b/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,8 @@ const (
 	roleKind               = "Role"
 	roleBindingKind        = "RoleBinding"
 	generatedByKey         = "olm.generated-by"
+	maxInstallPlanCount    = 5
+	maxDeletesPerSweep     = 5
 )
 
 // Operator represents a Kubernetes operator that executes InstallPlans by
@@ -777,6 +781,8 @@ func (o *Operator) syncResolvingNamespace(obj interface{}) error {
 		"id":        queueinformer.NewLoopID(),
 	})
 
+	o.gcInstallPlans(logger, namespace)
+
 	// get the set of sources that should be used for resolution and best-effort get their connections working
 	logger.Debug("resolving sources")
 
@@ -1180,6 +1186,77 @@ func (o *Operator) unpackBundles(plan *v1alpha1.InstallPlan) (bool, *v1alpha1.In
 	return unpacked, out, nil
 }
 
+// gcInstallPlans garbage collects installplans that are too old
+// installplans are ownerrefd to all subscription inputs, so they will not otherwise
+// be GCd unless all inputs have been deleted.
+func (o *Operator) gcInstallPlans(log logrus.FieldLogger, namespace string) {
+	allIps, err := o.lister.OperatorsV1alpha1().InstallPlanLister().InstallPlans(namespace).List(labels.Everything())
+	if err != nil {
+		log.Warn("unable to list installplans for GC")
+	}
+
+	if len(allIps) <= maxInstallPlanCount {
+		return
+	}
+
+	// we only consider maxDeletesPerSweep more than the allowed number of installplans for delete at one time
+	ips := allIps
+	if len(ips) > maxInstallPlanCount + maxDeletesPerSweep {
+		ips = allIps[:maxInstallPlanCount+maxDeletesPerSweep]
+	}
+
+	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 {
+			if !g[i].CreationTimestamp.Equal(&g[j].CreationTimestamp) {
+				return g[i].CreationTimestamp.Before(&g[j].CreationTimestamp)
+			}
+			// final fallback to lexicographic sort, in case many installplans are created with the same timestamp
+			return g[i].GetName() < g[j].GetName()
+		})
+		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 - may have already been deleted")
+		}
+	}
+}
+
 func (o *Operator) syncInstallPlans(obj interface{}) (syncError error) {
 	plan, ok := obj.(*v1alpha1.InstallPlan)
 	if !ok {


Test patch:
diff --git a/pkg/controller/operators/catalog/operator_test.go b/pkg/controller/operators/catalog/operator_test.go
index 7902572a7..0db22732a 100644
--- a/pkg/controller/operators/catalog/operator_test.go
+++ b/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,82 @@ func TestSyncInstallPlanUnhappy(t *testing.T) {
 	}
 }
 
+type ipSet []v1alpha1.InstallPlan
+
+func (ipSet) Generate(rand *rand.Rand, size int) reflect.Value {
+	ips := []v1alpha1.InstallPlan{}
+
+	// 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 TestGCInstallPlans(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.Generation; g > maxGen {
+				maxGen = g
+			}
+		}
+		objs := make([]runtime.Object, 0)
+		for _, i := range ips {
+			objs = append(objs, i.DeepCopy())
+		}
+		op, err := NewFakeOperator(ctx, "ns", []string{"ns"}, withClientObjs(objs...))
+		require.NoError(t, err)
+
+		out := make([]v1alpha1.InstallPlan, 0)
+		for {
+			op.gcInstallPlans(logrus.New(), "ns")
+			require.NoError(t, err)
+
+			outList, err := op.client.OperatorsV1alpha1().InstallPlans("ns").List(ctx, metav1.ListOptions{})
+			require.NoError(t, err)
+			out = outList.Items
+
+			if len(out) <= maxInstallPlanCount {
+				break
+			}
+		}
+
+		keptMax := false
+		for _, o := range out {
+			if o.Generation == maxGen {
+				keptMax = true
+				break
+			}
+		}
+		require.True(t, keptMax)
+
+		if len(ips) < maxInstallPlanCount {
+			return len(out) == len(ips)
+		}
+		return len(out) == maxInstallPlanCount
+	}
+	require.NoError(t, quick.Check(f, nil))
+}
+
 func TestExecutePlan(t *testing.T) {
 	namespace := "ns"