Trigger status depends on importer status by capri-xiyue · Pull Request #1849 · knative/eventing

capri-xiyue · 2019-09-09T21:01:59Z

Fixes #1734

Proposed Changes

Implemented the feature, trigger status depends on importer status

Release Note

capri-xiyue · 2019-09-09T21:02:43Z

/assign @grantr

grantr

Yay! 😄

capri-xiyue · 2019-09-09T21:18:45Z

/test pull-knative-eventing-integration-tests

knative-prow-robot · 2019-09-09T21:25:20Z

@capri-xiyue: GitHub didn't allow me to assign the following users: adamharwayne.

Note that only knative members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

Details

In response to this:

/assign @adamharwayne

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.

knative-prow-robot · 2019-09-09T21:25:58Z

@capri-xiyue: GitHub didn't allow me to assign the following users: adamharwayne.

Note that only knative members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

Details

In response to this:

/assign @adamharwayne

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.

capri-xiyue · 2019-09-09T21:45:03Z

I differentiate the status "False" and "Unknown" by following this doc https://github.com/knative/serving/blob/master/docs/spec/errors.md

Unknown when the controller is actively working to achieve the condition.
False when the reconciliation has failed. This should be a terminal failure state until user action occurs.
However, when Trigger try to reconcile broker and subscription, it doesn't follow these rules. For example, when the broker doesn't exist, we mark the broker status as False. Should we create another issue to fix this? @grantr

capri-xiyue · 2019-09-09T21:46:48Z

I'm wondering whether we have any restrictions of using third party libraries? I used https://github.com/hashicorp/go-multierror to handle multi error. @grantr

grantr · 2019-09-09T21:58:27Z

However, when Trigger try to reconcile broker and subscription, it doesn't follow these rules. For example, when the broker doesn't exist, we mark the broker status as False. Should we create another issue to fix this? @grantr

This sounds like a bug, so yes please create an issue about this @capri-xiyue!

grantr

I'm excited to see this go in! Thanks @capri-xiyue.

After finishing the review, I think I favor validating the format of the annotation in the webhook, not the reconciler. That simplifies the reconciler since there are fewer error states to worry about. Let me know if you need help getting started with this.

General question: Should a failure to reconcile the dependency's status due to a non-terminal error (like the object doesn't exist, or doesn't have a status) immediately return a reconcile error? The effect of returning immediately is that the Trigger's data plane configuration is never created. I think this might cause data loss. For example:

Importer created along with Trigger.
Importer isn't ready. Trigger reconcile returns before creating a Subscription.
Importer is ready and starts sending events.
Trigger reconcile creates a Subscription.

Between step 3 and 4, any events sent by the importer will be lost. I think it would be a better experience for the user if we continue reconciling the trigger even if the dependency isn't ready.

What do you think @capri-xiyue?

Co-Authored-By: mattmoor-sockpuppet <mattmoor+sockpuppet@google.com>

capri-xiyue · 2019-09-11T00:07:12Z

I'm excited to see this go in! Thanks @capri-xiyue.

After finishing the review, I think I favor validating the format of the annotation in the webhook, not the reconciler. That simplifies the reconciler since there are fewer error states to worry about. Let me know if you need help getting started with this.

General question: Should a failure to reconcile the dependency's status due to a non-terminal error (like the object doesn't exist, or doesn't have a status) immediately return a reconcile error? The effect of returning immediately is that the Trigger's data plane configuration is never created. I think this might cause data loss. For example:

Importer created along with Trigger.

Importer isn't ready. Trigger reconcile returns before creating a Subscription.

Importer is ready and starts sending events.

Trigger reconcile creates a Subscription.

Between step 3 and 4, any events sent by the importer will be lost. I think it would be a better experience for the user if we continue reconciling the trigger even if the dependency isn't ready.

What do you think @capri-xiyue?

Discussed offline with @grantr. When trigger is not ready, user won't have the expectation that they will receive events from trigger, so data loss should not be a problem. But it's good to have trigger reconciled ASAP. So it's better to allow trigger to keep reconciling other dependent resources even when it encounters problems when trying to reconcile some dependent resources. But it should return all errors together in the end and mark reconcile failed. Otherwise, when user try to kubectl describe trigger, it's quite confusing to find "trigger reconciled" logging but trigger status is still not ready.
For this PR, I will try to reconcile dependency annotation after we reconcile broker and subscription. And I will create another issue which will figure out whether it's necessary and possible to allow trigger to keep reconciling other dependent resources even when it encounters problems when trying to reconcile some dependent resources. And it should also return all errors together in the end and mark reconcile failed.(Kind of like apis.FieldError usage when doing validation, for example pkg/apis/eventing/v1alpha1/trigger_validation.go)

Harwayne

Overall looks really good. I think all the logic is mostly correct, just lots of minor comments.

Harwayne · 2019-09-11T15:42:39Z

+- apiGroups:
+  - "sources.eventing.knative.dev"
+  resources:
+    - "cronjobsources"


Do we need the status subresource as well? E.g.

- cronjobsources - cronjobsources/status - containersources - containersources/status - apiserversources - apiserversources/status

I think no, because we never write the status.

Makes sense. Probably worth giving this feedback on #1856, which says they must include the status subresources.

Harwayne · 2019-09-11T15:44:18Z

+      - "pubsub.cloud.run"
+      - "events.cloud.run"
+    resources:
+      - "pullsubscriptions"


Same question about status subresources.

We don't need to update status. Do we need the status subresources? I did manual testing. It works even without permission of status subresources.

…into issue_1734

capri-xiyue · 2019-09-13T17:24:55Z

@grantr @Harwayne , I've pushed new changes. It's ready for another round of code review.

knative-metrics-robot · 2019-09-13T20:42:28Z

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/trigger_lifecycle.go	100.0%	71.4%	-28.6
pkg/apis/eventing/v1alpha1/trigger_validation.go	97.6%	98.5%	0.9
pkg/reconciler/trigger/trigger.go	86.2%	84.8%	-1.4

grantr

/lgtm

knative-prow-robot · 2019-09-16T18:32:12Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: capri-xiyue, grantr

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~~ [grantr]

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

* Patch the upstream config file to repair or reenable old behavior Signed-off-by: Matthias Wessendorf <mwessend@redhat.com> * Running 'make RELEASE=v1.4 generate-release' Signed-off-by: Matthias Wessendorf <mwessend@redhat.com> * Adding check for (legacy) annotations Signed-off-by: Matthias Wessendorf <mwessend@redhat.com> Signed-off-by: Matthias Wessendorf <mwessend@redhat.com>

…ative#9012) (knative#1849) Generate test certs dynamically instead of hardcoding them Co-authored-by: Knative Prow Robot <automation+prow-robot@knative.team>

Trigger status depends on importer status, have not fixed unit tests

b7f8803

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 Sep 9, 2019

mattmoor-sockpuppet reviewed Sep 9, 2019

View reviewed changes

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

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

knative-prow-robot requested review from matzew and n3wscott September 9, 2019 21:02

knative-prow-robot assigned grantr Sep 9, 2019

grantr reviewed Sep 9, 2019

View reviewed changes

Comment thread pkg/reconciler/trigger/trigger.go Outdated

grantr reviewed Sep 9, 2019

View reviewed changes

Update config/200-source-resolvers-clusterrole.yaml

76fa667

Co-Authored-By: mattmoor-sockpuppet <mattmoor+sockpuppet@google.com>

This was referenced Sep 10, 2019

Add ClusterRole to get, list and watch "pullsubscriptions" and "storages" google/knative-gcp#252

Closed

Add ClusterRole to get, list and watch "awssqssources", "camelsources", "kafkasources" and "githubsources" knative/eventing-contrib#583

Closed

Harwayne reviewed Sep 11, 2019

View reviewed changes

capri-xiyue added 2 commits September 13, 2019 09:59

changed the error handling when reconcile dependency, added unit tests

da23a43

Merge branch 'issue_1734' of https://github.com/capri-xiyue/eventing …

6d5163f

…into issue_1734

capri-xiyue changed the title ~~[WIP]Trigger status depends on importer status~~ Trigger status depends on importer status Sep 13, 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 Sep 13, 2019

grantr reviewed Sep 13, 2019

View reviewed changes

changed error format

41fc57a

grantr approved these changes Sep 16, 2019

View reviewed changes

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

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

knative-prow-robot merged commit b8e91d3 into knative:master Sep 16, 2019

grantr mentioned this pull request Sep 17, 2019

Fix remaining github.com/knative imports #1902

Merged

grantr mentioned this pull request Feb 7, 2020

Add grantr as Eventing WG lead knative/community#83

Merged

6 tasks

Conversation

capri-xiyue commented Sep 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed Changes

Uh oh!

capri-xiyue commented Sep 9, 2019

Uh oh!

grantr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

capri-xiyue commented Sep 9, 2019

Uh oh!

knative-prow-robot commented Sep 9, 2019

Uh oh!

knative-prow-robot commented Sep 9, 2019

Uh oh!

capri-xiyue commented Sep 9, 2019

Uh oh!

capri-xiyue commented Sep 9, 2019

Uh oh!

grantr commented Sep 9, 2019

Uh oh!

grantr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

capri-xiyue commented Sep 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Harwayne left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Harwayne Sep 11, 2019

Choose a reason for hiding this comment

Uh oh!

grantr Sep 11, 2019

Choose a reason for hiding this comment

Uh oh!

Harwayne Sep 11, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Harwayne Sep 11, 2019

Choose a reason for hiding this comment

Uh oh!

capri-xiyue Sep 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

capri-xiyue commented Sep 13, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

knative-metrics-robot commented Sep 13, 2019

Uh oh!

grantr left a comment

Choose a reason for hiding this comment

Uh oh!

knative-prow-robot commented Sep 16, 2019

capri-xiyue commented Sep 9, 2019 •

edited

Loading

capri-xiyue commented Sep 11, 2019 •

edited

Loading

capri-xiyue Sep 13, 2019 •

edited

Loading