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

Proposed by Mike Pontillo on 2018-07-11
Status: Merged
Approved by: Mike Pontillo on 2018-07-12
Approved revision: 28700058f82627a613221c1187a2d5154c9b67ca
Merge reported by: MAAS Lander
Merged at revision: not available
Proposed branch: ~mpontillo/maas:default-dns-ip--bug-1776604
Merge into: maas:master
Diff against target: 212 lines (+132/-17)
3 files modified
src/maasserver/models/interface.py (+2/-0)
src/maasserver/models/staticipaddress.py (+30/-17)
src/maasserver/models/tests/test_staticipaddress.py (+100/-0)
Reviewer Review Type Date Requested Status
MAAS Lander Approve on 2018-07-12
Blake Rouse (community) Approve on 2018-07-12
Newell Jensen (community) 2018-07-11 Abstain on 2018-07-12
MAAS Maintainers risk-reduction 2018-07-11 Pending
Review via email: mp+349384@code.launchpad.net

Commit message

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.

To post a comment you must log in.
Newell Jensen (newell-jensen) wrote :

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) wrote :

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) wrote :

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.)

Mike Pontillo (mpontillo) wrote :

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) wrote :

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) wrote :

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) wrote :

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
MAAS Lander (maas-lander) wrote :

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
Mike Pontillo (mpontillo) wrote :

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) wrote :

Looks good.

review: Approve
MAAS Lander (maas-lander) wrote :

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/src/maasserver/models/interface.py b/src/maasserver/models/interface.py
2index 4624e8a..5839e73 100644
3--- a/src/maasserver/models/interface.py
4+++ b/src/maasserver/models/interface.py
5@@ -1128,6 +1128,8 @@ class Interface(CleanSave, TimestampedModel):
6 """Remove all the `IPAddress` link on the interface."""
7 for ip_address in self.ip_addresses.exclude(
8 alloc_type=IPADDRESS_TYPE.DISCOVERED):
9+ maaslog.info("%s: IP address automatically unlinked: %s" % (
10+ self.get_log_string(), ip_address))
11 self.unlink_ip_address(ip_address, clearing_config=clearing_config)
12
13 def claim_auto_ips(self, exclude_addresses=[]):
14diff --git a/src/maasserver/models/staticipaddress.py b/src/maasserver/models/staticipaddress.py
15index bfc9d61..68be5c5 100644
16--- a/src/maasserver/models/staticipaddress.py
17+++ b/src/maasserver/models/staticipaddress.py
18@@ -75,6 +75,8 @@ _special_mapping_result = _mapping_base_fields + (
19
20 _mapping_query_result = _mapping_base_fields + (
21 'is_boot',
22+ 'preference',
23+ 'family',
24 )
25
26 _interface_mapping_result = _mapping_base_fields + (
27@@ -504,7 +506,7 @@ class StaticIPAddressManager(Manager):
28 domain.ttl,
29 %s)""" % default_ttl
30 sql_query = """
31- SELECT DISTINCT ON (node.hostname, is_boot, family(staticip.ip))
32+ SELECT DISTINCT ON (fqdn, is_boot, family)
33 CONCAT(node.hostname, '.', domain.name) AS fqdn,
34 node.system_id,
35 node.node_type,
36@@ -515,16 +517,38 @@ class StaticIPAddressManager(Manager):
37 node.boot_interface_id IS NOT NULL AND
38 (
39 node.boot_interface_id = interface.id OR
40- node.boot_interface_id = parent.id
41+ node.boot_interface_id = parent.id OR
42+ node.boot_interface_id = parent_parent.id
43 ),
44 False
45- ) AS is_boot
46+ ) AS is_boot,
47+ CASE
48+ WHEN interface.type = 'bridge' AND
49+ parent_parent.id = node.boot_interface_id THEN 1
50+ WHEN interface.type = 'bridge' AND
51+ parent.id = node.boot_interface_id THEN 2
52+ WHEN interface.type = 'bond' AND
53+ parent.id = node.boot_interface_id THEN 3
54+ WHEN interface.type = 'physical' AND
55+ interface.id = node.boot_interface_id THEN 4
56+ WHEN interface.type = 'bond' THEN 5
57+ WHEN interface.type = 'physical' THEN 6
58+ WHEN interface.type = 'vlan' THEN 7
59+ WHEN interface.type = 'alias' THEN 8
60+ WHEN interface.type = 'unknown' THEN 9
61+ ELSE 10
62+ END AS preference,
63+ family(staticip.ip) AS family
64 FROM
65 maasserver_interface AS interface
66 LEFT OUTER JOIN maasserver_interfacerelationship AS rel ON
67 interface.id = rel.child_id
68 LEFT OUTER JOIN maasserver_interface AS parent ON
69 rel.parent_id = parent.id
70+ LEFT OUTER JOIN maasserver_interfacerelationship AS parent_rel ON
71+ parent.id = parent_rel.child_id
72+ LEFT OUTER JOIN maasserver_interface AS parent_parent ON
73+ parent_rel.parent_id = parent_parent.id
74 JOIN maasserver_node AS node ON
75 node.id = interface.node_id
76 JOIN maasserver_domain AS domain ON
77@@ -562,21 +586,10 @@ class StaticIPAddressManager(Manager):
78 staticip.ip IS NOT NULL AND
79 host(staticip.ip) != ''
80 ORDER BY
81- node.hostname,
82+ fqdn,
83 is_boot DESC,
84- family(staticip.ip),
85- CASE
86- WHEN interface.type = 'bond' AND
87- parent.id = node.boot_interface_id THEN 1
88- WHEN interface.type = 'physical' AND
89- interface.id = node.boot_interface_id THEN 2
90- WHEN interface.type = 'bond' THEN 3
91- WHEN interface.type = 'physical' THEN 4
92- WHEN interface.type = 'vlan' THEN 5
93- WHEN interface.type = 'alias' THEN 6
94- WHEN interface.type = 'unknown' THEN 7
95- ELSE 8
96- END,
97+ family,
98+ preference,
99 /*
100 * We want STICKY and USER_RESERVED addresses to be preferred,
101 * followed by AUTO, DHCP, and finally DISCOVERED.
102diff --git a/src/maasserver/models/tests/test_staticipaddress.py b/src/maasserver/models/tests/test_staticipaddress.py
103index 7b72861..7e1f945 100644
104--- a/src/maasserver/models/tests/test_staticipaddress.py
105+++ b/src/maasserver/models/tests/test_staticipaddress.py
106@@ -837,6 +837,106 @@ class TestStaticIPAddressManagerMapping(MAASServerTestCase):
107 node.system_id, 30, {vlanip.ip}, node.node_type)}
108 self.assertEqual(expected_mapping, mapping)
109
110+ def test_get_hostname_ip_mapping_prefers_bridged_bond_pxe_interface(self):
111+ subnet = factory.make_Subnet(
112+ cidr='10.0.0.0/24', dns_servers=[], gateway_ip='10.0.0.1')
113+ node = factory.make_Node_with_Interface_on_Subnet(
114+ hostname='host', subnet=subnet)
115+ eth0 = node.get_boot_interface()
116+ eth0.name = 'eth0'
117+ eth0.save()
118+ eth1 = factory.make_Interface(
119+ INTERFACE_TYPE.PHYSICAL, node=node, name='eth1', vlan=subnet.vlan)
120+ eth2 = factory.make_Interface(
121+ INTERFACE_TYPE.PHYSICAL, node=node, name='eth2', vlan=subnet.vlan)
122+ node.boot_interface = eth1
123+ node.save()
124+ bond0 = factory.make_Interface(
125+ INTERFACE_TYPE.BOND, node=node, parents=[eth1, eth2], name='bond0')
126+ br_bond0 = factory.make_Interface(
127+ INTERFACE_TYPE.BRIDGE, parents=[bond0], name='br-bond0')
128+ phy_staticip = factory.make_StaticIPAddress(
129+ alloc_type=IPADDRESS_TYPE.STICKY, interface=eth0,
130+ subnet=subnet, ip='10.0.0.2')
131+ bridge_ip = factory.make_StaticIPAddress(
132+ alloc_type=IPADDRESS_TYPE.STICKY, interface=br_bond0,
133+ subnet=subnet, ip='10.0.0.3')
134+ # Make an IP addresses that shouldn't be preferred, but is also on
135+ # an interface whose parent is a boot interface. (Some caution is
136+ # advised when doing this, since we have interface signals that will
137+ # automatically remove these addresses in some cases.)
138+ bond0_extra_ip = factory.make_StaticIPAddress(
139+ alloc_type=IPADDRESS_TYPE.STICKY, interface=bond0,
140+ subnet=subnet, ip='10.0.0.4')
141+ mapping = StaticIPAddress.objects.get_hostname_ip_mapping(
142+ node.domain)
143+ expected_mapping = {
144+ node.fqdn: HostnameIPMapping(
145+ node.system_id, 30, {bridge_ip.ip}, node.node_type),
146+ "%s.%s" % (eth0.name, node.fqdn): HostnameIPMapping(
147+ node.system_id, 30, {phy_staticip.ip}, node.node_type),
148+ "%s.%s" % (bond0.name, node.fqdn): HostnameIPMapping(
149+ node.system_id, 30, {bond0_extra_ip.ip}, node.node_type)
150+ }
151+ self.assertThat(mapping, Equals(expected_mapping))
152+
153+ def test_get_hostname_ip_mapping_with_v4_and_v6_and_bridged_bonds(self):
154+ subnet_v4 = factory.make_Subnet(
155+ cidr=str(factory.make_ipv4_network().cidr))
156+ subnet_v6 = factory.make_Subnet(
157+ cidr='2001:db8::/64')
158+ node = factory.make_Node_with_Interface_on_Subnet(
159+ hostname='host', subnet=subnet_v4)
160+ eth0 = node.get_boot_interface()
161+ eth0.name = 'eth0'
162+ eth0.save()
163+ eth1 = factory.make_Interface(
164+ INTERFACE_TYPE.PHYSICAL, node=node, name='eth1')
165+ eth2 = factory.make_Interface(
166+ INTERFACE_TYPE.PHYSICAL, node=node, name='eth2')
167+ node.boot_interface = eth1
168+ node.save()
169+ bond0 = factory.make_Interface(
170+ INTERFACE_TYPE.BOND, node=node, parents=[eth1, eth2], name='bond0')
171+ br_bond0 = factory.make_Interface(
172+ INTERFACE_TYPE.BRIDGE, parents=[bond0], name='br-bond0')
173+ phy_staticip_v4 = factory.make_StaticIPAddress(
174+ alloc_type=IPADDRESS_TYPE.STICKY, interface=eth0,
175+ subnet=subnet_v4)
176+ bridge_ip_v4 = factory.make_StaticIPAddress(
177+ alloc_type=IPADDRESS_TYPE.STICKY, interface=br_bond0,
178+ subnet=subnet_v4)
179+ phy_staticip_v6 = factory.make_StaticIPAddress(
180+ alloc_type=IPADDRESS_TYPE.STICKY, interface=eth0,
181+ subnet=subnet_v6)
182+ bridge_ip_v6 = factory.make_StaticIPAddress(
183+ alloc_type=IPADDRESS_TYPE.STICKY, interface=br_bond0,
184+ subnet=subnet_v6)
185+ # Make a few IP addresses that shouldn't be associated with the
186+ # default hosname. (They'll show up as associated with bond0, however.)
187+ # Technically MAAS should remove these since they belong to a parent
188+ # interface, but in this case the interface won't be re-saved.
189+ extra_ipv4 = factory.make_StaticIPAddress(
190+ alloc_type=IPADDRESS_TYPE.STICKY, interface=bond0,
191+ subnet=subnet_v4)
192+ extra_ipv6 = factory.make_StaticIPAddress(
193+ alloc_type=IPADDRESS_TYPE.STICKY, interface=bond0,
194+ subnet=subnet_v6)
195+ mapping = StaticIPAddress.objects.get_hostname_ip_mapping(
196+ node.domain)
197+ expected_mapping = {
198+ node.fqdn: HostnameIPMapping(
199+ node.system_id, 30, {bridge_ip_v4.ip, bridge_ip_v6.ip},
200+ node.node_type),
201+ "%s.%s" % (eth0.name, node.fqdn): HostnameIPMapping(
202+ node.system_id, 30, {phy_staticip_v4.ip, phy_staticip_v6.ip},
203+ node.node_type),
204+ "%s.%s" % (bond0.name, node.fqdn): HostnameIPMapping(
205+ node.system_id, 30, {extra_ipv4.ip, extra_ipv6.ip},
206+ node.node_type)
207+ }
208+ self.assertThat(mapping, Equals(expected_mapping))
209+
210 def test_get_hostname_ip_mapping_returns_domain_head_ips(self):
211 parent = factory.make_Domain()
212 name = factory.make_name()

Subscribers

People subscribed via source and target branches