Add a "controller engine" that manages the lifecycles of controllers #160

negz · 2020-04-23T08:46:00Z

Description of your changes

A controller engine is somewhat like a controller "sub-manager", in that it's effectively a group of controllers. Unlike a typical controller manager, the lifecycle of the controllers an engine manages are not coupled to the lifecycle of the engine itself. An engine may be used by a parent controller to start and stop child controllers in accordance with configuration provided by the custom resource that the parent controller watches

Checklist

I have:

Run make reviewable to ensure this PR is ready for review.
Ensured this PR contains a neat, self documenting set of commits.
Updated any relevant documentation, examples, or release notes.
Updated the RBAC permissions in clusterrole.yaml to include any new types.

kubernetes-sigs/controller-runtime#863 We'd like to use the above PR, which is not yet included in a controller-runtime release. Updating controller-runtime requires updating a few other dependencies, which changed the signature of client-go clientset methods. This commit removes the only two uses of clientset from crossplane-runtime. pkg/test/integration now uses a controller-runtime client.Client. pkg/test.Env has been removed, as it no longer has any known users. Signed-off-by: Nic Cope <negz@rk0n.org>

negz · 2020-04-23T08:47:01Z

pkg/controller/engine.go

+*/
+
+// Package controller provides utilties for working with controllers.
+package controller


I have mixed feelings about this package name; I like how controller.NewEngine reads but it seems like it will frequently conflict with controller-runtime/pkg/controller.

I prefer the symmetry despite the naming conflicts 👍

I've so far worked around this by importing the controller-runtime/pkg/controller as ctrlr (which is close to our convention of importing sigs.k8s.io/controller-runtime as ctrl).

kctrl for controller-runtime and ctrl for ours?

A controller engine is somewhat like a controller "sub-manager", in that it's effectively a group of controllers. Unlike a typical controller manager, the lifecycle of the controllers an engine manages are not coupled to the lifecycle of the engine itself. An engine may be used by a parent controller to start and stop child controllers in accordance with configuration provided by the custom resource that the parent controller watches. Signed-off-by: Nic Cope <negz@rk0n.org>

hasheddan · 2020-04-23T10:50:31Z

pkg/controller/engine.go

+*/
+
+// Package controller provides utilties for working with controllers.
+package controller


I prefer the symmetry despite the naming conflicts 👍

hasheddan · 2020-04-23T10:50:50Z

pkg/controller/engine.go

+// An Engine manages the lifecycles of controller-runtime controllers (and their
+// caches). The lifecycles of the controllers are not coupled to lifecycle of
+// the engine, nor to the lifecycle of the controller manager it uses.
+type Engine struct {


Will this replace https://github.com/crossplane/crossplane/blob/fc494b11777dda20e2c1f70d9f627c390324f7e4/pkg/controller/apiextensions/composite/engine.go#L44?

Yeah, that's the idea. I liked the ControllerEngine abstraction around the controller-runtime machinery, and it seems like we'll probably have at least 2-3 controllers that are managing groups of child controllers, so I wanted to clean it up a little and add it here.

negz · 2020-04-23T12:11:58Z

I've plumbed this up to @muvaf's work in crossplane/crossplane#1412 and run it to make sure it works as expected.

muvaf

Thanks for moving this into crossplane-runtime. I'm suspicious whether it will be used by any other than core crossplane but it won't hurt to have it here either.

Could you also explain somewhere as comment why you needed to deal with concurrency issues?

muvaf · 2020-04-23T15:50:12Z

pkg/controller/engine.go

+*/
+
+// Package controller provides utilties for working with controllers.
+package controller


kctrl for controller-runtime and ctrl for ours?

muvaf · 2020-04-23T15:53:45Z

pkg/controller/engine.go

+		e.newCtrl = fn
+	}
+}
+


I see that there isn't WithLogger option. Do you intend to keep engine itself log-less? The passed in reconcilers will have their own logger so it shouldn't be a big concern but just checking.

Yeah, at least for now. I'm not worried about adding one, but I noticed the logs we had were mostly just duplicating existing logs from controller-runtime.

muvaf · 2020-04-23T16:14:14Z

pkg/controller/engine.go

+	go func() {
+		<-e.mgr.Elected()
+		e.done(name, errors.Wrap(ca.Start(stop), errCrashCache))
+	}()


I don't exactly know why but controller-runtime waits for the cache to sync by calling ca.WaitForCacheSync(stop) after running its go routine.

Should be good to go after addressing this.

I figured we could avoid this because controller.NewUnmanaged calls WaitForCache before it starts the controller.

muvaf · 2020-04-23T16:48:41Z

pkg/controller/engine.go

+// The default new cache and new controller functions.
+var (
+	DefaultNewCacheFn      NewCacheFn      = cache.New
+	DefaultNewControllerFn NewControllerFn = controller.NewUnmanaged


Why export these and not directly use cache.New and controller.NewUnmanaged directly?

Mostly just for documentation purposes - possibly a bit overkill but I figured it would make it obvious to GoDoc readers what the defaults were.

muvaf · 2020-04-23T16:51:43Z

pkg/controller/engine.go

+
+// Err returns any error encountered by the named controller. The returned error
+// is always nil if the named controller is running.
+func (e *Engine) Err(name string) error {


I see that you're not using Err function in crossplane/crossplane#1440 How should this be used? Check on errors in every reconcile of whoever called Start?

Yeah, something like that. I figured it could be used to log or emit a condition if and when we discover that the controller has crashed. could be something like:

if !ctrl.IsRunning(name) { if err := ctrl.Err(name); err != nil { log.Debug("womp womp", "error", err) } _ = ctrl.Start(name, o) }

negz · 2020-04-23T19:40:00Z

Could you also explain somewhere as comment why you needed to deal with concurrency issues?

@muvaf I wanted to allow the cache and controller goroutines a way to surface any error they encountered, and also to close the stop channel if they encountered an error (or just stopped for some other reason). This ensures that if the controller crashes it stops the cache too and vice versa. The map of stop channels can't be shared across goroutines without some kind of lock, and channels also panic if they're closed multiple times, so I ended up playing with a few patterns trying to use sync.Once etc before just giving up and using a good old fashioned mutex.

negz requested review from muvaf, hasheddan and kasey April 23, 2020 08:46

negz commented Apr 23, 2020

View reviewed changes

negz force-pushed the outofcontrol branch from 0615a5c to 357942c Compare April 23, 2020 08:51

hasheddan reviewed Apr 23, 2020

View reviewed changes

muvaf reviewed Apr 23, 2020

View reviewed changes

hasheddan approved these changes Apr 23, 2020

View reviewed changes

negz merged commit dfff858 into crossplane:master Apr 23, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a "controller engine" that manages the lifecycles of controllers #160

Add a "controller engine" that manages the lifecycles of controllers #160

negz commented Apr 23, 2020

negz Apr 23, 2020

hasheddan Apr 23, 2020

negz Apr 23, 2020 •

edited

Loading

muvaf Apr 23, 2020

hasheddan Apr 23, 2020

hasheddan Apr 23, 2020

negz Apr 23, 2020

negz commented Apr 23, 2020

muvaf left a comment

muvaf Apr 23, 2020

muvaf Apr 23, 2020

negz Apr 23, 2020

muvaf Apr 23, 2020

muvaf Apr 23, 2020

negz Apr 23, 2020

muvaf Apr 23, 2020

negz Apr 23, 2020

muvaf Apr 23, 2020

negz Apr 23, 2020 •

edited

Loading

negz commented Apr 23, 2020

Add a "controller engine" that manages the lifecycles of controllers #160

Add a "controller engine" that manages the lifecycles of controllers #160

Conversation

negz commented Apr 23, 2020

Description of your changes

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

negz Apr 23, 2020 • edited Loading

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

Choose a reason for hiding this comment

negz commented Apr 23, 2020

muvaf left a comment

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

negz Apr 23, 2020 • edited Loading

Choose a reason for hiding this comment

negz commented Apr 23, 2020

negz Apr 23, 2020 •

edited

Loading

negz Apr 23, 2020 •

edited

Loading