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.

Sign up for GitHub

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

Jump to bottom

PipelineRun enters cancelled state even if cancelling TaskRuns fails #2381

Closed
ghost opened this issue Apr 13, 2020 · 6 comments · Fixed by #2434
Closed

PipelineRun enters cancelled state even if cancelling TaskRuns fails #2381

ghost opened this issue Apr 13, 2020 · 6 comments · Fixed by #2434
Assignees
@bobcatfish @jerop
Labels
kind/bug Categorizes issue or PR as related to a bug.

Comments

@ghost
Copy link

ghost commented Apr 13, 2020

Expected Behavior

Cancelling a PipelineRun should not succeed if the TaskRuns it created were not able to be cancelled.

Actual Behavior

In our PipelineRun cancellation code we immediately set the status of a PipelineRun to cancelled regardless of whether the TaskRuns it created were able to be cancelled.

This seems counter-intuitive and like a potential source of confusion for users. If TaskRuns couldn't be cancelled then that should probably be stored as some kind of error or event on the PipelineRun and the PipelineRun itself should remain in an un-cancelled state to indicate that there are still running processes related to it.
@ghost ghost added the kind/bug Categorizes issue or PR as related to a bug. label Apr 13, 2020
@bobcatfish bobcatfish assigned bobcatfish and unassigned bobcatfish Apr 14, 2020
@bobcatfish
Copy link
Collaborator

/assign jerop

@tekton-robot
Copy link
Collaborator

@bobcatfish: GitHub didn't allow me to assign the following users: jerop.

Note that only tektoncd members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/assign jerop

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.

@bobcatfish
Copy link
Collaborator

🤔

@jerop
Copy link
Member

jerop commented Apr 16, 2020

✋

@bobcatfish
Copy link
Collaborator

/assign jerop

@vdemeester
Copy link
Member

I ran the task manually to update members of the org 👼 We had.. a small issue on it 😝

bobcatfish added a commit to bobcatfish/pipeline that referenced this issue Apr 17, 2020
@bobcatfish
Only mark PipelineRun as cancelled if actually cancelled 🛑
3e382f0 
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 bobcatfish mentioned this issue Apr 17, 2020
Merged
2 tasks
bobcatfish added a commit to bobcatfish/pipeline that referenced this issue Apr 27, 2020
@bobcatfish
Only mark PipelineRun as cancelled if actually cancelled 🛑
d4519e8 
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 added a commit to bobcatfish/pipeline that referenced this issue May 11, 2020
@bobcatfish
Only mark PipelineRun as cancelled if actually cancelled 🛑
7b03775 
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
tekton-robot pushed a commit that referenced this issue May 11, 2020
@bobcatfish @tekton-robot
Only mark PipelineRun as cancelled if actually cancelled 🛑
62f41d8 
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@bobcatfish bobcatfish

@jerop jerop

Labels
kind/bug Categorizes issue or PR as related to a bug.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Only mark PipelineRun as cancelled if actually cancelled 🛑 bobcatfish/pipeline
4 participants
@vdemeester @bobcatfish @jerop @tekton-robot