Merge ~mpontillo/maas:update-vlans-based-on-discovered-machine-interfaces into maas:master

[Mike Pontillo]

Use information from DiscoveredMachineInterface objects to update dynamically composed machine interfaces.

[Newell Jensen (newell-jensen)]

Question inline.

review: Needs Information

[Mike Pontillo (mpontillo)]

See reply below.

[MAAS Lander (maas-lander)]

UNIT TESTS
-b update-vlans-based-on-discovered-machine-interfaces lp:~mpontillo/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: 25fcb7dc6582d3c42c5ed7683324bc68560c0f4a

review: Approve

[Newell Jensen (newell-jensen)]

Looks good. Thanks for the update!

review: Approve

[MAAS Lander (maas-lander)]

LANDING
-b update-vlans-based-on-discovered-machine-interfaces lp:~mpontillo/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED BUILD
LOG: http://maas-ci-jenkins.internal:8080/job/maas/job/branch-tester/3648/consoleText

[MAAS Lander (maas-lander)]

UNIT TESTS
-b update-vlans-based-on-discovered-machine-interfaces lp:~mpontillo/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: f957d5f40bbd22b1ab13054eb3eb8ecb471508bd

review: Approve

diff --git a/src/maasserver/models/bmc.py b/src/maasserver/models/bmc.py
index 13340f1..be93b79 100644
--- a/src/maasserver/models/bmc.py
+++ b/src/maasserver/models/bmc.py
@@ -821,21 +821,56 @@ class Pod(BMC):
             ]
         # Create the discovered interface and set the default networking
         # configuration.
+        created_interfaces = []
         for idx, discovered_nic in enumerate(discovered_machine.interfaces):
             interface = self._create_interface(
                 discovered_nic, machine, name=interface_names[idx])
+            created_interfaces.append(interface)
             if discovered_nic.boot:
                 machine.boot_interface = interface
                 machine.save(update_fields=['boot_interface'])
         if skip_commissioning:
             machine.set_initial_networking_configuration()
 
+        # We need a second pass here to configure interfaces that the above
+        # function call would otherwise change.
+        if self.host is not None:
+            self._update_vlans_based_on_pod_host(
+                created_interfaces, discovered_machine)
+
         # New machines get commission started immediately unless skipped.
         if not skip_commissioning:
             machine.start_commissioning(commissioning_user)
 
         return machine
 
+    def _update_vlans_based_on_pod_host(
+            self, created_interfaces, discovered_machine):
+        """Matches up newly-created interfaces with interfaces on the pod.host,
+        given a list of interfaces that were created for the machine, and
+        the DiscoveredMachine object.
+        """
+        # Circular imports.
+        from maasserver.models import Interface
+        interfaces = {
+            interface.name: interface
+            for interface in Interface.objects.all_interfaces_parents_first(
+                self.host)
+        }
+        for idx, discovered_nic in enumerate(discovered_machine.interfaces):
+            if discovered_nic.attach_type in ('bridge', 'macvlan'):
+                host_attach_interface = interfaces.get(
+                    discovered_nic.attach_name, None)
+                if host_attach_interface is not None:
+                    # If we get to this point, we found the interface the
+                    # the VM has been attached to. Update the VLAN (but
+                    # only if necessary).
+                    host_vlan = host_attach_interface.vlan
+                    if host_vlan != created_interfaces[idx].vlan:
+                        created_interfaces[idx].vlan = host_vlan
+                        created_interfaces[idx].save()
+                    continue
+
     def _sync_machine(self, discovered_machine, existing_machine):
         """Sync's the information from `discovered_machine` to update
         `existing_machine`."""
diff --git a/src/maasserver/models/signals/interfaces.py b/src/maasserver/models/signals/interfaces.py
index 6e9fb33..fa20c33 100644
--- a/src/maasserver/models/signals/interfaces.py
+++ b/src/maasserver/models/signals/interfaces.py
@@ -264,8 +264,13 @@ def interface_vlan_update(instance, old_values, **kwargs):
         # links except the DISCOVERED ones.
         instance.ip_addresses.exclude(
             alloc_type=IPADDRESS_TYPE.DISCOVERED).delete()
-        log.msg("%s: deleted IP addresses due to VLAN update (%s -> %s)." % (
-            instance.get_log_string(), old_vlan_id, new_vlan_id))
+        if old_vlan_id is not None:
+            # Don't bother logging if the VLAN was previously NULL, since there
+            # shouldn't be any IP addresses on the interface, anyway. (But keep
+            # the above database cleanup, just in case of the unexpected.)
+            log.msg(
+                "%s: deleted IP addresses due to VLAN update (%s -> %s)." % (
+                    instance.get_log_string(), old_vlan_id, new_vlan_id))
 
 
 for klass in INTERFACE_CLASSES:
diff --git a/src/maasserver/models/tests/test_bmc.py b/src/maasserver/models/tests/test_bmc.py
index 9861119..e54a351 100644
--- a/src/maasserver/models/tests/test_bmc.py
+++ b/src/maasserver/models/tests/test_bmc.py
@@ -999,6 +999,50 @@ class TestPod(MAASServerTestCase):
             list(machine.interface_set.order_by('id').values_list(
                 'name', flat=True)))
 
+    def test_create_machine_sets_interface_vlans_using_attach_names(self):
+        discovered_machine = self.make_discovered_machine()
+        self.patch(Machine, "set_default_storage_layout")
+        self.patch(Machine, "set_initial_networking_configuration")
+        self.patch(Machine, "start_commissioning")
+        fabric = factory.make_Fabric()
+        controller = factory.make_RackController()
+        vlan = factory.make_VLAN(
+            fabric=fabric, dhcp_on=True, primary_rack=controller)
+        subnet = factory.make_Subnet()
+        vlan2 = factory.make_VLAN(
+            fabric=fabric, dhcp_on=False, primary_rack=controller)
+        vlan3 = factory.make_VLAN(
+            fabric=fabric, dhcp_on=False, primary_rack=controller)
+        eth0 = factory.make_Interface(node=controller, vlan=vlan)
+        eth1 = factory.make_Interface(node=controller, vlan=vlan2)
+        eth2 = factory.make_Interface(node=controller, vlan=vlan3)
+        br0 = factory.make_Interface(
+            iftype=INTERFACE_TYPE.BRIDGE, node=controller, vlan=vlan,
+            parents=[eth0])
+        ip = factory.make_StaticIPAddress(subnet=subnet, interface=br0)
+        discovered_machine.interfaces[0].attach_type = 'bridge'
+        discovered_machine.interfaces[0].attach_name = br0.name
+        discovered_machine.interfaces[1].attach_type = 'macvlan'
+        discovered_machine.interfaces[1].attach_name = eth1.name
+        discovered_machine.interfaces[2].attach_type = 'macvlan'
+        discovered_machine.interfaces[2].attach_name = eth2.name
+        pod = factory.make_Pod(
+            host=controller, ip_address=ip)
+        machine = pod.create_machine(
+            discovered_machine, factory.make_User(),
+            interfaces=LabeledConstraintMap(
+                'eth0:vlan=id:%d;eth1:vlan=id:%d;eth2:vlan=id:%d' % (
+                    vlan.id, vlan2.id, vlan3.id),
+            )
+        )
+        interfaces = {
+            interface.name: interface
+            for interface in machine.interface_set.all()
+        }
+        self.assertThat(interfaces['eth0'].vlan, Equals(vlan))
+        self.assertThat(interfaces['eth1'].vlan, Equals(vlan2))
+        self.assertThat(interfaces['eth2'].vlan, Equals(vlan3))
+
     def test_create_machine_uses_default_ifnames_if_discovered_mismatch(self):
         """This makes sure that if the discovered machine comes back with
         a different number of interfaces than the constraint string, the