Run finally pipeline even if task is failed at the validation #8314
Conversation
tekton-robot
added
do-not-merge/work-in-progress
|
/kind bug |
|
/test check-pr-has-kind-label |
|
@divyansh42: The specified target(s) for
The following commands are available to trigger optional jobs:
Use In response to this:
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. |
|
/test pull-tekton-pipeline-go-coverage-df |
|
@divyansh42: The specified target(s) for
The following commands are available to trigger optional jobs:
Use In response to this:
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. |
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
divyansh42
force-pushed
the
fix-finally-not-running
branch
from
57012ed
to
9cb5f91
Compare
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
divyansh42
force-pushed
the
fix-finally-not-running
branch
from
9cb5f91
to
5265441
Compare
divyansh42
changed the title
tekton-robot
removed
the
do-not-merge/work-in-progress
|
/hold cancel |
|
/cc @vdemeester @afrittoli @chitrangpatel |
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
Presently if one of the task in pipeline is consuming result from the previous task but the previous failed to produce the result then pipeline fails without running the finally tasks. These changes handles tasks which got failed in the validation step. Signed-off-by: divyansh42 <[email protected]>
divyansh42
force-pushed
the
fix-finally-not-running
branch
from
5265441
to
ceb2f6c
Compare
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
/test pull-tekton-pipeline-go-coverage-df |
|
@afrittoli: The specified target(s) for
The following commands are available to trigger optional jobs:
Use In response to this:
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. |
|
The following is the coverage report on the affected files.
|
|
@afrittoli, I have made the changes as per the discussion that happened over the Slack channel. Could you please take a look again? |
| // The PipelineRun should be marked as failed due to InvalidTaskResultReference. | ||
| // The PipelineRun should be marked as failed |
There was a problem hiding this comment.
I guess the reason for failure will be available at TaskRun level, is that right?
There was a problem hiding this comment.
I was also concerned about not showing the exact reason for the failure. When I checked in the implementation inside the GetPipelineConditionStatus function, it gave status based on failed, and success. So it is more generalized.
The status will not be available at the TaskRun level as TaskRun will not be created.
Do you have any suggestions to deal with this in a better way so that we can have a proper reason for the failure?
Earlier we were failing right away so we were getting the exact error.
There was a problem hiding this comment.
One idea that I can think of is to have ValidationFailed as a map where it will store failed tasks along with the reason and inside the GetPipelineConditionStatus function we can have a check for the ValidationFailed and add the status accordingly.
If this looks good, I can try the implementation.
There was a problem hiding this comment.
One question that comes with this approach is when setting the reason in GetPipelineConditionStatus is how to decide the reason for the multiple Validation Failed Task.
May be we can set this to a generic error like https://github.com/tektoncd/pipeline/blob/main/pkg/apis/pipeline/v1/pipelinerun_types.go#L385
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: afrittoli 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:
Approvers can indicate their approval by writing |
tekton-robot
added
the
approved
|
@tektoncd/core-maintainers can we have a second review please? |
|
/lgtmz |
|
/lgtm |
|
Thank you @afrittoli @vdemeester for the reviews 🙏 |
|
@afrittoli should we backport it to LTS(es) ? (I missed it for 0.65.0...) |
|
/cherry-pick release-v0.65.x |
|
/cherry-pick release-v0.62.x |
|
/cherry-pick release-v0.59.x |
|
/cherry-pick release-v0.56.x |
|
@vdemeester: new pull request created: #8367 In response to this:
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: new pull request created: #8368 In response to this:
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: new pull request created: #8369 In response to this:
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: new pull request created: #8370 In response to this:
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. |
|
@afrittoli @vdemeester is it possible to have a patch release for this? I see |
|
@divyansh42 yes, we'll just wait for a few more bugfixes 👼🏼 |
Fixes: #7330
Changes
Presently if one of the tasks in the pipeline is consuming results from the previous task but the previous task failed to produce the result then the pipeline fails without running the
finallytasks.These changes handle tasks that failed in the validation step by adding it to the new field named
ValidationFailedTasksunder structPipelineRunFactswhich will have all the tasks for which the taskrun is not created as it failed in the validation step by the controller.These changes will also remove the logic of panic errors occurring if any of the validation fails. Instead of the controller returning a permanent error, it will remove all the tasks that have to be scheduled. This also changes the skip logic to skip the task and stop running the pipeline if any of the validation is failed.
Submitter Checklist
As the author of this PR, please check off the items in this checklist:
/kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tepRelease Notes