-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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.
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
implementing pipeline level finally #2661
Conversation
This PR cannot be merged: expecting exactly one kind/ label Available
|
2 similar comments
This PR cannot be merged: expecting exactly one kind/ label Available
|
This PR cannot be merged: expecting exactly one kind/ label Available
|
/kind feature |
The following is the coverage report on the affected files.
|
4444763
to
aca2872
Compare
The following is the coverage report on the affected files.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mostly just looked at the docs so some comments there, but mostly I think this feature is looking great!!! I was focusing on reviewing the validation PR (#2650) and can review this in more detail after :D
aca2872
to
5efba4d
Compare
The following is the coverage report on the affected files.
|
5efba4d
to
cc30f9f
Compare
The following is the coverage report on the affected files.
|
cc30f9f
to
b9b4525
Compare
The following is the coverage report on the affected files.
|
b9b4525
to
fc893af
Compare
The following is the coverage report on the affected files.
|
fc893af
to
afaa4ac
Compare
The following is the coverage report on the affected files.
|
afaa4ac
to
eb8f5a2
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-integration-tests |
48fdc2e
to
21c4c85
Compare
The following is the coverage report on the affected files.
|
@bobcatfish @afrittoli @vdemeester I think I have addressed all the comments so far and ready for review.
|
21c4c85
to
37f21da
Compare
The following is the coverage report on the affected files.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for all the back and forth on this @pritidesai !
I'm still not sure why we have isDAGTasksStopped and checkTasksDone (details below) but since we've gone back and forth on this so much and since this is a detail completely hidden from the users and since the functions aren't even exported i'm happy to merge as-is.
Other than that, I'm happy to merge! Consider the PR approved from me :)
@vdemeester @afrittoli do you have any other blocking feedback?
Okay about isDAGTasksStopped and checkTasksDone:
I don't understand why we need both isDAGTasksStopped and checkTasksDone - both are called with a dag.Graph and both seem to want to return true if all tasks in the dag.Graph have stopped executing, but they have different names and totally different logic
is it reasonable to have checkTasksDone be the only function you need to call to be able to tell if all tasks in a dag.Graph have finished executing?
@@ -498,7 +568,7 @@ func GetPipelineConditionStatus(pr *v1beta1.PipelineRun, state PipelineRunState, | |||
|
|||
// Hasn't timed out; not all tasks have finished.... | |||
// Must keep running then.... | |||
if failedTasks > 0 || cancelledTasks > 0 { | |||
if cancelledTasks > 0 || (failedTasks > 0 && state.checkTasksDone(dfinally)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pritidesai : this additional check is making sure that pipeline does not exit when final tasks are not done executing.
Without this additional check state.isFinalTasksDone(dfinally), GetPipelineConditionStatus changes the pipeline status to stopping irrespective of where the final tasks are in their execution cycle (scheduled, started, running, failed, succeeded, retrying etc)
Thanks for explaining - can you add a comment explaining this?
I think it might also be worth explaining this progression in the markdown docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hey @bobcatfish I have updated doc and added comment explaining this.
We can now specify a list of tasks needs to be executed just before pipeline exits (either after finishing all non-final tasks successfully or after a single failure) Most useful for tasks such as report test results, cleanup cluster resources, etc ``` apiVersion: tekton.dev/v1beta1 kind: Pipeline metadata: name: pipeline-with-final-tasks spec: tasks: - name: pre-work taskRef: Name: some-pre-work - name: unit-test taskRef: Name: run-unit-test runAfter: - pre-work - name: integration-test taskRef: Name: run-integration-test runAfter: - unit-test finally: - name: cleanup-test taskRef: Name: cleanup-cluster - name: report-results taskRef: Name: report-test-results ```
37f21da
to
3e22d0e
Compare
The following is the coverage report on the affected files.
|
No, I would go ahead with this.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the latest updates!
/lgtm
/hold |
Sorry for making it so difficult to understand 😢 I will make one more attempt to explain it (while refactoring GetPipelineConditionStatus as we speak) 🙏 Pipeline is assigned
This is the result of GetPipelineConditionStatus considering not started tasks skipped on a single failure. There is no issue with it except it is not tracked outside of GetPipelineConditionStatus. This decision can be moved to And with or without Hope this make sense. |
@bobcatfish @afrittoli if its helpful, these changes can directly go on top of this PR and gets rid of extra isDAGTasksStopped function we are concerned about. It simplifies |
No blocker from my side 👼 I would have prefered not using the /approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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 |
excellent!!!! thanks @pritidesai |
@pritidesai has been a reviewer on many PRs (37 by the count at https://github.com/tektoncd/pipeline/pulls?q=is%3Apr+is%3Aopen+reviewed-by%3Apritidesai) and she has been the author of several PRs (27 at https://github.com/tektoncd/pipeline/pulls/pritidesai) including finally (tektoncd#2661) which was one of the most substantial changes to pipeline execution since we implemented DAGs (not to mention that she patiently worked with the evolution of this feature over several months from the runOn feature which evolved into this one). Thanks so much @pritidesai!!!! So excited that you want to become even more involved in pipelines :D :D :D
@pritidesai has been a reviewer on many PRs (37 by the count at https://github.com/tektoncd/pipeline/pulls?q=is%3Apr+is%3Aopen+reviewed-by%3Apritidesai) and she has been the author of several PRs (27 at https://github.com/tektoncd/pipeline/pulls/pritidesai) including finally (#2661) which was one of the most substantial changes to pipeline execution since we implemented DAGs (not to mention that she patiently worked with the evolution of this feature over several months from the runOn feature which evolved into this one). Thanks so much @pritidesai!!!! So excited that you want to become even more involved in pipelines :D :D :D
Changes
We can now specify a list of tasks needs to be executed just before
pipeline exits (either after finishing all non-final tasks successfully or after
a single failure)
Most useful for tasks such as report test results, cleanup cluster resources, etc
Summary of Changes:
pipelines.md
: Documented final tasks including how to specifyworkspaces
andparams
, documenting updates to pipeline run status as a result of finally clause, and known limitations (task results,from
clause, pipeline results,runAfter
, andconditions
).pipelinerun-with-final-tasks.yaml
: Example pipeline demonstrating cloning a repo into shared workspace and cleaning up the workspace.pipelinerun.go
: Building graph forfinal
tasks, buildpipelineState
for both DAG tasks and final tasks, invoke final tasks once all DAG tasks are done (execution queue is empty)pipelinerun_test.go
: Test three major test pipelines, (1) pipeline when a DAG task fails but final task succeeds, (2) pipeline when all DAG task succeeds but a final task fails, (3) pipeline when one of the DAG tasks fail and a final task failspipelinerunresolution.go
: ReplaceSuccessfulPipelineTaskNames
withSuccessfulOrSkippedDAGTasks
like explained here. Two helper functionsisDAGTasksDone
,isDAGTaskFailed
.GetFinalTasks
to return final tasks once they are eligible to execute.pipelinefinally_test.go
: Validate pipelineRun Status and taskRun Status along with competition time making sure (1) Final tasks are executed after all DAG tasks succeeds (2) Pipeline results in failure since a DAG task fails but final tasks does get executed successfully (3) Pipeline exits with failure when final task execution fails (4) The DAG task is skipped due to condition failure, final tasks is still executed.Example:
Design doc: https://docs.google.com/document/d/1lxpYQHppiWOxsn4arqbwAFDo4T0-LCqpNa6p-TJdHrw/edit#
Part of #1684
Fixes #2446
Depends on: #2650
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:
cmd
dir, 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