Skip to content

fix: correct semaphore configmap keys for multiple semaphores #14184

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Feb 14, 2025
Merged

fix: correct semaphore configmap keys for multiple semaphores #14184

merged 1 commit into from
Feb 14, 2025

Conversation

Joibel
Copy link
Member

@Joibel Joibel commented Feb 11, 2025 •
edited
Loading

No known bug reports around this so far

Motivation

Whilst looking at deprecating single semaphores completely I discovered getSemaphoreConfigMapRef() was not multiple semaphore aware.

Modifications

Rename getSemaphoreConfigMapRef to getSemaphoreConfigMapRefs and return a list of pointers instead of a pointer.

Make the caller of this iterate the list.

Verification

Updated the test case that tests GetSemaphoreKeys which is the only user of getSemaphoreConfigMapRefs to test this and added an extra entry to be sure it can successfully manage a list.

Documentation

Not relevant, internal code

@Joibel Joibel force-pushed the semaphores-configkeys branch from a647d12 to 705bc26 Compare February 12, 2025 09:56
@Joibel Joibel marked this pull request as ready for review February 12, 2025 11:00
@Joibel Joibel requested a review from rohankmr414 February 13, 2025 08:21
rohankmr414
rohankmr414 approved these changes Feb 13, 2025
View reviewed changes
Copy link
Member

@rohankmr414 rohankmr414 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@rohankmr414
Copy link
Member

@Joibel I think we should also update TestWorkflowSemaphoreKeysIndexFunc here: https://github.com/argoproj/argo-workflows/blob/main/workflow/controller/indexes/workflow_index_test.go#L73, the assertions only check for 1 entry which is Synchronization.Semaphore and not Synchronization.Semaphores.

@rohankmr414
Copy link
Member

Also, I'm curious if multiple keys are expected here: https://github.com/argoproj/argo-workflows/blob/main/workflow/controller/controller.go#L271

@Joibel
Copy link
Member Author

Joibel commented Feb 13, 2025

@Joibel I think we should also update TestWorkflowSemaphoreKeysIndexFunc here: https://github.com/argoproj/argo-workflows/blob/main/workflow/controller/indexes/workflow_index_test.go#L73, the assertions only check for 1 entry which is Synchronization.Semaphore and not Synchronization.Semaphores.

I've got a follow up PR which removes Semaphore (as it'd deprecated) which will do this.

@Joibel
Copy link
Member Author

Joibel commented Feb 13, 2025

Also, I'm curious if multiple keys are expected here: https://github.com/argoproj/argo-workflows/blob/main/workflow/controller/controller.go#L271

This index is already multi-key aware, and this fix makes sure all the semaphores get into the index.

@Joibel Joibel mentioned this pull request Feb 13, 2025
Draft
@Joibel Joibel requested a review from isubasinghe February 14, 2025 09:24
@Joibel Joibel merged commit 581950f into argoproj:main Feb 14, 2025
31 checks passed
@Joibel
Copy link
Member Author

Joibel commented Feb 18, 2025

/cherry-pick release-3.6

1 similar comment
@Joibel
Copy link
Member Author

Joibel commented Mar 6, 2025

/cherry-pick release-3.6

Joibel added a commit that referenced this pull request Mar 10, 2025
@Joibel
fix: correct semaphore configmap keys for multiple semaphores (#14184)
6453971 
Signed-off-by: Alan Clucas <[email protected]>
(cherry picked from commit 581950f)
Joibel added a commit that referenced this pull request Mar 10, 2025
@Joibel
fix: correct semaphore configmap keys for multiple semaphores (#14184)
0210e69 
Signed-off-by: Alan Clucas <[email protected]>
(cherry picked from commit 581950f)
@Joibel Joibel mentioned this pull request Mar 10, 2025
Open
Joibel added a commit that referenced this pull request Mar 11, 2025
@Joibel
fix: correct semaphore configmap keys for multiple semaphores (#14184)…
0a0ef61 
… (release-3.6) (#14281)
kim-codefresh added a commit to codefresh-io/argo-workflows that referenced this pull request May 20, 2025
@kim-codefresh @Joibel
@gcp-cherry-pick-bot @tico24 @rogpeppe @MasonM @vnzongzna @dependabot @rohankmr414 @nixphix @jumpe1 @ryancurrah @Adrien-D @blkperl @isubasinghe @jswxstw
feat: sync release 3.6 to upstream 3.6.7 for small changes and vulner…
74e1203 
…abilities fixes (Cr 28355) (#358)

* fix: bump deps for k8schain to fix ecr-login (argoproj#14008) (release-3.6 cherry-pick) (argoproj#14174)

* fix(ci): python sdk release process (release-3.6) (argoproj#14183)

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

* docs: clarify qps/burst on controller (cherry-pick argoproj#14190) (argoproj#14192)

Signed-off-by: Tim Collins <[email protected]>
Co-authored-by: Tim Collins <[email protected]>

* fix(api/jsonschema): use unchanging JSON Schema version (cherry-pick argoproj#14092) (argoproj#14256)

Signed-off-by: Roger Peppe <[email protected]>
Co-authored-by: Roger Peppe <[email protected]>

* fix(api/jsonschema): use working `$id` (cherry-pick argoproj#14257) (argoproj#14258)

Signed-off-by: Roger Peppe <[email protected]>
Co-authored-by: Roger Peppe <[email protected]>

* docs: autogenerate tested k8s versions and centralize config (argoproj#14176) (release-3.6) (argoproj#14262)

Signed-off-by: Mason Malone <[email protected]>
Signed-off-by: Alan Clucas <[email protected]>
Co-authored-by: Mason Malone <[email protected]>

* chore(deps): bump minio-go to newer version (argoproj#14185) (release-3.6) (argoproj#14261)

Co-authored-by: Vaibhav Kaushik <[email protected]>

* fix: split pod controller from workflow controller (argoproj#14129) (release-3.6) (argoproj#14263)

* chore(deps): fix snyk (argoproj#14264) (release-3.6) (argoproj#14268)

* chore: revert to correct k8s versions

Accidental bump from argoproj#14176 cherry-pick

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

* chore(deps): bump github.com/go-jose/go-jose/v3 from 3.0.3 to 3.0.4 in the go_modules group (cherry-pick argoproj#14231) (argoproj#14269)

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* fix: wait for workflow informer to sync before pod informer (cherry-pick argoproj#14248) (argoproj#14266)

Signed-off-by: Rohan K <[email protected]>
Co-authored-by: Rohan K <[email protected]>

* fix(cli): remove red from log colour selection. Fixes argoproj#6740 (cherry-pick argoproj#14215) (argoproj#14278)

Signed-off-by: Prabakaran Kumaresshan <[email protected]>
Co-authored-by: Prabakaran Kumaresshan <[email protected]>

* fix: correct semaphore configmap keys for multiple semaphores (argoproj#14184) (release-3.6) (argoproj#14281)

* fix: don't print help for non-validation errors. Fixes argoproj#14234 (cherry-pick argoproj#14249) (argoproj#14283)

Signed-off-by: Koichi Shimada <[email protected]>
Signed-off-by: Mason Malone <[email protected]>
Co-authored-by: koichi <[email protected]>
Co-authored-by: Mason Malone <[email protected]>

* docs: fix kubernetes versions (release-3.6) (argoproj#14273)

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

* fix(workflow/sync): use RWMutex to prevent concurrent map access (cherry-pick argoproj#14321) (argoproj#14322)

Signed-off-by: Ryan Currah <[email protected]>
Co-authored-by: Ryan Currah <[email protected]>

* chore(lint): update golangci-lint to 2.1.1 (argoproj#14390) (cherry-pick release-3.6) (argoproj#14417)

* chore: bump golang 1.23->1.24 (argoproj#14385) (cherry-pick release-3.6) (argoproj#14418)

* fix: gracefully handle invalid CronWorkflows and simplify logic.  (cherry-pick argoproj#14197) (argoproj#14419)

Signed-off-by: Mason Malone <[email protected]>

* fix: prevent dfs sorter infinite recursion on cycle. Fixes argoproj#13395 (cherry-pick argoproj#14391) (argoproj#14420)

Signed-off-by: Adrien Delannoy <[email protected]>
Co-authored-by: Adrien Delannoy <[email protected]>

* chore(deps): bump github.com/expr-lang/expr from 1.16.9 to 1.17.0 (argoproj#14307) (release-3.6) (argoproj#14421)

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* chore(deps)!: update k8s and argo-events (release-3.6) (argoproj#14424)

Signed-off-by: dependabot[bot] <[email protected]>
Signed-off-by: william.vanhevelingen <[email protected]>
Signed-off-by: Mason Malone <[email protected]>
Signed-off-by: William Van Hevelingen <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: William Van Hevelingen <[email protected]>
Co-authored-by: Mason Malone <[email protected]>

* fix: correct retry logic (argoproj#13734) (release-3.6) (argoproj#14428)

Signed-off-by: isubasinghe <[email protected]>
Signed-off-by: Alan Clucas <[email protected]>
Co-authored-by: Isitha Subasinghe <[email protected]>

* fix: manual retries exit handler cleanup. Fixes argoproj#14180 (argoproj#14181) (release-3.6) (argoproj#14429)

Signed-off-by: isubasinghe <[email protected]>
Signed-off-by: Alan Clucas <[email protected]>
Co-authored-by: Isitha Subasinghe <[email protected]>

* fix: correct manual retry logic. Fixes argoproj#14124 (argoproj#14328) (release-3.6) (argoproj#14430)

Signed-off-by: oninowang <[email protected]>
Signed-off-by: Alan Clucas <[email protected]>
Co-authored-by: jswxstw <[email protected]>

* fix: disable ALPN in argo-server as a workaround (argoproj#14433)

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

* result of codegen

Signed-off-by: Kim <[email protected]>

* fix:lint

Signed-off-by: Kim <[email protected]>

---------

Signed-off-by: Alan Clucas <[email protected]>
Signed-off-by: Tim Collins <[email protected]>
Signed-off-by: Roger Peppe <[email protected]>
Signed-off-by: Mason Malone <[email protected]>
Signed-off-by: dependabot[bot] <[email protected]>
Signed-off-by: Rohan K <[email protected]>
Signed-off-by: Prabakaran Kumaresshan <[email protected]>
Signed-off-by: Koichi Shimada <[email protected]>
Signed-off-by: Ryan Currah <[email protected]>
Signed-off-by: Adrien Delannoy <[email protected]>
Signed-off-by: william.vanhevelingen <[email protected]>
Signed-off-by: William Van Hevelingen <[email protected]>
Signed-off-by: isubasinghe <[email protected]>
Signed-off-by: oninowang <[email protected]>
Signed-off-by: Kim <[email protected]>
Co-authored-by: Alan Clucas <[email protected]>
Co-authored-by: gcp-cherry-pick-bot[bot] <98988430+gcp-cherry-pick-bot[bot]@users.noreply.github.com>
Co-authored-by: Tim Collins <[email protected]>
Co-authored-by: Roger Peppe <[email protected]>
Co-authored-by: Mason Malone <[email protected]>
Co-authored-by: Vaibhav Kaushik <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Rohan K <[email protected]>
Co-authored-by: Prabakaran Kumaresshan <[email protected]>
Co-authored-by: koichi <[email protected]>
Co-authored-by: Ryan Currah <[email protected]>
Co-authored-by: Adrien Delannoy <[email protected]>
Co-authored-by: William Van Hevelingen <[email protected]>
Co-authored-by: Isitha Subasinghe <[email protected]>
Co-authored-by: jswxstw <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@isubasinghe isubasinghe isubasinghe approved these changes

@rohankmr414 rohankmr414 rohankmr414 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Joibel @rohankmr414 @isubasinghe