Merge lp:~mpontillo/maas/fix-bridges-on-vlans-1555679 into lp:~maas-committers/maas/trunk

[Mike Pontillo]

Fix bugs #1555679 and #1564657.

 * Only physical interfaces (and sometimes bonds) will now be matched
   by MAC address. For other interface types, it is legal for duplicates
   to be found. (though it will now be logged as a warning)
 * Removed a poorly-tested warning message which could sometimes trigger
   in incorrect situations.
 * Fix signal handling for interface save to avoid infinite recursion.
   (Note: indirectly tested by rack interface update tests, since
   without it, infinite recursion is seen in the tests.)
 * Fix MAC validation to consider all ancestors and successors, rather
   than just immediate family.
 * Remove tests for bonds and bridges "moving with MAC address" (they are
   now recreated if renamed, since it's not possible to uniquely identify
   a bond or bridge by MAC).
 * Add tests for multiple bridges with the same MAC as their parents.

[Gavin Panella (allenap)]

I truly don't have the experience of this part of MAAS to do a decent review, but I noticed one thing that I had to comment on.

review: Needs Fixing

[Mike Pontillo (mpontillo)]

Thanks Gavin. Could you take another look at the thread local bits? I've changed it to a subclass. I was thinking about how to test this change. I feel like it's redundant since it just tests what the standard library should already ensure, and it's also tested indirectly by the rack interface update/registration code. But it doesn't stress the multi-thread aspect. Thoughts?

[Mike Pontillo (mpontillo)]

Here's what I came up with for the test (see pastebin if you don't want to wade through the diff).

https://paste.ubuntu.com/15695629/

[Blake Rouse (blake-rouse)]

Looks really good. Just one comment that I am not going to block you on, but please fix before landing.

review: Approve

[Mike Pontillo (mpontillo)]

Thanks for the review. I'm going to go ahead and land this for the beta; Gavin, if you have more comments regarding the thread-local usage, I'll address them in a follow-on branch.

[Gavin Panella (allenap)]

I think you're right to be conservative with change right now, i.e. stick with a signal handler, even if doing this in save() might be better. I have a couple of really minor comments, neither of which materially affects this landing, and a +1 for posterity.

review: Approve

=== modified file 'src/maasserver/models/interface.py'
--- src/maasserver/models/interface.py	2016-04-06 05:03:18 +0000
+++ src/maasserver/models/interface.py	2016-04-08 19:03:47 +0000
@@ -305,10 +305,18 @@
         If the interface being created is a replacement for an interface that
         already exists, the caller is responsible for deleting it.
         """
-        interface = self.get_queryset().filter(
-            Q(mac_address=mac_address) | Q(name=name) & Q(node=node)).first()
+        if self.model.get_type() == INTERFACE_TYPE.PHYSICAL:
+            # Physical interfaces have a MAC uniqueness restriction.
+            interfaces = self.get_queryset().filter(
+                Q(mac_address=mac_address) | Q(name=name) & Q(node=node))
+            interface = interfaces.first()
+        else:
+            interfaces = self.get_queryset().filter(
+                Q(name=name) & Q(node=node) & Q(type=self.model.get_type()))
+            interface = interfaces.first()
         if interface is not None:
-            if interface.type != self.model.get_type():
+            if (interface.type != self.model.get_type() and
+                    interface.node.id == node.id):
                 # This means we found the interface on this node, but the type
                 # didn't match what we expected. This should not happen unless
                 # we changed the modeling of this interface type, or the admin
@@ -927,6 +935,36 @@
         auto_ip.save()
         return subnet, auto_ip
 
+    def get_ancestors(self):
+        """Returns all the ancestors of the interface (that is, including each
+        parent's parents, and so on.)
+        """
+        parents = set(rel.parent for rel in self.parent_relationships.all())
+        parent_relationships = set(self.parent_relationships.all())
+        for parent_rel in parent_relationships:
+            parents |= parent_rel.parent.get_ancestors()
+        return parents
+
+    def get_successors(self):
+        """Returns all the ancestors of the interface (that is, including each
+        child's children, and so on.)
+        """
+        children = set(rel.child for rel in self.children_relationships.all())
+        children_relationships = set(self.children_relationships.all())
+        for child_rel in children_relationships:
+            children |= child_rel.child.get_successors()
+        return children
+
+    def get_all_related_interfaces(self):
+        """Returns all of the related interfaces (including any ancestors,
+        successors, and ancestors' successors)."""
+        ancestors = self.get_ancestors()
+        all_related = set()
+        all_related |= ancestors
+        for ancestor in ancestors:
+            all_related |= ancestor.get_successors()
+        return all_related
+
     def delete(self, remove_ip_address=True):
         # We set the _skip_ip_address_removal so the signal can use it to
         # skip removing the IP addresses. This is normally only done by the
@@ -1043,7 +1081,7 @@
         if len(validation_errors) > 0:
             raise ValidationError(validation_errors)
 
-        # MAC address must be unique for all other PhysicalInterface's.
+        # MAC address must be unique amongst every PhysicalInterface.
         other_interfaces = PhysicalInterface.objects.filter(
             mac_address=self.mac_address)
         if self.id is not None:
@@ -1127,35 +1165,29 @@
         # its soon to be parents MAC address is already in use.
         if self.id is not None:
             interfaces = Interface.objects.filter(mac_address=self.mac_address)
-            parent_ids = [
+            related_ids = [
                 parent.id
-                for parent in self.parents.all()
-            ]
-            children_ids = [
-                rel.child.id
-                for rel in self.children_relationships.all()
+                for parent in self.get_all_related_interfaces()
             ]
             bad_interfaces = []
             for interface in interfaces:
                 if self.id == interface.id:
                     # Self in database so ignore.
                     continue
-                elif interface.id in parent_ids:
-                    # One of the parent MAC addresses.
-                    continue
-                elif interface.id in children_ids:
-                    # One of the children MAC addresses.
+                elif interface.id in related_ids:
+                    # Found the same MAC on either a parent, a child, or
+                    # another of the parents' children.
                     continue
                 else:
                     # Its not unique and its not a parent interface if we
                     # made it this far.
                     bad_interfaces.append(interface)
             if len(bad_interfaces) > 0:
-                raise ValidationError({
-                    "mac_address": [
-                        "This MAC address is already in use by %s." % (
-                            bad_interfaces[0].node.hostname)]
-                })
+                maaslog.warning(
+                    "While adding %s: "
+                    "found a MAC address already in use by %s." % (
+                        self.get_log_string(),
+                        bad_interfaces[0].get_log_string()))
 
 
 class BridgeInterface(ChildInterface):
@@ -1351,11 +1383,11 @@
             other_interfaces = other_interfaces.exclude(id=self.id)
         other_interfaces = other_interfaces.all()
         if len(other_interfaces) > 0:
-            raise ValidationError({
-                "mac_address": [
-                    "This MAC address is already in use by %s." % (
-                        other_interfaces[0].get_log_string())]
-                })
+            maaslog.warning(
+                "While adding %s: "
+                "found a MAC address already in use by %s." % (
+                    self.get_log_string(),
+                    other_interfaces[0].get_log_string()))
 
         # Cannot have any parents.
         if self.id is not None:

=== modified file 'src/maasserver/models/node.py'
--- src/maasserver/models/node.py	2016-04-06 05:03:33 +0000
+++ src/maasserver/models/node.py	2016-04-08 19:03:47 +0000
@@ -3243,20 +3243,6 @@
         parent_nics = Interface.objects.get_interfaces_on_node_by_name(
             self, ifnames)
 
-        # If we didn't create the parents yet, we need to know about it,
-        # because that indicates a potentially serious bug.
-        if len(config["parents"]) != len(parent_nics):
-            maaslog.warning(
-                "Could not find all parent interfaces for {ifname} "
-                "on {node_name}. Found: {modeled_interfaces}; "
-                "expected: {expected_interfaces}".format(
-                    ifname=name,
-                    node_name=self.hostname,
-                    modeled_interfaces=[
-                        iface.name for iface in parent_nics
-                    ].join(", "),
-                    expected_interfaces=ifnames.join(", ")))
-
         # Ignore child interfaces that don't have parents. MAAS won't know what
         # to do with them since they can't be connected to a fabric.
         if len(parent_nics) == 0:

=== modified file 'src/maasserver/models/signals/interfaces.py'
--- src/maasserver/models/signals/interfaces.py	2016-03-28 13:54:47 +0000
+++ src/maasserver/models/signals/interfaces.py	2016-04-08 19:03:47 +0000
@@ -7,6 +7,8 @@
     "signals",
 ]
 
+import threading
+
 from django.db.models.signals import post_save
 from maasserver.enum import (
     INTERFACE_TYPE,
@@ -129,16 +131,34 @@
         interface_mtu_params_update,
         klass, ['params'], delete=False)
 
+thread_local = threading.local()
+
+
+class InterfaceUpdateParentsThreadLocal(threading.local):
+    """Since infinite recursion could occur in an arbitrary interface
+    hierarchy, use thread-local stroage to ensure that each interface is only
+    visited once.
+    """
+    def __init__(self):
+        super().__init__()
+        self.visiting = set()
+
+update_parents_thread_local = InterfaceUpdateParentsThreadLocal()
+
 
 def update_interface_parents(sender, instance, created, **kwargs):
     """Update parents when an interface is created."""
     if instance.type in (INTERFACE_TYPE.BOND, INTERFACE_TYPE.BRIDGE):
+        visiting = update_parents_thread_local.visiting
         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()
+            if parent.vlan != instance.vlan and parent.id not in visiting:
+                try:
+                    visiting.add(parent.id)
+                    parent.vlan = instance.vlan
+                    parent.save()
+                finally:
+                    visiting.discard(parent.id)
 
 
 for klass in INTERFACE_CLASSES:

=== modified file 'src/maasserver/models/signals/tests/test_interfaces.py'
--- src/maasserver/models/signals/tests/test_interfaces.py	2016-03-28 13:54:47 +0000
+++ src/maasserver/models/signals/tests/test_interfaces.py	2016-04-08 19:03:47 +0000
@@ -6,15 +6,21 @@
 __all__ = []
 
 import random
+import threading
 
 from maasserver.enum import (
     INTERFACE_TYPE,
     IPADDRESS_TYPE,
     NODE_TYPE,
 )
+from maasserver.models.signals.interfaces import update_parents_thread_local
 from maasserver.testing.factory import factory
 from maasserver.testing.testcase import MAASServerTestCase
 from maasserver.utils.orm import reload_object
+from testtools.matchers import (
+    Contains,
+    Not,
+)
 
 
 class TestEnableAndDisableInterface(MAASServerTestCase):
@@ -133,16 +139,16 @@
         ("bridge", {"iftype": INTERFACE_TYPE.BRIDGE}),
     )
 
-    def test__updates_bond_parents(self):
+    def test__updates_interface_parents(self):
         parent1 = factory.make_Interface(INTERFACE_TYPE.PHYSICAL)
         parent2 = factory.make_Interface(
             INTERFACE_TYPE.PHYSICAL, node=parent1.node)
-        bond = factory.make_Interface(
+        child = factory.make_Interface(
             self.iftype, parents=[parent1, parent2])
-        self.assertEqual(bond.vlan, reload_object(parent1).vlan)
-        self.assertEqual(bond.vlan, reload_object(parent2).vlan)
+        self.assertEqual(child.vlan, reload_object(parent1).vlan)
+        self.assertEqual(child.vlan, reload_object(parent2).vlan)
 
-    def test__update_bond_clears_parent_links(self):
+    def test__update_interface_clears_parent_links(self):
         parent1 = factory.make_Interface(INTERFACE_TYPE.PHYSICAL)
         parent2 = factory.make_Interface(
             INTERFACE_TYPE.PHYSICAL, node=parent1.node)
@@ -151,6 +157,18 @@
             self.iftype, parents=[parent1, parent2])
         self.assertIsNone(reload_object(static_ip))
 
+    def test__visited_set_is_thread_local(self):
+        thread_local = update_parents_thread_local
+        thread_local.visiting.add("a")
+
+        def check_a_does_not_exist():
+            self.assertThat(thread_local.visiting, Not(Contains("a")))
+        thread = threading.Thread(target=check_a_does_not_exist)
+        thread.start()
+        thread.join()
+        self.assertThat(thread_local.visiting, Contains("a"))
+        thread_local.visiting.discard("a")
+
 
 class TestInterfaceVLANUpdateNotController(MAASServerTestCase):
 

=== modified file 'src/maasserver/models/tests/test_interface.py'
--- src/maasserver/models/tests/test_interface.py	2016-04-06 17:22:20 +0000
+++ src/maasserver/models/tests/test_interface.py	2016-04-08 19:03:47 +0000
@@ -13,6 +13,7 @@
 )
 from django.db import transaction
 from django.http import Http404
+from fixtures import FakeLogger
 from maasserver.enum import (
     INTERFACE_LINK_TYPE,
     INTERFACE_TYPE,
@@ -64,6 +65,7 @@
 )
 from testtools import ExpectedException
 from testtools.matchers import (
+    Contains,
     Equals,
     MatchesDict,
     MatchesListwise,
@@ -648,6 +650,39 @@
         node = reload_object(node)
         self.assertIsNone(node.gateway_link_ipv6)
 
+    def test_get_ancestors_includes_grandparents(self):
+        node = factory.make_Node()
+        eth0 = factory.make_Interface(INTERFACE_TYPE.PHYSICAL, node=node)
+        eth0_100 = factory.make_Interface(
+            INTERFACE_TYPE.VLAN, node=node, parents=[eth0])
+        br0 = factory.make_Interface(
+            INTERFACE_TYPE.BRIDGE, node=node, parents=[eth0_100])
+        self.assertThat(
+            br0.get_ancestors(), Equals({eth0, eth0_100}))
+
+    def test_get_successors_includes_grandchildren(self):
+        node = factory.make_Node()
+        eth0 = factory.make_Interface(INTERFACE_TYPE.PHYSICAL, node=node)
+        eth0_100 = factory.make_Interface(
+            INTERFACE_TYPE.VLAN, node=node, parents=[eth0])
+        br0 = factory.make_Interface(
+            INTERFACE_TYPE.BRIDGE, node=node, parents=[eth0_100])
+        self.assertThat(
+            eth0.get_successors(), Equals({eth0_100, br0}))
+
+    def test_get_all_related_interafces_includes_all_related(self):
+        node = factory.make_Node()
+        eth0 = factory.make_Interface(INTERFACE_TYPE.PHYSICAL, node=node)
+        eth0_100 = factory.make_Interface(
+            INTERFACE_TYPE.VLAN, node=node, parents=[eth0])
+        eth0_101 = factory.make_Interface(
+            INTERFACE_TYPE.VLAN, node=node, parents=[eth0])
+        br0 = factory.make_Interface(
+            INTERFACE_TYPE.BRIDGE, node=node, parents=[eth0_100])
+        self.assertThat(
+            eth0_100.get_all_related_interfaces(), Equals(
+                {eth0, eth0_100, eth0_101, br0}))
+
 
 class PhysicalInterfaceTest(MAASServerTestCase):
 
@@ -930,21 +965,19 @@
             INTERFACE_TYPE.BOND, mac_address=factory.make_mac_address(),
             parents=[parent1, parent2])
 
-    def test_cannot_use_none_unique_mac_address(self):
+    def test_warns_for_non_unique_mac_address(self):
+        logger = self.useFixture(FakeLogger("maas"))
         node = factory.make_Node()
         other_nic = factory.make_Interface(INTERFACE_TYPE.PHYSICAL)
         parent1 = factory.make_Interface(INTERFACE_TYPE.PHYSICAL, node=node)
         parent2 = factory.make_Interface(INTERFACE_TYPE.PHYSICAL, node=node)
-        # Test is that no error is raised.
-        error = self.assertRaises(
-            ValidationError, factory.make_Interface,
+        iface = factory.make_Interface(
             INTERFACE_TYPE.BOND, mac_address=other_nic.mac_address,
             parents=[parent1, parent2])
-        self.assertEqual({
-            "mac_address": [
-                "This MAC address is already in use by %s." % (
-                    other_nic.node.hostname)]
-            }, error.message_dict)
+        self.assertThat(logger.output, Contains(
+            "While adding %s: "
+            "found a MAC address already in use by %s." % (
+                iface.get_log_string(), other_nic.get_log_string())))
 
     def test_node_is_set_to_parents_node(self):
         node = factory.make_Node()
@@ -1049,21 +1082,20 @@
             INTERFACE_TYPE.BRIDGE, mac_address=factory.make_mac_address(),
             parents=[parent1, parent2])
 
-    def test_cannot_use_none_unique_mac_address(self):
+    def test_warns_for_non_unique_mac_address(self):
+        logger = self.useFixture(FakeLogger("maas"))
         node = factory.make_Node()
         other_nic = factory.make_Interface(INTERFACE_TYPE.PHYSICAL)
         parent1 = factory.make_Interface(INTERFACE_TYPE.PHYSICAL, node=node)
         parent2 = factory.make_Interface(INTERFACE_TYPE.PHYSICAL, node=node)
         # Test is that no error is raised.
-        error = self.assertRaises(
-            ValidationError, factory.make_Interface,
+        iface = factory.make_Interface(
             INTERFACE_TYPE.BRIDGE, mac_address=other_nic.mac_address,
             parents=[parent1, parent2])
-        self.assertEqual({
-            "mac_address": [
-                "This MAC address is already in use by %s." % (
-                    other_nic.node.hostname)]
-            }, error.message_dict)
+        self.assertThat(logger.output, Contains(
+            "While adding %s: "
+            "found a MAC address already in use by %s." % (
+                iface.get_log_string(), other_nic.get_log_string())))
 
     def test_node_is_set_to_parents_node(self):
         node = factory.make_Node()
@@ -1122,16 +1154,16 @@
             "node": ["This field must be blank."]
             }, error.message_dict)
 
-    def test_mac_address_must_be_unique(self):
+    def test_warns_for_non_unique_unknown_mac(self):
+        logger = self.useFixture(FakeLogger("maas"))
         interface = factory.make_Interface(INTERFACE_TYPE.PHYSICAL)
         unknown = UnknownInterface(
             name="eth0", mac_address=interface.mac_address)
-        error = self.assertRaises(ValidationError, unknown.save)
-        self.assertEqual({
-            "mac_address": [
-                "This MAC address is already in use by %s." % (
-                    interface.get_log_string())]
-            }, error.message_dict)
+        unknown.save()
+        self.assertThat(logger.output, Contains(
+            "While adding %s: "
+            "found a MAC address already in use by %s." % (
+                unknown.get_log_string(), interface.get_log_string())))
 
 
 class UpdateIpAddressesTest(MAASServerTestCase):

=== modified file 'src/maasserver/models/tests/test_node.py'
--- src/maasserver/models/tests/test_node.py	2016-04-06 05:08:54 +0000
+++ src/maasserver/models/tests/test_node.py	2016-04-08 19:03:47 +0000
@@ -5579,112 +5579,6 @@
             [parent.name for parent in br0.parents.all()],
             Equals(["eth0"]))
 
-    def test__bond_moves_bond_with_mac_address(self):
-        rack = self.create_empty_rack_controller()
-        fabric = factory.make_Fabric()
-        vlan = fabric.get_default_vlan()
-        eth0 = factory.make_Interface(
-            INTERFACE_TYPE.PHYSICAL, node=rack, vlan=vlan)
-        eth1 = factory.make_Interface(
-            INTERFACE_TYPE.PHYSICAL, node=rack, vlan=vlan)
-        other_node = factory.make_Node()
-        other_nic = factory.make_Interface(
-            INTERFACE_TYPE.PHYSICAL, node=other_node)
-        bond_mac_address = factory.make_mac_address()
-        bond_to_move = factory.make_Interface(
-            INTERFACE_TYPE.BOND, parents=[other_nic], node=other_node,
-            mac_address=bond_mac_address)
-        interfaces = {
-            "eth0": {
-                "type": "physical",
-                "mac_address": eth0.mac_address,
-                "parents": [],
-                "links": [],
-                "enabled": True,
-            },
-            "eth1": {
-                "type": "physical",
-                "mac_address": eth1.mac_address,
-                "parents": [],
-                "links": [],
-                "enabled": True,
-            },
-            "bond0": {
-                "type": "bond",
-                "mac_address": bond_mac_address,
-                "parents": ["eth0", "eth1"],
-                "links": [],
-                "enabled": True,
-            },
-        }
-        rack.update_interfaces(interfaces)
-        self.assertThat(rack.interface_set.count(), Equals(3))
-        self.assertThat(
-            reload_object(bond_to_move), MatchesStructure.byEquality(
-                type=INTERFACE_TYPE.BOND,
-                name="bond0",
-                mac_address=bond_mac_address,
-                enabled=True,
-                node=rack,
-                vlan=vlan,
-            ))
-        self.assertThat(
-            [parent.name for parent in bond_to_move.parents.all()],
-            MatchesSetwise(Equals("eth0"), Equals("eth1")))
-
-    def test__bridge_moves_bridge_with_mac_address(self):
-        rack = self.create_empty_rack_controller()
-        fabric = factory.make_Fabric()
-        vlan = fabric.get_default_vlan()
-        eth0 = factory.make_Interface(
-            INTERFACE_TYPE.PHYSICAL, node=rack, vlan=vlan)
-        eth1 = factory.make_Interface(
-            INTERFACE_TYPE.PHYSICAL, node=rack, vlan=vlan)
-        other_node = factory.make_Node()
-        other_nic = factory.make_Interface(
-            INTERFACE_TYPE.PHYSICAL, node=other_node)
-        bridge_mac_address = factory.make_mac_address()
-        bridge_to_move = factory.make_Interface(
-            INTERFACE_TYPE.BRIDGE, parents=[other_nic], node=other_node,
-            mac_address=bridge_mac_address)
-        interfaces = {
-            "eth0": {
-                "type": "physical",
-                "mac_address": eth0.mac_address,
-                "parents": [],
-                "links": [],
-                "enabled": True,
-            },
-            "eth1": {
-                "type": "physical",
-                "mac_address": eth1.mac_address,
-                "parents": [],
-                "links": [],
-                "enabled": True,
-            },
-            "br0": {
-                "type": "bridge",
-                "mac_address": bridge_mac_address,
-                "parents": ["eth0", "eth1"],
-                "links": [],
-                "enabled": True,
-            },
-        }
-        rack.update_interfaces(interfaces)
-        self.assertThat(rack.interface_set.count(), Equals(3))
-        self.assertThat(
-            reload_object(bridge_to_move), MatchesStructure.byEquality(
-                type=INTERFACE_TYPE.BRIDGE,
-                name="br0",
-                mac_address=bridge_mac_address,
-                enabled=True,
-                node=rack,
-                vlan=vlan,
-            ))
-        self.assertThat(
-            [parent.name for parent in bridge_to_move.parents.all()],
-            MatchesSetwise(Equals("eth0"), Equals("eth1")))
-
     def test__bond_creates_link_updates_parent_vlan(self):
         rack = self.create_empty_rack_controller()
         fabric = factory.make_Fabric()
@@ -5744,8 +5638,9 @@
                 enabled=True,
                 vlan=bond0_vlan,
             ))
+        bond0 = get_one(Interface.objects.filter_by_ip(str(ip)))
         self.assertThat(
-            reload_object(bond0), MatchesStructure.byEquality(
+            bond0, MatchesStructure.byEquality(
                 type=INTERFACE_TYPE.BOND,
                 name="bond0",
                 mac_address=bond0.mac_address,
@@ -5816,8 +5711,9 @@
                 enabled=True,
                 vlan=br0_vlan,
             ))
+        br0 = get_one(Interface.objects.filter_by_ip(str(ip)))
         self.assertThat(
-            reload_object(br0), MatchesStructure.byEquality(
+            br0, MatchesStructure.byEquality(
                 type=INTERFACE_TYPE.BRIDGE,
                 name="br0",
                 mac_address=br0.mac_address,
@@ -5853,7 +5749,7 @@
         eth1 = factory.make_Interface(
             INTERFACE_TYPE.PHYSICAL, node=rack, vlan=vlan)
         bond0 = factory.make_Interface(
-            INTERFACE_TYPE.BOND, parents=[eth0, eth1], vlan=vlan)
+            INTERFACE_TYPE.BOND, name="bond0", parents=[eth0, eth1], vlan=vlan)
         interfaces = {
             "eth0": {
                 "type": "physical",
@@ -5884,7 +5780,7 @@
         eth1 = factory.make_Interface(
             INTERFACE_TYPE.PHYSICAL, node=rack, vlan=vlan)
         br0 = factory.make_Interface(
-            INTERFACE_TYPE.BRIDGE, parents=[eth0, eth1], vlan=vlan)
+            INTERFACE_TYPE.BRIDGE, name="br0", parents=[eth0, eth1], vlan=vlan)
         interfaces = {
             "eth0": {
                 "type": "physical",
@@ -6355,6 +6251,381 @@
         r2_ens5_16 = get_one(Interface.objects.filter_by_ip("10.16.0.3"))
         self.assertIsNotNone(r2_ens5_16)
 
+    def test__all_new_bridge_on_vlan_interface_with_identical_macs(self):
+        rack = self.create_empty_rack_controller()
+        default_vlan = VLAN.objects.get_default_vlan()
+        br0_fabric = factory.make_Fabric()
+        eth0_100_vlan = factory.make_VLAN(vid=100, fabric=br0_fabric)
+        br0_subnet = factory.make_Subnet(vlan=eth0_100_vlan)
+        br0_ip = factory.pick_ip_in_Subnet(br0_subnet)
+        eth0_mac = factory.make_mac_address()
+        br1_fabric = factory.make_Fabric()
+        eth1_100_vlan = factory.make_VLAN(vid=100, fabric=br1_fabric)
+        br1_subnet = factory.make_Subnet(vlan=eth1_100_vlan)
+        br1_ip = factory.pick_ip_in_Subnet(br1_subnet)
+        eth1_mac = factory.make_mac_address()
+        eth0_101_vlan = factory.make_VLAN(vid=101, fabric=br1_fabric)
+        br101_subnet = factory.make_Subnet(vlan=eth0_101_vlan)
+        br101_ip = factory.pick_ip_in_Subnet(br101_subnet)
+        interfaces = {
+            "eth0": {
+                "type": "physical",
+                "mac_address": eth0_mac,
+                "parents": [],
+                "links": [],
+                "enabled": True,
+            },
+            "eth0.100": {
+                "type": "vlan",
+                "vid": 100,
+                "mac_address": eth0_mac,
+                "parents": ["eth0"],
+                "links": [],
+                "enabled": True,
+            },
+            "eth0.101": {
+                "type": "vlan",
+                "vid": 101,
+                "mac_address": eth0_mac,
+                "parents": ["eth0"],
+                "links": [],
+                "enabled": True,
+            },
+            "br0": {
+                "type": "bridge",
+                "mac_address": eth0_mac,
+                "parents": ["eth0.100"],
+                "links": [{
+                    "mode": "static",
+                    "address": "%s/%d" % (
+                        str(br0_ip), br0_subnet.get_ipnetwork().prefixlen),
+                }],
+                "enabled": True,
+            },
+            "br101": {
+                "type": "bridge",
+                "mac_address": eth0_mac,
+                "parents": ["eth0.101"],
+                "links": [{
+                    "mode": "static",
+                    "address": "%s/%d" % (
+                        str(br101_ip), br101_subnet.get_ipnetwork().prefixlen),
+                }],
+                "enabled": True,
+            },
+            "eth1": {
+                "type": "physical",
+                "mac_address": eth1_mac,
+                "parents": [],
+                "links": [],
+                "enabled": True,
+            },
+            "eth1.100": {
+                "type": "vlan",
+                "vid": 100,
+                "mac_address": eth1_mac,
+                "parents": ["eth0"],
+                "links": [],
+                "enabled": True,
+            },
+            "br1": {
+                "type": "bridge",
+                "mac_address": eth1_mac,
+                "parents": ["eth1.100"],
+                "links": [{
+                    "mode": "static",
+                    "address": "%s/%d" % (
+                        str(br1_ip), br1_subnet.get_ipnetwork().prefixlen),
+                }],
+                "enabled": True,
+            },
+        }
+        rack.update_interfaces(interfaces)
+        eth0 = PhysicalInterface.objects.get(
+            node=rack, mac_address=interfaces["eth0"]["mac_address"])
+        self.assertThat(
+            eth0, MatchesStructure.byEquality(
+                type=INTERFACE_TYPE.PHYSICAL,
+                name="eth0",
+                mac_address=interfaces["eth0"]["mac_address"],
+                enabled=True,
+                vlan=default_vlan,
+            ))
+        eth0_100 = VLANInterface.objects.get(
+            node=rack, name="eth0.100",
+            mac_address=interfaces["eth0.100"]["mac_address"])
+        self.assertThat(
+            eth0_100, MatchesStructure.byEquality(
+                type=INTERFACE_TYPE.VLAN,
+                name="eth0.100",
+                mac_address=interfaces["eth0.100"]["mac_address"],
+                enabled=True,
+                vlan=eth0_100_vlan,
+            ))
+        br0 = BridgeInterface.objects.get(
+            node=rack, name="br0",
+            mac_address=interfaces["br0"]["mac_address"])
+        self.assertThat(
+            br0, MatchesStructure.byEquality(
+                type=INTERFACE_TYPE.BRIDGE,
+                name="br0",
+                mac_address=interfaces["br0"]["mac_address"],
+                enabled=True,
+                vlan=eth0_100_vlan,
+            ))
+        br0_addresses = list(br0.ip_addresses.all())
+        self.assertThat(br0_addresses, HasLength(1))
+        self.assertThat(
+            br0_addresses[0], MatchesStructure.byEquality(
+                alloc_type=IPADDRESS_TYPE.STICKY,
+                ip=br0_ip,
+                subnet=br0_subnet,
+            ))
+        br0_nic = BridgeInterface.objects.get(
+            node=rack, vlan=eth0_100_vlan)
+        self.assertThat(
+            br0_nic, MatchesStructure.byEquality(
+                type=INTERFACE_TYPE.BRIDGE,
+                name="br0",
+                enabled=True,
+                vlan=eth0_100_vlan,
+            ))
+
+    def test__bridge_on_vlan_interface_with_identical_macs_replacing_phy(self):
+        rack = self.create_empty_rack_controller()
+        br0_fabric = factory.make_Fabric()
+        eth0_100_vlan = factory.make_VLAN(vid=100, fabric=br0_fabric)
+        br0_subnet = factory.make_Subnet(vlan=eth0_100_vlan)
+        br0_ip = factory.pick_ip_in_Subnet(br0_subnet)
+        eth0_mac = factory.make_mac_address()
+        # Before the fix for bug #1555679, bridges were modeled as "physical".
+        # Therefore, MAAS users needed to change the MAC of their bridge
+        # interfaces, rather than using the common practice of making it the
+        # same as the MAC of the parent interface.
+        bogus_br0_mac = factory.make_mac_address()
+        bogus_br1_mac = factory.make_mac_address()
+        bogus_br101_mac = factory.make_mac_address()
+        br1_fabric = factory.make_Fabric()
+        eth1_100_vlan = factory.make_VLAN(vid=100, fabric=br1_fabric)
+        br1_subnet = factory.make_Subnet(vlan=eth1_100_vlan)
+        br1_ip = factory.pick_ip_in_Subnet(br1_subnet)
+        eth1_mac = factory.make_mac_address()
+        eth0_101_vlan = factory.make_VLAN(vid=101, fabric=br1_fabric)
+        br101_subnet = factory.make_Subnet(vlan=eth0_101_vlan)
+        br101_ip = factory.pick_ip_in_Subnet(br101_subnet)
+        interfaces_old = {
+            "eth0": {
+                "type": "physical",
+                "mac_address": eth0_mac,
+                "parents": [],
+                "links": [],
+                "enabled": True,
+            },
+            "eth0.100": {
+                "type": "vlan",
+                "vid": 100,
+                "mac_address": eth0_mac,
+                "parents": ["eth0"],
+                "links": [],
+                "enabled": True,
+            },
+            "eth0.101": {
+                "type": "vlan",
+                "vid": 101,
+                "mac_address": eth0_mac,
+                "parents": ["eth0"],
+                "links": [],
+                "enabled": True,
+            },
+            "br0": {
+                "type": "physical",
+                "mac_address": bogus_br0_mac,
+                "parents": [],
+                "links": [{
+                    "mode": "static",
+                    "address": "%s/%d" % (
+                        str(br0_ip), br0_subnet.get_ipnetwork().prefixlen),
+                }],
+                "enabled": True,
+            },
+            "br101": {
+                "type": "bridge",
+                "mac_address": bogus_br101_mac,
+                "parents": ["eth0.101"],
+                "links": [{
+                    "mode": "static",
+                    "address": "%s/%d" % (
+                        str(br101_ip), br101_subnet.get_ipnetwork().prefixlen),
+                }],
+                "enabled": True,
+            },
+            "eth1": {
+                "type": "physical",
+                "mac_address": eth1_mac,
+                "parents": [],
+                "links": [],
+                "enabled": True,
+            },
+            "eth1.100": {
+                "type": "vlan",
+                "vid": 100,
+                "mac_address": eth1_mac,
+                "parents": ["eth0"],
+                "links": [],
+                "enabled": True,
+            },
+            "br1": {
+                "type": "bridge",
+                "mac_address": bogus_br1_mac,
+                "parents": ["eth1.100"],
+                "links": [{
+                    "mode": "static",
+                    "address": "%s/%d" % (
+                        str(br1_ip), br1_subnet.get_ipnetwork().prefixlen),
+                }],
+                "enabled": True,
+            },
+        }
+        rack.update_interfaces(interfaces_old)
+        eth0 = PhysicalInterface.objects.get(
+            node=rack, mac_address=interfaces_old["eth0"]["mac_address"])
+        self.assertThat(
+            eth0, MatchesStructure.byEquality(
+                type=INTERFACE_TYPE.PHYSICAL,
+                name="eth0",
+                mac_address=interfaces_old["eth0"]["mac_address"],
+                enabled=True,
+                vlan=eth0.vlan,
+            ))
+        # This is weird because it results in a model where eth0.100 is not
+        # on the same VLAN as br0. But it's something that the admin will need
+        # to fix after-the-fact, unfortunately...
+        br0 = get_one(Interface.objects.filter_by_ip(br0_ip))
+        br0_vlan = br0.vlan
+        interfaces = {
+            "eth0": {
+                "type": "physical",
+                "mac_address": eth0_mac,
+                "parents": [],
+                "links": [],
+                "enabled": True,
+            },
+            "eth0.100": {
+                "type": "vlan",
+                "vid": 100,
+                "mac_address": eth0_mac,
+                "parents": ["eth0"],
+                "links": [],
+                "enabled": True,
+            },
+            "eth0.101": {
+                "type": "vlan",
+                "vid": 101,
+                "mac_address": eth0_mac,
+                "parents": ["eth0"],
+                "links": [],
+                "enabled": True,
+            },
+            "br0": {
+                "type": "bridge",
+                "mac_address": eth0_mac,
+                "parents": ["eth0.100"],
+                "links": [{
+                    "mode": "static",
+                    "address": "%s/%d" % (
+                        str(br0_ip), br0_subnet.get_ipnetwork().prefixlen),
+                }],
+                "enabled": True,
+            },
+            "br101": {
+                "type": "bridge",
+                "mac_address": eth0_mac,
+                "parents": ["eth0.101"],
+                "links": [{
+                    "mode": "static",
+                    "address": "%s/%d" % (
+                        str(br101_ip), br101_subnet.get_ipnetwork().prefixlen),
+                }],
+                "enabled": True,
+            },
+            "eth1": {
+                "type": "physical",
+                "mac_address": eth1_mac,
+                "parents": [],
+                "links": [],
+                "enabled": True,
+            },
+            "eth1.100": {
+                "type": "vlan",
+                "vid": 100,
+                "mac_address": eth1_mac,
+                "parents": ["eth0"],
+                "links": [],
+                "enabled": True,
+            },
+            "br1": {
+                "type": "bridge",
+                "mac_address": eth1_mac,
+                "parents": ["eth1.100"],
+                "links": [{
+                    "mode": "static",
+                    "address": "%s/%d" % (
+                        str(br1_ip), br1_subnet.get_ipnetwork().prefixlen),
+                }],
+                "enabled": True,
+            },
+        }
+        rack.update_interfaces(interfaces)
+        eth0 = PhysicalInterface.objects.get(
+            node=rack, mac_address=interfaces["eth0"]["mac_address"])
+        self.assertThat(
+            eth0, MatchesStructure.byEquality(
+                type=INTERFACE_TYPE.PHYSICAL,
+                name="eth0",
+                mac_address=interfaces["eth0"]["mac_address"],
+                enabled=True,
+                vlan=eth0.vlan,
+            ))
+        eth0_100 = VLANInterface.objects.get(
+            node=rack, name="eth0.100",
+            mac_address=interfaces["eth0.100"]["mac_address"])
+        self.assertThat(
+            eth0_100, MatchesStructure.byEquality(
+                type=INTERFACE_TYPE.VLAN,
+                name="eth0.100",
+                mac_address=interfaces["eth0.100"]["mac_address"],
+                enabled=True,
+                vlan=eth0_100_vlan,
+            ))
+        br0 = BridgeInterface.objects.get(
+            node=rack, name="br0",
+            mac_address=interfaces["br0"]["mac_address"])
+        self.assertThat(
+            br0, MatchesStructure.byEquality(
+                type=INTERFACE_TYPE.BRIDGE,
+                name="br0",
+                mac_address=interfaces["br0"]["mac_address"],
+                enabled=True,
+                vlan=br0_vlan,
+            ))
+        br0_addresses = list(br0.ip_addresses.all())
+        self.assertThat(br0_addresses, HasLength(1))
+        self.assertThat(
+            br0_addresses[0], MatchesStructure.byEquality(
+                alloc_type=IPADDRESS_TYPE.STICKY,
+                ip=br0_ip,
+                subnet=br0_subnet,
+            ))
+        br0_nic = BridgeInterface.objects.get(
+            node=rack, vlan=eth0_100_vlan)
+        self.assertThat(
+            br0_nic, MatchesStructure.byEquality(
+                type=INTERFACE_TYPE.BRIDGE,
+                name="br0",
+                enabled=True,
+                vlan=br0_vlan,
+            ))
+
 
 class TestRackController(MAASServerTestCase):