Merge ~newell-jensen/maas:lp1806969 into maas:master

[Newell Jensen]

LP: #1806969 -- Explicitly search for bridges, bonds, followed by the remaining interfaces when retrieving a requested machine via interface constraints.

The reason this bug was occurring is that the interface constraints specified were being mapped to a list of interface_ids and the max id of this list was being chosen but the the bridge interface was the one with a lower id. This branch changes how the interfaces are chosen if there is a list of interface_ids by searching for a bridge or bond and then taking the max id of this. If nothing is found or if there isn't a list of interface_ids to check, the max is taken as was done before.

[Blake Rouse (blake-rouse)]

Why combine bridges and bonds? Should the search order be bridges first, then bonds, then other interfaces.

review: Needs Information

[Björn Tillenius (bjornt)]

I agree with Blake, that you should order first by bridges, and then by bonds, and then other interfaces.

I also think you need some more tests. See inline comments.

review: Needs Fixing

beed205... by Newell Jensen on 2019-07-16

Update queries so that we first try to match on bridges, followed by bonds, and then finally interfaces. Add better unit test coverage.

[Blake Rouse (blake-rouse)]

Some optimizations need to be done, but the test coverage now looks better.

review: Needs Fixing

1c00626... by Newell Jensen on 2019-07-17

Re-write django queries to improve the performance.

[Blake Rouse (blake-rouse)]

review: Needs Fixing

d547dd0... by Newell Jensen on 2019-07-17

Review fixes.

[Blake Rouse (blake-rouse)]

review: Needs Fixing

16517d8... by Newell Jensen on 2019-07-17

Review fixes +2.

[Blake Rouse (blake-rouse)]

Looks good! Thanks!

review: Approve

diff --git a/src/maasserver/forms/pods.py b/src/maasserver/forms/pods.py
index f54d875..4072a84 100644
--- a/src/maasserver/forms/pods.py
+++ b/src/maasserver/forms/pods.py
@@ -495,6 +495,19 @@ class ComposeMachineForm(forms.Form):
             known_host_interfaces=known_host_interfaces,
         )
 
+    def _pick_interface(self, interfaces):
+            bridge_interfaces = interfaces.filter(
+                type=INTERFACE_TYPE.BRIDGE).order_by('-id')
+            bond_interfaces = interfaces.filter(
+                type=INTERFACE_TYPE.BOND).order_by('-id')
+            bridge_interface = list(bridge_interfaces[:1])
+            if bridge_interface:
+                return bridge_interface[0]
+            bond_interface = list(bond_interfaces[:1])
+            if bond_interface:
+                return bond_interface[0]
+            return interfaces[0]
+
     def _get_requested_machine_interfaces_via_constraints(
             self, interfaces_label_map):
         requested_machine_interfaces = []
@@ -512,11 +525,15 @@ class ComposeMachineForm(forms.Form):
         for label in result.label_map.keys():
             # XXX: We might want to use the "deepest" interface in the
             # heirarchy, to ensure we get a bridge or bond (if configured)
-            # rather than its parent. max() is a good approximation, since
-            # child interfaces will be created after their parents.
+            # rather than its parent. Since child interfaces will be created
+            # after their parents, this is a good approximation.
+            # Search for bridges, followed by bonds, and finally with the
+            # remaining interfaces.
             interface_ids = result.label_map[label][pod_node_id]
-            interface_id = max(interface_ids)
-            interface = Interface.objects.get(id=interface_id)
+            interfaces = Interface.objects.filter(
+                id__in=interface_ids).order_by('-id')
+            interface = self._pick_interface(interfaces)
+
             # Check to see if we have a bootable VLAN,
             # we need at least one.
             if interface.has_bootable_vlan():
diff --git a/src/maasserver/forms/tests/test_pods.py b/src/maasserver/forms/tests/test_pods.py
index 03d332a..a218d84 100644
--- a/src/maasserver/forms/tests/test_pods.py
+++ b/src/maasserver/forms/tests/test_pods.py
@@ -1241,6 +1241,156 @@ class TestComposeMachineForm(MAASTransactionServerTestCase):
                             attach_options=Equals(MACVLAN_MODE.BRIDGE))),
                 ]))))
 
+    def test__get_machine_with_interfaces_by_subnets_bridge(self):
+        request = MagicMock()
+        cidr1 = "10.0.0.0/24"
+        cidr2 = "192.168.100.0/24"
+        vlan = factory.make_VLAN(dhcp_on=True)
+        subnet = factory.make_Subnet(cidr=cidr2, vlan=vlan)
+        pod_host = factory.make_Machine_with_Interface_on_Subnet(
+            cidr=cidr1)
+        space = factory.make_Space('dmz')
+        pod_host.boot_interface.vlan.space = space
+        pod_host.boot_interface.vlan.save()
+
+        # Create a bridge and non-bridge on the pod_host
+        bridge = factory.make_Interface(
+            iftype=INTERFACE_TYPE.BRIDGE, node=pod_host,
+            subnet=pod_host.boot_interface.vlan.subnet_set.first())
+        non_bridge = factory.make_Interface(node=pod_host, subnet=subnet)
+
+        pod = make_pod_with_hints()
+        pod.ip_address = pod_host.boot_interface.ip_addresses.first()
+        pod.save()
+        interfaces = "eth0:subnet=%s;eth1:subnet=%s" % (cidr1, cidr2)
+        form = ComposeMachineForm(data={
+            'interfaces': interfaces,
+        }, request=request, pod=pod)
+        self.assertTrue(form.is_valid(), form.errors)
+        request_machine = form.get_requested_machine(
+            make_known_host_interfaces(pod))
+        self.assertThat(request_machine, MatchesAll(
+            IsInstance(RequestedMachine),
+            MatchesStructure(
+                interfaces=MatchesListwise([
+                    MatchesAll(
+                        IsInstance(RequestedMachineInterface),
+                        MatchesStructure(
+                            attach_name=Equals(bridge.name),
+                            attach_type=Equals(InterfaceAttachType.BRIDGE),
+                            attach_options=Equals(None))),
+                    MatchesAll(
+                        IsInstance(RequestedMachineInterface),
+                        MatchesStructure(
+                            attach_name=Equals(non_bridge.name),
+                            attach_type=Equals(InterfaceAttachType.MACVLAN),
+                            attach_options=Equals(MACVLAN_MODE.BRIDGE))),
+                ]))))
+
+    def test__get_machine_with_interfaces_by_subnets_bond(self):
+        request = MagicMock()
+        cidr1 = "10.0.0.0/24"
+        cidr2 = "192.168.100.0/24"
+        vlan = factory.make_VLAN(dhcp_on=True)
+        subnet = factory.make_Subnet(cidr=cidr2, vlan=vlan)
+        pod_host = factory.make_Machine_with_Interface_on_Subnet(
+            cidr=cidr1)
+        space = factory.make_Space('dmz')
+        pod_host.boot_interface.vlan.space = space
+        pod_host.boot_interface.vlan.save()
+
+        # Create a bond and non-bond on the pod_host
+        bond_if = factory.make_Interface(
+            node=pod_host,
+            subnet=pod_host.boot_interface.vlan.subnet_set.first())
+        bond = factory.make_Interface(
+            iftype=INTERFACE_TYPE.BOND, node=pod_host, parents=[
+                pod_host.boot_interface, bond_if],
+            subnet=pod_host.boot_interface.vlan.subnet_set.first())
+        non_bond = factory.make_Interface(node=pod_host, subnet=subnet)
+
+        pod = make_pod_with_hints()
+        pod.ip_address = pod_host.boot_interface.ip_addresses.first()
+        pod.save()
+        interfaces = "eth0:subnet=%s;eth1:subnet=%s" % (cidr1, cidr2)
+        form = ComposeMachineForm(data={
+            'interfaces': interfaces,
+        }, request=request, pod=pod)
+        self.assertTrue(form.is_valid(), form.errors)
+        request_machine = form.get_requested_machine(
+            make_known_host_interfaces(pod))
+        self.assertThat(request_machine, MatchesAll(
+            IsInstance(RequestedMachine),
+            MatchesStructure(
+                interfaces=MatchesListwise([
+                    MatchesAll(
+                        IsInstance(RequestedMachineInterface),
+                        MatchesStructure(
+                            attach_name=Equals(bond.name),
+                            attach_type=Equals(InterfaceAttachType.MACVLAN),
+                            attach_options=Equals(MACVLAN_MODE.BRIDGE))),
+                    MatchesAll(
+                        IsInstance(RequestedMachineInterface),
+                        MatchesStructure(
+                            attach_name=Equals(non_bond.name),
+                            attach_type=Equals(InterfaceAttachType.MACVLAN),
+                            attach_options=Equals(MACVLAN_MODE.BRIDGE))),
+                ]))))
+
+    def test__get_machine_with_interfaces_by_subnets_bond_inside_bridge(self):
+        request = MagicMock()
+        cidr1 = "10.0.0.0/24"
+        cidr2 = "192.168.100.0/24"
+        vlan = factory.make_VLAN(dhcp_on=True)
+        subnet = factory.make_Subnet(cidr=cidr2, vlan=vlan)
+        pod_host = factory.make_Machine_with_Interface_on_Subnet(
+            cidr=cidr1)
+        space = factory.make_Space('dmz')
+        pod_host.boot_interface.vlan.space = space
+        pod_host.boot_interface.vlan.save()
+
+        # Create a bond and non-bond on the pod_host
+        bond_if = factory.make_Interface(
+            node=pod_host,
+            subnet=pod_host.boot_interface.vlan.subnet_set.first())
+        bond = factory.make_Interface(
+            iftype=INTERFACE_TYPE.BOND, node=pod_host, parents=[
+                pod_host.boot_interface, bond_if],
+            subnet=pod_host.boot_interface.vlan.subnet_set.first())
+        non_bond = factory.make_Interface(node=pod_host, subnet=subnet)
+        # Create bridge using the bond
+        bridge = factory.make_Interface(
+            iftype=INTERFACE_TYPE.BRIDGE, node=pod_host, parents=[bond],
+            subnet=pod_host.boot_interface.vlan.subnet_set.first())
+
+        pod = make_pod_with_hints()
+        pod.ip_address = pod_host.boot_interface.ip_addresses.first()
+        pod.save()
+        interfaces = "eth0:subnet=%s;eth1:subnet=%s" % (cidr1, cidr2)
+        form = ComposeMachineForm(data={
+            'interfaces': interfaces,
+        }, request=request, pod=pod)
+        self.assertTrue(form.is_valid(), form.errors)
+        request_machine = form.get_requested_machine(
+            make_known_host_interfaces(pod))
+        self.assertThat(request_machine, MatchesAll(
+            IsInstance(RequestedMachine),
+            MatchesStructure(
+                interfaces=MatchesListwise([
+                    MatchesAll(
+                        IsInstance(RequestedMachineInterface),
+                        MatchesStructure(
+                            attach_name=Equals(bridge.name),
+                            attach_type=Equals(InterfaceAttachType.BRIDGE),
+                            attach_options=Equals(None))),
+                    MatchesAll(
+                        IsInstance(RequestedMachineInterface),
+                        MatchesStructure(
+                            attach_name=Equals(non_bond.name),
+                            attach_type=Equals(InterfaceAttachType.MACVLAN),
+                            attach_options=Equals(MACVLAN_MODE.BRIDGE))),
+                ]))))
+
     def test__get_machine_with_interfaces_by_space_as_bridge(self):
         request = MagicMock()
         pod_host = factory.make_Machine_with_Interface_on_Subnet(