fix: make the process of patching pods exclusive

[sakai-ast]

Motivation

This is to avoid the timeout errors such as the following recent CI errors.

https://github.com/argoproj/argo-workflows/actions/runs/7707372476/job/21004514734

https://github.com/argoproj/argo-workflows/actions/runs/7707667909/job/21005263284

https://github.com/argoproj/argo-workflows/actions/runs/7709093677/job/21009561377

https://github.com/argoproj/argo-workflows/actions/runs/7721063793/job/21047011775

This seems to have happened after merging #12413.

The reason this happens is that DeleteFunc and labelPodCompleted call the enablePodForDeletion at the same time, and the patching fails, and eventually the pod is in an unexpected state and fails the tests. It's described below.
#12596 (comment)
#12596 (comment)

Modifications

I have implemented mutual exclusion based on pod names in the enablePodForDeletion.

Verification

ut and e2e tests

[agilgur5]

Hit this in #12609 and #12610 too. Seems to be happening extremely frequently, so this is a blocker to fix or at least temporarily skip

[sakai-ast]

Currently, the following flaky errors occur when the test-functional tests (especially PodCleanupSuite) fail.

controller: time="2024-02-03T09:08:29.485Z" level=info msg="cleaning up pod" action=terminateContainers key=argo/test-pod-cleanup-ljrzv/terminateContainers
controller: time="2024-02-03T09:08:30.512Z" level=info msg="patching pod" data="[{\"op\":\"remove\",\"path\":\"/metadata/finalizers/0\"}]" podName=test-pod-cleanup-ljrzv
controller: time="2024-02-03T09:08:30.516Z" level=info msg="cleaning up pod" action=labelPodCompleted key=argo/test-pod-cleanup-ljrzv/labelPodCompleted
controller: time="2024-02-03T09:08:30.521Z" level=info msg="patching pod" data="[{\"op\":\"remove\",\"path\":\"/metadata/finalizers/0\"},{\"op\":\"replace\",\"path\":\"/metadata/labels/workflows.argoproj.io~1completed\",\"value\":\"true\"}]" podName=test-pod-cleanup-ljrzv
controller: time="2024-02-03T09:08:30.523Z" level=info msg="insignificant pod change" key=argo/test-pod-cleanup-ljrzv
controller: time="2024-02-03T09:08:30.524Z" level=warning msg="failed to clean-up pod" action=labelPodCompleted error="the server rejected our request due to an error in our request" key=argo/test-pod-cleanup-ljrzv/labelPodCompleted
controller: time="2024-02-03T09:08:30.524Z" level=warning msg="Non-transient error: the server rejected our request due to an error in our request"
controller: time="2024-02-03T09:08:31.488Z" level=info msg="cleaning up pod" action=killContainers key=argo/test-pod-cleanup-ljrzv/killContainers

I don't know why, the controller try to apply the finalizer remove patch 2 times, so the server rejected our request due to an error in our request occur on second time. the error reference here

The flaky patching fails when the action is labelPodCompleted, resulting in a pod condition that is not expected for tests, as shown below.
metadata.labels.workflows.argoproj.io/completed: "false"

This issue causes the failure of the test-functional tests and is difficult to reproduce locally, where it always succeeds.

Therefore, I am attempting to debug it.

[agilgur5]

After adding some debug log settings, I found the cause of 2 times applying the finalizer removal patch is that DeleteFunc and labelPodCompleted are calling the enablePodForDeletion at the same time.

Looks like the E2E tests passed once you added the mutex fix -- great to solve it at the root cause instead of increasing timeouts for race conditions! I didn't review the code in depth to check if mutexes are the most optimal solution here yet though.

Unit tests are now failing on pod cleanup as well. The test is getting a nil pointer error now from the logs, so I imagine the test needs to be altered

[sakai-ast]

The tests seem alright after dealing with the issues, so I have fixed the PR title and description.

[juliev0]

nice remembering to remove it from memory

[juliev0]

I think this is probably good.

I actually just did an analysis of this type of thing on another repo. As you noticed, the underlying issue is multiple goroutines at the same time doing a "Get", then a "Patch" based on making some calculation from the "Get" (in this case determining if there was a finalizer) - if one does a "Get" and then makes a calculation, then the live state may have changed in the meantime and that calculation may no longer be valid.

I determined that there were 2 options: 1. put a lock around the whole thing like you are now; 2. Use an Update() (PUT) instead of a Patch. Doing a Get and then modifying the Resource, and then doing an Update() on that Resource means that there's a ResourceVersion embedded in the object, and the K8S API will notice if the live ResourceVersion has in the meantime changed, so that the whole Get()->calculation->Update() can be retried again.

To be extra cautious, we can also consider anywhere else in the code that these Pods could get updated to make sure any places that can run concurrently with this will work, if any.

[juliev0]

To be extra cautious, we can also consider anywhere else in the code that these Pods could get updated to make sure any places that can run concurrently with this will work, if any.

Okay, I just searched for Pods( in the code. The only other place I see a Pod getting updated is in the executor running on the Pod (wait container) where an Annotation can be set. I think probably no issue there really since both of them are touching/looking for totally separate things.

[agilgur5]

if one does a "Get" and then makes a calculation, then the live state may have changed in the meantime and that calculation may no longer be valid.

In other words, a lack of atomicity. So a lock is used to make it more like a transaction in this case.

I determined that there were 2 options: 1. put a lock around the whole thing like you are now; 2. Use an Update() (PUT) instead of a Patch.

I'm wondering if an update might be better. The mutex lock ensures that the Controller's goroutines won't overwrite each other (i.e. make it thread-safe), but not that something else outside of Argo doesn't overwrite the Pod. It sounds like, per your analysis, that the semantics of update are safer?

[juliev0]

I'm wondering if an update might be better. The mutex lock ensures that the Controller's goroutines won't overwrite each other (i.e. make it thread-safe), but not that something else outside of Argo doesn't overwrite the Pod. It sounds like, per your analysis, that the semantics of update are safer?

Looking at the code more closely, it looks like we get the live state, then determine if this particular Finalizer is in there and get its Index. I suppose there is a very small risk of somebody else maybe inserting their own Finalizer or removing one, such that we would remove the wrong Finalizer? Is it a realistic thing to be worried about do you think?

[agilgur5]

I suppose there is a very small risk of somebody else maybe inserting their own Finalizer or removing one, such that we would remove the wrong Finalizer? Is it a realistic thing to be worried about do you think?

It's an edge case of an edge case, but I do think it is quite possible, especially for a fundamental resource like Pods that could have various admission controllers etc apply to it.

[juliev0]

I suppose there is a very small risk of somebody else maybe inserting their own Finalizer or removing one, such that we would remove the wrong Finalizer? Is it a realistic thing to be worried about do you think?

It's an edge case of an edge case, but I do think it is quite possible, especially for a fundamental resource like Pods that could have various admission controllers etc apply to it.

What do you think @sakai-ast ? If we were to write an issue related to this, would you be open to converting the logic to use Update() rather than Patch(), with Retry capability in the case that the K8S API rejects the request due to ResourceVersion being outdated?

[sakai-ast]

I suppose there is a very small risk of somebody else maybe inserting their own Finalizer or removing one, such that we would remove the wrong Finalizer? Is it a realistic thing to be worried about do you think?

It's an edge case of an edge case, but I do think it is quite possible, especially for a fundamental resource like Pods that could have various admission controllers etc apply to it.

What do you think @sakai-ast ? If we were to write an issue related to this, would you be open to converting the logic to use Update() rather than Patch(), with Retry capability in the case that the K8S API rejects the request due to ResourceVersion being outdated?

I'm open to it and appreciate the suggestion. I have tried the following implementation using Update() with retry logic. Does this meet your expectations?
#12632

[juliev0]

I'm open to it and appreciate the suggestion. I have tried the following implementation using Update() with retry logic. Does this meet your expectations? #12632

very nice! yes, thanks :) I'll assign that one to myself and review more in depth later.