Merge lp:~julian-edwards/maas/rndc-crash-bug-1386488 into lp:~maas-committers/maas/trunk

[Julian Edwards]

Catch ExternalProcessError from rndc_command (and anywhere else that might raise it in the future) when dealing with node actions in the web UI and API. The API now returns a SERVICE_UNAVAILABLE instead of a 500 with a stacktrace in the appserver log.

I've done this with a middleware change for the API and followed the example of the RPC exceptions in the node actions and extended its concept to be a little more general, in particular the test.

[Julian Edwards (julian-edwards)]

Marking WIP, something is wrong with the diff.

[Julian Edwards (julian-edwards)]

Right, fixed, please to be reviewing.

[Graham Binns (gmb)]

Looks good to me. Rambly thought inline, but no changes needed.

review: Approve

[Julian Edwards (julian-edwards)]

On Wednesday 29 Oct 2014 07:36:57 you wrote:
> Ultimately, when NodeActionErrors are caught (in the form from which the
> action originated), the error's message is added to the UI as a warning.
> In this case, you might see something like:
>
> "Command `rndc -c /etc/bind/maas/rndc.conf.maas reload maas` returned
> non-zero exit status 1: rndc: 'reload' failed: dynamic zone"
>
> In the UI.
>
> Actually, that's not so terrible (in that it can then be reported to an
> admin, or the admin can go and fix it… I wonder if we shouldn't be
> sanitising some of those kind of messages for non-admins, though. Not,
> however, a concern for this branch; just a rambly 7:30am thought.

I think that given the likelihood of this error ever happening I don't mind
dumping the whole thing there. If a user sees it, they'll just grab an
operator/admin and report what they see, which is actually very useful.

Cheers, thanks for the review!

[MAAS Lander (maas-lander)]

The attempt to merge lp:~julian-edwards/maas/rndc-crash-bug-1386488 into lp:maas failed. Below is the output from the failed tests.

Ign http://security.ubuntu.com trusty-security InRelease
Ign http://nova.clouds.archive.ubuntu.com trusty InRelease
Get:1 http://security.ubuntu.com trusty-security Release.gpg [933 B]
Ign http://nova.clouds.archive.ubuntu.com trusty-updates InRelease
Get:2 http://security.ubuntu.com trusty-security Release [59.7 kB]
Hit http://nova.clouds.archive.ubuntu.com trusty Release.gpg
Get:3 http://nova.clouds.archive.ubuntu.com trusty-updates Release.gpg [933 B]
Hit http://nova.clouds.archive.ubuntu.com trusty Release
Get:4 http://nova.clouds.archive.ubuntu.com trusty-updates Release [59.7 kB]
Hit http://nova.clouds.archive.ubuntu.com trusty/main Sources
Hit http://nova.clouds.archive.ubuntu.com trusty/universe Sources
Get:5 http://security.ubuntu.com trusty-security/main Sources [48.3 kB]
Hit http://nova.clouds.archive.ubuntu.com trusty/main amd64 Packages
Hit http://nova.clouds.archive.ubuntu.com trusty/universe amd64 Packages
Hit http://nova.clouds.archive.ubuntu.com trusty/main Translation-en
Hit http://nova.clouds.archive.ubuntu.com trusty/universe Translation-en
Get:6 http://security.ubuntu.com trusty-security/universe Sources [11.2 kB]
Get:7 http://security.ubuntu.com trusty-security/main amd64 Packages [151 kB]
Get:8 http://nova.clouds.archive.ubuntu.com trusty-updates/main Sources [133 kB]
Get:9 http://nova.clouds.archive.ubuntu.com trusty-updates/universe Sources [89.3 kB]
Get:10 http://security.ubuntu.com trusty-security/universe amd64 Packages [50.4 kB]
Hit http://security.ubuntu.com trusty-security/main Translation-en
Get:11 http://nova.clouds.archive.ubuntu.com trusty-updates/main amd64 Packages [351 kB]
Hit http://security.ubuntu.com trusty-security/universe Translation-en
Get:12 http://nova.clouds.archive.ubuntu.com trusty-updates/universe amd64 Packages [216 kB]
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/main Translation-en
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/universe Translation-en
Ign http://nova.clouds.archive.ubuntu.com trusty/main Translation-en_US
Ign http://nova.clouds.archive.ubuntu.com trusty/universe Translation-en_US
Fetched 1,171 kB in 2s (410 kB/s)
Reading package lists...
sudo DEBIAN_FRONTEND=noninteractive apt-get -y \
     --no-install-recommends install apache2 authbind bind9 bind9utils build-essential bzr-builddeb curl daemontools debhelper dh-apport distro-info dnsutils firefox freeipmi-tools gjs ipython isc-dhcp-common libjs-raphael libjs-yui3-full libjs-yui3-min libpq-dev make pep8 postgresql pyflakes python-amqplib python-bzrlib python-celery python-convoy python-crochet python-cssselect python-curtin python-dev python-distro-info python-django python-django-piston python-django-south python-djorm-ext-pgarray python-docutils python-extras python-fixtures python-flake8 python-formencode python-hivex python-httplib2 python-jinja2 python-jsonschema python-lockfile python-lxml python-mimeparse python-mock python-netaddr python-netifaces python-nose python-oauth python-oops python-oops-amqp python-oops-datedir-repo python-oops-twisted python-oops-wsgi ...

[MAAS Lander (maas-lander)]

There are additional revisions which have not been approved in review. Please seek review and approval of these new revisions.

[MAAS Lander (maas-lander)]

The attempt to merge lp:~julian-edwards/maas/rndc-crash-bug-1386488 into lp:maas failed. Below is the output from the failed tests.

Ign http://security.ubuntu.com trusty-security InRelease
Get:1 http://security.ubuntu.com trusty-security Release.gpg [933 B]
Ign http://nova.clouds.archive.ubuntu.com trusty InRelease
Get:2 http://security.ubuntu.com trusty-security Release [59.7 kB]
Ign http://nova.clouds.archive.ubuntu.com trusty-updates InRelease
Hit http://nova.clouds.archive.ubuntu.com trusty Release.gpg
Get:3 http://nova.clouds.archive.ubuntu.com trusty-updates Release.gpg [933 B]
Hit http://nova.clouds.archive.ubuntu.com trusty Release
Get:4 http://nova.clouds.archive.ubuntu.com trusty-updates Release [59.7 kB]
Get:5 http://security.ubuntu.com trusty-security/main Sources [48.3 kB]
Hit http://nova.clouds.archive.ubuntu.com trusty/main Sources
Get:6 http://security.ubuntu.com trusty-security/universe Sources [11.2 kB]
Get:7 http://security.ubuntu.com trusty-security/main amd64 Packages [151 kB]
Get:8 http://security.ubuntu.com trusty-security/universe amd64 Packages [50.4 kB]
Hit http://nova.clouds.archive.ubuntu.com trusty/universe Sources
Hit http://nova.clouds.archive.ubuntu.com trusty/main amd64 Packages
Hit http://nova.clouds.archive.ubuntu.com trusty/universe amd64 Packages
Hit http://nova.clouds.archive.ubuntu.com trusty/main Translation-en
Hit http://security.ubuntu.com trusty-security/main Translation-en
Hit http://nova.clouds.archive.ubuntu.com trusty/universe Translation-en
Hit http://security.ubuntu.com trusty-security/universe Translation-en
Get:9 http://nova.clouds.archive.ubuntu.com trusty-updates/main Sources [133 kB]
Get:10 http://nova.clouds.archive.ubuntu.com trusty-updates/universe Sources [89.3 kB]
Get:11 http://nova.clouds.archive.ubuntu.com trusty-updates/main amd64 Packages [351 kB]
Get:12 http://nova.clouds.archive.ubuntu.com trusty-updates/universe amd64 Packages [216 kB]
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/main Translation-en
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/universe Translation-en
Ign http://nova.clouds.archive.ubuntu.com trusty/main Translation-en_US
Ign http://nova.clouds.archive.ubuntu.com trusty/universe Translation-en_US
Fetched 1,171 kB in 2s (407 kB/s)
Reading package lists...
sudo DEBIAN_FRONTEND=noninteractive apt-get -y \
     --no-install-recommends install apache2 authbind bind9 bind9utils build-essential bzr-builddeb curl daemontools debhelper dh-apport distro-info dnsutils firefox freeipmi-tools gjs ipython isc-dhcp-common libjs-raphael libjs-yui3-full libjs-yui3-min libpq-dev make pep8 postgresql pyflakes python-amqplib python-bzrlib python-celery python-convoy python-crochet python-cssselect python-curtin python-dev python-distro-info python-django python-django-piston python-django-south python-djorm-ext-pgarray python-docutils python-extras python-fixtures python-flake8 python-formencode python-hivex python-httplib2 python-jinja2 python-jsonschema python-lockfile python-lxml python-mimeparse python-mock python-netaddr python-netifaces python-nose python-oauth python-oops python-oops-amqp python-oops-datedir-repo python-oops-twisted python-oops-wsgi ...

[Julian Edwards (julian-edwards)]

Lint, pointless fscking makework since 1984.

=== modified file 'src/maasserver/middleware.py'
--- src/maasserver/middleware.py	2014-10-28 20:55:30 +0000
+++ src/maasserver/middleware.py	2014-10-30 05:37:05 +0000
@@ -60,6 +60,7 @@
     NoConnectionsAvailable,
     PowerActionAlreadyInProgress,
     )
+from provisioningserver.utils.shell import ExternalProcessError
 
 
 try:
@@ -238,6 +239,21 @@
             return HttpResponseForbidden(
                 content=unicode(exception).encode(encoding),
                 mimetype=b"text/plain; charset=%s" % encoding)
+        elif isinstance(exception, ExternalProcessError):
+            # Catch problems interacting with processes that the
+            # appserver spawns, e.g. rndc.
+            #
+            # While this is a serious error, it should be a temporary
+            # one as the admin should be checking and fixing, or it
+            # could be spurious.  There's no way of knowing, so the best
+            # course of action is to ask the caller to repeat.
+            response = HttpResponse(
+                content=unicode(exception).encode(encoding),
+                status=httplib.SERVICE_UNAVAILABLE,
+                mimetype=b"text/plain; charset=%s" % encoding)
+            response['Retry-After'] = (
+                self.RETRY_AFTER_SERVICE_UNAVAILABLE)
+            return response
         else:
             # Print a traceback.
             self.log_exception(exception)

=== modified file 'src/maasserver/node_action.py'
--- src/maasserver/node_action.py	2014-10-23 05:52:37 +0000
+++ src/maasserver/node_action.py	2014-10-30 05:37:05 +0000
@@ -55,6 +55,7 @@
     PowerActionAlreadyInProgress,
     )
 from provisioningserver.utils.enum import map_enum
+from provisioningserver.utils.shell import ExternalProcessError
 
 # All node statuses.
 ALL_STATUSES = set(NODE_STATUS_CHOICES_DICT.keys())
@@ -200,7 +201,7 @@
         """See `NodeAction.execute`."""
         try:
             self.node.start_commissioning(self.user)
-        except RPC_EXCEPTIONS as exception:
+        except RPC_EXCEPTIONS + (ExternalProcessError,) as exception:
             raise NodeActionError(exception)
         else:
             return "Node commissioning started."
@@ -219,7 +220,7 @@
         """See `NodeAction.execute`."""
         try:
             self.node.abort_commissioning(self.user)
-        except RPC_EXCEPTIONS as exception:
+        except RPC_EXCEPTIONS + (ExternalProcessError,) as exception:
             raise NodeActionError(exception)
         else:
             return "Node commissioning aborted."
@@ -238,7 +239,7 @@
         """See `NodeAction.execute`."""
         try:
             self.node.abort_operation(self.user)
-        except RPC_EXCEPTIONS as exception:
+        except RPC_EXCEPTIONS + (ExternalProcessError,) as exception:
             raise NodeActionError(exception)
         else:
             return "Node operation aborted."
@@ -347,7 +348,7 @@
             raise NodeActionError(
                 "%s: Failed to start, static IP addresses are exhausted."
                 % self.node.hostname)
-        except RPC_EXCEPTIONS as exception:
+        except RPC_EXCEPTIONS + (ExternalProcessError,) as exception:
             raise NodeActionError(exception)
         else:
             return "This node has been asked to start up."
@@ -376,7 +377,7 @@
         """See `NodeAction.execute`."""
         try:
             self.node.stop(self.user)
-        except RPC_EXCEPTIONS as exception:
+        except RPC_EXCEPTIONS + (ExternalProcessError,) as exception:
             raise NodeActionError(exception)
         else:
             return "This node has been asked to shut down."
@@ -398,7 +399,7 @@
         """See `NodeAction.execute`."""
         try:
             self.node.release_or_erase()
-        except RPC_EXCEPTIONS as exception:
+        except RPC_EXCEPTIONS + (ExternalProcessError,) as exception:
             raise NodeActionError(exception)
         else:
             return "This node is no longer allocated to you."

=== modified file 'src/maasserver/tests/test_middleware.py'
--- src/maasserver/tests/test_middleware.py	2014-10-28 20:55:30 +0000
+++ src/maasserver/tests/test_middleware.py	2014-10-30 05:37:05 +0000
@@ -67,9 +67,11 @@
     NoConnectionsAvailable,
     PowerActionAlreadyInProgress,
     )
+from provisioningserver.utils.shell import ExternalProcessError
 from provisioningserver.utils.text import normalise_whitespace
 from testtools.matchers import (
     Contains,
+    Equals,
     Not,
     )
 from twisted.python.failure import Failure
@@ -81,23 +83,29 @@
         """Return a path to handle exceptions for."""
         return "/%s" % factory.make_string()
 
-    def make_middleware(self, base_path):
+    def make_middleware(self, base_path, retry_after=None):
         """Create an ExceptionMiddleware for base_path."""
         class TestingExceptionMiddleware(ExceptionMiddleware):
             path_regex = base_path
 
-        return TestingExceptionMiddleware()
-
-    def process_exception(self, exception):
+        testing_middleware = TestingExceptionMiddleware()
+        if retry_after is not None:
+            testing_middleware.RETRY_AFTER_SERVICE_UNAVAILABLE = retry_after
+
+        return testing_middleware
+
+    def process_exception(self, exception, retry_after=None):
         """Run a given exception through a fake ExceptionMiddleware.
 
         :param exception: The exception to simulate.
         :type exception: Exception
+        :param retry_after: Value of the RETRY_AFTER_SERVICE_UNAVAILABLE to
+            use in the fake middleware.
         :return: The response as returned by the ExceptionMiddleware.
         :rtype: HttpResponse or None.
         """
         base_path = self.make_base_path()
-        middleware = self.make_middleware(base_path)
+        middleware = self.make_middleware(base_path, retry_after)
         request = factory.make_fake_request(base_path)
         return middleware.process_exception(request, exception)
 
@@ -170,6 +178,16 @@
         self.process_exception(Exception(error_text))
         self.assertThat(logger.output, Contains(error_text))
 
+    def test_reports_ExternalProcessError_as_ServiceUnavailable(self):
+        error_text = factory.make_string()
+        exception = ExternalProcessError(1, ["cmd"], error_text)
+        retry_after = random.randint(0, 10)
+        response = self.process_exception(exception, retry_after)
+        self.expectThat(
+            response.status_code, Equals(httplib.SERVICE_UNAVAILABLE))
+        self.expectThat(response.content, Equals(unicode(exception)))
+        self.expectThat(response['Retry-After'], Equals("%s" % retry_after))
+
 
 class APIErrorsMiddlewareTest(MAASServerTestCase):
 

=== modified file 'src/maasserver/tests/test_node_action.py'
--- src/maasserver/tests/test_node_action.py	2014-10-23 05:55:44 +0000
+++ src/maasserver/tests/test_node_action.py	2014-10-30 05:37:05 +0000
@@ -57,6 +57,7 @@
 from maastesting.matchers import MockCalledOnceWith
 from mock import ANY
 from provisioningserver.rpc.exceptions import MultipleFailures
+from provisioningserver.utils.shell import ExternalProcessError
 from testtools.matchers import Equals
 from twisted.python.failure import Failure
 
@@ -561,18 +562,24 @@
         self.assertItemsEqual([], actions)
 
 
-class TestRPCActionsErrorHandling(MAASServerTestCase):
-    """Tests for error handling in actions that need RPC."""
+class TestActionsErrorHandling(MAASServerTestCase):
+    """Tests for error handling in actions.
 
+    This covers RPC exceptions and `ExternalProcessError`s.
+    """
+    exceptions = RPC_EXCEPTIONS + (ExternalProcessError,)
     scenarios = [
         (exception_class.__name__, {"exception_class": exception_class})
-        for exception_class in RPC_EXCEPTIONS
+        for exception_class in exceptions
         ]
 
     def make_exception(self):
         if self.exception_class is MultipleFailures:
             exception = self.exception_class(
                 Failure(Exception(factory.make_name("exception"))))
+        elif self.exception_class is ExternalProcessError:
+            exception = self.exception_class(
+                1, ["cmd"], factory.make_name("exception"))
         else:
             exception = self.exception_class(factory.make_name("exception"))
         return exception