Merge lp:~tim-simpson/nova/multiple-notifiers into lp:~hudson-openstack/nova/trunk

[Tim Simpson]

The notifiers API was changed to take a list of notifiers. Some people might want to use more than one notifier so hopefully this will be accepted into trunk.

[Matt Dietz (cerberus)]

Tim, the code looks good. However, can you give me a hypothetical use case where someone would need to do more than one style of notification at a time? I'm not necessarily opposed to the idea, but I would like to minimize the burden of notifications on Nova if at all possible. That's why we're writing an application external to nova to aggregate and distribute notifications in a granular way.

review: Needs Fixing

[Matt Dietz (cerberus)]

Err

review: Needs Information

[Jason Kölker (jason-koelker)]

I would think that for this functionality, you would just write a fan_out notifier.

[Tim Simpson (tim-simpson)]

Hi Matt,

My case is that I want to write all notifications to the log files as well as do something more substantial in response to them. For project Red Dwarf (Database as a Service) we'll be needing to know if the compute manager fails during run_instance in order to free up additional resources that we are provisioning. Since we've forked Nova we could simply hack up the compute manager but would prefer to respond to the notifications to keep our changes easy to separate.

Jason,

I think you're right. My first reaction to the idea of a "fan_out" notifier is I almost can't see why someone would only use one.

However, thinking about it keeping only one "driver" per manager is consistent with the rest of Nova. When the external notifier application is created then only the driver for it will be needed in most situations and having a list will get in the way.

I'll go ahead and update this as you suggested.

Thank you both for the feedback.

[Sandy Walsh (sandy-walsh)]

Myself and Dragon discussed this a while back (specifically around retries for failed provisioning attempts and orchestration). I proposed the fanout solution, but Dragon talked me into the multiple notifiers scheme. I think it's a better solution, less to clean up if something fails.

Needs a proper test though. Specifically around one driver failing and the others continuing to work.

review: Needs Fixing

[Tim Simpson (tim-simpson)]

I pushed up a new version which instead of forcing the notifier.api to keep a list of drivers has a driver called "list_notifier" that can call a list of other drivers.

I thought calling it "fan_out_notifier" might infer additional qualities it lacked, so I went with a simpler name. I can rename it if anyone thinks the term "fan out" is a better fit.

Sandy this code has a test case for when one driver fails (it asserts an exception is logged) and another for when they all work.

[Sandy Walsh (sandy-walsh)]

Looks good ... minor nits

+34 single _ for private functions

+38 needs some error handling. If the import fails it'll bring down the service. It should simply not load that driver and log an error.

review: Needs Fixing

[Sandy Walsh (sandy-walsh)]

oh, and the tests should reflect the bad import case

[Sandy Walsh (sandy-walsh)]

oh, and you need to update the AUTHORS file

http://paste.openstack.org/show/2234/

review: Needs Fixing

[Tim Simpson (tim-simpson)]

Thanks Sandy.

When the notification driver can't be loaded I load a class which remembers the raised exception and raises it every time notify is called. So if a notification driver fails to load you see mention of it in the logs each time notify is called. The alternative was to log something the first time and never again. If people want to do this instead I'll go ahead and update the code.

I also changed the authors file and removed the extra underscore from the private function.

[Sandy Walsh (sandy-walsh)]

Hmm, you know what, I was thinking about this more last night. I don't think terminating the service is the right thing to do.

The reason being is we don't see the failure until a notification is sent. It's not like a failure on startup, it could happen any any random time in the future.

So, sadly, seems the right thing to do is to log the event, skip the failing import and press on.

review: Needs Fixing

[lp:~tim-simpson/nova/multiple-notifiers]

1441. By Tim Simpson on 2011-08-23

Switched list_notifier to log an exception each time notify is called, for each notification driver that failed to import.

1442. By Tim Simpson on 2011-08-23

Merged from upstream.

[Tim Simpson (tim-simpson)]

Yeah, killing it when the notification could happen later seems bad too.
I've changed the code back to logging an exception for drivers which couldn't be found.

[Sandy Walsh (sandy-walsh)]

looks good ... just some pep8 issues

http://paste.openstack.org/show/2258/

review: Needs Fixing

[lp:~tim-simpson/nova/multiple-notifiers]

1443. By Tim Simpson on 2011-08-23

Fixed some pep8 and pylint issues.

1444. By Tim Simpson on 2011-08-23

Merged from upstream.

[Sandy Walsh (sandy-walsh)]

Others might complain about ImportFailureNotifier being too noisy and blowing out the log files, but generally looks good to me. Nice work Tim!

review: Approve

[Matt Dietz (cerberus)]

Tim,

    That's actually *exactly* the reason I moved that kind of functionality external to Nova. I believe we should be focused on developing the minimum amount of functionality inside of Nova because anything else interferes with the core competency of virtualization.
    I'm afraid things like this ending up in the code open up the floodgate for people to create insanely complex notification systems. I also wanted Nova to have an "indifferent" attitude to notifying end users. I believe this allows Nova to send notification as quickly as possible in as non-specific a way as I could muster.
    With that said, I'm not actually worried so much about *this* particular patch. Because this is more of a philosophical argument than one pertaining to the functionality around this patch, I'm going to abstain. I'll have Dragon take a look.

review: Abstain

[Thierry Carrez (ttx)]

This should wait for Essex ?

review: Disapprove

[Matt Dietz (cerberus)]

ttx: ahh, indeed. Wasn't thinking of timelines here.

[Monsyne Dragon (mdragon)]

Now that this is changed to be a driver, it looks good to me. The import error logging is a bit... vocal, but I suppose that might be a good thing.

review: Approve

[Tim Simpson (tim-simpson)]

> This should wait for Essex ?
Is the motivation for this just the deadline? As its an additional driver, at the very least we know it won't break notifications, and the tests seem to prove it will work for anyone who needs it.

[Thierry Carrez (ttx)]

Sounds self-contained indeed, and doesn't change existing behavior, so go for it !

review: Approve (ffe)

=== modified file 'Authors'
--- Authors	2011-08-23 04:17:57 +0000
+++ Authors	2011-08-23 20:52:26 +0000
@@ -100,6 +100,7 @@
 Soren Hansen <[e-mail]>
 Stephanie Reese <[e-mail]>
 Thierry Carrez <[e-mail]>
+Tim Simpson <[e-mail]>
 Todd Willey <[e-mail]>
 Trey Morris <[e-mail]>
 Troy Toman <[e-mail]>

=== added file 'nova/notifier/list_notifier.py'
--- nova/notifier/list_notifier.py	1970-01-01 00:00:00 +0000
+++ nova/notifier/list_notifier.py	2011-08-23 20:52:26 +0000
@@ -0,0 +1,68 @@
+# Copyright 2011 OpenStack LLC.
+# 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.
+
+from nova import flags
+from nova import log as logging
+from nova import utils
+from nova.exception import ClassNotFound
+
+flags.DEFINE_multistring('list_notifier_drivers',
+                         ['nova.notifier.no_op_notifier'],
+                         'List of drivers to send notifications')
+
+FLAGS = flags.FLAGS
+
+LOG = logging.getLogger('nova.notifier.list_notifier')
+
+drivers = None
+
+
+class ImportFailureNotifier(object):
+    """Noisily re-raises some exception over-and-over when notify is called."""
+
+    def __init__(self, exception):
+        self.exception = exception
+
+    def notify(self, message):
+        raise self.exception
+
+
+def _get_drivers():
+    """Instantiates and returns drivers based on the flag values."""
+    global drivers
+    if not drivers:
+        drivers = []
+        for notification_driver in FLAGS.list_notifier_drivers:
+            try:
+                drivers.append(utils.import_object(notification_driver))
+            except ClassNotFound as e:
+                drivers.append(ImportFailureNotifier(e))
+    return drivers
+
+
+def notify(message):
+    """Passes notification to mulitple notifiers in a list."""
+    for driver in _get_drivers():
+        try:
+            driver.notify(message)
+        except Exception as e:
+            LOG.exception(_("Problem '%(e)s' attempting to send to "
+                            "notification driver %(driver)s." % locals()))
+
+
+def _reset_drivers():
+    """Used by unit tests to reset the drivers."""
+    global drivers
+    drivers = None

=== added directory 'nova/tests/notifier'
=== added file 'nova/tests/notifier/__init__.py'
--- nova/tests/notifier/__init__.py	1970-01-01 00:00:00 +0000
+++ nova/tests/notifier/__init__.py	2011-08-23 20:52:26 +0000
@@ -0,0 +1,16 @@
+# Copyright 2011 Openstack LLC.
+# 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.
+
+from nova.tests import *

=== added file 'nova/tests/notifier/test_list_notifier.py'
--- nova/tests/notifier/test_list_notifier.py	1970-01-01 00:00:00 +0000
+++ nova/tests/notifier/test_list_notifier.py	2011-08-23 20:52:26 +0000
@@ -0,0 +1,88 @@
+# Copyright 2011 OpenStack LLC.
+# 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 stubout
+import sys
+
+import nova
+from nova import log as logging
+import nova.notifier.api
+from nova.notifier.api import notify
+from nova.notifier import log_notifier
+from nova.notifier import no_op_notifier
+from nova.notifier import list_notifier
+from nova import test
+
+
+class NotifierListTestCase(test.TestCase):
+    """Test case for notifications"""
+
+    def setUp(self):
+        super(NotifierListTestCase, self).setUp()
+        list_notifier._reset_drivers()
+        self.stubs = stubout.StubOutForTesting()
+        # Mock log to add one to exception_count when log.exception is called
+
+        def mock_exception(cls, *args):
+            self.exception_count += 1
+
+        self.exception_count = 0
+        list_notifier_log = logging.getLogger('nova.notifier.list_notifier')
+        self.stubs.Set(list_notifier_log, "exception", mock_exception)
+        # Mock no_op notifier to add one to notify_count when called.
+
+        def mock_notify(cls, *args):
+            self.notify_count += 1
+
+        self.notify_count = 0
+        self.stubs.Set(nova.notifier.no_op_notifier, 'notify', mock_notify)
+        # Mock log_notifier to raise RuntimeError when called.
+
+        def mock_notify2(cls, *args):
+            raise RuntimeError("Bad notifier.")
+
+        self.stubs.Set(nova.notifier.log_notifier, 'notify', mock_notify2)
+
+    def tearDown(self):
+        self.stubs.UnsetAll()
+        list_notifier._reset_drivers()
+        super(NotifierListTestCase, self).tearDown()
+
+    def test_send_notifications_successfully(self):
+        self.flags(notification_driver='nova.notifier.list_notifier',
+                   list_notifier_drivers=['nova.notifier.no_op_notifier',
+                                          'nova.notifier.no_op_notifier'])
+        notify('publisher_id', 'event_type',
+                nova.notifier.api.WARN, dict(a=3))
+        self.assertEqual(self.notify_count, 2)
+        self.assertEqual(self.exception_count, 0)
+
+    def test_send_notifications_with_errors(self):
+
+        self.flags(notification_driver='nova.notifier.list_notifier',
+                   list_notifier_drivers=['nova.notifier.no_op_notifier',
+                                          'nova.notifier.log_notifier'])
+        notify('publisher_id', 'event_type', nova.notifier.api.WARN, dict(a=3))
+        self.assertEqual(self.notify_count, 1)
+        self.assertEqual(self.exception_count, 1)
+
+    def test_when_driver_fails_to_import(self):
+        self.flags(notification_driver='nova.notifier.list_notifier',
+                   list_notifier_drivers=['nova.notifier.no_op_notifier',
+                                          'nova.notifier.logo_notifier',
+                                          'fdsjgsdfhjkhgsfkj'])
+        notify('publisher_id', 'event_type', nova.notifier.api.WARN, dict(a=3))
+        self.assertEqual(self.exception_count, 2)
+        self.assertEqual(self.notify_count, 1)