Repository: argoproj/argo (go)

Issue:
Retry Strategy doesn't work in GKE preemptible node (pod deleted)
Checklist:

 * [x] I've included the version.
 * [ ] I've included reproduction steps.
 * [ ] I've included the workflow YAML.
 * [x] I've included the logs.
 
**What happened**:
I configured the retry strategy as the following
```
retryStrategy:
      limit: 4
      retryPolicy: "Always"
      backoff:
        duration: "12s"
        factor: 5
        maxDuration: "200m"
```
and run the Argo workflow in a GKE preemptible nodes.
When GKE required resources back deleting the nodes, the retry strategy doesn't work.
I received a red warning /!\ without the retry.

**What you expected to happen**:
I expected the retry strategy to works properly because I setup **retryPolicy: "Always"** as well.

**How to reproduce it (as minimally and precisely as possible)**:
Create a workflow with the following retry Strategy and run it in GKE GCP in a preemptible node
```
retryStrategy:
      limit: 4
      retryPolicy: "Always"
      backoff:
        duration: "12s"
        factor: 5
        maxDuration: "200m"
```

**Anything else we need to know?**:

**Environment**:
GKE (GCP)
- Argo version: v2.7.6
```
$ argo version
argo: v2.7.6+70facdb.dirty
  BuildDate: 2020-04-28T17:09:06Z
  GitCommit: 70facdb67207dbe115a9029e365f8e974e6156bc
  GitTreeState: dirty
  GitTag: v2.7.6
  GoVersion: go1.13.4
  Compiler: gc
  Platform: darwin/amd64
```
- Kubernetes version :
```
$ kubectl version -o yaml
clientVersion:
  buildDate: "2020-02-06T03:31:35Z"
  compiler: gc
  gitCommit: f5757a1dee5a89cc5e29cd7159076648bf21a02b
  gitTreeState: clean
  gitVersion: v1.14.10-dispatcher
  goVersion: go1.12.12b4
  major: "1"
  minor: 14+
  platform: darwin/amd64
serverVersion:
  buildDate: "2020-02-21T18:01:40Z"
  compiler: gc
  gitCommit: 145f9e21a4515947d6fb10819e5a336aff1b6959
  gitTreeState: clean
  gitVersion: v1.14.10-gke.27
  goVersion: go1.12.12b4
  major: "1"
  minor: 14+
  platform: linux/amd64
```

**Other debugging information (if applicable)**:

- workflow result:
![Screen Shot 2020-04-30 at 1 59 01 pm](https://user-images.githubusercontent.com/9264663/80672230-74d2a480-8aef-11ea-9637-795e0504e987.png)

---
<!-- Issue Author: Don't delete this message to encourage other users to support your issue! -->
**Message from the maintainers**:

If you are impacted by this bug please add a 👍 reaction to this issue! We often sort issues this way to know what to prioritize.


Pull request:
fix: Begin counting maxDuration from first child start
Fixes #2901 

MaxDuration now counts from first node start instead of first node finish. It is more reliable and makes more sense given the spec

Also adds a short-circuit feature that would fail a retry if waiting for the current backoff would exceed the max duration, saving the time it would have spent waiting.

Patch:
diff --git a/docs/cluster-workflow-templates.md b/docs/cluster-workflow-templates.md
index f9a329db4..fb036cc21 100644
--- a/docs/cluster-workflow-templates.md
+++ b/docs/cluster-workflow-templates.md
@@ -7,7 +7,7 @@
 `ClusterWorkflowTemplates` are cluster scoped `WorkflowTemplates`. `ClusterWorkflowTemplate` 
 can be created cluster scoped like `ClusterRole` and can be accessed all namespaces in the cluster. 
 
-`WorkflowTemplate` documentation [link](./workflow-template.md)
+`WorkflowTemplates` documentation [link](./workflow-templates.md)
 
 ## Defining `ClusterWorkflowTemplate`
 
diff --git a/docs/variables.md b/docs/variables.md
index 2a0432a2f..15254d20f 100644
--- a/docs/variables.md
+++ b/docs/variables.md
@@ -33,6 +33,7 @@ The following variables are made available to reference various metadata of a wo
 | Variable | Description|
 |----------|------------|
 | `pod.name` | Pod name of the container/script |
+| `retries` | The retry number of the container/script if retryStrategy is specified |
 | `inputs.artifacts.<NAME>.path` | Local path of the input artifact |
 | `outputs.artifacts.<NAME>.path` | Local path of the output artifact |
 | `outputs.parameters.<NAME>.path` | Local path of the output parameter |
diff --git a/examples/retry-with-steps.yaml b/examples/retry-with-steps.yaml
index bdfdbfbc3..a6bfcd4f5 100644
--- a/examples/retry-with-steps.yaml
+++ b/examples/retry-with-steps.yaml
@@ -21,4 +21,4 @@ spec:
       image: python:alpine3.6
       command: [python, -c]
       # fail with a 66% probability
-      args: ["import random; import sys; exit_code = random.choice([0, 1, 1]); sys.exit(exit_code)"]
+      args: ["import random; import sys; print('retries: {{retries}}'); exit_code = random.choice([0, 1, 1]); sys.exit(exit_code)"]
diff --git a/hooks/README.md b/hooks/README.md
old mode 100644
new mode 100755
diff --git a/workflow/common/common.go b/workflow/common/common.go
index 083ddbe83..a8eaeb338 100644
--- a/workflow/common/common.go
+++ b/workflow/common/common.go
@@ -132,6 +132,8 @@ const (
 	GlobalVarWorkflowParameters = "workflow.parameters"
 	// LocalVarPodName is a step level variable that references the name of the pod
 	LocalVarPodName = "pod.name"
+	// LocalVarRetries is a step level variable that references the retries number if retryStrategy is specified
+	LocalVarRetries = "retries"
 
 	KubeConfigDefaultMountPath    = "/kube/config"
 	KubeConfigDefaultVolumeName   = "kubeconfig"
diff --git a/workflow/controller/exec_control.go b/workflow/controller/exec_control.go
index c9f82eea9..387488cb0 100644
--- a/workflow/controller/exec_control.go
+++ b/workflow/controller/exec_control.go
@@ -75,7 +75,7 @@ func (woc *wfOperationCtx) applyExecutionControl(pod *apiv1.Pod, wfNodesLock *sy
 	var newDeadline *time.Time
 	if woc.wf.Spec.Shutdown != "" {
 		_, onExitPod := pod.Labels[common.LabelKeyOnExit]
-		if woc.wf.Spec.Shutdown == wfv1.ShutdownStrategyTerminate || (woc.wf.Spec.Shutdown == wfv1.ShutdownStrategyStop && !onExitPod) {
+		if !woc.wf.Spec.Shutdown.ShouldExecute(onExitPod) {
 			newDeadline = &time.Time{}
 		}
 	} else {
diff --git a/workflow/controller/operator.go b/workflow/controller/operator.go
index ab687ff40..f4c2c71ef 100644
--- a/workflow/controller/operator.go
+++ b/workflow/controller/operator.go
@@ -647,6 +647,7 @@ func (woc *wfOperationCtx) processNodeRetries(node *wfv1.NodeStatus, retryStrate
 		return woc.markNodePhase(node.Name, lastChildNode.Phase, message), true, nil
 	}
 
+	maxDurationDeadline := time.Time{}
 	if retryStrategy.Backoff != nil {
 		// Process max duration limit
 		if retryStrategy.Backoff.MaxDuration != "" && len(node.Children) > 0 {
@@ -658,7 +659,8 @@ func (woc *wfOperationCtx) processNodeRetries(node *wfv1.NodeStatus, retryStrate
 			if err != nil {
 				return nil, false, fmt.Errorf("Failed to find first child of node " + node.Name)
 			}
-			if time.Now().After(firstChildNode.FinishedAt.Add(maxDuration)) {
+			maxDurationDeadline = firstChildNode.StartedAt.Add(maxDuration)
+			if time.Now().After(maxDurationDeadline) {
 				woc.log.Infoln("Max duration limit exceeded. Failing...")
 				return woc.markNodePhase(node.Name, lastChildNode.Phase, "Max duration limit exceeded"), true, nil
 			}
@@ -668,6 +670,7 @@ func (woc *wfOperationCtx) processNodeRetries(node *wfv1.NodeStatus, retryStrate
 		if retryStrategy.Backoff.Duration == "" {
 			return nil, false, fmt.Errorf("no base duration specified for retryStrategy")
 		}
+
 		baseDuration, err := parseStringToDuration(retryStrategy.Backoff.Duration)
 		if err != nil {
 			return nil, false, err
@@ -680,6 +683,12 @@ func (woc *wfOperationCtx) processNodeRetries(node *wfv1.NodeStatus, retryStrate
 		}
 		waitingDeadline := lastChildNode.FinishedAt.Add(timeToWait)
 
+		// If the waiting deadline is after the max duration deadline, then it's futile to wait until then. Stop early
+		if !maxDurationDeadline.IsZero() && waitingDeadline.After(maxDurationDeadline) {
+			woc.log.Infoln("Backoff would exceed max duration limit. Failing...")
+			return woc.markNodePhase(node.Name, lastChildNode.Phase, "Backoff would exceed max duration limit"), true, nil
+		}
+
 		// See if we have waited past the deadline
 		if time.Now().Before(waitingDeadline) {
 			woc.requeue(timeToWait)
@@ -1421,6 +1430,7 @@ func (woc *wfOperationCtx) executeTemplate(nodeName string, orgTmpl wfv1.Templat
 	if resolvedTmpl.IsPodType() && resolvedTmpl.RetryStrategy == nil {
 		localParams[common.LocalVarPodName] = woc.wf.NodeID(nodeName)
 	}
+
 	// Inputs has been processed with arguments already, so pass empty arguments.
 	processedTmpl, err := common.ProcessArgs(resolvedTmpl, &args, woc.globalParams, localParams, false)
 	if err != nil {
@@ -1470,12 +1480,17 @@ func (woc *wfOperationCtx) executeTemplate(nodeName string, orgTmpl wfv1.Templat
 			woc.addChildNode(retryNodeName, nodeName)
 			node = nil
 
+			localParams := make(map[string]string)
 			// Change the `pod.name` variable to the new retry node name
 			if processedTmpl.IsPodType() {
-				processedTmpl, err = common.SubstituteParams(processedTmpl, map[string]string{}, map[string]string{common.LocalVarPodName: woc.wf.NodeID(nodeName)})
-				if err != nil {
-					return woc.initializeNodeOrMarkError(node, nodeName, wfv1.NodeTypeSkipped, templateScope, orgTmpl, opts.boundaryID, err), err
-				}
+				localParams[common.LocalVarPodName] = woc.wf.NodeID(nodeName)
+			}
+			// Inject the retryAttempt number
+			localParams[common.LocalVarRetries] = strconv.Itoa(len(retryParentNode.Children))
+
+			processedTmpl, err = common.SubstituteParams(processedTmpl, map[string]string{}, localParams)
+			if err != nil {
+				return woc.initializeNodeOrMarkError(node, nodeName, wfv1.NodeTypeSkipped, templateScope, orgTmpl, opts.boundaryID, err), err
 			}
 		}
 	}
diff --git a/workflow/validate/validate.go b/workflow/validate/validate.go
index cdc0e7e61..877761f66 100644
--- a/workflow/validate/validate.go
+++ b/workflow/validate/validate.go
@@ -318,6 +318,10 @@ func (ctx *templateValidationCtx) validateTemplate(tmpl *wfv1.Template, tmplCtx
 		localParams[common.LocalVarPodName] = placeholderGenerator.NextPlaceholder()
 		scope[common.LocalVarPodName] = placeholderGenerator.NextPlaceholder()
 	}
+	if tmpl.RetryStrategy != nil {
+		localParams[common.LocalVarRetries] = placeholderGenerator.NextPlaceholder()
+		scope[common.LocalVarRetries] = placeholderGenerator.NextPlaceholder()
+	}
 	if tmpl.IsLeaf() {
 		for _, art := range tmpl.Outputs.Artifacts {
 			if art.Path != "" {


Test patch:
diff --git a/workflow/controller/operator_test.go b/workflow/controller/operator_test.go
index ae2b65452..df75844d4 100644
--- a/workflow/controller/operator_test.go
+++ b/workflow/controller/operator_test.go
@@ -561,6 +561,111 @@ func TestBackoffMessage(t *testing.T) {
 	assert.Equal(t, "", newRetryNode.Message)
 }
 
+var retriesVariableTemplate = `
+apiVersion: argoproj.io/v1alpha1
+kind: Workflow
+metadata:
+  name: whalesay
+spec:
+  entrypoint: whalesay
+  templates:
+  - name: whalesay
+    retryStrategy:
+      limit: 10
+    container:
+      image: docker/whalesay:latest
+      command: [sh, -c]
+      args: ["cowsay {{retries}}"]
+`
+
+func TestRetriesVariable(t *testing.T) {
+	cancel, controller := newController()
+	defer cancel()
+	wfcset := controller.wfclientset.ArgoprojV1alpha1().Workflows("")
+	wf := unmarshalWF(retriesVariableTemplate)
+	wf, err := wfcset.Create(wf)
+	assert.Nil(t, err)
+	wf, err = wfcset.Get(wf.ObjectMeta.Name, metav1.GetOptions{})
+	assert.Nil(t, err)
+
+	iterations := 5
+	for i := 1; i <= iterations; i++ {
+		if i != 1 {
+			makePodsPhase(t, apiv1.PodFailed, controller.kubeclientset, wf.ObjectMeta.Namespace)
+			wf, err = wfcset.Get(wf.ObjectMeta.Name, metav1.GetOptions{})
+			assert.Nil(t, err)
+		}
+		woc := newWorkflowOperationCtx(wf, controller)
+		woc.operate()
+	}
+
+	pods, err := controller.kubeclientset.CoreV1().Pods("").List(metav1.ListOptions{})
+	assert.Nil(t, err)
+	assert.Equal(t, iterations, len(pods.Items))
+	for i := 0; i < iterations; i++ {
+		assert.Equal(t, fmt.Sprintf("cowsay %d", i), pods.Items[i].Spec.Containers[1].Args[0])
+	}
+}
+
+var stepsRetriesVariableTemplate = `
+apiVersion: argoproj.io/v1alpha1
+kind: Workflow
+metadata:
+  name: whalesay
+spec:
+  entrypoint: step-retry
+  templates:
+  - name: step-retry
+    retryStrategy:
+      limit: 10
+    steps:
+      - - name: whalesay-success
+          arguments:
+            parameters:
+            - name: retries
+              value: "{{retries}}"
+          template: whalesay
+
+  - name: whalesay
+    inputs:
+      parameters:
+        - name: retries
+    container:
+      image: docker/whalesay:latest
+      command: [sh, -c]
+      args: ["cowsay {{inputs.parameters.retries}}"]
+`
+
+func TestStepsRetriesVariable(t *testing.T) {
+	cancel, controller := newController()
+	defer cancel()
+	wfcset := controller.wfclientset.ArgoprojV1alpha1().Workflows("")
+	wf := unmarshalWF(stepsRetriesVariableTemplate)
+	wf, err := wfcset.Create(wf)
+	assert.Nil(t, err)
+	wf, err = wfcset.Get(wf.ObjectMeta.Name, metav1.GetOptions{})
+	assert.Nil(t, err)
+
+	iterations := 5
+	for i := 1; i <= iterations; i++ {
+		if i != 1 {
+			makePodsPhase(t, apiv1.PodFailed, controller.kubeclientset, wf.ObjectMeta.Namespace)
+			wf, err = wfcset.Get(wf.ObjectMeta.Name, metav1.GetOptions{})
+			assert.Nil(t, err)
+		}
+		// move to next retry step
+		woc := newWorkflowOperationCtx(wf, controller)
+		woc.operate()
+	}
+
+	pods, err := controller.kubeclientset.CoreV1().Pods("").List(metav1.ListOptions{})
+	assert.Nil(t, err)
+	assert.Equal(t, iterations, len(pods.Items))
+	for i := 0; i < iterations; i++ {
+		assert.Equal(t, fmt.Sprintf("cowsay %d", i), pods.Items[i].Spec.Containers[1].Args[0])
+	}
+}
+
 func TestAssessNodeStatus(t *testing.T) {
 	daemoned := true
 	tests := []struct {
@@ -2686,7 +2791,7 @@ metadata:
   name: artifact-repo-config-ref
 spec:
   entrypoint: whalesay
-  poddisruptionbudget: 
+  poddisruptionbudget:
     minavailable: 100%
   templates:
   - name: whalesay
@@ -3194,3 +3299,184 @@ func TestResolvePlaceholdersInGlobalVariables(t *testing.T) {
 	assert.NotEmpty(t, *serviceAccountNameValue)
 	assert.Equal(t, "testServiceAccountName", *serviceAccountNameValue)
 }
+
+var maxDurationOnErroredFirstNode = `
+apiVersion: argoproj.io/v1alpha1
+kind: Workflow
+metadata:
+  creationTimestamp: "2020-05-07T17:40:57Z"
+  generateName: echo-
+  generation: 4
+  labels:
+    workflows.argoproj.io/phase: Running
+  name: echo-wngc4
+  namespace: argo
+  resourceVersion: "6339"
+  selfLink: /apis/argoproj.io/v1alpha1/namespaces/argo/workflows/echo-wngc4
+  uid: bed2749b-2971-4172-a61e-455ef02c4379
+spec:
+  arguments: {}
+  entrypoint: echo
+  templates:
+  - arguments: {}
+    container:
+      args:
+      - sleep 10 && exit 1
+      command:
+      - sh
+      - -c
+      image: alpine:3.7
+      name: ""
+      resources: {}
+    inputs: {}
+    metadata: {}
+    name: echo
+    outputs: {}
+    retryStrategy:
+      retryPolicy: "Always"
+      backoff:
+        duration: "10"
+        factor: 1
+        maxDuration: 20m
+      limit: 4
+status:
+  finishedAt: null
+  nodes:
+    echo-wngc4:
+      children:
+      - echo-wngc4-1641470511
+      displayName: echo-wngc4
+      finishedAt: null
+      id: echo-wngc4
+      name: echo-wngc4
+      phase: Running
+      startedAt: "2020-05-07T17:40:57Z"
+      templateName: echo
+      templateScope: local/echo-wngc4
+      type: Retry
+    echo-wngc4-1641470511:
+      displayName: echo-wngc4(0)
+      finishedAt: null
+      hostNodeName: minikube
+      id: echo-wngc4-1641470511
+      name: echo-wngc4(0)
+      phase: Error
+      startedAt: "2020-05-07T17:40:57Z"
+      templateName: echo
+      templateScope: local/echo-wngc4
+      type: Pod
+  phase: Running
+  startedAt: "2020-05-07T17:40:57Z"
+`
+
+// This tests that retryStrategy.backoff.maxDuration works correctly even if the first child node was deleted without a
+// proper finishedTime tag.
+func TestMaxDurationOnErroredFirstNode(t *testing.T) {
+	wf := unmarshalWF(maxDurationOnErroredFirstNode)
+
+	// Simulate node failed just now
+	node := wf.Status.Nodes["echo-wngc4-1641470511"]
+	node.StartedAt = metav1.Time{Time: time.Now().Add(-1 * time.Second)}
+	wf.Status.Nodes["echo-wngc4-1641470511"] = node
+
+	woc := newWoc(*wf)
+	woc.operate()
+	assert.Equal(t, wfv1.NodeRunning, woc.wf.Status.Phase)
+}
+
+var backoffExceedsMaxDuration = `
+apiVersion: argoproj.io/v1alpha1
+kind: Workflow
+metadata:
+  name: echo-r6v49
+spec:
+  arguments: {}
+  entrypoint: echo
+  templates:
+  - arguments: {}
+    container:
+      args:
+      - exit 1
+      command:
+      - sh
+      - -c
+      image: alpine:3.7
+      name: ""
+      resources: {}
+    inputs: {}
+    metadata: {}
+    name: echo
+    outputs: {}
+    retryStrategy:
+      backoff:
+        duration: "120"
+        factor: 1
+        maxDuration: "60"
+      limit: 4
+status:
+  nodes:
+    echo-r6v49:
+      children:
+      - echo-r6v49-3721138751
+      displayName: echo-r6v49
+      id: echo-r6v49
+      name: echo-r6v49
+      phase: Running
+      startedAt: "2020-05-07T18:10:34Z"
+      templateName: echo
+      templateScope: local/echo-r6v49
+      type: Retry
+    echo-r6v49-3721138751:
+      displayName: echo-r6v49(0)
+      finishedAt: "2020-05-07T18:10:35Z"
+      hostNodeName: minikube
+      id: echo-r6v49-3721138751
+      message: failed with exit code 1
+      name: echo-r6v49(0)
+      outputs:
+        artifacts:
+        - archiveLogs: true
+          name: main-logs
+          s3:
+            accessKeySecret:
+              key: accesskey
+              name: my-minio-cred
+            bucket: my-bucket
+            endpoint: minio:9000
+            insecure: true
+            key: echo-r6v49/echo-r6v49-3721138751/main.log
+            secretKeySecret:
+              key: secretkey
+              name: my-minio-cred
+        exitCode: "1"
+      phase: Failed
+      resourcesDuration:
+        cpu: 1
+        memory: 0
+      startedAt: "2020-05-07T18:10:34Z"
+      templateName: echo
+      templateScope: local/echo-r6v49
+      type: Pod
+  phase: Running
+  resourcesDuration:
+    cpu: 1
+    memory: 0
+  startedAt: "2020-05-07T18:10:34Z"
+`
+
+// This tests that we don't wait a backoff if it would exceed the maxDuration anyway.
+func TestBackoffExceedsMaxDuration(t *testing.T) {
+	wf := unmarshalWF(backoffExceedsMaxDuration)
+
+	// Simulate node failed just now
+	node := wf.Status.Nodes["echo-r6v49-3721138751"]
+	node.StartedAt = metav1.Time{Time: time.Now().Add(-1 * time.Second)}
+	node.FinishedAt = metav1.Time{Time: time.Now()}
+	wf.Status.Nodes["echo-r6v49-3721138751"] = node
+
+	woc := newWoc(*wf)
+	woc.operate()
+	assert.Equal(t, wfv1.NodeFailed, woc.wf.Status.Phase)
+	assert.Equal(t, "Backoff would exceed max duration limit", woc.wf.Status.Nodes["echo-r6v49"].Message)
+	assert.Equal(t, "Backoff would exceed max duration limit", woc.wf.Status.Message)
+}