Add set_fact_persistent action and module. #26153

alikins · 2017-06-27T17:04:41Z

SUMMARY

Used just like set_fact, except facts set with set_fact_persistent
will be stored in the fact cache if fact caching is enabled.

set_fact only sets facts in the non_persistent_fact_cache, so they
are lost between invocations.

The implementations here are pretty much exact copies of set_fact.
The module likely needs to do that, but the action plugin could in theory
import set_fact and subclass it (all it needs is a different action name...)

ISSUE TYPE

Feature Pull Request
New Module Pull Request
Bugfix Pull Request
Docs Pull Request

COMPONENT NAME

modules/utilities/logic/set_fact.py
plugins/strategy/
plugins/action/

ANSIBLE VERSION

ansible 2.4.0 (set_fact_persistent c1e8b05ade) last updated 2017/08/02 10:00:27 (GMT -400)
  config file = /home/adrian/src/ansible/ansible.cfg
  configured module search path = [u'/home/adrian/.ansible/plugins/modules', u'/usr/share/ansible/plugins/modules']
  ansible python module location = /home/adrian/src/ansible/lib/ansible
  executable location = /home/adrian/src/ansible/bin/ansible
  python version = 2.7.13 (default, May 10 2017, 20:04:28) [GCC 6.3.1 20161221 (Red Hat 6.3.1-1)]

ADDITIONAL INFORMATION

ansibot · 2017-06-27T19:01:59Z

The test ansible-test sanity --test validate-modules failed with the following error:

lib/ansible/modules/utilities/logic/set_fact_persistent.py:0:0: E319 RETURN.ansible_facts.contains: required key not provided @ data['contains']. Got None

click here for bot help

bcoca · 2017-06-27T20:07:16Z

why not add a cacheable property to set_facts

alikins · 2017-06-27T20:36:27Z

why not add a cacheable property to set_facts

Mainly because this approach doesn't require changes to plugins/strategy/*
(strategies could check for a property/attr on the action instead of the explicit check for 'action == "set_fact"' currently used in strategy)

bcoca · 2017-06-27T20:38:10Z

using a 'cacheable' flag would be more flexible than the current 'set_fact' match, other modules could create non persistent/persistent facts

alikins · 2017-06-27T21:31:06Z

@bcoca can add a cacheable Attribute to Task, and set it from set_fact.py (or normal.py) and check it in strategy

set_fact module assumes all params passed to it are key=value pairs however, so adding a 'cacheable' arg to the module could cause conflicts (seemingly even 'args').

If we avoid the potential name conflict and dont add a module arg for skip_facts, then it will have to default to false and we need another module that could take a 'cacheable' arg. But at the moment, all other modules are already cacheable, but would still need a set_fact_persistent that enables cacheable and set_fact.

Or we could make 'cacheable' a prohibited fact name to use with set_fact...

bcoca · 2017-06-27T21:37:57Z

anyone setting 'args' as a var is already creating issues for themselves, other 'raw' parameter modules allow options (see creates= in shell/command) so it would not be a first.

my worry about introducing a new module is also that it introduces a new precedence step, while introducing a toggle between the 2 types of facts does not.

ansibot · 2017-06-28T16:03:34Z

cc @dagwieers
click here for bot help

alikins · 2017-06-28T16:09:41Z

Updated.

Not sure if the behavior on regular set_fact is correct. Logic was updated so that set_fact always sets non_persistent_facts and if cacheable, also sets host_facts.

That seems to do the right thing, but not sure about the details of the precedence.

Without the change, the behavior when using caching and non-caching set_fact on the same fact name is a little odd. A fact set non caching, and then later set with caching, the non caching value wins because of the higher precedence of non_persistent facts. Adding to non persistent facts and host_facts seems to DWIM there, but unsure of consequences elsewhere.

bcoca · 2017-06-28T16:49:16Z

its odd, but it is documented, set_fact wins over 'actual facts' .. the real difference being cacheable/non-cacheable.

I don't think this works well as task keyword as most tasks don't return facts, only module that return 'ansible_facts' do.

alikins · 2017-06-28T17:39:49Z

using a 'cacheable' flag would be more flexible than the current 'set_fact' match, other modules could create non persistent/persistent facts

I don't think this works well as task keyword as most tasks don't return facts, only module that return 'ansible_facts' do.

So, just a '_ansible_fact_cacheable' in module/action return and checked by strategy?

jmighion · 2017-07-06T17:27:52Z

I like this approach. It would allow me to cache facts that are important to me, but probably not important enough to make upstream changes to a specific fact module and eliminate maintaining local changes to modules over the course of Ansible releases.

isopy · 2017-08-01T20:51:23Z

Definitely gotta +1 a cacheable option for set_facts.

bpleines · 2017-08-01T21:09:11Z

+1

harrytruman · 2017-08-01T21:09:28Z

+1

Used just like set_fact, except facts set with cacheable: true will be stored in the fact cache if fact caching is enabled. set_fact normally only sets facts in the non_persistent_fact_cache, so they are lost between invocations.

We dont want to use 'ansible_facts_cacheable' result item or 'cacheable' arg as actual facts, so pop them out of the dicts.

* Add 'cacheable' param to set_fact action and module. Used just like set_fact, except facts set with cacheable: true will be stored in the fact cache if fact caching is enabled. set_fact normally only sets facts in the non_persistent_fact_cache, so they are lost between invocations. * update set_facts docs * use 'ansible_facts_cacheable' in module/actions result * pop fact cacheable related items out of args/results We dont want to use 'ansible_facts_cacheable' result item or 'cacheable' arg as actual facts, so pop them out of the dicts.

abadger · 2017-08-11T13:45:32Z

lib/ansible/plugins/strategy/__init__.py

@@ -511,12 +511,14 @@ def parent_handler_match(target_handler, handler_name):
                                for target_host in host_list:
                                    self._variable_manager.set_host_variable(target_host, var_name, var_value)
                        else:
+                            cacheable = result_item.pop('ansible_facts_cacheable', True)


I don't mess around in strategy often so I don't know if this is normal but,, why is the default here True but False everywhere else?

because set_fact is only module in which this was not true, all other 'ansible_facts' should be cacheable by default.

ansibot added ci_verified Changes made in this PR are causing tests to fail. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed community_review In order to be merged, this PR must follow the community review workflow. labels Jun 27, 2017

alikins closed this Jun 28, 2017

alikins reopened this Jun 28, 2017

ansibot removed the ci_verified Changes made in this PR are causing tests to fail. label Jun 28, 2017

ansibot added c:playbook/task c:plugins/strategy and removed needs_triage Needs a first human triage before being processed. new_module This PR includes a new module. labels Jun 28, 2017

ansibot added core_review In order to be merged, this PR must follow the core review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Jun 28, 2017

ansibot added the support:core This issue/PR relates to code supported by the Ansible Engineering Team. label Jun 28, 2017

ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Jul 18, 2017

alikins added this to the 2.4.0 milestone Aug 2, 2017

mckerrj assigned alikins Aug 2, 2017

alikins added 4 commits August 2, 2017 13:46

Add 'cacheable' param to set_fact action and module.

d4d99ee

Used just like set_fact, except facts set with cacheable: true will be stored in the fact cache if fact caching is enabled. set_fact normally only sets facts in the non_persistent_fact_cache, so they are lost between invocations.

update set_facts docs

1102d5e

use 'ansible_facts_cacheable' in module/actions result

13158b9

pop fact cacheable related items out of args/results

1403ff1

We dont want to use 'ansible_facts_cacheable' result item or 'cacheable' arg as actual facts, so pop them out of the dicts.

ansibot removed the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Aug 2, 2017

alikins merged commit 6fbd0a8 into ansible:devel Aug 2, 2017

bcoca mentioned this pull request Aug 10, 2017

allow 'nonpersistent facts' for other action plugins #17420

Closed

abadger reviewed Aug 11, 2017

View reviewed changes

alikins mentioned this pull request Oct 30, 2017

fix precedence issue with facts #32302

Merged

ansibot added feature This issue/PR relates to a feature request. and removed feature_pull_request labels Mar 5, 2018

ansible locked and limited conversation to collaborators Apr 26, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add set_fact_persistent action and module. #26153

Add set_fact_persistent action and module. #26153

alikins commented Jun 27, 2017 •

edited

ansibot commented Jun 27, 2017

bcoca commented Jun 27, 2017

alikins commented Jun 27, 2017

bcoca commented Jun 27, 2017

alikins commented Jun 27, 2017

bcoca commented Jun 27, 2017

ansibot commented Jun 28, 2017

alikins commented Jun 28, 2017

bcoca commented Jun 28, 2017

alikins commented Jun 28, 2017

jmighion commented Jul 6, 2017

isopy commented Aug 1, 2017

bpleines commented Aug 1, 2017

harrytruman commented Aug 1, 2017

abadger Aug 11, 2017

bcoca Aug 11, 2017

Add set_fact_persistent action and module. #26153

Add set_fact_persistent action and module. #26153

Conversation

alikins commented Jun 27, 2017 • edited

SUMMARY

ISSUE TYPE

COMPONENT NAME

ANSIBLE VERSION

ADDITIONAL INFORMATION

ansibot commented Jun 27, 2017

bcoca commented Jun 27, 2017

alikins commented Jun 27, 2017

bcoca commented Jun 27, 2017

alikins commented Jun 27, 2017

bcoca commented Jun 27, 2017

ansibot commented Jun 28, 2017

alikins commented Jun 28, 2017

bcoca commented Jun 28, 2017

alikins commented Jun 28, 2017

jmighion commented Jul 6, 2017

isopy commented Aug 1, 2017

bpleines commented Aug 1, 2017

harrytruman commented Aug 1, 2017

abadger Aug 11, 2017

Choose a reason for hiding this comment

bcoca Aug 11, 2017

Choose a reason for hiding this comment

alikins commented Jun 27, 2017 •

edited