Emit events from the PipelineRun controller #2545

afrittoli · 2020-05-05T12:46:12Z

Changes

Emit events from the PipelineRun controller

Emit events:

Pipeline Start
Pipeline Started Running
Various Error Conditions, Cancel, Timeout

Emit all events through the events.go module.
Align and simplify the reconcile structure to have clear points
for error handling and emitting events.

Added test for the normal reconcile to completion case.
Added more event checks to existing tests.

Partially addresses #2474
Partially addresses #2082

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

PipelineRuns emit an additional k8s event when starting to execute, to run and on different failure scenarios.

tekton-robot · 2020-05-05T12:46:17Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign afrittoli
You can assign the PR to them by writing /assign @afrittoli in a comment when ready.

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

Needs approval from an approver in each of these files:

OWNERS

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

tekton-robot · 2020-05-05T12:53:00Z

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/pipelinerun/cancel.go	72.7%	73.7%	1.0
pkg/reconciler/pipelinerun/pipelinerun.go	72.4%	73.7%	1.4

tekton-robot · 2020-05-13T12:26:24Z

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-13T12:28: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-15T17:32:38Z

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/pipelinerun/pipelinerun.go	73.5%	75.4%	1.9

tekton-robot · 2020-05-15T21:29:21Z

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/apis/pipeline/v1beta1/pipelinerun_types.go	90.5%	88.4%	-2.1
pkg/reconciler/pipelinerun/pipelinerun.go	73.5%	75.6%	2.1

tekton-robot · 2020-05-16T11:31:53Z

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/apis/pipeline/v1beta1/pipelinerun_types.go	90.5%	88.4%	-2.1
pkg/reconciler/pipelinerun/pipelinerun.go	73.5%	73.9%	0.4

tekton-robot · 2020-05-18T20:38:49Z

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/apis/pipeline/v1beta1/pipelinerun_types.go	90.5%	88.4%	-2.1
pkg/reconciler/pipelinerun/pipelinerun.go	73.5%	73.9%	0.4

tekton-robot · 2020-05-19T09:24:26Z

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.

vdemeester

Looking good 👍

afrittoli · 2020-06-06T08:51:55Z

hey @afrittoli i started reviewing this and i've only gotten part way through - im wondering if it might be reasonable to break up some of the refactoring into a separate PR or 2? I know that's a huge pain to do but id like to be able to review this carefully and it's a bit tough - the description of the PR is that it's expanding the events we are emitting but the content feels like a pretty significant refactoring of the existing logic - it'd be easier to review if we could review the refactoring and understand what that is changing without new functionality.

I'll try to break this down small pieces out of this where possible.
Some of the refactoring was now done in a separate PR, even though in a slightly different and better way #2749.

what do you think? if you think its unfeasible ill have to set aside a big block of time to review carefully as is

meanwhile I left some initial feedback!

Emit events: - Pipeline Start - Pipeline Started Running - Various Error Conditions, Cancel, Timeout Emit all events through the events.go module. Align and simplify the reconcile structure to have clear points for error handling and emitting events. Added test for the normal reconcile to completion case. Added more event checks to existing tests.

afrittoli · 2020-06-06T17:29:36Z

I addressed the initial comments, but I've not split this one yet.

afrittoli · 2020-06-06T17:42:21Z

There are a few things happening:

add a function for updating status and labels. That is going away with ReconcileKind so I might wait for that PR instead
add a function to finalize the reconcile, that does the update and emit events. this is still needed, until at least we integrate with the genreconile event handling
split reconcile in "prepare + reconcile"
start using permanent errors
change order of things in the isDone section

Roughly I can try to see if I can make a PR for each of this. @bobcatfish does it make sense?

bobcatfish · 2020-06-09T15:30:17Z

Roughly I can try to see if I can make a PR for each of this. @bobcatfish does it make sense?

Sounds great to me @afrittoli , apologies for the extra work.

I think in particular having these pieces separated out into 1 or 2 PRs would help a lot:

split reconcile in "prepare + reconcile"
start using permanent errors

afrittoli · 2020-06-09T20:41:08Z

pkg/reconciler/pipelinerun/pipelinerun_test.go

@@ -208,8 +236,10 @@ func TestReconcile(t *testing.T) {
 	defer cancel()
 	c := testAssets.Controller
 	clients := testAssets.Clients
+	reconciler := c.Reconciler.(*Reconciler)
+	fr := reconciler.Recorder.(*record.FakeRecorder)


This should be updated the same way it was done for the taskrun reconciler tests

afrittoli · 2020-06-10T16:29:27Z

Roughly I can try to see if I can make a PR for each of this. @bobcatfish does it make sense?

Sounds great to me @afrittoli , apologies for the extra work.

I think in particular having these pieces separated out into 1 or 2 PRs would help a lot:
* split reconcile in "prepare + reconcile"

* start using permanent errors

Using permanent errors: #2798
Define finishReconcileUpdateEmitEvents to exit reconcile: #2805

afrittoli · 2020-06-10T16:29:56Z

/hold

tekton-robot · 2020-07-01T10:20:36Z

@afrittoli: PR needs rebase.

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-08-15T02:47:16Z

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.
If this issue is safe to close now please do so with /close.

/lifecycle rotten

Send feedback to tektoncd/plumbing.

tekton-robot · 2020-08-15T02:47:17Z

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

/close

Send feedback to tektoncd/plumbing.

tekton-robot · 2020-08-15T02:47:19Z

@tekton-robot: Closed this PR.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

/close

Send feedback to tektoncd/plumbing.

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.

vdemeester · 2020-08-17T09:07:20Z

/remove-lifecycle rotten
/remove-lifecycle stale
/reopen

tekton-robot · 2020-08-17T09:07:22Z

@vdemeester: Reopened this PR.

In response to this:

/remove-lifecycle rotten
/remove-lifecycle stale
/reopen

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-08-17T09:08:25Z

@afrittoli: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-tekton-pipeline-unit-tests	`55b2db7`	link	`/test tekton-pipeline-unit-tests`
pull-tekton-pipeline-build-tests	`55b2db7`	link	`/test pull-tekton-pipeline-build-tests`
pull-tekton-pipeline-integration-tests	`55b2db7`	link	`/test pull-tekton-pipeline-integration-tests`
pull-tekton-pipeline-go-coverage	`55b2db7`	link	`/test pull-tekton-pipeline-go-coverage`

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

vdemeester · 2020-09-14T11:19:26Z

@afrittoli what is the status here ?

afrittoli · 2020-09-14T11:36:10Z

This PR has been broken down into pieces and merged.
The bit missing from this PR is the separation of the reconcile method into prepare + reconcile - however with the use the the gen reconciler and permanent errors I think there would be no benefit in that split other than making the method code a bit shorter.

tekton-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 5, 2020

tekton-robot requested review from dibyom and a user May 5, 2020 12:46

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

tekton-robot added the lifecycle/active Indicates that an issue or PR is actively being worked on by a contributor. label May 13, 2020

tekton-robot added kind/feature Categorizes issue or PR as related to a new feature. and removed lifecycle/active Indicates that an issue or PR is actively being worked on by a contributor. labels May 13, 2020

afrittoli force-pushed the issues/2082-pipelinerun-events branch 5 times, most recently from df1a567 to 9ea2fa7 Compare May 15, 2020 17:30

afrittoli force-pushed the issues/2082-pipelinerun-events branch from 9ea2fa7 to e3ee453 Compare May 15, 2020 21:26

afrittoli force-pushed the issues/2082-pipelinerun-events branch from e3ee453 to 3da3c74 Compare May 16, 2020 11:29

tekton-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 16, 2020

afrittoli force-pushed the issues/2082-pipelinerun-events branch from 3da3c74 to 67184d6 Compare May 18, 2020 20:36

afrittoli added 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 19, 2020

afrittoli force-pushed the issues/2082-pipelinerun-events branch from 67184d6 to f00dd88 Compare May 19, 2020 17:49

afrittoli changed the title ~~WIP Emit events from the PipelineRun controller~~ Emit events from the PipelineRun controller May 19, 2020

vdemeester reviewed Jun 5, 2020

View reviewed changes

afrittoli mentioned this pull request Jun 6, 2020

Add support for more cloud events #2677

Merged

3 tasks

afrittoli force-pushed the issues/2082-pipelinerun-events branch from 86823f0 to 55b2db7 Compare June 6, 2020 17:29

afrittoli commented Jun 9, 2020

View reviewed changes

tekton-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 10, 2020

afrittoli added this to the Pipelines v0.14 milestone Jun 10, 2020

afrittoli mentioned this pull request Jun 29, 2020

Finish setting up events for pipeline runs #2874

Merged

4 tasks

afrittoli removed this from the Pipelines v0.14 milestone Jun 29, 2020

tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 1, 2020

tekton-robot added the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Aug 15, 2020

tekton-robot closed this Aug 15, 2020

tekton-robot reopened this Aug 17, 2020

tekton-robot removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Aug 17, 2020

afrittoli closed this Sep 14, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Emit events from the PipelineRun controller #2545

Emit events from the PipelineRun controller #2545

afrittoli commented May 5, 2020 •

edited

Loading

tekton-robot commented May 5, 2020

tekton-robot commented May 5, 2020

tekton-robot commented May 13, 2020

tekton-robot commented May 13, 2020

tekton-robot commented May 15, 2020

tekton-robot commented May 15, 2020

tekton-robot commented May 16, 2020

tekton-robot commented May 18, 2020

tekton-robot commented May 19, 2020

vdemeester left a comment

afrittoli commented Jun 6, 2020

afrittoli commented Jun 6, 2020

afrittoli commented Jun 6, 2020

bobcatfish commented Jun 9, 2020

afrittoli Jun 9, 2020

afrittoli commented Jun 10, 2020 •

edited

Loading

afrittoli commented Jun 10, 2020

tekton-robot commented Jul 1, 2020

tekton-robot commented Aug 15, 2020

tekton-robot commented Aug 15, 2020

tekton-robot commented Aug 15, 2020

vdemeester commented Aug 17, 2020

tekton-robot commented Aug 17, 2020

tekton-robot commented Aug 17, 2020

vdemeester commented Sep 14, 2020

afrittoli commented Sep 14, 2020

Emit events from the PipelineRun controller #2545

Emit events from the PipelineRun controller #2545

Conversation

afrittoli commented May 5, 2020 • edited Loading

Changes

Submitter Checklist

Reviewer Notes

Release Notes

tekton-robot commented May 5, 2020

tekton-robot commented May 5, 2020

tekton-robot commented May 13, 2020

tekton-robot commented May 13, 2020

tekton-robot commented May 15, 2020

tekton-robot commented May 15, 2020

tekton-robot commented May 16, 2020

tekton-robot commented May 18, 2020

tekton-robot commented May 19, 2020

vdemeester left a comment

Choose a reason for hiding this comment

afrittoli commented Jun 6, 2020

afrittoli commented Jun 6, 2020

afrittoli commented Jun 6, 2020

bobcatfish commented Jun 9, 2020

afrittoli Jun 9, 2020

Choose a reason for hiding this comment

afrittoli commented Jun 10, 2020 • edited Loading

afrittoli commented Jun 10, 2020

tekton-robot commented Jul 1, 2020

tekton-robot commented Aug 15, 2020

tekton-robot commented Aug 15, 2020

tekton-robot commented Aug 15, 2020

vdemeester commented Aug 17, 2020

tekton-robot commented Aug 17, 2020

tekton-robot commented Aug 17, 2020

vdemeester commented Sep 14, 2020

afrittoli commented Sep 14, 2020

afrittoli commented May 5, 2020 •

edited

Loading

afrittoli commented Jun 10, 2020 •

edited

Loading