Proposed feature: Plugable revision deployers

[jroper]

This PR is being raised primarily to solicit feedback in the context of code. I expect there to be iterations back and forth on this change, and I hope to attend to the knative serving API working group to discuss there as well. Also, no tests yet, so at very least that needs to be addressed before merging.

The context (ie, use case) for this can be seen in this mailing list post:

https://groups.google.com/d/msg/knative-dev/ZjB1EJOkZRg/eu5Gic3iBAAJ

Based on feedback there, I've created this PR. What this PR does is makes the deployment of revisions plugable. This is done by adding a deployer field to the revision spec. It defaults to KnativeServing, and if it is KnativeServing, then the controller will create and update the deployment as normal. However, if it is anything other than KnativeServing, then the controller will not create or update the deployment, and it will expect another operator, supplied by a third party, to create and update the deployment instead.

Using this change, we have created an operator that uses this, and deploys functions with a sidecar written in Akka that implements stateful functions (in some ways, similar to Azure's stateful entities, however far more powerful). Note though that this use case is far more than just replacing the sidecar image, a custom deployer is needed to be able to wire the sidecar to an appropriate persistence store, as well as to configure the sidecar to form a cluster (this is used to shard event sourced entities across the user functions). So it wouldn't be enough to just make the sidecar image configurable, we needed to be able to customise the deployment itself.

You can see this work here, though it's a big project:

https://github.com/lightbend/stateful-serverless/tree/knative-integration

Perhaps most pertinent is what it actually looks like to deploy a function that uses this feature, that can be seen here:

apiVersion: serving.knative.dev/v1alpha1
kind: Service
metadata:
  name: shopping-cart
spec:
  template:
    spec:
      containers:
      - image: gcr.io/stateserv/js-shopping-cart:latest
        ports:
        - name: h2c
          containerPort: 8080
      deployer:
        name: EventSourced
        config:
          journal:
            name: cassandra
            config:
              keyspace: shoppingcart

It was suggested that we might use an annotation to specify the deployer, and while this would work for selecting the deployer, the problem with it is that deployers may need their own configuration, for example, in this case, the function needs to be associated with a configured journal resource (another CRD that our project provides), and some configuration is required to say how that journal should be used (in this case, the keyspace is specified). This configuration could all be supplied using annotations too, but that feels like an abuse of annotations - it feels more natural and logical to include it in the spec. That's why we've gone for an approach of including the deployer in the spec. The deployers config property is handled by the knative serving controller as raw JSON message, so it's passed from service to configuration to revision as is, and it's up to the deployer to make sense of it.

[knative-prow-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: jroper
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: mattmoor

If they are not already assigned, you can assign the PR to them by writing /assign @mattmoor in a comment when ready.

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:

• pkg/apis/OWNERS
• pkg/reconciler/OWNERS

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

[knative-prow-robot]

Hi @jroper. Thanks for your PR.

I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[dprotaso]

/assign @mattmoor @dgerd

[dprotaso]

/ok-to-test

[dprotaso]

My 2 cents here is the contract between (Revision, Autoscaler) <=> Deployment is very highly coupled and doesn't have a clear interface/separation

ie. There are expectations for Deployments to sidecar a queue-proxy for things to work.

Thus I think 3rd party Deployers will suffer a lot of churn as these informal contracts/assumptions change.

[mattmoor]

I agree. Perhaps we should hash this out in an issue first?