fix: workflow.status is now set properly in metrics. Fixes #8895

[dpadhiar]

Signed-off-by: Dillen Padhiar [email protected]

Fixes #8895

In Argo workflows v3.3.6, it was found that running an example workflow for custom metrics at the workflow & template levels would publish {{workflow.status}} as an empty value instead of showing the correct status. This is because the global runtime parameters are not accurately reflected by the stored workflow spec.

Example workflow (original issue):

apiVersion: argoproj.io/v1alpha1
kind: Workflow
metadata:
  generateName: dag-task-
spec:
  entrypoint: dag-task
  metrics: # Custom metric workflow level
    prometheus:
      - name: playground_workflow_duration
        help: "Duration gauge by workflow level"
        labels:
          - key: "playground_id_workflow"
            value: "test"
          - key: status
            value: "{{workflow.status}}"
        gauge:
          realtime: false
          value: "{{workflow.duration}}"
      - name: playground_workflow_result_counter
        help: "Count of workflow execution by result status  - workflow level"
        labels:
          - key: "playground_id_workflow_counter"
            value: "test"
          - key: status
            value: "{{workflow.status}}"
        counter:
          value: "1"
  templates:
  - name: dag-task
    dag:
      tasks:
      - name: TEST-ONE
        template: echo
        arguments:
          parameters: 
            - name: message
              value: "console output-->TEST-{{item.command}}"
            - name: tag
              value: "{{item.tag}}"
        withItems:
          - { tag: TEST-ONE-A, command: ONE-A }
          - { tag: TEST-ONE-B, command: ONE-B }

      - name: TEST-TWO
        template: echo
        arguments:
          parameters: 
            - name: message
              value: "console output-->TEST-{{item.command}}"
            - name: tag
              value: "{{item.tag}}"
        withItems:
          - { tag: TEST-TWO-A, command: TWO-A }
          - { tag: TEST-TWO-B, command: TWO-B }

  - name: echo
    inputs:
      parameters:
      - name: message
      - name: tag
    metrics: # Custom metric template level
      prometheus:
        - name: playground_workflow_duration_task_seconds
          help: "Duration gauge by task name in seconds - task level"
          labels:
            - key: "playground_task_name"
              value: "{{inputs.parameters.tag}}"
            - key: status
              value: "{{status}}"
          gauge:
            realtime: false
            value: "{{duration}}"
        - name: playground_workflow_result_task_counter
          help: "Count of task execution by result status  - task level"
          labels:
            - key: "playground_task_name_counter"
              value: "{{inputs.parameters.tag}}"
            - key: status
              value: "{{status}}"
          counter:
            value: "1"
    container:
      image: alpine:3.7
      command: [echo, "{{inputs.parameters.message}}"]

Completed workflow spec:

spec:
  activeDeadlineSeconds: 300
  arguments: {}
  entrypoint: dag-task
  metrics:
    prometheus:
    - gauge:
        realtime: false
        value: '{{workflow.duration}}'
      help: Duration gauge by workflow level
      labels:
      - key: playground_id_workflow
        value: test
      - key: status
        value: Succeeded
      name: playground_workflow_duration
    - counter:
        value: "1"
      help: Count of workflow execution by result status  - workflow level
      labels:
      - key: playground_id_workflow_counter
        value: test
      - key: status
        value: Succeeded
      name: playground_workflow_result_counter

[tczhao]

perhaps we should add something to the workflow/controller/operator_metrics_test.go as well

[sarabala1979]

@dpadhiar can you add test?

[dpadhiar]

@dpadhiar can you add test?

Yes, will do. @tczhao Will add in metrics test.

[tczhao]

Maybe the duration can be part of this function as well

argo-workflows/workflow/controller/operator.go

Lines 247 to 252 in 7fed546

	// Update workflow duration variable
	if woc.wf.Status.StartedAt.IsZero() {
	woc.globalParams[common.GlobalVarWorkflowDuration] = fmt.Sprintf("%f", time.Duration(0).Seconds())
	} else {
	woc.globalParams[common.GlobalVarWorkflowDuration] = fmt.Sprintf("%f", time.Since(woc.wf.Status.StartedAt.Time).Seconds())
	}

[dpadhiar]

Will add.

[tczhao]

should be "{{workflow.status}}"?

[tczhao]

I don't think this is testing the orignal bug.

You can write wf.yaml without the status
do operate twice with a pod phase change in between
and check metric before and after the 2nd operate to make sure it is changing

	makePodsPhase(ctx, woc, apiv1.PodSucceeded)
	woc = newWorkflowOperationCtx(woc.wf, controller)
	woc.operate(ctx)

[dpadhiar]

@tczhao When I am testing with {{workflow.status}} in the workflow, this is what error I am seeing with this test:

Error Trace:	operator_metrics_test.go:920
Error:      	Expected value not to be nil.
Test:       	TestRuntimeMetrics

The line it is failing on is metric := controller.metrics.GetCustomMetric(metricDesc) as it is returning nil metrics
However printing metricDesc shows it is as this string: playground_workflow_new{playground_id_workflow_counter=test,status={{workflow.status}},}

[tczhao]

Curious, I ran the failed e2e test locally and it worked fine

[dpadhiar]

Curious, I ran the failed e2e test locally and it worked fine

If I run the test again on Github, it may pass again. I've had multiple PRs where an e2e test that was unedited failed for taking too long at times.

[rvignesh89]

Is there any plan to release this in the upcoming release? I noticed this issue since v3.3.1 and have been waiting for the fix to be merged. Thanks!