reboot: Fix typo and support bare Linux systems #45607

dagwieers · 2018-09-13T14:46:43Z

SUMMARY

This fixes a problem for bare Linux systems that do not support 'who -b' or 'uptime -s'.
For example, OpenELEC does not ship with who and does not support uptime -s.

Probably needs a backport to Linux v2.7.

ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME

reboot

ANSIBLE VERSION

v2.8 and v2.7

This fixes a problem for bare Linux systems that do not support 'who -b' or 'uptime -s'.

lib/ansible/plugins/action/reboot.py

bcoca · 2018-09-13T14:50:20Z

lib/ansible/plugins/action/reboot.py

        if command_result['rc'] != 0:
            raise AnsibleError("%s: failed to get host boot time info, rc: %d, stdout: %s, stderr: %s"
-                               % (self._task.action, command_result.rc, to_native(command_result['stdout']), to_native(command_result['stderr'])))
+                               % (self._task.action, command_result['rc'], to_native(command_result['stdout']), to_native(command_result['stderr'])))


I would aggregate both errors here, both 'uptime' and 'cat /proc..'

Agree with this. Don't want to swallow the output from the initial command.

Long term, it feels like we may need to change distribution to being an attribute, that way we can run cat ... for Linux hosts, and use who -b for others, removing the need for uptime -s and a hard coded tree of commands to attempt.

I fixed it so we collect all the stdout and stderr information in case we need it for the AnsibleError.

The problem with having only a single guaranteed command per OS is

You would need to know the platform/OS you're working with

I don't think it's that clear-cut

So this fallback method, while not necessarily 100% clean, it does the job effectively.
And who -b and uptime -s are very sensible things to do before resorting to the random boot_id. (Which may not be present in all Linux kernels either ?)

My next question was going to be if the random boot ID is widely available. I guess I'll have to do some research to see when it was added.

I created a WIP PR (#45656) exploring what it would look like to add platform and dist to the plugin. I'm not terribly happy with it, but it's a start (and it accounts for Alpine, which is particularly annoying since it lacks both who and uptime for no good reason (IMO)).

That PR also changes to using the random boot ID by default rather than who -b. That has the nice side effect of being able to remove the uptime -s command for systems that lack a RTC, but at the cost of all the added complexity of platfrom and dist checking.

samdoran · 2018-09-13T18:16:35Z

lib/ansible/plugins/action/reboot.py

        if command_result['rc'] != 0:
            raise AnsibleError("%s: failed to get host boot time info, rc: %d, stdout: %s, stderr: %s"
-                               % (self._task.action, command_result.rc, to_native(command_result['stdout']), to_native(command_result['stderr'])))
+                               % (self._task.action, command_result['rc'], to_native(command_result['stdout']), to_native(command_result['stderr'])))


Agree with this. Don't want to swallow the output from the initial command.

Long term, it feels like we may need to change distribution to being an attribute, that way we can run cat ... for Linux hosts, and use who -b for others, removing the need for uptime -s and a hard coded tree of commands to attempt.

dagwieers · 2018-09-13T19:46:10Z

Please re-review !

samdoran

Please add a changelog fragment as well.

samdoran · 2018-09-14T15:18:43Z

lib/ansible/plugins/action/reboot.py

            command_result = self._low_level_execute_command('uptime -s', sudoable=self.DEFAULT_SUDOABLE)

+        # This is a last resort for bare Linux systems (e.g. OpenELEC) where 'who -b' or 'uptime -s' are not supported.
+        # Other options like parsing /proc/uptime or default uptime output are less reliable than this
        if command_result['rc'] != 0:


One issue with this is that win_reboot uses this same method. So if the windows DEFAULT_BOOT_TIME_COMMAND results in an rc != 0, it will then try this cat command, which will gloriously fail.

The previous conditional which runs uptime -s won't ever match on Windows because that string will never match the output of (Get-WmiObject -ClassName Win32_OperatingSystem).LastBootUpTime.

This is the point where it would be helpful to have platform/dist as a class attribute, either as an additional check here, or to look up the correct boot time command based on platform/dist. I'm also open to other ideas.

I don't mind if a fallback method fails though. As long as we cover all bases so in the end the function always works. For win_reboot it could be a lot easier if we have a list of commands, for both Windows and Unix.

In my WIP PR, I did change the "get the last boot time" command to be a variable, so that will remove the hard coding here.

Will fix in future™.

Still, I can see it being confusing to someone running win_reboot (assuming the first command failed to get the last boot time) if they get this error:

win_reboot: failed to get host boot time info, rc: 1, stdout: 20180913204044.576906+000 , stderr: cat : Cannot find path 'C:\proc\sys\kernel\random\boot_id' because it does not exist. At line:1 char:1 + cat /proc/sys/kernel/random/boot_id + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + CategoryInfo : ObjectNotFound: (C:\proc\sys\kernel\random\boot_id:String) [Get-Content], ItemNotFoundEx ception + FullyQualifiedErrorId : PathNotFound,Microsoft.PowerShell.Commands.GetContentCommand

Evidently Windows has a cat command now!?

Well, the distinction between Windows and Unix is now easy because it's a different entry-point. So we don't need to do a cat on Windows. That was my main point. But I also don't mind having the fallback mechanism on Unix, that first does who -b, then uptime -s, and then cat /.../boot_id. Which to me is quite sensible as well. It's a trade-off for simplicity and in most cases it only requires a single command who -b. If you implement support for getting the platform, you will always have to run at least 2 commands.

So basically what I am saying is that in general the fallback mechanism is more efficient than targeting based on platform.

And yes, PowerShell has aliases for common Unix commands, which is freaking a lot of people out (because they are not drop-in replacements). https://daniel.haxx.se/blog/2016/08/19/removing-the-powershell-curl-alias/

bcoca · 2018-09-17T18:04:57Z

lib/ansible/plugins/action/reboot.py

@@ -94,18 +94,31 @@ def construct_command(self):
        return reboot_command

    def get_system_boot_time(self):
+        stdout = ''
+        stderr = ''


this might not be py2/py3 compatible, we might want to force bytes

Yes, you are correct. One day I'll master that distinction. 😄

dagwieers · 2018-09-17T21:36:26Z

@samdoran I assumed your own PR would take over this. That's why I didn't bother doing the fragment.

samdoran · 2018-09-18T13:47:24Z

@dagwieers I wanted to get this fix in. My PR is probably too big a change to get in the short term, plus it's probably in the feature request territory.

dagwieers · 2018-09-18T13:56:37Z

Ok. Sorry for the confusion.

samdoran · 2018-09-18T13:58:25Z

Not at all. Thank you for finding and fixing this.

* reboot: Fix typo and support bare Linux systems This fixes a problem for bare Linux systems that do not support 'who -b' or 'uptime -s'. * Accumulate stdout and stderr information (cherry picked from commit a7a99c5)

reboot: Fix typo and support bare Linux systems

2b485b8

This fixes a problem for bare Linux systems that do not support 'who -b' or 'uptime -s'.

dagwieers added affects_2.7 This issue/PR affects Ansible v2.7 affects_2.8 This issue/PR affects Ansible v2.8 labels Sep 13, 2018

dagwieers requested review from samdoran and bcoca September 13, 2018 14:46

bcoca requested changes Sep 13, 2018

View reviewed changes

ansibot added bug This issue/PR relates to a bug. support:core This issue/PR relates to code supported by the Ansible Engineering Team. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Sep 13, 2018

samdoran suggested changes Sep 13, 2018

View reviewed changes

Accumulate stdout and stderr information

bdbfe41

ansibot added support:community This issue/PR relates to code supported by the Ansible community. and removed support:core This issue/PR relates to code supported by the Ansible Engineering Team. labels Sep 14, 2018

samdoran suggested changes Sep 14, 2018

View reviewed changes

samdoran approved these changes Sep 17, 2018

View reviewed changes

samdoran merged commit a7a99c5 into ansible:devel Sep 17, 2018

bcoca reviewed Sep 17, 2018

View reviewed changes

samdoran mentioned this pull request Sep 18, 2018

Backport #45607 and #45746 for 2.7 - reboot plugin fix variable and support bare Linux #45791

Merged

ansible locked and limited conversation to collaborators Jul 22, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

reboot: Fix typo and support bare Linux systems #45607

reboot: Fix typo and support bare Linux systems #45607

dagwieers commented Sep 13, 2018 •

edited

bcoca Sep 13, 2018

samdoran Sep 13, 2018 •

edited

dagwieers Sep 13, 2018

samdoran Sep 14, 2018

samdoran Sep 13, 2018 •

edited

dagwieers commented Sep 13, 2018

samdoran left a comment

samdoran Sep 14, 2018

dagwieers Sep 14, 2018

samdoran Sep 14, 2018

dagwieers Sep 14, 2018

dagwieers Sep 14, 2018 •

edited

bcoca Sep 17, 2018

samdoran Sep 17, 2018

dagwieers commented Sep 17, 2018

samdoran commented Sep 18, 2018

dagwieers commented Sep 18, 2018

samdoran commented Sep 18, 2018

reboot: Fix typo and support bare Linux systems #45607

reboot: Fix typo and support bare Linux systems #45607

Conversation

dagwieers commented Sep 13, 2018 • edited

SUMMARY

ISSUE TYPE

COMPONENT NAME

ANSIBLE VERSION

Choose a reason for hiding this comment

samdoran Sep 13, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samdoran Sep 13, 2018 • edited

Choose a reason for hiding this comment

dagwieers commented Sep 13, 2018

samdoran left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dagwieers Sep 14, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dagwieers commented Sep 17, 2018

samdoran commented Sep 18, 2018

dagwieers commented Sep 18, 2018

samdoran commented Sep 18, 2018

dagwieers commented Sep 13, 2018 •

edited

samdoran Sep 13, 2018 •

edited

samdoran Sep 13, 2018 •

edited

dagwieers Sep 14, 2018 •

edited