Repository: argoproj/argo (go)

Issue:
Duplicated events for Workflow Node Event
Checklist:

 * [x] I've included the version.
 * [x] I've included reproduction steps.
 * [x] I've included the workflow YAML.
 * [x] I've included the logs.
 
**What happened**:

Duplicated Workflow Node Events are emitted for task/step while running workflow.
The event duplication happens on both Steps and DAG.

https://github.com/argoproj/argo/blob/master/docs/workflow-events.md

**What you expected to happen**:

Workflow Node Events should be emitted only once for each task/step completion.

**How to reproduce it (as minimally and precisely as possible)**:

Can be reproduced by running examples with multiple tasks/steps.

https://github.com/argoproj/argo/blob/master/examples/dag-diamond.yaml
https://github.com/argoproj/argo/blob/master/examples/steps.yaml

**Anything else we need to know?**:

- Last task/step in DAG/Steps has no event duplication.
- The difference between duplicated events seems to be only resourceVersion of the involvedObject (=Workflow).
- Argo CLI hasn't been used. (Only kubectl command)

**Environment**:

- Argo version: 2.8.0-rc3
```
$ argo version
```
- Kubernetes version :  v1.17.4-gke.10
```
$ kubectl version -o yaml
```

**Other debugging information (if applicable)**:

None.

**Logs**

Following logs are event logs from `kubectl describe wf`.

DAG with Task A only: No event duplication
```
Events:
  Type    Reason                 Age        From               Message
  ----    ------                 ----       ----               -------
  Normal  WorkflowRunning        <invalid>  dag-diamond-dzkhc  Workflow Running
  Normal  WorkflowNodeSucceeded  <invalid>  dag-diamond-dzkhc  Succeeded node dag-diamond-dzkhc.A
  Normal  WorkflowNodeSucceeded  <invalid>  dag-diamond-dzkhc  Succeeded node dag-diamond-dzkhc
  Normal  WorkflowSucceeded      <invalid>  dag-diamond-dzkhc  Workflow completed
```

DAG with Task A,B: Event duplication for Task A.
```
Events:
  Type    Reason                 Age        From               Message
  ----    ------                 ----       ----               -------
  Normal  WorkflowRunning        <invalid>  dag-diamond-jgxfq  Workflow Running
  Normal  WorkflowNodeSucceeded  <invalid>  dag-diamond-jgxfq  Succeeded node dag-diamond-jgxfq.A
  Normal  WorkflowNodeSucceeded  <invalid>  dag-diamond-jgxfq  Succeeded node dag-diamond-jgxfq.A
  Normal  WorkflowNodeSucceeded  <invalid>  dag-diamond-jgxfq  Succeeded node dag-diamond-jgxfq.B
  Normal  WorkflowNodeSucceeded  <invalid>  dag-diamond-jgxfq  Succeeded node dag-diamond-jgxfq
  Normal  WorkflowSucceeded      <invalid>  dag-diamond-jgxfq  Workflow completed
```

DAG with Task A,B,C,D: Event duplication for Task A,B,C
```
Events:
  Type    Reason                 Age        From               Message
  ----    ------                 ----       ----               -------
  Normal  WorkflowRunning        <invalid>  dag-diamond-jzt9b  Workflow Running
  Normal  WorkflowNodeSucceeded  <invalid>  dag-diamond-jzt9b  Succeeded node dag-diamond-jzt9b.A
  Normal  WorkflowNodeSucceeded  <invalid>  dag-diamond-jzt9b  Succeeded node dag-diamond-jzt9b.A
  Normal  WorkflowNodeSucceeded  <invalid>  dag-diamond-jzt9b  Succeeded node dag-diamond-jzt9b.B
  Normal  WorkflowNodeSucceeded  <invalid>  dag-diamond-jzt9b  Succeeded node dag-diamond-jzt9b.C
  Normal  WorkflowNodeSucceeded  <invalid>  dag-diamond-jzt9b  Succeeded node dag-diamond-jzt9b.C
  Normal  WorkflowNodeSucceeded  <invalid>  dag-diamond-jzt9b  Succeeded node dag-diamond-jzt9b.D
  Normal  WorkflowNodeSucceeded  <invalid>  dag-diamond-jzt9b  Succeeded node dag-diamond-jzt9b
  Normal  WorkflowSucceeded      <invalid>  dag-diamond-jzt9b  Workflow completed

```

---
<!-- 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: Remove duplicate node event. Fixes #2961
Checklist:

* [x] Either (a) I've created an [enhancement proposal](https://github.com/argoproj/argo/issues/new/choose) and discussed it with the community, (b) this is a bug fix, or (c) this is a chore.
* [x] The title of the PR is (a) [conventional](https://www.conventionalcommits.org/en/v1.0.0/), (b) states what changed, and (c) suffixes the related issues number. E.g. `"fix(controller): Updates such and such. Fixes #1234"`.  
* [x] I've signed the CLA.
* [x] I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
* [x] My builds are green. Try syncing with master if they are not. 
* [x] My organization is added to USERS.md.

Fixes #2961

Patch:
diff --git a/Makefile b/Makefile
index bd5d9848b..a6b38858a 100644
--- a/Makefile
+++ b/Makefile
@@ -12,8 +12,11 @@ GIT_TREE_STATE         = $(shell if [ -z "`git status --porcelain`" ]; then echo
 
 export DOCKER_BUILDKIT = 1
 
-# To allow you to build with cache for debugging purposes.
+# To allow you to build with or without cache for debugging purposes.
 DOCKER_BUILD_OPTS     := --no-cache
+# Use a different Dockerfile, e.g. for building for Windows or dev images.
+DOCKERFILE            := Dockerfile
+
 
 # docker image publishing options
 IMAGE_NAMESPACE       ?= argoproj
@@ -58,6 +61,12 @@ DEV_IMAGE             := true
 endif
 endif
 
+# If we are building dev images, then we want to use the Docker cache for speed.
+ifeq ($(DEV_IMAGE),true)
+DOCKER_BUILD_OPTS     :=
+DOCKERFILE            := Dockerfile.dev
+endif
+
 # version change, so does the file location
 MANIFESTS_VERSION_FILE := dist/$(MANIFESTS_VERSION).manifests-version
 VERSION_FILE           := dist/$(VERSION).version
@@ -114,6 +123,15 @@ define restore_go_mod
 	# Restore the back-ups.
 	mv dist/go.mod dist/go.sum .
 endef
+# docker_build,image_name,binary_name,marker_file_name
+define docker_build
+	# If we're making a dev build, we build this locally (this will be faster due to existing Go build caches).
+	if [ $(DEV_IMAGE) = true ]; then $(MAKE) dist/$(2)-$(OUTPUT_IMAGE_OS)-$(OUTPUT_IMAGE_ARCH) && mv dist/$(2)-$(OUTPUT_IMAGE_OS)-$(OUTPUT_IMAGE_ARCH) $(2); fi
+	docker build $(DOCKER_BUILD_OPTS) -t $(IMAGE_NAMESPACE)/$(1):$(VERSION) --target $(1) -f $(DOCKERFILE) --build-arg IMAGE_OS=$(OUTPUT_IMAGE_OS) --build-arg IMAGE_ARCH=$(OUTPUT_IMAGE_ARCH) .
+	if [ $(DEV_IMAGE) = true ]; then mv $(2) dist/$(2)-$(OUTPUT_IMAGE_OS)-$(OUTPUT_IMAGE_ARCH); fi
+	if [ $(K3D) = true ]; then k3d import-images $(IMAGE_NAMESPACE)/$(1):$(VERSION); fi
+	touch $(3)
+endef
 
 .PHONY: build
 build: status clis executor-image controller-image manifests/install.yaml manifests/namespace-install.yaml manifests/quick-start-postgres.yaml manifests/quick-start-mysql.yaml
@@ -123,7 +141,7 @@ build: status clis executor-image controller-image manifests/install.yaml manife
 
 .PHONY: status
 status:
-	# GIT_TAG=$(GIT_TAG), GIT_BRANCH=$(GIT_BRANCH), GIT_TREE_STATE=$(GIT_TREE_STATE), MANIFESTS_VERSION=$(MANIFESTS_VERSION), VERSION=$(VERSION), DEV_IMAGE=$(DEV_IMAGE)
+	# GIT_TAG=$(GIT_TAG), GIT_BRANCH=$(GIT_BRANCH), GIT_TREE_STATE=$(GIT_TREE_STATE), MANIFESTS_VERSION=$(MANIFESTS_VERSION), VERSION=$(VERSION), DEV_IMAGE=$(DEV_IMAGE), K3D=$(K3D)
 
 # cli
 
@@ -178,8 +196,7 @@ argo-server.key:
 cli-image: $(CLI_IMAGE_FILE)
 
 $(CLI_IMAGE_FILE):
-	docker build $(DOCKER_BUILD_OPTS) -t $(IMAGE_NAMESPACE)/argocli:$(VERSION) --target argocli --build-arg IMAGE_OS=$(OUTPUT_IMAGE_OS) --build-arg IMAGE_ARCH=$(OUTPUT_IMAGE_ARCH) .
-	touch $(CLI_IMAGE_FILE)
+	$(call docker_build,argocli,argo,$(CLI_IMAGE_FILE))
 
 .PHONY: clis
 clis: dist/argo-linux-amd64 dist/argo-linux-arm64 dist/argo-linux-ppc64le dist/argo-linux-s390x dist/argo-darwin-amd64 dist/argo-windows-amd64 cli-image
@@ -201,8 +218,7 @@ dist/workflow-controller-%: $(CONTROLLER_PKGS)
 controller-image: $(CONTROLLER_IMAGE_FILE)
 
 $(CONTROLLER_IMAGE_FILE):
-	docker build $(DOCKER_BUILD_OPTS) -t $(IMAGE_NAMESPACE)/workflow-controller:$(VERSION) --target workflow-controller --build-arg IMAGE_OS=$(OUTPUT_IMAGE_OS) --build-arg IMAGE_ARCH=$(OUTPUT_IMAGE_ARCH) .
-	touch $(CONTROLLER_IMAGE_FILE)
+	$(call docker_build,workflow-controller,workflow-controller,$(CONTROLLER_IMAGE_FILE))
 
 # argoexec
 
@@ -216,19 +232,9 @@ dist/argoexec-%: $(ARGOEXEC_PKGS)
 .PHONY: executor-image
 executor-image: $(EXECUTOR_IMAGE_FILE)
 
-$(EXECUTOR_IMAGE_FILE): dist/argoexec-$(OUTPUT_IMAGE_OS)-$(OUTPUT_IMAGE_ARCH)
 	# Create executor image
-ifeq ($(DEV_IMAGE),true)
-	mv dist/argoexec-$(OUTPUT_IMAGE_OS)-$(OUTPUT_IMAGE_ARCH) argoexec
-	docker build -t $(IMAGE_NAMESPACE)/argoexec:$(VERSION) --target argoexec -f Dockerfile.dev --build-arg IMAGE_OS=$(OUTPUT_IMAGE_OS) --build-arg IMAGE_ARCH=$(OUTPUT_IMAGE_ARCH) .
-	mv argoexec dist/argoexec-$(OUTPUT_IMAGE_OS)-$(OUTPUT_IMAGE_ARCH)
-else
-	docker build $(DOCKER_BUILD_OPTS) -t $(IMAGE_NAMESPACE)/argoexec:$(VERSION) --target argoexec --build-arg IMAGE_OS=$(OUTPUT_IMAGE_OS) --build-arg IMAGE_ARCH=$(OUTPUT_IMAGE_ARCH) .
-endif
-ifeq ($(K3D),true)
-	k3d import-images $(IMAGE_NAMESPACE)/argoexec:$(VERSION)
-endif
-	touch $(EXECUTOR_IMAGE_FILE)
+$(EXECUTOR_IMAGE_FILE):
+	$(call docker_build,argoexec,argoexec,$(EXECUTOR_IMAGE_FILE))
 
 # generation
 
diff --git a/pkg/apis/workflow/v1alpha1/workflow_types.go b/pkg/apis/workflow/v1alpha1/workflow_types.go
index 34a895d82..ebf5ac5f9 100644
--- a/pkg/apis/workflow/v1alpha1/workflow_types.go
+++ b/pkg/apis/workflow/v1alpha1/workflow_types.go
@@ -277,6 +277,17 @@ const (
 	ShutdownStrategyStop      ShutdownStrategy = "Stop"
 )
 
+func (s ShutdownStrategy) ShouldExecute(isOnExitPod bool) bool {
+	switch s {
+	case ShutdownStrategyTerminate:
+		return false
+	case ShutdownStrategyStop:
+		return isOnExitPod
+	default:
+		return true
+	}
+}
+
 type ParallelSteps struct {
 	Steps []WorkflowStep `protobuf:"bytes,1,rep,name=steps"`
 }
diff --git a/workflow/controller/dag.go b/workflow/controller/dag.go
index 9c8f1f561..1ac51aea2 100644
--- a/workflow/controller/dag.go
+++ b/workflow/controller/dag.go
@@ -138,7 +138,9 @@ func (d *dagContext) assessDAGPhase(targetTasks []string, nodes map[string]wfv1.
 		if unsuccessfulPhase == "" && !(isRetryAttempt(node, nodes) || d.getTaskFromNode(&node).ContinuesOn(node.Phase)) {
 			unsuccessfulPhase = node.Phase
 		}
-		if node.Type == wfv1.NodeTypeRetry && d.hasMoreRetries(&node) {
+		// If the node is a Retry node and has more retry attempts and is not shutting down, do not fail the task as a whole
+		// and allow the remaining retries to be executed
+		if node.Type == wfv1.NodeTypeRetry && d.hasMoreRetries(&node) && d.wf.Spec.Shutdown.ShouldExecute(d.onExitTemplate) {
 			retriesExhausted = false
 		}
 	}
diff --git a/workflow/controller/exec_control.go b/workflow/controller/exec_control.go
index 2f7cccb03..c9f82eea9 100644
--- a/workflow/controller/exec_control.go
+++ b/workflow/controller/exec_control.go
@@ -29,7 +29,7 @@ func (woc *wfOperationCtx) applyExecutionControl(pod *apiv1.Pod, wfNodesLock *sy
 		if woc.wf.Spec.Shutdown != "" {
 			// Only delete pods that are not part of an onExit handler if we are "Stopping" or all pods if we are "Terminating"
 			_, 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) {
 				woc.log.Infof("Deleting Pending pod %s/%s as part of workflow shutdown with strategy: %s", pod.Namespace, pod.Name, woc.wf.Spec.Shutdown)
 				err := woc.controller.kubeclientset.CoreV1().Pods(pod.Namespace).Delete(pod.Name, &metav1.DeleteOptions{})
 				if err == nil {
@@ -74,8 +74,10 @@ func (woc *wfOperationCtx) applyExecutionControl(pod *apiv1.Pod, wfNodesLock *sy
 
 	var newDeadline *time.Time
 	if woc.wf.Spec.Shutdown != "" {
-		// Signal termination by setting a Zero deadline
-		newDeadline = &time.Time{}
+		_, onExitPod := pod.Labels[common.LabelKeyOnExit]
+		if woc.wf.Spec.Shutdown == wfv1.ShutdownStrategyTerminate || (woc.wf.Spec.Shutdown == wfv1.ShutdownStrategyStop && !onExitPod) {
+			newDeadline = &time.Time{}
+		}
 	} else {
 		if podExecCtl.Deadline == nil && woc.workflowDeadline == nil {
 			return nil
diff --git a/workflow/controller/operator.go b/workflow/controller/operator.go
index 85f9f66fd..ab687ff40 100644
--- a/workflow/controller/operator.go
+++ b/workflow/controller/operator.go
@@ -300,7 +300,7 @@ func (woc *wfOperationCtx) operate() {
 
 	workflowStatus := node.Phase
 	var onExitNode *wfv1.NodeStatus
-	if woc.wf.Spec.OnExit != "" && woc.wf.Spec.Shutdown != wfv1.ShutdownStrategyTerminate {
+	if woc.wf.Spec.OnExit != "" && woc.wf.Spec.Shutdown.ShouldExecute(true) {
 		if workflowStatus == wfv1.NodeSkipped {
 			// treat skipped the same as Succeeded for workflow.status
 			woc.globalParams[common.GlobalVarWorkflowStatus] = string(wfv1.NodeSucceeded)
@@ -766,7 +766,9 @@ func (woc *wfOperationCtx) podReconciliation() error {
 				if woc.shouldPrintPodSpec(node) {
 					printPodSpecLog(pod, woc.wf.Name)
 				}
-				woc.onNodeComplete(&node)
+				if !woc.orig.Status.Nodes[node.ID].Completed() {
+					woc.onNodeComplete(&node)
+				}
 			}
 			if node.Successful() {
 				woc.succeededPods[pod.ObjectMeta.Name] = true
@@ -826,13 +828,16 @@ func (woc *wfOperationCtx) podReconciliation() error {
 			node.Phase = wfv1.NodeError
 			woc.wf.Status.Nodes[nodeID] = node
 			woc.log.Warnf("pod %s deleted", nodeID)
+			woc.updated = true
 		} else {
 			// At this point we are certain that the pod associated with our node is running or has been run;
 			// it is safe to extract the k8s-node information given this knowledge.
-			node.HostNodeName = seenPods[nodeID].Spec.NodeName
-			woc.wf.Status.Nodes[nodeID] = node
+			if node.HostNodeName != seenPods[nodeID].Spec.NodeName {
+				node.HostNodeName = seenPods[nodeID].Spec.NodeName
+				woc.wf.Status.Nodes[nodeID] = node
+				woc.updated = true
+			}
 		}
-		woc.updated = true
 	}
 	return nil
 }
@@ -957,8 +962,8 @@ func (woc *wfOperationCtx) assessNodeStatus(pod *apiv1.Pod, node *wfv1.NodeStatu
 	case apiv1.PodRunning:
 		if pod.DeletionTimestamp != nil {
 			// pod is being terminated
-			newPhase = wfv1.NodeFailed
-			message = "pod termination"
+			newPhase = wfv1.NodeError
+			message = "pod deleted during operation"
 		} else {
 			newPhase = wfv1.NodeRunning
 			tmplStr, ok := pod.Annotations[common.AnnotationKeyTemplate]
@@ -1718,7 +1723,7 @@ func (woc *wfOperationCtx) markNodePhase(nodeName string, phase wfv1.NodePhase,
 		woc.log.Infof("node %s finished: %s", node.ID, node.FinishedAt)
 		woc.updated = true
 	}
-	if woc.updated && node.Completed() {
+	if !woc.orig.Status.Nodes[node.ID].Completed() && node.Completed() {
 		woc.onNodeComplete(node)
 	}
 	woc.wf.Status.Nodes[node.ID] = *node
@@ -2416,7 +2421,7 @@ func (woc *wfOperationCtx) createTemplateContext(scope wfv1.ResourceScope, resou
 }
 
 func (woc *wfOperationCtx) runOnExitNode(parentName, templateRef, boundaryID string, tmplCtx *templateresolution.Context) (bool, *wfv1.NodeStatus, error) {
-	if templateRef != "" && woc.wf.Spec.Shutdown != wfv1.ShutdownStrategyTerminate {
+	if templateRef != "" && woc.wf.Spec.Shutdown.ShouldExecute(true) {
 		woc.log.Infof("Running OnExit handler: %s", templateRef)
 		onExitNodeName := parentName + ".onExit"
 		onExitNode, err := woc.executeTemplate(onExitNodeName, &wfv1.WorkflowStep{Template: templateRef}, tmplCtx, woc.wf.Spec.Arguments, &executeTemplateOpts{


Test patch:
diff --git a/pkg/apis/workflow/v1alpha1/workflow_types_test.go b/pkg/apis/workflow/v1alpha1/workflow_types_test.go
index 047468e6d..6a30e23a3 100644
--- a/pkg/apis/workflow/v1alpha1/workflow_types_test.go
+++ b/pkg/apis/workflow/v1alpha1/workflow_types_test.go
@@ -63,3 +63,10 @@ func TestWorkflowConditions_UpsertConditionMessage(t *testing.T) {
 	wfCond.UpsertConditionMessage(WorkflowCondition{Type: WorkflowConditionCompleted, Message: "world!"})
 	assert.Equal(t, "Hello, world!", wfCond[0].Message)
 }
+
+func TestShutdownStrategy_ShouldExecute(t *testing.T) {
+	assert.False(t, ShutdownStrategyTerminate.ShouldExecute(true))
+	assert.False(t, ShutdownStrategyTerminate.ShouldExecute(false))
+	assert.False(t, ShutdownStrategyStop.ShouldExecute(false))
+	assert.True(t, ShutdownStrategyStop.ShouldExecute(true))
+}
diff --git a/test/e2e/functional_test.go b/test/e2e/functional_test.go
index ee156704a..cc1e89e0e 100644
--- a/test/e2e/functional_test.go
+++ b/test/e2e/functional_test.go
@@ -163,11 +163,11 @@ func (s *FunctionalSuite) TestFastFailOnPodTermination() {
 		WaitForWorkflow(120 * time.Second).
 		Then().
 		ExpectWorkflow(func(t *testing.T, _ *metav1.ObjectMeta, status *wfv1.WorkflowStatus) {
-			assert.Equal(t, wfv1.NodeFailed, status.Phase)
+			assert.Equal(t, wfv1.NodeError, status.Phase)
 			assert.Len(t, status.Nodes, 4)
 			nodeStatus := status.Nodes.FindByDisplayName("sleep")
-			assert.Equal(t, wfv1.NodeFailed, nodeStatus.Phase)
-			assert.Equal(t, "pod termination", nodeStatus.Message)
+			assert.Equal(t, wfv1.NodeError, nodeStatus.Phase)
+			assert.Equal(t, "pod deleted during operation", nodeStatus.Message)
 		})
 }
 
@@ -397,7 +397,6 @@ func (s *FunctionalSuite) TestGlobalScope() {
 }
 
 func (s *FunctionalSuite) TestStopBehavior() {
-	s.T().SkipNow()
 	s.Given().
 		Workflow("@functional/stop-terminate.yaml").
 		When().
diff --git a/workflow/controller/dag_test.go b/workflow/controller/dag_test.go
index edc392a43..5447c9216 100644
--- a/workflow/controller/dag_test.go
+++ b/workflow/controller/dag_test.go
@@ -653,3 +653,240 @@ func TestDAGWithParamAndGlobalParam(t *testing.T) {
 	woc.operate()
 	assert.Equal(t, wfv1.NodeRunning, woc.wf.Status.Phase)
 }
+
+var terminatingDAGWithRetryStrategyNodes = `
+apiVersion: argoproj.io/v1alpha1
+kind: Workflow
+metadata:
+  name: dag-diamond-xfww2
+spec:
+  arguments: {}
+  entrypoint: diamond
+  shutdown: Terminate
+  templates:
+  - arguments: {}
+    dag:
+      tasks:
+      - arguments: {}
+        name: A
+        template: echo
+      - arguments: {}
+        dependencies:
+        - A
+        name: B
+        template: echo
+      - arguments: {}
+        dependencies:
+        - A
+        name: C
+        template: echo
+      - arguments: {}
+        dependencies:
+        - B
+        - C
+        name: D
+        template: echo
+    inputs: {}
+    metadata: {}
+    name: diamond
+    outputs: {}
+  - arguments: {}
+    container:
+      args:
+      - sleep 10
+      command:
+      - sh
+      - -c
+      image: alpine:3.7
+      name: ""
+      resources: {}
+    inputs: {}
+    metadata: {}
+    name: echo
+    outputs: {}
+    retryStrategy:
+      limit: 4
+status:
+  finishedAt: null
+  nodes:
+    dag-diamond-xfww2:
+      children:
+      - dag-diamond-xfww2-1488588956
+      displayName: dag-diamond-xfww2
+      finishedAt: null
+      id: dag-diamond-xfww2
+      name: dag-diamond-xfww2
+      phase: Running
+      startedAt: "2020-05-06T16:15:38Z"
+      templateName: diamond
+      templateScope: local/dag-diamond-xfww2
+      type: DAG
+    dag-diamond-xfww2-990947287:
+      boundaryID: dag-diamond-xfww2
+      children:
+      - dag-diamond-xfww2-1522144194
+      - dag-diamond-xfww2-1538921813
+      displayName: A(0)
+      finishedAt: "2020-05-06T16:15:50Z"
+      hostNodeName: minikube
+      id: dag-diamond-xfww2-990947287
+      name: dag-diamond-xfww2.A(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: dag-diamond-xfww2/dag-diamond-xfww2-990947287/main.log
+            secretKeySecret:
+              key: secretkey
+              name: my-minio-cred
+      phase: Succeeded
+      resourcesDuration:
+        cpu: 21
+        memory: 0
+      startedAt: "2020-05-06T16:15:38Z"
+      templateName: echo
+      templateScope: local/dag-diamond-xfww2
+      type: Pod
+    dag-diamond-xfww2-1488588956:
+      boundaryID: dag-diamond-xfww2
+      children:
+      - dag-diamond-xfww2-990947287
+      displayName: A
+      finishedAt: "2020-05-06T16:15:51Z"
+      id: dag-diamond-xfww2-1488588956
+      name: dag-diamond-xfww2.A
+      outputs:
+        artifacts:
+        - archiveLogs: true
+          name: main-logs
+          s3:
+            accessKeySecret:
+              key: accesskey
+              name: my-minio-cred
+            bucket: my-bucket
+            endpoint: minio:9000
+            insecure: true
+            key: dag-diamond-xfww2/dag-diamond-xfww2-990947287/main.log
+            secretKeySecret:
+              key: secretkey
+              name: my-minio-cred
+      phase: Succeeded
+      startedAt: "2020-05-06T16:15:38Z"
+      templateName: echo
+      templateScope: local/dag-diamond-xfww2
+      type: Retry
+    dag-diamond-xfww2-1522144194:
+      boundaryID: dag-diamond-xfww2
+      children:
+      - dag-diamond-xfww2-2043927737
+      displayName: C
+      finishedAt: "2020-05-06T16:15:59Z"
+      id: dag-diamond-xfww2-1522144194
+      message: Stopped with strategy 'Terminate'
+      name: dag-diamond-xfww2.C
+      phase: Failed
+      startedAt: "2020-05-06T16:15:51Z"
+      templateName: echo
+      templateScope: local/dag-diamond-xfww2
+      type: Retry
+    dag-diamond-xfww2-1538921813:
+      boundaryID: dag-diamond-xfww2
+      children:
+      - dag-diamond-xfww2-3629114292
+      displayName: B
+      finishedAt: "2020-05-06T16:15:59Z"
+      id: dag-diamond-xfww2-1538921813
+      message: Stopped with strategy 'Terminate'
+      name: dag-diamond-xfww2.B
+      phase: Failed
+      startedAt: "2020-05-06T16:15:52Z"
+      templateName: echo
+      templateScope: local/dag-diamond-xfww2
+      type: Retry
+    dag-diamond-xfww2-2043927737:
+      boundaryID: dag-diamond-xfww2
+      displayName: C(0)
+      finishedAt: "2020-05-06T16:15:58Z"
+      hostNodeName: minikube
+      id: dag-diamond-xfww2-2043927737
+      message: terminated
+      name: dag-diamond-xfww2.C(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: dag-diamond-xfww2/dag-diamond-xfww2-2043927737/main.log
+            secretKeySecret:
+              key: secretkey
+              name: my-minio-cred
+      phase: Failed
+      resourcesDuration:
+        cpu: 11
+        memory: 0
+      startedAt: "2020-05-06T16:15:51Z"
+      templateName: echo
+      templateScope: local/dag-diamond-xfww2
+      type: Pod
+    dag-diamond-xfww2-3629114292:
+      boundaryID: dag-diamond-xfww2
+      displayName: B(0)
+      finishedAt: "2020-05-06T16:15:58Z"
+      hostNodeName: minikube
+      id: dag-diamond-xfww2-3629114292
+      message: terminated
+      name: dag-diamond-xfww2.B(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: dag-diamond-xfww2/dag-diamond-xfww2-3629114292/main.log
+            secretKeySecret:
+              key: secretkey
+              name: my-minio-cred
+      phase: Failed
+      resourcesDuration:
+        cpu: 9
+        memory: 0
+      startedAt: "2020-05-06T16:15:52Z"
+      templateName: echo
+      templateScope: local/dag-diamond-xfww2
+      type: Pod
+  phase: Running
+  startedAt: "2020-05-06T16:15:38Z"
+`
+
+// This tests that a DAG with retry strategy in its tasks fails successfully when terminated
+func TestTerminatingDAGWithRetryStrategyNodes(t *testing.T) {
+	cancel, controller := newController()
+	defer cancel()
+	wfcset := controller.wfclientset.ArgoprojV1alpha1().Workflows("")
+
+	wf := unmarshalWF(terminatingDAGWithRetryStrategyNodes)
+	wf, err := wfcset.Create(wf)
+	assert.NoError(t, err)
+	woc := newWorkflowOperationCtx(wf, controller)
+
+	woc.operate()
+	assert.Equal(t, wfv1.NodeFailed, woc.wf.Status.Phase)
+}
diff --git a/workflow/controller/operator_test.go b/workflow/controller/operator_test.go
index 0fb2e1a28..ae2b65452 100644
--- a/workflow/controller/operator_test.go
+++ b/workflow/controller/operator_test.go
@@ -606,7 +606,7 @@ func TestAssessNodeStatus(t *testing.T) {
 		node: &wfv1.NodeStatus{},
 		want: wfv1.NodeFailed,
 	}, {
-		name: "pod termination",
+		name: "pod deleted during operation",
 		pod: &apiv1.Pod{
 			ObjectMeta: metav1.ObjectMeta{DeletionTimestamp: &metav1.Time{Time: time.Now()}},
 			Status: apiv1.PodStatus{
@@ -614,7 +614,7 @@ func TestAssessNodeStatus(t *testing.T) {
 			},
 		},
 		node: &wfv1.NodeStatus{},
-		want: wfv1.NodeFailed,
+		want: wfv1.NodeError,
 	}, {
 		name: "pod running",
 		pod: &apiv1.Pod{