fix: Consider expanded tasks in getTaskFromNode

[simster7]

Fixes: #2743

[simster7]

The woc receiver was not used.

[nenadzaric]

Hi @simster7,

First, thank you for considering this fix, I really appreciate it!

Since I depend a lot on this feature, because of the scalability of my workflows, I wanted to test it for myself.

It looks like it is not working yet, and I am not sure if you finished working on this, but just in case I wanted to share what is happening.

kind: Workflow
metadata:
  generateName: parameter-aggregation-one-will-fail2-
spec:
  entrypoint: parameter-aggregation-one-will-fail2
  templates:
  - name: parameter-aggregation-one-will-fail2
    dag:
      tasks:

      - name: generate
        template: gen-number-list
        continueOn:
            failed: true
        
      - name: one-will-fail
        template: one-will-fail
        dependencies:
            - generate
        arguments:
          parameters:
          - name: num
            value: "{{item}}"
        withParam: "{{tasks.generate.outputs.result}}"
        continueOn:
          failed: true

      - name: whalesay
        template: whalesay
        dependencies:
          - one-will-fail
        continueOn:
          failed: true

  - name: one-will-fail
    inputs:
      parameters:
      - name: num
    container:
      image: alpine:latest
      command: [sh, -xc]
      args:
      - |
        if [ $(({{inputs.parameters.num}})) == 1 ]; then
          exit 1;
        else
          echo {{inputs.parameters.num}}
        fi

  - name: whalesay
    container:
      image: docker/whalesay:latest
      command: [cowsay]

  - name: gen-number-list
    script:
      image: python:alpine3.6
      command: [python]
      source: |
        import json
        import sys
        json.dump([i for i in range(0, 2)], sys.stdout)

So if parameters are passed through the parameter aggregation JSON list, with withParam: "{{tasks.generate.outputs.result}}" the workflow will fail.

Name:                parameter-aggregation-one-will-fail2-kv5dk
Namespace:           default
ServiceAccount:      default
Status:              Error
Message:             could not expand task one-will-fail while looking for task corresponding to node parameter-aggregation-one-will-fail2-kv5dk.one-will-fail(1:1): withParam value could not be parsed as a JSON list: {{tasks.generate.outputs.result}}
Created:             Mon Apr 20 16:24:53 +0200 (15 seconds ago)
Started:             Mon Apr 20 16:24:53 +0200 (15 seconds ago)
Finished:            Mon Apr 20 16:25:08 +0200 (now)
Duration:            15 seconds

STEP                                           TEMPLATE                              PODNAME                                                DURATION  MESSAGE
 ⚠ parameter-aggregation-one-will-fail2-kv5dk  parameter-aggregation-one-will-fail2                                                                   Workflow operation error
 ├-✔ generate                                  gen-number-list                       parameter-aggregation-one-will-fail2-kv5dk-2730234955  6s
 ├-✔ one-will-fail(0:0)                        one-will-fail                         parameter-aggregation-one-will-fail2-kv5dk-1938682683  5s
 └-✖ one-will-fail(1:1)                        one-will-fail                         parameter-aggregation-one-will-fail2-kv5dk-528872991   3s        failed with exit code 1

With an error

could not expand task one-will-fail while looking for task corresponding to node parameter-aggregation-one-will-fail2-kv5dk.one-will-fail(1:1): withParam value could not be parsed as a JSON list: {{tasks.generate.outputs.result}}

By the way, the workflow mentioned in the previous issue is working as intended.

Thanks!

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
0 Code Smells

87.5% Coverage
0.0% Duplication

[simster7]

After much thinking, I've found this to be the most robust way to get a task name from a node name: expanding all tasks when to match them to the node is costly and would require a code refactor, storing a node name to task name map is futile because tasks are not expanded on every operation of the Workflow, adding a field to NodeStatus is undesirable because it exposes implementation details to the API and the field is only used by DAG Nodes.

This function is safe because node names generated from task names are deterministic, so we should always be able to recover the node name from the task name.

If the naming convention of node names changes, this function will also have to change. To ensure that this happens I have created tests that cover this and have also added assert statements that ensure a node name can be reversed to the correct task name every time one is created.

[simster7]

I've made sure that this code is unreachable now, but if the naming convention is changed in the future this is meant to catch the discrepancy before the code can be shipped.

[simster7]

Also, panics are recovered in the operator, so they fail gracefully: https://github.com/argoproj/argo/blob/c6ef1ff19e1c3f74b4ef146be37e74bd0b748cd7/workflow/controller/operator.go#L159-L168

[alexec]

you should panic if there is a nasty bug - we log and report on panics

[simster7]

Thanks for the catch @nenadzaric!

[alexec]

thank you for the tests

[simster7]

Back-ported to 2.7