Repository: ansible/ansible
Pull request: dont clobber facts in loop
fixes #71031

##### 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 -->
facts

Problem statement:
task fails when run in a loop using ansible_facts in a conditional and the module returns ansible_facts
##### SUMMARY
<!--- Explain the problem briefly below -->

(I'm so sorry for that title)

When a module that returns `ansible_facts` is run in a loop and has a conditional that is using `ansible_facts`, the first iteration of the loop works but subsequent iterations fail with the following error:

```
error while evaluating conditional (ansible_facts.virtualization_type != 'docker'): 'dict object' has no attribute 'virtualization_type'
```

##### ISSUE TYPE
- Bug Report

##### COMPONENT NAME
<!--- Write the short name of the module, plugin, task or feature below, use your best guess if unsure -->
`lib/ansible/executor/task_executor.py`
##### ANSIBLE VERSION
<!--- Paste verbatim output from "ansible --version" between quotes -->
```paste below
ansible 2.11.0.dev0
  config file = /Users/sdoran/.ansible.cfg
  configured module search path = ['/usr/local/share/ansible/library']
  ansible python module location = /Users/sdoran/Source/ansible/lib/ansible
  ansible collection location = /Users/sdoran/.ansible/collections:/usr/local/share/ansible/collections
  executable location = /Users/sdoran/Source/ansible/bin/ansible
  python version = 3.8.5 (default, Jul 28 2020, 14:08:44) [Clang 11.0.3 (clang-1103.0.32.62)]
```

##### CONFIGURATION
<!--- Paste verbatim output from "ansible-config dump --only-changed" between quotes -->
```paste below
DEFAULT_LOG_PATH(env: ANSIBLE_LOG_PATH) = /Users/sdoran/.ansible/ansible.log
DEFAULT_VAULT_PASSWORD_FILE(env: ANSIBLE_VAULT_PASSWORD_FILE) = /Users/sdoran/bin/getvaultkey.sh
DEVEL_WARNING(env: ANSIBLE_DEVEL_WARNING) = False
```

##### OS / ENVIRONMENT
<!--- Provide all relevant information below, e.g. target OS versions, network device firmware, etc. -->
```
ProductName:	Mac OS X
ProductVersion:	10.15.6
BuildVersion:	19G73
```

##### STEPS TO REPRODUCE
<!--- Describe exactly how to reproduce the problem, using a minimal test-case -->
Run the following playbook using the provided custom module
<!--- Paste example playbooks or commands between quotes below -->
`test-local.yml`
```yaml
- name: Local testing playbook
  hosts: localhost
  gather_facts: yes

  tasks:
    - testmodule:
      when: ansible_facts.virtualization_type != 'docker'
      loop:
        - one
        - two
        - three
```


`testmodule.py`
```python
#!/usr/bin/python
# -*- coding: utf-8 -*-

from __future__ import absolute_import, division, print_function
__metaclass__ = type

from ansible.module_utils.basic import AnsibleModule


def main():
    module = AnsibleModule(argument_spec={})
    module.exit_json(msg='exit', ansible_facts={})


if __name__ == '__main__':
    main()
```

<!--- HINT: You can paste gist.github.com links for larger files -->

##### EXPECTED RESULTS
<!--- Describe what you expected to happen when running the steps above -->
Each task in the loop executes successfully.

##### ACTUAL RESULTS
<!--- Describe what actually happened. If possible run with extra verbosity (-vvvv) -->

<!--- Paste verbatim command output between quotes -->
```paste below
> ansible-playbook test-local.yml -v
Using /Users/sdoran/Projects/Vagrant/Ansible Lab/ansible.cfg as config file
[WARNING]: Collection ansible.posix does not support Ansible version 2.11.0.dev0

PLAY [Local testing playbook] *****************************************************************************************************************************

TASK [Gathering Facts] ************************************************************************************************************************************
ok: [localhost]

TASK [reproducer] *****************************************************************************************************************************************
ok: [localhost] => (item=one) => {
    "ansible_facts": {
        "ansible_nodename": "mynode"
    },
    "ansible_loop_var": "item",
    "changed": false,
    "item": "one"
}

MSG:

exit
fatal: [localhost]: FAILED! => {}

MSG:

The conditional check 'ansible_facts.virtualization_type != 'docker'' failed. The error was: error while evaluating conditional (ansible_facts.virtualization_type != 'docker'): 'dict object' has no attribute 'virtualization_type'

The error appears to be in '/Users/sdoran/Projects/Vagrant/Ansible Lab/test-local.yml': line 6, column 7, but may
be elsewhere in the file depending on the exact syntax problem.

The offending line appears to be:

  tasks:
    - name: reproducer
      ^ here


PLAY RECAP ************************************************************************************************************************************************
localhost                  : ok=1    changed=0    unreachable=0    failed=1    skipped=0    rescued=0    ignored=0
```

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

If these files are incorrect, 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 --->
This patch from @bcoca seems to fix it in my limited testing:
```patch
diff --git a/lib/ansible/executor/task_executor.py b/lib/ansible/executor/task_executor.py
index 4d7162803c..d29af44812 100644
--- a/lib/ansible/executor/task_executor.py
+++ b/lib/ansible/executor/task_executor.py
@@ -621,7 +621,7 @@ class TaskExecutor:
                 else:
                     # TODO: cleaning of facts should eventually become part of taskresults instead of vars
                     af = wrap_var(result['ansible_facts'])
-                    vars_copy.update(namespace_facts(af))
+                    vars_copy['ansible_facts'] = combine_vars(vars_copy['ansible_facts'], namespace_facts(af))
                     if C.INJECT_FACTS_AS_VARS:
                         vars_copy.update(clean_facts(af))
@@ -685,7 +685,7 @@ class TaskExecutor:
             else:
                 # TODO: cleaning of facts should eventually become part of taskresults instead of vars
                 af = wrap_var(result['ansible_facts'])
-                variables.update(namespace_facts(af))
+                variables['ansible_facts'] = combine_vars(variables['ansible_facts'], namespace_facts(af))
                 if C.INJECT_FACTS_AS_VARS:
                     variables.update(clean_facts(@AnsibleFestAtlanta2019))
```

Commit message: allow for no facts in source

Patch to review:
diff --git a/lib/ansible/executor/task_executor.py b/lib/ansible/executor/task_executor.py
index ee0de5f9d6784b..2e64b613439c01 100644
--- a/lib/ansible/executor/task_executor.py
+++ b/lib/ansible/executor/task_executor.py
@@ -628,7 +628,7 @@ def _evaluate_failed_when_result(result):
                 else:
                     # TODO: cleaning of facts should eventually become part of taskresults instead of vars
                     af = wrap_var(result['ansible_facts'])
-                    vars_copy.update(namespace_facts(af))
+                    vars_copy['ansible_facts'] = combine_vars(vars_copy.get('ansible_facts', {}), namespace_facts(af))
                     if C.INJECT_FACTS_AS_VARS:
                         vars_copy.update(clean_facts(af))
 
@@ -692,7 +692,7 @@ def _evaluate_failed_when_result(result):
             else:
                 # TODO: cleaning of facts should eventually become part of taskresults instead of vars
                 af = wrap_var(result['ansible_facts'])
-                variables.update(namespace_facts(af))
+                variables['ansible_facts'] = combine_vars(variables.get('ansible_facts', {}), namespace_facts(af))
                 if C.INJECT_FACTS_AS_VARS:
                     variables.update(clean_facts(af))
 
diff --git a/test/integration/targets/gathering_facts/prevent_clobbering.yml b/test/integration/targets/gathering_facts/prevent_clobbering.yml
new file mode 100644
index 00000000000000..dff8f5dbca4ffd
--- /dev/null
+++ b/test/integration/targets/gathering_facts/prevent_clobbering.yml
@@ -0,0 +1,6 @@
+- hosts: localhost
+  gather_facts: True
+  tasks:
+  - bogus_facts:
+    loop: [1, 2, 3]
+    when: ansible_facts['virtualization_type'] != 'docker'
diff --git a/test/integration/targets/gathering_facts/runme.sh b/test/integration/targets/gathering_facts/runme.sh
index 5244b9ed051fc7..4635562710eea1 100755
--- a/test/integration/targets/gathering_facts/runme.sh
+++ b/test/integration/targets/gathering_facts/runme.sh
@@ -13,3 +13,6 @@ ansible-playbook test_prevent_injection.yml -i inventory -v "$@"
 
 # ensure fact merging is working properly
 ansible-playbook verify_merge_facts.yml -v "$@" -e 'ansible_facts_parallel: False'
+
+# ensure we dont clobber facts in loop
+ansible-playbook prevent_clobbering.yml -v "$@"


Code review comments:
diff_hunk: @@ -0,0 +1,6 @@
+- hosts: localhost
+  gather_facts: True
+  tasks:
+  - bogus_facts:
+    loop: [1, 2, 3]
+    when: ansible_facts['virtualization_type'] != 'docker'
path: test/integration/targets/gathering_facts/prevent_clobbering.yml
text: @user1:
This expression needs to be a conditional that always evaluates to `True`. It's currenty being skipped on our containers.

```
04:33 skipping: [localhost] => (item=1)  => {"ansible_loop_var": "item", "changed": false, "item": 1, "skip_reason": "Conditional result was False"}
04:33 skipping: [localhost] => (item=2)  => {"ansible_loop_var": "item", "changed": false, "item": 2, "skip_reason": "Conditional result was False"}
04:33 skipping: [localhost] => (item=3)  => {"ansible_loop_var": "item", "changed": false, "item": 3, "skip_reason": "Conditional result was False"}
```

We should also add a comment about what this test is actually testing since it's a bit obscure.

@author:
~skipping is fine, we are looking for undefined, so that just means 'it worked' .. but i'll change it anyways~ xcept i need one loop at least to work .... to try to overwrite the facts.