Multiple Subscriptions overwrite each other (eventing.knative.dev/v1alpha1)

[adamharwayne]

Expected Behavior

When I create two eventing.knative.dev/v1alpha1 Subscriptions that have the same from, I expect that the from object's Subscriptions field will have two entries.

Actual Behavior

It actually only has one. Seemingly the most recently reconciled Subscription.

Steps to Reproduce the Problem

Note that I modified the Subscription controller slightly to allow K8s Services to be called.

1. Create an eventing.knative.dev/v1alpha1 Channel.

   apiVersion: eventing.knative.dev/v1alpha1
   kind: Channel
   metadata:
     name: qux-2
   spec:
     provisioner:
       ref:
         apiVersion: eventing.knative.dev/v1alpha1
         kind: ClusterProvisioner
         name: in-memory-bus-provisioner

2. Create two eventing.knative.dev/v1alpha1 Subscriptions pointing at that Channel.

   apiVersion: eventing.knative.dev/v1alpha1
   kind: Subscription
   metadata:
     name: qux-2-dumper-foo
   spec:
     from:
       apiVersion: eventing.knative.dev/v1alpha1
       kind: Channel
       name: qux-2
     call:
       target:
         apiVersion: v1
         kind: Service
         name: message-dumper-foo
   
   ---
   
   apiVersion: eventing.knative.dev/v1alpha1
   kind: Subscription
   metadata:
     name: qux-2-dumper-bar
   spec:
     from:
       apiVersion: eventing.knative.dev/v1alpha1
       kind: Channel
       name: qux-2
     call:
       target:
         apiVersion: v1
         kind: Service
         name: message-dumper-bar

3. Get the Subscriptions and see that they are both marked as Ready: True.

   kubectl get subscriptions.eventing.knative.dev -oyaml qux-2-dumper-{foo,bar}

   apiVersion: v1
   items:
   - apiVersion: eventing.knative.dev/v1alpha1
     kind: Subscription
     metadata:
       annotations:
         kubectl.kubernetes.io/last-applied-configuration: ...
       clusterName: ""
       creationTimestamp: 2018-10-02T16:56:07Z
       generation: 1
       name: qux-2-dumper-foo
       namespace: default
       resourceVersion: "4824922"
       selfLink: /apis/eventing.knative.dev/v1alpha1/namespaces/default/subscriptions/qux-2-dumper-foo
       uid: 0e1d2516-c664-11e8-99f5-42010a80004a
     spec:
       call:
         target:
           apiVersion: v1
           kind: Service
           name: message-dumper-foo
       from:
         apiVersion: eventing.knative.dev/v1alpha1
         kind: Channel
         name: qux-2
       generation: 1
     status:
       conditions:
       - lastTransitionTime: 2018-10-02T16:56:08Z
         status: "True"
         type: FromReady
       - lastTransitionTime: 2018-10-02T16:56:08Z
         status: "True"
         type: Ready
       - lastTransitionTime: 2018-10-02T16:56:07Z
         status: "True"
         type: Resolved
       subscribable:
         channelable: {}
   - apiVersion: eventing.knative.dev/v1alpha1
     kind: Subscription
     metadata:
       annotations:
         kubectl.kubernetes.io/last-applied-configuration: ...
       clusterName: ""
       creationTimestamp: 2018-10-02T16:56:07Z
       generation: 1
       name: qux-2-dumper-bar
       namespace: default
       resourceVersion: "4824927"
       selfLink: /apis/eventing.knative.dev/v1alpha1/namespaces/default/subscriptions/qux-2-dumper-bar
       uid: 0e2ca285-c664-11e8-99f5-42010a80004a
     spec:
       call:
         target:
           apiVersion: v1
           kind: Service
           name: message-dumper-bar
       from:
         apiVersion: eventing.knative.dev/v1alpha1
         kind: Channel
         name: qux-2
       generation: 1
     status:
       conditions:
       - lastTransitionTime: 2018-10-02T16:56:09Z
         status: "True"
         type: FromReady
       - lastTransitionTime: 2018-10-02T16:56:09Z
         status: "True"
         type: Ready
       - lastTransitionTime: 2018-10-02T16:56:08Z
         status: "True"
         type: Resolved
       subscribable:
         channelable: {}
   kind: List
   metadata:
     resourceVersion: ""
     selfLink: ""

4. Get the Channel and notice that it only has one of the two subscriptions.

kubectl get channels.eventing.knative.dev -oyaml qux-2

apiVersion: eventing.knative.dev/v1alpha1
kind: Channel
metadata:
annotations:
  kubectl.kubernetes.io/last-applied-configuration: |
    {"apiVersion":"eventing.knative.dev/v1alpha1","kind":"Channel","metadata":{"annotations":{},"name":"qux-2","namespace":"default"},"spec":{"provisioner":{"ref":{"apiVersion":"eventing.knative.dev/v1alpha1","kind":"ClusterProvisioner","name":"in-memory-bus-provisioner"}}}}
clusterName: ""
creationTimestamp: 2018-10-02T16:56:06Z
finalizers:
- in-memory-bus-channel-controller
generation: 1
name: qux-2
namespace: default
resourceVersion: "4824938"
selfLink: /apis/eventing.knative.dev/v1alpha1/namespaces/default/channels/qux-2
uid: 0defa96c-c664-11e8-99f5-42010a80004a
spec:
channelable:
  subscribers:
  - callableDomain: message-dumper-bar.default.svc.cluster.local
generation: 5
provisioner:
  ref:
    apiVersion: eventing.knative.dev/v1alpha1
    kind: ClusterProvisioner
    name: in-memory-bus-provisioner
status:
conditions:
- lastTransitionTime: null
  status: "True"
  type: Ready
sinkable:
  domainInternal: qux-2-channel.default.svc.cluster.local
subscribable:
  channelable:
    apiVersion: eventing.knative.dev/v1alpha1
    kind: Channel
    name: qux-2
    namespace: default

Additional Info

This seems to be caused by the merging of Subscriptions via Patch. It seems to always 'patch' over any existing Subscriptions.

With some enhanced logging in the Subscription controller:

The first one to reconcile adds appropriately:

Original:

Channelable: nil

Patch: '[{Operation:add Path:/spec/channelable Value:map[subscribers:[map[callableDomain:message-dumper-foo.default.svc.cluster.local]]]}]'

Patched:

Channelable: &v1alpha1.Channelable{
  Subscribers: []v1alpha1.ChannelSubscriberSpec{
    {
      CallableDomain: "message-dumper-foo.default.svc.cluster.local"
    }
  }
}

The next one to reconcile overwrites it:

Original:

Channelable: &v1alpha1.Channelable{
  Subscribers: []v1alpha1.ChannelSubscriberSpec{
    {
      CallableDomain: "message-dumper-foo.default.svc.cluster.local"
    }
  }
}

Patch: '[{Operation:replace Path:/spec/channelable/subscribers/0/callableDomain Value:message-dumper-bar.default.svc.cluster.local}]'

Patched:

Channelable: &v1alpha1.Channelable{
  Subscribers: []v1alpha1.ChannelSubscriberSpec{
    {
      CallableDomain: "message-dumper-bar.default.svc.cluster.local"
    }
  }
}

Possible Solutions

1. Set the Subscriptions array to patchStrategy:"merge".

   • Can this be done without a key? Essentially making this logically a Set.
     • How do we handle deletes?
   • If there needs to be a key, what is the key? Neither sinkableDomain nor callableDomain will work.

2. Always append to the Subscriptions array.

   • This won't really work. Nothing can ever be safely removed.

3. Any time any Subscription changes, the Subscription Controller set the entire Subscriptions list for that Subscription's from Channel. By listing all Subscriptions in the namespace, getting those with the same from, and calculating the entire list in-memory.

   • Computationally more expensive to do for both the Controller and the API server.
   • Conceptually simple.
   • Matches a pattern proposed in the in-memory bus controller (Create the in-memory-channel ClusterProvisioner #484).

4. Alter Channel's Channelable to be unique to Channel (i.e. no longer using duck.Channelable). It is identical to duck's Channelable, but it adds a subscriptionName parameter into each entry in Subscriptions and uses patchStrategy:"merge" patchMergeKey:"subscriptionName".

5. Add a key field to duck's ChannelSubscriberSpec. Then set Subscriptions to patchStrategy:"merge" patchMergeKey:"key".

   • For Eventing, key would be in the format 'subscriptionNamespace/subscriptionName'.

My personal preference is the last solution, as I think anything that tries to use this duck type will face the same issue.

Finalizer

Regardless of which solution we choose, we will likely need to add a finalizer to Subscriptions, so that the Controller syncs on Subscription deletion.

[adamharwayne]

/assign @vaikas-google

[grantr]

This seems related to knative/pkg#96