Add Choice CRD by lionelvillard · Pull Request #1529 · knative/eventing

lionelvillard · 2019-07-10T20:34:45Z

This PR only provides the wiring shown in the design document. Currently there is no guarantee that only one branch will be executed. It's up to the user to provide "proper" filter functions.

(moved open issues to design document)

Proposed Changes

Add new Choice CRD

Release Note

A Choice CRD has been added for defining functions to execute based on some condition.

knative-prow-robot · 2019-07-10T20:34:59Z

Hi @lionelvillard. 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.

Details

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.

lionelvillard · 2019-07-11T18:20:55Z

Removing WIP. The limitations have been captured in the design document and can be addressed in other PRs.

lionelvillard · 2019-07-11T20:59:48Z

/ok-to-test

lionelvillard · 2019-07-17T17:53:19Z

@vaikas-google can you take a look at this?

vaikas

Thanks and my most sincere apologies for this having taken sooooo long... life... 😞
Here's some thoughts and questions about the shape of the API while I'm trying to understand all the everythings going on here.

vaikas · 2019-07-25T17:27:29Z

+
+// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object
+
+// ChoiceList is a collection of Sequences.


nit: looks like copy / paste error here?

vaikas · 2019-07-25T17:29:02Z

+    - name: Reason
+      type: string
+      JSONPath: ".status.conditions[?(@.type==\"Ready\")].reason"
+    - name: Hostname


should we be trying to use the URL now that it's seemingly mature in the v1beta1 of serving?

vaikas · 2019-07-25T17:55:40Z

+	// ChannelTemplate specifies which Channel CRD to use
+	ChannelTemplate ChannelTemplateSpec `json:"channelTemplate"`
+
+	// Reply is a Reference to where the result of the last Subscriber of a given branch gets sent to.


The comment 'a given branch' would imply that this is per branch, but it seems like by setting this each branch will "terminate" here. Is this really what we want, or should the reply be maybe overrideable at the Case level and if not specified, use the Reply here and then all Cases will terminate to this Reply?

in the design document I described both cases (Single vs multiple prongs section) as being mutually exclusive, either use a global Reply or per branch. I don't see any problems to have the case reply overriding the global reply, wrt. to composability for instance. Also error-handling will be much easier to implement.

vaikas · 2019-07-25T17:59:38Z

+	// Reconciling choice is pretty straightforward, it does the following things:
+	// 1. Create a channel fronting the whole choice
+	// 2. For each of the Cases:
+	//     2.1 create a Subscription to the previous Channel and subscribe the filter


"previous Channel" -> channel in step 1? As in, for Sequence it made sense to refer to previous channel, but here there's only one channel fronting the choice, then all Cases subscribe to it?

yup bad comment.

vaikas · 2019-07-25T18:00:25Z

+	// 1. Create a channel fronting the whole choice
+	// 2. For each of the Cases:
+	//     2.1 create a Subscription to the previous Channel and subscribe the filter
+	//     2.2 create a Subscription to the filter Channel and subcribe the subscriber


can't step 4 be done here and set the reply as part of this step?

... time passes... looks like there's no step 4 and it's instead done as part of the step 2.2 above?

vaikas · 2019-07-25T18:27:21Z

+	// * Conditions - the latest available observations of a resource's current state.
+	duckv1beta1.Status `json:",inline"`
+
+	// ChoiceStatuses is an array of corresponding Subscription statuses.


Wonder if it would be better to have a ChoiseCaseStatus and then an array here. I think it's little more complicated than just subscriptionstatus/channelstatus, because each Case actually has 2 subscriptions? It has one that subscribes the filter to the "ingress" channel, and then there's a subscription created from filter (and I'd assume a channel between the filter / "actual" subscriber), so at the very least it seems like you'd need to represent state of 2 subscriptions in each CaseStatus as well as probably one channel (between filter / "actual" subscription) and maybe a Channel after the "actual" subscriber that a Reply gets subscribed to?

yup agree. The flat array was just to get started.

We might also want to propagate the Reply status to make it clear which one has been applied (global reply or case reply).

vaikas

one small change (unless you want to add the default channel behaviour now?)

vaikas · 2019-07-26T16:54:50Z

+	Cases []ChoiceCase `json:"cases"`
+
+	// ChannelTemplate specifies which Channel CRD to use
+	ChannelTemplate eventingduckv1alpha1.ChannelTemplateSpec `json:"channelTemplate"`


Take a look at:
#1569
Might as well add the default channel support to make these easier to use.
TODO & issue would be fine for now if you don't want to add it. But I'd at least change this to a pointer for now to make the transition easier.

vaikas · 2019-07-26T19:45:10Z

+	// FilterSubscriptionStatus corresponds to the filter subscription status.
+	FilterSubscriptionStatus ChoiceSubscriptionStatus `json:"filterSubscriptionStatus"`
+
+	// ChannelStatus corresponds to the filter channel status.


nit: FilterChannelStatus

vaikas · 2019-07-26T19:49:30Z

+var _ runtime.Object = (*Choice)(nil)
+var _ webhook.GenericCRD = (*Choice)(nil)
+
+type ChoiceSpec struct {


Please file a tracking bug for documenting the usage and examples. Feel free to use something like this as a starting point:
https://github.com/knative/docs/blob/master/docs/eventing/sequence.md

Also, there's a pointer to a simple transformer that might be useful to use in the examples:
instead of step numbers, you could bake in the "branch name" or something:
https://github.com/vaikas-google/transformer

knative-metrics-robot · 2019-07-29T18:31:13Z

The following is the coverage report on pkg/.
Say /test pull-knative-eventing-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage
pkg/apis/messaging/v1alpha1/choice_defaults.go	Do not exist	100.0%
pkg/apis/messaging/v1alpha1/choice_lifecycle.go	Do not exist	98.3%
pkg/apis/messaging/v1alpha1/choice_types.go	Do not exist	100.0%
pkg/apis/messaging/v1alpha1/choice_validation.go	Do not exist	85.7%
pkg/reconciler/choice/choice.go	Do not exist	72.4%
pkg/reconciler/choice/controller.go	Do not exist	100.0%

n3wscott · 2019-07-29T18:44:41Z

+	//  with a reference to the Addressable duck type. If the resource does not meet this contract,
+	//  it will be reflected in the Subscription's status.
+	// +optional
+	Reply *corev1.ObjectReference `json:"reply,omitempty"`


Does it make sense for choice to support multi-leg Subscriber/Reply?

I was going back and forth on this one and I think one good use case is error-handling, which can be done by having one leg sending the reply to an error sink when the event is an error (TBD).

vaikas

Could you create an issue to add an e2e test for choice, like:
https://github.com/knative/eventing/blob/master/test/e2e/sequence_test.go

vaikas · 2019-07-29T20:48:59Z

+	// provided, until one pass (returns true)
+	Cases []ChoiceCase `json:"cases"`
+
+	// ChannelTemplate specifies which Channel CRD to use


Thanks for changing this, could you also add a comment here about the default behaviour since it's an API file.

vaikas · 2019-07-29T21:07:05Z

/approve
leaving lgtm to @n3wscott since he had comments. Doc changes and e2e tests can be done in a followup PR.

knative-prow-robot · 2019-07-29T21:07:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lionelvillard, vaikas-google

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [vaikas-google]

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

n3wscott · 2019-07-29T21:09:11Z

/lgtm

I would like to play with this, there might be changes in the future but given our limitations today I think this is a good starting spot.

Signed-off-by: Matthias Wessendorf <mwessend@redhat.com> Co-authored-by: Matthias Wessendorf <mwessend@redhat.com>

Signed-off-by: red-hat-konflux-kflux-prd-rh02 <190377777+red-hat-konflux-kflux-prd-rh02[bot]@users.noreply.github.com> Co-authored-by: red-hat-konflux-kflux-prd-rh02[bot] <190377777+red-hat-konflux-kflux-prd-rh02[bot]@users.noreply.github.com>

knative-prow-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 10, 2019

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Jul 10, 2019

knative-prow-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jul 10, 2019

knative-prow-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 10, 2019

knative-prow-robot requested review from grantr and matzew July 10, 2019 20:35

lionelvillard changed the title ~~WIP: add Choice CRD~~ Add Choice CRD Jul 11, 2019

knative-prow-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 11, 2019

lionelvillard force-pushed the choice branch from dbfc95f to b8394a8 Compare July 11, 2019 18:19

knative-prow-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 11, 2019

aslom mentioned this pull request Jul 12, 2019

Provide builtin filter function(s) #1527

Closed

lionelvillard force-pushed the choice branch from b8394a8 to 936e473 Compare July 15, 2019 17:17

lionelvillard mentioned this pull request Jul 15, 2019

Be able to easily define branches #1525

Closed

lionelvillard force-pushed the choice branch 3 times, most recently from c7479bd to 95b6a80 Compare July 17, 2019 14:49

lionelvillard mentioned this pull request Jul 24, 2019

Broker internal design improvements #1555

Closed

vaikas reviewed Jul 25, 2019

View reviewed changes

lionelvillard force-pushed the choice branch 3 times, most recently from e4e0d87 to 4e259bd Compare July 26, 2019 13:20

vaikas reviewed Jul 26, 2019

View reviewed changes

lionelvillard added 3 commits July 29, 2019 14:00

WIP: add Choice CRD

46c68ad

add more tests

f3ff02c

add support for case reply. Misc changes

42e7f27

lionelvillard added 2 commits July 29, 2019 14:00

add more tests

7295f90

add support for default channel template

9f673d9

lionelvillard force-pushed the choice branch from 246bd24 to 9f673d9 Compare July 29, 2019 18:18

lionelvillard mentioned this pull request Jul 29, 2019

Document Choice CRD knative/docs#1623

Closed

fix typo

e0a6dd4

n3wscott reviewed Jul 29, 2019

View reviewed changes

vaikas reviewed Jul 29, 2019

View reviewed changes

lionelvillard mentioned this pull request Jul 29, 2019

Add E2E tests for Choice CRD #1590

Closed

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 29, 2019

knative-prow-robot assigned n3wscott Jul 29, 2019

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

knative-prow-robot merged commit ecd7c29 into knative:master Jul 29, 2019

lionelvillard deleted the choice branch January 20, 2020 14:59

pierDipi pushed a commit to pierDipi/eventing that referenced this pull request Dec 7, 2021

clap: update to 1.19 product build (knative#1529)

e404a3f

Signed-off-by: Matthias Wessendorf <mwessend@redhat.com> Co-authored-by: Matthias Wessendorf <mwessend@redhat.com>


		// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object

		// ChoiceList is a collection of Sequences.

Conversation

lionelvillard commented Jul 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed Changes

Uh oh!

knative-prow-robot commented Jul 10, 2019

Uh oh!

lionelvillard commented Jul 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lionelvillard commented Jul 11, 2019

Uh oh!

lionelvillard commented Jul 17, 2019

Uh oh!

vaikas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vaikas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

knative-metrics-robot commented Jul 29, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vaikas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vaikas commented Jul 29, 2019

Uh oh!

knative-prow-robot commented Jul 29, 2019

Uh oh!

n3wscott commented Jul 29, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

lionelvillard commented Jul 10, 2019 •

edited

Loading

lionelvillard commented Jul 11, 2019 •

edited

Loading