Share Controller and Reconciler test tooling.

[mattmoor]

These are copied directly from knative/serving#1930

[knative-prow-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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:

• 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

[dprotaso]

I think prior to pulling the table testing into this repo I think there's more experimentation that could be done within serving.

My concerns of the current state are:

• creating test fixtures is a pain and verbose
• lots of duplication of similar objects in table rows
• table tests are huge
• the current patterns evolved from writing the tests after the fact

Two potential areas of focus

Improve testing infrastructure

• builders for fixtures
  • (upside) readability is improved
  • (downside) api specific so these wouldn't be moved to pkg, could write a generator
• provide higher abstractions for writing table rows
  • (upside) focuses on intent
  • (upside) more succinct table rows
  • (downside) adds additional complexity

This approach addresses the symptoms but not the cause - credit to @tanzeeb for pointing this out

Improve reconciler design (suggested)

Right now a reconciler has the responsibility of reconciling an object through creating and updating child resources. I think for the most part managing of the resources is independent of one another.

Thus we can think of reconciliation of an object to be the sum of nested reconciliations of (Object, Child Resource) tuples

To visualize a revision's reconciler it's composed of the following tuples

(Revision, Deployment) => Deployment
(Revision, K8s Service) => K8s Service
(Revision, FluentdConfigMap) => FluentdConfigMap
(Revision, KPA) => KPA
(Revision, VPA) => VPA

Given this observation we can pull out the specific responsibility of managing a child resource to a new component where we can create more specified table tests that focus solely on testing the (Object, Child Resource) => Child Resource transformations.

A positive side effect is that reconciler's can be composed with different 'sub-reconcilers'. This would help support optionality through plug-ability. For example the route controller could swap out the istio resources with some other solution.

[mattmoor]

@dprotaso Ack, I don't by any means think moving this freezes it. My hope is simply that as folks improve it in these ways that all of the repos can move forward.

I'd love to see more PoCs like your builder PR, to illustrate the improvements you have in mind. I by no means think any of this is perfect. :)

[dprotaso]

I don't by any means think moving this freezes it. My hope is simply that as folks improve it in these ways that all of the repos can move forward.

I'm not worried about freezing but adopters going through churn

I'd love to see more PoCs like your builder PR

Let me get started

[mattmoor]

@dprotaso My naive hope is that we can get all of those into the mold I keep outlining:

func (r *Reconciler) reconcileBar(foo *vN.Foo) error {
  barName := names.Bar(foo)
  bar, getBarErr := r.barLister.Get(foo.Namespace, barName)
  if IsNotFound(getBarErr) {
    bar, err = r.createBar(foo)
    if err != nil {
      return err
    }
  } else if getBarErr != nil {
    return getBarErr
  }

  // Take action based on the state of foo/bar
  ...

  // Reflect status from bar into foo.
  ...
}

In my perfect world, I'd define a generic interface through which to drive this, but without generics this seems like it would be a glorious pain to make generic.

IMO everything that this rough pattern reaches out to should be encapsulated and tested separately.

This PR feels like an awkward place for this discussion. Do you want to open an issue outlining what you have in mind, and we can discuss there?

[dprotaso]

This PR feels like an awkward place for this discussion. Do you want to open an issue outlining what you have in mind, and we can discuss there?

Sure

[dprotaso]

FYI: I made some minor changes here: knative/serving#1977 that affect items in this PR