feat: allow cross-namespace locking for semaphore and mutex #11096

woehrl01 · 2023-05-17T12:38:08Z

This Pull Request introduces the functionality to specify a namespace for the semaphore configmap references, allowing for locking on shared resources across namespaces.

Motivation

Enhancing the semaphore configuration with a namespace property enables better resource management in environments with shared resources across multiple namespaces.

Modifications

Introduced a namespace property to the semaphore configuration. This property is designed to specify the namespace in which the semaphore configmap resides.

Design choices:

The namespace is not directly added to the ConfigMapKeyRef in an attempt to avoid violating the official definition and to prevent potential misunderstandings. This design decision was made to keep the definition of ConfigMapKeyRef as standard and clear as possible.

Verification

Added a unit test

woehrl01 · 2023-05-25T15:41:11Z

@terrytangyuan @jessesuen @alexec highly appreciate your feedback on this. Is there anything missing, to have this merged? Thanks!

terrytangyuan · 2023-05-25T18:20:14Z

cc @sarabala1979 @isubasinghe

isubasinghe

Looks good to me. I think this PR should also implement this support for mutexes though.
It would be somewhat odd if semaphores supported this while mutexes didn't.

isubasinghe · 2023-05-28T07:09:33Z

The tests pass for me locally, might just be flaky tests, give it a re-run

woehrl01 · 2023-05-28T09:06:50Z

Thanks for your feedback @isubasinghe I just added the namespace for mutex, too.

isubasinghe

No worries @woehrl01, I am just undecided if "Namespace is the namespace of the configmap ..." is okay for the mutex.
I mean technically the mutex is backed by a configmap so it is technically correct.

I'm going to approve and leave that to the discretion of yourself and @terrytangyuan

woehrl01 · 2023-05-30T07:57:13Z

Yes, you're right, I rephrase the comment. Thank you!

isubasinghe · 2023-05-30T08:11:30Z

docs/executor_swagger.md

@@ -2468,6 +2468,7 @@ than the MaxAge, it will be ignored. |  |
 | Name | Type | Go type | Required | Default | Description | Example |
 |------|------|---------|:--------:| ------- |-------------|---------|
 | name | string| `string` |  | | name of the mutex |  |
+| namespace | string| `string` |  | `"namespace of workflow)"`|  |  |


fix default here

isubasinghe · 2023-05-30T08:12:06Z

docs/executor_swagger.md

@@ -4229,6 +4230,7 @@ Note that this field cannot be set when spec.os.name is windows.
 | Name | Type | Go type | Required | Default | Description | Example |
 |------|------|---------|:--------:| ------- |-------------|---------|
 | configMapKeyRef | [ConfigMapKeySelector](#config-map-key-selector)| `ConfigMapKeySelector` |  | |  |  |
+| namespace | string| `string` |  | `"namespace of workflow)"`|  |  |


also here, there is a right paren and its inside `` and "".
Should just be:
namespace of workflow

isubasinghe

typo in docs, I made two comments

isubasinghe

Apoligies: meant to request changes not comment, see previous comment.

Signed-off-by: Lukas Wöhrl <lukas.woehrl@plentymarkets.com>

woehrl01 · 2023-05-30T09:55:00Z

Just renamed the default again and put it in brackets to be more visible of a "variable" argument. Unfortunately, I can't remove the quotes, they are added automatically. Alternative would be to put the default into the description, but I'm not a fan of that either.

isubasinghe · 2023-05-30T10:02:29Z

Thanks for the changes @woehrl01

isubasinghe

LGTM

terrytangyuan

This would be great enhancement. Thanks!

…#11096)

…#11096) Signed-off-by: Jeremy Hager <47301461+jeremyhager@users.noreply.github.com>

…#11096) Signed-off-by: Lukas Wöhrl <lukas.woehrl@plentymarkets.com>

woehrl01 force-pushed the feat_crossnamespace_semaphore branch from b17f284 to 5d79538 Compare May 17, 2023 12:43

woehrl01 changed the title ~~feat: allow crossnamespace locking via sempahores~~ feat: allow crossnamespace locking via semaphores May 17, 2023

woehrl01 force-pushed the feat_crossnamespace_semaphore branch from 5d79538 to 13feff6 Compare May 18, 2023 19:46

woehrl01 marked this pull request as ready for review May 18, 2023 20:25

woehrl01 requested review from jessesuen and alexec as code owners May 18, 2023 20:25

woehrl01 force-pushed the feat_crossnamespace_semaphore branch 2 times, most recently from a82df3e to ab7b89a Compare May 19, 2023 06:47

woehrl01 changed the title ~~feat: allow crossnamespace locking via semaphores~~ feat: allow cross-namespace locking via semaphores May 26, 2023

isubasinghe reviewed May 28, 2023

View reviewed changes

woehrl01 force-pushed the feat_crossnamespace_semaphore branch from ab7b89a to abb4f0b Compare May 28, 2023 09:05

woehrl01 changed the title ~~feat: allow cross-namespace locking via semaphores~~ feat: allow cross-namespace locking for semaphore and mutex May 28, 2023

isubasinghe approved these changes May 30, 2023

View reviewed changes

woehrl01 force-pushed the feat_crossnamespace_semaphore branch from abb4f0b to 4e526ea Compare May 30, 2023 08:05

isubasinghe reviewed May 30, 2023

View reviewed changes

isubasinghe requested changes May 30, 2023

View reviewed changes

feat: allow cross-namespace locking for sempahore and mutex

a2038a7

Signed-off-by: Lukas Wöhrl <lukas.woehrl@plentymarkets.com>

woehrl01 force-pushed the feat_crossnamespace_semaphore branch from 4e526ea to a2038a7 Compare May 30, 2023 09:53

isubasinghe approved these changes May 30, 2023

View reviewed changes

terrytangyuan approved these changes May 31, 2023

View reviewed changes

terrytangyuan merged commit c9ebf42 into argoproj:master May 31, 2023

woehrl01 deleted the feat_crossnamespace_semaphore branch May 31, 2023 21:34

JPZ13 pushed a commit to pipekit/argo-workflows that referenced this pull request Jul 4, 2023

feat: allow cross-namespace locking for semaphore and mutex (argoproj…

c701bde

…#11096)

jeremyhager pushed a commit to jeremyhager/argo-workflows that referenced this pull request Jul 7, 2023

feat: allow cross-namespace locking for semaphore and mutex (argoproj…

7caa9e4

…#11096) Signed-off-by: Jeremy Hager <47301461+jeremyhager@users.noreply.github.com>

woehrl01 added a commit to woehrl01/argo-workflows that referenced this pull request Jul 20, 2023

feat: allow cross-namespace locking for semaphore and mutex (argoproj…

0ec226e

…#11096) Signed-off-by: Lukas Wöhrl <lukas.woehrl@plentymarkets.com>

terrytangyuan mentioned this pull request Aug 2, 2023

Release v3.5 preparation #11381

Closed

isubasinghe mentioned this pull request Jan 30, 2024

REQUEST: Promotion to Approver for @isubasinghe argoproj/argoproj#232

Closed

7 tasks

agilgur5 added the area/mutex-semaphore label May 4, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: allow cross-namespace locking for semaphore and mutex #11096

feat: allow cross-namespace locking for semaphore and mutex #11096

woehrl01 commented May 17, 2023 •

edited

Loading

woehrl01 commented May 25, 2023

terrytangyuan commented May 25, 2023

isubasinghe left a comment

isubasinghe commented May 28, 2023

woehrl01 commented May 28, 2023

isubasinghe left a comment

woehrl01 commented May 30, 2023

isubasinghe May 30, 2023

isubasinghe May 30, 2023 •

edited

Loading

isubasinghe left a comment

isubasinghe left a comment

woehrl01 commented May 30, 2023

isubasinghe commented May 30, 2023

isubasinghe left a comment

terrytangyuan left a comment

feat: allow cross-namespace locking for semaphore and mutex #11096

feat: allow cross-namespace locking for semaphore and mutex #11096

Conversation

woehrl01 commented May 17, 2023 • edited Loading

Motivation

Modifications

Design choices:

Verification

woehrl01 commented May 25, 2023

terrytangyuan commented May 25, 2023

isubasinghe left a comment

Choose a reason for hiding this comment

isubasinghe commented May 28, 2023

woehrl01 commented May 28, 2023

isubasinghe left a comment

Choose a reason for hiding this comment

woehrl01 commented May 30, 2023

isubasinghe May 30, 2023

Choose a reason for hiding this comment

isubasinghe May 30, 2023 • edited Loading

Choose a reason for hiding this comment

isubasinghe left a comment

Choose a reason for hiding this comment

isubasinghe left a comment

Choose a reason for hiding this comment

woehrl01 commented May 30, 2023

isubasinghe commented May 30, 2023

isubasinghe left a comment

Choose a reason for hiding this comment

terrytangyuan left a comment

Choose a reason for hiding this comment

woehrl01 commented May 17, 2023 •

edited

Loading

isubasinghe May 30, 2023 •

edited

Loading