Refactor common functionality in controllers by mdemirhan · Pull Request #892 · knative/serving

mdemirhan · 2018-05-15T01:55:52Z

All of our controllers have some boilerplate initialization and running code all copied and pasted between files. This PR puts the boilerplate & common code into controller.go.

This PR is still not complete. I did the refactoring and applied it only to two controllers (config and revision) to see how it fits. Remaining controllers need to be refactored as well, but I prefer doing it only after I get an initial feedback (in case there is strong pushback to the methodology, I don't want to waste too much time refactoring others as well :)).

PS: I also removed metrics from one of the controllers and i Plan to remove the remaining ones as well. Those metrics were meant to just showcase how metrics can be generated from our code, but they are not very useful metrics for developers or us. We have a much better usecase of metrics in autoscaler controller code and we no longer need this sample-ish code in our controllers.

…educe the repetition in various controllers.

dprotaso · 2018-05-15T03:44:03Z

+	informer := base.ElaInformerFactory.Elafros().V1alpha1().Configurations()
+	revisionInformer := base.ElaInformerFactory.Elafros().V1alpha1().Revisions()

+	base.Init(controllerAgentName, "Configurations", informer.Informer())


This to me indicates there's a circular dependency between the controller and the base. Having the construction of the base happen in two phases (NewController+base.Init) can be error prone and potentially restrictive.

If possible I would instead to separate the concerns strictly by using consumer interfaces to loosely couple the base and the specific crd logic. I think of this similar to how go's http package is meant to be used. Create an http.Server and provide it http.Handlers. With the handlers not needing to know about the underlying server.

Thus I could imagine creating a controller and hooking in the necessary CRD logic as a handler

I haven't looked too closely at it, but the operator framework stuff does something similar, it seems:
https://github.com/operator-framework/operator-sdk/blob/master/doc/user-guide.md#define-the-handler

Not sure why there is a circular dependency. There is a base struct that implements the common functionality and is driven by the concrete implementation. It is not that far off from http handlers, as the concrete implementation provides a handler (syncHandler) to the base and base simply keeps calling it. If you have an example of how you would implement your proposal in this context, that will help me understand your proposal better.

dprotaso · 2018-05-15T03:49:24Z

-	kubeclientset  kubernetes.Interface
-	elaclientset   clientset.Interface
-	buildclientset buildclientset.Interface
+	base *controller.ControllerBase


FYI: by not naming the nested struct explicitly you won't need to use c.base everywhere
ie.

type Controller struct { *controller.ControllerBase ... }

then c.base.Run would be c.Run

dprotaso · 2018-05-15T03:54:59Z

+	base *controller.ControllerBase,
+	buildClientSet buildclientset.Interface,
 	config *rest.Config,
 	controllerConfig controller.Config) controller.Interface {


I have a preference for returning the concrete type instead of controller.Interface - especially given that the tests end up casting it.

I would like to keep this PR scoped to reducing code duplication. I don't want to mix this with other changes we want in the controllers.

mattmoor · 2018-05-15T04:10:25Z

/assign @grantr
/assign @jonjohnsonjr

dprotaso · 2018-05-15T13:20:46Z

There was some discussion about using generators to help with the boilerplate here: https://elafros.slack.com/archives/CA4DNJ9A4/p1525180892000341

The two that were mentioned:
https://github.com/kubernetes-sigs/kubebuilder
https://github.com/GoogleCloudPlatform/metacontroller

mdemirhan · 2018-05-15T15:33:10Z

I am not sure if it is worth writing generators for the four controllers we have. If the direction is that, I will abandon this PR and create an issue tracking this. My aim with this one was very simple: minimize the copy paste between our controller code by simply pulling that functionality in a common file. The reason I attempted this was because I was moving the controller code to a structured logging library and I had to make the same change in four different places. I also want to keep this PR to be more practical - while generators, ...etc might be the way to go, if we are not going to do that anytime soon, we might as well take a baby step first.

Let me know what you think.

grantr · 2018-05-15T16:32:18Z

I'm fine with this PR going in (though I haven't reviewed it yet) but FYI I plan to attempt a similar refactoring using kubebuilder. It's unclear at this point whether kubebuilder will be an improvement.

mattmoor · 2018-05-15T17:38:45Z

@grantr @mdemirhan @dprotaso

I think we will want to continue to explore ways to refactor things to reduce boilerplate (and frankly make better use of encapsulation vs. having so much inlined into the controllers themselves). Generally, I do think this is an improvement, but IDK that anyone is going to confuse it for the final improvement(tm).

I think kubebuilder is definitely a thread worth pulling on (and perhaps if not now, it will be ready later).

Does that sounds fair?

dprotaso · 2018-05-15T17:46:26Z

Yup - I'm all for doing this incrementally.

vaikas · 2018-05-15T17:36:40Z

 }
+
+type ControllerBase struct {
+	// kubeClient allows us to talk to the k8s for core APIs


nits here, but the names in the comments do not match the actual variable. Here and below.

vaikas · 2018-05-15T17:52:09Z

+	// simultaneously in two different workers.
+	WorkQueue workqueue.RateLimitingInterface
+
+	initialized bool


does this need to be synchronized?

Bazed on Matt's feedback, I removed this two stage initialization and we no longer need this one.

Dave's and Matt's feedback to be precise :)

mattmoor · 2018-05-15T17:52:19Z

-	buildclientset buildclientset.Interface,
-	kubeInformerFactory kubeinformers.SharedInformerFactory,
-	elaInformerFactory informers.SharedInformerFactory,
+	base *controller.ControllerBase,


I'd recommend against changing the signature in this change. This will let us fold the Init functionality into a constructor we call from inside here.

mattmoor · 2018-05-15T17:53:01Z

-		ctrl.Config{},
-	).(*Controller)
+	base := ctrl.NewControllerBase(kubeClient, elaClient, kubeInformer, elaInformer)
+	controller = NewController(base, buildClient, &rest.Config{}, ctrl.Config{}).(*Controller)


This and other files can be reverted once the signature doesn't change :)

mattmoor · 2018-05-15T21:17:36Z

+
+// NewControllerBase instantiates a new instance of ControllerBase implementing
+// the common & boilerplate code between our controllers.
+func NewControllerBase(


nit: here and the struct, drop Controller since it's already in the package name.

I think controller.Base and controller.NewBase are cleaner.

…ack.

mdemirhan · 2018-05-17T15:23:20Z

@dprotaso @mattmoor @grantr Should we continue with this refactoring or abandon it? Let me know your thoughts and if all agree to proceed, please approve the request :)

mattmoor · 2018-05-17T16:23:52Z

I'm going to interpret @grantr's comment (before mine) and @dprotaso's response as consensus that this is incremental forward progress. Let's get it in. I'll take another look now (racing my next meeting).

mattmoor · 2018-05-17T16:24:52Z

/approve

Will let @grantr LGTM to confirm I'm not wildly misinterpreting his comment above :)

google-prow-robot · 2018-05-17T16:24:56Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mattmoor, mdemirhan

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~pkg/controller/OWNERS~~ [mattmoor]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

grantr

/lgtm

mdemirhan · 2018-05-17T17:09:36Z

Thanks all!

QQ: Is it possible to override the auto merge behavior? Auto merged PRs have really bad git commit messages (a union of all individual commit messages, so things like "addressed PR comments", "added more documentation", ...etc that are part of iteration but not necessarily should be part of the commit message are all part of the commit message).

grantr · 2018-05-17T17:15:31Z

The only way I know of is applying a /hold. There has been some talk in the past of requiring a /merge command before merging, but no conclusion on whether that's a good idea.

mattmoor · 2018-05-18T14:08:05Z

Alternately, you can git commit --amend and git push -f ${USER} branch-name to update PRs, but this makes it harder for reviewers to see changes.

Refactor common functionality in controllers into controller.go and r…

ffbebb4

…educe the repetition in various controllers.

mdemirhan requested review from grantr, mattmoor and vaikas May 15, 2018 01:55

mdemirhan requested a review from tcnghia as a code owner May 15, 2018 01:55

mdemirhan requested a review from a team May 15, 2018 01:55

google-prow-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label May 15, 2018

dprotaso reviewed May 15, 2018

View reviewed changes

google-prow-robot assigned grantr and jonjohnsonjr May 15, 2018

evankanderson assigned dprotaso May 15, 2018

vaikas reviewed May 15, 2018

View reviewed changes

mattmoor suggested changes May 15, 2018

View reviewed changes

mdemirhan added 3 commits May 15, 2018 11:39

Address PR comments.

cffd094

Documentation fixes.

e4fcc15

Refactor the remaining two controllers.

bb30938

google-prow-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 15, 2018

mattmoor reviewed May 15, 2018

View reviewed changes

Rename controller.ControllerBase to controller.Base based on PR feedb…

2610399

…ack.

mattmoor approved these changes May 17, 2018

View reviewed changes

google-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 17, 2018

grantr approved these changes May 17, 2018

View reviewed changes

google-prow-robot added the lgtm Indicates that a PR is ready to be merged. label May 17, 2018

google-prow-robot merged commit afd4dca into knative:master May 17, 2018

mdemirhan deleted the contrefactor branch May 17, 2018 17:09

dprotaso mentioned this pull request Jun 19, 2019

Add @dprotaso as a serving approver #4426

Merged

skonto pushed a commit to skonto/serving that referenced this pull request Aug 30, 2021

Add missing RootCA additions to checks (knative#892)

0663043

markusthoemmes added a commit to markusthoemmes/knative-serving that referenced this pull request Sep 24, 2021

Add missing RootCA additions to checks (knative#892)

0889a24

Conversation

mdemirhan commented May 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dprotaso May 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattmoor commented May 15, 2018

Uh oh!

dprotaso commented May 15, 2018

Uh oh!

mdemirhan commented May 15, 2018

Uh oh!

grantr commented May 15, 2018

Uh oh!

mattmoor commented May 15, 2018

Uh oh!

dprotaso commented May 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mdemirhan commented May 17, 2018

Uh oh!

mattmoor commented May 17, 2018

Uh oh!

mattmoor commented May 17, 2018

Uh oh!

google-prow-robot commented May 17, 2018

Uh oh!

grantr left a comment

Choose a reason for hiding this comment

Uh oh!

mdemirhan commented May 17, 2018

Uh oh!

grantr commented May 17, 2018

Uh oh!

mattmoor commented May 18, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

dprotaso May 15, 2018 •

edited

Loading