Skip to content
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

fix: ensure namespace parallelism and parallelism work together. Fixes #10985 #14039

Merged
merged 22 commits into from
Jan 9, 2025
Merged

fix: ensure namespace parallelism and parallelism work together. Fixes #10985 #14039

merged 22 commits into from
Jan 9, 2025
+451 −434

Conversation

isubasinghe
Copy link
Member

@isubasinghe isubasinghe commented Dec 30, 2024 •
edited
Loading

Fixes #10985

Motivation

The current throttler code does not work when used in unison with namespace parallelism and parallelism. This is because the concept of the chain throttler unfortunately doesn't work.

Modifications

Corrected a bug where workflows weren't added into the running list/map when the parallelism was set to 0.
Added tracking for namespace counts into a new throttler.

Verification

Relied on tests to pass

@isubasinghe
fix: make throttler understand both namespace and global parallelism
Loading
Loading status checks…
76b839c 
Signed-off-by: isubasinghe <isitha@pipekit.io>
@isubasinghe
fix: ensure tests pass
Loading
Loading status checks…
9f6bb24 
Signed-off-by: isubasinghe <isitha@pipekit.io>
@isubasinghe isubasinghe changed the title Fix namespace parallelism. Fixes #10985 fix: ensure namespace parallelism and parallelism work together. Fixes #10985 Dec 30, 2024
@isubasinghe
fix: removed redundant return
Loading
Loading status checks…
7e4e245 
Signed-off-by: isubasinghe <isitha@pipekit.io>
@isubasinghe
fix: test
Loading
Loading status checks…
14599e0 
Signed-off-by: isubasinghe <isitha@pipekit.io>
@isubasinghe
fix: linted file
343caf3 
Signed-off-by: isubasinghe <isitha@pipekit.io>
@isubasinghe
fix: increase namespace parallelism limits
f82477e 
Signed-off-by: isubasinghe <isitha@pipekit.io>
@isubasinghe
fix: remove from pending when called
4ef7f07 
Signed-off-by: isubasinghe <isitha@pipekit.io>
@isubasinghe
fix: derive namespace counts
ceed4d5 
Signed-off-by: isubasinghe <isitha@pipekit.io>
@isubasinghe
fix: linted file
2d438c0 
Signed-off-by: isubasinghe <isitha@pipekit.io>
@isubasinghe
fix: removed unused logging
6edcfb4 
Signed-off-by: isubasinghe <isitha@pipekit.io>
@isubasinghe
fix: removed logging
c749ad0 
Signed-off-by: isubasinghe <isitha@pipekit.io>
@isubasinghe
Copy link
Member Author

/retest

@isubasinghe
fix: removed unnecessary struct
9204838 
Signed-off-by: isubasinghe <isitha@pipekit.io>
@isubasinghe
Copy link
Member Author

/retest

@Joibel Joibel self-assigned this Jan 2, 2025
@Joibel Joibel added area/controller Controller issues, panics area/parallelism `parallelism` for the Controller, Workflows, or templates labels Jan 2, 2025
@isubasinghe
Copy link
Member Author

/retest

2 similar comments
@isubasinghe
Copy link
Member Author

/retest

@isubasinghe
Copy link
Member Author

/retest

@isubasinghe isubasinghe marked this pull request as ready for review January 3, 2025 06:32
@isubasinghe
Copy link
Member Author

/retest

@MasonM
Copy link
Member

MasonM commented Jan 6, 2025

@isubasinghe The E2E tests are broken due to a bug in Kit. I have a fix for it in #14048. Could you please review that PR?

@isubasinghe isubasinghe force-pushed the fix-namespace-parallelism branch from 183177a to 8be3e3c Compare January 6, 2025 02:06
@isubasinghe
Copy link
Member Author

/retest

@isubasinghe
Copy link
Member Author

@isubasinghe The E2E tests are broken due to a bug in Kit. I have a fix for it in #14048. Could you please review that PR?

Thanks, I will do

@isubasinghe
fix: added printing for debug purposes
8d21515 
Signed-off-by: isubasinghe <isitha@pipekit.io>
@isubasinghe
fix: simplified queueThrottled
ef494d6 
Signed-off-by: isubasinghe <isitha@pipekit.io>
@isubasinghe
Copy link
Member Author

/retest

Joibel
Joibel requested changes Jan 6, 2025
View reviewed changes
Copy link
Member

@Joibel Joibel left a comment

Choose a reason for hiding this comment

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

Also needs some documentation in docs/parallelism.md to document that when using namespaceParallelism a lower priority workflow in a namespace that has capacity will run before a high priority workflow in a namespace with no capacity.

I assume this is the intentional behaviour - it feels the sane option.

@isubasinghe
fix: address PR feedback
576cd1f 
Signed-off-by: isubasinghe <isitha@pipekit.io>
@isubasinghe
docs: added information about namespace parallelism limits
5baa224 
Signed-off-by: isubasinghe <isitha@pipekit.io>
@isubasinghe
docs: linted file
9330773 
Signed-off-by: isubasinghe <isitha@pipekit.io>
@isubasinghe
Copy link
Member Author

/retest

1 similar comment
@isubasinghe
Copy link
Member Author

/retest

@isubasinghe
Copy link
Member Author

/retest

@isubasinghe
Copy link
Member Author

The test have gotten flakier, unsure if this contributed or something else did.

@isubasinghe
fix: added alans feedback
aaa4c26 
Signed-off-by: isubasinghe <isitha@pipekit.io>
@isubasinghe
Copy link
Member Author

/retest

Joibel
Joibel approved these changes Jan 9, 2025
View reviewed changes
Copy link
Member

@Joibel Joibel left a comment

Choose a reason for hiding this comment

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

LGTM, thank you

@isubasinghe isubasinghe merged commit 09d5ee7 into argoproj:main Jan 9, 2025
31 checks passed
@Joibel Joibel deleted the fix-namespace-parallelism branch January 10, 2025 13:38
@Joibel
Copy link
Member

Joibel commented Jan 27, 2025

This PR seems to be the cause of the SignalsSuite test failures - they almost always fail with this change, and reverting makes them much better.

isubasinghe added a commit to pipekit/argo-workflows that referenced this pull request Jan 30, 2025
@isubasinghe
fix: ensure namespace parallelism and parallelism work together. Fixes …
378601f 
…argoproj#10985 (argoproj#14039)

Signed-off-by: isubasinghe <isitha@pipekit.io>
isubasinghe added a commit to pipekit/argo-workflows that referenced this pull request Jan 30, 2025
@isubasinghe
fix: ensure namespace parallelism and parallelism work together. Fixes …
442362b 
…argoproj#10985 (argoproj#14039)

Signed-off-by: isubasinghe <isitha@pipekit.io>
isubasinghe added a commit to pipekit/argo-workflows that referenced this pull request Jan 31, 2025
@isubasinghe
fix: ensure namespace parallelism and parallelism work together. Fixes …
64c52ff 
…argoproj#10985 (argoproj#14039)

Signed-off-by: isubasinghe <isitha@pipekit.io>
isubasinghe added a commit that referenced this pull request Jan 31, 2025
@isubasinghe
fix: ensure namespace parallelism and parallelism work together. Fixes …
7021548 
…#10985 (#14039)

Signed-off-by: isubasinghe <isitha@pipekit.io>
@Joibel Joibel mentioned this pull request Feb 3, 2025
Open
4 tasks
Joibel pushed a commit that referenced this pull request Feb 7, 2025
@isubasinghe @Joibel
fix: ensure namespace parallelism and parallelism work together. Fixes …
293b4a5 
…#10985 (#14039)

Signed-off-by: isubasinghe <isitha@pipekit.io>
(cherry picked from commit 09d5ee7)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Joibel Joibel

Assignees

@Joibel Joibel

Labels
area/controller Controller issues, panics area/parallelism `parallelism` for the Controller, Workflows, or templates
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Cannot set both parallelism and namespaceParallelism
3 participants
@isubasinghe @MasonM @Joibel