Repository: ansible/ansible
Pull request: Reboot - add parameter for paths to search for shutdown command
##### SUMMARY

When [`molly-guard`](https://en.wiktionary.org/wiki/molly-guard) is installed, the normal `shutdown` command is moved to `/lib/molly-guard/`. Add a parameter to allow the default search paths to be overridden.

Fixes #51190 

##### ISSUE TYPE
<!--- Pick one below and delete the rest -->
- Feature Pull Request


##### COMPONENT NAME
`reboot.py`

Problem statement:
[reboot] Ansible hangs on molly-guard prompt if molly-guard is installed on target host.
<!--- Verify first that your issue is not already reported on GitHub -->
<!--- Also test if the latest release and devel branch are affected too -->
<!--- Complete *all* sections as described, this form is processed automatically -->

##### SUMMARY
<!--- Explain the problem briefly below -->
The `reboot` module that was introduced in Ansible 2.7 does not work when `molly-guard` is installed on the remote host.

##### ISSUE TYPE
- Bug Report

##### COMPONENT NAME
<!--- Write the short name of the module, plugin, task or feature below, use your best guess if unsure -->
reboot

##### ANSIBLE VERSION
<!--- Paste verbatim output from "ansible --version" between quotes -->
```paste below
ansible 2.7.6
 config file = /home/redacted/.ansible.cfg
 configured module search path = ['/home/redacted/.ansible/plugins/modules', '/usr/share/ansible/plugins/modules']
 ansible python module location = /home/redacted/redacted/.venv/lib/python3.7/site-packages/ansible
 executable location = /home/redacted/redacted/.venv/bin/ansible
 python version = 3.7.2 (default, Jan  3 2019, 02:55:40) [GCC 8.2.0]
```

##### CONFIGURATION
<!--- Paste verbatim output from "ansible-config dump --only-changed" between quotes -->
```paste below
ANSIBLE_NOCOWS(/home/redacted/.ansible.cfg) = True
DEFAULT_ACTION_PLUGIN_PATH(/home/redacted/.ansible.cfg) = ['/home/redacted/.ansible/plugins/action', '/usr/share/ansible/plugins/action']
DEFAULT_CACHE_PLUGIN_PATH(/home/redacted/.ansible.cfg) = ['/home/redacted/.ansible/plugins/cache', '/usr/share/ansible/plugins/cache']
DEFAULT_CALLBACK_PLUGIN_PATH(/home/redacted/.ansible.cfg) = ['/home/redacted/.ansible/plugins/callback', '/usr/share/ansible/plugins/callback']
DEFAULT_CONNECTION_PLUGIN_PATH(/home/redacted/.ansible.cfg) = ['/home/redacted/.ansible/plugins/connection', '/usr/share/ansible/plugins/connection']
DEFAULT_FILTER_PLUGIN_PATH(/home/redacted/.ansible.cfg) = ['/home/redacted/.ansible/plugins/filter', '/usr/share/ansible/plugins/filter']
DEFAULT_INVENTORY_PLUGIN_PATH(/home/redacted/.ansible.cfg) = ['/home/redacted/.ansible/plugins/inventory', '/usr/share/ansible/plugins/inventory']
DEFAULT_LOOKUP_PLUGIN_PATH(/home/redacted/.ansible.cfg) = ['/home/redacted/.ansible/plugins/lookup', '/usr/share/ansible/plugins/lookup']
DEFAULT_REMOTE_USER(/home/redacted/.ansible.cfg) = redacted
DEFAULT_STRATEGY_PLUGIN_PATH(/home/redacted/.ansible.cfg) = ['/home/redacted/.ansible/plugins/strategy', '/usr/share/ansible/plugins/strategy']
DEFAULT_TERMINAL_PLUGIN_PATH(/home/redacted/.ansible.cfg) = ['/home/redacted/.ansible/plugins/terminal', '/usr/share/ansible/plugins/terminal']
DEFAULT_TEST_PLUGIN_PATH(/home/redacted/.ansible.cfg) = ['/home/redacted/.ansible/plugins/test', '/usr/share/ansible/plugins/test']
DEFAULT_VARS_PLUGIN_PATH(/home/redacted/.ansible.cfg) = ['/home/redacted/.ansible/plugins/vars', '/usr/share/ansible/plugins/vars']
HOST_KEY_CHECKING(/home/redacted/.ansible.cfg) = False
```

##### OS / ENVIRONMENT
<!--- Provide all relevant information below, e.g. target OS versions, network device firmware, etc. -->
Target OS: Ubuntu 16.04.5

##### STEPS TO REPRODUCE
<!--- Describe exactly how to reproduce the problem, using a minimal test-case -->
On the target host running Ubuntu 16.04:
```
apt-get install molly-guard
```

Then, run the `reboot` module on that host.

<!--- Paste example playbooks or commands between quotes below -->
```yaml
---
- hosts: all
  tasks:
    - reboot:
```

<!--- HINT: You can paste gist.github.com links for larger files -->

##### EXPECTED RESULTS
<!--- Describe what you expected to happen when running the steps above -->
The host should reboot and the playbook should continue once the host is back online.

With molly-guard installed, the reboot module would presumably circumvent molly-guard by running `/lib/molly-guard/reboot` instead of `/sbin/reboot`.

##### ACTUAL RESULTS
<!--- Describe what actually happened. If possible run with extra verbosity (-vvvv) -->
Ansible hangs while molly-guard waits for ansible to enter the machine's hostname, which it never does.

<!--- Paste verbatim command output between quotes -->
```paste below
(No output given as Ansible will just hang.)
```

Hints from issue discussion:
Files identified in the description:
* [lib/ansible/modules/system/reboot.py](https://github.com/ansible/ansible/blob/devel/lib/ansible/modules/system/reboot.py)

If these files are inaccurate, please update the `component name` section of the description or use the `!component` bot command.

[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: components_banner --->
cc @nitzmahone @samdoran
[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: notify --->
This is probably the point at which we should move to making `DEFAULT_SHUTDOWN_COMMAND` and `DEFAULT_SHUTDOWN_COMMAND_ARGS` parameters that can be passed to the plugin. I have been hesitant to do that, but that is probably better than coding around this.
Actually, I just tested adding `/lib/molly-guard/` to the paths to search for the shutdown command and that works. Maybe that is the simplest solution.
@GoneWacko Can you test with #51194 and add a `search_paths:` that includes `/lib/molly-guard` before other paths?

Since this is a new feature it will not be available until Ansible >= 2.8.0.
@samdoran I tested with an inventory of 4 hosts, two of which had molly-guard installed, and two of which did not (all were Ubuntu 16.04).

I can verify that it works; all four hosts rebooted correctly - very nice! Thanks for the quick work.

Playbook used:
```
---
- hosts: all
  gather_facts: no
  tasks:
    - name: Reboot
      become: yes
      reboot:
        search_paths: ['/lib/molly-guard', '/sbin']
```
Thank you for the feedback. I will get this merged.

Commit message: Address feedback

Patch to review:
diff --git a/changelogs/fragments/reboot-add-search-paths-parameter.yaml b/changelogs/fragments/reboot-add-search-paths-parameter.yaml
new file mode 100644
index 00000000000000..aab697c6dcde8b
--- /dev/null
+++ b/changelogs/fragments/reboot-add-search-paths-parameter.yaml
@@ -0,0 +1,2 @@
+minor_changes:
+  - 'reboot - add parameter for specifying paths to search for the ``shutdown`` command (https://github.com/ansible/ansible/issues/51190)'
diff --git a/lib/ansible/modules/system/reboot.py b/lib/ansible/modules/system/reboot.py
index 9e45ed40031e90..c22969678a94ad 100644
--- a/lib/ansible/modules/system/reboot.py
+++ b/lib/ansible/modules/system/reboot.py
@@ -13,6 +13,9 @@
 DOCUMENTATION = r'''
 module: reboot
 short_description: Reboot a machine
+notes:
+  - C(PATH) is ignored on the remote node when searching for the C(shutdown) command. Use C(search_paths)
+    to specify locations to search if the default paths do not work.
 description:
     - Reboot a machine, wait for it to go down, come back up, and respond to commands.
     - For Windows targets, use the M(win_reboot) module instead.
@@ -54,6 +57,14 @@
       - Message to display to users before reboot.
     type: str
     default: Reboot initiated by Ansible
+
+  search_paths:
+    description:
+      - Paths to search on the remote machine for the C(shutdown) command.
+      - I(Only) these paths will be searched for the C(shutdown) command. C(PATH) is ignored in the remote node when searching for the C(shutdown) command.
+    type: list
+    default: ['/sbin', '/usr/sbin', '/usr/local/sbin']
+    version_added: '2.8'
 seealso:
 - module: win_reboot
 author:
diff --git a/lib/ansible/plugins/action/reboot.py b/lib/ansible/plugins/action/reboot.py
index c2d92e86a376ca..b1cea8196fe3f3 100644
--- a/lib/ansible/plugins/action/reboot.py
+++ b/lib/ansible/plugins/action/reboot.py
@@ -12,6 +12,7 @@
 
 from ansible.errors import AnsibleError, AnsibleConnectionFailure
 from ansible.module_utils._text import to_native, to_text
+from ansible.module_utils.common.collections import is_string
 from ansible.plugins.action import ActionBase
 from ansible.utils.display import Display
 
@@ -24,7 +25,7 @@ class TimedOutException(Exception):
 
 class ActionModule(ActionBase):
     TRANSFERS_FILES = False
-    _VALID_ARGS = frozenset(('connect_timeout', 'msg', 'post_reboot_delay', 'pre_reboot_delay', 'test_command', 'reboot_timeout'))
+    _VALID_ARGS = frozenset(('connect_timeout', 'msg', 'post_reboot_delay', 'pre_reboot_delay', 'test_command', 'reboot_timeout', 'search_paths'))
 
     DEFAULT_REBOOT_TIMEOUT = 600
     DEFAULT_CONNECT_TIMEOUT = None
@@ -130,13 +131,37 @@ def get_distribution(self, task_vars):
 
     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))
+        default_search_paths = ['/sbin', '/usr/sbin', '/usr/local/sbin']
+        search_paths = self._task.args.get('search_paths', default_search_paths)
+
+        # FIXME: switch all this to user arg spec validation methods when they are available
+        # Convert bare strings to a list
+        if is_string(search_paths):
+            search_paths = [search_paths]
+
+        # Error if we didn't get a list
+        if not isinstance(search_paths, list):
+            raise AnsibleError("'search_paths' must be a string or list, got {0}: {1}".format(type(search_paths), search_paths))
+
+        # If we did get a list, ensure it's a flat list and does not contain nested data structures
+        nested_list = any(isinstance(x, (dict, list)) for x in search_paths)
+        if nested_list:
+            raise AnsibleError("'search_paths' must be a flat list of paths, got {0}".format(search_paths))
+
+        # Ensure each search path is a string
+        for path in search_paths:
+            if not is_string(path):
+                raise AnsibleError("'search_paths' must contain strings, got {0} for {1}".format(type(path), path))
+
+        display.debug('{action}: running find module looking in {paths} to get path for "{command}"'.format(
+            action=self._task.action,
+            command=shutdown_bin,
+            paths=search_paths))
         find_result = self._execute_module(
             task_vars=task_vars,
             module_name='find',
             module_args={
-                'paths': ['/sbin', '/usr/sbin', '/usr/local/sbin'],
+                'paths': search_paths,
                 'patterns': [shutdown_bin],
                 'file_type': 'any'
             }
@@ -144,7 +169,7 @@ def get_shutdown_command(self, task_vars, distribution):
 
         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))
+            raise AnsibleError('Unable to find command "{0}" in search paths: {1}'.format(shutdown_bin, search_paths))
         self._shutdown_command = full_path[0]
         return self._shutdown_command
 
diff --git a/test/integration/targets/reboot/tasks/main.yml b/test/integration/targets/reboot/tasks/main.yml
index 7f7d1bb72e6d10..2568b9b2902c2a 100644
--- a/test/integration/targets/reboot/tasks/main.yml
+++ b/test/integration/targets/reboot/tasks/main.yml
@@ -37,11 +37,13 @@
     - name: Reboot with all options
       reboot:
         connect_timeout: 30
+        search_paths: /usr/local/bin
         msg: Rebooting
         post_reboot_delay: 1
         pre_reboot_delay: 61
         test_command: uptime
         reboot_timeout: 500
+      register: reboot_result
 
     - import_tasks: check_reboot.yml
 
@@ -51,6 +53,7 @@
       reboot:
         post_reboot_delay: -0.5
         pre_reboot_delay: -61
+      register: reboot_result
 
     - import_tasks: check_reboot.yml
 
@@ -75,6 +78,27 @@
       vars:
         timeout: "{{ timeout_value[ansible_facts['distribution'] | lower] | default(60) }}"
 
+    - name: Test molly-guard
+      block:
+        - import_tasks: get_boot_time.yml
+
+        - name: Install molly-guard
+          apt:
+            update_cache: yes
+            name: molly-guard
+            state: present
+
+        - name: Reboot when molly-guard is installed
+          reboot:
+            search_paths: /lib/molly-guard
+          register: reboot_result
+
+        - import_tasks: check_reboot.yml
+
+      when: ansible_facts.distribution in ['Debian', 'Ubuntu']
+      tags:
+        - molly-guard
+
   always:
     - name: Cleanup temp file
       file:


Code review comments:
diff_hunk: @@ -142,10 +143,15 @@ def get_shutdown_command(self, task_vars, distribution):
         if not isinstance(search_paths, list):
             raise AnsibleError("'search_paths' must be a string or list, got {0}: {1}".format(type(search_paths), search_paths))
 
-        # If we did get a list, ensure it's a flast list and does not contain nested data structures
+        # If we did get a list, ensure it's a flat list and does not contain nested data structures
         nested_list = any(isinstance(x, (dict, list)) for x in search_paths)
path: lib/ansible/plugins/action/reboot.py
text: @user1:
redundant with the is_string check below now; just meld those two together (so you're keeping the more declarative "any" rather than the explicit loop) and you're good to go

@author:
Thanks for the info. I simplified it quite a bit.