OperatorGroup expansion/contraction #736

ecordell · 2019-03-02T21:22:07Z

This takes the commits from #675, and rebases them one at a time to get them working with newer changes to OperatorGroups.

I'd like to double check / write a test for any possible edge cases here. The one that comes to mind is: the same CSV supports AllNamespaces and SingleNamespace, and we accidentally overwrite a "real" CSV with a "copied" CSV. It may be better to go ahead with these and follow up with that test case / fix, though, given that this is blocking.

ecordell · 2019-03-04T01:13:40Z

/retest

ecordell · 2019-03-04T11:55:15Z

/retest

ecordell · 2019-03-04T14:10:48Z

/test unit

njhale

I found some things that may cause problems. I also found some things that we may want to look at in follow up PRs. Here are my thoughts:

pkg/controller/operators/olm/operator.go

pkg/controller/operators/olm/operatorgroup.go

test/e2e/operator_groups_e2e_test.go

njhale · 2019-03-04T15:45:45Z

test/e2e/operator_groups_e2e_test.go

+
+	t.Log("Waiting to ensure copied CSV shows up in other namespace")
+	err = wait.Poll(pollInterval, pollDuration, func() (bool, error) {


You can use awaitCSV(...) which will wait until the CSV exists and then apply the given status checker func.

test/e2e/operator_groups_e2e_test.go

njhale · 2019-03-04T15:51:51Z

test/e2e/operator_groups_e2e_test.go

+
+	// verify created CSV is cleaned up after operator group is "contracted"
+	t.Log("Modifying operator group to no longer watch all namespaces")


I wonder if we should just generate a new OperatorGroup for this test so we avoid modifications to the initial setup.

test/e2e/util_test.go

pkg/controller/operators/olm/operatorgroup.go

pkg/controller/operators/olm/operator.go

jpeeler · 2019-03-04T21:26:01Z

pkg/controller/operators/olm/operator_test.go

@@ -3578,7 +3578,7 @@ func TestSyncOperatorGroups(t *testing.T) {

 			operatorGroup, err := op.GetClient().OperatorsV1alpha2().OperatorGroups(tt.initial.operatorGroup.GetNamespace()).Get(tt.initial.operatorGroup.GetName(), metav1.GetOptions{})
 			require.NoError(t, err)
-			assert.Equal(t, tt.expectedStatus, operatorGroup.Status)
+			assert.EqualValues(t, tt.expectedStatus, operatorGroup.Status)


I know this is annoying to fix, but doesn't this mask the difference between "" vs nil?

Doesn't look like it? https://github.com/stretchr/testify/blob/master/assert/assertions.go#L78:6

ecordell · 2019-03-04T21:32:27Z

/retest

ecordell · 2019-03-04T22:29:15Z

/retest

ecordell · 2019-03-04T22:48:36Z

/retest

ecordell · 2019-03-05T00:37:19Z

/test unit

pkg/controller/operators/olm/operatorgroup.go

njhale · 2019-03-05T02:37:33Z

pkg/controller/operators/olm/operatorgroup.go

+
+	errlist := []error{}
+	for _, csv := range fetchedCSVs {
+		if csv.IsCopied() && csv.GetAnnotations()[v1alpha2.OperatorGroupAnnotationKey] == operatorGroupName {


I think for the sake of completeness we should check the olm.operatorNamespace annotation as well.

njhale · 2019-03-05T03:01:49Z

/retest

pkg/controller/operators/olm/operator.go

can output an allnamespace operatorgroup

This adds requeuing for both the namespace and operator group sync loops, which fixes syncing issues after initial CSVs have been copied.

After switching to owner labels for cross-namespace ownerrefs, we didn't update this requeueing logic, causing some delays for reconciliation of those objects.

this adds annotations to all csvs in an operator group, and copies them to target namespaces.

copyable states

makes use of more of the new namespaceset utilities from the resolver also doesn't bother precomputing the namespaces to add/prune. instead it gets all namespaces and adjusts each as needed, visiting each once.

processing

We were treating CSVs that had empty phases as "copyable", but that happens before the checks to see if operatorgroups conflict. This was causing a race where CSVs could be copied to target namespaces incorrectly

are available this avoids some unnecessary resyncing

expansion/contraction

ecordell · 2019-03-10T20:31:53Z

pkg/controller/operators/olm/operator.go

@@ -1255,3 +1321,51 @@ func (a *Operator) cleanupCSVDeployments(logger *logrus.Entry, csv *v1alpha1.Clu
 		}
 	}
 }
+
+func (a *Operator) ensureDeploymentAnnotations(logger *logrus.Entry, csv *v1alpha1.ClusterServiceVersion) error {


Added this while looking into why copying was failing - small optimization that skips the walk through Failed if the annotations change in an operatorgroup.

ecordell · 2019-03-10T20:32:23Z

pkg/controller/operators/olm/operator_test.go

@@ -2067,6 +2085,51 @@ func TestTransitionCSV(t *testing.T) {
 				},
 			},
 		},
+		{
+			name: "SingleCSVSucceededToSucceeded/OperatorGroupChanged",


Verifying we don't have to transition to failed to update the operatorgroup

ecordell · 2019-03-10T20:34:38Z

pkg/controller/operators/olm/operatorgroup.go

+			}
+		}
+
+		// requeue csvs in original namespaces or in new target namespaces (to capture removed/added namespaces)


this ended up being the reason contraction wasn't working after the rebase. somewhere in reconciling those commits we lost the code that requeues CSVs from the old set of namespaces, which is important to make sure we pick up CSVs that need to be cleaned up when an operatorgroup is contracted.

Added some abstractions around namespacesets to help make this clearer

Isn't this effectively done around line 110?

Line 110 doesn't capture the "contraction" case, right?

If you remove the the "not copied" predicate in getAllCSVsThatProvide(...), then I think it would since that returns the set of CSVs across all namespaces that provide the given APIs. As I recall, the input it's given in syncOperatorGroup represents the union of APIs in the providedAPIs annotation and the CSVs in the OperatorGroup's namespace.

njhale

/lgtm

openshift-ci-robot · 2019-03-11T14:08:09Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ecordell, njhale

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~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

ecordell requested review from jpeeler and njhale March 2, 2019 21:22

openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Mar 2, 2019

ecordell force-pushed the opgroup-expand-contract branch from 3bfff0a to 10dc2c2 Compare March 2, 2019 21:27

njhale reviewed Mar 4, 2019

View reviewed changes

pkg/controller/operators/olm/operatorgroup.go Outdated Show resolved Hide resolved

njhale reviewed Mar 4, 2019

View reviewed changes

pkg/controller/operators/olm/operator.go Outdated Show resolved Hide resolved

njhale reviewed Mar 4, 2019

View reviewed changes

pkg/controller/operators/olm/operator.go Outdated Show resolved Hide resolved

sferich888 mentioned this pull request Mar 4, 2019

fix resource creation/deletion after operator group config change #675

Closed

jpeeler reviewed Mar 4, 2019

View reviewed changes

njhale reviewed Mar 5, 2019

View reviewed changes

pkg/controller/operators/olm/operatorgroup.go Show resolved Hide resolved

njhale reviewed Mar 5, 2019

View reviewed changes

openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Mar 5, 2019

njhale reviewed Mar 5, 2019

View reviewed changes

pkg/controller/operators/olm/operator.go Outdated Show resolved Hide resolved

ecordell force-pushed the opgroup-expand-contract branch 6 times, most recently from 883f402 to be33ddf Compare March 5, 2019 23:39

ecordell and others added 19 commits March 10, 2019 15:27

chore(opgroup): allow selector to be nullable so that marshaller

78750fe

can output an allnamespace operatorgroup

fix(olm): change operator group / CSV requeuing

30c553d

This adds requeuing for both the namespace and operator group sync loops, which fixes syncing issues after initial CSVs have been copied.

fix(requeue): requeue owners that are referenced with labels

b071288

After switching to owner labels for cross-namespace ownerrefs, we didn't update this requeueing logic, causing some delays for reconciliation of those objects.

fix(opgroup): revert order of operatorgroup updates

ae87c3b

fix(olm): delete stale resources after op group change

261b5fa

fix(operatorgroup): annotate all csvs and allow collisions

28248b3

this adds annotations to all csvs in an operator group, and copies them to target namespaces.

fix(olm): delete copied child csvs with no parent annotations

e09e6cc

feat(olm): don't copy csv to operatorgroups if it's not in certain

8ae47b0

copyable states

feat(namespaceSet): add Contains method to namespace set

2f5074f

refactor(olm): simplify CSV copying/pruning

304d944

makes use of more of the new namespaceset utilities from the resolver also doesn't bother precomputing the namespaces to add/prune. instead it gets all namespaces and adjusts each as needed, visiting each once.

refactor(olm): refactor provided api pruning

ba58953

fix(e2e): fix e2e tests for operatorgroups

b7594dd

fix(olm): move csv copying into its own workqueue to free up csv

37bccd5

processing

fix(operatorgroup): set labels on copied resources correctly

58f3e62

fix(opgroup): incorrectly detecting CSVs as "copyable"

a651111

We were treating CSVs that had empty phases as "copyable", but that happens before the checks to see if operatorgroups conflict. This was causing a race where CSVs could be copied to target namespaces incorrectly

chore(rebase): fix errors from rebasing

69fe266

fix(e2e): fixing e2e test after rebase

fb90516

feat(olm): update deployment annotations as soon as the csv annotations

cc7265f

are available this avoids some unnecessary resyncing

fix(contraction): requeue any csv that may need it after operatorgroup

121243a

expansion/contraction

ecordell force-pushed the opgroup-expand-contract branch from 2a6bd65 to 121243a Compare March 10, 2019 19:28

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 10, 2019

ecordell commented Mar 10, 2019

View reviewed changes

njhale approved these changes Mar 11, 2019

View reviewed changes

openshift-ci-robot assigned njhale Mar 11, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 11, 2019

openshift-merge-robot merged commit 25a917e into operator-framework:master Mar 11, 2019

ecordell added the kind/feature Categorizes issue or PR as related to a new feature. label Mar 19, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OperatorGroup expansion/contraction #736

OperatorGroup expansion/contraction #736

ecordell commented Mar 2, 2019

ecordell commented Mar 4, 2019

ecordell commented Mar 4, 2019

ecordell commented Mar 4, 2019

njhale left a comment

njhale Mar 4, 2019

njhale Mar 4, 2019

jpeeler Mar 4, 2019

ecordell Mar 4, 2019

ecordell commented Mar 4, 2019

ecordell commented Mar 4, 2019

ecordell commented Mar 4, 2019

ecordell commented Mar 5, 2019

njhale Mar 5, 2019

njhale commented Mar 5, 2019

ecordell Mar 10, 2019

ecordell Mar 10, 2019

ecordell Mar 10, 2019

njhale Mar 11, 2019

ecordell Mar 11, 2019

njhale Mar 11, 2019

njhale left a comment

openshift-ci-robot commented Mar 11, 2019


		t.Log("Waiting to ensure copied CSV shows up in other namespace")
		err = wait.Poll(pollInterval, pollDuration, func() (bool, error) {


		// verify created CSV is cleaned up after operator group is "contracted"
		t.Log("Modifying operator group to no longer watch all namespaces")

OperatorGroup expansion/contraction #736

OperatorGroup expansion/contraction #736

Conversation

ecordell commented Mar 2, 2019

ecordell commented Mar 4, 2019

ecordell commented Mar 4, 2019

ecordell commented Mar 4, 2019

njhale left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ecordell commented Mar 4, 2019

ecordell commented Mar 4, 2019

ecordell commented Mar 4, 2019

ecordell commented Mar 5, 2019

Choose a reason for hiding this comment

njhale commented Mar 5, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

njhale left a comment

Choose a reason for hiding this comment

openshift-ci-robot commented Mar 11, 2019