Change v2_playbook_on_start logic to positively detect legacy plugins

In order to support legacy plugins, the following two method signatures
are allowed for `CallbackBase.v2_playbook_on_start`:

def v2_playbook_on_start(self):
def v2_playbook_on_start(self, playbook):

Previously, the logic to handle this divergence checked to see if the
callback plugin being called supported an argument named `playbook`
in its `v2_playbook_on_start` method. This was fragile in a few ways:
 - if a plugin author did not use the literal `playbook` to name their
   method argument, their plugin would not be called correctly
 - if a plugin author wrapped their `v2_playbook_on_start` method and
   by doing so changed the argspec to no longer expose an argument
   with that literal name, their plugin would not be called correctly

In order to continue to support both types of callback for backwards
compatibility while making the call more robust for plugin authors,
the logic can be reversed in order to have a positive check for the old
method signature instead of a positive check for the new one.

Signed-off-by: Steve Kuznetsov <skuznets@redhat.com>

lib/ansible/executor/task_queue_manager.py

@@ -353,17 +353,20 @@ class TaskQueueManager:
 
             for method in methods:
                 try:
-                    # temporary hack, required due to a change in the callback API, so
-                    # we don't break backwards compatibility with callbacks which were
-                    # designed to use the original API
+                    # Previously, the `v2_playbook_on_start` callback API did not accept
+                    # any arguments. In recent versions of the v2 callback API, the play-
+                    # book that started execution is given. In order to support both of
+                    # these method signatures, we need to use this `inspect` hack to send
+                    # no arguments to the methods that don't accept them. This way, we can
+                    # not break backwards compatibility until that API is deprecated.
                     # FIXME: target for removal and revert to the original code here after a year (2017-01-14)
                     if method_name == 'v2_playbook_on_start':
                         import inspect
-                        (f_args, f_varargs, f_keywords, f_defaults) = inspect.getargspec(method)
-                        if 'playbook' in f_args:
-                            method(*args, **kwargs)
-                        else:
+                        argspec = inspect.getargspec(method)
+                        if argspec.args == ['self']:
                             method()
+                        else:
+                            method(*args, **kwargs)
                     else:
                         method(*args, **kwargs)
                 except Exception as e:

test/units/executor/test_task_queue_manager_callbacks.py

@@ -0,0 +1,141 @@
+# (c) 2016, Steve Kuznetsov <skuznets@redhat.com>
+#
+# This file is part of Ansible
+#
+# Ansible is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+#
+# Ansible is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with Ansible.  If not, see <http://www.gnu.org/licenses/>.
+
+# Make coding more python3-ish
+from __future__ import (absolute_import, division, print_function)
+
+from ansible.compat.tests import unittest
+from ansible.compat.tests.mock import MagicMock
+from ansible.executor.task_queue_manager import TaskQueueManager
+from ansible.playbook import Playbook
+from ansible.plugins.callback import CallbackBase
+
+__metaclass__ = type
+
+
+class TestTaskQueueManagerCallbacks(unittest.TestCase):
+    def setUp(self):
+        inventory = MagicMock()
+        variable_manager = MagicMock()
+        loader = MagicMock()
+        options = MagicMock()
+        passwords = []
+
+        self._tqm = TaskQueueManager(inventory, variable_manager, loader, options, passwords)
+        self._playbook = Playbook(loader)
+
+        # we use a MagicMock to register the result of the call we
+        # expect to `v2_playbook_on_call`. We don't mock out the
+        # method since we're testing code that uses `inspect` to
+        # look at that method's argspec and we want to ensure this
+        # test is easy to reason about.
+        self._register = MagicMock()
+
+    def tearDown(self):
+        pass
+
+    def test_task_queue_manager_callbacks_v2_playbook_on_start_legacy(self):
+        """
+        Assert that no exceptions are raised when sending a Playbook
+        start callback to a legacy callback module plugin.
+        """
+        register = self._register
+
+        class LegacyCallbackModule(CallbackBase):
+            """
+            This is a callback module with the legacy
+            method signature for `v2_playbook_on_start`.
+            """
+            CALLBACK_VERSION = 2.0
+            CALLBACK_TYPE = 'notification'
+            CALLBACK_NAME = 'legacy_module'
+
+            def v2_playbook_on_start(self):
+                register(self)
+
+        callback_module = LegacyCallbackModule()
+        self._tqm._callback_plugins.append(callback_module)
+        self._tqm.send_callback('v2_playbook_on_start', self._playbook)
+        register.assert_called_once_with(callback_module)
+
+    def test_task_queue_manager_callbacks_v2_playbook_on_start(self):
+        """
+        Assert that no exceptions are raised when sending a Playbook
+        start callback to a current callback module plugin.
+        """
+        register = self._register
+
+        class CallbackModule(CallbackBase):
+            """
+            This is a callback module with the current
+            method signature for `v2_playbook_on_start`.
+            """
+            CALLBACK_VERSION = 2.0
+            CALLBACK_TYPE = 'notification'
+            CALLBACK_NAME = 'current_module'
+
+            def v2_playbook_on_start(self, playbook):
+                register(self, playbook)
+
+        callback_module = CallbackModule()
+        self._tqm._callback_plugins.append(callback_module)
+        self._tqm.send_callback('v2_playbook_on_start', self._playbook)
+        register.assert_called_once_with(callback_module, self._playbook)
+
+    def test_task_queue_manager_callbacks_v2_playbook_on_start_wrapped(self):
+        """
+        Assert that no exceptions are raised when sending a Playbook
+        start callback to a wrapped current callback module plugin.
+        """
+        register = self._register
+
+        def wrap_callback(func):
+            """
+            This wrapper changes the exposed argument
+            names for a method from the original names
+            to (*args, **kwargs). This is used in order
+            to validate that wrappers which change par-
+            ameter names do not break the TQM callback
+            system.
+
+            :param func: function to decorate
+            :return: decorated function
+            """
+
+            def wrapper(*args, **kwargs):
+                return func(*args, **kwargs)
+
+            return wrapper
+
+        class WrappedCallbackModule(CallbackBase):
+            """
+            This is a callback module with the current
+            method signature for `v2_playbook_on_start`
+            wrapped in order to change the signature.
+            """
+            CALLBACK_VERSION = 2.0
+            CALLBACK_TYPE = 'notification'
+            CALLBACK_NAME = 'current_module'
+
+            @wrap_callback
+            def v2_playbook_on_start(self, playbook):
+                register(self, playbook)
+
+        callback_module = WrappedCallbackModule()
+        self._tqm._callback_plugins.append(callback_module)
+        self._tqm.send_callback('v2_playbook_on_start', self._playbook)
+        register.assert_called_once_with(callback_module, self._playbook)