launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #06837
[Merge] lp:~jtv/maas/missing-rabbit into lp:maas
Jeroen T. Vermeulen has proposed merging lp:~jtv/maas/missing-rabbit into lp:maas.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~jtv/maas/missing-rabbit/+merge/98781
MAAS currently oopses when it can't connect to RabbitMQ. That's bad.
This branch makes the test suite pass when rabbit is not present:
* Introduces a new NoRabbit exception for “no Rabbit found running.”
* Disables longpoll when no rabbit is found.
* Logs warning, but does not break, when model changes can't be published on rabbit.
* Otherwise, still breaks as before (and as expected) when rabbit runs amok.
Any tests that actually require rabbit fire up their own instance, so the test suite passes unchanged. Plus of course I added some new tests for the new behaviours.
In the rabbit tests, I made the use of Rabbit explicit, and demoted the test-case-with-rabbit base class from a TestCase (it didn't contain any tests itself and clearly wasn't meant to) to a helper mix-in.
Note that RabbitSession.connection looks like a property, but it makes network connections so it can take oodles of time and fail in any number of ways. This is very bad style, but I left it in place because Doris is feeling a little cold this morning. It also means that I need to test failures with ExpectedException instead of a simple self.assertRaises: I can't pass session.connection to assertRaises without evaluating it.
Jeroen
--
https://code.launchpad.net/~jtv/maas/missing-rabbit/+merge/98781
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/maas/missing-rabbit into lp:maas.
=== modified file 'src/maasserver/exceptions.py'
--- src/maasserver/exceptions.py 2012-03-19 06:04:57 +0000
+++ src/maasserver/exceptions.py 2012-03-22 05:31:18 +0000
@@ -34,6 +34,10 @@
"""System profile does not exist."""
+class NoRabbit(MAASException):
+ """Could not reach RabbitMQ."""
+
+
class MAASAPIException(Exception):
"""Base class for MAAS' API exceptions.
=== modified file 'src/maasserver/messages.py'
--- src/maasserver/messages.py 2012-03-15 13:58:32 +0000
+++ src/maasserver/messages.py 2012-03-22 05:31:18 +0000
@@ -20,6 +20,7 @@
ABCMeta,
abstractmethod,
)
+from logging import getLogger
from django.conf import settings
from django.core.serializers.json import DjangoJSONEncoder
@@ -27,6 +28,7 @@
post_delete,
post_save,
)
+from maasserver.exceptions import NoRabbit
from maasserver.models import Node
from maasserver.rabbit import RabbitMessaging
@@ -62,16 +64,26 @@
def create_msg(self, event_name, instance):
"""Format a message from the given event_name and instance."""
+ def publish_message(self, message):
+ """Attempt to publish `message` on the producer.
+
+ If RabbitMQ is not available, log an error message but return
+ normally.
+ """
+ try:
+ self.producer.publish(message)
+ except NoRabbit as e:
+ getLogger('maasserver').warn("Could not reach RabbitMQ: %s", e)
+
def update_obj(self, sender, instance, created, **kwargs):
event_name = (
MESSENGER_EVENT.CREATED if created
else MESSENGER_EVENT.UPDATED)
- message = self.create_msg(event_name, instance)
- self.producer.publish(message)
+ self.publish_message(self.create_msg(event_name, instance))
def delete_obj(self, sender, instance, **kwargs):
message = self.create_msg(MESSENGER_EVENT.DELETED, instance)
- self.producer.publish(message)
+ self.publish_message(message)
def register(self):
post_save.connect(
=== modified file 'src/maasserver/rabbit.py'
--- src/maasserver/rabbit.py 2012-03-13 11:43:09 +0000
+++ src/maasserver/rabbit.py 2012-03-22 05:31:18 +0000
@@ -17,20 +17,29 @@
]
+from errno import ECONNREFUSED
+import socket
import threading
from amqplib import client_0_8 as amqp
from django.conf import settings
+from maasserver.exceptions import NoRabbit
def connect():
"""Connect to AMQP."""
- return amqp.Connection(
- host=settings.RABBITMQ_HOST,
- userid=settings.RABBITMQ_USERID,
- password=settings.RABBITMQ_PASSWORD,
- virtual_host=settings.RABBITMQ_VIRTUAL_HOST,
- insist=False)
+ try:
+ return amqp.Connection(
+ host=settings.RABBITMQ_HOST,
+ userid=settings.RABBITMQ_USERID,
+ password=settings.RABBITMQ_PASSWORD,
+ virtual_host=settings.RABBITMQ_VIRTUAL_HOST,
+ insist=False)
+ except socket.error as e:
+ if e.errno == ECONNREFUSED:
+ raise NoRabbit(e.message)
+ else:
+ raise
class RabbitSession(threading.local):
=== modified file 'src/maasserver/tests/test_messages.py'
--- src/maasserver/tests/test_messages.py 2012-03-15 13:58:32 +0000
+++ src/maasserver/tests/test_messages.py 2012-03-22 05:31:18 +0000
@@ -12,7 +12,9 @@
__all__ = []
import json
+import socket
+from maasserver.exceptions import NoRabbit
from maasserver.messages import (
MAASMessenger,
MESSENGER_EVENT,
@@ -97,6 +99,41 @@
self.assertEqual(
[[MESSENGER_EVENT.DELETED, obj]], producer.messages)
+ def test_publish_message_publishes_message(self):
+ event = factory.getRandomString()
+ instance = {factory.getRandomString(): factory.getRandomString()}
+ messenger = TestMessenger(MessagesTestModel, FakeProducer())
+ messenger.publish_message(messenger.create_msg(event, instance))
+ self.assertEqual([[event, instance]], messenger.producer.messages)
+
+ def test_publish_message_swallows_missing_rabbit(self):
+ event = factory.getRandomString()
+ instance = {factory.getRandomString(): factory.getRandomString()}
+
+ def fail_for_lack_of_rabbit(*args, **kwargs):
+ raise NoRabbit("I'm pretending not to have a RabbitMQ.")
+
+ messenger = TestMessenger(MessagesTestModel, FakeProducer())
+ messenger.producer.publish = fail_for_lack_of_rabbit
+
+ messenger.publish_message(messenger.create_msg(event, instance))
+ self.assertEqual([], messenger.producer.messages)
+
+ def test_publish_message_propagates_exceptions(self):
+ event = factory.getRandomString()
+ instance = {factory.getRandomString(): factory.getRandomString()}
+
+ def fail_despite_having_a_rabbit(*args, **kwargs):
+ raise socket.error("I have a rabbit but I fail anyway.")
+
+ messenger = TestMessenger(MessagesTestModel, FakeProducer())
+ messenger.producer.publish = fail_despite_having_a_rabbit
+
+ self.assertRaises(
+ socket.error,
+ messenger.publish_message, messenger.create_msg(event, instance))
+ self.assertEqual([], messenger.producer.messages)
+
class MAASMessengerTest(TestModelTestCase):
=== modified file 'src/maasserver/tests/test_rabbit.py'
--- src/maasserver/tests/test_rabbit.py 2012-03-15 14:32:25 +0000
+++ src/maasserver/tests/test_rabbit.py 2012-03-22 05:31:18 +0000
@@ -12,10 +12,13 @@
__all__ = []
+import socket
import time
from amqplib import client_0_8 as amqp
+from django.conf import settings
from fixtures import MonkeyPatch
+from maasserver.exceptions import NoRabbit
from maasserver.rabbit import (
RabbitBase,
RabbitExchange,
@@ -26,12 +29,12 @@
from maasserver.testing.factory import factory
from maastesting.testcase import TestCase
from rabbitfixture.server import RabbitServer
-
-
-class RabbitTestCase(TestCase):
-
- def setUp(self):
- super(RabbitTestCase, self).setUp()
+from testtools.testcase import ExpectedException
+
+
+class RabbitTestBase:
+
+ def use_rabbit(self):
self.rabbit_server = self.useFixture(RabbitServer())
self.rabbit_env = self.rabbit_server.runner.environment
patch = MonkeyPatch(
@@ -43,9 +46,10 @@
return self.rabbit_env.rabbitctl(str(command))[0]
-class TestRabbitSession(RabbitTestCase):
+class TestRabbitSession(TestCase, RabbitTestBase):
- def test_session_connection(self):
+ def test_connection_gets_connection(self):
+ self.use_rabbit()
session = RabbitSession()
# Referencing the connection property causes a connection to be
# created.
@@ -54,15 +58,35 @@
# The same connection is returned every time.
self.assertIs(connection, session.connection)
- def test_session_disconnect(self):
+ def test_connection_raises_NoRabbit_if_cannot_connect(self):
+ # Attempt to connect to a RabbitMQ on the local "discard"
+ # service. The connection will be refused.
+ self.patch(settings, 'RABBITMQ_HOST', 'localhost:9')
+ session = RabbitSession()
+ with ExpectedException(NoRabbit):
+ session.connection
+
+ def test_connection_propagates_exceptions(self):
+
+ def fail(*args, **kwargs):
+ raise socket.error("Connection not refused, but failed anyway.")
+
+ self.patch(amqp, 'Connection', fail)
+ session = RabbitSession()
+ with ExpectedException(socket.error):
+ session.connection
+
+ def test_disconnect(self):
+ self.use_rabbit()
session = RabbitSession()
session.disconnect()
self.assertIsNone(session._connection)
-class TestRabbitMessaging(RabbitTestCase):
+class TestRabbitMessaging(TestCase, RabbitTestBase):
def test_messaging_getExchange(self):
+ self.use_rabbit()
exchange_name = factory.getRandomString()
messaging = RabbitMessaging(exchange_name)
exchange = messaging.getExchange()
@@ -71,6 +95,7 @@
self.assertEqual(exchange_name, exchange.exchange_name)
def test_messaging_getQueue(self):
+ self.use_rabbit()
exchange_name = factory.getRandomString()
messaging = RabbitMessaging(exchange_name)
queue = messaging.getQueue()
@@ -79,19 +104,22 @@
self.assertEqual(exchange_name, queue.exchange_name)
-class TestRabbitBase(RabbitTestCase):
+class TestRabbitBase(TestCase, RabbitTestBase):
def test_rabbitbase_contains_session(self):
+ self.use_rabbit()
exchange_name = factory.getRandomString()
rabbitbase = RabbitBase(RabbitSession(), exchange_name)
self.assertIsInstance(rabbitbase._session, RabbitSession)
def test_base_has_exchange_name(self):
+ self.use_rabbit()
exchange_name = factory.getRandomString()
rabbitbase = RabbitBase(RabbitSession(), exchange_name)
self.assertEqual(exchange_name, rabbitbase.exchange_name)
def test_base_channel(self):
+ self.use_rabbit()
rabbitbase = RabbitBase(RabbitSession(), factory.getRandomString())
# Referencing the channel property causes an open channel to be
# created.
@@ -102,6 +130,7 @@
self.assertIs(channel, rabbitbase.channel)
def test_base_channel_creates_exchange(self):
+ self.use_rabbit()
exchange_name = factory.getRandomString()
rabbitbase = RabbitBase(RabbitSession(), exchange_name)
rabbitbase.channel
@@ -110,7 +139,7 @@
self.get_command_output('list_exchanges'))
-class TestRabbitExchange(RabbitTestCase):
+class TestRabbitExchange(TestCase, RabbitTestBase):
def basic_get(self, channel, queue_name, timeout):
endtime = time.time() + timeout
@@ -124,6 +153,7 @@
return message
def test_exchange_publish(self):
+ self.use_rabbit()
exchange_name = factory.getRandomString()
message_content = factory.getRandomString()
exchange = RabbitExchange(RabbitSession(), exchange_name)
@@ -136,9 +166,10 @@
self.assertEqual(message_content, message.body)
-class TestRabbitQueue(RabbitTestCase):
+class TestRabbitQueue(TestCase, RabbitTestBase):
def test_rabbit_queue_binds_queue(self):
+ self.use_rabbit()
exchange_name = factory.getRandomString()
message_content = factory.getRandomString()
queue = RabbitQueue(RabbitSession(), exchange_name)
=== modified file 'src/maasserver/tests/test_views.py'
--- src/maasserver/tests/test_views.py 2012-03-22 04:29:00 +0000
+++ src/maasserver/tests/test_views.py 2012-03-22 05:31:18 +0000
@@ -21,8 +21,11 @@
from django.contrib.auth.models import User
from django.core.urlresolvers import reverse
from lxml.html import fromstring
-from maasserver import views
-from maasserver.messages import get_messaging
+from maasserver import (
+ messages,
+ views,
+ )
+from maasserver.exceptions import NoRabbit
from maasserver.models import (
Config,
NODE_AFTER_COMMISSIONING_ACTION,
@@ -248,12 +251,23 @@
def test_get_longpoll_context_empty_if_rabbitmq_publish_is_none(self):
self.patch(settings, 'RABBITMQ_PUBLISH', None)
- self.patch(views, 'messaging', get_messaging())
+ self.patch(views, 'messaging', messages.get_messaging())
+ self.assertEqual({}, get_longpoll_context())
+
+ def test_get_longpoll_context_returns_empty_if_rabbit_not_running(self):
+
+ class FakeMessaging:
+ """Fake :class:`RabbitMessaging`: fail with `NoRabbit`."""
+
+ def getQueue(self, *args, **kwargs):
+ raise NoRabbit("Pretending not to have a rabbit.")
+
+ self.patch(messages, 'messaging', FakeMessaging())
self.assertEqual({}, get_longpoll_context())
def test_get_longpoll_context_empty_if_longpoll_url_is_None(self):
self.patch(settings, 'LONGPOLL_PATH', None)
- self.patch(views, 'messaging', get_messaging())
+ self.patch(views, 'messaging', messages.get_messaging())
self.assertEqual({}, get_longpoll_context())
@uses_rabbit_fixture
@@ -261,7 +275,7 @@
longpoll = factory.getRandomString()
self.patch(settings, 'LONGPOLL_PATH', longpoll)
self.patch(settings, 'RABBITMQ_PUBLISH', True)
- self.patch(views, 'messaging', get_messaging())
+ self.patch(views, 'messaging', messages.get_messaging())
context = get_longpoll_context()
self.assertItemsEqual(
['LONGPOLL_PATH', 'longpoll_queue'], list(context))
=== modified file 'src/maasserver/views.py'
--- src/maasserver/views.py 2012-03-22 04:29:00 +0000
+++ src/maasserver/views.py 2012-03-22 05:31:18 +0000
@@ -16,6 +16,7 @@
"NodeView",
]
+from logging import getLogger
import mimetypes
import os
import urllib2
@@ -52,7 +53,10 @@
ListView,
UpdateView,
)
-from maasserver.exceptions import CannotDeleteUserException
+from maasserver.exceptions import (
+ CannotDeleteUserException,
+ NoRabbit,
+ )
from maasserver.forms import (
AddArchiveForm,
CommissioningForm,
@@ -96,10 +100,15 @@
def get_longpoll_context():
if messaging is not None and django_settings.LONGPOLL_PATH is not None:
- return {
- 'longpoll_queue': messaging.getQueue().name,
- 'LONGPOLL_PATH': django_settings.LONGPOLL_PATH,
- }
+ try:
+ return {
+ 'longpoll_queue': messaging.getQueue().name,
+ 'LONGPOLL_PATH': django_settings.LONGPOLL_PATH,
+ }
+ except NoRabbit as e:
+ getLogger('maasserver').warn(
+ "Could not connect to RabbitMQ: %s", e)
+ return {}
else:
return {}
Follow ups
