fix(controller): remove ArtifactGC finalizer when no artifacts. Fixes #13499

[chengjoey]

Fixes #13499

Motivation

if workflow pods length is zero, anyPodSuccess should be true, then the removeFinalizer can be executed without force

Modifications

anyPodSuccess := len(pods) == 0

Verification

run examples/artifact-gc-workflow.yaml on mac m1 os, and then delete the workflow

[agilgur5]

I'm not sure this is quite correct. A length of 0 could also mean that an ArtifactGC Pod has yet to run.

In your issue, the ArtifactGC Pod failed, and this is exactly what the forceFinalizerRemoval field is for

[chengjoey]

argo-workflows/workflow/controller/operator.go

Lines 242 to 250 in ddbb3c7

	// Do artifact GC if task result reconciliation is complete.
	if woc.wf.Status.Fulfilled() {
	if err := woc.garbageCollectArtifacts(ctx); err != nil {
	woc.log.WithError(err).Error("failed to GC artifacts")
	return
	}
	} else {
	woc.log.Debug("Skipping artifact GC")
	}

hi @agilgur5 , artifacts gc will only be running when task reconciliation is completed. I think the pod has finished running at this time. and i think this is a normal operation to delete the workflow, no need to force, increase the cost of the deletion operation, because this is not a gc problem that requires forced deletion

[agilgur5]

TaskResult reconciliation should already be completed. In your issue, I'm pretty sure the ArtifactGC Pod itself fails (due to the architecture). As it failed, the Controller cannot be sure that the artifact was deleted or not

[agilgur5]

I'm pretty sure the ArtifactGC Pod itself fails (due to the architecture)

Hmm, no, ArtifactGC can run on arm64; your Pod that fails due to arch is the argosay image's arch -- see #11613 (comment) .

artifacts gc will only be running when task reconciliation is completed

TaskResult reconciliation should already be completed

Or you're experiencing a different race condition from v3.5.3+ / v3.5.5+. This actually sounds like the root cause is #12993, which was recently fixed in #13454

[chengjoey]

I'm pretty sure the ArtifactGC Pod itself fails (due to the architecture)

Hmm, no, ArtifactGC can run on arm64; your Pod that fails due to arch is the argosay image's arch -- see #11613 (comment) that I was going to fix recently as I may have permissions to the image repositories.

Yes, I know that the reason why I failed to run is because the image argoproj/argosay:v2 in the examples does not support the linux/arm64 architecture, not because artifact-gc cannot run in arm64. I raised this PR because my workflow failed and no artifact was generated, so there is no need to do GC. I hope to be able to delete the workflow directly without adding additional operations force

[agilgur5]

I raised this PR because my workflow failed and no artifact was generated, so there is no need to do GC.

Yes, the second half of my previous comment is relevant with regard to TaskResult reconciliation. This looks like it is caused by a bug in TaskResult reconciliation and not specific to ArtifactGC

[chengjoey]

thanks @agilgur5 , i got it

[agilgur5]

Ok this makes more sense now given your last comment on the issue, thanks again for elaborating!

From my first comment above though:

A length of 0 could also mean that an ArtifactGC Pod has yet to run

I think we could potentially remove this variable entirely and just replace with a check that all ArtifactGC Pods were recouped?

Otherwise it looks like anyPodSuccess is only used for the one finalizer removal check you linked to, so perhaps want to invert that or rename the variable if it's still needed.

cc @juliev0 here on the PR as well

[juliev0]

Thanks for finding this.

So, I commented in the issue here, but will add here as well:

Looking at the anyPodSuccess code, I'm pretty sure it was probably just for the purpose of eliminating unnecessary work, to avoid doing the woc.allArtifactsDeleted() check if none of the Artifact GC Pods had even finished and succeeded yet. We can remove it if we need to. I'm looking at the logic for woc.allArtifactsDeleted() and it actually doesn't look that expensive , so I'd be okay to remove the anyPodSuccess altogether if it makes the code simpler.

I'm surprised we haven't encountered this issue before, of a Workflow that creates no artifacts.

[chengjoey]

thanks for your patience @agilgur5 @juliev0 , In this PR I removed anyPodSuccess, allArtifactsDeleted is enough to ensure that artifacts can be GCed

[juliev0]

thanks for the modification! The e2e tests are sometimes flakey, so if the e2e test failures are unrelated to your changes, feel free to push empty commits until it passes

[agilgur5]

so I'd be okay to remove the anyPodSuccess altogether if it makes the code simpler.

@juliev0 did you see my comment above? I thought the check might be to make sure that there were no leftover ArtifactGC Pods that were not yet recouped. But if it's even simpler than that, that works

feel free to push empty commits until it passes

Given that we have 2 approvers here, it'd be more efficient for one of us to just rerun the unsuccessful jobs (and any member can do /retest now)

I'm surprised we haven't encountered this issue before, of a Workflow that creates no artifacts.

It's very possible people have been using forceFinalizerRemoval to workaround this. Although I also just think there aren't that many ArtifactGC users yet.

Speaking of, it'd be nice to add an E2E test for this

[agilgur5]

@chengjoey do you think you could add a regression test for this scenario? If I'm not mistaken, you should be able to use any main container that intentionally fails while having output.artifacts specified

[chengjoey]

Ok, I will add an e2e test

[juliev0]

I thought the check might be to make sure that there were no leftover ArtifactGC Pods that were not yet recouped.

Hmm, does it accomplish that? I'm having trouble following if so. I think it was just to eliminate work.

Thanks for adding the test @chengjoey. If I'm not mistaken, I think you should be able to add it as another case to this test. This test verifies that the finalizer is removed by checking that the workflow actually gets deleted.

[agilgur5]

mostly consistency comments on the test below

[chengjoey]

mostly consistency comments on the test below

thanks @agilgur5 , all has done

[agilgur5]

LGTM, thanks for the iteration!

Will let Julie take another look at the tests before merging

[juliev0]

Yes, thank you @chengjoey !