Controller Plugin framework #6520

markturansky · 2015-04-07T17:24:00Z

This is a sketch for discussion. I'm curious to know if this is a desirable direction for kube-controller-manager.

Goals:

Change controllers into plugins. Each is similar in implementation. Make a framework to run many plugins in kube-controller-manager. Plugin arch copied from VolumePlugin and NetworkPlugin.
Be ready to support @lavalamp's Controller Framework (Controller framework #5270)
Port 1 controller (i used EndpointsController) as an example.
Future enhancements:
Potentially change kube-controller-manager into a server to allow queries into its status. The manager can query each of its controllers and report back on each of them.
Allow graceful shutdown of all controllers via stop channels (Smith's Controller Framework has a similar stop mechanism)
Allow set/override default sync values (defaults set by plugins, override by CLI flags)
Refactor CLI sync flags to allow plugin to add these somehow, as opposed to explicitly adding each.

Optional: Potentially rename all controllers to something akin to "_Manager" or "_Synchronizer" to differentiate a process from a kind. Example: PersistentVolumeManager is the sync loop. PersistentVolumeController is a proposed Kind in the API that makes new PVs.

Genesis: I wrote a controller in which I thought I needed to reconcile 2 objects and didn't like the duplication required. It wasn't hard to genericize, after which I thought I'd take a shot at making all controllers work similarly. Porting endpoints_controller was very easy to do.

@thockin @bgrant0607

googlebot · 2015-04-07T17:24:02Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project, in which case you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please let us know the company's name.

bgrant0607 · 2015-04-07T17:58:15Z

We definitely intend to convert replication controller manager to a plugin. #3058 (comment)

satnam6502 · 2015-04-07T18:01:09Z

pkg/controller/endpoints/endpoints_mgr.go

+	return endpointsManagerName
+}
+
+func (plugin *endpointsManager) GetDefaultSyncPeriod() time.Duration {


Minor, but I think this should be DefaultSyncPeriod and likewise for the others below that start with Get.
http://golang.org/doc/effective_go.html#Getters

satnam6502 · 2015-04-07T20:28:26Z

I am on vacation for most of this week and for the rest of the time I have to focus on some non-Kubernetes work so I would prefer it if someone else reviewed this to avoid holding up this PR since I don't have the time in the coming days to do it justice.

markturansky · 2015-04-11T20:26:11Z

@thockin @lavalamp PTAL?

I refactored my plugins to use @lavalamp's new controller framework, including the graceful termination of all plugins via a stop channel.
A Controller can sync many objects, not just one. The Controller itself runs many of @lavalamp's control loops.
The unit test and working example watches 2 objects.
I removed Endpoints entirely to avoid distraction. The unit test has a fully working example.
I'd like to re-implement the PersistentVolumeClaimBinder in this framework.

lavalamp · 2015-04-13T01:00:14Z

I will try to take a look at this tomorrow.

thockin · 2015-04-13T20:55:23Z

Is this still RFC or should we update the title?

markturansky · 2015-04-13T20:57:02Z

Since I haven't gotten shotdown for turning controllers into plugins, I'll rename to WIP :)

thockin · 2015-04-13T21:14:10Z

pkg/controller/controller.go

+	"time"
+)
+
+// ControllerPlugin is an interface to controller plugins that can be run


I would put this definition in plugins.go.

Optional: Consider just calling it Plugin, which is more go-idiomatic. Volumes are named as such to disambiguate between multiple kinds of plugin. I don't think we need that here.

Moved and renamed.

NetworkPlugin doesn't follow this pattern, though its Host is named simply "Host".

k8s-bot · 2015-09-10T02:13:50Z

GCE e2e build/test passed for commit a1a7ae3e4f006d4f5d9e589070c3079d63fd9313.

k8s-bot · 2015-09-11T17:06:57Z

GCE e2e build/test failed for commit b7e72b84c19d7f2de398dfb1327c2aca9269ce44.

k8s-bot · 2015-09-14T17:25:47Z

GCE e2e build/test passed for commit eea4c3b.

lavalamp · 2015-09-14T20:43:54Z

pkg/controller/testing.go

+limitations under the License.
+*/
+
+package controller


Move to a separate fake package?

I can, but this follows the pattern in pkg/volume.

This is part of raising the code quality bar. Test-only code should not be in non-test-only packages. We're guilty of it all over the place and we need to stop.

Any other test package in the code can easily include the controller/testing pkg for the Fakes. I'll move this.

lavalamp · 2015-09-14T21:24:29Z

I'm not sure if I see the overall benefit to this. In kubelet, you have to have a way of plugging things in, and you have to be able to leverage that to calling out to other binaries when needed. But for controllers, there's no need for that later case, ever-- kube-controller-manager does not have to talk to frobber-controller-manager; if you want a 3rd party controller, you can just write one and run it as a separate binary, there's no need for a plug-in mechanism.

I'm not saying I'm against this, either, necessarily.

lavalamp · 2015-09-14T21:25:13Z

pkg/controller/plugins.go

+limitations under the License.
+*/
+
+package controller


This should be in a controller/plugin/manager package, IMO.

Cloudprovider, Network, and Volume all have their plugins.go and interfaces in the top level of the pkg. Why would this one be different?

Well... this one will be different because we'll do it right. :)

More seriously, I'm very focused lately on getting the package dependency
tree into a sane state. This means separating interfaces like this so that
plugin code doesn't need to import manager code and vice versa. The
interfaces need to be in common packages, but no actual code should be.

On Mon, Sep 14, 2015 at 7:55 PM, Mark Turansky notifications@github.com
wrote:

In pkg/controller/plugins.go
#6520 (comment):

+Copyright 2015 The Kubernetes Authors All rights reserved.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+

http://www.apache.org/licenses/LICENSE-2.0

+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package controller

Cloudprovider, Network, and Volume all have their plugins.go and
interfaces in the top level of the pkg. Why would this one be different?

—
Reply to this email directly or view it on GitHub
https://github.com/kubernetes/kubernetes/pull/6520/files#r39470075.

markturansky · 2015-09-15T02:52:05Z

@lavalamp thanks for all the feedback. I see a few good reasons for this PR. None earth shattering, but I think overall in the right direction.

controllermanager.go becomes smaller and simpler. It's currently all bespoke initialization of controllers line after line. Plugins would reduce that dramatically.
This PR provides a common runtime for all controllers and common start/stop mechanics. Controllers are already similar to each other. This would make them more so.
There's no way today of knowing what controllers are running and their health. The binary can be made into a server (more like kubelet) and it's status could be queried at runtime to view all controllers' health. Can't do that today. Might be useful to an admin running a cluster. Perhaps controllers can collect their own stats and provide them in Status.

That last one is difficult to do without the common runtime.

lavalamp · 2015-09-15T17:36:37Z

Yeah, I think all of those are fine reasons. I guess I'll just point out
that the motivation is different than the motivation for the volume/network
plugins.

On Mon, Sep 14, 2015 at 7:52 PM, Mark Turansky notifications@github.com
wrote:

@lavalamp https://github.com/lavalamp thanks for all the feedback. I
see a few good reasons for this PR. None earth shattering, but I think
overall in the right direction.

controllermanager.go becomes smaller and simpler. It's currently all
bespoke initialization of controllers line after line. Plugins would reduce
that dramatically.
2.

This PR provides a common runtime for all controllers and common
start/stop mechanics. Controllers are already similar to each other. This
would make them more so.
3.

There's no way today of knowing what controllers are running and their
health. The binary can be made into a server (more like kubelet) and it's
status could be queried at runtime to view all controllers' health. Can't
do that today. Might be useful to an admin running a cluster. Perhaps
controllers can collect their own stats and provide them in Status.

That last one is difficult to do without the common runtime.

—
Reply to this email directly or view it on GitHub
#6520 (comment)
.

k8s-bot · 2015-10-15T17:32:47Z

GCE e2e test build/test passed for commit eea4c3b.

markturansky · 2015-12-03T15:22:54Z

@lavalamp @thockin Is there any appetite for this? The feedback was lukewarm to begin with.

At the very least, this PR seems low priority. Close and re-open in the future if there's a need/desire? Add all the suggested feedback and push it through?

sttts · 2015-12-03T15:28:49Z

For contrib/mesos we have interest in this to get rid of our controller manager fork which includes quite some code duplication now.

markturansky · 2016-02-26T17:39:53Z

Closing this effort.

googlebot added the cla: no label Apr 7, 2015

satnam6502 reviewed Apr 7, 2015
View reviewed changes

vishh added the team/cluster label Apr 7, 2015

markturansky force-pushed the controllers_framework branch from 8348fc9 to bc4ca05 Compare April 7, 2015 19:39

vishh assigned satnam6502 Apr 7, 2015

satnam6502 removed their assignment Apr 7, 2015

markturansky mentioned this pull request Apr 7, 2015

Endpoints controller issues maaany lists #6518

Closed

vishh assigned thockin Apr 7, 2015

markturansky force-pushed the controllers_framework branch from bc4ca05 to a13e16e Compare April 11, 2015 20:21

markturansky force-pushed the controllers_framework branch 2 times, most recently from 8b7d07e to 6011209 Compare April 12, 2015 01:36

markturansky referenced this pull request in derekwaynecarr/kubernetes Apr 13, 2015

Remove delay when deleting namespaces, move to new controller framework

ce87f9c

markturansky mentioned this pull request Apr 13, 2015

PersistentVolumeClaimBinder #6105

Merged

markturansky changed the title ~~RFC: Controller Plugin framework~~ WIP: Controller Plugin framework Apr 13, 2015

thockin reviewed Apr 13, 2015
View reviewed changes

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

markturansky force-pushed the controllers_framework branch from a1a7ae3 to b7e72b8 Compare September 11, 2015 16:19

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

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

Added Controller plugins and ControllerConfig

eea4c3b

markturansky force-pushed the controllers_framework branch from b7e72b8 to eea4c3b Compare September 14, 2015 17:01

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

lavalamp reviewed Sep 14, 2015
View reviewed changes

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

markturansky closed this Feb 26, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Controller Plugin framework #6520

Controller Plugin framework #6520

markturansky commented Apr 7, 2015

googlebot commented Apr 7, 2015

bgrant0607 commented Apr 7, 2015

satnam6502 Apr 7, 2015

satnam6502 commented Apr 7, 2015

markturansky commented Apr 11, 2015

lavalamp commented Apr 13, 2015

thockin commented Apr 13, 2015

markturansky commented Apr 13, 2015

thockin Apr 13, 2015

markturansky Apr 23, 2015

k8s-bot commented Sep 10, 2015

k8s-bot commented Sep 11, 2015

k8s-bot commented Sep 14, 2015

lavalamp Sep 14, 2015

markturansky Sep 14, 2015

lavalamp Sep 14, 2015

markturansky Sep 15, 2015

lavalamp commented Sep 14, 2015

lavalamp Sep 14, 2015

markturansky Sep 15, 2015

lavalamp Sep 15, 2015

markturansky commented Sep 15, 2015

lavalamp commented Sep 15, 2015

k8s-bot commented Oct 15, 2015

markturansky commented Dec 3, 2015

sttts commented Dec 3, 2015

markturansky commented Feb 26, 2016

Controller Plugin framework #6520

Controller Plugin framework #6520

Conversation

markturansky commented Apr 7, 2015

googlebot commented Apr 7, 2015

bgrant0607 commented Apr 7, 2015

Choose a reason for hiding this comment

satnam6502 commented Apr 7, 2015

markturansky commented Apr 11, 2015

lavalamp commented Apr 13, 2015

thockin commented Apr 13, 2015

markturansky commented Apr 13, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-bot commented Sep 10, 2015

k8s-bot commented Sep 11, 2015

k8s-bot commented Sep 14, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lavalamp commented Sep 14, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markturansky commented Sep 15, 2015

lavalamp commented Sep 15, 2015

k8s-bot commented Oct 15, 2015

markturansky commented Dec 3, 2015

sttts commented Dec 3, 2015

markturansky commented Feb 26, 2016