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.

Sign up for GitHub

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

Jump to bottom

reboot: Fix typo and support bare Linux systems #45607

Merged
merged 2 commits into from
Sep 17, 2018
Merged

reboot: Fix typo and support bare Linux systems #45607

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2b485b8
reboot: Fix typo and support bare Linux systems
dagwieers Sep 13, 2018
bdbfe41
Accumulate stdout and stderr information
dagwieers Sep 13, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
15 changes: 14 additions & 1 deletion lib/ansible/plugins/action/reboot.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,18 +94,31 @@ def construct_command(self):
return reboot_command

def get_system_boot_time(self):
stdout = ''
stderr = ''
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

command_result = self._low_level_execute_command(self.DEFAULT_BOOT_TIME_COMMAND, sudoable=self.DEFAULT_SUDOABLE)

# For single board computers, e.g., Raspberry Pi, that lack a real time clock and are using fake-hwclock
# launched by systemd, the update of utmp/wtmp is not done correctly.
# Fall back to using uptime -s for those systems.
# https://github.com/systemd/systemd/issues/6057
if '1970-01-01 00:00' in command_result['stdout']:
stdout += command_result['stdout']
stderr += command_result['stderr']
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:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@dagwieers dagwieers Sep 14, 2018 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/

stdout += command_result['stdout']
stderr += command_result['stderr']
command_result = self._low_level_execute_command('cat /proc/sys/kernel/random/boot_id', sudoable=self.DEFAULT_SUDOABLE)

if command_result['rc'] != 0:
stdout += command_result['stdout']
stderr += command_result['stderr']
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(stdout), to_native(stderr)))

return command_result['stdout'].strip()

Expand Down