launchpad-reviewers team mailing list archive

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

Raphaël Badin has proposed merging lp:~rvb/maas/bug-1035998 into lp:maas.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1035998 in MAAS: "named.conf.maas is emptied on startup"
  https://bugs.launchpad.net/maas/+bug/1035998

For more details, see:
https://code.launchpad.net/~rvb/maas/bug-1035998/+merge/119319

This branch fixes bug 1035998: the DNS dev service writes an empty DNS configuration file (which is needed if the DNS service is run on it's own) and the webapp service fires tasks to write a full DNS config file (with all the zones declarations).  Unfortunately, these two events are completely asynchronous and thus, although the DNS service is started before the webapp service, sometimes, the writing of the empty DNS config happens *after* the full DNS config has been written.  To fix that, I've added an option to the set_up_dns command to avoid overwriting the DNS config if it already exists.

= Notes =

I decided to change the atomic_write method to make the race window (see my comment in atomic_write) as narrow as possible.  If we come up with a way to do this without a race condition (for instance by locking the file [os.O_EXCL]), we will simply need to change that utility method.
-- 
https://code.launchpad.net/~rvb/maas/bug-1035998/+merge/119319
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~rvb/maas/bug-1035998 into lp:maas.

=== modified file 'services/dns/run'
--- services/dns/run	2012-07-17 12:15:56 +0000
+++ services/dns/run	2012-08-13 11:29:21 +0000
@@ -20,8 +20,9 @@
     --create-config-only --overwrite-config \
     --homedir $homedir \
     --port $port --rndc-port $rndc_port
-# Create MAAS' DNS config.
-./bin/maas set_up_dns
+# Create MAAS' DNS config: do not overwrite an existing config file
+# as the writing of the full configuration might have happened already.
+./bin/maas set_up_dns --no-clobber
 # Edit the fixture's named.conf to include MAAS' DNS config.
 ./bin/maas get_named_conf --edit --config_path \
     `pwd`/run/named/named.conf

=== modified file 'src/maasserver/management/commands/set_up_dns.py'
--- src/maasserver/management/commands/set_up_dns.py	2012-07-16 13:22:56 +0000
+++ src/maasserver/management/commands/set_up_dns.py	2012-08-13 11:29:21 +0000
@@ -21,6 +21,7 @@
     'Command',
     ]
 
+from optparse import make_option
 
 from django.core.management.base import BaseCommand
 from provisioningserver.dns.config import (
@@ -30,12 +31,22 @@
 
 
 class Command(BaseCommand):
+    option_list = BaseCommand.option_list + (
+        make_option(
+            '--no-clobber', dest='no_clobber', action='store_true',
+            default=False,
+            help=(
+                "Don't overwrite the configuration file if it already "
+                "exists.")),
+            )
     help = (
         "Set up MAAS DNS configuration: a blank configuration and "
         "all the RNDC configuration options allowing MAAS to reload "
         "BIND once zones configuration files will be written.")
 
     def handle(self, *args, **options):
+        no_clobber = options.get('no_clobber')
         setup_rndc()
         config = DNSConfig()
-        config.write_config(zone_names=(), reverse_zone_names=())
+        config.write_config(
+            overwrite=not no_clobber, zone_names=(), reverse_zone_names=())

=== modified file 'src/maasserver/tests/test_commands_set_up_dns.py'
--- src/maasserver/tests/test_commands_set_up_dns.py	2012-07-16 13:07:14 +0000
+++ src/maasserver/tests/test_commands_set_up_dns.py	2012-08-13 11:29:21 +0000
@@ -18,12 +18,14 @@
 from celery.conf import conf
 from django.core.management import call_command
 from maasserver.testing.testcase import TestCase
+from maastesting.factory import factory
 from provisioningserver.dns.config import (
     MAAS_NAMED_CONF_NAME,
     MAAS_RNDC_CONF_NAME,
     )
 from testtools.matchers import (
     AllMatch,
+    FileContains,
     FileExists,
     )
 
@@ -37,3 +39,15 @@
         named_config = os.path.join(dns_conf_dir, MAAS_NAMED_CONF_NAME)
         rndc_conf_path = os.path.join(dns_conf_dir, MAAS_RNDC_CONF_NAME)
         self.assertThat([rndc_conf_path, named_config], AllMatch(FileExists()))
+
+    def test_set_up_dns_does_not_overwrite_config(self):
+        dns_conf_dir = self.make_dir()
+        self.patch(conf, 'DNS_CONFIG_DIR', dns_conf_dir)
+        random_content = factory.getRandomString()
+        factory.make_file(
+            location=dns_conf_dir, name=MAAS_NAMED_CONF_NAME,
+            contents=random_content)
+        call_command('set_up_dns', no_clobber=True)
+        self.assertThat(
+            os.path.join(dns_conf_dir, MAAS_NAMED_CONF_NAME),
+            FileContains(random_content))

=== modified file 'src/provisioningserver/dns/config.py'
--- src/provisioningserver/dns/config.py	2012-07-27 08:19:45 +0000
+++ src/provisioningserver/dns/config.py	2012-08-13 11:29:21 +0000
@@ -148,12 +148,12 @@
         parameters used when rendering the template."""
         return {}
 
-    def write_config(self, **kwargs):
+    def write_config(self, overwrite=True, **kwargs):
         """Write out this DNS config file."""
         template = self.get_template()
         kwargs.update(self.get_context())
         rendered = self.render_template(template, **kwargs)
-        atomic_write(rendered, self.target_path)
+        atomic_write(rendered, self.target_path, overwrite=overwrite)
 
 
 class DNSConfig(DNSConfigBase):
@@ -167,6 +167,7 @@
 
     def __init__(self, zones=()):
         self.zones = zones
+        return super(DNSConfig, self).__init__()
 
     @property
     def template_path(self):

=== modified file 'src/provisioningserver/dns/tests/test_config.py'
--- src/provisioningserver/dns/tests/test_config.py	2012-07-30 08:01:04 +0000
+++ src/provisioningserver/dns/tests/test_config.py	2012-08-13 11:29:21 +0000
@@ -117,6 +117,33 @@
             DNSConfigFail, dnsconfig.render_template, template)
         self.assertIn("'test' is not defined", exception.message)
 
+    def test_write_config_skips_writing_if_overwrite_false(self):
+        # If DNSConfig is created with overwrite=False, it won't
+        # overwrite an existing config file.
+        target_dir = self.make_dir()
+        self.patch(DNSConfig, 'target_dir', target_dir)
+        random_content = factory.getRandomString()
+        factory.make_file(
+            location=target_dir, name=MAAS_NAMED_CONF_NAME,
+            contents=random_content)
+        dnsconfig = DNSConfig()
+        dnsconfig.write_config(overwrite=False)
+        self.assertThat(
+            os.path.join(target_dir, MAAS_NAMED_CONF_NAME),
+            FileContains(random_content))
+
+    def test_write_config_writes_config_if_no_existing_file(self):
+        # If DNSConfig is created with overwrite=False, the config file
+        # will be written if no config file exists.
+        target_dir = self.make_dir()
+        self.patch(DNSConfig, 'target_dir', target_dir)
+        dnsconfig = DNSConfig()
+        dnsconfig.write_config(overwrite=False)
+        self.assertThat(
+            os.path.join(target_dir, MAAS_NAMED_CONF_NAME),
+            FileContains(
+                matcher=Contains("Zone declarations.")))
+
     def test_write_config_writes_config(self):
         target_dir = self.make_dir()
         self.patch(DNSConfig, 'target_dir', target_dir)

=== modified file 'src/provisioningserver/tests/test_utils.py'
--- src/provisioningserver/tests/test_utils.py	2012-08-07 10:20:38 +0000
+++ src/provisioningserver/tests/test_utils.py	2012-08-13 11:29:21 +0000
@@ -62,6 +62,25 @@
         atomic_write(content, filename)
         self.assertThat(filename, FileContains(content))
 
+    def test_atomic_write_does_not_overwrite_file_if_overwrite_false(self):
+        content = factory.getRandomString()
+        random_content = factory.getRandomString()
+        filename = self.make_file(contents=random_content)
+        atomic_write(content, filename, False)
+        self.assertThat(filename, FileContains(random_content))
+
+    def test_atomic_write_cleans_up_temp_file(self):
+        # If the writing of the file is skipped because overwrite is
+        # False and the file already exists, the temporary file which
+        # would have been used for the copy operation is removed.
+        content = factory.getRandomString()
+        random_content = factory.getRandomString()
+        filename = self.make_file(contents=random_content)
+        atomic_write(content, filename, False)
+        self.assertEqual(
+            [os.path.basename(filename)],
+            os.listdir(os.path.dirname(filename)))
+
 
 class TestIncrementalWrite(TestCase):
     """Test `incremental_write`."""

=== modified file 'src/provisioningserver/utils.py'
--- src/provisioningserver/utils.py	2012-08-07 10:20:38 +0000
+++ src/provisioningserver/utils.py	2012-08-13 11:29:21 +0000
@@ -72,9 +72,8 @@
     return decorate
 
 
-def atomic_write(content, filename):
-    """Write the given `content` into the file `filename` in an atomic
-    fashion.
+def atomic_write(content, filename, overwrite=True):
+    """Write `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).
@@ -84,9 +83,16 @@
         prefix=".%s." % os.path.basename(filename))
     with os.fdopen(temp_fd, "wb") as f:
         f.write(content)
-    # Rename the temporary file to `filename`, that operation is atomic on
-    # POSIX systems.
-    os.rename(temp_file, filename)
+    # Do not overwrite the file is overwrite=True and the file already
+    # exists.  There is a small race condition window here as the file
+    # can appear after its existence is checked and before the rename
+    # is performed.
+    if not overwrite and os.path.isfile(filename):
+        os.remove(temp_file)
+    else:
+        # Rename the temporary file to `filename`, that operation is atomic on
+        # POSIX systems.
+        os.rename(temp_file, filename)
 
 
 def incremental_write(content, filename):