operator: sync the cloud config in bootstrap and cluster mode #591

abhinavdahiya · 2019-03-28T03:22:24Z

- What I did

Use the cloud config defined in openshift/api#245 & openshift/installer#1479 to make sure mco can use it in both bootstrap and cluster.

Also updated the internal templates to write the cloudconfig to /etc/kubernetes/cloud.conf so that kubelets on nodes can use it.

- How to verify it

- Description for the changelog

Sync the cloud config to kubelet on nodes.

/cc @staebler @derekwaynecarr

runcom · 2019-03-28T08:25:57Z

/hold

is this fixing a bug and has clearance to go in?

abhinavdahiya · 2019-04-22T16:30:30Z

This PR is required for Azure work to proceed. as kubelets for Azure need to use this cloudconfig.

crawford · 2019-04-22T16:47:13Z

@runcom This can be merged as long as you are confident it won't affect existing functionality.

cmd/machine-config-operator/bootstrap.go

cgwalters · 2019-04-22T19:26:59Z

pkg/operator/operator.go

@@ -421,6 +430,18 @@ func (optr *Operator) getCAsFromConfigMap(namespace, name, key string) ([]byte,
 	}
 }

+func (optr *Operator) getCloudConfigFromConfigMap(namespace, name, key string) (string, error) {
+	cm, err := optr.kubeClient.CoreV1().ConfigMaps(namespace).Get(name, metav1.GetOptions{})


We have optr.clusterCmLister.ConfigMaps(namespace) now.

cgwalters · 2019-04-22T19:28:03Z

Only one minor nit, otherwise looks sane to me.

pkg/controller/template/render.go

runcom · 2019-04-22T19:34:51Z

pkg/controller/template/test_data/templates/master/01-master-kubelet/aws/units/kubelet.service

@@ -25,7 +25,7 @@ contents: |
        --client-ca-file=/etc/kubernetes/ca.crt \
        --cloud-provider=aws \
        --volume-plugin-dir=/etc/kubernetes/kubelet-plugins/volume/exec \
-        \
+         \


because of change here

pkg/controller/template/render.go

runcom · 2019-04-22T19:38:16Z

Haven't fully reviewed this but looks rightly wrapped to not affect other stuff in the MCO. @cgwalters @kikisdeliveryservice wdyt?

kikisdeliveryservice · 2019-04-22T22:24:08Z

confirming that in CI seeing cloud.conf refs in form:

                       "files": [
                            {
                                "contents": {
                                    "source": "data:,",
                                    "verification": {}
                                },
                                "filesystem": "root",
                                "mode": 420,
                                "path": "/etc/kubernetes/cloud.conf"
                            },

in 01-master-kubelet and 01-worker-kubelet that end up in rendered-master & rendered-worker configs

that CI run failed for other reasons, awaiting completed test runs.

awesomenix · 2019-04-23T19:49:23Z

/retest

The cloud provider config is optionaly present on the bootstrap node based on the user choice or platform. When the `.spec.cloudConfig` [1] is set, that requires the cloud provider config file to be provided. In cluster operator uses the same field from `infrastructure.config.openshift.io/cluster` object to selectively fetch the cloud config configmap reference. [1]: openshift/api#245

When the `.CloudProviderConfig` is non empty, - the contents must be stored at `/etc/kubernetes/cloud.conf` - and the kubelet service must configure kubelet to include `--cloud-config=/etc/kubernetes/cloud.conf` flag [1] [1]: https://kubernetes.io/docs/reference/command-line-tools-reference/kubelet/#options

abhinavdahiya · 2019-04-24T00:14:43Z

rebased around #648

kikisdeliveryservice · 2019-04-24T17:03:46Z

/approve

runcom · 2019-04-24T17:19:29Z

/lgtm

openshift-ci-robot · 2019-04-24T17:19:39Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinavdahiya, kikisdeliveryservice, runcom

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~~ [kikisdeliveryservice,runcom]

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

runcom · 2019-04-24T17:24:38Z

/hold cancel

abhinavdahiya · 2019-04-24T18:42:22Z

e2e-aws: failed due to flake :(

Failing tests:

[Feature:Builds][Conformance] oc new-app  should succeed with a --name of 58 characters [Suite:openshift/conformance/parallel/minimal]

/retest

derekwaynecarr · 2019-04-24T19:49:08Z

this looks fine to me, we will need to handle migration when external cloud controller managers are available when kubelets no longer need this conf file. that is a few releases away.

…e path Catching up to MCO supporting reading the cloud conf file on bootstrap node only when the infrastructures.config.openshift.io has the cloud provider config location set [1] [1]: openshift/machine-config-operator#591

PR [1] introduced wrong decoding for the cloud conf configmap file. It was using the openshift/client-go 's config scheme to decode ConfigMap resource when it should have been using the k8s.io/client-go's kubernetes scheme [1]: openshift#591

openshift-ci-robot requested review from derekwaynecarr and staebler March 28, 2019 03:22

openshift-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Mar 28, 2019

abhinavdahiya force-pushed the plumb_cloud_config branch from 1f93cf4 to 5be647a Compare March 28, 2019 05:52

openshift-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Mar 28, 2019

abhinavdahiya force-pushed the plumb_cloud_config branch from 5be647a to 3e6181f Compare April 22, 2019 16:28

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 22, 2019

abhinavdahiya force-pushed the plumb_cloud_config branch from 3e6181f to 3ca7166 Compare April 22, 2019 17:51

kikisdeliveryservice reviewed Apr 22, 2019

View reviewed changes

cmd/machine-config-operator/bootstrap.go Show resolved Hide resolved

cgwalters reviewed Apr 22, 2019

View reviewed changes

kikisdeliveryservice reviewed Apr 22, 2019

View reviewed changes

pkg/controller/template/render.go Outdated Show resolved Hide resolved

runcom reviewed Apr 22, 2019

View reviewed changes

kikisdeliveryservice reviewed Apr 22, 2019

View reviewed changes

pkg/controller/template/render.go Outdated Show resolved Hide resolved

abhinavdahiya force-pushed the plumb_cloud_config branch from 3ca7166 to 2425b71 Compare April 22, 2019 20:47

abhinavdahiya added 3 commits April 23, 2019 17:09

template/test_data: update to test the non-empty cloudproviderconfig

8251247

abhinavdahiya force-pushed the plumb_cloud_config branch from 2425b71 to 8251247 Compare April 24, 2019 00:14

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

openshift-ci-robot assigned runcom Apr 24, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 24, 2019

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 24, 2019

openshift-merge-robot merged commit 2bd29e5 into openshift:master Apr 24, 2019

abhinavdahiya mentioned this pull request Apr 25, 2019

data/bootstrap: update the mco bootstrap to include the cloudconf file path openshift/installer#1679

Merged

kikisdeliveryservice mentioned this pull request Apr 25, 2019

Cloud provider conf #177

Closed

abhinavdahiya mentioned this pull request Apr 25, 2019

operator/bootstrap: fix the decoding scheme for cloud conf configmap #666

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

operator: sync the cloud config in bootstrap and cluster mode #591

operator: sync the cloud config in bootstrap and cluster mode #591

abhinavdahiya commented Mar 28, 2019

runcom commented Mar 28, 2019

abhinavdahiya commented Apr 22, 2019

crawford commented Apr 22, 2019

cgwalters Apr 22, 2019

abhinavdahiya Apr 22, 2019

cgwalters commented Apr 22, 2019

runcom Apr 22, 2019

abhinavdahiya Apr 22, 2019

runcom commented Apr 22, 2019

kikisdeliveryservice commented Apr 22, 2019 •

edited

Loading

awesomenix commented Apr 23, 2019

abhinavdahiya commented Apr 24, 2019

kikisdeliveryservice commented Apr 24, 2019

runcom commented Apr 24, 2019

openshift-ci-robot commented Apr 24, 2019

runcom commented Apr 24, 2019

abhinavdahiya commented Apr 24, 2019

derekwaynecarr commented Apr 24, 2019

operator: sync the cloud config in bootstrap and cluster mode #591

operator: sync the cloud config in bootstrap and cluster mode #591

Conversation

abhinavdahiya commented Mar 28, 2019

runcom commented Mar 28, 2019

abhinavdahiya commented Apr 22, 2019

crawford commented Apr 22, 2019

cgwalters Apr 22, 2019

Choose a reason for hiding this comment

abhinavdahiya Apr 22, 2019

Choose a reason for hiding this comment

cgwalters commented Apr 22, 2019

runcom Apr 22, 2019

Choose a reason for hiding this comment

abhinavdahiya Apr 22, 2019

Choose a reason for hiding this comment

runcom commented Apr 22, 2019

kikisdeliveryservice commented Apr 22, 2019 • edited Loading

awesomenix commented Apr 23, 2019

abhinavdahiya commented Apr 24, 2019

kikisdeliveryservice commented Apr 24, 2019

runcom commented Apr 24, 2019

openshift-ci-robot commented Apr 24, 2019

runcom commented Apr 24, 2019

abhinavdahiya commented Apr 24, 2019

derekwaynecarr commented Apr 24, 2019

kikisdeliveryservice commented Apr 22, 2019 •

edited

Loading