Bug 1860035: Fix SubscriptionConfig NodeSelector field #1716

awgreene · 2020-08-07T18:56:31Z

Problem: OLM's Subscription CRD allows cluster admins to set
nodeSelectors on Operators they deploy. Currently this API is exposed
but performs no action.

Solution: Wire the NodeSelector specified in the SubscriptionConfig into
the pods deployed when installing the operator.

openshift-ci-robot · 2020-08-07T18:57:24Z

@awgreene: This pull request references Bugzilla bug 1860035, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.6.0) matches configured target release for branch (4.6.0)
bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1860035: Fix SubscriptionConfig NodeSelector field

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.

exdx

Looks good, just some nits
/lgtm

exdx · 2020-08-07T19:00:33Z

doc/design/subscription-config.md

+
+### NodeSelector
+
+The `nodeSelector` field defines a [NodeSelector](https://kubernetes.io/docs/concepts/scheduling-eviction/assign-pod-node/) for the Pod created by OLM.


nit: this is a bit vague. Could we say "for the pods associated with the operator deployment" instead?

This also future-proofs it a bit where in the future OLM supports more than just deploying operators, something we discussed recently

nit: this is a bit vague. Could we say "for the pods associated with the operator deployment" instead?

Technically this config is applied to all deployments created by OLM.

This also future-proofs it a bit where in the future OLM supports more than just deploying operators, something we discussed recently

OLM also creates deployments for webhooks, which could be stored in the same deployment as the operator but isn't guaranteed, so I don't know how OLM could only create deployments for operators.

exdx · 2020-08-07T19:03:32Z

pkg/controller/operators/olm/overrides/inject.go

+//
+// If any Container in PodSpec already defines a NodeSelector it will
+// be overwritten.
+func InjectNodeSelectorIntoDeployment(podSpec *corev1.PodSpec, nodeSelector map[string]string) error {


nit: this function is called InjectNodeSelectorIntoDeployment but injects the nodeselector into a PodSpec

maybe calling it InjectNodeSelectorIntoPodSpec makes it a little clearer?

This makes sense, but none of the other methods in this file distinguish injecting into the deployment or podSpec. I suspect this was either an oversight when the code was written or assuming its fine given that the podspec is part of the deployment. Should we change the distinction in all places or follow convention.

of-deploy-bot · 2020-08-07T20:16:21Z

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

totaltestcount: 1
flaketestcount: 3
skippedtestcount: 4
flaketests:

classname: End-to-end
name: 'Subscription creation with pod config'
counts: 3
details:

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

	Error Trace:	subscription_e2e_test.go:1187
	            				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:
	            	timed out waiting for the condition

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

meandurationsec: 330.616282

classname: End-to-end
name: 'Garbage collection for dependent resources when a bundle with configmap
and secret objects is installed when the CSV is deleted OLM should delete
the associated configmap and secret'
counts: 1
details:

/home/runner/work/operator-lifecycle-manager/operator-lifecycle-manager/test/e2e/gc_e2e_test.go:296
could not get installplan at complete phase
Unexpected error:
    <*errors.errorString | 0xc0001782a0>: {
        s: "timed out waiting for the condition",
    }
    timed out waiting for the condition
occurred
/home/runner/work/operator-lifecycle-manager/operator-lifecycle-manager/test/e2e/gc_e2e_test.go:341

meandurationsec: 313.178593

classname: End-to-end
name: 'CSV create requirements met CRD'
counts: 1
details:

/home/runner/work/operator-lifecycle-manager/operator-lifecycle-manager/test/e2e/csv_e2e_test.go:459

	Error Trace:	csv_e2e_test.go:701
	            				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:
	            	timed out waiting for the condition

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

meandurationsec: 328.260173
skippedtests:

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: 24.124069
classname: End-to-end
name: 'Catalog image update'
counts: 1
details: []
meandurationsec: 0.366264
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: 21.094421
classname: End-to-end
name: 'Subscription updates existing install plan'
counts: 1
details: []
meandurationsec: 0.413831

ecordell · 2020-08-07T21:12:26Z

/approve

openshift-ci-robot · 2020-08-07T21:12:30Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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]

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

awgreene · 2020-08-07T23:32:37Z

/retest

of-deploy-bot · 2020-08-08T00:41:21Z

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

totaltestcount: 1
flaketestcount: 6
skippedtestcount: 4
flaketests:

classname: End-to-end
name: 'Subscription creation with pod config'
counts: 3
details:

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

	Error Trace:	subscription_e2e_test.go:1187
	            				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:
	            	timed out waiting for the condition

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

meandurationsec: 318.4868383333333

classname: End-to-end
name: 'Install Plan with CSVs across multiple catalog sources'
counts: 2
details:

/home/runner/work/operator-lifecycle-manager/operator-lifecycle-manager/test/e2e/installplan_e2e_test.go:45
Resource does not match expected value:   &v1alpha1.InstallPlan{
  	TypeMeta: {},
  	ObjectMeta: v1.ObjectMeta{
  		... // 3 identical fields
  		SelfLink:                   "/apis/operators.coreos.com/v1alpha1/namespaces/operators/install"...,
  		UID:                        "590c586d-3155-4da7-a8a5-deb0f6878baf",
- 		ResourceVersion:            "13442",
+ 		ResourceVersion:            "13449",
  		Generation:                 1,
  		CreationTimestamp:          {Time: s"2020-08-07 23:55:25 +0000 UTC"},
  		DeletionTimestamp:          nil,
  		DeletionGracePeriodSeconds: nil,
  		Labels: map[string]string{
  			"operators.coreos.com/nginx-tdtr7.operators":    "",
+ 			"operators.coreos.com/nginxdep-s6ccq.operators": "",
  		},
  		Annotations:     nil,
  		OwnerReferences: {{APIVersion: "operators.coreos.com/v1alpha1", Kind: "Subscription", Name: "sub-nginx-tvlvj", UID: "670ef166-e345-485d-ae4a-cb4c0f4c150d", ...}},
  		... // 3 identical fields
  	},
  	Spec:   {ClusterServiceVersionNames: {"nginxdep-s6ccq-stable", "nginx-tdtr7-stable"}, Approval: "Automatic", Approved: true, Generation: 1},
  	Status: {Phase: "Complete", Conditions: {{Type: "Installed", Status: "True", LastUpdateTime: s"2020-08-07 23:55:28 +0000 UTC", LastTransitionTime: s"2020-08-07 23:55:28 +0000 UTC", ...}}, CatalogSources: {"mock-ocs-dependent-mgfmv", "mock-ocs-main-ql8lp"}, Plan: {&{Resolving: "nginxdep-s6ccq-stable", Resource: {CatalogSource: "mock-ocs-dependent-mgfmv", CatalogSourceNamespace: "operators", Version: "v1alpha1", Kind: "ClusterServiceVersion", ...}, Status: "Present"}, &{Resolving: "nginx-tdtr7-stable", Resource: {CatalogSource: "mock-ocs-main-ql8lp", CatalogSourceNamespace: "operators", Version: "v1alpha1", Kind: "ClusterServiceVersion", ...}, Status: "Present"}, &{Resolving: "nginxdep-s6ccq-stable", Resource: {CatalogSource: "mock-ocs-dependent-mgfmv", CatalogSourceNamespace: "operators", Group: "apiextensions.k8s.io", Version: "v1beta1", ...}, Status: "Present"}, &{Resolving: "nginxdep-s6ccq-stable", Resource: {CatalogSource: "mock-ocs-dependent-mgfmv", CatalogSourceNamespace: "operators", Group: "operators.coreos.com", Version: "v1alpha1", ...}, Status: "Present"}}, ...},
  }

/home/runner/work/operator-lifecycle-manager/operator-lifecycle-manager/test/e2e/installplan_e2e_test.go:2700

meandurationsec: 189.2262245

classname: End-to-end
name: 'CRD Versions blocks a CRD upgrade that could cause data loss'
counts: 1
details:

/home/runner/work/operator-lifecycle-manager/operator-lifecycle-manager/test/e2e/crd_e2e_test.go:178
Unexpected error:
    <*errors.errorString | 0xc0001f4280>: {
        s: "timed out waiting for the condition",
    }
    timed out waiting for the condition
occurred
/home/runner/work/operator-lifecycle-manager/operator-lifecycle-manager/test/e2e/crd_e2e_test.go:286

meandurationsec: 320.09633

classname: End-to-end
name: 'Garbage collection for dependent resources when a bundle with configmap
and secret objects is installed when the CSV is deleted OLM should delete
the associated configmap and secret'
counts: 1
details:

/home/runner/work/operator-lifecycle-manager/operator-lifecycle-manager/test/e2e/gc_e2e_test.go:296
could not get installplan at complete phase
Unexpected error:
    <*errors.errorString | 0xc0001f4280>: {
        s: "timed out waiting for the condition",
    }
    timed out waiting for the condition
occurred
/home/runner/work/operator-lifecycle-manager/operator-lifecycle-manager/test/e2e/gc_e2e_test.go:341

meandurationsec: 311.08044

classname: End-to-end
name: 'CRD Versions creates v1 CRDs with a v1 schema successfully'
counts: 1
details:

/home/runner/work/operator-lifecycle-manager/operator-lifecycle-manager/test/e2e/crd_e2e_test.go:106
Unexpected error:
    <*errors.errorString | 0xc0001f4280>: {
        s: "timed out waiting for the condition",
    }
    timed out waiting for the condition
occurred
/home/runner/work/operator-lifecycle-manager/operator-lifecycle-manager/test/e2e/crd_e2e_test.go:164

meandurationsec: 322.140323

classname: End-to-end
name: 'Subscription creation in case of transferring providedAPIs'
counts: 1
details:

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

	Error Trace:	subscription_e2e_test.go:1921
	            				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:
	            	clusterserviceversions.operators.coreos.com "nginx-new-a" not found

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

meandurationsec: 67.779862
skippedtests:

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: 23.093505
classname: End-to-end
name: 'Catalog image update'
counts: 1
details: []
meandurationsec: 0.279937
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: 11.284437
classname: End-to-end
name: 'Subscription updates existing install plan'
counts: 1
details: []
meandurationsec: 0.018524

awgreene · 2020-08-08T00:58:23Z

/retest

awgreene · 2020-08-08T04:29:02Z

/retest

awgreene · 2020-08-08T20:18:36Z

/retest

openshift-bot · 2020-08-09T12:04:10Z

/retest