launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

Jeroen T. Vermeulen has proposed merging lp:~jtv/maas/doris-does-mtime into lp:maas.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~jtv/maas/doris-does-mtime/+merge/119683

Management of modification times for DNS zone files deals with some devil-in-the-details problems with filesystem timestamp resolution and BIND behaviours.  Because the problem came as a surprise while the code was already working on ext4 systems, the solution underwent a lot of changes and experimentation.  As a result, the solution ended up being a bit murky and not all tests, identifiers etc. were quite up to date with the latest disturbing discoveries.

Having come across this while doing other work, and given that the code is now stable and good for ext3, I took a bit of time to refactor it based on current insights.  Division of labour is a bit different, complexity is encapsulated a bit more, the docstring (I hope) is all written from the current perspective, and test setup is simpler.  We do get a few more tests.


Jeroen
-- 
https://code.launchpad.net/~jtv/maas/doris-does-mtime/+merge/119683
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/maas/doris-does-mtime into lp:maas.

=== modified file 'src/provisioningserver/tests/test_utils.py'
--- src/provisioningserver/tests/test_utils.py	2012-08-14 10:02:04 +0000
+++ src/provisioningserver/tests/test_utils.py	2012-08-15 08:05:29 +0000
@@ -17,22 +17,24 @@
     Namespace,
     )
 import os
-import random
 from random import randint
 import StringIO
 from subprocess import CalledProcessError
 import sys
+import time
 import types
 
 from maastesting.factory import factory
+from maastesting.fakemethod import FakeMethod
 from maastesting.testcase import TestCase
 from provisioningserver.utils import (
     ActionScript,
     atomic_write,
-    increment_age,
+    get_mtime,
     incremental_write,
     MainScript,
     parse_key_value_file,
+    pick_new_mtime,
     Safe,
     ShellTemplate,
     )
@@ -103,32 +105,51 @@
             os.stat(filename).st_mtime, old_mtime + 1, delta=0.01)
 
 
-class TestIncrementAge(TestCase):
-    """Test `increment_age`."""
-
-    def setUp(self):
-        super(TestIncrementAge, self).setUp()
-        self.filename = self.make_file()
-        self.now = os.stat(self.filename).st_mtime
-
-    def test_increment_age_sets_mtime_in_the_past(self):
-        delta = random.randint(100, 200)
-        increment_age(self.filename, old_mtime=None, delta=delta)
-        self.assertAlmostEqual(
-            os.stat(self.filename).st_mtime,
-            self.now - delta, delta=2)
-
-    def test_increment_age_increments_mtime(self):
-        old_mtime = self.now - 200
-        increment_age(self.filename, old_mtime=old_mtime)
-        self.assertAlmostEqual(
-            os.stat(self.filename).st_mtime, old_mtime + 1, delta=0.01)
-
-    def test_increment_age_does_not_increment_mtime_if_in_future(self):
-        old_mtime = self.now + 200
-        increment_age(self.filename, old_mtime=old_mtime)
-        self.assertAlmostEqual(
-            os.stat(self.filename).st_mtime, old_mtime, delta=0.01)
+class TestGetMTime(TestCase):
+    """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 very very small errors seem to be possible.  That's just
+        # the way of floating-point numbers, although it'd be nice to
+        # know exactly where these came from.
+        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(TestCase):
+    """Test `pick_new_mtime`."""
+
+    def test_pick_new_mtime_applies_starting_age_to_new_file(self):
+        before = time.time()
+        starting_age = randint(0, 5)
+        recommended_age = pick_new_mtime(None, starting_age=starting_age)
+        now = time.time()
+        self.assertAlmostEqual(
+            now - starting_age,
+            recommended_age,
+            delta=(now - before))
+
+    def test_pick_new_mtime_increments_mtime_if_possible(self):
+        past = time.time() - 2
+        self.assertEqual(past + 1, pick_new_mtime(past))
+
+    def test_pick_new_mtime_refuses_to_move_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, pick_new_mtime(now))
 
 
 class ParseConfigTest(TestCase):

=== modified file 'src/provisioningserver/utils.py'
--- src/provisioningserver/utils.py	2012-08-14 09:55:09 +0000
+++ src/provisioningserver/utils.py	2012-08-15 08:05:29 +0000
@@ -22,6 +22,7 @@
     ]
 
 from argparse import ArgumentParser
+import errno
 from functools import wraps
 import os
 from os import fdopen
@@ -112,16 +113,27 @@
     """Write the given `content` into the file `filename` and
     increment the modification time by 1 sec.
     """
-    old_mtime = None
-    if os.path.exists(filename):
-        old_mtime = os.stat(filename).st_mtime
+    old_mtime = get_mtime(filename)
     atomic_write(content, filename)
-    increment_age(filename, old_mtime=old_mtime)
-
-
-def increment_age(filename, old_mtime=None, delta=1000):
-    """Increment the modification time by 1 sec compared to the given
-    `old_mtime`.
+    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 pick_new_mtime(old_mtime=None, starting_age=1000):
+    """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
@@ -129,31 +141,35 @@
     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.
+    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.
+
+    However we also want to be careful not to set the modification time
+    in the future, mostly because BIND does not deal with that very
+    well.
+
+    :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.
+    :param starting_age: If the file did not exist previously, set its
+        modification time this many seconds in the past.
     """
     now = time()
     if old_mtime is None:
-        # Set modification time in the past to have room for
+        # File is new.  Set modification time in the past to have room for
         # sub-second modifications.
-        new_mtime = now - delta
+        return now - starting_age
+    elif old_mtime + 1 <= now:
+        # There is room to increment the file's mtime by one second
+        # without ending up in the future.
+        return old_mtime + 1
     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))
+        # We can't increase the file's modification time.  Give up and
+        # return the previous modification time.
+        return old_mtime
 
 
 def split_lines(input, separator):