[TEP-0050] Add proposed design details

[QuanZhang-William]

This PR proposes adding a new field OnErrorto PipelineTaskdefinition to allow users define task failure strategy

[QuanZhang-William]

/cc @jerop @pritidesai

[dibyom]

/kind tep

[jerop]

/assign

[jerop]

Thank you @QuanZhang-William!

[jerop]

/assign @pritidesai

[tekton-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jerop, 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:

• teps/OWNERS [jerop,vdemeester]

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

[pritidesai]

This paragraph is little confusing and inconsistent with step.onError.

The linked issue has been partially fixed in which pipeline results are emitted even when a task fails. Pipeline results are initialized with the task results from all successful tasks until a task has failed.

step.onError does support emitting results and consuming them - https://github.com/tektoncd/pipeline/blob/main/docs/tasks.md#produce-a-task-result-with-onerror

As long as a task has initialized results before failing, they can be made available for the consumption. This only should be supported with task.onFail: continue.

When a pipeline author has chosen a strategy where a task failure is expected, the author must have complete understanding of what the result could mean and how to consume that result if it's initialized before failing.

For example, git-clone produces commit as a task result. If pipeline author chooses to ignore failure from git-clone, the author is aware that the commit might/might not be initialized depending on the failure. Now, as a pipeline author, when a git-clone failed for some reason but commit is available, its guaranteed that my repo was cloned as expected. If the commit is not initialized and git-clone failed, the repo was not cloned for some reason.

Another example is boskos-acquire. As a pipeline author, I can choose to ignore failure from boskos-acquire. If boskos-acquire failed for some reason to create heartbeat pod but was able to lease resources, leased-resource will be initialized and can be made available to the cleanup task if its initialized.

[QuanZhang-William]

As long as a task has initialized results before failing, they can be made available for the consumption. This only should be supported with task.onFail: continue.

When a pipeline author has chosen a strategy where a task failure is expected, the author must have complete understanding of what the result could mean and how to consume that result if it's initialized before failing.

Thanks for your input, @pritidesai! This is a good point and the question is essentially: Should we emit results for failed tasks with onFail:continue? And this may need some discussion.

Currently we don't emit results for failed tasks. There are some discussions around it and I'm not sure why we ended up not implement it. Looking at the requirement 3 of this TEP: the task would be considered "successful" for the purposes of determining the status of the pipeline, it seems to me we should respect the existing behaviour about emitting result, and change behaviour of emitting task is another discussion/issue/TEP? Is it a good idea that we respect the existing behaviour as default, and create another TEP/Issue specifically deal with the emitting result behaviour if we can such request from users?

In terms of consistency, I think step.OnError and task.OnFail are different things. step.onError emits the result only when the overall task run status is success, but in task.OnFail we keep the task run status as failed even when it is set to continue

Thoughts? @jerop @vdemeester ?

[pritidesai]

I think we have an opportunity here to design this right for many different use cases we have seen. I would recommend attempting to solve here rather than delegating it to a separate TEP.

task.onError is a failure strategy and no different from step.onError. Pipeline is not permitted to continue running after a first failure is encountered. With task.onError, even though the task is failed (container exit code stays non-zero), the pipeline is allowed to continue executing the rest of the tasks.

[QuanZhang-William]

Sure. If we want to consolidate the behaviour about emitting results from failed task results in this TEP, I think it makes more sense to make the emitting result behaviour consistent (i.e. we emit or not emit results from failed tasks regardless of the OnErrorType), otherwise such inconsistency would be confusing to users, since OnErrorType simply describes "should we stop executing the pipeline on task failure or not", and not about "should we emit results from such failed tasks or not".

Looking at the discussion and some feedback from this PR, it looks we do have such requests, why don't we emit result for failed tasks across the board?

[QuanZhang-William]

@pritidesai @lbernick @jerop

I have added a new section Emit Results from Ignored Failed Tasks and updated Task with Missing Resource Dependency section based on our discussion. Please take a look

[pritidesai]

Thanks a bunch @QuanZhang-William for taking this forward, appreciate all the efforts you have put in 🤗.

@jerop @vdemeester I have left some comments on the design, please feel free to ignore them if they are not aligned, thanks 🙏

[pritidesai]

API WG - @QuanZhang-William working on comments

[lbernick]

Curious if we have any desire for this to be consistent with step onError, or if they're addressing different use cases? If so, might be worth including a short section discussing similarities/differences between the two features. Not blocking

[pritidesai]

Thank you @QuanZhang-William once again 🤗I have one last NIT but will not block this PR.

“continueAndFail” sounds very confusing to me 🙏 “continue” and “fail” are two opposites states and meant for two different runs here I.e. continue pipelineRun but fail taskRun. The simpler options are “continue” (continue execution of PipelineRun) or “ignoreFailure” (ignore taskRun failure).

Please feel free to address this NIT in a separate PR or we implement as is.

/lgtm

[pritidesai]

/retest

[pritidesai]

/test pull-community-teps-lin

[tekton-robot]

@pritidesai: No presubmit jobs available for tektoncd/community@main

In response to this:

/test pull-community-teps-lin

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.