clear_host_errors - fix handling unreachable errors

[s-hertel]

SUMMARY

Fixes #35086
Fixes #86208

On devel, unreachable hosts pick up where they left off.

On this branch, unreachable hosts pick up at the next play.

I'm not sure if this is the right behavior, because I can imagine content relying on the behavior of devel (it's the only mechanism to recover an unreachable host, and it could be intuitive using the free strategy), and people using the linear strategy could expect the host should start at the task following the next applicable task following the clear_host_errors task instead of the next play.

I'd also be open to just documenting the current behavior or making it configurable.

ISSUE TYPE

• Bugfix Pull Request

[mkrizek]

As a note I looked at fixing 31543 in the past and had this in git stash:

diff --git a/lib/ansible/plugins/strategy/linear.py b/lib/ansible/plugins/strategy/linear.py
index d90d347d3e3..01c2bbfeb01 100644
--- a/lib/ansible/plugins/strategy/linear.py
+++ b/lib/ansible/plugins/strategy/linear.py
@@ -411,23 +411,20 @@ class StrategyModule(StrategyBase):
                 for res in results:
                     # execute_meta() does not set 'failed' in the TaskResult
                     # so we skip checking it with the meta tasks and look just at the iterator
-                    if (res.is_failed() or res._task.action in C._ACTION_META) and iterator.is_failed(res._host):
+                    #if (res.is_failed() or res._task.action in C._ACTION_META) and iterator.is_failed(res._host):
+                    if res.is_failed():
                         failed_hosts.append(res._host.name)
                     elif res.is_unreachable():
                         unreachable_hosts.append(res._host.name)

                 # 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):
-                    dont_fail_states = frozenset([IteratingStates.RESCUE, IteratingStates.ALWAYS])
+                    # would probably need a new flag to abort the play (and for PlayIterator to revert the flag when rescue)
+                    iterator.end_play = True
+                    # FIXME self._tqm.RUN_FAILED_BREAK_PLAY
                     for host in hosts_left:
-                        (s, _) = iterator.get_next_task_for_host(host, peek=True)
-                        # the state may actually be in a child state, use the get_active_state()
-                        # method in the iterator to figure out the true active state
-                        s = iterator.get_active_state(s)
-                        if s.run_state not in dont_fail_states or \
-                           s.run_state == IteratingStates.RESCUE and s.fail_state & FailedStates.RESCUE != 0:
-                            self._tqm._failed_hosts[host.name] = True
-                            result |= self._tqm.RUN_FAILED_BREAK_PLAY
+                        if host.name not in failed_hosts:
+                            #self._tqm._failed_hosts[host.name] = True
+                            iterator.mark_host_failed(host)
                 display.debug("done checking for any_errors_fatal")

                 display.debug("checking for max_fail_percentage")

Basically the idea was to utilize iterator.mark_host_errors() to fail other hosts and for PlayIterator to handle the case where rescue section is present to undo the failure(s). You can see that I commented out some lines, it probably broke something but I am not sure what it was. One thing I know is that with the diff the return code of ansible-playbook changes since strategy does not return self._tqm.RUN_FAILED_BREAK_PLAY anymore.

It would also fix #73246 it seems.

I feel like there are too many fixes in the current (devel) implemetation (dont_fail_states, using iterator.get_active_state) at this point.

There is probably a reason why this wasn't done like this when it was originally written though? 🧐

Anyway just thought I'd share this in case you wanted to experiment with that idea as well.

[mkrizek]

I'd also be open to just documenting the current behavior or making it configurable.

I suppose another option would be to document the current behavior and add a new meta task meta: clear_unreachable_hosts (likely a different name) that would implement the same behavior as in this PR?

[s-hertel]

Do you think the current behavior of clear_host_errors on unreachable hosts is potentially useful? I wanted to be cautious about changing the current behavior since this is one of only two mechanisms for handling unreachable errors, but I think it's the best option. #86208 (comment) changed my mind that it's useful for the free strategy.

On a tangential note, I'm questioning how useful clear_host_errors is in general. I searched Github for use cases, but largely found it used in strange places, like at the beginning of plays, in roles, in rescue... Since it affects all play hosts, and interacts with block/rescue/always, it seems brittle (even with linear, if dynamic, conditional includes are involved). To keep content portable, I think it's best to handle unreachable errors using ignore_unreachable + task conditionals, and non-unreachable errors using rescue (or ignore_errors + task conditionals).