Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

in 2.0.1.0 and later, skipped plays impact gathering=smart #15744

Closed
larsks opened this issue May 5, 2016 · 4 comments · Fixed by #15747
Closed

in 2.0.1.0 and later, skipped plays impact gathering=smart #15744

larsks opened this issue May 5, 2016 · 4 comments · Fixed by #15747
Labels
bug This issue/PR relates to a bug. P2 Priority 2 - Issue Blocks Release
Milestone

Comments

@larsks
Copy link
Contributor

larsks commented May 5, 2016

ISSUE TYPE
  • Bug Report
ANSIBLE VERSION
ansible 2.0.1.0
  config file = 
  configured module search path = Default w/o overrides
CONFIGURATION

N/A

OS / ENVIRONMENT

N/A

SUMMARY

When ANSIBLE_GATHERING is smart, a skipped play targeting a
particular host will cause Ansible to not collect facts for that host.

STEPS TO REPRODUCE

Use this playbook:

- hosts: localhost
  tags:
    - sampletag
  tasks:
    - debug:
        msg: "this play can be skipped with --skip-tags sampletag"

- hosts: localhost
  tasks:
    - assert:
        that: ansible_distribution is defined

Compare:

ANSIBLE_GATHERING=implicit ansible-playbook playbook.yml \
  --skip-tags sampletag

With:

ANSIBLE_GATHERING=smart ansible-playbook playbook.yml \
  --skip-tags sampletag

In version 2.0.0.0, you will see:

PLAY ***************************************************************************

TASK [setup] *******************************************************************
ok: [localhost]

PLAY ***************************************************************************

TASK [assert] ******************************************************************
ok: [localhost]

PLAY RECAP *********************************************************************
localhost                  : ok=2    changed=0    unreachable=0    failed=0   

But in version 2.0.1.0 and 2.0.2.0 you will see:

PLAY ***************************************************************************

PLAY ***************************************************************************

TASK [assert] ******************************************************************
fatal: [localhost]: FAILED! => {"assertion": "ansible_distribution is defined", "changed": false, "evaluated_to": false, "failed": true}

NO MORE HOSTS LEFT *************************************************************
        to retry, use: --limit @playbook.retry

PLAY RECAP *********************************************************************
localhost                  : ok=0    changed=0    unreachable=0    failed=1   

That is, a play that is completely skipped and produces no output
during the ansible-playbook and does not run the setup module
still results in Ansible not running setup in subsequent tasks.

EXPECTED RESULTS

I expect Ansible to connect to the target host and gather facts at
least once during the play.

ACTUAL RESULTS

No facts are gathered.

@nitzmahone nitzmahone added bug_report P2 Priority 2 - Issue Blocks Release labels May 5, 2016
@nitzmahone nitzmahone added this to the 2.1.0 milestone May 5, 2016
@larsks
Copy link
Contributor Author

larsks commented May 6, 2016

@jimi-c This appears to have been introduced by 5587b08.

@larsks
Copy link
Contributor Author

larsks commented May 6, 2016

I think the solution is to not set gathered_facts=True in _get_next_task_from_state unless there are tasks to be executed. For devel, I think that means:

diff --git a/lib/ansible/executor/play_iterator.py b/lib/ansible/executor/play_iterator.py
index 3ab2e28..b5fc8bf 100644
--- a/lib/ansible/executor/play_iterator.py
+++ b/lib/ansible/executor/play_iterator.py
@@ -295,10 +295,10 @@ class PlayIterator:
                         setup_block = self._blocks[0]
                         if setup_block.has_tasks() and len(setup_block.block) > 0:
                             task = setup_block.block[0]
-                        if not peek:
-                            # mark the host as having gathered facts, because we're
-                            # returning the setup task to be executed
-                            host.set_gathered_facts(True)
+                            if not peek:
+                                # mark the host as having gathered facts, because we're
+                                # returning the setup task to be executed
+                                host.set_gathered_facts(True)
                 else:
                     # This is the second trip through ITERATING_SETUP, so we clear
                     # the flag and move onto the next block in the list while setting

And for 2.0.1.0 I think that means:

diff --git a/lib/ansible/executor/play_iterator.py b/lib/ansible/executor/play_iterator.py
index 1826432..1382133 100644
--- a/lib/ansible/executor/play_iterator.py
+++ b/lib/ansible/executor/play_iterator.py
@@ -255,9 +255,9 @@ class PlayIterator:
                        (gathering == 'explicit' and boolean(self._play.gather_facts)) or \
                        (gathering == 'smart' and implied and not host._gathered_facts):
                         # mark the host as having gathered facts
-                        host.set_gathered_facts(True)
                         setup_block = self._blocks[0]
                         if setup_block.has_tasks() and len(setup_block.block) > 0:
+                            host.set_gathered_facts(True)
                             task = setup_block.block[0]
                 else:
                     state.pending_setup = False

These changes seem to result in the appropriate behavior.

larsks added a commit to larsks/ansible that referenced this issue May 6, 2016
In `lib/ansible/executor/play_iterator.py`, ansible sets a host's
`_gathered_facts` property to `True` without checking to see if there
are any tasks to be executed.  In the event that the entire play is
skipped, `_gathered_facts` will be `True` even though the `setup`
module was never run.

This patch modifies the logic to only set `_gathered_facts` to `True`
when there are tasks to execute.

Closes ansible#15744.
@larsks
Copy link
Contributor Author

larsks commented May 6, 2016

With the above patch in place, running the above reproducer correctly results in:

PLAY [localhost] ***************************************************************

PLAY [localhost] ***************************************************************

TASK [setup] *******************************************************************
ok: [localhost]

TASK [assert] ******************************************************************
ok: [localhost]

PLAY RECAP *********************************************************************
localhost                  : ok=2    changed=0    unreachable=0    failed=0   

jimi-c pushed a commit that referenced this issue May 12, 2016
In `lib/ansible/executor/play_iterator.py`, ansible sets a host's
`_gathered_facts` property to `True` without checking to see if there
are any tasks to be executed.  In the event that the entire play is
skipped, `_gathered_facts` will be `True` even though the `setup`
module was never run.

This patch modifies the logic to only set `_gathered_facts` to `True`
when there are tasks to execute.

Closes #15744.
@jimi-c
Copy link
Member

jimi-c commented May 12, 2016

Closing This Ticket

Hi!

We believe the above commit should resolve this problem for you. This will also be included in the next release.

If you continue seeing any problems related to this issue, or if you have any further questions, please let us know by stopping by one of the two mailing lists, as appropriate:

Because this project is very active, we're unlikely to see comments made on closed tickets, but the mailing list is a great way to ask questions, or post if you don't think this particular issue is resolved.

Thank you!

openstack-gerrit pushed a commit to openstack-archive/tripleo-quickstart that referenced this issue May 12, 2016
with Gabriele's upcoming patches that allow the quickstart to run
without performing teardown operations, we hit Ansible issue 15744
(ansible/ansible#15744), in which Ansible
will incorrectly assume it has gathered facts for a host if it *skips*
a play that would otherwise target that host.

This commit removes `gathering=smart` from our ansible.cfg.  We can
restore that configuration if we pin Ansible to 2.1 or later at some
point in the future.

Change-Id: Idda5289a6ea4c724ad2e936b6feb1128925d0e6f
sshnaidm pushed a commit to sshnaidm/tripleo-quickstart that referenced this issue May 19, 2016
with Gabriele's upcoming patches that allow the quickstart to run
without performing teardown operations, we hit Ansible issue 15744
(ansible/ansible#15744), in which Ansible
will incorrectly assume it has gathered facts for a host if it *skips*
a play that would otherwise target that host.

This commit removes `gathering=smart` from our ansible.cfg.  We can
restore that configuration if we pin Ansible to 2.1 or later at some
point in the future.

Change-Id: Idda5289a6ea4c724ad2e936b6feb1128925d0e6f
gescheit pushed a commit to gescheit/ansible that referenced this issue Jun 3, 2016
In `lib/ansible/executor/play_iterator.py`, ansible sets a host's
`_gathered_facts` property to `True` without checking to see if there
are any tasks to be executed.  In the event that the entire play is
skipped, `_gathered_facts` will be `True` even though the `setup`
module was never run.

This patch modifies the logic to only set `_gathered_facts` to `True`
when there are tasks to execute.

Closes ansible#15744.
@ansibot ansibot added bug This issue/PR relates to a bug. and removed bug_report labels Mar 7, 2018
@ansible ansible locked and limited conversation to collaborators Apr 25, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug This issue/PR relates to a bug. P2 Priority 2 - Issue Blocks Release
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants