make collection callbacks follow normal flow (#59932) (#72228)

* make collection callbacks follow normal flow (#59932)

make collections whitelist follow normal flow

* fixes missing set_options call and adhoc and stdout processing rules
* avoid dupes
* fixed to handle redirects
* also updated tests with new and more accurate skip message
* fix callback tests for envs with cowsay installed
* lots MOAR comments on why the code is as it is, some todos to refactor in future

Co-authored-by: Matt Davis <nitzmahone@users.noreply.github.com>
(cherry picked from commit 5ec53f9db8)

* fixed bad merge

* hack in redirected names

* ensure we run additional calblacks

changelogs/fragments/collections_cb_fix.yml

@@ -0,0 +1,2 @@
+bugfixes:
+    - Collection callbacks were ignoring options and rules for stdout and adhoc cases.

lib/ansible/executor/task_queue_manager.py

@@ -139,35 +139,71 @@ class TaskQueueManager:
         else:
             raise AnsibleError("callback must be an instance of CallbackBase or the name of a callback plugin")
 
-        for callback_plugin in callback_loader.all(class_only=True):
+        # get all configured loadable callbacks (adjacent, builtin)
+        callback_list = list(callback_loader.all(class_only=True))
+
+        # add whitelisted callbacks that refer to collections, which might not appear in normal listing
+        for c in C.DEFAULT_CALLBACK_WHITELIST:
+            # load all, as collection ones might be using short/redirected names and not a fqcn
+            plugin = callback_loader.get(c, class_only=True)
+
+            # TODO: check if this skip is redundant, loader should handle bad file/plugin cases already
+            if plugin:
+                # avoids incorrect and dupes possible due to collections
+                if plugin not in callback_list:
+                    setattr(plugin, '_redirected_names', [c])  # here for backport as newer versions of plugin_loader already do this
+                    callback_list.append(plugin)
+            else:
+                display.warning("Skipping callback plugin '%s', unable to load" % c)
+
+        # for each callback in the list see if we should add it to 'active callbacks' used in the play
+        for callback_plugin in callback_list:
+
             callback_type = getattr(callback_plugin, 'CALLBACK_TYPE', '')
             callback_needs_whitelist = getattr(callback_plugin, 'CALLBACK_NEEDS_WHITELIST', False)
-            (callback_name, _) = os.path.splitext(os.path.basename(callback_plugin._original_path))
+
+            # try to get colleciotn world name first
+            cnames = getattr(callback_plugin, '_redirected_names', [])
+            if cnames:
+                # store the name the plugin was loaded as, as that's what we'll need to compare to the configured callback list later
+                callback_name = cnames[0]
+            else:
+                # fallback to 'old loader name'
+                (callback_name, _) = os.path.splitext(os.path.basename(callback_plugin._original_path))
+
+            display.vvvvv("Attempting to use '%s' callback." % (callback_name))
             if callback_type == 'stdout':
                 # we only allow one callback of type 'stdout' to be loaded,
                 if callback_name != self._stdout_callback or stdout_callback_loaded:
+                    display.vv("Skipping callback '%s', as we already have a stdout callback." % (callback_name))
                     continue
                 stdout_callback_loaded = True
             elif callback_name == 'tree' and self._run_tree:
-                # special case for ansible cli option
+                # TODO: remove special case for tree, which is an adhoc cli option --tree
                 pass
             elif not self._run_additional_callbacks or (callback_needs_whitelist and (
+                # only run if not adhoc, or adhoc was specifically configured to run + check enabled list
                     C.DEFAULT_CALLBACK_WHITELIST is None or callback_name not in C.DEFAULT_CALLBACK_WHITELIST)):
                 # 2.x plugins shipped with ansible should require whitelisting, older or non shipped should load automatically
                 continue
 
-            callback_obj = callback_plugin()
-            callback_obj.set_options()
-            self._callback_plugins.append(callback_obj)
-
-        for callback_plugin_name in (c for c in C.DEFAULT_CALLBACK_WHITELIST if AnsibleCollectionRef.is_valid_fqcr(c)):
-            # TODO: need to extend/duplicate the stdout callback check here (and possible move this ahead of the old way
-            callback_obj = callback_loader.get(callback_plugin_name)
-            if callback_obj:
-                callback_obj.set_options()
-                self._callback_plugins.append(callback_obj)
-            else:
-                display.warning("Skipping '%s', unable to load or use as a callback" % callback_plugin_name)
+            try:
+                callback_obj = callback_plugin()
+                # avoid bad plugin not returning an object, only needed cause we do class_only load and bypass loader checks,
+                # really a bug in the plugin itself which we ignore as callback errors are not supposed to be fatal.
+                if callback_obj:
+                    # skip initializing if we already did the work for the same plugin (even with diff names)
+                    if callback_obj not in self._callback_plugins:
+                        callback_obj.set_options()
+                        self._callback_plugins.append(callback_obj)
+                    else:
+                        display.vv("Skipping callback '%s', already loaded as '%s'." % (callback_plugin, callback_name))
+                else:
+                    display.warning("Skipping callback '%s', as it does not create a valid plugin instance." % callback_name)
+                    continue
+            except Exception as e:
+                display.warning("Skipping callback '%s', unable to load due to: %s" % (callback_name, to_native(e)))
+                continue
 
         self._callbacks_loaded = True
 

test/integration/targets/callback_default/runme.sh

@@ -16,6 +16,9 @@ set -eux
 run_test() {
 	local testname=$1
 
+	# outout was recorded w/o cowsay, ensure we reproduce the same
+	export ANSIBLE_NOCOWS=1
+
 	# The shenanigans with redirection and 'tee' are to capture STDOUT and
 	# STDERR separately while still displaying both to the console
 	{ ansible-playbook -i inventory test.yml \
@@ -33,6 +36,9 @@ 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
+	export ANSIBLE_NOCOWS=1
+
 	# This needed to satisfy shellcheck that can not accept unquoted variable
 	cmd="ansible-playbook -i inventory ${chk} test_dryrun.yml"
 

test/integration/targets/collections/runme.sh

@@ -13,7 +13,7 @@ ipath=../../$(basename "${INVENTORY_PATH}")
 export INVENTORY_PATH="$ipath"
 
 # test callback
-ANSIBLE_CALLBACK_WHITELIST=testns.testcoll.usercallback ansible localhost -m ping | grep "usercallback says ok"
+ANSIBLE_LOAD_CALLBACK_PLUGINS=1 ANSIBLE_CALLBACK_WHITELIST=testns.testcoll.usercallback ansible localhost -m ping | grep "usercallback says ok"
 
 # test documentation
 ansible-doc testns.testcoll.testmodule -vvv | grep -- "- normal_doc_frag"