Repository: argoproj/argo (go)

Issue:
Retryable tasks with template-level metrics are falsely labelled as Running
## Summary 

When using a metric label value of `{{status}}` I expected a status like `Failed` or `Succeeded`. However, when a retryStrategy is defined on the task, the metric ends up having a label value of `Running`.

## Diagnostics

What Kubernetes provider are you using? Google Kubernetes Engine (GKE).

What version of Argo Workflows are you running? 2.12.3

`kube get wf -o yaml metrics-eg-j6w2z`
```yaml
apiVersion: argoproj.io/v1alpha1
kind: Workflow
metadata:
  creationTimestamp: "2021-01-08T05:48:52Z"
  generateName: metrics-eg-
  generation: 3
  labels:
    workflows.argoproj.io/completed: "true"
    workflows.argoproj.io/phase: Succeeded
  name: metrics-eg-j6w2z
  namespace: argo
  resourceVersion: "189179375"
  selfLink: /apis/argoproj.io/v1alpha1/namespaces/argo/workflows/metrics-eg-j6w2z
  uid: b07a727a-4913-411b-8877-05e619f2751f
spec:
  arguments: {}
  entrypoint: my-dag
  serviceAccountName: argo
  templates:
  - arguments: {}
    dag:
      tasks:
      - arguments: {}
        name: A
        template: A
    inputs: {}
    metadata: {}
    name: my-dag
    outputs: {}
  - arguments: {}
    container:
      args:
      - hello from A
      command:
      - cowsay
      image: docker/whalesay
      name: ""
      resources: {}
    inputs: {}
    metadata:
      annotations:
        sidecar.istio.io/inject: "false"
    metrics:
      prometheus:
      - counter:
          value: "1"
        help: Number of argo workflows
        labels:
        - key: work_unit
          value: metrics-eg::A
        - key: workflow_result
          value: '{{status}}'
        name: result_counter
    name: A
    outputs: {}
    retryStrategy:
      backoff:
        duration: 2s
        factor: 1
        maxDuration: 6m
      limit: 2
      retryPolicy: Always
status:
  conditions:
  - status: "True"
    type: Completed
  finishedAt: "2021-01-08T05:49:02Z"
  nodes:
    metrics-eg-j6w2z:
      children:
      - metrics-eg-j6w2z-1803019796
      displayName: metrics-eg-j6w2z
      finishedAt: "2021-01-08T05:49:02Z"
      id: metrics-eg-j6w2z
      name: metrics-eg-j6w2z
      outboundNodes:
      - metrics-eg-j6w2z-2894961471
      phase: Succeeded
      progress: 1/1
      resourcesDuration:
        cpu: 1
        memory: 1
      startedAt: "2021-01-08T05:48:52Z"
      templateName: my-dag
      templateScope: local/metrics-eg-j6w2z
      type: DAG
    metrics-eg-j6w2z-1803019796:
      boundaryID: metrics-eg-j6w2z
      children:
      - metrics-eg-j6w2z-2894961471
      displayName: A
      finishedAt: "2021-01-08T05:49:02Z"
      id: metrics-eg-j6w2z-1803019796
      name: metrics-eg-j6w2z.A
      outputs:
        exitCode: "0"
      phase: Succeeded
      progress: 1/1
      resourcesDuration:
        cpu: 1
        memory: 1
      startedAt: "2021-01-08T05:48:52Z"
      templateName: A
      templateScope: local/metrics-eg-j6w2z
      type: Retry
    metrics-eg-j6w2z-2894961471:
      boundaryID: metrics-eg-j6w2z
      displayName: A(0)
      finishedAt: "2021-01-08T05:48:55Z"
      id: metrics-eg-j6w2z-2894961471
      name: metrics-eg-j6w2z.A(0)
      outputs:
        exitCode: "0"
      phase: Succeeded
      progress: 1/1
      resourcesDuration:
        cpu: 1
        memory: 1
      startedAt: "2021-01-08T05:48:52Z"
      templateName: A
      templateScope: local/metrics-eg-j6w2z
      type: Pod
  phase: Succeeded
  progress: 1/1
  resourcesDuration:
    cpu: 1
    memory: 1
  startedAt: "2021-01-08T05:48:52Z"
```

`kubectl logs -n argo $(kubectl get pods -l app=workflow-controller -n argo -o name) | grep metrics-eg-j6w2z`
```
time="2021-01-08T05:48:52.575Z" level=info msg="Processing workflow" namespace=argo workflow=metrics-eg-j6w2z
time="2021-01-08T05:48:52.575Z" level=info msg="Updated phase  -> Running" namespace=argo workflow=metrics-eg-j6w2z
time="2021-01-08T05:48:52.576Z" level=info msg="DAG node metrics-eg-j6w2z initialized Running" namespace=argo workflow=metrics-eg-j6w2z
time="2021-01-08T05:48:52.576Z" level=info msg="All of node metrics-eg-j6w2z.A dependencies [] completed" namespace=argo workflow=metrics-eg-j6w2z
time="2021-01-08T05:48:52.577Z" level=info msg="Retry node metrics-eg-j6w2z-1803019796 initialized Running" namespace=argo workflow=metrics-eg-j6w2z
time="2021-01-08T05:48:52.577Z" level=info msg="Pod node metrics-eg-j6w2z-2894961471 initialized Pending" namespace=argo workflow=metrics-eg-j6w2z
time="2021-01-08T05:48:52.603Z" level=info msg="Created pod: metrics-eg-j6w2z.A(0) (metrics-eg-j6w2z-2894961471)" namespace=argo workflow=metrics-eg-j6w2z
time="2021-01-08T05:48:52.609Z" level=info msg="Workflow update successful" namespace=argo phase=Running resourceVersion=189179261 workflow=metrics-eg-j6w2z
time="2021-01-08T05:49:02.604Z" level=info msg="Processing workflow" namespace=argo workflow=metrics-eg-j6w2z
time="2021-01-08T05:49:02.605Z" level=info msg="Setting node metrics-eg-j6w2z-2894961471 outputs"
time="2021-01-08T05:49:02.605Z" level=info msg="Updating node metrics-eg-j6w2z-2894961471 status Pending -> Succeeded"
time="2021-01-08T05:49:02.605Z" level=info msg="node metrics-eg-j6w2z-1803019796 phase Running -> Succeeded" namespace=argo workflow=metrics-eg-j6w2z
time="2021-01-08T05:49:02.605Z" level=info msg="node metrics-eg-j6w2z-1803019796 finished: 2021-01-08 05:49:02.605855624 +0000 UTC" namespace=argo workflow=metrics-eg-j6w2z
time="2021-01-08T05:49:02.606Z" level=info msg="Outbound nodes of metrics-eg-j6w2z set to [metrics-eg-j6w2z-2894961471]" namespace=argo workflow=metrics-eg-j6w2z
time="2021-01-08T05:49:02.606Z" level=info msg="node metrics-eg-j6w2z phase Running -> Succeeded" namespace=argo workflow=metrics-eg-j6w2z
time="2021-01-08T05:49:02.606Z" level=info msg="node metrics-eg-j6w2z finished: 2021-01-08 05:49:02.606084969 +0000 UTC" namespace=argo workflow=metrics-eg-j6w2z
time="2021-01-08T05:49:02.606Z" level=info msg="Checking daemoned children of metrics-eg-j6w2z" namespace=argo workflow=metrics-eg-j6w2z
time="2021-01-08T05:49:02.606Z" level=info msg="Updated phase Running -> Succeeded" namespace=argo workflow=metrics-eg-j6w2z
time="2021-01-08T05:49:02.606Z" level=info msg="Marking workflow completed" namespace=argo workflow=metrics-eg-j6w2z
time="2021-01-08T05:49:02.606Z" level=info msg="Checking daemoned children of " namespace=argo workflow=metrics-eg-j6w2z
time="2021-01-08T05:49:02.614Z" level=info msg="Workflow update successful" namespace=argo phase=Succeeded resourceVersion=189179375 workflow=metrics-eg-j6w2z
time="2021-01-08T05:49:02.625Z" level=info msg="Labeled pod completed" namespace=argo pod=metrics-eg-j6w2z-2894961471
```

---
<!-- Issue Author: Don't delete this message to encourage other users to support your issue! -->
**Message from the maintainers**:

Impacted by this bug? Give it a 👍. We prioritise the issues with the most 👍.


Pull request description:
fix(controller): Consider processed retry node in metrics. Fixes #4846
Fixes https://github.com/argoproj/argo/issues/4846

Patch:
diff --git a/workflow/controller/operator.go b/workflow/controller/operator.go
index 9665e0b53..78437e74f 100644
--- a/workflow/controller/operator.go
+++ b/workflow/controller/operator.go
@@ -1698,7 +1698,7 @@ func (woc *wfOperationCtx) executeTemplate(ctx context.Context, nodeName string,
 				// The statement "(!ok || !prevNodeStatus.Fulfilled())" checks for this behavior and represents the material conditional
 				// "ok -> !prevNodeStatus.Fulfilled()" (https://en.wikipedia.org/wiki/Material_conditional)
 				if prevNodeStatus, ok := woc.preExecutionNodePhases[retryParentNode.ID]; (!ok || !prevNodeStatus.Fulfilled()) && retryParentNode.Fulfilled() {
-					localScope, realTimeScope := woc.prepareMetricScope(node)
+					localScope, realTimeScope := woc.prepareMetricScope(processedRetryParentNode)
 					woc.computeMetrics(processedTmpl.Metrics.Prometheus, localScope, realTimeScope, false)
 				}
 			}


Test patch:
diff --git a/workflow/controller/operator_metrics_test.go b/workflow/controller/operator_metrics_test.go
index 1e9d5ba82..06139cd29 100644
--- a/workflow/controller/operator_metrics_test.go
+++ b/workflow/controller/operator_metrics_test.go
@@ -485,3 +485,104 @@ func TestRealtimeWorkflowMetricWithGlobalParameters(t *testing.T) {
 	assert.NoError(t, err)
 	assert.Contains(t, metricErrorCounterString, `label:<name:"workflowName" value:"test-foobar" > gauge:<value:`)
 }
+
+var testProcessedRetryNode = `
+apiVersion: argoproj.io/v1alpha1
+kind: Workflow
+metadata:
+  name: metrics-eg-lq4nj
+spec:
+  entrypoint: my-dag
+  templates:
+  - dag:
+      tasks:
+      - name: A
+        template: A
+    name: my-dag
+  - container:
+      args:
+      - hello from A
+      command:
+      - cowsay
+      image: docker/whalesay
+    metrics:
+      prometheus:
+      - counter:
+          value: "1"
+        help: Number of argo workflows
+        labels:
+        - key: work_unit
+          value: metrics-eg::A
+        - key: workflow_result
+          value: '{{status}}'
+        name: result_counter
+    name: A
+    retryStrategy:
+      backoff:
+        duration: 2s
+        factor: 1
+        maxDuration: 6m
+      limit: 2
+      retryPolicy: Always
+status:
+  nodes:
+    metrics-eg-lq4nj:
+      children:
+      - metrics-eg-lq4nj-4266717436
+      displayName: metrics-eg-lq4nj
+      finishedAt: "2021-01-13T16:14:03Z"
+      id: metrics-eg-lq4nj
+      name: metrics-eg-lq4nj
+      outboundNodes:
+      - metrics-eg-lq4nj-2568729143
+      phase: Running
+      startedAt: "2021-01-13T16:13:53Z"
+      templateName: my-dag
+      templateScope: local/metrics-eg-lq4nj
+      type: DAG
+    metrics-eg-lq4nj-2568729143:
+      boundaryID: metrics-eg-lq4nj
+      displayName: A(0)
+      finishedAt: "2021-01-13T16:13:57Z"
+      id: metrics-eg-lq4nj-2568729143
+      name: metrics-eg-lq4nj.A(0)
+      phase: Succeeded
+      startedAt: "2021-01-13T16:13:53Z"
+      templateName: A
+      templateScope: local/metrics-eg-lq4nj
+      type: Pod
+    metrics-eg-lq4nj-4266717436:
+      boundaryID: metrics-eg-lq4nj
+      children:
+      - metrics-eg-lq4nj-2568729143
+      displayName: A
+      finishedAt: "2021-01-13T16:14:03Z"
+      id: metrics-eg-lq4nj-4266717436
+      name: metrics-eg-lq4nj.A
+      phase: Running
+      startedAt: "2021-01-13T16:13:53Z"
+      templateName: A
+      templateScope: local/metrics-eg-lq4nj
+      type: Retry
+  phase: Running
+  startedAt: "2021-01-13T16:13:53Z"
+`
+
+func TestProcessedRetryNode(t *testing.T) {
+	cancel, controller := newController()
+	defer cancel()
+	ctx := context.Background()
+	wfcset := controller.wfclientset.ArgoprojV1alpha1().Workflows("")
+	wf := unmarshalWF(testProcessedRetryNode)
+	_, err := wfcset.Create(ctx, wf, metav1.CreateOptions{})
+	assert.NoError(t, err)
+	woc := newWorkflowOperationCtx(wf, controller)
+
+	woc.operate(ctx)
+
+	metric := controller.metrics.GetCustomMetric("result_counter{work_unit=metrics-eg::A,workflow_result=Succeeded,}")
+	assert.NotNil(t, metric)
+	metricErrorCounterString, err := getMetricStringValue(metric)
+	assert.NoError(t, err)
+	assert.Contains(t, metricErrorCounterString, `value:1`)
+}