fix: don't necessarily include all artifacts from templates in node outputs #13066

juliev0 · 2024-05-19T15:41:23Z

The fact that it's being listed in the NodeStatus is causing ArtifactGC to attempt to delete it (and fail doing so).

Motivation

If an Artifact file didn't get written, it shouldn't be in the outputs for the TaskResult (and thus the Outputs for the Node) just because it was listed as an Artifact in the Workflow.

Modifications

Only if the Artifact file is successfully written does the wait container include it as one of the Output Artifacts in its WorkflowTaskResult. The Workflow Controller uses that WorkflowTaskResult information to populate the NodeStatus of the Workflow. This is the information that ArtifactGC uses to determine what needs to be deleted.

Verification

The person who submitted the Issue tested the image I made, and it repeatedly worked to solve his issue of ArtifactGC failing. (Note it also prevented his other artifacts from getting deleted, because the failed deletion preempted the other deletions from being attempted (as @agilgur5 pointed out, this could be another improvement, to continue the deletion process independent of failure)).

I did start to create an e2e test for this issue. But it seems that with minio, I can't repeat the same failure using the "main" branch. It seems that attempting to delete an artifact which doesn't exist doesn't result in a failure for some reason.

…utputs Signed-off-by: Julie Vogelman <julievogelman0@gmail.com>

juliev0 · 2024-05-19T15:42:07Z

workflow/executor/executor.go

 		}
-		we.Template.Outputs.Artifacts[i] = art


instead of updating this directly, create a new list of the artifacts that we actually saved and return that

LGTM. So before we were using the we.Template.Outputs.Artifacts to pass to ReportOutputs but now we are creating a fresh wfv1.Artifacts object that will only include the successfully saved artifacts. This makes sense.

Hmmm I do see a small issue here, but I think this already existed beforehand:
If the wait container is interrupted during this loop, some artifacts may save to S3 (etc) but not be reported

But since adding artifacts to Outputs.Artifacts wouldn't report them until the very end anyway, I think this doesn't change any existing behavior

If the wait container is interrupted during this loop, some artifacts may save to S3 (etc) but not be reported

How do you mean it wouldn't be reported? Since the background context is passed into both SaveArtifacts() and ReportOutputs() below, I believe all of that logic is still supposed to be executed. Is there something different you're referring to besides the context being cancelled?

If the loop is long or the container is otherwise non-gracefully terminated, the request context won't matter.

Also realized that saving artifacts can be parallelized here to reduce that chance and improve throughput (related: #12442). Similarly pre-existing logic though.

Signed-off-by: Julie Vogelman <julievogelman0@gmail.com>

Garett-MacGowan

LGTM

Garett-MacGowan · 2024-06-08T00:44:17Z

workflow/executor/executor.go

 		}
-		we.Template.Outputs.Artifacts[i] = art


LGTM. So before we were using the we.Template.Outputs.Artifacts to pass to ReportOutputs but now we are creating a fresh wfv1.Artifacts object that will only include the successfully saved artifacts. This makes sense.

…, including when no artifact is written Signed-off-by: Julie Vogelman <julievogelman0@gmail.com>

agilgur5 · 2024-06-09T02:33:13Z

(as @agilgur5 pointed out, this could be another improvement, to continue the deletion process independent of failure)

I was thinking about this and it will probably be fixed by parallel artifact GC (#11768), as that inherently has to process each artifact independently.

Vaguely related to this, I was wondering if creating more WorkflowTaskResults, e.g. child results, might help with some of these races, i.e. via create-only immutable records of progress. Particularly it could resolve a security issue with malicious Workflows as mentioned in #12783 (comment) as the Executor would then only need create permissions, and not patch.

Signed-off-by: Julie Vogelman <julievogelman0@gmail.com>

juliev0 · 2024-06-09T20:12:57Z

test/e2e/artifacts_test.go

@@ -352,7 +368,7 @@ func (s *ArtifactsSuite) TestArtifactGC() {
 				}
 			})

-		if when.WorkflowCondition(func(wf *wfv1.Workflow) bool {
+		if tt.workflowShouldSucceed && when.WorkflowCondition(func(wf *wfv1.Workflow) bool {


was previously trying to avoid intermittent test failures due to the workflow itself failing when it shouldn't - but now there's a Workflow which fails in here so this is how I'm allowing that one to be failed

Signed-off-by: Julie Vogelman <julievogelman0@gmail.com>

agilgur5

Mostly LGTM. I left a few style comments on the tests, one suggestion for the test case, and a question on the saving + reporting. Still learning my way around the artifacts part of the codebase

test/e2e/testdata/artifactgc/artgc-artifact-not-written.yaml

test/e2e/artifacts_test.go

agilgur5 · 2024-06-10T17:32:11Z

test/e2e/artifacts_test.go

 			expectedGCPodsOnWFCompletion: 0,
 			expectedArtifacts:            []artifactState{},


hmm to ensure that GC works in the presence of a missing artifact, should we make the Workflow have a normal, successful artifact as well? i.e. the same case as the bug report; some successful artifacts, some unsuccessful

in this current test case, GC doesn't run at all, right?

Okay, I've modified the test Workflow so that it also successfully saves an artifact file and garbage collects it. Had to update the test to look for artifact keys names that are auto-generated.

agilgur5 · 2024-06-10T17:55:45Z

workflow/executor/executor.go

 		}
-		we.Template.Outputs.Artifacts[i] = art


Hmmm I do see a small issue here, but I think this already existed beforehand:
If the wait container is interrupted during this loop, some artifacts may save to S3 (etc) but not be reported

But since adding artifacts to Outputs.Artifacts wouldn't report them until the very end anyway, I think this doesn't change any existing behavior

Co-authored-by: Anton Gilgur <4970083+agilgur5@users.noreply.github.com> Signed-off-by: Julie Vogelman <julievogelman0@gmail.com>

test/e2e/artifacts_test.go

Signed-off-by: Julie Vogelman <julievogelman0@gmail.com>

fix: don't necessarily include all artifacts from templates in node o…

c2bc603

…utputs Signed-off-by: Julie Vogelman <julievogelman0@gmail.com>

juliev0 commented May 19, 2024

View reviewed changes

juliev0 added 2 commits May 19, 2024 08:44

fix: unit test to compile

5734a5a

Signed-off-by: Julie Vogelman <julievogelman0@gmail.com>

fix: lint issue

afb55a8

Signed-off-by: Julie Vogelman <julievogelman0@gmail.com>

agilgur5 added the area/artifacts S3/GCP/OSS/Git/HDFS etc label May 29, 2024

juliev0 marked this pull request as ready for review June 1, 2024 23:55

juliev0 requested a review from Garett-MacGowan June 3, 2024 17:19

Garett-MacGowan approved these changes Jun 8, 2024

View reviewed changes

agilgur5 self-assigned this Jun 8, 2024

chore: verify Workflow finalizer is being removed in artifactgc tests…

c650be0

…, including when no artifact is written Signed-off-by: Julie Vogelman <julievogelman0@gmail.com>

juliev0 force-pushed the workflows-save-artifacts branch from c14ffb5 to c650be0 Compare June 9, 2024 02:13

fix: empty commit

1546ba7

Signed-off-by: Julie Vogelman <julievogelman0@gmail.com>

juliev0 commented Jun 9, 2024

View reviewed changes

fix: empty commit

8e8c603

Signed-off-by: Julie Vogelman <julievogelman0@gmail.com>

agilgur5 added this to the v3.5.x patches milestone Jun 10, 2024

agilgur5 mentioned this pull request Jun 10, 2024

ArtifactGC fails when workflows are retried #13161

Open

4 tasks

agilgur5 linked an issue Jun 10, 2024 that may be closed by this pull request

ArtifactGC fails when workflows are retried #13161

Open

4 tasks

agilgur5 reviewed Jun 10, 2024

View reviewed changes

agilgur5 added the area/gc Garbage collection, such as TTLs, retentionPolicy, delays, and more label Jun 10, 2024

juliev0 and others added 4 commits June 11, 2024 05:29

Update test/e2e/artifacts_test.go

67adb6c

Co-authored-by: Anton Gilgur <4970083+agilgur5@users.noreply.github.com> Signed-off-by: Julie Vogelman <julievogelman0@gmail.com>

Update test/e2e/testdata/artifactgc/artgc-artifact-not-written.yaml

2d69a29

Co-authored-by: Anton Gilgur <4970083+agilgur5@users.noreply.github.com> Signed-off-by: Julie Vogelman <julievogelman0@gmail.com>

Update test/e2e/testdata/artifactgc/artgc-artifact-not-written.yaml

707c0cb

Co-authored-by: Anton Gilgur <4970083+agilgur5@users.noreply.github.com> Signed-off-by: Julie Vogelman <julievogelman0@gmail.com>

Update test/e2e/testdata/artifactgc/artgc-artifact-not-written.yaml

ccb5178

Co-authored-by: Anton Gilgur <4970083+agilgur5@users.noreply.github.com> Signed-off-by: Julie Vogelman <julievogelman0@gmail.com>

agilgur5 reviewed Jun 11, 2024

View reviewed changes

test/e2e/artifacts_test.go Outdated Show resolved Hide resolved

chore: improve test

9e5b1c5

Signed-off-by: Julie Vogelman <julievogelman0@gmail.com>

juliev0 force-pushed the workflows-save-artifacts branch from 7fe0f16 to 9e5b1c5 Compare June 11, 2024 23:34

chore: improve test

3c57e69

Signed-off-by: Julie Vogelman <julievogelman0@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: don't necessarily include all artifacts from templates in node outputs #13066

fix: don't necessarily include all artifacts from templates in node outputs #13066

juliev0 commented May 19, 2024 •

edited by agilgur5

juliev0 May 19, 2024

Garett-MacGowan Jun 8, 2024 •

edited by agilgur5

agilgur5 Jun 10, 2024 •

edited

juliev0 Jun 10, 2024

agilgur5 Jun 10, 2024 •

edited

Garett-MacGowan left a comment

Garett-MacGowan Jun 8, 2024 •

edited by agilgur5

agilgur5 commented Jun 9, 2024 •

edited

juliev0 Jun 9, 2024

agilgur5 left a comment •

edited

agilgur5 Jun 10, 2024

juliev0 Jun 11, 2024

agilgur5 Jun 10, 2024 •

edited

		expectedGCPodsOnWFCompletion: 0,
		expectedArtifacts: []artifactState{},

fix: don't necessarily include all artifacts from templates in node outputs #13066

Are you sure you want to change the base?

fix: don't necessarily include all artifacts from templates in node outputs #13066

Conversation

juliev0 commented May 19, 2024 • edited by agilgur5

Motivation

Modifications

Verification

juliev0 May 19, 2024

Choose a reason for hiding this comment

Garett-MacGowan Jun 8, 2024 • edited by agilgur5

Choose a reason for hiding this comment

agilgur5 Jun 10, 2024 • edited

Choose a reason for hiding this comment

juliev0 Jun 10, 2024

Choose a reason for hiding this comment

agilgur5 Jun 10, 2024 • edited

Choose a reason for hiding this comment

Garett-MacGowan left a comment

Choose a reason for hiding this comment

Garett-MacGowan Jun 8, 2024 • edited by agilgur5

Choose a reason for hiding this comment

agilgur5 commented Jun 9, 2024 • edited

juliev0 Jun 9, 2024

Choose a reason for hiding this comment

agilgur5 left a comment • edited

Choose a reason for hiding this comment

agilgur5 Jun 10, 2024

Choose a reason for hiding this comment

juliev0 Jun 11, 2024

Choose a reason for hiding this comment

agilgur5 Jun 10, 2024 • edited

Choose a reason for hiding this comment

juliev0 commented May 19, 2024 •

edited by agilgur5

Garett-MacGowan Jun 8, 2024 •

edited by agilgur5

agilgur5 Jun 10, 2024 •

edited

agilgur5 Jun 10, 2024 •

edited

Garett-MacGowan Jun 8, 2024 •

edited by agilgur5

agilgur5 commented Jun 9, 2024 •

edited

agilgur5 left a comment •

edited

agilgur5 Jun 10, 2024 •

edited