fix: make sure taskresult completed when mark node succeed when it has outputs

[shuangkun]

When my cluster has lots of workflows, I meet some errors.

="Mark error node" error="failed to evaluate expression: cannot fetch steps-init-artifact from <nil> (1:6)\n | steps['init-artifact'].outputs.parameters['workflow_artifact_key']\n | .....^" namespace=argo nodeName="workflow-bhr9k[3].energy(0:0)[1].energy-steps(0:0)[3].comp-binding-energy-steps(0:0)[15]" workflow=workflow-bhr9k

When the number of workflows is not large, there is no such error.

My workflow has lots of template like this, the next step refer the output of the previous step. Like hello2a refer hello1 in parameter steps['hello1'].outputs.parameters['workflow_artifact_key'].

apiVersion: argoproj.io/v1alpha1
kind: Workflow
metadata:
  generateName: steps-
spec:
  entrypoint: hello-hello-hello
  arguments:
    parameters:
    - name: message1
      value: hello world
    - name: message2
      value: foobar
  # This spec contains two templates: hello-hello-hello and whalesay
  templates:
  - name: hello-hello-hello
    # Instead of just running a container
    # This template has a sequence of steps
    steps:
    - - name: hello1            # hello1 is run before the following steps
        continueOn: {}
        template: whalesay
        arguments:
          parameters:
          - name: message
            value: "hello1"
          - name: workflow_artifact_key
            value: "{{ workflow.parameters.message2}}"
    - - name: hello2a           # double dash => run after previous step
        template: whalesay
        arguments:
          parameters:
          - name: message
            value: "{{=steps['hello1'].outputs.parameters['workflow_artifact_key']}}"

  # This is the same template as from the previous example
  - name: whalesay
    metadata:
      annotations:
        k8s.aliyun.com/eci-spot-strategy: "SpotAsPriceGo"
    inputs:
      parameters:
      - name: message
    outputs:
      parameters:
      - name: workflow_artifact_key
        value: '{{workflow.name}}'
    script:
      image: python:alpine3.6
      command: [python]
      source: |
        import random
        i = random.randint(1, 100)
        print(i)

When I search the logs. I find the time of preStep(hello1)‘s “node changed” to succeed are earlier than "task-result changed". And
this cause the hello2a's evaluate expression error. So I want to make sure taskresult completed when mark node succeed when it has outputs.

Motivation

Modifications

Verification

[juliev0]

So, it sounds like previously the execution of a Workflow was allowed to continue even if the previous Step's Outputs weren't reconciled? Are you essentially preventing the next Step from running yet in that case?

[shuangkun]

So, it sounds like previously the execution of a Workflow was allowed to continue even if the previous Step's Outputs weren't reconciled? Are you essentially preventing the next Step from running yet in that case?

Yes, I want to prevent the next step from running.

[shuangkun]

Yes, I think this was the case before, although under normal circumstances output will be processed before pod status normally, because this resource is indeed created earlier. But on a large scale, these two processing orders may be caused by high pressure Events arrive at APIserver in different order

[juliev0]

Is it possible to see if this worked on some older versions of code? I'm curious if something broke this. It seems like core functionality.

[juliev0]

Yes, I think this was the case before, although under normal circumstances output will be processed before pod status normally, because this resource is indeed created earlier. But on a large scale, these two processing orders may be caused by high pressure Events arrive at APIserver in different order

I see. So, maybe this is a good enough answer to my request that you test on an older version - perhaps this case is just an unusual one? I am kind of curious if other people have logged similar bugs.

[shuangkun]

Is it possible to see if this worked on some older versions of code? I'm curious if something broke this. It seems like core functionality.

I think this may be related to the introduction of taskresult resources since 3.4. Maybe it is hard to support old， because there is a lack of records recording whether the taskresult was processed（Originally I needed to add this record, but found that it was included in the latest version.)

[shuangkun]

Yes, I think this was the case before, although under normal circumstances output will be processed before pod status normally, because this resource is indeed created earlier. But on a large scale, these two processing orders may be caused by high pressure Events arrive at APIserver in different order

I see. So, maybe this is a good enough answer to my request that you test on an older version - perhaps this case is just an unusual one? I am kind of curious if other people have logged similar bugs.

Yes, I think this was the case before, although under normal circumstances output will be processed before pod status normally, because this resource is indeed created earlier. But on a large scale, these two processing orders may be caused by high pressure Events arrive at APIserver in different order

I see. So, maybe this is a good enough answer to my request that you test on an older version - perhaps this case is just an unusual one? I am kind of curious if other people have logged similar bugs.

Yes，

failed to evaluate expression

Yes, I think this was the case before, although under normal circumstances output will be processed before pod status normally, because this resource is indeed created earlier. But on a large scale, these two processing orders may be caused by high pressure Events arrive at APIserver in different order

I see. So, maybe this is a good enough answer to my request that you test on an older version - perhaps this case is just an unusual one? I am kind of curious if other people have logged similar bugs.

Yes, I tested it on version 3.4.12 for few weeks. Looks well. There will be no "failed to evaluate expression" error like before.

[juliev0]

@Garett-MacGowan do you want to look at this too?

[Garett-MacGowan]

@Garett-MacGowan do you want to look at this too?

I just skimmed it. I can take a proper look after I 😴. In general, if we're proceeding to next steps before outputs are reconciled, it seems important that we add the wait behavior. As you said, it seems like core functionality, so I'm surprised if it's not already accounted for.

I'm wondering if this can be tested.

[Garett-MacGowan]

Same comment along the lines of what @juliev0 was saying, I don't think this is an error. We just need to flag needReconcileTaskResult. Could maybe just log it normally if you want the log?

[juliev0]

right, probably a Debug line

[juliev0]

By the way, do we need to call it for both woc.wf.Status.IsTaskResultInCompleted(node.ID) && woc.wf.Status.IsTaskResultInCompleted(pod.Name)?

[Garett-MacGowan]

Oh yeah, I was thinking this but had to step away and forgot to ask. I think it should just be tmpl.HasOutputs() && woc.wf.Status.IsTaskResultInCompleted(node.ID)

[Garett-MacGowan]

Maybe confusion from the comment here

[juliev0]

seeing that comment about the comment made me think of it :)

[shuangkun]

By the way, do we need to call it for both woc.wf.Status.IsTaskResultInCompleted(node.ID) && woc.wf.Status.IsTaskResultInCompleted(pod.Name)?

Yes, I thought about this problem at first. But there is a problem. If the outputs are in pod annotations or in taskresult, the key values ​​are different. Maybe we can unify to podName or NodeId. I think nodeId is better, how about you?

May be I can add a func named pod.GetNodeId()

	if x, ok := pod.Annotations[common.AnnotationKeyReportOutputsCompleted]; ok {
		woc.log.Warn("workflow uses legacy/insecure pod patch, see https://argo-workflows.readthedocs.io/en/latest/workflow-rbac/")
		resultName := pod.GetName()
		if x == "true" {
			woc.wf.Status.MarkTaskResultComplete(resultName)
		} else {
			woc.wf.Status.MarkTaskResultIncomplete(resultName)
		}
	}

[Garett-MacGowan]

Yes, just unify to Node ID.

[juliev0]

Sorry, I just realized that we probably need to move the if err != nil clause above the if !podReconciliationCompleted {, since we can return err, false

[juliev0]

hopefully after that we should be good, thank you for the iterations!

[shuangkun]

Yes, I changed it. Thanks!