Update Template Resolution Logic

[dtaniwaki]

Fixes #1738 #1713

The current template resolution logic processes input parameters and arguments while it merges other fields of templates. This logic caused lots of unexpected incorrect parameter values. I removed this feature and store all the templates used to resolve templates including intermediate ones and resolve other templates with them.

Haven't finished the implementation.

[dtaniwaki]

I tested the fix with the corresponded issues and got the following results.

#1738

$ argo get dep-sample-cwd7f
Name:                dep-sample-cwd7f
Namespace:           default
ServiceAccount:      default
Status:              Succeeded
Created:             Sun Nov 10 20:40:54 +0900 (1 minute ago)
Started:             Sun Nov 10 20:40:54 +0900 (1 minute ago)
Finished:            Sun Nov 10 20:41:03 +0900 (58 seconds ago)
Duration:            9 seconds
Parameters:
  msg-list:          ["A","B","C"]


STEP                              PODNAME                      DURATION  MESSAGE
 ✔ dep-sample-cwd7f (dep-sample)
 ├-✔ PT(0:A) (dep-sample-C)
 | ├---✔ T1 (echo)                dep-sample-cwd7f-270352588   3s
 | └---✔ T2 (echo)                dep-sample-cwd7f-3420327622  3s
 ├-✔ PT(1:B) (dep-sample-C)
 | ├---✔ T1 (echo)                dep-sample-cwd7f-2430481498  3s
 | └---✔ T2 (echo)                dep-sample-cwd7f-1055401976  3s
 └-✔ PT(2:C) (dep-sample-C)
   ├---✔ T1 (echo)                dep-sample-cwd7f-1878469948  3s
   └---✔ T2 (echo)                dep-sample-cwd7f-1335797558  2s
$ argo logs dep-sample-cwd7f -w
T1:     B
T1:     A
T1:     C
T2:     A
T2:     C
T2:     B

#1713

$ argo get parallelism-nested-sfjkn
Name:                parallelism-nested-sfjkn
Namespace:           default
ServiceAccount:      default
Status:              Succeeded
Created:             Sun Nov 10 20:35:31 +0900 (8 minutes ago)
Started:             Sun Nov 10 20:35:31 +0900 (8 minutes ago)
Finished:            Sun Nov 10 20:36:43 +0900 (7 minutes ago)
Duration:            1 minute 12 seconds
Parameters:
  seq-list:          ["a","b","c","d"]

  parallel-list:     [1,2,3,4]


STEP                                           PODNAME                              DURATION  MESSAGE
 ✔ parallelism-nested-sfjkn (parallel-worker)
 └-·-✔ parallel-worker(0:1) (seq-worker)
   | └-·-✔ seq-step(0:a) (one-job)             parallelism-nested-sfjkn-3959650130  18s
   |   ├-✔ seq-step(1:b) (one-job)             parallelism-nested-sfjkn-3831948056  14s
   |   ├-✔ seq-step(2:c) (one-job)             parallelism-nested-sfjkn-315680698   15s
   |   └-✔ seq-step(3:d) (one-job)             parallelism-nested-sfjkn-2002479964  16s
   ├-✔ parallel-worker(1:2) (seq-worker)
   | └-·-✔ seq-step(0:a) (one-job)             parallelism-nested-sfjkn-2266686296  15s
   |   ├-✔ seq-step(1:b) (one-job)             parallelism-nested-sfjkn-2114689010  14s
   |   ├-✔ seq-step(2:c) (one-job)             parallelism-nested-sfjkn-1059507776  14s
   |   └-✔ seq-step(3:d) (one-job)             parallelism-nested-sfjkn-883215278   15s
   ├-✔ parallel-worker(2:3) (seq-worker)
   | └-·-✔ seq-step(0:a) (one-job)             parallelism-nested-sfjkn-3021866506  21s
   |   ├-✔ seq-step(1:b) (one-job)             parallelism-nested-sfjkn-3208425328  15s
   |   ├-✔ seq-step(2:c) (one-job)             parallelism-nested-sfjkn-2923803410  15s
   |   └-✔ seq-step(3:d) (one-job)             parallelism-nested-sfjkn-2999951252  15s
   └-✔ parallel-worker(3:4) (seq-worker)
     └-·-✔ seq-step(0:a) (one-job)             parallelism-nested-sfjkn-3704432332  24s
       ├-✔ seq-step(1:b) (one-job)             parallelism-nested-sfjkn-3820876950  16s
       ├-✔ seq-step(2:c) (one-job)             parallelism-nested-sfjkn-3833729540  14s
       └-✔ seq-step(3:d) (one-job)             parallelism-nested-sfjkn-2110064090  15s
$ argo logs parallelism-nested-sfjkn  -w
seq-step(0:a):  2 a
seq-step(0:a):  1 a
seq-step(0:a):  3 a
seq-step(0:a):  4 a
seq-step(1:b):  2 b
seq-step(1:b):  1 b
seq-step(1:b):  3 b
seq-step(1:b):  4 b
seq-step(2:c):  2 c
seq-step(2:c):  1 c
seq-step(2:c):  3 c
seq-step(2:c):  4 c
seq-step(3:d):  2 d
seq-step(3:d):  1 d
seq-step(3:d):  3 d
seq-step(3:d):  4 d

[oskoss]

We built an image with this and confirms it fixes #1738

[simster7]

Great work @dtaniwaki! I just have a question and some minor comments

[simster7]

I'm sure I'm missing something obvious, but could you help me understand why this is implemented in both Workflow and wfOperationCtx? It seems like all the implementation for wfOperationCtx does is call the implementation of the Workflow:

// GetStoredTemplate retrieves a template from stored templates of the workflow.
func (woc *wfOperationCtx) GetStoredTemplate(templateScope string, holder wfv1.TemplateHolder) *wfv1.Template {
	return woc.wf.GetStoredTemplate(templateScope, holder)
}

Why do you not pass the Workflow as the TemplateStorage instead of the wfOperationCtx in operator.go:

	woc.tmplCtx = templateresolution.NewContext(wfc.wftmplInformer.Lister().WorkflowTemplates(wf.Namespace), wf, &woc)

Like you do in validate.go:

tmplCtx := templateresolution.NewContextFromClientset(wfClientset.ArgoprojV1alpha1().WorkflowTemplates(namespace), wf, wf)

Sorry if this is an obvious question.

[dtaniwaki]

That's because I wanted to let the operator save the workflow.
https://github.com/argoproj/argo/blob/fae7382686d917d78e3909d1f6db79c272a1aa11/workflow/controller/operator.go#L381-L384

Need to flag woc.updated if new stored templates are saved.

[simster7]

Thanks @dtaniwaki. Please make sure you take a look at #1713 (comment) if you haven't already!

[dtaniwaki]

@simster7 Thank you for pointing it out. I think PodSpecPatch didn't exist when I started this branch. I will take care of it.

[dtaniwaki]

@simster7 I tried examples/pod-spec-patch-wf-tmpl.yaml in my local and confirmed the patched resources are applied with the current branch HEAD 2390abe. Maybe, he tried the branch HEAD without merges from the master?

$ argo get -o yaml pod-spec-patch-pgnp5
metadata:
  creationTimestamp: "2019-11-13T15:29:57Z"
  generateName: pod-spec-patch-
  generation: 5
  labels:
    workflows.argoproj.io/completed: "true"
    workflows.argoproj.io/phase: Succeeded
  name: pod-spec-patch-pgnp5
  namespace: default
  resourceVersion: "3360702"
  selfLink: /apis/argoproj.io/v1alpha1/namespaces/default/workflows/pod-spec-patch-pgnp5
  uid: 72aa9ef5-2e03-4f75-94ee-bcbf53b48a31
spec:
  arguments:
    parameters:
    - name: cpu-limit
      value: 100m
    - name: mem-limit
      value: 100Mi
  entrypoint: whalesay
  podSpecPatch: |
    containers:
      - name: main
        resources:
          limits:
            memory: "{{workflow.parameters.mem-limit}}"
  templates:
  - arguments: {}
    container:
      args:
      - hello world
      command:
      - cowsay
      image: docker/whalesay:latest
      name: ""
      resources: {}
    inputs: {}
    metadata: {}
    name: whalesay
    outputs: {}
    podSpecPatch: '{"containers":[{"name":"main", "resources":{"limits":{"cpu": "{{workflow.parameters.cpu-limit}}"
      }}}]}'
status:
  finishedAt: "2019-11-13T15:30:05Z"
  nodes:
    pod-spec-patch-pgnp5:
      displayName: pod-spec-patch-pgnp5
      finishedAt: "2019-11-13T15:30:05Z"
      id: pod-spec-patch-pgnp5
      name: pod-spec-patch-pgnp5
      phase: Succeeded
      startedAt: "2019-11-13T15:29:57Z"
      templateName: whalesay
      type: Pod
  phase: Succeeded
  startedAt: "2019-11-13T15:29:57Z"

[dtaniwaki]

Or, maybe CLI version was different based on his comment of does not get applied or even stored with a workflow.

[oskoss]

Hi @dtaniwaki I am working with @simonwa7, and I'd be happy to deploy and test this again if you have already built an image that I can use to test the podSpecPatch? Do you mind letting us know the image name? Cheers!

[oskoss]

Or, maybe CLI version was different based on his comment of does not get applied or even stored with a workflow.

We are using the latest... 2.4.2 CLI

[dtaniwaki]

@oskoss Could you give me the output of argo version command and manifest so I can try the same situation as yours?

[simonwa7]

@dtaniwaki, @oskoss and I have verified that this fixes the issues we were seeing related to the podSpecPatch and default input parameters not being overwritten! Hope we can get this merged in soon 🙃

[simster7]

We're aiming for a release next week. Team has been super busy the last two weeks over Kubecon, unfortunately.

[simster7]

@dtaniwaki Could you resolve the conflicts when you get a chance?

[dtaniwaki]

@simster7 I fixed the conflicts with the master. Please also check #1732 for next release.

[simster7]

@dtaniwaki Thanks a lot. I'll make sure that both get looked at

[sarabala1979]

What is the use of this field? I can see failed node status has templateref which has the same information

    workflow-template-retry-with-steps-8ddk4-3945074098:
      boundaryID: workflow-template-retry-with-steps-8ddk4
      displayName: hello2a(0)
      finishedAt: "2019-11-25T21:38:27Z"
      id: workflow-template-retry-with-steps-8ddk4-3945074098
      message: failed with exit code 1
      name: workflow-template-retry-with-steps-8ddk4[1].hello2a(0)
      phase: Failed
      startedAt: "2019-11-25T21:38:23Z"
      templateRef:
        name: workflow-template-random-fail-template
        template: random-fail-template
      templateScope: workflow-template-random-fail-template
      type: Pod

[dtaniwaki]

TemplateScope is used to resolve workflow-template-local templates. Because we use 2 types of references: Template and TemplateRef, this field is necessary. If we can remove Template and all the references are made with TemplateRef, we don't need TemplateScope and simplify the code. What do you think?

[Ark-kun]

I still not fully understand the need for this.
We can just have resolution logic where .TemplateRef is checked only when .Template is empty.

[sarabala1979]

Do we really need this function?

[sarabala1979]

same as above comments

[sarabala1979]

@dtaniwaki I am playing with your branch. argo retry and retryStrategy both cases are looking the workflowtemplate from api call instead of referring from stored template. I tried below template to make a failure in DAG workflow.

Steps:

1. Submit the WF
2. once it failed WF, delete the workflow-template-random-failure-template
3. retry WF
4. WF failed with workflow template workflow-template-random-failure-template not found

Do you think it is expected behavior? I kind of thinking we have already content in WF storedworkflow. WF should execute that step/task with stored content.

apiVersion: argoproj.io/v1alpha1
kind: WorkflowTemplate
metadata:
  name: workflow-template-inner-dag
spec:
  templates:
  - name: whalesay-template
    inputs:
      parameters:
      - name: message
    container:
      image: docker/whalesay
      command: [cowsay]
      args: ["{{inputs.parameters.message}}"]
  - name: inner-diamond
    dag:
      tasks:
      - name: inner-A
        templateRef:
          name: workflow-template-random-failure-template
          template: random-fail-template
        arguments:
          parameters:
          - name: message
            value: inner-A
      - name: inner-B
        dependencies: [inner-A]
        template: whalesay-template
        arguments:
          parameters:
          - name: message
            value: inner-B
      - name: inner-C
        dependencies: [inner-A]
        template: whalesay-template
        arguments:
          parameters:
          - name: message
            value: inner-C
      - name: inner-D
        dependencies: [inner-B, inner-C]
        templateRef:
          name: workflow-template-whalesay-template
          template: whalesay-template
        arguments:
          parameters:
          - name: message
            value: inner-D
---
apiVersion: argoproj.io/v1alpha1
kind: WorkflowTemplate
metadata:
  name: workflow-template-random-failure-template
spec:
  templates:
    - name: random-fail-template
      container:
        image: python:alpine3.6
        command: [python, -c]
        # fail with a 66% probability
        args: ["import random; import sys; exit_code = random.choice([0, 1, 1]); sys.exit(exit_code)"]

[dtaniwaki]

Thank you for testing. Let me check it in my environment.

[dtaniwaki]

I realized the implementation worked with modified workflow templates while it didn't with missing workflow templates because the template resolution context cannot hold the template base info even if it won't use it.

Fixed it and confirmed #1744 (comment) has been fixed.

[salanki]

Please be aware of a bug introduced by this PR: #1876