Invert the relationship between controller.Base and derivatives. #1770

mattmoor · 2018-07-31T20:55:34Z

The way we use controller.Base today is very reminiscent of OOP; it is effectively trying to be a base class, and many things are painful trying to write this way in Go (e.g. we cannot simply make Reconcile an abstract method to be implemented by derivatives).

With this change, we will instead move to a model where Base becomes Impl and takes an instance of the following interface:

package controller

type Reconciler interface {
  Reconcile(key string) error
}

In this model, controller.Impl is the single shared controller implementation, and the NewController methods become responsible for instantiating it with the appropriate implementation of Reconciler and wiring the appropriate events up via the informers.

This cleans up the separation between ./pkg/controller and ./pkg/controller/foo, and will hopefully start to make the latter quite small. The plan is to share the former via knative/pkg in a subsequent change. I'm debating followup changes such as moving to a ./pkg/reconciler/foo structure, once only Reconcilers live in this repo.

Fixes: #1505

cc @evankanderson @vaikas-google @n3wscott @grantr @imjasonh

dprotaso · 2018-07-31T21:07:28Z

Woooo

…

On Tue, Jul 31, 2018 at 17:04 Google Prow Robot ***@***.***> wrote: @mattmoor <https://github.com/mattmoor>: The following tests *failed*, say /retest to rerun them all: Test name Commit Details Rerun command pull-knative-serving-unit-tests 203ad04 <203ad04> link <https://gubernator-internal.googleplex.com/build/knative-prow/pr-logs/pull/knative_serving/1770/pull-knative-serving-unit-tests/317/> /test pull-knative-serving-unit-tests pull-knative-serving-build-tests 203ad04 <203ad04> link <https://gubernator-internal.googleplex.com/build/knative-prow/pr-logs/pull/knative_serving/1770/pull-knative-serving-build-tests/298/> /test pull-knative-serving-build-tests Full PR test history <https://gubernator-internal.googleplex.com/pr/knative_serving/1770>. Your PR dashboard <https://gubernator-internal.googleplex.com/pr/mattmoor>. Instructions for interacting with me using PR comments are available here <https://git.k8s.io/community/contributors/guide/pull-requests.md>. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra <https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:> repository. I understand the commands that are listed here <https://go.k8s.io/bot-commands>. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#1770 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABIgvrY8-NpeYQlvbdTS_x8OKiSaprrks5uMMZDgaJpZM4VpZQB> .

mattmoor · 2018-07-31T22:06:23Z

These test failures are quite weird, I wonder if I need to rebase or something...

mattmoor · 2018-07-31T22:08:15Z

Oh yeah, Chen's change....

The way we use `controller.Base` today is very reminiscent of OOP; it is effectively trying to be a base class, and many things are painful trying to write this way in Go (e.g. we cannot simply make `Reconcile` an abstract method to be implemented by derivatives). With this change, we will instead move to a model where `Base` becomes `Impl` and takes an instance of the following interface: ```go package controller type Reconciler interface { Reconcile(key string) error } ``` In this model, `controller.Impl` is the single shared controller implementation, and the `NewController` methods become responsible for instantiating it with the appropriate implementation of `Reconciler` and wiring the appropriate events up via the informers. This cleans up the separation between `./pkg/controller` and `./pkg/controller/foo`, and will hopefully start to make the latter quite small. Ultimately the plan is to share the former via `knative/pkg`. Fixes: knative#1505

mattmoor · 2018-08-01T02:38:25Z

/test pull-knative-serving-integration-tests

The `controller.go` is from: knative/serving#1770, however, this adds 100% coverage of `controller.go`, which we have been missing in `knative/serving`. Fixes: knative#8

mattmoor · 2018-08-01T03:57:38Z

FWIW, I also have knative/pkg#33 in flight, which adds 100% coverage of controller.go. I didn't bother adding it here because we don't have any tests for it today, and the plan was to move it anyways.

mattmoor · 2018-08-01T13:46:21Z

I think that (in the move to knative/pkg) I am going to add a context.Context to the Reconcile() signature. I find having context.TODO() sprinkled everywhere to be a bit of an eyesore. This will also let us pass the logkey to NewBase and share code like this:

// loggerWithServiceInfo enriches the logs with service name and namespace.
func loggerWithServiceInfo(logger *zap.SugaredLogger, ns string, name string) *zap.SugaredLogger {
	return logger.With(zap.String(commonlogkey.Namespace, ns), zap.String(logkey.Service, name))
}

and this:

	// Wrap our logger with the additional context of the configuration that we are reconciling.
	logger := loggerWithServiceInfo(c.Logger, namespace, name)
	ctx := logging.WithLogger(context.TODO(), logger)

I may then also make the Logger on the Base Reconciler private, so that folks access it through the context.Context. Thoughts?

The `controller.go` is from: knative/serving#1770, however, this adds 100% coverage of `controller.go`, which we have been missing in `knative/serving`. This also adds a `context.Context` to the `Reconcile` signature, to enable better sharing of logger setup across controllers. Fixes: knative#8

jonjohnsonjr · 2018-08-01T16:50:21Z

pkg/controller/controller.go

-	syncHandler func(string) error,
-	controllerName string) error {
-
+func (c *Impl) Run(threadiness int, stopCh <-chan struct{}) error {


Can you update the comment for this method?

jonjohnsonjr · 2018-08-01T16:51:02Z

pkg/controller/controller.go

 	// Launch workers to process Revision resources
-	logger.Info("Starting workers")
+	logger := c.logger


The comment above this mentions Revisions, which feels like a copy/paste oversight.

jonjohnsonjr · 2018-08-01T16:51:50Z

pkg/controller/controller.go

 	// Launch workers to process Revision resources
-	logger.Info("Starting workers")
+	logger := c.logger
+	logger.Info("Starting controller and workers")


It's a bit of a shame that we're losing the controller kind in this log message, but I think we should be able to see that in the structured logging, right?

We're actually not. This message appears in a JSON blob that has been decorated with this context (in some form) already.

The `controller.go` is from: knative/serving#1770, however, this adds 100% coverage of `controller.go`, which we have been missing in `knative/serving`. This also adds a `context.Context` to the `Reconcile` signature, to enable better sharing of logger setup across controllers. Fixes: knative#8

jonjohnsonjr · 2018-08-01T17:15:55Z

/lgtm
/approve

google-prow-robot · 2018-08-01T17:16:03Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~cmd/controller/OWNERS~~ [mattmoor]
~~cmd/multitenant-autoscaler/OWNERS~~ [mattmoor]
~~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

knative-metrics-robot · 2018-08-01T17:19:52Z

The following is the coverage report on pkg/.
Say /test pull-knative-serving-go-coverage to run the coverage report again

File	Old Coverage	New Coverage	Delta
pkg/controller/autoscaling/autoscaling.go	Do not exist	82.6%
pkg/controller/route/cruds.go	92.6%	92.7%	0.1
pkg/controller/route/labels.go	93.5%	92.3%	-1.2
pkg/controller/route/route.go	96.6%	96.6%	0.1
pkg/controller/revision/revision.go	85.0%	85.1%	0.1
pkg/controller/service/service.go	91.3%	92.4%	1.1

mattmoor · 2018-08-01T18:39:06Z

/test pull-knative-serving-integration-tests

The `controller.go` is from: knative/serving#1770, however, this adds 100% coverage of `controller.go`, which we have been missing in `knative/serving`. This also adds a `context.Context` to the `Reconcile` signature, to enable better sharing of logger setup across controllers. Fixes: #8

…tive#1770) * Invert the relationship between controller.Base and derivatives. The way we use `controller.Base` today is very reminiscent of OOP; it is effectively trying to be a base class, and many things are painful trying to write this way in Go (e.g. we cannot simply make `Reconcile` an abstract method to be implemented by derivatives). With this change, we will instead move to a model where `Base` becomes `Impl` and takes an instance of the following interface: ```go package controller type Reconciler interface { Reconcile(key string) error } ``` In this model, `controller.Impl` is the single shared controller implementation, and the `NewController` methods become responsible for instantiating it with the appropriate implementation of `Reconciler` and wiring the appropriate events up via the informers. This cleans up the separation between `./pkg/controller` and `./pkg/controller/foo`, and will hopefully start to make the latter quite small. Ultimately the plan is to share the former via `knative/pkg`. Fixes: knative#1505 * Address Jon code review feedback.

mattmoor assigned tcnghia and jonjohnsonjr Jul 31, 2018

google-prow-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jul 31, 2018

google-prow-robot requested review from grantr, josephburnett, mdemirhan and tcnghia July 31, 2018 20:55

mattmoor force-pushed the reconciler branch from 203ad04 to 819614e Compare July 31, 2018 22:08

mattmoor assigned dprotaso Jul 31, 2018

mattmoor force-pushed the reconciler branch from 819614e to ab3bb17 Compare August 1, 2018 01:20

mattmoor mentioned this pull request Aug 1, 2018

Move our base controller infrastructure into knative/pkg. knative/pkg#33

Merged

jonjohnsonjr reviewed Aug 1, 2018

View reviewed changes

Address Jon code review feedback.

ee7a181

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

google-prow-robot merged commit 09e9ed0 into knative:master Aug 1, 2018

mattmoor deleted the reconciler branch August 1, 2018 19:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Invert the relationship between controller.Base and derivatives. #1770

Invert the relationship between controller.Base and derivatives. #1770

mattmoor commented Jul 31, 2018

dprotaso commented Jul 31, 2018 via email

mattmoor commented Jul 31, 2018

mattmoor commented Jul 31, 2018

mattmoor commented Aug 1, 2018

mattmoor commented Aug 1, 2018

mattmoor commented Aug 1, 2018

jonjohnsonjr Aug 1, 2018

mattmoor Aug 1, 2018

jonjohnsonjr Aug 1, 2018

mattmoor Aug 1, 2018

jonjohnsonjr Aug 1, 2018

mattmoor Aug 1, 2018

jonjohnsonjr commented Aug 1, 2018

google-prow-robot commented Aug 1, 2018

knative-metrics-robot commented Aug 1, 2018

mattmoor commented Aug 1, 2018

Invert the relationship between controller.Base and derivatives. #1770

Invert the relationship between controller.Base and derivatives. #1770

Conversation

mattmoor commented Jul 31, 2018

dprotaso commented Jul 31, 2018 via email

mattmoor commented Jul 31, 2018

mattmoor commented Jul 31, 2018

mattmoor commented Aug 1, 2018

mattmoor commented Aug 1, 2018

mattmoor commented Aug 1, 2018

jonjohnsonjr Aug 1, 2018

Choose a reason for hiding this comment

mattmoor Aug 1, 2018

Choose a reason for hiding this comment

jonjohnsonjr Aug 1, 2018

Choose a reason for hiding this comment

mattmoor Aug 1, 2018

Choose a reason for hiding this comment

jonjohnsonjr Aug 1, 2018

Choose a reason for hiding this comment

mattmoor Aug 1, 2018

Choose a reason for hiding this comment

jonjohnsonjr commented Aug 1, 2018

google-prow-robot commented Aug 1, 2018

knative-metrics-robot commented Aug 1, 2018

mattmoor commented Aug 1, 2018