Merge ~mpontillo/maas:fix-subnet-links-for-juju--bug-1789495 into maas:master

[Mike Pontillo]

LP: #1789495 - Fix subnet links for composed machines.

[Newell Jensen (newell-jensen)]

Looks good. Approving but I did have a question inline and won't block.

review: Approve

[Mike Pontillo (mpontillo)]

Thanks for the comments; reply below!

7fcdd2e... by Mike Pontillo on 2018-08-31

Fix typo.

diff --git a/src/maasserver/models/bmc.py b/src/maasserver/models/bmc.py
index 01f23a3..f813057 100644
--- a/src/maasserver/models/bmc.py
+++ b/src/maasserver/models/bmc.py
@@ -83,6 +83,7 @@ from provisioningserver.drivers.power.registry import PowerDriverRegistry
 from provisioningserver.enum import MACVLAN_MODE_CHOICES
 from provisioningserver.logger import get_maas_logger
 from provisioningserver.utils.constraints import LabeledConstraintMap
+from provisioningserver.utils.network import get_ifname_for_label
 from provisioningserver.utils.twisted import asynchronous
 from twisted.internet.defer import inlineCallbacks
 
@@ -97,7 +98,10 @@ def get_requested_ips(requested_machine):
         requested_ips = {
             interface.ifname: interface.requested_ips
             for interface in requested_machine.interfaces
-            if interface.ifname is not None
+            if (
+                interface.ifname is not None and
+                len(interface.requested_ips) > 0
+            )
         }
     else:
         requested_ips = {}
@@ -748,7 +752,10 @@ class Pod(BMC):
         # New machines get commission started immediately unless skipped.
         if not skip_commissioning:
             skip_networking = False
-            if len(requested_ips) > 0:
+            # If an interfaces constraint was specified, don't reset the
+            # networking parameters. (Instead, allow them to be set based on
+            # what was requested in the constraints string.)
+            if interfaces is not None and len(interfaces) > 0:
                 skip_networking = True
             machine.start_commissioning(
                 commissioning_user, skip_networking=skip_networking)
@@ -790,7 +797,7 @@ class Pod(BMC):
         # dictionaries in Python 3.6+ preserve insertion order.)
         if interface_constraints is not None:
             interface_names = [
-                label[:15]
+                get_ifname_for_label(label)
                 for label in interface_constraints
             ]
         else:
@@ -882,9 +889,12 @@ class Pod(BMC):
                     # the VM has been attached to. Update the VLAN (but
                     # only if necessary).
                     host_vlan = host_attach_interface.vlan
-                    if host_vlan != created_interfaces[idx].vlan:
-                        created_interfaces[idx].vlan = host_vlan
-                        created_interfaces[idx].save()
+                    interface = created_interfaces[idx]
+                    if host_vlan != interface.vlan:
+                        interface.vlan = host_vlan
+                        interface.save()
+                    if interface.ip_addresses.count() == 0:
+                        interface.force_auto_or_dhcp_link()
                     continue
 
     def _sync_machine(self, discovered_machine, existing_machine):
diff --git a/src/maasserver/models/tests/test_bmc.py b/src/maasserver/models/tests/test_bmc.py
index bdb6624..00a5ad2 100644
--- a/src/maasserver/models/tests/test_bmc.py
+++ b/src/maasserver/models/tests/test_bmc.py
@@ -1043,7 +1043,7 @@ class TestPod(MAASServerTestCase):
         sip3 = StaticIPAddress.objects.filter(ip=ip3).first()
         self.assertThat(sip3.get_interface().node, Equals(machine))
 
-    def test_create_machine_sets_interface_vlans_using_attach_names(self):
+    def test_create_machine_sets_up_interface_vlans_correctly(self):
         discovered_machine = self.make_discovered_machine()
         self.patch(Machine, "set_default_storage_layout")
         self.patch(Machine, "set_initial_networking_configuration")
@@ -1052,11 +1052,14 @@ class TestPod(MAASServerTestCase):
         controller = factory.make_RackController()
         vlan = factory.make_VLAN(
             fabric=fabric, dhcp_on=True, primary_rack=controller)
-        subnet = factory.make_Subnet()
         vlan2 = factory.make_VLAN(
             fabric=fabric, dhcp_on=False, primary_rack=controller)
         vlan3 = factory.make_VLAN(
             fabric=fabric, dhcp_on=False, primary_rack=controller)
+        # Create subnets, so we can test to ensure they get linked up.
+        subnet = factory.make_Subnet(vlan=vlan)
+        factory.make_Subnet(vlan=vlan2)
+        factory.make_Subnet(vlan=vlan3)
         eth0 = factory.make_Interface(node=controller, vlan=vlan)
         eth1 = factory.make_Interface(node=controller, vlan=vlan2)
         eth2 = factory.make_Interface(node=controller, vlan=vlan3)
@@ -1074,11 +1077,13 @@ class TestPod(MAASServerTestCase):
             InterfaceAttachType.MACVLAN)
         discovered_machine.interfaces[2].attach_name = eth2.name
         pod = factory.make_Pod(ip_address=ip)
-        print(pod.ip_address)
+        # Skip commissioning on creation so that we can test that VLANs
+        # are properly set based on the interface constraint.
         machine = pod.create_machine(
             discovered_machine, factory.make_User(),
+            # Use numeric names to mimic what Juju will do.
             interfaces=LabeledConstraintMap(
-                'eth0:vlan=id:%d;eth1:vlan=id:%d;eth2:vlan=id:%d' % (
+                '0:vlan=id:%d;1:vlan=id:%d;2:vlan=id:%d' % (
                     vlan.id, vlan2.id, vlan3.id),
             )
         )
@@ -1089,6 +1094,10 @@ class TestPod(MAASServerTestCase):
         self.assertThat(interfaces['eth0'].vlan, Equals(vlan))
         self.assertThat(interfaces['eth1'].vlan, Equals(vlan2))
         self.assertThat(interfaces['eth2'].vlan, Equals(vlan3))
+        # Make sure all interfaces also have a subnet link.
+        self.assertThat(interfaces['eth0'].ip_addresses.count(), Equals(1))
+        self.assertThat(interfaces['eth1'].ip_addresses.count(), Equals(1))
+        self.assertThat(interfaces['eth2'].ip_addresses.count(), Equals(1))
 
     def test_create_machine_uses_default_ifnames_if_discovered_mismatch(self):
         """This makes sure that if the discovered machine comes back with
@@ -1766,3 +1775,16 @@ class TestGetRequestedIPs(MAASServerTestCase):
                 "eth0": ['10.0.0.1', '2001:db8::1'],
                 "eth1": ['10.0.0.2', '2001:db8::2']
             }))
+
+    def test__leaves_out_keys_with_no_assigned_ips(self):
+        interface = RequestedMachineInterface(
+            ifname='eth0', requested_ips=['10.0.0.1', '2001:db8::1'])
+        interface2 = RequestedMachineInterface(
+            ifname='eth1', requested_ips=[])
+        interfaces = [interface, interface2]
+        requested_machine = RequestedMachine(
+            factory.make_hostname(), 'amd64', 1, 1024, [], interfaces)
+        self.assertThat(
+            get_requested_ips(requested_machine), Equals({
+                "eth0": ['10.0.0.1', '2001:db8::1'],
+            }))
diff --git a/src/provisioningserver/utils/constraints.py b/src/provisioningserver/utils/constraints.py
index e38fc23..d00c9c7 100644
--- a/src/provisioningserver/utils/constraints.py
+++ b/src/provisioningserver/utils/constraints.py
@@ -30,6 +30,11 @@ class LabeledConstraintMap(object):
     def __str__(self):
         return self.value
 
+    def __len__(self):
+        if self.map is None:
+            return 0
+        return len(self.map)
+
     def __iter__(self):
         if self.map is None:
             return iter([])
diff --git a/src/provisioningserver/utils/tests/test_constraints.py b/src/provisioningserver/utils/tests/test_constraints.py
index d797d11..c3a26bf 100644
--- a/src/provisioningserver/utils/tests/test_constraints.py
+++ b/src/provisioningserver/utils/tests/test_constraints.py
@@ -7,12 +7,14 @@ __all__ = []
 
 from maastesting.testcase import MAASTestCase
 from provisioningserver.utils.constraints import (
+    LabeledConstraintMap,
     parse_labeled_constraint_map,
     validate_constraint_label_name,
 )
 from testtools import ExpectedException
 from testtools.matchers import (
     Equals,
+    HasLength,
     Is,
 )
 
@@ -113,3 +115,18 @@ class TestGetLabeledConstraintsMap(MAASTestCase):
             "foo": {"a": ["b"], "c": ["d"]},
             "bar": {"e": ["f"], "g": ["h"]},
         }))
+
+
+class TestLabeledConstraintMap(MAASTestCase):
+
+    def test__len__for_null_map(self):
+        lcm = LabeledConstraintMap(None)
+        self.assertThat(lcm, HasLength(0))
+
+    def test__len__for_empty_map(self):
+        lcm = LabeledConstraintMap('')
+        self.assertThat(lcm, HasLength(0))
+
+    def test__len__for_populated_map(self):
+        lcm = LabeledConstraintMap('eth0:space=1;eth1:space=2')
+        self.assertThat(lcm, HasLength(2))