Unify the controller infrastructures across various Knative components

[vaikas]

There seems to be (at least between serving / eventing) some differences in how things are started up and configured at the controller level. For example, eventing uses controller runtime to stand up the controllers, whereas serving brings these up manually.

https://github.com/knative/eventing/blob/master/cmd/controller/main.go
https://github.com/knative/serving/blob/master/cmd/controller/main.go

https://github.com/knative/eventing/tree/master/pkg/controller/eventing/subscription
https://github.com/knative/serving/tree/master/pkg/reconciler/v1alpha1/revision

I think this divergence has several downsides and makes the cognitive load moving between repos difficult.

We should ensure that the repos are consistent.
@mattmoor
@grantr
@n3wscott

I'd like to target this asap because the longer this continues, the harder it will be to reconcile these differences and I don't see a good reason for the divergence. If there is, we should jot things down and really make a conscious decision on why this is better.

[n3wscott]

we talked about this when we were making the transition to controller runtime. The group wanted to align with controller runtime.

Controller runtime gives us a lot with the cost of internal knative divergence. I am in favor of basing our controllers on what is in pkg and then eventing and serving can migrate to whatever we pick together.

[n3wscott]

One more thing that is an example of how we are hurting ourselves: Metrics provided by controller-runtime are not going to be the same as the metrics added by the knative team for the pkg controller. We can remap the keys, but we can never say for sure that the measurements are apples to apples, as timing and counting choices can be very different.

But on the positive side, Controller Runtime added reconciliation counts without us needing to implement that code. So leveraging a community package does get us "free" work.

[mattmoor]

+1, if it's worth doing (not sure), it's worth doing everywhere (or nowhere). We're hurting ourselves and our contributors by not being consistent.

[grantr]

👍 I'll take a closer look at what serving and build are doing and try to write up the major differences as a first step.

[vaikas]

Have there been things that we couldn't do because we had to wait for the runtime to do something for us? As I said in the slack I agree with @mattmoor on the hurting our contributors.
"I find myself confused (even more than my usual state 😉 ) moving between repos. I view any friction when moving between repos as a warning sign."

[n3wscott]

Yes, like temporarily fork controller runtime deps in order to upgrade the kubernetes clients.

[grantr]

I've written up some recommendations in this doc in the Eventing team drive (membership in the knative-dev Google group required).

• Remove use of controller-tools and kustomize from eventing-sources. Done in Removing kubebuilder. eventing-contrib#192.
• Change eventing and eventing-sources pkg/ directory structures to match serving. WIP at [WIP] Refactor controller paths #771 and Move cronjobsource to knative/serving style reconcile directory structures. eventing-contrib#198.
• Update eventing and eventing-sources reconcile signatures to take a context and object similar to serving: func (c *Reconciler) Reconcile(ctx context.Context, obj runtime.Object) error. WIP at Change Signature of KnativeReconciler.Reconcile eventing-contrib#197.
• Replace revision-specific constraints or overrides in Knative repos with version constraints.
• Tag knative/pkg versions frequently so dependent repos can use version constraints.

[vaikas]

Just tickling this issue again. My understanding is that there are two pretty big problems with the use of controller runtime:

1. Harder to test reconcilers
2. Monitoring will take extra work

Serving will not move to controller runtime at least some time after 1.0, more and more features are being added to pkg/ that we'll effectively have to redo in eventing impl and I'm concerned that by not having the eventing stack use the common features being developed there, we'll just have to re-implement or wait for these features to get added to CR.

[vaikas]

@mdemirhan - Thoughts on monitoring since you have spent a fair amount of time on this :)
@mattmoor - Thoughts on impact for production readiness and what work would have to be reimplemented.

[grantr]

Harder to test reconcilers

Curious about this one. What makes reconcilers harder to test in eventing?

[n3wscott]

What makes reconcilers harder to test in eventing?

We have a much harder time testing real reconciliation in eventing compared to what serving can do if you look at the test coverage numbers. We are getting better because we shim and sync caches but this work has already happened in serving and we are duplicating effort with no benefit to Knative as a whole.

[evankanderson]

/kind feature-request