Merge lp:~rvb/maas/bug-1384001-redux-2 into lp:~maas-committers/maas/trunk

[Raphaël Badin]

When node.wait_for_power_commands raises only one exception: re-raise this exception instead of MultipleFailures. This allows the proper response (e.g. a 503 response) to be returned by the API instead of the default 500 response.

This is a minimal fix that I'm doing so that a 503 response is returned when a power action is already in progress. Turns out node.wait_for_power_commands is only called for one node at a time and thus the MultipleFailures exception isn't really useful.

I'm planning to backport this to 1.7 and this is why this change is minimal: a more thorough cleanup is in progress (to completely get rid of MutlipleFailures) but we will land it only on trunk.

Additionally, this raises the default waiting time to 10 seconds: since this is a default we apply to any operation, it's probably best to be on the safe side; when this done on a pre-exception (and thus per-power type) basis, we will use something more fine-grained.

[Graham Binns (gmb)]

review: Approve

[Christian Reis (kiko)]

Approved for 1.7 and trunk.

review: Approve

[Gavin Panella (allenap)]

I'm on the fence; see my one comment.

review: Needs Information

[Gavin Panella (allenap)]

Raphaël has said that he'll do this in the middleware instead. I think that's slightly less risky, so +1 on that basis so he's not blocked on me. Someone should still give the code a once-over before landing.

MultipleFailures is also planned for removal in trunk.

review: Approve

[Gavin Panella (allenap)]

Nice.

review: Approve

[Julian Edwards (julian-edwards)]

On Tuesday 28 Oct 2014 17:21:59 you wrote:
> I know it doesn't fit into Python's exception model very well, but then
> Python's exception model doesn't allow for concurrent exceptions in a
> single thread. It's a sign that we ought to move error-handling code closer
> to the call-site. The division between synchronous and asynchronous code
> is, I think, partially responsible.

When Raph and I discussed this, for trunk we agreed that we'd remove
MultipleFailures entirely (I believe you added it because of the multiple node
start-up stuff) and handle errors declared in the AMP message at the callsites
themselves. General errors should still get handled in the middleware.

=== modified file 'src/maasserver/middleware.py'
--- src/maasserver/middleware.py	2014-10-27 13:22:08 +0000
+++ src/maasserver/middleware.py	2014-10-28 20:55:59 +0000
@@ -357,7 +357,7 @@
 
     # Default 'Retry-After' header sent for httplib.SERVICE_UNAVAILABLE
     # responses.
-    RETRY_AFTER_SERVICE_UNAVAILABLE = 5
+    RETRY_AFTER_SERVICE_UNAVAILABLE = 10
 
     def process_exception(self, request, exception):
         path_matcher = re.compile(settings.API_URL_REGEXP)
@@ -374,6 +374,12 @@
 
         status = self.handled_exceptions[exception.__class__]
         if isinstance(exception, MultipleFailures):
+            # If only one exception has been raised, process this exception:
+            # this allows MAAS to convert this exception into the proper
+            # type of response (e.g. 503) instead of the 500 response that
+            # MultipleFailures is transformed into.
+            if len(exception.args) == 1:
+                return self.process_exception(request, exception.args[0].value)
             for failure in exception.args:
                 logging.exception(exception)
             error_message = "\n".join(
 
=== modified file 'src/maasserver/tests/test_middleware.py'
--- src/maasserver/tests/test_middleware.py	2014-10-27 10:38:45 +0000
+++ src/maasserver/tests/test_middleware.py	2014-10-28 20:55:59 +0000
@@ -494,6 +494,19 @@
             (httplib.INTERNAL_SERVER_ERROR, expected_error_message),
             (response.status_code, response.content))
 
+    def test_multiple_failures_with_one_exception(self):
+        middleware = APIRPCErrorsMiddleware()
+        request = factory.make_fake_request(
+            "/api/1.0/" + factory.make_string(), 'POST')
+        expected_error_message = factory.make_name("error")
+        unique_exception = PowerActionAlreadyInProgress(expected_error_message)
+        exception = MultipleFailures(Failure(unique_exception))
+        response = middleware.process_exception(request, exception)
+
+        self.assertEqual(
+            (httplib.SERVICE_UNAVAILABLE, expected_error_message),
+            (response.status_code, response.content))
+
     def test_handles_TimeoutError(self):
         middleware = APIRPCErrorsMiddleware()
         request = factory.make_fake_request(