From 119b65f919bc3c596841471135817b8cfe0a886c Mon Sep 17 00:00:00 2001 From: Brian Coca Date: Tue, 15 Jan 2019 15:20:33 -0500 Subject: [PATCH] Clarify clear facts (#50667) * Revert "avoid x2 setting of set_fact when 'cacheable' (#50564)" This reverts commit 207848f354efb556fb79664c41d426d7ba3ea4d8. * clarify clear_facts with set_fact cacheable revert previous 'fix' as it will break playbooks by changing precedence opted to leave current behaviour but document it on both plugins to mitigate confusion fixes #50556 also fix grammer, add comment, remove unused e --- changelogs/fragments/fix_set_fact_cacheable.yml | 2 -- docs/docsite/rst/user_guide/playbooks_variables.rst | 3 ++- lib/ansible/executor/task_executor.py | 2 +- lib/ansible/modules/utilities/helper/meta.py | 1 + lib/ansible/modules/utilities/logic/set_fact.py | 3 +++ lib/ansible/plugins/strategy/__init__.py | 13 +++++++++---- 6 files changed, 16 insertions(+), 8 deletions(-) delete mode 100644 changelogs/fragments/fix_set_fact_cacheable.yml diff --git a/changelogs/fragments/fix_set_fact_cacheable.yml b/changelogs/fragments/fix_set_fact_cacheable.yml deleted file mode 100644 index 1b6a283ac7e..00000000000 --- a/changelogs/fragments/fix_set_fact_cacheable.yml +++ /dev/null @@ -1,2 +0,0 @@ -bugfixes: - - fix issue in which 'cacheable' was not being properly applied and the fact was x2 defined diff --git a/docs/docsite/rst/user_guide/playbooks_variables.rst b/docs/docsite/rst/user_guide/playbooks_variables.rst index cd35e646200..e16de78d555 100644 --- a/docs/docsite/rst/user_guide/playbooks_variables.rst +++ b/docs/docsite/rst/user_guide/playbooks_variables.rst @@ -1036,7 +1036,8 @@ Basically, anything that goes into "role defaults" (the defaults folder inside t .. [1] Tasks in each role will see their own role's defaults. Tasks defined outside of a role will see the last role's defaults. .. [2] Variables defined in inventory file or provided by dynamic inventory. .. [3] Includes vars added by 'vars plugins' as well as host_vars and group_vars which are added by the default vars plugin shipped with Ansible. -.. [4] When created with set_facts's cacheable option. +.. [4] When created with set_facts's cacheable option, variables will have the high precedence in the play, + but will be the same as a host facts precedence when they come from the cache. .. note:: Within any section, redefining a var will overwrite the previous instance. If multiple groups have the same variable, the last one loaded wins. diff --git a/lib/ansible/executor/task_executor.py b/lib/ansible/executor/task_executor.py index fc5605210ca..c462e70bef7 100644 --- a/lib/ansible/executor/task_executor.py +++ b/lib/ansible/executor/task_executor.py @@ -684,7 +684,7 @@ class TaskExecutor: return failed_when_result if 'ansible_facts' in result: - if self._task.action in ('include_vars', 'set_fact'): + if self._task.action in ('set_fact', 'include_vars'): vars_copy.update(result['ansible_facts']) else: # TODO: cleaning of facts should eventually become part of taskresults instead of vars diff --git a/lib/ansible/modules/utilities/helper/meta.py b/lib/ansible/modules/utilities/helper/meta.py index 150c61e5aaf..08dbea0bac4 100644 --- a/lib/ansible/modules/utilities/helper/meta.py +++ b/lib/ansible/modules/utilities/helper/meta.py @@ -45,6 +45,7 @@ options: notes: - C(meta) is not really a module nor action_plugin as such it cannot be overwritten. - This module is also supported for Windows targets. + - "C(clear_facts) will remove the persistent facts from ``set_fact: cacheable=True``, but not the current host variable it creates for the current run." author: - "Ansible Core Team" ''' diff --git a/lib/ansible/modules/utilities/logic/set_fact.py b/lib/ansible/modules/utilities/logic/set_fact.py index bd5915b7cf6..eba883cbfa3 100644 --- a/lib/ansible/modules/utilities/logic/set_fact.py +++ b/lib/ansible/modules/utilities/logic/set_fact.py @@ -40,6 +40,9 @@ options: - Normally this module creates 'host level variables' and has much higher precedence, this option changes the nature and precedence (by 7 steps) of the variable created. https://docs.ansible.com/ansible/latest/user_guide/playbooks_variables.html#variable-precedence-where-should-i-put-a-variable + - "This actually creates 2 copies of the variable, a normal 'set_fact' host variable with high precedence and + a lower 'ansible_fact' one that is available for persistance via the facts cache plugin. + This creates a possibly confusing interaction with ``meta: clear_facts`` as it will remove the 'ansible_fact' but not the host variable." type: bool default: 'no' version_added: "2.4" diff --git a/lib/ansible/plugins/strategy/__init__.py b/lib/ansible/plugins/strategy/__init__.py index eefcad51c13..4c98ab023e2 100644 --- a/lib/ansible/plugins/strategy/__init__.py +++ b/lib/ansible/plugins/strategy/__init__.py @@ -570,10 +570,15 @@ class StrategyBase: else: cacheable = result_item.pop('_ansible_facts_cacheable', False) for target_host in host_list: - if original_task.action == 'set_fact' and not cacheable: - self._variable_manager.set_nonpersistent_facts(target_host, result_item['ansible_facts'].copy()) - else: + # so set_fact is a misnomer but 'cacheable = true' was meant to create an 'actual fact' + # to avoid issues with precedence and confusion with set_fact normal operation, + # we set BOTH fact and nonpersistent_facts (aka hostvar) + # when fact is retrieved from cache in subsequent operations it will have the lower precedence, + # but for playbook setting it the 'higher' precedence is kept + if original_task.action != 'set_fact' or cacheable: self._variable_manager.set_host_facts(target_host, result_item['ansible_facts'].copy()) + if original_task.action == 'set_fact': + self._variable_manager.set_nonpersistent_facts(target_host, result_item['ansible_facts'].copy()) if 'ansible_stats' in result_item and 'data' in result_item['ansible_stats'] and result_item['ansible_stats']['data']: @@ -892,7 +897,7 @@ class StrategyBase: loader=self._loader, variable_manager=self._variable_manager ) - except AnsibleError as e: + except AnsibleError: return False result = True