Update (rewrite) eventing specs #25

evankanderson · 2021-05-09T21:57:12Z

This builds on #24

Changes

Remove helper.md and sources.md, as it's not clear how much here needs to be standardized.
Combine remaining control-plane documents (broker.md, channel.md, and interfaces.md) into a single control-plane.md file, similar to serving.
All the changes from Update (rewrite) overview, motivation, and data plane #24
Introduce a bunch of MAY/MUST language in addition to SHOULD.

This should probably be considered a rewrite; there are a number of places where I either clarified or rewrote APIs (in particular, status and deadLetterSink, which were not handled consistently).

/kind cleanup api-change enhancement

Fixes knative/eventing#4595

Release Note

Rewrite eventing control plane and event routing spec.

Docs

This may need to search & replace a few links in knative/docs. Assuming that the update looks reasonable, I'll add a PR here to do that in the next day or two.

…l plane now.

evankanderson · 2021-05-09T21:57:41Z

/assign @vaikas @n3wscott @lionelvillard

vaikas

Thanks!!

specs/eventing/control-plane.md

vaikas · 2021-05-10T10:13:53Z

specs/eventing/control-plane.md

+the `spec.subscriber.ref` field points to a resource which does not exist or
+cannot be resolved via [Addressable resolution](#addressable-resolution), the
+Trigger MUST set the `Ready` condition to `false`, and at least one condition
+should indicate the reason for the error.


Just jotting this down so I don't forget, but we should also mention about the Dependency Annotation errors.
Specifying the annotation: knative.dev/dependency

This never made it to spec :(

Can you point to either code or doc for this?

I'm guessing this is knative/eventing#4486 ?

How critical is it to have this in the spec, vs it being a documented addition by the OSS software? i.e. is this a foundational part of the interface, or a make-things-easier for the current implementation?

vaikas · 2021-05-10T10:37:32Z

specs/eventing/control-plane.md

+[metav1.ObjectMeta](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.21/#objectmeta-v1-meta)
+resource.
+
+### Spec:


There's also the dependency annotation that I think I mentioned in the other PR or somewhere else here?

knative/eventing#4486

(which is still open)

If it's important, I'm willing to enforce it on all specs.

specs/eventing/control-plane.md

vaikas · 2021-05-10T10:41:38Z

specs/eventing/control-plane.md

+## duckv1.Destination
+
+Destination is used to indicate the destination for event delivery. This is an
+exclusive union; excatly one field MUST be set.


nit, typo: excatly => exactly

vaikas · 2021-05-10T10:43:11Z

specs/eventing/control-plane.md

+## duckv1.Destination
+
+Destination is used to indicate the destination for event delivery. This is an
+exclusive union; excatly one field MUST be set.


It's not an exclusive union.
https://github.com/knative/pkg/blob/main/apis/duck/v1/destination.go#L25

Whoops, thanks!

vaikas · 2021-05-10T10:47:43Z

specs/eventing/control-plane.md

+  <tr>
+    <td><code>attributes</code></td>
+    <td>map[string]string</td>
+    <td>Event filter using exact match on event context attributes. Each key in the map is compared with the equivalent key in the event context. An event passes the filter if the event attribute values are equal to the specified values.</td>


If there's an empty string, it means match like '*'
knative/eventing#4847

Thanks, done.

lionelvillard · 2021-05-10T14:44:04Z

specs/eventing/data-plane.md

+
+- Knative Eventing aims to enable highly-reliable event processing workflows. As
+  such, it prefers duplicate delivery to discarded events. The CloudEvents spec
+  does not take a stance here.


I found this a bit too restrictive. In particular, the IMC-based broker offers no such guarantees.

It does prefer them :) They are just not persisted anywhere, but point taken :)

The in-memory-controller SHOULD do the best it can to prefer duplicate delivery to discarded events. This means that it should implement retry, dead-letter, etc to the best of its ability.

Of course, its durability is limited by the backing storage, but that's true of other platforms like Kafka, too (i.e. if you run Kafka with default.replication.factor=1 and lose a drive, there's also data loss).

And it's even harder to have guarantees if you have to worry about flooding, meteor strikes, etc.

There are use cases for at-most-once delivery guarantee, which prefer losing events to duplicate delivery.

There are such use cases, but Knative eventing doesn't particularly design for that use case, which is different.

specs/eventing/data-plane.md

lionelvillard · 2021-05-10T15:04:45Z

specs/eventing/data-plane.md

+
+Event recipients MUST use the HTTP response code to indicate acceptance of an
+event. The recipient MUST NOT return a response accepting the event until it has
+handled event (processed the event or stored in stable storage). The following


I don't know how this can be enforced and IMO it's too restrictive.

I agree that it might be hard to prove that an implementation was playing fast and loose. I can switch this to SHOULD if you'd prefer.

I think the objection about IMC is a red herring; there are a lot of other decisions (retry, timeout, etc) which could have a higher impact on failure rate than the pod lifetime, depending on the environment.

specs/eventing/data-plane.md

specs/eventing/control-plane.md

n3wscott · 2021-05-10T21:42:20Z

specs/eventing/control-plane.md

+1. Attempt delivery to the `status.subscriberUri` URL following the
+   [data plane contract](./data-plane.md).
+
+   1. If the event delivery fails with a retriable error, it SHOULD be retried


MUST? Should this SHOULD be a MUST?

Good question. I'm happy to promote this to a MUST, but I wasn't sure how e.g. crashing in the middle of a delivery attempt might affect these parameters.

specs/eventing/control-plane.md

evankanderson · 2021-07-20T22:16:10Z

I just discovered that I (somehow) flubbed duckv1.KReference; it had somehow had an address field instead of name. More importantly, it turns out a KReference does include a namespace field, so I'll go back through the comments where I asserted that there weren't cross-namespace references and correct and reopen those comments.

…es *server-side* requirements.

evankanderson · 2021-07-20T23:22:20Z

I updated the language in control-plane.md to highlight that these requirements were for server-side implementation.

aslom · 2021-07-22T16:01:39Z

I opened issue #28 about supporting future upgrades, version mixing and version detection.

It seems it is not supported today? What happens in future when Knative is upgraded and event processor were built to support 1.0 (sources, cloud apps that act sinks etc.) - can Knative adapt to mixed 1.0 and newer event processors?

specs/eventing/data-plane.md

evankanderson · 2021-07-22T18:41:21Z

I owe cleaning up (per KTC notes):

Outstanding big discussions

Subscription.reply magic
Last column of detailed resource tables
Generation… fields

duglin · 2021-07-22T18:46:49Z

On the TM call today we agreed to merge the PR (once Evan gives me the "ok") and then work on the outstanding items in follow-on PRs.

Evan (speaking for Brenda) gave VMware's approval
Spencer gave Google's approval
Doug gave IBM's approval

duglin · 2021-07-22T18:47:22Z

HUGE thanks to @evankanderson for all his work and patience in this process!!!!

evankanderson · 2021-07-22T19:18:34Z

I opened issue #28 about supporting future upgrades, version mixing and version detection.

It seems it is not supported today? What happens in future when Knative is upgraded and event processor were built to support 1.0 (sources, cloud apps that act sinks etc.) - can Knative adapt to mixed 1.0 and newer event processors?

I think the 1.1 spec needs to take 1.0 compatibility into account; I don't think there's anything we can do today to ensure that any 1.1 would be compatible with this 1.0.

evankanderson · 2021-07-22T21:50:56Z

I updated:

Last column of detailed resource tables
So I have "Remove Subscription.reply magic" and "Document generation/observedGeneration" left for separate PRs.

duglin · 2021-07-22T22:41:47Z

/lgtm
/approve

knative-prow-robot · 2021-07-22T22:42:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: duglin, evankanderson

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:

~~OWNERS~~ [duglin,evankanderson]

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

devguyio

I know I am late to the party and this looks like a very late review so I'm gonna leave my comments fwiw. Late better than never. I only reached 1/4th of the control plane. Will keep adding my comments in batches.

devguyio · 2021-07-22T17:10:48Z

specs/eventing/control-plane.md

+example_ of the minimal profile rather than a requirement. This Role is
+sufficient to develop, deploy, and manage event routing for an application
+within a single namespace. Knative Conformance tests against "MUST", "MUST NOT",
+"SHALL", "SHALL NOT", and "REQUIRED" conditions are expected to pass when using


what does expected here mean? is it a "SHOULD" ? Would this affect conformance? i.e. if the RBAC profile needed is different or stricter, would it still be compliant with this spec? specially given that it states above that this is an example

Kubernetes RBAC is not a requirement for Knative Eventing. The conformance requirement is that there is a cluster and a user set up (with a KUBECONFIG file for auth) which the tests execute aginst.

It would be possible (for example) to have a conformant cluster which implemented no access control, only resource validation. On the other hand, a cluster where Channel / Subscription are forbidden to all users can't pass the conformance tests and so isn't conformant, even if it's running code which might otherwise be conformant.

devguyio · 2021-07-23T05:24:13Z

specs/eventing/control-plane.md

+and MUST attempt to forward the received events for filtering to each associated
+Trigger whose `Ready` condition is `true`. As described in the
+[Trigger Lifecycle](#trigger-lifecycle) section, a Broker MAY forward events to
+an associated Trigger destination which which does not currently have a `true`


Suggested change

an associated Trigger destination which which does not currently have a `true`

an associated Trigger destination which does not currently have a `true`

devguyio · 2021-07-23T05:29:46Z

specs/eventing/control-plane.md

+`Ready` condition, including events received by the Broker before the Trigger
+was created.
+
+The annotation `eventing.knative.dev/broker.class` SHOULD be used to select a


Does that mean that a platform which ignores the annotation and forces users to use a specific Broker regardless of other installed Knative brokers is considered Knative compliant? Is there a reason this is not a MUST ?

Yes.

In particular, there's been discussion at KTC about alternate implementations which might or might not use the OSS code. You can imagine an implementation which only offers a single Broker type and ignores the annotation.

It's not clear how, from a conformance-testing point of view, we'd enforce a MUST here.

devguyio · 2021-07-23T07:06:32Z

specs/eventing/control-plane.md

+`spec.delivery.deadLetterSink` as described in
+[Destination resolution](#destination-resolution) before setting the `Ready`
+condition to `true`. If any of the addressable fields fails resolution, the
+Subscription MUST set the `Ready` condition to `false`, and at least one


Is there a reason here false is used while on 219:220 MUST NOT be true is used?

219-220 says:

if the channel does not exist or its Ready condition is not true, the Subscription's Ready condition MUST NOT be true.

There are three values for a condition: true, false, and "" (unknown). If a Channel's Ready status is unknown, a Subscription's Ready status could also reasonably be unknown.

Here, we definitively know that there is a problem (resolution failed, i.e. it's not in progress anymore), so we should affirmatively set Ready to false, rather than allowing it to be unknown.

Good question, my the way -- this tri-state nature of conditions is subtle!

devguyio · 2021-07-23T10:34:13Z

specs/eventing/control-plane.md

+`status.physicalSubscription` URIs to the empty string if the corresponding
+`spec` reference cannot be resolved.
+
+At least one of `spec.subscriber` and `spec.reply` MUST be set; if only


the behavior is equivalent to setting spec.subscriber

Does that mean that it's a MUST? i.e. the channel controller MUST append to the subscribers the one specified in reply? if so, is there a reason MUST is not used here?

See #30; Doug pointed out that this would be simpler to require spec.subscriber all the time, and Lionel / Ville agreed on Slack.

We can revisit in #30 if we think there's a good reason to allow subscriber to be unset and reply set instead.

devguyio · 2021-07-23T11:52:55Z

specs/eventing/control-plane.md

+Once created, the Subscription's `spec.channel` MUST NOT permit updates; to
+change the `spec.channel`, the Subscription MUST be deleted and re-created. This
+pattern is chosen to make it clear that changing the `spec.channel` is not an
+atomic operation, as it might span multiple storage systems. Changes to


If I may ask, what do you mean by it might span multiple storage systems here?

If you change:

spec: channel: apiVersion: messaging.knative.dev/v1 kind: Channel name: foo-kafka ...

to

spec: channel: apiVersion: messaging.knative.dev/v1 kind: Channel name: foo-in-memory ...

Even if the same events are delivered to foo-kafka and foo-in-memory, changing the Subscription from one to the other may cause duplicate or missed events from the logical stream.

We could cut this justification, since there's nothing required in that block.

devguyio · 2021-07-23T11:56:09Z

specs/eventing/control-plane.md

+the Subscription or the Channel), the Subscription MUST only set the `Ready`
+condition to `true` after the Channel has been configured to send all future
+events to the Subscription's `spec.subscriber`. The Channel MAY send some events
+to the Subscription before prior to the Subscription's `Ready` condition being


to the Subscription before prior to the Subscription's Ready condition being

Typo, delete prior or before.

evankanderson · 2021-07-25T16:51:42Z

I know I am late to the party and this looks like a very late review so I'm gonna leave my comments fwiw. Late better than never. I only reached 1/4th of the control plane. Will keep adding my comments in batches.

No problem, I'm happy to make fix-up PRs (or you can). This started as a clearnup, but I discovered that both the content (all SHOULD requirements) and the structure (mixed control and data plane requirements, schemas by reference).

I'm hopeful that all the PRs going forward can be smaller or more focused. 😁

devguyio

@evankanderson fwiw, I realized that I had these two questions pending and were not submitted.

devguyio · 2021-07-23T12:23:42Z

specs/eventing/control-plane.md

+Broker can retry event delivery beyond the duration of receiving the event).
+
+For each event received by the Broker, the Broker MUST evaluate each associated
+Trigger **exactly once** (where "associated" means a Trigger with a


Is there a reason behind this exactly once ? What about cases where the broker introduces some trigger filters optimization and/or caches the filters state? I can imagine for some cases the Broker would choose to not even evaluate any associated Trigger. Am I missing something?

devguyio · 2021-07-23T13:53:17Z

specs/eventing/control-plane.md

+
+### Event Delivery
+
+Once a Trigger or Subscription has decided to deliver an event, it MUST do the


Is there a reason behind being explicit that:

The trigger is the deciding component? Can't it be the broker itself in some designs based on the filters?

That step 1. needs to take place each time a delivery decision is taken? For instance what if object's status are cached and not read for every event delivery?

Signed-off-by: Alfusainey Jallow <alf.jallow@gmail.com>

Evan Anderson and others added 11 commits April 30, 2021 16:53

Checkpoint work on eventing specs

99aea7b

More work on data plane & control plane. Mostly need to finish contro…

e07658d

…l plane now.

Add at-least-once detail on data plane.

70d7e8b

Finish off most of lifecycle

dc743ca

Update (rewrite) overview, motivation, and data plane

8ef8f61

Update lifecycle to be a bit clearer and cover more fields

86f9c55

Fill in detailed resources.

e90a3ad

Document event delivery (Broker & Channel)

0d7b246

Updates from n3wscott review

0a33307

Clean up no longer needed files.

7e9b26b

Updates from n3wscott review

4964fd7

google-cla bot added the cla: yes Indicates the PR's author has signed the CLA. label May 9, 2021

knative-prow-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label May 9, 2021

knative-prow-robot assigned lionelvillard, n3wscott and vaikas May 9, 2021

vaikas reviewed May 10, 2021

View reviewed changes

lionelvillard reviewed May 10, 2021

View reviewed changes

specs/eventing/data-plane.md Show resolved Hide resolved

lionelvillard reviewed May 10, 2021

View reviewed changes

Evan Anderson added 2 commits May 10, 2021 10:35

Update data-plane language per Ville's suggestions.

6216969

Overview revisions based on vaikas feedback.

e776f36

n3wscott reviewed May 10, 2021

View reviewed changes

Evan Anderson added 2 commits May 10, 2021 15:29

Merge remote-tracking branch 'origin/re-spect-part-1' into re-spect

f4c3b09

Merge branch 'main' of https://github.com/knative/specs into re-spect

8cffd9c

A few updates to clarify that the control-plane specification indicat…

8aa07df

…es *server-side* requirements.

Require that Subscriptions reference Channels in the same namespace.

7a678bd

duglin reviewed Jul 22, 2021

View reviewed changes

specs/eventing/data-plane.md Outdated Show resolved Hide resolved

duglin reviewed Jul 22, 2021

View reviewed changes

specs/eventing/data-plane.md Outdated Show resolved Hide resolved

Allow additional Destinaiton resolution mechanisms.

fad55f5

Evan Anderson added 4 commits July 22, 2021 14:35

Fix two Doug nits.

8333aa7

Remove schema requirements.

bef17aa

Align SHOULD NOT / MUST NOT

ac54635

Capitalize "MAY"

f5d15dc

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 22, 2021

knative-prow-robot merged commit c348f50 into knative:main Jul 22, 2021

devguyio reviewed Jul 23, 2021

View reviewed changes

evankanderson mentioned this pull request Jul 25, 2021

Require that spec.subscriber is set for Subscription. #30

Merged

This was referenced Jul 25, 2021

Mention management of metadata.generation and status.observedGeneration. #31

Open

Fixes noticed by Ahmed #32

Merged

devguyio reviewed Aug 11, 2021

View reviewed changes

antoineco mentioned this pull request May 20, 2022

Dead link to Broker spec at Eventing / Brokers / About Brokers knative/docs#4969

Closed

antoineco mentioned this pull request May 27, 2022

Broker Spec Link broken knative/docs#4942

Closed

odacremolbap pushed a commit to odacremolbap/specs that referenced this pull request Jun 19, 2022

Remove hardcoded GOPATH from developer docs. (knative#25)

f02d452

Signed-off-by: Alfusainey Jallow <alf.jallow@gmail.com>

	an associated Trigger destination which which does not currently have a `true`
	an associated Trigger destination which does not currently have a `true`


		### Event Delivery

		Once a Trigger or Subscription has decided to deliver an event, it MUST do the

Update (rewrite) eventing specs #25

Update (rewrite) eventing specs #25

Conversation

evankanderson commented May 9, 2021

Changes

evankanderson commented May 9, 2021

vaikas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evankanderson commented Jul 20, 2021

evankanderson commented Jul 20, 2021

aslom commented Jul 22, 2021

evankanderson commented Jul 22, 2021

duglin commented Jul 22, 2021

duglin commented Jul 22, 2021

evankanderson commented Jul 22, 2021

evankanderson commented Jul 22, 2021

duglin commented Jul 22, 2021

knative-prow-robot commented Jul 22, 2021

devguyio left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devguyio Jul 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evankanderson commented Jul 25, 2021

devguyio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devguyio left a comment •

edited

Loading

devguyio Jul 23, 2021 •

edited

Loading