rpc: Rework 'get_notifier', 'wrap_exception'

The 'nova.exception_wrapper.wrap_exception' decorator accepted either a
pre-configured notifier or a 'get_notifier' function, but the forget was
never provided and the latter was consistently a notifier created via a
call to 'nova.rpc.get_notifier'. Simplify things by passing the
arguments relied by 'get_notifier' into 'wrap_exception', allowing the
latter to create the former for us.

While doing this rework, it became obvious that 'get_notifier' accepted
a 'published_id' that is never provided nowadays, so that is dropped. In
addition, a number of calls to 'get_notifier' were passing in
'host=CONF.host', which duplicated the default value for this parameter
and is therefore unnecessary. Finally, the unit tests are split up by
file, as they should be.

Change-Id: I89e1c13e8a0df18594593b1e80c60d177e0d9c4c
Signed-off-by: Stephen Finucane <stephenfin@redhat.com>

nova/compute/api.py

@@ -85,15 +85,13 @@ from nova.volume import cinder
 
 LOG = logging.getLogger(__name__)
 
-get_notifier = functools.partial(rpc.get_notifier, service='compute')
 # NOTE(gibi): legacy notification used compute as a service but these
 # calls still run on the client side of the compute service which is
 # nova-api. By setting the binary to nova-api below, we can make sure
 # that the new versioned notifications has the right publisher_id but the
 # legacy notifications does not change.
-wrap_exception = functools.partial(exception_wrapper.wrap_exception,
+wrap_exception = functools.partial(
-                                   get_notifier=get_notifier,
+    exception_wrapper.wrap_exception, service='compute', binary='nova-api')
-                                   binary='nova-api')
 CONF = nova.conf.CONF
 
 AGGREGATE_ACTION_UPDATE = 'Update'
@@ -328,7 +326,7 @@ class API(base.Base):
         self.compute_task_api = conductor.ComputeTaskAPI()
         self.servicegroup_api = servicegroup.API()
         self.host_api = HostAPI(self.compute_rpcapi, self.servicegroup_api)
-        self.notifier = rpc.get_notifier('compute', CONF.host)
+        self.notifier = rpc.get_notifier('compute')
         if CONF.ephemeral_storage_encryption.enabled:
             self.key_manager = key_manager.API()
         # Help us to record host in EventReporter
@@ -6326,10 +6324,12 @@ class AggregateAPI(base.Base):
 class KeypairAPI(base.Base):
     """Subset of the Compute Manager API for managing key pairs."""
 
-    get_notifier = functools.partial(rpc.get_notifier, service='api')
+    wrap_exception = functools.partial(
-    wrap_exception = functools.partial(exception_wrapper.wrap_exception,
+        exception_wrapper.wrap_exception, service='api', binary='nova-api')
-                                       get_notifier=get_notifier,
+
-                                       binary='nova-api')
+    def __init__(self):
+        super().__init__()
+        self.notifier = rpc.get_notifier('api')
 
     def _notify(self, context, event_suffix, keypair_name):
         payload = {
@@ -6337,8 +6337,7 @@ class KeypairAPI(base.Base):
             'user_id': context.user_id,
             'key_name': keypair_name,
         }
-        notify = self.get_notifier()
+        self.notifier.info(context, 'keypair.%s' % event_suffix, payload)
-        notify.info(context, 'keypair.%s' % event_suffix, payload)
 
     def _validate_new_key_pair(self, context, user_id, key_name, key_type):
         safe_chars = "_- " + string.digits + string.ascii_letters

nova/compute/manager.py

@@ -85,7 +85,6 @@ from nova.objects import instance as obj_instance
 from nova.objects import migrate_data as migrate_data_obj
 from nova.pci import request as pci_req_module
 from nova.pci import whitelist
-from nova import rpc
 from nova import safe_utils
 from nova.scheduler.client import query
 from nova.scheduler.client import report
@@ -104,10 +103,8 @@ CONF = nova.conf.CONF
 
 LOG = logging.getLogger(__name__)
 
-get_notifier = functools.partial(rpc.get_notifier, service='compute')
+wrap_exception = functools.partial(
-wrap_exception = functools.partial(exception_wrapper.wrap_exception,
+    exception_wrapper.wrap_exception, service='compute', binary='nova-compute')
-                                   get_notifier=get_notifier,
-                                   binary='nova-compute')
 
 
 @contextlib.contextmanager

nova/conductor/manager.py

@@ -244,7 +244,7 @@ class ComputeTaskManager(base.Base):
         self.servicegroup_api = servicegroup.API()
         self.query_client = query.SchedulerQueryClient()
         self.report_client = report.SchedulerReportClient()
-        self.notifier = rpc.get_notifier('compute', CONF.host)
+        self.notifier = rpc.get_notifier('compute')
         # Help us to record host in EventReporter
         self.host = CONF.host
 

nova/exception_wrapper.py

@@ -25,14 +25,6 @@ from nova import safe_utils
 CONF = nova.conf.CONF
 
 
-def _emit_exception_notification(
-    notifier, context, exception, function_name, args, source,
-):
-    _emit_legacy_exception_notification(
-        notifier, context, exception, function_name, args)
-    _emit_versioned_exception_notification(context, exception, source)
-
-
 @rpc.if_notifications_enabled
 def _emit_versioned_exception_notification(context, exception, source):
     payload = exception_obj.ExceptionPayload.from_exception(exception)
@@ -50,13 +42,15 @@ def _emit_versioned_exception_notification(context, exception, source):
     notification.emit(context)
 
 
-def _emit_legacy_exception_notification(notifier, context, ex, function_name,
+def _emit_legacy_exception_notification(
-                                       args):
+    context, exception, service, function_name, args,
-    payload = dict(exception=ex, args=args)
+):
+    notifier = rpc.get_notifier(service)
+    payload = {'exception': exception, 'args': args}
     notifier.error(context, function_name, payload)
 
 
-def wrap_exception(notifier=None, get_notifier=None, binary=None):
+def wrap_exception(service, binary):
     """This decorator wraps a method to catch any exceptions that may
     get thrown. It also optionally sends the exception to the notification
     system.
@@ -69,14 +63,13 @@ def wrap_exception(notifier=None, get_notifier=None, binary=None):
                 return f(self, context, *args, **kw)
             except Exception as exc:
                 with excutils.save_and_reraise_exception():
-                    if notifier or get_notifier:
+                    call_dict = _get_call_dict(f, self, context, *args, **kw)
-                        call_dict = _get_call_dict(
+                    function_name = f.__name__
-                            f, self, context, *args, **kw)
-                        function_name = f.__name__
-                        _emit_exception_notification(
-                            notifier or get_notifier(), context, exc,
-                            function_name, call_dict, binary)
 
+                    _emit_legacy_exception_notification(
+                        context, exc, service, function_name, call_dict)
+                    _emit_versioned_exception_notification(
+                        context, exc, binary)
         return functools.wraps(f)(wrapped)
     return inner
 

nova/rpc.py

@@ -227,12 +227,11 @@ def get_server(target, endpoints, serializer=None):
                                     access_policy=access_policy)
 
 
-def get_notifier(service, host=None, publisher_id=None):
+def get_notifier(service, host=None):
     assert LEGACY_NOTIFIER is not None
-    if not publisher_id:
+    publisher_id = '%s.%s' % (service, host or CONF.host)
-        publisher_id = "%s.%s" % (service, host or CONF.host)
     return LegacyValidatingNotifier(
-            LEGACY_NOTIFIER.prepare(publisher_id=publisher_id))
+        LEGACY_NOTIFIER.prepare(publisher_id=publisher_id))
 
 
 def get_versioned_notifier(publisher_id):

nova/tests/unit/compute/test_compute_mgr.py

@@ -7131,14 +7131,19 @@ class ComputeManagerBuildInstanceTestCase(test.NoDBTestCase):
         mock_succeeded.assert_not_called()
 
     @mock.patch.object(manager.ComputeManager, '_do_build_and_run_instance')
-    @mock.patch('nova.exception_wrapper._emit_exception_notification')
+    @mock.patch(
-    @mock.patch('nova.compute.utils.add_instance_fault_from_exc')
+        'nova.exception_wrapper._emit_versioned_exception_notification',
+        new=mock.Mock())
+    @mock.patch(
+        'nova.exception_wrapper._emit_legacy_exception_notification',
+        new=mock.Mock())
+    @mock.patch(
+        'nova.compute.utils.add_instance_fault_from_exc', new=mock.Mock())
     @mock.patch.object(manager.ComputeManager, '_build_failed')
     @mock.patch.object(manager.ComputeManager, '_build_succeeded')
-    def test_build_exceptions_reported(self, mock_succeeded,
+    def test_build_exceptions_reported(
-                                       mock_failed,
+        self, mock_succeeded, mock_failed, mock_dbari,
-                                       mock_if, mock_notify,
+    ):
-                                       mock_dbari):
         mock_dbari.side_effect = test.TestingException()
         instance = objects.Instance(uuid=uuids.instance,
                                     task_state=None)

nova/tests/unit/test_exception.py

@@ -17,128 +17,10 @@
 import inspect
 
 import fixtures
-import mock
 from webob.util import status_reasons
 
-from nova import context
 from nova import exception
-from nova import exception_wrapper
-from nova import rpc
 from nova import test
-from nova.tests.unit import fake_notifier
-
-
-def good_function(self, context):
-    return 99
-
-
-def bad_function_exception(self, context, extra, blah="a", boo="b", zoo=None):
-    raise test.TestingException('bad things happened')
-
-
-def bad_function_unknown_module(self, context):
-    """Example traceback that points to a module that getmodule() can't find.
-
-    Traceback (most recent call last):
-      File "<stdin>", line 1, in <module>
-      File "src/lxml/lxml.etree.pyx", line 2402, in
-          lxml.etree._Attrib.__setitem__ (src/lxml/lxml.etree.c:67548)
-      File "src/lxml/apihelpers.pxi", line 570, in
-          lxml.etree._setAttributeValue (src/lxml/lxml.etree.c:21551)
-      File "src/lxml/apihelpers.pxi", line 1437, in
-          lxml.etree._utf8 (src/lxml/lxml.etree.c:30194)
-    TypeError: Argument must be bytes or unicode, got 'NoneType'
-
-    """
-    from lxml import etree
-    x = etree.fromstring('<hello/>')
-    x.attrib['foo'] = None
-
-
-class WrapExceptionTestCase(test.NoDBTestCase):
-    def setUp(self):
-        super(WrapExceptionTestCase, self).setUp()
-        fake_notifier.stub_notifier(self)
-        self.addCleanup(fake_notifier.reset)
-
-    def test_wrap_exception_good_return(self):
-        wrapped = exception_wrapper.wrap_exception(rpc.get_notifier('fake'))
-        self.assertEqual(99, wrapped(good_function)(1, 2))
-        self.assertEqual(0, len(fake_notifier.NOTIFICATIONS))
-        self.assertEqual(0, len(fake_notifier.VERSIONED_NOTIFICATIONS))
-
-    def test_wrap_exception_unknown_module(self):
-        ctxt = context.get_admin_context()
-        wrapped = exception_wrapper.wrap_exception(
-            rpc.get_notifier('fake'), binary='nova-compute')
-        self.assertRaises(
-            TypeError, wrapped(bad_function_unknown_module), None, ctxt)
-        self.assertEqual(1, len(fake_notifier.VERSIONED_NOTIFICATIONS))
-        notification = fake_notifier.VERSIONED_NOTIFICATIONS[0]
-        payload = notification['payload']['nova_object.data']
-        self.assertEqual('unknown', payload['module_name'])
-
-    def test_wrap_exception_with_notifier(self):
-        wrapped = exception_wrapper.wrap_exception(rpc.get_notifier('fake'),
-                                                   binary='nova-compute')
-        ctxt = context.get_admin_context()
-        self.assertRaises(test.TestingException,
-                          wrapped(bad_function_exception), 1, ctxt, 3, zoo=3)
-
-        self.assertEqual(1, len(fake_notifier.NOTIFICATIONS))
-        notification = fake_notifier.NOTIFICATIONS[0]
-        self.assertEqual('bad_function_exception', notification.event_type)
-        self.assertEqual(ctxt, notification.context)
-        self.assertEqual(3, notification.payload['args']['extra'])
-        for key in ['exception', 'args']:
-            self.assertIn(key, notification.payload.keys())
-        self.assertNotIn('context', notification.payload['args'].keys())
-
-        self.assertEqual(1, len(fake_notifier.VERSIONED_NOTIFICATIONS))
-        notification = fake_notifier.VERSIONED_NOTIFICATIONS[0]
-        self.assertEqual('compute.exception', notification['event_type'])
-        self.assertEqual('nova-compute:fake-mini',
-                         notification['publisher_id'])
-        self.assertEqual('ERROR', notification['priority'])
-
-        payload = notification['payload']
-        self.assertEqual('ExceptionPayload', payload['nova_object.name'])
-        self.assertEqual('1.1', payload['nova_object.version'])
-
-        payload = payload['nova_object.data']
-        self.assertEqual('TestingException', payload['exception'])
-        self.assertEqual('bad things happened', payload['exception_message'])
-        self.assertEqual('bad_function_exception', payload['function_name'])
-        self.assertEqual('nova.tests.unit.test_exception',
-                         payload['module_name'])
-        self.assertIn('bad_function_exception', payload['traceback'])
-
-    @mock.patch('nova.rpc.NOTIFIER')
-    @mock.patch('nova.notifications.objects.exception.'
-                'ExceptionNotification.__init__')
-    def test_wrap_exception_notification_not_emitted_if_disabled(
-            self, mock_notification, mock_notifier):
-        mock_notifier.is_enabled.return_value = False
-
-        wrapped = exception_wrapper.wrap_exception(rpc.get_notifier('fake'),
-                                                   binary='nova-compute')
-        ctxt = context.get_admin_context()
-        self.assertRaises(test.TestingException,
-                          wrapped(bad_function_exception), 1, ctxt, 3, zoo=3)
-        self.assertFalse(mock_notification.called)
-
-    @mock.patch('nova.notifications.objects.exception.'
-                'ExceptionNotification.__init__')
-    def test_wrap_exception_notification_not_emitted_if_unversioned(
-            self, mock_notifier):
-        self.flags(notification_format='unversioned', group='notifications')
-
-        wrapped = exception_wrapper.wrap_exception(rpc.get_notifier('fake'),
-                                                   binary='nova-compute')
-        ctxt = context.get_admin_context()
-        self.assertRaises(test.TestingException,
-                          wrapped(bad_function_exception), 1, ctxt, 3, zoo=3)
-        self.assertFalse(mock_notifier.called)
 
 
 class NovaExceptionTestCase(test.NoDBTestCase):
@@ -184,13 +66,6 @@ class NovaExceptionTestCase(test.NoDBTestCase):
         exc = FakeNovaException(code=404)
         self.assertEqual(exc.kwargs['code'], 404)
 
-    def test_cleanse_dict(self):
-        kwargs = {'foo': 1, 'blah_pass': 2, 'zoo_password': 3, '_pass': 4}
-        self.assertEqual({'foo': 1}, exception_wrapper._cleanse_dict(kwargs))
-
-        kwargs = {}
-        self.assertEqual({}, exception_wrapper._cleanse_dict(kwargs))
-
     def test_format_message_local(self):
         class FakeNovaException(exception.NovaException):
             msg_fmt = "some message"

nova/tests/unit/test_exception_wrapper.py

@@ -0,0 +1,143 @@
+# Copyright 2010 United States Government as represented by the
+# Administrator of the National Aeronautics and Space Administration.
+# All Rights Reserved.
+#
+# Licensed under the Apache License, Version 2.0 (the "License"); you may
+# not use this file except in compliance with the License. You may obtain
+# a copy of the License at
+#
+#      http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+# License for the specific language governing permissions and limitations
+# under the License.
+
+import mock
+
+from nova import context as nova_context
+from nova import exception_wrapper
+from nova import test
+from nova.tests.unit import fake_notifier
+
+
+def bad_function_exception(self, context, extra, blah="a", boo="b", zoo=None):
+    raise test.TestingException('bad things happened')
+
+
+def bad_function_unknown_module(self, context):
+    """Example traceback that points to a module that getmodule() can't find.
+
+    Traceback (most recent call last):
+      File "<stdin>", line 1, in <module>
+      File "src/lxml/lxml.etree.pyx", line 2402, in
+          lxml.etree._Attrib.__setitem__ (src/lxml/lxml.etree.c:67548)
+      File "src/lxml/apihelpers.pxi", line 570, in
+          lxml.etree._setAttributeValue (src/lxml/lxml.etree.c:21551)
+      File "src/lxml/apihelpers.pxi", line 1437, in
+          lxml.etree._utf8 (src/lxml/lxml.etree.c:30194)
+    TypeError: Argument must be bytes or unicode, got 'NoneType'
+
+    """
+    from lxml import etree
+    x = etree.fromstring('<hello/>')
+    x.attrib['foo'] = None
+
+
+def good_function(self, context):
+    return 99
+
+
+class WrapExceptionTestCase(test.NoDBTestCase):
+    def setUp(self):
+        super(WrapExceptionTestCase, self).setUp()
+        fake_notifier.stub_notifier(self)
+        self.addCleanup(fake_notifier.reset)
+
+    def test_cleanse_dict(self):
+        kwargs = {'foo': 1, 'blah_pass': 2, 'zoo_password': 3, '_pass': 4}
+        self.assertEqual({'foo': 1}, exception_wrapper._cleanse_dict(kwargs))
+
+        kwargs = {}
+        self.assertEqual({}, exception_wrapper._cleanse_dict(kwargs))
+
+    def test_wrap_exception_good_return(self):
+        wrapped = exception_wrapper.wrap_exception(
+            service='compute', binary='nova-compute')
+        self.assertEqual(99, wrapped(good_function)(1, 2))
+        self.assertEqual(0, len(fake_notifier.NOTIFICATIONS))
+        self.assertEqual(0, len(fake_notifier.VERSIONED_NOTIFICATIONS))
+
+    def test_wrap_exception_unknown_module(self):
+        ctxt = nova_context.get_admin_context()
+        wrapped = exception_wrapper.wrap_exception(
+            service='compute', binary='nova-compute')
+        self.assertRaises(
+            TypeError, wrapped(bad_function_unknown_module), None, ctxt)
+        self.assertEqual(1, len(fake_notifier.VERSIONED_NOTIFICATIONS))
+        notification = fake_notifier.VERSIONED_NOTIFICATIONS[0]
+        payload = notification['payload']['nova_object.data']
+        self.assertEqual('unknown', payload['module_name'])
+
+    def test_wrap_exception_with_notifier(self):
+        wrapped = exception_wrapper.wrap_exception(
+            service='compute', binary='nova-compute')
+        ctxt = nova_context.get_admin_context()
+        self.assertRaises(test.TestingException,
+                          wrapped(bad_function_exception), 1, ctxt, 3, zoo=3)
+
+        self.assertEqual(1, len(fake_notifier.NOTIFICATIONS))
+        notification = fake_notifier.NOTIFICATIONS[0]
+        self.assertEqual('bad_function_exception', notification.event_type)
+        self.assertEqual(ctxt, notification.context)
+        self.assertEqual(3, notification.payload['args']['extra'])
+        for key in ['exception', 'args']:
+            self.assertIn(key, notification.payload.keys())
+        self.assertNotIn('context', notification.payload['args'].keys())
+
+        self.assertEqual(1, len(fake_notifier.VERSIONED_NOTIFICATIONS))
+        notification = fake_notifier.VERSIONED_NOTIFICATIONS[0]
+        self.assertEqual('compute.exception', notification['event_type'])
+        self.assertEqual('nova-compute:fake-mini',
+                         notification['publisher_id'])
+        self.assertEqual('ERROR', notification['priority'])
+
+        payload = notification['payload']
+        self.assertEqual('ExceptionPayload', payload['nova_object.name'])
+        self.assertEqual('1.1', payload['nova_object.version'])
+
+        payload = payload['nova_object.data']
+        self.assertEqual('TestingException', payload['exception'])
+        self.assertEqual('bad things happened', payload['exception_message'])
+        self.assertEqual('bad_function_exception', payload['function_name'])
+        self.assertEqual('nova.tests.unit.test_exception_wrapper',
+                         payload['module_name'])
+        self.assertIn('bad_function_exception', payload['traceback'])
+
+    @mock.patch('nova.rpc.NOTIFIER')
+    @mock.patch('nova.notifications.objects.exception.'
+                'ExceptionNotification.__init__')
+    def test_wrap_exception_notification_not_emitted_if_disabled(
+            self, mock_notification, mock_notifier):
+        mock_notifier.is_enabled.return_value = False
+
+        wrapped = exception_wrapper.wrap_exception(
+            service='compute', binary='nova-compute')
+        ctxt = nova_context.get_admin_context()
+        self.assertRaises(test.TestingException,
+                          wrapped(bad_function_exception), 1, ctxt, 3, zoo=3)
+        self.assertFalse(mock_notification.called)
+
+    @mock.patch('nova.notifications.objects.exception.'
+                'ExceptionNotification.__init__')
+    def test_wrap_exception_notification_not_emitted_if_unversioned(
+            self, mock_notifier):
+        self.flags(notification_format='unversioned', group='notifications')
+
+        wrapped = exception_wrapper.wrap_exception(
+            service='compute', binary='nova-compute')
+        ctxt = nova_context.get_admin_context()
+        self.assertRaises(test.TestingException,
+                          wrapped(bad_function_exception), 1, ctxt, 3, zoo=3)
+        self.assertFalse(mock_notifier.called)

nova/tests/unit/test_fake_notifier.py

@@ -12,11 +12,8 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.
 
-import functools
-
 from nova import context
 from nova import exception_wrapper
-from nova import rpc
 from nova import test
 from nova.tests.unit import fake_notifier
 
@@ -30,10 +27,7 @@ class FakeVersionedNotifierTestCase(test.NoDBTestCase):
 
         self.context = context.RequestContext()
 
-    _get_notifier = functools.partial(rpc.get_notifier, 'compute')
+    @exception_wrapper.wrap_exception(service='compute', binary='nova-compute')
-
-    @exception_wrapper.wrap_exception(get_notifier=_get_notifier,
-                                      binary='nova-compute')
     def _raise_exception(self, context):
         raise test.TestingException
 

nova/tests/unit/test_rpc.py

@@ -297,18 +297,6 @@ class TestRPC(test.NoDBTestCase):
         mock_prep.return_value = 'notifier'
         mock_LEGACY_NOTIFIER.prepare = mock_prep
 
-        notifier = rpc.get_notifier('service', publisher_id='foo')
-
-        mock_prep.assert_called_once_with(publisher_id='foo')
-        self.assertIsInstance(notifier, rpc.LegacyValidatingNotifier)
-        self.assertEqual('notifier', notifier.notifier)
-
-    @mock.patch.object(rpc, 'LEGACY_NOTIFIER')
-    def test_get_notifier_null_publisher(self, mock_LEGACY_NOTIFIER):
-        mock_prep = mock.Mock()
-        mock_prep.return_value = 'notifier'
-        mock_LEGACY_NOTIFIER.prepare = mock_prep
-
         notifier = rpc.get_notifier('service', host='bar')
 
         mock_prep.assert_called_once_with(publisher_id='service.bar')