From e7171fd74a683a1118c03506ef1600333193abb5 Mon Sep 17 00:00:00 2001 From: Gorka Eguileor Date: Wed, 1 Feb 2017 12:17:39 +0100 Subject: [PATCH] Fix notification short-circuit We recently introduced a mechanism to short-circuit notifications when they were disabled, unfortunately the mechanism was not backward compatible with deprecated oslo notification setup that defaults to use the transport mechanism of RPC. In Mitaka oslo messaging introduced the transport_url specific for notifications under the oslo_messaging_notifications section, but some projects still use the default transport_url defined in the DEFAULT section. This patch fixes the notification short-circuit, now we don't check the transport_url but the driver option in the oslo_messaging_notifications section, which is a more robust way of checking, since it is backward compatible. Change-Id: Iccf51756701759e3727d3d989bab08bc05cac9a7 Closes-Bug: #1660928 --- cinder/rpc.py | 2 +- cinder/test.py | 12 +++++++++--- cinder/tests/unit/test_rpc.py | 8 ++++++-- cinder/tests/unit/test_utils.py | 6 ++++-- cinder/tests/unit/utils.py | 11 ----------- cinder/utils.py | 8 +++++++- 6 files changed, 27 insertions(+), 20 deletions(-) diff --git a/cinder/rpc.py b/cinder/rpc.py index b59d2323479..d58ca1d344e 100644 --- a/cinder/rpc.py +++ b/cinder/rpc.py @@ -77,7 +77,7 @@ def init(conf): # get_notification_transport has loaded oslo_messaging_notifications config # group, so we can now check if notifications are actually enabled. - if conf.oslo_messaging_notifications.transport_url: + if utils.notifications_enabled(conf): json_serializer = messaging.JsonPayloadSerializer() serializer = RequestContextSerializer(json_serializer) NOTIFIER = messaging.Notifier(NOTIFICATION_TRANSPORT, diff --git a/cinder/test.py b/cinder/test.py index eb98350a91c..03b4a9e0cec 100644 --- a/cinder/test.py +++ b/cinder/test.py @@ -31,6 +31,7 @@ from oslo_concurrency import lockutils from oslo_config import cfg from oslo_config import fixture as config_fixture from oslo_log.fixture import logging_error as log_fixture +import oslo_messaging from oslo_messaging import conffixture as messaging_conffixture from oslo_serialization import jsonutils from oslo_utils import strutils @@ -51,7 +52,6 @@ from cinder import service from cinder.tests import fixtures as cinder_fixtures from cinder.tests.unit import conf_fixture from cinder.tests.unit import fake_notifier -from cinder.tests.unit import utils as test_utils from cinder.volume import utils @@ -154,6 +154,14 @@ class TestCase(testtools.TestCase): self.messaging_conf.transport_driver = 'fake' self.messaging_conf.response_timeout = 15 self.useFixture(self.messaging_conf) + + # Load oslo_messaging_notifications config group so we can set an + # override to prevent notifications from being ignored due to the + # short-circuit mechanism. + oslo_messaging.get_notification_transport(CONF) + # We need to use a valid driver for the notifications, so we use test. + self.override_config('driver', ['test'], + group='oslo_messaging_notifications') rpc.init(CONF) # NOTE(geguileo): This is required because _determine_obj_version_cap @@ -234,8 +242,6 @@ class TestCase(testtools.TestCase): coordination.COORDINATOR.start() self.addCleanup(coordination.COORDINATOR.stop) - test_utils.set_normal_rpc_notifier(self) - def _restore_obj_registry(self): objects_base.CinderObjectRegistry._registry._obj_classes = \ self._base_test_obj_backup diff --git a/cinder/tests/unit/test_rpc.py b/cinder/tests/unit/test_rpc.py index 39ef198f9ac..87c4f4a0c01 100644 --- a/cinder/tests/unit/test_rpc.py +++ b/cinder/tests/unit/test_rpc.py @@ -12,6 +12,7 @@ # License for the specific language governing permissions and limitations # under the License. +import ddt import mock from cinder.objects import base @@ -25,6 +26,7 @@ class FakeAPI(rpc.RPCAPI): BINARY = 'cinder-scheduler' +@ddt.ddt class RPCAPITestCase(test.TestCase): """Tests RPCAPI mixin aggregating stuff related to RPC compatibility.""" @@ -84,9 +86,11 @@ class RPCAPITestCase(test.TestCase): self.assertFalse(get_min_obj.called) self.assertFalse(get_min_rpc.called) + @ddt.data([], ['noop'], ['noop', 'noop']) @mock.patch('oslo_messaging.JsonPayloadSerializer', wraps=True) - def test_init_no_notifications(self, serializer_mock): - self.override_config('transport_url', '', + def test_init_no_notifications(self, driver, serializer_mock): + """Test short-circuiting notifications with default and noop driver.""" + self.override_config('driver', driver, group='oslo_messaging_notifications') rpc.init(test.CONF) self.assertEqual(rpc.utils.DO_NOTHING, rpc.NOTIFIER) diff --git a/cinder/tests/unit/test_utils.py b/cinder/tests/unit/test_utils.py index 788f1e77a91..3740089ff0c 100644 --- a/cinder/tests/unit/test_utils.py +++ b/cinder/tests/unit/test_utils.py @@ -1416,6 +1416,7 @@ class TestValidateInteger(test.TestCase): value, 'limit', min_value=-1, max_value=(2 ** 31)) +@ddt.ddt class TestNotificationShortCircuit(test.TestCase): def test_do_nothing_getter(self): """Test any attribute will always return the same instance (self).""" @@ -1441,9 +1442,10 @@ class TestNotificationShortCircuit(test.TestCase): result = self._decorated_method() self.assertEqual(mock.sentinel.success, result) - def test_if_notification_enabled_when_disabled(self): + @ddt.data([], ['noop'], ['noop', 'noop']) + def test_if_notification_enabled_when_disabled(self, driver): """Test method is not called when notifications are disabled.""" - self.override_config('transport_url', '', + self.override_config('driver', driver, group='oslo_messaging_notifications') result = self._decorated_method() self.assertEqual(utils.DO_NOTHING, result) diff --git a/cinder/tests/unit/utils.py b/cinder/tests/unit/utils.py index b96cfd76aa3..b7560717868 100644 --- a/cinder/tests/unit/utils.py +++ b/cinder/tests/unit/utils.py @@ -28,7 +28,6 @@ from cinder import db from cinder import exception from cinder import objects from cinder.objects import fields -from cinder import rpc from cinder.tests.unit import fake_constants as fake @@ -504,13 +503,3 @@ def create_populated_cluster(ctxt, num_services, num_down_svcs=0, **values): for i in range(num_services) ] return cluster, svcs - - -def set_normal_rpc_notifier(test_case): - """Instead of shortcircuiting notifications, user Oslo's notifier.""" - test_case.override_config('transport_url', 'fake_transport', - group='oslo_messaging_notifications') - json_serializer = rpc.messaging.JsonPayloadSerializer() - serializer = rpc.RequestContextSerializer(json_serializer) - rpc.NOTIFIER = rpc.messaging.Notifier(rpc.NOTIFICATION_TRANSPORT, - serializer=serializer) diff --git a/cinder/utils.py b/cinder/utils.py index 2d79f0b10f3..8ef6f2467d5 100644 --- a/cinder/utils.py +++ b/cinder/utils.py @@ -1089,11 +1089,17 @@ class DoNothing(str): DO_NOTHING = DoNothing() +def notifications_enabled(conf): + """Check if oslo notifications are enabled.""" + notifications_driver = set(conf.oslo_messaging_notifications.driver) + return notifications_driver and notifications_driver != {'noop'} + + def if_notifications_enabled(f): """Calls decorated method only if notifications are enabled.""" @functools.wraps(f) def wrapped(*args, **kwargs): - if CONF.oslo_messaging_notifications.transport_url: + if notifications_enabled(CONF): return f(*args, **kwargs) return DO_NOTHING return wrapped