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

Power-off when shutting down FreeBSD, NetBSD, and OpenBSD #53935

Merged
merged 2 commits into from
Dec 14, 2019
Merged

Power-off when shutting down FreeBSD, NetBSD, and OpenBSD #53935

merged 2 commits into from
Dec 14, 2019

Conversation

morganwillcock
Copy link
Contributor

@morganwillcock morganwillcock commented Jul 19, 2019

What does this PR do?

Adds platform checks into the shutdown function of the system execution module, since FreeBSD, NetBSD, and OpenBSD have different shutdown results to other platforms.

What issues does this PR fix or reference?

These platforms don't power-off by default when halted, instead they prompt for input at the console. The system.shutdown currently uses 'shutdown -h' to shutdown these platforms,which means they are not powered off.

Previous Behavior

Calling system.shutdown on a physical minion leaves the hardware powered on. The only way to restart it at this point is by being at the console or by using out-of-band power management.

Calling system.shutdown on a virtual minion leaves the VM using resources on the hypervisor. This can cause excessive CPU usage depending on how the hypervisor deals with the halt state, or lead to later failures when trying to programmatically start the minion (since it is technically still running).

New Behavior

If the platform is FreeBSD, NetBSD, or OpenBSD, use 'shutdown -p' to shutdown, instead of the default 'shutdown -h'. This aligns with the system.shutdown result of other platforms: power off is requested.

Tests written?

Yes?/No
I imagine this is probably out-of-scope for the test suite?

Commits signed with GPG?

Yes/No

@morganwillcock morganwillcock requested a review from a team as a code owner July 19, 2019 20:56
@ghost ghost requested a review from twangboy July 19, 2019 20:56
@twangboy
Copy link
Contributor

@morganwillcock There's actually a unit test for this... but it's pretty crappy. It needs to be changed to make sure the cmd.run function is called with the parameters you're expecting. You can do this with the assert_called_once_with assertion. There are many examples in salt code, for example:

        mock = MagicMock()
        with patch.dict(macpackage.__salt__, {'cmd.run': mock}):
            macpackage.install_app('/path/to/file.app/')
            mock.assert_called_once_with('rsync -a --delete "/path/to/file.app/" '
                                         '"/Applications/file.app"')

@morganwillcock
Copy link
Contributor Author

I've searched the documentation and I can't find an obvious answer...
How would I fake the platform to test both outcomes?

@waynew
Copy link
Contributor

waynew commented Jul 29, 2019

Should be something like with patch('salt.utils.platform.is_freebsd', MagicMock(return_value=True)):. There may be other examples throughout the tests.

@morganwillcock
Copy link
Contributor Author

@twangboy is 05da4c5 the sort of thing you would be expecting for the tests?

@waynew
Copy link
Contributor

waynew commented Aug 9, 2019

@morganwillcock Yeah, I think that's reasonable

@morganwillcock
Copy link
Contributor Author

Should I submit this against a different branch?

@waynew
Copy link
Contributor

waynew commented Dec 2, 2019

@morganwillcock Yes, please - against master. If you'd like, you can simply rebase these changes against master, force push, and then edit this PR.

Alternatively, you can close this PR and just open a new one. Either approach is fine.

@morganwillcock morganwillcock changed the base branch from 2018.3 to master December 2, 2019 20:01
@morganwillcock
Copy link
Contributor Author

@waynew I've rebased and switched the target to master

@dwoz dwoz merged commit 9962b4b into saltstack:master Dec 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants