Enable minion's IPC channel to aggregate results from spawned jobber processes #61468
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…processes. Use a long-running request channel in the minion parent process to communicate job results back to the master via broker-based or broker-less transport.
This is a necessary optimization for transports that prefer a sustained long-running connection because connection create/dispose operations are expensive. The working assumption is that this change benefits all supported transports.
Testing Done:
* this tests provide coverage for this use case:
.../salt/tests/pytests/integration/minion.*
|
re-run full all |
jfindlay
reviewed
Jan 19, 2022
|
You might consider also updating https://github.com/saltstack/salt/blob/master/doc/topics/development/architecture.rst#minion-job-flow. |
Thanks. I will update https://github.com/saltstack/salt/blob/master/doc/topics/development/architecture.rst#minion-job-flow shortly. |
|
re-run full all |
…r events intended for the master's ReqServer. Tag these specific events with a specific tag "__master_req_channel_payload" Update the architecture flow accordingly: https://github.com/saltstack/salt/blob/master/doc/topics/development/architecture.rst#minion-job-flow Testing Done: - updated unit tests, - tested manually with zeromq
6 tasks
|
Thanks. I can reproduce the test failure locally. Looks like the fix may need be in the TCP transport itself. I will try to update the PR with the fix. |
dwoz
previously approved these changes
Feb 8, 2022
twangboy
previously approved these changes
Feb 8, 2022
s0undt3ch
previously approved these changes
Feb 9, 2022
|
@devkits Can you please add a changelog for this PR? Thanks! |
dwoz
dismissed stale reviews from s0undt3ch, twangboy, and themself
via
7215701
|
Congratulations on your first PR being merged! 🎉 |
frebib
added a commit
to frebib/salt
that referenced
this pull request
Oct 7, 2022
Since [1] minions now return all returns to all masters, instead of just the master that spawned the job. The upstream change in behaviour overloads our global masters making them unusable, so this aims to revert to the previous behaviour whilst maintaining the single-channel return improvements also introduced in [1]. [1]: saltstack#61468 Upstream-bug: saltstack#62834 Signed-off-by: Joe Groocock <jgroocock@cloudflare.com>
meaksh
added a commit
to openSUSE/salt
that referenced
this pull request
Aug 31, 2023
…sc#1213257) This reverts commits: saltstack/salt@a99ffb5 saltstack/salt@80ae518 saltstack/salt@3c7e1ec saltstack/salt@171926c From this PR: saltstack/salt#61468 See: saltstack/salt#62959 (comment)
Merged
3 tasks
vzhestkov
pushed a commit
to openSUSE/salt
that referenced
this pull request
Sep 1, 2023
…sc#1213257) * Revert usage of long running REQ channel (bsc#1213960, bsc#1213630, bsc#1213257) This reverts commits: saltstack/salt@a99ffb5 saltstack/salt@80ae518 saltstack/salt@3c7e1ec saltstack/salt@171926c From this PR: saltstack/salt#61468 See: saltstack/salt#62959 (comment) * Revert "Fix linter" This reverts commit d09d2d3. * Revert "add a regression test" This reverts commit b2c32be. * Fix failing tests after reverting commits
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Enable minion's IPC channel to aggregate results from spawned jobber processes. Use a long-running request channel in the minion parent process to communicate job results back to the master via broker-based or broker-less transport.
This is a necessary optimization at scale for transports that prefer a sustained long-running connection because connection create/dispose operations are expensive. The working assumption is that this change benefits all supported transports.
Testing Done:
.../salt/tests/pytests/integration/minion.*
Previous Behavior
Minion would spawn a process that would run a job and create/dispose a new transport connection just to communicate one job result. This does not scale well for transports that prefer persistent connections because connection operations are expensive.
New Behavior
Minion would spawn a process that would run a job and communicate result via an IPC channel to the parent minion process. Parent minion process would use a long-running connection to communicate result back to master. This approach scales better because it reduces connection churn when thousands of minions are responding to the master.
Fixes: #61274