Merge lp:~mpontillo/maas/ntp-issues--bug-1695083 into lp:~maas-committers/maas/trunk

[Mike Pontillo]

When selecting an NTP server address to provide via DHCP, prefer subnets on VLANs where DHCP is enabled.

[Blake Rouse (blake-rouse)]

Looks good.

review: Approve

=== modified file 'src/maasserver/dhcp.py'
--- src/maasserver/dhcp.py	2017-05-22 13:39:28 +0000
+++ src/maasserver/dhcp.py	2017-06-03 19:25:25 +0000
@@ -256,19 +256,36 @@
             IPADDRESS_TYPE.STICKY, IPADDRESS_TYPE.USER_RESERVED})
     rack_addresses = rack_addresses.exclude(subnet__isnull=True)
     rack_addresses = rack_addresses.order_by(
-        "subnet__vlan__space_id", "subnet__cidr", "ip")
+        # Prefer subnets with DHCP enabled.
+        "-subnet__vlan__dhcp_on",
+        "subnet__vlan__space_id",
+        "subnet__cidr",
+        "ip"
+    )
     rack_addresses = rack_addresses.values_list(
-        "subnet__vlan__space_id", "subnet__cidr", "ip")
+        "subnet__vlan__dhcp_on",
+        "subnet__vlan__space_id",
+        "subnet__cidr",
+        "ip"
+    )
 
     def get_space_id_and_family(record):
-        space_id, cidr, ip = record
+        dhcp_on, space_id, cidr, ip = record
         return space_id, IPNetwork(cidr).version
 
+    def sort_key__dhcp_on__ip(record):
+        dhcp_on, space_id, cidr, ip = record
+        return -int(dhcp_on), IPAddress(ip)
+
+    best_ntp_servers = {
+        space_id_and_family:
+            min(group, key=sort_key__dhcp_on__ip)
+            for space_id_and_family, group in groupby(
+                rack_addresses, get_space_id_and_family)
+    }
     return {
-        space_id_and_family: min(
-            (ip for _, _, ip in group), key=IPAddress)
-        for space_id_and_family, group in groupby(
-            rack_addresses, get_space_id_and_family)
+        key: value[3]
+        for key, value in best_ntp_servers.items()
     }
 
 

=== modified file 'src/maasserver/tests/test_dhcp.py'
--- src/maasserver/tests/test_dhcp.py	2017-05-30 18:36:16 +0000
+++ src/maasserver/tests/test_dhcp.py	2017-06-03 19:25:25 +0000
@@ -748,6 +748,33 @@
                     (address.ip for address in addresses), key=IPAddress),
             }))
 
+    def test__returned_dict_prefers_vlans_with_dhcp_on(self):
+        rack = factory.make_RackController()
+        space = factory.make_Space()
+        ip_version = random.choice([4, 6])
+        cidr1 = factory.make_ip4_or_6_network(version=ip_version, host_bits=16)
+        cidr2 = factory.make_ip4_or_6_network(version=ip_version, host_bits=16)
+        subnet1 = factory.make_Subnet(space=space, cidr=cidr1)
+        subnet2 = factory.make_Subnet(space=space, cidr=cidr2)
+        # Expect subnet2 to be selected, since DHCP is enabled.
+        subnet2.vlan.dhcp_on = True
+        subnet2.vlan.save()
+        interface = factory.make_Interface(node=rack)
+        # Make some addresses that won't be selected since they're on the
+        # incorrect VLAN (without DHCP enabled).
+        for _ in range(3):
+            factory.make_StaticIPAddress(
+                interface=interface, subnet=subnet1,
+                alloc_type=IPADDRESS_TYPE.STICKY)
+        expected_address = factory.make_StaticIPAddress(
+            interface=interface, subnet=subnet2,
+            alloc_type=IPADDRESS_TYPE.STICKY)
+        self.assertThat(
+            dhcp.get_ntp_server_addresses_for_rack(rack), Equals({
+                (space.id, subnet2.get_ipnetwork().version):
+                    expected_address.ip
+            }))
+
     def test__constant_query_count(self):
         rack = factory.make_RackController()
         interface = factory.make_Interface(node=rack)