Merge lp:~jtv/maas/bug-1042877 into lp:maas/trunk
| Status: | Merged | ||||
|---|---|---|---|---|---|
| Approved by: | Jeroen T. Vermeulen on 2012-08-30 | ||||
| Approved revision: | 958 | ||||
| Merged at revision: | 949 | ||||
| Proposed branch: | lp:~jtv/maas/bug-1042877 | ||||
| Merge into: | lp: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) | 2012-08-30 | Approve on 2012-08-30 | |
|
Review via email:
|
|||
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.
- 958. By Jeroen T. Vermeulen on 2012-08-30
-
Review change.


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.