Merge lp:~jtv/maas/unify-get_name_and_vlan_from_cluster_interface into lp:~maas-committers/maas/trunk

[Jeroen T. Vermeulen]

Unify our two implementations of get_name_and_vlan_from_cluster_interface, so that we can then fix bug 1391139 in one place, with tests.

This was harder than I expected: there were some other helpers that needed moving. Fixing the actual bug should be easy by comparison.

Along the way I made the function independent of the model, otherwise it wouldn't be advisable to use it in migration code. It was a simple matter of externalising retrieval of two fields, and making those the parameters. But otherwise, nothing substantial changes.

Jeroen

[Gavin Panella (allenap)]

Looks fine. I don't like FakeRandInt much, but I don't have a better suggestion offhand, and you've only moved it anyway.

review: Approve

[Jeroen T. Vermeulen (jtv)]

Thanks. Made some changes "inspired by" your notes, as they say in Hollywood nowadays. :-)

=== modified file 'src/maasserver/forms.py'
--- src/maasserver/forms.py	2014-11-13 02:17:06 +0000
+++ src/maasserver/forms.py	2014-11-14 14:24:50 +0000
@@ -125,7 +125,6 @@
     Tag,
     Zone,
     )
-from maasserver.models.network import get_name_and_vlan_from_cluster_interface
 from maasserver.models.node import (
     fqdn_is_duplicate,
     nodegroup_fqdn,
@@ -138,7 +137,10 @@
     )
 from maasserver.utils import strip_domain
 from maasserver.utils.forms import compose_invalid_choice_text
-from maasserver.utils.interfaces import make_name_from_interface
+from maasserver.utils.interfaces import (
+    get_name_and_vlan_from_cluster_interface,
+    make_name_from_interface,
+    )
 from maasserver.utils.orm import get_one
 from maasserver.utils.osystems import (
     get_distro_series_initial,
@@ -1409,7 +1411,8 @@
         # Can be None or empty string, do nothing if so.
         return
 
-    name, vlan_tag = get_name_and_vlan_from_cluster_interface(interface)
+    name, vlan_tag = get_name_and_vlan_from_cluster_interface(
+        interface.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-08-25 02:47:21 +0000
+++ src/maasserver/migrations/0099_convert_cluster_interfaces_to_networks.py	2014-11-14 14:24:50 +0000
@@ -5,28 +5,15 @@
     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
 
 
-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 Migration(DataMigration):
 
     def forwards(self, orm):
@@ -36,8 +23,10 @@
                 # Can be None or empty string, do nothing if so.
                 continue
 
-            name, vlan_tag = get_name_and_vlan_from_cluster_interface(interface)
-            ipnetwork = IPNetwork("%s/%s" % (interface.ip, interface.subnet_mask))
+            name, vlan_tag = get_name_and_vlan_from_cluster_interface(
+                interface.nodegroup.name, interface.interface)
+            ipnetwork = IPNetwork(
+                "%s/%s" % (interface.ip, interface.subnet_mask))
             network = orm['maasserver.Network'](
                 name=name,
                 ip=unicode(ipnetwork.network),
 
=== modified file 'src/maasserver/models/network.py'
--- src/maasserver/models/network.py	2014-09-17 09:20:05 +0000
+++ src/maasserver/models/network.py	2014-11-14 14:24:50 +0000
@@ -186,22 +186,6 @@
     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-09-19 03:12:47 +0000
+++ src/maasserver/models/tests/test_network.py	2014-11-14 14:24:50 +0000
@@ -20,7 +20,6 @@
 from django.db.models.query import QuerySet
 from maasserver.models import Network
 from maasserver.models.network import (
-    get_name_and_vlan_from_cluster_interface,
     get_specifier_type,
     IPSpecifier,
     NameSpecifier,
@@ -30,7 +29,6 @@
 from maasserver.testing.factory import factory
 from maasserver.testing.orm import reload_object
 from maasserver.testing.testcase import MAASServerTestCase
-from mock import sentinel
 from netaddr import (
     IPAddress,
     IPNetwork,
@@ -132,43 +130,6 @@
         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/testing/factory.py'
--- src/maasserver/testing/factory.py	2014-11-13 02:17:06 +0000
+++ src/maasserver/testing/factory.py	2014-11-14 14:24:50 +0000
@@ -748,29 +748,6 @@
 
     make_zone = make_Zone
 
-    def make_vlan_tag(self, allow_none=False, but_not=None):
-        """Create a random VLAN tag.
-
-        :param allow_none: Whether `None` ("no VLAN") can be allowed as an
-            outcome.  If `True`, `None` will be included in the possible
-            results with a deliberately over-represented probability, in order
-            to help trip up bugs that might only show up once in about 4094
-            calls otherwise.
-        :param but_not: A list of tags that should not be returned.  Any zero
-            or `None` entries will be ignored.
-        """
-        if but_not is None:
-            but_not = []
-        if allow_none and random.randint(0, 1) == 0:
-            return None
-        else:
-            for _ in range(100):
-                vlan_tag = random.randint(1, 0xffe)
-                if vlan_tag not in but_not:
-                    return vlan_tag
-            raise maastesting.factory.TooManyRandomRetries(
-                "Could not find an available VLAN tag.")
-
     def make_Network(self, name=None, network=None, vlan_tag=NO_VALUE,
                      description=None, sortable_name=False,
                      disjoint_from=None, default_gateway=None,
 
=== modified file 'src/maasserver/testing/tests/test_factory.py'
--- src/maasserver/testing/tests/test_factory.py	2014-09-10 16:20:31 +0000
+++ src/maasserver/testing/tests/test_factory.py	2014-11-14 14:24:50 +0000
@@ -24,28 +24,7 @@
 from maasserver.testing.orm import reload_object
 from maasserver.testing.testcase import MAASServerTestCase
 from maastesting.factory import TooManyRandomRetries
-
-
-class FakeRandInt:
-    """Fake `randint` which forced limitations on its range.
-
-    This lets you set a forced minimum, and/or a forced maximum, on the range
-    of any call.  For example, if you pass `forced_maximum=3`, then a call
-    will never return more than 3.  If you don't set a maximum, or if the
-    call's maximum argument is less than the forced maximum, then the call's
-    maximum will be respected.
-    """
-    def __init__(self, real_randint, forced_minimum=None, forced_maximum=None):
-        self.real_randint = real_randint
-        self.minimum = forced_minimum
-        self.maximum = forced_maximum
-
-    def __call__(self, minimum, maximum):
-        if self.minimum is not None:
-            minimum = max(minimum, self.minimum)
-        if self.maximum is not None:
-            maximum = min(maximum, self.maximum)
-        return self.real_randint(minimum, maximum)
+from maastesting.utils import FakeRandInt
 
 
 class TestFactory(MAASServerTestCase):
@@ -127,23 +106,6 @@
         node = reload_object(node)
         self.assertEqual(previous_zone, node.zone)
 
-    def test_make_vlan_tag_excludes_None_by_default(self):
-        # Artificially limit randint to a very narrow range, to guarantee
-        # some repetition in its output, and virtually guarantee that we test
-        # both outcomes of the flip-a-coin call in make_vlan_tag.
-        self.patch(random, 'randint', FakeRandInt(random.randint, 0, 1))
-        outcomes = {factory.make_vlan_tag() for _ in range(1000)}
-        self.assertEqual({1}, outcomes)
-
-    def test_make_vlan_tags_includes_None_if_allow_none(self):
-        self.patch(random, 'randint', FakeRandInt(random.randint, 0, 1))
-        self.assertEqual(
-            {None, 1},
-            {
-                factory.make_vlan_tag(allow_none=True)
-                for _ in range(1000)
-            })
-
     def test_make_Networks_lowers_names_if_sortable_name(self):
         networks = factory.make_Networks(10, sortable_name=True)
         self.assertEqual(
@@ -183,4 +145,5 @@
     def test_make_Networks_gives_up_if_random_tags_keep_clashing(self):
         self.patch(factory, 'make_Network')
         self.patch(random, 'randint', lambda *args: 1)
+        self.patch(factory, 'pick_bool', lambda *args: False)
         self.assertRaises(TooManyRandomRetries, factory.make_Networks, 2)
 
=== modified file 'src/maasserver/tests/test_forms_nodegroupinterface.py'
--- src/maasserver/tests/test_forms_nodegroupinterface.py	2014-10-30 11:08:43 +0000
+++ src/maasserver/tests/test_forms_nodegroupinterface.py	2014-11-14 14:24:50 +0000
@@ -29,10 +29,12 @@
     Network,
     NodeGroupInterface,
     )
-from maasserver.models.network import get_name_and_vlan_from_cluster_interface
 from maasserver.models.staticipaddress import StaticIPAddress
 from maasserver.testing.factory import factory
 from maasserver.testing.testcase import MAASServerTestCase
+from maasserver.utils.interfaces import (
+    get_name_and_vlan_from_cluster_interface,
+    )
 from maastesting.matchers import MockCalledOnceWith
 from netaddr import (
     IPAddress,
@@ -369,7 +371,8 @@
         form = NodeGroupInterfaceForm(data=int_settings, instance=interface)
         form.save()
         [network] = Network.objects.all()
-        expected, _ = get_name_and_vlan_from_cluster_interface(interface)
+        expected, _ = get_name_and_vlan_from_cluster_interface(
+            interface.name, interface.interface)
         self.assertEqual(expected, network.name)
 
     def test_sets_vlan_tag(self):
 
=== modified file 'src/maasserver/utils/interfaces.py'
--- src/maasserver/utils/interfaces.py	2014-08-13 21:49:35 +0000
+++ src/maasserver/utils/interfaces.py	2014-11-14 14:24:50 +0000
@@ -13,6 +13,7 @@
 
 __metaclass__ = type
 __all__ = [
+    'get_name_and_vlan_from_cluster_interface',
     'make_name_from_interface',
     ]
 
@@ -34,3 +35,21 @@
     else:
         base_name = interface
     return re.sub(u'[^\w:.-]', '--', base_name)
+
+
+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)
+        name = name.replace('.', '-')
+    name = name.replace(':', '-')
+    network_name = "-".join((cluster_name, name))
+    return network_name, vlan_tag
 
=== modified file 'src/maasserver/utils/tests/test_interfaces.py'
--- src/maasserver/utils/tests/test_interfaces.py	2014-07-04 12:10:01 +0000
+++ src/maasserver/utils/tests/test_interfaces.py	2014-11-14 14:24:50 +0000
@@ -14,7 +14,12 @@
 __metaclass__ = type
 __all__ = []
 
-from maasserver.utils.interfaces import make_name_from_interface
+from random import randint
+
+from maasserver.utils.interfaces import (
+    get_name_and_vlan_from_cluster_interface,
+    make_name_from_interface,
+    )
 from maastesting.factory import factory
 from maastesting.testcase import MAASTestCase
 
@@ -38,3 +43,56 @@
         self.assertNotEqual(
             make_name_from_interface(''),
             make_name_from_interface(''))
+
+
+class TestGetNameAndVlanFromClusterInterface(MAASTestCase):
+    """Tests for `get_name_and_vlan_from_cluster_interface`."""
+
+    def make_interface(self):
+        """Return a simple network interface name."""
+        return 'eth%d' % randint(0, 99)
+
+    def test_returns_simple_name_unaltered(self):
+        cluster = factory.make_name('cluster')
+        interface = factory.make_name('iface')
+        expected_name = '%s-%s' % (cluster, interface)
+        self.assertEqual(
+            (expected_name, None),
+            get_name_and_vlan_from_cluster_interface(cluster, interface))
+
+    def test_substitutes_colon(self):
+        cluster = factory.make_name('cluster')
+        base_interface = self.make_interface()
+        alias = randint(0, 99)
+        interface = '%s:%d' % (base_interface, alias)
+        expected_name = '%s-%s-%d' % (cluster, base_interface, alias)
+        self.assertEqual(
+            (expected_name, None),
+            get_name_and_vlan_from_cluster_interface(cluster, interface))
+
+    def test_returns_with_vlan_tag(self):
+        cluster = factory.make_name('cluster')
+        base_interface = self.make_interface()
+        vlan_tag = factory.make_vlan_tag()
+        interface = '%s.%d' % (base_interface, vlan_tag)
+        expected_name = '%s-%s-%d' % (cluster, base_interface, vlan_tag)
+        self.assertEqual(
+            (expected_name, '%d' % vlan_tag),
+            get_name_and_vlan_from_cluster_interface(cluster, interface))
+
+    def test_returns_name_with_crazy_colon_and_vlan(self):
+        # For truly twisted network admins.
+        cluster = factory.make_name('cluster')
+        base_interface = self.make_interface()
+        vlan_tag = factory.make_vlan_tag()
+        alias = randint(0, 99)
+        interface = '%s:%d.%d' % (base_interface, alias, vlan_tag)
+        expected_name = '%s-%s-%d-%d' % (
+            cluster,
+            base_interface,
+            alias,
+            vlan_tag,
+            )
+        self.assertEqual(
+            (expected_name, '%d' % vlan_tag),
+            get_name_and_vlan_from_cluster_interface(cluster, interface))
 
=== modified file 'src/maastesting/factory.py'
--- src/maastesting/factory.py	2014-11-07 16:11:53 +0000
+++ src/maastesting/factory.py	2014-11-14 14:24:50 +0000
@@ -133,6 +133,28 @@
         assert port_min >= 0 and port_max <= 65535
         return random.randint(port_min, port_max)
 
+    def make_vlan_tag(self, allow_none=False, but_not=None):
+        """Create a random VLAN tag.
+
+        :param allow_none: Whether `None` ("no VLAN") can be allowed as an
+            outcome.  If `True`, `None` will be included in the possible
+            results with a deliberately over-represented probability, in order
+            to help trip up bugs that might only show up once in about 4094
+            calls otherwise.
+        :param but_not: A list of tags that should not be returned.  Any zero
+            or `None` entries will be ignored.
+        """
+        if but_not is None:
+            but_not = []
+        if allow_none and self.pick_bool():
+            return None
+        else:
+            for _ in range(100):
+                vlan_tag = random.randint(1, 0xffe)
+                if vlan_tag not in but_not:
+                    return vlan_tag
+            raise TooManyRandomRetries("Could not find an available VLAN tag.")
+
     def make_ipv4_address(self):
         octets = islice(self.random_octets, 4)
         return '%d.%d.%d.%d' % tuple(octets)
 
=== modified file 'src/maastesting/tests/test_factory.py'
--- src/maastesting/tests/test_factory.py	2014-09-17 08:31:08 +0000
+++ src/maastesting/tests/test_factory.py	2014-11-14 14:24:50 +0000
@@ -1,4 +1,5 @@
 # Copyright 2012-2014 Canonical Ltd.  This software is licensed under the
+
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Test the factory where appropriate.  Don't overdo this."""
@@ -17,6 +18,7 @@
 from datetime import datetime
 from itertools import count
 import os.path
+import random
 from random import randint
 import subprocess
 
@@ -25,6 +27,7 @@
     TooManyRandomRetries,
     )
 from maastesting.testcase import MAASTestCase
+from maastesting.utils import FakeRandInt
 from netaddr import (
     IPAddress,
     IPNetwork,
@@ -52,6 +55,23 @@
     def test_pick_port_returns_int(self):
         self.assertIsInstance(factory.pick_port(), int)
 
+    def test_make_vlan_tag_excludes_None_by_default(self):
+        # Artificially limit randint to a very narrow range, to guarantee
+        # some repetition in its output, and virtually guarantee that we test
+        # both outcomes of the flip-a-coin call in make_vlan_tag.
+        self.patch(random, 'randint', FakeRandInt(random.randint, 0, 1))
+        outcomes = {factory.make_vlan_tag() for _ in range(1000)}
+        self.assertEqual({1}, outcomes)
+
+    def test_make_vlan_tag_includes_None_if_allow_none(self):
+        self.patch(random, 'randint', FakeRandInt(random.randint, 0, 1))
+        self.assertEqual(
+            {None, 1},
+            {
+                factory.make_vlan_tag(allow_none=True)
+                for _ in range(1000)
+            })
+
     def test_make_ipv4_address(self):
         ip_address = factory.make_ipv4_address()
         self.assertIsInstance(ip_address, unicode)
 
=== modified file 'src/maastesting/utils.py'
--- src/maastesting/utils.py	2014-08-22 13:32:56 +0000
+++ src/maastesting/utils.py	2014-11-14 14:24:50 +0000
@@ -1,4 +1,4 @@
-# Copyright 2012 Canonical Ltd.  This software is licensed under the
+# Copyright 2012-2014 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Testing utilities."""
@@ -17,6 +17,7 @@
     "content_from_file",
     "extract_word_list",
     "get_write_time",
+    "FakeRandInt",
     "preexec_fn",
     "run_isolated",
     "sample_binary_data",
@@ -130,3 +131,25 @@
 # (1) Provided, of course, that man know only about ASCII and
 # UTF.
 sample_binary_data = codecs.BOM64_LE + codecs.BOM64_BE + b'\x00\xff\x00'
+
+
+class FakeRandInt:
+    """Fake `randint` with forced limitations on its range.
+
+    This lets you set a forced minimum, and/or a forced maximum, on the range
+    of any call.  For example, if you pass `forced_maximum=3`, then a call
+    will never return more than 3.  If you don't set a maximum, or if the
+    call's maximum argument is less than the forced maximum, then the call's
+    maximum will be respected.
+    """
+    def __init__(self, real_randint, forced_minimum=None, forced_maximum=None):
+        self.real_randint = real_randint
+        self.minimum = forced_minimum
+        self.maximum = forced_maximum
+
+    def __call__(self, minimum, maximum):
+        if self.minimum is not None:
+            minimum = max(minimum, self.minimum)
+        if self.maximum is not None:
+            maximum = min(maximum, self.maximum)
+        return self.real_randint(minimum, maximum)