Skip to content
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.

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

Allow steps to run regardless of previous step errors #1573

Closed
wants to merge 1 commit into from
Closed

Allow steps to run regardless of previous step errors #1573

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Nov 14, 2019

Fixes #1559

Changes

Outstanding TODOs:

  • Implement defaultErrorStrategy behaviour: its value should be copied into any steps that dont include their own errorStrategy field.
  • Confirm how this interacts with existing output pipeline resources.

This commit introduces an errorStrategy field to Steps and a defaultErrorStrategy field to Tasks. These fields can currently accept one of two possible strategies. The first is SkipOnPriorStepErrors and tells the Step to skip its work if a prior step has failed. This is how Tekton works today and is therefore the default value if the field is omitted or left blank. The second strategy is IgnorePriorStepErrors and tells a step to run regardless of whether a previous step has already errored out.

This feature is intended to serve two similar use-cases:

  1. Unit test failures in a step can be written to the task workspace and then uploaded to persistent storage in a subsequent step, even though the unit test failure resulted in a non-zero exit code.
  2. Pipeline resources that need to report on the failed status of a step can do so. For example the pull request resource could be updated to add a final step to a Task that updates a PR with the status of a build step. Currently if a build step fails then any later steps will be skipped.

Submitter Checklist

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

See the contribution guide for more details.

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

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

The steps of a task can now be run regardless of previous steps' errors. A step can now upload test results even if the test runner step before it failed.

@tekton-robot 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 Nov 14, 2019
@googlebot googlebot added the cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit label Nov 14, 2019
@tekton-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign sbwsg
You can assign the PR to them by writing /assign @sbwsg 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:

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

@tekton-robot tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 14, 2019
@tekton-robot
Copy link
Collaborator

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

File Old Coverage New Coverage Delta
cmd/entrypoint/waiter.go 63.6% 70.0% 6.4
pkg/apis/pipeline/v1alpha1/task_defaults.go 80.0% 88.9% 8.9
pkg/apis/pipeline/v1alpha1/task_validation.go 81.5% 80.3% -1.2
pkg/entrypoint/error_strategies.go Do not exist 80.0%
pkg/entrypoint/skip_error.go Do not exist 0.0%
pkg/reconciler/taskrun/entrypoint/entrypoint.go 76.9% 77.5% 0.6

@tekton-robot
Copy link
Collaborator

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

File Old Coverage New Coverage Delta
cmd/entrypoint/waiter.go 63.6% 70.0% 6.4
pkg/apis/pipeline/v1alpha1/task_defaults.go 80.0% 88.9% 8.9
pkg/apis/pipeline/v1alpha1/task_validation.go 81.5% 80.3% -1.2
pkg/entrypoint/error_strategies.go Do not exist 80.0%
pkg/entrypoint/skip_error.go Do not exist 0.0%
pkg/reconciler/taskrun/entrypoint/entrypoint.go 76.9% 77.5% 0.6

@tekton-robot
Copy link
Collaborator

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

File Old Coverage New Coverage Delta
cmd/entrypoint/waiter.go 63.6% 70.0% 6.4
pkg/apis/pipeline/v1alpha1/task_defaults.go 80.0% 88.9% 8.9
pkg/apis/pipeline/v1alpha1/task_validation.go 81.5% 80.3% -1.2
pkg/entrypoint/error_strategies.go Do not exist 80.0%
pkg/entrypoint/skip_error.go Do not exist 0.0%
pkg/reconciler/taskrun/entrypoint/entrypoint.go 76.9% 77.5% 0.6

@tekton-robot
Copy link
Collaborator

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

File Old Coverage New Coverage Delta
cmd/entrypoint/waiter.go 63.6% 70.0% 6.4
pkg/apis/pipeline/v1alpha1/task_defaults.go 80.0% 88.9% 8.9
pkg/apis/pipeline/v1alpha1/task_validation.go 81.5% 80.3% -1.2
pkg/entrypoint/error_strategies.go Do not exist 100.0%
pkg/entrypoint/skip_error.go Do not exist 0.0%
pkg/reconciler/taskrun/entrypoint/entrypoint.go 76.9% 77.5% 0.6

@tekton-robot
Copy link
Collaborator

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

File Old Coverage New Coverage Delta
cmd/entrypoint/waiter.go 63.6% 70.0% 6.4
pkg/apis/pipeline/v1alpha1/task_defaults.go 80.0% 88.9% 8.9
pkg/apis/pipeline/v1alpha1/task_validation.go 81.5% 80.9% -0.6
pkg/entrypoint/error_strategies.go Do not exist 100.0%
pkg/entrypoint/skip_error.go Do not exist 0.0%
pkg/reconciler/taskrun/entrypoint/entrypoint.go 76.9% 77.5% 0.6

@tekton-robot
Copy link
Collaborator

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

File Old Coverage New Coverage Delta
cmd/entrypoint/waiter.go 63.6% 70.0% 6.4
pkg/apis/pipeline/v1alpha1/task_validation.go 81.5% 80.9% -0.6
pkg/entrypoint/error_strategies.go Do not exist 100.0%
pkg/entrypoint/skip_error.go Do not exist 0.0%
pkg/reconciler/taskrun/entrypoint/entrypoint.go 76.9% 77.5% 0.6
pkg/reconciler/taskrun/taskrun.go 72.5% 73.2% 0.7
test/builder/step.go 10.0% 9.5% -0.5
test/builder/task.go 82.6% 81.8% -0.8

This commit introduces an `errorStrategy` field to Steps and
a `defaultErrorStrategy` field to Tasks. These fields can currently
accept one of two possible strategies. The first is
`SkipOnPriorStepErrors` and tells the Step to skip its work if a prior
step has failed. This is how Tekton works today and is therefore the
default value if the field is omitted or left blank. The second strategy
is `IgnorePriorStepErrors` and tells a step to run regardless of whether
a previous step has already errored out.

This feature is intended to serve two similar use-cases:

1. Unit test failures in a step can be written to the task workspace and
then uploaded to persistent storage in a subsequent step, even though
the unit test failure resulted in a non-zero exit code.
2. Pipeline resources that need to report on the failed status of a step
can do so. For example the pull request resource could be updated to add
a final step to a Task that updates a PR with the status of a build
step. Currently if a build step fails then any later steps will be
skipped.
@tekton-robot
Copy link
Collaborator

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

File Old Coverage New Coverage Delta
cmd/entrypoint/waiter.go 63.6% 70.0% 6.4
pkg/apis/pipeline/v1alpha1/task_validation.go 81.5% 80.9% -0.6
pkg/entrypoint/error_strategies.go Do not exist 100.0%
pkg/entrypoint/skip_error.go Do not exist 0.0%
pkg/reconciler/taskrun/entrypoint/entrypoint.go 76.9% 77.5% 0.6
pkg/reconciler/taskrun/taskrun.go 72.5% 73.2% 0.7
test/builder/step.go 10.0% 9.5% -0.5
test/builder/task.go 82.6% 81.8% -0.8

@ghost
Copy link
Author

ghost commented Nov 15, 2019

With respect to output pipeline resources I don't observe any obvious change in their behaviour with this PR. I modified examples/pipelineruns/output-pipelinerun.yaml so that the first Task's first step errored out and the subsequent step ignores the error. Apart from the second executing there were no perceivable differences in the execution results of the pipelinerun. Both before and after the error strategy support was introduced the PipelineRun failed because the first TaskRun failed. The only difference was that within that first TaskRun the second Step was allowed to finish executing despite the first step's failure.

@ghost ghost changed the title WIP Allow steps to run regardless of previous step errors Allow steps to run regardless of previous step errors Nov 15, 2019
@tekton-robot 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 Nov 15, 2019
@ghost
Copy link
Author

ghost commented Nov 15, 2019

ok i think this is ready to review now.

@bobcatfish
Copy link
Collaborator

/hold

humble request to be able to review this before it goes in - but am swamped with kubecon until after next week 🙏 which im guessing is the case for most OWNERS

@tekton-robot tekton-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 16, 2019
@ghost
Copy link
Author

ghost commented Nov 18, 2019

i'm going to close this until after kubecon when it can be discussed further. will just keep it as part of my resources branch.

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a field to Step that allows it to ignore failed prior Steps *within the same Task*
3 participants