Merge lp:~rvb/maas/net-name-bug-1400909 into lp:~maas-committers/maas/trunk

[Raphaël Badin]

Fix call to get_name_and_vlan_from_cluster_interface in create_Network_from_NodeGroupInterface. It was using the interface's name in lieu of the cluster's name.

I also added the tests for create_Network_from_NodeGroupInterface, it was completely untested.

Drive-by fix: copy the get_name_and_vlan_from_cluster_interface utility in the migration which uses it. It's generally a bad idea to use utilities from migrations: the code might have to be changed but the version used by the migration needs to work with the old code.

[Blake Rouse (blake-rouse)]

Nice catch with the migration.

Looks good.

review: Approve

[Raphaël Badin (rvb)]

Thanks for the review!

[MAAS Lander (maas-lander)]

The attempt to merge lp:~rvb/maas/net-name-bug-1400909 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
Hit http://security.ubuntu.com trusty-security Release.gpg
Ign http://nova.clouds.archive.ubuntu.com trusty-updates InRelease
Hit http://security.ubuntu.com trusty-security Release
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://nova.clouds.archive.ubuntu.com trusty/main Sources
Hit http://security.ubuntu.com trusty-security/main amd64 Packages
Hit http://nova.clouds.archive.ubuntu.com trusty/universe Sources
Hit http://security.ubuntu.com trusty-security/universe amd64 Packages
Hit http://nova.clouds.archive.ubuntu.com trusty/main amd64 Packages
Hit http://nova.clouds.archive.ubuntu.com trusty/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 Translation-en
Hit http://nova.clouds.archive.ubuntu.com trusty/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
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
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 python-openssl python-paramiko python-pexpect python-pip python-pocket-lint python-psycopg2 python-pyinotify python-seamicroclient python-simplejson python-simplestreams python-sphinx py...

=== modified file 'src/maasserver/forms.py'
--- src/maasserver/forms.py	2014-12-10 21:16:37 +0000
+++ src/maasserver/forms.py	2014-12-11 15:14:43 +0000
@@ -1421,14 +1421,12 @@
 
 def create_Network_from_NodeGroupInterface(interface):
     """Given a `NodeGroupInterface`, create its Network counterpart."""
-    # This method cannot use non-orm model properties because it needs
-    # to be used in a data migration, where they won't work.
     if not interface.subnet_mask:
         # Can be None or empty string, do nothing if so.
         return
 
     name, vlan_tag = get_name_and_vlan_from_cluster_interface(
-        interface.name, interface.interface)
+        interface.nodegroup.name, interface.interface)
     ipnetwork = make_network(interface.ip, interface.subnet_mask)
     network = Network(
         name=name,

=== modified file 'src/maasserver/migrations/0099_convert_cluster_interfaces_to_networks.py'
--- src/maasserver/migrations/0099_convert_cluster_interfaces_to_networks.py	2014-11-13 19:43:11 +0000
+++ src/maasserver/migrations/0099_convert_cluster_interfaces_to_networks.py	2014-12-11 15:14:43 +0000
@@ -5,14 +5,32 @@
     transaction,
     )
 from django.db.utils import IntegrityError
-from maasserver.utils.interfaces import (
-    get_name_and_vlan_from_cluster_interface,
-    )
 from netaddr import IPNetwork
 from south.db import db
 from south.utils import datetime_utils as datetime
 from south.v2 import DataMigration
 
+# "Frozen" method from maasserver.utils.interfaces.
+def get_name_and_vlan_from_cluster_interface(cluster_name, interface):
+    """Return a name suitable for a `Network` managed by a cluster interface.
+
+    :param interface: Network interface name, e.g. `eth0:1`.
+    :param cluster_name: Name of the cluster.
+    :return: a tuple of the new name and the interface's VLAN tag.  The VLAN
+        tag may be None.
+    """
+    name = interface
+    vlan_tag = None
+    if '.' in name:
+        _, vlan_tag = name.split('.', 1)
+        if ':' in vlan_tag:
+            # Nasty: there's an alias after the VLAN tag.
+            vlan_tag, _ = vlan_tag.split(':', 1)
+        name = name.replace('.', '-')
+    name = name.replace(':', '-')
+    network_name = "-".join((cluster_name, name))
+    return network_name, vlan_tag
+
 
 class Migration(DataMigration):
 

=== modified file 'src/maasserver/tests/test_forms_network.py'
--- src/maasserver/tests/test_forms_network.py	2014-11-25 14:24:38 +0000
+++ src/maasserver/tests/test_forms_network.py	2014-12-11 15:14:43 +0000
@@ -14,8 +14,13 @@
 __metaclass__ = type
 __all__ = []
 
+from maasserver import forms as forms_module
 from maasserver.dns import config as dns_config_module
-from maasserver.forms import NetworkForm
+from maasserver.enum import NODEGROUPINTERFACE_MANAGEMENT
+from maasserver.forms import (
+    create_Network_from_NodeGroupInterface,
+    NetworkForm,
+    )
 from maasserver.models import (
     MACAddress,
     Network,
@@ -25,6 +30,7 @@
 from maasserver.testing.testcase import MAASServerTestCase
 from maastesting.matchers import MockCalledOnceWith
 from netaddr import IPNetwork
+from testtools.matchers import Contains
 
 
 class TestNetworkForm(MAASServerTestCase):
@@ -196,3 +202,67 @@
             dns_config_module, "write_full_dns_config")
         network.delete()
         self.assertThat(write_full_dns_config, MockCalledOnceWith())
+
+
+class TestCreateNetworkFromNodeGroupInterface(MAASServerTestCase):
+
+    def test_skips_creation_if_netmask_undefined(self):
+        nodegroup = factory.make_NodeGroup()
+        interface = factory.make_NodeGroupInterface(
+            nodegroup, management=NODEGROUPINTERFACE_MANAGEMENT.UNMANAGED)
+        interface.subnet_mask = None
+        network = create_Network_from_NodeGroupInterface(interface)
+        self.assertIsNone(network)
+        self.assertItemsEqual([], Network.objects.all())
+
+    def test_creates_network_without_vlan(self):
+        nodegroup = factory.make_NodeGroup()
+        interface = factory.make_NodeGroupInterface(nodegroup)
+        network = create_Network_from_NodeGroupInterface(interface)
+        definition = {
+            'name': "%s-%s" % (
+                interface.nodegroup.name, interface.interface),
+            'description': (
+                "Auto created when creating interface %s on cluster %s" % (
+                    interface.name, interface.nodegroup.name)),
+            'ip': "%s" % interface.network.ip,
+            'netmask': "%s" % interface.network.netmask,
+            'vlan_tag': None,
+        }
+        network_obj = Network.objects.get(id=network.id)
+        self.assertAttributes(network_obj, definition)
+
+    def test_creates_network_with_vlan(self):
+        nodegroup = factory.make_NodeGroup()
+        intf = 'eth0'
+        vlan = 1
+        interface = factory.make_NodeGroupInterface(
+            nodegroup, interface="%s.%d" % (intf, vlan))
+        network = create_Network_from_NodeGroupInterface(interface)
+        net_name = "%s-%s" % (interface.nodegroup.name, interface.interface)
+        net_name = net_name.replace('.', '-')
+        definition = {
+            'name': net_name,
+            'description': (
+                "Auto created when creating interface %s on cluster %s" % (
+                    interface.name, interface.nodegroup.name)),
+            'ip': "%s" % interface.network.ip,
+            'netmask': "%s" % interface.network.netmask,
+            'vlan_tag': vlan,
+        }
+        network_obj = Network.objects.get(id=network.id)
+        self.assertAttributes(network_obj, definition)
+
+    def test_skips_creation_if_network_already_exists(self):
+        nodegroup = factory.make_NodeGroup()
+        interface = factory.make_NodeGroupInterface(nodegroup)
+        create_Network_from_NodeGroupInterface(interface)
+        maaslog = self.patch(forms_module, 'maaslog')
+
+        self.assertIsNone(create_Network_from_NodeGroupInterface(interface))
+        self.assertEqual(
+            1, maaslog.warning.call_count,
+            "maaslog.warning hasn't been called")
+        self.assertThat(
+            maaslog.warning.call_args[0][0],
+            Contains("Failed to create Network"))

=== modified file 'src/maasserver/tests/test_forms_nodegroupinterface.py'
--- src/maasserver/tests/test_forms_nodegroupinterface.py	2014-11-13 19:43:11 +0000
+++ src/maasserver/tests/test_forms_nodegroupinterface.py	2014-12-11 15:14:43 +0000
@@ -372,7 +372,7 @@
         form.save()
         [network] = Network.objects.all()
         expected, _ = get_name_and_vlan_from_cluster_interface(
-            interface.name, interface.interface)
+            interface.nodegroup.name, interface.interface)
         self.assertEqual(expected, network.name)
 
     def test_sets_vlan_tag(self):