Error handling for Sequence&Parallel

[denny-lclin]

Problem
There is no 'delivery' segment in Sequence&Parallel as in Broker/Subcription. Maually modify a generated by Sequence&Parallel would failed to pass the admission webhook validation (could not be patched: admission webhook "validation.webhook.eventing.knative.dev" denied the request: validation failed: Immutable fields changed)

Persona:
Which persona is this feature for?
System Integrator
Exit Criteria
A measurable (binary) test that would indicate that the problem has been resolved.

Time Estimate (optional):
How many developer-days do you think this may take to resolve?

Additional context (optional)
Add any other context about the feature request here.

[grantr]

Thanks for the request @denny-lclin! Where would you like the delivery spec to apply in the sequence? At every step, or only some steps? Do you need to be able to override delivery spec for a single step?

[denny-lclin]

Hi @grantr, I think a format like

apiVersion: flows.knative.dev/v1
kind: Sequence
metadata:
  namespace: knative-demo
  name: sequence
spec:
  channelTemplate:
    apiVersion: messaging.knative.dev/v1
    kind: InMemoryChannel
  steps:
    - ref:
        apiVersion: serving.knative.dev/v1
        kind: Service
        name: sequence-first
      delivery: # Optional
        retry: 5
        backoffPolicy: exponential 
        backoffDelay: "PT0.5S"
        deadLetterSink:
          ref:
            apiVersion: serving.knative.dev/v1
            kind: Service
            name: error-handler
    - ref:
        apiVersion: serving.knative.dev/v1
        kind: Service
        name: sequence-second
      delivery: # Optional
        retry: 5
        backoffPolicy: exponential 
        backoffDelay: "PT0.5S"
        deadLetterSink:
          ref:
            apiVersion: serving.knative.dev/v1
            kind: Service
            name: error-handler-2

is reasonable for common use case.

And a per sequence/parallel delivery config like

apiVersion: flows.knative.dev/v1
kind: Sequence
...
spec:
  ...
  steps:
    ...
  delivery:
    retry: 5
    backoffPolicy: exponential 
    backoffDelay: "PT0.5S"
    deadLetterSink:
      ref:
         apiVersion: serving.knative.dev/v1
          kind: Service
          name: error-handler

with the per steps > per sequence overwrite definition will be a nice-to-have feature.

By the way, may I ask that what's the definition of 'event delivery failure' in knative ecosystem ? (Or where can I look it up) I've discovered that the http response without necessary attribute for CloudEvent would be considered as failure by experiments but I'd like a formal definition.
And another question is that does KNative encourage users to use Channel&Subscription directly or we should just use high level objects like Broker+Trigger/Sequence/Parallel. Although I see some use cases in some knative relaitve examples, I don't find any examples of Channel+Subscription in official docs.

[vaikas]

Given the focus on getting the API to v1, I don't think we can make these changes prior to it. But this seems like a useful feature and it's been discussed just right now there doesn't seem to be resources to get this in (including the docs, tests, etc. etc.).
As far as preference for what abstractions to use, I personally think that Broker/Trigger model is easier to reason about.
Lastly, our apologies, the docs are still a bit lacking and we're working hard to bring them up to the desired level.
One thing that you may find useful in the meantime (it's very rough) about some differences between the models:
https://docs.google.com/document/d/15sLf_1HMV94gdP_FW0P2Ldo62jjCu8PbmNOGQ1wSBfc/edit#heading=h.wowd2jl072p0

[github-actions]

This issue is stale because it has been open for 90 days with no
activity. It will automatically close after 30 more days of
inactivity. Reopen the issue with /reopen. Mark the issue as
fresh by adding the comment /remove-lifecycle stale.

[lionelvillard]

/remove-lifecycle stale

[tzununbekov]

/assign

[tzununbekov]

Looks like per-step delivery was added to both Sequence and Parallel: #3146, #3148
Are we interested in a per-object delivery config?

[lionelvillard]

I like this for a consistency point of view, though I'm not sure there is a valid use case for this particular override.

AFAIR, we still haven't properly define what's overriding means in the context of delivery options. Independently of whether we think it's a good idea to support per-object delivery config, we should at least start by formally defining it.

WDYT?

[tzununbekov]

I think that the idea of per-object delivery was to make it a default config that's applied on every step. And if step has its own delivery, it overrides default values for this particular step. I don't see any other ways to make them work together actually.

[github-actions]

This issue is stale because it has been open for 90 days with no
activity. It will automatically close after 30 more days of
inactivity. Reopen the issue with /reopen. Mark the issue as
fresh by adding the comment /remove-lifecycle stale.

[tzununbekov]

Doesn't look like I'll have time for this one anytime soon

/unassign

[github-actions]

This issue is stale because it has been open for 90 days with no
activity. It will automatically close after 30 more days of
inactivity. Reopen the issue with /reopen. Mark the issue as
fresh by adding the comment /remove-lifecycle stale.

[pierDipi]

/remove-lifecycle stale

[github-actions]

This issue is stale because it has been open for 90 days with no
activity. It will automatically close after 30 more days of
inactivity. Reopen the issue with /reopen. Mark the issue as
fresh by adding the comment /remove-lifecycle stale.