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.

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

TEP-0049: access aggregate status of the dag tasks from finally #371

Merged

Conversation

pritidesai
Copy link
Member

@pritidesai pritidesai commented Mar 5, 2021

TEP-0028 implemented params such as $(tasks.<pipelineTask>.status) to access the execution status of a dag task from the finally section. This works great for a pipeline where a finally task depends on the execution status of an individual dag task. We have discovered additional use cases where a finally task needs to be executed if ANY one of the dag tasks fail.

This TEP is proposing an additional variable $(tasks.status) to access aggregate status of the dag tasks.

Related issues and PR:

tektoncd/pipeline#3806
tektoncd/pipeline#1020 (comment)
tektoncd/pipeline#3738 (comment)

@tekton-robot tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 5, 2021
@pritidesai
Copy link
Member Author

/kind tep

@tekton-robot tekton-robot added the kind/tep Categorizes issue or PR as related to a TEP (or needs a TEP). label Mar 5, 2021
@jerop
Copy link
Member

jerop commented Mar 5, 2021

/assign

Copy link
Member

@jerop jerop left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

excited for this feature 🎉

thanks @pritidesai!

teps/0049-aggregate-status-of-dag-tasks.md Outdated Show resolved Hide resolved
@dibyom
Copy link
Member

dibyom commented Mar 8, 2021

/assign @skaegi

@pritidesai pritidesai force-pushed the pt-aggregate-status-in-finally branch 2 times, most recently from 0864902 to fac9248 Compare March 9, 2021 06:38
@tekton-robot tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 17, 2021
@pritidesai pritidesai force-pushed the pt-aggregate-status-in-finally branch from fac9248 to 935bba6 Compare March 17, 2021 17:27
@tekton-robot tekton-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 17, 2021
@eliihen
Copy link

eliihen commented Mar 25, 2021

Looks great, @pritidesai! You requested my feedback on if this matches my expectations in tektoncd/pipeline#3806 (comment), and it does indeed 👏🏻

It may be out of scope of this TEP, but the coming "pipeline-in-pipeline" proposal may affect this proposal and vice versa. If they work together it will be a pretty cool feature though. Just checking if you thought about any potential issues with the implementation there.

Finally, I just wanted to sanity check this proposal. Say we have a pipeline of tasks A -> B -> C. Would this proposal be necessary if we instead did the following:

finally:
  - when:
    - input: "$(tasks.C.status)"
      operator: notin
      values: ["Succeeded"]

Here we have a when condition that checks the status of the final task in the pipeline. If any of the tasks before task C fails, task C will not have the status of "Succeeded", so the finally will run. Thus the final task's status is sort of an aggregate of the pipeline's status. This was not immediately obvious to me earlier, but it seems to work. I'm not sure if this misses any usecases (what about forking pipelines for example), but it's fair to consider it. In any case this proposal provides a nice a nice shorthand and makes it easier to move tasks around.

@pritidesai
Copy link
Member Author

Looks great, @pritidesai! You requested my feedback on if this matches my expectations in tektoncd/pipeline#3806 (comment), and it does indeed 👏🏻

Thanks @esphen ❤️

It may be out of scope of this TEP, but the coming "pipeline-in-pipeline" proposal may affect this proposal and vice versa. If they work together it will be a pretty cool feature though. Just checking if you thought about any potential issues with the implementation there.

pipeline-in-pipeline is a great feature and still being designed. It will be great to have exposure to the subpipelines status in finally.

Finally, I just wanted to sanity check this proposal. Say we have a pipeline of tasks A -> B -> C. Would this proposal be necessary if we instead did the following:

finally:
  - when:
    - input: "$(tasks.C.status)"
      operator: notin
      values: ["Succeeded"]

Here we have a when condition that checks the status of the final task in the pipeline. If any of the tasks before task C fails, task C will not have the status of "Succeeded", so the finally will run. Thus the final task's status is sort of an aggregate of the pipeline's status. This was not immediately obvious to me earlier, but it seems to work. I'm not sure if this misses any usecases (what about forking pipelines for example), but it's fair to consider it.

This workaround would not work for the following pipeline with multiple last tasks:

          A
      /        \
    B          C

B and C are executed at the same time. One of them can fail while the other could succeed/skipped. Can not rely on the last tasks for the aggregate status in this example.

In any case this proposal provides a nice a nice shorthand and makes it easier to move tasks around.

Thank you 👍

Proposal to retrieve aggregate status of dag tasks in a finally task.
@pritidesai pritidesai force-pushed the pt-aggregate-status-in-finally branch from 935bba6 to 469e655 Compare March 25, 2021 22:48
Copy link
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 1, 2021
finally:
- name: notify-any-failure # executed only when one of the dag tasks fail
when:
- input: $(tasks.status)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can maybe leave this to the implementation, but we probably want to put this on the pipeline not on tasks like pipeline.status

@pierretasci
Copy link
Contributor

/approve

@tekton-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jerop, pierretasci

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 1, 2021
@tekton-robot tekton-robot merged commit 80d2aba into tektoncd:main Apr 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/tep Categorizes issue or PR as related to a TEP (or needs a TEP). lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants