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.

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

ping_interval: use service.restart instead of signaling #49615

Merged
merged 2 commits into from Sep 21, 2018

Conversation

Projects
None yet
3 participants
@terminalmage
Copy link
Contributor

commented Sep 11, 2018

When multiprocessing does not relay the signal properly, ping_interval will fail to respawn the multiprocessing.Process instance and the minion will end up in a bad state, where there are still salt-minion procs running but the minion is not connected to the master and thus does not respond to pubs from it.

This commit ditches signaling in favor of using service.restart to restart the minion daemon.

@terminalmage terminalmage requested a review from saltstack/team-core as a code owner Sep 11, 2018

@salt-jenkins salt-jenkins requested a review from saltstack/team-suse Sep 11, 2018

@terminalmage terminalmage force-pushed the terminalmage:ping-interval branch from d7a17dc to 7eb9968 Sep 11, 2018

ping_interval: use service.restart instead of signaling
When multiprocessing does not relay the signal properly, ping_interval
will fail to respawn the multiprocessing.Process instance and the minion
will end up in a bad state, where there are still salt-minion procs
running but the minion is not connected to the master and thus does not
respond to pubs from it.

This commit ditches signaling in favor of using `service.restart` to
restart the minion daemon.

@terminalmage terminalmage force-pushed the terminalmage:ping-interval branch from 7eb9968 to 2fddba3 Sep 11, 2018

@isbm
Copy link
Contributor

left a comment

@terminalmage it is a benevolent request. 😉

self.functions['service.restart'](
'salt_minion' if 'bsd' in sys.platform
else 'salt-minion'
)

This comment has been minimized.

Copy link
@isbm

isbm Sep 12, 2018

Contributor

I would personally move it to some sort of def _get_minion_service_name function and then:

self.functions['service.restart'](_get_minion_service_name())

Because while it works and maybe nothing else is needed today, still it would be much safer and structured to have a place, where all these nasty checks are done. You never know if BSD will not move to some junkyardsysv in the next release, using slashes or apostrophes...

This comment has been minimized.

Copy link
@terminalmage

terminalmage Sep 17, 2018

Author Contributor

Good idea. I just made this change.

@rallytime
Copy link
Contributor

left a comment

I think this looks good, but it's probably a good idea to move that to a helper function for the future.

@terminalmage

This comment has been minimized.

Copy link
Contributor Author

commented Sep 17, 2018

The logic to get the service name has been moved to a separate function in salt/minion.py. I decided to make it a public function rather than private, since it could be useful outside of minion.py.

@rallytime rallytime requested a review from isbm Sep 20, 2018

@isbm

isbm approved these changes Sep 21, 2018

@rallytime rallytime merged commit f745e43 into saltstack:2018.3 Sep 21, 2018

6 of 10 checks passed

continuous-integration/jenkins/pr-merge This commit cannot be built
Details
jenkins/pr/py2-ubuntu-1604 The py2-ubuntu-1604 job has failed
Details
jenkins/pr/py2-windows-2016 The py2-windows-2016 job has failed
Details
jenkins/pr/py3-windows-2016 The py3-windows-2016 job has failed
Details
WIP ready for review
Details
jenkins/pr/docs The docs job has passed
Details
jenkins/pr/lint The lint job has passed
Details
jenkins/pr/py2-centos-7 The py2-centos-7 job has passed
Details
jenkins/pr/py3-centos-7 The py3-centos-7 job has passed
Details
jenkins/pr/py3-ubuntu-1604 The py3-ubuntu-1604 job has passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.