Reboot - Fix command not found, add Apline support, fix Solaris command (#49272)

* Fix various bugs related in reboot

- Use format strings for consistency and improve debug log messages
- Use local variables instead of class attributes in order to be thread safe
- Run setup module to get distribution and version
- Run find module to get full path of shutdown command
- Use ansible_os_family and ansible_distribution to find commands and args
- Use same command for all Solaris/SunOS distributions
- Move delay calculations to properties
- Reliably check for module run failure
- Fix bug in run_test_command() that accidentally made the method work properly
- Use better exceptions rather than Exception
- Use dict literals rather than constructors
- Correct _check_delay() so it always returns a value, not None
- Don't store and return result in run_test_command() because it's not used anywhere
- add test for post reboot command that fails
- test negative values for delay parameters

changelogs/fragments/reboot-path-alpine-solaris.yaml

@@ -0,0 +1,4 @@
+bugfixes:
+  - reboot - search common paths for the shutdown command and use the full path to the binary rather than depending on the PATH of the remote system (https://github.com/ansible/ansible/issues/47131)
+  - reboot - gather distribution information in order to support Alpine and other distributions (https://github.com/ansible/ansible/issues/46723)
+  - reboot - use a common set of commands for older and newer Solaris and SunOS variants (https://github.com/ansible/ansible/pull/48986)

lib/ansible/plugins/action/reboot.py

@@ -34,91 +34,146 @@ class ActionModule(ActionBase):
     DEFAULT_BOOT_TIME_COMMAND = 'cat /proc/sys/kernel/random/boot_id'
     DEFAULT_REBOOT_MESSAGE = 'Reboot initiated by Ansible'
     DEFAULT_SHUTDOWN_COMMAND = 'shutdown'
+    DEFAULT_SHUTDOWN_COMMAND_ARGS = '-r {delay_min} "{message}"'
     DEFAULT_SUDOABLE = True
 
     DEPRECATED_ARGS = {}
 
     BOOT_TIME_COMMANDS = {
-        'openbsd': '/sbin/sysctl kern.boottime',
         'freebsd': '/sbin/sysctl kern.boottime',
+        'openbsd': '/sbin/sysctl kern.boottime',
+        'macosx': 'who -b',
+        'solaris': 'who -b',
         'sunos': 'who -b',
-        'darwin': 'who -b',
     }
 
     SHUTDOWN_COMMANDS = {
-        'linux': DEFAULT_SHUTDOWN_COMMAND,
-        'freebsd': DEFAULT_SHUTDOWN_COMMAND,
-        'openbsd': DEFAULT_SHUTDOWN_COMMAND,
-        'sunos': '/usr/sbin/shutdown',
-        'darwin': '/sbin/shutdown',
+        'alpine': 'reboot',
     }
 
     SHUTDOWN_COMMAND_ARGS = {
-        'linux': '-r {delay_min} "{message}"',
+        'alpine': '',
         'freebsd': '-r +{delay_sec}s "{message}"',
-        'sunos': '-i 6 -y -g {delay_sec} "{message}"',
-        'darwin': '-r +{delay_min} "{message}"',
+        'linux': DEFAULT_SHUTDOWN_COMMAND_ARGS,
+        'macosx': '-r +{delay_min} "{message}"',
         'openbsd': '-r +{delay_min} "{message}"',
+        'solaris': '-y -g {delay_sec} -i 6 "{message}"',
+        'sunos': '-y -g {delay_sec} -i 6 "{message}"',
+    }
+
+    TEST_COMMANDS = {
+        'solaris': 'who'
     }
 
     def __init__(self, *args, **kwargs):
         super(ActionModule, self).__init__(*args, **kwargs)
 
-        self._original_connection_timeout = None
-        self._previous_boot_time = None
+    @property
+    def pre_reboot_delay(self):
+        return self._check_delay('pre_reboot_delay', self.DEFAULT_PRE_REBOOT_DELAY)
+
+    @property
+    def post_reboot_delay(self):
+        return self._check_delay('post_reboot_delay', self.DEFAULT_POST_REBOOT_DELAY)
+
+    def _check_delay(self, key, default):
+        """Ensure that the value is positive or zero"""
+        value = int(self._task.args.get(key, self._task.args.get(key + '_sec', default)))
+        if value < 0:
+            value = 0
+        return value
+
+    def _get_value_from_facts(self, variable_name, distribution, default_value):
+        """Get dist+version specific args first, then distribution, then family, lastly use default"""
+        attr = getattr(self, variable_name)
+        value = attr.get(
+            distribution['name'] + distribution['version'],
+            attr.get(
+                distribution['name'],
+                attr.get(
+                    distribution['family'],
+                    getattr(self, default_value))))
+        return value
+
+    def get_shutdown_command_args(self, distribution):
+        args = self._get_value_from_facts('SHUTDOWN_COMMAND_ARGS', distribution, 'DEFAULT_SHUTDOWN_COMMAND_ARGS')
+        # Convert seconds to minutes. If less that 60, set it to 0.
+        delay_min = self.pre_reboot_delay // 60
+        reboot_message = self._task.args.get('msg', self.DEFAULT_REBOOT_MESSAGE)
+        return args.format(delay_sec=self.pre_reboot_delay, delay_min=delay_min, message=reboot_message)
+
+    def get_distribution(self, task_vars):
+        distribution = {}
+        display.debug('{action}: running setup module to get distribution'.format(action=self._task.action))
+        module_output = self._execute_module(
+            task_vars=task_vars,
+            module_name='setup',
+            module_args={'gather_subset': 'min'})
+        try:
+            if module_output.get('failed', False):
+                raise AnsibleError('Failed to determine system distribution. {0}, {1}'.format(
+                    to_native(module_output['module_stdout']).strip(),
+                    to_native(module_output['module_stderr']).strip()))
+            distribution['name'] = module_output['ansible_facts']['ansible_distribution'].lower()
+            distribution['version'] = to_text(module_output['ansible_facts']['ansible_distribution_version'].split('.')[0])
+            distribution['family'] = to_text(module_output['ansible_facts']['ansible_os_family'].lower())
+            display.debug("{action}: distribution: {dist}".format(action=self._task.action, dist=distribution))
+            return distribution
+        except KeyError as ke:
+            raise AnsibleError('Failed to get distribution information. Missing "{0}" in output.'.format(ke.args[0]))
+
+    def get_shutdown_command(self, task_vars, distribution):
+        shutdown_bin = self._get_value_from_facts('SHUTDOWN_COMMANDS', distribution, 'DEFAULT_SHUTDOWN_COMMAND')
+
+        display.debug('{action}: running find module to get path for "{command}"'.format(action=self._task.action, command=shutdown_bin))
+        find_result = self._execute_module(
+            task_vars=task_vars,
+            module_name='find',
+            module_args={
+                'paths': ['/sbin', '/usr/sbin', '/usr/local/sbin'],
+                'patterns': [shutdown_bin],
+                'file_type': 'any'
+            }
+        )
+
+        full_path = [x['path'] for x in find_result['files']]
+        if not full_path:
+            raise AnsibleError('Unable to find command "{0}" in system paths.'.format(shutdown_bin))
+        self._shutdown_command = full_path[0]
+        return self._shutdown_command
 
     def deprecated_args(self):
         for arg, version in self.DEPRECATED_ARGS.items():
             if self._task.args.get(arg) is not None:
-                display.warning("Since Ansible %s, %s is no longer a valid option for %s" % (version, arg, self._task.action))
+                display.warning("Since Ansible {version}, {arg} is no longer a valid option for {action}".format(
+                    version=version,
+                    arg=arg,
+                    action=self._task.action))
 
-    def construct_command(self):
-        # Determine the system distribution in order to use the correct shutdown command arguments
-        uname_result = self._low_level_execute_command('uname')
-        distribution = uname_result['stdout'].strip().lower()
-
-        shutdown_command = self.SHUTDOWN_COMMANDS.get(distribution, self.SHUTDOWN_COMMANDS['linux'])
-        shutdown_command_args = self.SHUTDOWN_COMMAND_ARGS.get(distribution, self.SHUTDOWN_COMMAND_ARGS['linux'])
-
-        pre_reboot_delay = int(self._task.args.get('pre_reboot_delay', self.DEFAULT_PRE_REBOOT_DELAY))
-        if pre_reboot_delay < 0:
-            pre_reboot_delay = 0
-
-        # Convert seconds to minutes. If less that 60, set it to 0.
-        delay_min = pre_reboot_delay // 60
-        msg = self._task.args.get('msg', self.DEFAULT_REBOOT_MESSAGE)
-
-        shutdown_command_args = shutdown_command_args.format(delay_sec=pre_reboot_delay, delay_min=delay_min, message=msg)
-
-        reboot_command = '%s %s' % (shutdown_command, shutdown_command_args)
-        return reboot_command
-
-    def get_system_boot_time(self):
-        stdout = u''
-        stderr = u''
-
-        # Determine the system distribution in order to use the correct shutdown command arguments
-        uname_result = self._low_level_execute_command('uname')
-        distribution = uname_result['stdout'].strip().lower()
-
-        boot_time_command = self.BOOT_TIME_COMMANDS.get(distribution, self.DEFAULT_BOOT_TIME_COMMAND)
+    def get_system_boot_time(self, distribution):
+        boot_time_command = self._get_value_from_facts('BOOT_TIME_COMMANDS', distribution, 'DEFAULT_BOOT_TIME_COMMAND')
+        display.debug("{action}: getting boot time with command: '{command}'".format(action=self._task.action, command=boot_time_command))
         command_result = self._low_level_execute_command(boot_time_command, 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(stdout), to_native(stderr)))
-
+            stdout = command_result['stdout']
+            stderr = command_result['stderr']
+            raise AnsibleError("{action}: failed to get host boot time info, rc: {rc}, stdout: {out}, stderr: {err}".format(
+                               action=self._task.action,
+                               rc=command_result['rc'],
+                               out=to_native(stdout),
+                               err=to_native(stderr)))
+        display.debug("{action}: last boot time: {boot}".format(action=self._task.action, boot=command_result['stdout'].strip()))
         return command_result['stdout'].strip()
 
-    def check_boot_time(self):
-        display.vvv("%s: attempting to get system boot time" % self._task.action)
+    def check_boot_time(self, distribution, previous_boot_time):
+        display.vvv("{action}: attempting to get system boot time".format(action=self._task.action))
         connect_timeout = self._task.args.get('connect_timeout', self._task.args.get('connect_timeout_sec', self.DEFAULT_CONNECT_TIMEOUT))
 
         # override connection timeout from defaults to custom value
         if connect_timeout:
             try:
+                display.debug("{action}: setting connect_timeout to {value}".format(action=self._task.action, value=connect_timeout))
                 self._connection.set_option("connection_timeout", connect_timeout)
                 self._connection.reset()
             except AttributeError:
@@ -126,18 +181,19 @@ class ActionModule(ActionBase):
 
         # try and get boot time
         try:
-            current_boot_time = self.get_system_boot_time()
+            current_boot_time = self.get_system_boot_time(distribution)
         except Exception as e:
             raise e
 
         # FreeBSD returns an empty string immediately before reboot so adding a length
         # check to prevent prematurely assuming system has rebooted
-        if len(current_boot_time) == 0 or current_boot_time == self._previous_boot_time:
-            raise Exception("boot time has not changed")
+        if len(current_boot_time) == 0 or current_boot_time == previous_boot_time:
+            raise ValueError("boot time has not changed")
 
-    def run_test_command(self, **kwargs):
-        test_command = self._task.args.get('test_command', self.DEFAULT_TEST_COMMAND)
-        display.vvv("%s: attempting post-reboot test command '%s'" % (self._task.action, test_command))
+    def run_test_command(self, distribution, **kwargs):
+        test_command = self._task.args.get('test_command', self._get_value_from_facts('TEST_COMMANDS', distribution, 'DEFAULT_TEST_COMMAND'))
+        display.vvv("{action}: attempting post-reboot test command".format(action=self._task.action))
+        display.debug("{action}: attempting post-reboot test command '{command}'".format(action=self._task.action, command=test_command))
         try:
             command_result = self._low_level_execute_command(test_command, sudoable=self.DEFAULT_SUDOABLE)
         except Exception:
@@ -149,26 +205,27 @@ class ActionModule(ActionBase):
                 pass
             raise
 
-        result = {}
         if command_result['rc'] != 0:
-            result['failed'] = True
-            result['msg'] = 'test command failed: %s %s' % (to_native(command_result['stderr'], to_native(command_result['stdout'])))
-        else:
-            result['msg'] = to_native(command_result['stdout'])
+            msg = 'Test command failed: {err} {out}'.format(
+                err=to_native(command_result['stderr']),
+                out=to_native(command_result['stdout']))
+            raise RuntimeError(msg)
 
-        return result
+        display.vvv("{action}: system sucessfully rebooted".format(action=self._task.action))
 
-    def do_until_success_or_timeout(self, action, reboot_timeout, action_desc):
+    def do_until_success_or_timeout(self, action, reboot_timeout, action_desc, distribution, action_kwargs=None):
         max_end_time = datetime.utcnow() + timedelta(seconds=reboot_timeout)
+        if action_kwargs is None:
+            action_kwargs = {}
 
         fail_count = 0
         max_fail_sleep = 12
 
         while datetime.utcnow() < max_end_time:
             try:
-                action()
+                action(distribution=distribution, **action_kwargs)
                 if action_desc:
-                    display.debug('%s: %s success' % (self._task.action, action_desc))
+                    display.debug('{action}: {desc} success'.format(action=self._task.action, desc=action_desc))
                 return
             except Exception as e:
                 if isinstance(e, AnsibleConnectionFailure):
@@ -187,26 +244,30 @@ class ActionModule(ActionBase):
                         error = to_text(e).splitlines()[-1]
                     except IndexError as e:
                         error = to_text(e)
-                    display.debug("{0}: {1} fail '{2}', retrying in {3:.4} seconds...".format(self._task.action, action_desc,
-                                                                                              error, fail_sleep))
+                    display.debug("{action}: {desc} fail '{err}', retrying in {sleep:.4} seconds...".format(
+                        action=self._task.action,
+                        desc=action_desc,
+                        err=error,
+                        sleep=fail_sleep))
                 fail_count += 1
                 time.sleep(fail_sleep)
 
-        raise TimedOutException('Timed out waiting for %s (timeout=%s)' % (action_desc, reboot_timeout))
-
-    def perform_reboot(self):
-        display.debug("%s: rebooting server" % self._task.action)
-
-        remote_command = self.construct_command()
+        raise TimedOutException('Timed out waiting for {desc} (timeout={timeout})'.format(desc=action_desc, timeout=reboot_timeout))
 
+    def perform_reboot(self, task_vars, distribution):
         result = {}
         reboot_result = {}
+        shutdown_command = self.get_shutdown_command(task_vars, distribution)
+        shutdown_command_args = self.get_shutdown_command_args(distribution)
+        reboot_command = '{0} {1}'.format(shutdown_command, shutdown_command_args)
 
         try:
-            reboot_result = self._low_level_execute_command(remote_command, sudoable=self.DEFAULT_SUDOABLE)
+            display.vvv("{action}: rebooting server...".format(action=self._task.action))
+            display.debug("{action}: rebooting server with command '{command}'".format(action=self._task.action, command=reboot_command))
+            reboot_result = self._low_level_execute_command(reboot_command, sudoable=self.DEFAULT_SUDOABLE)
         except AnsibleConnectionFailure as e:
             # If the connection is closed too quickly due to the system being shutdown, carry on
-            display.debug('%s: AnsibleConnectionFailure caught and handled: %s' % (self._task.action, to_native(e)))
+            display.debug('{action}: AnsibleConnectionFailure caught and handled: {error}'.format(action=self._task.action, error=to_native(e)))
             reboot_result['rc'] = 0
 
         result['start'] = datetime.utcnow()
@@ -214,42 +275,49 @@ class ActionModule(ActionBase):
         if reboot_result['rc'] != 0:
             result['failed'] = True
             result['rebooted'] = False
-            result['msg'] = "Shutdown command failed. Error was %s, %s" % (
-                to_native(reboot_result['stdout'].strip()), to_native(reboot_result['stderr'].strip()))
+            result['msg'] = "Reboot command failed. Error was {stdout}, {stderr}".format(
+                stdout=to_native(reboot_result['stdout'].strip()),
+                stderr=to_native(reboot_result['stderr'].strip()))
             return result
 
         result['failed'] = False
-
-        # attempt to store the original connection_timeout option var so it can be reset after
-        self._original_connection_timeout = None
-        try:
-            self._original_connection_timeout = self._connection.get_option('connection_timeout')
-        except AnsibleError:
-            display.debug("%s: connect_timeout connection option has not been set" % self._task.action)
-
         return result
 
-    def validate_reboot(self):
-        display.debug('%s: Validating reboot' % self._task.action)
+    def validate_reboot(self, distribution, original_connection_timeout=None, action_kwargs=None):
+        display.vvv('{action}: validating reboot'.format(action=self._task.action))
         result = {}
 
         try:
             # keep on checking system boot_time with short connection responses
             reboot_timeout = int(self._task.args.get('reboot_timeout', self._task.args.get('reboot_timeout_sec', self.DEFAULT_REBOOT_TIMEOUT)))
             connect_timeout = self._task.args.get('connect_timeout', self._task.args.get('connect_timeout_sec', self.DEFAULT_CONNECT_TIMEOUT))
-            self.do_until_success_or_timeout(self.check_boot_time, reboot_timeout, action_desc="boot_time check")
 
-            if connect_timeout:
+            self.do_until_success_or_timeout(
+                action=self.check_boot_time,
+                action_desc="last boot time check",
+                reboot_timeout=reboot_timeout,
+                distribution=distribution,
+                action_kwargs=action_kwargs)
+
+            if connect_timeout and original_connection_timeout:
                 try:
-                    self._connection.set_option("connection_timeout", connect_timeout)
+                    display.debug("{action}: setting connect_timeout back to original value of {value}".format(
+                        action=self._task.action,
+                        value=original_connection_timeout))
+                    self._connection.set_option("connection_timeout", original_connection_timeout)
                     self._connection.reset()
                 except (AnsibleError, AttributeError) as e:
                     # reset the connection to clear the custom connection timeout
-                    display.debug("Failed to reset connection_timeout back to default: %s" % to_text(e))
+                    display.debug("{action}: failed to reset connection_timeout back to default: {error}".format(action=self._task.action, error=to_text(e)))
 
             # finally run test command to ensure everything is working
             # FUTURE: add a stability check (system must remain up for N seconds) to deal with self-multi-reboot updates
-            self.do_until_success_or_timeout(self.run_test_command, reboot_timeout, action_desc="post-reboot test command")
+            self.do_until_success_or_timeout(
+                action=self.run_test_command,
+                action_desc="post-reboot test command",
+                reboot_timeout=reboot_timeout,
+                distribution=distribution,
+                action_kwargs=action_kwargs)
 
             result['rebooted'] = True
             result['changed'] = True
@@ -269,13 +337,13 @@ class ActionModule(ActionBase):
         # If running with local connection, fail so we don't reboot ourself
         if self._connection.transport == 'local':
             msg = 'Running {0} with local connection would reboot the control node.'.format(self._task.action)
-            return dict(changed=False, elapsed=0, rebooted=False, failed=True, msg=msg)
+            return {'changed': False, 'elapsed': 0, 'rebooted': False, 'failed': True, 'msg': msg}
 
         if self._play_context.check_mode:
-            return dict(changed=True, elapsed=0, rebooted=True)
+            return {'changed': True, 'elapsed': 0, 'rebooted': True}
 
         if task_vars is None:
-            task_vars = dict()
+            task_vars = {}
 
         self.deprecated_args()
 
@@ -284,17 +352,26 @@ class ActionModule(ActionBase):
         if result.get('skipped', False) or result.get('failed', False):
             return result
 
+        distribution = self.get_distribution(task_vars)
+
         # Get current boot time
         try:
-            self._previous_boot_time = self.get_system_boot_time()
+            previous_boot_time = self.get_system_boot_time(distribution)
         except Exception as e:
             result['failed'] = True
             result['reboot'] = False
             result['msg'] = to_text(e)
             return result
 
+        # Get the original connection_timeout option var so it can be reset after
+        original_connection_timeout = None
+        try:
+            original_connection_timeout = self._connection.get_option('connection_timeout')
+            display.debug("{action}: saving original connect_timeout of {timeout}".format(action=self._task.action, timeout=original_connection_timeout))
+        except AnsibleError:
+            display.debug("{action}: connect_timeout connection option has not been set".format(action=self._task.action))
         # Initiate reboot
-        reboot_result = self.perform_reboot()
+        reboot_result = self.perform_reboot(task_vars, distribution)
 
         if reboot_result['failed']:
             result = reboot_result
@@ -302,16 +379,13 @@ class ActionModule(ActionBase):
             result['elapsed'] = elapsed.seconds
             return result
 
-        post_reboot_delay = int(self._task.args.get('post_reboot_delay', self._task.args.get('post_reboot_delay_sec', self.DEFAULT_POST_REBOOT_DELAY)))
-        if post_reboot_delay < 0:
-            post_reboot_delay = 0
-
-        if post_reboot_delay != 0:
-            display.vvv("%s: waiting an additional %d seconds" % (self._task.action, post_reboot_delay))
-            time.sleep(post_reboot_delay)
+        if self.post_reboot_delay != 0:
+            display.debug("{action}: waiting an additional {delay} seconds".format(action=self._task.action, delay=self.post_reboot_delay))
+            display.vvv("{action}: waiting an additional {delay} seconds".format(action=self._task.action, delay=self.post_reboot_delay))
+            time.sleep(self.post_reboot_delay)
 
         # Make sure reboot was successful
-        result = self.validate_reboot()
+        result = self.validate_reboot(distribution, original_connection_timeout, action_kwargs={'previous_boot_time': previous_boot_time})
 
         elapsed = datetime.utcnow() - reboot_result['start']
         result['elapsed'] = elapsed.seconds

lib/ansible/plugins/action/win_reboot.py

@@ -29,33 +29,36 @@ class ActionModule(RebootActionModule, ActionBase):
     DEFAULT_BOOT_TIME_COMMAND = "(Get-WmiObject -ClassName Win32_OperatingSystem).LastBootUpTime"
     DEFAULT_CONNECT_TIMEOUT = 5
     DEFAULT_PRE_REBOOT_DELAY = 2
-    DEFAULT_SHUTDOWN_COMMAND_ARGS = '/r /t %d /c "%s"'
     DEFAULT_SUDOABLE = False
+    DEFAULT_SHUTDOWN_COMMAND_ARGS = '/r /t {delay_sec} /c "{message}"'
 
     DEPRECATED_ARGS = {
         'shutdown_timeout': '2.5',
         'shutdown_timeout_sec': '2.5',
     }
 
-    def construct_command(self):
-        shutdown_command = self.DEFAULT_SHUTDOWN_COMMAND
-        pre_reboot_delay = int(self._task.args.get('pre_reboot_delay', self._task.args.get('pre_reboot_delay_sec', self.DEFAULT_PRE_REBOOT_DELAY)))
-        msg = self._task.args.get('msg', self.DEFAULT_REBOOT_MESSAGE)
-        shutdown_command_args = self.DEFAULT_SHUTDOWN_COMMAND_ARGS % (pre_reboot_delay, msg)
+    def __init__(self, *args, **kwargs):
+        super(ActionModule, self).__init__(*args, **kwargs)
 
-        reboot_command = '%s %s' % (shutdown_command, shutdown_command_args)
-        return reboot_command
+    def get_distribution(self, task_vars):
+        return {'name': 'windows', 'version': '', 'family': ''}
 
-    def perform_reboot(self):
-        display.debug("Rebooting server")
+    def get_shutdown_command(self, task_vars, distribution):
+        return self.DEFAULT_SHUTDOWN_COMMAND
+
+    def perform_reboot(self, task_vars, distribution):
+        shutdown_command = self.get_shutdown_command(task_vars, distribution)
+        shutdown_command_args = self.get_shutdown_command_args(distribution)
+        reboot_command = '{0} {1}'.format(shutdown_command, shutdown_command_args)
+
+        display.vvv("{action}: rebooting server...".format(action=self._task.action))
+        display.debug("{action}: distribution: {dist}".format(action=self._task.action, dist=distribution))
+        display.debug("{action}: rebooting server with command '{command}'".format(action=self._task.action, command=reboot_command))
 
-        remote_command = self.construct_command()
-        reboot_result = self._low_level_execute_command(remote_command, sudoable=self.DEFAULT_SUDOABLE)
         result = {}
+        reboot_result = self._low_level_execute_command(reboot_command, sudoable=self.DEFAULT_SUDOABLE)
         result['start'] = datetime.utcnow()
 
-        pre_reboot_delay = int(self._task.args.get('pre_reboot_delay', self._task.args.get('pre_reboot_delay_sec', self.DEFAULT_PRE_REBOOT_DELAY)))
-
         # Test for "A system shutdown has already been scheduled. (1190)" and handle it gracefully
         stdout = reboot_result['stdout']
         stderr = reboot_result['stderr']
@@ -66,7 +69,7 @@ class ActionModule(RebootActionModule, ActionBase):
             result1 = self._low_level_execute_command('shutdown /a', sudoable=self.DEFAULT_SUDOABLE)
 
             # Initiate reboot again
-            result2 = self._low_level_execute_command('shutdown /r /t %d' % pre_reboot_delay, sudoable=self.DEFAULT_SUDOABLE)
+            result2 = self._low_level_execute_command(reboot_command, sudoable=self.DEFAULT_SUDOABLE)
 
             reboot_result['rc'] = result2['rc']
             stdout += result1['stdout'] + result2['stdout']
@@ -75,15 +78,10 @@ class ActionModule(RebootActionModule, ActionBase):
         if reboot_result['rc'] != 0:
             result['failed'] = True
             result['rebooted'] = False
-            result['msg'] = "Shutdown command failed, error was: %s %s" % (to_native(stdout.strip()), to_native(stderr.strip()))
+            result['msg'] = "Reboot command failed, error was: {stdout} {stderr}".format(
+                stdout=to_native(stdout.strip()),
+                stderr=to_native(stderr.strip()))
             return result
 
         result['failed'] = False
-
-        # Get the original connection_timeout option var so it can be reset after
-        try:
-            self._original_connection_timeout = self._connection.get_option('connection_timeout')
-        except AnsibleError:
-            display.debug("%s: connect_timeout connection option has not been set" % self._task.action)
-
         return result

test/integration/targets/reboot/tasks/check_reboot.yml

@@ -0,0 +1,10 @@
+- name: Get current boot time
+  command: "{{ boot_time_command[ansible_facts['distribution'] | lower] | default('cat /proc/sys/kernel/random/boot_id') }}"
+  register: after_boot_time
+
+- name: Ensure system was actually rebooted
+  assert:
+    that:
+      - reboot_result is changed
+      - reboot_result.elapsed > 10
+      - before_boot_time.stdout != after_boot_time.stdout

test/integration/targets/reboot/tasks/get_boot_time.yml

@@ -0,0 +1,3 @@
+- name: Get current boot time
+  command: "{{ boot_time_command[ansible_facts['distribution'] | lower] | default('cat /proc/sys/kernel/random/boot_id') }}"
+  register: before_boot_time

test/integration/targets/reboot/tasks/main.yml

@@ -7,6 +7,8 @@
             content: 'I am the control node'
             dest: /tmp/Anything-Nutlike-Nuzzle-Plow-Overdue
           delegate_to: localhost
+          connection: local
+          when: inventory_hostname == ansible_play_hosts[0]
 
         - name: See if the temp file exists on the managed node
           stat:
@@ -22,29 +24,40 @@
             that:
               - not controller_temp_file.stat.exists
 
-    - name: Get current boot time
-      command: who -b
-      register: before_boot_time
+    - import_tasks: get_boot_time.yml
 
     - name: Reboot with default settings
       reboot:
       register: reboot_result
 
-    - name: Get current boot time
-      command: who -b
-      register: after_boot_time
+    - import_tasks: check_reboot.yml
 
-    - name: Ensure system was actually rebooted
-      assert:
-        that:
-          - reboot_result is changed
-          - reboot_result.elapsed > 10
-          - before_boot_time.stdout != after_boot_time.stdout
+    - import_tasks: get_boot_time.yml
+
+    - name: Reboot with all options
+      reboot:
+        connect_timeout: 30
+        msg: Rebooting
+        post_reboot_delay: 1
+        pre_reboot_delay: 61
+        test_command: uptime
+        reboot_timeout: 500
+
+    - import_tasks: check_reboot.yml
+
+    - import_tasks: get_boot_time.yml
+
+    - name: Test with negative values for delays
+      reboot:
+        post_reboot_delay: -0.5
+        pre_reboot_delay: -61
+
+    - import_tasks: check_reboot.yml
 
     - name: Use invalid parameter
       reboot:
         foo: bar
-      ignore_errors: yes
+      ignore_errors: true
       register: invalid_parameter
 
     - name: Ensure task fails with error
@@ -53,10 +66,22 @@
         - invalid_parameter is failed
         - "invalid_parameter.msg == 'Invalid options for reboot: foo'"
 
+    - name: Reboot with test command that fails
+      reboot:
+        test_command: 'FAIL'
+        reboot_timeout: "{{ timeout }}"
+      register: reboot_fail_test
+      failed_when: "reboot_fail_test.msg != 'Timed out waiting for post-reboot test command (timeout=' ~ timeout ~ ')'"
+      vars:
+        timeout: "{{ timeout_value[ansible_facts['distribution'] | lower] | default(60) }}"
+
   always:
     - name: Cleanup temp file
       file:
         path: /tmp/Anything-Nutlike-Nuzzle-Plow-Overdue
         state: absent
+      delegate_to: localhost
+      connection: local
+      when: inventory_hostname == ansible_play_hosts[0]
 
   when: ansible_virtualization_type | default('') != 'docker'

test/integration/targets/reboot/vars/main.yml

@@ -0,0 +1,9 @@
+boot_time_command:
+  freebsd: '/sbin/sysctl kern.boottime'
+  openbsd: '/sbin/sysctl kern.boottime'
+  macosx: 'who -b'
+  solaris: 'who -b'
+  sunos: 'who -b'
+
+timeout_value:
+  solaris: 120

test/integration/targets/win_reboot/tasks/main.yml

@@ -7,6 +7,11 @@
 - name: reboot with defaults
   win_reboot:
 
+- name: test with negative values for delays
+  win_reboot:
+    post_reboot_delay: -0.5
+    pre_reboot_delay: -61
+
 - name: schedule a reboot for sometime in the future
   win_command: shutdown.exe /r /t 599
 
@@ -23,6 +28,13 @@
   win_reboot:
     test_command: powershell.exe -NoProfile -EncodedCommand {{lookup('template', 'post_reboot.ps1')|b64encode(encoding='utf-16-le')}}
 
+- name: reboot with test command that fails
+  win_reboot:
+    test_command: 'FAIL'
+    reboot_timeout: 120
+  register: reboot_fail_test
+  failed_when: "reboot_fail_test.msg != 'Timed out waiting for post-reboot test command (timeout=120)'"
+
 # try and reboot the host with a non admin user, we expect an error here
 # this requires a bit of setup to create the user and allow it to connect
 # over WinRM
@@ -59,28 +71,28 @@
     Set-Item -Path WSMan:\localhost\Service\RootSDDL -Value $new_sddl -Force
 
 - block:
-  - name: fail to reboot with non admin user
-    win_reboot:
-    vars:
-      ansible_user: '{{standard_user}}'
-      ansible_password: '{{standard_pass}}'
-      ansible_winrm_transport: ntlm
-    register: fail_shutdown
-    failed_when: "fail_shutdown.msg != 'Shutdown command failed, error was:  Access is denied.(5)'"
+    - name: fail to reboot with non admin user
+      win_reboot:
+      vars:
+        ansible_user: '{{standard_user}}'
+        ansible_password: '{{standard_pass}}'
+        ansible_winrm_transport: ntlm
+      register: fail_shutdown
+      failed_when: "fail_shutdown.msg != 'Reboot command failed, error was:  Access is denied.(5)'"
 
   always:
-  - name: set the original SDDL to the WinRM listener
-    win_shell: Set-Item -Path WSMan:\localhost\Service\RootSDDL -Value "{{original_sddl.stdout_lines[0]}}" -Force
+    - name: set the original SDDL to the WinRM listener
+      win_shell: 'Set-Item -Path WSMan:\localhost\Service\RootSDDL -Value "{{original_sddl.stdout_lines[0]}}" -Force'
 
-  - name: remove standard user
-    win_user:
-      name: '{{standard_user}}'
-      state: absent
+    - name: remove standard user
+      win_user:
+        name: '{{standard_user}}'
+        state: absent
 
 - name: Use invalid parameter
   reboot:
     foo: bar
-  ignore_errors: yes
+  ignore_errors: true
   register: invalid_parameter
 
 - name: Ensure task fails with error