Introduce process_count_max minion configuration parameter (develop) #43669
Conversation
|
@moio, thanks for your PR! By analyzing the history of the files in this pull request, we identified @s0undt3ch, @jacksontj and @cachedout to be potential reviewers. |
moio
force-pushed
the
develop-limit-minion-processes
branch
2 times, most recently
from
1f73d69 to
84daaf2
Compare
salt/minion.py
Outdated
There was a problem hiding this comment.
I have some real concerns here. This is perhaps the most performance-sensitive area of the minion and I really don't like introducing more disk hits and de-serialization overhead.
I would feel much more comfortable if this configuration option were disabled by default but I'll also let @thatch45 and others comment here.
There was a problem hiding this comment.
My reasoning went along the lines that hitting the disk (I believe you are referring to salt.utils.minion.running here) is certainly less expensive than spawning a process. At least that is what I empirically observed in my setup - going above ~100 concurrent processes becomes intractable quickly.
That said I was also wondering whether it would be wiser to disable by default (retaining the current behavior) or implement the safety net by default, which is ultimately what I chose. In any case I am absolutely open to change course if you think it's more appropriate.
Thanks for looking into this @cachedout!
There was a problem hiding this comment.
My concern is just that we incur that overhead even in cases where we'll never be concerned about hitting an upper limit on processes. We go to great pains to avoid that. The only way I think I'll feel comfortable here is if this is disabled by default, I think.
moio
force-pushed
the
develop-limit-minion-processes
branch
from
84daaf2 to
b81069b
Compare
|
@cachedout: I added a completely optional, self contained commit on top that disables the mechanism by default. I did not find significant performance impacts for my typical use cases but I am totally fine if you judge it might not be OK in general - in that case simply include this commit, otherwise leave it out from the merge. Thanks for your time |
doc/man/salt.7
Outdated
There was a problem hiding this comment.
Please remove all the changes to the man page. They are compiled separately and shouldn't be updated directly.
doc/ref/configuration/minion.rst
Outdated
There was a problem hiding this comment.
This needs to be updated to -1
doc/ref/configuration/minion.rst
Outdated
There was a problem hiding this comment.
This needs to be updated to -1 also.
salt/config/__init__.py
Outdated
There was a problem hiding this comment.
Thanks. :] I appreciate you being willing to change this.
This allows users to limit the number of processes or threads a minion will start in response to published messages, prevents resource exhaustion in case a high number of concurrent jobs is scheduled in a short time.
moio
force-pushed
the
develop-limit-minion-processes
branch
from
b81069b to
fd4194a
Compare
|
All fixed! Jenkins failures do not seem related to me. |
3 participants
What does this PR do?
This adds a minion configuration parameter to limit the number of processes or threads a minion will start in response to published messages, preventing resource exhaustion in case a high number of concurrent jobs is scheduled in a short time.
What issues does this PR fix or reference?
#43665
Previous Behavior
salt-minioncould consume significant resources if a large number of functions calls is scheduled in parallel, as it would spawn one process for each.New Behavior
By default, nothing changes. If
process_count_maxis specified,salt-minionwill not spawn more than a certain amount of processes/threads.Tests written?
One new unit test was added and existing tests were updated.