feat(retries) Design and implement retries #658

joseblas · 2019-03-22T13:30:27Z

closes #221

Changes

Adding retries to pipeline. Pipeline/Tasks will have retries that will be passed to the subsequent pipeline|task run.

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.

Release Notes

Adds the possibility to execute automated retries when something fails, could be applied to Pipelines or Tasks.

Adds support for specifying `retries` at the level of a `Task` within a `Pipeline`

abayer · 2019-03-22T14:00:04Z

/ok-to-test

bobcatfish · 2019-03-24T20:21:39Z

Hey @joseblas !

So to resurrect our conversation from #510, the design that makes the most sense to me given our requirements is still having retries on Pipeline.Spec.Tasks, e.g.:

apiVersion: tekton.dev/v1alpha1
kind: Pipeline
..
  tasks:
  - name: build-skaffold-web
    retries: 3
 ...
  - name: build-skaffold-app
    retries: 3
...
  - name: deploy-app
  ...
  - name: deploy-web

It doesn't seem to me like we need to have retries in the Run specs, at least initially (unless there is a use case I am completely missing where we need to override this at runtime?)

I thought about static/dynamic information, so the number of retries to be done should be in the specs in the runs info and the number of retries done within the Status. (From #510)

We are definitely on the same page that the number of retries should be in the spec of the CRD, and the status should keep track of how many have been attempted, the big difference is that I think it makes more sense (at least initially) to put this information into the Pipeline spec and not into PipelineRun or TaskRun.

If you want to discuss more there is time in our Tuesday 9am PST working group meeting (https://github.com/tektoncd/pipeline/blob/master/CONTRIBUTING.md#contact) for design discussions now so we could keep the discussion going there if you want to speed it up :)

joseblas · 2019-03-25T17:55:41Z

Hi @bobcatfish,
following up that, retries will be applied only to tasks, right?

bobcatfish · 2019-03-25T23:32:48Z

Quick update: @joseblas and I discussed further offline :)

abayer · 2019-04-05T13:26:45Z

/test pull-tekton-pipeline-integration-tests

joseblas · 2019-04-05T14:55:19Z

/test pull-tekton-pipeline-integration-tests

abayer · 2019-04-08T14:40:22Z

/test pull-tekton-pipeline-integration-tests

abayer · 2019-04-08T15:10:52Z

@joseblas suggests we may want to hold off on merging this until #731 is resolved - timeouts are generally flaky, basically.

abayer · 2019-04-08T15:10:59Z

/hold

bobcatfish

Really coming along! The interface is looking great :D

I have a lot of miscellaneous comments. One high level thing I noticed is that I don't see any logic handling cancellations - if a taskrun is cancelled we shouldn't retry it

(note i didnt look at TestReconcileWithTimeoutAndRetry yet - i have a feeling we need to add more test cases at a lower level and possibly a couple at that level as well)

bobcatfish · 2019-04-15T23:30:35Z

docs/pipelines.md

@@ -140,6 +140,16 @@ tasks:
      name: build-push
 ```

+There is an optional attribute called `retries`, which declares how many times that task should be retries in case of failure,


can you make this a section and add a link to the "optional" section at the top of the doc re. attributes of tasks?

e.g. in the section at the top:

- Optional: - [`resources`](#declared-resources) - Specifies which [`PipelineResources`](resources.md) of which types the `Pipeline` will be using in its [Tasks](#pipeline-tasks) - `tasks` - `resources.inputs` / `resource.outputs` - [`from`](#from) - Used when the content of the [`PipelineResource`](resources.md) should come from the [output](tasks.md#output) of a previous [Pipeline Task](#pipeline-tasks) - [`runAfter`](#runAfter) - Used when the [Pipeline Task](#pipeline-task) should be executed after another Pipeline Task, but there is no [output linking](#from) required - [`retries`](#retries)

And here:

#### Retries An optional attribute....

docs/pipelines.md

bobcatfish · 2019-04-15T23:33:00Z

pkg/apis/pipeline/v1alpha1/pipeline_types.go

@@ -73,6 +73,10 @@ type PipelineTask struct {
 	Name    string  `json:"name"`
 	TaskRef TaskRef `json:"taskRef"`

+	// Number of retries to be run


this comment could also use some more detail re. what it means to retry and under what circumstances that will happen

pkg/apis/pipeline/v1alpha1/taskrun_types.go

test/pipelinerun_test.go

pkg/reconciler/v1alpha1/pipelinerun/resources/pipelinerunresolution.go

pkg/reconciler/v1alpha1/pipelinerun/resources/pipelinerunresolution_test.go

pkg/reconciler/v1alpha1/pipelinerun/resources/pipelinerunresolution.go

pkg/reconciler/v1alpha1/pipelinerun/pipelinerun.go

joseblas · 2019-04-29T14:54:22Z

/test pull-tekton-pipeline-integration-tests

joseblas · 2019-04-29T17:53:12Z

Hi @bobcatfish, I've pushed some code according your comments. I did do a comment earlier but I deleted it as there was an error in the code. Sorry for that.

abayer · 2019-04-30T15:30:35Z

/hold cancel

pkg/reconciler/v1alpha1/pipelinerun/pipelinerun.go

docs/pipelines.md

pkg/reconciler/v1alpha1/pipelinerun/pipelinerun.go

pkg/reconciler/v1alpha1/pipelinerun/resources/pipelinerunresolution.go

ghost · 2019-05-06T13:36:41Z

Chatted offline about all of the above comments. Everything looks good👍

closes #221

abayer · 2019-05-06T14:25:19Z

/lgtm

abayer · 2019-05-06T14:26:44Z

oh wait yeah
/approve

abayer · 2019-05-06T14:27:17Z

ugh, I don't have approve rights... @bobcatfish, halp? =)

bobcatfish · 2019-05-06T17:35:21Z

/approve

it's happening!!! Thanks for your hard work on this and patience @joseblas 🎉 ❤️

/meow space

tekton-robot · 2019-05-06T17:35:22Z

@bobcatfish:

In response to this:

/approve

it's happening!!! Thanks for your hard work on this and patience @joseblas 🎉 ❤️

/meow space

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 · 2019-05-06T17:35:25Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abayer, bobcatfish, joseblas

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

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

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 Mar 22, 2019

tekton-robot requested review from dlorenc and vdemeester March 22, 2019 13:30

googlebot added the cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit label Mar 22, 2019

tekton-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 22, 2019

tekton-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Mar 22, 2019

vdemeester mentioned this pull request Mar 23, 2019

feat(retries) Design and implement retries #510

Closed

dicarlo2 mentioned this pull request Apr 2, 2019

Is Tekton compatible with injected sidecars? (E.g. istio) #701

Closed

joseblas changed the title ~~WIP feat(retries) Design and implement retries~~ feat(retries) Design and implement retries Apr 5, 2019

tekton-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 5, 2019

tekton-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 8, 2019

bobcatfish reviewed Apr 16, 2019

View reviewed changes

tekton-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Apr 24, 2019

dicarlo2 mentioned this pull request Apr 26, 2019

Recreate pod on TaskRun's pod deletion #758

Merged

3 tasks

abayer mentioned this pull request Apr 29, 2019

retry build Job jenkins-x/jx#3784

Closed

tekton-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Apr 29, 2019

tekton-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 30, 2019

ghost reviewed May 3, 2019

View reviewed changes

feat(retries) Design and implement retries

b1c0074

closes #221

tekton-robot assigned abayer May 6, 2019

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

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

tekton-robot merged commit 295bf9c into tektoncd:master May 6, 2019

lbernick mentioned this pull request Aug 1, 2022

TaskRun retries: Improve separation of concerns between PipelineRun and TaskRun reconciler #5248

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(retries) Design and implement retries #658

feat(retries) Design and implement retries #658

joseblas commented Mar 22, 2019 •

edited by bobcatfish

Loading

abayer commented Mar 22, 2019

bobcatfish commented Mar 24, 2019

joseblas commented Mar 25, 2019

bobcatfish commented Mar 25, 2019

abayer commented Apr 5, 2019

joseblas commented Apr 5, 2019

abayer commented Apr 8, 2019

abayer commented Apr 8, 2019

abayer commented Apr 8, 2019

bobcatfish left a comment

bobcatfish Apr 15, 2019

joseblas Apr 26, 2019

bobcatfish Apr 15, 2019

joseblas commented Apr 29, 2019

joseblas commented Apr 29, 2019

abayer commented Apr 30, 2019

ghost commented May 6, 2019

abayer commented May 6, 2019

abayer commented May 6, 2019

abayer commented May 6, 2019

bobcatfish commented May 6, 2019

tekton-robot commented May 6, 2019

tekton-robot commented May 6, 2019

feat(retries) Design and implement retries #658

feat(retries) Design and implement retries #658

Conversation

joseblas commented Mar 22, 2019 • edited by bobcatfish Loading

Changes

Submitter Checklist

Release Notes

abayer commented Mar 22, 2019

bobcatfish commented Mar 24, 2019

joseblas commented Mar 25, 2019

bobcatfish commented Mar 25, 2019

abayer commented Apr 5, 2019

joseblas commented Apr 5, 2019

abayer commented Apr 8, 2019

abayer commented Apr 8, 2019

abayer commented Apr 8, 2019

bobcatfish left a comment

Choose a reason for hiding this comment

bobcatfish Apr 15, 2019

Choose a reason for hiding this comment

joseblas Apr 26, 2019

Choose a reason for hiding this comment

bobcatfish Apr 15, 2019

Choose a reason for hiding this comment

joseblas commented Apr 29, 2019

joseblas commented Apr 29, 2019

abayer commented Apr 30, 2019

ghost commented May 6, 2019

abayer commented May 6, 2019

abayer commented May 6, 2019

abayer commented May 6, 2019

bobcatfish commented May 6, 2019

tekton-robot commented May 6, 2019

tekton-robot commented May 6, 2019

joseblas commented Mar 22, 2019 •

edited by bobcatfish

Loading