From 4d6e55f40ce88414480df01b1f59ba1ed758e7d4 Mon Sep 17 00:00:00 2001 From: Mark McLoughlin <markmc@redhat.com> Date: Mon, 16 Jun 2014 17:19:08 +0100 Subject: [PATCH] notifier: simply notifier_strategy compat support In order to handle the deprecated notifier_strategy configuration option we just need to translate the notifier_strategy value as follows: default -> transport_url=None, driver='noop' noop -> transport_url=None, driver='noop' logging -> transport_url=None, driver='log' rabbit -> transport_url=rabbit:///, driver='messaging' qpid -> transport_url=qpid:///, driver='messaging' None -> transport_url=None, driver=None where the last bit simply means that the user hasn't specified any notifier_strategy and we should just have oslo.messaging load a notifier based on oslo.messaging's config options. It really doesn't need to be any more complicated than that, and we certainly shouldn't need to poke at what the user may have configured for transport_url in their config file. The reason this is important is that CONF.transport_url is a private configuration option which soon will not be visible to users of the oslo.messaging API. See I9c3bf74f13a820ce295aa77a2bbcd781778b5926. Also, simplify the testing of this stuff by mocking the oslo.messaging APIs used and using the test scenarios outlined above. This also means we stop poking around in oslo.messaging internals like: self.assertEqual(str(nfier._transport._driver._url), 'qpid:///') Naughty, naughty! Note: it's arguable we should just remove notifier_strategy in Juno since we deprecated it in Icehouse, but this patch will be useful to backport to stable/icehouse in any case. Change-Id: I067248974baee50a2a0afb38bf022b65d1bea57a --- glance/notifier.py | 55 +++++++---------------- glance/tests/unit/test_notifier.py | 72 +++++++++++++++++------------- 2 files changed, 56 insertions(+), 71 deletions(-) diff --git a/glance/notifier.py b/glance/notifier.py index 78e414b998..ffa6ac4e0e 100644 --- a/glance/notifier.py +++ b/glance/notifier.py @@ -26,7 +26,7 @@ import glance.openstack.common.log as logging from glance.openstack.common import timeutils notifier_opts = [ - cfg.StrOpt('notifier_strategy', default='default', + cfg.StrOpt('notifier_strategy', help=_('Notifications can be sent when images are create, ' 'updated or deleted. There are three methods of sending ' 'notifications, logging (via the log_file directive), ' @@ -61,57 +61,32 @@ _ALIASES = { class Notifier(object): """Uses a notification strategy to send out messages about events.""" - def __init__(self, strategy=None): + def __init__(self): - _driver = None - _strategy = strategy + driver = None + transport_url = None + publisher_id = CONF.default_publisher_id - if CONF.notifier_strategy != 'default': + if CONF.notifier_strategy: msg = _("notifier_strategy was deprecated in " "favor of `notification_driver`") LOG.warn(msg) + strategy = CONF.notifier_strategy + # NOTE(flaper87): Use this to keep backwards # compatibility. We'll try to get an oslo.messaging # driver from the specified strategy. - _strategy = strategy or CONF.notifier_strategy - _driver = _STRATEGY_ALIASES.get(_strategy) + driver = _STRATEGY_ALIASES.get(strategy) + if driver == 'messaging': + transport_url = strategy + ':///' - publisher_id = CONF.default_publisher_id - - try: - # NOTE(flaper87): Assume the user has configured - # the transport url. - self._transport = messaging.get_transport(CONF, - aliases=_ALIASES) - except messaging.DriverLoadFailure: - # NOTE(flaper87): Catch driver load failures and re-raise - # them *just* if the `transport_url` option was set. This - # step is intended to keep backwards compatibility and avoid - # weird behaviors (like exceptions on missing dependencies) - # when the old notifier options are used. - if CONF.transport_url is not None: - with excutils.save_and_reraise_exception(): - LOG.exception(_('Error loading the notifier')) - - # NOTE(flaper87): This needs to be checked - # here because the `get_transport` call - # registers `transport_url` into ConfigOpts. - if not CONF.transport_url: - # NOTE(flaper87): The next 3 lines help - # with the migration to oslo.messaging. - # Without them, gate tests won't know - # what driver should be loaded. - # Once this patch lands, devstack will be - # updated and then these lines will be removed. - url = None - if _strategy in ['rabbit', 'qpid']: - url = _strategy + '://' - self._transport = messaging.get_transport(CONF, url, - aliases=_ALIASES) + self._transport = messaging.get_transport(CONF, + url=transport_url, + aliases=_ALIASES) self._notifier = messaging.Notifier(self._transport, - driver=_driver, + driver=driver, publisher_id=publisher_id) def warn(self, event_type, payload): diff --git a/glance/tests/unit/test_notifier.py b/glance/tests/unit/test_notifier.py index 5d79ccf46f..669b15c00a 100644 --- a/glance/tests/unit/test_notifier.py +++ b/glance/tests/unit/test_notifier.py @@ -17,6 +17,8 @@ import datetime import mock +from oslo.config import cfg +from oslo import messaging import webob from glance.common import exception @@ -94,42 +96,50 @@ class TaskRepoStub(object): class TestNotifier(utils.BaseTestCase): - def test_load_rabbit(self): - nfier = notifier.Notifier('rabbit') - self.assertIsNotNone(nfier._transport) - - def test_load_qpid(self): - nfier = notifier.Notifier('qpid') - self.assertIsNotNone(nfier._transport) - self.assertEqual(str(nfier._transport._driver._url), - 'qpid:///') - - def test_notifier_strategy(self): - self.config(notifier_strategy='qpid') + @mock.patch.object(messaging, 'Notifier') + @mock.patch.object(messaging, 'get_transport') + def _test_load_strategy(self, + mock_get_transport, mock_notifier, + strategy, url, driver): + if strategy is not None: + self.config(notifier_strategy=strategy) nfier = notifier.Notifier() + mock_get_transport.assert_called_with(cfg.CONF, url=url, + aliases=notifier._ALIASES) self.assertIsNotNone(nfier._transport) - self.assertEqual(str(nfier._transport._driver._url), - 'qpid:///') + mock_notifier.assert_called_with(nfier._transport, driver=driver, + publisher_id='image.localhost') + self.assertIsNotNone(nfier._notifier) - def test_transport_url(self): - transport_url = "qpid://superhost:5672/" - self.config(transport_url=transport_url) - notify = notifier.Notifier() - self.assertEqual(str(notify._transport._driver._url), - transport_url) + def test_notifier_strategy_default(self): + self._test_load_strategy(strategy='default', + url=None, + driver='noop') - def test_notification_driver_option(self): - self.config(rpc_backend='qpid') - self.config(notification_driver='messaging') - self.config(notifier_strategy='rabbit') - notify = notifier.Notifier() - self.assertEqual(str(notify._transport._driver._url), - 'rabbit:///') + def test_notifier_strategy_noop(self): + self._test_load_strategy(strategy='noop', + url=None, + driver='noop') - self.config(notifier_strategy='default') - notify = notifier.Notifier() - self.assertEqual(str(notify._transport._driver._url), - 'qpid:///') + def test_notifier_strategy_rabbit(self): + self._test_load_strategy(strategy='rabbit', + url='rabbit:///', + driver='messaging') + + def test_notifier_strategy_qpid(self): + self._test_load_strategy(strategy='qpid', + url='qpid:///', + driver='messaging') + + def test_notifier_strategy_logging(self): + self._test_load_strategy(strategy='logging', + url=None, + driver='log') + + def test_notifier_strategy_none(self): + self._test_load_strategy(strategy=None, + url=None, + driver=None) class TestImageNotifications(utils.BaseTestCase):