Add cloud-controller-manager support for Kubernetes cluster #1584

karataliu · 2017-10-12T05:00:53Z

What this PR does / why we need it:
Kubernetes v1.8 supports cloud-controller-manager which runs cloud provider related logic, and it is planned in v1.10 that using cloud-controller-manager becomes the only way to interact with clouds.

This PR adds an option 'UseCloudControllerManager' in apimodel for Kubernetes v1.8 version. If turned on, the cluster will be using cloud controller manager mode.

Currently it uses the common image 'cloud-controller-manager-amd64:v1.8.0' which includes all cloud provider implementation. Will later switch to azure dedicated cloud-controller-manager image once ready.

One TODO item is RBAC support, which pends on upstream Kubernetes#53511 .

Please find more details here: kubernetes/kubernetes#50752

Which issue this PR fixes

Special notes for your reviewer:

Release note:

Add cloud-controller-manager support for Kubernetes cluster

karataliu · 2017-10-17T01:23:50Z

cc @anhowe @jdumars This PR is for supporting standalone Azure cloud provider from acs-engine side. Could you please take a look? Thanks!

colemickens · 2017-10-17T22:53:10Z

parts/kubernetesmaster-cloud-controller-manager.yaml

+        - "--cloud-provider=azure"
+        - "--cloud-config=/etc/kubernetes/azure.json"
+        - "--leader-elect=true"
+# TODO: RBAC support


What's needed or outstanding for enabling RBAC?

This is actually meant for enabling 'use-service-account-credentials', which does not work now, pending on kubernetes#53511

colemickens · 2017-10-17T22:54:22Z

parts/kubernetesmastercustomdata.yml

+    sed -i "s|<kubernetesCcmImageSpec>|{{WrapAsVariable "kubernetesCcmImageSpec"}}|g; s|<masterFqdnPrefix>|{{WrapAsVariable "masterFqdnPrefix"}}|g; s|<allocateNodeCidrs>|{{WrapAsVariable "allocateNodeCidrs"}}|g; s|<kubeClusterCidr>|{{WrapAsVariable "kubeClusterCidr"}}|g; s|<kubernetesCtrlMgrRouteReconciliationPeriod>|{{WrapAsVariable "kubernetesCtrlMgrRouteReconciliationPeriod"}}|g" \
+      /etc/kubernetes/manifests/cloud-controller-manager.yaml
+
+    sed -i "/--\(cloud-config\|cloud-provider\|route-reconciliation-period\)=/d" \


Is this stripping off cloud-provider and cloud-config from apiserver? Doesn't that also need to be stripped off of controller-manager too?

Yeah, both need to be updated. LIne323~324 is for kube-controller-manager, Line325~326 is for apiserver.

colemickens · 2017-10-17T22:55:36Z

parts/kubernetesagentcustomdata.yml

+    CLOUD_PROVIDER=external
+{{else}}
+    CLOUD_PROVIDER=azure
+{{end}}

 - path: "/etc/systemd/system/kubelet.service"


Is there no change needed for kubelet.service to include the --provider-id flag?

provider-id can be detected now:
kubernetes/kubernetes#50730
https://github.com/kubernetes/kubernetes/blob/master/pkg/cloudprovider/providers/azure/azure_instances.go#L103

colemickens · 2017-10-17T23:09:29Z

What's the status of this @karataliu? What level of functionality does the cluster have when this is enabled? I'm guessing volumes don't work, but does networking + load balancing?

I'm trying to understand how many of the items mentioned in the upstream issue need to happen before CCM + Azure is functional for most non-volume use-cases. Thanks.

karataliu · 2017-10-18T05:39:29Z

@colemickens Yes, volumes feature does not work. I've tried with e2e conformance tests and most cases would pass excepted for the volume ones.

The upstream issue is more about migration work, Azure provider running with cloud-controller-manager could already work on a cluster. But since the whole feature is under beta stage (kubernetes/enhancements#88), it is not recommended for production use.

colemickens · 2017-10-26T23:16:10Z

Thanks for the info. Change seems good to me. I made a similar change in Tectonic-Installer and everything seemed to work (at least,the sunset I expected to work). (Granted the network setup in Tectonic-Installer is Calico and not kubelet...)

…

On Oct 17, 2017 10:39 PM, "Dong Liu" ***@***.***> wrote: @colemickens <https://github.com/colemickens> Yes, volumes feature does not work. I've tried with e2e conformance tests and most cases would pass excepted for the volume ones. The upstream issue is more about migration work, Azure provider running with cloud-controller-manager could already work on a cluster. But since the whole feature is under beta stage (kubernetes/enhancements#88 <kubernetes/enhancements#88>), it is not recommended for production use. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1584 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAT9dG4RzGC0mOAm_QQbud8UANLZ20_Pks5stY8VgaJpZM4P2eEO> .

karataliu · 2017-11-09T07:11:03Z

Could someone please take a look? Thanks.

colemickens · 2017-11-09T23:44:42Z

(FWIW, this looks good to me. And in a few weeks hopefully, there should be alpha support for CCM under hyperkube as well, so there won't need to be a separate image/process... at least until CCM totally splits out of the main repo and is yanked back out of hyperkube, but that could be a while).

karataliu · 2017-11-10T09:37:20Z

@colemickens Thanks, I saw the ongoing work for hyperkube incorporating CCM.
For the change here, a dedicate parameter for CCM image is still required as it could be set to an out of band CCM release. Also during CI test, a new CCM image is built and tested every time.

jackfrancis · 2017-11-15T17:41:54Z

Thanks for your patience @karataliu, and apologies for the rebase needs. Not a few things have changed in the conflicting surface area, I'd be happy to spawn a new PR with this work if that's easier.

karataliu · 2017-11-20T07:20:15Z

@jackfrancis Updated, please take a look. Thanks.

jackfrancis · 2017-11-20T17:29:55Z

Thanks @karataliu ! Running E2E now.

jackfrancis

This looks good. The only suggestion I'd make is to make the "enable/disable" property a *bool, so that we can more easily default to true at a later date. See this PR for an example:

https://github.com/Azure/acs-engine/pull/1763/files

If I had to do that PR again I'd make the return value here reference a boolean constant, and not a boolean literal, for easier future maintenance:

https://github.com/Azure/acs-engine/pull/1763/files#diff-ff04be9832c108c30374068a6cfe4953R792

e.g.:

if <enableThing) == nil {
    return DefaultThingEnabled

Let me know if that doesn't make sense! Thanks again!

karataliu · 2017-11-21T08:27:41Z

In fact the default value should be dependent on k8s version, and as planned for 1.10+ release it should only have true value.

Anyway it is good to switch to ternary here, I've updated accordingly.

jackfrancis

lgtm, thanks @karataliu !

karataliu force-pushed the accm18n branch 3 times, most recently from 6d0d214 to 34651f8 Compare October 16, 2017 03:24

jchauncey added area/engine area/templates needs approval size/medium waiting for ci labels Oct 17, 2017

colemickens reviewed Oct 17, 2017

View reviewed changes

jackfrancis added awaiting review and removed needs approval labels Nov 6, 2017

karataliu force-pushed the accm18n branch from 34651f8 to 16febc4 Compare November 7, 2017 01:51

Add cloud-controller-manager support

d7fbba8

karataliu force-pushed the accm18n branch from 16febc4 to d7fbba8 Compare November 20, 2017 07:19

jackfrancis suggested changes Nov 20, 2017

View reviewed changes

Make UseCloudControllerManager pointer

7670815

jackfrancis approved these changes Nov 21, 2017

View reviewed changes

jackfrancis merged commit 6339a0b into Azure:master Nov 21, 2017

ghost removed the awaiting review label Nov 21, 2017

karataliu mentioned this pull request Mar 30, 2018

Add support of Azure standard load balancer and public IP kubernetes/kubernetes#61884

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add cloud-controller-manager support for Kubernetes cluster #1584

Add cloud-controller-manager support for Kubernetes cluster #1584

karataliu commented Oct 12, 2017

karataliu commented Oct 17, 2017

colemickens Oct 17, 2017

karataliu Oct 18, 2017

colemickens Oct 17, 2017

karataliu Oct 18, 2017 •

edited

Loading

colemickens Oct 17, 2017

karataliu Oct 18, 2017

colemickens commented Oct 17, 2017

karataliu commented Oct 18, 2017

colemickens commented Oct 26, 2017 via email

karataliu commented Nov 9, 2017

colemickens commented Nov 9, 2017

karataliu commented Nov 10, 2017

jackfrancis commented Nov 15, 2017

karataliu commented Nov 20, 2017

jackfrancis commented Nov 20, 2017

jackfrancis left a comment

karataliu commented Nov 21, 2017

jackfrancis left a comment

Add cloud-controller-manager support for Kubernetes cluster #1584

Add cloud-controller-manager support for Kubernetes cluster #1584

Conversation

karataliu commented Oct 12, 2017

karataliu commented Oct 17, 2017

colemickens Oct 17, 2017

Choose a reason for hiding this comment

karataliu Oct 18, 2017

Choose a reason for hiding this comment

colemickens Oct 17, 2017

Choose a reason for hiding this comment

karataliu Oct 18, 2017 • edited Loading

Choose a reason for hiding this comment

colemickens Oct 17, 2017

Choose a reason for hiding this comment

karataliu Oct 18, 2017

Choose a reason for hiding this comment

colemickens commented Oct 17, 2017

karataliu commented Oct 18, 2017

colemickens commented Oct 26, 2017 via email

karataliu commented Nov 9, 2017

colemickens commented Nov 9, 2017

karataliu commented Nov 10, 2017

jackfrancis commented Nov 15, 2017

karataliu commented Nov 20, 2017

jackfrancis commented Nov 20, 2017

jackfrancis left a comment

Choose a reason for hiding this comment

karataliu commented Nov 21, 2017

jackfrancis left a comment

Choose a reason for hiding this comment

karataliu Oct 18, 2017 •

edited

Loading