Merge ~mpontillo/maas:ha-dns-servers--bug-1753493 into maas:master

Proposed by Mike Pontillo on 2018-04-11
Status: Merged
Approved by: Mike Pontillo on 2018-04-11
Approved revision: 04614560bd9c49c79fc2ff6c0799eb19ed336386
Merge reported by: MAAS Lander
Merged at revision: not available
Proposed branch: ~mpontillo/maas:ha-dns-servers--bug-1753493
Merge into: maas:master
Diff against target: 105 lines (+41/-7)
2 files modified
src/maasserver/server_address.py (+12/-3)
src/maasserver/tests/test_server_address.py (+29/-4)
Reviewer Review Type Date Requested Status
Andres Rodriguez (community) Approve on 2018-04-11
MAAS Lander Approve on 2018-04-11
Alberto Donato 2018-04-11 Needs Fixing on 2018-04-11
Review via email: mp+342979@code.launchpad.net

Commit message

LP: #1753493 - Make region IP addresses consistent for HA.

Description of the change

Note: I backported this to MAAS 2.3 and verified that it does the right thing.

To post a comment you must log in.
MAAS Lander (maas-lander) wrote :

UNIT TESTS
-b ha-dns-servers--bug-1753493 lp:~mpontillo/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci-jenkins.internal:8080/job/maas/job/branch-tester/2379/console
COMMIT: a566ab08c57e4ebd9749d30dacdebbf3fbbca182

review: Needs Fixing
40ef157... by Mike Pontillo on 2018-04-11

Fix maasserver.models.tests.test_node:TestGetDefaultDNSServers.

MAAS Lander (maas-lander) wrote :

UNIT TESTS
-b ha-dns-servers--bug-1753493 lp:~mpontillo/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: 40ef15783f0ad0a1694811c082f9b5201550c78a

review: Approve
Alberto Donato (ack) :
review: Needs Fixing
Andres Rodriguez (andreserl) wrote :

Mike I have some inline comments, but also I have a few questions.

Say we have this:

1. Scenario 1:
region/rack1 - 10.1
region/rack2 - 10.2

This would yield DNS such as 10.1, 10.2

2. Scenario 2:
region/rack1 - 10.1 | foalting IP on same subnet 10.3
region/rack2 - 10.2

This would yield to DNS be 10.3, 10.1, 10.2

3. Scenario 3 (isolated region/rack networks):
region/rack1 - 9.1
region/rack2 - 9.2
rackd.conf:maas_url in both points to: 10.1 and they have a proxy in the rack.

This would yield to DNS be 10.1 and not (9.1 nor 9.2). This should be the same for deployed machines.

review: Needs Information
Andres Rodriguez (andreserl) wrote :

fwiw, in other words:

Do we always add what's in rackd.conf:maas_url as a DNS + whatever other IP addresses of the region controller in the same subnet ?

Andres Rodriguez (andreserl) wrote :

Ok, so I've quickly tested this, and it is dropping the IP of maas_url. I have this:

region/rack1: 90.1 & 90.2 (the latter is a "vip")
 - rackd.conf:maas_url: http://...90.1:5240/MAAS

region/rack2: 90.3
 - rackd:confmaas_url: http://...90.2:5240/MAAS

Before this change region/rackd2 have the following, which is missing 1 ip (90.3), but it is correctly using the value on rackd.conf:maas_url (which is the VIP 90.2)

ubuntu@node01:~$ cat /var/lib/maas/dhcpd.conf | grep domain-name-server
           option domain-name-servers 10.90.90.2, 10.90.90.1;
           option domain-name-servers 10.90.90.2, 10.90.90.1;

AFter this change it is correctly using the value of both *actual* regions, but it is dropping the VIP.

ubuntu@node01:~$ cat /var/lib/maas/dhcpd.conf | grep domain-name-server
           option domain-name-servers 10.90.90.1, 10.90.90.3;
           option domain-name-servers 10.90.90.1, 10.90.90.3;

This branch is regressing things even further.

review: Needs Fixing
Andres Rodriguez (andreserl) wrote :

FWIW, what's missing here (90.2) is the IP where a rack is told the region is. So this should have:

1. The IP the rack controller knows is maas_url
2. The IP of all regions in the same subnet as maas_url IP, if exist.

But (1) should never be missing.

Mike Pontillo (mpontillo) wrote :

This is why I wanted to split up this function or add an additional
'purpose' parameter. (But I didn't do that yet, because I wanted the
smallest possible fix to backport.) Unless we know a floating IP address is
going to be used for HA, and not a reverse proxy, we cannot assume the IP
address from maas_url can be used for DNS. The simplest solution is to not
return it, but only in certain cases.

We will still return the maas_url based URL if you only ask for one IP
address. If you want alternatives, that means you're asking for DNS
servers, so the maas_url could actually be wrong, and what you really want
is to use the maas_url as an example of a known reachable subnet, and
return all the region IPs in that subnet. That's what this branch does.

On Wed, Apr 11, 2018, 07:26 Andres Rodriguez <email address hidden>
wrote:

> FWIW, what's missing here (90.2) is the IP where a rack is told the region
> is. So this should have:
>
> 1. The IP the rack controller knows is maas_url
> 2. The IP of all regions in the same subnet as maas_url IP, if exist.
>
> But (1) should never be missing.
> --
> https://code.launchpad.net/~mpontillo/maas/+git/maas/+merge/342979
> You are the owner of ~mpontillo/maas:ha-dns-servers--bug-1753493.
>

Andres Rodriguez (andreserl) wrote :

Mike,

Lets go to the original design and the original intent of how this is supposed to work.

It does *not* matter whether this IP is a VIP or a real IP assigned to a machine that is on the same subnet, or on a routed subnet. The premise is that the IP needs to be routable for the machines.

As such, the design is that machines need to be told two things:

1. The IP of maas_url from the rack they boot from, because that's the only place where we know where the region is (regardless of whether this is a VIP or not).
2. The *other* region IP address that face the machines.

What's missing here is maas_url which should *always* be present regardless of it being a single region/rack, a multi-region in the same subnet, a multi-region with VIP's or a split region/rack with a routed l3.

review: Needs Fixing
Andres Rodriguez (andreserl) wrote :

FWIW, this is the output of a deployed machine. The rackd.conf:maas_url: http://10.90.90.2:5240/MAAS

That tells the machines being deployed that;

1. The region is in 90.2
2. The metadata server is in 90.2
3. The proxy is in 90.2
4. The DNS server is in 90.2

As such, it doesn't matter if this IP is routed, on same subnet, a VIP or whatever. MAAS needs to be smart about this, because:

1. Machine facing rack controllers know where the region is (10.2).
2. MAAS knows the network model, and can determine that there's regions on the same subnet (10.1, 10.3)

Based on your changes, there's a lot of inconsistencies here provided that everything but DNS is correct (note that NTP is a different story, because NTP is based on rack connections).

ubuntu@pod-node01:/etc$ grep -sr 10.90.90. *
apt/apt.conf.d/90curtin-aptproxy:Acquire::http::Proxy "http://10.90.90.2:8000/";
cloud/cloud.cfg.d/90_dpkg_local_cloud_config.cfg: proxy: http://10.90.90.2:8000/
cloud/cloud.cfg.d/90_dpkg_local_cloud_config.cfg:apt_proxy: http://10.90.90.2:8000/
cloud/cloud.cfg.d/90_dpkg_local_cloud_config.cfg: maas: {consumer_key: vxJUs6nQy7SZ4rfsdy, endpoint: 'http://10.90.90.2:5240/MAAS/metadata/status/r73ym7',
cloud/cloud.cfg.d/50-curtin-networking.cfg: - address: 10.90.90.189/24
cloud/cloud.cfg.d/50-curtin-networking.cfg: gateway: 10.90.90.1
cloud/cloud.cfg.d/50-curtin-networking.cfg: - 10.90.90.3
cloud/cloud.cfg.d/50-curtin-networking.cfg: - 10.90.90.1
network/interfaces.d/50-cloud-init.cfg: dns-nameservers 10.90.90.3 10.90.90.1
network/interfaces.d/50-cloud-init.cfg: address 10.90.90.189/24
network/interfaces.d/50-cloud-init.cfg: gateway 10.90.90.1
ntp.conf:server 10.90.90.1 iburst
ntp.conf:server 10.90.90.3 iburst
resolv.conf:nameserver 10.90.90.3
resolv.conf:nameserver 10.90.90.1

Mike Pontillo (mpontillo) wrote :

@ack, thanks for your comments; the reason a sorted() set wasn't used is because the first returned IP address was supposed to be the maas_url, so we only wanted a partial sort. I'll consider your feedback as I'm fixing up this branch though.

Mike Pontillo (mpontillo) wrote :

Additional comments inline.

e160aa7... by Mike Pontillo on 2018-04-11

Address review comments.

0461456... by Mike Pontillo on 2018-04-11

Clean up comments.

MAAS Lander (maas-lander) wrote :

UNIT TESTS
-b ha-dns-servers--bug-1753493 lp:~mpontillo/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: 04614560bd9c49c79fc2ff6c0799eb19ed336386

review: Approve
Andres Rodriguez (andreserl) wrote :

questions inline.

Mike Pontillo (mpontillo) wrote :

Responses inline.

Andres Rodriguez (andreserl) wrote :

Testing went well. I did notice a weird issue but could have been due to manual modifucation of the code. ONce this lands I'll do another round of testing.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/src/maasserver/server_address.py b/src/maasserver/server_address.py
2index b543556..e786068 100644
3--- a/src/maasserver/server_address.py
4+++ b/src/maasserver/server_address.py
5@@ -110,17 +110,21 @@ def get_maas_facing_server_addresses(
6 raise UnresolvableHost("No address found for host %s." % hostname)
7 if not link_local:
8 addresses = [ip for ip in addresses if not ip.is_link_local()]
9+ # Addresses must be returned in a consistent order, with the set of IP
10+ # addresses found from get_maas_facing_server_host() (i.e. maas_url)
11+ # coming first.
12+ addresses = sorted(addresses)
13 if include_alternates:
14 maas_id = get_maas_id()
15 if maas_id is not None:
16 # Circular imports
17 from maasserver.models import Subnet
18 from maasserver.models import StaticIPAddress
19- # Keep track of the regions already represented.
20+ # Don't include more than one alternate IP address, per region,
21+ # per address-family.
22 regions = set()
23 alternate_ips = []
24 for ip in addresses:
25- regions.add(maas_id + str(ip.version))
26 if not ip.is_link_local():
27 # Since we only know that the IP address given in the MAAS
28 # URL is reachable, alternates must be pulled from the same
29@@ -149,5 +153,10 @@ def get_maas_facing_server_addresses(
30 else:
31 regions.add(id_plus_family)
32 alternate_ips.append(ipa)
33- addresses.extend(alternate_ips)
34+ # Append non-duplicate region IP addresses to the list of
35+ # addresses to return. We don't want duplicates, but we
36+ # also need to preserve the existing order.
37+ for address in alternate_ips:
38+ if address not in addresses:
39+ addresses.append(address)
40 return addresses
41diff --git a/src/maasserver/tests/test_server_address.py b/src/maasserver/tests/test_server_address.py
42index ff7640c..57d8ade 100644
43--- a/src/maasserver/tests/test_server_address.py
44+++ b/src/maasserver/tests/test_server_address.py
45@@ -214,12 +214,36 @@ class TestGetMAASFacingServerAddresses(MAASServerTestCase):
46 region_ips,
47 Equals([
48 IPAddress("192.168.0.254"),
49+ IPAddress("192.168.0.1"),
50 IPAddress("192.168.0.2"),
51 IPAddress("192.168.0.4"),
52 ])
53 )
54
55- def test__alternates_include_one_ip_address_per_region(self):
56+ def test__alternates_do_not_contain_duplicate_for_maas_url_ip(self):
57+ # See bug #1753493. (This tests to ensure we don't provide the same
58+ # IP address from maas_url twice.) Also ensures that the IP address
59+ # from maas_url comes first.
60+ factory.make_Subnet(cidr='192.168.0.0/24')
61+ maas_url = 'http://192.168.0.2/MAAS'
62+ rack = factory.make_RackController(url=maas_url)
63+ r1 = factory.make_RegionController()
64+ factory.make_Interface(node=r1, ip='192.168.0.1')
65+ r2 = factory.make_RegionController()
66+ factory.make_Interface(node=r2, ip='192.168.0.2')
67+ # Make the "current" region controller r1.
68+ self.patch(server_address, 'get_maas_id').return_value = r1.system_id
69+ region_ips = get_maas_facing_server_addresses(
70+ rack, include_alternates=True)
71+ self.assertThat(
72+ region_ips,
73+ Equals([
74+ IPAddress("192.168.0.2"),
75+ IPAddress("192.168.0.1"),
76+ ])
77+ )
78+
79+ def test__alternates_include_one_ip_address_per_region_and_maas_url(self):
80 factory.make_Subnet(cidr='192.168.0.0/24')
81 maas_url = 'http://192.168.0.254/MAAS'
82 rack = factory.make_RackController(url=maas_url)
83@@ -240,6 +264,7 @@ class TestGetMAASFacingServerAddresses(MAASServerTestCase):
84 region_ips,
85 Equals([
86 IPAddress("192.168.0.254"),
87+ IPAddress("192.168.0.1"),
88 IPAddress("192.168.0.2"),
89 IPAddress("192.168.0.4"),
90 ])
91@@ -296,12 +321,12 @@ class TestGetMAASFacingServerAddresses(MAASServerTestCase):
92 self.assertThat(
93 region_ips,
94 Equals([
95- IPAddress("2001:db8::1"),
96 IPAddress("192.168.0.1"),
97- IPAddress("2001:db8::2"),
98- IPAddress("2001:db8::4"),
99+ IPAddress("2001:db8::1"),
100 IPAddress("192.168.0.2"),
101 IPAddress("192.168.0.4"),
102+ IPAddress("2001:db8::2"),
103+ IPAddress("2001:db8::4"),
104 ])
105 )
106

Subscribers

People subscribed via source and target branches