fix: all written artifacts should be saved and garbage collected (#13678

) Signed-off-by: Julie Vogelman <julievogelman0@gmail.com>
argoproj · Oct 1, 2024 · ca6c414 · ca6c414
1 parent a0ba3c7
commit ca6c414
Show file tree

Hide file tree

Showing 4 changed files with 105 additions and 33 deletions.
diff --git a/test/e2e/artifacts_test.go b/test/e2e/artifacts_test.go
@@ -307,11 +307,11 @@ func (s *ArtifactsSuite) TestArtifactGC() {
 			workflowShouldSucceed:        true,
 			expectedGCPodsOnWFCompletion: 2,
 			expectedArtifacts: []artifactState{
-				artifactState{s3Location{bucketName: "my-bucket-2", specifiedKey: "first-on-completion-1"}, true, false},
-				artifactState{s3Location{bucketName: "my-bucket-3", specifiedKey: "first-on-completion-2"}, true, false},
-				artifactState{s3Location{bucketName: "my-bucket-3", specifiedKey: "first-no-deletion"}, false, false},
-				artifactState{s3Location{bucketName: "my-bucket-3", specifiedKey: "second-on-deletion"}, false, true},
-				artifactState{s3Location{bucketName: "my-bucket-2", specifiedKey: "second-on-completion"}, true, false},
+				{s3Location{bucketName: "my-bucket-2", specifiedKey: "first-on-completion-1"}, true, false},
+				{s3Location{bucketName: "my-bucket-3", specifiedKey: "first-on-completion-2"}, true, false},
+				{s3Location{bucketName: "my-bucket-3", specifiedKey: "first-no-deletion"}, false, false},
+				{s3Location{bucketName: "my-bucket-3", specifiedKey: "second-on-deletion"}, false, true},
+				{s3Location{bucketName: "my-bucket-2", specifiedKey: "second-on-completion"}, true, false},
 			},
 		},
 		// entire Workflow based on a WorkflowTemplate
@@ -321,8 +321,8 @@ func (s *ArtifactsSuite) TestArtifactGC() {
 			workflowShouldSucceed:        true,
 			expectedGCPodsOnWFCompletion: 1,
 			expectedArtifacts: []artifactState{
-				artifactState{s3Location{bucketName: "my-bucket-2", specifiedKey: "on-completion"}, true, false},
-				artifactState{s3Location{bucketName: "my-bucket-2", specifiedKey: "on-deletion"}, false, true},
+				{s3Location{bucketName: "my-bucket-2", specifiedKey: "on-completion"}, true, false},
+				{s3Location{bucketName: "my-bucket-2", specifiedKey: "on-deletion"}, false, true},
 			},
 		},
 		// entire Workflow based on a WorkflowTemplate
@@ -332,8 +332,8 @@ func (s *ArtifactsSuite) TestArtifactGC() {
 			workflowShouldSucceed:        true,
 			expectedGCPodsOnWFCompletion: 1,
 			expectedArtifacts: []artifactState{
-				artifactState{s3Location{bucketName: "my-bucket-2", specifiedKey: "on-completion"}, true, false},
-				artifactState{s3Location{bucketName: "my-bucket-2", specifiedKey: "on-deletion"}, false, true},
+				{s3Location{bucketName: "my-bucket-2", specifiedKey: "on-completion"}, true, false},
+				{s3Location{bucketName: "my-bucket-2", specifiedKey: "on-deletion"}, false, true},
 			},
 		},
 		// Step in Workflow references a WorkflowTemplate's template
@@ -343,8 +343,8 @@ func (s *ArtifactsSuite) TestArtifactGC() {
 			workflowShouldSucceed:        true,
 			expectedGCPodsOnWFCompletion: 1,
 			expectedArtifacts: []artifactState{
-				artifactState{s3Location{bucketName: "my-bucket-2", specifiedKey: "on-completion"}, true, false},
-				artifactState{s3Location{bucketName: "my-bucket-2", specifiedKey: "on-deletion"}, false, true},
+				{s3Location{bucketName: "my-bucket-2", specifiedKey: "on-completion"}, true, false},
+				{s3Location{bucketName: "my-bucket-2", specifiedKey: "on-deletion"}, false, true},
 			},
 		},
 		// Step in Workflow references a WorkflowTemplate's template
@@ -354,8 +354,8 @@ func (s *ArtifactsSuite) TestArtifactGC() {
 			workflowShouldSucceed:        true,
 			expectedGCPodsOnWFCompletion: 1,
 			expectedArtifacts: []artifactState{
-				artifactState{s3Location{bucketName: "my-bucket-2", specifiedKey: "on-completion"}, true, false},
-				artifactState{s3Location{bucketName: "my-bucket-2", specifiedKey: "on-deletion"}, false, false},
+				{s3Location{bucketName: "my-bucket-2", specifiedKey: "on-completion"}, true, false},
+				{s3Location{bucketName: "my-bucket-2", specifiedKey: "on-deletion"}, false, false},
 			},
 		},
 		// entire Workflow based on a WorkflowTemplate which has a Step that references another WorkflowTemplate's template
@@ -365,8 +365,8 @@ func (s *ArtifactsSuite) TestArtifactGC() {
 			workflowShouldSucceed:        true,
 			expectedGCPodsOnWFCompletion: 1,
 			expectedArtifacts: []artifactState{
-				artifactState{s3Location{bucketName: "my-bucket-2", specifiedKey: "on-completion"}, true, false},
-				artifactState{s3Location{bucketName: "my-bucket-2", specifiedKey: "on-deletion"}, false, true},
+				{s3Location{bucketName: "my-bucket-2", specifiedKey: "on-completion"}, true, false},
+				{s3Location{bucketName: "my-bucket-2", specifiedKey: "on-deletion"}, false, true},
 			},
 		},
 		// Step in Workflow references a WorkflowTemplate's template
@@ -380,12 +380,24 @@ func (s *ArtifactsSuite) TestArtifactGC() {
 		},
 		// Workflow fails to write an artifact that's been defined as an Output
 		{
-			workflowFile:                 "@testdata/artifactgc/artgc-artifact-not-written.yaml",
+			workflowFile:                 "@testdata/artifactgc/artgc-non-optional-artifact-not-written.yaml",
 			hasGC:                        true,
 			workflowShouldSucceed:        false, // artifact not being present causes Workflow to fail
 			expectedGCPodsOnWFCompletion: 0,
 			expectedArtifacts: []artifactState{
-				artifactState{s3Location{bucketName: "my-bucket", derivedKey: &artifactDerivedKey{templateName: "artifact-written", artifactName: "present"}}, false, true},
+				{s3Location{bucketName: "my-bucket", derivedKey: &artifactDerivedKey{templateName: "artifact-written", artifactName: "present"}}, false, true},
+				{s3Location{bucketName: "my-bucket", derivedKey: &artifactDerivedKey{templateName: "some-artifacts-not-written", artifactName: "present"}}, false, true},
+			},
+		},
+		// Workflow doesn't write an artifact that's been defined as an Output, but it's an Optional artifact, so Workflow succeeds
+		{
+			workflowFile:                 "@testdata/artifactgc/artgc-optional-artifact-not-written.yaml",
+			hasGC:                        true,
+			workflowShouldSucceed:        true,
+			expectedGCPodsOnWFCompletion: 0,
+			expectedArtifacts: []artifactState{
+				{s3Location{bucketName: "my-bucket", derivedKey: &artifactDerivedKey{templateName: "artifact-written", artifactName: "present"}}, false, true},
+				{s3Location{bucketName: "my-bucket", derivedKey: &artifactDerivedKey{templateName: "some-artifacts-not-written", artifactName: "present"}}, false, true},
 			},
 		},
 		// Workflow defined output artifact but execution failed, no artifacts to be gced

diff --git a/...rtifactgc/artgc-artifact-not-written.yaml → ...gc-non-optional-artifact-not-written.yaml b/...rtifactgc/artgc-artifact-not-written.yaml → ...gc-non-optional-artifact-not-written.yaml
@@ -1,7 +1,7 @@
 apiVersion: argoproj.io/v1alpha1
 kind: Workflow
 metadata:
-  generateName: artgc-artifact-not-written-
+  generateName: artgc-non-optional-artifact-not-written-
 spec:
   entrypoint: entrypoint
   artifactGC:
@@ -13,8 +13,8 @@ spec:
       steps:
       - - name: artifact-written
           template: artifact-written
-      - - name: artifact-not-written
-          template: artifact-not-written
+      - - name: some-artifacts-not-written
+          template: some-artifacts-not-written
     - name: artifact-written
       container:
         image: argoproj/argosay:v2
@@ -28,16 +28,18 @@ spec:
         artifacts:
           - name: present
             path: /tmp/present
-    - name: artifact-not-written
+    - name: some-artifacts-not-written
       container:
         image: argoproj/argosay:v2
         command:
           - sh
           - -c
         args:
           - |
-            echo "intentionally not writing anything to disk"
+            echo "something" > /tmp/present
       outputs:
         artifacts:
           - name: notpresent
-            path: /tmp/notpresent
+            path: /tmp/notpresent
+          - name: present
+            path: /tmp/present
diff --git a/test/e2e/testdata/artifactgc/artgc-optional-artifact-not-written.yaml b/test/e2e/testdata/artifactgc/artgc-optional-artifact-not-written.yaml
@@ -0,0 +1,46 @@
+apiVersion: argoproj.io/v1alpha1
+kind: Workflow
+metadata:
+  generateName: artgc-optional-artifact-not-written-
+spec:
+  entrypoint: entrypoint
+  artifactGC:
+    strategy: OnWorkflowDeletion
+  podGC:
+    strategy: ""
+  templates:
+    - name: entrypoint
+      steps:
+      - - name: some-artifacts-not-written
+          template: some-artifacts-not-written
+      - - name: artifact-written
+          template: artifact-written
+    - name: some-artifacts-not-written
+      container:
+        image: argoproj/argosay:v2
+        command:
+          - sh
+          - -c
+        args:
+          - |
+            echo "something" > /tmp/present
+      outputs:
+        artifacts:
+          - name: notpresent
+            path: /tmp/notpresent
+            optional: true   # artifact is optional - therefore, Workflow can succeed
+          - name: present
+            path: /tmp/present
+    - name: artifact-written
+      container:
+        image: argoproj/argosay:v2
+        command:
+          - sh
+          - -c
+        args:
+          - |
+            echo "something" > /tmp/present
+      outputs:
+        artifacts:
+          - name: present
+            path: /tmp/present
diff --git a/workflow/executor/executor.go b/workflow/executor/executor.go
@@ -306,39 +306,51 @@ func (we *WorkflowExecutor) SaveArtifacts(ctx context.Context) (wfv1.Artifacts,
 		return artifacts, argoerrs.InternalWrapError(err)
 	}
 
+	aggregateError := ""
 	for _, art := range we.Template.Outputs.Artifacts {
-		err := we.saveArtifact(ctx, common.MainContainerName, &art)
+		saved, err := we.saveArtifact(ctx, common.MainContainerName, &art)
+
 		if err != nil {
-			return artifacts, err
+			aggregateError += err.Error() + "; "
+		}
+		if saved {
+			artifacts = append(artifacts, art)
 		}
-		artifacts = append(artifacts, art)
 	}
-	return artifacts, nil
+	if aggregateError == "" {
+		return artifacts, nil
+	} else {
+		return artifacts, errors.New(aggregateError)
+	}
+
 }
 
-func (we *WorkflowExecutor) saveArtifact(ctx context.Context, containerName string, art *wfv1.Artifact) error {
+// save artifact
+// return whether artifact was in fact saved, and if there was an error
+func (we *WorkflowExecutor) saveArtifact(ctx context.Context, containerName string, art *wfv1.Artifact) (bool, error) {
 	// Determine the file path of where to find the artifact
 	err := art.CleanPath()
 	if err != nil {
-		return err
+		return false, err
 	}
 	fileName, localArtPath, err := we.stageArchiveFile(containerName, art)
 	if err != nil {
 		if art.Optional && argoerrs.IsCode(argoerrs.CodeNotFound, err) {
 			log.Warnf("Ignoring optional artifact '%s' which does not exist in path '%s': %v", art.Name, art.Path, err)
-			return nil
+			return false, nil
 		}
-		return err
+		return false, err
 	}
 	fi, err := os.Stat(localArtPath)
 	if err != nil {
-		return err
+		return false, err
 	}
 	size := fi.Size()
 	if size == 0 {
 		log.Warnf("The file %q is empty. It may not be uploaded successfully depending on the artifact driver", localArtPath)
 	}
-	return we.saveArtifactFromFile(ctx, art, fileName, localArtPath)
+	err = we.saveArtifactFromFile(ctx, art, fileName, localArtPath)
+	return err == nil, err
 }
 
 // fileBase is probably path.Base(filePath), but can be something else