Separate loop and plugin control

[cheftako]

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]

/assign @wlan0

[cheftako]

/assign @thockin

[wlan0]

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

[thockin]

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?

[luxas]

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

[wlan0]

@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]

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]

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

[thockin]

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

[cheftako]

/test pull-kubernetes-unit

[k8s-ci-robot]

@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]

/retest

[wlan0]

/approve no-issue

[wlan0]

/assign @thockin

Ready for your review

[dims]

/assign @mikedanese

/approve

[mikedanese]

/approve

[dims]

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

/lgtm

[k8s-ci-robot]

[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]

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]

@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:

1. 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
2. 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.