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

[Mike Pontillo]

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

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

[MAAS Lander (maas-lander)]

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

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

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

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

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

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

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

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

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

@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)]

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

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

questions inline.

[Mike Pontillo (mpontillo)]

Responses inline.

[Andres Rodriguez (andreserl)]

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

diff --git a/src/maasserver/server_address.py b/src/maasserver/server_address.py
index b543556..e786068 100644
--- a/src/maasserver/server_address.py
+++ b/src/maasserver/server_address.py
@@ -110,17 +110,21 @@ def get_maas_facing_server_addresses(
         raise UnresolvableHost("No address found for host %s." % hostname)
     if not link_local:
         addresses = [ip for ip in addresses if not ip.is_link_local()]
+    # Addresses must be returned in a consistent order, with the set of IP
+    # addresses found from get_maas_facing_server_host() (i.e. maas_url)
+    # coming first.
+    addresses = sorted(addresses)
     if include_alternates:
         maas_id = get_maas_id()
         if maas_id is not None:
             # Circular imports
             from maasserver.models import Subnet
             from maasserver.models import StaticIPAddress
-            # Keep track of the regions already represented.
+            # Don't include more than one alternate IP address, per region,
+            # per address-family.
             regions = set()
             alternate_ips = []
             for ip in addresses:
-                regions.add(maas_id + str(ip.version))
                 if not ip.is_link_local():
                     # Since we only know that the IP address given in the MAAS
                     # URL is reachable, alternates must be pulled from the same
@@ -149,5 +153,10 @@ def get_maas_facing_server_addresses(
                             else:
                                 regions.add(id_plus_family)
                                 alternate_ips.append(ipa)
-            addresses.extend(alternate_ips)
+            # Append non-duplicate region IP addresses to the list of
+            # addresses to return. We don't want duplicates, but we
+            # also need to preserve the existing order.
+            for address in alternate_ips:
+                if address not in addresses:
+                    addresses.append(address)
     return addresses
diff --git a/src/maasserver/tests/test_server_address.py b/src/maasserver/tests/test_server_address.py
index ff7640c..57d8ade 100644
--- a/src/maasserver/tests/test_server_address.py
+++ b/src/maasserver/tests/test_server_address.py
@@ -214,12 +214,36 @@ class TestGetMAASFacingServerAddresses(MAASServerTestCase):
             region_ips,
             Equals([
                 IPAddress("192.168.0.254"),
+                IPAddress("192.168.0.1"),
                 IPAddress("192.168.0.2"),
                 IPAddress("192.168.0.4"),
             ])
         )
 
-    def test__alternates_include_one_ip_address_per_region(self):
+    def test__alternates_do_not_contain_duplicate_for_maas_url_ip(self):
+        # See bug #1753493. (This tests to ensure we don't provide the same
+        # IP address from maas_url twice.) Also ensures that the IP address
+        # from maas_url comes first.
+        factory.make_Subnet(cidr='192.168.0.0/24')
+        maas_url = 'http://192.168.0.2/MAAS'
+        rack = factory.make_RackController(url=maas_url)
+        r1 = factory.make_RegionController()
+        factory.make_Interface(node=r1, ip='192.168.0.1')
+        r2 = factory.make_RegionController()
+        factory.make_Interface(node=r2, ip='192.168.0.2')
+        # Make the "current" region controller r1.
+        self.patch(server_address, 'get_maas_id').return_value = r1.system_id
+        region_ips = get_maas_facing_server_addresses(
+            rack, include_alternates=True)
+        self.assertThat(
+            region_ips,
+            Equals([
+                IPAddress("192.168.0.2"),
+                IPAddress("192.168.0.1"),
+            ])
+        )
+
+    def test__alternates_include_one_ip_address_per_region_and_maas_url(self):
         factory.make_Subnet(cidr='192.168.0.0/24')
         maas_url = 'http://192.168.0.254/MAAS'
         rack = factory.make_RackController(url=maas_url)
@@ -240,6 +264,7 @@ class TestGetMAASFacingServerAddresses(MAASServerTestCase):
             region_ips,
             Equals([
                 IPAddress("192.168.0.254"),
+                IPAddress("192.168.0.1"),
                 IPAddress("192.168.0.2"),
                 IPAddress("192.168.0.4"),
             ])
@@ -296,12 +321,12 @@ class TestGetMAASFacingServerAddresses(MAASServerTestCase):
         self.assertThat(
             region_ips,
             Equals([
-                IPAddress("2001:db8::1"),
                 IPAddress("192.168.0.1"),
-                IPAddress("2001:db8::2"),
-                IPAddress("2001:db8::4"),
+                IPAddress("2001:db8::1"),
                 IPAddress("192.168.0.2"),
                 IPAddress("192.168.0.4"),
+                IPAddress("2001:db8::2"),
+                IPAddress("2001:db8::4"),
             ])
         )