From 06cd28590154f59cddc7279f8ef438c24b72743a Mon Sep 17 00:00:00 2001 From: Martin Krizek Date: Tue, 23 Jan 2024 15:57:05 +0100 Subject: [PATCH] Play recap stats and callbacks for include_role, consolidate with include_tasks (#79260) This moves handling of callbacks and play recap stats from _load_included_file to individual strategies so include_role tasks are accounted for, not just include_tasks. Fixes #77336 --- .../77336-include_role-callbacks-stats.yml | 2 + lib/ansible/playbook/included_file.py | 2 +- lib/ansible/plugins/strategy/__init__.py | 39 +++++++++++-------- lib/ansible/plugins/strategy/free.py | 20 ++++++++-- lib/ansible/plugins/strategy/linear.py | 21 +++++++--- .../callback_list_include_role_fail.expected | 9 +++++ .../callbacks_list.expected | 4 +- .../include_role-fail.yml | 5 +++ .../ansible-playbook-callbacks/runme.sh | 9 +++-- ...back_default.out.include_role_fails.stderr | 12 ++++++ ...back_default.out.include_role_fails.stdout | 9 +++++ .../targets/callback_default/runme.sh | 15 +++---- .../test_include_role_fails.yml | 5 +++ .../targets/include_import/runme.sh | 2 +- 14 files changed, 115 insertions(+), 39 deletions(-) create mode 100644 changelogs/fragments/77336-include_role-callbacks-stats.yml create mode 100644 test/integration/targets/ansible-playbook-callbacks/callback_list_include_role_fail.expected create mode 100644 test/integration/targets/ansible-playbook-callbacks/include_role-fail.yml create mode 100644 test/integration/targets/callback_default/callback_default.out.include_role_fails.stderr create mode 100644 test/integration/targets/callback_default/callback_default.out.include_role_fails.stdout create mode 100644 test/integration/targets/callback_default/test_include_role_fails.yml diff --git a/changelogs/fragments/77336-include_role-callbacks-stats.yml b/changelogs/fragments/77336-include_role-callbacks-stats.yml new file mode 100644 index 00000000000..55133d49e3e --- /dev/null +++ b/changelogs/fragments/77336-include_role-callbacks-stats.yml @@ -0,0 +1,2 @@ +bugfixes: + - "``include_role`` - properly execute ``v2_playbook_on_include`` and ``v2_runner_on_failed`` callbacks as well as increase ``ok`` and ``failed`` stats in the play recap, when appropriate (https://github.com/ansible/ansible/issues/77336)" diff --git a/lib/ansible/playbook/included_file.py b/lib/ansible/playbook/included_file.py index 14d8982231f..8ec43977124 100644 --- a/lib/ansible/playbook/included_file.py +++ b/lib/ansible/playbook/included_file.py @@ -89,7 +89,7 @@ class IncludedFile: except KeyError: task_vars = task_vars_cache[cache_key] = variable_manager.get_vars(play=iterator._play, host=original_host, task=original_task) - include_args = include_result.get('include_args', dict()) + include_args = include_result.pop('include_args', dict()) special_vars = {} loop_var = include_result.get('ansible_loop_var', 'item') index_var = include_result.get('ansible_index_var') diff --git a/lib/ansible/plugins/strategy/__init__.py b/lib/ansible/plugins/strategy/__init__.py index 4c5e352b17e..2fe9313089e 100644 --- a/lib/ansible/plugins/strategy/__init__.py +++ b/lib/ansible/plugins/strategy/__init__.py @@ -843,7 +843,7 @@ class StrategyBase: return ti_copy - def _load_included_file(self, included_file, iterator, is_handler=False): + def _load_included_file(self, included_file, iterator, is_handler=False, handle_stats_and_callbacks=True): ''' Loads an included YAML file of tasks, applying the optional set of variables. @@ -851,6 +851,15 @@ class StrategyBase: in such case the caller is responsible for marking the host(s) as failed using PlayIterator.mark_host_failed(). ''' + if handle_stats_and_callbacks: + display.deprecated( + "Reporting play recap stats and running callbacks functionality for " + "``include_tasks`` in ``StrategyBase._load_included_file`` is deprecated. " + "See ``https://github.com/ansible/ansible/pull/79260`` for guidance on how to " + "move the reporting into specific strategy plugins to account for " + "``include_role`` tasks as well.", + version="2.21" + ) display.debug("loading included file: %s" % included_file._filename) try: data = self._loader.load_from_file(included_file._filename) @@ -870,11 +879,9 @@ class StrategyBase: loader=self._loader, variable_manager=self._variable_manager, ) - - # since we skip incrementing the stats when the task result is - # first processed, we do so now for each host in the list - for host in included_file._hosts: - self._tqm._stats.increment('ok', host.name) + if handle_stats_and_callbacks: + for host in included_file._hosts: + self._tqm._stats.increment('ok', host.name) except AnsibleParserError: raise except AnsibleError as e: @@ -882,18 +889,18 @@ class StrategyBase: reason = "Could not find or access '%s' on the Ansible Controller." % to_text(e.file_name) else: reason = to_text(e) - - for r in included_file._results: - r._result['failed'] = True - - for host in included_file._hosts: - tr = TaskResult(host=host, task=included_file._task, return_data=dict(failed=True, reason=reason)) - self._tqm._stats.increment('failures', host.name) - self._tqm.send_callback('v2_runner_on_failed', tr) + if handle_stats_and_callbacks: + for r in included_file._results: + r._result['failed'] = True + + for host in included_file._hosts: + tr = TaskResult(host=host, task=included_file._task, return_data=dict(failed=True, reason=reason)) + self._tqm._stats.increment('failures', host.name) + self._tqm.send_callback('v2_runner_on_failed', tr) raise AnsibleError(reason) from e - # finally, send the callback and return the list of blocks loaded - self._tqm.send_callback('v2_playbook_on_include', included_file) + if handle_stats_and_callbacks: + self._tqm.send_callback('v2_playbook_on_include', included_file) display.debug("done processing included file") return block_list diff --git a/lib/ansible/plugins/strategy/free.py b/lib/ansible/plugins/strategy/free.py index 2c981101416..fda3a0dd633 100644 --- a/lib/ansible/plugins/strategy/free.py +++ b/lib/ansible/plugins/strategy/free.py @@ -248,7 +248,12 @@ class StrategyModule(StrategyBase): ) else: is_handler = isinstance(included_file._task, Handler) - new_blocks = self._load_included_file(included_file, iterator=iterator, is_handler=is_handler) + new_blocks = self._load_included_file( + included_file, + iterator=iterator, + is_handler=is_handler, + handle_stats_and_callbacks=False, + ) # let PlayIterator know about any new handlers included via include_role or # import_role within include_role/include_taks @@ -256,13 +261,20 @@ class StrategyModule(StrategyBase): except AnsibleParserError: raise except AnsibleError as e: - if included_file._is_role: - # include_role does not have on_include callback so display the error - display.error(to_text(e), wrap_text=False) + display.error(to_text(e), wrap_text=False) for r in included_file._results: r._result['failed'] = True + r._result['reason'] = str(e) + self._tqm._stats.increment('failures', r._host.name) + self._tqm.send_callback('v2_runner_on_failed', r) failed_includes_hosts.add(r._host) continue + else: + # since we skip incrementing the stats when the task result is + # first processed, we do so now for each host in the list + for host in included_file._hosts: + self._tqm._stats.increment('ok', host.name) + self._tqm.send_callback('v2_playbook_on_include', included_file) for new_block in new_blocks: if is_handler: diff --git a/lib/ansible/plugins/strategy/linear.py b/lib/ansible/plugins/strategy/linear.py index 14d45d405da..71d6380211f 100644 --- a/lib/ansible/plugins/strategy/linear.py +++ b/lib/ansible/plugins/strategy/linear.py @@ -291,7 +291,12 @@ class StrategyModule(StrategyBase): ) else: is_handler = isinstance(included_file._task, Handler) - new_blocks = self._load_included_file(included_file, iterator=iterator, is_handler=is_handler) + new_blocks = self._load_included_file( + included_file, + iterator=iterator, + is_handler=is_handler, + handle_stats_and_callbacks=False, + ) # let PlayIterator know about any new handlers included via include_role or # import_role within include_role/include_taks @@ -324,13 +329,19 @@ class StrategyModule(StrategyBase): except AnsibleParserError: raise except AnsibleError as e: - if included_file._is_role: - # include_role does not have on_include callback so display the error - display.error(to_text(e), wrap_text=False) + display.error(to_text(e), wrap_text=False) for r in included_file._results: r._result['failed'] = True + r._result['reason'] = str(e) + self._tqm._stats.increment('failures', r._host.name) + self._tqm.send_callback('v2_runner_on_failed', r) failed_includes_hosts.add(r._host) - continue + else: + # since we skip incrementing the stats when the task result is + # first processed, we do so now for each host in the list + for host in included_file._hosts: + self._tqm._stats.increment('ok', host.name) + self._tqm.send_callback('v2_playbook_on_include', included_file) for host in failed_includes_hosts: self._tqm._failed_hosts[host.name] = True diff --git a/test/integration/targets/ansible-playbook-callbacks/callback_list_include_role_fail.expected b/test/integration/targets/ansible-playbook-callbacks/callback_list_include_role_fail.expected new file mode 100644 index 00000000000..ea2f4fe4e09 --- /dev/null +++ b/test/integration/targets/ansible-playbook-callbacks/callback_list_include_role_fail.expected @@ -0,0 +1,9 @@ +1 __init__ +7 v2_on_any +1 v2_playbook_on_play_start +1 v2_playbook_on_start +1 v2_playbook_on_stats +1 v2_playbook_on_task_start +1 v2_runner_on_failed +1 v2_runner_on_ok +1 v2_runner_on_start diff --git a/test/integration/targets/ansible-playbook-callbacks/callbacks_list.expected b/test/integration/targets/ansible-playbook-callbacks/callbacks_list.expected index 1d064a23db1..4a785acc41e 100644 --- a/test/integration/targets/ansible-playbook-callbacks/callbacks_list.expected +++ b/test/integration/targets/ansible-playbook-callbacks/callbacks_list.expected @@ -1,8 +1,8 @@ 1 __init__ -92 v2_on_any +93 v2_on_any 1 v2_on_file_diff 4 v2_playbook_on_handler_task_start - 2 v2_playbook_on_include + 3 v2_playbook_on_include 1 v2_playbook_on_no_hosts_matched 3 v2_playbook_on_notify 3 v2_playbook_on_play_start diff --git a/test/integration/targets/ansible-playbook-callbacks/include_role-fail.yml b/test/integration/targets/ansible-playbook-callbacks/include_role-fail.yml new file mode 100644 index 00000000000..fbfeb465af9 --- /dev/null +++ b/test/integration/targets/ansible-playbook-callbacks/include_role-fail.yml @@ -0,0 +1,5 @@ +- hosts: localhost + gather_facts: false + tasks: + - include_role: + name: does-not-exist diff --git a/test/integration/targets/ansible-playbook-callbacks/runme.sh b/test/integration/targets/ansible-playbook-callbacks/runme.sh index 933863e572f..77a5d9b70ac 100755 --- a/test/integration/targets/ansible-playbook-callbacks/runme.sh +++ b/test/integration/targets/ansible-playbook-callbacks/runme.sh @@ -5,8 +5,11 @@ set -eux export ANSIBLE_CALLBACK_PLUGINS=../support-callback_plugins/callback_plugins export ANSIBLE_ROLES_PATH=../ export ANSIBLE_STDOUT_CALLBACK=callback_debug -export ANSIBLE_HOST_PATTERN_MISMATCH=warning - -ansible-playbook all-callbacks.yml 2>/dev/null | sort | uniq -c | tee callbacks_list.out +ANSIBLE_HOST_PATTERN_MISMATCH=warning ansible-playbook all-callbacks.yml 2>/dev/null | sort | uniq -c | tee callbacks_list.out diff -w callbacks_list.out callbacks_list.expected + +for strategy in linear free; do + ANSIBLE_STRATEGY=$strategy ansible-playbook include_role-fail.yml 2>/dev/null | sort | uniq -c | tee callback_list_include_role_fail.out + diff -w callback_list_include_role_fail.out callback_list_include_role_fail.expected +done diff --git a/test/integration/targets/callback_default/callback_default.out.include_role_fails.stderr b/test/integration/targets/callback_default/callback_default.out.include_role_fails.stderr new file mode 100644 index 00000000000..315f17bbfe4 --- /dev/null +++ b/test/integration/targets/callback_default/callback_default.out.include_role_fails.stderr @@ -0,0 +1,12 @@ ++ ansible-playbook -i inventory test_include_role_fails.yml +++ set +x +ERROR! the role 'does-not-exist' was not found in TEST_PATH/roles:/root/.ansible/roles:/usr/share/ansible/roles:/etc/ansible/roles:TEST_PATH + +The error appears to be in 'TEST_PATH/test_include_role_fails.yml': line 5, column 15, but may +be elsewhere in the file depending on the exact syntax problem. + +The offending line appears to be: + + - include_role: + name: does-not-exist + ^ here diff --git a/test/integration/targets/callback_default/callback_default.out.include_role_fails.stdout b/test/integration/targets/callback_default/callback_default.out.include_role_fails.stdout new file mode 100644 index 00000000000..adfd21b65bc --- /dev/null +++ b/test/integration/targets/callback_default/callback_default.out.include_role_fails.stdout @@ -0,0 +1,9 @@ + +PLAY [testhost] **************************************************************** + +TASK [include_role : does-not-exist] ******************************************* +fatal: [testhost]: FAILED! => {"changed": false, "reason": "the role 'does-not-exist' was not found in TEST_PATH/roles:/root/.ansible/roles:/usr/share/ansible/roles:/etc/ansible/roles:TEST_PATH\n\nThe error appears to be in 'TEST_PATH/test_include_role_fails.yml': line 5, column 15, but may\nbe elsewhere in the file depending on the exact syntax problem.\n\nThe offending line appears to be:\n\n - include_role:\n name: does-not-exist\n ^ here\n"} + +PLAY RECAP ********************************************************************* +testhost : ok=0 changed=0 unreachable=0 failed=1 skipped=0 rescued=0 ignored=0 + diff --git a/test/integration/targets/callback_default/runme.sh b/test/integration/targets/callback_default/runme.sh index eab50bfb328..4dab4f40ae8 100755 --- a/test/integration/targets/callback_default/runme.sh +++ b/test/integration/targets/callback_default/runme.sh @@ -25,12 +25,11 @@ run_test() { { ansible-playbook -i inventory "$playbook" "${@:3}" \ > >(set +x; tee "${OUTFILE}.${testname}.stdout"); } \ 2> >(set +x; tee "${OUTFILE}.${testname}.stderr" >&2) - # Scrub deprication warning that shows up in Python 2.6 on CentOS 6 - sed -i -e '/RandomPool_DeprecationWarning/d' "${OUTFILE}.${testname}.stderr" sed -i -e 's/included: .*\/test\/integration/included: ...\/test\/integration/g' "${OUTFILE}.${testname}.stdout" sed -i -e 's/@@ -1,1 +1,1 @@/@@ -1 +1 @@/g' "${OUTFILE}.${testname}.stdout" sed -i -e 's/: .*\/test_diff\.txt/: ...\/test_diff.txt/g' "${OUTFILE}.${testname}.stdout" - sed -i -e "s#${ANSIBLE_PLAYBOOK_DIR}#TEST_PATH#g" "${OUTFILE}.${testname}.stdout" + sed -i -e "s#${ANSIBLE_PLAYBOOK_DIR}#TEST_PATH#g" "${OUTFILE}.${testname}.stdout" "${OUTFILE}.${testname}.stderr" + sed -i -e "s#/var/root/#/root/#g" "${OUTFILE}.${testname}.stdout" "${OUTFILE}.${testname}.stderr" # macos sed -i -e 's/^Using .*//g' "${OUTFILE}.${testname}.stdout" sed -i -e 's/[0-9]:[0-9]\{2\}:[0-9]\{2\}\.[0-9]\{6\}/0:00:00.000000/g' "${OUTFILE}.${testname}.stdout" sed -i -e 's/[0-9]\{4\}-[0-9]\{2\}-[0-9]\{2\} [0-9]\{2\}:[0-9]\{2\}:[0-9]\{2\}\.[0-9]\{6\}/0000-00-00 00:00:00.000000/g' "${OUTFILE}.${testname}.stdout" @@ -47,7 +46,7 @@ run_test_dryrun() { # optional, pass --check to run a dry run local chk=${2:-} - # outout was recorded w/o cowsay, ensure we reproduce the same + # output was recorded w/o cowsay, ensure we reproduce the same export ANSIBLE_NOCOWS=1 # This needed to satisfy shellcheck that can not accept unquoted variable @@ -58,8 +57,6 @@ run_test_dryrun() { { $cmd \ > >(set +x; tee "${OUTFILE}.${testname}.stdout"); } \ 2> >(set +x; tee "${OUTFILE}.${testname}.stderr" >&2) - # Scrub deprication warning that shows up in Python 2.6 on CentOS 6 - sed -i -e '/RandomPool_DeprecationWarning/d' "${OUTFILE}.${testname}.stderr" diff -u "${ORIGFILE}.${testname}.stdout" "${OUTFILE}.${testname}.stdout" || diff_failure diff -u "${ORIGFILE}.${testname}.stderr" "${OUTFILE}.${testname}.stderr" || diff_failure @@ -132,6 +129,10 @@ export ANSIBLE_CHECK_MODE_MARKERS=0 run_test default test.yml +set +e +ANSIBLE_CALLBACKS_ENABLED=default run_test include_role_fails test_include_role_fails.yml +set -e + # Check for async output # NOTE: regex to match 1 or more digits works for both BSD and GNU grep ansible-playbook -i inventory test_async.yml 2>&1 | tee async_test.out @@ -177,7 +178,7 @@ export ANSIBLE_DISPLAY_OK_HOSTS=1 export ANSIBLE_DISPLAY_FAILED_STDERR=1 export ANSIBLE_TIMEOUT=1 -# Check if UNREACHBLE is available in stderr +# Check if UNREACHABLE is available in stderr set +e ansible-playbook -i inventory test_2.yml > >(set +x; tee "${BASEFILE}.unreachable.stdout";) 2> >(set +x; tee "${BASEFILE}.unreachable.stderr" >&2) || true set -e diff --git a/test/integration/targets/callback_default/test_include_role_fails.yml b/test/integration/targets/callback_default/test_include_role_fails.yml new file mode 100644 index 00000000000..3749c905939 --- /dev/null +++ b/test/integration/targets/callback_default/test_include_role_fails.yml @@ -0,0 +1,5 @@ +- hosts: testhost + gather_facts: false + tasks: + - include_role: + name: does-not-exist diff --git a/test/integration/targets/include_import/runme.sh b/test/integration/targets/include_import/runme.sh index 078f080b0db..98bc8c31612 100755 --- a/test/integration/targets/include_import/runme.sh +++ b/test/integration/targets/include_import/runme.sh @@ -123,7 +123,7 @@ test "$(grep -c 'ok=3' test_allow_single_role_dup.out)" = 1 # test templating public, allow_duplicates, and rolespec_validate ansible-playbook tasks/test_templating_IncludeRole_FA.yml 2>&1 | tee IncludeRole_FA_template.out -test "$(grep -c 'ok=4' IncludeRole_FA_template.out)" = 1 +test "$(grep -c 'ok=6' IncludeRole_FA_template.out)" = 1 test "$(grep -c 'failed=0' IncludeRole_FA_template.out)" = 1 # https://github.com/ansible/ansible/issues/66764