Ansible may run more handlers than originally planned

[Radosław Piliszek]

For example in check-containers.yml there is a task which checks container parameters. Supposedly it notifies only the restart handler for the service which has any changes. However, this is not the case - all containers on such a host are restarted when one has a differing config (limited to that role). It seems loops with "notify" notify all the handlers when at least one item "changed".

bug in Ansible: https://github.com/ansible/ansible/issues/22579

Identified places:
- check_containers
- copy config.json

[Radosław Piliszek (yoctozepto)]

Aaand it seems it is a known issue in Ansible :/ https://github.com/ansible/ansible/issues/22579

[Radosław Piliszek (yoctozepto)]

We could work around this by using include_tasks in loop as it lifts most of those pesky ansible quirks related to looped tasks...

[Mark Goddard (mgoddard)]

I was certain that this worked, but I just tested and it does not :(

[Mark Goddard (mgoddard)]

This breaks quite a lot of our configuration and container checking code. I guess this is only a problem since we (I) removed all the handler conditional logic.

[Radosław Piliszek (yoctozepto)]

Could be; maybe some of these deserve discussion during meeting?

[Radosław Piliszek (yoctozepto)]

landed with Train's: https://review.opendev.org/647699 [Simplify handler conditionals]

[Mark Goddard (mgoddard)]

Introduced by https://review.opendev.org/647699, which landed in Train.

[Radosław Piliszek (yoctozepto)]

Some points to remember (as discussed during meeting):
1) the issue is in looping with notify - don't rely on it solely
2) there could be performance issues with include_tasks at scale
3) there is also an issue that interrupted deploy/reconfigure may *not* restart containers if it failed between config update and planned restart (due to no change happening later)
  3a) we could do some touch dance
  3b) "kolla_set_configs --check" or similar
4) at some point in time we might want to switch to podman which relies on systemd (no idea how it affects this but nice to remember)

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: master
Review: https://review.opendev.org/723342

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: master
Review: https://review.opendev.org/744207

[OpenStack Infra (hudson-openstack)]

Change abandoned by Radosław Piliszek (<email address hidden>) on branch: master
Review: https://review.opendev.org/744207
Reason: superseded by https://github.com/ansible/ansible/pull/71118/files and https://review.opendev.org/745164

[Radosław Piliszek (yoctozepto)]

So, one approach to this is in: https://gist.github.com/yoctozepto/4408d70fee31ac3ba9d8a6491c370dfe

[Michal Arbet (michalarbet)]

Shouldn't we put a little pressure on ansible dev and start another one discussion about this ?
For me it sounds very funny to reimplement all in kolla while it's only needed to patch one line in ansible.

[Michal Arbet (michalarbet)]

Well, not everything, but definitely a bigger part than one line

[Radosław Piliszek (yoctozepto)]

As far as I am concerned, feel welcome to discuss this further with the Ansible team. We gave up because we don't have time to invest in learning all Ansible internals to prove our reasoning is right.

[dalekseev (dalekseev)]

so why don't we use workaround in kolla-ansible if it didn't work out with ansible guys anyway ?

[Radosław Piliszek (yoctozepto)]

Lack of time to finish the implementation. It is just still pending...

[Roman Krček (r-krcek)]

Hello, is this issue/optimalization still relevant? I would like to take a crack at it. :)