fix: controller: ensures workflow reconciling task result properly when failing to received timely updates from api server

[bom-d-van]

There seems to be no related open issue, so I just submit a pr. but it's related to #12537

Motivation

error scenario: a pod for a step in a workflow has completed, and its task result are properly created and finalized by its wait container (judging from the exit status of the wait container), however, the task result informer in the controller leader has not received any updates about it (due to overloaded api server or etcd).

currently, the argo workflow controller doesn't handle the above scenario properly. it would mark the workflow node succeeded and shows no artifact outputs (even though they are already uploaded to the repository).

we did run into this situation in our production instance (it's v3.5.8).

it's not easy to reproduce this problem, but we can have a manual fault injection in workflow/controller/taskresult.go:func (woc *wfOperationCtx) taskResultReconciliation() to simulate the situation and I did reproduce the issue on release v3.6.2:

+++ workflow/controller/taskresult.go
@@ -1,7 +1,9 @@
 package controller

 import (
+	"os"
 	"reflect"
+	"strings"
 	"time"

 	log "github.com/sirupsen/logrus"
@@ -62,6 +64,12 @@ func (woc *wfOperationCtx) taskResultReconciliation() {
 	objs, _ := woc.controller.taskResultInformer.GetIndexer().ByIndex(indexes.WorkflowIndex, woc.wf.Namespace+"/"+woc.wf.Name)
 	woc.log.WithField("numObjs", len(objs)).Info("Task-result reconciliation")

+	if strings.Contains(woc.wf.Name, "-xhu-debug-") {
+		if _, err := os.Stat("/tmp/xhu-debug-control"); err != nil {
+			return
+		}
+	}

Modifications

the change is to forcefully mark the workflow having incomplete TaskResult in assessNodeStatus.

this fix doesn't handle the case when a pod failed, there are too many potentially failure scenarios (like the wait container might not be able to insert a task result). plus, a retry is probably needed when there are failures. the loss is probably not as great as a successful one.

Verification

it's covered by the updated test case and my manual verification details are included bellow (using the fault injection above).

test workflow to verify the fix:

apiVersion: argoproj.io/v1alpha1
kind: Workflow
metadata:
  generateName: test-xhu-debug-
spec:
  entrypoint: bash-script-example
  activeDeadlineSeconds: 3600
  templates:
  - name: bash-script-example
    steps:
    - - name: generate
        template: gen-random-int-bash
    - - name: print
        template: print-message
        arguments:
          parameters:
          - name: message
            value: "{{steps.generate.outputs.result}}"  # The result of the here-script

  - name: gen-random-int-bash
    outputs:
      artifacts:
      # generate hello-art artifact from /tmp/hello_world.txt
      # artifacts can be directories as well as files
      - name: hello-art
        path: /tmp/hello_world.txt
    script:
      image: reg.deeproute.ai/deeproute-public/nicolaka/netshoot:v0.13
      command: [bash]
      source: |                                         # Contents of the here-script
        touch /tmp/hello_world.txt
        cat /dev/urandom | od -N2 -An -i | awk -v f=1 -v r=100 '{printf "%i\n", f + r * $1 / 65536}'

  - name: print-message
    inputs:
      parameters:
      - name: message
    outputs:
      artifacts:
      # generate hello-art artifact from /tmp/hello_world.txt
      # artifacts can be directories as well as files
      - name: hello-art2
        path: /tmp/hello_world.txt
    container:
      image: reg.deeproute.ai/deeproute-public/nicolaka/netshoot:v0.13
      command: [sh, -c]
      args: ["touch /tmp/hello_world.txt; echo result was: {{inputs.parameters.message}}"]

with the manual fault injection, without the fix:

with the manual fault injection, with the fix:

[tczhao]

Great finding.
A delay in taskresultInformer will result in the issue you are seeing.
However, if you experience k8sapi pressure and the retry inside informer fails, you will see a much bigger problem. e.g. argo uses the cache from informer, the k8sapi pressure may result in this cache out of sync, and they will never sync until you restarts controller

@shuangkun has more context related to this change, will ask him to review instead

[tczhao]

we have retry for transient errors in wait container to handle this

argo-workflows/workflow/executor/executor.go

Lines 852 to 858 in 27a5938

	func (we *WorkflowExecutor) reportResult(ctx context.Context, result wfv1.NodeResult) error {
	return retryutil.OnError(wait.Backoff{
	Duration: time.Second,
	Factor: 2,
	Jitter: 0.1,
	Steps: 5,
	Cap: 30 * time.Second,

[bom-d-van]

Great finding. A delay in taskresultInformer will result in the issue you are seeing. However, if you experience k8sapi pressure and the retry inside informer fails, you will see a much bigger problem. e.g. argo uses the cache from informer, the k8sapi pressure may result in this cache out of sync, and they will never sync until you restarts controller

@shuangkun has more context related to this change, will ask him to review instead

yep, but in a way, a controller restart could help us resolving the issue when things went terribly wrong.
arguably, at least for us, it's still better than having partially succeeded workflows.