Merge ~mpontillo/maas:fix-ha-rack-dhcp-fabric-creation--bug-1705654 into maas:master

[Mike Pontillo]

Fix fabric creation for rack DHCP IP addresses.

LP: #1705654

[Blake Rouse (blake-rouse)]

Looks good.

review: Approve

[MAAS Lander (maas-lander)]

LANDING
-b fix-ha-rack-dhcp-fabric-creation--bug-1705654 lp:~mpontillo/maas into -b master lp:~maas-committers/maas

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

[MAAS Lander (maas-lander)]

LANDING
-b fix-ha-rack-dhcp-fabric-creation--bug-1705654 lp:~mpontillo/maas into -b master lp:~maas-committers/maas

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

[MAAS Lander (maas-lander)]

LANDING
-b fix-ha-rack-dhcp-fabric-creation--bug-1705654 lp:~mpontillo/maas into -b master lp:~maas-committers/maas

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

[MAAS Lander (maas-lander)]

LANDING
-b fix-ha-rack-dhcp-fabric-creation--bug-1705654 lp:~mpontillo/maas into -b master lp:~maas-committers/maas

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

[MAAS Lander (maas-lander)]

LANDING
-b fix-ha-rack-dhcp-fabric-creation--bug-1705654 lp:~mpontillo/maas into -b master lp:~maas-committers/maas

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

diff --git a/src/maasserver/models/node.py b/src/maasserver/models/node.py
index d27a111..7ff13c5 100644
--- a/src/maasserver/models/node.py
+++ b/src/maasserver/models/node.py
@@ -4184,7 +4184,7 @@ class Controller(Node):
     def _update_physical_links(self, interface, config, new_vlan,
                                update_fields):
         update_ip_addresses = self._update_links(interface, config["links"])
-        linked_vlan = self._get_first_sticky_vlan_from_ip_addresses(
+        linked_vlan = self._guess_best_vlan_from_ip_addresses(
             update_ip_addresses)
         if linked_vlan is not None:
             interface.vlan = linked_vlan
@@ -4381,7 +4381,7 @@ class Controller(Node):
         If a static IP address is allocated, give preferential treatment to
         the VLAN that IP address resides on.
         """
-        linked_vlan = self._get_first_sticky_vlan_from_ip_addresses(
+        linked_vlan = self._guess_best_vlan_from_ip_addresses(
             update_ip_addresses)
         if linked_vlan is not None:
             interface.vlan = linked_vlan
@@ -4417,8 +4417,11 @@ class Controller(Node):
         """Return a set of subnets that `links` belongs to."""
         subnets = set()
         for link in links:
-            if link["mode"] == "static":
-                ip_addr = IPNetwork(link["address"])
+            if link["mode"] == "static" or link["mode"] == "dhcp":
+                address = link.get("address", None)
+                if address is None:
+                    continue
+                ip_addr = IPNetwork(address)
                 subnet = get_one(Subnet.objects.filter(cidr=str(ip_addr.cidr)))
                 if subnet is not None:
                     subnets.add(subnet)
@@ -4439,12 +4442,15 @@ class Controller(Node):
                 return ip_address
         return None
 
-    def _get_first_sticky_vlan_from_ip_addresses(self, ip_addresses):
+    def _guess_best_vlan_from_ip_addresses(self, ip_addresses):
         """Return the first VLAN for a STICKY IP address in `ip_addresses`."""
+        second_best = None
         for ip_address in ip_addresses:
             if ip_address.alloc_type == IPADDRESS_TYPE.STICKY:
                 return ip_address.subnet.vlan
-        return None
+            elif ip_address.alloc_type == IPADDRESS_TYPE.DISCOVERED:
+                second_best = ip_address.subnet.vlan
+        return second_best
 
     def _update_links(
             self, interface, links, force_vlan=False, use_interface_vlan=True):
diff --git a/src/maasserver/models/signals/interfaces.py b/src/maasserver/models/signals/interfaces.py
index 5c6e75a..baac5eb 100644
--- a/src/maasserver/models/signals/interfaces.py
+++ b/src/maasserver/models/signals/interfaces.py
@@ -214,6 +214,8 @@ def interface_vlan_update(instance, old_values, **kwargs):
             NODE_TYPE.REGION_CONTROLLER,
             NODE_TYPE.RACK_CONTROLLER,
             NODE_TYPE.REGION_AND_RACK_CONTROLLER):
+        if old_vlan_id is None:
+            return
         # Interface VLAN was changed on a controller. Move all linked subnets
         # to that new VLAN, unless the new VLAN is None. When the VLAN is
         # None then the administrator is say that the interface is now
diff --git a/src/maasserver/models/signals/tests/test_interfaces.py b/src/maasserver/models/signals/tests/test_interfaces.py
index 6af1fa9..d15235b 100644
--- a/src/maasserver/models/signals/tests/test_interfaces.py
+++ b/src/maasserver/models/signals/tests/test_interfaces.py
@@ -240,7 +240,7 @@ class TestInterfaceVLANUpdateController(MAASServerTestCase):
         interface.save()
         self.assertEquals(new_vlan, reload_object(subnet).vlan)
 
-    def test__doesnt_move_link_subnets_when_vlan_is_None(self):
+    def test__doesnt_move_link_subnets_when_target_vlan_is_None(self):
         node = self.maker()
         interface = factory.make_Interface(INTERFACE_TYPE.PHYSICAL, node=node)
         old_vlan = interface.vlan
@@ -251,6 +251,18 @@ class TestInterfaceVLANUpdateController(MAASServerTestCase):
         interface.save()
         self.assertEquals(old_vlan, reload_object(subnet).vlan)
 
+    def test__doesnt_move_link_subnets_when_source_vlan_is_None(self):
+        node = self.maker()
+        interface = factory.make_Interface(
+            INTERFACE_TYPE.PHYSICAL, node=node, vlan=None)
+        old_vlan = interface.vlan
+        subnet = factory.make_Subnet(vlan=old_vlan)
+        factory.make_StaticIPAddress(
+            subnet=subnet, interface=interface)
+        interface.vlan = factory.make_VLAN()
+        interface.save()
+        self.assertEquals(interface.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)
diff --git a/src/maasserver/models/tests/test_node.py b/src/maasserver/models/tests/test_node.py
index 88ad200..42fa230 100644
--- a/src/maasserver/models/tests/test_node.py
+++ b/src/maasserver/models/tests/test_node.py
@@ -9241,6 +9241,53 @@ class TestUpdateInterfaces(MAASServerTestCase):
         self.assertIsNotNone(eth0.vlan)
         self.assertIsNone(eth1.vlan)
 
+    def test_subnet_seen_on_second_controller_does_not_create_fabric(self):
+        alice = self.create_empty_controller()
+        bob = self.create_empty_controller()
+        alice_interfaces = {
+            'eth0': {
+                'enabled': True,
+                'links': [{'address': '192.168.0.1/24', 'mode': 'dhcp'}],
+                'mac_address': '52:54:00:77:15:e3',
+                'parents': [],
+                'source': 'ipaddr',
+                'type': 'physical'
+            },
+            'eth1': {
+                'enabled': False,
+                'links': [],
+                'mac_address': '52:54:00:77:15:e4',
+                'parents': [],
+                'source': 'ipaddr',
+                'type': 'physical'
+            },
+        }
+        bob_interfaces = {
+            'eth0': {
+                'enabled': True,
+                'links': [{'address': '192.168.0.2/24', 'mode': 'dhcp'}],
+                'mac_address': '52:54:00:87:25:f3',
+                'parents': [],
+                'source': 'ipaddr',
+                'type': 'physical'
+            },
+            'eth1': {
+                'enabled': False,
+                'links': [],
+                'mac_address': '52:54:00:87:25:f4',
+                'parents': [],
+                'source': 'ipaddr',
+                'type': 'physical'
+            },
+        }
+        self.update_interfaces(alice, alice_interfaces)
+        self.update_interfaces(bob, bob_interfaces)
+        alice_eth0 = get_one(
+            PhysicalInterface.objects.filter(node=alice, name='eth0'))
+        bob_eth0 = get_one(
+            PhysicalInterface.objects.filter(node=bob, name='eth0'))
+        self.assertThat(alice_eth0.vlan, Equals(bob_eth0.vlan))
+
 
 class TestRackControllerRefresh(MAASTransactionServerTestCase):