Merge lp:~blake-rouse/maas/rack-nic-fabric-change into lp:~maas-committers/maas/trunk

[Blake Rouse]

Fix forms to only allow physical and bond interfaces to be on untagged vlans. Fix forms to only allow VLAN interface to be on the same fabric as its parent interface. Migrate subnets and child VLAN interfaces that are attached to a controllers interface when the interfaces fabric is changed.

[Blake Rouse (blake-rouse)]

This is only the backend part of this. I need to modify the API and UI to allow only the fabric to be changed on the controller interfaces. All other API's for controllers should not be allowed.

[Mike Pontillo (mpontillo)]

I disagree with this change. We have received feedback from the field team that this set of assumptions is not working for customers.

First, the use of the term "untagged" is very misleading. It is a term to describe a *switch port*, not a term to describe an attribute of a VLAN. A switch can have many ports in a fabric, some of which are "untagged" in (for example) the VLAN with VID 10, some of which are untagged in (for example) the VLAN with VID 20. The first thing we should do is rename that. (perhaps "default" would be better, but it's more like "unknown" - it's *one of* the 4094 VLANs, but we aren't sure which one.)

Assuming we're using the correct terminology (and then, the idea that a single fabric only ever has one "untagged" VLAN is out of the way), think about the case of the multiple different VLANs being "untagged" on different switch ports. Users MUST be able to change the model so that physical interfaces can appear on ports with a known VID. (Otherwise, you're forced to model it as two separate fabrics - and therefore you cannot support the VLAN interface creation feature in MAAS.)

This also breaks HA in the case where an inter-switch link uses tagged VLANs to aggregate multiple racks, and expects each rack controller to demultiplex the VLAN tags.

I understand and agree with the vision that MAAS will work better if all the switch ports are configured the same. However, I am opposed to the idea that we should force the model to conform to that. (yes, it should be the default, well-supported way to do things in MAAS, but we should not prevent MAAS from working if it doesn't match reality.)

review: Disapprove

[Blake Rouse (blake-rouse)]

I have removed the code that validations the untagged VLANs. Please give it another look. Thanks.

[Mike Pontillo (mpontillo)]

I'm good with this branch now; thanks.

For the record, only validations I had a problem with in revision 4710 were these:
 - "A physical interface can only belong to an untagged VLAN."
 - "A bond interface can only belong to an untagged VLAN."

(Again, in my view, we have received direction that we are not modeling switches, therefore it would be inappropriate to enforce those restrictions.)

The following other validations are perfectly reasonable (necessary, even):
 - "A VLAN interface can only belong to a tagged VLAN."
 - "A VLAN interface can only belong to a tagged VLAN on the same fabric as its parent interface."
 - "All [bond] (sic) parents must belong to the same VLAN."

review: Approve

=== modified file 'src/maasserver/models/interface.py'
--- src/maasserver/models/interface.py	2016-02-25 16:46:49 +0000
+++ src/maasserver/models/interface.py	2016-03-03 21:19:22 +0000
@@ -714,11 +714,12 @@
             # Nothing to do, already has links.
             return
         else:
-            # Use an associated subnet if it exists, else it will just be a
+            # Use an associated subnet if it exists and its on the same VLAN
+            # the interface is currently connected, else it will just be a
             # LINK_UP without a subnet.
             discovered_address = self.ip_addresses.filter(
                 alloc_type=IPADDRESS_TYPE.DISCOVERED,
-                subnet__isnull=False).first()
+                subnet__vlan=self.vlan).first()
             if discovered_address is not None:
                 subnet = discovered_address.subnet
             else:

=== modified file 'src/maasserver/models/signals/interfaces.py'
--- src/maasserver/models/signals/interfaces.py	2016-01-20 00:07:16 +0000
+++ src/maasserver/models/signals/interfaces.py	2016-03-03 21:19:22 +0000
@@ -7,19 +7,29 @@
     "signals",
 ]
 
+from django.db.models.signals import post_save
 from maasserver.enum import (
     INTERFACE_TYPE,
     IPADDRESS_TYPE,
+    NODE_TYPE,
 )
 from maasserver.models import (
     BondInterface,
     Interface,
     PhysicalInterface,
+    VLAN,
     VLANInterface,
 )
 from maasserver.utils.signals import SignalsManager
 
 
+INTERFACE_CLASSES = [
+    Interface,
+    PhysicalInterface,
+    BondInterface,
+    VLANInterface,
+]
+
 signals = SignalsManager()
 
 
@@ -48,12 +58,10 @@
                         ip_address, clearing_config=True)
 
 
-signals.watch_fields(
-    interface_enabled_or_disabled,
-    Interface, ['enabled'], delete=False)
-signals.watch_fields(
-    interface_enabled_or_disabled,
-    PhysicalInterface, ['enabled'], delete=False)
+for klass in INTERFACE_CLASSES:
+    signals.watch_fields(
+        interface_enabled_or_disabled,
+        klass, ['enabled'], delete=False)
 
 
 def interface_mtu_params_update(instance, old_values, **kwargs):
@@ -114,19 +122,81 @@
             parent.save()
 
 
-signals.watch_fields(
-    interface_mtu_params_update,
-    Interface, ['params'], delete=False)
-signals.watch_fields(
-    interface_mtu_params_update,
-    PhysicalInterface, ['params'], delete=False)
-signals.watch_fields(
-    interface_mtu_params_update,
-    BondInterface, ['params'], delete=False)
-signals.watch_fields(
-    interface_mtu_params_update,
-    VLANInterface, ['params'], delete=False)
-
+for klass in INTERFACE_CLASSES:
+    signals.watch_fields(
+        interface_mtu_params_update,
+        klass, ['params'], delete=False)
+
+
+def update_bond_parents(sender, instance, created, **kwargs):
+    """Update bond parents when interface created."""
+    if instance.type == INTERFACE_TYPE.BOND:
+        for parent in instance.parents.all():
+            # Make sure the parent has not links as well, just to be sure.
+            parent.clear_all_links(clearing_config=True)
+            if parent.vlan != instance.vlan:
+                parent.vlan = instance.vlan
+                parent.save()
+
+
+for klass in INTERFACE_CLASSES:
+    signals.watch(
+        post_save, update_bond_parents,
+        sender=klass)
+
+
+def interface_vlan_update(instance, old_values, **kwargs):
+    """When an interfaces VLAN is changed we need to do the following.
+
+        * If its a controller move all assigned subnets to the new VLAN. This
+          is done because the subnets defined are discovered on the
+          controller and an administrator cannot change them.
+        * If its a machine or device then we need to remove all links if the
+          VLAN is different.
+    """
+    new_vlan_id = instance.vlan_id
+    [old_vlan_id] = old_values
+    if new_vlan_id == old_vlan_id:
+        # Nothing changed do nothing.
+        return
+    if instance.node is None:
+        # Not assigned to a node. Nothing to do.
+        return
+
+    new_vlan = instance.vlan
+    if instance.node.node_type in (
+            NODE_TYPE.REGION_CONTROLLER,
+            NODE_TYPE.RACK_CONTROLLER,
+            NODE_TYPE.REGION_AND_RACK_CONTROLLER):
+        # Interface VLAN was changed on a controller. Move all linked subnets
+        # to that new VLAN.
+        for ip_address in instance.ip_addresses.all():
+            if ip_address.subnet is not None:
+                ip_address.subnet.vlan = new_vlan
+                ip_address.subnet.save()
+
+        # If any children are VLAN interfaces then we need to move those
+        # VLANs into the same fabric as the parent.
+        for rel in instance.children_relationships.all():
+            if rel.child.type == INTERFACE_TYPE.VLAN:
+                new_child_vlan, _ = VLAN.objects.get_or_create(
+                    fabric=new_vlan.fabric, vid=rel.child.vlan.vid)
+                rel.child.vlan = new_child_vlan
+                rel.child.save()
+                # No need to update the IP addresses here this function
+                # will be called again because the child has been saved.
+
+    else:
+        # Interface VLAN was changed on a machine or device. Remove all its
+        # links except the DISCOVERED ones.
+        instance.ip_addresses.exclude(
+            alloc_type=IPADDRESS_TYPE.DISCOVERED).delete()
+
+
+for klass in INTERFACE_CLASSES:
+    signals.watch_fields(
+        interface_vlan_update,
+        klass, ['vlan_id'], delete=False)
 
 # Enable all signals by default.
 signals.enable()

=== modified file 'src/maasserver/models/signals/nodes.py'
--- src/maasserver/models/signals/nodes.py	2016-02-26 21:50:26 +0000
+++ src/maasserver/models/signals/nodes.py	2016-03-03 21:19:22 +0000
@@ -43,10 +43,10 @@
     if owner_id_new != owner_id_old:
         NodeKey.objects.clear_token_for_node(node)
 
-for kclass in NODE_CLASSES:
+for klass in NODE_CLASSES:
     signals.watch_fields(
         clear_nodekey_when_owner_changes,
-        kclass, ['owner_id'], delete=False)
+        klass, ['owner_id'], delete=False)
 
 
 def create_services_on_node_type_change(node, old_values, deleted=False):
@@ -62,13 +62,13 @@
     if created:
         Service.objects.create_services_for(instance)
 
-for kclass in NODE_CLASSES:
+for klass in NODE_CLASSES:
     signals.watch_fields(
         create_services_on_node_type_change,
-        kclass, ['node_type'], delete=False)
+        klass, ['node_type'], delete=False)
     signals.watch(
         post_save, create_services_on_create,
-        sender=kclass)
+        sender=klass)
 
 
 # Enable all signals by default.

=== modified file 'src/maasserver/models/signals/tests/test_interfaces.py'
--- src/maasserver/models/signals/tests/test_interfaces.py	2016-02-26 18:39:26 +0000
+++ src/maasserver/models/signals/tests/test_interfaces.py	2016-03-03 21:19:22 +0000
@@ -10,6 +10,7 @@
 from maasserver.enum import (
     INTERFACE_TYPE,
     IPADDRESS_TYPE,
+    NODE_TYPE,
 )
 from maasserver.testing.factory import factory
 from maasserver.testing.testcase import MAASServerTestCase
@@ -123,3 +124,103 @@
         self.assertEqual({
             'mtu': bond_mtu,
             }, reload_object(physical3_interface).params)
+
+
+class TestUpdateBondParents(MAASServerTestCase):
+
+    def test__updates_bond_parents(self):
+        parent1 = factory.make_Interface(INTERFACE_TYPE.PHYSICAL)
+        parent2 = factory.make_Interface(
+            INTERFACE_TYPE.PHYSICAL, node=parent1.node)
+        bond = factory.make_Interface(
+            INTERFACE_TYPE.BOND, parents=[parent1, parent2])
+        self.assertEqual(bond.vlan, reload_object(parent1).vlan)
+        self.assertEqual(bond.vlan, reload_object(parent2).vlan)
+
+    def test__update_bond_clears_parent_links(self):
+        parent1 = factory.make_Interface(INTERFACE_TYPE.PHYSICAL)
+        parent2 = factory.make_Interface(
+            INTERFACE_TYPE.PHYSICAL, node=parent1.node)
+        static_ip = factory.make_StaticIPAddress(interface=parent1)
+        factory.make_Interface(
+            INTERFACE_TYPE.BOND, parents=[parent1, parent2])
+        self.assertIsNone(reload_object(static_ip))
+
+
+class TestInterfaceVLANUpdateNotController(MAASServerTestCase):
+
+    scenarios = (
+        ("machine", {
+            "maker": factory.make_Node,
+        }),
+        ("device", {
+            "maker": factory.make_Device,
+        }),
+    )
+
+    def test__removes_links(self):
+        node = self.maker()
+        interface = factory.make_Interface(INTERFACE_TYPE.PHYSICAL, node=node)
+        static_ip = factory.make_StaticIPAddress(interface=interface)
+        discovered_ip = factory.make_StaticIPAddress(
+            alloc_type=IPADDRESS_TYPE.DISCOVERED, interface=interface)
+        new_fabric = factory.make_Fabric()
+        new_vlan = new_fabric.get_default_vlan()
+        interface.vlan = new_vlan
+        interface.save()
+        self.assertIsNone(reload_object(static_ip))
+        self.assertIsNotNone(reload_object(discovered_ip))
+
+
+class TestInterfaceVLANUpdateController(MAASServerTestCase):
+
+    scenarios = (
+        ("region", {
+            "maker": factory.make_RegionController,
+        }),
+        ("rack", {
+            "maker": factory.make_RackController,
+        }),
+        ("region-rack", {
+            "maker": lambda: factory.make_Node(
+                node_type=NODE_TYPE.REGION_AND_RACK_CONTROLLER)
+        }),
+    )
+
+    def test__moves_link_subnets_to_same_vlan(self):
+        node = self.maker()
+        interface = factory.make_Interface(INTERFACE_TYPE.PHYSICAL, node=node)
+        subnet = factory.make_Subnet(vlan=interface.vlan)
+        factory.make_StaticIPAddress(
+            subnet=subnet, interface=interface)
+        new_fabric = factory.make_Fabric()
+        new_vlan = new_fabric.get_default_vlan()
+        interface.vlan = new_vlan
+        interface.save()
+        self.assertEquals(new_vlan, reload_object(subnet).vlan)
+
+    def test__moves_children_vlans_to_same_fabric(self):
+        node = self.maker()
+        parent = factory.make_Interface(INTERFACE_TYPE.PHYSICAL, node=node)
+        subnet = factory.make_Subnet(vlan=parent.vlan)
+        factory.make_StaticIPAddress(
+            subnet=subnet, interface=parent)
+        old_vlan = factory.make_VLAN(fabric=parent.vlan.fabric)
+        vlan_interface = factory.make_Interface(
+            INTERFACE_TYPE.VLAN, vlan=old_vlan, parents=[parent])
+        vlan_subnet = factory.make_Subnet(vlan=old_vlan)
+        factory.make_StaticIPAddress(
+            subnet=vlan_subnet, interface=vlan_interface)
+        new_fabric = factory.make_Fabric()
+        new_vlan = new_fabric.get_default_vlan()
+        parent.vlan = new_vlan
+        parent.save()
+        self.assertEquals(new_vlan, reload_object(subnet).vlan)
+        vlan_interface = reload_object(vlan_interface)
+        self.assertEquals(
+            (new_fabric.id, old_vlan.vid),
+            (vlan_interface.vlan.fabric.id, vlan_interface.vlan.vid))
+        vlan_subnet = reload_object(vlan_subnet)
+        self.assertEquals(
+            (new_fabric.id, old_vlan.vid),
+            (vlan_subnet.vlan.fabric.id, vlan_subnet.vlan.vid))

=== modified file 'src/maasserver/models/tests/test_interface.py'
--- src/maasserver/models/tests/test_interface.py	2016-02-27 02:31:24 +0000
+++ src/maasserver/models/tests/test_interface.py	2016-03-03 21:19:22 +0000
@@ -241,31 +241,51 @@
                 ["id:%s" % iface1.id, "id:%s" % iface2.id]), [iface1, iface2])
 
     def test__filter_by_specifiers_matches_vid(self):
-        iface1 = factory.make_Interface()
-        iface2 = factory.make_Interface()
-        self.assertItemsEqual(
-            Interface.objects.filter_by_specifiers(
-                "vid:%s" % iface1.vlan.vid), [iface1])
-        self.assertItemsEqual(
-            Interface.objects.filter_by_specifiers(
-                "vid:%s" % iface2.vlan.vid), [iface2])
-        self.assertItemsEqual(
-            Interface.objects.filter_by_specifiers(
-                ["vid:%s" % iface1.vlan.vid, "vid:%s" % iface2.vlan.vid]),
+        fabric1 = factory.make_Fabric()
+        parent1 = factory.make_Interface(
+            INTERFACE_TYPE.PHYSICAL, vlan=fabric1.get_default_vlan())
+        vlan1 = factory.make_VLAN(fabric=fabric1)
+        iface1 = factory.make_Interface(
+            INTERFACE_TYPE.VLAN, vlan=vlan1, parents=[parent1])
+        fabric2 = factory.make_Fabric()
+        parent2 = factory.make_Interface(
+            INTERFACE_TYPE.PHYSICAL, vlan=fabric2.get_default_vlan())
+        vlan2 = factory.make_VLAN(fabric=fabric2)
+        iface2 = factory.make_Interface(
+            INTERFACE_TYPE.VLAN, vlan=vlan2, parents=[parent2])
+        self.assertItemsEqual(
+            Interface.objects.filter_by_specifiers(
+                "vid:%s" % vlan1.vid), [iface1])
+        self.assertItemsEqual(
+            Interface.objects.filter_by_specifiers(
+                "vid:%s" % vlan2.vid), [iface2])
+        self.assertItemsEqual(
+            Interface.objects.filter_by_specifiers(
+                ["vid:%s" % vlan1.vid, "vid:%s" % vlan2.vid]),
             [iface1, iface2])
 
     def test__filter_by_specifiers_matches_vlan(self):
-        iface1 = factory.make_Interface()
-        iface2 = factory.make_Interface()
-        self.assertItemsEqual(
-            Interface.objects.filter_by_specifiers(
-                "vlan:%s" % iface1.vlan.vid), [iface1])
-        self.assertItemsEqual(
-            Interface.objects.filter_by_specifiers(
-                "vlan:%s" % iface2.vlan.vid), [iface2])
-        self.assertItemsEqual(
-            Interface.objects.filter_by_specifiers(
-                ["vlan:%s" % iface1.vlan.vid, "vlan:%s" % iface2.vlan.vid]),
+        fabric1 = factory.make_Fabric()
+        parent1 = factory.make_Interface(
+            INTERFACE_TYPE.PHYSICAL, vlan=fabric1.get_default_vlan())
+        vlan1 = factory.make_VLAN(fabric=fabric1)
+        iface1 = factory.make_Interface(
+            INTERFACE_TYPE.VLAN, vlan=vlan1, parents=[parent1])
+        fabric2 = factory.make_Fabric()
+        parent2 = factory.make_Interface(
+            INTERFACE_TYPE.PHYSICAL, vlan=fabric2.get_default_vlan())
+        vlan2 = factory.make_VLAN(fabric=fabric2)
+        iface2 = factory.make_Interface(
+            INTERFACE_TYPE.VLAN, vlan=vlan2, parents=[parent2])
+        self.assertItemsEqual(
+            Interface.objects.filter_by_specifiers(
+                "vlan:%s" % vlan1.vid), [iface1])
+        self.assertItemsEqual(
+            Interface.objects.filter_by_specifiers(
+                "vlan:%s" % vlan2.vid), [iface2])
+        self.assertItemsEqual(
+            Interface.objects.filter_by_specifiers(
+                ["vlan:%s" % vlan1.vid, "vlan:%s" % vlan2.vid]),
             [iface1, iface2])
 
     def test__filter_by_specifiers_matches_subnet_specifier(self):
@@ -1401,7 +1421,7 @@
             1, interface.ip_addresses.count(),
             "Should only have one IP address assigned.")
 
-    def test__creates_link_up_to_discovered_subnet(self):
+    def test__creates_link_up_to_discovered_subnet_on_same_vlan(self):
         interface = factory.make_Interface(INTERFACE_TYPE.PHYSICAL)
         subnet = factory.make_Subnet(vlan=interface.vlan)
         factory.make_StaticIPAddress(
@@ -1413,6 +1433,18 @@
         self.assertIsNone(link_ip.ip)
         self.assertEqual(subnet, link_ip.subnet)
 
+    def test__creates_link_up_to_no_subnet_when_on_different_vlan(self):
+        interface = factory.make_Interface(INTERFACE_TYPE.PHYSICAL)
+        subnet = factory.make_Subnet()
+        factory.make_StaticIPAddress(
+            alloc_type=IPADDRESS_TYPE.DISCOVERED, ip="",
+            subnet=subnet, interface=interface)
+        interface.ensure_link_up()
+        link_ip = interface.ip_addresses.filter(
+            alloc_type=IPADDRESS_TYPE.STICKY).first()
+        self.assertIsNone(link_ip.ip)
+        self.assertIsNone(link_ip.subnet)
+
     def test__creates_link_up_to_no_subnet(self):
         interface = factory.make_Interface(INTERFACE_TYPE.PHYSICAL)
         interface.ensure_link_up()

=== modified file 'src/maasserver/testing/factory.py'
--- src/maasserver/testing/factory.py	2016-03-02 13:42:26 +0000
+++ src/maasserver/testing/factory.py	2016-03-03 21:19:22 +0000
@@ -413,14 +413,7 @@
                     subnet=subnet, interface=rack_interface)
             else:
                 ip_address = existing_static_ips[0]
-            current_ip_addresses = [
-                sip.ip
-                for sip in StaticIPAddress.objects.filter(
-                    subnet=ip_address.subnet)
-                if sip.ip
-            ]
-            bmc_ip_address = self.pick_ip_in_Subnet(
-                ip_address.subnet, but_not=current_ip_addresses)
+            bmc_ip_address = self.pick_ip_in_Subnet(ip_address.subnet)
             node.power_parameters = {
                 "power_address": "qemu+ssh://user@%s/system" % bmc_ip_address
             }
@@ -610,8 +603,12 @@
         if vlan is None and subnet is not None:
             vlan = subnet.vlan
         if vlan is None:
+            if fabric is None:
+                fabric = factory.make_Fabric()
+            vlan = fabric.get_default_vlan()
             dhcp_on = with_dhcp_rack_primary or with_dhcp_rack_secondary
-            vlan = self.make_VLAN(fabric=fabric, dhcp_on=dhcp_on)
+            vlan.dhcp_on = dhcp_on
+            vlan.save()
         if subnet is None:
             subnet = self.make_Subnet(vlan=vlan, cidr=cidr)
         boot_interface = self.make_Interface(
@@ -661,7 +658,7 @@
                     secondary_rack = vlan.secondary_rack
             vlan.secondary_rack = secondary_rack
             vlan.save()
-        return node
+        return reload_object(node)
 
     UNDEFINED = float('NaN')
 
@@ -860,7 +857,19 @@
         if iftype is None:
             iftype = INTERFACE_TYPE.PHYSICAL
         if vlan is None:
-            vlan = self.make_VLAN(fabric=fabric)
+            if fabric is not None:
+                if iftype == INTERFACE_TYPE.VLAN:
+                    vlan = self.make_VLAN(fabric=fabric)
+                else:
+                    vlan = fabric.get_default_vlan()
+            else:
+                if iftype == INTERFACE_TYPE.VLAN and parents:
+                    vlan = self.make_VLAN(fabric=parents[0].vlan.fabric)
+                elif iftype == INTERFACE_TYPE.BOND and parents:
+                    vlan = parents[0].vlan
+                else:
+                    fabric = self.make_Fabric()
+                    vlan = fabric.get_default_vlan()
         if (mac_address is None and
                 iftype in [
                     INTERFACE_TYPE.PHYSICAL,