launchpad-reviewers team mailing list archive

[Raphaël Badin <raphael.badin@xxxxxxxxxxxxx>]

Raphaël Badin has proposed merging lp:~rvb/maas/maas-dns-utils into lp:maas with lp:~rvb/maas/maas-dns2 as a prerequisite.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~rvb/maas/maas-dns-utils/+merge/114837

This branch adds a hack to atomic_write.  A small and optional hack to be precise.  I've tried to explain this as much as possible in the code itself but basically this is needed when writing DNS zone configuration files.  BIND tries to be clever and will not reload a file if it's been written twice in the same second (because the file's modification time will be the same).  This obviously makes some of the tests I have in a follow-up branch fail because everything happens inside a second.  Since this might also happen in production (when, for instance, a rack full of servers boots up and a new DNS zone file is modified many times inside a second) so I decided it should be fixed properly.  Also, the test inside the BIND codebase is old_mtime < new_mtime (and not old_mtime != new_mtime) so changing the mtime to a date in the past is not an option.  Finally, BIND doesn't deal well with modification times in the future.

= Pre-imp =

This was discussed with Gavin.
-- 
https://code.launchpad.net/~rvb/maas/maas-dns-utils/+merge/114837
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~rvb/maas/maas-dns-utils into lp:maas.

=== modified file 'src/provisioningserver/tests/test_utils.py'
--- src/provisioningserver/tests/test_utils.py	2012-07-05 15:57:59 +0000
+++ src/provisioningserver/tests/test_utils.py	2012-07-13 11:21:20 +0000
@@ -17,9 +17,12 @@
     Namespace,
     )
 from io import BytesIO
+import os
+import random
 from random import randint
 from subprocess import CalledProcessError
 import sys
+import time
 import types
 
 from maastesting.factory import factory
@@ -27,6 +30,7 @@
 from provisioningserver.utils import (
     ActionScript,
     atomic_write,
+    increment_age,
     Safe,
     ShellTemplate,
     )
@@ -56,6 +60,45 @@
         atomic_write(content, filename)
         self.assertThat(filename, FileContains(content))
 
+    def test_atomic_write_increments_modification_time(self):
+        content = factory.getRandomString()
+        filename = self.make_file(contents=factory.getRandomString())
+        # 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))
+        atomic_write(content, filename, incremental_age=True)
+        self.assertEqual(
+            os.stat(filename).st_mtime, old_mtime + 1)
+
+
+class TestIncrementAge(TestCase):
+    """Test `increment_age`."""
+
+    def test_increment_age_sets_mtime_in_the_past(self):
+        filename = self.make_file()
+        delta = random.randint(10, 200)
+        increment_age(filename, old_mtime=None, delta=delta)
+        now = time.mktime(time.localtime())
+        self.assertEqual(
+            os.stat(filename).st_mtime, now - delta)
+
+    def test_increment_age_increments_mtime(self):
+        filename = self.make_file()
+        now = time.mktime(time.localtime())
+        old_mtime = now - 200
+        increment_age(filename, old_mtime=old_mtime)
+        self.assertEqual(
+            os.stat(filename).st_mtime, old_mtime + 1)
+
+    def test_increment_age_does_not_increment_mtime_if_in_future(self):
+        filename = self.make_file()
+        now = time.mktime(time.localtime())
+        old_mtime = now + 200
+        increment_age(filename, old_mtime=old_mtime)
+        self.assertEqual(
+            os.stat(filename).st_mtime, old_mtime)
+
 
 class TestShellTemplate(TestCase):
     """Test `ShellTemplate`."""

=== modified file 'src/provisioningserver/utils.py'
--- src/provisioningserver/utils.py	2012-07-05 17:59:09 +0000
+++ src/provisioningserver/utils.py	2012-07-13 11:21:20 +0000
@@ -27,6 +27,7 @@
 from subprocess import CalledProcessError
 import sys
 import tempfile
+import time
 
 import tempita
 from twisted.internet.defer import maybeDeferred
@@ -67,7 +68,7 @@
     return decorate
 
 
-def atomic_write(content, filename):
+def atomic_write(content, filename, incremental_age=False):
     """Write the given `content` into the file `filename` in an atomic
     fashion.
     """
@@ -81,7 +82,56 @@
         f.write(content)
     # Rename the temporary file to `filename`, that operation is atomic on
     # POSIX systems.
+
+    old_mtime = None
+    if incremental_age and os.path.exists(filename):
+        old_mtime = os.stat(filename).st_mtime
     os.rename(temp_file, filename)
+    if incremental_age:
+        increment_age(filename, old_mtime=old_mtime)
+
+
+# Default delta (in seconds) used to set the modification time for
+# files in the past (to be able to increment the modification
+# time each time the file is written again).
+DELTA = 1000
+
+
+def increment_age(filename, old_mtime=None, delta=DELTA):
+    """Increment the modification time by 1 sec compared to the given
+    `old_mtime`.
+
+    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.
+
+    Since the resolution of the modification time is one second,
+    we want to manually set the modification time in the past
+    the first time the file is written and increment the mod
+    time by 1 manually each time the file gets written again.
+
+    We also want to be careful not to set the modification time in
+    the future (mostly because BIND doesn't deal with that well).
+
+    Finally, note that the access time is set to the same value as
+    the modification time.
+    """
+    now = time.mktime(time.localtime())
+    if old_mtime is None:
+        # Set modification time in the past to have room for
+        # sub-second modifications.
+        new_mtime = now - delta
+    else:
+        # If the modification time can be incremented by 1 sec
+        # without being in the future, do it.  Otherwise we give
+        # up and set it to 'now'.
+        if old_mtime + 1 <= now:
+            new_mtime = old_mtime + 1
+        else:
+            new_mtime = old_mtime
+    os.utime(filename, (new_mtime, new_mtime))
 
 
 class Safe: