Repository: operator-framework/operator-lifecycle-manager
Pull request #1669 (2020-07-22 17:27:48)

Bug 1859178: 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:**
This adds a GC check in the installplan controller.

If it finds more than 5 installplans for a namespace, it will delete the oldest ones. Age is determined first by `Generation` and second via `CreationTimestamp` (to account for any bugs that result in installplans with the same generation, which should never happen).

**Motivation for the change:**
It was possible in the current resolver to construct a catalog that would cause installplans to be created endlessly. This change protects against potential problems like this.

The new resolver will begin to output installplans even on failed resolution attempts, so this limit will support that feature as well.

**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: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/operator-framework/operator-lifecycle-manager/pull/1669#" 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/master/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: This pull request references [Bugzilla bug 1859178](https://bugzilla.redhat.com/show_bug.cgi?id=1859178), which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

<details><summary>3 validation(s) were run on this bug</summary>

* bug is open, matching expected state (open)
* bug target release (4.6.0) matches configured target release for branch (4.6.0)
* bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)</details>

<details>

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

>Bug 1859178: 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>
kevinrizza: /lgtm
ecordell: /hold

I want to perform some additional testing and consider comments here before this merges
openshift-ci-robot: New changes are detected. LGTM label has been removed.
openshift-ci-robot: @ecordell: The following test **failed**, say `/retest` to rerun all failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
ci/prow/e2e-aws-olm | a43ed755543a1de71f8163fb048cec3d12ee9270 | [link](https://prow.ci.openshift.org/view/gs/origin-ci-test/pr-logs/pull/operator-framework_operator-lifecycle-manager/1669/pull-ci-operator-framework-operator-lifecycle-manager-master-e2e-aws-olm/1286022803711070208) | `/test e2e-aws-olm`

[Full PR test history](https://prow.ci.openshift.org/pr-history?org=operator-framework&repo=operator-lifecycle-manager&pr=1669). [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: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/operator-framework/operator-lifecycle-manager/pull/1669#" title="Author self-approved">ecordell</a>*, *<a href="https://github.com/operator-framework/operator-lifecycle-manager/pull/1669#pullrequestreview-453637522" title="LGTM">njhale</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/master/OWNERS)~~ [ecordell,njhale]

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":[]} -->
ecordell: /hold cancel

tested in a 4.4 cluster with lots of installplans generated, and this kept them down to the five latest.
openshift-ci-robot: @ecordell: The following tests **failed**, say `/retest` to rerun all failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
ci/prow/e2e-aws-olm | a43ed755543a1de71f8163fb048cec3d12ee9270 | [link](https://prow.ci.openshift.org/view/gs/origin-ci-test/pr-logs/pull/operator-framework_operator-lifecycle-manager/1669/pull-ci-operator-framework-operator-lifecycle-manager-master-e2e-aws-olm/1286022803711070208) | `/test e2e-aws-olm`
ci/prow/e2e-aws-console-olm | a43ed755543a1de71f8163fb048cec3d12ee9270 | [link](https://prow.ci.openshift.org/view/gs/origin-ci-test/pr-logs/pull/operator-framework_operator-lifecycle-manager/1669/pull-ci-operator-framework-operator-lifecycle-manager-master-e2e-aws-console-olm/1286022789521739776) | `/test e2e-aws-console-olm`

[Full PR test history](https://prow.ci.openshift.org/pr-history?org=operator-framework&repo=operator-lifecycle-manager&pr=1669). [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 -->
ecordell: /retest
openshift-ci-robot: @ecordell: The following tests **failed**, say `/retest` to rerun all failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
ci/prow/e2e-aws-olm | a43ed755543a1de71f8163fb048cec3d12ee9270 | [link](https://prow.ci.openshift.org/view/gs/origin-ci-test/pr-logs/pull/operator-framework_operator-lifecycle-manager/1669/pull-ci-operator-framework-operator-lifecycle-manager-master-e2e-aws-olm/1286022803711070208) | `/test e2e-aws-olm`
ci/prow/e2e-aws-console-olm | a43ed755543a1de71f8163fb048cec3d12ee9270 | [link](https://prow.ci.openshift.org/view/gs/origin-ci-test/pr-logs/pull/operator-framework_operator-lifecycle-manager/1669/pull-ci-operator-framework-operator-lifecycle-manager-master-e2e-aws-console-olm/1286022789521739776) | `/test e2e-aws-console-olm`
ci/prow/e2e-gcp-upgrade | a43ed755543a1de71f8163fb048cec3d12ee9270 | [link](https://prow.ci.openshift.org/view/gs/origin-ci-test/pr-logs/pull/operator-framework_operator-lifecycle-manager/1669/pull-ci-operator-framework-operator-lifecycle-manager-master-e2e-gcp-upgrade/1286022768650883072) | `/test e2e-gcp-upgrade`

[Full PR test history](https://prow.ci.openshift.org/pr-history?org=operator-framework&repo=operator-lifecycle-manager&pr=1669). [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-bot: /retest

Please review the [full test history](https://prow.ci.openshift.org/pr-history/?org=operator-framework&repo=operator-lifecycle-manager&pr=1669) for this PR and help us cut down flakes.
openshift-ci-robot: @ecordell: The following tests **failed**, say `/retest` to rerun all failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
ci/prow/e2e-gcp-upgrade | a43ed755543a1de71f8163fb048cec3d12ee9270 | [link](https://prow.ci.openshift.org/view/gs/origin-ci-test/pr-logs/pull/operator-framework_operator-lifecycle-manager/1669/pull-ci-operator-framework-operator-lifecycle-manager-master-e2e-gcp-upgrade/1286022768650883072) | `/test e2e-gcp-upgrade`
ci/prow/e2e-aws-olm | a43ed755543a1de71f8163fb048cec3d12ee9270 | [link](https://prow.ci.openshift.org/view/gs/origin-ci-test/pr-logs/pull/operator-framework_operator-lifecycle-manager/1669/pull-ci-operator-framework-operator-lifecycle-manager-master-e2e-aws-olm/1286039106891026432) | `/test e2e-aws-olm`

[Full PR test history](https://prow.ci.openshift.org/pr-history?org=operator-framework&repo=operator-lifecycle-manager&pr=1669). [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-bot: /retest

Please review the [full test history](https://prow.ci.openshift.org/pr-history/?org=operator-framework&repo=operator-lifecycle-manager&pr=1669) for this PR and help us cut down flakes.

Patch:
diff --git a/.github/workflows/flake-analyzer-periodics.yml b/.github/workflows/flake-analyzer-periodics.yml
deleted file mode 100644
index daeb83bd9..000000000
--- a/.github/workflows/flake-analyzer-periodics.yml
+++ /dev/null
@@ -1,25 +0,0 @@
-name: flake-analyzer-periodics
-on:
-  schedule:
-    - cron: '0 1 * * *'
-jobs:
-  generate-flake-analysis-report:
-    runs-on: ubuntu-latest
-    steps:
-      - name: Periodic Flake Report
-        env:
-          OWNER: operator-framework
-          REPO: operator-lifecycle-manager
-          TEST_SUITE: e2e-test-output
-          TOKEN: ${{ secrets.GITHUB_TOKEN }}
-        run: |
-          git clone -b v0.1.1 https://github.com/operator-framework/flake-analyzer.git
-          cd ./flake-analyzer
-          make report-today  OUTPUT_FILE=./report/artifacts/flake-report-today-$(date +"%m-%d-%Y").yaml
-          make report-last-7-days OUTPUT_FILE=./report/artifacts/flake-report-last-7-days-$(date +"%m-%d-%Y").yaml
-          make report-prev-7-days OUTPUT_FILE=./report/artifacts/flake-report-prev-7-days-$(date +"%m-%d-%Y").yaml
-      - name: Archive Reoport artifacts # test results are only uploaded if any of the e2e tests fails
-        uses: actions/upload-artifact@v2
-        with:
-          name: flake-report-${{ github.run_id }}
-          path: ${{ github.workspace }}/flake-analyzer/report/artifacts/*
diff --git a/pkg/controller/operators/catalog/operator.go b/pkg/controller/operators/catalog/operator.go
index 3b357b3f7..f57969e8f 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/.github/workflows/e2e-tests.yml b/.github/workflows/e2e-tests.yml
index d7e33eeff..f187fc793 100644
--- a/.github/workflows/e2e-tests.yml
+++ b/.github/workflows/e2e-tests.yml
@@ -10,7 +10,7 @@ jobs:
     steps:
       - uses: actions/checkout@v2
       - run: make -f x.mk e2e-local NODES=2 JUNIT_DIRECTORY=./artifacts/
-      - name: Archive Test Artifacts # test results are only uploaded if any of the e2e tests fails
+      - name: Archive production artifacts # test results are only uploaded if any of the e2e tests fails
         if: ${{ failure() }}
         uses: actions/upload-artifact@v2
         with:
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"