Merge lp:~gmb/maas/add-alerts-for-disconnected-clusters-bug-1341121 into lp:~maas-committers/maas/trunk

[Graham Binns]

Add a persistent error when there are disconnected clusters, and remove it when all the clusters are connected again.

Previously, the only way to know about clusters being disconnected was to go to the cluster listing and look.

[Jeroen T. Vermeulen (jtv)]

I'm sure this will be appreciated, even if it may be a little annoying to some people with distributed clusters.

Review notes inline.

review: Approve

[Graham Binns (gmb)]

On 3 October 2014 12:09, Jeroen T. Vermeulen <email address hidden> wrote:
> I'm sure this will be appreciated, even if it may be a little annoying to some people with distributed clusters.

Definitely! Thank you! Don't worry about being annoying.

Oh, wait, you mean the branch. Nevermind ;P

> Review notes inline.
>
> Diff comments:
>
> Tiny suggestion: instead of...
>
> any(
> not cluster.is_connected() for cluster in accepted_clusters)
>
> you could probably write...
>
> not all(
> cluster.is_connected() for cluster in accepted_clusters)

Yep, done.

>> +class ExternalComponentsMiddlewareTest(MAASServerTestCase):
>> + """Tests for the ExternalComponentsMiddleware."""
>> +
>> + def test__checks_connectivity_of_accepted_clusters(self):
>> + get_client_for = self.patch(nodegroup_module, 'getClientFor')
>> + middleware = ExternalComponentsMiddleware()
>> + clusters = {
>> + factory.make_NodeGroup(status=NODEGROUP_STATUS.ACCEPTED)
>> + for _ in range(3)}
>
> Why are we creating 3 of everything? For luck? If the code is correct for 2, it's probably correct for 3. (And in Python, if it's correct for 1 chances are it's correct for 2. It's not like C where you might do *ptr instead of ptr[i] for every i.)
>
> Also, we've taken to putting the closing brace/bracket of a comprehension on a line of its own.

You know, I honestly don't know why. Habit, I guess. Three is a magic
number. It's not really relevant to have > 1 cluster in this test
anyway, I'll change it.

>> +
>> + request = factory.make_fake_request(factory.make_string(), 'GET')
>> + middleware.process_request(request)
>> +
>> + self.assertThat(
>> + get_client_for, MockCallsMatch(
>> + *[call(cluster.uuid, timeout=0) for cluster in clusters]))
>> +
>> + def test__ignores_non_accepted_clusters(self):
>> + get_client_for = self.patch(nodegroup_module, 'getClientFor')
>> + middleware = ExternalComponentsMiddleware()
>> + statuses = (
>> + NODEGROUP_STATUS.PENDING,
>> + NODEGROUP_STATUS.ACCEPTED,
>> + NODEGROUP_STATUS.REJECTED)
>> + clusters = {
>> + factory.make_NodeGroup(status=random.choice(statuses))
>> + for _ in range(3)}
>
> Again with the three! But why bother testing three random items from a list of 3 values, when it's actually easier to test all values?
>
> Although in this case I personally would do it differently: write one simple test just for ACCEPTED, and one simple test for a random other status. In the event that we get it wrong for pending clusters but not rejected ones, or vice versa, that leaves a fifty-fifty chance of the test passing by accident on any given run, so up to you to judge how likely that is.

Right.

> Also, there's no need to enumerate the statuses manually; just use factory.pick_enum(NODEGROUP_STATUS, but_not=[NODEGROUP_STATUS.ACCEPTED]).

Ah, didn't know about that, thanks.

>> + accepted_cluster_uuids = (
>> + cluster.uuid for cluster in clusters
>> + if cluster.status == NODEGROUP_STATUS....

[Graham Binns (gmb)]

On 3 October 2014 12:11, Raphaël Badin <email address hidden> wrote:
>
>
> Diff comments:
>
>> === modified file 'src/maasserver/enum.py'
>> --- src/maasserver/enum.py 2014-09-27 02:22:32 +0000
>> +++ src/maasserver/enum.py 2014-10-03 10:53:21 +0000
>> @@ -41,6 +41,7 @@
>> """Major moving parts of the application that may have failure states."""
>> PSERV = 'provisioning server'
>> IMPORT_PXE_FILES = 'maas-import-pxe-files script'
>> + CLUSTERS = 'clusters'
>
> We might want to have other component errors related to clusters. Maybe this should be renamed "DISCONNECTED_CLUSTERS"

Agreed.

>> class NODE_STATUS:
>>
>> + accepted_clusters = NodeGroup.objects.filter(
>> + status=NODEGROUP_STATUS.ACCEPTED)
>> + disconnected_clusters_found = any(
>> + not cluster.is_connected() for cluster in accepted_clusters)
>> + if disconnected_clusters_found:
>> + register_persistent_error(
>> + COMPONENT.CLUSTERS,
>> + "One or more clusters are currently disconnected. Visit "
>> + "the <a href=\"%s\">clusters</a> page for more "
>
> I think the link should apply to "clusters page" instead of just "clusters" but it's a detail.
>

Fixed.

[Raphaël Badin (rvb)]

review: Approve

[MAAS Lander (maas-lander)]

The attempt to merge lp:~gmb/maas/add-alerts-for-disconnected-clusters-bug-1341121 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 [46.3 kB]
Hit http://nova.clouds.archive.ubuntu.com trusty/main Sources
Get:6 http://security.ubuntu.com trusty-security/universe Sources [10.8 kB]
Get:7 http://security.ubuntu.com trusty-security/main amd64 Packages [146 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://nova.clouds.archive.ubuntu.com trusty/universe Translation-en
Get:8 http://security.ubuntu.com trusty-security/universe amd64 Packages [49.1 kB]
Hit http://security.ubuntu.com trusty-security/main Translation-en
Hit http://security.ubuntu.com trusty-security/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
Get:9 http://nova.clouds.archive.ubuntu.com trusty-updates/main Sources [125 kB]
Get:10 http://nova.clouds.archive.ubuntu.com trusty-updates/universe Sources [86.2 kB]
Get:11 http://nova.clouds.archive.ubuntu.com trusty-updates/main amd64 Packages [336 kB]
Get:12 http://nova.clouds.archive.ubuntu.com trusty-updates/universe amd64 Packages [209 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
Fetched 1,129 kB in 0s (1,852 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 ...

=== modified file 'src/maasserver/api/tests/test_nodegroup.py'
--- src/maasserver/api/tests/test_nodegroup.py	2014-09-24 14:20:51 +0000
+++ src/maasserver/api/tests/test_nodegroup.py	2014-10-03 14:52:27 +0000
@@ -218,6 +218,9 @@
         self.assertEqual(httplib.FORBIDDEN, response.status_code)
 
     def test_import_boot_images_for_all_accepted_clusters(self):
+        # Patch out the is_connected method of NodeGroup so as to avoid
+        # extra calls to getClient()
+        self.patch(nodegroup_module.NodeGroup, 'is_connected')
         self.become_admin()
         mock_getClientFor = self.patch(nodegroup_module, 'getClientFor')
         accepted_nodegroups = [
 
=== modified file 'src/maasserver/enum.py'
--- src/maasserver/enum.py	2014-09-27 02:22:32 +0000
+++ src/maasserver/enum.py	2014-10-03 14:52:27 +0000
@@ -41,6 +41,7 @@
     """Major moving parts of the application that may have failure states."""
     PSERV = 'provisioning server'
     IMPORT_PXE_FILES = 'maas-import-pxe-files script'
+    CLUSTERS = 'clusters'
 
 
 class NODE_STATUS:
 
=== modified file 'src/maasserver/middleware.py'
--- src/maasserver/middleware.py	2014-10-01 16:50:53 +0000
+++ src/maasserver/middleware.py	2014-10-03 14:52:27 +0000
@@ -44,6 +44,14 @@
     HttpResponseForbidden,
     HttpResponseRedirect,
     )
+from maasserver.components import (
+    discard_persistent_error,
+    register_persistent_error,
+    )
+from maasserver.enum import (
+    COMPONENT,
+    NODEGROUP_STATUS,
+    )
 from maasserver.models.nodegroup import NodeGroup
 from provisioningserver.rpc.exceptions import (
     MultipleFailures,
@@ -139,8 +147,19 @@
         # disturbing the handling of the request.  Proper error reporting
         # should be handled in the check method itself.
         try:
-            # TODO: Components checks here.
-            pass
+            # Check cluster connectivity.
+            accepted_clusters = NodeGroup.objects.filter(
+                status=NODEGROUP_STATUS.ACCEPTED)
+            disconnected_clusters_found = not all(
+                cluster.is_connected() for cluster in accepted_clusters)
+            if disconnected_clusters_found:
+                register_persistent_error(
+                    COMPONENT.CLUSTERS,
+                    "One or more clusters are currently disconnected. Visit "
+                    "the <a href=\"%s\">clusters page</a> for more "
+                    "information." % reverse('cluster-list'))
+            else:
+                discard_persistent_error(COMPONENT.CLUSTERS)
         except Exception:
             pass
         return None
 
=== modified file 'src/maasserver/tests/test_middleware.py'
--- src/maasserver/tests/test_middleware.py	2014-10-01 16:50:53 +0000
+++ src/maasserver/tests/test_middleware.py	2014-10-03 14:52:27 +0000
@@ -25,9 +25,18 @@
     PermissionDenied,
     ValidationError,
     )
+from django.core.urlresolvers import reverse
 from django.http import HttpResponse
 from django.http.request import build_request_repr
 from fixtures import FakeLogger
+from maasserver.components import (
+    get_persistent_error,
+    register_persistent_error,
+    )
+from maasserver.enum import (
+    COMPONENT,
+    NODEGROUP_STATUS,
+    )
 from maasserver.exceptions import (
     ExternalComponentException,
     MAASAPIException,
@@ -40,11 +49,17 @@
     DebuggingLoggerMiddleware,
     ErrorsMiddleware,
     ExceptionMiddleware,
+    ExternalComponentsMiddleware,
     RPCErrorsMiddleware,
     )
+from maasserver.models import nodegroup as nodegroup_module
 from maasserver.testing import extract_redirect
 from maasserver.testing.factory import factory
 from maasserver.testing.testcase import MAASServerTestCase
+from maastesting.matchers import (
+    MockCalledOnceWith,
+    MockNotCalled,
+    )
 from maastesting.utils import sample_binary_data
 from provisioningserver.rpc.exceptions import (
     MultipleFailures,
@@ -506,3 +521,78 @@
             "but that's just peanuts to space!")
         response = middleware.process_exception(request, exception)
         self.assertIsNone(response)
+
+
+class ExternalComponentsMiddlewareTest(MAASServerTestCase):
+    """Tests for the ExternalComponentsMiddleware."""
+
+    def test__checks_connectivity_of_accepted_clusters(self):
+        get_client_for = self.patch(nodegroup_module, 'getClientFor')
+        middleware = ExternalComponentsMiddleware()
+        cluster = factory.make_NodeGroup(status=NODEGROUP_STATUS.ACCEPTED)
+
+        request = factory.make_fake_request(factory.make_string(), 'GET')
+        middleware.process_request(request)
+
+        self.assertThat(
+            get_client_for, MockCalledOnceWith(cluster.uuid, timeout=0))
+
+    def test__ignores_non_accepted_clusters(self):
+        get_client_for = self.patch(nodegroup_module, 'getClientFor')
+        factory.make_NodeGroup(
+            status=factory.pick_enum(
+                NODEGROUP_STATUS, but_not=[NODEGROUP_STATUS.ACCEPTED]))
+        middleware = ExternalComponentsMiddleware()
+        request = factory.make_fake_request(factory.make_string(), 'GET')
+        middleware.process_request(request)
+
+        self.assertThat(get_client_for, MockNotCalled())
+
+    def test__registers_error_if_all_clusters_are_disconnected(self):
+        get_client_for = self.patch(nodegroup_module, 'getClientFor')
+        get_client_for.side_effect = NoConnectionsAvailable(
+            "Why, it's a jet-propelled, guided NAAFI!")
+        middleware = ExternalComponentsMiddleware()
+        factory.make_NodeGroup(status=NODEGROUP_STATUS.ACCEPTED)
+
+        request = factory.make_fake_request(factory.make_string(), 'GET')
+        middleware.process_request(request)
+
+        error = get_persistent_error(COMPONENT.CLUSTERS)
+        self.assertEqual(
+            "One or more clusters are currently disconnected. Visit the "
+            "<a href=\"%s\">clusters page</a> for more information." %
+            reverse('cluster-list'),
+            error)
+
+    def test__registers_error_if_any_clusters_are_disconnected(self):
+        get_client_for = self.patch(nodegroup_module, 'getClientFor')
+        get_client_for.side_effect = [
+            NoConnectionsAvailable("Why, it's a jet-propelled, guided NAAFI!"),
+            None,
+            ]
+        middleware = ExternalComponentsMiddleware()
+        factory.make_NodeGroup(status=NODEGROUP_STATUS.ACCEPTED)
+
+        request = factory.make_fake_request(factory.make_string(), 'GET')
+        middleware.process_request(request)
+
+        error = get_persistent_error(COMPONENT.CLUSTERS)
+        self.assertEqual(
+            "One or more clusters are currently disconnected. Visit the "
+            "<a href=\"%s\">clusters page</a> for more information." %
+            reverse('cluster-list'),
+            error)
+
+    def test__removes_error_once_all_clusters_are_connected(self):
+        # Patch getClientFor() to ensure that we don't actually try to
+        # connect to the cluster.
+        self.patch(nodegroup_module, 'getClientFor')
+        middleware = ExternalComponentsMiddleware()
+        factory.make_NodeGroup(status=NODEGROUP_STATUS.ACCEPTED)
+
+        register_persistent_error(
+            COMPONENT.CLUSTERS, "Who flung that batter pudding?")
+        request = factory.make_fake_request(factory.make_string(), 'GET')
+        middleware.process_request(request)
+        self.assertIsNone(get_persistent_error(COMPONENT.CLUSTERS))