Provider image substitution by Danil-Grigorev · Pull Request #29 · openshift/cluster-cloud-controller-manager-operator

Danil-Grigorev · 2021-03-25T15:43:27Z

This PR allows operator to dynamically configure image in provisioned resources such as CCM Deployment.

This is done based on assumption that every CCM controller will be in a named container containing controller substring in it's name. All those containers images will be managed by operator, others will remain unchanged from initial manifest.

Operator is now populating Namespace for each resource it provisions. Cluster-scoped resources are also getting this namespace value, which is ok as it is not accounted and is not stored by API server on apply. This allows two operators to manage equivalent set of resources in different namespaces without clashes.

Testing

JoelSpeed · 2021-03-25T16:55:29Z

+var (
+	// commonControllerNamingSubstringPattern contains a substring which all CCM controller container names should have
+	// to be substituted with image from config
+	commonControllerNamingSubstringPattern = "controller"


I think the pattern here is good, might be better to use something explicit though, eg, if we have a list of predefined allowed values cloud-controller-manager, node-controller-manager etc that the manifests must obide by? WDYT?

We need some sort of CI task which will check that only those names are on containers and other subresources all the time.

JoelSpeed · 2021-03-25T16:56:50Z

+	for i, objectTemplate := range templates {
+		templateCopy := objectTemplate
+
+		// Set namespaces for all object. Namespace on cluster-wide objects is stripped by API server and is not applied


Good comment

Danil-Grigorev · 2021-04-20T16:22:10Z

/test e2e-openstack

JoelSpeed

I think we need to clean this up a bit before we merge. I'm concerned about the side effects of some of the functions and also premature optimisations we are making. Let's keep the code clean now and worry about future changes later when we get to that point

JoelSpeed

In general, would be good to see some more unit testing for the methods we are adding here. There are lots of small parts (which is good!) but at the moment their functionality is only tested as a whole. If we introduce more scoped down unit testing this will help us to prove that individual functions are doing what we are intending them to do and in the long run will help identifying bugs (add test cases to specific functions and check the outputs)

Danil-Grigorev · 2021-04-26T09:32:19Z

/retest

Danil-Grigorev · 2021-04-26T12:43:38Z

/retest

Danil-Grigorev · 2021-04-27T01:40:18Z

/retest

Danil-Grigorev · 2021-04-27T08:37:06Z

https://prow.ci.openshift.org/view/gs/origin-ci-test/pr-logs/pull/openshift_cluster-cloud-controller-manager-operator/29/pull-ci-openshift-cluster-cloud-controller-manager-operator-master-e2e-aws-ccm/1386857755955957760 passed CI, will record it here as planing to address missed comment.

JoelSpeed

Couple of nits but otherwise LGTM

JoelSpeed

/approve

Thanks Danil, looks great!

Danil-Grigorev · 2021-04-29T15:41:34Z

/retetst

elmiko

this mostly makes sense to me, i have a couple questions though

elmiko · 2021-04-29T19:40:19Z

+type imagesReference struct {
+	CloudControllerManagerAWS       string `json:"cloudControllerManagerAWS"`
+	CloudControllerManagerOpenStack string `json:"cloudControllerManagerOpenStack"`
+}


i'm just curious, will a cluster ever use multiple images from this reference?

Yes, this is possible, there is definitely going to be a kube-rbac-proxy image here once we will start investigating metrics support.

elmiko · 2021-04-29T19:45:46Z

+}
+
+func getImagesFromJSONFile(filePath string) (imagesReference, error) {
+	data, err := ioutil.ReadFile(filepath.Clean(filePath))


are these file paths loaded as volume mounts in the container?

Yes, they are loaded from a mounted config map introduced in #31

elmiko

looking mostly good Danil, i have one small suggestion and a question about the tests

openshift-ci · 2021-05-03T12:06:37Z

@Danil-Grigorev: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
ci/prow/e2e-aws-ccm	`e2a53a0`	link	`/test e2e-aws-ccm`

Full PR test history. Your PR dashboard.

Details

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.

elmiko

thanks Danil!
/lgtm

openshift-ci-robot · 2021-05-03T14:30:47Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: elmiko, JoelSpeed

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [JoelSpeed,elmiko]

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

openshift-bot · 2021-05-03T14:59:13Z

/retest

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

openshift-bot · 2021-05-03T15:47:23Z

/retest

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

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 Mar 25, 2021

openshift-ci-robot requested review from JoelSpeed and mdbooth March 25, 2021 15:43

JoelSpeed reviewed Mar 25, 2021

View reviewed changes

Danil-Grigorev force-pushed the image-substitution branch from ef0f516 to 26457f0 Compare April 20, 2021 15:58

Danil-Grigorev changed the title ~~[WIP] Provider image substitution~~ Provider image substitution Apr 20, 2021

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 20, 2021

Danil-Grigorev force-pushed the image-substitution branch from 26457f0 to 8ad70be Compare April 21, 2021 09:08

JoelSpeed reviewed Apr 21, 2021

View reviewed changes

Comment thread controllers/clusteroperator_controller.go Outdated

Comment thread controllers/config.go Outdated

Comment thread controllers/substitution.go

Danil-Grigorev force-pushed the image-substitution branch 3 times, most recently from 8c032ed to 96f8c01 Compare April 21, 2021 18:23

JoelSpeed reviewed Apr 22, 2021

View reviewed changes

Comment thread controllers/clusteroperator_controller.go Outdated

Danil-Grigorev force-pushed the image-substitution branch 2 times, most recently from 4f18105 to c7c1fcb Compare April 23, 2021 13:46

JoelSpeed reviewed Apr 27, 2021

View reviewed changes

Comment thread controllers/config.go Outdated

Comment thread controllers/config_test.go Outdated

Danil-Grigorev force-pushed the image-substitution branch from c3ed167 to ee4773e Compare April 29, 2021 04:33

Danil-Grigorev requested a review from JoelSpeed April 29, 2021 04:33

Danil-Grigorev force-pushed the image-substitution branch from ee4773e to 6484d5c Compare April 29, 2021 04:37

JoelSpeed reviewed Apr 29, 2021

View reviewed changes

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 29, 2021

elmiko reviewed Apr 29, 2021

View reviewed changes

Danil-Grigorev force-pushed the image-substitution branch from 6484d5c to f2de5dc Compare April 30, 2021 09:00

Danil-Grigorev requested a review from elmiko April 30, 2021 11:23

elmiko reviewed Apr 30, 2021

View reviewed changes

Comment thread controllers/clusteroperator_controller_test.go

elmiko reviewed Apr 30, 2021

View reviewed changes

Add config integration and image substitution in main sync loop

e2a53a0

Danil-Grigorev force-pushed the image-substitution branch from a153b4f to e2a53a0 Compare May 3, 2021 08:11

Danil-Grigorev requested a review from elmiko May 3, 2021 08:12

elmiko approved these changes May 3, 2021

View reviewed changes

openshift-ci-robot assigned elmiko May 3, 2021

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

openshift-merge-robot merged commit e4fbd6c into openshift:master May 3, 2021

Conversation

Danil-Grigorev commented Mar 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JoelSpeed Mar 25, 2021

Choose a reason for hiding this comment

Uh oh!

Danil-Grigorev Apr 21, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

JoelSpeed Mar 25, 2021

Choose a reason for hiding this comment

Uh oh!

Danil-Grigorev commented Apr 20, 2021

Uh oh!

JoelSpeed left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JoelSpeed left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Danil-Grigorev commented Apr 26, 2021

Uh oh!

Danil-Grigorev commented Apr 26, 2021

Uh oh!

Danil-Grigorev commented Apr 27, 2021

Uh oh!

Danil-Grigorev commented Apr 27, 2021

Uh oh!

JoelSpeed left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

JoelSpeed left a comment

Choose a reason for hiding this comment

Uh oh!

Danil-Grigorev commented Apr 29, 2021

Uh oh!

elmiko left a comment

Choose a reason for hiding this comment

Uh oh!

elmiko Apr 29, 2021

Choose a reason for hiding this comment

Uh oh!

Danil-Grigorev Apr 30, 2021

Choose a reason for hiding this comment

Uh oh!

elmiko Apr 29, 2021

Choose a reason for hiding this comment

Uh oh!

Danil-Grigorev Apr 30, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

elmiko left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ci Bot commented May 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elmiko left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ci-robot commented May 3, 2021

Uh oh!

openshift-bot commented May 3, 2021

Uh oh!

Danil-Grigorev commented Mar 25, 2021 •

edited

Loading

openshift-ci Bot commented May 3, 2021 •

edited

Loading