launchpad-reviewers team mailing list archive

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

Raphaël Badin has proposed merging lp:~rvb/maas/atomic-modif into lp:maas with lp:~rvb/maas/dns-config-tasks as a prerequisite.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~rvb/maas/atomic-modif/+merge/113601

This branch adds a tiny utility to write a file in an atomic fashion: writing to a temp file and then moving that temp file to the destination path (os.rename is atomic [1]).  This will be useful when writing configuration files as we don't want a server to reload half-written files.

I admit the testing is pretty minimal but it tests that the file gets written (and overrides an existing file) which is all we want from this method.

[1] http://docs.python.org/library/os.html#os.rename
"""If successful, the renaming will be an atomic operation (this is a POSIX requirement)."""

Note that: """On Windows, if dst already exists, OSError will be raised even if it is a file; there may be no way to implement an atomic rename when dst names an existing file.""" but I don't think this is something we should care about here.
-- 
https://code.launchpad.net/~rvb/maas/atomic-modif/+merge/113601
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~rvb/maas/atomic-modif into lp:maas.

=== modified file 'src/provisioningserver/dns/config.py'
--- src/provisioningserver/dns/config.py	2012-07-05 15:59:19 +0000
+++ src/provisioningserver/dns/config.py	2012-07-05 15:59:19 +0000
@@ -29,6 +29,7 @@
     )
 
 from celery.conf import conf
+from provisioningserver.utils import atomic_write
 import tempita
 
 
@@ -127,8 +128,7 @@
         template = self.get_template()
         kwargs.update(self.get_extra_context())
         rendered = self.render_template(template, **kwargs)
-        with open(self.target_path, "wb") as f:
-            f.write(rendered)
+        atomic_write(rendered, self.target_path)
 
 
 class DNSConfig(DNSConfigBase):

=== modified file 'src/provisioningserver/tests/test_utils.py'
--- src/provisioningserver/tests/test_utils.py	2012-06-15 14:41:42 +0000
+++ src/provisioningserver/tests/test_utils.py	2012-07-05 15:59:19 +0000
@@ -26,9 +26,11 @@
 from maastesting.testcase import TestCase
 from provisioningserver.utils import (
     ActionScript,
+    atomic_write,
     Safe,
     ShellTemplate,
     )
+from testtools.matchers import FileContains
 
 
 class TestSafe(TestCase):
@@ -45,6 +47,16 @@
         self.assertEqual("<Safe %r>" % string, repr(safe))
 
 
+class TestWriteAtomic(TestCase):
+    """Test `atomic_write`."""
+
+    def test_atomic_write_overwrites_dest_file(self):
+        content = factory.getRandomString()
+        filename = self.make_file(contents=factory.getRandomString())
+        atomic_write(content, filename)
+        self.assertThat(filename, FileContains(content))
+
+
 class TestShellTemplate(TestCase):
     """Test `ShellTemplate`."""
 

=== modified file 'src/provisioningserver/utils.py'
--- src/provisioningserver/utils.py	2012-06-15 14:41:42 +0000
+++ src/provisioningserver/utils.py	2012-07-05 15:59:19 +0000
@@ -14,16 +14,19 @@
     "ActionScript",
     "deferred",
     "ShellTemplate",
+    "atomic_write",
     "xmlrpc_export",
     ]
 
 from argparse import ArgumentParser
 from functools import wraps
+import os
 from os import fdopen
 from pipes import quote
 import signal
 from subprocess import CalledProcessError
 import sys
+import tempfile
 
 import tempita
 from twisted.internet.defer import maybeDeferred
@@ -64,6 +67,23 @@
     return decorate
 
 
+def atomic_write(content, filename):
+    """Write the given `content` into the file `filename` in an atomic
+    fashion.
+    """
+    # Write the file to a temporary place (next to the target destination,
+    # to ensure that it is on the same filesystem).
+    directory = os.path.dirname(filename)
+    _, temp_file = tempfile.mkstemp(dir=directory)
+    with open(temp_file, "wb") as f:
+        f.write(content)
+        f.flush()
+        os.fsync(f.fileno())
+    # Rename the temporary file to `filename`, that operation is atomic on
+    # POSIX systems.
+    os.rename(temp_file, filename)
+
+
 class Safe:
     """An object that is safe to render as-is."""