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

[Raphaël Badin]

Return 503 response for PowerActionAlreadyInProgress exceptions. Add 'Retry-after' header for 503 responses: this will allow clients to retry failed requests.

The first change is simply a cleanup: we now use the middleware instead of raising an ad-hoc exception (the test for this already exists).

The second change is a bit more profound: it adds a default Retry-After header for all 503 responses. This will allow API clients such as Juju to retry failed requests.

The related change to gomaasapi is implemented in https://code.launchpad.net/~rvb/gomaasapi/gomaasapi-bug-1384001/+merge/239738.

[Graham Binns (gmb)]

review: Approve

[Julian Edwards (julian-edwards)]

I appreciate what you're doing but I don't like the direction the code is taking here.

Firstly, IMO mass catch-all middleware is dangerous as developers will be unknowingly shielded from potentially immense cock-ups. Developers need to be aware of all the errors/exceptions coming out of a function all, and deal with them *as soon as possible*. This is a basic tenet of software development.

Secondly, I believe the original middleware fix did not work because the bug that I fixed in the change you're reverting was to stop a *500* error, not a 409 (conflict), which is what should have happened if that middleware was catching the PowerActionAlreadyInProgress correctly.

Thirdly, I don't think a global retry-after value is appropriate, it should be set by each site that raises the error, depending on the situation.

I'm happy to work with you on this if you want to chat about it.

review: Disapprove

[Julian Edwards (julian-edwards)]

Ok I just had a long chat with Raph about this and the upshot is:

1. We came to some middle (haha) ground with the middleware. Raph was referring to general exceptions that can get thrown from anywhere, such as NoConnectionsAvailable. I was referring to exceptions that are declared in the AMP message itself, which are *known* (or should be) to the call sites as being specific to that call. So the upshot is that I am happy for middleware to catch general errors but I still think call sites are responsible for catching known exceptions, because they are best-placed to deal with them. For example, in the case of PowerOn, if we get a PowerActionAlreadyInProgress error, because that code still deals with potentially many nodes at once, it needs to catch and deal with the error because otherwise you'd rollback the entire transaction and potentially leave nodes started up when they should not be. However we decided to compromise on this for now and leave this caught in the middleware, and let Graham look at how to deal with it when he gets around to finishing the work on making node operations work on one at a time.

2. My second and third points above still stand. I convinced Raph that the third needs callsite-specific values with the example of AMT vs IPMI failures; AMT is very much slower at everything and needs a slower retry period.

Did I cover everything Raph? :)

review: Approve

[Raphaël Badin (rvb)]

> Ok I just had a long chat with Raph about this and the upshot is:
>
> 1. We came to some middle (haha) ground with the middleware. Raph was
> referring to general exceptions that can get thrown from anywhere, such as
> NoConnectionsAvailable. I was referring to exceptions that are declared in
> the AMP message itself, which are *known* (or should be) to the call sites as
> being specific to that call. So the upshot is that I am happy for middleware
> to catch general errors but I still think call sites are responsible for
> catching known exceptions, because they are best-placed to deal with them.
> For example, in the case of PowerOn, if we get a PowerActionAlreadyInProgress
> error, because that code still deals with potentially many nodes at once, it
> needs to catch and deal with the error because otherwise you'd rollback the
> entire transaction and potentially leave nodes started up when they should not
> be. However we decided to compromise on this for now and leave this caught in
> the middleware, and let Graham look at how to deal with it when he gets around
> to finishing the work on making node operations work on one at a time.
>
> 2. My second and third points above still stand. I convinced Raph that the
> third needs callsite-specific values with the example of AMT vs IPMI failures;
> AMT is very much slower at everything and needs a slower retry period.
>
> Did I cover everything Raph? :)

Yep, I think that's all; my side of the story:

- point 1 is valid but with the code that we have now it's much simpler to let the middlware handle the errors directly; we might want to revisit this when the refactoring gmb is working on lands.

- point 2 will be resolved when a fix for bug 1386549 lands.

- point 3 is worth updating this branch, I agree, I'll work on it now.

[Raphaël Badin (rvb)]

One more thing: the work I'm planning to do to address point #3 will be done in another branch. This is because it's not entirely sure that we will be able to backport it to 1.7. This gives us the option to only backport this branch to 1.7 if we need to.

[Julian Edwards (julian-edwards)]

On Tuesday 28 Oct 2014 10:52:27 you wrote:
> One more thing: the work I'm planning to do to address point #3 will be done
> in another branch. This is because it's not entirely sure that we will be
> able to backport it to 1.7. This gives us the option to only backport this
> branch to 1.7 if we need to.

+1!

[Christian Reis (kiko)]

This looks good for trunk and 1.7, minor comments below:

review: Approve

[Julian Edwards (julian-edwards)]

On Tuesday 28 Oct 2014 15:38:18 you wrote:
> > + RETRY_AFTER_SERVICE_UNAVAILABLE = 5
>
> Can the comment say this is seconds?

Better still, use:

RETRY_AFTER_SERVICE_UNAVAILABLE = timdelta(seconds=5).total_seconds()

=== modified file 'src/maasserver/api/nodes.py'
--- src/maasserver/api/nodes.py	2014-10-23 05:18:42 +0000
+++ src/maasserver/api/nodes.py	2014-10-27 13:22:20 +0000
@@ -82,10 +82,7 @@
     )
 from provisioningserver.power_schema import UNKNOWN_POWER_TYPE
 from provisioningserver.rpc.cluster import PowerQuery
-from provisioningserver.rpc.exceptions import (
-    NoConnectionsAvailable,
-    PowerActionAlreadyInProgress,
-    )
+from provisioningserver.rpc.exceptions import NoConnectionsAvailable
 import simplejson as json
 
 # Node's fields exposed on the API.
@@ -268,10 +265,7 @@
         node = Node.objects.get_node_or_404(
             system_id=system_id, user=request.user,
             perm=NODE_PERMISSION.EDIT)
-        try:
-            power_action_sent = node.stop(request.user, stop_mode=stop_mode)
-        except PowerActionAlreadyInProgress as e:
-            raise PowerProblem(e)
+        power_action_sent = node.stop(request.user, stop_mode=stop_mode)
         if power_action_sent:
             return node
         else:
 
=== modified file 'src/maasserver/middleware.py'
--- src/maasserver/middleware.py	2014-10-10 11:03:50 +0000
+++ src/maasserver/middleware.py	2014-10-27 13:22:20 +0000
@@ -350,11 +350,15 @@
 
     handled_exceptions = {
         NoConnectionsAvailable: httplib.SERVICE_UNAVAILABLE,
-        PowerActionAlreadyInProgress: httplib.CONFLICT,
+        PowerActionAlreadyInProgress: httplib.SERVICE_UNAVAILABLE,
         MultipleFailures: httplib.INTERNAL_SERVER_ERROR,
         TimeoutError: httplib.GATEWAY_TIMEOUT,
         }
 
+    # Default 'Retry-After' header sent for httplib.SERVICE_UNAVAILABLE
+    # responses.
+    RETRY_AFTER_SERVICE_UNAVAILABLE = 5
+
     def process_exception(self, request, exception):
         path_matcher = re.compile(settings.API_URL_REGEXP)
         if not path_matcher.match(get_relative_path(request.path)):
@@ -380,6 +384,10 @@
             error_message = get_error_message_for_exception(exception)
 
         encoding = b'utf-8'
-        return HttpResponse(
+        response = HttpResponse(
             content=error_message.encode(encoding), status=status,
             mimetype=b"text/plain; charset=%s" % encoding)
+        if status == httplib.SERVICE_UNAVAILABLE:
+            response['Retry-After'] = (
+                self.RETRY_AFTER_SERVICE_UNAVAILABLE)
+        return response
 
=== modified file 'src/maasserver/tests/test_middleware.py'
--- src/maasserver/tests/test_middleware.py	2014-10-10 13:25:21 +0000
+++ src/maasserver/tests/test_middleware.py	2014-10-27 13:22:20 +0000
@@ -446,7 +446,21 @@
             (httplib.SERVICE_UNAVAILABLE, error_message),
             (response.status_code, response.content))
 
-    def test_power_action_already_in_progress_returned_as_409(self):
+    def test_503_response_includes_retry_after_header_by_default(self):
+        middleware = APIRPCErrorsMiddleware()
+        request = factory.make_fake_request(
+            "/api/1.0/" + factory.make_string(), 'POST')
+        error = NoConnectionsAvailable(factory.make_name())
+        response = middleware.process_exception(request, error)
+
+        self.assertEqual(
+            (
+                httplib.SERVICE_UNAVAILABLE,
+                '%s' % middleware.RETRY_AFTER_SERVICE_UNAVAILABLE,
+            ),
+            (response.status_code, response['Retry-after']))
+
+    def test_power_action_already_in_progress_returned_as_503(self):
         middleware = APIRPCErrorsMiddleware()
         request = factory.make_fake_request(
             "/api/1.0/" + factory.make_string(), 'POST')
@@ -457,7 +471,7 @@
         response = middleware.process_exception(request, error)
 
         self.assertEqual(
-            (httplib.CONFLICT, error_message),
+            (httplib.SERVICE_UNAVAILABLE, error_message),
             (response.status_code, response.content))
 
     def test_multiple_failures_returned_as_500(self):