Merge lp:~jtv/maas/delegate-write_config into lp:~maas-committers/maas/trunk

[Jeroen T. Vermeulen]

Untangle data flows between the DNSZoneConfig classes.

This is where my cleanups for the dns config module really come together. To understand what's going on, imagine the class hierarchy: there's an abstract DNSZoneConfigBase class, with two concrete child classes DNSForwardZoneConfig and DNSReverseZoneConfig. The most important thing they do is write a zone config file. This is implemented in the base class, but calls on various abstract methods and properties to get specific information from the concrete class. It is that convoluted, bidirectional data flow which makes it so hard to understand and change the module.

In this branch you'll see:
 * the "mapping" attribute move from the base class to just one child class. It was passed in other places too, but never actually used.
 * zone_name become a simple instance variable, explicitly computed by the derived-class constructor and passed to the base-class constructor.
 * template_path disappear, because render_dns_template already knows how to compute this path.
 * get_context be demoted from an abstract method with 2 implementations to a pair of merely similar helpers.
 * write_config() delegating explicitly to a base-class method, after gathering its data up-front.
 * zone composition on DNSReverseZoneConfig become a class method, ruling out hidden data flows.

There's a bit more polish coming, but not much: the implicit data flows through instance variables within the concrete classes can now be replaced with explicit parameters, and perhaps one or two instance variables can be retired altogether. (Actually it looks as if several are currently only used for inspection by tests.) But the module is now in a state where we can see what data is needed, and what for. We can then start answering questions like "how are we going to deal with hostnames that are in multiple networks?"

Jeroen

[Julian Edwards (julian-edwards)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

 review: approve
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.15 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iEYEARECAAYFAlLfaZIACgkQWhGlTF8G/HcYQgCglwEnMn+KP6JyY7IRuBjrpAyp
k18AnAo2zkY3w85NacqQDA/jjcQ7YajA
=BA2Q
-----END PGP SIGNATURE-----

review: Approve

=== modified file 'src/maasserver/dns.py'
--- src/maasserver/dns.py	2014-01-21 16:39:55 +0000
+++ src/maasserver/dns.py	2014-01-22 04:15:05 +0000
@@ -226,14 +226,14 @@
                 )
 
     @staticmethod
-    def _gen_reverse_zones(nodegroups, serial, mappings, networks):
+    def _gen_reverse_zones(nodegroups, serial, networks):
         """Generator of reverse zones, sorted by network."""
         get_domain = lambda nodegroup: nodegroup.name
         reverse_nodegroups = sorted(nodegroups, key=networks.get)
         for nodegroup in reverse_nodegroups:
             yield DNSReverseZoneConfig(
                 get_domain(nodegroup), serial=serial,
-                mapping=mappings[nodegroup], network=networks[nodegroup])
+                network=networks[nodegroup])
 
     def __iter__(self):
         forward_nodegroups = self._get_forward_nodegroups(self.nodegroups)
@@ -245,7 +245,7 @@
             self._gen_forward_zones(
                 forward_nodegroups, serial, mappings, networks),
             self._gen_reverse_zones(
-                reverse_nodegroups, serial, mappings, networks),
+                reverse_nodegroups, serial, networks),
             )
 
     def as_list(self):

=== modified file 'src/provisioningserver/dns/config.py'
--- src/provisioningserver/dns/config.py	2014-01-22 02:32:04 +0000
+++ src/provisioningserver/dns/config.py	2014-01-22 04:15:05 +0000
@@ -21,11 +21,7 @@
     ]
 
 
-from abc import (
-    ABCMeta,
-    abstractmethod,
-    abstractproperty,
-    )
+from abc import ABCMeta
 from contextlib import contextmanager
 from datetime import datetime
 import errno
@@ -252,14 +248,13 @@
         :raises DNSConfigDirectoryMissing: if the DNS configuration directory
             does not exist.
         """
-        template_path = locate_config(TEMPLATES_DIR, self.template_file_name)
         context = {
             'zones': self.zones,
             'DNS_CONFIG_DIR': conf.DNS_CONFIG_DIR,
             'named_rndc_conf_path': get_named_rndc_conf_path(),
             'modified': unicode(datetime.today()),
         }
-        content = render_dns_template(template_path, kwargs, context)
+        content = render_dns_template(self.template_file_name, kwargs, context)
         target_path = compose_config_path(self.target_file_name)
         with report_missing_config_dir():
             atomic_write(content, target_path, overwrite=overwrite, mode=0644)
@@ -279,68 +274,50 @@
 
     template_file_name = 'zone.template'
 
-    def __init__(self, domain, serial=None, mapping=None):
+    def __init__(self, domain, zone_name, serial=None):
         """
         :param domain: The domain name of the forward zone.
+        :param zone_name: Fully-qualified zone name.
         :param serial: The serial to use in the zone file. This must increment
             on each change.
-        :param mapping: A hostname:ip-address mapping for all known hosts in
-            the zone.
         """
         self.domain = domain
+        self.zone_name = zone_name
         self.serial = serial
-        self.mapping = {} if mapping is None else mapping
-
-    @abstractproperty
-    def zone_name(self):
-        """Return the zone's fully-qualified name."""
-
-    @property
-    def template_path(self):
-        return locate_config(TEMPLATES_DIR, self.template_file_name)
-
-    @property
-    def target_path(self):
-        """Return the full path of the DNS zone file."""
-        return compose_config_path('zone.%s' % self.zone_name)
-
-    @abstractmethod
-    def get_context(self):
-        """Dictionary containing parameters to be included in the
-        parameters used when rendering the template."""
-
-    def write_config(self):
-        """Write out this DNS zone file.
+        self.target_path = compose_config_path('zone.%s' % self.zone_name)
+
+    @classmethod
+    def write_zone_file(cls, output_file, parameters):
+        """Write a zone file based on the zone file template.
 
         There is a subtlety with zone files: their filesystem timestamp must
         increase with every rewrite.  Some filesystems (ext3?) only seem to
         support a resolution of one second, and so this method may set an
         unexpected modification time in order to maintain that property.
         """
-        content = render_dns_template(self.template_path, self.get_context())
+        content = render_dns_template(cls.template_file_name, parameters)
         with report_missing_config_dir():
-            incremental_write(content, self.target_path, mode=0644)
+            incremental_write(content, output_file, mode=0644)
 
 
 class DNSForwardZoneConfig(DNSZoneConfigBase):
     """Writes forward zone files."""
 
-    def __init__(self, *args, **kwargs):
+    def __init__(self, domain, **kwargs):
         """See `DNSZoneConfigBase.__init__`.
 
         :param networks: The networks that the mapping exists within.
         :type networks: Sequence of :class:`netaddr.IPNetwork`
         :param dns_ip: The IP address of the DNS server authoritative for this
             zone.
+        :param mapping: A hostname:ip-address mapping for all known hosts in
+            the zone.
         """
         self.networks = kwargs.pop('networks', [])
         self.dns_ip = kwargs.pop('dns_ip', None)
-        super(DNSForwardZoneConfig, self).__init__(*args, **kwargs)
-
-    @property
-    def zone_name(self):
-        """Return the name of the forward zone."""
-        return self.domain
+        self.mapping = kwargs.pop('mapping', {})
+        super(DNSForwardZoneConfig, self).__init__(
+            domain, zone_name=domain, **kwargs)
 
     def get_cname_mapping(self):
         """Return a generator with the mapping: hostname->generated hostname.
@@ -384,23 +361,29 @@
                 }
             }
 
+    def write_config(self):
+        """Write the zone file."""
+        self.write_zone_file(self.target_path, self.get_context())
+
 
 class DNSReverseZoneConfig(DNSZoneConfigBase):
     """Writes reverse zone files."""
 
-    def __init__(self, *args, **kwargs):
+    def __init__(self, domain, **kwargs):
         """See `DNSZoneConfigBase.__init__`.
 
         :param network: The network that the mapping exists within.
         :type network: :class:`netaddr.IPNetwork`
         """
         self.network = kwargs.pop("network", None)
-        super(DNSReverseZoneConfig, self).__init__(*args, **kwargs)
+        zone_name = self.compose_zone_name(self.network)
+        super(DNSReverseZoneConfig, self).__init__(
+            domain, zone_name=zone_name, **kwargs)
 
-    @property
-    def zone_name(self):
+    @classmethod
+    def compose_zone_name(cls, network):
         """Return the name of the reverse zone."""
-        broadcast, netmask = self.network.broadcast, self.network.netmask
+        broadcast, netmask = network.broadcast, network.netmask
         octets = broadcast.words[:netmask.words.count(255)]
         return '%s.in-addr.arpa' % '.'.join(imap(unicode, reversed(octets)))
 
@@ -445,3 +428,7 @@
                 'PTR': self.get_static_mapping(),
                 }
             }
+
+    def write_config(self):
+        """Write the zone file."""
+        self.write_zone_file(self.target_path, self.get_context())

=== modified file 'src/provisioningserver/dns/tests/test_config.py'
--- src/provisioningserver/dns/tests/test_config.py	2014-01-22 02:32:04 +0000
+++ src/provisioningserver/dns/tests/test_config.py	2014-01-22 04:15:05 +0000
@@ -52,11 +52,9 @@
     report_missing_config_dir,
     set_up_options_conf,
     setup_rndc,
-    TEMPLATES_DIR,
     uncomment_named_conf,
     )
 from provisioningserver.dns.utils import generated_hostname
-from provisioningserver.utils import locate_config
 from testtools.matchers import (
     Contains,
     ContainsAll,
@@ -339,9 +337,7 @@
         forward_zone = DNSForwardZoneConfig(
             domain, mapping={factory.getRandomString(): ip},
             networks=[network])
-        reverse_zone = DNSReverseZoneConfig(
-            domain, mapping={factory.getRandomString(): ip},
-            network=network)
+        reverse_zone = DNSReverseZoneConfig(domain, network=network)
         dnsconfig = DNSConfig((forward_zone, reverse_zone))
         dnsconfig.write_config()
         self.assertThat(
@@ -385,7 +381,7 @@
         ip = factory.getRandomIPInNetwork(network)
         mapping = {hostname: ip}
         dns_zone_config = DNSForwardZoneConfig(
-            domain, serial, mapping, networks=[network])
+            domain, serial=serial, mapping=mapping, networks=[network])
         self.assertThat(
             dns_zone_config,
             MatchesStructure.byEquality(
@@ -400,14 +396,8 @@
         domain = factory.make_name('zone')
         dns_zone_config = DNSForwardZoneConfig(domain)
         self.assertEqual(
-            (
-                locate_config(TEMPLATES_DIR, 'zone.template'),
-                os.path.join(conf.DNS_CONFIG_DIR, 'zone.%s' % domain),
-            ),
-            (
-                dns_zone_config.template_path,
-                dns_zone_config.target_path,
-            ))
+            os.path.join(conf.DNS_CONFIG_DIR, 'zone.%s' % domain),
+            dns_zone_config.target_path)
 
     def test_forward_zone_get_cname_mapping_returns_iterator(self):
         name = factory.getRandomString()
@@ -536,18 +526,14 @@
     def test_fields(self):
         domain = factory.getRandomString()
         serial = random.randint(1, 200)
-        hostname = factory.getRandomString()
         network = factory.getRandomNetwork()
-        ip = factory.getRandomIPInNetwork(network)
-        mapping = {hostname: ip}
         dns_zone_config = DNSReverseZoneConfig(
-            domain, serial, mapping, network=network)
+            domain, serial=serial, network=network)
         self.assertThat(
             dns_zone_config,
             MatchesStructure.byEquality(
                 domain=domain,
                 serial=serial,
-                mapping=mapping,
                 network=network,
                 )
             )
@@ -576,24 +562,15 @@
         dns_zone_config = DNSReverseZoneConfig(
             domain, network=IPNetwork("192.168.0.0/22"))
         self.assertEqual(
-            (
-                locate_config(TEMPLATES_DIR, 'zone.template'),
-                os.path.join(conf.DNS_CONFIG_DIR, reverse_file_name)
-            ),
-            (
-                dns_zone_config.template_path,
-                dns_zone_config.target_path,
-            ))
+            os.path.join(conf.DNS_CONFIG_DIR, reverse_file_name),
+            dns_zone_config.target_path)
 
     def test_reverse_data_slash_24(self):
         # DNSReverseZoneConfig calculates the reverse data correctly for
         # a /24 network.
         domain = factory.make_name('zone')
-        hostname = factory.getRandomString()
-        ip = '192.168.0.5'
         network = IPNetwork('192.168.0.1/24')
-        dns_zone_config = DNSReverseZoneConfig(
-            domain, mapping={hostname: ip}, network=network)
+        dns_zone_config = DNSReverseZoneConfig(domain, network=network)
         self.assertEqual(
             '0.168.192.in-addr.arpa',
             dns_zone_config.zone_name)
@@ -602,11 +579,8 @@
         # DNSReverseZoneConfig calculates the reverse data correctly for
         # a /22 network.
         domain = factory.getRandomString()
-        hostname = factory.getRandomString()
-        ip = '192.168.0.10'
         network = IPNetwork('192.168.0.1/22')
-        dns_zone_config = DNSReverseZoneConfig(
-            domain, mapping={hostname: ip}, network=network)
+        dns_zone_config = DNSReverseZoneConfig(domain, network=network)
         self.assertEqual(
             '168.192.in-addr.arpa',
             dns_zone_config.zone_name)

=== modified file 'src/provisioningserver/tests/test_tasks.py'
--- src/provisioningserver/tests/test_tasks.py	2014-01-21 08:12:06 +0000
+++ src/provisioningserver/tests/test_tasks.py	2014-01-22 04:15:05 +0000
@@ -347,8 +347,7 @@
             domain, serial=random.randint(1, 100),
             mapping={factory.getRandomString(): ip}, networks=[network])
         reverse_zone = DNSReverseZoneConfig(
-            domain, serial=random.randint(1, 100),
-            mapping={factory.getRandomString(): ip}, network=network)
+            domain, serial=random.randint(1, 100), network=network)
         result = write_dns_zone_config.delay(
             zones=[forward_zone, reverse_zone],
             callback=rndc_command.subtask(args=[command]))
@@ -461,9 +460,7 @@
                 mapping={factory.getRandomString(): ip},
                 networks=[network]),
             DNSReverseZoneConfig(
-                domain, serial=random.randint(1, 100),
-                mapping={factory.getRandomString(): ip},
-                network=network),
+                domain, serial=random.randint(1, 100), network=network),
             ]
         command = factory.getRandomString()
         result = write_full_dns_config.delay(