Merge ~mpontillo/maas:allow-deferred-fabric-creation into maas:master

[Mike Pontillo]

Refactor rack registration to allow deferred fabric creation.

This allows update_interfaces() to be called in two modes: one which
creates all fabrics immediately (the current behavior), and the other
which only creates 'disconnected' physical interfaces and skips child
interface creation and deletion.

This work is necessary so that in the future, a rack can register,
perform beaconing, and update its interfaces with any additional
topology data the beacons can provide.

Also updates all test scenarios for update_interfaces() to make sure
they work with the old 'one phase' approach and the new 'two phase'
approach. (That is, the interfaces are created in exactly the same
way even if fabric creation is deferred.)

[Blake Rouse (blake-rouse)]

Looks like a clean refactor. Only comments is on the why you check for a boolean value in if statements. Just a plain if would be better. If your worried about the type then I would do a check for the type at the start of the function:

if not isinstance(create_fabrics, bool):
    raise TypeError('expected create_fabrics to be a bool; received %s' % type(create_fabrics).__name__)

review: Approve

[MAAS Lander (maas-lander)]

LANDING
-b allow-deferred-fabric-creation lp:~mpontillo/maas into -b master lp:~maas-committers/maas

STATUS: FAILED MERGE
LOG: http://ci-maas-jenkins.internal:8080/job/maas/job/branch-lander/14376/console

[Mike Pontillo (mpontillo)]

Not sure why this failed to merge. There were no conflicts. Trying again.

[MAAS Lander (maas-lander)]

LANDING
-b allow-deferred-fabric-creation lp:~mpontillo/maas into -b master lp:~maas-committers/maas

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

[Mike Pontillo (mpontillo)]

Strange failure:

    http://paste.ubuntu.com/25077309/

I wasn't able to replicate this:

$ bin/test.region metadataserver.tests.test_api:TestCommissioningAPI.test_signal_stores_multiple_files{,,,,,,,,,}{,,,,,,,,,}
....................................................................................................
----------------------------------------------------------------------
Ran 100 tests in 24.142s

[MAAS Lander (maas-lander)]

LANDING
-b allow-deferred-fabric-creation lp:~mpontillo/maas into -b master lp:~maas-committers/maas

STATUS: FAILED MERGE
LOG: http://ci-maas-jenkins.internal:8080/job/maas/job/branch-lander/14440/console

[Mike Pontillo (mpontillo)]

Another weird lander failure.

http://paste.ubuntu.com/25077511/

[MAAS Lander (maas-lander)]

LANDING
-b allow-deferred-fabric-creation lp:~mpontillo/maas into -b master lp:~maas-committers/maas

STATUS: FAILED MERGE
LOG: http://ci-maas-jenkins.internal:8080/job/maas/job/branch-lander/14455/console

[Mike Pontillo (mpontillo)]

Looks like the lander is messing up the git command line when trying to merge this branch:

[master] + git commit -a -m Refactor rack registration to allow deferred fabric creation.
[master]
[master] This allows update_interfaces() to be called in two modes: one which
[master] creates all fabrics immediately (the current behavior), and the other
[master] which only creates disconnected physical interfaces and skips child
[master] interface creation and deletion.
[master]
[master] This work is necessary so that in the future, a rack can register,
[master] perform beaconing, and update its interfaces with any additional
[master] topology data the beacons can provide.
[master]
[master] Also updates all test scenarios for update_interfaces() to make sure
[master] they work with the old one phase approach and the new two phase
[master] approach. (That is, the interfaces are created in exactly the same
[master] way even if fabric creation is deferred.)
[master] fatal: Paths with -a does not make sense.

[Mike Pontillo (mpontillo)]

The problem was double-quotes in the commit message. Changing them to single quotes fixed the issue.

diff --git a/src/maasserver/models/node.py b/src/maasserver/models/node.py
index 3166bee..74028a2 100644
--- a/src/maasserver/models/node.py
+++ b/src/maasserver/models/node.py
@@ -1312,7 +1312,7 @@ class Node(CleanSave, TimestampedModel):
         """Mark a node as being deployed."""
         # Avoid circular dependencies
         from metadataserver.models import ScriptSet
-        if self.on_network() is False:
+        if not self.on_network():
             raise ValidationError(
                 {"network":
                  ["Node must be configured to use a network"]})
@@ -4108,7 +4108,7 @@ class Controller(Node):
         machine must have power information."""
         return self.status == NODE_STATUS.DEPLOYED and self.bmc is not None
 
-    def _update_interface(self, name, config):
+    def _update_interface(self, name, config, create_fabrics=True):
         """Update a interface.
 
         :param name: Name of the interface.
@@ -4116,7 +4116,11 @@ class Controller(Node):
             /etc/network/interfaces on the rack controller.
         """
         if config["type"] == "physical":
-            return self._update_physical_interface(name, config)
+            return self._update_physical_interface(
+                name, config, create_fabrics=create_fabrics)
+        elif not create_fabrics:
+            # Defer child interface creation until fabrics are known.
+            return None
         elif config["type"] == "vlan":
             return self._update_vlan_interface(name, config)
         elif config["type"] == "bond":
@@ -4128,7 +4132,7 @@ class Controller(Node):
                 "Unkwown interface type '%s' for '%s'." % (
                     config["type"], name))
 
-    def _update_physical_interface(self, name, config):
+    def _update_physical_interface(self, name, config, create_fabrics=True):
         """Update a physical interface.
 
         :param name: Name of the interface.
@@ -4137,57 +4141,75 @@ class Controller(Node):
         """
         new_vlan = None
         mac_address = config["mac_address"]
+        update_fields = set()
+        is_enabled = config['enabled']
         interface, created = PhysicalInterface.objects.get_or_create(
             mac_address=mac_address, defaults={
                 "node": self,
                 "name": name,
-                "enabled": config["enabled"],
+                "enabled": is_enabled,
             })
-        if created:
-            # 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.
-            connected_to_subnets = self._get_connected_subnets(
-                config["links"])
-            if len(connected_to_subnets) == 0:
-                # Not connected to any known subnets. We add it to its own
-                # fabric unless this is the very first interface on this
-                # rack controller. The first interface always goes on
-                # the default VLAN on the default fabric.
-                if Interface.objects.filter(node=self).count() > 1:
-                    new_fabric = Fabric.objects.create()
-                    new_vlan = new_fabric.get_default_vlan()
-                    interface.vlan = new_vlan
-                    interface.save()
-                else:
-                    interface.vlan = (
-                        Fabric.objects.get_default_fabric().get_default_vlan())
-                    interface.save()
-        else:
+        if create_fabrics and interface.vlan is None:
+            new_vlan = self._update_vlan_for_interface(
+                interface, config, new_vlan)
+        if not created:
             if interface.node.id != self.id:
                 # MAC address was on a different node. We need to move
                 # it to its new owner. In the process we delete all of its
                 # current links because they are completely wrong.
                 interface.ip_addresses.all().delete()
                 interface.node = self
+                update_fields.add('node')
             interface.name = name
-            interface.enabled = config["enabled"]
-            interface.save()
+            update_fields.add('name')
+        if interface.enabled != is_enabled:
+            interface.enabled = is_enabled
+            update_fields.add('enabled')
 
         # Update all the IP address on this interface. Fix the VLAN the
         # interface belongs to so its the same as the links.
+        if create_fabrics:
+            self._update_physical_links(
+                interface, config, new_vlan, update_fields)
+        if len(update_fields) > 0:
+            interface.save(update_fields=list(update_fields))
+        return interface
+
+    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(
             update_ip_addresses)
         if linked_vlan is not None:
             interface.vlan = linked_vlan
-            interface.save()
+            update_fields.add('vlan')
             if new_vlan is not None and linked_vlan.id != new_vlan.id:
                 # Create a new VLAN for this interface and it was not used as
                 # a link re-assigned the VLAN this interface is connected to.
                 new_vlan.fabric.delete()
-        return interface
+
+    def _update_vlan_for_interface(self, interface, config, new_vlan):
+        # 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.
+        connected_to_subnets = self._get_connected_subnets(
+            config["links"])
+        if len(connected_to_subnets) == 0:
+            # 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).count()
+            if interfaces_on_default_vlan == 0:
+                new_vlan = default_vlan
+            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):
         """Wrapper to get_or_create for VLAN interfaces.
@@ -4598,11 +4620,14 @@ class Controller(Node):
             for interface in interfaces
         }
 
-    def update_interfaces(self, interfaces):
+    def update_interfaces(self, interfaces, create_fabrics=True):
         """Update the interfaces attached to the controller.
 
         :param interfaces: Interfaces dictionary that was parsed from
             /etc/network/interfaces on the controller.
+        :param create_fabrics: If True, creates fabrics associated with each
+            VLAN. Otherwise, creates the interfaces but does not create any
+            links or VLANs.
         """
         # Get all of the current interfaces on this controller.
         current_interfaces = {
@@ -4634,12 +4659,18 @@ class Controller(Node):
             # Note: the interface that comes back from this call may be None,
             # if we decided not to model an interface based on what the rack
             # sent.
-            interface = self._update_interface(name, settings)
+            interface = self._update_interface(
+                name, settings, create_fabrics=create_fabrics)
             if interface is not None:
                 interface.update_discovery_state(discovery_mode, settings)
             if interface is not None and interface.id in current_interfaces:
                 del current_interfaces[interface.id]
 
+        if not create_fabrics:
+            # This could be an existing rack controller re-registering,
+            # so don't delete interfaces during this phase.
+            return
+
         # Remove all the interfaces that no longer exist. We do this in reverse
         # order so the child is deleted before the parent.
         deletion_order = {}
diff --git a/src/maasserver/models/tests/test_node.py b/src/maasserver/models/tests/test_node.py
index c07d74d..bbd7915 100644
--- a/src/maasserver/models/tests/test_node.py
+++ b/src/maasserver/models/tests/test_node.py
@@ -6693,15 +6693,36 @@ class TestReportMDNSEntries(MAASServerTestCase):
 class TestUpdateInterfaces(MAASServerTestCase):
 
     scenarios = (
-        ("rack", dict(node_type=NODE_TYPE.RACK_CONTROLLER)),
-        ("region", dict(node_type=NODE_TYPE.REGION_CONTROLLER)),
+        ("rack", dict(
+            node_type=NODE_TYPE.RACK_CONTROLLER,
+            two_phase=False)),
+        ("region", dict(
+            node_type=NODE_TYPE.REGION_CONTROLLER,
+            two_phase=False)),
         ("region+rack", dict(
-            node_type=NODE_TYPE.REGION_AND_RACK_CONTROLLER)),
+            node_type=NODE_TYPE.REGION_AND_RACK_CONTROLLER,
+            two_phase=False)),
+        ("rack_two_phase", dict(
+            node_type=NODE_TYPE.RACK_CONTROLLER,
+            two_phase=True)),
+        ("region_two_phase", dict(
+            node_type=NODE_TYPE.REGION_CONTROLLER,
+            two_phase=True)),
+        ("region+rack_two_phase", dict(
+            node_type=NODE_TYPE.REGION_AND_RACK_CONTROLLER,
+            two_phase=True)),
     )
 
     def create_empty_controller(self):
         return factory.make_Node(node_type=self.node_type).as_self()
 
+    def update_interfaces(self, controller, interfaces):
+        if not self.two_phase:
+            controller.update_interfaces(interfaces)
+        else:
+            controller.update_interfaces(interfaces, create_fabrics=False)
+            controller.update_interfaces(interfaces, create_fabrics=True)
+
     def test__order_of_calls_to_update_interface_is_always_the_same(self):
         controller = self.create_empty_controller()
         interfaces = {
@@ -6741,18 +6762,32 @@ class TestUpdateInterfaces(MAASServerTestCase):
                 "enabled": True,
             },
         }
-        expected_call_order = [
-            call("eth0", interfaces["eth0"]),
-            call("eth1", interfaces["eth1"]),
-            call("eth2", interfaces["eth2"]),
-            call("bond0", interfaces["bond0"]),
-            call("bond0.10", interfaces["bond0.10"]),
-        ]
+        if not self.two_phase:
+            expected_call_order = [
+                call("eth0", interfaces["eth0"], create_fabrics=True),
+                call("eth1", interfaces["eth1"], create_fabrics=True),
+                call("eth2", interfaces["eth2"], create_fabrics=True),
+                call("bond0", interfaces["bond0"], create_fabrics=True),
+                call("bond0.10", interfaces["bond0.10"], create_fabrics=True),
+            ]
+        else:
+            expected_call_order = [
+                call("eth0", interfaces["eth0"], create_fabrics=False),
+                call("eth1", interfaces["eth1"], create_fabrics=False),
+                call("eth2", interfaces["eth2"], create_fabrics=False),
+                call("bond0", interfaces["bond0"], create_fabrics=False),
+                call("bond0.10", interfaces["bond0.10"], create_fabrics=False),
+                call("eth0", interfaces["eth0"], create_fabrics=True),
+                call("eth1", interfaces["eth1"], create_fabrics=True),
+                call("eth2", interfaces["eth2"], create_fabrics=True),
+                call("bond0", interfaces["bond0"], create_fabrics=True),
+                call("bond0.10", interfaces["bond0.10"], create_fabrics=True),
+            ]
         # Perform multiple times to make sure the call order is always
         # the same.
         for _ in range(5):
             mock_update_interface = self.patch(controller, "_update_interface")
-            controller.update_interfaces(interfaces)
+            self.update_interfaces(controller, interfaces)
             self.assertThat(
                 mock_update_interface, MockCallsMatch(*expected_call_order))
 
@@ -6774,7 +6809,7 @@ class TestUpdateInterfaces(MAASServerTestCase):
                 "enabled": False,
             },
         }
-        controller.update_interfaces(interfaces)
+        self.update_interfaces(controller, interfaces)
         eth0 = Interface.objects.get(name="eth0", node=controller)
         self.assertThat(
             eth0, MatchesStructure.byEquality(
@@ -6847,7 +6882,7 @@ class TestUpdateInterfaces(MAASServerTestCase):
 
     def _test_vlans_with_alternate_naming_conventions(
             self, controller, interfaces):
-        controller.update_interfaces(interfaces)
+        self.update_interfaces(controller, interfaces)
         eth0 = Interface.objects.get(name="eth0", node=controller)
         self.assertThat(
             eth0, MatchesStructure.byEquality(
@@ -6886,7 +6921,7 @@ class TestUpdateInterfaces(MAASServerTestCase):
                 enabled=True,
             ))
         self.assertThat(list(eth0_0102.parents.all()), Equals([eth0]))
-        controller.update_interfaces(interfaces)
+        self.update_interfaces(controller, interfaces)
         eth0 = Interface.objects.get(name="eth0", node=controller)
         self.assertThat(
             eth0, MatchesStructure.byEquality(
@@ -6957,7 +6992,7 @@ class TestUpdateInterfaces(MAASServerTestCase):
                 "monitored": False
             }
         }
-        controller.update_interfaces(interfaces)
+        self.update_interfaces(controller, interfaces)
         eth0 = Interface.objects.get(name="eth0", node=controller)
         self.assertThat(
             eth0, MatchesStructure.byEquality(
@@ -7002,7 +7037,7 @@ class TestUpdateInterfaces(MAASServerTestCase):
                 "monitored": False
             },
         }
-        controller.update_interfaces(interfaces)
+        self.update_interfaces(controller, interfaces)
         # Disable the interfaces so that we can make sure neighbour discovery
         # is properly disabled on update.
         interfaces = {
@@ -7024,7 +7059,7 @@ class TestUpdateInterfaces(MAASServerTestCase):
                 "monitored": False
             },
         }
-        controller.update_interfaces(interfaces)
+        self.update_interfaces(controller, interfaces)
         eth0 = Interface.objects.get(name="eth0", node=controller)
         self.assertThat(
             eth0, MatchesStructure.byEquality(
@@ -7065,7 +7100,7 @@ class TestUpdateInterfaces(MAASServerTestCase):
                 "enabled": True,
             },
         }
-        controller.update_interfaces(interfaces)
+        self.update_interfaces(controller, interfaces)
         eth0 = Interface.objects.get(name="eth0", node=controller)
         default_vlan = Fabric.objects.get_default_fabric().get_default_vlan()
         self.assertThat(
@@ -7109,7 +7144,7 @@ class TestUpdateInterfaces(MAASServerTestCase):
                 "enabled": True,
             },
         }
-        controller.update_interfaces(interfaces)
+        self.update_interfaces(controller, interfaces)
         eth0 = Interface.objects.get(name="eth0", node=controller)
         default_vlan = Fabric.objects.get_default_fabric().get_default_vlan()
         self.assertThat(
@@ -7163,7 +7198,7 @@ class TestUpdateInterfaces(MAASServerTestCase):
                 "enabled": True,
             },
         }
-        controller.update_interfaces(interfaces)
+        self.update_interfaces(controller, interfaces)
         eth0 = Interface.objects.get(name="eth0", node=controller)
         default_vlan = Fabric.objects.get_default_fabric().get_default_vlan()
         self.assertThat(
@@ -7210,7 +7245,7 @@ class TestUpdateInterfaces(MAASServerTestCase):
                 "enabled": True,
             },
         }
-        controller.update_interfaces(interfaces)
+        self.update_interfaces(controller, interfaces)
         eth0 = Interface.objects.get(name="eth0", node=controller)
         self.assertThat(
             eth0, MatchesStructure.byEquality(
@@ -7255,7 +7290,7 @@ class TestUpdateInterfaces(MAASServerTestCase):
                 "enabled": True,
             },
         }
-        controller.update_interfaces(interfaces)
+        self.update_interfaces(controller, interfaces)
         eth0 = Interface.objects.get(name="eth0", node=controller)
         self.assertThat(
             eth0, MatchesStructure.byEquality(
@@ -7306,7 +7341,7 @@ class TestUpdateInterfaces(MAASServerTestCase):
                 "enabled": True,
             },
         }
-        controller.update_interfaces(interfaces)
+        self.update_interfaces(controller, interfaces)
         eth0 = Interface.objects.get(name="eth0", node=controller)
         self.assertThat(
             eth0, MatchesStructure.byEquality(
@@ -7354,7 +7389,7 @@ class TestUpdateInterfaces(MAASServerTestCase):
                 "enabled": True,
             },
         }
-        controller.update_interfaces(interfaces)
+        self.update_interfaces(controller, interfaces)
         self.assertThat(controller.interface_set.count(), Equals(1))
         self.assertThat(
             reload_object(interface), MatchesStructure.byEquality(
@@ -7396,7 +7431,7 @@ class TestUpdateInterfaces(MAASServerTestCase):
                 "enabled": True,
             },
         }
-        controller.update_interfaces(interfaces)
+        self.update_interfaces(controller, interfaces)
         self.assertThat(controller.interface_set.count(), Equals(1))
         self.assertThat(
             reload_object(interface), MatchesStructure.byEquality(
@@ -7444,7 +7479,7 @@ class TestUpdateInterfaces(MAASServerTestCase):
                 "enabled": True,
             },
         }
-        controller.update_interfaces(interfaces)
+        self.update_interfaces(controller, interfaces)
         self.assertThat(controller.interface_set.count(), Equals(1))
         self.assertThat(
             reload_object(interface), MatchesStructure.byEquality(
@@ -7497,7 +7532,7 @@ class TestUpdateInterfaces(MAASServerTestCase):
             "enabled": True,
             "vid": vid_on_fabric,
         }
-        controller.update_interfaces(interfaces)
+        self.update_interfaces(controller, interfaces)
         self.assertThat(controller.interface_set.count(), Equals(2))
         self.assertThat(
             reload_object(interface), MatchesStructure.byEquality(
@@ -7564,7 +7599,7 @@ class TestUpdateInterfaces(MAASServerTestCase):
             "enabled": True,
             "vid": vid_on_fabric,
         }
-        controller.update_interfaces(interfaces)
+        self.update_interfaces(controller, interfaces)
         self.assertThat(controller.interface_set.count(), Equals(2))
         self.assertThat(
             reload_object(interface), MatchesStructure.byEquality(
@@ -7647,7 +7682,7 @@ class TestUpdateInterfaces(MAASServerTestCase):
             "vid": vid_on_fabric,
         }
         maaslog = self.patch(node_module, "maaslog")
-        controller.update_interfaces(interfaces)
+        self.update_interfaces(controller, interfaces)
         self.assertThat(controller.interface_set.count(), Equals(2))
         self.assertThat(
             reload_object(interface), MatchesStructure.byEquality(
@@ -7709,7 +7744,7 @@ class TestUpdateInterfaces(MAASServerTestCase):
             "enabled": True,
             "vid": vid_on_fabric,
         }
-        controller.update_interfaces(interfaces)
+        self.update_interfaces(controller, interfaces)
         self.assertThat(controller.interface_set.count(), Equals(2))
         self.assertThat(
             reload_object(interface), MatchesStructure.byEquality(
@@ -7760,7 +7795,7 @@ class TestUpdateInterfaces(MAASServerTestCase):
             "enabled": True,
             "vid": new_vlan.vid,
         }
-        controller.update_interfaces(interfaces)
+        self.update_interfaces(controller, interfaces)
         self.assertThat(controller.interface_set.count(), Equals(2))
         self.assertThat(
             reload_object(interface), MatchesStructure.byEquality(
@@ -7825,7 +7860,7 @@ class TestUpdateInterfaces(MAASServerTestCase):
             "vid": new_vlan.vid,
         }
         maaslog = self.patch(node_module, "maaslog")
-        controller.update_interfaces(interfaces)
+        self.update_interfaces(controller, interfaces)
         self.assertThat(controller.interface_set.count(), Equals(2))
         self.assertThat(
             reload_object(interface), MatchesStructure.byEquality(
@@ -7891,7 +7926,7 @@ class TestUpdateInterfaces(MAASServerTestCase):
                 "enabled": True,
             },
         }
-        controller.update_interfaces(interfaces)
+        self.update_interfaces(controller, interfaces)
         self.assertThat(controller.interface_set.count(), Equals(3))
         bond_interface = BondInterface.objects.get(
             node=controller, mac_address=interfaces["bond0"]["mac_address"])
@@ -7938,7 +7973,7 @@ class TestUpdateInterfaces(MAASServerTestCase):
                 "enabled": True,
             },
         }
-        controller.update_interfaces(interfaces)
+        self.update_interfaces(controller, interfaces)
         self.assertThat(controller.interface_set.count(), Equals(3))
         bond_interface = BridgeInterface.objects.get(
             node=controller, mac_address=interfaces["br0"]["mac_address"])
@@ -7989,7 +8024,7 @@ class TestUpdateInterfaces(MAASServerTestCase):
                 "enabled": True,
             },
         }
-        controller.update_interfaces(interfaces)
+        self.update_interfaces(controller, interfaces)
         self.assertThat(controller.interface_set.count(), Equals(3))
         self.assertThat(
             reload_object(bond0), MatchesStructure.byEquality(
@@ -8038,7 +8073,7 @@ class TestUpdateInterfaces(MAASServerTestCase):
                 "enabled": True,
             },
         }
-        controller.update_interfaces(interfaces)
+        self.update_interfaces(controller, interfaces)
         self.assertThat(controller.interface_set.count(), Equals(3))
         self.assertThat(
             reload_object(br0), MatchesStructure.byEquality(
@@ -8093,7 +8128,7 @@ class TestUpdateInterfaces(MAASServerTestCase):
                 "enabled": True,
             },
         }
-        controller.update_interfaces(interfaces)
+        self.update_interfaces(controller, interfaces)
         self.assertThat(controller.interface_set.count(), Equals(3))
         self.assertThat(
             reload_object(eth0), MatchesStructure.byEquality(
@@ -8166,7 +8201,7 @@ class TestUpdateInterfaces(MAASServerTestCase):
                 "enabled": True,
             },
         }
-        controller.update_interfaces(interfaces)
+        self.update_interfaces(controller, interfaces)
         self.assertThat(controller.interface_set.count(), Equals(3))
         self.assertThat(
             reload_object(eth0), MatchesStructure.byEquality(
@@ -8239,7 +8274,7 @@ class TestUpdateInterfaces(MAASServerTestCase):
                 "enabled": True,
             },
         }
-        controller.update_interfaces(interfaces)
+        self.update_interfaces(controller, interfaces)
         self.assertThat(reload_object(eth0), Not(Is(None)))
         self.assertThat(reload_object(eth1), Is(None))
         self.assertThat(reload_object(bond0), Not(Is(None)))
@@ -8270,7 +8305,7 @@ class TestUpdateInterfaces(MAASServerTestCase):
                 "enabled": True,
             },
         }
-        controller.update_interfaces(interfaces)
+        self.update_interfaces(controller, interfaces)
         self.assertThat(reload_object(eth0), Not(Is(None)))
         self.assertThat(reload_object(eth1), Is(None))
         self.assertThat(reload_object(br0), Not(Is(None)))
@@ -8294,7 +8329,7 @@ class TestUpdateInterfaces(MAASServerTestCase):
                 "enabled": True,
             },
         }
-        controller.update_interfaces(interfaces)
+        self.update_interfaces(controller, interfaces)
         self.assertThat(reload_object(eth0), Not(Is(None)))
         self.assertThat(reload_object(eth1), Is(None))
         self.assertThat(reload_object(bond0), Is(None))
@@ -8318,7 +8353,7 @@ class TestUpdateInterfaces(MAASServerTestCase):
                 "enabled": True,
             },
         }
-        controller.update_interfaces(interfaces)
+        self.update_interfaces(controller, interfaces)
         self.assertThat(reload_object(eth0), Not(Is(None)))
         self.assertThat(reload_object(eth1), Is(None))
         self.assertThat(reload_object(br0), Is(None))
@@ -8371,7 +8406,7 @@ class TestUpdateInterfaces(MAASServerTestCase):
             "vid": bond0_vlan.vid,
             "enabled": True,
         }
-        controller.update_interfaces(interfaces)
+        self.update_interfaces(controller, interfaces)
         eth0 = PhysicalInterface.objects.get(
             node=controller, mac_address=interfaces["eth0"]["mac_address"])
         self.assertThat(
@@ -8482,7 +8517,7 @@ class TestUpdateInterfaces(MAASServerTestCase):
             "vid": br0_vlan.vid,
             "enabled": True,
         }
-        controller.update_interfaces(interfaces)
+        self.update_interfaces(controller, interfaces)
         eth0 = PhysicalInterface.objects.get(
             node=controller, mac_address=interfaces["eth0"]["mac_address"])
         self.assertThat(
@@ -8813,7 +8848,7 @@ class TestUpdateInterfaces(MAASServerTestCase):
                 "enabled": True,
             },
         }
-        controller.update_interfaces(interfaces)
+        self.update_interfaces(controller, interfaces)
         eth0 = PhysicalInterface.objects.get(
             node=controller, mac_address=interfaces["eth0"]["mac_address"])
         self.assertThat(
@@ -9048,7 +9083,7 @@ class TestUpdateInterfaces(MAASServerTestCase):
                 "enabled": True,
             },
         }
-        controller.update_interfaces(interfaces)
+        self.update_interfaces(controller, interfaces)
         eth0 = PhysicalInterface.objects.get(
             node=controller, mac_address=interfaces["eth0"]["mac_address"])
         self.assertThat(
@@ -9122,7 +9157,7 @@ class TestUpdateInterfaces(MAASServerTestCase):
                 'type': 'physical'
             }
         }
-        controller.update_interfaces(interfaces)
+        self.update_interfaces(controller, interfaces)
         eth0 = get_one(
             PhysicalInterface.objects.filter(node=controller, name='eth0'))
         br0 = get_one(
@@ -9153,7 +9188,7 @@ class TestUpdateInterfaces(MAASServerTestCase):
                 'type': 'physical'
             },
         }
-        controller.update_interfaces(interfaces)
+        self.update_interfaces(controller, interfaces)
         eth0 = get_one(
             PhysicalInterface.objects.filter(node=controller, name='eth0'))
         br0 = get_one(
diff --git a/src/maasserver/rpc/rackcontrollers.py b/src/maasserver/rpc/rackcontrollers.py
index 9c49532..2d8e30a 100644
--- a/src/maasserver/rpc/rackcontrollers.py
+++ b/src/maasserver/rpc/rackcontrollers.py
@@ -76,7 +76,7 @@ def handle_upgrade(rack_controller, nodegroup_uuid):
 @transactional
 def register(
         system_id=None, hostname='', interfaces=None,
-        url=None, is_loopback=None):
+        url=None, is_loopback=None, create_fabrics=True):
     """Register a new rack controller if not already registered.
 
     Attempt to see if the rack controller was already registered as a node.
@@ -156,8 +156,8 @@ def register(
         rackcontroller.owner = worker_user.get_worker_user()
         update_fields.append("owner")
     rackcontroller.save(update_fields=update_fields)
-    # Update networking information every time we see a rack.
-    rackcontroller.update_interfaces(interfaces)
+    # Update interfaces, if requested.
+    rackcontroller.update_interfaces(interfaces, create_fabrics=create_fabrics)
     return rackcontroller
 
 
diff --git a/src/maasserver/rpc/regionservice.py b/src/maasserver/rpc/regionservice.py
index b622cfb..8d394cc 100644
--- a/src/maasserver/rpc/regionservice.py
+++ b/src/maasserver/rpc/regionservice.py
@@ -531,6 +531,27 @@ def getRegionID():
             lambda advertising: advertising.region_id)
 
 
+@inlineCallbacks
+def isLoopbackURL(url):
+    """Checks if the specified URL refers to a loopback address.
+
+    :return: True if the URL refers to the loopback interface, otherwise False.
+    """
+    if url is not None:
+        if url.hostname is not None:
+            is_loopback = yield deferToThread(
+                resolves_to_loopback_address, url.hostname)
+        else:
+            # Empty URL == localhost.
+            is_loopback = True
+    else:
+        # We need to pass is_loopback in, but it is only checked if url
+        # is not None.  None is the "I don't know and you won't ask"
+        # state for this boolean.
+        is_loopback = None
+    return is_loopback
+
+
 @implementer(IConnection)
 class RegionServer(Region):
     """The RPC protocol supported by a region controller, server version.
@@ -551,6 +572,33 @@ class RegionServer(Region):
     hostIsRemote = False
 
     @inlineCallbacks
+    def initResponder(self, rack_controller):
+        """Set up local connection identifiers for this RPC connection.
+
+        Sets up connection identifiers, and adds the connection into the
+        service and the database.
+
+        :param rack_controller: A RackController model object representing the
+            remote rack controller.
+        """
+        self.ident = rack_controller.system_id
+        self.factory.service._addConnectionFor(self.ident, self)
+        # A local rack is treated differently to one that's remote.
+        self.host = self.transport.getHost()
+        self.hostIsRemote = isinstance(
+            self.host, (IPv4Address, IPv6Address))
+        # Only register the connection into the database when it's a valid
+        # IPv4 or IPv6. Only time it is not an IPv4 or IPv6 address is
+        # when mocking a connection.
+        if self.hostIsRemote:
+            advertising = yield (
+                self.factory.service.advertiser.advertising.get())
+            process = yield deferToDatabase(advertising.getRegionProcess)
+            yield deferToDatabase(
+                advertising.registerConnection,
+                process, rack_controller, self.host)
+
+    @inlineCallbacks
     def authenticateCluster(self):
         """Authenticate the cluster."""
         secret = yield get_shared_secret()
@@ -565,52 +613,29 @@ class RegionServer(Region):
     @inlineCallbacks
     def register(
             self, system_id, hostname, interfaces, url, nodegroup_uuid=None):
+        result = yield self._register(
+            system_id, hostname, interfaces, url, nodegroup_uuid)
+        return result
 
+    @inlineCallbacks
+    def _register(
+            self, system_id, hostname, interfaces, url, nodegroup_uuid=None,
+            create_fabrics=True):
         try:
             # Register, which includes updating interfaces.
-            if url is not None:
-                if url.hostname is not None:
-                    is_loopback = yield deferToThread(
-                        resolves_to_loopback_address, url.hostname)
-                else:
-                    # Empty URL == localhost.
-                    is_loopback = True
-            else:
-                # We need to pass is_loopback in, but it is only checked if url
-                # is not None.  None is the "I don't know and you won't ask"
-                # state for this boolean.
-                is_loopback = None
+            is_loopback = yield isLoopbackURL(url)
             rack_controller = yield deferToDatabase(
                 rackcontrollers.register, system_id=system_id,
                 hostname=hostname, interfaces=interfaces, url=url,
-                is_loopback=is_loopback)
+                is_loopback=is_loopback, create_fabrics=create_fabrics)
 
             # Check for upgrade.
             if nodegroup_uuid is not None:
                 yield deferToDatabase(
-                    rackcontrollers.handle_upgrade,
-                    rack_controller, nodegroup_uuid)
-
-            # Set the identifier and add the connection into the service
-            # and into the database.
-            self.ident = rack_controller.system_id
-            self.factory.service._addConnectionFor(self.ident, self)
-
-            # A local rack is treated differently to one that's remote.
-            self.host = self.transport.getHost()
-            self.hostIsRemote = isinstance(
-                self.host, (IPv4Address, IPv6Address))
-
-            # Only register the connection into the database when it's a valid
-            # IPv4 or IPv6. Only time it is not an IPv4 or IPv6 address is
-            # when mocking a connection.
-            if self.hostIsRemote:
-                advertising = yield (
-                    self.factory.service.advertiser.advertising.get())
-                process = yield deferToDatabase(advertising.getRegionProcess)
-                yield deferToDatabase(
-                    advertising.registerConnection,
-                    process, rack_controller, self.host)
+                    rackcontrollers.handle_upgrade, rack_controller,
+                    nodegroup_uuid)
+
+            yield self.initResponder(rack_controller)
 
             # Rack controller is now registered. Log this status.
             log.msg(
diff --git a/src/provisioningserver/rpc/region.py b/src/provisioningserver/rpc/region.py
index 974bba1..23bcdc7 100644
--- a/src/provisioningserver/rpc/region.py
+++ b/src/provisioningserver/rpc/region.py
@@ -61,7 +61,7 @@ from twisted.protocols import amp
 class RegisterRackController(amp.Command):
     """Register a rack controller with the region controller.
 
-    This is the second step of the Authenticate, Register, Commision
+    This is the second step of the Authenticate, Register, Commission
     process.
 
     :since: 2.0