launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #09512
[Merge] lp:~jtv/maas/dhcp-dir-structure into lp:maas
Jeroen T. Vermeulen has proposed merging lp:~jtv/maas/dhcp-dir-structure into lp:maas.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~jtv/maas/dhcp-dir-structure/+merge/113151
Due to time constraints I haven't been able to discuss this much with anyone. I hope it's straightforward enough that nothing here will be controversial, but there would have been other ways to do things.
To run you through the changes, point by point:
First I cleaned up some tests. The test case defined a setUp just to inject two fields into the test case, and one of those was only used inside setUp itself. Both the tests then used the remaining field for patching. The tests I was about to add to the test case did not need any of this, so I combined the setup and patching into a helper that is now called explicitly. And I extracted a helper for composing a parameters dict that will be enough to satisfy the template. I think the tests ended up prettier and punchier.
An easy shortcut to the functionality I needed would have been to hard-code the new PXE paths into the DHCP config template, but that would have created a place outside of tftppath that needs to understand and compose TFTP paths. Two anticipated changes (to wit bootloader versioning and special naming for empty bootloaders) would each have caused that to break. So I used our existing module for composing TFTP paths, and passed them to the DHCP template as parameters.
Here's a potentially controversial choice: the bootloader locations are more tightly coupled to information embedded in the template (i.e. exactly which architectures we have bootloaders for) than to anything the MAAS owner might change. So rather than pass the bootloader information in to get_config(), I generate it inside get_config(). On the downside, the config parameters now come from a mixture of sources. On the upside, the comment in get_config() already implies that this sort of information is meant to be incapsulated in there. The goal of the parameters is to support configuration for a network architecture, not to make the caller jump through hoops.
Then, I had to provide multiple bootloader paths for multiple architectures. The list will grow in the future. Rather than provide one bootloader parameter per architecture/subarchitecture, I passed them all as a single dict. It's not a dict of dicts, keyed first by architecture and then by subarchitecture. That would have fit the data, but it complicates the code and especially the tests. You lose a lot of simplicity and declarativeness in using comprehensions with nested dicts. I index the dict by a tuple instead: (architecture, subarchitecture).
Jeroen
--
https://code.launchpad.net/~jtv/maas/dhcp-dir-structure/+merge/113151
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/maas/dhcp-dir-structure into lp:maas.
=== modified file 'src/provisioningserver/dhcp/config.py'
--- src/provisioningserver/dhcp/config.py 2012-06-08 11:14:44 +0000
+++ src/provisioningserver/dhcp/config.py 2012-07-03 05:51:19 +0000
@@ -18,6 +18,7 @@
from textwrap import dedent
+from provisioningserver.pxe.tftppath import compose_bootloader_path
import tempita
@@ -25,6 +26,15 @@
"""Exception raised for errors processing the DHCP config."""
+# Architectures that we support netbooting for.
+# Actually we don't really do amd64; those systems get to use the i386
+# bootloader instead.
+bootloader_architectures = [
+ ('amd64', 'generic'),
+ ('arm', 'highbank'),
+ ('i386', 'generic'),
+ ]
+
template_content = dedent("""\
class "pxe" {
match if substring (option vendor-class-identifier, 0, 9) = "PXEClient";
@@ -44,11 +54,11 @@
pool {
allow members of "uboot-highbank";
- filename "/arm/highbank/empty";
+ filename "{{bootloaders['arm', 'highbank']}}";
}
pool {
allow members of "pxe";
- filename "/x86/pxelinux.0";
+ filename "{{bootloaders['i386', 'generic']}}";
}
}
""")
@@ -57,6 +67,20 @@
template_content, name="%s.template" % __name__)
+def compose_bootloaders():
+ """Compose "bootloaders" dict for substitution in the DHCP template.
+
+ The bootloaders dict maps tuples of (architecture, subarchitecture) to
+ their respective TFTP bootloader paths.
+
+ Thus, bootloaders['i386', 'generic'] yields the TFTP path for the i386
+ bootloader.
+ """
+ return {
+ (arch, subarch): compose_bootloader_path(arch, subarch)
+ for arch, subarch in bootloader_architectures}
+
+
def get_config(**params):
"""Return a DHCP config file based on the supplied parameters.
@@ -73,6 +97,8 @@
:param high_range: The last IP address in the range of IP addresses to
allocate
"""
+ params['bootloaders'] = compose_bootloaders()
+
# This is a really simple substitution for now but it's encapsulated
# here so that its implementation can be changed later if required.
try:
=== modified file 'src/provisioningserver/dhcp/tests/test_config.py'
--- src/provisioningserver/dhcp/tests/test_config.py 2012-06-08 12:24:20 +0000
+++ src/provisioningserver/dhcp/tests/test_config.py 2012-07-03 05:51:19 +0000
@@ -15,59 +15,66 @@
from textwrap import dedent
from provisioningserver.dhcp import config
+from provisioningserver.pxe.tftppath import compose_bootloader_path
import tempita
from testtools import TestCase
-from testtools.matchers import MatchesRegex
+from testtools.matchers import (
+ Contains,
+ MatchesAll,
+ MatchesRegex,
+ )
+
+# Simple test version of the DHCP template. Contains parameter
+# substitutions, but none that aren't also in the real template.
+sample_template = dedent("""\
+ {{subnet}}
+ {{subnet_mask}}
+ {{next_server}}
+ {{broadcast_address}}
+ {{dns_servers}}
+ {{gateway}}
+ {{low_range}}
+ {{high_range}}
+""")
+
+
+def make_sample_params():
+ """Produce a dict of sample parameters.
+
+ The sample provides all parameters used by the DHCP template.
+ """
+ return dict(
+ subnet="10.0.0.0",
+ subnet_mask="255.0.0.0",
+ next_server="10.0.0.1",
+ broadcast_address="10.255.255.255",
+ dns_servers="10.1.0.1 10.1.0.2",
+ gateway="10.0.0.2",
+ low_range="10.0.0.3",
+ high_range="10.0.0.254",
+ )
class TestDHCPConfig(TestCase):
- def setUp(self):
- super(TestDHCPConfig, self).setUp()
- self.template_content = dedent("""\
- {{subnet}}
- {{subnet_mask}}
- {{next_server}}
- {{broadcast_address}}
- {{dns_servers}}
- {{gateway}}
- {{low_range}}
- {{high_range}}
- """)
- self.template = tempita.Template(
- content=self.template_content,
- name="%s.template" % self.__class__.__name__)
+ def patch_template(self, template_content=sample_template):
+ """Patch the DHCP config template with the given contents."""
+ name = "%s.template" % self.__class__.__name__
+ template = tempita.Template(content=template_content, name=name)
+ self.patch(config, "template", template)
+ return template
def test_param_substitution(self):
- self.patch(config, "template", self.template)
-
- params = dict(
- subnet="10.0.0.0",
- subnet_mask="255.0.0.0",
- next_server="10.0.0.1",
- broadcast_address="10.255.255.255",
- dns_servers="10.1.0.1 10.1.0.2",
- gateway="10.0.0.2",
- low_range="10.0.0.3",
- high_range="10.0.0.254")
-
- output = config.get_config(**params)
-
- expected = self.template.substitute(params)
- self.assertEqual(expected, output)
+ template = self.patch_template()
+ params = make_sample_params()
+ self.assertEqual(
+ template.substitute(params),
+ config.get_config(**params))
def test_get_config_with_too_few_parameters(self):
- self.patch(config, "template", self.template)
-
- params = dict(
- # subnet is missing
- subnet_mask="255.0.0.0",
- next_server="10.0.0.1",
- broadcast_address="10.255.255.255",
- dns_servers="10.1.0.1 10.1.0.2",
- gateway="10.0.0.2",
- low_range="10.0.0.3",
- high_range="10.0.0.254")
+ template = self.patch_template()
+ params = make_sample_params()
+ del params['subnet']
e = self.assertRaises(
config.DHCPConfigError, config.get_config, **params)
@@ -75,4 +82,23 @@
self.assertThat(
e.message, MatchesRegex(
"name 'subnet' is not defined at line \d+ column \d+ "
- "in file %s" % self.template.name))
+ "in file %s" % template.name))
+
+ def test_config_refers_to_PXE_for_supported_architectures(self):
+ params = make_sample_params()
+ bootloaders = config.compose_bootloaders()
+ archs = [
+ ('i386', 'generic'),
+ ('arm', 'highbank'),
+ ]
+ paths = [bootloaders[arch] for arch in archs]
+ output = config.get_config(**params)
+ self.assertThat(
+ output,
+ MatchesAll(*[Contains(path) for path in paths]))
+
+ def test_compose_bootloaders_lists_tftp_paths(self):
+ sample_arch = ('i386', 'generic')
+ self.assertEqual(
+ compose_bootloader_path(*sample_arch),
+ config.compose_bootloaders()[sample_arch])
