Trigger responses are sent back into the Broker. by Harwayne · Pull Request #917 · knative/eventing

Harwayne · 2019-03-18T21:03:37Z

Fixes #899.
Fixes #905.

Proposed Changes

Create an internal only Ingress Channel for each Broker.
Trigger's Subscription sets its reply to the Broker's Ingress Channel.

Release Note

Events may not be delivered for a few seconds during a reconcile attempt when this is first deployed.

Tracing Ids will get dropped when going through Triggers.

…w-model

In a follow up PR, I plan to include matching of expressions. It turns our that they are somewhat more involved than expected.

vaikas

Thanks for doing this!

vaikas · 2019-03-19T22:37:34Z

 	// 2. Filter Deployment.
 	// 3. Ingress Deployment.
 	// 4. K8s Services that point at the Deployments.
+	// 5. Ingress Channel is created to get events from Triggers back into this Broker via the


Could we add a note here saying something that this is necessary because subscription can only route to Channel objects and hence we need to create an artificial channel instead of feeding it to the Ingress Service directly. If I understood correctly.

Also, probably open an issue to track about relaxing the constraint that only channels can be used as a target for response from a Subscription.

Added a note and #925.

I think the channel here isn't actually so bad because it decouples the publish and consume failure domains. With the ingress channel, consume doesn't depend on the broker ingress availability, only channel availability (which the consumer already depends on via its subscription to the main trigger channel).

For the intentionally simple "respond with event" use case, having simpler failure modes is good IMO.

knative-metrics-robot · 2019-03-20T15:49:20Z

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	Delta
pkg/apis/eventing/v1alpha1/broker_types.go	76.9%	100.0%	23.1
pkg/broker/receiver.go	96.2%	82.7%	-13.4
pkg/reconciler/v1alpha1/broker/broker.go	86.6%	89.8%	3.2
pkg/reconciler/v1alpha1/trigger/trigger.go	78.9%	79.5%	0.6

Harwayne · 2019-03-20T16:46:45Z

This PR's switch to using the CloudEvents SDK will cause Broker & Trigger to stop propagating trace headers.

vaikas · 2019-03-20T19:40:34Z

/approve
since @grantr had comments, leaving out the LGTM for them.

knative-prow-robot · 2019-03-20T19:40:43Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Harwayne, 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~~ [Harwayne,vaikas-google]

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

vaikas · 2019-03-20T20:04:16Z

talked offline, good to go.
/lgtm

* Nit (knative#4385) Signed-off-by: Francesco Guardiani <francescoguard@gmail.com> (cherry picked from commit 3ceaad4) Signed-off-by: Francesco Guardiani <francescoguard@gmail.com> * emit a k8s event when dropping events (knative#4389) * emit a k8s event when dropping events Signed-off-by: Ville Aikas <vaikas@vmware.com> * go imports Signed-off-by: Ville Aikas <vaikas@vmware.com> * tags Signed-off-by: Ville Aikas <vaikas@vmware.com> * fix silliness Signed-off-by: Ville Aikas <vaikas@vmware.com> * simplify Signed-off-by: Ville Aikas <vaikas@vmware.com> (cherry picked from commit 0a54fd9) * [recordevents] Removed EventBroadcaster usage and replaced with manual send (knative#4393) * Removed EventBroadcaster usage and replaced with manual creation and send of events Signed-off-by: Francesco Guardiani <francescoguard@gmail.com> * Boilerplate Signed-off-by: Francesco Guardiani <francescoguard@gmail.com> * Remove redundant format Removed sequence annotation Signed-off-by: Francesco Guardiani <francescoguard@gmail.com> * Added required value Signed-off-by: Francesco Guardiani <francescoguard@gmail.com> * ?!?! Signed-off-by: Francesco Guardiani <francescoguard@gmail.com> * Maybe this one fix the issue? Signed-off-by: Francesco Guardiani <francescoguard@gmail.com> * Maybe this one fix the issue? Signed-off-by: Francesco Guardiani <francescoguard@gmail.com> * Removed useless double log line Signed-off-by: Francesco Guardiani <francescoguard@gmail.com> * Remove useless headers Signed-off-by: Francesco Guardiani <francescoguard@gmail.com> * Missing host header Signed-off-by: Francesco Guardiani <francescoguard@gmail.com> * Nit Signed-off-by: Francesco Guardiani <francescoguard@gmail.com> * Now it works on my machine, i'm warning you prow! Signed-off-by: Francesco Guardiani <francescoguard@gmail.com> * Nit Signed-off-by: Francesco Guardiani <francescoguard@gmail.com> * Now it works for long events too Signed-off-by: Francesco Guardiani <francescoguard@gmail.com> * Suggestions Signed-off-by: Francesco Guardiani <francescoguard@gmail.com> * Fixed the dropped counter thing Signed-off-by: Francesco Guardiani <francescoguard@gmail.com> * Nit Signed-off-by: Francesco Guardiani <francescoguard@gmail.com> (cherry picked from commit 7de59ec) Signed-off-by: Francesco Guardiani <francescoguard@gmail.com> * Wrong merge fix Signed-off-by: Francesco Guardiani <francescoguard@gmail.com> Co-authored-by: Ville Aikas <11279988+vaikas@users.noreply.github.com>

Harwayne and others added 30 commits January 28, 2019 09:13

Initial API outline for Broker.

d7ed39b

Metadata is exported

080f5eb

Initial Trigger API.

f20a2df

Remove old comments.

163474d

Register the types.

5f53e05

Initial work on the Broker controller.

2f6a03f

Initial work on the Trigger controller.

3ee0b8b

Initial work on the new model and its corresponding broker controller.

da9fe36

Initial work on the Trigger controller.

f8cca2e

Add simple mains.

d7ebdcb

Small fixes, still not working.

c5f7a83

Add the Istio injection annotation.

054c10d

Resolve subscriber in the Trigger controller.

942b979

Standardize on 'Any'.

afd71af

Make Broker and Trigger generational

7a1f69b

Happy control path.

2be56bd

TargetPort is 8080

10acfa3

Watch channels.

2108d83

Custom service account for filter (needs trigger watch).

6aa7086

Increase logging in the Filter to debug level.

3cd236f

Use the default channel provisioner.

be350a0

Adding filtering using k8s label selectors

0068232

Watch namespaces and create a default Broker.

b4e2358

Merge remote-tracking branch 'broker/broker-new-model' into broker-ne…

3a9fd94

…w-model

Updating trigger example with filters

5ca1b9f

Broker changes cause the namespace watcher to reconcile.

bd53cda

Merge remote-tracking branch 'broker/broker-new-model' into broker-ne…

17889fb

…w-model

Merge branch 'master' into broker-new-model

582eb4c

Move the Logging package import and format.

4f803a2

Updating after review comments. Only doing exact header matching.

eec0f8d

In a follow up PR, I plan to include matching of expressions. It turns our that they are somewhat more involved than expected.

Harwayne added 8 commits March 18, 2019 15:59

Partial update to the unit tests.

f27c63a

Unit tests working.

747e290

Merge branch 'master' into broker-ingress-cloudevents

2b9a2e0

Increase broker_types.go code coverage.

f3121ae

hack/update-deps.sh

50aa925

Increase receiver.go code coverage.

c63f23e

Increase trigger.go code coverage.

ad9bf7c

Constraint, not override.

b314abd

vaikas reviewed Mar 19, 2019

View reviewed changes

broker.go unit tests.

a9d7111

Harwayne mentioned this pull request Mar 19, 2019

Subscription.Reply to non-Channels #925

Closed

Harwayne changed the title ~~[WIP] Trigger responses are sent back into the Broker.~~ Trigger responses are sent back into the Broker. Mar 19, 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 Mar 19, 2019

Harwayne added 2 commits March 19, 2019 16:50

Merge branch 'master' into broker-ingress-cloudevents

bd47d55

Better receiver shutdown logic.

36ef2a4

Harwayne mentioned this pull request Mar 20, 2019

Switch the Broker's Ingress to use CloudEvents SDK #933

Merged

Harwayne mentioned this pull request Mar 20, 2019

Tracing works with Broker and Trigger #936

Merged

knative-prow-robot assigned vaikas Mar 20, 2019

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

knative-prow-robot merged commit 82d9baf into knative:master Mar 20, 2019

Harwayne added a commit to Harwayne/knative-eventing that referenced this pull request Mar 25, 2019

Update docs to reflect knative#917.

8833f69

Harwayne mentioned this pull request Mar 25, 2019

Update docs to reflect #917. #971

Merged

knative-prow-robot pushed a commit that referenced this pull request Mar 26, 2019

Update docs to reflect #917. (#971)

45f9a8d

Harwayne deleted the broker-ingress-cloudevents branch May 9, 2019 18:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Trigger responses are sent back into the Broker.#917

Trigger responses are sent back into the Broker.#917
knative-prow-robot merged 170 commits into
knative:masterfrom
Harwayne:broker-ingress-cloudevents

Harwayne commented Mar 18, 2019 •

edited

Loading

Uh oh!

vaikas left a comment

Uh oh!

vaikas Mar 19, 2019

Uh oh!

vaikas Mar 19, 2019

Uh oh!

Harwayne Mar 19, 2019

Uh oh!

grantr Mar 19, 2019

Uh oh!

knative-metrics-robot commented Mar 20, 2019

Uh oh!

Harwayne commented Mar 20, 2019

Uh oh!

vaikas commented Mar 20, 2019

Uh oh!

knative-prow-robot commented Mar 20, 2019

Uh oh!

vaikas commented Mar 20, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Conversation

Harwayne commented Mar 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed Changes

Uh oh!

vaikas left a comment

Choose a reason for hiding this comment

Uh oh!

vaikas Mar 19, 2019

Choose a reason for hiding this comment

Uh oh!

vaikas Mar 19, 2019

Choose a reason for hiding this comment

Uh oh!

Harwayne Mar 19, 2019

Choose a reason for hiding this comment

Uh oh!

grantr Mar 19, 2019

Choose a reason for hiding this comment

Uh oh!

knative-metrics-robot commented Mar 20, 2019

Uh oh!

Harwayne commented Mar 20, 2019

Uh oh!

vaikas commented Mar 20, 2019

Uh oh!

knative-prow-robot commented Mar 20, 2019

Uh oh!

vaikas commented Mar 20, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Harwayne commented Mar 18, 2019 •

edited

Loading