Merge lp:~rvb/maas/cluster-connected into lp:~maas-committers/maas/trunk

[Raphaël Badin]

Display if clusters are connected on the cluster listing page.

Here is what it looks like:
connected: http://people.canonical.com/~rvb/connected_cluster.png
disconnected: http://people.canonical.com/~rvb/disconnected_cluster.png

The plan is to integrate this with a proper notification system when it's implemented but this needs some serious UI work. This branch is a first step: backend work + basic display in the UI.

[Jeroen T. Vermeulen (jtv)]

I guess there are no HTML entity codes for the special characters you wanted... shame.

The naming trick in test_listing_displays_connection_status is a bit hairy. I'd explain that right up front so that the reader understands how the code fits together.

Also in that test, instead of “self.name[-1] == '0'” I'd say “self.name.endswith('0')”.

And finally, test_warning_is_displayed_if_a_cluster_is_not_connected could use the HasLength matcher.

review: Approve

[Raphaël Badin (rvb)]

Thanks for the review!

> I guess there are no HTML entity codes for the special characters you
> wanted... shame.

Well, yes: ✓ and ❌. I'm even using them here. Am I missing something?

>
> The naming trick in test_listing_displays_connection_status is a bit hairy.
> I'd explain that right up front so that the reader understands how the code
> fits together.

Done. I thought about alternative solutions like using the UUID of the nodegroup but it's not trivial mainly because the UUIDs are not really random.

> Also in that test, instead of “self.name[-1] == '0'” I'd say
> “self.name.endswith('0')”.

Done.

> And finally, test_warning_is_displayed_if_a_cluster_is_not_connected could use
> the HasLength matcher.

Done.

[MAAS Lander (maas-lander)]

The attempt to merge lp:~rvb/maas/cluster-connected into lp:maas failed. Below is the output from the failed tests.

Ign http://security.ubuntu.com trusty-security InRelease
Hit http://security.ubuntu.com trusty-security Release.gpg
Hit http://security.ubuntu.com trusty-security Release
Ign http://nova.clouds.archive.ubuntu.com trusty InRelease
Ign http://nova.clouds.archive.ubuntu.com trusty-updates InRelease
Hit http://nova.clouds.archive.ubuntu.com trusty Release.gpg
Hit http://nova.clouds.archive.ubuntu.com trusty-updates Release.gpg
Hit http://nova.clouds.archive.ubuntu.com trusty Release
Hit http://nova.clouds.archive.ubuntu.com trusty-updates Release
Hit http://security.ubuntu.com trusty-security/main Sources
Hit http://security.ubuntu.com trusty-security/universe Sources
Hit http://security.ubuntu.com trusty-security/main amd64 Packages
Hit http://security.ubuntu.com trusty-security/universe amd64 Packages
Hit http://security.ubuntu.com trusty-security/main Translation-en
Hit http://security.ubuntu.com trusty-security/universe Translation-en
Hit http://nova.clouds.archive.ubuntu.com trusty/main Sources
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
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/main Sources
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/universe Sources
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/main amd64 Packages
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/universe amd64 Packages
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
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 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 python-openssl python-paramiko python-pexpect python-pip python-pocket-lint python-psycopg2 python-pyinotify python-seamicroclient python-simplejson python-simplestreams python-sphinx python-su...

[Jeroen T. Vermeulen (jtv)]

I was thinking of ✓ and ✗ for the entity codes. Those do actually seem to be in the HTML5 spec: http://dev.w3.org/html5/html-author/charref

[Raphaël Badin (rvb)]

> I was thinking of ✓ and ✗ for the entity codes. Those do actually
> seem to be in the HTML5 spec: http://dev.w3.org/html5/html-author/charref

Right, done.

=== modified file 'src/maasserver/models/nodegroup.py'
--- src/maasserver/models/nodegroup.py	2014-09-11 10:08:50 +0000
+++ src/maasserver/models/nodegroup.py	2014-09-11 12:52:50 +0000
@@ -229,6 +229,16 @@
             # Persist the dhcp_key without triggering the signals.
             NodeGroup.objects.filter(id=self.id).update(dhcp_key=dhcp_key)
 
+    def is_connected(self):
+        """Is this cluster connected to his provisioning server?"""
+        try:
+            # Use a timeout of zero not to block.
+            getClientFor(self.uuid, timeout=0)
+        except NoConnectionsAvailable:
+            return False
+        else:
+            return True
+
     @property
     def work_queue(self):
         """The name of the queue for tasks specific to this nodegroup."""

=== modified file 'src/maasserver/models/tests/test_nodegroup.py'
--- src/maasserver/models/tests/test_nodegroup.py	2014-09-11 10:08:50 +0000
+++ src/maasserver/models/tests/test_nodegroup.py	2014-09-11 12:52:50 +0000
@@ -44,6 +44,7 @@
     ANY,
     call,
     Mock,
+    sentinel,
     )
 from provisioningserver.dhcp.omshell import generate_omapi_key
 from provisioningserver.rpc.cluster import (
@@ -420,6 +421,24 @@
             protocol.ImportBootImages,
             MockCalledOnceWith(ANY, sources=[get_simplestream_endpoint()]))
 
+    def test_is_connected_returns_true_if_connection(self):
+        mock_getClientFor = self.patch(nodegroup_module, 'getClientFor')
+        mock_getClientFor.return_value = sentinel
+        nodegroup = factory.make_NodeGroup()
+        connected = nodegroup.is_connected()
+        self.assertThat(
+            mock_getClientFor, MockCalledOnceWith(nodegroup.uuid, timeout=0))
+        self.assertTrue(connected)
+
+    def test_is_connected_returns_true_if_no_connection(self):
+        mock_getClientFor = self.patch(nodegroup_module, 'getClientFor')
+        mock_getClientFor.side_effect = NoConnectionsAvailable
+        nodegroup = factory.make_NodeGroup()
+        connected = nodegroup.is_connected()
+        self.assertThat(
+            mock_getClientFor, MockCalledOnceWith(nodegroup.uuid, timeout=0))
+        self.assertFalse(connected)
+
     def test_add_virsh_end_to_end(self):
         nodegroup = factory.make_NodeGroup(status=NODEGROUP_STATUS.ACCEPTED)
 

=== modified file 'src/maasserver/templates/maasserver/cluster_listing.html'
--- src/maasserver/templates/maasserver/cluster_listing.html	2014-09-04 10:32:51 +0000
+++ src/maasserver/templates/maasserver/cluster_listing.html	2014-09-11 12:52:50 +0000
@@ -14,7 +14,7 @@
   <tbody>
     {% for cluster in cluster_list %}
       {% cycle 'even' 'odd' as cycle silent %}
-      {% include "maasserver/cluster_listing_row.html" with cycle=cycle warning_no_images=warn_no_images %}
+      {% include "maasserver/cluster_listing_row.html" with cycle=cycle display_warnings=display_warnings %}
     {% endfor %}
   </tbody>
 </table>

=== modified file 'src/maasserver/templates/maasserver/cluster_listing_head.html'
--- src/maasserver/templates/maasserver/cluster_listing_head.html	2014-04-02 13:53:19 +0000
+++ src/maasserver/templates/maasserver/cluster_listing_head.html	2014-09-11 12:52:50 +0000
@@ -1,6 +1,7 @@
 <thead>
   <tr>
     <th>Name</th>
+    <th><span title="Is the region connected to this cluster?">Connected</span></th>
     <th>Managed interfaces</th>
     <th>Boot images</th>
     <th>Nodes</th>

=== modified file 'src/maasserver/templates/maasserver/cluster_listing_row.html'
--- src/maasserver/templates/maasserver/cluster_listing_row.html	2014-04-02 13:53:19 +0000
+++ src/maasserver/templates/maasserver/cluster_listing_row.html	2014-09-11 12:52:50 +0000
@@ -1,13 +1,28 @@
-<tr class="cluster {{ cycle }} {% if warning_no_images and cluster.bootimage_set.count == 0 %}warning{% endif %}"
+  <tr class="cluster {{ cycle }}
+                     {% if display_warnings %}
+                       {% if cluster.bootimage_set.count == 0 or not cluster.is_connected %}
+                         warning
+                       {% endif %}
+                     {% endif %}"
     id="{{ cluster.uuid }}">
   <td>
     <a href="{% url 'cluster-edit' cluster.uuid %}">{{ cluster.cluster_name }}</a>
   </td>
+  <td id="{{ cluster.uuid }}_connection">
+  {% if cluster.is_connected %}
+    &check;
+  {% else %}
+    &cross;
+    {% if display_warnings%}
+      <img src="{{ STATIC_URL }}img/warning.png" title="Warning: this cluster isn't connected."/>
+    {% endif %}
+  {% endif %}
+  </td>
   <td>
   {{ cluster.get_managed_interfaces|length }}
   </td>
   <td>
-  {% if warning_no_images and cluster.bootimage_set.count == 0 %}
+  {% if display_warnings and cluster.bootimage_set.count == 0 %}
     0 <img src="{{ STATIC_URL }}img/warning.png" title="Warning: this cluster has no boot images."/>
   {% else %}
     {% if cluster.bootimage_set.count == 0 %}

=== modified file 'src/maasserver/views/clusters.py'
--- src/maasserver/views/clusters.py	2014-08-26 18:01:08 +0000
+++ src/maasserver/views/clusters.py	2014-09-11 12:52:50 +0000
@@ -109,9 +109,9 @@
         context['current_count'] = NodeGroup.objects.filter(
             status=self.status).count()
         context['title'] = self.make_cluster_listing_title()
-        # Display warnings for clusters that have no images, but only for the
-        # display of 'accepted' clusters.
-        context['warn_no_images'] = self.status == NODEGROUP_STATUS.ACCEPTED
+        # Display warnings (no images, cluster not connected) for clusters,
+        # but only for the display of 'accepted' clusters.
+        context['display_warnings'] = self.status == NODEGROUP_STATUS.ACCEPTED
         context['status'] = self.status
         context['statuses'] = NODEGROUP_STATUS
         context['status_name'] = NODEGROUP_STATUS_CHOICES[self.status][1]

=== modified file 'src/maasserver/views/tests/test_clusters.py'
--- src/maasserver/views/tests/test_clusters.py	2014-09-05 16:38:32 +0000
+++ src/maasserver/views/tests/test_clusters.py	2014-09-11 12:52:50 +0000
@@ -15,6 +15,7 @@
 __all__ = []
 
 import httplib
+import random
 
 from django.core.urlresolvers import reverse
 from lxml.html import fromstring
@@ -143,6 +144,41 @@
             HasLength(1),
             "Couldn't find pagination tag.")
 
+    def test_listing_displays_connection_status(self):
+        # We're using a little trick here: we create nodegroups
+        # whose name end with either '0' or '1' and we then
+        # patch nodegroup.is_connected so that it returns
+        # True or False based of the name of the nodegroup.
+        self.client_log_in(as_admin=True)
+        nodegroup_connections = {}
+        connection_representation = {
+            '&check;': True,
+            '&cross;': False,
+        }
+
+        def mock_is_connected(self):
+            # Return a boolean based on the last character in the
+            # nodegroup's name.
+            return True if self.name.endswith('0') else False
+        self.patch(NodeGroup, 'is_connected', mock_is_connected)
+
+        for _ in range(3):
+            # Create a name with a random value of '0' or '1' at the end.
+            name = "nodegroup-%s-%s" % (
+                factory.make_name(), random.choice(['0', '1']))
+            nodegroup = factory.make_NodeGroup(
+                status=self.status, name=name)
+            nodegroup_connections[nodegroup.uuid] = nodegroup.is_connected()
+        response = self.client.get(self.get_url())
+        document = fromstring(response.content)
+        connection_displayed = {}
+        for uuid in nodegroup_connections:
+            connection_row = document.xpath(
+                "//td[@id='%s_connection']" % uuid)[0]
+            connection_displayed[uuid] = connection_representation[
+                connection_row.text.strip()]
+        self.assertEqual(nodegroup_connections, connection_displayed)
+
 
 class ClusterListingAccess(MAASServerTestCase):
 
@@ -226,8 +262,29 @@
         warning_elems = (
             nodegroup_row.xpath(
                 "//img[@title='Warning: this cluster has no boot images.']"))
-        self.assertEqual(
-            1, len(warning_elems), "No warning about missing boot images.")
+        self.assertThat(
+            warning_elems, HasLength(1),
+            "No warning about missing boot images.")
+
+    def test_warning_is_displayed_if_a_cluster_is_not_connected(self):
+        self.client_log_in(as_admin=True)
+        nodegroup = factory.make_NodeGroup(
+            status=NODEGROUP_STATUS.ACCEPTED)
+        # Create a boot image so that the warning about the missing boot images
+        # isn't displayed.
+        factory.make_BootImage(nodegroup=nodegroup)
+
+        self.patch(NodeGroup, 'is_connected', lambda _: False)
+        response = self.client.get(reverse('cluster-list'))
+        document = fromstring(response.content)
+        nodegroup_row = document.xpath("//tr[@id='%s']" % nodegroup.uuid)[0]
+        self.assertIn('warning', nodegroup_row.get('class'))
+        warning_elems = (
+            nodegroup_row.xpath(
+                """//img[@title="Warning: this cluster isn't connected."]"""))
+        self.assertThat(
+            warning_elems, HasLength(1),
+            "No warning about disconnected cluster.")
 
 
 class ClusterDeleteTest(MAASServerTestCase):