add niceness control for subprocesses of salt master #50905
Conversation
mattp-
force-pushed
the
niceness
branch
5 times, most recently
from
0bcebb6
to
86eca7f
Compare
|
Way cool! We'll definitely need docs written before we can get this in, though. |
mattp-
force-pushed
the
niceness
branch
2 times, most recently
from
96e767c
to
a3aaeea
Compare
|
@cachedout working on it, i'll ping ... not you :) (its likely not going to be by eod tomorrow) when tests and docs have this ready to go. thanks |
mattp-
changed the title
mattp-
force-pushed
the
niceness
branch
2 times, most recently
from
e090b10
to
11fa1fc
Compare
mattp-
force-pushed
the
niceness
branch
2 times, most recently
from
fa44239
to
c3b6fc1
Compare
|
Yes, I really like this addition, a great idea! Once we have docs in for this it should be good to merge! |
mattp-
force-pushed
the
niceness
branch
2 times, most recently
from
aa066ff
to
1ab37cc
Compare
|
for completeness, do we see any value in add niceness tuning for fileserverupdate, maintenance, etc? |
|
Personally I'd like to understand the driver for this better. Specifically which processes are causing the high load and why? Generally, I think of niceness as something that should be set overall for a program by something outside of the said program (not by the program itself). |
|
@dwoz Since we're forking our own procs and those processes have different roles, I think there is a good argument for the application to take control over setting nice values appropriately per-process. |
|
@mattp, @dwoz raises a valid point to further the discussion and we would like to put this through our RFC process. Can you please look at this template and submit an RFC PR? https://github.com/saltstack/salt/blob/develop/rfcs/0000-template.md |
|
@sagetherage sure I can try to get that together. but basically @dwoz to answer your question any orchestration heavy workload is going to cause cpu contention on the master. introduce orchestrations that call orchestrations, it adds up quickly. what we've seen is the ipcmessageserver and publisher was falling over/significantly falling behind on loads that are on the order of 30-40x core count of the server. using nice to bump priority for it was a large boon in performance under this load. |
|
@mattp Thanks for the explanation. I think this is an easy and immediate fix for the issue you have described. Longer term, I think we should try to determine what is causing the load. Then look into ways of mitigating the problem. Perhaps we could batch the orchestrations or otherwise optimize the process so it will play nice on it's own. My concern for having salt set it's own nice settings via the config file is that it seems like a non-standard way for system administrators to set niceness for a process. It'd be nice if we could separate and name processes accordingly so that this could be done outside of salt in the standard way for the specific os. This would be Overall, I'm not opposed to this getting merged. However, I think we could discuss and possibly implement some of the solutions above which would allow us to remove these setting in the future. We can discuss all this more in the RFC if you make one. |
|
While I agree that limits.conf and doing this from a systems level makes sense, I also think that having this in here is a good idea. Since salt is spinning up so many procs I think it will be wise to allow users to set this up in this way |
|
Any movement on this? FWIW, this code has been deployed in a production setting and working ... nicely. |
|
Did the ability to nice the maintenance proc get in here? IMHO, it would be great to be able to dial back the priority of that proc in certain cases. |
|
@cachedout good catch I did not add this to the maintenance proc - will take care of that too |
|
@mattp- What is the status of this? |
|
@dwoz i think the last standing issue was putting together an RFC which I've not had time to do. the original goal of this was to let the mworkers run under a lower priority to not starve the other processes of the salt-master - I don't think you can use limits.conf to do this externally (if you can, I'd happily close this and move to that). if we are ok with foregoing the RFC I can get the maintenance added as a switch. what do you think? |
|
@mattp- Based on the conversation above, I am fine with this provided it's an optional setting. From the looks of the changes it is indeed optional. I think all that is needed is to add the option for the maintenance process. |
|
FYI I migrated this PR from develop to neon to ensure it is included in the upcoming neon release. Let me know if this caused any issues. Thanks |
|
@dwoz added some docs, the opt for maintenance, as well as mworker_queue which I also missed. thanks |
under high load these can be useful to tweak for ensuring critical parts of salt aren't being resource starved.
|
rebased on current neon |
|
@mattp- Can you rebase this against the |
garethgreenaway
added
ZRelease-Sodium
|
I rebased this against the master branch: #57365 |
What does this PR do?
under high load these can be useful to tweak for ensuring critical parts
of salt aren't being resource starved.
Tests written?
No
Commits signed with GPG?
No
Please review Salt's Contributing Guide for best practices.
See GitHub's page on GPG signing for more information about signing commits with GPG.