Merge lp:~julian-edwards/maas/cluster-interfaces-as-networks into lp:~maas-committers/maas/trunk

[Julian Edwards]

Copy any network information from cluster interface edits into the Network table itself.

Pre-imped with jtv.

First part of a few branches to make a change that will list on which network each Node's MAC resides.

This simply copies network info from a cluster into the Network model itself, creating a new model as necessary, whose name is <nodegroup name>-<interface name> and the vlan_tag set as appropriate where it was present in the interface name.

At some point, we can reference the network model itself and remove the duplicate fields on the cluster interface. (I've not done it here because it would be a large change involving complex form stuff that I need to get advice on first.)

Subsequent branches will add a data migration to populate networks based on the cluster interfaces, and a web UI and API change to show the networks next to the MACs.

[Jeroen T. Vermeulen (jtv)]

There are some finicky details here. I left comments inline; some of them I think really need addressing before landing.

review: Approve

[Julian Edwards (julian-edwards)]

On Friday 08 Aug 2014 05:32:16 you wrote:
> Review: Approve
>
> There are some finicky details here. I left comments inline; some of them I
> think really need addressing before landing.

Finicky is good sometimes, especially with networking! Thank you for the
thoughtful review, as ever.

> > + def save(self, *args, **kwargs):
> Can you document very briefly what unusual thing this method does? Saves
> the reader having to go through the code.

Done, well spotted.

> > + try:
> > + network.save()
> > + except ValidationError:
> > + # It already exists, keep calm and carry on.
>
> It may already exist, or you may have hit a difference between cluster
> interface validation and network validation, or there may be a bug which
> now leaves no trace. If there is no separate exception type for this, and
> you can't make an explicit call to just the uniqueness check, then at least
> log the error.

I've added the logging, thanks for calling this out.

> > + name = cluster_interface.interface
> > + nodegroup_name = cluster_interface.nodegroup.name
> > + vlan_tag = None
> > + if '.' in name:
> > + name, vlan_tag = name.split('.', 1)
>
> Won't this generate duplicate names for e.g. eth0 and eth0.1? The form's
> blanket amnesty for ValidationErrors might hide the problem, but AFAICT one
> of the networks would simply not appear.
>
> ISTM you need to replace the dot, not strip off the vlan suffix.

Well spotted! In fact, with some more stringent testing I added later, this
actually got caught.

> > +class TestMangleNameFromClusterInterface(MAASServerTestCase):
> The test case's name no longer matches that of the function it tests.

Argh. I renamed the damn function with the best of intentions as well!

> > + def test_returns_name_unaltered(self):
> Sometimes it does, yes. Could you make the condition under which this
> happens a bit more explicit, e.g. "straightforward name"?

Done. "Simple name".

> > + interface = sentinel.interface
> > + interface.nodegroup = sentinel.nodegroup
> > + interface.nodegroup.name = factory.make_name('name')
> > + interface.interface = 'eth0'
>
> Consider extracting these 4 lines (which recur in each of these tests) into
> a helper.

Done.

> > + def test_returns_with_colon_substituted(self):
> Maybe just "substitutes colon" instead of "returns with colon substituted"?

Done.

> All these tests use "eth0" as the base network interface. Not that that's
> particularly dangerous here, but it's a bad habit: some maniac could decide
> to take the base interface name from the operating system, or hard-code
> "eth0" for experimental changes, or assume that any colon or dot always
> comes after exactly 4 characters, without breaking the tests. There have
> to be at least some tests to show that you get the right value regardless
> of what that value actually is.

Right. I've changed the "simple" name test to use a random string for the
name.

> > +class TestNodeGroupInterfaceFormNetworkCreation(MAASServerTestCase):
> > + """Tests for when NodeGroupInterfaceForm creates a Network."""
>
> Given that lack of error detail ...

[MAAS Lander (maas-lander)]

The attempt to merge lp:~julian-edwards/maas/cluster-interfaces-as-networks 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
Ign http://nova.clouds.archive.ubuntu.com trusty InRelease
Hit http://security.ubuntu.com trusty-security Release
Ign http://nova.clouds.archive.ubuntu.com trusty-updates InRelease
Hit http://nova.clouds.archive.ubuntu.com trusty Release.gpg
Get:1 http://nova.clouds.archive.ubuntu.com trusty-updates Release.gpg [933 B]
Hit http://nova.clouds.archive.ubuntu.com trusty Release
Hit http://security.ubuntu.com trusty-security/main Sources
Get:2 http://nova.clouds.archive.ubuntu.com trusty-updates Release [59.7 kB]
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
Get:3 http://nova.clouds.archive.ubuntu.com trusty-updates/main Sources [106 kB]
Get:4 http://nova.clouds.archive.ubuntu.com trusty-updates/universe Sources [67.1 kB]
Get:5 http://nova.clouds.archive.ubuntu.com trusty-updates/main amd64 Packages [285 kB]
Get:6 http://nova.clouds.archive.ubuntu.com trusty-updates/universe amd64 Packages [168 kB]
Get:7 http://nova.clouds.archive.ubuntu.com trusty-updates/main Translation-en [125 kB]
Get:8 http://nova.clouds.archive.ubuntu.com trusty-updates/universe Translation-en [82.7 kB]
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 894 kB in 0s (1,468 kB/s)
Reading package lists...
sudo DEBIAN_FRONTEND=noninteractive apt-get -y \
     --no-install-recommends install apache2 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 postgresql 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-formencode python-hivex python-httplib2 python-jinja2 python-jsonschema python-lockfile python-lxml python-netaddr python-netifaces 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-simplestreams p...

=== modified file 'src/maasserver/forms.py'
--- src/maasserver/forms.py	2014-08-04 16:18:21 +0000
+++ src/maasserver/forms.py	2014-08-11 06:31:29 +0000
@@ -111,6 +111,7 @@
     Tag,
     Zone,
     )
+from maasserver.models.network import get_name_and_vlan_from_cluster_interface
 from maasserver.models.node import (
     fqdn_is_duplicate,
     nodegroup_fqdn,
@@ -137,8 +138,11 @@
 from metadataserver.fields import Bin
 from metadataserver.models import CommissioningScript
 from provisioningserver.drivers.osystem import OperatingSystemRegistry
+from provisioningserver.logger import get_maas_logger
 from provisioningserver.utils.network import make_network
 
+maaslog = get_maas_logger()
+
 # A reusable null-option for choice fields.
 BLANK_CHOICE = ('', '-------')
 
@@ -1176,6 +1180,34 @@
             'static_ip_range_high',
             )
 
+    def save(self, *args, **kwargs):
+        """Override `ModelForm`.save() so that the network data is copied
+        to a `Network` instance."""
+        interface = super(NodeGroupInterfaceForm, self).save(*args, **kwargs)
+        if interface.network is None:
+            return interface
+        name, vlan_tag = get_name_and_vlan_from_cluster_interface(interface)
+        network = Network(
+            name=name,
+            ip=unicode(interface.network.network),
+            netmask=unicode(interface.network.netmask),
+            vlan_tag=vlan_tag,
+            # I bloody hate the damn linter. It actually prefers this.
+            description="Auto created when creating interface %s on "
+                        "cluster %s" % (
+                            interface.name, interface.nodegroup.name),
+            )
+        try:
+            network.save()
+        except ValidationError as e:
+            # It probably already exists, keep calm and carry on.
+            maaslog.warning(
+                "Failed to create Network when adding/editing cluster "
+                "interface %s with error [%s]. This is OK if it already "
+                "exists, or it could be another error" % (name, unicode(e)))
+            pass
+        return interface
+
     def compute_name(self):
         """Return the value the `name` field should have.
 

=== modified file 'src/maasserver/models/network.py'
--- src/maasserver/models/network.py	2014-07-08 07:33:43 +0000
+++ src/maasserver/models/network.py	2014-08-11 06:31:29 +0000
@@ -186,6 +186,22 @@
     return specifier_class(spec)
 
 
+def get_name_and_vlan_from_cluster_interface(cluster_interface):
+    """Given a `NodeGroupInterface`, return a name suitable for a `Network`.
+
+    :return: a tuple of the new name and vlan tag. vlan tag may be None
+    """
+    name = cluster_interface.interface
+    nodegroup_name = cluster_interface.nodegroup.name
+    vlan_tag = None
+    if '.' in name:
+        _, vlan_tag = name.split('.', 1)
+        name = name.replace('.', '-')
+    name = name.replace(':', '-')
+    network_name = "-".join((nodegroup_name, name))
+    return network_name, vlan_tag
+
+
 class NetworkManager(Manager):
     """Manager for :class:`Network` model class.
 

=== modified file 'src/maasserver/models/tests/test_network.py'
--- src/maasserver/models/tests/test_network.py	2014-07-16 14:12:13 +0000
+++ src/maasserver/models/tests/test_network.py	2014-08-11 06:31:29 +0000
@@ -22,12 +22,14 @@
 from maasserver.models.network import (
     get_specifier_type,
     IPSpecifier,
+    get_name_and_vlan_from_cluster_interface,
     NameSpecifier,
     parse_network_spec,
     VLANSpecifier,
     )
 from maasserver.testing.factory import factory
 from maasserver.testing.testcase import MAASServerTestCase
+from mock import sentinel
 from netaddr import (
     IPAddress,
     IPNetwork,
@@ -129,6 +131,43 @@
         self.assertRaises(ValidationError, parse_network_spec, '10.4.4.4')
 
 
+class TestGetNameAndVlanFromClusterInterface(MAASServerTestCase):
+    """Tests for `get_name_and_vlan_from_cluster_interface`."""
+
+    def make_interface_name(self, basename):
+        interface = sentinel.interface
+        interface.nodegroup = sentinel.nodegroup
+        interface.nodegroup.name = factory.make_name('name')
+        interface.interface = basename
+        return interface
+
+    def test_returns_simple_name_unaltered(self):
+        interface = self.make_interface_name(factory.make_name('iface'))
+        name, vlan_tag = get_name_and_vlan_from_cluster_interface(interface)
+        expected_name = '%s-%s' % (
+            interface.nodegroup.name, interface.interface)
+        self.assertEqual((expected_name, None), (name, vlan_tag))
+
+    def test_substitutes_colon(self):
+        interface = self.make_interface_name('eth0:0')
+        name, vlan_tag = get_name_and_vlan_from_cluster_interface(interface)
+        expected_name = '%s-eth0-0' % interface.nodegroup.name
+        self.assertEqual((expected_name, None), (name, vlan_tag))
+
+    def test_returns_with_vlan_tag(self):
+        interface = self.make_interface_name('eth0.5')
+        name, vlan_tag = get_name_and_vlan_from_cluster_interface(interface)
+        expected_name = '%s-eth0-5' % interface.nodegroup.name
+        self.assertEqual((expected_name, '5'), (name, vlan_tag))
+
+    def test_returns_name_with_crazy_colon_and_vlan(self):
+        # For truly twisted network admins.
+        interface = self.make_interface_name('eth0:2.3')
+        name, vlan_tag = get_name_and_vlan_from_cluster_interface(interface)
+        expected_name = '%s-eth0-2-3' % interface.nodegroup.name
+        self.assertEqual((expected_name, '3'), (name, vlan_tag))
+
+
 class TestNetworkManager(MAASServerTestCase):
 
     def test_get_from_spec_validates_first(self):

=== modified file 'src/maasserver/tests/test_forms.py'
--- src/maasserver/tests/test_forms.py	2014-08-08 19:04:18 +0000
+++ src/maasserver/tests/test_forms.py	2014-08-11 06:31:29 +0000
@@ -16,6 +16,7 @@
 
 from cStringIO import StringIO
 import json
+import random
 
 from django import forms
 from django.conf import settings
@@ -93,6 +94,7 @@
     Zone,
     )
 from maasserver.models.config import DEFAULT_CONFIG
+from maasserver.models.network import get_name_and_vlan_from_cluster_interface
 from maasserver.models.staticipaddress import StaticIPAddress
 from maasserver.node_action import (
     Commission,
@@ -1052,23 +1054,24 @@
     ]
 
 
+def make_ngi_instance(nodegroup=None):
+    """Create a `NodeGroupInterface` with nothing set but `nodegroup`.
+
+    This is used by tests to instantiate the cluster interface form for
+    a given cluster.  We create an initial cluster interface object just
+    to tell it which cluster that is.
+    """
+    if nodegroup is None:
+        nodegroup = factory.make_node_group()
+    return NodeGroupInterface(nodegroup=nodegroup)
+
+
 class TestNodeGroupInterfaceForm(MAASServerTestCase):
 
-    def make_ngi_instance(self, nodegroup=None):
-        """Create a `NodeGroupInterface` with nothing set but `nodegroup`.
-
-        This is used by tests to instantiate the cluster interface form for
-        a given cluster.  We create an initial cluster interface object just
-        to tell it which cluster that is.
-        """
-        if nodegroup is None:
-            nodegroup = factory.make_node_group()
-        return NodeGroupInterface(nodegroup=nodegroup)
-
     def test__validates_parameters(self):
         form = NodeGroupInterfaceForm(
             data={'ip': factory.make_string()},
-            instance=self.make_ngi_instance())
+            instance=make_ngi_instance())
         self.assertFalse(form.is_valid())
         self.assertEquals(
             {'ip': ['Enter a valid IPv4 or IPv6 address.']}, form._errors)
@@ -1079,7 +1082,7 @@
         for field_name in nullable_fields:
             del int_settings[field_name]
         form = NodeGroupInterfaceForm(
-            data=int_settings, instance=self.make_ngi_instance())
+            data=int_settings, instance=make_ngi_instance())
         interface = form.save()
         field_values = [
             getattr(interface, field_name) for field_name in nullable_fields]
@@ -1090,7 +1093,7 @@
         int_settings = factory.get_interface_fields()
         int_settings['name'] = name
         form = NodeGroupInterfaceForm(
-            data=int_settings, instance=self.make_ngi_instance())
+            data=int_settings, instance=make_ngi_instance())
         interface = form.save()
         self.assertEqual(name, interface.name)
 
@@ -1099,7 +1102,7 @@
         int_settings['interface'] = factory.make_name('ether')
         del int_settings['name']
         form = NodeGroupInterfaceForm(
-            data=int_settings, instance=self.make_ngi_instance())
+            data=int_settings, instance=make_ngi_instance())
         interface = form.save()
         self.assertEqual(int_settings['interface'], interface.name)
 
@@ -1108,7 +1111,7 @@
         int_settings['interface'] = 'eth1+1'
         del int_settings['name']
         form = NodeGroupInterfaceForm(
-            data=int_settings, instance=self.make_ngi_instance())
+            data=int_settings, instance=make_ngi_instance())
         interface = form.save()
         self.assertEqual('eth1--1', interface.name)
 
@@ -1118,10 +1121,10 @@
         del int_settings['name']
         del int_settings['interface']
         form1 = NodeGroupInterfaceForm(
-            data=int_settings, instance=self.make_ngi_instance())
+            data=int_settings, instance=make_ngi_instance())
         interface1 = form1.save()
         form2 = NodeGroupInterfaceForm(
-            data=int_settings, instance=self.make_ngi_instance())
+            data=int_settings, instance=make_ngi_instance())
         interface2 = form2.save()
         self.assertNotIn(interface1.name, [None, ''])
         self.assertNotIn(interface2.name, [None, ''])
@@ -1134,7 +1137,7 @@
         del int_settings['name']
         int_settings['interface'] = existing_interface.name
         form = NodeGroupInterfaceForm(
-            data=int_settings, instance=self.make_ngi_instance(cluster))
+            data=int_settings, instance=make_ngi_instance(cluster))
         interface = form.save()
         self.assertThat(interface.name, StartsWith(int_settings['interface']))
         self.assertNotEqual(int_settings['interface'], interface.name)
@@ -1165,7 +1168,7 @@
         int_settings = factory.get_interface_fields(
             management=NODEGROUPINTERFACE_MANAGEMENT.DHCP_AND_DNS)
         form = NodeGroupInterfaceForm(
-            data=int_settings, instance=self.make_ngi_instance())
+            data=int_settings, instance=make_ngi_instance())
         mock = self.patch(form, "get_duplicate_fqdns")
         self.assertTrue(form.is_valid(), form.errors)
         self.assertThat(mock, MockCalledOnceWith())
@@ -1174,7 +1177,7 @@
         int_settings = factory.get_interface_fields(
             management=NODEGROUPINTERFACE_MANAGEMENT.DHCP_AND_DNS)
         form = NodeGroupInterfaceForm(
-            data=int_settings, instance=self.make_ngi_instance())
+            data=int_settings, instance=make_ngi_instance())
         mock = self.patch(form, "get_duplicate_fqdns")
         hostnames = [
             factory.make_hostname("duplicate") for _ in range(0, 3)]
@@ -1239,6 +1242,75 @@
         self.assertEqual(expected_duplicates, duplicates)
 
 
+class TestNodeGroupInterfaceFormNetworkCreation(MAASServerTestCase):
+    """Tests for when NodeGroupInterfaceForm creates a Network."""
+
+    def test_creates_network_name(self):
+        int_settings = factory.get_interface_fields()
+        int_settings['interface'] = 'eth0:1'
+        interface = make_ngi_instance()
+        form = NodeGroupInterfaceForm(data=int_settings, instance=interface)
+        form.save()
+        [network] = Network.objects.all()
+        expected, _ = get_name_and_vlan_from_cluster_interface(interface)
+        self.assertEqual(expected, network.name)
+
+    def test_sets_vlan_tag(self):
+        int_settings = factory.get_interface_fields()
+        vlan_tag = random.randint(1, 10)
+        int_settings['interface'] = 'eth0.%s' % vlan_tag
+        interface = make_ngi_instance()
+        form = NodeGroupInterfaceForm(data=int_settings, instance=interface)
+        form.save()
+        [network] = Network.objects.all()
+        self.assertEqual(vlan_tag, network.vlan_tag)
+
+    def test_vlan_tag_is_None_if_no_vlan(self):
+        int_settings = factory.get_interface_fields()
+        int_settings['interface'] = 'eth0:1'
+        interface = make_ngi_instance()
+        form = NodeGroupInterfaceForm(data=int_settings, instance=interface)
+        form.save()
+        [network] = Network.objects.all()
+        self.assertIs(None, network.vlan_tag)
+
+    def test_sets_network_values(self):
+        int_settings = factory.get_interface_fields()
+        interface = make_ngi_instance()
+        form = NodeGroupInterfaceForm(data=int_settings, instance=interface)
+        form.save()
+        [network] = Network.objects.all()
+        expected_net_address = unicode(interface.network.network)
+        expected_netmask = unicode(interface.network.netmask)
+        self.assertThat(
+            network, MatchesStructure.byEquality(
+                ip=expected_net_address,
+                netmask=expected_netmask))
+
+    def test_does_not_create_new_network_if_already_exists(self):
+        int_settings = factory.get_interface_fields()
+        interface = make_ngi_instance()
+        form = NodeGroupInterfaceForm(data=int_settings, instance=interface)
+        # The easiest way to pre-create the same network is just to save
+        # the form twice.
+        form.save()
+        [existing_network] = Network.objects.all()
+        form.save()
+        self.assertItemsEqual([existing_network], Network.objects.all())
+
+    def test_creates_many_unique_networks(self):
+        names = ('eth0', 'eth0:1', 'eth0.1', 'eth0:1.2')
+        for name in names:
+            int_settings = factory.get_interface_fields()
+            int_settings['interface'] = name
+            interface = make_ngi_instance()
+            form = NodeGroupInterfaceForm(
+                data=int_settings, instance=interface)
+            form.save()
+
+        self.assertEqual(len(names), len(Network.objects.all()))
+
+
 class TestValidateNewStaticIPRanges(MAASServerTestCase):
     """Tests for `validate_new_static_ip_ranges`()."""