46255 fix minion.restart #68225
Open
46255 fix minion.restart #68225
Conversation
This file contains hidden or 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
175cf4f to
093e95f
Compare
|
@dwoz - looks like this passes all the tests when run with Could you re-add your approval when you get a moment - nothing has changed in the PR. |
Updates minion.restart to use service.restart if system uses systemd or is a Windows system. Existing methods don't work if salt-minion is managed by the Windows service manager or systemd. This adds functionality to check for those and use service.restart if so.
Adds options to enable use of systemd or windows service to restart the salt minion. Both options default to True as we should prefer to use the service managers, but give the option to disable where users are doing something unusual like running another service supervisor.
Adds unit tests for the minion.restart function to test existing and new functionality. Also adds tests for helper functions.
Adds support for creating a scheduled job to retry starting the minion process if it has died and not restarted correctly. This will work on Windows and systemd based systems.
Adds unit tests for the schedule_retry functionality in the minion.restart function. Tests the helper functions that do the scheduling for windows and systemd, tests the dispatcher function and also testsing the logic in minion.restart itself.
Refactors minion.restart to move the duplicated code for systemd and windows into a helper function
189ba1e to
3149089
Compare
dwoz
approved these changes
Aug 26, 2025
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?
Adds support to
minion.restartto detect if the minion is running under systemd or the Windows service manager. If so, it will useservice.restartto restart the minion rather than killing and attempting to restart it.If
minion_restart_commandis set it will follow the old behaviour and use that command to handle the restart.If
minion_restart_commandisn't set and systemd/windows aren't detected, preserves the old behaviour of looking atsys.argvand either just killing the minion process or killing it and attempting to restart the process with the same args if-d(for daemon mode) is insys.argv. Whilst I can see that the former option may be useful if running the minion under another process supervisor, I'm not really sure how the killing and starting the a new process could ever work... I've left it in for the moment, but it should maybe be removed in 3008.What issues does this PR fix or reference?
Fixes #46255
Previous Behavior
Calling
minion.restarton a systemd based system would not work - it would just kill the minion processNew Behavior
Calling
minion.restart on a systemd based system will now useservice.restart` to restart the minion process.Merge requirements satisfied?
[NOTICE] Bug fixes or features added to Salt require tests.
Commits signed with SSH?
Yes