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

Design: "Finally" Steps in Tasks #2448

Closed
chhsia0 opened this issue Apr 20, 2020 · 18 comments
Closed

Design: "Finally" Steps in Tasks #2448

chhsia0 opened this issue Apr 20, 2020 · 18 comments
Labels
area/api Indicates an issue or PR that deals with the API. kind/feature Categorizes issue or PR as related to a new feature. lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete.

Comments

@chhsia0
Copy link
Contributor

chhsia0 commented Apr 20, 2020

Related issue: #1559

Motivating Use Cases

  • A unit test step fails but a subsequent step in the same Task should upload the results to a bucket regardless of that failure.
  • A Pull Request pipeline resource should update a PR with a comment reflecting the status of a failed build. The build happens in one step and then the PR resource could inject a step at the end which runs regardless of the error and knows how to update github/gitlab/etc with the status comment.

Proposed User-Facing Changes

Add a new Finally field to specify a list of sequential steps that run regardless of whether previous steps have succeeded or not. Example:

kind: Task
metadata:
  name: hello-world-finally-steps
spec:
  steps:
  - name: hello
    image: busybox
    command: ["echo", "hello"]
  - name: fail
    image: busybox
    command: ["false"]
  finally:
  - name: world
    image: busybox
    command: ["echo", "world"]

Design Doc

https://docs.google.com/document/d/1e1fagYDErliLnIwU1g_N3YKVyAProhs1l14UyPIoZuM/edit?usp=sharing

Alternative Design

Error Strategy in Task Steps (#1573)

@dibyom
Copy link
Member

dibyom commented Apr 20, 2020

/kind feature
/area api

Also related to #1684

@tekton-robot tekton-robot added kind/feature Categorizes issue or PR as related to a new feature. area/api Indicates an issue or PR that deals with the API. labels Apr 20, 2020
@dibyom dibyom added the priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. label Apr 23, 2020
@NikeNano
Copy link

The design doc. Is behind restricted access :( Should I apply for access through the google docs link or should the document be made public @dibyom?

@bobcatfish
Copy link
Collaborator

Hi @NikeNano ! Joining our mailing list should give you access: https://github.com/tektoncd/community/blob/master/contact.md#mailing-list

@NikeNano
Copy link

Thanks @bobcatfish :)

@afrittoli
Copy link
Member

@chhsia0 Are you still looking into this?

This may be rendered unnecessary by the pipeline resource redesign. If you feel this is high prio still feel free to bring it back at the API WG

@tekton-robot
Copy link
Collaborator

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.
If this issue is safe to close now please do so with /close.

/lifecycle rotten

Send feedback to tektoncd/plumbing.

@tekton-robot
Copy link
Collaborator

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

/close

Send feedback to tektoncd/plumbing.

@tekton-robot tekton-robot added the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Aug 14, 2020
@tekton-robot
Copy link
Collaborator

@tekton-robot: Closing this issue.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

/close

Send feedback to tektoncd/plumbing.

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.

@vdemeester
Copy link
Member

/remove-lifecycle rotten
/remove-lifecycle stale
/reopen

@tekton-robot
Copy link
Collaborator

@vdemeester: Reopened this issue.

In response to this:

/remove-lifecycle rotten
/remove-lifecycle stale
/reopen

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.

@tekton-robot tekton-robot reopened this Aug 17, 2020
@tekton-robot tekton-robot removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Aug 17, 2020
@tekton-robot
Copy link
Collaborator

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
If this issue is safe to close now please do so with /close.

/lifecycle stale

Send feedback to tektoncd/plumbing.

@tekton-robot tekton-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Nov 15, 2020
@vdemeester
Copy link
Member

/remove-lifecycle stale

@tekton-robot tekton-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Nov 16, 2020
@tekton-robot
Copy link
Collaborator

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale with a justification.
Stale issues rot after an additional 30d of inactivity and eventually close.
If this issue is safe to close now please do so with /close with a justification.
If this issue should be exempted, mark the issue as frozen with /lifecycle frozen with a justification.

/lifecycle stale

Send feedback to tektoncd/plumbing.

@tekton-robot tekton-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Feb 14, 2021
@jerop
Copy link
Member

jerop commented Feb 16, 2021

The use cases described here could be better solved using Pipelines in Pipelines because it:

  • allows us to reuse Finally Tasks instead of introducing Finally Steps
  • encourages writing Tasks with singular concerns then allowing them to be composed together as a unit of execution -- and this further supports reusing of tasks, such as slack and github catalog tasks to do the notifications when the notification is at task level instead of step level

A unit test step fails but a subsequent step in the same Task should upload the results to a bucket regardless of that failure.

This use case can be solved using a unit-test-and-upload Pipeline which has a unit-test Task and a upload Finally Task, and the unit-test-and-upload Pipeline is passed in as a Task in the main Pipeline

apiVersion: tekton.dev/v1beta1
kind: Pipeline
metadata:
  name: unit-test-and-upload
spec:
  tasks:
    - name: unit-test
      taskRef:
        name: unit-test
  finally:  
    - name: upload
      taskRef:
        name: upload

---

apiVersion: tekton.dev/v1beta1
kind: Pipeline
metadata:
  name: pipeline
spec:
  tasks:
    - name: unit-test-and-upload
      taskRef:
        apiVersion: tekton.dev/v1beta1
        kind: Pipeline
        name: unit-test-and-upload
... 

A Pull Request pipeline resource should update a PR with a comment reflecting the status of a failed build. The build happens in one step and then the PR resource could inject a step at the end which runs regardless of the error and knows how to update github/gitlab/etc with the status comment.

This use case can be solved using a build-and-notify Pipeline which has a build Task and a guarded notify Finally Task, and the build-and-notify Pipeline is passed in as a Task in the main Pipeline

apiVersion: tekton.dev/v1beta1
kind: Pipeline
metadata:
  name: build-and-notify
spec:
  tasks:
    - name: build
      taskRef:
        name: build
  finally:  
    - name: notify
      when:
        - input: $(tasks.build.status)
          operator: in
          values: [ "Failed" ] 
      taskRef:
        name: notify

---

apiVersion: tekton.dev/v1beta1
kind: Pipeline
metadata:
  name: pipeline
spec:
  tasks:
    - name: build-and-notify
      taskRef:
        apiVersion: tekton.dev/v1beta1
        kind: Pipeline
        name: build-and-notify
... 

So I propose going forward with Pipelines in Pipelines to solve these use cases, and this still leaves Finally Steps as an option we can explore in the future if needed, please let me know what you think @chhsia0

/cc @imjasonh @pritidesai @bobcatfish @afrittoli

@bobcatfish
Copy link
Collaborator

Thanks for summarizing this and adding examples @jerop !

One detail that I would add which makes using Pipelines as the solution difficult is that sharing data between Tasks in a Pipeline requires a workspace (and probably a backing PVC) in order to make the data available to all the Tasks (pods) (e.g. the unit test upload example above would probably need a workspace too).

If it was possible to colocate Tasks and Workspaces (TEP-0046) or if we had a way of composing Tasks with Tasks (executing in the same pod) (TEP-0044) that would be a different story!

@tekton-robot
Copy link
Collaborator

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten with a justification.
Rotten issues close after an additional 30d of inactivity.
If this issue is safe to close now please do so with /close with a justification.
If this issue should be exempted, mark the issue as frozen with /lifecycle frozen with a justification.

/lifecycle rotten

Send feedback to tektoncd/plumbing.

@tekton-robot tekton-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Mar 18, 2021
@tekton-robot
Copy link
Collaborator

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen with a justification.
Mark the issue as fresh with /remove-lifecycle rotten with a justification.
If this issue should be exempted, mark the issue as frozen with /lifecycle frozen with a justification.

/close

Send feedback to tektoncd/plumbing.

@tekton-robot
Copy link
Collaborator

@tekton-robot: Closing this issue.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen with a justification.
Mark the issue as fresh with /remove-lifecycle rotten with a justification.
If this issue should be exempted, mark the issue as frozen with /lifecycle frozen with a justification.

/close

Send feedback to tektoncd/plumbing.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/api Indicates an issue or PR that deals with the API. kind/feature Categorizes issue or PR as related to a new feature. lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete.
Projects
None yet
Development

No branches or pull requests

8 participants