Merge lp:~lamont/maas/dns-testfailures-check-soa into lp:~maas-committers/maas/trunk

[LaMont Jones]

Have Sequence remember what it last handed out via next(), and make sure that's initialized. Update dns test suite to check for the expected SOA for a zone before it actually tests the answer it wants. Remove extra (incorrect) bumping of the SOA.

Have Sequence remember what it last handed out via next(), and make sure that's initialized. Update dns test suite to check for the expected SOA for a zone before it actually tests the answer it wants. Remove extra (incorrect) bumping of the SOA.

[Mike Pontillo (mpontillo)]

Tested this on the lander; noted some failures while running bin/test.region maasserver.dns.tests -

https://paste.ubuntu.com/15270806/

[LaMont Jones (lamont)]

With bind9 1:9.10.3.dfsg.P2-5 (in xenial since last night), and this branch, 199 iterations of test_config.py running on the lander yielded 73128 total lines of output, and zero failures.

[Gavin Panella (allenap)]

This is awesome; thank you for fixing those tests, and for the BIND work.

I do have some serious concerns about the code in here, particularly around the changes to Sequence. However, I don't think anything invalidates your approach, it just needs fixing.

I wrote some code as I reviewed this. Where I've proposed new or altered code, you can get it by merging lp:~allenap/maas/dns-testfailures-check-soa if you want.

review: Needs Fixing

[Gavin Panella (allenap)]

Tip top.

review: Approve

=== modified file 'required-packages/dev'
--- required-packages/dev	2016-02-17 16:12:30 +0000
+++ required-packages/dev	2016-03-08 17:17:00 +0000
@@ -15,5 +15,6 @@
 npm
 python-pocket-lint
 python3-django-nose
+python3-dnspython
 socat
 xvfb
 
=== modified file 'src/maasserver/dns/config.py'
--- src/maasserver/dns/config.py	2016-02-11 23:37:45 +0000
+++ src/maasserver/dns/config.py	2016-03-08 17:17:00 +0000
@@ -89,12 +89,12 @@
         return
 
     default_ttl = Config.objects.get_config('default_dns_ttl')
+    serial = next_zone_serial()
     zones_to_write = ZoneGenerator(
         domains, subnets, default_ttl,
-        serial_generator=next_zone_serial).as_list()
+        serial).as_list()
     if len(zones_to_write) == 0:
         return None
-    serial = next_zone_serial()
     # Compute non-None zones.
     zones = ZoneGenerator(
         Domain.objects.all(), Subnet.objects.all(),
@@ -232,9 +232,10 @@
     domains = Domain.objects.filter(authoritative=True)
     subnets = Subnet.objects.exclude(rdns_mode=RDNS_MODE.DISABLED)
     default_ttl = Config.objects.get_config('default_dns_ttl')
+    serial = next_zone_serial()
     zones = ZoneGenerator(
         domains, subnets, default_ttl,
-        serial_generator=next_zone_serial).as_list()
+        serial).as_list()
     bind_write_zones(zones)
 
     # We should not be calling bind_write_options() here; call-sites should be
 
=== modified file 'src/maasserver/dns/tests/test_config.py'
--- src/maasserver/dns/tests/test_config.py	2016-03-02 16:01:57 +0000
+++ src/maasserver/dns/tests/test_config.py	2016-03-08 17:17:00 +0000
@@ -7,10 +7,10 @@
 
 import random
 import time
-from unittest import SkipTest
 
 from django.conf import settings
 from django.core.management import call_command
+import dns.resolver
 from maasserver import locks
 from maasserver.config import RegionConfiguration
 from maasserver.dns import config as dns_config_module
@@ -72,6 +72,7 @@
     BINDServer,
 )
 from provisioningserver.testing.tests.test_bindfixture import dig_call
+from provisioningserver.utils.twisted import retries
 from testtools.matchers import (
     Contains,
     FileContains,
@@ -84,12 +85,6 @@
 from twisted.internet.defer import Deferred
 
 
-def setUp():
-    raise SkipTest(
-        "XXX: GavinPanella 2016-03-02 bug=1550540: DNS test suite needs "
-        "to do better lockstep on checking that DNS updates happened.")
-
-
 def get_expected_names(node, ip):
     expected_names = [
         "%s.%s." % (iface.name, node.fqdn)
@@ -489,6 +484,10 @@
         self.patch(dns_config_module, "DNS_DEFER_UPDATES", False)
         # Create a DNS server.
         self.bind = self.useFixture(BINDServer())
+        # Use the dnspython resolver for at least some queries.
+        self.resolver = dns.resolver.Resolver()
+        self.resolver.nameservers = ['127.0.0.1']
+        self.resolver.port = self.bind.config.port
         patch_dns_config_path(self, self.bind.config.homedir)
         # Use a random port for rndc.
         patch_dns_rndc_port(self, allocate_ports("localhost")[0])
@@ -522,11 +521,66 @@
         dns_update_zones_now([domain], [subnet])
         return node, static_ip
 
-    def dig_resolve(self, fqdn, version=4):
+    def dns_wait_soa(self, fqdn, removing=False):
+        # Get the serial number for the zone containing the FQDN by asking DNS
+        # nicely for the SOA for the FQDN.  If it's top-of-zone, we get an
+        # answer, if it's not, we get the SOA in authority.
+
+        if not fqdn.endswith('.'):
+            fqdn = fqdn + '.'
+
+        for elapsed, remaining, wait in retries(15, 0.02):
+            query_name = fqdn
+
+            # Loop until we have a value for serial, be that numeric or None.
+            serial = undefined = object()
+            while serial is undefined:
+                try:
+                    ans = self.resolver.query(
+                        query_name, 'SOA', raise_on_no_answer=False)
+                except dns.resolver.NXDOMAIN:
+                    if removing:
+                        # The zone has gone; we're done.
+                        return
+                    elif "." in query_name:
+                        # Query the parent domain for the SOA record.
+                        # For most things, this will be the correct DNS zone.
+                        # In the case of SRV records, we'll actually need to
+                        # strip more, hence the loop.
+                        query_name = query_name.split('.', 1)[1]
+                    else:
+                        # We've hit the root zone; no SOA found.
+                        serial = None
+                except dns.resolver.NoNameservers:
+                    # No DNS service as yet.
+                    serial = None
+                else:
+                    # If we got here, then we either have (1) a situation where
+                    # the LHS exists in the DNS, but no SOA RR exists for that
+                    # LHS (because it's a node with an A or AAAA RR, and not
+                    # the domain...) or (2) an answer to our SOA query.
+                    # Either way, we get exactly one SOA in the reply: in the
+                    # first case, it's in the Authority section, in the second,
+                    # it's in the Answer section.
+                    if ans.rrset is None:
+                        serial = ans.response.authority[0].items[0].serial
+                    else:
+                        serial = ans.rrset.items[0].serial
+
+            if serial == zone_serial.current():
+                # The zone is up-to-date; we're done.
+                return
+            else:
+                time.sleep(wait)
+
+        self.fail("Timed-out waiting for %s to update." % fqdn)
+
+    def dig_resolve(self, fqdn, version=4, removing=False):
         """Resolve `fqdn` using dig.  Returns a list of results."""
         # Using version=6 has two effects:
         # - it changes the type of query from 'A' to 'AAAA';
         # - it forces dig to only use IPv6 query transport.
+        self.dns_wait_soa(fqdn, removing)
         record_type = 'AAAA' if version == 6 else 'A'
         commands = [fqdn, '+short', '-%i' % version, record_type]
         output = dig_call(
@@ -534,8 +588,9 @@
             commands=commands)
         return output.split('\n')
 
-    def dig_reverse_resolve(self, ip, version=4):
+    def dig_reverse_resolve(self, ip, version=4, removing=False):
         """Reverse resolve `ip` using dig.  Returns a list of results."""
+        self.dns_wait_soa(IPAddress(ip).reverse_dns, removing)
         output = dig_call(
             port=self.bind.config.port,
             commands=['-x', ip, '+short', '-%i' % version])
@@ -546,9 +601,6 @@
         if version == -1:
             version = IPAddress(ip).version
         fqdn = "%s.%s" % (hostname, domain)
-        start = time.time()
-        now = time.time()
-        forward_lookup_result = ['']
         # Give BIND enough time to process the rndc request.
         # XXX 2016-03-01 lamont bug=1550540 We should really query DNS for the
         # SOA that we (can) know to be the correct one, and wait for that
@@ -556,21 +608,14 @@
         # all of our tests go from having no answer for forward and/or reverse,
         # to having the expected answer, and just wait for a non-empty return,
         # or timeout (15 seconds because of slow jenkins sometimes.)
-        while forward_lookup_result == [''] and now - start < 15:
-            forward_lookup_result = self.dig_resolve(fqdn, version=version)
-            now = time.time()
-            time.sleep(0.05)
+        forward_lookup_result = self.dig_resolve(fqdn, version=version)
         self.assertThat(
             forward_lookup_result, Contains(ip),
             "Failed to resolve '%s' (results: '%s')." % (
                 fqdn, ','.join(forward_lookup_result)))
         # A reverse lookup on the IP address returns the hostname.
-        reverse_lookup_result = ['']
-        while reverse_lookup_result == [''] and now - start < 15:
-            reverse_lookup_result = self.dig_reverse_resolve(
-                ip, version=version)
-            now = time.time()
-            time.sleep(0.05)
+        reverse_lookup_result = self.dig_reverse_resolve(
+            ip, version=version)
         self.assertThat(
             reverse_lookup_result, Contains("%s." % fqdn),
             "Failed to reverse resolve '%s' missing '%s' (results: '%s')." % (
@@ -701,16 +746,15 @@
             FileContains(matcher=Contains(trusted_network)))
 
     def test_dns_config_has_NS_record(self):
+        self.patch(settings, 'DNS_CONNECT', True)
         ip = factory.make_ipv4_address()
         with RegionConfiguration.open_for_update() as config:
             config.maas_url = 'http://%s/' % ip
         domain = factory.make_Domain()
         node, static = self.create_node_with_static_ip(domain=domain)
-        self.patch(settings, 'DNS_CONNECT', True)
-        dns_update_all_zones_now()
-        # Sleep half a second to make sure bind is fully-ready. This is not the
-        # best, but it does prevent this tests from failing randomly.
-        time.sleep(0.5)
+        # Creating the domain triggered writing the zone file and updating the
+        # DNS.
+        self.dns_wait_soa(domain.name)
         # Get the NS record for the zone 'domain.name'.
         ns_record = dig_call(
             port=self.bind.config.port,
@@ -718,6 +762,7 @@
         self.assertGreater(
             len(ns_record), 0, "No NS record for domain.name.")
         # Resolve that hostname.
+        self.dns_wait_soa(ns_record)
         ip_of_ns_record = dig_call(
             port=self.bind.config.port, commands=[ns_record, '+short'])
         self.assertEqual(ip, ip_of_ns_record)
@@ -736,7 +781,8 @@
         subnet.dns_servers = []
         subnet.save()
         # The IP from the old network does not resolve anymore.
-        self.assertEqual([''], self.dig_reverse_resolve(lease.ip))
+        self.assertEqual([''], self.dig_reverse_resolve(
+            lease.ip, removing=True))
         # A lease in the new network resolves.
         node, lease = self.create_node_with_static_ip(subnet=subnet)
         self.assertTrue(
@@ -752,7 +798,7 @@
         ip = factory.pick_ip_in_network(network)
         domain = factory.make_Domain()
         domain.delete()
-        self.assertEqual([''], self.dig_reverse_resolve(ip))
+        self.assertEqual([''], self.dig_reverse_resolve(ip, removing=True))
 
     def test_add_node_updates_zone(self):
         self.patch(settings, "DNS_CONNECT", True)
@@ -764,7 +810,7 @@
         node, static = self.create_node_with_static_ip()
         node.delete()
         fqdn = "%s.%s" % (node.hostname, node.domain.name)
-        self.assertEqual([''], self.dig_resolve(fqdn))
+        self.assertEqual([''], self.dig_resolve(fqdn, removing=True))
 
     def test_change_node_hostname_updates_zone(self):
         self.patch(settings, "DNS_CONNECT", True)
 
=== modified file 'src/maasserver/dns/tests/test_zonegenerator.py'
--- src/maasserver/dns/tests/test_zonegenerator.py	2016-02-18 17:31:45 +0000
+++ src/maasserver/dns/tests/test_zonegenerator.py	2016-03-08 17:17:00 +0000
@@ -284,10 +284,10 @@
     def test_empty_yields_nothing(self):
         self.assertEqual(
             [],
-            ZoneGenerator((), (), serial_generator=Mock()).as_list())
+            ZoneGenerator((), (), serial=random.randint(0, 65535)).as_list())
 
     def test_defaults_ttl(self):
-        zonegen = ZoneGenerator((), (), serial_generator=Mock())
+        zonegen = ZoneGenerator((), (), serial=random.randint(0, 65535))
         self.assertEqual(
             Config.objects.get_config('default_dns_ttl'),
             zonegen.default_ttl)
@@ -296,7 +296,7 @@
     def test_accepts_default_ttl(self):
         default_ttl = random.randint(10, 1000)
         zonegen = ZoneGenerator(
-            (), (), default_ttl=default_ttl, serial_generator=Mock())
+            (), (), default_ttl=default_ttl, serial=random.randint(0, 65535))
         self.assertEqual(default_ttl, zonegen.default_ttl)
 
     def test_yields_forward_and_reverse_zone(self):
@@ -304,7 +304,7 @@
         domain = factory.make_Domain(name='henry')
         subnet = factory.make_Subnet(cidr=str(IPNetwork("10/29").cidr))
         zones = ZoneGenerator(
-            domain, subnet, serial_generator=Mock()).as_list()
+            domain, subnet, serial=random.randint(0, 65535)).as_list()
         self.assertThat(
             zones, MatchesSetwise(
                 forward_zone("henry"),
@@ -318,7 +318,7 @@
         factory.make_Node_with_Interface_on_Subnet(
             subnet=subnet, vlan=subnet.vlan, fabric=subnet.vlan.fabric)
         zones = ZoneGenerator(
-            domain, subnet, serial_generator=Mock()).as_list()
+            domain, subnet, serial=random.randint(0, 65535)).as_list()
         self.assertThat(
             zones, MatchesSetwise(
                 forward_zone("henry"),
@@ -347,7 +347,7 @@
         Config.objects.set_config('default_dns_ttl', default_ttl)
         zones = ZoneGenerator(
             domain, subnet, default_ttl=default_ttl,
-            serial_generator=Mock()).as_list()
+            serial=random.randint(0, 65535)).as_list()
         self.assertThat(
             zones, MatchesSetwise(
                 forward_zone("henry"),
@@ -404,7 +404,7 @@
         Config.objects.set_config('default_dns_ttl', default_ttl)
         zones = ZoneGenerator(
             domain, [subnet1, subnet2], default_ttl=default_ttl,
-            serial_generator=Mock()).as_list()
+            serial=random.randint(0, 65535)).as_list()
         self.assertThat(
             zones, MatchesSetwise(
                 forward_zone(domain.name),
@@ -436,7 +436,8 @@
         ]
         self.assertThat(
             ZoneGenerator(
-                domain, [subnet1, subnet2], serial_generator=Mock()).as_list(),
+                domain, [subnet1, subnet2],
+                serial=random.randint(0, 65535)).as_list(),
             MatchesSetwise(*expected_zones))
 
     def test_with_many_yields_many_zones(self):
@@ -459,7 +460,7 @@
                 expected_zones.add(
                     reverse_zone(default_domain.name, rfc2317_net.cidr))
         actual_zones = ZoneGenerator(
-            domains, subnets, serial_generator=Mock()).as_list()
+            domains, subnets, serial=random.randint(0, 65535)).as_list()
         self.assertThat(actual_zones, MatchesSetwise(*expected_zones))
 
     def test_zone_generator_handles_rdns_mode_equal_enabled(self):
@@ -475,7 +476,8 @@
             reverse_zone(default_domain.name, "10/29"),
             )
         self.assertThat(
-            ZoneGenerator(domains, subnets, serial_generator=Mock()).as_list(),
+            ZoneGenerator(
+                domains, subnets, serial=random.randint(0, 65535)).as_list(),
             MatchesSetwise(*expected_zones))
 
 
@@ -502,7 +504,7 @@
                 node.system_id, domain.ttl, {boot_ip.ip}, node.node_type)}
         zones = ZoneGenerator(
             domain, subnet, default_ttl=global_ttl,
-            serial_generator=Mock()).as_list()
+            serial=random.randint(0, 65535)).as_list()
         self.assertItemsEqual(
             expected_forward.items(), zones[0]._mapping.items())
         self.assertItemsEqual(
@@ -532,7 +534,7 @@
                 node.node_type)}
         zones = ZoneGenerator(
             domain, subnet, default_ttl=global_ttl,
-            serial_generator=Mock()).as_list()
+            serial=random.randint(0, 65535)).as_list()
         self.assertItemsEqual(
             expected_forward.items(), zones[0]._mapping.items())
         self.assertItemsEqual(
@@ -568,7 +570,7 @@
                 node.node_type)}
         zones = ZoneGenerator(
             domain, subnet, default_ttl=global_ttl,
-            serial_generator=Mock()).as_list()
+            serial=random.randint(0, 65535)).as_list()
         self.assertItemsEqual(
             expected_forward.items(), zones[0]._mapping.items())
         self.assertItemsEqual(
@@ -608,7 +610,7 @@
                 node.node_type)}
         zones = ZoneGenerator(
             domain, subnet, default_ttl=global_ttl,
-            serial_generator=Mock()).as_list()
+            serial=random.randint(0, 65535)).as_list()
         self.assertItemsEqual(
             expected_forward.items(), zones[0]._mapping.items())
         self.assertItemsEqual(
@@ -629,7 +631,7 @@
             None, {(global_ttl, dnsdata.rrtype, dnsdata.rrdata)})}
         zones = ZoneGenerator(
             domain, subnet, default_ttl=global_ttl,
-            serial_generator=Mock()).as_list()
+            serial=random.randint(0, 65535)).as_list()
         self.assertItemsEqual(
             expected_forward.items(), zones[0]._other_mapping.items())
         self.assertItemsEqual([], zones[0]._mapping.items())
@@ -651,7 +653,7 @@
             None, {(domain.ttl, dnsdata.rrtype, dnsdata.rrdata)})}
         zones = ZoneGenerator(
             domain, subnet, default_ttl=global_ttl,
-            serial_generator=Mock()).as_list()
+            serial=random.randint(0, 65535)).as_list()
         self.assertItemsEqual(
             expected_forward.items(), zones[0]._other_mapping.items())
         self.assertItemsEqual([], zones[0]._mapping.items())
@@ -673,7 +675,7 @@
             None, {(dnsdata.ttl, dnsdata.rrtype, dnsdata.rrdata)})}
         zones = ZoneGenerator(
             domain, subnet, default_ttl=global_ttl,
-            serial_generator=Mock()).as_list()
+            serial=random.randint(0, 65535)).as_list()
         self.assertItemsEqual(
             expected_forward.items(), zones[0]._other_mapping.items())
         self.assertItemsEqual([], zones[0]._mapping.items())
 
=== modified file 'src/maasserver/dns/zonegenerator.py'
--- src/maasserver/dns/zonegenerator.py	2016-03-07 15:15:36 +0000
+++ src/maasserver/dns/zonegenerator.py	2016-03-08 17:17:00 +0000
@@ -152,12 +152,9 @@
     We generate zones for the domains (forward), and subnets (reverse) passed.
     """
 
-    def __init__(self, domains, subnets, default_ttl=None,
-                 serial=None, serial_generator=None):
+    def __init__(self, domains, subnets, default_ttl=None, serial=None):
         """
         :param serial: A serial number to reuse when creating zones in bulk.
-        :param serial_generator: As an alternative to `serial`, a callback
-            that returns a fresh serial number on every call.
         """
         self.domains = sequence(domains)
         self.subnets = sequence(subnets)
@@ -166,7 +163,6 @@
         else:
             self.default_ttl = default_ttl
         self.serial = serial
-        self.serial_generator = serial_generator
 
     @staticmethod
     def _get_mappings():
@@ -356,12 +352,11 @@
         """
         # For testing and such it's fine if we don't have a serial, but once
         # we get to this point, we really need one.
-        assert not (self.serial is None and self.serial_generator is None), (
-            "No serial number or serial number generator specified.")
+        assert not (self.serial is None), ("No serial number specified.")
 
         mappings = self._get_mappings()
         rrset_mappings = self._get_rrset_mappings()
-        serial = self.serial or self.serial_generator()
+        serial = self.serial
         default_ttl = self.default_ttl
         return chain(
             self._gen_forward_zones(
 
=== modified file 'src/maasserver/sequence.py'
--- src/maasserver/sequence.py	2016-01-25 11:43:50 +0000
+++ src/maasserver/sequence.py	2016-03-08 17:17:00 +0000
@@ -20,6 +20,7 @@
 from provisioningserver.utils import typed
 from psycopg2.errorcodes import (
     DUPLICATE_TABLE,
+    OBJECT_NOT_IN_PREREQUISITE_STATE,
     UNDEFINED_TABLE,
 )
 
@@ -153,6 +154,29 @@
             else:
                 raise
 
+    def current(self):
+        """Return the current value of this sequence, or `None`.
+
+        :return: The sequence value, or None if there is no current value for
+            the sequence in the database session or if the sequence does not
+            exist.
+        :rtype: int / None
+        """
+        try:
+            with transaction.atomic():
+                with connection.cursor() as cursor:
+                    cursor.execute("SELECT currval(%s)", [self.name])
+                    return cursor.fetchone()[0]
+        except (utils.OperationalError, utils.ProgrammingError) as error:
+            if is_postgres_error(error, OBJECT_NOT_IN_PREREQUISITE_STATE):
+                # There is no current value for the sequence.
+                return None
+            elif is_postgres_error(error, UNDEFINED_TABLE):
+                # The sequence does not exist, hence has no current value.
+                return None
+            else:
+                raise
+
 
 def is_postgres_error(error, *pgcodes):
     # Unwrap Django's lowest-common-denominator exception.
 
=== modified file 'src/maasserver/tests/test_sequence.py'
--- src/maasserver/tests/test_sequence.py	2016-01-25 11:43:50 +0000
+++ src/maasserver/tests/test_sequence.py	2016-03-08 17:17:00 +0000
@@ -43,7 +43,7 @@
         seq.create()
         self.assertRaisesRegex(ProgrammingError, "already exists", seq.create)
 
-    def test_create_if_not_exists_does_not_fails_if_sequence_exists(self):
+    def test_create_if_not_exists_does_not_fail_if_sequence_exists(self):
         name = factory.make_name('seq', sep='')
         seq = Sequence(name)
         seq.create()
@@ -151,3 +151,33 @@
         seq.create()
         self.assertSequenceEqual(
             list(range(1, 11)), list(islice(seq, 10)))
+
+    def test_current_returns_none_when_table_does_not_exist(self):
+        name = factory.make_name('seq', sep='')
+        seq = Sequence(name)
+        self.assertEqual(None, seq.current())
+
+    def test_current_returns_none_when_no_current_value(self):
+        name = factory.make_name('seq', sep='')
+        seq = Sequence(name)
+        seq.create()
+        self.assertEqual(None, seq.current())
+
+    def test_current_returns_current_value(self):
+        name = factory.make_name('seq', sep='')
+        seq = Sequence(name)
+        seq.create()
+        expected = next(seq)
+        self.assertEqual(expected, seq.current())
+        self.assertEqual(expected, seq.current())
+
+    def test_current_returns_correct_value(self):
+        name = factory.make_name('seq', sep='')
+        seq = Sequence(name)
+        seq.create()
+        expected = next(seq)
+        self.assertEqual(expected, seq.current())
+        self.assertEqual(expected, seq.current())
+        expected = next(seq)
+        self.assertEqual(expected, seq.current())
+        self.assertEqual(expected, seq.current())
 
=== modified file 'src/provisioningserver/testing/bindfixture.py'
--- src/provisioningserver/testing/bindfixture.py	2015-12-01 18:12:59 +0000
+++ src/provisioningserver/testing/bindfixture.py	2016-03-08 17:17:00 +0000
@@ -119,8 +119,9 @@
       logging{
         channel simple_log {
           file "{{log_file}}";
-          severity info;
+          severity debug;
           print-severity yes;
+          print-time yes;
         };
         category default{
           simple_log;
 
=== modified file 'src/provisioningserver/utils/fs.py'
--- src/provisioningserver/utils/fs.py	2016-03-08 11:44:02 +0000
+++ src/provisioningserver/utils/fs.py	2016-03-08 17:17:00 +0000
@@ -42,10 +42,7 @@
 )
 import tempfile
 import threading
-from time import (
-    sleep,
-    time,
-)
+from time import sleep
 
 from provisioningserver.utils import sudo
 from provisioningserver.utils.shell import ExternalProcessError
@@ -201,61 +198,27 @@
         raise
 
 
-def pick_new_mtime(old_mtime=None):
-    """Choose a new modification time for a file that needs it updated.
-
-    This function is used to manage the modification time of files
-    for which we need to see an increment in the modification time
-    each time the file is modified.  This is the case for DNS zone
-    files which only get properly reloaded if BIND sees that the
-    modification time is > to the time it has in its database.
-
-    Modification time can have a resolution as low as one second in
-    some relevant environments (we have observed this with ext3).
-    To produce mtime changes regardless, we set a file's modification
-    time in the past when it is first written, and
-    increment it by 1 second on each subsequent write.
-
-    :param old_mtime: File's previous modification time, as a number
-        with a unity of one second, or None if it did not previously
-        exist.
-    """
-    now = time()
-    if old_mtime is None or old_mtime + 1 < now:
-        # File is new.  Set modification time in the past to have room for
-        # sub-second modifications.
-        return now
-    else:
-        # Let the file timestamp march into the future, so that BIND will
-        # reload the zone.  Our future selves will notice that mtime is enough
-        # before the then-now to go back to having it match us.
-        return old_mtime + 1
-
-
 def incremental_write(content, filename, mode=0o600):
-    """Write the given `content` into the file `filename` and
-    increment the modification time by 1 sec.
+    """Write the given `content` into the file `filename`.  In the past, this
+    would potentially change modification time to arbitrary values.
 
     :type content: `bytes`
     :param mode: Access permissions for the file.
     """
-    old_mtime = get_mtime(filename)
+    # We used to change modification time on the files, in an attempt to out
+    # smart BIND into loading zones on file systems where time was not granular
+    # enough.  BIND got smarter about how it loads zones and remembers when it
+    # last loaded the zone: either the then-current time, or the mtime of the
+    # file, whichever is later.  When BIND then gets a reload request, it
+    # compares the current time to the loadtime for the domain, and skips it if
+    # the file is not new.  If we set mtime in the past, then zones don't load.
+    # If we set it in the future, then we WILL sometimes hit the race condition
+    # where BIND looks at the time after we atomic_write, but before we manage
+    # to set the time into the future.  N.B.: /etc on filesystems with 1-second
+    # granularity are no longer supported by MAAS.  The good news is that since
+    # 2.6, linux has supported nanosecond-granular time.  As of bind9
+    # 1:9.10.3.dfsg.P2-5, BIND even uses it.
     atomic_write(content, filename, mode=mode)
-    new_mtime = pick_new_mtime(old_mtime)
-    os.utime(filename, (new_mtime, new_mtime))
-
-
-def get_mtime(filename):
-    """Return a file's modification time, or None if it does not exist."""
-    try:
-        return os.stat(filename).st_mtime
-    except OSError as e:
-        if e.errno == errno.ENOENT:
-            # File does not exist.  Be helpful, return None.
-            return None
-        else:
-            # Other failure.  The caller will want to know.
-            raise
 
 
 def sudo_write_file(filename, contents, mode=0o644):
 
=== modified file 'src/provisioningserver/utils/tests/test_fs.py'
--- src/provisioningserver/utils/tests/test_fs.py	2016-03-07 14:35:29 +0000
+++ src/provisioningserver/utils/tests/test_fs.py	2016-03-08 17:17:00 +0000
@@ -8,7 +8,6 @@
 from base64 import urlsafe_b64encode
 import os
 import os.path
-from random import randint
 import re
 from shutil import rmtree
 import stat
@@ -21,7 +20,6 @@
 
 from maastesting import root
 from maastesting.factory import factory
-from maastesting.fakemethod import FakeMethod
 from maastesting.matchers import (
     FileContains,
     MockCalledOnceWith,
@@ -43,9 +41,7 @@
     ensure_dir,
     FileLock,
     get_maas_provision_command,
-    get_mtime,
     incremental_write,
-    pick_new_mtime,
     read_text_file,
     RunLock,
     sudo_delete_file,
@@ -250,7 +246,7 @@
 class TestIncrementalWrite(MAASTestCase):
     """Test `incremental_write`."""
 
-    def test_incremental_write_increments_modification_time(self):
+    def test_incremental_write_updates_modification_time(self):
         content = factory.make_bytes()
         filename = self.make_file(contents=factory.make_string())
         # Pretend that this file is older than it is.  So that
@@ -259,8 +255,21 @@
         os.utime(filename, (old_mtime, old_mtime))
         incremental_write(content, filename)
         new_time = time.time()
-        self.assertAlmostEqual(
-            os.stat(filename).st_mtime, new_time, delta=0.01)
+        # should be much closer to new_time than to old_mtime.
+        self.assertAlmostEqual(
+            os.stat(filename).st_mtime, new_time, delta=2.0)
+
+    def test_incremental_write_does_not_set_future_time(self):
+        content = factory.make_bytes()
+        filename = self.make_file(contents=factory.make_string())
+        # Pretend that this file is older than it is.  So that
+        # incrementing its mtime won't put it in the future.
+        old_mtime = os.stat(filename).st_mtime + 10
+        os.utime(filename, (old_mtime, old_mtime))
+        incremental_write(content, filename)
+        new_time = time.time()
+        self.assertAlmostEqual(
+            os.stat(filename).st_mtime, new_time, delta=2.0)
 
     def test_incremental_write_sets_permissions(self):
         atomic_file = self.make_file()
@@ -269,45 +278,6 @@
         self.assertEqual(mode, stat.S_IMODE(os.stat(atomic_file).st_mode))
 
 
-class TestGetMTime(MAASTestCase):
-    """Test `get_mtime`."""
-
-    def test_get_mtime_returns_None_for_nonexistent_file(self):
-        nonexistent_file = os.path.join(
-            self.make_dir(), factory.make_name('nonexistent-file'))
-        self.assertIsNone(get_mtime(nonexistent_file))
-
-    def test_get_mtime_returns_mtime(self):
-        existing_file = self.make_file()
-        mtime = os.stat(existing_file).st_mtime - randint(0, 100)
-        os.utime(existing_file, (mtime, mtime))
-        # Some small rounding/representation errors can happen here.
-        # That's just the way of floating-point numbers.  According to
-        # Gavin there's a conversion to fixed-point along the way, which
-        # would raise representability issues.
-        self.assertAlmostEqual(mtime, get_mtime(existing_file), delta=0.00001)
-
-    def test_get_mtime_passes_on_other_error(self):
-        forbidden_file = self.make_file()
-        self.patch(os, 'stat', FakeMethod(failure=OSError("Forbidden file")))
-        self.assertRaises(OSError, get_mtime, forbidden_file)
-
-
-class TestPickNewMTime(MAASTestCase):
-    """Test `pick_new_mtime`."""
-
-    def test_pick_new_mtime_increments_mtime_if_possible(self):
-        now = time.time()
-        new_time = pick_new_mtime(now - 2)
-        self.assertAlmostEqual(new_time, now, delta=0.0001)
-
-    def test_pick_new_mtime_happily_moves_mtime_into_the_future(self):
-        # Race condition: this will fail if the test gets held up for
-        # a second between readings of the clock.
-        now = time.time()
-        self.assertEqual(now + 1, pick_new_mtime(now))
-
-
 class TestGetMAASProvisionCommand(MAASTestCase):
 
     def test__returns_just_command_for_production(self):
 
=== modified file 'utilities/check-imports'
--- utilities/check-imports	2016-02-29 14:42:45 +0000
+++ utilities/check-imports	2016-03-08 17:17:00 +0000
@@ -148,6 +148,7 @@
     map("{0}|{0}.**".format, python_standard_libs))
 TestingLibraries = Pattern(
     "django_nose|django_nose.**",
+    "dns|dns.**",
     "fixtures|fixtures.**",
     "hypothesis|hypothesis.**",
     "maastesting|maastesting.**",