Repository: ansible/ansible
Pull request: Fix delegate_facts with interpreter not being set
##### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
Fixes https://github.com/ansible/ansible/issues/70168

Introduced in https://github.com/ansible/ansible/pull/69604 which was backported to `2.9.10`. This would need to be backported to `2.9` and `2.10`.

This is another take to get these fixes in after they were reverted in https://github.com/ansible/ansible/pull/70272 due to issues with testing the fixes in our CI.
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->

##### ISSUE TYPE
<!--- Pick one below and delete the rest -->
- Bugfix Pull Request

##### COMPONENT NAME
<!--- Write the short name of the module, plugin, task or feature below -->
` lib/ansible/plugins/action/__init__.py`

Problem statement:
2.9.10: UnboundLocalError: local variable 'module_style' referenced before assignment
<!--- 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 -->
after upgrade from 2.9.9 to 2.9.10 we see an exception in a task using ~~lineinfile~~ `delegate_facts=true`

##### ISSUE TYPE
- Bug Report

##### COMPONENT NAME
<!--- Write the short name of the module, plugin, task or feature below, use your best guess if unsure -->

##### ANSIBLE VERSION
<!--- Paste verbatim output from "ansible --version" between quotes -->
```paste below
2.9.10
```

##### CONFIGURATION
<!--- Paste verbatim output from "ansible-config dump --only-changed" between quotes -->
```paste below
ANSIBLE_NOCOWS(/home/rmoser/Projects/puzzle/ansible-puzzle/ansible.cfg) = True
ANSIBLE_PIPELINING(/home/rmoser/Projects/puzzle/ansible-puzzle/ansible.cfg) = True
ANSIBLE_SSH_ARGS(/home/rmoser/Projects/puzzle/ansible-puzzle/ansible.cfg) = -o ControlMaster=auto -o ControlPersist=60s -o UserKnownHostsFile=/dev/null -o StrictHostKeyChecking=no
CACHE_PLUGIN(/home/rmoser/Projects/puzzle/ansible-puzzle/ansible.cfg) = jsonfile
CACHE_PLUGIN_CONNECTION(/home/rmoser/Projects/puzzle/ansible-puzzle/ansible.cfg) = .cache/facts
CACHE_PLUGIN_TIMEOUT(/home/rmoser/Projects/puzzle/ansible-puzzle/ansible.cfg) = 86400
COLLECTIONS_PATHS(/home/rmoser/Projects/puzzle/ansible-puzzle/ansible.cfg) = [u'/home/rmoser/Projects/puzzle/ansible-puzzle/collections_galaxy']
CONDITIONAL_BARE_VARS(/home/rmoser/Projects/puzzle/ansible-puzzle/ansible.cfg) = False
DEFAULT_BECOME(/home/rmoser/Projects/puzzle/ansible-puzzle/ansible.cfg) = True
DEFAULT_BECOME_ASK_PASS(/home/rmoser/Projects/puzzle/ansible-puzzle/ansible.cfg) = False
DEFAULT_BECOME_METHOD(/home/rmoser/Projects/puzzle/ansible-puzzle/ansible.cfg) = sudo
DEFAULT_BECOME_USER(/home/rmoser/Projects/puzzle/ansible-puzzle/ansible.cfg) = root
DEFAULT_FORKS(/home/rmoser/Projects/puzzle/ansible-puzzle/ansible.cfg) = 20
DEFAULT_GATHERING(/home/rmoser/Projects/puzzle/ansible-puzzle/ansible.cfg) = smart
DEFAULT_HOST_LIST(/home/rmoser/Projects/puzzle/ansible-puzzle/ansible.cfg) = [u'/home/rmoser/Projects/puzzle/ansible-puzzle/inventories/cloud_staging']
DEFAULT_LOAD_CALLBACK_PLUGINS(/home/rmoser/Projects/puzzle/ansible-puzzle/ansible.cfg) = False
DEFAULT_MANAGED_STR(/home/rmoser/Projects/puzzle/ansible-puzzle/ansible.cfg) = Warning: File is managed by Ansible
DEFAULT_PRIVATE_KEY_FILE(/home/rmoser/Projects/puzzle/ansible-puzzle/ansible.cfg) = /home/rmoser/.ssh/id_rsa
DEFAULT_REMOTE_USER(/home/rmoser/Projects/puzzle/ansible-puzzle/ansible.cfg) = ansible
DEFAULT_ROLES_PATH(/home/rmoser/Projects/puzzle/ansible-puzzle/ansible.cfg) = [u'/home/rmoser/Projects/puzzle/ansible-puzzle/roles_galaxy', u'/home/rmoser/Projects/puzzle/ansible-puzzle/roles']
DEFAULT_STDOUT_CALLBACK(/home/rmoser/Projects/puzzle/ansible-puzzle/ansible.cfg) = yaml
HOST_KEY_CHECKING(/home/rmoser/Projects/puzzle/ansible-puzzle/ansible.cfg) = True
INTERPRETER_PYTHON(/home/rmoser/Projects/puzzle/ansible-puzzle/ansible.cfg) = auto
RETRY_FILES_ENABLED(/home/rmoser/Projects/puzzle/ansible-puzzle/ansible.cfg) = False

```

##### OS / ENVIRONMENT
<!--- Provide all relevant information below, e.g. target OS versions, network device firmware, etc. -->


##### STEPS TO REPRODUCE
<!--- Describe exactly how to reproduce the problem, using a minimal test-case -->

<!--- Paste example playbooks or commands between quotes below -->
```yaml
---
- hosts: my.example.com
  gather_facts: yes
  tasks:
  - name: make something
    delegate_facts: True
    delegate_to: other.example.com
    lineinfile:
      path: /tmp/foo.txt
      regexp: '^Listen '
      insertafter: '^#Listen '
      line: Listen 8080

```

<!--- HINT: You can paste gist.github.com links for larger files -->

##### EXPECTED RESULTS
<!--- Describe what you expected to happen when running the steps above -->


##### ACTUAL RESULTS
<!--- Describe what actually happened. If possible run with extra verbosity (-vvvv) -->

<!--- Paste verbatim command output between quotes -->
```paste below
The full traceback is:
Traceback (most recent call last):
  File "/home/rmoser/Projects/resmo/ansible/lib/ansible/executor/task_executor.py", line 146, in run
    res = self._execute()
  File "/home/rmoser/Projects/resmo/ansible/lib/ansible/executor/task_executor.py", line 654, in _execute
    result = self._handler.run(task_vars=variables)
  File "/home/rmoser/Projects/resmo/ansible/lib/ansible/plugins/action/normal.py", line 46, in run
    result = merge_hash(result, self._execute_module(task_vars=task_vars, wrap_async=wrap_async))
  File "/home/rmoser/Projects/resmo/ansible/lib/ansible/plugins/action/__init__.py", line 831, in _execute_module
    (module_style, shebang, module_data, module_path) = self._configure_module(module_name=module_name, module_args=module_args, task_vars=task_vars)
  File "/home/rmoser/Projects/resmo/ansible/lib/ansible/plugins/action/__init__.py", line 244, in _configure_module
    return (module_style, module_shebang, module_data, module_path)
UnboundLocalError: local variable 'module_style' referenced before assignment
fatal: [server.example.com]: FAILED! => 
  msg: Unexpected failure during module execution.
  stdout: ''

```

Hints from issue discussion:
I couldn't reproduce this with an arbitrary lineinfile task. We'll need you to provide a minimal reproducer.
@mkrizek I added a reproducer. It seems related to `delegate_facts=true`
Sounds like this might be related to #69604.

cc @bcoca 
Can confirm, we have the same problem, though it only triggers in particular case (localhost -> localhost)

Playbook:
```
# this might seem as a complex construct for just runnig setup/facts gathering on the `all` group,                        
# but this way, we are sure to have all the facts of all the hosts even if the play was run on a `--limit`ed set of hosts!
- name: Setup facts
  hosts: localhost
  tasks:
  ┊ - name: Gather facts from hosts
  ┊ ┊ setup:
  ┊ ┊ ┊ gather_subset: "all"
  ┊ ┊ delegate_to: "{{ item }}"
  ┊ ┊ delegate_facts: True
  ┊ ┊ with_items: "{{ groups['all'] }}"
  tags:
  ┊ - always

```

Results:

![20200619143809](https://user-images.githubusercontent.com/382239/85133538-2a41ee80-b23b-11ea-9fd9-f0b138aca4ee.png)

Since we now pass `use_vars` instead of `task_vars` to `modify_module`, we need to update information in `use_vars` for the next iteration too. The following makes the problem go away:
```diff
diff --git a/lib/ansible/plugins/action/__init__.py b/lib/ansible/plugins/action/__init__.py
index 83c7715d3e..62a185a3fd 100644
--- a/lib/ansible/plugins/action/__init__.py
+++ b/lib/ansible/plugins/action/__init__.py
@@ -249,7 +249,9 @@ class ActionBase(with_metaclass(ABCMeta, object)):
                     # store in local task_vars facts collection for the retry and any other usages in this worker
                     if use_vars.get('ansible_facts') is None:
                         task_vars['ansible_facts'] = {}
+                        use_vars['ansible_facts'] = {}
                     task_vars['ansible_facts'][discovered_key] = self._discovered_interpreter
+                    use_vars['ansible_facts'][discovered_key] = self._discovered_interpreter
                     # preserve this so _execute_module can propagate back to controller as a fact
                     self._discovered_interpreter_key = discovered_key
                 else:
```
The similar would need to be changed in the `else` branch as well. I am not sure if changing `task_vars` is needed though, or if changing all occurrences of `task_vars` to `use_vars` would be the fix.
Reopening since the fix for this was reverted in #70272.
as per IRC chat in #ansible-devel :

> @mkrizek iirc the issue was related to using `delegate_to` AND `delegate_facts` AND `ansible_python_interpreter` not being explicitly set (set set to `auto`)

that explains why it's seen in ceph-ansible ci but we aren't seeing it in tripleo ci; in tripleo we forcibly set ansible_python_interpreter [1]

1. https://github.com/openstack/tripleo-heat-templates/blob/stable/train/common/deploy-steps.j2#L1091

Commit message: Fix permissions

Patch to review:
diff --git a/changelogs/fragments/70168-fix-delegate_facts-without-interpreter-set.yml b/changelogs/fragments/70168-fix-delegate_facts-without-interpreter-set.yml
new file mode 100644
index 00000000000000..371778eabb2fc3
--- /dev/null
+++ b/changelogs/fragments/70168-fix-delegate_facts-without-interpreter-set.yml
@@ -0,0 +1,2 @@
+bugfixes:
+  - "Fix ``delegate_facts: true`` when ``ansible_python_interpreter`` is not set. (https://github.com/ansible/ansible/issues/70168)"
diff --git a/lib/ansible/plugins/action/__init__.py b/lib/ansible/plugins/action/__init__.py
index 83c7715d3ee16e..f1e0a14bca6e66 100644
--- a/lib/ansible/plugins/action/__init__.py
+++ b/lib/ansible/plugins/action/__init__.py
@@ -160,7 +160,7 @@ def _configure_module(self, module_name, module_args, task_vars=None):
         '''
 
         if task_vars is None:
-            use_vars = dict()
+            task_vars = {}
 
         if self._task.delegate_to:
             use_vars = task_vars.get('ansible_delegated_vars')[self._task.delegate_to]
@@ -242,18 +242,22 @@ def _configure_module(self, module_name, module_args, task_vars=None):
                 # we'll propagate back to the controller in the task result
                 discovered_key = 'discovered_interpreter_%s' % idre.interpreter_name
 
+                # update the local vars copy for the retry
+                if use_vars.get('ansible_facts') is None:
+                    use_vars['ansible_facts'] = {}
+                use_vars['ansible_facts'][discovered_key] = self._discovered_interpreter
+
                 # TODO: this condition prevents 'wrong host' from being updated
                 # but in future we would want to be able to update 'delegated host facts'
                 # irrespective of task settings
                 if not self._task.delegate_to or self._task.delegate_facts:
                     # store in local task_vars facts collection for the retry and any other usages in this worker
-                    if use_vars.get('ansible_facts') is None:
+                    if task_vars.get('ansible_facts') is None:
                         task_vars['ansible_facts'] = {}
                     task_vars['ansible_facts'][discovered_key] = self._discovered_interpreter
                     # preserve this so _execute_module can propagate back to controller as a fact
                     self._discovered_interpreter_key = discovered_key
                 else:
-                    task_vars['ansible_delegated_vars'][self._task.delegate_to]
                     if task_vars['ansible_delegated_vars'][self._task.delegate_to].get('ansible_facts') is None:
                         task_vars['ansible_delegated_vars'][self._task.delegate_to]['ansible_facts'] = {}
                     task_vars['ansible_delegated_vars'][self._task.delegate_to]['ansible_facts'][discovered_key] = self._discovered_interpreter
diff --git a/test/integration/targets/interpreter_discovery_python_delegate_facts/aliases b/test/integration/targets/interpreter_discovery_python_delegate_facts/aliases
new file mode 100644
index 00000000000000..dc9ac4682ba94a
--- /dev/null
+++ b/test/integration/targets/interpreter_discovery_python_delegate_facts/aliases
@@ -0,0 +1,2 @@
+shippable/posix/group1
+non_local  # this test requires interpreter discovery, which means code coverage must be disabled
diff --git a/test/integration/targets/interpreter_discovery_python_delegate_facts/delegate_facts.yml b/test/integration/targets/interpreter_discovery_python_delegate_facts/delegate_facts.yml
new file mode 100644
index 00000000000000..535269d1602ce0
--- /dev/null
+++ b/test/integration/targets/interpreter_discovery_python_delegate_facts/delegate_facts.yml
@@ -0,0 +1,10 @@
+- hosts: localhost
+  gather_facts: no
+  tasks:
+    - name: Test python interpreter discovery with delegate_to without delegate_facts
+      ping:
+      delegate_to: testhost
+    - name: Test python interpreter discovery with delegate_to with delegate_facts
+      ping:
+      delegate_to: testhost
+      delegate_facts: yes
diff --git a/test/integration/targets/interpreter_discovery_python_delegate_facts/inventory b/test/integration/targets/interpreter_discovery_python_delegate_facts/inventory
new file mode 100644
index 00000000000000..350f3e89996a41
--- /dev/null
+++ b/test/integration/targets/interpreter_discovery_python_delegate_facts/inventory
@@ -0,0 +1,2 @@
+[local]
+testhost ansible_connection=local ansible_python_interpreter=auto  # interpreter discovery required
diff --git a/test/integration/targets/interpreter_discovery_python_delegate_facts/runme.sh b/test/integration/targets/interpreter_discovery_python_delegate_facts/runme.sh
new file mode 100755
index 00000000000000..ca2caa1c86d7c1
--- /dev/null
+++ b/test/integration/targets/interpreter_discovery_python_delegate_facts/runme.sh
@@ -0,0 +1,5 @@
+#!/usr/bin/env bash
+
+set -eux
+
+ansible-playbook delegate_facts.yml -i inventory "$@"


Code review comments:
diff_hunk: @@ -242,18 +242,22 @@ def _configure_module(self, module_name, module_args, task_vars=None):
                 # we'll propagate back to the controller in the task result
                 discovered_key = 'discovered_interpreter_%s' % idre.interpreter_name
 
+                # update the local vars copy for the retry
+                if use_vars.get('ansible_facts') is None:
+                    use_vars['ansible_facts'] = {}
+                use_vars['ansible_facts'][discovered_key] = self._discovered_interpreter
+
                 # TODO: this condition prevents 'wrong host' from being updated
                 # but in future we would want to be able to update 'delegated host facts'
                 # irrespective of task settings
                 if not self._task.delegate_to or self._task.delegate_facts:
                     # store in local task_vars facts collection for the retry and any other usages in this worker
-                    if use_vars.get('ansible_facts') is None:
+                    if task_vars.get('ansible_facts') is None:
path: lib/ansible/plugins/action/__init__.py
text: @user1:
we should always have ansible_facts here, the if/set to empty should not be needed here, nor above.