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

meta: flush_handlers. Wrong conditional behaviour #77616

Closed
1 task done
jmnavarrol opened this issue Apr 23, 2022 · 4 comments · Fixed by #77955
Closed
1 task done

meta: flush_handlers. Wrong conditional behaviour #77616

jmnavarrol opened this issue Apr 23, 2022 · 4 comments · Fixed by #77955
Labels
affects_2.12 bug This issue/PR relates to a bug. has_pr This issue has an associated PR. support:core This issue/PR relates to code supported by the Ansible Engineering Team. waiting_on_contributor This would be accepted but there are no plans to actively work on it.

Comments

@jmnavarrol
Copy link

jmnavarrol commented Apr 23, 2022

Summary

meta: flush_handlers does the wrong thing when conditionally invoked.

No matter the surrounding conditions, meta: flush_handlers gets run whenever ansible interpreter finds it.

Most notably, it doesn't honor conditionals, either straight away (i.e.: with a when clause attached to the task), or by means of conditional inclusion (be it by means of role or include_tasks, etc.)

Issue Type

Bug Report
Feature Request

Component Name

ansible-playbook

Ansible Version

$ ansible --version
ansible [core 2.12.4]
  config file = None
  configured module search path = ['/home/jmnav/.ansible/plugins/modules', '/usr/share/ansible/plugins/modules']
  ansible python module location = /home/jmnav/.virtualenvs/ansible-issues/lib/python3.9/site-packages/ansible
  ansible collection location = /home/jmnav/.ansible/collections:/usr/share/ansible/collections
  executable location = /home/jmnav/.virtualenvs/ansible-issues/bin/ansible
  python version = 3.9.2 (default, Feb 28 2021, 17:03:44) [GCC 10.2.1 20210110]
  jinja version = 3.0.1
  libyaml = True

Configuration

$ ansible-config dump --only-changed
(no output)

OS / Environment

Ubuntu Bionic, running Ansible on a virtualenv (but don't think this is that relevant in this case)

Steps to Reproduce

I set a repository at jmnavarrol/ansible-issue-flush-handlers to showcase this bug.

The playbook below calls flush_handlers with a when condition guaranteed to always evaluate to false (when: 1 == 2) so it should obviously never be triggered (see my ansible playbook).

---
- name: "A test for #77616. See https://github.com/ansible/ansible/issues/77616"
  become: False
  hosts: localhost
  connection: local
  
#---
# PRE-TASKS
#---
  pre_tasks:
  - name: Shows involved versions
    debug:
      msg:
      - "Python version: '{{ ansible_python_version }}'."
      - "Ansible version '{{ ansible_version.string }}'."
      - "This is always going to be the First Message."
      - "See tasks' sorting below..."

#---
# TASKS
#---
  tasks:
  - name: "First task (it triggers 'my_handler')"
    set_fact:
      my_task: "This was triggered from 'first_task'."
    changed_when: true  # this makes sure the handler is always notified
    notify: my_handler

  - name: "This 'flush_handlers' should NEVER be triggered as the 'when' clause ALWAYS evaluates to False!"
    meta: flush_handlers
    when: 1 == 2

  - name: Last task
    debug:
      msg:
      - 'This is the last task.'
      - "If 'flush_handlers' worked as it should, this would appear BEFORE the triggered handler!!!"

#---
# HANDLERS
#---
  handlers:
  - name: my_handler
    debug:
      msg:
      - "{{ my_task }}"
      - "If 'flush_handlers' worked as it should, this would be the LAST message!!!"

Expected Results

Since the when condition will always be false, the flush_handlers task shouldn't be triggered and the handler task should be run the last, after all the other playbook's tasks, so run order should be:

  1. "First task (it triggers 'my_handler')"
  2. Last task
  3. my_handler

As a general matter, the expectation is twofold:

  1. meta being tasks (albeit of "a special kind") "...that can be used anywhere within your playbook", to behave as tasks do and, more to the point, not against the standard and expected behaviour of tasks (that is: they should honor a when clause).
  2. for flush_handlers to behave exactly the same as the "implicit flush_handlers" already implemented by the end of a playbook section (pre_tasks, roles, tasks, post_tasks) only "now", subjected to any other standard tasks behaviour (inclusion, conditionals, etc.).

Actual Results

$ ansible-playbook playbook.yml
[WARNING]: No inventory was parsed, only implicit localhost is available
[WARNING]: provided hosts list is empty, only localhost is available. Note that the implicit localhost does not match 'all'

PLAY [A test for 'flush_handlers' action. See https://github.com/ansible/ansible/issues/77616] *****************************************************************

TASK [Gathering Facts] *****************************************************************************************************************************************
ok: [localhost]

TASK [Shows involved versions] *********************************************************************************************************************************
ok: [localhost] => {
    "msg": [
        "Python version: '3.9.2'.",
        "Ansible version '2.12.4'.",
        "This is always going to be the First Message.",
        "See tasks' sorting below..."
    ]
}

TASK [First task (it triggers 'my_handler')] *******************************************************************************************************************
changed: [localhost]

TASK [This 'flush_handlers' should NEVER be triggered as the 'when' clause ALWAYS evaluates to False!] *********************************************************
[WARNING]: flush_handlers task does not support when conditional

RUNNING HANDLER [my_handler] ***********************************************************************************************************************************
ok: [localhost] => {
    "msg": [
        "This was triggered from 'first_task'.",
        "If 'flush_handlers' worked as intended, this would be the LAST message!!!"
    ]
}

TASK [Last task] ***********************************************************************************************************************************************
ok: [localhost] => {
    "msg": [
        "This is the last task.",
        "If 'flush_handlers' worked as intended, this would appear BEFORE the triggered handler!!!"
    ]
}

PLAY RECAP *****************************************************************************************************************************************************

$

NOTES:

  • As per the warning [WARNING]: flush_handlers task does not support when conditional, this is an already known bug by developers but being known doesn't make it anyless of a bug.
  • It is a bug because:
    • This behaviour, despite the warning, is in contradiction both with the documented meta and flush_handlers intended behaviours (see meta module documentation):
      • "Meta tasks are a special kind of task which can influence Ansible internal execution or state." - therefore still a task, so it should honor standard tasks' behaviour.
      • "Meta tasks can be used anywhere within your playbook." - this should include roles and included task files.
      • "ignore_conditional: partial Only some options support conditionals and when they do they act ‘bypassing the host loop’, taking the values from first available host" - this is a honest declaration in that it implicitly concedes the proper behaviour for some -or all, meta tasks may still not be fully implemented.
        But please pay attention that, in this case, flush_handlers not honoring conditionals is not only "incomplete behaviour" but that the already, currently implemented behaviour, is the wrong one (so this should not be considered a feature request but a bug).
    • It renders flush_handlers basically unusable but in the simplest scenarios as it can't be logically encapsulated. And in those simplest scenarios it can be trivially replaced by an in-place task that can properly consider conditionals, previously set facts, etc.

Given the above, and after thinking about it for long, I reckon that completely eliminating flush_handlers would produce a better outcome than current behaviour. So bad I think it is. Since I think I found a workaround quite possibly valid for a lot of cases, I changed my mind: I consider current behaviour plus workaround being superior to not having flush_handlers at all (See #77616 (comment)).

Of course, I don't want for flush_handlers to be taken away, since its intended behaviour is a very much needed one, but for flush_handlers to work as it should.

Code of Conduct

  • I agree to follow the Ansible Code of Conduct
@ansibot
Copy link
Contributor

ansibot commented Apr 23, 2022

Files identified in the description:

  • lib/ansible/playbook

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

@ansibot ansibot added affects_2.12 bug This issue/PR relates to a bug. needs_triage Needs a first human triage before being processed. support:core This issue/PR relates to code supported by the Ansible Engineering Team. labels Apr 23, 2022
@jmnavarrol
Copy link
Author

jmnavarrol commented Apr 23, 2022

This behaviour was also considered by #41313 (and many others).

#41313, being considered a feature request, was closed after an inactivity period.

But, any way I think of it, I can't rationalize this being a request for a new feature but a true bug, so I'm opening this one so it can be reconsidered.

@sivel sivel added feature This issue/PR relates to a feature request. bug This issue/PR relates to a bug. and removed bug This issue/PR relates to a bug. feature This issue/PR relates to a feature request. labels Apr 25, 2022
@bcoca bcoca added waiting_on_contributor This would be accepted but there are no plans to actively work on it. and removed needs_triage Needs a first human triage before being processed. labels Apr 26, 2022
@jmnavarrol
Copy link
Author

Thanks a lot to @bcoca and others for considering this behaviour a bug instead of just a feature request this time. Let's hope this helps in attracting attention/focus towards its resolution.

WORKAROUND: After re-reading the long comment history from #41313, I added to my repository another two test cases offering a suitable workaround valid at least for some cases, if not most: the brief of it is "put your 'flush_handlers' task within a dynamic include".

Also: I also re-read current meta module documentation and tested other meta tasks. Much to my surprise some of them honor when clauses while some don't.

@bcoca
Copy link
Member

bcoca commented May 2, 2022

@jmnavarrol it was not our intention to classify this as a bug, it is not, the meta commands were designed this way (you might opine it is a design flaw) so they are behaving as expected.

One of the reason we added 'attributes' is to have a finer detail about behavior and interactions in the documentation
https://docs.ansible.com/ansible/latest/collections/ansible/builtin/meta_module.html#attribute-ignore_conditional

mkrizek added a commit to mkrizek/ansible that referenced this issue Aug 9, 2022
mkrizek added a commit to mkrizek/ansible that referenced this issue Aug 10, 2022
@ansibot ansibot added the has_pr This issue has an associated PR. label Aug 11, 2022
mkrizek added a commit to mkrizek/ansible that referenced this issue Aug 11, 2022
mkrizek added a commit to mkrizek/ansible that referenced this issue Aug 15, 2022
mkrizek added a commit that referenced this issue Aug 16, 2022
@ansible ansible locked and limited conversation to collaborators Aug 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.12 bug This issue/PR relates to a bug. has_pr This issue has an associated PR. support:core This issue/PR relates to code supported by the Ansible Engineering Team. waiting_on_contributor This would be accepted but there are no plans to actively work on it.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants