Only mark PipelineRun as cancelled if actually cancelled 🛑 #2434
Conversation
googlebot
added
the
cla: yes
tekton-robot
added
the
size/M
|
@jerop :D |
tekton-robot
added
the
approved
|
|
||
| // Make the patch call fail, i.e. make it so that the controller fails to cancel the TaskRun | ||
| clients.Pipeline.PrependReactor("patch", "taskruns", func(action k8stesting.Action) (bool, runtime.Object, error) { | ||
| fmt.Println(action.GetVerb(), action.GetResource()) |
There was a problem hiding this comment.
intentionally left in the test code or just for debugging? If intentional, suggest using t.Logf if possible to make that clear.
There was a problem hiding this comment.
whoops! definitely not on purpose 😅
bobcatfish
force-pushed
the
handle_cancel_failure
branch
from
3e382f0 to
d4519e8
Compare
While working on tektoncd#2369 (flakey tests around cancellation that actually revealed underlying bugs) we were running into a case where trying to cancel a PipelineRun's TaskRuns was failing (due to a race condition). In that case, the PipelineRun would be marked as cancelled and done, even though the PipelineRun was actually still executing in that one or more TaskRuns were actually still running. Now when that happens we will indicate that the PipelineRun is still running and return an error, which will mean that on the next reconcile, the Reconciler will try to reconcile again, and the PipelineRun conditions will reflect what is actually happening. Co-authored-by: Sharon Jerop Kipruto <[email protected]> Fixes tektoncd#2381
bobcatfish
force-pushed
the
handle_cancel_failure
branch
from
d4519e8 to
7b03775
Compare
|
Rebased this, PTAL! |
|
The following is the coverage report on the affected files.
|
|
|
||
| // Make the patch call fail, i.e. make it so that the controller fails to cancel the TaskRun | ||
| clients.Pipeline.PrependReactor("patch", "taskruns", func(action k8stesting.Action) (bool, runtime.Object, error) { | ||
| return true, nil, fmt.Errorf("i'm sorry Dave, i'm afraid i can't do that") |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dibyom, 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 |
|
/lgtm |
|
hmmm is this a flake? cuz if it is, im build cop tomorrow so that's /test pull-tekton-pipeline-integration-tests |
Changes
While working on #2369 (flakey tests around cancellation that actually
revealed underlying bugs) we were running into a case where trying to
cancel a PipelineRun's TaskRuns was failing (due to a race condition).
In that case, the PipelineRun would be marked as cancelled and done,
even though the PipelineRun was actually still executing in that one or
more TaskRuns were actually still running.
Now when that happens we will indicate that the PipelineRun is still
running and return an error, which will mean that on the next reconcile,
the Reconciler will try to reconcile again, and the PipelineRun
conditions will reflect what is actually happening.
Co-authored-by: Sharon Jerop Kipruto [email protected]
Fixes #2381
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:
cmddir, please updatethe release Task to build and release this image.
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