Merge lp:~gmb/maas/rpc-error-handling-middleware into lp:~maas-committers/maas/trunk

[Graham Binns]

Add middleware to handle errors from RPC connections.

At present, "handling" means "display an error message to the user". There may be a Better Way but this is a good start.

I've tested this on a local test rig with VMs, so I'm reasonably confident that it'll work fine. I'm not sure whether we should *also* be logging the errors to maas-django.log; that seems sane but there's already a lot of cruft in there.

[Gavin Panella (allenap)]

Looks good, but I think the handling of MultipleFailures needs changing.

review: Needs Fixing

[Gavin Panella (allenap)]

Bootiful.

review: Approve

=== modified file 'src/maas/settings.py'
--- src/maas/settings.py	2014-09-24 14:54:56 +0000
+++ src/maas/settings.py	2014-09-29 21:37:53 +0000
@@ -233,6 +233,7 @@
     'maasserver.middleware.ErrorsMiddleware',
     'maasserver.middleware.APIErrorsMiddleware',
     'maasserver.middleware.ExternalComponentsMiddleware',
+    'maasserver.middleware.RPCErrorsMiddleware',
     'metadataserver.middleware.MetadataErrorsMiddleware',
     'django.middleware.transaction.TransactionMiddleware',
     'django.middleware.clickjacking.XFrameOptionsMiddleware',

=== modified file 'src/maasserver/middleware.py'
--- src/maasserver/middleware.py	2014-09-02 16:20:36 +0000
+++ src/maasserver/middleware.py	2014-09-29 21:37:53 +0000
@@ -43,6 +43,11 @@
     HttpResponseForbidden,
     HttpResponseRedirect,
     )
+from provisioningserver.rpc.exceptions import (
+    MultipleFailures,
+    NoConnectionsAvailable,
+    PowerActionAlreadyInProgress,
+    )
 
 
 try:
@@ -260,3 +265,39 @@
                     self.log_level,
                     "%s\n%s", header, decoded_content)
         return response  # Return response unaltered.
+
+
+class RPCErrorsMiddleware:
+    """A middleware for handling RPC errors."""
+
+    handled_exceptions = (
+        NoConnectionsAvailable,
+        PowerActionAlreadyInProgress,
+        )
+
+    def _handle_exception(self, request, exception):
+        logging.exception(exception)
+        error_message = unicode(exception)
+        if len(error_message) == 0:
+            # Make a pretty message for exceptions without a message.
+            error_message = (
+                "Unexpected exception: %s. See "
+                "/var/log/maas/maas-django.log "
+                "on the region server for more information." %
+                exception.__class__.__name__)
+        messages.error(request, error_message)
+
+    def process_exception(self, request, exception):
+        if isinstance(exception, MultipleFailures):
+            exceptions = [
+                failure.value for failure in exception.args]
+            for exception in exceptions:
+                self._handle_exception(request, exception)
+            return HttpResponseRedirect(request.path)
+        elif isinstance(exception, self.handled_exceptions):
+            self._handle_exception(request, exception)
+            return HttpResponseRedirect(request.path)
+        else:
+            # Nothing to do, since we don't care about anything other
+            # than MultipleFailures and handled_exception.
+            return None

=== modified file 'src/maasserver/tests/test_middleware.py'
--- src/maasserver/tests/test_middleware.py	2014-09-02 16:12:03 +0000
+++ src/maasserver/tests/test_middleware.py	2014-09-29 21:37:53 +0000
@@ -17,6 +17,7 @@
 import httplib
 import json
 import logging
+import random
 
 from django.contrib.messages import constants
 from django.core.exceptions import (
@@ -37,15 +38,22 @@
     DebuggingLoggerMiddleware,
     ErrorsMiddleware,
     ExceptionMiddleware,
+    RPCErrorsMiddleware,
     )
 from maasserver.testing import extract_redirect
 from maasserver.testing.factory import factory
 from maasserver.testing.testcase import MAASServerTestCase
 from maastesting.utils import sample_binary_data
+from provisioningserver.rpc.exceptions import (
+    MultipleFailures,
+    NoConnectionsAvailable,
+    PowerActionAlreadyInProgress,
+    )
 from testtools.matchers import (
     Contains,
     Not,
     )
+from twisted.python.failure import Failure
 
 
 class ExceptionMiddlewareTest(MAASServerTestCase):
@@ -247,3 +255,98 @@
         # An error message has been published.
         self.assertEqual(
             [(constants.ERROR, error_message, '')], request._messages.messages)
+
+
+class RPCErrorsMiddlewareTest(MAASServerTestCase):
+
+    def test_handles_PowerActionAlreadyInProgress(self):
+        middleware = RPCErrorsMiddleware()
+        request = factory.make_fake_request(factory.make_string(), 'POST')
+        error_message = (
+            "Unable to execute power action: another action is already in "
+            "progress for node %s" % factory.make_name('node'))
+        error = PowerActionAlreadyInProgress(error_message)
+        response = middleware.process_exception(request, error)
+
+        # The response is a redirect.
+        self.assertEqual(request.path, extract_redirect(response))
+        # An error message has been published.
+        self.assertEqual(
+            [(constants.ERROR, error_message, '')],
+            request._messages.messages)
+
+    def test_handles_MultipleFailures(self):
+        middleware = RPCErrorsMiddleware()
+        request = factory.make_fake_request(factory.make_string(), 'POST')
+        failures = []
+        for _ in range(3):
+            error_message = factory.make_name("error-")
+            exception_class = random.choice(
+                (NoConnectionsAvailable, PowerActionAlreadyInProgress))
+            failures.append(Failure(exception_class(error_message)))
+        exception = MultipleFailures(*failures)
+        response = middleware.process_exception(request, exception)
+
+        # The response is a redirect.
+        self.assertEqual(request.path, extract_redirect(response))
+        # An error message has been published for each exception.
+        self.assertEqual(
+            [(constants.ERROR, unicode(failure.value), '')
+                for failure in failures],
+            request._messages.messages)
+
+    def test_handles_NoConnectionsAvailable(self):
+        middleware = RPCErrorsMiddleware()
+        request = factory.make_fake_request(factory.make_string(), 'POST')
+        error_message = (
+            "No connections availble for cluster %s" %
+            factory.make_name('cluster'))
+        error = PowerActionAlreadyInProgress(error_message)
+        response = middleware.process_exception(request, error)
+
+        # The response is a redirect.
+        self.assertEqual(request.path, extract_redirect(response))
+        # An error message has been published.
+        self.assertEqual(
+            [(constants.ERROR, error_message, '')],
+            request._messages.messages)
+
+    def test_ignores_non_rpc_errors(self):
+        middleware = RPCErrorsMiddleware()
+        request = factory.make_fake_request(factory.make_string(), 'POST')
+        exception = ZeroDivisionError(
+            "You may think it's a long walk down the street to the chemist "
+            "but that's just peanuts to space!")
+        response = middleware.process_exception(request, exception)
+        self.assertIsNone(response)
+
+    def test_adds_message_for_unknown_errors_in_multiple_failures(self):
+        # If an exception has no message, the middleware will generate a
+        # useful one and display it to the user.
+        middleware = RPCErrorsMiddleware()
+        request = factory.make_fake_request(factory.make_string(), 'POST')
+        unknown_exception = ZeroDivisionError()
+        failures = [
+            Failure(unknown_exception),
+            Failure(PowerActionAlreadyInProgress("Unzip a banana!")),
+            ]
+        exception = MultipleFailures(*failures)
+        response = middleware.process_exception(request, exception)
+        self.assertEqual(request.path, extract_redirect(response))
+
+        expected_messages = [
+            (
+                constants.ERROR,
+                (
+                    "Unexpected exception: %s. See "
+                    "/var/log/maas/maas-django.log "
+                    "on the region server for more information." %
+                    unknown_exception.__class__.__name__
+                ),
+                ''
+            ),
+            (constants.ERROR, unicode(failures[1].value), ''),
+            ]
+        self.assertEqual(
+            expected_messages,
+            request._messages.messages)

=== modified file 'src/provisioningserver/rpc/power.py'
--- src/provisioningserver/rpc/power.py	2014-09-26 19:14:19 +0000
+++ src/provisioningserver/rpc/power.py	2014-09-29 21:37:53 +0000
@@ -164,8 +164,9 @@
     registered_power_action = power_action_registry.get(system_id, None)
     if registered_power_action is not None:
         raise PowerActionAlreadyInProgress(
-            "Power action %s is already in progress for node %s" %
-            (registered_power_action, system_id))
+            "Unable to change power state to '%s' for node %s: another "
+            "action is already in progress for that node." %
+            (power_change, hostname))
     power_action_registry[system_id] = power_change
 
     yield power_change_starting(system_id, hostname, power_change)