From 0d4c3cc65b78bafef69afc45cb156afe38f857c3 Mon Sep 17 00:00:00 2001 From: Balazs Gibizer Date: Mon, 18 Sep 2017 16:53:32 +0200 Subject: [PATCH] Remove dead code of api.fault notification sending Based on the description of the notify_on_api_faults config option [1] and the code that uses it [2] nova sends and api.fault notification if the nova-api service encounters an unhandle exception. There is a FaultWrapper class [3] added to the pipeline of the REST request which catches every exception and calls the notification sending. Based on some debugging in devstack this FaultWrapper never catches any exception. Every REST API method is decorated with expected_errors decorator [5] which as a last resort translate the unexpected exception to HTTPInternalServerError. In the wsgi stack the actual REST api call is guarded with ResourceExceptionHandler context manager [7] which translates HTTPException to a Fault [8]. Then Fault is catched and translated to the REST response [7]. This way the exception never propagates back to the FaultWrapper and therefore the api.fault notification is never emitted. Based on the git history of the expected_errors decorator this notification was never emitted for v2.1 API and as the v2.0 API now supported with the same codebase than v2.1 it is not emitted for v2.0 calls either. As nobody reported a bug I assume that nobody tried to use this notification for a very long time. Therefore instead of fixing this bug this patch propses to remove the dead code. See a bit more detailed description on the ML [9]. [1] https://github.com/openstack/nova/blob/0aeaa2bce8ad15bc2f28e00b30d688514b992e13/nova/conf/notifications.py#L49 [2] https://github.com/openstack/nova/blob/0aeaa2bce8ad15bc2f28e00b30d688514b992e13/nova/notifications/base.py#L84 [3] https://github.com/openstack/nova/blob/0aeaa2bce8ad15bc2f28e00b30d688514b992e13/nova/api/openstack/__init__.py#L78 [5] https://github.com/openstack/nova/blob/0aeaa2bce8ad15bc2f28e00b30d688514b992e13/nova/api/openstack/extensions.py#L325 [7] https://github.com/openstack/nova/blob/0aeaa2bce8ad15bc2f28e00b30d688514b992e13/nova/api/openstack/wsgi.py#L637 [8] https://github.com/openstack/nova/blob/0aeaa2bce8ad15bc2f28e00b30d688514b992e13/nova/api/openstack/wsgi.py#L418 [9] http://lists.openstack.org/pipermail/openstack-dev/2017-June/118639.html Change-Id: I608b6ebdc69d31eb2a11ac6479fa4f2e8c20f7d1 Closes-Bug: #1699115 --- doc/source/reference/notifications.rst | 2 +- nova/api/openstack/__init__.py | 2 - nova/conf/notifications.py | 10 -- nova/notifications/__init__.py | 1 - nova/notifications/base.py | 16 ---- nova/rpc.py | 1 - nova/tests/unit/test_notifications.py | 91 ------------------- ...notification-removal-9f3142ba7cb13ca9.yaml | 12 +++ 8 files changed, 13 insertions(+), 122 deletions(-) create mode 100644 releasenotes/notes/api.fault-notification-removal-9f3142ba7cb13ca9.yaml diff --git a/doc/source/reference/notifications.rst b/doc/source/reference/notifications.rst index 9924029d368c..9a3f2a9e04ac 100644 --- a/doc/source/reference/notifications.rst +++ b/doc/source/reference/notifications.rst @@ -41,7 +41,7 @@ Notifier object depends on the parameters of the get_notifier call and the value of the oslo.messaging configuration options `driver` and `topics`. There are notification configuration options in Nova which are specific for certain notification types like `notifications.notify_on_state_change`, -`notifications.notify_on_api_faults`, `notifications.default_level`, etc. +`notifications.default_level`, etc. The structure of the payload of the unversioned notifications is defined in the code that emits the notification and no documentation or enforced backward diff --git a/nova/api/openstack/__init__.py b/nova/api/openstack/__init__.py index 823609a01b1a..3443554015ff 100644 --- a/nova/api/openstack/__init__.py +++ b/nova/api/openstack/__init__.py @@ -26,7 +26,6 @@ import webob.exc from nova.api.openstack import wsgi import nova.conf from nova.i18n import translate -from nova import notifications from nova import utils from nova import wsgi as base_wsgi @@ -75,7 +74,6 @@ class FaultWrapper(base_wsgi.Middleware): outer.explanation = '%s: %s' % (inner.__class__.__name__, inner_msg) - notifications.send_api_fault(req.url, status, inner) return wsgi.Fault(outer) @webob.dec.wsgify(RequestClass=wsgi.Request) diff --git a/nova/conf/notifications.py b/nova/conf/notifications.py index 95e0d29ccc71..1afe8fc3e58c 100644 --- a/nova/conf/notifications.py +++ b/nova/conf/notifications.py @@ -43,16 +43,6 @@ Possible values: * None - no notifications * "vm_state" - notifications on VM state changes * "vm_and_task_state" - notifications on VM and task state changes -"""), - - cfg.BoolOpt( - 'notify_on_api_faults', - default=False, - deprecated_group='DEFAULT', - deprecated_name='notify_api_faults', - help=""" -If enabled, send api.fault notifications on caught exceptions in the -API service. """), cfg.StrOpt( diff --git a/nova/notifications/__init__.py b/nova/notifications/__init__.py index aee73704128b..ff27fc2457df 100644 --- a/nova/notifications/__init__.py +++ b/nova/notifications/__init__.py @@ -22,6 +22,5 @@ from nova.notifications.base import bandwidth_usage # noqa from nova.notifications.base import image_meta # noqa from nova.notifications.base import info_from_instance # noqa from nova.notifications.base import notify_decorator # noqa -from nova.notifications.base import send_api_fault # noqa from nova.notifications.base import send_update # noqa from nova.notifications.base import send_update_with_states # noqa diff --git a/nova/notifications/base.py b/nova/notifications/base.py index 77d10a0d22af..9b3ae2f773d7 100644 --- a/nova/notifications/base.py +++ b/nova/notifications/base.py @@ -24,7 +24,6 @@ from oslo_context import context as common_context from oslo_log import log from oslo_utils import excutils from oslo_utils import timeutils -import six import nova.conf import nova.context @@ -81,21 +80,6 @@ def notify_decorator(name, fn): return wrapped_func -def send_api_fault(url, status, exception): - """Send an api.fault notification.""" - - if not CONF.notifications.notify_on_api_faults: - return - - payload = {'url': url, 'exception': six.text_type(exception), - 'status': status} - - rpc.get_notifier('api').error(common_context.get_current() or - nova.context.get_admin_context(), - 'api.fault', - payload) - - def send_update(context, old_instance, new_instance, service="compute", host=None): """Send compute.instance.update notification to report any changes occurred diff --git a/nova/rpc.py b/nova/rpc.py index 969e2c229adb..87797b63b49c 100644 --- a/nova/rpc.py +++ b/nova/rpc.py @@ -260,7 +260,6 @@ class LegacyValidatingNotifier(object): 'aggregate.updatemetadata.start', 'aggregate.updateprop.end', 'aggregate.updateprop.start', - 'api.fault', 'compute.instance.create.end', 'compute.instance.create.error', 'compute.instance.create_ip.end', diff --git a/nova/tests/unit/test_notifications.py b/nova/tests/unit/test_notifications.py index 483ed8657876..f994f692de45 100644 --- a/nova/tests/unit/test_notifications.py +++ b/nova/tests/unit/test_notifications.py @@ -99,97 +99,6 @@ class NotificationsTestCase(test.TestCase): inst.create() return inst - def test_send_api_fault_disabled(self): - self.flags(notify_on_api_faults=False, group='notifications') - notifications.send_api_fault("http://example.com/foo", 500, None) - self.assertEqual(0, len(fake_notifier.NOTIFICATIONS)) - - def test_send_api_fault(self): - self.flags(notify_on_api_faults=True, group='notifications') - exception = None - try: - # Get a real exception with a call stack. - raise test.TestingException("junk") - except test.TestingException as e: - exception = e - - notifications.send_api_fault("http://example.com/foo", 500, exception) - - self.assertEqual(1, len(fake_notifier.NOTIFICATIONS)) - n = fake_notifier.NOTIFICATIONS[0] - self.assertEqual(n.priority, 'ERROR') - self.assertEqual(n.event_type, 'api.fault') - self.assertEqual(n.payload['url'], 'http://example.com/foo') - self.assertEqual(n.payload['status'], 500) - self.assertIsNotNone(n.payload['exception']) - - def test_send_api_fault_fresh_context(self): - self.flags(notify_on_api_faults=True, group='notifications') - exception = None - try: - # Get a real exception with a call stack. - raise test.TestingException("junk") - except test.TestingException as e: - exception = e - - ctxt = context.RequestContext(overwrite=True) - notifications.send_api_fault("http://example.com/foo", 500, exception) - - self.assertEqual(1, len(fake_notifier.NOTIFICATIONS)) - n = fake_notifier.NOTIFICATIONS[0] - self.assertEqual(n.priority, 'ERROR') - self.assertEqual(n.event_type, 'api.fault') - self.assertEqual(n.payload['url'], 'http://example.com/foo') - self.assertEqual(n.payload['status'], 500) - self.assertIsNotNone(n.payload['exception']) - self.assertEqual(ctxt, n.context) - - def test_send_api_fault_fake_context(self): - self.flags(notify_on_api_faults=True, group='notifications') - exception = None - try: - # Get a real exception with a call stack. - raise test.TestingException("junk") - except test.TestingException as e: - exception = e - - ctxt = o_context.get_current() - self.assertIsNotNone(ctxt) - notifications.send_api_fault("http://example.com/foo", 500, exception) - - self.assertEqual(1, len(fake_notifier.NOTIFICATIONS)) - n = fake_notifier.NOTIFICATIONS[0] - self.assertEqual(n.priority, 'ERROR') - self.assertEqual(n.event_type, 'api.fault') - self.assertEqual(n.payload['url'], 'http://example.com/foo') - self.assertEqual(n.payload['status'], 500) - self.assertIsNotNone(n.payload['exception']) - self.assertIsNotNone(n.context) - self.assertEqual(ctxt, n.context) - - def test_send_api_fault_admin_context(self): - self.flags(notify_on_api_faults=True, group='notifications') - exception = None - try: - # Get a real exception with a call stack. - raise test.TestingException("junk") - except test.TestingException as e: - exception = e - - self.fixture._remove_cached_context() - self.assertIsNone(o_context.get_current()) - notifications.send_api_fault("http://example.com/foo", 500, exception) - - self.assertEqual(1, len(fake_notifier.NOTIFICATIONS)) - n = fake_notifier.NOTIFICATIONS[0] - self.assertEqual(n.priority, 'ERROR') - self.assertEqual(n.event_type, 'api.fault') - self.assertEqual(n.payload['url'], 'http://example.com/foo') - self.assertEqual(n.payload['status'], 500) - self.assertIsNotNone(n.payload['exception']) - self.assertIsNotNone(n.context) - self.assertTrue(n.context.is_admin) - def test_notif_disabled(self): # test config disable of the notifications diff --git a/releasenotes/notes/api.fault-notification-removal-9f3142ba7cb13ca9.yaml b/releasenotes/notes/api.fault-notification-removal-9f3142ba7cb13ca9.yaml new file mode 100644 index 000000000000..b6777a1d718e --- /dev/null +++ b/releasenotes/notes/api.fault-notification-removal-9f3142ba7cb13ca9.yaml @@ -0,0 +1,12 @@ +--- +upgrade: + - | + The ``notify_on_api_faults`` config option and the ``api.fault`` + notification it enabled have been removed. As noted in `bug 1699115`_, + the ``api.fault`` notification has not worked since the v2.1 API was + introduced. As the v2.0 API is supported with the v2.1 codebase since + Newton, this notification has not been emitted since Newton. Given that + no one has reported an issue with this in that time, it is simply + removed. + + .. _`bug 1699115`: https://bugs.launchpad.net/nova/+bug/1699115