server: make DI practical for controllers #4250

milas · 2021-02-25T20:52:05Z

This is a bit roundabout, but the key issue is that controllers
(Reconcilers) need the controller client to do their work, which
comes from the ctrl.Manager, but isn't available until the manager
is actually connected to the API server, so it's not suitable for
normal DI.

A ControllerBuilder provides the glue between the manager and
reconcilers by providing them with the client and calling the
SetupWithManager methods on them. The latter of these is a standard
method that's bootstrapped by the codegen (but not part of the
Reconciler interface).

This is a bit roundabout, but the key issue is that controllers (`Reconciler`s) need the controller client to do their work, which comes from the `ctrl.Manager`, but isn't available until the manager is actually connected to the API server, so it's not suitable for normal DI. A `ControllerBuilder` provides the glue between the manager and reconcilers by providing them with the client and calling the `SetupWithManager` methods on them. The latter of these is a standard method that's bootstrapped by the codegen (but not part of the `Reconciler` interface).

milas · 2021-02-25T20:55:37Z

@nicks This is really pretty kludgy but I can't find a cleaner pattern. If you've got ideas, feel free to use this branch to play around.

The only other thought I had was to make a some sort of injector that explicitly takes in the client:

func injectControllers(client ctrlclient.Client) []Controller, error {
   wire.Build(ProvideControllers)
   return []Controller{}, nil
}

Then the manager could just call that with its client (deferring the construction of the controllers until its ready) and do all the work when it's initializing, but that creates all kinds of import cycles since you also need the main app deps.

nicks · 2021-02-25T21:10:17Z

internal/controllers/core/filewatch_controller.go

+func (r *FileWatchController) SetupWithManager(mgr ctrl.Manager) error {
+	return ctrl.NewControllerManagedBy(mgr).
+		For(&filewatches.FileWatch{}).
+		Owns(&filewatches.FileWatch{}).


i don't think this is right?

Oops I'll take that off, this controller is in a weird state because it just exists to ensure DI stuff is functional. (It might be worth making a "sample" entity in tilt-apiserver for testing type purposes)

nicks · 2021-02-25T21:10:39Z

internal/controllers/core/filewatch_controller.go

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


i was assuming we'd put each controller in its own package

Sounds good - this was mostly an artifact of running kubebuilder/apiserver-boot at some point, I'll move it

nicks · 2021-02-25T21:13:45Z

internal/controllers/manager.go

 	ctx, m.cancel = context.WithCancel(ctx)

 	// TODO(milas): we should provide a logr.Logger facade for our logger rather than using zap
 	w := logger.Get(ctx).Writer(logger.DebugLvl)
 	ctrl.SetLogger(zap.New(zap.UseDevMode(true), zap.WriteTo(w)))

-	utilruntime.Must(corev1alpha1.AddToScheme(scheme))
-
 	mgr, err := ctrl.NewManager(m.config, ctrl.Options{


can we dependency-inject the manager?

i think that would avoid a lot of the circular initialization issues you're running into

Unfortunately the manager is the problem -- ctrl.NewManager creates a client that tries to establish a connection to API server and errors out if it's not up. (I originally tried to split the construction and .Start() call to do exactly what you're suggesting, which is how I discovered this mess)

Another possibility would be to create a DeferredClient that just returns errors until provided with a real implementation? That way there's no hard dependency and given that the client is only meant to be used within Reconcile() it shouldn't ever be able to get called before initialization anyway.

Take a look at cc45e52 and see if you think that's an improvement? It really just lets controllers pretend to get created as expected by getting a ctrlclient.Client injected. The impl is *deferredClient that returns errors (or nil for methods that don't return error which is less than ideal) until the manager initializes it.

Not sure that the extra indirection is worth saving on needing to call SetClient(), but at least it hides some of the oddities from each controller implementation.

nicks · 2021-02-25T23:06:34Z

ah, ok thanks for checking!

nicks

looked at the other commit - i don't have a strong pref of one vs the other

nicks · 2021-02-26T16:32:10Z

internal/controllers/scheme.go

+	corev1alpha1 "github.com/tilt-dev/tilt/pkg/apis/core/v1alpha1"
+)
+
+func NewScheme() *runtime.Scheme {


i think GenericConfig in apiserver.Config should have a scheme on it now?

Ah, yep, it's on ExtraConfig. I'm hitting a funky issue though, not sure if we can share schemes between the actual API server and controller manager:

error initializing *filewatch.Controller controller: multiple group-version-kinds associated with type *v1alpha1.FileWatch, refusing to guess at one

Comes from https://github.com/kubernetes-sigs/controller-runtime/blob/e388e1e1f5a745166955442294bb8aede1f761e2/pkg/client/apiutil/apimachinery.go#L108-L113

The two versions are __internal and v1alpha1. The former comes from the API server's scheme registration:
https://github.com/tilt-dev/tilt-apiserver/blob/095950403c2cbc559e2f201da1c22e66b425e202/pkg/server/builder/resource/types.go#L119-L120

(Since it's kind of weird/indirect, when there are multi versions of an entity, the storage version is always going to get the __internal version in API server scheme.)

LMK if I'm missing something or doing something wrong - I am not familiar with the depths of the API server

i don't know, let's just check this in and i can debug what's going on...

…troller-wire

This prevents the HUD server blocking forever since the controller manager is still connected.

milas · 2021-02-26T21:21:48Z

@nicks Want to call out latest commit - I changed subscribers to get torn down in reverse order. Integration tests were getting stuck shutting down API server before controller manager because the controller manager was presumably holding onto a connection to the API server that it was waiting to be drained.

tilt/internal/store/subscriber.go

Lines 103 to 105 in 855ad35

    
           for i := len(subscribers) - 1; i >= 0; i-- { 
        
           	subscribers[i].maybeTeardown(ctx) 
        
           }

nicks · 2021-02-26T21:27:45Z

sgtm

…

On Fri, Feb 26, 2021 at 4:22 PM Milas Bowman ***@***.***> wrote: @nicks <https://github.com/nicks> Want to call out latest commit - I changed subscribers to get torn down in *reverse* order. Integration tests were getting stuck shutting down API server before controller manager because the controller manager was presumably holding onto a connection to the API server that it was waiting to be drained. https://github.com/tilt-dev/tilt/blob/855ad3583335cd82d6c6de705a73d1a46fdb8f77/internal/store/subscriber.go#L103-L105 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4250 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACEA4PL5UWDQ3ICQJKMLPDTBAGHVANCNFSM4YHGYKAQ> .

milas requested a review from nicks February 25, 2021 20:52

nicks reviewed Feb 25, 2021

View reviewed changes

nicks approved these changes Feb 25, 2021

View reviewed changes

server: clean up stub filewatch controller

b420b9c

nicks approved these changes Feb 26, 2021

View reviewed changes

milas added 2 commits February 26, 2021 15:00

Merge remote-tracking branch 'origin/master' into milas/apiserver-con…

ac10512

…troller-wire

store: remove subscribers in reverse order

855ad35

This prevents the HUD server blocking forever since the controller manager is still connected.

milas merged commit 7a2db0a into master Feb 26, 2021

milas deleted the milas/apiserver-controller-wire branch February 26, 2021 22:02

milas mentioned this pull request Jun 7, 2021

reconcile api objects instead of state objects [ch12124] #4613

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

server: make DI practical for controllers #4250

server: make DI practical for controllers #4250

milas commented Feb 25, 2021

milas commented Feb 25, 2021

nicks Feb 25, 2021

milas Feb 25, 2021

nicks Feb 25, 2021

milas Feb 25, 2021

nicks Feb 25, 2021

milas Feb 25, 2021 •

edited

milas Feb 26, 2021

nicks commented Feb 25, 2021

nicks left a comment

nicks Feb 26, 2021

milas Feb 26, 2021

nicks Feb 26, 2021

milas commented Feb 26, 2021 •

edited

nicks commented Feb 26, 2021 via email

server: make DI practical for controllers #4250

server: make DI practical for controllers #4250

Conversation

milas commented Feb 25, 2021

milas commented Feb 25, 2021

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

milas Feb 25, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicks commented Feb 25, 2021

nicks 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

milas commented Feb 26, 2021 • edited

nicks commented Feb 26, 2021 via email

milas Feb 25, 2021 •

edited

milas commented Feb 26, 2021 •

edited