go.*,pkg,vendor: Bump controller-runtime to v0.10.1 #2368

timflannagan · 2021-09-21T19:15:40Z

Description of the change:
Bumps the controller runtime dependency to v0.10.1 which contains the race condition fix around metadata-only watchers.

Motivation for the change:
Removes the replace pin in the root go.mod.

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 /doc
Commit messages sensible and descriptive

Fixes #2353

awgreene · 2021-12-06T15:53:55Z

test/e2e/deprecated_e2e_test.go

@@ -30,15 +30,15 @@ var _ = Describe("Not found APIs", func() {
 		// each entry is an installplan with a deprecated resource
 		type payload struct {
 			name       string
-			ip         *operatorsv1alpha1.InstallPlan
+			IP         *operatorsv1alpha1.InstallPlan


Non-blocking: Why was this exported?

awgreene · 2021-12-06T15:56:53Z

/approve

openshift-ci · 2021-12-06T15:57:10Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: awgreene, timflannagan

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~~ [awgreene]

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

awgreene · 2021-12-06T16:02:28Z

[BeforeEach] Operator API
  /home/runner/work/operator-lifecycle-manager/operator-lifecycle-manager/test/e2e/operator_test.go:41
[It] should surface components in its status
  /home/runner/work/operator-lifecycle-manager/operator-lifecycle-manager/test/e2e/operator_test.go:73
STEP: eventually having a status that contains its component label selector
STEP: eventually listing a single component reference
first patch error: Operation cannot be fulfilled on serviceaccounts "sa-a-gcg2h": the object has been modified; please apply your changes to the latest version and try again
STEP: eventually listing multiple component references

Is there potentially a race condition here?

awgreene · 2021-12-06T16:05:05Z

/retest

tylerslaton · 2021-12-06T16:17:19Z

pkg/controller/operators/suite_test.go

-			crds.OperatorGroup(),
-			crds.Operator(),
-			crds.OperatorCondition(),
+		CRDs: []apiextensionsv1.CustomResourceDefinition{


nb: Is this something we should move to across the board when using controller-runtime moving forward?

tylerslaton · 2021-12-06T16:24:51Z

/lgtm

openshift-bot · 2021-12-06T17:01:32Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-12-06T18:04:34Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

timflannagan · 2021-12-06T18:16:15Z

Holding so the bot doesn't go crazy.

/hold

timflannagan

It seems like we're continuously running into the following errors:

Summarizing 2 Failures:

[Fail] Operator API [It] should surface components in its status 
/home/runner/work/operator-lifecycle-manager/operator-lifecycle-manager/test/e2e/util_test.go:838

[Fail] Operator API when a subscription to a package exists [It] should automatically adopt components 
/home/runner/work/operator-lifecycle-manager/operator-lifecycle-manager/test/e2e/operator_test.go:346

When poking around these errors locally, it seems like both of the test failures are around existing ServiceAccount's that aren't being propagated to the list of status.components.refs array. Maybe we're missing that cache event entirely in some cases?

Note: in the context of the second test case, it looks like the ServiceAccount in the kiali bundle was labeled with the operator component. Is the adoption controller, which also uses metadata-only watches for a subset of the resource watches, responsible for ensuring that resource gets labeled?

timflannagan · 2021-12-08T20:57:56Z

Hmm weird it looks like we do get the mapping function event properly but we're somehow not able to update the Operator resource:

2021-12-08T19:40:37.265148583Z stderr F 2021-12-08T19:40:37.265Z	INFO	controllers.adoption	mapToClusterServiceVersions: requeuing a csv	{"gvk": "&TypeMeta{Kind:,APIVersion:,}", "name": "kiali-operator", "namespace": "ns-4f8q4"}
2021-12-08T19:40:37.265357386Z stderr F 2021-12-08T19:40:37.265Z	INFO	controllers.operator	mapComponentRequests: requeuing operator resource	{"gvk": "&TypeMeta{Kind:,APIVersion:,}", "name": "kiali-operator", "namespace": "ns-4f8q4"}

I'm going to try and update to also collect the state of that Operator resource.

estroz · 2021-12-10T02:45:18Z

@timflannagan hoping #2515 doesn't screw this PR up. @dinhxuanvu and I needed to get unblocked for constraint stuff added to api.

timflannagan · 2021-12-10T03:22:40Z

@estroz Let's definitely touch base tomorrow. We'll need to cut an o-f/api minor release in any case, but there's some complexity involved with certain controller runtime versions, and it's compatibility with OLM.

cc @awgreene @njhale

Signed-off-by: timflannagan <[email protected]>

Update the CRD field to use the apiextensionsv1.CustomResourceDefinition type instead of the client.Object interface. It looks like this change was introduced during the sweep to k8s 1.22 bump: kubernetes-sigs/controller-runtime@b5eeb71 Signed-off-by: timflannagan <[email protected]>

Update the o/api dependency and pull in a new version that contains v1 CRDs. Newer controller-runtime versions don't support v1beta1 CRDs Signed-off-by: timflannagan <[email protected]>

…pendency Signed-off-by: timflannagan <[email protected]>

Update the adoption controller test subscription fixture and remove the status sub-resource block that's shared between tests. When an object is created, it's status sub-resource is now removed, so changes are centered around setting this resource before firing off any Update calls used throughout this test package. Update any test/e2e APIs that also follow a similiar pattern. Signed-off-by: timflannagan <[email protected]>

estroz · 2021-12-10T19:39:50Z

/lgtm
/hold cancel

timflannagan · 2021-12-12T18:43:45Z

go.mod

 	// Patch for a race condition involving metadata-only
 	// informers until it can be resolved upstream:
-	sigs.k8s.io/controller-runtime v0.9.2 => github.com/benluddy/controller-runtime v0.9.3-0.20210720171926-9bcb99bd9bd3
+	sigs.k8s.io/controller-runtime v0.10.0 => github.com/timflannagan/controller-runtime v0.10.1-0.20211210161403-6756a4203e70


Tracking the removal of this pin in #2353 (comment).

openshift-ci bot requested review from gallettilance and joelanford September 21, 2021 19:15

timflannagan force-pushed the bump-controller-runtime branch from 0273ed2 to 1c3da5a Compare September 21, 2021 19:15

timflannagan closed this Sep 23, 2021

timflannagan reopened this Sep 23, 2021

timflannagan force-pushed the bump-controller-runtime branch 3 times, most recently from 181ed76 to bfe84d4 Compare December 3, 2021 16:41

awgreene reviewed Dec 6, 2021

View reviewed changes

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 6, 2021

tylerslaton reviewed Dec 6, 2021

View reviewed changes

openshift-ci bot assigned tylerslaton Dec 6, 2021

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 6, 2021

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 6, 2021

timflannagan commented Dec 6, 2021

View reviewed changes

timflannagan mentioned this pull request Dec 7, 2021

OLM is depending on an old version of github.com/openshift/api #2505

Closed

timflannagan force-pushed the bump-controller-runtime branch from bfe84d4 to ffb4b0f Compare December 7, 2021 19:11

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Dec 7, 2021

timflannagan force-pushed the bump-controller-runtime branch from bbe44c5 to babce58 Compare December 8, 2021 19:01

awgreene mentioned this pull request Dec 8, 2021

WIP: Introduce olm config (Revert CR Bump in API Repo) #2513

Closed

timflannagan force-pushed the bump-controller-runtime branch from e136f98 to 6cb7f2d Compare December 8, 2021 21:20

timflannagan force-pushed the bump-controller-runtime branch 10 times, most recently from ad0cf83 to 5e3ee03 Compare December 8, 2021 23:59

timflannagan added 5 commits December 10, 2021 11:21

go.*,vendor: Bump controller-runtime to v0.10.1

c2d8b29

Signed-off-by: timflannagan <[email protected]>

go.*,vendor: Bump the o/api dependency for v1 CRDs

de27cbd

Update the o/api dependency and pull in a new version that contains v1 CRDs. Newer controller-runtime versions don't support v1beta1 CRDs Signed-off-by: timflannagan <[email protected]>

pkg/controller: Update openshift-related tests after bumping o/api de…

363f1e0

…pendency Signed-off-by: timflannagan <[email protected]>

timflannagan force-pushed the bump-controller-runtime branch from 05bd756 to 83c4cad Compare December 10, 2021 16:21

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 10, 2021

openshift-ci bot assigned estroz Dec 10, 2021

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 10, 2021

openshift-merge-robot merged commit 1f2472f into operator-framework:master Dec 10, 2021

timflannagan deleted the bump-controller-runtime branch December 10, 2021 19:41

timflannagan mentioned this pull request Dec 10, 2021

chore(deps): upgrade o-f/api for new types #2515

Merged

5 tasks

timflannagan commented Dec 12, 2021

View reviewed changes

fgiloux mentioned this pull request Jan 21, 2022

Bump k8s dependencies to 1.23 to align with o-f/api #2574

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

go.*,pkg,vendor: Bump controller-runtime to v0.10.1 #2368

go.*,pkg,vendor: Bump controller-runtime to v0.10.1 #2368

timflannagan commented Sep 21, 2021 •

edited

Loading

awgreene Dec 6, 2021

awgreene commented Dec 6, 2021

openshift-ci bot commented Dec 6, 2021

awgreene commented Dec 6, 2021

awgreene commented Dec 6, 2021

tylerslaton Dec 6, 2021 •

edited

Loading

tylerslaton commented Dec 6, 2021

openshift-bot commented Dec 6, 2021

openshift-bot commented Dec 6, 2021

timflannagan commented Dec 6, 2021

timflannagan left a comment •

edited

Loading

timflannagan commented Dec 8, 2021

estroz commented Dec 10, 2021

timflannagan commented Dec 10, 2021

estroz commented Dec 10, 2021

timflannagan Dec 12, 2021

go.*,pkg,vendor: Bump controller-runtime to v0.10.1 #2368

go.*,pkg,vendor: Bump controller-runtime to v0.10.1 #2368

Conversation

timflannagan commented Sep 21, 2021 • edited Loading

awgreene Dec 6, 2021

Choose a reason for hiding this comment

awgreene commented Dec 6, 2021

openshift-ci bot commented Dec 6, 2021

awgreene commented Dec 6, 2021

awgreene commented Dec 6, 2021

tylerslaton Dec 6, 2021 • edited Loading

Choose a reason for hiding this comment

tylerslaton commented Dec 6, 2021

openshift-bot commented Dec 6, 2021

openshift-bot commented Dec 6, 2021

timflannagan commented Dec 6, 2021

timflannagan left a comment • edited Loading

Choose a reason for hiding this comment

timflannagan commented Dec 8, 2021

estroz commented Dec 10, 2021

timflannagan commented Dec 10, 2021

estroz commented Dec 10, 2021

timflannagan Dec 12, 2021

Choose a reason for hiding this comment

timflannagan commented Sep 21, 2021 •

edited

Loading

tylerslaton Dec 6, 2021 •

edited

Loading

timflannagan left a comment •

edited

Loading