wip(operators): watch preferred kinds dynamically #1670

njhale · 2020-07-22T18:49:36Z

Description of the change:

Allow associating resources of all preferred GVKs in a cluster with an Operator resource.
Enable automatic adoption of components for all preferred GVKs

This means that any resource on the cluster can be labelled as an Operator component.

Motivation for the change:

The set of possible resource types that can comprise an operator changes dynamically as a cluster's control-plane is extended; w/ CRDs and extension APIServices. Often, an operator author considers certain custom resources of kinds they provide to be a component of their operator deployment ( e.g. a config CR).

openshift-ci-robot · 2020-07-22T18:49:39Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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~~ [njhale]

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

njhale · 2020-07-22T18:52:15Z

pkg/controller/operators/adoption_controller.go

-		Watches(&source.Kind{Type: &apiregistrationv1.APIService{}}, enqueueCSV).
-		Watches(&source.Kind{Type: &operatorsv1alpha1.Subscription{}}, enqueueCSV).
-		Watches(&source.Kind{Type: &operatorsv1alpha1.InstallPlan{}}, enqueueCSV).
+		Watches(r.source, enqueueCSV).


I need to figure out a way to share caches/watches between controllers under a single manager. I don't like the idea watching and storing k|cluster resources|.

njhale · 2020-07-22T18:54:26Z

pkg/controller/operators/adoption_controller.go

+	var componentLists []runtime.Object
+	for _, gvk := range informable {
+		gvk.Kind = gvk.Kind + "List"
+		ul := &unstructured.UnstructuredList{}
+		ul.SetGroupVersionKind(gvk)
+		componentLists = append(componentLists, ul)


Needs to be deduplicated across the operator and adoption controller. This is probably generic enough to be it's own layer on top of the controller-runtime client.

njhale · 2020-07-22T18:55:57Z

pkg/controller/operators/decorators/operator.go

@@ -218,6 +218,7 @@ func (o *Operator) AdoptComponent(component runtime.Object) (adopted bool, err e
 		labels[labelKey] = ""
 		adopted = true
 	}
+	m.SetLabels(labels)


Makes the changes stick for unstructured.Unstructured.

njhale · 2020-07-22T18:57:15Z

pkg/lib/controller-runtime/source/dynamic.go

+)
+
+// InformableGVKs returns the set of GVKs available in discovery that support the list and watch verbs (informable).
+func (d *Dynamic) InformableGVKs() (informables []schema.GroupVersionKind, err error) {


Going to factor this out of Dynamic.

getInformableGVKs might make the code more readable at the site this function is called.

https://dmitripavlutin.com/coding-like-shakespeare-practical-function-naming-conventions/#2-prefer-explanatory-names

It's actually quite a useful function in a standalone way

getInformableGVKs might make the code more readable at the site this function is called.

https://dmitripavlutin.com/coding-like-shakespeare-practical-function-naming-conventions/#2-prefer-explanatory-names

I actually disagree, I think it's Go code convention not to have things like Get... in function names, unlike say JavaScript
https://golang.org/doc/effective_go.html#Getters

It's a pet peeve of mine 😄

Okay fair point. I concede.

njhale · 2020-07-22T18:59:36Z

pkg/lib/controller-runtime/source/dynamic.go

+	}
+
+	var resourceLists []*metav1.APIResourceList
+	resourceLists, err = d.client.ServerPreferredResources()


Fails when discovery breaks for any resource.

njhale · 2020-07-22T19:00:33Z

pkg/lib/controller-runtime/source/dynamic.go

+		informablesLock.RLock()
+		defer informablesLock.RUnlock()
+		log.Error(err, "could not get preferred resources, returning latest known gvks")
+		return knownInformables, nil


Going to memoize the last successful APIResourceList and whittle it down with direct checks against the discovery client instead. This will let us weed out the problem resource types before returning.

njhale · 2020-07-22T19:02:37Z

pkg/lib/controller-runtime/source/dynamic.go

+			}
+		}
+
+	}, 15*time.Second, d.stop)


What do we think about triggering this when APIServices and CRDs change?

Makes sense. Maybe we can expand the InformableGVKs() function to not only memoize on known GVKs in case of discovery errors but also use that as a cache so that when new CRDs (GVKs) become available we can use the diff instead of going through the ServerPreferredResources again

njhale · 2020-07-22T19:04:31Z

pkg/lib/controller-runtime/source/dynamic.go

+			}
+		}
+
+	}, 15*time.Second, d.stop)


Make this configurable?

njhale · 2020-07-22T19:04:57Z

pkg/lib/controller-runtime/source/dynamic.go

+
+	}, 15*time.Second, d.stop)
+
+	synced.Wait()


Block until we set up the initial set of sources.

njhale · 2020-07-22T19:17:18Z

pkg/lib/controller-runtime/source/dynamic.go

+	r := &unstructured.Unstructured{}
+	r.SetGroupVersionKind(gvk)
+	s := &source.Kind{Type: r}
+	if err = s.InjectCache(d.cache); err != nil {


controller-runtime doesn't expose a way to stop individual sources/informers, see:

Allow removing individual informers from the Cache kubernetes-sigs/controller-runtime#935

✨ Allow removing individual informers from the cache (#935) kubernetes-sigs/controller-runtime#936

https://github.com/gardener/gardener-resource-manager/issues/61

I'm curious to see if anyone has suggestions for how to get around this short of reimplementing controller-runtime's informerCache.

Bowenislandsong · 2020-07-22T19:34:45Z

This PR failed tests for 1 times with 1 individual failed tests and 4 skipped tests. A test is considered flaky if failed on multiple commits.

totaltestcount: 1
flaketestcount: 1
skippedtestcount: 4
flaketests:

classname: End-to-end
name: 'Subscription skip range'
counts: 1
details:

count: 1
error: |4-

/home/runner/work/operator-lifecycle-manager/operator-lifecycle-manager/test/e2e/subscription_e2e_test.go:92

	Error Trace:	subscription_e2e_test.go:164
	            				runner.go:113
	            				runner.go:64
	            				it_node.go:26
	            				spec.go:215
	            				spec.go:138
	            				spec_runner.go:200
	            				spec_runner.go:170
	            				spec_runner.go:66
	            				suite.go:62
	            				ginkgo_dsl.go:226
	            				ginkgo_dsl.go:214
	            				e2e_test.go:54
	Error:      	Received unexpected error:
	            	etcdserver: request timed out

/home/runner/work/operator-lifecycle-manager/operator-lifecycle-manager/vendor/github.com/stretchr/testify/require/require.go:1005

meandurationsec: 28.239263
skippedtests:

classname: End-to-end
name: 'Subscription updates existing install plan'
counts: 1
details: []
meandurationsec: 0.056757
classname: End-to-end
name: 'Catalog image update'
counts: 1
details: []
meandurationsec: 0.307005
classname: End-to-end
name: 'Subscriptions create required objects from Catalogs Given a Namespace
when a CatalogSource is created with a bundle that contains prometheus objects
creating a subscription using the CatalogSource should have created the expected
prometheus objects'
counts: 1
details: []
meandurationsec: 2.184881
classname: End-to-end
name: 'Subscriptions create required objects from Catalogs Given a Namespace
when a CatalogSource is created with a bundle that contains prometheus objects
creating a subscription using the CatalogSource should install the operator
successfully'
counts: 1
details: []
meandurationsec: 3.170511

njhale · 2020-07-23T16:00:28Z

/test e2e-gcp-upgrade

anik120 · 2020-07-23T16:04:42Z

pkg/lib/controller-runtime/source/dynamic.go

+}
+
+// InjectConfig injects the the REST Config dependency.
+func (d *Dynamic) InjectConfig(config *rest.Config) (err error) {


Is InjectRESTConfigDependency too big of a name? It'll would get rid of the comment, and be self explanatory in the calling site too.

I don't disagree, but we don't control the method signatures used for injection.

It'll would get rid of the comment

Also, golangci-lint generally complains about exports w/o comments.

exdx

Cool! just leaving some general feedback

exdx · 2020-07-23T17:32:58Z

pkg/lib/controller-runtime/source/dynamic.go

+	synced.Add(1)
+
+	go wait.Until(func() {
+		defer once.Do(func() {


The intention here is to only call synced.Done() once after the function exits - but since the function is called within the Until() scope every 15 seconds are you sure this doesn't just get triggered the first time the function is executed within the Until loop?

This is probably standard code, just want to confirm.

The intention here is to only call synced.Done() once after the function exits - but since the function is called within the Until() scope every 15 seconds are you sure this doesn't just get triggered the first time the function is executed within the Until loop?

I'm not understanding the delineation you're making here. Could you rephrase?

exdx · 2020-07-23T17:40:07Z

pkg/lib/controller-runtime/source/dynamic.go

+			}
+		}
+
+	}, 15*time.Second, d.stop)


Makes sense. Maybe we can expand the InformableGVKs() function to not only memoize on known GVKs in case of discovery errors but also use that as a cache so that when new CRDs (GVKs) become available we can use the diff instead of going through the ServerPreferredResources again

exdx · 2020-07-23T17:40:50Z

pkg/lib/controller-runtime/source/dynamic.go

+)
+
+// InformableGVKs returns the set of GVKs available in discovery that support the list and watch verbs (informable).
+func (d *Dynamic) InformableGVKs() (informables []schema.GroupVersionKind, err error) {


It's actually quite a useful function in a standalone way

exdx · 2020-07-23T17:42:27Z

pkg/lib/controller-runtime/source/dynamic.go

+	}
+
+	if len(informables) > 0 {
+		// Memoize the latest informables in case discovery fails on successive calls


I like this touch a lot. May be interesting to play around with an e2e test that fails discovery calls intentionally and sees how the operator API behaves

exdx · 2020-07-23T17:45:27Z

pkg/controller/operators/operator_controller.go

-		&operatorsv1alpha1.SubscriptionList{},
-		&operatorsv1alpha1.InstallPlanList{},
-		&operatorsv1alpha1.ClusterServiceVersionList{},
+	informable, err := r.source.InformableGVKs()


nit: this may take a while on an Openshift cluster with a ton of GVKs...maybe adding a log message indicating that this process is happening could be insightful

exdx · 2020-07-23T17:48:11Z

I think a lot of this work will be useful in the "OLM supports arbitrary kube types" feature upcoming in the next release. In fact if there's anything here that we could expose to that effort that would be super helpful.

openshift-ci-robot · 2020-08-02T09:40:32Z

@njhale: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci-robot · 2020-09-09T17:16:54Z

@njhale: The following tests failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
ci/prow/e2e-gcp-upgrade	`e37184e`	link	`/test e2e-gcp-upgrade`
ci/prow/e2e-upgrade	`e37184e`	link	`/test e2e-upgrade`

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

njhale · 2020-09-17T00:36:21Z

Closing this out to pursue an alternative: for each API group, whenever the default version set changes, spin up a new pod to watch the resources of that set.

wip(operators): watch preferred kinds dynamically

e37184e

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 22, 2020

openshift-ci-robot requested review from awgreene and kevinrizza July 22, 2020 18:49

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 22, 2020

njhale commented Jul 22, 2020

View reviewed changes

pkg/lib/controller-runtime/source/dynamic.go

}

}

}, 15*time.Second, d.stop)

Copy link

Member Author

njhale Jul 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make this configurable?

njhale commented Jul 22, 2020

View reviewed changes

anik120 reviewed Jul 23, 2020

View reviewed changes

exdx reviewed Jul 23, 2020

View reviewed changes

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 2, 2020

njhale closed this Sep 17, 2020

njhale deleted the dynsrc branch July 23, 2021 02:49

wip(operators): watch preferred kinds dynamically #1670

wip(operators): watch preferred kinds dynamically #1670

Uh oh!

Conversation

njhale commented Jul 22, 2020

Uh oh!

openshift-ci-robot commented Jul 22, 2020

Uh oh!

njhale Jul 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Bowenislandsong commented Jul 22, 2020

Uh oh!

njhale commented Jul 23, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

njhale Jul 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

exdx left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

exdx commented Jul 23, 2020

Uh oh!

openshift-ci-robot commented Aug 2, 2020

Uh oh!

openshift-ci-robot commented Sep 9, 2020

Uh oh!

njhale commented Sep 17, 2020

Uh oh!

Uh oh!

njhale Jul 22, 2020 •

edited

Loading

njhale Jul 23, 2020 •

edited

Loading