feat: multiple mutexes and semaphores

[Joibel]

Fixes #5022
Fixes #11859

This is mostly backwards compatible. It is minorly breaking only in as much as semaphore + mutex at the same time will now actually take both, whereas before it ignored the mutex.

Motivation

As a workflows users I would like to be able to use more than one sync option in a workflow or template.

Modifications

Added mutexes and semaphores to the synchronization block in workflow types as lists. Legacy mutex and semaphore kept in and continue to work (will be added to the mutexes/semaphores list) but marked for deprecation.

Logic changed to acquire a full list of all synchronization items (syncItem) when TryAcquire and Release from sync manager.

TryAcquire from sync_manager calls a new checkAcquire method, whilst holding the sync_manager internal golang sync.Mutex to check if they are all acquirable before acquiring them. This honors priority.

TryAcquire also returns the failed lock name in the case of failure to acquire to reduce the number of exported package sync methods and complexity outside of sync.

The interface to the workflow status block has not changed.

Reduced the exported types and methods from the sync module as far as possible (lower case instead of upper case).

Removed the internal golang sync mutex from PrioritySemaphore as all methods are called from sync_manager.go when under it's own lock. The alternative was unnecessarily complicating the calls inside semaphore.go. This is faster, but less clean in case someone wants to reuse the PrioritySemaphore type.

Renamed the manager in sync_manager.go where used: cm->sm and concurrencyManager->syncManager.

Synchronization documentation updated -- EDIT: split to #13393

Verification

New and updated unit tests.
New and updated e2e smoke tests.

Stacked PR

This is stacked on top of #13393 now.

Signed-off-by: Alan Clucas [email protected]

[terrytangyuan]

@jessesuen Would you like to review this as well since you might be relying on the existing behavior of the synchronization feature?

[Joibel]

@jessesuen Would you like to review this as well since you might be relying on the existing behavior of the synchronization feature?

It shouldn't be a breaking change for existing users.

[agilgur5]

@Joibel could you please re-format your PR descriptions? I think they're line-breaking at 80 chars or something, which depending on the width of the GH description, can sometimes cause a double line-break or otherwise look weird. It also makes quoting look strange too, as other comments are not at that line width. I manually re-formatted a few of your other PRs, but this one is quite long

I can review this as I had worked briefly on these, but I'm not promising any timeline as I already have too many things on my plate (including all the metrics PRs, and docs/UI/Server and some others are often falling on my plate too 😕 most features have docs portions too...)
EDIT: This one is substantially smaller, and I have familiarity with the exact feature and portion of the codebase, and so I can get to it before all the metrics ones. I'm just going to push those further back then.

[agilgur5]

This is mostly backwards compatible. It is minorly breaking only in as much as semaphore + mutex at the same time will now actually take both, whereas before it ignored the mutex.

Are we considering this breaking? The silent ignore is a pure bug to me, where the user intent was to do both, but you'd find out after that didn't happen.

I was actually thinking that would be its own fix PR that would be backported to older versions, whereas the feature to have multiple ofc would not be backported

[agilgur5]

Version specifiers are needed everywhere. Also the parallelism doc should be split into its own PR as it is very backport worthy.

[Joibel]

This is mostly backwards compatible. It is minorly breaking only in as much as semaphore + mutex at the same time will now actually take both, whereas before it ignored the mutex.

Are we considering this breaking? The silent ignore is a pure bug to me, where the user intent was to do both, but you'd find out after that didn't happen.

I was actually thinking that would be its own fix PR that would be backported to older versions, whereas the feature to have multiple ofc would not be backported

These are intertwined features in my implementation. One semaphore and one mutex is just a special case of multiple synchronization items. The bulk of this PR is necessary to make that case work, and I'd consider it too invasive to backport.

I consider the Alternatively wording sufficient to say it was not supposed to work before, hence should not be backported.

argo-workflows/docs/synchronization.md

Line 11 in c16c5e4

from a workflow or template within a workflow. Alternatively, users can

I'm not fussed whether we call this breaking though.

[agilgur5]

The bulk of this PR is necessary to make that case work, and I'd consider it too invasive to backport.

Gotcha. Yea when I examined the issue it could not be done easily either (whereas multi-semaphore was more straightforward).

In that case, I am wondering whether we should have a separate "fix" to backport that errors out when both are provided. Since this feature will only land in 3.6 and the silent ignore in earlier versions is very confusing/surprising as it stands

[Joibel]

In that case, I am wondering whether we should have a separate "fix" to backport that errors out when both are provided. Since this feature will only land in 3.6 and the silent ignore in earlier versions is very confusing/surprising as it stands

Yes, this would be nice if it got done.

[JPZ13]

Hey @agilgur5 - have @Joibel's additional commits addressed your change request? We need to get this one moving for a customer

[sarabala1979]

I had discuss with @Joibel. I liked the Approach and approved. @agilgur5 is aready reviewing the code indetail. Please approved it once comments are address

Dismissing my review to unblock this. We can iterate on the docs later

[agilgur5]

I haven't been feeling great the past week, so dismissed my review to unblock this. Can iterate more on docs later as I do have some thoughts on them, but haven't quite had the brainpower to review properly

[isubasinghe]

/retest

[isubasinghe]

/retest

[agilgur5]

I haven't been feeling great the past week, so dismissed my review to unblock this. Can iterate more on docs later as I do have some thoughts on them, but haven't quite had the brainpower to review properly

I wrote up a follow-up docs PR in #13645 which also addresses the immediate user confusion with versions per #13644 et al

(I also suddenly started feeling much better on Friday, hence all my contributions this weekend. maybe the new meds have kicked in 🤷 🤞)

[agilgur5]

In that case, I am wondering whether we should have a separate "fix" to backport that errors out when both are provided. Since this feature will only land in 3.6 and the silent ignore in earlier versions is very confusing/surprising as it stands

Yes, this would be nice if it got done.

I have a WIP PR for this in #13669. Might want someone here to take that over as some of the logic would be desirable to transfer to 3.6+ as well. It would also be a good place to add deprecation warnings in the logs in 3.6+