When workflow level retry expression evaluated False, workflow message is always retryStrategy.expression evaluated to false regardless of actual failure

[tczhao]

Pre-requisites

• I have double-checked my configuration
• I have tested with the :latest image tag (i.e. quay.io/argoproj/workflow-controller:latest) and can confirm the issue still exists on :latest. If not, I have explained why, in detail, in my description below.
• I have searched existing issues and could not find a match for this bug
• I'd like to contribute the fix myself (see contributing guide)

What happened/what did you expect to happen?

When workflow level retryExpression is used.
regardless of workflow succeed or failure,
the message at workflow level is always retryStrategy.expression evaluated to false

when no expression is used

• workflow succeed -> no workflow message
• workflow fail -> no more retry left

when expression is used

• workflow succeed -> retryStrategy.expression evaluated to false
• workflow fail (expression evaluated False) -> retryStrategy.expression evaluated to false
• workflow fail (expression evaluated True) -> no more retry left

This is problematic if one relies on the workflow message for metrc, analytics or debug.
We expect when expression is used, the workflow level message follows the same pattern as normal retry

Version

v3.5.2, latest (71f1d86)

Paste a small workflow that reproduces the issue. We must be able to run the workflow; don't enter a workflows that uses private images.

apiVersion: argoproj.io/v1alpha1
kind: Workflow
metadata:
  generateName: dag-nested-
  namespace: argo
spec:
  templates:
    - name: echo
      outputs: {}
      metadata: {}
      container:
        name: ''
        image: alpine:3.7
        command:
          - exit
          - '1'
        resources: {}
    - name: nested-diamond
      outputs: {}
      metadata: {}
      dag:
        tasks:
          - name: A
            template: echo
          - name: B
            template: echo
            depends: A
          - name: C
            template: echo
            depends: A
          - name: D
            template: echo
            depends: B && C
  entrypoint: nested-diamond
  arguments: {}
  retryStrategy:
    retryPolicy: OnError
    expression: >-
      lastRetry.status == 'Errored'

Logs from the workflow controller

kubectl logs -n argo deploy/workflow-controller | grep ${workflow}

Logs from in your workflow's wait container

kubectl logs -n argo -c wait -l workflows.argoproj.io/workflow=${workflow},workflow.argoproj.io/phase!=Succeeded

[agilgur5]

v3.5.8

Did you mean v3.4.8? or v3.5.6? Because the version you wrote doesn't exist right now

[tczhao]

Sorry, I was meant to add the commit hash for latest. Nothing to do with x.x.8
Updated

[Joibel]

The expression is evaluated regardless of strategy, and should it return false it will do what you're seeing. I'm not sure why this is the first time anyone has noticed this.

[tooptoop4]

won't this mean the backoff wait is done regardless of retry expression being evaluated to true?

argo-workflows/workflow/controller/operator.go

Lines 1041 to 1042 in 6201d75

	woc.requeueAfter(timeToWait)
	retryMessage := fmt.Sprintf("Backoff for %s", humanize.Duration(timeToWait))