Separate loop and plugin control #52371

cheftako · 2017-09-12T23:01:48Z

What this PR does / why we need it: Separate loop and plugin control in the kube-controller-manager.
Adding an "--external-plugin" flag to specify a plugin to load when
cloud-provider is set to "external". Flag has no effect currently
when the cloud-provider is not set to external. The expectation is
that the cloud provider and external plugin flags would go away once
all cloud providers are on stage 2 cloud-controller-manager solutions.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #52369

Special notes for your reviewer:

Release note:

cheftako · 2017-09-12T23:02:04Z

/assign @wlan0

cheftako · 2017-09-12T23:02:16Z

/assign @thockin

wlan0 · 2017-09-12T23:08:25Z

cmd/kube-controller-manager/app/options/options.go

@@ -131,6 +131,7 @@ func (s *CMServer) AddFlags(fs *pflag.FlagSet, allControllers []string, disabled
 	fs.BoolVar(&s.UseServiceAccountCredentials, "use-service-account-credentials", s.UseServiceAccountCredentials, "If true, use individual service account credentials for each controller.")
 	fs.StringVar(&s.CloudProvider, "cloud-provider", s.CloudProvider, "The provider for cloud services.  Empty string for no provider.")
 	fs.StringVar(&s.CloudConfigFile, "cloud-config", s.CloudConfigFile, "The path to the cloud provider configuration file.  Empty string for no configuration file.")
+	fs.StringVar(&s.ExternalPlugin, "external-plugin", s.ExternalPlugin, "The plugin to use when cloud provider external. Can be empty, should only be set when cloud-provider is external.")


Volume plugin to use when Cloud provider is external. Eg.
--external-plugin=aws (for ebs )
--external-plugin=gce (for pd) etc.

Shouldn't the name say "volume" if it is about volumes? But it's not a volume plugin, it's a cloud plugin. Can we just make this extra extra clear?

I was very confused as well. What @thockin said.

wlan0 · 2017-09-12T23:18:48Z

@cheftako Thanks for the PR. Initializing external plugin using the cloud initializer will start the cloud dependent controllers. If the cloudprovider is not null, then the controllers will start.

For eg. The service controller uses the cloud==nil check to decide if it should run or not - https://github.com/kubernetes/kubernetes/blob/master/pkg/controller/service/service_controller.go#L210

With the current PR, cloud variable will not be nil if we set external-plugin

I believe the feature we want is for the volume controller only to be started with that cloudprovider if --external-plugin is set right?

luxas

What is actually different when you specify --external-plugin? To me it seems like the behavior of the kcm would be the same...

Can you please explain further why this is needed?

cheftako · 2017-09-13T20:09:10Z

Hi @luxas as @wlan0 mentioned there is a problem in this fix which I am working on fixing but may be making it tougher to understand the point of the fix. As the cloud provider initiative (https://github.com/kubernetes/community/blob/master/contributors/design-proposals/cloud-provider-refactoring.md) proceeds kcm should not be running the following controller loops, node, route, service (and volume). We have broken out the first 3 loops from the kcm to the ccm based on the --cloud-provider flag being set to external. Volume is in parentheses because we are still a ways from being ready to break that out. However the cloud provider flag also controls initializing the cloud plugin. Without a working cloud plugin the volume controller loop breaks for volumes of type aws_ebs, gce_pd etc. So the idea is to separate control of which loops are being run from which plugin is being loaded. As @wlan0 points out I missed that we used the existence of the plugin to control the execution of the loops rather than controlling it directly off of the flag itself.

luxas · 2017-09-13T20:33:32Z

cc @shashidharatd This will make us able to use the ccm for federation and kube-anywhere :)

thockin · 2017-09-20T03:50:53Z

Assign back to me once it is LGTM'ed and ready for approval.

cheftako · 2017-11-22T18:36:20Z

/test pull-kubernetes-unit

@wlan0

Seperate loop and plugin control in the kube-controller-manager. Adding an "--external-plugin" flag to specify a plugin to load when cloud-provider is set to "external". Flag has no effect currently when the cloud-provider is not set to external. The expectation is that the cloud provider and external plugin flags would go away once all cloud providers are on stage 2 cloud-controller-manager solutions. Managing the control loops more directly based on start up flags. Addressing issue brought up by @wlan0 Switched to using the main node controller in CCM. Changes to enable full NodeController to start in CCM. Fix related tests. Unifying some common code between KCM and CCM. Fix related tests and comments. Folded in feedback from @jhorwit2 and @wlan0

k8s-ci-robot · 2017-12-18T19:19:35Z

@cheftako: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-e2e-gce-bazel	b72461a777af484081c41a7f6428ad8d7cb4d292	link	`/test pull-kubernetes-e2e-gce-bazel`
pull-kubernetes-e2e-gce-gpu	b72461a777af484081c41a7f6428ad8d7cb4d292	link	`/test pull-kubernetes-e2e-gce-gpu`
pull-kubernetes-e2e-gce-etcd3	b72461a777af484081c41a7f6428ad8d7cb4d292	link	`/test pull-kubernetes-e2e-gce-etcd3`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

wlan0 · 2017-12-18T19:25:06Z

/retest

wlan0 · 2017-12-18T19:25:15Z

/approve no-issue

wlan0 · 2017-12-18T19:25:28Z

/assign @thockin

Ready for your review

dims · 2017-12-19T13:34:08Z

/assign @mikedanese

/approve

mikedanese · 2017-12-19T17:46:24Z

/approve

dims · 2017-12-19T18:30:31Z

Let's ship it! before we have to rebase :)

/lgtm

k8s-ci-robot · 2017-12-19T18:30:37Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cheftako, dims, mikedanese, wlan0

Associated issue: #52369

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

Needs approval from an approver in each of these OWNERS Files:

~~cmd/OWNERS~~ [mikedanese]
~~pkg/apis/componentconfig/OWNERS~~ [mikedanese]
~~pkg/controller/node/OWNERS~~ [mikedanese]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-github-robot · 2017-12-19T20:55:38Z

Automatic merge from submit-queue (batch tested with PRs 56716, 52371). If you want to cherry-pick this change to another branch, please follow the instructions here.

deads2k · 2018-01-15T17:29:25Z

@cheftako @lavalamp @sttts I think the intent of this pull is create a generic set of controller library code (https://github.com/kubernetes/kubernetes/blob/master/cmd/controller-manager/app/options/utils.go#L30), but a lot of controller specific code has leaked into it (https://github.com/kubernetes/kubernetes/blob/master/cmd/controller-manager/app/options/utils.go#L71-L74 as a for instance).

I think we should follow a pattern like we did for the genericapiserver of requiring only generic things be move into common code. Sometimes that forces other refactors to happen first, but it keeps the generic library purer and generally exposes more problems.

A few concrete items here need followups:

https://github.com/kubernetes/kubernetes/blob/master/cmd/controller-manager/app/options/utils.go#L51 is a specific type of config that include config for kube-controllers. It shouldn't be in a generic package
IncludeCloudLoops (https://github.com/kubernetes/kubernetes/pull/52371/files#diff-2aa9cfe6a01016c1679752b0fcfc9244R274) looks off. Rather than drive it that way, we have a way to enable and disable controllers and a way to eventually disable them by default. That would provide clean separation and a clear migration path forward.

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Sep 12, 2017

k8s-github-robot assigned yifan-gu and cjcullen Sep 12, 2017

k8s-ci-robot assigned wlan0 Sep 12, 2017

k8s-github-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Sep 12, 2017

k8s-ci-robot assigned thockin Sep 12, 2017

wlan0 reviewed Sep 12, 2017

View reviewed changes

wlan0 mentioned this pull request Sep 13, 2017

Migrate federation cluster bringup method to k8s-anywhere kubernetes/test-infra#3858

Closed

6 tasks

luxas reviewed Sep 13, 2017

View reviewed changes

thockin removed their assignment Sep 20, 2017

cheftako changed the title ~~Separate loop and plugin control~~ [WIP] Separate loop and plugin control Oct 6, 2017

k8s-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 Oct 6, 2017

cheftako force-pushed the external branch from 25d0285 to 7662765 Compare October 13, 2017 20:38

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 13, 2017

cheftako force-pushed the external branch from 7662765 to d2c5cbd Compare October 16, 2017 21:49

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 16, 2017

wlan0 mentioned this pull request Oct 16, 2017

add conceptual information behind CCM kubernetes/website#5928

Merged

cheftako force-pushed the external branch from d2c5cbd to b72461a Compare October 17, 2017 00:58

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 17, 2017

cheftako force-pushed the external branch from b72461a to cc55de5 Compare October 28, 2017 00:39

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 21, 2017

cheftako force-pushed the external branch from aaac376 to bbdb827 Compare November 21, 2017 21:58

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 21, 2017

cheftako force-pushed the external branch from bbdb827 to 8a6a07f Compare November 22, 2017 00:40

cheftako force-pushed the external branch from 8a6a07f to 0f920e8 Compare November 22, 2017 20:52

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 16, 2017

cheftako force-pushed the external branch from 0f920e8 to 070a7b5 Compare December 18, 2017 19:00

kubernetes deleted a comment from k8s-github-robot Dec 18, 2017

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 18, 2017

k8s-ci-robot assigned thockin Dec 18, 2017

k8s-ci-robot assigned mikedanese Dec 19, 2017

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 19, 2017

k8s-ci-robot assigned dims Dec 19, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 19, 2017

colemickens mentioned this pull request Dec 19, 2017

hack/quickstart: enable setting kubelet cloud provider separately from controller manager kubernetes-retired/bootkube#804

Closed

k8s-github-robot merged commit 5e4ff8a into kubernetes:master Dec 19, 2017

cheftako mentioned this pull request Dec 22, 2017

[WIP] Initial changes for cloud-provider with GCE. #50811

Closed

cheftako mentioned this pull request Jan 11, 2018

Adding the rough draft of the cloud-provider KEP document. kubernetes/community#1455

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Separate loop and plugin control #52371

Separate loop and plugin control #52371

cheftako commented Sep 12, 2017

cheftako commented Sep 12, 2017

cheftako commented Sep 12, 2017

wlan0 Sep 12, 2017

thockin Sep 15, 2017

luxas Sep 15, 2017

wlan0 commented Sep 12, 2017 •

edited

Loading

luxas left a comment

cheftako commented Sep 13, 2017

luxas commented Sep 13, 2017

thockin commented Sep 20, 2017

cheftako commented Nov 22, 2017

k8s-ci-robot commented Dec 18, 2017 •

edited

Loading

wlan0 commented Dec 18, 2017

wlan0 commented Dec 18, 2017

wlan0 commented Dec 18, 2017

dims commented Dec 19, 2017

mikedanese commented Dec 19, 2017

dims commented Dec 19, 2017

k8s-ci-robot commented Dec 19, 2017

k8s-github-robot commented Dec 19, 2017

deads2k commented Jan 15, 2018

Separate loop and plugin control #52371

Separate loop and plugin control #52371

Conversation

cheftako commented Sep 12, 2017

cheftako commented Sep 12, 2017

cheftako commented Sep 12, 2017

wlan0 Sep 12, 2017

Choose a reason for hiding this comment

thockin Sep 15, 2017

Choose a reason for hiding this comment

luxas Sep 15, 2017

Choose a reason for hiding this comment

wlan0 commented Sep 12, 2017 • edited Loading

luxas left a comment

Choose a reason for hiding this comment

cheftako commented Sep 13, 2017

luxas commented Sep 13, 2017

thockin commented Sep 20, 2017

cheftako commented Nov 22, 2017

k8s-ci-robot commented Dec 18, 2017 • edited Loading

wlan0 commented Dec 18, 2017

wlan0 commented Dec 18, 2017

wlan0 commented Dec 18, 2017

dims commented Dec 19, 2017

mikedanese commented Dec 19, 2017

dims commented Dec 19, 2017

k8s-ci-robot commented Dec 19, 2017

k8s-github-robot commented Dec 19, 2017

deads2k commented Jan 15, 2018

wlan0 commented Sep 12, 2017 •

edited

Loading

k8s-ci-robot commented Dec 18, 2017 •

edited

Loading