Merge ~mpontillo/maas:dont-create-fabrics-for-disabled-interfaces into maas:master

[Mike Pontillo]

Ensure that disabled interfaces do not create new fabrics.

[Blake Rouse (blake-rouse)]

Looks good.

review: Approve

[MAAS Lander (maas-lander)]

LANDING
-b dont-create-fabrics-for-disabled-interfaces 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/118/console

[Mike Pontillo (mpontillo)]

Looks like some type of random failure with postgresql.

http://paste.ubuntu.com/25124643/

[Andres Rodriguez (andreserl)]

Let's file a bug for that :)

On Wed, Jul 19, 2017 at 12:10 PM Mike Pontillo <email address hidden>
wrote:

> Looks like some type of random failure with postgresql.
>
> http://paste.ubuntu.com/25124643/
> --
> https://code.launchpad.net/~mpontillo/maas/+git/maas/+merge/327688
> Your team MAAS Committers is subscribed to branch maas:master.
>
--
Andres Rodriguez
Engineering Manager, MAAS
Canonical USA, Inc.

[Mike Pontillo (mpontillo)]

I would file a bug, but it would be Incomplete. =(

diff --git a/src/maasserver/models/node.py b/src/maasserver/models/node.py
index 0b92fb1..d27a111 100644
--- a/src/maasserver/models/node.py
+++ b/src/maasserver/models/node.py
@@ -4148,9 +4148,16 @@ class Controller(Node):
                 "name": name,
                 "enabled": is_enabled,
             })
-        if create_fabrics and interface.vlan is None:
-            new_vlan = self._update_vlan_for_interface(
-                interface, config, new_vlan)
+        # Don't update the VLAN unless:
+        # (1) We're at the phase where we're creating fabrics.
+        #     (that is, beaconing has already completed)
+        # (2) The interface's VLAN wasn't previously known.
+        # (3) The interface is administratively enabled.
+        if create_fabrics and interface.vlan is None and is_enabled:
+            new_vlan = self._guess_vlan_for_interface(interface, config)
+            if new_vlan is not None:
+                interface.vlan = new_vlan
+                update_fields.add('vlan')
         if not created:
             if interface.node.id != self.id:
                 # MAC address was on a different node. We need to move
@@ -4187,14 +4194,20 @@ class Controller(Node):
                 # a link re-assigned the VLAN this interface is connected to.
                 new_vlan.fabric.delete()
 
-    def _update_vlan_for_interface(self, interface, config, new_vlan):
+    def _guess_vlan_for_interface(self, interface, config):
         # Make sure that the VLAN on the interface is correct. When
         # links exists on this interface we place it into the correct
         # VLAN. If it cannot be determined and its a new interface it
         # gets placed on its own fabric.
+        new_vlan = None
         connected_to_subnets = self._get_connected_subnets(
             config["links"])
-        if len(connected_to_subnets) == 0:
+        if len(connected_to_subnets) > 0:
+            for subnet in connected_to_subnets:
+                new_vlan = subnet.vlan
+                if new_vlan is not None:
+                    break
+        if new_vlan is None:
             # If the default VLAN on the default fabric has no interfaces
             # associated with it, the first interface will be placed there
             # (rather than creating a new fabric).
@@ -4206,8 +4219,6 @@ class Controller(Node):
             else:
                 new_fabric = Fabric.objects.create()
                 new_vlan = new_fabric.get_default_vlan()
-            interface.vlan = new_vlan
-            interface.save()
         return new_vlan
 
     def _get_or_create_vlan_interface(self, name, vlan, parent):
diff --git a/src/maasserver/models/tests/test_node.py b/src/maasserver/models/tests/test_node.py
index 60d82db..88ad200 100644
--- a/src/maasserver/models/tests/test_node.py
+++ b/src/maasserver/models/tests/test_node.py
@@ -9213,6 +9213,34 @@ class TestUpdateInterfaces(MAASServerTestCase):
         self.assertIsNotNone(eth0)
         self.assertIsNotNone(br0)
 
+    def test_disabled_interfaces_do_not_create_fabrics(self):
+        controller = self.create_empty_controller()
+        interfaces = {
+            'eth0': {
+                'enabled': True,
+                'links': [],
+                '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'
+            },
+        }
+        self.update_interfaces(controller, interfaces)
+        eth0 = get_one(
+            PhysicalInterface.objects.filter(node=controller, name='eth0'))
+        eth1 = get_one(
+            PhysicalInterface.objects.filter(node=controller, name='eth1'))
+        self.assertIsNotNone(eth0.vlan)
+        self.assertIsNone(eth1.vlan)
+
 
 class TestRackControllerRefresh(MAASTransactionServerTestCase):