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

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)"

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')

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
 

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:

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

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

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

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

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

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

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   
+

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

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

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