Merge lp://staging/~smoser/maas/kernel-cmdline-cleanup into lp://staging/~maas-committers/maas/trunk
| Status: | Merged |
|---|---|
| Approved by: | Scott Moser |
| Approved revision: | no longer in the source branch. |
| Merged at revision: | 1002 |
| Proposed branch: | lp://staging/~smoser/maas/kernel-cmdline-cleanup |
| Merge into: | lp://staging/~maas-committers/maas/trunk |
| Diff against target: |
283 lines (+79/-60) 4 files modified
src/provisioningserver/kernel_opts.py (+28/-33) src/provisioningserver/pxe/config.commissioning.template (+2/-0) src/provisioningserver/pxe/config.template (+1/-0) src/provisioningserver/tests/test_kernel_opts.py (+48/-27) |
| To merge this branch: | bzr merge lp://staging/~smoser/maas/kernel-cmdline-cleanup |
| Related bugs: |
| Reviewer | Review Type | Date Requested | Status |
|---|---|---|---|
| Raphaël Badin (community) | Approve | ||
|
Review via email:
|
|||
Commit message
cleanup of kernel command line arguments, add iscsi_inititator param
Move some options to purpose specific, drop some, change some.
ephemeral/
* add iscsi_initiator: this was always supposed to be required, but 12.04
initramfs seem to work without it, so we didn't have it. 12.10 fail
to configure the iscsi target in the initramfs without it. (LP: #1050523)
* change url= to cloud-config-
This was really just re-named to cloud-config-url. cloud-init supports
both 'url=' and 'cloud-config-url', but cloud-config-url is preferable
as it is obviously more explicit in purpose.
* change ip=dhcp to ip=::::<hostname>
LP: #1046405 has more information, but this is how you would specify
that the initramfs's dhcp client should specify hostname
* add overlayroot=tmpfs
cloud images for quantal now use the overlayroot package to accomplish
read-only root. 12.04 images used a un-packaged earlier version that
was hard-coded in the images to on. 12.10 and later need a kernel param
to enable it.
move to install specific:
netcfg/
common:
* add to intel: 'console=tty1 console=ttyS0'.
This should log kernel messages to both serial console and graphical
console if it is present. Note, the kernel assigns /dev/console to
the last valid argument.
* add 'nomodeset' to not switch video mode
* remove 'suite'. Nothing that I am aware of reads this.
Other:
* add 'SAY' of the kernel cmdline. At least in testing with kvm
and -curses, this is very helpful as it goes to wherever the bios
console is, and you can see it even if 'console=ttyS0' is given
and the kernel's log of that information goes elsewhere.
Description of the change
cleanup of kernel command line arguments, add iscsi_inititator param
This generally cleans up the parameters that we pass on the kernel command
line.
ephemeral/
* add iscsi_initiator: this was always supposed to be required, but 12.04
initramfs seem to work without it, so we didn't have it. 12.10 fail
to configure the iscsi target in the initramfs without it. (LP: #1050523)
* change url= to cloud-config-
This was really just re-named to cloud-config-url. cloud-init supports
both 'url=' and 'cloud-config-url', but cloud-config-url is preferable
as it is obviously more explicit in purpose.
* change ip=dhcp to ip=::::<hostname>
LP: #1046405 has more information, but this is how you would specify
that the initramfs's dhcp client should specify hostname
* add overlayroot=tmpfs
cloud images for quantal now use the overlayroot package to accomplish
read-only root. 12.04 images used a un-packaged earlier version that
was hard-coded in the images to on. 12.10 and later need a kernel param
to enable it.
move to install specific:
netcfg/
common:
* add to intel: 'console=tty1 console=ttyS0'.
This should log kernel messages to both serial console and graphical
console if it is present. Note, the kernel assigns /dev/console to
the last valid argument.
* add 'nomodeset' to not switch video mode
* remove 'suite'. Nothing that I am aware of reads this.
Other:
* add 'SAY' of the kernel cmdline. At least in testing with kvm
and -curses, this is very helpful as it goes to wherever the bios
console is, and you can see it even if 'console=ttyS0' is given
and the kernel's log of that information goes elsewhere.
Looks good, thanks for the comments in the code. I suppose you've field-tested this. A few remarks:
[0]
162 +def make_kernel_
Looks like 'content' could be renamed 'extra_parameters' or something…?
[1]
Some things are untested, I think it's worth adding a few tests or changing existing tests:
48 + "ip=::::%s" % params.hostname,
65 + "text priority=%s" % "critical",
79 + return ["console=tty1 console=ttyS0"]
97 + options = ["nomodeset"]
[2]
244 + self.assertIn(
245 + self.assertIn(
246 + self.assertIn(
If something goes wrong, this would be more easy to fix it it was written as a one line check:
Something along the lines of
]))
Even if I admit this is slightly less readable, the reasoning is that, if none of the strings that should be present in the cmdline are present, you'll have that information in one go as opposed to learning that "root=LABEL=
[3]
88 - compose_initrd_opt(
89 - params.arch, params.subarch,
90 - params.release, params.purpose),
The method 'compose_
fwiw, I spotted that in the coverage information:
$ ./bin/maas test --with-coverage --cover-
kernel_opts.py -s
[...]
Name Stmts Miss Cover Missing
-------
provisioningser
-------
Ran 13 tests in 0.183s
[4]
$ make lint
src/provisionin