Make pipeline cancel robust to missing resources #5288
Conversation
When a pipelinerun is cancelled, it may be that some of the resources managed by the pipelinerun are missing (NotFound). In this case we should not fail, and let the pipelinerun be cancelled. Discussion about this was initially triggered by tektoncd#5282, but this feature makes sense regardless of the bug that exposed its need. Signed-off-by: Andrea Frittoli <andrea.frittoli@uk.ibm.com>
|
/cc @abayer |
|
The following is the coverage report on the affected files.
|
|
/test pull-tekton-pipeline-unit-tests |
|
@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. |
|
/retest |
|
/test pull-tekton-pipeline-unit-tests |
|
@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. |
| @@ -73,6 +74,21 @@ func init() { | |||
|
|
|||
| func cancelRun(ctx context.Context, runName string, namespace string, clientSet clientset.Interface) error { | |||
| _, err := clientSet.TektonV1alpha1().Runs(namespace).Patch(ctx, runName, types.JSONPatchType, cancelRunPatchBytes, metav1.PatchOptions{}, "") | |||
| if errors.IsNotFound(err) { | |||
There was a problem hiding this comment.
Should we instead first check if the resource exists before cancelling it?
There was a problem hiding this comment.
I personally prefer just ignoring not found errors - it keeps the code simpler and achieves the same end result.
There was a problem hiding this comment.
I agree but that causes unnecessary error showing up in the logs which can be confusing. I would rather avoid such error which will be ignored later by the same function
There was a problem hiding this comment.
I don't think it would show any error - it just returns nil if the TaskRun or Run doesn't exist.
|
thanks @afrittoli will this be part of the Tekton 0.38.3 release as well? |
|
/test check-pr-has-kind-label |
|
@abayer: 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. |
|
/assign @vdemeester |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: abayer, 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:
Approvers can indicate their approval by writing |
Changes
When a pipelinerun is cancelled, it may be that some of the resources
managed by the pipelinerun are missing (NotFound). In this case we
should not fail, and let the pipelinerun be cancelled.
Discussion about this was initially triggered by
#5282, but this feature
makes sense regardless of the bug that exposed its need.
Signed-off-by: Andrea Frittoli andrea.frittoli@uk.ibm.com
Submitter Checklist
As the author of this PR, please check off the items in this checklist:
functionality, content, code)
/kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tepRelease Notes
/kind feature