feat: Add finalizer to workflow pod to prevent pod deleted. Fixes #8783. Continuing Work of #9058

[sakai-ast]

Fixes #8783 Continuing Work of #9058

Motivation

Building upon the foundational work of #9058, I am advancing the implementation to address our needs.
see: #9058 (comment)

Modifications

Verification

ut and e2e tests

[juliev0]

I'm surprised but I've just become aware that the Argo Server deletes Pods as part of RetryWorkflow() (here). I figured it was something that would only be the role of the Controller to do, but in any case, I guess this is an example where pod deletion is happening outside of these calls. I suppose the labelPodCompleted code would happen in any case, but I'm not sure what happens if you try to invoke this on an incomplete Workflow.

[sakai-ast]

I suppose the labelPodCompleted code would happen in any case

I agree. In my local tests, retried pods always have the labelPodCompleted and the finalizer is removed.

but I'm not sure what happens if you try to invoke this on an incomplete Workflow.

I believe the feature flag is intended for situations like the one described above.

[juliev0]

Would you mind adding to the Description more information on how you decided on your approach and your investigation of corner cases?

[isubasinghe]

I've added a couple more comments, could you please address them? Just so that we are on the same page.

[juliev0]

Approved before waiting for e2e tests....will approve once they all do

[alexec]

🍾

[imliuda]

if workflow-controller stopped for a while, eg, maintaining or upgrading, and then workflows deleted by somebody, if this will still work properly? during the gap, some pods completed, but the finalizer have no chance to get removed, how can we ensure that pods can be deleted.

[Joibel]

You can't ensure the pods will be deleted. The deletion will hang until either the controller is started or someone through another means deletes the finalizer words on the pod. The point of this change is to do this, and prevent inconsistency of state between the workflow and pods.

If you are happy with the alternative that a workflow may have unexpectedly missing pods you may delete the finalizer manually from the pods. This is scriptable.

[imliuda]

It is unnecessary for keeping consistency between workflow and pods, once workflow-controller has finished processing pods (that is node.Fullfilled()), then the pod can be deleted safely. Or, that may induce a memory leak of kube-apiserver due to large amount pods (kube-controller-manager will do pod gc of Failed and Succeed pods). Since finalizer is added by workflow-controller, then it should have to be responsible for removing it when the corresponding workflow deleted. Removing finalizer by a script is not appropriate.

I can make some optimizations for situation when pod's owner workflow has been deleted, and open a new PR for that.