Merge ~bjornt/maas:bug-2043970-vlan-interfaces-3.4 into maas:3.4

[Björn Tillenius]

Bug #2043970: MAAS 3.2.9 creates for Calico Interfaces 80.000 fabrics

Redo the fix for bug #2043970.

In some cases we need to create VLANs/fabrics for interfaces that don't have
links. For example for VLAN interfaces, often it's the VLAN interface that has
an address, while the underlying physical interface often is without any
address. In that case, the physical interface should have a VLAN that is on the
same fabric as the VLAN interface.

Also removed the check whether a machine is a controller. A controller is not
special. It's a deployed machine. If Calico is installed on a controller, we
should not create useless fabrics.

(cherry picked from commit 94dac1082c3a3958e7c01f29e1d0320a775633ae)
(cherry picked from commit 3c34344be9f9ab6f283a505193c678ff039c17ef)

[Björn Tillenius (bjornt)]

Self-approve backport

review: Approve

diff --git a/src/metadataserver/builtin_scripts/network.py b/src/metadataserver/builtin_scripts/network.py
index 9282986..58f8120 100644
--- a/src/metadataserver/builtin_scripts/network.py
+++ b/src/metadataserver/builtin_scripts/network.py
@@ -13,7 +13,7 @@ from maasserver.models.interface import (
 )
 from maasserver.models.staticipaddress import StaticIPAddress
 from maasserver.models.subnet import Subnet
-from maasserver.models.vlan import VLAN
+from maasserver.models.vlan import DEFAULT_VID, VLAN
 from maasserver.utils.orm import transactional
 from provisioningserver.events import EVENT_TYPES
 from provisioningserver.logger import get_maas_logger
@@ -266,7 +266,7 @@ def update_physical_interface(
         if hints is not None:
             new_vlan = guess_vlan_from_hints(node, name, hints)
         if new_vlan is None:
-            new_vlan = guess_vlan_for_interface(node, links)
+            new_vlan = guess_vlan_for_interface(node, interface, links)
         if new_vlan is not None:
             interface.vlan = new_vlan
             update_fields.add("vlan")
@@ -362,16 +362,19 @@ def update_vlan_interface(node, name, network, links):
     parent_nic = Interface.objects.get(
         node_config=node.current_config, name=parent_name
     )
+    parent_fabric = None
+    if parent_nic.vlan is not None:
+        parent_fabric = parent_nic.vlan.fabric
     links_vlan = get_interface_vlan_from_links(links)
+    interface = VLANInterface.objects.filter(
+        node_config=node.current_config,
+        name=name,
+        parents__id=parent_nic.id,
+        vlan__vid=vid,
+    ).first()
     if links_vlan:
         vlan = links_vlan
-        if parent_nic.vlan.fabric_id != vlan.fabric_id:
-            # XXX: We should surface this error to the API and UI, since
-            # it's something the user needs to fix.
-            maaslog.error(
-                f"Interface '{parent_nic.name}' on controller '{node.hostname}' "
-                f"is not on the same fabric as VLAN interface '{name}'."
-            )
+        parent_fabric = vlan.fabric
         if links_vlan.vid != vid:
             maaslog.error(
                 f"VLAN interface '{name}' reports VLAN {vid} "
@@ -379,41 +382,31 @@ def update_vlan_interface(node, name, network, links):
             )
     else:
         vlan = None
+        if interface is not None and interface.vlan is not None:
+            vlan = interface.vlan
+            parent_fabric = vlan.fabric
+        elif parent_fabric is None:
+            maaslog.info(
+                f"Unable to detect fabric VLAN Interface '{name}', using default"
+            )
+            parent_fabric = Fabric.objects.get_default_fabric()
+
+    if parent_nic.vlan_id is None:
+        parent_vlan, _ = VLAN.objects.get_or_create(
+            fabric=parent_fabric,
+            vid=DEFAULT_VID,
+        )
+        parent_nic.vlan = parent_vlan
+        parent_nic.save()
 
     from metadataserver.builtin_scripts.hooks import HARDWARE_SYNC_ACTIONS
 
-    interface = VLANInterface.objects.filter(
-        node_config=node.current_config,
-        name=name,
-        parents__id=parent_nic.id,
-        vlan__vid=vid,
-    ).first()
-    if interface is None:
-        if vlan is None:
-            # Since no suitable VLAN is found, create a new one in the same
-            # fabric as the parent interface.
-            if parent_nic.vlan:
-                vlan, _ = VLAN.objects.get_or_create(
-                    fabric=parent_nic.vlan.fabric, vid=vid
-                )
-            elif network["addresses"]:  # use IP address to find VLAN
-                address = network["addresses"][0]["address"]
-                try:
-                    subnet = Subnet.objects.get_subnet_for_ip(address)
-                except Subnet.DoesNotExist:
-                    pass
-                else:
-                    vlan, _ = VLAN.objects.get_or_create(
-                        fabric=subnet.vlan.fabric, vid=vid
-                    )
-            if vlan is None:
-                maaslog.error(
-                    f"Unable to detect fabric VLAN Interface '{name}', using default"
-                )
-                vlan, _ = VLAN.objects.get_or_create(
-                    fabric=Fabric.objects.get_default_fabric(), vid=vid
-                )
+    if vlan is None:
+        # Since no suitable VLAN is found, create a new one in the same
+        # fabric as the parent interface.
+        vlan, _ = VLAN.objects.get_or_create(fabric=parent_fabric, vid=vid)
 
+    if interface is None:
         interface, created = VLANInterface.objects.get_or_create(
             node_config=node.current_config,
             name=name,
@@ -435,6 +428,14 @@ def update_vlan_interface(node, name, network, links):
             node, interface, HARDWARE_SYNC_ACTIONS.UPDATED
         )
 
+    if parent_nic.vlan.fabric_id != vlan.fabric_id:
+        # XXX: We should surface this error to the API and UI, since
+        # it's something the user needs to fix.
+        maaslog.error(
+            f"Interface '{parent_nic.name}' on controller '{node.hostname}' "
+            f"is not on the same fabric as VLAN interface '{name}'."
+        )
+
     update_links(node, interface, links, force_vlan=True)
     return interface
 
@@ -507,10 +508,6 @@ def update_parent_vlans(node, interface, parent_nics, update_ip_addresses):
 
 
 def auto_vlan_creation(node) -> bool:
-    if node.is_controller:
-        # Always create controller's vlans
-        return True
-
     if node.status == NODE_STATUS.DEPLOYED:
         # don't create empty vlan/fabric for interfaces discovered by HW sync,
         # they cannot be used.
@@ -519,25 +516,26 @@ def auto_vlan_creation(node) -> bool:
     return Config.objects.get_config("auto_vlan_creation", True)
 
 
-def guess_vlan_for_interface(node, links):
+def guess_vlan_for_interface(node, interface, links):
     # 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 = get_interface_vlan_from_links(links)
-    if new_vlan is None and auto_vlan_creation(node):
-        # 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).
-        default_vlan = VLAN.objects.get_default_vlan()
-        interfaces_on_default_vlan = Interface.objects.filter(
-            vlan=default_vlan
-        ).exists()
-        if not interfaces_on_default_vlan:
-            new_vlan = default_vlan
-        else:
-            new_fabric = Fabric.objects.create()
-            new_vlan = new_fabric.get_default_vlan()
+    if new_vlan is None:
+        if links:
+            update_links(node, interface, links)
+            new_vlan = get_interface_vlan_from_links(links)
+        elif auto_vlan_creation(node):
+            default_vlan = VLAN.objects.get_default_vlan()
+            interfaces_on_default_vlan = Interface.objects.filter(
+                vlan=default_vlan
+            ).exists()
+            if not interfaces_on_default_vlan:
+                new_vlan = default_vlan
+            else:
+                new_fabric = Fabric.objects.create()
+                new_vlan = new_fabric.get_default_vlan()
     return new_vlan
 
 
@@ -636,8 +634,15 @@ def update_links(
     if use_interface_vlan and interface.vlan is not None:
         vlan = interface.vlan
     elif links:
-        fabric = Fabric.objects.create()
-        vlan = fabric.get_default_vlan()
+        default_vlan = VLAN.objects.get_default_vlan()
+        interfaces_on_default_vlan = Interface.objects.filter(
+            vlan=default_vlan
+        ).exists()
+        if not interfaces_on_default_vlan:
+            vlan = default_vlan
+        else:
+            new_fabric = Fabric.objects.create()
+            vlan = new_fabric.get_default_vlan()
         interface.vlan = vlan
         interface.save()
 
diff --git a/src/metadataserver/builtin_scripts/tests/test_network.py b/src/metadataserver/builtin_scripts/tests/test_network.py
index 214df73..24c1fa8 100644
--- a/src/metadataserver/builtin_scripts/tests/test_network.py
+++ b/src/metadataserver/builtin_scripts/tests/test_network.py
@@ -87,7 +87,9 @@ class TestUpdateInterfaces(MAASServerTestCase, UpdateInterfacesMixin):
         eth0 = node.current_config.interface_set.get(name="eth0")
         self.assertEqual(eth0.numa_node, node.default_numanode)
 
-    def test_dont_create_default_vlan_for_deployed_machines(self):
+    def test_dont_create_default_vlan_for_missing_links_deployed_machines(
+        self,
+    ):
         node = factory.make_Machine(status=NODE_STATUS.DEPLOYED)
         data = FakeCommissioningData()
         data_eth0 = data.create_physical_network(
@@ -101,6 +103,174 @@ class TestUpdateInterfaces(MAASServerTestCase, UpdateInterfacesMixin):
         )
         self.assertIsNone(eth0.vlan)
 
+    def test_vlan_interfaces_with_known_link_deployed_machine(self):
+        node = factory.make_Machine(status=NODE_STATUS.DEPLOYED)
+        vlan123 = factory.make_VLAN(vid=123)
+        vlan456 = factory.make_VLAN(vid=456, fabric=vlan123.fabric)
+        factory.make_Subnet(cidr="10.10.10.0/24", vlan=vlan123)
+        factory.make_Subnet(cidr="10.10.20.0/24", vlan=vlan456)
+        data = FakeCommissioningData()
+        data_eth0 = data.create_physical_network(
+            "eth0",
+            mac_address="11:11:11:11:11:11",
+        )
+        data_eth0.state = "up"
+        vlan_network123 = data.create_vlan_network(
+            "eth0.123", parent=data_eth0, vid=123
+        )
+        vlan_network123.addresses = [LXDAddress("10.10.10.10", 24)]
+        vlan_network456 = data.create_vlan_network(
+            "eth0.456", parent=data_eth0, vid=456
+        )
+        vlan_network456.addresses = [LXDAddress("10.10.20.10", 24)]
+        self.update_interfaces(node, data)
+        eth0 = PhysicalInterface.objects.get(
+            name="eth0", node_config=node.current_config
+        )
+        eth0_vlan123 = Interface.objects.get(
+            name="eth0.123", node_config=node.current_config
+        )
+        eth0_vlan456 = Interface.objects.get(
+            name="eth0.456", node_config=node.current_config
+        )
+        self.assertEqual(INTERFACE_TYPE.VLAN, eth0_vlan123.type)
+        self.assertEqual(123, eth0_vlan123.vlan.vid)
+        self.assertEqual(INTERFACE_TYPE.VLAN, eth0_vlan456.type)
+        self.assertEqual(456, eth0_vlan456.vlan.vid)
+        self.assertEqual(
+            eth0_vlan123.vlan.fabric_id, eth0_vlan456.vlan.fabric_id
+        )
+        self.assertIsNotNone(eth0.vlan_id)
+        self.assertNotIn(
+            eth0.vlan_id, [eth0_vlan456.vlan_id, eth0_vlan123.vlan_id]
+        )
+        self.assertEqual(eth0.vlan.fabric_id, eth0_vlan123.vlan.fabric_id)
+        self.assertEqual(eth0.vlan.fabric_id, eth0_vlan456.vlan.fabric_id)
+
+    def test_vlan_interfaces_with_new_known_link_deployed_machine(self):
+        node = factory.make_Machine(status=NODE_STATUS.DEPLOYED)
+        vlan456 = factory.make_VLAN(vid=456)
+        factory.make_Subnet(cidr="10.10.20.0/24", vlan=vlan456)
+        data = FakeCommissioningData()
+        data_eth0 = data.create_physical_network(
+            "eth0",
+            mac_address="11:11:11:11:11:11",
+        )
+        data_eth0.state = "up"
+        vlan_network123 = data.create_vlan_network(
+            "eth0.123", parent=data_eth0, vid=123
+        )
+        vlan_network123.addresses = [LXDAddress("10.10.10.10", 24)]
+        vlan_network456 = data.create_vlan_network(
+            "eth0.456", parent=data_eth0, vid=456
+        )
+        vlan_network456.addresses = [LXDAddress("10.10.20.10", 24)]
+        self.update_interfaces(node, data)
+        eth0 = PhysicalInterface.objects.get(
+            name="eth0", node_config=node.current_config
+        )
+        eth0_vlan123 = Interface.objects.get(
+            name="eth0.123", node_config=node.current_config
+        )
+        eth0_vlan456 = Interface.objects.get(
+            name="eth0.456", node_config=node.current_config
+        )
+        self.assertEqual(INTERFACE_TYPE.VLAN, eth0_vlan123.type)
+        self.assertEqual(123, eth0_vlan123.vlan.vid)
+        self.assertEqual(INTERFACE_TYPE.VLAN, eth0_vlan456.type)
+        self.assertEqual(456, eth0_vlan456.vlan.vid)
+        self.assertEqual(
+            eth0_vlan123.vlan.fabric_id, eth0_vlan456.vlan.fabric_id
+        )
+        self.assertIsNotNone(eth0.vlan_id)
+        self.assertNotIn(
+            eth0.vlan_id, [eth0_vlan456.vlan_id, eth0_vlan123.vlan_id]
+        )
+        self.assertEqual(eth0.vlan.fabric_id, eth0_vlan123.vlan.fabric_id)
+        self.assertEqual(eth0.vlan.fabric_id, eth0_vlan456.vlan.fabric_id)
+
+    def test_vlan_interfaces_with_new_unknown_link_deployed_machine(self):
+        node = factory.make_Machine(status=NODE_STATUS.DEPLOYED)
+        vlan123 = factory.make_VLAN(vid=123)
+        factory.make_Subnet(cidr="10.10.10.0/24", vlan=vlan123)
+        data = FakeCommissioningData()
+        data_eth0 = data.create_physical_network(
+            "eth0",
+            mac_address="11:11:11:11:11:11",
+        )
+        data_eth0.state = "up"
+        vlan_network123 = data.create_vlan_network(
+            "eth0.123", parent=data_eth0, vid=123
+        )
+        vlan_network123.addresses = [LXDAddress("10.10.10.10", 24)]
+        vlan_network456 = data.create_vlan_network(
+            "eth0.456", parent=data_eth0, vid=456
+        )
+        vlan_network456.addresses = [LXDAddress("10.10.20.10", 24)]
+        self.update_interfaces(node, data)
+        eth0 = PhysicalInterface.objects.get(
+            name="eth0", node_config=node.current_config
+        )
+        eth0_vlan123 = Interface.objects.get(
+            name="eth0.123", node_config=node.current_config
+        )
+        eth0_vlan456 = Interface.objects.get(
+            name="eth0.456", node_config=node.current_config
+        )
+        self.assertEqual(INTERFACE_TYPE.VLAN, eth0_vlan123.type)
+        self.assertEqual(123, eth0_vlan123.vlan.vid)
+        self.assertEqual(INTERFACE_TYPE.VLAN, eth0_vlan456.type)
+        self.assertEqual(456, eth0_vlan456.vlan.vid)
+        self.assertEqual(
+            eth0_vlan123.vlan.fabric_id, eth0_vlan456.vlan.fabric_id
+        )
+        self.assertIsNotNone(eth0.vlan_id)
+        self.assertNotIn(
+            eth0.vlan_id, [eth0_vlan456.vlan_id, eth0_vlan123.vlan_id]
+        )
+        self.assertEqual(eth0.vlan.fabric_id, eth0_vlan123.vlan.fabric_id)
+        self.assertEqual(eth0.vlan.fabric_id, eth0_vlan456.vlan.fabric_id)
+
+    def test_vlan_interfaces_with_unknown_link_deployed_machine(self):
+        node = factory.make_Machine(status=NODE_STATUS.DEPLOYED)
+        data = FakeCommissioningData()
+        data_eth0 = data.create_physical_network(
+            "eth0",
+            mac_address="11:11:11:11:11:11",
+        )
+        data_eth0.state = "up"
+        vlan_network123 = data.create_vlan_network(
+            "eth0.123", parent=data_eth0, vid=123
+        )
+        vlan_network123.addresses = [LXDAddress("10.10.10.10", 24)]
+        vlan_network456 = data.create_vlan_network(
+            "eth0.456", parent=data_eth0, vid=456
+        )
+        vlan_network456.addresses = [LXDAddress("10.10.20.10", 24)]
+        self.update_interfaces(node, data)
+        eth0 = PhysicalInterface.objects.get(
+            name="eth0", node_config=node.current_config
+        )
+        eth0_vlan123 = Interface.objects.get(
+            name="eth0.123", node_config=node.current_config
+        )
+        eth0_vlan456 = Interface.objects.get(
+            name="eth0.456", node_config=node.current_config
+        )
+        self.assertEqual(INTERFACE_TYPE.VLAN, eth0_vlan123.type)
+        self.assertEqual(123, eth0_vlan123.vlan.vid)
+        self.assertEqual(INTERFACE_TYPE.VLAN, eth0_vlan456.type)
+        self.assertEqual(456, eth0_vlan456.vlan.vid)
+        self.assertEqual(
+            eth0_vlan123.vlan.fabric_id, eth0_vlan456.vlan.fabric_id
+        )
+        self.assertIsNotNone(eth0.vlan_id)
+        self.assertNotIn(
+            eth0.vlan_id, [eth0_vlan456.vlan_id, eth0_vlan123.vlan_id]
+        )
+        self.assertEqual(eth0.vlan.fabric_id, eth0_vlan123.vlan.fabric_id)
+        self.assertEqual(eth0.vlan.fabric_id, eth0_vlan456.vlan.fabric_id)
+
     def test_dont_create_default_vlan_if_disabled(self):
         Config.objects.set_config(name="auto_vlan_creation", value=False)
         node = factory.make_Machine(status=NODE_STATUS.NEW)
@@ -138,9 +308,7 @@ class TestUpdateInterfaces(MAASServerTestCase, UpdateInterfacesMixin):
         self.assertEqual("11:11:11:11:11:11", eth0.mac_address)
         self.assertTrue(eth0.enabled)
         self.assertTrue(eth0.link_connected)
-        self.assertEqual(
-            Fabric.objects.get_default_fabric().get_default_vlan(), eth0.vlan
-        )
+        self.assertIsNone(eth0.vlan)
         self.assertEqual([], list(eth0.parents.all()))
         eth1 = PhysicalInterface.objects.get(
             name="eth1", node_config=controller.current_config
@@ -547,10 +715,9 @@ class TestUpdateInterfaces(MAASServerTestCase, UpdateInterfacesMixin):
             name="eth0", node_config=controller.current_config
         )
 
-        default_vlan = Fabric.objects.get_default_fabric().get_default_vlan()
         self.assertEqual(INTERFACE_TYPE.PHYSICAL, eth0.type)
         self.assertEqual("11:11:11:11:11:11", eth0.mac_address)
-        self.assertEqual(default_vlan, eth0.vlan)
+        self.assertIsNone(eth0.vlan)
         self.assertTrue(eth0.enabled)
 
         eth0_addresses = list(eth0.ip_addresses.all())
@@ -1103,13 +1270,13 @@ class TestUpdateInterfaces(MAASServerTestCase, UpdateInterfacesMixin):
             maaslog.method_calls,
             [
                 call.error(
-                    f"Interface 'eth0' on controller '{controller.hostname}' "
-                    f"is not on the same fabric as VLAN interface '{vlan_interface.name}'."
-                ),
-                call.error(
                     f"VLAN interface '{vlan_interface.name}' reports VLAN {vid_on_fabric} "
                     f"but links are on VLAN {other_vlan.vid}"
                 ),
+                call.error(
+                    f"Interface 'eth0' on controller '{controller.hostname}' "
+                    f"is not on the same fabric as VLAN interface '{vlan_interface.name}'."
+                ),
             ]
             * passes,
         )
@@ -1270,13 +1437,13 @@ class TestUpdateInterfaces(MAASServerTestCase, UpdateInterfacesMixin):
             maaslog.method_calls,
             [
                 call.error(
-                    f"Interface 'eth0' on controller '{controller.hostname}' "
-                    f"is not on the same fabric as VLAN interface '{vlan_interface.name}'."
-                ),
-                call.error(
                     f"VLAN interface '{vlan_interface.name}' reports VLAN {other_vlan.vid} "
                     f"but links are on VLAN {new_vlan.vid}"
                 ),
+                call.error(
+                    f"Interface 'eth0' on controller '{controller.hostname}' "
+                    f"is not on the same fabric as VLAN interface '{vlan_interface.name}'."
+                ),
             ]
             * passes,
         )
@@ -2224,17 +2391,11 @@ class TestUpdateInterfaces(MAASServerTestCase, UpdateInterfacesMixin):
 
         self.update_interfaces(controller, data)
 
-        eth0 = get_one(
-            PhysicalInterface.objects.filter(
-                node_config=controller.current_config, name="eth0"
-            )
-        )
         eth1 = get_one(
             PhysicalInterface.objects.filter(
                 node_config=controller.current_config, name="eth1"
             )
         )
-        self.assertIsNotNone(eth0.vlan)
         self.assertIsNone(eth1.vlan)
 
     def test_subnet_seen_on_second_controller_does_not_create_fabric(self):