Refactor to avoid cache races #837

jpeeler · 2019-05-01T18:54:02Z

This is essentially a fix for unit tests that I noticed locally. It involves making sure a newly created CSV is not attempted to be retrieved using a lister within the same sync loop.

Obsoletes #814.

As much as I'd like to say this fixes every flake, this might only fix one of them. But I haven't seen any locally with this fix.

ecordell

I know you've been hard at work finding and fixing these flakes but I think we need to rethink how we're writing the tests or some different approach. I'm concerned that this will make our performance problems return and we'll start dealing with flakes in the e2e tests instead

ecordell · 2019-05-01T21:22:29Z

pkg/controller/operators/olm/operator.go

+	}
+	// Ensure operator has access to targetnamespaces with cluster RBAC
+	// (roles/rolebindings are checked for each target namespace in syncCopyCSV)
+	if err := a.ensureRBACInTargetNamespace(clusterServiceVersion, operatorGroup); err != nil {


I'm sure moving this here fixes the unit test flakes, but the reason we needed to move this into the syncCopyCSV in the first place was because of performance issues with large numbers of namespaces (well, not that large - 50 or so).

This method has been changed to only handle cluster roles and bindings. Do you still think this would be a problem?

Gotcha, thanks - I think that makes this fine :)

ecordell · 2019-05-01T21:24:01Z

pkg/controller/operators/olm/operatorgroup.go

+		a.Log.Errorf("operatorgroup '%v' should have non-nil status", operatorGroup.GetName())
+		return nil
+	}
+	for _, ns := range targetNamespaces {


Does moving this here fix anything in particular? Don't we call:

ensureCSVsInNamespaces

ensureTenantRBAC

so this change is basically folding them into one?

ensureTenantRBAC has been modified to not use the lister for retrieving the CSV, which is made possible by copyToNamespace returning the CSV.
Also, related to your other point, this keeps RBAC operations for anything other than all namespaces to stay in the syncCopyCSV loop.

jpeeler

I would definitely not want to put this code in before feature freeze, but I still think what I've written is ok. I might still be able to move ensureRBACInTargetNamespace back to syncCopyCSV as I was really just focused on the necessary coupling of ensureCSVsInNamespaces and ensureTenantRBAC (as you pointed out).

jpeeler · 2019-05-01T22:28:00Z

pkg/controller/operators/olm/operator.go

+	}
+	// Ensure operator has access to targetnamespaces with cluster RBAC
+	// (roles/rolebindings are checked for each target namespace in syncCopyCSV)
+	if err := a.ensureRBACInTargetNamespace(clusterServiceVersion, operatorGroup); err != nil {


This method has been changed to only handle cluster roles and bindings. Do you still think this would be a problem?

jpeeler · 2019-05-01T22:28:10Z

pkg/controller/operators/olm/operatorgroup.go

+		a.Log.Errorf("operatorgroup '%v' should have non-nil status", operatorGroup.GetName())
+		return nil
+	}
+	for _, ns := range targetNamespaces {


ensureTenantRBAC has been modified to not use the lister for retrieving the CSV, which is made possible by copyToNamespace returning the CSV.
Also, related to your other point, this keeps RBAC operations for anything other than all namespaces to stay in the syncCopyCSV loop.

jpeeler · 2019-05-01T22:39:51Z

/retest
I didn't see any e2e failures locally, but I'll be slightly concerned if they don't pass this time. (And maybe that speaks for itself...)

jpeeler · 2019-05-03T18:03:39Z

/retest
now that #838 has merged, e2e should pass.

jpeeler · 2019-05-03T20:57:15Z

/retest
Cluster didn't come up.

ecordell · 2019-05-09T14:32:32Z

Makefile

@@ -24,7 +25,7 @@ all: test build
 test: clean cover.out

 unit:
-	go test $(MOD_FLAGS) -v -race -tags=json1 -count=1 ./pkg/...
+	go test $(MOD_FLAGS) $(SPECIFIC_UNIT_TEST) -v -race -tags=json1 -count=1 ./pkg/...


Awesome, thanks!

ecordell · 2019-05-09T14:50:18Z

pkg/controller/operators/olm/operator.go

+	}
+	// Ensure operator has access to targetnamespaces with cluster RBAC
+	// (roles/rolebindings are checked for each target namespace in syncCopyCSV)
+	if err := a.ensureRBACInTargetNamespace(clusterServiceVersion, operatorGroup); err != nil {


Gotcha, thanks - I think that makes this fine :)

ecordell · 2019-05-09T14:51:28Z

@jpeeler this needs a rebase, and thanks for explaining these changes to me :)

I'd like to see the tests pass and then play with it in a real cluster just to check some of the performance characteristics, but otherwise it looks good to me.

ecordell · 2019-05-09T16:20:01Z

/hold

holding until master is open for 4.2 😢

ecordell · 2019-05-13T19:59:37Z

/hold cancel

jpeeler · 2019-05-16T14:50:32Z

pkg/controller/operators/olm/operatorgroup.go

@@ -468,7 +468,7 @@ func (a *Operator) ensureTenantRBAC(operatorNamespace, targetNamespace string, c
 		if !ownerutil.IsOwnedBy(ownedRoleBinding, csv) {
 			continue
 		}
-		_, ok := targetRolesByName[ownedRoleBinding.GetName()]
+		_, ok := targetRoleBindingsByName[ownedRoleBinding.GetName()]


I'm not sure about this change, but it looks right.

jpeeler · 2019-05-16T14:52:00Z

pkg/controller/operators/olm/operatorgroup.go

@@ -583,7 +591,7 @@ func (a *Operator) copyToNamespace(csv *v1alpha1.ClusterServiceVersion, namespac
 			fetchedCSV.SetLabels(utillabels.AddLabel(fetchedCSV.GetLabels(), v1alpha1.CopiedLabelKey, csv.GetNamespace()))
 			// CRs don't support strategic merge patching, but in the future if they do this should be updated to patch
 			logger.Debug("updating target CSV")
-			if _, err := a.client.OperatorsV1alpha1().ClusterServiceVersions(namespace).Update(fetchedCSV); err != nil {
+			if fetchedCSV, err = a.client.OperatorsV1alpha1().ClusterServiceVersions(namespace).Update(fetchedCSV); err != nil {


This change and the one below is just getting a more refreshed copy of the CSV. But the way it's written to overwrite the copy from the lister may not be acceptable?

This mirrors what's currently done for e2e tests. Although unit test execution is much faster, being able to specify a single test for repeated refinement still saves time.

This refactors the implementation to no longer race itself with reading from the cache, which sometimes would fail. Essentially, the code has been modified to not create a CSV and then later within the same sync loop try to look up the same CSV in the lister. Changes: -ensureRBACInTargetNamespace was moved from syncCopyCSV to syncClusterServiceVersion -ensureRBACInTargetNamespace changed to no longer create role/rolebindings, just cluster level RBAC -ensureTenantRBAC was moved into ensureCSVsInNamespaces -ensureTenantRBAC was changed to utilize newly created CSV of target namespace instead of retrieving from the cache

This forces TestSyncOperatorGroups to allow the cache to catch up as the number of resources that are checked here is high. The additional ignoreCopyError parameter in the tests have been added to handle the scenario where syncCopyCSV would never be executed in a production workflow, but for testing it's being called explicitly.

While polling if the CSV is not found in the lister, make sure to return no error so that the polling continues.

jpeeler · 2019-05-16T17:44:47Z

/test e2e-aws-console-olm

jpeeler · 2019-05-16T18:36:35Z

/test e2e-aws-console-olm

jpeeler · 2019-05-17T13:28:48Z

/test e2e-aws-console-olm

jpeeler · 2019-05-20T16:50:47Z

/test e2e-aws-console-olm

jpeeler · 2019-05-20T19:02:45Z

Dear reviewer, please verify ensureTenantRBAC is right. Otherwise, I think everything else is good.

ecordell · 2019-05-20T22:05:16Z

/lgtm

🎉

openshift-ci-robot · 2019-05-20T22:05:19Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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,jpeeler]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci-robot requested review from ecordell and njhale May 1, 2019 18:54

openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 1, 2019

ecordell requested changes May 1, 2019

View reviewed changes

jpeeler commented May 1, 2019

View reviewed changes

jpeeler mentioned this pull request May 7, 2019

Update catalog if image changes #816

Merged

ecordell reviewed May 9, 2019

View reviewed changes

openshift-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 9, 2019

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 13, 2019

jpeeler force-pushed the unit-flake-fix branch from 3a5b2ea to c97955e Compare May 16, 2019 14:48

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

jpeeler commented May 16, 2019

View reviewed changes

jpeeler added 3 commits May 16, 2019 11:04

feat(unit): allow specific unit test execution

66cc9dd

This mirrors what's currently done for e2e tests. Although unit test execution is much faster, being able to specify a single test for repeated refinement still saves time.

jpeeler force-pushed the unit-flake-fix branch from c97955e to 84c8914 Compare May 16, 2019 15:39

fix(unit): fix flake in TestUpdate

03a7f6b

While polling if the CSV is not found in the lister, make sure to return no error so that the polling continues.

openshift-ci-robot assigned ecordell May 20, 2019

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

openshift-merge-robot merged commit f144c0f into operator-framework:master May 21, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor to avoid cache races #837

Refactor to avoid cache races #837

jpeeler commented May 1, 2019

ecordell left a comment

ecordell May 1, 2019

jpeeler May 1, 2019

ecordell May 9, 2019

ecordell May 1, 2019

jpeeler May 1, 2019

jpeeler left a comment

jpeeler May 1, 2019

jpeeler May 1, 2019

jpeeler commented May 1, 2019

jpeeler commented May 3, 2019

jpeeler commented May 3, 2019

ecordell May 9, 2019

ecordell May 9, 2019

ecordell commented May 9, 2019

ecordell commented May 9, 2019

ecordell commented May 13, 2019

jpeeler May 16, 2019

jpeeler May 16, 2019

jpeeler commented May 16, 2019

jpeeler commented May 16, 2019

jpeeler commented May 17, 2019

jpeeler commented May 20, 2019

jpeeler commented May 20, 2019

ecordell commented May 20, 2019

openshift-ci-robot commented May 20, 2019

Refactor to avoid cache races #837

Refactor to avoid cache races #837

Conversation

jpeeler commented May 1, 2019

ecordell 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

Choose a reason for hiding this comment

jpeeler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpeeler commented May 1, 2019

jpeeler commented May 3, 2019

jpeeler commented May 3, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ecordell commented May 9, 2019

ecordell commented May 9, 2019

ecordell commented May 13, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpeeler commented May 16, 2019

jpeeler commented May 16, 2019

jpeeler commented May 17, 2019

jpeeler commented May 20, 2019

jpeeler commented May 20, 2019

ecordell commented May 20, 2019

openshift-ci-robot commented May 20, 2019