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

Proposed by Tim Simpson
Status: Merged
Approved by: Sandy Walsh
Approved revision: 1444
Merged at revision: 1490
Proposed branch: lp:~tim-simpson/nova/multiple-notifiers
Merge into: lp:~hudson-openstack/nova/trunk
Diff against target: 199 lines (+173/-0)
4 files modified
Authors (+1/-0)
nova/notifier/list_notifier.py (+68/-0)
nova/tests/notifier/__init__.py (+16/-0)
nova/tests/notifier/test_list_notifier.py (+88/-0)
To merge this branch: bzr merge lp:~tim-simpson/nova/multiple-notifiers
Reviewer Review Type Date Requested Status
Thierry Carrez (community) ffe Approve
Monsyne Dragon (community) Approve
Matt Dietz (community) Abstain
Sandy Walsh (community) Approve
Review via email: mp+71370@code.launchpad.net

Description of the change

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.

To post a comment you must log in.
Revision history for this message
Matt Dietz (cerberus) wrote :

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
Revision history for this message
Matt Dietz (cerberus) wrote :

Err

review: Needs Information
Revision history for this message
Jason Kölker (jason-koelker) wrote :

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

Revision history for this message
Tim Simpson (tim-simpson) wrote :

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.

Revision history for this message
Sandy Walsh (sandy-walsh) wrote :

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
Revision history for this message
Tim Simpson (tim-simpson) wrote :

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.

Revision history for this message
Sandy Walsh (sandy-walsh) wrote :

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
Revision history for this message
Sandy Walsh (sandy-walsh) wrote :

oh, and the tests should reflect the bad import case

Revision history for this message
Sandy Walsh (sandy-walsh) wrote :

oh, and you need to update the AUTHORS file

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

review: Needs Fixing
Revision history for this message
Tim Simpson (tim-simpson) wrote :

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.

Revision history for this message
Sandy Walsh (sandy-walsh) wrote :

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
1441. By Tim Simpson

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

1442. By Tim Simpson

Merged from upstream.

Revision history for this message
Tim Simpson (tim-simpson) wrote :

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.

Revision history for this message
Sandy Walsh (sandy-walsh) wrote :

looks good ... just some pep8 issues

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

review: Needs Fixing
1443. By Tim Simpson

Fixed some pep8 and pylint issues.

1444. By Tim Simpson

Merged from upstream.

Revision history for this message
Sandy Walsh (sandy-walsh) wrote :

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
Revision history for this message
Matt Dietz (cerberus) wrote :

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
Revision history for this message
Thierry Carrez (ttx) wrote :

This should wait for Essex ?

review: Disapprove
Revision history for this message
Matt Dietz (cerberus) wrote :

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

Revision history for this message
Monsyne Dragon (mdragon) wrote :

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
Revision history for this message
Tim Simpson (tim-simpson) wrote :

> 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.

Revision history for this message
Thierry Carrez (ttx) wrote :

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

review: Approve (ffe)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'Authors'
--- Authors 2011-08-23 04:17:57 +0000
+++ Authors 2011-08-23 20:52:26 +0000
@@ -100,6 +100,7 @@
100Soren Hansen <soren.hansen@rackspace.com>100Soren Hansen <soren.hansen@rackspace.com>
101Stephanie Reese <reese.sm@gmail.com>101Stephanie Reese <reese.sm@gmail.com>
102Thierry Carrez <thierry@openstack.org>102Thierry Carrez <thierry@openstack.org>
103Tim Simpson <tim.simpson@rackspace.com>
103Todd Willey <todd@ansolabs.com>104Todd Willey <todd@ansolabs.com>
104Trey Morris <trey.morris@rackspace.com>105Trey Morris <trey.morris@rackspace.com>
105Troy Toman <troy.toman@rackspace.com>106Troy Toman <troy.toman@rackspace.com>
106107
=== 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 @@
1# Copyright 2011 OpenStack LLC.
2# All Rights Reserved.
3#
4# Licensed under the Apache License, Version 2.0 (the "License"); you may
5# not use this file except in compliance with the License. You may obtain
6# a copy of the License at
7#
8# http://www.apache.org/licenses/LICENSE-2.0
9#
10# Unless required by applicable law or agreed to in writing, software
11# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
12# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
13# License for the specific language governing permissions and limitations
14# under the License.
15
16from nova import flags
17from nova import log as logging
18from nova import utils
19from nova.exception import ClassNotFound
20
21flags.DEFINE_multistring('list_notifier_drivers',
22 ['nova.notifier.no_op_notifier'],
23 'List of drivers to send notifications')
24
25FLAGS = flags.FLAGS
26
27LOG = logging.getLogger('nova.notifier.list_notifier')
28
29drivers = None
30
31
32class ImportFailureNotifier(object):
33 """Noisily re-raises some exception over-and-over when notify is called."""
34
35 def __init__(self, exception):
36 self.exception = exception
37
38 def notify(self, message):
39 raise self.exception
40
41
42def _get_drivers():
43 """Instantiates and returns drivers based on the flag values."""
44 global drivers
45 if not drivers:
46 drivers = []
47 for notification_driver in FLAGS.list_notifier_drivers:
48 try:
49 drivers.append(utils.import_object(notification_driver))
50 except ClassNotFound as e:
51 drivers.append(ImportFailureNotifier(e))
52 return drivers
53
54
55def notify(message):
56 """Passes notification to mulitple notifiers in a list."""
57 for driver in _get_drivers():
58 try:
59 driver.notify(message)
60 except Exception as e:
61 LOG.exception(_("Problem '%(e)s' attempting to send to "
62 "notification driver %(driver)s." % locals()))
63
64
65def _reset_drivers():
66 """Used by unit tests to reset the drivers."""
67 global drivers
68 drivers = None
069
=== 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 @@
1# Copyright 2011 Openstack LLC.
2# All Rights Reserved.
3#
4# Licensed under the Apache License, Version 2.0 (the "License"); you may
5# not use this file except in compliance with the License. You may obtain
6# a copy of the License at
7#
8# http://www.apache.org/licenses/LICENSE-2.0
9#
10# Unless required by applicable law or agreed to in writing, software
11# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
12# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
13# License for the specific language governing permissions and limitations
14# under the License.
15
16from nova.tests import *
017
=== 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 @@
1# Copyright 2011 OpenStack LLC.
2# All Rights Reserved.
3#
4# Licensed under the Apache License, Version 2.0 (the "License"); you may
5# not use this file except in compliance with the License. You may obtain
6# a copy of the License at
7#
8# http://www.apache.org/licenses/LICENSE-2.0
9#
10# Unless required by applicable law or agreed to in writing, software
11# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
12# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
13# License for the specific language governing permissions and limitations
14# under the License.
15
16import stubout
17import sys
18
19import nova
20from nova import log as logging
21import nova.notifier.api
22from nova.notifier.api import notify
23from nova.notifier import log_notifier
24from nova.notifier import no_op_notifier
25from nova.notifier import list_notifier
26from nova import test
27
28
29class NotifierListTestCase(test.TestCase):
30 """Test case for notifications"""
31
32 def setUp(self):
33 super(NotifierListTestCase, self).setUp()
34 list_notifier._reset_drivers()
35 self.stubs = stubout.StubOutForTesting()
36 # Mock log to add one to exception_count when log.exception is called
37
38 def mock_exception(cls, *args):
39 self.exception_count += 1
40
41 self.exception_count = 0
42 list_notifier_log = logging.getLogger('nova.notifier.list_notifier')
43 self.stubs.Set(list_notifier_log, "exception", mock_exception)
44 # Mock no_op notifier to add one to notify_count when called.
45
46 def mock_notify(cls, *args):
47 self.notify_count += 1
48
49 self.notify_count = 0
50 self.stubs.Set(nova.notifier.no_op_notifier, 'notify', mock_notify)
51 # Mock log_notifier to raise RuntimeError when called.
52
53 def mock_notify2(cls, *args):
54 raise RuntimeError("Bad notifier.")
55
56 self.stubs.Set(nova.notifier.log_notifier, 'notify', mock_notify2)
57
58 def tearDown(self):
59 self.stubs.UnsetAll()
60 list_notifier._reset_drivers()
61 super(NotifierListTestCase, self).tearDown()
62
63 def test_send_notifications_successfully(self):
64 self.flags(notification_driver='nova.notifier.list_notifier',
65 list_notifier_drivers=['nova.notifier.no_op_notifier',
66 'nova.notifier.no_op_notifier'])
67 notify('publisher_id', 'event_type',
68 nova.notifier.api.WARN, dict(a=3))
69 self.assertEqual(self.notify_count, 2)
70 self.assertEqual(self.exception_count, 0)
71
72 def test_send_notifications_with_errors(self):
73
74 self.flags(notification_driver='nova.notifier.list_notifier',
75 list_notifier_drivers=['nova.notifier.no_op_notifier',
76 'nova.notifier.log_notifier'])
77 notify('publisher_id', 'event_type', nova.notifier.api.WARN, dict(a=3))
78 self.assertEqual(self.notify_count, 1)
79 self.assertEqual(self.exception_count, 1)
80
81 def test_when_driver_fails_to_import(self):
82 self.flags(notification_driver='nova.notifier.list_notifier',
83 list_notifier_drivers=['nova.notifier.no_op_notifier',
84 'nova.notifier.logo_notifier',
85 'fdsjgsdfhjkhgsfkj'])
86 notify('publisher_id', 'event_type', nova.notifier.api.WARN, dict(a=3))
87 self.assertEqual(self.exception_count, 2)
88 self.assertEqual(self.notify_count, 1)