feat: Added onExit handlers to arbitrary templates

[simster7]

Fixes: #1688. Added onExit handlers to any template.

Example:

apiVersion: argoproj.io/v1alpha1
kind: Workflow
metadata:
  generateName: suspend-template-
spec:
  entrypoint: suspend
  templates:
  - name: suspend
    steps:
    - - name: steps1
        template: stepsTempalte
    - - name: steps2
        template: stepsTempalte

  - name: stepsTempalte
    onExit: exitContainer
    steps:
    - - name: leafA
        template: whalesay
    - - name: leafB
        template: whalesay

  - name: whalesay
    container:
      image: docker/whalesay
      command: [cowsay]
      args: ["hello world"]

  - name: exitContainer
    container:
      image: docker/whalesay
      command: [cowsay]
      args: ["goodbye world"]

Output:

Name:                suspend-template-nxslc
Namespace:           argo
ServiceAccount:      default
Status:              Succeeded
Created:             Tue Oct 29 09:08:18 -0700 (37 seconds ago)
Started:             Tue Oct 29 09:08:18 -0700 (37 seconds ago)
Finished:            Tue Oct 29 09:08:55 -0700 (now)
Duration:            37 seconds

STEP                                 PODNAME                            DURATION  MESSAGE
 ✔ suspend-template-nxslc (suspend)
 ├---✔ steps1 (stepsTempalte)
 |   ├---✔ leafA (whalesay)          suspend-template-nxslc-4023060684  4s
 |   ├---✔ leafB (whalesay)          suspend-template-nxslc-1080227016  4s
 |   └-✔ onExit (exitContainer)      suspend-template-nxslc-3814888236  5s
 └---✔ steps2 (stepsTempalte)
     ├---✔ leafA (whalesay)          suspend-template-nxslc-3324890710  4s
     ├---✔ leafB (whalesay)          suspend-template-nxslc-4274891518  4s
     └-✔ onExit (exitContainer)      suspend-template-nxslc-2655944446  5s

TODO:

• Validation
• Consider leaf nodes?
• Tests
• Examples

[simster7]

Implementation is now done and should be ready for review. onExit handlers work for all template types, inducing leaf nodes. Some examples:

1. container (suspend, script, and resource work similarly):

apiVersion: argoproj.io/v1alpha1
kind: Workflow
metadata:
  generateName: suspend-template-
spec:
  entrypoint: whalesay
  templates:
  - name: whalesay
    onExit: exitContainer
    suspend:
      duration: 5

  - name: exitContainer
    container:
      image: docker/whalesay
      command: [cowsay]
      args: ["goodbye world"]

Output:

Name:                suspend-template-szt9p
Namespace:           argo
ServiceAccount:      default
Status:              Succeeded
Created:             Wed Nov 06 08:32:20 -0800 (6 minutes ago)
Started:             Wed Nov 06 08:32:20 -0800 (6 minutes ago)
Finished:            Wed Nov 06 08:32:31 -0800 (5 minutes ago)
Duration:            11 seconds

STEP                                              PODNAME                           DURATION  MESSAGE
 ✔ suspend-template-szt9p (whalesay)              suspend-template-szt9p            5s

 ✔ suspend-template-szt9p.onExit (exitContainer)  suspend-template-szt9p-633898630  4s

2. container in steps (works similarly as in dag as well as nested steps and dags): See example file: https://github.com/argoproj/argo/blob/538415c9902d8a04ccdee2f563e69a37c06bbc24/examples/template-on-exit.yaml

Output:

Name:                container-on-exit-m6wq5
Namespace:           argo
ServiceAccount:      default
Status:              Succeeded
Created:             Wed Nov 06 08:25:16 -0800 (15 minutes ago)
Started:             Wed Nov 06 08:25:16 -0800 (15 minutes ago)
Finished:            Wed Nov 06 08:25:39 -0800 (15 minutes ago)
Duration:            23 seconds

STEP                                        PODNAME                             DURATION  MESSAGE
 ✔ container-on-exit-m6wq5 (step-template)
 ├---✔ stepA (whalesay)                     container-on-exit-m6wq5-3072376347  4s
 ├-✔ stepA.onExit (exitContainer)           container-on-exit-m6wq5-2711812994  4s
 ├---✔ stepB (whalesay)                     container-on-exit-m6wq5-2803967195  5s
 └-✔ stepB.onExit (exitContainer)           container-on-exit-m6wq5-22494146    4s

[mak-454]

@simster7 Leaf node exit handler may need access to Node status to be able to use "when" conditions.
Something like below,
- name: celebrate
template: celebrate
when: "{{templates.X.tasks.Y.status}} == Succeeded"
- name: cry
template: cry
when: "{{templates.X.tasks.Y.status}} != Succeeded"

If an exit handler is configured for a node can the node status be made available globally similar to workflow.status? something like below,

nodeid := fmt.Sprintf("templates.%s.tasks.%s.status",tmpl.Name, name)
woc.globalParams[nodeid] = string(node.Phase)

I think same applies for per template exit handler, it may need access to that template status.

[simster7]

@mak-454 when conditions are only available for WorkflowSteps and DAGTasks, they are not intended to be available for individual templates. Furthermore, onExit nodes are supposed to run regardless of any conditions (even if the parent node fails), so adding a when condition to them would be breaking this pattern.

If you're looking for a way to run a template conditionally after another, you can always create a steps or dag template and do so.

[mak-454]

@simster7 I was referring to an example similar like exit-handlers
Where an exit handler could be a template of WorkflowSteps and steps run based on the exit status of the actual DAGTask/WorkflowStep.
Is this a valid scenario for Exit handlers of a DAG task?

[simster7]

@mak-454 I see what you're saying now, sorry for the misunderstanding. Let me think about this.

[mak-454]

@simster7 no issues :)
I can think of one way is to make node phase available in global variable - woc.globalParams
which can be referred in exit handlers like,
templates..tasks..status or
templates..steps..status

[simster7]

@mak-454 I think what your request boils down to is to expose a step or dag node's status outside of their own template (in essence, expose it globally). Since I think this feature is independent of this PR, could you please open up a separate issue for it? That way we can keep the discussion here on-topic to this PR.

[mak-454]

@simster7 ok will create a separate issue.
Without this support - is there any other existing way for an exit handler of a task/step to have access to status/outputs of the actual task/step?

[naiduasn]

@simster7 any idea if this is going to merge in the coming release?

[sarabala1979]

This change will be released soon with workflow templates refactoring. We are planning to release v2.4.3 last week of November

[Ark-kun]

@simster7 Thank you for adding the more granular onExit handlers.
Do you think it's possible to move the onExit attribute from Template to DagTask and WorkflowStep? I think this would be more logical - we want to add exit handler to particular template instance which is Task/Step. This will be in line with how conditional execution (when) was added. Notice that when is added to tasks, not templates.
It's also similar how onExit works on the pipeline level: Pipeline specifies template (entrypoint), arguments and onExit. The DagTask can be specifying the same: template, arguments, onExit.

What do you think?

[simster7]

Hey @Ark-kun, thank you for your comment.

I am inclined to agree with you. Here is a bit of my thought process.

As a whole, I think onExit handlers are superfluous since one can always encapsulate a template to be "onExit-ed" within a steps template with continueOn: {"failed": true, "error": true}.

For example:

...
spec:
  entrypoint: whalesay
  templates:
  - name: whalesay
    onExit: exitContainer
    container:
      ...

  - name: exitContainer
    container:
      ...

Is really a shorthand for:

...
spec:
  entrypoint: whalesay
  templates:
  - name: onExitWhalesay
    steps:
      - - name: whalesay
          template: whalesay
          continueOn:
            failed: true
            error: true
      - - name: onExit
          template: exitContainer

  - name: whalesay
    container:
      ...

  - name: exitContainer
    container:
      ...

Since onExit is essentially a convenience feature, we would have to decide where on the spectrum of convenience we want to support. On one end is no onExit at all – in said scenario users would have to resort to the second example. On the other end is the current implementation, where basically anything that runs is allowed to have an onExit however superfluous that may be.

Your suggestion is on the middle of the two; we would essentially be trading convenience for some edge cases for a more consistent design. I am inclined to say that it's worth the trade-off, but others may feel it's up for discussion.

What I can say with certainty is that the design of this feature is not set in stone and still has to go through some internal discussions. I will certainly be bringing this up when they happen after KubeCon.

[mak-454]

@simster7 thanks for the explanation.
I was contemplating on the same thought regarding per template exit handler which can essentially be achieved by using continueOn as you have explained and with additional failFast: false if template was DAG. I understand now that onExit is more for convenience. For our internal workflows I could achieve the desired functionality using above flags.

I see one merit in moving onExit handlers to DAG Task or a workflowStep is that onExit template can have access to the status of the parent node. This will enable in writing onExit templates like below,

...
spec:
  entrypoint: whalesay
  templates:
  - name: onExitWhalesay
    templates:
      - - name: whalesay
          template: whalesay
          continueOn:
            failed: true
            error: true
      - - name: onExit
	  arguments:
	    parameters:
	    - name: status
              value: "{{steps.whalesay.status}}"
          template: exitHandler

  - name: whalesay
    container:
      image: docker/whalesay
      command: [cowsay]
      args: ["hello world"]

  - name: exitHandler
    inputs:
        parameters:
        - name: status
    steps:
    - - name: celebrate
        template: celebrate
        when: "{{inputs.parameters.status}} == Succeeded"
      - name: cry
        template: cry
        when: "{{inputs.parameters.status}} != Succeeded"

[Ark-kun]

which can essentially be achieved by using continueOn

BTW, This issue proposes a more capable replacement for continueOn: #1781

[simster7]

Update: After a discussion we have decided to go with the approach suggested by @Ark-kun and move the onExit handlers to DAGTask and WorkflowStep.

[sarabala1979]

@Ark-kun Thanks for your feedback. It makes sense. @simster7 will refactor it.

[simster7]

The refactor is complete. onExit is now only available on DAGTask and WorkflowStep.

Example:

apiVersion: argoproj.io/v1alpha1
kind: Workflow
metadata:
  generateName: container-on-exit-
spec:
  entrypoint: step-template
  templates:
    - name: step-template
      steps:
        - - name: stepA
            onExit: exitContainer
            template: whalesay
        - - name: stepB
            onExit: exitContainer
            template: whalesay

    - name: whalesay
      container:
        image: docker/whalesay
        command: [cowsay]
        args: ["hello world"]

    - name: exitContainer
      container:
        image: docker/whalesay
        command: [cowsay]
        args: ["goodbye world"]
---
apiVersion: argoproj.io/v1alpha1
kind: Workflow
metadata:
  generateName: dag-diamond-
spec:
  entrypoint: diamond
  templates:
  - name: diamond
    dag:
      tasks:
      - name: A
        template: whalesay
        onExit: exitContainer
      - name: B
        dependencies: [A]
        template: whalesay
        onExit: exitContainer
      - name: C
        dependencies: [A]
        template: whalesay
        onExit: exitContainer
      - name: D
        dependencies: [B, C]
        template: whalesay
        onExit: exitContainer

  - name: whalesay
    container:
      image: docker/whalesay
      command: [cowsay]
      args: ["hello world"]

  - name: exitContainer
    container:
      image: docker/whalesay
      command: [cowsay]
      args: ["goodbye world"]

[simster7]

@alexec @sarabala1979 Can I get another review on this since the code was refactored?

[alexec]

@sarabala1979 as you've more experience and have already commented - can I ask you to review this please?

[dtaniwaki]

Changed the code to use correct template contexts.
937ba2f