Don't immediately return failed for any_errors_fatal tasksissue_16937_run_once_block_break_play

Instead of immediately returning a failed code (indicating a break in
the play execution), we internally 'or' that failure code with the result
(now an integer flag instead of a boolean) so that we can properly handle
the rescue/always portions of blocks and still remember that the break
condition was hit.

Fixes #16937

5 files changed, 26 insertions, 25 deletions

diff --git a/lib/ansible/executor/playbook_executor.py b/lib/ansible/executor/playbook_executor.py
index 6362075c1c..3d64407d07 100644
--- a/lib/ansible/executor/playbook_executor.py
+++ b/lib/ansible/executor/playbook_executor.py
@@ -147,7 +147,7 @@ class PlaybookExecutor:
                             result = self._tqm.run(play=play)
 
                             # break the play if the result equals the special return code
-                            if result == self._tqm.RUN_FAILED_BREAK_PLAY:
+                            if result & self._tqm.RUN_FAILED_BREAK_PLAY != 0:
                                 result = self._tqm.RUN_FAILED_HOSTS
                                 break_play = True
 
diff --git a/lib/ansible/executor/task_queue_manager.py b/lib/ansible/executor/task_queue_manager.py
index 7212b06aea..407f02f227 100644
--- a/lib/ansible/executor/task_queue_manager.py
+++ b/lib/ansible/executor/task_queue_manager.py
@@ -61,8 +61,8 @@ class TaskQueueManager:
     RUN_OK                = 0
     RUN_ERROR             = 1
     RUN_FAILED_HOSTS      = 2
-    RUN_UNREACHABLE_HOSTS = 3
-    RUN_FAILED_BREAK_PLAY = 4
+    RUN_UNREACHABLE_HOSTS = 4
+    RUN_FAILED_BREAK_PLAY = 8
     RUN_UNKNOWN_ERROR     = 255
 
     def __init__(self, inventory, variable_manager, loader, options, passwords, stdout_callback=None, run_additional_callbacks=True, run_tree=False):
diff --git a/lib/ansible/plugins/strategy/__init__.py b/lib/ansible/plugins/strategy/__init__.py
index 7639f2cbc7..d47826cfd8 100644
--- a/lib/ansible/plugins/strategy/__init__.py
+++ b/lib/ansible/plugins/strategy/__init__.py
@@ -119,14 +119,18 @@ class StrategyBase:
         # outstanding tasks still in queue
         self._blocked_hosts     = dict()
 
-    def run(self, iterator, play_context, result=True):
+    def run(self, iterator, play_context, result=0):
         # save the failed/unreachable hosts, as the run_handlers()
         # method will clear that information during its execution
         failed_hosts      = iterator.get_failed_hosts()
         unreachable_hosts = self._tqm._unreachable_hosts.keys()
 
         display.debug("running handlers")
-        result &= self.run_handlers(iterator, play_context)
+        handler_result = self.run_handlers(iterator, play_context)
+        if isinstance(handler_result, bool) and not handler_result:
+            result |= self._tqm.RUN_ERROR
+        elif not handler_result:
+            result |= handler_result
 
         # now update with the hosts (if any) that failed or were
         # unreachable during the handler execution phase
@@ -140,8 +144,6 @@ class StrategyBase:
             return self._tqm.RUN_UNREACHABLE_HOSTS
         elif len(failed_hosts) > 0:
             return self._tqm.RUN_FAILED_HOSTS
-        elif isinstance(result, bool) and not result:
-            return self._tqm.RUN_ERROR
         else:
             return self._tqm.RUN_OK
 
@@ -296,7 +298,11 @@ class StrategyBase:
                         display.debug("marking %s as failed" % original_host.name)
                         if original_task.run_once:
                             # if we're using run_once, we have to fail every host here
-                            [iterator.mark_host_failed(h) for h in self._inventory.get_hosts(iterator._play.hosts) if h.name not in self._tqm._unreachable_hosts]
+                            for h in self._inventory.get_hosts(iterator._play.hosts):
+                                if h.name not in self._tqm._unreachable_hosts:
+                                    state, _ = iterator.get_next_task_for_host(h, peek=True)
+                                    iterator.mark_host_failed(h)
+                                    state, new_task = iterator.get_next_task_for_host(h, peek=True)
                         else:
                             iterator.mark_host_failed(original_host)
 
@@ -631,7 +637,7 @@ class StrategyBase:
         Runs handlers on those hosts which have been notified.
         '''
 
-        result = True
+        result = self._tqm.RUN_OK
 
         for handler_block in iterator._play.handlers:
             # FIXME: handlers need to support the rescue/always portions of blocks too,
@@ -671,7 +677,7 @@ class StrategyBase:
 
         host_results = []
         for host in notified_hosts:
-            if not handler.has_triggered(host) and (host.name not in self._tqm._failed_hosts or play_context.force_handlers):
+            if not handler.has_triggered(host) and (not iterator.is_failed(host) or play_context.force_handlers):
                 task_vars = self._variable_manager.get_vars(loader=self._loader, play=iterator._play, host=host, task=handler)
                 self.add_tqm_variables(task_vars, play=iterator._play)
                 self._queue_task(host, handler, task_vars, play_context)
diff --git a/lib/ansible/plugins/strategy/linear.py b/lib/ansible/plugins/strategy/linear.py
index c45af9b678..fe5dbef67c 100644
--- a/lib/ansible/plugins/strategy/linear.py
+++ b/lib/ansible/plugins/strategy/linear.py
@@ -157,7 +157,7 @@ class StrategyModule(StrategyBase):
         '''
 
         # iteratate over each task, while there is one left to run
-        result     = True
+        result = self._tqm.RUN_OK
         work_to_do = True
         while work_to_do and not self._tqm._terminated:
 
@@ -269,12 +269,6 @@ class StrategyModule(StrategyBase):
                 results += self._wait_on_pending_results(iterator)
                 host_results.extend(results)
 
-                if not work_to_do and len(iterator.get_failed_hosts()) > 0:
-                    display.debug("out of hosts to run on")
-                    self._tqm.send_callback('v2_playbook_on_no_hosts_remaining')
-                    result = self._tqm.RUN_ERROR
-                    break
-
                 try:
                     included_files = IncludedFile.process_include_results(
                         host_results,
@@ -285,6 +279,7 @@ class StrategyModule(StrategyBase):
                         variable_manager=self._variable_manager
                     )
                 except AnsibleError as e:
+                    # this is a fatal error, so we abort here regardless of block state
                     return self._tqm.RUN_ERROR
 
                 include_failure = False
@@ -360,13 +355,10 @@ class StrategyModule(StrategyBase):
                 # if any_errors_fatal and we had an error, mark all hosts as failed
                 if any_errors_fatal and (len(failed_hosts) > 0 or len(unreachable_hosts) > 0):
                     for host in hosts_left:
-                        # don't double-mark hosts, or the iterator will potentially
-                        # fail them out of the rescue/always states
-                        if host.name not in failed_hosts:
+                        (s, _) = iterator.get_next_task_for_host(host, peek=True)
+                        if s.run_state != iterator.ITERATING_RESCUE:
                             self._tqm._failed_hosts[host.name] = True
-                            iterator.mark_host_failed(host)
-                    self._tqm.send_callback('v2_playbook_on_no_hosts_remaining')
-                    return self._tqm.RUN_FAILED_BREAK_PLAY
+                            result |= self._tqm.RUN_FAILED_BREAK_PLAY
                 display.debug("done checking for any_errors_fatal")
 
                 display.debug("checking for max_fail_percentage")
@@ -381,7 +373,7 @@ class StrategyModule(StrategyBase):
                                 self._tqm._failed_hosts[host.name] = True
                                 iterator.mark_host_failed(host)
                         self._tqm.send_callback('v2_playbook_on_no_hosts_remaining')
-                        return self._tqm.RUN_FAILED_BREAK_PLAY
+                        result |= self._tqm.RUN_FAILED_BREAK_PLAY
                 display.debug("done checking for max_fail_percentage")
 
             except (IOError, EOFError) as e:
diff --git a/test/units/plugins/strategies/test_strategy_base.py b/test/units/plugins/strategies/test_strategy_base.py
index 46c3729fd2..a00771a48c 100644
--- a/test/units/plugins/strategies/test_strategy_base.py
+++ b/test/units/plugins/strategies/test_strategy_base.py
@@ -60,6 +60,9 @@ class TestStrategyBase(unittest.TestCase):
         mock_tqm._listening_handlers = {}
         mock_tqm.send_callback.return_value = None
 
+        for attr in ('RUN_OK', 'RUN_ERROR', 'RUN_FAILED_HOSTS', 'RUN_UNREACHABLE_HOSTS'):
+            setattr(mock_tqm, attr, getattr(TaskQueueManager, attr))
+
         mock_iterator  = MagicMock()
         mock_iterator._play = MagicMock()
         mock_iterator._play.handlers = []
@@ -77,7 +80,7 @@ class TestStrategyBase(unittest.TestCase):
         mock_host.name = 'host1'
 
         self.assertEqual(strategy_base.run(iterator=mock_iterator, play_context=mock_play_context), mock_tqm.RUN_OK)
-        self.assertEqual(strategy_base.run(iterator=mock_iterator, play_context=mock_play_context, result=False), mock_tqm.RUN_ERROR)
+        self.assertEqual(strategy_base.run(iterator=mock_iterator, play_context=mock_play_context, result=TaskQueueManager.RUN_ERROR), mock_tqm.RUN_ERROR)
         mock_tqm._failed_hosts = dict(host1=True)
         mock_iterator.get_failed_hosts.return_value = [mock_host]
         self.assertEqual(strategy_base.run(iterator=mock_iterator, play_context=mock_play_context, result=False), mock_tqm.RUN_FAILED_HOSTS)