Merge lp:~cerberus/nova/nova_notifications into lp:~hudson-openstack/nova/trunk

[Matt Dietz]

Implements a basic mechanism for pushing notifications out to interested parties. The rationale for implementing notifications this way is that the responsibility for them shouldn't fall to Nova. As such, we simply will be pushing messages to a queue where another worker entirely can be written to push messages around to subscribers.

[Jay Pipes (jaypipes)]

Looks reasonable as a starting point for a watch system. One request, though, could you elaborate in the docstrings what the "model" is expected to be? Mapping? Object? A string?

review: Needs Information

[Matt Dietz (cerberus)]

Absolutely Jay. I'll get on that.

[Vish Ishaya (vishvananda)]

FWIW I really hate importing things in __init__.py because it leads to strange import chains where things get imported when you didn't expect them to. Is there a reason to put this call here instead of for example in notify/api.py?

Vish

On Apr 22, 2011, at 8:41 AM, Matt Dietz wrote:

> Matt Dietz has proposed merging lp:~cerberus/nova/nova_notifications into lp:nova.
>
> Requested reviews:
> Nova Core (nova-core)
>
> For more details, see:
> https://code.launchpad.net/~cerberus/nova/nova_notifications/+merge/58825
>
> Implements a basic mechanism for pushing notifications out to interested parties. The rationale for implementing notifications this way is that the responsibility for them shouldn't fall to Nova. As such, we simply will be pushing messages to a queue where another worker entirely can be written to push messages around to subscribers.
> --
> https://code.launchpad.net/~cerberus/nova/nova_notifications/+merge/58825
> You are subscribed to branch lp:nova.
> === modified file 'nova/flags.py'
> --- nova/flags.py 2011-04-21 23:01:38 +0000
> +++ nova/flags.py 2011-04-22 15:41:24 +0000
> @@ -369,6 +369,9 @@
> DEFINE_string('node_availability_zone', 'nova',
> 'availability zone of this node')
>
> +DEFINE_string('notification_driver',
> + 'nova.notifier.no_op_notifier.NoopNotifier',
> + 'Default driver for sending notifications')
> DEFINE_string('zone_name', 'nova', 'name of this zone')
> DEFINE_list('zone_capabilities',
> ['hypervisor=xenserver;kvm', 'os=linux;windows'],
>
> === added directory 'nova/notifier'
> === added file 'nova/notifier/__init__.py'
> --- nova/notifier/__init__.py 1970-01-01 00:00:00 +0000
> +++ nova/notifier/__init__.py 2011-04-22 15:41:24 +0000
> @@ -0,0 +1,24 @@
> +# 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 utils
> +
> +FLAGS = flags.FLAGS
> +
> +def notify(event_name, model):
> + """Sends a notification using the specified driver"""
> + driver = utils.import_class(FLAGS.notification_driver)()
> + driver.notify(event_name, model)
>
> === added file 'nova/notifier/no_op_notifier.py'
> --- nova/notifier/no_op_notifier.py 1970-01-01 00:00:00 +0000
> +++ nova/notifier/no_op_notifier.py 2011-04-22 15:41:24 +0000
> @@ -0,0 +1,19 @@
> +# 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...

[Brian Waldon (bcwaldon)]

Looking good.

review: Approve

[Matt Dietz (cerberus)]

I'm planning to revamp this branch with feedback obtained at the summit. Changes forthcoming...

[Jay Pipes (jaypipes)]

Nice improvements, Matt!

Little nits:

145 + """ log notifications using nova's default logging system """

Please remove the spaces before and after enclosing """

107 + raise BadPriorityException('%s not in valid priorities' % priority)

Need i18n in the string there...

Other than those tiny things, looks excellent.

Cheers,
jay

review: Needs Fixing

[Brian Waldon (bcwaldon)]

147-154, 179, 217-224: Can we use "message" instead of "payload"? The naming is a little confusing.

Would you mind documenting the notifier drivers a bit better through a base class?

review: Needs Fixing

[Matt Dietz (cerberus)]

Thanks for the feedback, guys.

Jay: I think I've cleaned up what you suggested.

Brian: I made the message changes

I'm a little confused by the request to implement a base class here. Each notifier only has a single argument "notify" method(but the self reference), and that's all there would be to document amongst them. The functionality after that point is entirely different. Beyond that, I feel like the api.notify method is the real one to watch.

Regarding the idea of base classes, I'm opposed to the practice of implementing interfaces in Python. Given the "duck typing" nature of Python, it seems superfluous. If there was methods or member variables to be shared amongst them, it would of course be different.

I've noticed the practice in Nova is starting to lean the way of implementing interfaces, but the argument that's been made is that it's intended to catch missing functionality from driver classes. However, the right answer, in my opinion, is to ensure you've written tests to that implicit interface.

[Brian Waldon (bcwaldon)]

> Brian: I made the message changes
>
> I'm a little confused by the request to implement a base class here. Each
> notifier only has a single argument "notify" method(but the self reference),
> and that's all there would be to document amongst them. The functionality
> after that point is entirely different. Beyond that, I feel like the
> api.notify method is the real one to watch.
>
> Regarding the idea of base classes, I'm opposed to the practice of
> implementing interfaces in Python. Given the "duck typing" nature of Python,
> it seems superfluous. If there was methods or member variables to be shared
> amongst them, it would of course be different.
>
> I've noticed the practice in Nova is starting to lean the way of implementing
> interfaces, but the argument that's been made is that it's intended to catch
> missing functionality from driver classes. However, the right answer, in my
> opinion, is to ensure you've written tests to that implicit interface.

You're definitely right, and the reason I suggested it was more to help with documentation rather than enforcing interfaces. It took me longer than it should have to wrap my head around the system as a whole, and I thought it would help others get up to speed quicker. It really isn't a big deal, just an idea I wanted to run by you. I am ready to approve once the following are addressed:

78: you don't seem to use 'event_name', 'message' should be 'payload'

84: this line doesn't belong

90: 'message' should be 'payload'

92: remove 'payload'

103: 'message' should be 'payload'

114: 'message' should be 'payload'

review: Needs Fixing

[Rick Harris (rconradharris)]

Looks great.

Not really a nit, just an observation:

> 215 +class RabbitNotifier(object):
> 216 + """Sends notifications to a specific RabbitMQ server and topic"""
> 217 +
> 218 + def notify(self, message):

It looks like the Notifier /class/ is superflous since we define a single stateless function inside of it and we don't derive the implementations from a base-class.

Should we just make each driver's `notify` proc a function which is called by `api.notify`?

As a side benefit, we get to avoid the cost of instantiating an object for each notification sent (of which there will be a gazillion), as well as the small penalty of later having to gc them.

review: Approve

[Matt Dietz (cerberus)]

Good catches Brian. No clue how I messed that up!

Rick: Very good point. Let me rework this a bit...

[Matt Dietz (cerberus)]

84: this line doesn't belong

Rewrote the comment to better explain what I meant by including that.

Think I covered the rest. Also made changes based on Rick's suggestion

[Brian Waldon (bcwaldon)]

> Think I covered the rest. Also made changes based on Rick's suggestion

78, 88, 118: I still feel like 'message' on these lines should be 'payload'. It's confusing to give the notify function a 'message' which is encapsulated as the 'payload' in a new type of 'message' structure.

[lp:~cerberus/nova/nova_notifications]

773. By Matt Dietz on 2011-05-16

Conceded :-D

[Jay Pipes (jaypipes)]

lgtm.

review: Approve

[Rick Harris (rconradharris)]

Brian:

Looks like patch has been updated to use 'payload', if it looks good, minding throwing an 'Approve' on there?

[Brian Waldon (bcwaldon)]

Thanks for the work, Matt. Sorry to be so picky.

review: Approve

[Chris Behrens (cbehrens)]

Looks pretty awesome.

Minor nit that I notice in the tests:

test_send_notification uses self.notify_called when you don't need to set this in the instance. Ie, you can just do:

notify_called = False

def mock_notify(cls, *args):
    notify_called = True

Similar thing in the other tests. self.mock_cast and self.test_topic.

[Josh Kearney (jk0)]

Per HACKING, I would stick an extra newline after #59, #145 and #208.

[lp:~cerberus/nova/nova_notifications]

774. By Matt Dietz on 2011-05-18

Merge from trunk

775. By Matt Dietz on 2011-05-18

Spacing changes

[Matt Dietz (cerberus)]

Chris: that actually doesn't work because of the way Python handles scoping in assignments. Your way would create variables scope local to the mock method only, so there would be no way of checking them later in the test.

Josh: Spacing changes made.

[Josh Kearney (jk0)]

LGTM!

review: Approve

[Rick Harris (rconradharris)]

> Your way would create variables scope local to the mock method only, so there would be no way of checking them later in the test.

A bit of a hack, but, one way around this is to create a locally-scoped dict which the mocked function can mutate.

monitored = dict(notified=False)
def fake_notify():
    monitored['notified'] = True

self.assertTrue(monitored['notified])

(* Neutral on whether it should be used in this case *)

=== modified file 'nova/flags.py'
--- nova/flags.py	2011-04-29 22:06:18 +0000
+++ nova/flags.py	2011-05-18 17:56:56 +0000
@@ -369,6 +369,9 @@
 DEFINE_string('node_availability_zone', 'nova',
               'availability zone of this node')
 
+DEFINE_string('notification_driver',
+              'nova.notifier.no_op_notifier',
+              'Default driver for sending notifications')
 DEFINE_list('memcached_servers', None,
             'Memcached servers or None for in process cache.')
 

=== added directory 'nova/notifier'
=== added file 'nova/notifier/__init__.py'
--- nova/notifier/__init__.py	1970-01-01 00:00:00 +0000
+++ nova/notifier/__init__.py	2011-05-18 17:56:56 +0000
@@ -0,0 +1,14 @@
+# 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.

=== added file 'nova/notifier/api.py'
--- nova/notifier/api.py	1970-01-01 00:00:00 +0000
+++ nova/notifier/api.py	2011-05-18 17:56:56 +0000
@@ -0,0 +1,83 @@
+# 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 datetime
+
+import datetime
+import uuid
+
+from nova import flags
+from nova import utils
+
+
+FLAGS = flags.FLAGS
+
+flags.DEFINE_string('default_notification_level', 'INFO',
+                    'Default notification level for outgoing notifications')
+
+WARN = 'WARN'
+INFO = 'INFO'
+ERROR = 'ERROR'
+CRITICAL = 'CRITICAL'
+DEBUG = 'DEBUG'
+
+log_levels = (DEBUG, WARN, INFO, ERROR, CRITICAL)
+
+
+class BadPriorityException(Exception):
+    pass
+
+
+def notify(publisher_id, event_type, priority, payload):
+    """
+    Sends a notification using the specified driver
+
+    Notify parameters:
+
+    publisher_id - the source worker_type.host of the message
+    event_type - the literal type of event (ex. Instance Creation)
+    priority - patterned after the enumeration of Python logging levels in
+               the set (DEBUG, WARN, INFO, ERROR, CRITICAL)
+    payload - A python dictionary of attributes
+
+    Outgoing message format includes the above parameters, and appends the
+    following:
+
+    message_id - a UUID representing the id for this notification
+    timestamp - the GMT timestamp the notification was sent at
+
+    The composite message will be constructed as a dictionary of the above
+    attributes, which will then be sent via the transport mechanism defined
+    by the driver.
+
+    Message example:
+
+    {'message_id': str(uuid.uuid4()),
+     'publisher_id': 'compute.host1',
+     'timestamp': datetime.datetime.utcnow(),
+     'priority': 'WARN',
+     'event_type': 'compute.create_instance',
+     'payload': {'instance_id': 12, ... }}
+
+    """
+    if priority not in log_levels:
+        raise BadPriorityException(
+                 _('%s not in valid priorities' % priority))
+    driver = utils.import_object(FLAGS.notification_driver)
+    msg = dict(message_id=str(uuid.uuid4()),
+                   publisher_id=publisher_id,
+                   event_type=event_type,
+                   priority=priority,
+                   payload=payload,
+                   timestamp=str(datetime.datetime.utcnow()))
+    driver.notify(msg)

=== added file 'nova/notifier/log_notifier.py'
--- nova/notifier/log_notifier.py	1970-01-01 00:00:00 +0000
+++ nova/notifier/log_notifier.py	2011-05-18 17:56:56 +0000
@@ -0,0 +1,34 @@
+# 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 json
+
+from nova import flags
+from nova import log as logging
+
+
+FLAGS = flags.FLAGS
+
+
+def notify(message):
+    """Notifies the recipient of the desired event given the model.
+    Log notifications using nova's default logging system"""
+
+    priority = message.get('priority',
+                           FLAGS.default_notification_level)
+    priority = priority.lower()
+    logger = logging.getLogger(
+            'nova.notification.%s' % message['event_type'])
+    getattr(logger, priority)(json.dumps(message))

=== added file 'nova/notifier/no_op_notifier.py'
--- nova/notifier/no_op_notifier.py	1970-01-01 00:00:00 +0000
+++ nova/notifier/no_op_notifier.py	2011-05-18 17:56:56 +0000
@@ -0,0 +1,19 @@
+# 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.
+
+
+def notify(message):
+    """Notifies the recipient of the desired event given the model"""
+    pass

=== added file 'nova/notifier/rabbit_notifier.py'
--- nova/notifier/rabbit_notifier.py	1970-01-01 00:00:00 +0000
+++ nova/notifier/rabbit_notifier.py	2011-05-18 17:56:56 +0000
@@ -0,0 +1,36 @@
+# 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 nova.context
+
+from nova import flags
+from nova import rpc
+
+
+FLAGS = flags.FLAGS
+
+flags.DEFINE_string('notification_topic', 'notifications',
+                    'RabbitMQ topic used for Nova notifications')
+
+
+def notify(message):
+    """Sends a notification to the RabbitMQ"""
+    context = nova.context.get_admin_context()
+    priority = message.get('priority',
+                           FLAGS.default_notification_level)
+    priority = priority.lower()
+    topic = '%s.%s' % (FLAGS.notification_topic, priority)
+    rpc.cast(context, topic, message)

=== added file 'nova/tests/test_notifier.py'
--- nova/tests/test_notifier.py	1970-01-01 00:00:00 +0000
+++ nova/tests/test_notifier.py	2011-05-18 17:56:56 +0000
@@ -0,0 +1,117 @@
+# 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 nova
+
+from nova import context
+from nova import flags
+from nova import rpc
+import nova.notifier.api
+from nova.notifier.api import notify
+from nova.notifier import no_op_notifier
+from nova.notifier import rabbit_notifier
+from nova import test
+
+import stubout
+
+
+class NotifierTestCase(test.TestCase):
+    """Test case for notifications"""
+    def setUp(self):
+        super(NotifierTestCase, self).setUp()
+        self.stubs = stubout.StubOutForTesting()
+
+    def tearDown(self):
+        self.stubs.UnsetAll()
+        super(NotifierTestCase, self).tearDown()
+
+    def test_send_notification(self):
+        self.notify_called = False
+
+        def mock_notify(cls, *args):
+            self.notify_called = True
+
+        self.stubs.Set(nova.notifier.no_op_notifier, 'notify',
+                mock_notify)
+
+        class Mock(object):
+            pass
+        notify('publisher_id', 'event_type',
+                nova.notifier.api.WARN, dict(a=3))
+        self.assertEqual(self.notify_called, True)
+
+    def test_verify_message_format(self):
+        """A test to ensure changing the message format is prohibitively
+        annoying"""
+
+        def message_assert(message):
+            fields = [('publisher_id', 'publisher_id'),
+                      ('event_type', 'event_type'),
+                      ('priority', 'WARN'),
+                      ('payload', dict(a=3))]
+            for k, v in fields:
+                self.assertEqual(message[k], v)
+            self.assertTrue(len(message['message_id']) > 0)
+            self.assertTrue(len(message['timestamp']) > 0)
+
+        self.stubs.Set(nova.notifier.no_op_notifier, 'notify',
+                message_assert)
+        notify('publisher_id', 'event_type',
+                nova.notifier.api.WARN, dict(a=3))
+
+    def test_send_rabbit_notification(self):
+        self.stubs.Set(nova.flags.FLAGS, 'notification_driver',
+                'nova.notifier.rabbit_notifier')
+        self.mock_cast = False
+
+        def mock_cast(cls, *args):
+            self.mock_cast = True
+
+        class Mock(object):
+            pass
+
+        self.stubs.Set(nova.rpc, 'cast', mock_cast)
+        notify('publisher_id', 'event_type',
+                nova.notifier.api.WARN, dict(a=3))
+
+        self.assertEqual(self.mock_cast, True)
+
+    def test_invalid_priority(self):
+        def mock_cast(cls, *args):
+            pass
+
+        class Mock(object):
+            pass
+
+        self.stubs.Set(nova.rpc, 'cast', mock_cast)
+        self.assertRaises(nova.notifier.api.BadPriorityException,
+                notify, 'publisher_id',
+                'event_type', 'not a priority', dict(a=3))
+
+    def test_rabbit_priority_queue(self):
+        self.stubs.Set(nova.flags.FLAGS, 'notification_driver',
+                'nova.notifier.rabbit_notifier')
+        self.stubs.Set(nova.flags.FLAGS, 'notification_topic',
+                'testnotify')
+
+        self.test_topic = None
+
+        def mock_cast(context, topic, msg):
+            self.test_topic = topic
+
+        self.stubs.Set(nova.rpc, 'cast', mock_cast)
+        notify('publisher_id',
+                'event_type', 'DEBUG', dict(a=3))
+        self.assertEqual(self.test_topic, 'testnotify.debug')