Merge ~mpontillo/maas:default-dns-ip--bug-1776604 into maas:master
- Git
- lp:~mpontillo/maas
- default-dns-ip--bug-1776604
- Merge into master
Status: | Merged |
---|---|
Approved by: | Mike Pontillo |
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) |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
MAAS Lander | Approve | ||
Blake Rouse (community) | Approve | ||
Newell Jensen (community) | Abstain | ||
MAAS Maintainers | risk-reduction | 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.
Description of the change
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: 9835e6fb2882191
Mike Pontillo (mpontillo) wrote : | # |
Let me explain a little more about what is going on here.
The get_hostname_
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-
- 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.
... 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
-
Sort is_boot before preference, just in case preference returns an incorrect result.
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: bad432ebc000900
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. ;-)
- 19310d3... by Mike Pontillo
-
Remove Python portion of the code (it was unnecessary).
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: 19310d33b988eca
- 2870005... by Mike Pontillo
-
Further simplify SQL query. Add additional logging and tests.
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.
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: 28700058f82627a
Preview Diff
1 | diff --git a/src/maasserver/models/interface.py b/src/maasserver/models/interface.py | |||
2 | index 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 | 1128 | """Remove all the `IPAddress` link on the interface.""" | 1128 | """Remove all the `IPAddress` link on the interface.""" |
7 | 1129 | for ip_address in self.ip_addresses.exclude( | 1129 | for ip_address in self.ip_addresses.exclude( |
8 | 1130 | alloc_type=IPADDRESS_TYPE.DISCOVERED): | 1130 | alloc_type=IPADDRESS_TYPE.DISCOVERED): |
9 | 1131 | maaslog.info("%s: IP address automatically unlinked: %s" % ( | ||
10 | 1132 | self.get_log_string(), ip_address)) | ||
11 | 1131 | self.unlink_ip_address(ip_address, clearing_config=clearing_config) | 1133 | self.unlink_ip_address(ip_address, clearing_config=clearing_config) |
12 | 1132 | 1134 | ||
13 | 1133 | def claim_auto_ips(self, exclude_addresses=[]): | 1135 | def claim_auto_ips(self, exclude_addresses=[]): |
14 | diff --git a/src/maasserver/models/staticipaddress.py b/src/maasserver/models/staticipaddress.py | |||
15 | index 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 | 75 | 75 | ||
20 | 76 | _mapping_query_result = _mapping_base_fields + ( | 76 | _mapping_query_result = _mapping_base_fields + ( |
21 | 77 | 'is_boot', | 77 | 'is_boot', |
22 | 78 | 'preference', | ||
23 | 79 | 'family', | ||
24 | 78 | ) | 80 | ) |
25 | 79 | 81 | ||
26 | 80 | _interface_mapping_result = _mapping_base_fields + ( | 82 | _interface_mapping_result = _mapping_base_fields + ( |
27 | @@ -504,7 +506,7 @@ class StaticIPAddressManager(Manager): | |||
28 | 504 | domain.ttl, | 506 | domain.ttl, |
29 | 505 | %s)""" % default_ttl | 507 | %s)""" % default_ttl |
30 | 506 | sql_query = """ | 508 | sql_query = """ |
32 | 507 | SELECT DISTINCT ON (node.hostname, is_boot, family(staticip.ip)) | 509 | SELECT DISTINCT ON (fqdn, is_boot, family) |
33 | 508 | CONCAT(node.hostname, '.', domain.name) AS fqdn, | 510 | CONCAT(node.hostname, '.', domain.name) AS fqdn, |
34 | 509 | node.system_id, | 511 | node.system_id, |
35 | 510 | node.node_type, | 512 | node.node_type, |
36 | @@ -515,16 +517,38 @@ class StaticIPAddressManager(Manager): | |||
37 | 515 | node.boot_interface_id IS NOT NULL AND | 517 | node.boot_interface_id IS NOT NULL AND |
38 | 516 | ( | 518 | ( |
39 | 517 | node.boot_interface_id = interface.id OR | 519 | node.boot_interface_id = interface.id OR |
41 | 518 | node.boot_interface_id = parent.id | 520 | node.boot_interface_id = parent.id OR |
42 | 521 | node.boot_interface_id = parent_parent.id | ||
43 | 519 | ), | 522 | ), |
44 | 520 | False | 523 | False |
46 | 521 | ) AS is_boot | 524 | ) AS is_boot, |
47 | 525 | CASE | ||
48 | 526 | WHEN interface.type = 'bridge' AND | ||
49 | 527 | parent_parent.id = node.boot_interface_id THEN 1 | ||
50 | 528 | WHEN interface.type = 'bridge' AND | ||
51 | 529 | parent.id = node.boot_interface_id THEN 2 | ||
52 | 530 | WHEN interface.type = 'bond' AND | ||
53 | 531 | parent.id = node.boot_interface_id THEN 3 | ||
54 | 532 | WHEN interface.type = 'physical' AND | ||
55 | 533 | interface.id = node.boot_interface_id THEN 4 | ||
56 | 534 | WHEN interface.type = 'bond' THEN 5 | ||
57 | 535 | WHEN interface.type = 'physical' THEN 6 | ||
58 | 536 | WHEN interface.type = 'vlan' THEN 7 | ||
59 | 537 | WHEN interface.type = 'alias' THEN 8 | ||
60 | 538 | WHEN interface.type = 'unknown' THEN 9 | ||
61 | 539 | ELSE 10 | ||
62 | 540 | END AS preference, | ||
63 | 541 | family(staticip.ip) AS family | ||
64 | 522 | FROM | 542 | FROM |
65 | 523 | maasserver_interface AS interface | 543 | maasserver_interface AS interface |
66 | 524 | LEFT OUTER JOIN maasserver_interfacerelationship AS rel ON | 544 | LEFT OUTER JOIN maasserver_interfacerelationship AS rel ON |
67 | 525 | interface.id = rel.child_id | 545 | interface.id = rel.child_id |
68 | 526 | LEFT OUTER JOIN maasserver_interface AS parent ON | 546 | LEFT OUTER JOIN maasserver_interface AS parent ON |
69 | 527 | rel.parent_id = parent.id | 547 | rel.parent_id = parent.id |
70 | 548 | LEFT OUTER JOIN maasserver_interfacerelationship AS parent_rel ON | ||
71 | 549 | parent.id = parent_rel.child_id | ||
72 | 550 | LEFT OUTER JOIN maasserver_interface AS parent_parent ON | ||
73 | 551 | parent_rel.parent_id = parent_parent.id | ||
74 | 528 | JOIN maasserver_node AS node ON | 552 | JOIN maasserver_node AS node ON |
75 | 529 | node.id = interface.node_id | 553 | node.id = interface.node_id |
76 | 530 | JOIN maasserver_domain AS domain ON | 554 | JOIN maasserver_domain AS domain ON |
77 | @@ -562,21 +586,10 @@ class StaticIPAddressManager(Manager): | |||
78 | 562 | staticip.ip IS NOT NULL AND | 586 | staticip.ip IS NOT NULL AND |
79 | 563 | host(staticip.ip) != '' | 587 | host(staticip.ip) != '' |
80 | 564 | ORDER BY | 588 | ORDER BY |
82 | 565 | node.hostname, | 589 | fqdn, |
83 | 566 | is_boot DESC, | 590 | is_boot DESC, |
97 | 567 | family(staticip.ip), | 591 | family, |
98 | 568 | CASE | 592 | preference, |
86 | 569 | WHEN interface.type = 'bond' AND | ||
87 | 570 | parent.id = node.boot_interface_id THEN 1 | ||
88 | 571 | WHEN interface.type = 'physical' AND | ||
89 | 572 | interface.id = node.boot_interface_id THEN 2 | ||
90 | 573 | WHEN interface.type = 'bond' THEN 3 | ||
91 | 574 | WHEN interface.type = 'physical' THEN 4 | ||
92 | 575 | WHEN interface.type = 'vlan' THEN 5 | ||
93 | 576 | WHEN interface.type = 'alias' THEN 6 | ||
94 | 577 | WHEN interface.type = 'unknown' THEN 7 | ||
95 | 578 | ELSE 8 | ||
96 | 579 | END, | ||
99 | 580 | /* | 593 | /* |
100 | 581 | * We want STICKY and USER_RESERVED addresses to be preferred, | 594 | * We want STICKY and USER_RESERVED addresses to be preferred, |
101 | 582 | * followed by AUTO, DHCP, and finally DISCOVERED. | 595 | * followed by AUTO, DHCP, and finally DISCOVERED. |
102 | diff --git a/src/maasserver/models/tests/test_staticipaddress.py b/src/maasserver/models/tests/test_staticipaddress.py | |||
103 | index 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 | 837 | node.system_id, 30, {vlanip.ip}, node.node_type)} | 837 | node.system_id, 30, {vlanip.ip}, node.node_type)} |
108 | 838 | self.assertEqual(expected_mapping, mapping) | 838 | self.assertEqual(expected_mapping, mapping) |
109 | 839 | 839 | ||
110 | 840 | def test_get_hostname_ip_mapping_prefers_bridged_bond_pxe_interface(self): | ||
111 | 841 | subnet = factory.make_Subnet( | ||
112 | 842 | cidr='10.0.0.0/24', dns_servers=[], gateway_ip='10.0.0.1') | ||
113 | 843 | node = factory.make_Node_with_Interface_on_Subnet( | ||
114 | 844 | hostname='host', subnet=subnet) | ||
115 | 845 | eth0 = node.get_boot_interface() | ||
116 | 846 | eth0.name = 'eth0' | ||
117 | 847 | eth0.save() | ||
118 | 848 | eth1 = factory.make_Interface( | ||
119 | 849 | INTERFACE_TYPE.PHYSICAL, node=node, name='eth1', vlan=subnet.vlan) | ||
120 | 850 | eth2 = factory.make_Interface( | ||
121 | 851 | INTERFACE_TYPE.PHYSICAL, node=node, name='eth2', vlan=subnet.vlan) | ||
122 | 852 | node.boot_interface = eth1 | ||
123 | 853 | node.save() | ||
124 | 854 | bond0 = factory.make_Interface( | ||
125 | 855 | INTERFACE_TYPE.BOND, node=node, parents=[eth1, eth2], name='bond0') | ||
126 | 856 | br_bond0 = factory.make_Interface( | ||
127 | 857 | INTERFACE_TYPE.BRIDGE, parents=[bond0], name='br-bond0') | ||
128 | 858 | phy_staticip = factory.make_StaticIPAddress( | ||
129 | 859 | alloc_type=IPADDRESS_TYPE.STICKY, interface=eth0, | ||
130 | 860 | subnet=subnet, ip='10.0.0.2') | ||
131 | 861 | bridge_ip = factory.make_StaticIPAddress( | ||
132 | 862 | alloc_type=IPADDRESS_TYPE.STICKY, interface=br_bond0, | ||
133 | 863 | subnet=subnet, ip='10.0.0.3') | ||
134 | 864 | # Make an IP addresses that shouldn't be preferred, but is also on | ||
135 | 865 | # an interface whose parent is a boot interface. (Some caution is | ||
136 | 866 | # advised when doing this, since we have interface signals that will | ||
137 | 867 | # automatically remove these addresses in some cases.) | ||
138 | 868 | bond0_extra_ip = factory.make_StaticIPAddress( | ||
139 | 869 | alloc_type=IPADDRESS_TYPE.STICKY, interface=bond0, | ||
140 | 870 | subnet=subnet, ip='10.0.0.4') | ||
141 | 871 | mapping = StaticIPAddress.objects.get_hostname_ip_mapping( | ||
142 | 872 | node.domain) | ||
143 | 873 | expected_mapping = { | ||
144 | 874 | node.fqdn: HostnameIPMapping( | ||
145 | 875 | node.system_id, 30, {bridge_ip.ip}, node.node_type), | ||
146 | 876 | "%s.%s" % (eth0.name, node.fqdn): HostnameIPMapping( | ||
147 | 877 | node.system_id, 30, {phy_staticip.ip}, node.node_type), | ||
148 | 878 | "%s.%s" % (bond0.name, node.fqdn): HostnameIPMapping( | ||
149 | 879 | node.system_id, 30, {bond0_extra_ip.ip}, node.node_type) | ||
150 | 880 | } | ||
151 | 881 | self.assertThat(mapping, Equals(expected_mapping)) | ||
152 | 882 | |||
153 | 883 | def test_get_hostname_ip_mapping_with_v4_and_v6_and_bridged_bonds(self): | ||
154 | 884 | subnet_v4 = factory.make_Subnet( | ||
155 | 885 | cidr=str(factory.make_ipv4_network().cidr)) | ||
156 | 886 | subnet_v6 = factory.make_Subnet( | ||
157 | 887 | cidr='2001:db8::/64') | ||
158 | 888 | node = factory.make_Node_with_Interface_on_Subnet( | ||
159 | 889 | hostname='host', subnet=subnet_v4) | ||
160 | 890 | eth0 = node.get_boot_interface() | ||
161 | 891 | eth0.name = 'eth0' | ||
162 | 892 | eth0.save() | ||
163 | 893 | eth1 = factory.make_Interface( | ||
164 | 894 | INTERFACE_TYPE.PHYSICAL, node=node, name='eth1') | ||
165 | 895 | eth2 = factory.make_Interface( | ||
166 | 896 | INTERFACE_TYPE.PHYSICAL, node=node, name='eth2') | ||
167 | 897 | node.boot_interface = eth1 | ||
168 | 898 | node.save() | ||
169 | 899 | bond0 = factory.make_Interface( | ||
170 | 900 | INTERFACE_TYPE.BOND, node=node, parents=[eth1, eth2], name='bond0') | ||
171 | 901 | br_bond0 = factory.make_Interface( | ||
172 | 902 | INTERFACE_TYPE.BRIDGE, parents=[bond0], name='br-bond0') | ||
173 | 903 | phy_staticip_v4 = factory.make_StaticIPAddress( | ||
174 | 904 | alloc_type=IPADDRESS_TYPE.STICKY, interface=eth0, | ||
175 | 905 | subnet=subnet_v4) | ||
176 | 906 | bridge_ip_v4 = factory.make_StaticIPAddress( | ||
177 | 907 | alloc_type=IPADDRESS_TYPE.STICKY, interface=br_bond0, | ||
178 | 908 | subnet=subnet_v4) | ||
179 | 909 | phy_staticip_v6 = factory.make_StaticIPAddress( | ||
180 | 910 | alloc_type=IPADDRESS_TYPE.STICKY, interface=eth0, | ||
181 | 911 | subnet=subnet_v6) | ||
182 | 912 | bridge_ip_v6 = factory.make_StaticIPAddress( | ||
183 | 913 | alloc_type=IPADDRESS_TYPE.STICKY, interface=br_bond0, | ||
184 | 914 | subnet=subnet_v6) | ||
185 | 915 | # Make a few IP addresses that shouldn't be associated with the | ||
186 | 916 | # default hosname. (They'll show up as associated with bond0, however.) | ||
187 | 917 | # Technically MAAS should remove these since they belong to a parent | ||
188 | 918 | # interface, but in this case the interface won't be re-saved. | ||
189 | 919 | extra_ipv4 = factory.make_StaticIPAddress( | ||
190 | 920 | alloc_type=IPADDRESS_TYPE.STICKY, interface=bond0, | ||
191 | 921 | subnet=subnet_v4) | ||
192 | 922 | extra_ipv6 = factory.make_StaticIPAddress( | ||
193 | 923 | alloc_type=IPADDRESS_TYPE.STICKY, interface=bond0, | ||
194 | 924 | subnet=subnet_v6) | ||
195 | 925 | mapping = StaticIPAddress.objects.get_hostname_ip_mapping( | ||
196 | 926 | node.domain) | ||
197 | 927 | expected_mapping = { | ||
198 | 928 | node.fqdn: HostnameIPMapping( | ||
199 | 929 | node.system_id, 30, {bridge_ip_v4.ip, bridge_ip_v6.ip}, | ||
200 | 930 | node.node_type), | ||
201 | 931 | "%s.%s" % (eth0.name, node.fqdn): HostnameIPMapping( | ||
202 | 932 | node.system_id, 30, {phy_staticip_v4.ip, phy_staticip_v6.ip}, | ||
203 | 933 | node.node_type), | ||
204 | 934 | "%s.%s" % (bond0.name, node.fqdn): HostnameIPMapping( | ||
205 | 935 | node.system_id, 30, {extra_ipv4.ip, extra_ipv6.ip}, | ||
206 | 936 | node.node_type) | ||
207 | 937 | } | ||
208 | 938 | self.assertThat(mapping, Equals(expected_mapping)) | ||
209 | 939 | |||
210 | 840 | def test_get_hostname_ip_mapping_returns_domain_head_ips(self): | 940 | def test_get_hostname_ip_mapping_returns_domain_head_ips(self): |
211 | 841 | parent = factory.make_Domain() | 941 | parent = factory.make_Domain() |
212 | 842 | name = factory.make_name() | 942 | name = factory.make_name() |
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.