From 0f1cd68e5ff63bd00df81d00851ea66c839f9b56 Mon Sep 17 00:00:00 2001
From: chengjoey <30427474+chengjoey@users.noreply.github.com>
Date: Tue, 27 Aug 2024 01:24:13 +0800
Subject: [PATCH] fix(controller): remove ArtifactGC `finalizer` when no
 artifacts. Fixes #13499 (#13500)

Signed-off-by: joey <zchengjoey@gmail.com>
---
 test/e2e/artifacts_test.go                    |  8 ++++++
 .../artgc-artifact-not-written-failed.yaml    | 27 +++++++++++++++++++
 workflow/controller/artifact_gc.go            |  7 ++---
 3 files changed, 37 insertions(+), 5 deletions(-)
 create mode 100644 test/e2e/testdata/artifactgc/artgc-artifact-not-written-failed.yaml

diff --git a/test/e2e/artifacts_test.go b/test/e2e/artifacts_test.go
index 1caa76d4c35c..b86a4b81e603 100644
--- a/test/e2e/artifacts_test.go
+++ b/test/e2e/artifacts_test.go
@@ -388,6 +388,14 @@ func (s *ArtifactsSuite) TestArtifactGC() {
 				artifactState{s3Location{bucketName: "my-bucket", derivedKey: &artifactDerivedKey{templateName: "artifact-written", artifactName: "present"}}, false, true},
 			},
 		},
+		// Workflow defined output artifact but execution failed, no artifacts to be gced
+		{
+			workflowFile:                 "@testdata/artifactgc/artgc-artifact-not-written-failed.yaml",
+			hasGC:                        true,
+			workflowShouldSucceed:        false,
+			expectedGCPodsOnWFCompletion: 0,
+			expectedArtifacts:            []artifactState{},
+		},
 	} {
 		// for each test make sure that:
 		// 1. the finalizer gets added
diff --git a/test/e2e/testdata/artifactgc/artgc-artifact-not-written-failed.yaml b/test/e2e/testdata/artifactgc/artgc-artifact-not-written-failed.yaml
new file mode 100644
index 000000000000..31f9e6707bad
--- /dev/null
+++ b/test/e2e/testdata/artifactgc/artgc-artifact-not-written-failed.yaml
@@ -0,0 +1,27 @@
+apiVersion: argoproj.io/v1alpha1
+kind: Workflow
+metadata:
+  generateName: artgc-artifact-not-written-failed-
+spec:
+  entrypoint: main
+  artifactGC:
+    strategy: OnWorkflowDeletion
+  templates:
+    - name: main
+      container:
+        image: argoproj/argosay:v2
+        command:
+          - sh
+          - -c
+        args:
+          - |
+            echo "intentionally not writing anything to disk and intentional failure"
+            exit 1
+      outputs:
+        artifacts:
+          - name: on-completion
+            path: /tmp/on-completion.txt
+            s3:
+              key: on-completion.txt
+            artifactGC:
+              strategy: OnWorkflowCompletion
\ No newline at end of file
diff --git a/workflow/controller/artifact_gc.go b/workflow/controller/artifact_gc.go
index 7bb8ed627d05..1fe9d9da153c 100644
--- a/workflow/controller/artifact_gc.go
+++ b/workflow/controller/artifact_gc.go
@@ -514,7 +514,6 @@ func (woc *wfOperationCtx) processArtifactGCCompletion(ctx context.Context) erro
 		return fmt.Errorf("failed to get pods from informer: %w", err)
 	}
 
-	anyPodSuccess := false
 	for _, obj := range pods {
 		pod := obj.(*corev1.Pod)
 		if pod.Labels[common.LabelKeyComponent] != artifactGCComponent { // make sure it's an Artifact GC Pod
@@ -540,10 +539,8 @@ func (woc *wfOperationCtx) processArtifactGCCompletion(ctx context.Context) erro
 			if err != nil {
 				return err
 			}
+
 			woc.wf.Status.ArtifactGCStatus.SetArtifactGCPodRecouped(pod.Name, true)
-			if phase == corev1.PodSucceeded {
-				anyPodSuccess = true
-			}
 			woc.updated = true
 		}
 	}
@@ -554,7 +551,7 @@ func (woc *wfOperationCtx) processArtifactGCCompletion(ctx context.Context) erro
 		removeFinalizer = woc.wf.Status.ArtifactGCStatus.AllArtifactGCPodsRecouped()
 	} else {
 		// check if all artifacts have been deleted and if so remove Finalizer
-		removeFinalizer = anyPodSuccess && woc.allArtifactsDeleted()
+		removeFinalizer = woc.allArtifactsDeleted()
 	}
 	if removeFinalizer {
 		woc.log.Infof("no remaining artifacts to GC, removing artifact GC finalizer (forceFinalizerRemoval=%v)", forceFinalizerRemoval)