Proper handling of cluster-scoped resources in operators #1306

kensipe · 2020-01-24T02:10:44Z

What this PR does / why we need it:

Summary:

proper applying and re-applying of cluster-scoped resources in the task_apply.go
ownership is NOT set for cluster-scoped resources to the Instances

Fixes #1288
Fixes #1265

Signed-off-by: Ken Sipe kensipe@gmail.com

Signed-off-by: Ken Sipe <kensipe@gmail.com>

pkg/engine/task/task_apply.go

gerred · 2020-01-27T15:29:37Z

pkg/engine/task/task_apply_test.go

+
+	// just a small sanity check
+	assert.True(t, got[schema.GroupVersionKind{Group: "apps", Version: "v1", Kind: "ReplicaSet"}])
+	assert.False(t, got[schema.GroupVersionKind{Group: "apiextensions.k8s.io", Version: "v1", Kind: "CustomResourceDefinition"}])


can we test if a custom resource for both a Namespaced CRD and a Cluster scoped CRD also work here, just so we have it in the event of any regressions (and we're talking, in Kubernetes terms, about two different API servers here)

You mean a CR (not CRD), right? Because CRDs are always cluster-scoped. CRs might be not.

kensipe · 2020-01-27T15:33:46Z

tests fail... this test was added as a unit test but it looks like an integration test

--- FAIL: Test_namespacedGVKResources (0.00s)
    task_apply_test.go:196: 
        	Error Trace:	task_apply_test.go:196
        	Error:      	Received unexpected error:
        	            	failed to fetch cluster REST config: could not locate a kubeconfig
        	Test:       	Test_namespacedGVKResources
    task_apply_test.go:199: 
        	Error Trace:	task_apply_test.go:199
        	Error:      	Should be true
        	Test:       	Test_namespacedGVKResources

kensipe · 2020-01-27T15:34:58Z

I'm also concerned... what happens if the resource isn't in the map?

gerred · 2020-01-27T15:36:07Z

I'm also concerned... what happens if the resource isn't in the map?

an explicit failure with an Event would be good here - this would be the case if a CRD doesn't exist.

zen-dog · 2020-01-27T15:48:35Z

I'm also concerned... what happens if the resource isn't in the map?

Good point. currently, it will be false but... an explicit error might be better? 🤔

zen-dog · 2020-01-27T15:51:00Z

this would be the case if a CRD doesn't exist.

what do you mean? We don't check for specific CRDs, just if GVK GroupVersionKind{Group: "apiextensions.k8s.io", Version: "v1", Kind: "CustomResourceDefinition"} as such is available.

gerred · 2020-01-27T15:52:47Z

@zen-dog If the Prometheus Operator ServiceMonitor CRD isn't created but a ServiceMonitor CR is in a plan, this would fail, wouldn't it?

gerred · 2020-01-27T15:53:10Z

(or some cluster-scoped form of ServiceMonitor, that is.. so maybe something from Istio?)

zen-dog · 2020-01-27T15:54:31Z

If the Prometheus Operator ServiceMonitor CRD isn't created but a ServiceMonitor CR is in a plan, this would fail, wouldn't it?

this would fail either way (independent of namespaced vs cluster-scoped) because KUDO would fail to apply an unknown resource Kind, right?

gerred · 2020-01-27T15:58:00Z

Oh yeah, because nothing happens right now when no match is discovered in the GVK map. ObjectKeyFromObject happens before the actual apply though - so I suppose to bounce off of @kensipe, do we want to catch it in the path for ObjectKeyFromObject before we actually try to apply?

zen-dog · 2020-01-27T16:00:58Z

do we want to catch it in the path for ObjectKeyFromObject before we actually try to apply?

should be a one-liner. I can add it. Though, there are multiple other places where we "let it run and fail" because figuring it out upfront is too much work.

…resource list

zen-dog

Approved the "simple fix" presented in the first commit. / cc @kensipe We'll work on a proper one in a followup PR

…the API resource list" This reverts commit 5d34c34.

…esources" This reverts commit 6ecf51b.

alenkacz · 2020-01-28T16:36:14Z

pkg/engine/task/task_apply.go

+func namespacedGVKResources() (map[schema.GroupVersionKind]bool, error) {
+	namespaced := map[schema.GroupVersionKind]bool{}
+
+	restCfg, err := config.GetConfig()


you should probably inject the config rather than load it here. You would have to find a good place from which to pass it in though, the most upper level for this is manager.GetConfig in main.go and then carry it through controller

…t out Signed-off-by: Ken Sipe <kensipe@gmail.com>

Co-authored-by: Aleksey Dukhovniy <adukhovniy@mesosphere.io> Signed-off-by: Ken Sipe <kensipe@gmail.com> Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

Co-authored-by: Aleksey Dukhovniy <adukhovniy@mesosphere.io> Signed-off-by: Ken Sipe <kensipe@gmail.com> Signed-off-by: Thomas Runyon <runyontr@gmail.com>

Cluster Resource in Plans work

6eab14e

Signed-off-by: Ken Sipe <kensipe@gmail.com>

kensipe requested review from alenkacz, gerred, nfnt and zen-dog as code owners January 24, 2020 02:10

kensipe mentioned this pull request Jan 24, 2020

[Spark Operator] Deploy plan hangs due to a transient error #1265

Closed

alenkacz reviewed Jan 24, 2020

View reviewed changes

pkg/engine/task/task_apply.go Show resolved Hide resolved

zen-dog reviewed Jan 24, 2020

View reviewed changes

pkg/engine/task/task_apply.go Show resolved Hide resolved

pkg/engine/task/task_apply.go Show resolved Hide resolved

pkg/engine/task/task_apply.go Show resolved Hide resolved

Using discovery client to properly check for cluster-scoped resources

6ecf51b

gerred reviewed Jan 27, 2020

View reviewed changes

Return an error if a resource with a given GVK is missing in the API …

5d34c34

…resource list

zen-dog changed the title ~~Cluster Resources in Plans Work~~ Proper handling of cluster-scoped resources in operators Jan 27, 2020

zen-dog approved these changes Jan 28, 2020

View reviewed changes

kensipe added 2 commits January 28, 2020 10:40

Revert "Return an error if a resource with a given GVK is missing in …

529a1fe

…the API resource list" This reverts commit 5d34c34.

Revert "Using discovery client to properly check for cluster-scoped r…

2681b5a

…esources" This reverts commit 6ecf51b.

alenkacz reviewed Jan 28, 2020

View reviewed changes

reverting back to quick fix. proper fix coming. it needs to be though…

0abbcfd

…t out Signed-off-by: Ken Sipe <kensipe@gmail.com>

kensipe mentioned this pull request Jan 28, 2020

Proper Discovery of Cluster Resource #1316

Closed

kensipe merged commit 7343faa into master Jan 28, 2020

kensipe deleted the ken/cluster-resource-apply branch January 28, 2020 19:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Proper handling of cluster-scoped resources in operators #1306

Proper handling of cluster-scoped resources in operators #1306

kensipe commented Jan 24, 2020 •

edited by zen-dog

Loading

gerred Jan 27, 2020

zen-dog Jan 27, 2020

kensipe commented Jan 27, 2020

kensipe commented Jan 27, 2020

gerred commented Jan 27, 2020

zen-dog commented Jan 27, 2020

zen-dog commented Jan 27, 2020

gerred commented Jan 27, 2020

gerred commented Jan 27, 2020

zen-dog commented Jan 27, 2020

gerred commented Jan 27, 2020

zen-dog commented Jan 27, 2020 •

edited

Loading

zen-dog left a comment

alenkacz Jan 28, 2020

Proper handling of cluster-scoped resources in operators #1306

Proper handling of cluster-scoped resources in operators #1306

Conversation

kensipe commented Jan 24, 2020 • edited by zen-dog Loading

gerred Jan 27, 2020

Choose a reason for hiding this comment

zen-dog Jan 27, 2020

Choose a reason for hiding this comment

kensipe commented Jan 27, 2020

kensipe commented Jan 27, 2020

gerred commented Jan 27, 2020

zen-dog commented Jan 27, 2020

zen-dog commented Jan 27, 2020

gerred commented Jan 27, 2020

gerred commented Jan 27, 2020

zen-dog commented Jan 27, 2020

gerred commented Jan 27, 2020

zen-dog commented Jan 27, 2020 • edited Loading

zen-dog left a comment

Choose a reason for hiding this comment

alenkacz Jan 28, 2020

Choose a reason for hiding this comment

kensipe commented Jan 24, 2020 •

edited by zen-dog

Loading

zen-dog commented Jan 27, 2020 •

edited

Loading