Bring back ContainerSource?

[nachocano]

Problem
In 0.12 we deprecated ContainerSource in favor of SinkBinding. Once 0.13 was cut, we removed ContainerSource #2693, according to our deprecation guidelines.
Thanks to @matzew that started the migration plan, I think we can now easily compare both solutions, and revisit whether we made the right call or not.

My opinion is that we should bring back ContainerSource as it provides a clearer UX for (non-advanced) Developers, whom don't know about core K8s constructs (nor need to learn about them).
IMO we made a step forward in adding SinkBinding and all its power, but two steps back when we decided to remove ContainerSource in lieu of the former.

From the migration doc we can clearly see both experiences:

1- ContainerSource

apiVersion: sources.eventing.knative.dev/v1alpha1
kind: ContainerSource
metadata:
  name: urbanobservatory-event-source
spec:
  image: quay.io/openshift-knative/knative-eventing-sources-websocketsource:latest
  args:
    - '--source=wss://api.usb.urbanobservatory.ac.uk/stream'
    - '--eventType=my.custom.event'
  sink:
    apiVersion: serving.knative.dev/v1
    kind: Service
    name: wss-event-display

vs.

2- SinkBinding + Deployment

apiVersion: sources.knative.dev/v1alpha2
kind: SinkBinding
metadata:
  name: bind-wss
spec:
  subject:
    apiVersion: apps/v1
    kind: Deployment
    selector:
      matchLabels:
        app: wss
  sink:
    ref:
      apiVersion: serving.knative.dev/v1
      kind: Service
      name: wss-event-display

AND

apiVersion: apps/v1
kind: Deployment
metadata:
  name: wss
  labels:
    app: wss
spec:
  selector:
    matchLabels:
      app: wss
  template:
    metadata:
      labels:
        app: wss
    spec:
      containers:
        - image: quay.io/openshift-knative/knative-eventing-sources-websocketsource:latest
          name: wss
          args:
            - '--source=wss://api.usb.urbanobservatory.ac.uk/stream'
            - '--eventType=my.custom.event'

I think most people would argue that option 1 is simpler to use, yet we removed it because SinkBinding covers more cases (bind to any PodSpecable). That should also be possible with a better impl of ContainerSource.
There were thoughts of moving ContainerSource to eventing-contrib. Although I think that is better than not having it at all (and at first I agreed to it), it comes with the problem that it needs its own controller and webhook, thus making it less compelling... And it also wouldn't have native CLI support

Persona:
Developer

Time Estimate (optional):
Decision by EOW (3/13)

Exit Criteria
A decision of whether to bring back ContainerSource to core or not.

Additional context (optional)
#2693
#2499

/cc @vaikas @grantr @lionelvillard as eventing WG leaders, and others.

[grantr]

I think ContainerSource should return as a source in this repo.

We've heard feedback that there are too many concepts to understand when getting started in eventing. Here's a recent one:

Eventing tho... is a giant pile of brain spaghetti with a dozen new abstractions and abstractions of abstractions to learn.

One way to keep users from running away screaming is to reduce the number of concepts they need to understand before they "get it", or feel invested enough that they're excited to explore on their own. IMO the simple case of a container sending to an Addressable is an important bootstrap step to keep new users engaged once they're bored with logging ping events to event-display.

For this user and this use case, ContainerSource is a better interface than SinkBinding+Deployment. Its interface is straightforward without any extraneous features and saves the user from understanding two additional advanced concepts in Deployment and SinkBinding (arguably three with labels and selectors).

If ContainerSource is for new users playing with Knative Eventing, it should be in the default install so tutorials can assume it will always be available regardless of what options the user chose. ContainerSource has no dependencies so there is no disadvantage to including it in all installs. With our current release capabilities, it must live in the knative/eventing repo to be part of the default install.

The reason this whole conversation got started is that we wanted ContainerSource-like reference resolving behavior for other PodSpecable things like KService, Job, CronJob, etc. We don't need that anymore now that we have SinkBinding, so ContainerSource can be focused on the new user case and doesn't need to do any more than the simplest thing.

[duglin]

For a while now I've been thinking that Eventing needs a "Steve".... the way Serving used to.

[matzew]

I still like Container Source Writing the migration shows, converting is not that hard. But I also ended up struggling with a typo in the label, preventing it to work directly Perhaps it makes sense to continue it, for newbies? Thinking a bit more ... perhaps we should also have samples/doc how to write the image for Sinkbinding / ContainerSource ? like the hello world in serving?

On Tue 10. Mar 2020 at 23:57, Grant Rodgers ***@***.***> wrote: I think ContainerSource should return as a source in this repo. We've heard feedback that there are too many concepts to understand when getting started in eventing. Here's a recent one <https://twitter.com/KarlKFI/status/1234686021675012096>: Eventing tho... is a giant pile of brain spaghetti with a dozen new abstractions and abstractions of abstractions to learn. One way to keep users from running away screaming is to reduce the number of concepts they need to understand before they "get it", or feel invested enough that they're excited to explore on their own. IMO the simple case of a container sending to an Addressable is an important bootstrap step to keep new users engaged once they're bored with logging ping events to event-display. For this user and this use case, *ContainerSource is a better interface than SinkBinding+Deployment.* Its interface is straightforward without any extraneous features and saves the user from understanding two additional advanced concepts in Deployment and SinkBinding (arguably three with labels and selectors). If ContainerSource is for new users playing with Knative Eventing, it should be in the default install so tutorials can assume it will always be available regardless of what options the user chose. ContainerSource has no dependencies so there is no disadvantage to including it in all installs. *With our current release capabilities, it must live in the knative/eventing repo to be part of the default install.* The reason this whole conversation got started is that we wanted ContainerSource-like reference resolving behavior for other PodSpecable things like KService, Job, CronJob, etc. We don't need that anymore now that we have SinkBinding, so *ContainerSource can be focused on the new user case and doesn't need to do any more than the simplest thing.* — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2739?email_source=notifications&email_token=AABGPTXLCAHZLZ5IPHD5D4TRG3AW3A5CNFSM4LETPE4KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEONP6KI#issuecomment-597360425>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABGPTS75S4NFMSDHBI3WXTRG3AW3ANCNFSM4LETPE4A> .

-- Sent from Gmail Mobile

[aslom]

I think UX or DX (developer experience) is very important to success of native eventing.

It should be easy to get started, get simple things done, and then build on it to do more complex.

Simple event sources are good striating point if they are simple to create. Having one YAML file that has exactly all I need is simple starting point. Then if users need more flexibility they may start using sink binding and we can show in docs how the two are equivalent? That should provide good learning experience and show eventing building blocks.

[n3wscott]

If you bring it back, it should land in eventing-contrib, not eventing. It should be an optional piece.

[lionelvillard]

I looked at the migration guide (thanks @matzew) and it occurred to me that it only works when the SinkBinding selection mode is set to exclusion. Yet another concept to learn.

[lionelvillard]

TD;LR: +1 for bringing back ContainerSource.

Here my take, assuming the primary use case for ContainerSource is to make writing sources easier (as opposed as being able to consume events from existing Deployments) for the user.

A source usually consists of:

• a runtime producing events (and reporting metrics, logging, etc...)
• a sink
• event types
• delivery options (not yet supported by any sources but we talked about it)

ContainerSource allows the specification of only the two first items (runtime and sink). I believe it should also includes the two last items, event types and delivery options.

In a sense ContainerSource is very similar to Sequence, they are both high-level objects capturing a very common pattern, and as such IMHO both should be part of the eventing core (Sequence is already part of it). They are both complex enough to justify the need for a new type of objects (CRD). It's a win for the user because instead of managing many objects, he/she only have to manage only one (much bigger win for sequence though).

From an implementation PoV, ContainerSource just generates lower-level objects, ie SinkBinding, EventType and a podspecable (eg Deployment, Kservice, etc..). One object instead of three, that's something :-). (maybe more if we also inject Logging and Metrics object, LoggingBinding, MetricsBindings, etc...)

Bottom line, instead of removing ContainerSource I would instead extend it and promote it as being the primary way to implement CRD-less sources, assuming some conditions are met (TBD).

As a bonus, here how it should look like:

apiVersion: sources.knative.dev/v1alpha1
kind: ContainerSource
metadata:
  name: urbanobservatory-event-source
spec:
  template:
    spec:
      containers:
      - image: quay.io/openshift-knative/knative-eventing-sources-websocketsource:latest
        args:
        - '--source=wss://api.usb.urbanobservatory.ac.uk/stream'
        - '--eventType=my.custom.event'
  eventTypes:
  - type: my.custom.event
     schema:
     description:
  delivery:
    deadLetterSink:
      ref:
        apiVersion: serving.knative.dev/v1
        kind: Service
        name: error-handler  
  sink:
    apiVersion: serving.knative.dev/v1
    kind: Service
    name: wss-event-display

Disclaimer: I never used ContainerSource.

[duglin]

would it make sense to include additional flags, such as whether it needs a URL/Route to accept incoming events? E.g. Github vs Cron

[lionelvillard]

I think so.

A flag or would you be happy if the selected podspecable is a Knative Service, which indirectly provide a URL?

[duglin]

being a KnService has some nice aspects to it. We'd just need to also support making it a "private" KnService so people can choose to not expose it on the internet.

However, if I'm understanding how ContainerSources should work, not all of them would require an http server in the image - e.g. Cron. And KnServices don't do well with images w/o http servers. I do have a PoC that supports those though :-) And at that point as long as there was some way to set minScale=maxScale=X then it makes sense to me.

[sebgoa]

Having been MIA for a bit I discovered SinkBinding couple weeks back and I still don't get it. The Deployment still needs to emit CloudEvents so why not use ContainerSource directly. It may be more powerful but is certainly confusing.

+1 on getting ContainerSource back, we use it a ton.

[cab105]

Having spent a little bit of time with eventing over the last few weeks, and looking at ContainerSource as a means of rolling my own event source, I was surprised that the current documentation (tagged as v0.13) makes no mention that this is deprecated let alone slated for removal.

Without proper documentation reflecting the discontinued use other than a one-line item in the release notes, the amount of impact due to the rework will be quite large.

[nachocano]

Today was our ETA for the decision.

Thanks everyone for commenting. Going over the comments, I see that the vast majority of the community considers we should bring ContainerSource back. So let's do so.

I'll take care of it. Will have a PR ready sometime next week.