Merge lp:~jtv/maas/bug-1042877 into lp:~maas-committers/maas/trunk
- bug-1042877
- Merge into trunk
Status: | Merged | ||||
---|---|---|---|---|---|
Approved by: | Jeroen T. Vermeulen | ||||
Approved revision: | no longer in the source branch. | ||||
Merged at revision: | 949 | ||||
Proposed branch: | lp:~jtv/maas/bug-1042877 | ||||
Merge into: | lp:~maas-committers/maas/trunk | ||||
Diff against target: |
590 lines (+59/-130) 16 files modified
etc/pserv.yaml (+1/-1) scripts/maas-import-pxe-files (+13/-31) src/provisioningserver/config.py (+1/-1) src/provisioningserver/pxe/config.commissioning.template (+4/-4) src/provisioningserver/pxe/config.py (+5/-15) src/provisioningserver/pxe/config.template (+2/-2) src/provisioningserver/pxe/install_bootloader.py (+1/-1) src/provisioningserver/pxe/install_image.py (+1/-1) src/provisioningserver/pxe/tests/test_config.py (+7/-16) src/provisioningserver/pxe/tests/test_install_image.py (+3/-3) src/provisioningserver/pxe/tests/test_tftppath.py (+3/-3) src/provisioningserver/pxe/tftppath.py (+3/-3) src/provisioningserver/tests/test_config.py (+1/-1) src/provisioningserver/tests/test_maas_import_pxe_files.py (+6/-23) src/provisioningserver/tests/test_tftp.py (+7/-17) src/provisioningserver/tftp.py (+1/-8) |
||||
To merge this branch: | bzr merge lp:~jtv/maas/bug-1042877 | ||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Gavin Panella (community) | Approve | ||
Review via email: mp+122049@code.launchpad.net |
Commit message
Move our TFTP hierachy, and dispense with the bootpath (the "maas/" prefix in PXE TFTP paths).
Description of the change
I pre-imped this with Gavin, but also called in Daviey and Robie for details. A lot changes, and unfortunately it all more or less had to do so in one go:
* Instead of re-using /var/lib/tftpboot/, create our own /var/lib/
* No more "maas/" prefix: pxelinux.0, pxelinux.
* Don't download pxelinux.0; take just the i386 binary from syslinux.
* Consistently get all pxelinux files from /usr/lib/syslinux; don't mix installed modules with downloaded loaders.
* Drop bootpath and related complexity.
One small bit that I managed to leave for later: CURRENT_RELEASE should no longer be needed in maas-import-
Rather than trawl through the MP diff, you may find it easier to read the commit log as a blow-by-blow account.
Jeroen
Jeroen T. Vermeulen (jtv) wrote : | # |
Thanks for the reviews.
> [1]
>
> - # root: /var/lib/tftpboot
> + root: /var/lib/maas/tftp
>
> The approach I've been taking with this file is to include the default
> value, commented out. This is the expectation that Andres has now. So,
> the value change here is fine, but it should still be commented. I see
> that you've updated the ConfigTFTP.root default.
Okay: I commented this one out because it matches the (new) default.
> [2]
>
> In tftp.py:
>
> - ^/?
> ...
> + ^/*
>
> I think this should stay as zero-or-one rather than zero-or-more. The
> latter speaks of slapdashery. The zero-or-one is there to accomodate
> ambiguity in the TFTP spec, iirc.
On the other hand, you'd normally expect multiple consecutive slashes to be treated as a single slash. If not to satisfy general Unix sensibilities, then because it's what the standard Ubuntu tftpd does.
I agree that multiple slashes would be a bit suspicious, but when it comes to debugging, wouldn't a failure to find e.g. //pxelinux.0 be a massive red herring when it comes to debugging? I could imagine circumstances where it would be convenient to risk including an extra slash, and if that means that we end up normalizing slashes so as not to displease our own server, then all we've gained is some extra complexity to compensate for our own unwarranted strictness. Bear in mind that we want users to edit the templates that these paths occur in: we don't want to frustrate them with unexpected breakage. Be strict in what you generate and liberal in what you accept.
Conversely, in a case where a double leading slash is actually a sign of something wrong, you'd expect a file-not-found anyway. We don't even have items of the same name occurring at different depths in the tree.
Preview Diff
1 | === modified file 'etc/pserv.yaml' |
2 | --- etc/pserv.yaml 2012-08-30 06:25:25 +0000 |
3 | +++ etc/pserv.yaml 2012-08-30 16:38:18 +0000 |
4 | @@ -31,7 +31,7 @@ |
5 | ## TFTP configuration. |
6 | # |
7 | tftp: |
8 | - # root: /var/lib/tftpboot |
9 | + # root: /var/lib/maas/tftp |
10 | # port: 69 |
11 | port: 5244 |
12 | ## The URL to be contacted to generate PXE configurations. |
13 | |
14 | === modified file 'scripts/maas-import-pxe-files' |
15 | --- scripts/maas-import-pxe-files 2012-08-28 23:35:55 +0000 |
16 | +++ scripts/maas-import-pxe-files 2012-08-30 16:38:18 +0000 |
17 | @@ -6,7 +6,7 @@ |
18 | # pre-boot loader, kernels, and initrd images. |
19 | # |
20 | # This script downloads the required files into the TFTP home directory |
21 | -# (by default, /var/lib/tftpboot). Run it with the necessarily privileges |
22 | +# (by default, /var/lib/maas/tftp). Run it with the necessarily privileges |
23 | # to write them there. |
24 | |
25 | # Exit immediately if a command exits with a non-zero status. |
26 | @@ -64,31 +64,15 @@ |
27 | } |
28 | |
29 | |
30 | -# Download the pre-boot loader, pxelinux.0, for architecture $1. If |
31 | -# successful, install it for netboot use. (Not all architectures need this |
32 | -# file, and there's rarely an urgent need for the very latest file, so if |
33 | -# the download fails this function just skips it.) |
34 | +# Copy the pre-boot loader pxelinux.0, and modules we need, from the |
35 | +# installed syslinux version. Install it into the TFTP tree for |
36 | +# netbooting. |
37 | update_pre_boot_loader() { |
38 | - local arch=$1 |
39 | - local url=$(compose_installer_download_url $arch $CURRENT_RELEASE) |
40 | - if ! $DOWNLOAD $url/pxelinux.0 |
41 | - then |
42 | - echo "Could not download pre-boot loader for $arch; skipping." |
43 | - return |
44 | - fi |
45 | - |
46 | - # If the file has changed, move it into place (replacing any previous |
47 | - # version). Otherwise, leave the filesystem alone. |
48 | - if [ -f pxelinux.0 ] |
49 | - then |
50 | - # TODO: pxelinux.0 is downloaded but there's no reason why it can't |
51 | - # come from the syslinux package like chain.c32 is. |
52 | - maas-provision install-pxe-bootloader --loader='pxelinux.0' |
53 | - maas-provision install-pxe-bootloader \ |
54 | - --loader='/usr/lib/syslinux/chain.c32' |
55 | - maas-provision install-pxe-bootloader \ |
56 | - --loader='/usr/lib/syslinux/ifcpu64.c32' |
57 | - fi |
58 | + for loader_file in pxelinux.0 chain.c32 ifcpu64.c32 |
59 | + do |
60 | + maas-provision install-pxe-bootloader \ |
61 | + --loader="/usr/lib/syslinux/$loader_file" |
62 | + done |
63 | } |
64 | |
65 | |
66 | @@ -122,14 +106,8 @@ |
67 | echo "Downloading to temporary location $DOWNLOAD_DIR." |
68 | pushd -- $DOWNLOAD_DIR |
69 | |
70 | - # All files we create here are public. The TFTP user will need to be |
71 | - # able to read them. |
72 | - umask a+r |
73 | - |
74 | for arch in $ARCHES |
75 | do |
76 | - update_pre_boot_loader $arch |
77 | - |
78 | for release in $RELEASES |
79 | do |
80 | update_install_files $arch $release |
81 | @@ -151,7 +129,11 @@ |
82 | |
83 | |
84 | main() { |
85 | + # All files we create here are public. The TFTP user will need to be |
86 | + # able to read them. |
87 | + umask a+r |
88 | |
89 | + update_pre_boot_loader |
90 | import_install_images |
91 | import_ephemeral_images |
92 | } |
93 | |
94 | === modified file 'src/provisioningserver/config.py' |
95 | --- src/provisioningserver/config.py 2012-08-30 06:25:25 +0000 |
96 | +++ src/provisioningserver/config.py 2012-08-30 16:38:18 +0000 |
97 | @@ -59,7 +59,7 @@ |
98 | |
99 | if_key_missing = None |
100 | |
101 | - root = String(if_missing="/var/lib/tftpboot") |
102 | + root = String(if_missing="/var/lib/maas/tftp") |
103 | port = Int(min=1, max=65535, if_missing=69) |
104 | generator = String(if_missing=b"http://localhost/MAAS/api/1.0/pxeconfig/") |
105 | |
106 | |
107 | === modified file 'src/provisioningserver/pxe/config.commissioning.template' |
108 | --- src/provisioningserver/pxe/config.commissioning.template 2012-08-24 12:48:12 +0000 |
109 | +++ src/provisioningserver/pxe/config.commissioning.template 2012-08-30 16:38:18 +0000 |
110 | @@ -6,14 +6,14 @@ |
111 | |
112 | LABEL amd64 |
113 | SAY Booting (amd64) under MAAS direction... |
114 | - KERNEL {{kernel_params(arch="amd64") | kernel_path | relative}} |
115 | - INITRD {{kernel_params(arch="amd64") | initrd_path | relative}} |
116 | + KERNEL {{kernel_params(arch="amd64") | kernel_path }} |
117 | + INITRD {{kernel_params(arch="amd64") | initrd_path }} |
118 | APPEND {{kernel_params(arch="amd64") | kernel_command}} |
119 | IPAPPEND 2 |
120 | |
121 | LABEL i386 |
122 | SAY Booting (i386) under MAAS direction... |
123 | - KERNEL {{kernel_params(arch="i386") | kernel_path | relative}} |
124 | - INITRD {{kernel_params(arch="i386") | initrd_path | relative}} |
125 | + KERNEL {{kernel_params(arch="i386") | kernel_path }} |
126 | + INITRD {{kernel_params(arch="i386") | initrd_path }} |
127 | APPEND {{kernel_params(arch="i386") | kernel_command}} |
128 | IPAPPEND 2 |
129 | |
130 | === modified file 'src/provisioningserver/pxe/config.py' |
131 | --- src/provisioningserver/pxe/config.py 2012-08-24 19:14:58 +0000 |
132 | +++ src/provisioningserver/pxe/config.py 2012-08-30 16:38:18 +0000 |
133 | @@ -23,7 +23,6 @@ |
134 | from errno import ENOENT |
135 | from os import path |
136 | |
137 | -import posixpath |
138 | from provisioningserver.kernel_opts import compose_kernel_command_line_new |
139 | from provisioningserver.pxe.tftppath import compose_image_path |
140 | import tempita |
141 | @@ -71,18 +70,14 @@ |
142 | "No PXE template found in %r!" % template_dir) |
143 | |
144 | |
145 | -def render_pxe_config(bootpath, kernel_params, **extra): |
146 | +def render_pxe_config(kernel_params, **extra): |
147 | """Render a PXE configuration file as a unicode string. |
148 | |
149 | - :param bootpath: The directory path of `pxelinux.0`. |
150 | :param kernel_params: An instance of `KernelParameters`. |
151 | :param extra: Allow for other arguments. This is a safety valve; |
152 | parameters generated in another component (for example, see |
153 | `TFTPBackend.get_config_reader`) won't cause this to break. |
154 | """ |
155 | - if bootpath is None: |
156 | - bootpath = '' |
157 | - |
158 | template = get_pxe_template( |
159 | kernel_params.purpose, kernel_params.arch, |
160 | kernel_params.subarch) |
161 | @@ -95,24 +90,19 @@ |
162 | params.arch, params.subarch, |
163 | params.release, params.purpose) |
164 | |
165 | - def initrd(params): |
166 | + def initrd_path(params): |
167 | return "%s/initrd.gz" % image_dir(params) |
168 | |
169 | - def kernel(params): |
170 | + def kernel_path(params): |
171 | return "%s/linux" % image_dir(params) |
172 | |
173 | def kernel_command(params): |
174 | return compose_kernel_command_line_new(params) |
175 | |
176 | - def relative(path): |
177 | - # Return `path` relative to `bootpath`. |
178 | - return posixpath.relpath(path, start=bootpath) |
179 | - |
180 | namespace = { |
181 | - "initrd_path": initrd, |
182 | + "initrd_path": initrd_path, |
183 | "kernel_command": kernel_command, |
184 | "kernel_params": kernel_params, |
185 | - "kernel_path": kernel, |
186 | - "relative": relative, |
187 | + "kernel_path": kernel_path, |
188 | } |
189 | return template.substitute(namespace) |
190 | |
191 | === modified file 'src/provisioningserver/pxe/config.template' |
192 | --- src/provisioningserver/pxe/config.template 2012-08-24 12:48:12 +0000 |
193 | +++ src/provisioningserver/pxe/config.template 2012-08-30 16:38:18 +0000 |
194 | @@ -2,7 +2,7 @@ |
195 | |
196 | LABEL execute |
197 | SAY Booting under MAAS direction... |
198 | - KERNEL {{kernel_params | kernel_path | relative}} |
199 | - INITRD {{kernel_params | initrd_path | relative}} |
200 | + KERNEL {{kernel_params | kernel_path }} |
201 | + INITRD {{kernel_params | initrd_path }} |
202 | APPEND {{kernel_params | kernel_command}} |
203 | IPAPPEND 2 |
204 | |
205 | === modified file 'src/provisioningserver/pxe/install_bootloader.py' |
206 | --- src/provisioningserver/pxe/install_bootloader.py 2012-08-17 14:28:27 +0000 |
207 | +++ src/provisioningserver/pxe/install_bootloader.py 2012-08-30 16:38:18 +0000 |
208 | @@ -30,7 +30,7 @@ |
209 | """Locate a loader's destination, creating the directory if needed. |
210 | |
211 | :param tftproot: The root directory served up by the TFTP server, |
212 | - e.g. /var/lib/tftpboot/. |
213 | + e.g. /var/lib/maas/tftp/. |
214 | :return: Full path describing the directory that the installed loader |
215 | should end up having. |
216 | """ |
217 | |
218 | === modified file 'src/provisioningserver/pxe/install_image.py' |
219 | --- src/provisioningserver/pxe/install_image.py 2012-07-23 10:24:39 +0000 |
220 | +++ src/provisioningserver/pxe/install_image.py 2012-08-30 16:38:18 +0000 |
221 | @@ -33,7 +33,7 @@ |
222 | """Locate an image's destination. Create containing directory if needed. |
223 | |
224 | :param tftproot: The root directory served up by the TFTP server, |
225 | - e.g. /var/lib/tftpboot/. |
226 | + e.g. /var/lib/maas/tftp/. |
227 | :param arch: Main architecture to locate the destination for. |
228 | :param subarch: Sub-architecture of the main architecture. |
229 | :param release: OS release name, e.g. "precise". |
230 | |
231 | === modified file 'src/provisioningserver/pxe/tests/test_config.py' |
232 | --- src/provisioningserver/pxe/tests/test_config.py 2012-08-24 16:23:15 +0000 |
233 | +++ src/provisioningserver/pxe/tests/test_config.py 2012-08-30 16:38:18 +0000 |
234 | @@ -21,7 +21,6 @@ |
235 | from maastesting.matchers import ContainsAll |
236 | from maastesting.testcase import TestCase |
237 | import mock |
238 | -import posixpath |
239 | from provisioningserver import kernel_opts |
240 | from provisioningserver.pxe import config |
241 | from provisioningserver.pxe.config import render_pxe_config |
242 | @@ -150,9 +149,8 @@ |
243 | def test_render(self): |
244 | # Given the right configuration options, the PXE configuration is |
245 | # correctly rendered. |
246 | - bootpath = factory.make_name("bootpath") |
247 | params = make_kernel_parameters() |
248 | - output = render_pxe_config(bootpath=bootpath, kernel_params=params) |
249 | + output = render_pxe_config(kernel_params=params) |
250 | # The output is always a Unicode string. |
251 | self.assertThat(output, IsInstance(unicode)) |
252 | # The template has rendered without error. PXELINUX configurations |
253 | @@ -162,7 +160,6 @@ |
254 | image_dir = compose_image_path( |
255 | arch=params.arch, subarch=params.subarch, |
256 | release=params.release, purpose=params.purpose) |
257 | - image_dir = posixpath.relpath(image_dir, bootpath) |
258 | self.assertThat( |
259 | output, MatchesAll( |
260 | MatchesRegex( |
261 | @@ -177,10 +174,7 @@ |
262 | |
263 | def test_render_with_extra_arguments_does_not_affect_output(self): |
264 | # render_pxe_config() allows any keyword arguments as a safety valve. |
265 | - options = { |
266 | - "bootpath": factory.make_name("bootpath"), |
267 | - "kernel_params": make_kernel_parameters(), |
268 | - } |
269 | + options = {"kernel_params": make_kernel_parameters()} |
270 | # Capture the output before sprinking in some random options. |
271 | output_before = render_pxe_config(**options) |
272 | # Sprinkle some magic in. |
273 | @@ -196,7 +190,6 @@ |
274 | # If purpose is "local", the config.localboot.template should be |
275 | # used. |
276 | options = { |
277 | - "bootpath": factory.make_name("bootpath"), |
278 | "kernel_params": |
279 | make_kernel_parameters()._replace(purpose="local"), |
280 | } |
281 | @@ -207,7 +200,6 @@ |
282 | # Intel i386 is a special case and needs to use the chain.c32 |
283 | # loader as the LOCALBOOT PXE directive is unreliable. |
284 | options = { |
285 | - "bootpath": factory.make_name("bootpath"), |
286 | "kernel_params": make_kernel_parameters()._replace( |
287 | arch="i386", purpose="local"), |
288 | } |
289 | @@ -219,7 +211,6 @@ |
290 | # Intel amd64 is a special case and needs to use the chain.c32 |
291 | # loader as the LOCALBOOT PXE directive is unreliable. |
292 | options = { |
293 | - "bootpath": factory.make_name("bootpath"), |
294 | "kernel_params": make_kernel_parameters()._replace( |
295 | arch="amd64", purpose="local"), |
296 | } |
297 | @@ -233,7 +224,6 @@ |
298 | get_ephemeral_name = self.patch(kernel_opts, "get_ephemeral_name") |
299 | get_ephemeral_name.return_value = factory.make_name("ephemeral") |
300 | options = { |
301 | - "bootpath": factory.make_name("bootpath"), |
302 | "kernel_params": make_kernel_parameters()._replace( |
303 | purpose="commissioning"), |
304 | } |
305 | @@ -251,13 +241,14 @@ |
306 | default_section["APPEND"].split()) |
307 | # Both "i386" and "amd64" sections exist. |
308 | self.assertThat(config, ContainsAll(("i386", "amd64"))) |
309 | - # Each section defines KERNEL, INITRD, and APPEND settings, each |
310 | - # containing paths referring to their architectures. |
311 | + # Each section defines KERNEL, INITRD, and APPEND settings. The |
312 | + # KERNEL and INITRD ones contain paths referring to their |
313 | + # architectures. |
314 | for section_label in ("i386", "amd64"): |
315 | section = config[section_label] |
316 | self.assertThat( |
317 | section, ContainsAll(("KERNEL", "INITRD", "APPEND"))) |
318 | - contains_arch_path = Contains("/%s/" % section_label) |
319 | + contains_arch_path = StartsWith("%s/" % section_label) |
320 | self.assertThat(section["KERNEL"], contains_arch_path) |
321 | self.assertThat(section["INITRD"], contains_arch_path) |
322 | - self.assertThat(section["APPEND"], contains_arch_path) |
323 | + self.assertIn("APPEND", section) |
324 | |
325 | === modified file 'src/provisioningserver/pxe/tests/test_install_image.py' |
326 | --- src/provisioningserver/pxe/tests/test_install_image.py 2012-07-23 13:25:15 +0000 |
327 | +++ src/provisioningserver/pxe/tests/test_install_image.py 2012-08-30 16:38:18 +0000 |
328 | @@ -79,13 +79,13 @@ |
329 | def test_make_destination_follows_pxe_path_conventions(self): |
330 | # The directory that make_destination returns follows the PXE |
331 | # directory hierarchy specified for MAAS: |
332 | - # /var/lib/tftproot/maas/<arch>/<subarch>/<release>/<purpose> |
333 | - # (Where the /var/lib/tftproot/ part is configurable, so we |
334 | + # /var/lib/maas/tftp/<arch>/<subarch>/<release>/<purpose> |
335 | + # (Where the /var/lib/maas/tftp/ part is configurable, so we |
336 | # can test this without overwriting system files). |
337 | tftproot = self.make_dir() |
338 | arch, subarch, release, purpose = make_arch_subarch_release_purpose() |
339 | self.assertEqual( |
340 | - os.path.join(tftproot, 'maas', arch, subarch, release, purpose), |
341 | + os.path.join(tftproot, arch, subarch, release, purpose), |
342 | make_destination(tftproot, arch, subarch, release, purpose)) |
343 | |
344 | def test_make_destination_creates_directory_if_not_present(self): |
345 | |
346 | === modified file 'src/provisioningserver/pxe/tests/test_tftppath.py' |
347 | --- src/provisioningserver/pxe/tests/test_tftppath.py 2012-08-17 14:11:20 +0000 |
348 | +++ src/provisioningserver/pxe/tests/test_tftppath.py 2012-08-30 16:38:18 +0000 |
349 | @@ -41,7 +41,7 @@ |
350 | def test_compose_config_path_follows_maas_pxe_directory_layout(self): |
351 | name = factory.make_name('config') |
352 | self.assertEqual( |
353 | - 'maas/pxelinux.cfg/%02x-%s' % (ARP_HTYPE.ETHERNET, name), |
354 | + 'pxelinux.cfg/%02x-%s' % (ARP_HTYPE.ETHERNET, name), |
355 | compose_config_path(name)) |
356 | |
357 | def test_compose_config_path_does_not_include_tftp_root(self): |
358 | @@ -56,7 +56,7 @@ |
359 | release = factory.make_name('release') |
360 | purpose = factory.make_name('purpose') |
361 | self.assertEqual( |
362 | - 'maas/%s/%s/%s/%s' % (arch, subarch, release, purpose), |
363 | + '%s/%s/%s/%s' % (arch, subarch, release, purpose), |
364 | compose_image_path(arch, subarch, release, purpose)) |
365 | |
366 | def test_compose_image_path_does_not_include_tftp_root(self): |
367 | @@ -69,7 +69,7 @@ |
368 | Not(StartsWith(self.tftproot))) |
369 | |
370 | def test_compose_bootloader_path_follows_maas_pxe_directory_layout(self): |
371 | - self.assertEqual('maas/pxelinux.0', compose_bootloader_path()) |
372 | + self.assertEqual('pxelinux.0', compose_bootloader_path()) |
373 | |
374 | def test_compose_bootloader_path_does_not_include_tftp_root(self): |
375 | self.assertThat( |
376 | |
377 | === modified file 'src/provisioningserver/pxe/tftppath.py' |
378 | --- src/provisioningserver/pxe/tftppath.py 2012-08-30 06:25:25 +0000 |
379 | +++ src/provisioningserver/pxe/tftppath.py 2012-08-30 16:38:18 +0000 |
380 | @@ -29,7 +29,7 @@ |
381 | simulate PXELINUX and don't actually load `pxelinux.0`, but use its path |
382 | to figure out where configuration files are located. |
383 | """ |
384 | - return "maas/pxelinux.0" |
385 | + return "pxelinux.0" |
386 | |
387 | |
388 | # TODO: move this; it is now only used for testing. |
389 | @@ -48,7 +48,7 @@ |
390 | # Not using os.path.join: this is a TFTP path, not a native path. Yes, in |
391 | # practice for us they're the same. We always assume that the ARP HTYPE |
392 | # (hardware type) that PXELINUX sends is Ethernet. |
393 | - return "maas/pxelinux.cfg/{htype:02x}-{mac}".format( |
394 | + return "pxelinux.cfg/{htype:02x}-{mac}".format( |
395 | htype=ARP_HTYPE.ETHERNET, mac=mac) |
396 | |
397 | |
398 | @@ -66,7 +66,7 @@ |
399 | :return: Path for the corresponding image directory (containing a |
400 | kernel and initrd) as exposed over TFTP. |
401 | """ |
402 | - return '/'.join(['maas', arch, subarch, release, purpose]) |
403 | + return '/'.join([arch, subarch, release, purpose]) |
404 | |
405 | |
406 | def locate_tftp_path(path, tftproot): |
407 | |
408 | === modified file 'src/provisioningserver/tests/test_config.py' |
409 | --- src/provisioningserver/tests/test_config.py 2012-08-24 10:53:26 +0000 |
410 | +++ src/provisioningserver/tests/test_config.py 2012-08-30 16:38:18 +0000 |
411 | @@ -147,7 +147,7 @@ |
412 | 'tftp': { |
413 | 'generator': 'http://localhost/MAAS/api/1.0/pxeconfig/', |
414 | 'port': 69, |
415 | - 'root': "/var/lib/tftpboot", |
416 | + 'root': "/var/lib/maas/tftp", |
417 | }, |
418 | } |
419 | |
420 | |
421 | === modified file 'src/provisioningserver/tests/test_maas_import_pxe_files.py' |
422 | --- src/provisioningserver/tests/test_maas_import_pxe_files.py 2012-08-17 14:45:13 +0000 |
423 | +++ src/provisioningserver/tests/test_maas_import_pxe_files.py 2012-08-30 16:38:18 +0000 |
424 | @@ -23,11 +23,7 @@ |
425 | ) |
426 | from provisioningserver.pxe import tftppath |
427 | from provisioningserver.testing.config import ConfigFixture |
428 | -from testtools.matchers import ( |
429 | - FileContains, |
430 | - FileExists, |
431 | - Not, |
432 | - ) |
433 | +from testtools.matchers import FileContains |
434 | |
435 | |
436 | def read_file(path, name): |
437 | @@ -97,7 +93,7 @@ |
438 | archive = self.make_dir() |
439 | download = compose_download_dir(archive, arch, release) |
440 | os.makedirs(download) |
441 | - for filename in ['initrd.gz', 'linux', 'pxelinux.0']: |
442 | + for filename in ['initrd.gz', 'linux']: |
443 | factory.make_file(download, filename) |
444 | return archive |
445 | |
446 | @@ -135,40 +131,27 @@ |
447 | with open(os.devnull, 'wb') as dev_null: |
448 | check_call(script, env=env, stdout=dev_null) |
449 | |
450 | - def test_downloads_pre_boot_loader(self): |
451 | + def test_procures_pre_boot_loader(self): |
452 | arch = factory.make_name('arch') |
453 | release = 'precise' |
454 | archive = self.make_downloads(arch=arch, release=release) |
455 | self.call_script(archive, self.tftproot, arch=arch, release=release) |
456 | tftp_path = compose_tftp_bootloader_path(self.tftproot) |
457 | - download_path = compose_download_dir(archive, arch, release) |
458 | - expected_contents = read_file(download_path, 'pxelinux.0') |
459 | + expected_contents = read_file('/usr/lib/syslinux', 'pxelinux.0') |
460 | self.assertThat(tftp_path, FileContains(expected_contents)) |
461 | |
462 | - def test_ignores_missing_pre_boot_loader(self): |
463 | - arch = factory.make_name('arch') |
464 | - release = 'precise' |
465 | - archive = self.make_downloads(arch=arch, release=release) |
466 | - download_path = compose_download_dir(archive, arch, release) |
467 | - os.remove(os.path.join(download_path, 'pxelinux.0')) |
468 | - self.call_script(archive, self.tftproot, arch=arch, release=release) |
469 | - tftp_path = compose_tftp_bootloader_path(self.tftproot) |
470 | - self.assertThat(tftp_path, Not(FileExists())) |
471 | - |
472 | def test_updates_pre_boot_loader(self): |
473 | arch = factory.make_name('arch') |
474 | release = 'precise' |
475 | tftp_path = compose_tftp_bootloader_path(self.tftproot) |
476 | - os.makedirs(os.path.dirname(tftp_path)) |
477 | with open(tftp_path, 'w') as existing_file: |
478 | existing_file.write(factory.getRandomString()) |
479 | archive = self.make_downloads(arch=arch, release=release) |
480 | self.call_script(archive, self.tftproot, arch=arch, release=release) |
481 | - download_path = compose_download_dir(archive, arch, release) |
482 | - expected_contents = read_file(download_path, 'pxelinux.0') |
483 | + expected_contents = read_file('/usr/lib/syslinux', 'pxelinux.0') |
484 | self.assertThat(tftp_path, FileContains(expected_contents)) |
485 | |
486 | - def test_downloads_install_image(self): |
487 | + def test_procures_install_image(self): |
488 | arch = factory.make_name('arch') |
489 | release = 'precise' |
490 | archive = self.make_downloads(arch=arch, release=release) |
491 | |
492 | === modified file 'src/provisioningserver/tests/test_tftp.py' |
493 | --- src/provisioningserver/tests/test_tftp.py 2012-08-27 12:00:44 +0000 |
494 | +++ src/provisioningserver/tests/test_tftp.py 2012-08-30 16:38:18 +0000 |
495 | @@ -70,10 +70,7 @@ |
496 | The path is intended to match `re_config_file`, and the components are |
497 | the expected groups from a match. |
498 | """ |
499 | - components = { |
500 | - "bootpath": b"maas", # Static. |
501 | - "mac": factory.getRandomMACAddress(b"-"), |
502 | - } |
503 | + components = {"mac": factory.getRandomMACAddress(b"-")} |
504 | config_path = compose_config_path(components["mac"]) |
505 | return config_path, components |
506 | |
507 | @@ -115,17 +112,17 @@ |
508 | mac = 'aa-bb-cc-dd-ee-ff' |
509 | match = TFTPBackend.re_config_file.match('pxelinux.cfg/01-%s' % mac) |
510 | self.assertIsNotNone(match) |
511 | - self.assertEqual({'mac': mac, 'bootpath': None}, match.groupdict()) |
512 | + self.assertEqual({'mac': mac}, match.groupdict()) |
513 | |
514 | def test_re_config_file_matches_pxelinux_cfg_with_leading_slash(self): |
515 | mac = 'aa-bb-cc-dd-ee-ff' |
516 | match = TFTPBackend.re_config_file.match('/pxelinux.cfg/01-%s' % mac) |
517 | self.assertIsNotNone(match) |
518 | - self.assertEqual({'mac': mac, 'bootpath': None}, match.groupdict()) |
519 | + self.assertEqual({'mac': mac}, match.groupdict()) |
520 | |
521 | def test_re_config_file_does_not_match_non_config_file(self): |
522 | self.assertIsNone( |
523 | - TFTPBackend.re_config_file.match('maas/pxelinux.cfg/kernel')) |
524 | + TFTPBackend.re_config_file.match('pxelinux.cfg/kernel')) |
525 | |
526 | def test_re_config_file_does_not_match_file_in_root(self): |
527 | self.assertIsNone( |
528 | @@ -153,18 +150,16 @@ |
529 | def test_get_generator_url(self): |
530 | # get_generator_url() merges the parameters obtained from the request |
531 | # file path (arch, subarch, name) into the configured generator URL. |
532 | - bootpath = factory.make_name("bootpath") |
533 | mac = factory.getRandomMACAddress(b"-") |
534 | dummy = factory.make_name("dummy").encode("ascii") |
535 | backend_url = b"http://example.com/?" + urlencode({b"dummy": dummy}) |
536 | backend = TFTPBackend(self.make_dir(), backend_url) |
537 | # params is an example of the parameters obtained from a request. |
538 | - params = {"bootpath": bootpath, "mac": mac} |
539 | + params = {"mac": mac} |
540 | generator_url = urlparse(backend.get_generator_url(params)) |
541 | self.assertEqual("example.com", generator_url.hostname) |
542 | query = parse_qsl(generator_url.query) |
543 | query_expected = [ |
544 | - ("bootpath", bootpath), |
545 | ("dummy", dummy), |
546 | ("mac", mac), |
547 | ] |
548 | @@ -200,9 +195,7 @@ |
549 | |
550 | reader = yield backend.get_reader(config_path) |
551 | output = reader.read(10000) |
552 | - # The expected parameters include bootpath; this is extracted from the |
553 | - # file path by re_config_file. |
554 | - expected_params = dict(mac=mac, bootpath="maas") |
555 | + expected_params = dict(mac=mac) |
556 | observed_params = json.loads(output) |
557 | self.assertEqual(expected_params, observed_params) |
558 | |
559 | @@ -212,10 +205,7 @@ |
560 | # `IReader` of a PXE configuration, rendered by `render_pxe_config`. |
561 | backend = TFTPBackend(self.make_dir(), b"http://example.com/") |
562 | # Fake configuration parameters, as discovered from the file path. |
563 | - fake_params = { |
564 | - "bootpath": "maas", |
565 | - "mac": factory.getRandomMACAddress(b"-"), |
566 | - } |
567 | + fake_params = {"mac": factory.getRandomMACAddress(b"-")} |
568 | # Fake kernel configuration parameters, as returned from the API call. |
569 | fake_kernel_params = make_kernel_parameters() |
570 | |
571 | |
572 | === modified file 'src/provisioningserver/tftp.py' |
573 | --- src/provisioningserver/tftp.py 2012-08-24 18:34:57 +0000 |
574 | +++ src/provisioningserver/tftp.py 2012-08-30 16:38:18 +0000 |
575 | @@ -86,14 +86,7 @@ |
576 | re_config_file = re.compile( |
577 | r''' |
578 | # Optional leading slash(es). |
579 | - ^/? |
580 | - # Optional boot path prefix (separated from the rest by slash): |
581 | - (?: |
582 | - (?P<bootpath> |
583 | - maas |
584 | - ) |
585 | - / |
586 | - )? |
587 | + ^/* |
588 | pxelinux[.]cfg # PXELINUX expects this. |
589 | / |
590 | {htype:02x} # ARP HTYPE. |
Bootiful, as they say in Norfolk (means: jolly spiffing).
[1]
- # root: /var/lib/tftpboot
+ root: /var/lib/maas/tftp
The approach I've been taking with this file is to include the default
value, commented out. This is the expectation that Andres has now. So,
the value change here is fine, but it should still be commented. I see
that you've updated the ConfigTFTP.root default.
[2]
In tftp.py:
- ^/?
...
+ ^/*
I think this should stay as zero-or-one rather than zero-or-more. The
latter speaks of slapdashery. The zero-or-one is there to accomodate
ambiguity in the TFTP spec, iirc.