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

Proposed by Raphaël Badin on 2014-10-28
Status: Merged
Approved by: Raphaël Badin on 2014-10-28
Approved revision: 3308
Merged at revision: 3305
Proposed branch: lp:~rvb/maas/bug-1384001-redux-2
Merge into: lp:maas/trunk
Diff against target: 49 lines (+20/-1)
2 files modified
src/maasserver/middleware.py (+7/-1)
src/maasserver/tests/test_middleware.py (+13/-0)
To merge this branch: bzr merge lp:~rvb/maas/bug-1384001-redux-2
Reviewer Review Type Date Requested Status
Gavin Panella (community) Approve on 2014-10-28
Christian Reis (community) Approve on 2014-10-28
Graham Binns (community) 2014-10-28 Approve on 2014-10-28
Review via email: mp+239855@code.launchpad.net

Commit message

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.

Description of the change

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.

To post a comment you must log in.
Graham Binns (gmb) :
review: Approve
lp:~rvb/maas/bug-1384001-redux-2 updated on 2014-10-28
3307. By Raphaël Badin on 2014-10-28

10 seconds.

Christian Reis (kiko) wrote :

Approved for 1.7 and trunk.

review: Approve
Gavin Panella (allenap) wrote :

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

review: Needs Information
Gavin Panella (allenap) wrote :

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
lp:~rvb/maas/bug-1384001-redux-2 updated on 2014-10-28
3308. By Raphaël Badin on 2014-10-28

Fix this in the middleware instead.

Gavin Panella (allenap) wrote :

Nice.

review: Approve
Julian Edwards (julian-edwards) wrote :

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.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/maasserver/middleware.py'
2--- src/maasserver/middleware.py 2014-10-27 13:22:08 +0000
3+++ src/maasserver/middleware.py 2014-10-28 20:55:59 +0000
4@@ -357,7 +357,7 @@
5
6 # Default 'Retry-After' header sent for httplib.SERVICE_UNAVAILABLE
7 # responses.
8- RETRY_AFTER_SERVICE_UNAVAILABLE = 5
9+ RETRY_AFTER_SERVICE_UNAVAILABLE = 10
10
11 def process_exception(self, request, exception):
12 path_matcher = re.compile(settings.API_URL_REGEXP)
13@@ -374,6 +374,12 @@
14
15 status = self.handled_exceptions[exception.__class__]
16 if isinstance(exception, MultipleFailures):
17+ # If only one exception has been raised, process this exception:
18+ # this allows MAAS to convert this exception into the proper
19+ # type of response (e.g. 503) instead of the 500 response that
20+ # MultipleFailures is transformed into.
21+ if len(exception.args) == 1:
22+ return self.process_exception(request, exception.args[0].value)
23 for failure in exception.args:
24 logging.exception(exception)
25 error_message = "\n".join(
26
27=== modified file 'src/maasserver/tests/test_middleware.py'
28--- src/maasserver/tests/test_middleware.py 2014-10-27 10:38:45 +0000
29+++ src/maasserver/tests/test_middleware.py 2014-10-28 20:55:59 +0000
30@@ -494,6 +494,19 @@
31 (httplib.INTERNAL_SERVER_ERROR, expected_error_message),
32 (response.status_code, response.content))
33
34+ def test_multiple_failures_with_one_exception(self):
35+ middleware = APIRPCErrorsMiddleware()
36+ request = factory.make_fake_request(
37+ "/api/1.0/" + factory.make_string(), 'POST')
38+ expected_error_message = factory.make_name("error")
39+ unique_exception = PowerActionAlreadyInProgress(expected_error_message)
40+ exception = MultipleFailures(Failure(unique_exception))
41+ response = middleware.process_exception(request, exception)
42+
43+ self.assertEqual(
44+ (httplib.SERVICE_UNAVAILABLE, expected_error_message),
45+ (response.status_code, response.content))
46+
47 def test_handles_TimeoutError(self):
48 middleware = APIRPCErrorsMiddleware()
49 request = factory.make_fake_request(