Emit events when we fail to update the taskrun #2526

afrittoli · 2020-05-02T18:18:25Z

Changes

Similarly to how the pipeline controller does, we should emit events
when we fail to update the taskrun.

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

Includes tests (if functionality changed/added)
Includes docs (if user facing)
Commit messages follow commit message best practices

See the contribution guide for more details.

Double check this list of stuff that's easy to miss:

If you are adding a new binary/image to the cmd dir, please update
the release Task to build and release this image.

Reviewer Notes

If API changes are included, additive changes must be approved by at least two OWNERS and backwards incompatible changes must be approved by more than 50% of the OWNERS, and they must first be added in a backwards compatible way.

Release Notes

We now emit warning events from the taskrun controller if the controller fails to sync the updated taskrun back.

tekton-robot · 2020-05-02T18:22:11Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/taskrun/taskrun.go	74.7%	74.5%	-0.2

tekton-robot · 2020-05-04T09:15:41Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/taskrun/taskrun.go	74.7%	74.5%	-0.2

Emit additional events: - Pipeline Start Emit all events through the events.go module. Align and simplify the reconcile structure to have clear points for error handling and emitting events. Depends on tektoncd#2543 Depends on tektoncd#2526

vdemeester

/meow

tekton-robot · 2020-05-07T10:19:01Z

@vdemeester:

In response to this:

/meow

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.

tekton-robot · 2020-05-07T10:19:04Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vdemeester

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

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

bobcatfish · 2020-05-12T20:53:08Z

pkg/reconciler/event.go

+
+// EmitErrorEvent emits a failure associated to an error
+func EmitErrorEvent(c record.EventRecorder, err error, object runtime.Object) {
+	if err != nil {


feels a bit odd to have this function take an error as an argument vs checking the error in the place where it originates - is this a pattern we do other places as well?

Well, it's a bit different because in this case the error is the event that we want to send.

On knative they went a bit further, they have now an event object that also implements the error interface.
They have a generated Reconcile that invokes a ReconcileKind which returns an event that may be an error - at least that's how I understood it - so this goes a bit in that direction.

I could check if the err is nil at the source, but it felt safer to do here.

https://github.com/knative/pkg/tree/master/injection#generated-reconciler-responsibilities

My plan for upcoming patches was to centralize the event logic in event.go i.e. to receive all requests events / error notifications from the taskrun/pipelinerun controller in there, and to decide there when to emit what, k8s event and/or cloudevent.

pkg/reconciler/event_test.go

bobcatfish · 2020-05-12T20:53:59Z

pkg/reconciler/event_test.go

+
+		err := checkEvents(fr, ts.name, ts.wantEvent)
+		if err != nil {
+			t.Errorf(err.Error())


might be nice to add some context, e.g. "expected error when blah blah"

The context is added in the error by checkEvents already.
I could add unexpected error when checking for events: %#v", err.Error() but I'm not sure it would add much.

pkg/reconciler/event_test.go

bobcatfish · 2020-05-12T20:55:22Z

pkg/reconciler/taskrun/taskrun.go

+	afterCondition := tr.Status.GetCondition(apis.ConditionSucceeded)
+	reconciler.EmitEvent(c.Recorder, beforeCondition, afterCondition, tr)
+	err := c.updateStatusLabelsAndAnnotations(tr, original)
+	reconciler.EmitErrorEvent(c.Recorder, err, tr)


personally i think id rather see an explicit "if err" here vs having EmitErrorEvent handle it, just cuz it doesnt seem consistent with how we usually do error handling - maybe you have a bigger plan around this tho that im not seeing

I discovered https://github.com/knative/pkg/tree/master/injection#generated-reconciler-responsibilities after I wrote this patch. Maybe I should just try and adopt that :P

I'm trying to centralize the logic about what event to send when of what type in events.go, so I included there the check for nil too

I havent seen https://github.com/knative/pkg/tree/master/injection#generated-reconciler-responsibilities before! Glancing at it I'm not 100% sure what implications it has for this PR, im really glad to see some docs for pkg tho.

I'm trying to centralize the logic about what event to send when of what type in events.go, so I included there the check for nil too

okay, maybe it will make more sense to me as it evolves

test/cancel_test.go

Emit additional events: - Pipeline Start Emit all events through the events.go module. Align and simplify the reconcile structure to have clear points for error handling and emitting events. Depends on tektoncd#2543 Depends on tektoncd#2526

tekton-robot · 2020-05-13T14:21:22Z

This PR cannot be merged: expecting exactly one kind/ label

Available kind/ labels are:

kind/bug: Categorizes issue or PR as related to a bug.
kind/flake: Categorizes issue or PR as related to a flakey test
kind/cleanup: Categorizes issue or PR as related to cleaning up code, process, or technical debt.
kind/design: Categorizes issue or PR as related to design.
kind/documentation: Categorizes issue or PR as related to documentation.
kind/feature: Categorizes issue or PR as related to a new feature.
kind/misc: Categorizes issue or PR as a miscellaneuous one.

tekton-robot · 2020-05-13T14:22:05Z

This PR cannot be merged: expecting exactly one kind/ label

Available kind/ labels are:

kind/bug: Categorizes issue or PR as related to a bug.
kind/flake: Categorizes issue or PR as related to a flakey test
kind/cleanup: Categorizes issue or PR as related to cleaning up code, process, or technical debt.
kind/design: Categorizes issue or PR as related to design.
kind/documentation: Categorizes issue or PR as related to documentation.
kind/feature: Categorizes issue or PR as related to a new feature.
kind/misc: Categorizes issue or PR as a miscellaneuous one.

tekton-robot · 2020-05-13T14:23:06Z

This PR cannot be merged: expecting exactly one kind/ label

Available kind/ labels are:

kind/bug: Categorizes issue or PR as related to a bug.
kind/flake: Categorizes issue or PR as related to a flakey test
kind/cleanup: Categorizes issue or PR as related to cleaning up code, process, or technical debt.
kind/design: Categorizes issue or PR as related to design.
kind/documentation: Categorizes issue or PR as related to documentation.
kind/feature: Categorizes issue or PR as related to a new feature.
kind/misc: Categorizes issue or PR as a miscellaneuous one.

bobcatfish · 2020-05-13T15:59:59Z

/lgtm
/kind feature

bobcatfish · 2020-05-13T16:00:15Z

/kind feature

Similarly to how the pipeline controller does, we should emit events when we fail to update the taskrun. Events on reconcile error use a dedicated reason.

afrittoli · 2020-05-13T16:58:07Z

@bobcatfish I had to rebase because of the stored v1beta1 version :)

tekton-robot · 2020-05-13T17:00:19Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/taskrun/taskrun.go	75.5%	75.2%	-0.2

afrittoli · 2020-05-13T18:02:11Z

/test pull-tekton-pipeline-integration-tests

bobcatfish · 2020-05-13T18:08:19Z

/lgtm

Emit additional events: - Pipeline Start Emit all events through the events.go module. Align and simplify the reconcile structure to have clear points for error handling and emitting events. Depends on tektoncd#2543 Depends on tektoncd#2526

tekton-robot requested review from bobcatfish and vdemeester May 2, 2020 18:18

tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 2, 2020

afrittoli force-pushed the issues/2522_emit_errors branch from a468b43 to 1f411f8 Compare May 4, 2020 09:11

afrittoli mentioned this pull request May 5, 2020

Emit events from the PipelineRun controller #2545

Closed

3 tasks

afrittoli added the kind/feature Categorizes issue or PR as related to a new feature. label May 7, 2020

vdemeester approved these changes May 7, 2020

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 7, 2020

bobcatfish reviewed May 12, 2020

View reviewed changes

afrittoli added kind/failing-test Categorizes issue or PR as related to a consistently or frequently failing test. kind/feature Categorizes issue or PR as related to a new feature. and removed kind/feature Categorizes issue or PR as related to a new feature. labels May 13, 2020

afrittoli removed the kind/failing-test Categorizes issue or PR as related to a consistently or frequently failing test. label May 13, 2020

tekton-robot assigned bobcatfish May 13, 2020

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label May 13, 2020

Emit events when we fail to update the taskrun

a8d40e2

Similarly to how the pipeline controller does, we should emit events when we fail to update the taskrun. Events on reconcile error use a dedicated reason.

afrittoli force-pushed the issues/2522_emit_errors branch from 1f411f8 to a8d40e2 Compare May 13, 2020 16:57

tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label May 13, 2020

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label May 13, 2020

tekton-robot merged commit ed4d021 into tektoncd:master May 13, 2020

bobcatfish mentioned this pull request May 14, 2020

Tekton Enhancement Proposals proposal 🤴 tektoncd/community#82

Merged

3 tasks

afrittoli mentioned this pull request May 19, 2020

Emit event when failing to update a taskrun #2522

Closed

pritidesai mentioned this pull request Jun 18, 2020

Define a common function to exit ReconcileKind #2805

Merged

3 tasks

qu1queee mentioned this pull request Jul 24, 2020

Upgrade the Tekton version to the latest v0.12 version shipwright-io/build#221

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Emit events when we fail to update the taskrun #2526

Emit events when we fail to update the taskrun #2526

afrittoli commented May 2, 2020 •

edited

Loading

tekton-robot commented May 2, 2020

tekton-robot commented May 4, 2020

vdemeester left a comment

tekton-robot commented May 7, 2020

tekton-robot commented May 7, 2020

bobcatfish May 12, 2020

afrittoli May 12, 2020

afrittoli May 12, 2020

afrittoli May 13, 2020

bobcatfish May 12, 2020

afrittoli May 13, 2020

bobcatfish May 12, 2020

afrittoli May 12, 2020

afrittoli May 13, 2020

bobcatfish May 13, 2020

tekton-robot commented May 13, 2020

tekton-robot commented May 13, 2020

tekton-robot commented May 13, 2020

bobcatfish commented May 13, 2020

bobcatfish commented May 13, 2020

afrittoli commented May 13, 2020

tekton-robot commented May 13, 2020

afrittoli commented May 13, 2020

bobcatfish commented May 13, 2020

Emit events when we fail to update the taskrun #2526

Emit events when we fail to update the taskrun #2526

Conversation

afrittoli commented May 2, 2020 • edited Loading

Changes

Submitter Checklist

Reviewer Notes

Release Notes

tekton-robot commented May 2, 2020

tekton-robot commented May 4, 2020

vdemeester left a comment

Choose a reason for hiding this comment

tekton-robot commented May 7, 2020

tekton-robot commented May 7, 2020

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

tekton-robot commented May 13, 2020

tekton-robot commented May 13, 2020

tekton-robot commented May 13, 2020

bobcatfish commented May 13, 2020

bobcatfish commented May 13, 2020

afrittoli commented May 13, 2020

tekton-robot commented May 13, 2020

afrittoli commented May 13, 2020

bobcatfish commented May 13, 2020

afrittoli commented May 2, 2020 •

edited

Loading