Fix block rescue when some hosts are failing and rescued not affecting other hosts with run_once as true #83388
Conversation
ansibot
added
bug
|
Fixing unrelated test failures in #83391. |
mattclay
removed
the
needs_triage
s-hertel
removed
the
ci_verified
> > Co-authored-by: Martin Krizek <martin.krizek@gmail.com>
… with run_once, any_errors_fatal takes precedence
ansibot
removed
the
needs_revision
|
I must've misinterpreted them, you're right. |
| if task.args.get('_raw_params', None) not in ('noop', 'reset_connection', 'end_host', 'role_complete', 'flush_handlers'): | ||
| run_once = True | ||
| if (task.any_errors_fatal or run_once) and not task.ignore_errors: | ||
| if task.any_errors_fatal and not task.ignore_errors: |
There was a problem hiding this comment.
I am not sure this is correct. There are several things at play here. Meta tasks' results do not go through the normal results processing in StrategyBase._process_pending_results() (until possibly #83252 is merged anyway) which contains the code to handle the run_once functionality (if the task fails and not ignore_errors, fail all other hosts) so that means that run_once is not implemented for meta. I think that is why the local variable any_errors_fatal is set also for run_once so the code that handles any_errors_fatal is used to "implement" run_once for meta tasks because effectively both features just fails all other hosts.
But even if the meta tasks' results went through the normal results processing, it still would not work because there we check original_task.run_once to trigger the run_once functionality but that value does not reflect what we want because above we just set run_once to a local variable here for hardcoded set of meta tasks.
There was a problem hiding this comment.
On the other hand, StrategyBase._execute_meta() does not appear to create a failing TaskResult ever. That would mean that the any_errors_fatal code would not be triggered because for that it requires a failing task result and therefore run_once on meta task only means that the task would be run on only one host and the second part of the feature (failing other hosts in case of a task failure) is not needed? So this change is in the end fine??
There was a problem hiding this comment.
I would skip this particular change for this PR (that we want to backport).
| run_once = templar.template(task.run_once) or action and getattr(action, 'BYPASS_HOST_LOOP', False) | ||
|
|
||
| if (task.any_errors_fatal or run_once) and not task.ignore_errors: | ||
| if task.any_errors_fatal and not task.ignore_errors: |
There was a problem hiding this comment.
I wonder if the second paragraph from my first comment above applies here in some way too because here we declare that run_once is equivalent to templar.template(task.run_once) or action and getattr(action, 'BYPASS_HOST_LOOP', False) but in StrategyBase._process_pending_results() we check only against original_task.run_once.
Both of my comments however make me wonder in what scenarios is the code to handle run_once in StrategyBase._process_pending_results() even needed since it is currently handled, it seems, by the any_errors_fatal code.
Note that the code for run_once and any_errors_fatal are similar but not the same, it appears to be different for unreachable hosts - #82852.
There was a problem hiding this comment.
So I think this is correct change but as I mentioned above it would break the following playbook:
- hosts: h1,h2
gather_facts: false
tasks:
- name: missing task args, this fails
add_host:
- debug:
msg: add_host is BYPASS_HOST_LOOP action and implies run_once, this should not happenbecause we don't check for BYPASS_HOST_LOOP in StrategyBase to fail all other hosts, we just check for run_once. So I think that BYPASS_HOST_LOOP check needs to be somehow added there.
ansibot
added
the
stale_ci
mkrizek
left a comment
mkrizek
left a comment
There was a problem hiding this comment.
Got back to thinking about this and found a couple of issues that need to be addressed before merging it.
| @@ -0,0 +1,19 @@ | |||
| - hosts: testhost,testhost2 | |||
| gather_facts: false | |||
| any_errors_fatal: "{{ any_errors_fatal | default(omit) }}" | |||
There was a problem hiding this comment.
+100 for testing multiple combinations of run_once and any_errors_fatal. Unfortunately due to #61025 we need to structure the tests differently (using any_errors_fatal env var?) because as it is currently written, any_errors_fatal: "{{ any_errors_fatal | default(omit) }}" always evaluates to truthy value (until data tagging probably).
| if task.args.get('_raw_params', None) not in ('noop', 'reset_connection', 'end_host', 'role_complete', 'flush_handlers'): | ||
| run_once = True | ||
| if (task.any_errors_fatal or run_once) and not task.ignore_errors: | ||
| if task.any_errors_fatal and not task.ignore_errors: |
There was a problem hiding this comment.
I would skip this particular change for this PR (that we want to backport).
| run_once = templar.template(task.run_once) or action and getattr(action, 'BYPASS_HOST_LOOP', False) | ||
|
|
||
| if (task.any_errors_fatal or run_once) and not task.ignore_errors: | ||
| if task.any_errors_fatal and not task.ignore_errors: |
There was a problem hiding this comment.
So I think this is correct change but as I mentioned above it would break the following playbook:
- hosts: h1,h2
gather_facts: false
tasks:
- name: missing task args, this fails
add_host:
- debug:
msg: add_host is BYPASS_HOST_LOOP action and implies run_once, this should not happenbecause we don't check for BYPASS_HOST_LOOP in StrategyBase to fail all other hosts, we just check for run_once. So I think that BYPASS_HOST_LOOP check needs to be somehow added there.
ansibot
added
the
needs_revision
ansibot
added
the
stale_pr
6 participants
SUMMARY
Reattempt for #83363
Fix the issue(#83292) of playbook with any_errors_fatal: true, the rescue section only executes on the host where a fail task with run_once: true is triggered, not affecting other hosts as expected.
Fixes #83292
ISSUE TYPE