launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #10512
[Merge] lp:~allenap/maas/render-pxe-in-tftp-server into lp:maas
Gavin Panella has proposed merging lp:~allenap/maas/render-pxe-in-tftp-server into lp:maas.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~allenap/maas/render-pxe-in-tftp-server/+merge/117811
--
https://code.launchpad.net/~allenap/maas/render-pxe-in-tftp-server/+merge/117811
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~allenap/maas/render-pxe-in-tftp-server into lp:maas.
=== modified file 'src/maasserver/api.py'
--- src/maasserver/api.py 2012-07-31 15:54:16 +0000
+++ src/maasserver/api.py 2012-08-01 22:05:27 +0000
@@ -1132,8 +1132,9 @@
# XXX: allenap 2012-07-31 bug=1013146: 'precise' is hardcoded here.
release = "precise"
+ params = dict(
+ title=title, arch=arch, subarch=subarch, release=release,
+ purpose=get_boot_purpose(node), append=append)
+
return HttpResponse(
- render_pxe_config(
- title=title, arch=arch, subarch=subarch, release=release,
- purpose=get_boot_purpose(node), append=append),
- content_type="text/plain; charset=utf-8")
+ json.dumps(params), content_type="application/json")
=== modified file 'src/maasserver/tests/test_api.py'
--- src/maasserver/tests/test_api.py 2012-07-31 13:27:05 +0000
+++ src/maasserver/tests/test_api.py 2012-08-01 22:05:27 +0000
@@ -2268,9 +2268,9 @@
def get_optional_params(self):
return ['subarch', 'mac']
- def test_pxe_config_returns_config(self):
- response = self.client.get(reverse('pxeconfig'), self.get_params())
-
+ def test_pxeconfig_returns_config(self):
+ params_in = self.get_params()
+ response = self.client.get(reverse('pxeconfig'), params_in)
self.assertThat(
(
response.status_code,
@@ -2280,17 +2280,32 @@
MatchesListwise(
(
Equals(httplib.OK),
- Equals("text/plain; charset=utf-8"),
- StartsWith('DEFAULT menu'),
+ Equals("application/json"),
+ StartsWith(b'{'),
)),
response)
+ params_out = json.loads(response.content)
+ # Some parameters are unchanged.
+ params_unchanged = {"arch", "subarch", "title"}
+ self.assertEqual(
+ {name: params_in[name] for name in params_unchanged},
+ {name: params_out[name] for name in params_unchanged})
+ # The append parameter is... appended to.
+ self.assertThat(
+ params_out["append"], StartsWith(
+ "%(append)s auto url=http://"; % params_in))
+ # Some parameters are added.
+ params_added = {"release", "purpose"}
+ self.assertEqual(set(params_out).difference(params_in), params_added)
+ # The release is always "precise".
+ self.assertEqual("precise", params_out["release"])
def get_without_param(self, param):
params = self.get_params()
del params[param]
return self.client.get(reverse('pxeconfig'), params)
- def test_pxe_config_missing_parameters(self):
+ def test_pxeconfig_missing_parameters(self):
# Some parameters are optional, others are mandatory. The
# absence of a mandatory parameter always results in a BAD
# REQUEST response.
@@ -2344,17 +2359,21 @@
"auto url=%s" % api.compose_enlistment_preseed_url(),
api.compose_preseed_kernel_opt(None))
- def test_pxe_config_appends_enlistment_preseed_url_for_unknown_node(self):
+ def test_pxeconfig_appends_enlistment_preseed_url_for_unknown_node(self):
params = self.get_params()
params['mac'] = factory.getRandomMACAddress()
response = self.client.get(reverse('pxeconfig'), params)
- self.assertIn(api.compose_enlistment_preseed_url(), response.content)
+ self.assertIn(
+ api.compose_enlistment_preseed_url(),
+ json.loads(response.content)["append"])
- def test_pxe_config_appends_preseed_url_for_known_node(self):
+ def test_pxeconfig_appends_preseed_url_for_known_node(self):
params = self.get_params()
node = MACAddress.objects.get(mac_address=params['mac']).node
response = self.client.get(reverse('pxeconfig'), params)
- self.assertIn(api.compose_preseed_url(node), response.content)
+ self.assertIn(
+ api.compose_preseed_url(node),
+ json.loads(response.content)["append"])
def test_get_boot_purpose_unknown_node(self):
# A node that's not yet known to MAAS is assumed to be enlisting,
@@ -2381,11 +2400,13 @@
setattr(node, name, value)
self.assertEqual(purpose, api.get_boot_purpose(node))
- def test_pxe_config_uses_boot_purpose(self):
+ def test_pxeconfig_uses_boot_purpose(self):
fake_boot_purpose = factory.make_name("purpose")
self.patch(api, "get_boot_purpose", lambda node: fake_boot_purpose)
response = self.client.get(reverse('pxeconfig'), self.get_params())
- self.assertThat(response.content, Contains(fake_boot_purpose))
+ self.assertEqual(
+ fake_boot_purpose,
+ json.loads(response.content)["purpose"])
class TestNodeGroupsAPI(APITestCase):
=== modified file 'src/provisioningserver/pxe/config.py'
--- src/provisioningserver/pxe/config.py 2012-07-31 15:25:35 +0000
+++ src/provisioningserver/pxe/config.py 2012-08-01 22:05:27 +0000
@@ -31,7 +31,8 @@
template = tempita.Template.from_filename(template_filename, encoding="UTF-8")
-def render_pxe_config(title, arch, subarch, release, purpose, append):
+def render_pxe_config(
+ title, arch, subarch, release, purpose, append, **extra):
"""Render a PXE configuration file as a unicode string.
:param title: Title that the node should show on its boot menu.
@@ -40,6 +41,9 @@
:param release: The OS release, e.g. "precise".
:param purpose: What's the purpose of this boot, e.g. "install".
:param append: Additional kernel parameters.
+ :param extra: Allow for other arguments. This is a safety valve;
+ parameters generated in another component (for example, see
+ `TFTPBackend.get_config_reader`) won't cause this to break.
"""
image_dir = compose_image_path(arch, subarch, release, purpose)
return template.substitute(
=== modified file 'src/provisioningserver/pxe/tests/test_config.py'
--- src/provisioningserver/pxe/tests/test_config.py 2012-07-31 10:15:07 +0000
+++ src/provisioningserver/pxe/tests/test_config.py 2012-08-01 22:05:27 +0000
@@ -64,3 +64,24 @@
MatchesRegex(
r'.*^\s+APPEND %s$' % re.escape(options["append"]),
re.MULTILINE | re.DOTALL)))
+
+ def test_render_with_extra_arguments(self):
+ # render_pxe_config() allows any keyword arguments as a safety valve.
+ options = {
+ "title": factory.make_name("title"),
+ "arch": factory.make_name("arch"),
+ "subarch": factory.make_name("subarch"),
+ "release": factory.make_name("release"),
+ "purpose": factory.make_name("purpose"),
+ "append": factory.make_name("append"),
+ }
+ # Capture the output before sprinking in some random options.
+ output_before = render_pxe_config(**options)
+ # Sprinkle some magic in.
+ options.update(
+ (factory.make_name("name"), factory.make_name("value"))
+ for _ in range(10))
+ # Capture the output after sprinking in some random options.
+ output_after = render_pxe_config(**options)
+ # The generated template is the same.
+ self.assertEqual(output_before, output_after)
=== modified file 'src/provisioningserver/tests/test_tftp.py'
--- src/provisioningserver/tests/test_tftp.py 2012-07-31 13:22:15 +0000
+++ src/provisioningserver/tests/test_tftp.py 2012-08-01 22:05:27 +0000
@@ -13,6 +13,7 @@
__all__ = []
from functools import partial
+import json
from os import path
from urllib import urlencode
from urlparse import (
@@ -73,6 +74,8 @@
"subarch": factory.make_name("subarch"),
"mac": factory.getRandomMACAddress(b"-"),
}
+ # The bootpath component is a superset of arch and subarch.
+ components["bootpath"] = "maas/{arch}/{subarch}".format(**components)
config_path = compose_config_path(
arch=components["arch"], subarch=components["subarch"],
name=components["mac"])
@@ -174,17 +177,59 @@
config_path = compose_config_path(arch, subarch, mac)
backend = TFTPBackend(self.make_dir(), b"http://example.com/";)
- # Patch get_generator_url() to check params.
- generator_url = factory.make_name("generator-url").encode("ascii")
-
- @partial(self.patch, backend, "get_generator_url")
- def get_generator_url(params):
- expected_params = {"arch": arch, "subarch": subarch, "mac": mac}
- self.assertEqual(expected_params, params)
- return generator_url
-
- backend.get_page = succeed # Return the URL, via a Deferred.
- reader = yield backend.get_reader(config_path.lstrip("/"))
+ @partial(self.patch, backend, "get_config_reader")
+ def get_config_reader(params):
+ params_json = json.dumps(params)
+ params_json_reader = BytesReader(params_json)
+ return succeed(params_json_reader)
+
+ reader = yield backend.get_reader(config_path)
+ output = reader.read(10000)
+ # The expected parameters include bootpath; this is extracted from the
+ # file path by re_config_file.
+ expected_params = dict(
+ arch=arch, subarch=subarch, mac=mac,
+ bootpath="maas/%s/%s" % (arch, subarch))
+ observed_params = json.loads(output)
+ self.assertEqual(expected_params, observed_params)
+
+ @inlineCallbacks
+ def test_get_config_reader(self):
+ # get_config_reader() takes a dict() of parameters and returns an
+ # `IReader` of a PXE configuration, rendered by `render_pxe_config`.
+ backend = TFTPBackend(self.make_dir(), b"http://example.com/";)
+ # Fake configuration parameters, as discovered from the file path.
+ fake_params = dict(
+ arch=factory.make_name("arch"),
+ subarch=factory.make_name("subarch"),
+ mac=factory.getRandomMACAddress(b"-"))
+ fake_params.update(
+ bootpath="maas/%(arch)s/%(subarch)s" % fake_params)
+ # Fake configuration parameters, as returned from the API call.
+ fake_api_params = dict(fake_params)
+ fake_api_params.update(
+ append=factory.make_name("append"),
+ purpose=factory.make_name("purpose"),
+ release=factory.make_name("release"),
+ title=factory.make_name("title"))
+ # Add a title to the first set of parameters. This will later help
+ # demonstrate that the API parameters take precedence over the file
+ # path parameters.
+ fake_params["title"] = factory.make_name("original-title")
+ # Stub get_page to return the fake API configuration parameters.
+ fake_api_params_json = json.dumps(fake_api_params)
+ backend.get_page = lambda url: succeed(fake_api_params_json)
+ # Stub render_pxe_config to return the render parameters.
+ backend.render_pxe_config = lambda **kwargs: json.dumps(kwargs)
+ # Get the rendered configuration, which will actually be a JSON dump
+ # of the render-time parameters.
+ reader = yield backend.get_config_reader(fake_api_params)
self.addCleanup(reader.finish)
self.assertIsInstance(reader, BytesReader)
- self.assertEqual(generator_url, reader.read(1000))
+ output = reader.read(10000)
+ # The expected render-time parameters are a merge of previous
+ # parameters. Note that the API parameters take precedence.
+ expected_render_params = dict(fake_params)
+ expected_render_params.update(fake_api_params)
+ observed_render_params = json.loads(output)
+ self.assertEqual(expected_render_params, observed_render_params)
=== modified file 'src/provisioningserver/tftp.py'
--- src/provisioningserver/tftp.py 2012-07-31 13:22:15 +0000
+++ src/provisioningserver/tftp.py 2012-08-01 22:05:27 +0000
@@ -16,6 +16,7 @@
from io import BytesIO
from itertools import repeat
+import json
import re
from urllib import urlencode
from urlparse import (
@@ -24,6 +25,7 @@
)
from provisioningserver.enum import ARP_HTYPE
+from provisioningserver.pxe.config import render_pxe_config
from provisioningserver.utils import deferred
from tftp.backend import (
FilesystemSynchronousBackend,
@@ -62,6 +64,7 @@
"""
get_page = staticmethod(getPage)
+ render_pxe_config = staticmethod(render_pxe_config)
# This is how PXELINUX represents a MAC address. See
# http://www.syslinux.org/wiki/index.php/PXELINUX.
@@ -74,11 +77,13 @@
re_config_file = re.compile(
r'''
^/?
- maas # Static namespacing.
- /
- (?P<arch>[^/]+) # Capture arch.
- /
- (?P<subarch>[^/]+) # Capture subarch.
+ (?P<bootpath>
+ maas # Static namespacing.
+ /
+ (?P<arch>[^/]+) # Capture arch.
+ /
+ (?P<subarch>[^/]+) # Capture subarch.
+ ) # Capture boot directory.
/
pxelinux[.]cfg # PXELINUX expects this.
/
@@ -103,7 +108,8 @@
self.generator_url = urlparse(generator_url)
def get_generator_url(self, params):
- """Calculate the URL, including query, that the PXE config is at.
+ """Calculate the URL, including query, from which we can fetch
+ additional configuration parameters.
:param params: A dict, or iterable suitable for updating a dict, of
additional query parameters.
@@ -124,6 +130,26 @@
return url.geturl().encode("ascii")
@deferred
+ def get_config_reader(self, params):
+ """Return an `IReader` for a PXE config.
+
+ :param params: Parameters so far obtained, typically from the file
+ path requested.
+ """
+ url = self.get_generator_url(params)
+
+ def generate_config(api_params):
+ params.update(api_params)
+ config = self.render_pxe_config(**params)
+ return config.encode("utf-8")
+
+ d = self.get_page(url)
+ d.addCallback(json.loads)
+ d.addCallback(generate_config)
+ d.addCallback(BytesReader)
+ return d
+
+ @deferred
def get_reader(self, file_name):
"""See `IBackend.get_reader()`.
@@ -136,7 +162,4 @@
return super(TFTPBackend, self).get_reader(file_name)
else:
params = config_file_match.groupdict()
- url = self.get_generator_url(params)
- d = self.get_page(url)
- d.addCallback(BytesReader)
- return d
+ return self.get_config_reader(params)
Follow ups
