Merge ~blake-rouse/maas:improved-dns-reload into maas:master

[Blake Rouse]

Fixes LP: #1710308, #1710278 - After BIND reload verify that the serial for all authoritative domains the new value before continuing.

[Mike Pontillo (mpontillo)]

Looks good so far; I have some comments and questions inline below.

review: Needs Information

[Mike Pontillo (mpontillo)]

Thanks for the replies. Sounds good.

review: Approve

[MAAS Lander (maas-lander)]

LANDING
-b improved-dns-reload lp:~blake-rouse/maas into -b master lp:~maas-committers/maas

STATUS: FAILED BUILD
LOG: http://maas-ci-jenkins.internal:8080/job/maas/job/branch-tester/228/consoleText

[Felipe Reyes (freyes)]

Hello,

I'm trying to understand how this patch "throttles bind9 reloads", if I'm reading this correctly, this will only check (after rndc reload is ran) that the reload succeeded querying and verifying the serial is the expected one.

If there is a storm of dhcp requests and produces tenths of events (e.g. commissioning many nodes at the same time, or a rack coming back from a power failure), bind9 will still be reloaded for everyone of those events even if they are within the same second.

Best,

[Mike Pontillo (mpontillo)]

@freyes, it was my understanding after looking at the code with Blake that the surrounding code in MAAS already prevents concurrent reloads. (That is, the `rdnc` command sends a message to the BIND process, the BIND process asynchronously processes it, and control is returned to MAAS almost immediately.) This patch just ensures that the next reload starts after the previous reload has completed.

[Felipe Reyes (freyes)]

there are no concurrent reloads, but there is no throttling, e.g. if 2 events within the same second arrive, maas will run 'rndc reload' twice, while it could just run it once at the end of processing of the second event (the last that finishes it task), that's what it's suggested in the bug description:

""" - Throttle consecutive updates. (For example, when the first DNS change occurs, schedule a task for a few seconds in the future which will update DNS, and don't ever schedule more additional updates while that task is pending.)"""

[Mike Pontillo (mpontillo)]

That's true. My bug description did overstep a little bit in that it described how I would have personally fixed the issue. Since this patch doesn't address that aspect of the solution, I'll probably need to change the title/description and file another bug for the "excessive reloads" aspect.

[Mike Pontillo (mpontillo)]

I've filed bug #1712205 to address the idea that we should batch multiple consecutive DNS changes into a single reload.

[Andres Rodriguez (andreserl)]

We should double check, but I thought Maas already did that (before this
bugfix?)

On Mon, Aug 21, 2017 at 5:36 PM Mike Pontillo <email address hidden>
wrote:

> I've filed bug #1712205 to address the idea that we should batch multiple
> consecutive DNS changes into a single reload.
> --
> https://code.launchpad.net/~blake-rouse/maas/+git/maas/+merge/329260
> Your team MAAS Committers is subscribed to branch maas:master.
>
--
Andres Rodriguez
Engineering Manager, MAAS
Canonical USA, Inc.

[Mike Pontillo (mpontillo)]

What MAAS already did was to prevent **concurrent outstanding reload requests**. That is, if the "rndc reload" command was already being executed and another DNS change came in, MAAS would (even before this branch) not try to issue *another* reload at the same time. However, the "rndc reload" returns so quickly (as it asynchronously causes BIND to start reloading) that we could still effectively have two concurrent reloads prior to this branch landing.

[Mike Pontillo (mpontillo)]

After discussing this again, Blake reminded me that there is an implicit throttle here when we pause for a second to await the DNS query timeout while we check, then pause for two seconds while we wait for the zone to reload.

So this branch *will* implicitly thottle reload requests, unless BIND reloads extremely fast and MAAS checks and confirms the zone before any of the pauses occur.

[Felipe Reyes (freyes)]

Mike, thanks for addressing my comments, and specially for filing another bug for the throttling part. I will keep an eye on that one ;)

Best,

[Blake Rouse (blake-rouse)]

Felipe,

The throttling bug has been changed, as its not a throttling issue. My branch introduces throttling based on how the DNS service handles the culling of updates.

But I am looking into improving the DNS triggers so less DNS updates are performed.

[Felipe Reyes (freyes)]

Yes, I got it ;)

diff --git a/src/maasserver/dns/config.py b/src/maasserver/dns/config.py
index 855cc47..e27773a 100644
--- a/src/maasserver/dns/config.py
+++ b/src/maasserver/dns/config.py
@@ -88,6 +88,12 @@ def dns_update_all_zones(reload_retry=False):
     else:
         bind_reload()
 
+    # Return the current serial and list of domain names.
+    return serial, [
+        domain.name
+        for domain in domains
+    ]
+
 
 def get_upstream_dns():
     """Return the IP addresses of configured upstream DNS servers.
diff --git a/src/maasserver/dns/tests/test_config.py b/src/maasserver/dns/tests/test_config.py
index 029e874..c163420 100644
--- a/src/maasserver/dns/tests/test_config.py
+++ b/src/maasserver/dns/tests/test_config.py
@@ -53,6 +53,7 @@ from testtools.matchers import (
     Contains,
     Equals,
     FileContains,
+    MatchesSetwise,
     MatchesStructure,
 )
 
@@ -302,6 +303,24 @@ class TestDNSConfigModifications(TestDNSServer):
             port=self.bind.config.port, commands=[ns_record, '+short'])
         self.assertEqual(ip, ip_of_ns_record)
 
+    def test_dns_update_all_zones_returns_serial_and_domains(self):
+        self.patch(settings, 'DNS_CONNECT', True)
+        domain = factory.make_Domain()
+        # These domains should not show up. Just to test we create them.
+        for _ in range(3):
+            factory.make_Domain(authoritative=False)
+        node, static = self.create_node_with_static_ip(domain=domain)
+        fake_serial = random.randint(1, 1000)
+        self.patch(
+            dns_config_module,
+            "current_zone_serial").return_value = fake_serial
+        serial, domains = dns_update_all_zones()
+        self.assertThat(serial, Equals(fake_serial))
+        self.assertThat(domains, MatchesSetwise(*[
+            Equals(domain.name)
+            for domain in Domain.objects.filter(authoritative=True)
+        ]))
+
 
 class TestDNSDynamicIPAddresses(TestDNSServer):
     """Allocated nodes with IP addresses in the dynamic range get a DNS
diff --git a/src/maasserver/region_controller.py b/src/maasserver/region_controller.py
index f3a51f3..0f9e181 100644
--- a/src/maasserver/region_controller.py
+++ b/src/maasserver/region_controller.py
@@ -31,16 +31,25 @@ from provisioningserver.logger import LegacyLogger
 from provisioningserver.utils.twisted import (
     asynchronous,
     FOREVER,
+    pause,
 )
 from twisted.application.service import Service
 from twisted.internet import reactor
-from twisted.internet.defer import DeferredList
+from twisted.internet.defer import (
+    DeferredList,
+    inlineCallbacks,
+)
 from twisted.internet.task import LoopingCall
+from twisted.names.client import Resolver
 
 
 log = LegacyLogger()
 
 
+class DNSReloadError(Exception):
+    """Error raised when the bind never fully reloads the zone."""
+
+
 class RegionControllerService(Service):
     """
     A service that controllers external services that are in MAAS's control on
@@ -64,6 +73,9 @@ class RegionControllerService(Service):
         self.needsDNSUpdate = False
         self.needsProxyUpdate = False
         self.postgresListener = postgresListener
+        self.dnsResolver = Resolver(
+            resolv=None, servers=[('127.0.0.1', 53)],
+            timeout=(1,), reactor=clock)
 
     @asynchronous(timeout=FOREVER)
     def startService(self):
@@ -108,9 +120,12 @@ class RegionControllerService(Service):
         if self.needsDNSUpdate:
             self.needsDNSUpdate = False
             d = deferToDatabase(transactional(dns_update_all_zones))
+            d.addCallback(self._check_serial)
             d.addCallback(
-                lambda _: log.msg(
-                    "Successfully configured DNS."))
+                lambda domains: log.msg(
+                    "Successfully configured DNS "
+                    "authoritative zones: %s." % (
+                        ', '.join(domains))))
             d.addErrback(
                 log.err,
                 "Failed configuring DNS.")
@@ -131,3 +146,31 @@ class RegionControllerService(Service):
             self.processingDefer = None
         else:
             return DeferredList(defers)
+
+    @inlineCallbacks
+    def _check_serial(self, result):
+        """Check that the serial of the domain is updated."""
+        serial, domain_names = result
+        not_matching_domains = set(domain_names)
+        loop = 0
+        while len(not_matching_domains) > 0 and loop != 30:
+            for domain in list(not_matching_domains):
+                try:
+                    answers, _, _ = yield self.dnsResolver.lookupAuthority(
+                        domain)
+                except (ValueError, TimeoutError):
+                    answers = []
+                if len(answers) > 0:
+                    if int(answers[0].payload.serial) == int(serial):
+                        not_matching_domains.remove(domain)
+            loop += 1
+            yield pause(2)
+        # 30 retries with 2 second pauses (aka. 60 seconds) has passed and
+        # there still is a domain that has the wrong serial. For now just
+        # raise the error, in the future we should take action and force
+        # restart bind.
+        if len(not_matching_domains) > 0:
+            raise DNSReloadError(
+                "Failed to reload DNS; serial mismatch "
+                "on domains %s" % ', '.join(not_matching_domains))
+        return domain_names
diff --git a/src/maasserver/tests/test_region_controller.py b/src/maasserver/tests/test_region_controller.py
index 9dbe4da..a4e83d2 100644
--- a/src/maasserver/tests/test_region_controller.py
+++ b/src/maasserver/tests/test_region_controller.py
@@ -5,6 +5,7 @@
 
 __all__ = []
 
+import random
 from unittest.mock import (
     ANY,
     call,
@@ -14,7 +15,10 @@ from unittest.mock import (
 
 from crochet import wait_for
 from maasserver import region_controller
-from maasserver.region_controller import RegionControllerService
+from maasserver.region_controller import (
+    DNSReloadError,
+    RegionControllerService,
+)
 from maasserver.testing.factory import factory
 from maasserver.testing.testcase import MAASServerTestCase
 from maastesting.matchers import (
@@ -22,6 +26,7 @@ from maastesting.matchers import (
     MockCallsMatch,
     MockNotCalled,
 )
+from testtools import ExpectedException
 from testtools.matchers import MatchesStructure
 from twisted.internet import reactor
 from twisted.internet.defer import (
@@ -29,6 +34,12 @@ from twisted.internet.defer import (
     inlineCallbacks,
     succeed,
 )
+from twisted.names.dns import (
+    A,
+    Record_SOA,
+    RRHeader,
+    SOA,
+)
 
 
 wait_for_reactor = wait_for(30)  # 30 seconds.
@@ -166,16 +177,28 @@ class TestRegionControllerService(MAASServerTestCase):
     def test_process_updates_zones(self):
         service = RegionControllerService(sentinel.listener)
         service.needsDNSUpdate = True
+        dns_result = (
+            random.randint(1, 1000), [
+                factory.make_name('domain')
+                for _ in range(3)
+            ])
         mock_dns_update_all_zones = self.patch(
             region_controller, "dns_update_all_zones")
+        mock_dns_update_all_zones.return_value = dns_result
+        mock_check_serial = self.patch(service, "_check_serial")
+        mock_check_serial.return_value = succeed(dns_result[1])
         mock_msg = self.patch(
             region_controller.log, "msg")
         service.startProcessing()
         yield service.processingDefer
         self.assertThat(mock_dns_update_all_zones, MockCalledOnceWith())
+        self.assertThat(mock_check_serial, MockCalledOnceWith(dns_result))
         self.assertThat(
             mock_msg,
-            MockCalledOnceWith("Successfully configured DNS."))
+            MockCalledOnceWith(
+                "Successfully configured DNS "
+                "authoritative zones: %s." % (
+                    ', '.join(dns_result[1]))))
 
     @wait_for_reactor
     @inlineCallbacks
@@ -236,8 +259,16 @@ class TestRegionControllerService(MAASServerTestCase):
         service = RegionControllerService(sentinel.listener)
         service.needsDNSUpdate = True
         service.needsProxyUpdate = True
+        dns_result = (
+            random.randint(1, 1000), [
+                factory.make_name('domain')
+                for _ in range(3)
+            ])
         mock_dns_update_all_zones = self.patch(
             region_controller, "dns_update_all_zones")
+        mock_dns_update_all_zones.return_value = dns_result
+        mock_check_serial = self.patch(service, "_check_serial")
+        mock_check_serial.return_value = succeed(dns_result[1])
         mock_proxy_update_config = self.patch(
             region_controller, "proxy_update_config")
         mock_proxy_update_config.return_value = succeed(None)
@@ -245,5 +276,97 @@ class TestRegionControllerService(MAASServerTestCase):
         yield service.processingDefer
         self.assertThat(
             mock_dns_update_all_zones, MockCalledOnceWith())
+        self.assertThat(mock_check_serial, MockCalledOnceWith(dns_result))
         self.assertThat(
             mock_proxy_update_config, MockCalledOnceWith(reload_proxy=True))
+
+    def make_soa_result(self, serial):
+        return RRHeader(
+            type=SOA, cls=A, ttl=30, payload=Record_SOA(serial=serial))
+
+    @wait_for_reactor
+    def test__check_serial_doesnt_raise_error_on_successful_serial_match(self):
+        service = RegionControllerService(sentinel.listener)
+        result_serial = random.randint(1, 1000)
+        formatted_serial = '{0:10d}'.format(result_serial)
+        dns_names = [
+            factory.make_name('domain')
+            for _ in range(3)
+        ]
+        # Mock pause so test runs faster.
+        self.patch(region_controller, "pause").return_value = succeed(None)
+        mock_lookup = self.patch(service.dnsResolver, "lookupAuthority")
+        mock_lookup.side_effect = [
+            # First pass no results.
+            succeed(([], [], [])),
+            succeed(([], [], [])),
+            succeed(([], [], [])),
+            # First domain valid result.
+            succeed(([self.make_soa_result(result_serial)], [], [])),
+            succeed(([], [], [])),
+            succeed(([], [], [])),
+            # Second domain wrong serial.
+            succeed(([self.make_soa_result(result_serial - 1)], [], [])),
+            succeed(([], [], [])),
+            # Third domain correct serial.
+            succeed(([], [], [])),
+            succeed(([self.make_soa_result(result_serial)], [], [])),
+            # Second domain correct serial.
+            succeed(([self.make_soa_result(result_serial)], [], [])),
+        ]
+        # Error should not be raised.
+        return service._check_serial((formatted_serial, dns_names))
+
+    @wait_for_reactor
+    @inlineCallbacks
+    def test__check_serial_raise_error_after_30_tries(self):
+        service = RegionControllerService(sentinel.listener)
+        result_serial = random.randint(1, 1000)
+        formatted_serial = '{0:10d}'.format(result_serial)
+        dns_names = [
+            factory.make_name('domain')
+            for _ in range(3)
+        ]
+        # Mock pause so test runs faster.
+        self.patch(region_controller, "pause").return_value = succeed(None)
+        mock_lookup = self.patch(service.dnsResolver, "lookupAuthority")
+        mock_lookup.side_effect = lambda *args: succeed(([], [], []))
+        # Error should not be raised.
+        with ExpectedException(DNSReloadError):
+            yield service._check_serial((formatted_serial, dns_names))
+
+    @wait_for_reactor
+    @inlineCallbacks
+    def test__check_serial_handles_ValueError(self):
+        service = RegionControllerService(sentinel.listener)
+        result_serial = random.randint(1, 1000)
+        formatted_serial = '{0:10d}'.format(result_serial)
+        dns_names = [
+            factory.make_name('domain')
+            for _ in range(3)
+        ]
+        # Mock pause so test runs faster.
+        self.patch(region_controller, "pause").return_value = succeed(None)
+        mock_lookup = self.patch(service.dnsResolver, "lookupAuthority")
+        mock_lookup.side_effect = ValueError()
+        # Error should not be raised.
+        with ExpectedException(DNSReloadError):
+            yield service._check_serial((formatted_serial, dns_names))
+
+    @wait_for_reactor
+    @inlineCallbacks
+    def test__check_serial_handles_TimeoutError(self):
+        service = RegionControllerService(sentinel.listener)
+        result_serial = random.randint(1, 1000)
+        formatted_serial = '{0:10d}'.format(result_serial)
+        dns_names = [
+            factory.make_name('domain')
+            for _ in range(3)
+        ]
+        # Mock pause so test runs faster.
+        self.patch(region_controller, "pause").return_value = succeed(None)
+        mock_lookup = self.patch(service.dnsResolver, "lookupAuthority")
+        mock_lookup.side_effect = TimeoutError()
+        # Error should not be raised.
+        with ExpectedException(DNSReloadError):
+            yield service._check_serial((formatted_serial, dns_names))