Merge ~mpontillo/maas:default-dns-ip--bug-1776604 into maas:master

[Mike Pontillo]

LP: #1776604 - Improve hostname mapping heuristic for bonded bridges

This commit changes the default hostname to IP address mapping to
account for situations where an interface parent's parent is a
machine's boot interface.

[Newell Jensen (newell-jensen)]

After talking with Mike some about this branch and the complexity, it is hard to tell what exactly is happening here and whether or not this will actually fix the bug without regressions. Mike voiced these reservations as well. I think a good thing to do might be to add more unit tests if possible to squelch, if possible, any potential regression concerns. Going to abstain from approving this although the Python code does look good.

review: Abstain

[MAAS Lander (maas-lander)]

UNIT TESTS
-b default-dns-ip--bug-1776604 lp:~mpontillo/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: 9835e6fb28821913e4ddcb86214a81d1a1a01547

review: Approve

[Mike Pontillo (mpontillo)]

Let me explain a little more about what is going on here.

The get_hostname_ip_mapping() function basically takes in a subnet or domain, and returns a dictionary of {hostname: (ttl, [ip])} mappings.

Most of the function is a self-described "SQL horror". The reason for that is, we want to be able to do this in a single query for performance reasons. (Doing this in Django would be even worse.)

Previously, the SQL query itself tried to ensure that only one mapping would be returned per (fqdn, address_family) pair. It did this by using a SELECT DISTINCT to try to determine whether or not the interface was the boot interface. But this wasn't enough, because we have to account for a few different scenarios:

 - There might be many boot interfaces per machine, depending on the interface hierarchy. For example, if we have a physical interface 'eth0' and we boot from it, that's great, but what if the user also configured an 'eth1' and bonded {eth0, eth1} together? We won't see a proper IP address assigned to eth0 or eth1 in that case, so we need to dig deeper.

 - In addition to the above point, you might have a bridge interface whose parent is the bond (where your IP addresses are actually assigned). That means you have to check if the parent's parent is a boot interface before determining if it should get the default hostname for the machine. So the SQL had to be changed to account for the parent-of-a-parent-being-a-boot-interface scenario.

 - If we don't include the preference in the SELECT DISTINCT, we can't include it in the ORDER BY statement, otherwise the following error is raised:

    django.db.utils.ProgrammingError: SELECT DISTINCT ON expressions must match initial ORDER BY expressions

   ... therefore, the SELECT DISTINCT must include the preference, and we must filter out the rows we don't want in the Python code. (That's what the Python portion of this change does.)

bad432e... by Mike Pontillo on 2018-07-12

Sort is_boot before preference, just in case preference returns an incorrect result.

[Mike Pontillo (mpontillo)]

Also: we can /almost/ just remove the `preference` field, except we have to handle the case where there is no known boot interface.

[Mike Pontillo (mpontillo)]

One last comment. I think it's difficult to add more tests to this file, because effectively this function is a heuristic. It tries to guess the best interface to assign the node's primary hostname to. In cases where it guesses wrong, it's possible that customers have come to rely on the previous behavior. For example, it looks like we may be missing cases where there the PXE interface is configured with a triple-parent, such as:

physical (eth0) -> bond ({eth0, eth1} via bond0) -> vlan (bond0.11) -> bridge (br-vlan11)

In this case it's possible that a customer might be relying on MAAS to return an IP address assigned to bond0, when in fact we /could/ have a good argument that we should change the logic to return an IP address assigned to br-vlan11.

[MAAS Lander (maas-lander)]

UNIT TESTS
-b default-dns-ip--bug-1776604 lp:~mpontillo/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: bad432ebc000900f66d1cfe80661d63142a37089

review: Approve

[Blake Rouse (blake-rouse)]

This SQL makes me want to vomit. With all the original unit tests passing and the new unit test passing, I say it works as expected. At least how we believe it should be working. ;-)

review: Approve

19310d3... by Mike Pontillo on 2018-07-12

Remove Python portion of the code (it was unnecessary).

[MAAS Lander (maas-lander)]

UNIT TESTS
-b default-dns-ip--bug-1776604 lp:~mpontillo/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: 19310d33b988ecaba665461d438c637b19cca516

review: Approve

2870005... by Mike Pontillo on 2018-07-12

Further simplify SQL query. Add additional logging and tests.

[Mike Pontillo (mpontillo)]

I was able to simplify the code a little, removing the extra Python bits.

I also added a log statement for when IP addresses are automatically removed, added another test case, and added some extra IPs within the tests to make sure the correct addresses are always returned for a a node's default hostname.

[Blake Rouse (blake-rouse)]

Looks good.

review: Approve

[MAAS Lander (maas-lander)]

UNIT TESTS
-b default-dns-ip--bug-1776604 lp:~mpontillo/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: 28700058f82627a613221c1187a2d5154c9b67ca

review: Approve

diff --git a/src/maasserver/models/interface.py b/src/maasserver/models/interface.py
index 4624e8a..5839e73 100644
--- a/src/maasserver/models/interface.py
+++ b/src/maasserver/models/interface.py
@@ -1128,6 +1128,8 @@ class Interface(CleanSave, TimestampedModel):
         """Remove all the `IPAddress` link on the interface."""
         for ip_address in self.ip_addresses.exclude(
                 alloc_type=IPADDRESS_TYPE.DISCOVERED):
+            maaslog.info("%s: IP address automatically unlinked: %s" % (
+                self.get_log_string(), ip_address))
             self.unlink_ip_address(ip_address, clearing_config=clearing_config)
 
     def claim_auto_ips(self, exclude_addresses=[]):
diff --git a/src/maasserver/models/staticipaddress.py b/src/maasserver/models/staticipaddress.py
index bfc9d61..68be5c5 100644
--- a/src/maasserver/models/staticipaddress.py
+++ b/src/maasserver/models/staticipaddress.py
@@ -75,6 +75,8 @@ _special_mapping_result = _mapping_base_fields + (
 
 _mapping_query_result = _mapping_base_fields + (
     'is_boot',
+    'preference',
+    'family',
 )
 
 _interface_mapping_result = _mapping_base_fields + (
@@ -504,7 +506,7 @@ class StaticIPAddressManager(Manager):
                     domain.ttl,
                     %s)""" % default_ttl
         sql_query = """
-            SELECT DISTINCT ON (node.hostname, is_boot, family(staticip.ip))
+            SELECT DISTINCT ON (fqdn, is_boot, family)
                 CONCAT(node.hostname, '.', domain.name) AS fqdn,
                 node.system_id,
                 node.node_type,
@@ -515,16 +517,38 @@ class StaticIPAddressManager(Manager):
                     node.boot_interface_id IS NOT NULL AND
                     (
                         node.boot_interface_id = interface.id OR
-                        node.boot_interface_id = parent.id
+                        node.boot_interface_id = parent.id OR
+                        node.boot_interface_id = parent_parent.id
                     ),
                     False
-                ) AS is_boot
+                ) AS is_boot,
+                CASE
+                    WHEN interface.type = 'bridge' AND
+                        parent_parent.id = node.boot_interface_id THEN 1
+                    WHEN interface.type = 'bridge' AND
+                        parent.id = node.boot_interface_id THEN 2
+                    WHEN interface.type = 'bond' AND
+                        parent.id = node.boot_interface_id THEN 3
+                    WHEN interface.type = 'physical' AND
+                        interface.id = node.boot_interface_id THEN 4
+                    WHEN interface.type = 'bond' THEN 5
+                    WHEN interface.type = 'physical' THEN 6
+                    WHEN interface.type = 'vlan' THEN 7
+                    WHEN interface.type = 'alias' THEN 8
+                    WHEN interface.type = 'unknown' THEN 9
+                    ELSE 10
+                END AS preference,
+                family(staticip.ip) AS family
             FROM
                 maasserver_interface AS interface
             LEFT OUTER JOIN maasserver_interfacerelationship AS rel ON
                 interface.id = rel.child_id
             LEFT OUTER JOIN maasserver_interface AS parent ON
                 rel.parent_id = parent.id
+            LEFT OUTER JOIN maasserver_interfacerelationship AS parent_rel ON
+                parent.id = parent_rel.child_id
+            LEFT OUTER JOIN maasserver_interface AS parent_parent ON
+                parent_rel.parent_id = parent_parent.id
             JOIN maasserver_node AS node ON
                 node.id = interface.node_id
             JOIN maasserver_domain AS domain ON
@@ -562,21 +586,10 @@ class StaticIPAddressManager(Manager):
                 staticip.ip IS NOT NULL AND
                 host(staticip.ip) != ''
             ORDER BY
-                node.hostname,
+                fqdn,
                 is_boot DESC,
-                family(staticip.ip),
-                CASE
-                    WHEN interface.type = 'bond' AND
-                        parent.id = node.boot_interface_id THEN 1
-                    WHEN interface.type = 'physical' AND
-                        interface.id = node.boot_interface_id THEN 2
-                    WHEN interface.type = 'bond' THEN 3
-                    WHEN interface.type = 'physical' THEN 4
-                    WHEN interface.type = 'vlan' THEN 5
-                    WHEN interface.type = 'alias' THEN 6
-                    WHEN interface.type = 'unknown' THEN 7
-                    ELSE 8
-                END,
+                family,
+                preference,
                 /*
                  * We want STICKY and USER_RESERVED addresses to be preferred,
                  * followed by AUTO, DHCP, and finally DISCOVERED.
diff --git a/src/maasserver/models/tests/test_staticipaddress.py b/src/maasserver/models/tests/test_staticipaddress.py
index 7b72861..7e1f945 100644
--- a/src/maasserver/models/tests/test_staticipaddress.py
+++ b/src/maasserver/models/tests/test_staticipaddress.py
@@ -837,6 +837,106 @@ class TestStaticIPAddressManagerMapping(MAASServerTestCase):
                 node.system_id, 30, {vlanip.ip}, node.node_type)}
         self.assertEqual(expected_mapping, mapping)
 
+    def test_get_hostname_ip_mapping_prefers_bridged_bond_pxe_interface(self):
+        subnet = factory.make_Subnet(
+            cidr='10.0.0.0/24', dns_servers=[], gateway_ip='10.0.0.1')
+        node = factory.make_Node_with_Interface_on_Subnet(
+            hostname='host', subnet=subnet)
+        eth0 = node.get_boot_interface()
+        eth0.name = 'eth0'
+        eth0.save()
+        eth1 = factory.make_Interface(
+            INTERFACE_TYPE.PHYSICAL, node=node, name='eth1', vlan=subnet.vlan)
+        eth2 = factory.make_Interface(
+            INTERFACE_TYPE.PHYSICAL, node=node, name='eth2', vlan=subnet.vlan)
+        node.boot_interface = eth1
+        node.save()
+        bond0 = factory.make_Interface(
+            INTERFACE_TYPE.BOND, node=node, parents=[eth1, eth2], name='bond0')
+        br_bond0 = factory.make_Interface(
+            INTERFACE_TYPE.BRIDGE, parents=[bond0], name='br-bond0')
+        phy_staticip = factory.make_StaticIPAddress(
+            alloc_type=IPADDRESS_TYPE.STICKY, interface=eth0,
+            subnet=subnet, ip='10.0.0.2')
+        bridge_ip = factory.make_StaticIPAddress(
+            alloc_type=IPADDRESS_TYPE.STICKY, interface=br_bond0,
+            subnet=subnet, ip='10.0.0.3')
+        # Make an IP addresses that shouldn't be preferred, but is also on
+        # an interface whose parent is a boot interface. (Some caution is
+        # advised when doing this, since we have interface signals that will
+        # automatically remove these addresses in some cases.)
+        bond0_extra_ip = factory.make_StaticIPAddress(
+            alloc_type=IPADDRESS_TYPE.STICKY, interface=bond0,
+            subnet=subnet, ip='10.0.0.4')
+        mapping = StaticIPAddress.objects.get_hostname_ip_mapping(
+            node.domain)
+        expected_mapping = {
+            node.fqdn: HostnameIPMapping(
+                node.system_id, 30, {bridge_ip.ip}, node.node_type),
+            "%s.%s" % (eth0.name, node.fqdn): HostnameIPMapping(
+                node.system_id, 30, {phy_staticip.ip}, node.node_type),
+            "%s.%s" % (bond0.name, node.fqdn): HostnameIPMapping(
+                node.system_id, 30, {bond0_extra_ip.ip}, node.node_type)
+        }
+        self.assertThat(mapping, Equals(expected_mapping))
+
+    def test_get_hostname_ip_mapping_with_v4_and_v6_and_bridged_bonds(self):
+        subnet_v4 = factory.make_Subnet(
+            cidr=str(factory.make_ipv4_network().cidr))
+        subnet_v6 = factory.make_Subnet(
+            cidr='2001:db8::/64')
+        node = factory.make_Node_with_Interface_on_Subnet(
+            hostname='host', subnet=subnet_v4)
+        eth0 = node.get_boot_interface()
+        eth0.name = 'eth0'
+        eth0.save()
+        eth1 = factory.make_Interface(
+            INTERFACE_TYPE.PHYSICAL, node=node, name='eth1')
+        eth2 = factory.make_Interface(
+            INTERFACE_TYPE.PHYSICAL, node=node, name='eth2')
+        node.boot_interface = eth1
+        node.save()
+        bond0 = factory.make_Interface(
+            INTERFACE_TYPE.BOND, node=node, parents=[eth1, eth2], name='bond0')
+        br_bond0 = factory.make_Interface(
+            INTERFACE_TYPE.BRIDGE, parents=[bond0], name='br-bond0')
+        phy_staticip_v4 = factory.make_StaticIPAddress(
+            alloc_type=IPADDRESS_TYPE.STICKY, interface=eth0,
+            subnet=subnet_v4)
+        bridge_ip_v4 = factory.make_StaticIPAddress(
+            alloc_type=IPADDRESS_TYPE.STICKY, interface=br_bond0,
+            subnet=subnet_v4)
+        phy_staticip_v6 = factory.make_StaticIPAddress(
+            alloc_type=IPADDRESS_TYPE.STICKY, interface=eth0,
+            subnet=subnet_v6)
+        bridge_ip_v6 = factory.make_StaticIPAddress(
+            alloc_type=IPADDRESS_TYPE.STICKY, interface=br_bond0,
+            subnet=subnet_v6)
+        # Make a few IP addresses that shouldn't be associated with the
+        # default hosname. (They'll show up as associated with bond0, however.)
+        # Technically MAAS should remove these since they belong to a parent
+        # interface, but in this case the interface won't be re-saved.
+        extra_ipv4 = factory.make_StaticIPAddress(
+            alloc_type=IPADDRESS_TYPE.STICKY, interface=bond0,
+            subnet=subnet_v4)
+        extra_ipv6 = factory.make_StaticIPAddress(
+            alloc_type=IPADDRESS_TYPE.STICKY, interface=bond0,
+            subnet=subnet_v6)
+        mapping = StaticIPAddress.objects.get_hostname_ip_mapping(
+            node.domain)
+        expected_mapping = {
+            node.fqdn: HostnameIPMapping(
+                node.system_id, 30, {bridge_ip_v4.ip, bridge_ip_v6.ip},
+                node.node_type),
+            "%s.%s" % (eth0.name, node.fqdn): HostnameIPMapping(
+                node.system_id, 30, {phy_staticip_v4.ip, phy_staticip_v6.ip},
+                node.node_type),
+            "%s.%s" % (bond0.name, node.fqdn): HostnameIPMapping(
+                node.system_id, 30, {extra_ipv4.ip, extra_ipv6.ip},
+                node.node_type)
+        }
+        self.assertThat(mapping, Equals(expected_mapping))
+
     def test_get_hostname_ip_mapping_returns_domain_head_ips(self):
         parent = factory.make_Domain()
         name = factory.make_name()