Repository: ansible/ansible
Pull request: free strategy - include failed hosts that were notified
##### SUMMARY
The free strategy was excluding failed hosts that were notified so the base class didn't run handlers on those hosts even when --force-handlers was used.

~WIP while I add a test for it to keep it from breaking in the future.~

Fixes #65254

##### ISSUE TYPE
- Bugfix Pull Request


Problem statement:
The --force-handlers option if used with 'strategy: free' does not run handlers on all hosts. 
<!--- Verify first that your issue is not already reported on GitHub -->
<!--- Also test if the latest release and devel branch are affected too -->
<!--- Complete *all* sections as described, this form is processed automatically -->

##### SUMMARY
<!--- Explain the problem briefly below -->
As of Ansible 2.7 the `--force-handlers` option is no longer running the handler/s on all hosts when used with strategy free. Only tested back to 2.5 and this was working as I would expect it to on 2.5 & 2.6.

##### ISSUE TYPE
- Bug Report

##### COMPONENT NAME
<!--- Write the short name of the module, plugin, task or feature below, use your best guess if unsure -->
plugins/strategy 

##### ANSIBLE VERSION
<!--- Paste verbatim output from "ansible --version" between quotes -->
```paste below
ansible 2.10.0.dev0
  config file = /Users/bdudas/dev-net/handlers/ansible.cfg
  configured module search path = [u'/Users/bdudas/.ansible/plugins/modules', u'/usr/share/ansible/plugins/modules']
  ansible python module location = /Users/bdudas/dev-net/handlers/ansible/lib/ansible
  executable location = /Users/bdudas/dev-net/handlers/ansible/bin/ansible
  python version = 2.7.16 (default, Oct 16 2019, 00:34:56) [GCC 4.2.1 Compatible Apple LLVM 10.0.1 (clang-1001.0.37.14)]
```

##### CONFIGURATION
<!--- Paste verbatim output from "ansible-config dump --only-changed" between quotes -->
```paste below
DEFAULT_STRATEGY(/Users/bdudas/dev-net/handlers/ansible.cfg) = free
HOST_KEY_CHECKING(/Users/bdudas/dev-net/handlers/ansible.cfg) = False
RETRY_FILES_ENABLED(/Users/bdudas/dev-net/handlers/ansible.cfg) = False
```

##### OS / ENVIRONMENT
<!--- Provide all relevant information below, e.g. target OS versions, network device firmware, etc. -->


##### STEPS TO REPRODUCE
<!--- Describe exactly how to reproduce the problem, using a minimal test-case -->

<!--- Paste example playbooks or commands between quotes below -->
```yaml
---
- hosts: all
  gather_facts: false

  tasks:
    - command: hostname
      register: id
      notify: recovery_x

    - debug:
        var: id

    - name: failure
      fail: msg="Testing Failure"
      when: inventory_hostname == 'covfefe'

  handlers:
    - name: recovery_x
      shell: hostname
...
```
ansible-playbook <playbook.yml> --force-handlers
<!--- HINT: You can paste gist.github.com links for larger files -->

##### EXPECTED RESULTS
<!--- Describe what you expected to happen when running the steps above -->
Handlers will run against all hosts when using the `--force-handlers` option. 

##### ACTUAL RESULTS
<!--- Describe what actually happened. If possible run with extra verbosity (-vvvv) -->
Handler is only run on 1 of the 2 target hosts.
<!--- Paste verbatim command output between quotes -->
```paste below

```

Hints from issue discussion:
Files identified in the description:
* [`lib/ansible/plugins/strategy/__init__.py`](https://github.com/ansible/ansible/blob/devel/lib/ansible/plugins/strategy/__init__.py)

If these files are inaccurate, please update the `component name` section of the description or use the `!component` bot command.

[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: components_banner --->

Commit message: Use the play iterator instead of TQM for accurate failure representation in blocks

Patch to review:
diff --git a/changelogs/fragments/65576-fix-free-strategy-handler-filtering.yaml b/changelogs/fragments/65576-fix-free-strategy-handler-filtering.yaml
new file mode 100644
index 00000000000000..87f5fbe095857e
--- /dev/null
+++ b/changelogs/fragments/65576-fix-free-strategy-handler-filtering.yaml
@@ -0,0 +1,4 @@
+bugfixes:
+  - free strategy - Include failed hosts when filtering notified hosts for handlers. The strategy base should
+    determine whether or not to run handlers on those hosts depending on whether forcing handlers is enabled
+    (https://github.com/ansible/ansible/issues/65254).
diff --git a/lib/ansible/plugins/strategy/free.py b/lib/ansible/plugins/strategy/free.py
index 264c0e109569cb..490a9c60d68ea4 100644
--- a/lib/ansible/plugins/strategy/free.py
+++ b/lib/ansible/plugins/strategy/free.py
@@ -55,14 +55,18 @@ def _filter_notified_hosts(self, notified_hosts):
         Filter notified hosts accordingly to strategy
         '''
 
-        # We act only on hosts that are ready to flush handlers
+        # We act only on hosts that are ready to flush handlers or failed hosts in notified_hosts if --force-handlers is used
         return [host for host in notified_hosts
-                if host in self._flushed_hosts and self._flushed_hosts[host]]
+                if (host in self._flushed_hosts and self._flushed_hosts[host]) or self.iterator.is_failed(host)]
 
     def __init__(self, tqm):
         super(StrategyModule, self).__init__(tqm)
         self._host_pinned = False
 
+        # The iterator is needed to check failed hosts that have not completed (for example, have an 'always' section to run)
+        # Initialize to None and set in run
+        self.iterator = None
+
     def run(self, iterator, play_context):
         '''
         The "free" strategy is a bit more complex, in that it allows tasks to
@@ -76,6 +80,7 @@ def run(self, iterator, play_context):
         list again, which would end up favoring hosts near the beginning of the
         list.
         '''
+        self.iterator = iterator
 
         # the last host to be given a task
         last_host = 0
diff --git a/test/integration/targets/handlers/runme.sh b/test/integration/targets/handlers/runme.sh
index b87e342de48348..59c81bcec09b6b 100755
--- a/test/integration/targets/handlers/runme.sh
+++ b/test/integration/targets/handlers/runme.sh
@@ -17,33 +17,42 @@ ansible-playbook test_listening_handlers.yml -i inventory.handlers -v "$@"
 [ "$(ansible-playbook test_handlers.yml -i inventory.handlers -v "$@" --tags scenario2 -l A \
 | grep -E -o 'RUNNING HANDLER \[test_handlers : .*?]')" = "RUNNING HANDLER [test_handlers : test handler]" ]
 
-# Not forcing, should only run on successful host
-[ "$(ansible-playbook test_force_handlers.yml -i inventory.handlers -v "$@" --tags normal \
-| grep -E -o CALLED_HANDLER_. | sort | uniq | xargs)" = "CALLED_HANDLER_B" ]
+# Test forcing handlers using the linear and free strategy
+for strategy in linear free; do
 
-# Forcing from command line
-[ "$(ansible-playbook test_force_handlers.yml -i inventory.handlers -v "$@" --tags normal --force-handlers \
-| grep -E -o CALLED_HANDLER_. | sort | uniq | xargs)" = "CALLED_HANDLER_A CALLED_HANDLER_B" ]
+  export ANSIBLE_STRATEGY=$strategy
 
-# Forcing from command line, should only run later tasks on unfailed hosts
-[ "$(ansible-playbook test_force_handlers.yml -i inventory.handlers -v "$@" --tags normal --force-handlers \
-| grep -E -o CALLED_TASK_. | sort | uniq | xargs)" = "CALLED_TASK_B CALLED_TASK_D CALLED_TASK_E" ]
+  # Not forcing, should only run on successful host
+  [ "$(ansible-playbook test_force_handlers.yml -i inventory.handlers -v "$@" --tags normal \
+  | grep -E -o CALLED_HANDLER_. | sort | uniq | xargs)" = "CALLED_HANDLER_B" ]
 
-# Forcing from command line, should call handlers even if all hosts fail
-[ "$(ansible-playbook test_force_handlers.yml -i inventory.handlers -v "$@" --tags normal --force-handlers -e fail_all=yes \
-| grep -E -o CALLED_HANDLER_. | sort | uniq | xargs)" = "CALLED_HANDLER_A CALLED_HANDLER_B" ]
+  # Forcing from command line
+  [ "$(ansible-playbook test_force_handlers.yml -i inventory.handlers -v "$@" --tags normal --force-handlers \
+  | grep -E -o CALLED_HANDLER_. | sort | uniq | xargs)" = "CALLED_HANDLER_A CALLED_HANDLER_B" ]
 
-# Forcing from ansible.cfg
-[ "$(ANSIBLE_FORCE_HANDLERS=true ansible-playbook test_force_handlers.yml -i inventory.handlers -v "$@" --tags normal \
-| grep -E -o CALLED_HANDLER_. | sort | uniq | xargs)" = "CALLED_HANDLER_A CALLED_HANDLER_B" ]
+  # Forcing from command line, should only run later tasks on unfailed hosts
+  [ "$(ansible-playbook test_force_handlers.yml -i inventory.handlers -v "$@" --tags normal --force-handlers \
+  | grep -E -o CALLED_TASK_. | sort | uniq | xargs)" = "CALLED_TASK_B CALLED_TASK_D CALLED_TASK_E" ]
 
-# Forcing true in play
-[ "$(ansible-playbook test_force_handlers.yml -i inventory.handlers -v "$@" --tags force_true_in_play \
-| grep -E -o CALLED_HANDLER_. | sort | uniq | xargs)" = "CALLED_HANDLER_A CALLED_HANDLER_B" ]
+  # Forcing from command line, should call handlers even if all hosts fail
+  [ "$(ansible-playbook test_force_handlers.yml -i inventory.handlers -v "$@" --tags normal --force-handlers -e fail_all=yes \
+  | grep -E -o CALLED_HANDLER_. | sort | uniq | xargs)" = "CALLED_HANDLER_A CALLED_HANDLER_B" ]
 
-# Forcing false in play, which overrides command line
-[ "$(ansible-playbook test_force_handlers.yml -i inventory.handlers -v "$@" --tags force_false_in_play --force-handlers \
-| grep -E -o CALLED_HANDLER_. | sort | uniq | xargs)" = "CALLED_HANDLER_B" ]
+  # Forcing from ansible.cfg
+  [ "$(ANSIBLE_FORCE_HANDLERS=true ansible-playbook test_force_handlers.yml -i inventory.handlers -v "$@" --tags normal \
+  | grep -E -o CALLED_HANDLER_. | sort | uniq | xargs)" = "CALLED_HANDLER_A CALLED_HANDLER_B" ]
+
+  # Forcing true in play
+  [ "$(ansible-playbook test_force_handlers.yml -i inventory.handlers -v "$@" --tags force_true_in_play \
+  | grep -E -o CALLED_HANDLER_. | sort | uniq | xargs)" = "CALLED_HANDLER_A CALLED_HANDLER_B" ]
+
+  # Forcing false in play, which overrides command line
+  [ "$(ansible-playbook test_force_handlers.yml -i inventory.handlers -v "$@" --tags force_false_in_play --force-handlers \
+  | grep -E -o CALLED_HANDLER_. | sort | uniq | xargs)" = "CALLED_HANDLER_B" ]
+
+  unset ANSIBLE_STRATEGY
+
+done
 
 [ "$(ansible-playbook test_handlers_include.yml -i ../../inventory -v "$@" --tags playbook_include_handlers \
 | grep -E -o 'RUNNING HANDLER \[.*?]')" = "RUNNING HANDLER [test handler]" ]


Code review comments:
diff_hunk: @@ -76,6 +80,7 @@ def run(self, iterator, play_context):
         list again, which would end up favoring hosts near the beginning of the
         list.
         '''
+        self.iterator = iterator
path: lib/ansible/plugins/strategy/free.py
text: @user1:
I kind of question this line here.  I understand what you are doing and trying to achieve, but it feels kind of like a hack to inject this into class at this point...working around a shortcoming of the class.  This makes me feel like that should be something that is addressed.  Not sure, just offering opinions on my immediate reaction.

@author:
@user1 Thanks for commenting. I'm not happy with this line either and it's why I attempted to use the TQM at first. I could instead pass the iterator to the filtering function. Do you think that would be better?

@author:
My only concern with that is that it could break 3rd party strategy plugins.