fix: if pod fails without container termination, don't mark node succeeded always (cherry-pick #15150 for 3.7) (#15155)

Signed-off-by: Alan Clucas <[email protected]>

Author: Joibel

workflow/controller/operator.go

@@ -1683,6 +1683,12 @@ func (woc *wfOperationCtx) inferFailedReason(pod *apiv1.Pod, tmpl *wfv1.Template
 		return wfv1.NodeFailed, fmt.Sprintf("can't find failed message for pod %s namespace %s", pod.Name, pod.Namespace)
 	}
 
+	// Track whether critical containers completed successfully (terminated with exit code 0).
+	// We must confirm both to return success—otherwise a pod-level failure (eviction, node death)
+	// could be incorrectly reported as success.
+	mainContainerSucceeded := false
+	waitContainerSucceeded := false
+
 	for _, ctr := range ctrs {
 
 		// Virtual Kubelet environment will not set the terminate on waiting container
@@ -1694,11 +1700,15 @@ func (woc *wfOperationCtx) inferFailedReason(pod *apiv1.Pod, tmpl *wfv1.Template
 		}
 		t := ctr.State.Terminated
 		if t == nil {
-			// We should never get here
 			log.Warnf("Pod %s phase was Failed but %s did not have terminated state", pod.Name, ctr.Name)
 			continue
 		}
 		if t.ExitCode == 0 {
+			if tmpl.IsMainContainerName(ctr.Name) {
+				mainContainerSucceeded = true
+			} else if ctr.Name == common.WaitContainerName {
+				waitContainerSucceeded = true
+			}
 			continue
 		}
 
@@ -1727,11 +1737,22 @@ func (woc *wfOperationCtx) inferFailedReason(pod *apiv1.Pod, tmpl *wfv1.Template
 		}
 	}
 
-	// If we get here, we have detected that the main/wait containers succeed but the sidecar(s)
-	// were  SIGKILL'd. The executor may have had to forcefully terminate the sidecar (kill -9),
-	// resulting in a 137 exit code (which we had ignored earlier). If failMessages is empty, it
-	// indicates that this is the case and we return Success instead of Failure.
-	return wfv1.NodeSucceeded, ""
+	// Determine final status based on whether we confirmed main and wait succeeded
+	// Slightly convulted approach to avoid the exhaustive linter getting upset
+	if mainContainerSucceeded {
+		if waitContainerSucceeded {
+			// Both succeeded - sidecars may have been force-killed (137/143), which is fine
+			return wfv1.NodeSucceeded, ""
+		} else {
+			return wfv1.NodeFailed, "pod failed: wait container did not complete successfully"
+		}
+	} else {
+		if waitContainerSucceeded {
+			return wfv1.NodeFailed, "pod failed: main container did not complete successfully"
+		} else {
+			return wfv1.NodeFailed, "pod failed: neither main nor wait container completed successfully"
+		}
+	}
 }
 
 func (woc *wfOperationCtx) createPVCs(ctx context.Context) error {

workflow/controller/operator_test.go

@@ -7171,6 +7171,134 @@ func TestPodFailureWithContainerWaitingState(t *testing.T) {
 	assert.Equal(t, "Pod failed before main container starts due to ContainerCreating: Container is creating", msg)
 }
 
+// Test that when containers are in Running state (not Terminated) on a Failed pod,
+// we correctly return Failed instead of incorrectly returning Succeeded.
+// This can happen during node eviction or other pod-level failures.
+var podFailedWithRunningContainers = `
+apiVersion: v1
+kind: Pod
+metadata:
+  name: test-pod
+spec:
+  containers:
+  - name: main
+    env:
+    - name: ARGO_CONTAINER_NAME
+      value: main
+status:
+  phase: Failed
+  reason: Evicted
+  message: "The node was low on resource: memory."
+  containerStatuses:
+  - name: main
+    ready: false
+    restartCount: 0
+    state:
+      running:
+        startedAt: "2021-01-22T09:50:16Z"
+  - name: wait
+    ready: false
+    restartCount: 0
+    state:
+      running:
+        startedAt: "2021-01-22T09:50:16Z"
+`
+
+func TestPodFailureWithRunningContainers(t *testing.T) {
+	var pod apiv1.Pod
+	wfv1.MustUnmarshal(podFailedWithRunningContainers, &pod)
+	assert.NotNil(t, pod)
+
+	// Pod has a message, so it should return that
+	nodeStatus, msg := newWoc().inferFailedReason(&pod, nil)
+	assert.Equal(t, wfv1.NodeFailed, nodeStatus)
+	assert.Equal(t, "The node was low on resource: memory.", msg)
+}
+
+// Test that when containers don't have terminated state and there's no pod message,
+// we correctly return Failed with information about which containers couldn't be confirmed.
+var podFailedWithRunningContainersNoMessage = `
+apiVersion: v1
+kind: Pod
+metadata:
+  name: test-pod
+spec:
+  containers:
+  - name: main
+    env:
+    - name: ARGO_CONTAINER_NAME
+      value: main
+status:
+  phase: Failed
+  containerStatuses:
+  - name: main
+    ready: false
+    restartCount: 0
+    state:
+      running:
+        startedAt: "2021-01-22T09:50:16Z"
+  - name: wait
+    ready: false
+    restartCount: 0
+    state:
+      running:
+        startedAt: "2021-01-22T09:50:16Z"
+`
+
+func TestPodFailureWithRunningContainersNoMessage(t *testing.T) {
+	var pod apiv1.Pod
+	wfv1.MustUnmarshal(podFailedWithRunningContainersNoMessage, &pod)
+	assert.NotNil(t, pod)
+	nodeStatus, msg := newWoc().inferFailedReason(&pod, nil)
+	assert.Equal(t, wfv1.NodeFailed, nodeStatus)
+	assert.Equal(t, "pod failed: neither main nor wait container completed successfully", msg)
+}
+
+// Test that sidecar SIGKILL still results in success when main and wait succeeded
+var podFailedWithSidecarSigkill = `
+apiVersion: v1
+kind: Pod
+metadata:
+  name: test-pod
+spec:
+  containers:
+  - name: main
+    env:
+    - name: ARGO_CONTAINER_NAME
+      value: main
+status:
+  phase: Failed
+  containerStatuses:
+  - name: main
+    ready: false
+    state:
+      terminated:
+        exitCode: 0
+        reason: Completed
+  - name: wait
+    ready: false
+    state:
+      terminated:
+        exitCode: 0
+        reason: Completed
+  - name: sidecar
+    ready: false
+    state:
+      terminated:
+        exitCode: 137
+        reason: Error
+`
+
+func TestPodFailureWithSidecarSigkill(t *testing.T) {
+	var pod apiv1.Pod
+	wfv1.MustUnmarshal(podFailedWithSidecarSigkill, &pod)
+	assert.NotNil(t, pod)
+	nodeStatus, msg := newWoc().inferFailedReason(&pod, nil)
+	// Main and wait succeeded, sidecar was SIGKILL'd - this should be success
+	assert.Equal(t, wfv1.NodeSucceeded, nodeStatus)
+	assert.Empty(t, msg)
+}
+
 var podWithWaitContainerOOM = `
 apiVersion: v1
 kind: Pod