TEP-0090: Add `TaskRuns` to `ResolvedPipelineRunTask` and implement `isFailure` #4951

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Merged

tekton-robot merged 1 commit into tektoncd:main from jerop:tep-0090-arrays

Jun 10, 2022

Member

jerop commented Jun 9, 2022 •

edited

Loading

Changes

TEP-0090: Matrix proposed executing a PipelineTask in parallel TaskRuns and Runs with substitutions from combinations of Parameters in a Matrix.

In this change, we add TaskRuns field to ResolvedPipelineRunTask alongside the TaskRun field.

When a PipelineTask does not have a Matrix, the TaskRun field is populated (as is already being done, no change here).
When a PipelineTask has a Matrix, TaskRuns will be populated to track all TaskRuns from a given matrixed ResolvedPipelineRunTask.

When we promote Matrix to Beta, we could remove TaskRun field and migrate to using TaskRuns field only where it would have one TaskRun only when the PipelineTask does not have a Matrix. However, the current separation is helpful to isolate the Matrix feature in alpha.

In this change, we also implement the isFailure and other member functions of ResolvedPipelineRunTask that isFailure uses: isCancelled and hasRemainingRetries. The failure strategy in Matrix is to fail fast. We can explore other failure strategies, if needed, before promoting to Beta.

/kind feature

Submitter Checklist

As the author of this PR, please check off the items in this checklist:

[n/a] Docs included if any changes are user facing
Tests included if any functionality added or changed
Follows the commit message standard
Meets the Tekton contributor standards (including functionality, content, code)
Release notes block below has been filled in (if there are no user facing changes, use release note "NONE")

Release Notes

NONE

tekton-robot added release-note-none kind/feature labels

tekton-robot requested review from dibyom and vdemeester

June 9, 2022 12:29

tekton-robot added the size/L label

jerop force-pushed the tep-0090-arrays branch from f8ae371 to 3a9b3fa Compare

June 9, 2022 12:40

Collaborator

tekton-robot commented Jun 9, 2022

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/resources/pipelinerunresolution.go	93.7%	93.8%	0.2

jerop force-pushed the tep-0090-arrays branch 2 times, most recently from 7cc1907 to 2c55235 Compare

June 9, 2022 12:44

Collaborator

tekton-robot commented Jun 9, 2022

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/resources/pipelinerunresolution.go	93.7%	93.8%	0.2

Collaborator

tekton-robot commented Jun 9, 2022

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/resources/pipelinerunresolution.go	93.7%	93.8%	0.2

jerop force-pushed the tep-0090-arrays branch from 2c55235 to 9895b4e Compare

June 9, 2022 12:47

Collaborator

tekton-robot commented Jun 9, 2022

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/resources/pipelinerunresolution.go	93.7%	93.8%	0.2


          TEP-0090: Add TaskRuns to RPRT and implement isFailure

c33604a

[TEP-0090: Matrix][tep-0090] proposed executing a `PipelineTask` in
parallel `TaskRuns` and `Runs` with substitutions from combinations
of `Parameters` in a `Matrix`.

In this change, we add `TaskRuns` field to `ResolvedPipelineRunTask`
alongside the `TaskRun` field.
- When a `PipelineTask` does not have a `Matrix`, the `TaskRun` field
is populated (as is already being done, no change here).
- When a `PipelineTask` has a `Matrix`, `TaskRuns` will be populated
to track all `TaskRuns` from a given matrixed `ResolvedPipelineRunTask`.

When we promote `Matrix` to Beta, we could remove `TaskRun` field and
migrate to using `TaskRuns` field only where it would have one `TaskRun`
only when the `PipelineTask` does not have a `Matrix`. However, the
current separation is helpful to isolate the Matrix feature in  alpha.

In this change, we also implement the `isFailure` and other member
functions of `ResolvedPipelineRunTask` that `isFailure` uses:
`isCancelled` and `hasRemainingRetries`. The failure strategy
in `Matrix` is to fail fast. We can explore other failure strategies,
if needed, before promoting to Beta.

[tep-0090]: https://github.com/tektoncd/community/blob/main/teps/0090-matrix.md

jerop force-pushed the tep-0090-arrays branch from 9895b4e to c33604a Compare

June 9, 2022 15:56

Collaborator

tekton-robot commented Jun 9, 2022

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/resources/pipelinerunresolution.go	93.7%	93.8%	0.1

dibyom approved these changes

View reviewed changes

Collaborator

tekton-robot commented Jun 9, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dibyom

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

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 approved label

lbernick reviewed

View reviewed changes

pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go Show resolved Hide resolved

pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go Show resolved Hide resolved

Member Author

jerop commented Jun 9, 2022 •

edited

Loading

/hold

need to update the TEP-0090: Matrix with fail-fast failure strategy - tektoncd/community#724

tekton-robot added the do-not-merge/hold label

jerop mentioned this pull request

TEP-0090: Get Names of TaskRuns #4958

Merged

4 tasks

Contributor

abayer commented Jun 10, 2022

/lgtm

I don’t love this - it’s fine for just supporting matrixed TaskRuns, but it feels like it’ll get messy when we add support for matrixed custom tasks (and eventually, matrixed PipelineRuns). But given that matrixed custom tasks aren’t in scope for this PR, and matrixed PipelineRuns aren’t coming in the near future, this will do.

tekton-robot assigned abayer

tekton-robot added the lgtm label

Member Author

jerop commented Jun 10, 2022

opened tektoncd/community#724 to discuss the failure strategies, will unhold this PR so that we can make progress - matrix is not yet fully functional so we can update this if we land on a different approach other than fail-fast

/hold cancel

tekton-robot removed the do-not-merge/hold label

Member Author

jerop commented Jun 10, 2022

/test tekton-pipeline-unit-tests

tekton-robot merged commit 71a291f into tektoncd:main

Member

lbernick commented Jun 10, 2022

It doesn't look like my comments about the function docstrings were resolved?

Member

pritidesai commented Jun 10, 2022

/lgtm

I don’t love this - it’s fine for just supporting matrixed TaskRuns, but it feels like it’ll get messy when we add support for matrixed custom tasks (and eventually, matrixed PipelineRuns). But given that matrixed custom tasks aren’t in scope for this PR, and matrixed PipelineRuns aren’t coming in the near future, this will do.

thank you @abayer 👍

Runs are coming in the next milestone - https://github.com/tektoncd/community/blob/main/teps/0090-matrix.md#milestone-2-execute-runs

I haven't gotten chance to review this PR but please feel free to suggest better approach, best to follow @vdemeester's reference quote - No is temporary, yes is permanent.

Member Author

jerop commented Jun 10, 2022

It doesn't look like my comments about the function docstrings were resolved?

@lbernick my bad, forgot to push that update to docstrings, here you go: #4970

jerop deleted the tep-0090-arrays branch

June 11, 2022 01:40

jerop mentioned this pull request

TEP-0090: Failure Strategies - Remove Fail Fast #4972

Merged

4 tasks

tekton-robot pushed a commit that referenced this pull request


          TEP-0090: Failure Strategies - Remove Fail Fast

cfc931c

In #4951, we implemented
`isFailure` for matrixed `TaskRuns` where we applied fail-fast
failure strategy. We discussed failure strategies further in this
PR - tektoncd/community#724 - and API WG
on 13 June 2022. We agreed to leave early termination upon failure
out of scope for the initial release of Matrix. We plan to explore
failure strategies, including fail-fast, in future work. And these
failure strategies may apply more broadly beyond Matrix.

In this change, we update `isFailure` to evaluate to `true` only when
there's a failure and there are no running `TaskRuns` in the `rprt`.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved kind/feature lgtm release-note-none size/L