Repository: argoproj/argo (go)

Issue:
Inconsistent behavior on deleting a running pod in DAG
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**:
When a running pod of a DAG from a workflow is deleted, the pod fails with either
```yaml
phase: Failed
message: pod termination
```
or
```yaml
phase: Error
message: pod deleted
```

**What you expected to happen**:
Expected the pod to fail with `phase: Error`.

**How to reproduce it (as minimally and precisely as possible)**:
- Submit the workflow
```
argo submit https://gist.githubusercontent.com/jyotishp/6375f62044d549f907d6f79ba49d6a3f/raw/9c541dd53df1a966dd9c8dfa116d6920d76de0a1/workflow.yaml
```
- Identify the running pod (named "B") and run
```
kubectl delete pod --force <pod>
```
Sometimes the pod status is set to `Failed` and sometimes to `Error`.

**Anything else we need to know?**:

**Environment**:

- Argo version:
```
$ argo version
argo: v2.8.0
  BuildDate: 2020-05-11T22:55:16Z
  GitCommit: 8f696174746ed01b9bf1941ad03da62d312df641
  GitTreeState: clean
  GitTag: v2.8.0
  GoVersion: go1.13.4
  Compiler: gc
  Platform: linux/amd64
```
- Kubernetes version :
```
$ kubectl version -o yaml
clientVersion:
  buildDate: "2020-04-23T22:11:11Z"
  compiler: gc
  gitCommit: 52c56ce7a8272c798dbc29846288d7cd9fbae032
  gitTreeState: archive
  gitVersion: v1.18.2
  goVersion: go1.14.2
  major: "1"
  minor: "18"
  platform: linux/amd64
serverVersion:
  buildDate: "2020-04-06T16:33:17Z"
  compiler: gc
  gitCommit: 34a615f32e9a0c9e97cdb9f749adb392758349a6
  gitTreeState: clean
  gitVersion: v1.14.10-gke.36
  goVersion: go1.12.12b4
  major: "1"
  minor: 14+
  platform: linux/amd64
```

**Other debugging information (if applicable)**:

- Workflow result for `phase: Error`:

```
Name:                wf-v8rff
Namespace:           default
ServiceAccount:      default
Status:              Error
Created:             Sun May 24 02:30:57 +0530 (22 seconds ago)
Started:             Sun May 24 02:30:57 +0530 (22 seconds ago)
Finished:            Sun May 24 02:31:13 +0530 (6 seconds ago)
Duration:            16 seconds

STEP         TEMPLATE  PODNAME             DURATION  MESSAGE
 ⚠ wf-v8rff  run-dag
 └-⚠ B       B         wf-v8rff-770678605  22s       pod deleted
```

- Workflow result for `phase: Failed`:

```
Name:                wf-sl7jp
Namespace:           default
ServiceAccount:      default
Status:              Failed
Created:             Sun May 24 02:31:25 +0530 (20 seconds ago)
Started:             Sun May 24 02:31:25 +0530 (20 seconds ago)
Finished:            Sun May 24 02:31:39 +0530 (6 seconds ago)
Duration:            14 seconds

STEP         TEMPLATE  PODNAME              DURATION  MESSAGE
 ✖ wf-sl7jp  run-dag
 └-✖ B       B         wf-sl7jp-3474479707  14s       pod termination
```

- [Workflow YAML](https://gist.github.com/jyotishp/6375f62044d549f907d6f79ba49d6a3f#file-workflow-yaml)

**Logs**

- [`argo get -o yaml` for `phase: Failed`](https://gist.github.com/jyotishp/6375f62044d549f907d6f79ba49d6a3f#file-failed-wf-log-yaml)
- [Controller logs for `phase: Failed`](https://gist.github.com/jyotishp/6375f62044d549f907d6f79ba49d6a3f#file-failed-wf-controller-logs-txt)
- [`argo get -o yaml` for `phase: Error`](https://gist.github.com/jyotishp/6375f62044d549f907d6f79ba49d6a3f#file-error-wf-log-yaml)
- [Controller logs for `phase: Error`](https://gist.github.com/jyotishp/6375f62044d549f907d6f79ba49d6a3f#file-error-wf-controller-logs-txt)


---
<!-- 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 description:
fix(controller): Fail workflow when pod is deleted with --force. Fixes #3097
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](https://github.com/argoproj/argo/blob/master/USERS.md).

Fixes #3097

This is actually not the original problem in #3097, instead introduced by `SignificantPodChange`, but only when `kubectl delete --force pod ...` is used. The informer treats this delete as a update rather than a delete. @jessesuen @alexmt informer weirdness you might want to be aware of.

Patch:
diff --git a/workflow/controller/controller.go b/workflow/controller/controller.go
index 54db26bf5..99a2aaa54 100644
--- a/workflow/controller/controller.go
+++ b/workflow/controller/controller.go
@@ -726,30 +726,34 @@ func (wfc *WorkflowController) newPodInformer() cache.SharedIndexInformer {
 		cache.ResourceEventHandlerFuncs{
 			AddFunc: func(obj interface{}) {
 				key, err := cache.MetaNamespaceKeyFunc(obj)
-				if err == nil {
-					wfc.podQueue.Add(key)
+				if err != nil {
+					return
 				}
+				wfc.podQueue.Add(key)
 			},
 			UpdateFunc: func(old, new interface{}) {
+				key, err := cache.MetaNamespaceKeyFunc(new)
+				if err != nil {
+					return
+				}
 				oldPod, newPod := old.(*apiv1.Pod), new.(*apiv1.Pod)
 				if oldPod.ResourceVersion == newPod.ResourceVersion {
 					return
 				}
 				if !pod.SignificantPodChange(oldPod, newPod) {
+					log.WithField("key", key).Info("insignificant pod change")
 					return
 				}
-				key, err := cache.MetaNamespaceKeyFunc(new)
-				if err == nil {
-					wfc.podQueue.Add(key)
-				}
+				wfc.podQueue.Add(key)
 			},
 			DeleteFunc: func(obj interface{}) {
 				// IndexerInformer uses a delta queue, therefore for deletes we have to use this
 				// key function.
 				key, err := cache.DeletionHandlingMetaNamespaceKeyFunc(obj)
-				if err == nil {
-					wfc.podQueue.Add(key)
+				if err != nil {
+					return
 				}
+				wfc.podQueue.Add(key)
 			},
 		},
 	)
diff --git a/workflow/controller/pod/significant.go b/workflow/controller/pod/significant.go
index f42ec7dec..e9c2b7f7d 100644
--- a/workflow/controller/pod/significant.go
+++ b/workflow/controller/pod/significant.go
@@ -9,6 +9,7 @@ func SignificantPodChange(from *apiv1.Pod, to *apiv1.Pod) bool {
 		from.Status.Phase != to.Status.Phase ||
 		from.Status.Message != to.Status.Message ||
 		from.Status.PodIP != to.Status.PodIP ||
+		from.GetDeletionTimestamp() != to.GetDeletionTimestamp() ||
 		significantContainerStatusesChange(from.Status.ContainerStatuses, to.Status.ContainerStatuses) ||
 		significantContainerStatusesChange(from.Status.InitContainerStatuses, to.Status.InitContainerStatuses)
 }


Test patch:
diff --git a/workflow/controller/pod/significant_test.go b/workflow/controller/pod/significant_test.go
index eae08ea55..534d1b7fa 100644
--- a/workflow/controller/pod/significant_test.go
+++ b/workflow/controller/pod/significant_test.go
@@ -5,10 +5,16 @@ import (
 
 	"github.com/stretchr/testify/assert"
 	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
 func Test_SgnificantPodChange(t *testing.T) {
 	assert.False(t, SignificantPodChange(&corev1.Pod{}, &corev1.Pod{}), "No change")
+
+	t.Run("DeletionTimestamp", func(t *testing.T) {
+		now := metav1.Now()
+		assert.True(t, SignificantPodChange(&corev1.Pod{}, &corev1.Pod{ObjectMeta: metav1.ObjectMeta{DeletionTimestamp: &now}}), "deletion timestamp change")
+	})
 	t.Run("Spec", func(t *testing.T) {
 		assert.True(t, SignificantPodChange(&corev1.Pod{}, &corev1.Pod{Spec: corev1.PodSpec{NodeName: "from"}}), "Node name change")