Merge lp:~mpontillo/maas/autocreate-fabrics-and-vlans into lp:~maas-committers/maas/trunk

[Mike Pontillo]

Automatically create VLANs and Fabrics when cluster interfaces are created. (and when it's appropriate)

[Mike Pontillo (mpontillo)]

This still needs more unit tests written. But I'd like some additional eyes on this code before I move forward.

I've been testing this end-to-end and it seems to do what we want, at least for creating VLANs and fabrics automatically when the cluster registers.

Adding cluster interfaces by hand, (and editing existing interfaces to make changes that SHOULD probably put them in a different VLAN or Fabric) will most likely do the wrong thing still.

[LaMont Jones (lamont)]

Nothing leaps out at me as broken (other than collision detection for the multiple ip addresses) - I'd prefer to not see the randint() call at all, for repeatability sake (which might allow a change in the testcase.)

review: Needs Fixing

[LaMont Jones (lamont)]

looks good.

review: Approve

[Blake Rouse (blake-rouse)]

Looks good. Just move that nasty hash calculation to its own function.

review: Approve

=== modified file 'src/maasserver/forms.py'
--- src/maasserver/forms.py	2015-10-23 18:56:34 +0000
+++ src/maasserver/forms.py	2015-10-23 21:41:16 +0000
@@ -46,9 +46,10 @@
 import collections
 from collections import Counter
 from functools import partial
+import hashlib
 import json
+import os
 import pipes
-from random import randint
 import re
 
 from crochet import TimeoutError
@@ -126,6 +127,7 @@
     Config,
     Device,
     DownloadProgress,
+    Fabric,
     Filesystem,
     Interface,
     LargeFile,
@@ -142,6 +144,7 @@
     SSLKey,
     Tag,
     VirtualBlockDevice,
+    VLAN,
     VolumeGroup,
     Zone,
 )
@@ -192,11 +195,15 @@
     valid_ipv6,
 )
 from provisioningserver.logger import get_maas_logger
-from provisioningserver.network import filter_likely_unmanaged_networks
+from provisioningserver.network import (
+    filter_and_annotate_networks,
+    sort_networks_by_priority,
+)
 from provisioningserver.rpc.exceptions import (
     NoConnectionsAvailable,
     NoSuchOperatingSystem,
 )
+from provisioningserver.utils.ipaddr import get_vid_from_ifname
 from provisioningserver.utils.network import (
     ip_range_within_network,
     make_network,
@@ -1546,35 +1553,6 @@
     return True
 
 
-def disambiguate_name(original_name, ip_address):
-    """Return a unique variant of `original_name` for a cluster interface.
-
-    This function has no knowledge of other existing cluster interfaces.  It
-    disambiguates based purely on the given data and random numbers.
-
-    :param original_name: The originally proposed name for the cluster
-        interface, which presumably turned out to be ambiguous.
-    :param ip_address: IP address for the cluster interface (either as a
-        string or as an `IPAddress`).  Used to determine whether the interface
-        is an IPv4 one or an IPv6 one.
-    :return: A version of `original_name` with a disambiguating suffix.  The
-        suffix contains both the IP version (`ipv4` or `ipv6`) and possibly a
-        random part to avoid further clashes.
-    """
-    ip_version = IPAddress(ip_address).version
-    assert ip_version in (4, 6)
-    if ip_version == 6:
-        # IPv6 cluster interface.  In principle there could be many of these
-        # on the same network interface, so add a random suffix.
-        suffix = 'ipv6-%d' % randint(10000, 99999)
-    else:
-        # IPv4 cluster interface.  There can be only one of these on the
-        # network interface, so just suffixing '-ipv4' to the name should make
-        # it unique.
-        suffix = 'ipv4'
-    return '%s-%s' % (original_name, suffix)
-
-
 class NodeGroupInterfaceForm(MAASModelForm):
 
     management = forms.TypedChoiceField(
@@ -1628,6 +1606,7 @@
 
         subnet = None
         if self.instance and self.instance.subnet and ip and subnet_mask:
+            # Existing NodeGroupInterface, so just update the existing Subnet.
             cidr = create_cidr(ip, subnet_mask)
             subnet = self.instance.subnet
             subnet.update_cidr(cidr)
@@ -1635,14 +1614,55 @@
                 subnet.gateway_ip = router_ip
             subnet.save()
         elif subnet_mask:
+            # New NodeGroupInterface, so create the Subnet (if we have enough
+            # information).
             cidr = create_cidr(ip, subnet_mask)
-            subnet, _ = Subnet.objects.get_or_create(
+            subnet, created = Subnet.objects.get_or_create(
                 cidr=cidr, defaults={
                     'name': cidr,
                     'cidr': cidr,
                     'gateway_ip': router_ip,
                     'space': Space.objects.get_default_space()
                 })
+            if created:
+                # Check if a Subnet was already created for a matching
+                # interface. (this could happen if multiple IP addresses
+                # are assigned to the same interface.)
+                vlan = None
+                ifname = self.cleaned_data.get('interface')
+                if self.instance:
+                    vlan = VLAN.objects.filter_by_nodegroup_interface(
+                        self.instance.nodegroup, ifname).order_by('id').first()
+                if vlan is not None:
+                    # Found an existing VLAN that matches this interface.
+                    # No need to create a new one; just assign that VLAN
+                    # to the subnet that was found.
+                    subnet.vlan = vlan
+                    subnet.save()
+                elif self.data.get('type') == 'ethernet.vlan':
+                    # If we're creating a VLAN, we must be intending to put
+                    # the VLAN on a Fabric that already exists.
+                    # Check the VLAN's parent interface to see if it is
+                    # already associated with a VLAN (and hence a Fabric).
+                    vid = self.data.get('vid', get_vid_from_ifname(ifname))
+                    parent = self.data.get('parent')
+                    fabric = Fabric.objects.filter_by_nodegroup_interface(
+                        self.instance.nodegroup, parent).first()
+                    if fabric is None:
+                        fabric = Fabric.objects.get_or_create_for_subnet(
+                            subnet)
+                    vlan = VLAN.objects.create(vid=vid, fabric=fabric)
+                    subnet.vlan = vlan
+                    subnet.save()
+                else:
+                    # If we created a new Subnet, and it's *not* a tagged VLAN
+                    # (and we didn't find an existing interface above), that
+                    # means we need to create a new Fabric and place the
+                    # subnet in its default VLAN.
+                    fabric = Fabric.objects.get_or_create_for_subnet(
+                        subnet)
+                    subnet.vlan = fabric.get_default_vlan()
+                    subnet.save()
 
         interface = super(NodeGroupInterfaceForm, self).save(*args, **kwargs)
         if interface.subnet != subnet:
@@ -1650,6 +1670,45 @@
             interface.save()
         return interface
 
+    def _ensure_unique_cluster_interface_name(self, name, interface):
+        """
+        Ensures that the specified interface name is unique. Appends a portion
+        of a unique sha256 hex string (and the IP version) if not.
+
+        :param name: The tentative "friendly" name of this cluster interface.
+        :param interface: The name of the network interface.
+        :return:str
+        """
+        # Get the cluster to which this interface is attached.  There may not
+        # be one, since it may be a placeholder instance that is still being
+        # initialised by the same request that is also creating the interface.
+        # In that case, self.instance.nodegroup will not be None, but rather
+        # an ORM stub which crashes when accessed.
+        cluster = get_one(
+            NodeGroup.objects.filter(id=self.instance.nodegroup_id))
+        if cluster is not None and interface:
+            siblings = cluster.nodegroupinterface_set
+            while siblings.filter(name=name).exists():
+                # In case the interface name already exists, use a
+                # cryptographic hash based on the interface name and address
+                # (plus some random bytes for good measure).
+                ip_address = self.cleaned_data['ip']
+                ip_version = IPAddress(ip_address).version
+                sha = hashlib.sha256()
+                sha.update(name)
+                sha.update(ip_address)
+                sha.update(self.cleaned_data['subnet_mask'])
+                sha.update(os.urandom(32))
+                digest = sha.hexdigest()
+                for i in range(4, 65):
+                    # The upper bound of this range() will be 64, so that
+                    # we can get the full sha256 hex string if needed.
+                    suffix = digest[:i]
+                    new_name = '%s-ipv%d-%s' % (name, ip_version, suffix)
+                    if not siblings.filter(name=new_name).exists():
+                        return new_name
+        return name
+
     def compute_name(self):
         """Return the value the `name` field should have.
 
@@ -1670,20 +1729,9 @@
         # compatibility with clients that expect the pre-1.6 behaviour, where
         # the 'name' and 'interface' fields were the same thing.
         interface = self.cleaned_data.get('interface')
-        name = make_name_from_interface(interface)
-        # Get the cluster to which this interface is attached.  There may not
-        # be one, since it may be a placeholder instance that is still being
-        # initialised by the same request that is also creating the interface.
-        # In that case, self.instance.nodegroup will not be None, but rather
-        # an ORM stub which crashes when accessed.
-        cluster = get_one(
-            NodeGroup.objects.filter(id=self.instance.nodegroup_id))
-        if cluster is not None and interface:
-            siblings = cluster.nodegroupinterface_set
-            if siblings.filter(name=name).exists():
-                # This name is already in use.  Add a suffix to make it unique.
-                return disambiguate_name(name, self.cleaned_data['ip'])
-
+        name = make_name_from_interface(
+            interface, alias=self.data.get('alias'))
+        name = self._ensure_unique_cluster_interface_name(name, interface)
         return name
 
     def get_duplicate_fqdns(self):
@@ -1706,6 +1754,18 @@
 
         return set(duplicates)
 
+    def clean_interface(self):
+        # Interface names must be consistent, so that if eth0 and eth0:1
+        # are added as cluster interfaces, they can be associated with the
+        # same VLANs. Save the alias so that we can use it in to
+        # disambiguate the interface name.
+        ifname = self.cleaned_data['interface']
+        if ':' in ifname:
+            alias = ifname.strip().split(':', 1)[1]
+            self.data['alias'] = alias
+            ifname = ifname.strip().split(':')[0]
+        return ifname
+
     def clean_management(self):
         management = self.cleaned_data['management']
 
@@ -2018,7 +2078,7 @@
 
         # Try to only manage physical Ethernet interfaces.
         ip_addr_json = self.data.get('ip_addr_json', None)
-        interfaces = filter_likely_unmanaged_networks(interfaces, ip_addr_json)
+        interfaces = filter_and_annotate_networks(interfaces, ip_addr_json)
 
         for interface in interfaces:
             validate_nodegroupinterface_definition(interface)
@@ -2033,9 +2093,7 @@
         # unique names for cluster interfaces on the same network interface,
         # the IPv4 one will get first stab at getting the exact same name as
         # the network interface.
-        interfaces = sorted(
-            self.cleaned_data['interfaces'],
-            key=lambda definition: IPAddress(definition['ip']).version)
+        interfaces = sort_networks_by_priority(self.cleaned_data['interfaces'])
         for interface in interfaces:
             instance = NodeGroupInterface(nodegroup=nodegroup)
             form = NodeGroupInterfaceForm(data=interface, instance=instance)

=== modified file 'src/maasserver/models/fabric.py'
--- src/maasserver/models/fabric.py	2015-10-21 21:13:01 +0000
+++ src/maasserver/models/fabric.py	2015-10-23 21:41:16 +0000
@@ -70,6 +70,29 @@
             fabric._create_default_vlan()
         return fabric
 
+    def get_or_create_for_subnet(self, subnet):
+        """Given an existing fabric_id (by default, the default fabric)
+        creates and returns a new Fabric if there is an existing Subnet in
+        the fabric already. Exclude the specified subnet (which will be one
+        that was just created).
+        """
+        from maasserver.models import Subnet
+        default_fabric = self.get_default_fabric()
+        if Subnet.objects.filter(
+                vlan__fabric=default_fabric).exclude(
+                id=subnet.id).count() == 0:
+            return default_fabric
+        else:
+            return Fabric.objects.create()
+
+    def filter_by_nodegroup_interface(self, nodegroup, ifname):
+        """Query for the Fabric associated with the specified NodeGroup,
+        where the NodeGroupInterface matches the specified name.
+        """
+        return self.filter(
+            vlan__subnet__nodegroupinterface__nodegroup=nodegroup,
+            vlan__subnet__nodegroupinterface__interface=ifname)
+
     def get_fabric_or_404(self, id, user, perm):
         """Fetch a `Fabric` by its id.  Raise exceptions if no `Fabric` with
         this id exist or if the provided user has not the required permission

=== modified file 'src/maasserver/models/nodegroupinterface.py'
--- src/maasserver/models/nodegroupinterface.py	2015-09-24 16:22:12 +0000
+++ src/maasserver/models/nodegroupinterface.py	2015-10-23 21:41:16 +0000
@@ -400,7 +400,8 @@
             return
         ip_version = IPAddress(self.ip).version
         similar_interfaces = self.nodegroup.nodegroupinterface_set.filter(
-            interface=self.interface)
+            interface=self.interface).exclude(
+            management=NODEGROUPINTERFACE_MANAGEMENT.UNMANAGED)
         if self.id is not None:
             similar_interfaces = similar_interfaces.exclude(id=self.id)
         potential_clashes = [

=== modified file 'src/maasserver/models/tests/test_nodegroupinterface.py'
--- src/maasserver/models/tests/test_nodegroupinterface.py	2015-09-24 16:22:12 +0000
+++ src/maasserver/models/tests/test_nodegroupinterface.py	2015-10-23 21:41:16 +0000
@@ -627,25 +627,25 @@
             NodeGroupInterface.objects.filter(interface=net_interface),
             HasLength(2))
 
-    def test_validation_rejects_two_IPv4_interfaces_on_net_interface(self):
+    def test_rejects_two_managed_IPv4_interfaces_on_net_interface(self):
         cluster = factory.make_NodeGroup()
         net_interface = factory.make_name('eth')
         factory.make_NodeGroupInterface(
             cluster, interface=net_interface,
+            management=NODEGROUPINTERFACE_MANAGEMENT.DHCP_AND_DNS,
             network=factory.make_ipv4_network())
         network = factory.make_ipv4_network()
         extra_interface = NodeGroupInterface(
             nodegroup=cluster, interface=net_interface,
+            management=NODEGROUPINTERFACE_MANAGEMENT.DHCP_AND_DNS,
             ip=factory.pick_ip_in_network(network))
 
         error = self.assertRaises(ValidationError, extra_interface.save)
-        self.assertEqual(
-            [
+        self.assertThat(
+            error.messages, Contains(
                 "Another cluster interface already connects "
                 "network interface %s to an IPv4 network."
-                % net_interface
-            ],
-            error.messages)
+                % net_interface))
 
     def test_validation_accepts_two_IPv6_interfaces_on_net_interface(self):
         cluster = factory.make_NodeGroup()
@@ -660,29 +660,29 @@
         extra_interface.save()
         self.assertThat(cluster.nodegroupinterface_set.all(), HasLength(2))
 
-    def test_validation_rejects_two_IPv6_static_ranges_on_net_interface(self):
+    def test_rejects_two_managed_IPv6_static_ranges_on_net_interface(self):
         cluster = factory.make_NodeGroup()
         net_interface = factory.make_name('eth')
         factory.make_NodeGroupInterface(
             cluster, interface=net_interface,
+            management=NODEGROUPINTERFACE_MANAGEMENT.DHCP_AND_DNS,
             network=factory.make_ipv6_network(slash=64))
         network = factory.make_ipv6_network(slash=64)
         static_low = unicode(IPAddress(network.first + 1))
         static_high = unicode(IPAddress(network.last - 1))
         extra_interface = NodeGroupInterface(
             nodegroup=cluster, interface=net_interface,
+            management=NODEGROUPINTERFACE_MANAGEMENT.DHCP_AND_DNS,
             ip=factory.pick_ip_in_network(network),
             static_ip_range_low=static_low,
             static_ip_range_high=static_high)
 
         error = self.assertRaises(ValidationError, extra_interface.save)
-        self.assertEqual(
-            [
+        self.assertThat(
+            error.messages, Contains(
                 "Another cluster interface with a static address range "
                 "already connects network interface %s to an IPv6 network."
-                % net_interface
-            ],
-            error.messages)
+                % net_interface))
 
     def test_validation_knows_update_from_new_interface(self):
         cluster = factory.make_NodeGroup()

=== modified file 'src/maasserver/models/vlan.py'
--- src/maasserver/models/vlan.py	2015-10-09 06:06:47 +0000
+++ src/maasserver/models/vlan.py	2015-10-23 21:41:16 +0000
@@ -49,6 +49,14 @@
         from maasserver.models.fabric import Fabric
         return Fabric.objects.get_default_fabric().get_default_vlan()
 
+    def filter_by_nodegroup_interface(self, nodegroup, ifname):
+        """Query fot the VLAN that matches the specified NodeGroup, whose
+        interface matches the specified name.
+        """
+        return self.filter(
+            subnet__nodegroupinterface__nodegroup=nodegroup,
+            subnet__nodegroupinterface__interface=ifname)
+
 
 class VLAN(CleanSave, TimestampedModel):
     """A `VLAN`.

=== modified file 'src/maasserver/tests/test_forms_nodegroupinterface.py'
--- src/maasserver/tests/test_forms_nodegroupinterface.py	2015-09-08 18:41:57 +0000
+++ src/maasserver/tests/test_forms_nodegroupinterface.py	2015-10-23 21:41:16 +0000
@@ -23,7 +23,11 @@
     ERROR_MESSAGE_STATIC_RANGE_IN_USE,
     NodeGroupInterfaceForm,
 )
-from maasserver.models import NodeGroupInterface
+from maasserver.models import (
+    Fabric,
+    NodeGroupInterface,
+    VLAN,
+)
 from maasserver.models.staticipaddress import StaticIPAddress
 from maasserver.testing.factory import factory
 from maasserver.testing.testcase import MAASServerTestCase
@@ -182,7 +186,9 @@
             data=int_settings, instance=make_ngi_instance(cluster))
         self.assertTrue(form.is_valid())
         interface = form.save()
-        self.assertEqual('%s-ipv4' % int_settings['interface'], interface.name)
+        self.assertThat(
+            interface.name,
+            StartsWith('%s-ipv4-' % int_settings['interface']))
 
     def test__disambiguates_IPv6_interface_with_ipv6_suffix(self):
         cluster = factory.make_NodeGroup()
@@ -458,3 +464,102 @@
         # print(list(Subnet.objects.all()))
         # self.assertThat(ngi.network, Equals(new_network.cidr))
         self.assertThat(Subnet.objects.count(), Equals(1))
+
+    def test_multiple_subnets_on_single_interface_uses_existing_vlan(self):
+        ng = factory.make_NodeGroup()
+        ngi1 = NodeGroupInterface(nodegroup=ng)
+        form = NodeGroupInterfaceForm(data=dict(
+            interface='eth0', ip='192.168.0.1', subnet_mask='255.255.255.0'),
+            instance=ngi1)
+        self.assertThat(form.is_valid(), Equals(True))
+        ngi1 = form.save()
+        self.assertIsNotNone(ngi1)
+        ngi2 = NodeGroupInterface(nodegroup=ng)
+        form = NodeGroupInterfaceForm(data=dict(
+            interface='eth0', ip='192.168.1.1', subnet_mask='255.255.255.0'),
+            instance=ngi2)
+        self.assertThat(form.is_valid(), Equals(True))
+        ngi2 = form.save()
+        self.assertIsNotNone(ngi2)
+        self.assertThat(VLAN.objects.all().count(), Equals(1))
+        self.assertThat(ngi1.vlan, Equals(ngi2.vlan))
+
+    def test_subnet_vlan_creation_uses_default_fabric_if_empty(self):
+        ng = factory.make_NodeGroup()
+        ngi1 = NodeGroupInterface(nodegroup=ng)
+        form = NodeGroupInterfaceForm(data=dict(
+            interface='eth0', ip='192.168.0.1', subnet_mask='255.255.255.0'),
+            instance=ngi1)
+        self.assertThat(form.is_valid(), Equals(True))
+        ngi1 = form.save()
+        self.assertIsNotNone(ngi1)
+        self.assertThat(Fabric.objects.all().count(), Equals(1))
+        self.assertThat(ngi1.vlan.fabric.id, Equals(0))
+
+    def test_creates_new_fabric_if_alt_subnet_exists_in_default_fabric(self):
+        ng = factory.make_NodeGroup()
+        ngi1 = NodeGroupInterface(nodegroup=ng)
+        form = NodeGroupInterfaceForm(data=dict(
+            interface='eth0', ip='192.168.0.1', subnet_mask='255.255.255.0'),
+            instance=ngi1)
+        self.assertThat(form.is_valid(), Equals(True))
+        ngi1 = form.save()
+        self.assertIsNotNone(ngi1)
+        self.assertThat(Fabric.objects.all().count(), Equals(1))
+        self.assertThat(ngi1.vlan.fabric.id, Equals(0))
+        ngi2 = NodeGroupInterface(nodegroup=ng)
+        form = NodeGroupInterfaceForm(data=dict(
+            interface='eth1', ip='192.168.1.1', subnet_mask='255.255.255.0'),
+            instance=ngi2)
+        self.assertThat(form.is_valid(), Equals(True))
+        ngi2 = form.save()
+        self.assertIsNotNone(ngi2)
+        self.assertThat(Fabric.objects.all().count(), Equals(2))
+        # The first NodeGroupInterface we saved should be using the default
+        # Fabric
+        self.assertThat(ngi1.vlan.fabric.id, Equals(0))
+        self.assertIsNotNone(ngi2.vlan.fabric)
+
+    def test_creates_vlan_interface_if_interface_type_and_parent_known(self):
+        ng = factory.make_NodeGroup()
+        ngi1 = NodeGroupInterface(nodegroup=ng)
+        form = NodeGroupInterfaceForm(data=dict(
+            interface='eth0', ip='192.168.0.1', subnet_mask='255.255.255.0'),
+            instance=ngi1)
+        self.assertThat(form.is_valid(), Equals(True))
+        ngi1 = form.save()
+        self.assertIsNotNone(ngi1)
+        self.assertThat(Fabric.objects.all().count(), Equals(1))
+        self.assertThat(ngi1.vlan.fabric.id, Equals(0))
+        ngi2 = NodeGroupInterface(nodegroup=ng)
+        form = NodeGroupInterfaceForm(data=dict(
+            interface='vlan12', ip='192.168.1.1', subnet_mask='255.255.255.0',
+            parent='eth0', type='ethernet.vlan'), instance=ngi2)
+        self.assertThat(form.is_valid(), Equals(True))
+        ngi2 = form.save()
+        self.assertIsNotNone(ngi2)
+        self.assertThat(Fabric.objects.all().count(), Equals(1))
+        self.assertThat(VLAN.objects.filter(vid=12).count(), Equals(1))
+
+    def test_creates_vlan_plus_new_fabric_if_no_parent_untagged_exists(self):
+        ng = factory.make_NodeGroup()
+        ngi1 = NodeGroupInterface(nodegroup=ng)
+        form = NodeGroupInterfaceForm(data=dict(
+            interface='eth0', ip='192.168.0.1', subnet_mask='255.255.255.0'),
+            instance=ngi1)
+        self.assertThat(form.is_valid(), Equals(True))
+        ngi1 = form.save()
+        self.assertIsNotNone(ngi1)
+        self.assertThat(Fabric.objects.all().count(), Equals(1))
+        self.assertThat(ngi1.vlan.fabric.id, Equals(0))
+        ngi2 = NodeGroupInterface(nodegroup=ng)
+        form = NodeGroupInterfaceForm(data=dict(
+            interface='eth0.12', ip='192.168.1.1', subnet_mask='255.255.255.0',
+            type='ethernet.vlan'), instance=ngi2)
+        self.assertThat(form.is_valid(), Equals(True))
+        ngi2 = form.save()
+        self.assertIsNotNone(ngi2)
+        self.assertThat(Fabric.objects.all().count(), Equals(2))
+        # Check that VLAN 12 was created on the non-default VLAN.
+        self.assertThat(VLAN.objects.filter(
+            vid=12, fabric__id__gt=0).count(), Equals(1))

=== modified file 'src/maasserver/utils/interfaces.py'
--- src/maasserver/utils/interfaces.py	2014-11-13 20:11:48 +0000
+++ src/maasserver/utils/interfaces.py	2015-10-23 21:41:16 +0000
@@ -21,7 +21,7 @@
 import re
 
 
-def make_name_from_interface(interface):
+def make_name_from_interface(interface, alias=None):
     """Generate a cluster interface name based on a network interface name.
 
     The name is used as an identifier in API URLs, so awkward characters are
@@ -30,6 +30,8 @@
 
     If `interface` is `None`, or empty, a name will be made up.
     """
+    if alias:
+        interface = "%s:%s" % (interface, alias)
     if interface is None or interface == u'':
         base_name = u'unnamed-%d' % randint(1000000, 9999999)
     else:

=== modified file 'src/provisioningserver/network.py'
--- src/provisioningserver/network.py	2015-10-21 23:36:45 +0000
+++ src/provisioningserver/network.py	2015-10-23 21:41:16 +0000
@@ -149,11 +149,10 @@
     return ip_addr_json
 
 
-def _filter_likely_physical_networks_using_interface_name(networks):
+def _filter_managed_networks_by_ifname(networks):
     """
-    Given the specified list of networks and corresponding JSON, filters
-    the list of networks and returns any that may be physical interfaces
-    (based on the interface name).
+    Given the specified list of networks, filters the list of networks and
+    returns any that may be physical interfaces (based on the interface name).
 
     :param networks: A list of network dictionaries. Must contain an
         'interface' key containing the interface name.
@@ -170,12 +169,90 @@
         ]
 
 
-def _filter_likely_physical_networks_using_json_data(networks, ip_addr_json):
+def _annotate_network_with_interface_information(network, addr_info):
+    """Adds a 'type' field to a specified dictionary which represents a network
+    interface.
+    """
+    iface = addr_info.get(network['interface'], None)
+    if iface is not None and 'type' in iface:
+        network['type'] = iface['type']
+        if 'vid' in iface:
+            network['vid'] = iface['vid']
+        if 'bridged_interfaces' in iface:
+            network['bridged_interfaces'] = ' '.join(
+                iface['bridged_interfaces'])
+        if 'bonded_interfaces' in iface:
+            network['bonded_interfaces'] = ' '.join(
+                iface['bonded_interfaces'])
+        if 'parent' in iface:
+            network['parent'] = iface['parent']
+    return network
+
+
+def _bridges_a_physical_interface(ifname, addr_info):
+    """Returns True if the bridge interface with the specified name bridges
+    at least one physical Ethernet interface. Otherwise, returns False.
+    """
+    bridge_interface = addr_info.get(ifname)
+    for interface_name in bridge_interface.get('bridged_interfaces', []):
+        iface = addr_info.get(interface_name, {})
+        if iface.get('type') == 'ethernet.physical':
+            return True
+    return False
+
+
+def _belongs_to_a_vlan(ifname, addr_info):
+    """Returns True if the interface with the specified name is needed
+    because a VLAN interface depends on it.
+    """
+    for interface_name in addr_info:
+        iface = addr_info.get(interface_name, {})
+        if iface.get('type') == 'ethernet.vlan':
+            if iface.get('parent') == ifname:
+                return True
+    return False
+
+
+def _network_name(network):
+    """Returns interface name for the specified network. (removes a trailing
+    alias, if present.)
+    """
+    return network['interface'].split(':')[0]
+
+
+def _should_manage_network(network, addr_info):
+    """Returns True if this network should be managed; otherwise returns False.
+    """
+    ifname = _network_name(network)
+    addrinfo = addr_info.get(ifname, {})
+    iftype = addrinfo.get('type', '')
+    # In general, only physical Ethernet interfaces, VLANs, and bonds
+    # are going to be managed. Since they are most likely irrelevant, (and
+    # we don't want them to create superfluous subnets) filter out virtual
+    # interfaces (whose specific type cannot be determined) and bridges.
+    # However, reconsider bridges as "possibly managed" if they are
+    # present in support of a physical Ethernet device, or a VLAN is
+    # defined on top of the bridge.
+    return (
+        addrinfo and
+        (_belongs_to_a_vlan(ifname, addr_info) or
+         iftype == 'ethernet.physical' or
+         iftype == 'ethernet.vlan' or
+         iftype == 'ethernet.bond' or
+         (iftype == 'ethernet.bridge' and
+          _bridges_a_physical_interface(ifname, addr_info))
+         )
+    )
+
+
+def _filter_and_annotate_managed_networks(networks, ip_addr_json):
     """
     Given the specified list of networks and corresponding JSON, filters
     the list of networks and returns any that are known to be physical
     interfaces. (based on driver information gathered from /sys)
 
+    Also annotates the list of networks with each network's type.
+
     :param networks: A list of network dictionaries. Must contain an
         'interface' key containing the interface name.
     :param ip_addr_json: A JSON string returned from `get_ip_addr_json()`.
@@ -184,26 +261,26 @@
     addr_info = json.loads(ip_addr_json)
     assert isinstance(addr_info, dict)
     return [
-        network
+        _annotate_network_with_interface_information(network, addr_info)
         for network in networks
-        if (network['interface'] in addr_info and
-            'type' in addr_info[network['interface']] and
-            (addr_info[network['interface']]['type'] == 'ethernet.physical' or
-             addr_info[network['interface']]['type'] == 'ethernet.vlan' or
-             addr_info[network['interface']]['type'] == 'ethernet.bond'))
+        if _should_manage_network(network, addr_info)
         ]
 
 
-def filter_likely_unmanaged_networks(networks, ip_addr_json=None):
+def filter_and_annotate_networks(networks, ip_addr_json=None):
     """
     Given the specified list of networks and optional corresponding JSON,
-    filters the list of networks and returns any that are known to be physical
-    interfaces.
+    filters the list of networks and returns any that may correspond to managed
+    networks. (that is, any physical Ethernet interfaces, plus bonds and
+    VLANs.)
 
     If no interfaces are found, fall back to using the interface name to
     filter the list in a reasonable manner. (this allows support for running
     on LXCs, where all interfaces may be virtual.)
 
+    Also annotates the list of networks with their type, and other metadata
+    such as VLAN VID, bonded/bridged interfaces, or parent.
+
     :param networks: A list of network dictionaries. Must contain an
         'interface' key containing the interface name.
     :param ip_addr_json: A JSON string returned from `get_ip_addr_json()`.
@@ -211,9 +288,9 @@
     """
     assert networks is not None
     if ip_addr_json is None:
-        return _filter_likely_physical_networks_using_interface_name(networks)
+        return _filter_managed_networks_by_ifname(networks)
     else:
-        physical_networks = _filter_likely_physical_networks_using_json_data(
+        physical_networks = _filter_and_annotate_managed_networks(
             networks, ip_addr_json)
         if len(physical_networks) > 0:
             return physical_networks
@@ -221,5 +298,57 @@
             # Since we couldn't find anything, fall back to using the heuristic
             # based on names. (we could be running inside a container with only
             # virtual interfaces, etc.)
-            return _filter_likely_physical_networks_using_interface_name(
+            return _filter_managed_networks_by_ifname(
                 networks)
+
+
+def _get_interface_type_priority(iface):
+    """Returns a sort key based on interface types we prefer to process
+    first when adding them to a NodeGroup.
+
+    The most important thing is that we need to process VLANs last, since they
+    require the underlying Fabric to be created first.
+    """
+    iftype = iface.get('type')
+    # Physical interfaces first, followed by bonds, followed by bridges.
+    # VLAN interfaces last.
+    # This will ensure that underlying Fabric objects can be created before
+    # any VLANs that may belong to each Fabric.
+    if iftype == "ethernet.physical":
+        return 0
+    elif iftype == "ethernet.wireless":
+        return 1
+    elif iftype == "ethernet":
+        return 2
+    elif iftype == "ethernet.bond":
+        return 3
+    elif iftype == "ethernet.bridge":
+        return 4
+    elif iftype == "ethernet.vlan":
+        return 5
+    else:
+        # We don't really care what the sort order is; they should be filtered
+        # out anyway.
+        return -1
+
+
+def _network_priority_sort_key(iface):
+    """Returns a sort key used for processing interfaces before adding them
+    to a NodeGroup.
+
+    First sorts by interface type, then interface name, then address family.
+    (Since MAAS usually manages IPv4 addresses, and we have a name
+    disambiguation funciton that can produce somewhat unfriendly names,
+    make sure the IPv4 interfaces get to go first.)
+    """
+    return (
+        _get_interface_type_priority(iface),
+        iface['interface'],
+        IPAddress(iface['ip']).version
+    )
+
+
+def sort_networks_by_priority(networks):
+    """Sorts the specified list of networks in the order in which we would
+    prefer to add them to a NodeGroup."""
+    return sorted(networks, key=_network_priority_sort_key)

=== modified file 'src/provisioningserver/rpc/clusterservice.py'
--- src/provisioningserver/rpc/clusterservice.py	2015-10-21 18:38:54 +0000
+++ src/provisioningserver/rpc/clusterservice.py	2015-10-23 21:41:16 +0000
@@ -525,6 +525,7 @@
 
         networks = discover_networks()
 
+        ip_addr_json = None
         try:
             ip_addr_json = get_ip_addr_json()
         except ExternalProcessError as epe:

=== modified file 'src/provisioningserver/tests/test_network.py'
--- src/provisioningserver/tests/test_network.py	2015-10-21 23:36:45 +0000
+++ src/provisioningserver/tests/test_network.py	2015-10-23 21:41:16 +0000
@@ -25,7 +25,11 @@
     AF_INET6,
 )
 from provisioningserver import network
-from testtools.matchers import HasLength
+from provisioningserver.network import sort_networks_by_priority
+from testtools.matchers import (
+    Equals,
+    HasLength,
+)
 
 
 def make_inet_address(subnet=None):
@@ -216,3 +220,31 @@
         self.assertEqual(
             network.filter_unique_networks(networks),
             network.filter_unique_networks(reversed(networks)))
+
+
+class TestSortNetworksByPriority(MAASTestCase):
+
+    def test__sorts_by_type_then_ip_version(self):
+        interfaces = [
+            {'ip': "2001:db8::1",
+             'type': "ethernet.vlan",
+             'interface': 'vlan40'},
+            {'ip': "10.0.0.1",
+             'type': "ethernet.vlan",
+             'interface': 'vlan40'},
+            {'ip': "2001:db8:1::1",
+             'type': "ethernet.physical",
+             'interface': 'eth1'},
+            {'ip': "10.0.1.1",
+             'type': "ethernet.physical",
+             'interface': 'eth1'},
+            {'ip': "10.0.2.1",
+             'type': "ethernet.bridge",
+             'interface': 'br0'},
+        ]
+        sorted_interfaces = sort_networks_by_priority(interfaces)
+        self.expectThat(sorted_interfaces[0], Equals(interfaces[3]))
+        self.expectThat(sorted_interfaces[1], Equals(interfaces[2]))
+        self.expectThat(sorted_interfaces[2], Equals(interfaces[4]))
+        self.expectThat(sorted_interfaces[3], Equals(interfaces[1]))
+        self.expectThat(sorted_interfaces[4], Equals(interfaces[0]))

=== modified file 'src/provisioningserver/utils/ipaddr.py'
--- src/provisioningserver/utils/ipaddr.py	2015-10-22 00:48:38 +0000
+++ src/provisioningserver/utils/ipaddr.py	2015-10-23 21:41:16 +0000
@@ -108,7 +108,10 @@
         string.strip, line.split(':'))
 
     interface['index'] = int(index)
-    interface['name'] = name.split('@')[0]
+    names = name.split('@')
+    interface['name'] = names[0]
+    if len(names) > 1:
+        interface['parent'] = names[1]
 
     # Now parse the <properties> part from above.
     # This will be in the form "<FLAG1,FLAG2> key1 value1 key2 value2 ..."
@@ -196,6 +199,35 @@
         raise ValueError("Unknown IP address family: %s" % network.version)
 
 
+def get_bonded_interfaces(ifname, sys_class_net="/sys/class/net"):
+    """Returns a list of interface names which are part of the specified
+    Ethernet bond.
+
+    :return:list
+    """
+    bonding_slaves_file = os.path.join(
+        sys_class_net, ifname, 'bonding', 'slaves')
+    if os.path.isfile(bonding_slaves_file):
+        with open(bonding_slaves_file) as f:
+            return f.read().split()
+    else:
+        return []
+
+
+def get_bridged_interfaces(ifname, sys_class_net="/sys/class/net"):
+    """Returns a list of interface names which are part of the specified
+    Ethernet bridge interface.
+
+    :return:list
+    """
+    bridged_interfaces_dir = os.path.join(
+        sys_class_net, ifname, 'brif')
+    if os.path.isdir(bridged_interfaces_dir):
+        return os.listdir(bridged_interfaces_dir)
+    else:
+        return []
+
+
 def get_interface_type(
         ifname, sys_class_net="/sys/class/net",
         proc_net_vlan='/proc/net/vlan'):
@@ -229,11 +261,11 @@
     If the interface can be determined to be a non-Ethernet type, the type
     that is found will be returned. (For example, 'loopback' or 'ipip'.)
     """
-    sys_path = '%s/%s' % (sys_class_net, ifname)
+    sys_path = os.path.join(sys_class_net, ifname)
     if not os.path.isdir(sys_path):
         return 'missing'
 
-    sys_type_path = '%s/type' % sys_path
+    sys_type_path = os.path.join(sys_path, 'type')
     with open(sys_type_path) as f:
         iftype = int(f.read().strip())
 
@@ -278,5 +310,32 @@
     (if found) for each interface.
     """
     for name in interfaces:
-        interfaces[name]['type'] = get_interface_type(name)
+        iface = interfaces[name]
+        iftype = get_interface_type(name)
+        interfaces[name]['type'] = iftype
+        if iftype == 'ethernet.bond':
+            iface['bonded_interfaces'] = get_bonded_interfaces(name)
+        elif iftype == 'ethernet.vlan':
+            iface['vid'] = get_vid_from_ifname(name)
+        elif iftype == 'ethernet.bridge':
+            iface['bridged_interfaces'] = get_bridged_interfaces(name)
     return interfaces
+
+
+def get_vid_from_ifname(ifname):
+    """Returns the VID for the specified VLAN interface name.
+
+    Returns 0 if the VID could not be determined.
+
+    :return:int
+    """
+    vid = 0
+    iface_vid_re = re.compile('.*\.([0-9]+)$')
+    iface_vid_match = iface_vid_re.match(ifname)
+    vlan_vid_re = re.compile('vlan([0-9]+)$')
+    vlan_vid_match = vlan_vid_re.match(ifname)
+    if iface_vid_match:
+        vid = int(iface_vid_match.group(1))
+    elif vlan_vid_match:
+        vid = int(vlan_vid_match.group(1))
+    return vid

=== modified file 'src/provisioningserver/utils/tests/test_ipaddr.py'
--- src/provisioningserver/utils/tests/test_ipaddr.py	2015-10-22 00:48:38 +0000
+++ src/provisioningserver/utils/tests/test_ipaddr.py	2015-10-23 21:41:16 +0000
@@ -22,12 +22,13 @@
 from textwrap import dedent
 
 from maastesting.testcase import MAASTestCase
-from provisioningserver.network import filter_likely_unmanaged_networks
+from provisioningserver.network import filter_and_annotate_networks
 from provisioningserver.utils.ipaddr import (
     _add_additional_interface_properties,
     _get_settings_dict,
     _parse_interface_definition,
     annotate_with_driver_information,
+    get_bonded_interfaces,
     get_interface_type,
     parse_ip_addr,
 )
@@ -328,7 +329,8 @@
 
     def createInterfaceType(
             self, ifname, iftype, is_bridge=False, is_vlan=False,
-            is_bond=False, is_wireless=False, is_physical=False):
+            is_bond=False, is_wireless=False, is_physical=False,
+            bonded_interfaces=None):
         ifdir = os.path.join(self.tmp_sys_net, ifname)
         os.mkdir(ifdir)
         type_file = os.path.join(ifdir, 'type')
@@ -342,6 +344,10 @@
             f.close()
         if is_bond:
             os.mkdir(os.path.join(ifdir, 'bonding'))
+            if bonded_interfaces is not None:
+                f = open(os.path.join(ifdir, 'bonding', 'slaves'), 'w')
+                f.write(b"%s\n" % ' '.join(bonded_interfaces).encode('utf-8'))
+                f.close()
         if is_physical or is_wireless:
             os.mkdir(os.path.join(ifdir, 'device'))
             os.mkdir(os.path.join(ifdir, 'device', 'driver'))
@@ -384,6 +390,14 @@
             Equals('ethernet.bond')
         )
 
+    def test__identifies_bonded_interfaces(self):
+        self.createEthernetInterface(
+            'bond0', is_bond=True, bonded_interfaces=['eth0', 'eth1'])
+        self.assertThat(get_bonded_interfaces(
+            'bond0', sys_class_net=self.tmp_sys_net),
+            Equals(['eth0', 'eth1'])
+        )
+
     def test__identifies_vlan_interface(self):
         self.createEthernetInterface('vlan42', is_vlan=True)
         self.assertThat(get_interface_type(
@@ -452,7 +466,14 @@
         interfaces = parse_ip_addr(ip_addr_output)
         interfaces_with_types = annotate_with_driver_information(interfaces)
         for name in interfaces:
-            self.assertThat(interfaces_with_types[name], Contains('type'))
+            iface = interfaces_with_types[name]
+            self.assertThat(iface, Contains('type'))
+            if iface['type'] == 'ethernet.vlan':
+                self.expectThat(iface, Contains('vid'))
+            elif iface['type'] == 'ethernet.bond':
+                self.expectThat(iface, Contains('bonded_interfaces'))
+            elif iface['type'] == 'ethernet.bridge':
+                self.expectThat(iface, Contains('bridged_interfaces'))
 
 
 class TestFilterLikelyUnmanagedNetworks(MAASTestCase):
@@ -467,7 +488,7 @@
             {"interface": "wlan0"},
             {"interface": "avian0"},
         ]
-        actual_networks = filter_likely_unmanaged_networks(input_networks)
+        actual_networks = filter_and_annotate_networks(input_networks)
         expected_networks = [
             {"interface": "em0"},
             {"interface": "eth0"},
@@ -476,7 +497,7 @@
         ]
         self.assertThat(actual_networks, Equals(expected_networks))
 
-    def test__filters_based_on_json_data_if_available(self):
+    def test__filters_and_annotates_based_on_json_data_if_available(self):
         input_networks = [
             {"interface": "em0"},
             {"interface": "eth0"},
@@ -484,21 +505,35 @@
             {"interface": "bond0"},
             {"interface": "avian0"},
             {"interface": "br0"},
+            {"interface": "br1"},
+            {"interface": "br2"},
+            {"interface": "br3"},
             {"interface": "wlan0"},
         ]
         # Wow, these are some poorly named interfaces.
         # Though I guess technically an avian carrier is a physical interface.
         input_json = {
             "avian0": {"type": "ethernet.physical"},
-            "br0": {"type": "ethernet.vlan"},
-            "wlan0": {"type": "ethernet.bond"}
+            "br0": {"type": "ethernet.vlan", "vid": 123, "parent": "br3"},
+            "br1": {"type": "ethernet.bridge",
+                    "bridged_interfaces": ["eth1", "eth2"]},
+            "br2": {"type": "ethernet.bridge",
+                    "bridged_interfaces": ["avian0"]},
+            "br3": {"type": "ethernet.bridge"},
+            "wlan0": {"type": "ethernet.bond",
+                      "bonded_interfaces": ["em0", "eth0"]}
         }
-        actual_networks = filter_likely_unmanaged_networks(
+        actual_networks = filter_and_annotate_networks(
             input_networks, json.dumps(input_json))
         expected_networks = [
-            {"interface": "avian0"},
-            {"interface": "br0"},
-            {"interface": "wlan0"},
+            {"interface": "avian0", 'type': "ethernet.physical"},
+            {"interface": "br0", 'type': "ethernet.vlan", "vid": 123,
+             "parent": "br3"},
+            {"interface": "br2", 'type': "ethernet.bridge",
+             "bridged_interfaces": "avian0"},
+            {"interface": "br3", 'type': "ethernet.bridge"},
+            {"interface": "wlan0", 'type': "ethernet.bond",
+             "bonded_interfaces": "em0 eth0"},
         ]
         self.assertThat(actual_networks, Equals(expected_networks))
 
@@ -514,7 +549,7 @@
         ]
         input_json = {
         }
-        actual_networks = filter_likely_unmanaged_networks(
+        actual_networks = filter_and_annotate_networks(
             input_networks, json.dumps(input_json))
         expected_networks = [
             {"interface": "em0"},