Code review comment for lp:~blake-rouse/maas/powernv-support

Blake, not done reviewing yet but here's my comments so far:

[1]

=== added file 'etc/maas/templates/pxe/config.commissioning.ppc64el.template'
--- etc/maas/templates/pxe/config.commissioning.ppc64el.template 1970-01-01 00:00:00 +0000
+++ etc/maas/templates/pxe/config.commissioning.ppc64el.template 2014-05-28 21:38:08 +0000
@@ -0,0 +1,6 @@
+DEFAULT execute
+
+LABEL execute
+ KERNEL {{kernel_params | kernel_path }}
+ INITRD {{kernel_params | initrd_path }}
+ APPEND {{kernel_params | kernel_command}}

=== added file 'etc/maas/templates/pxe/config.install.ppc64el.template'
...
=== added file 'etc/maas/templates/pxe/config.xinstall.ppc64el.template'

I think the duplicate files here should be symlinks - armhf does this
right now.

[2]

+# PXELINUX represents a MAC address in IEEE 802 hyphen-separated
+# format. See http://www.syslinux.org/wiki/index.php/PXELINUX.
+re_mac_address_octet = r'[0-9a-f]{2}'
+re_mac_address = re.compile(
+ "-".join(repeat(re_mac_address_octet, 6)))

This is copied directly from pxe.py. It should either be imported
from there or some other common location.

+# The pxelinux.cfg path is prefixed with the architecture for the
+# PowerNV nodes. This prefix is set by the path-prefix dhcpd option.
+# We assume that the ARP HTYPE (hardware type) that PXELINUX sends is
+# always Ethernet.
+re_config_file = r'''
+ # Optional leading slash(es).
+ ^/*
+ ppc64el # PowerNV pxe prefix, set by dhcpd
+ /
+ pxelinux[.]cfg # PXELINUX expects this.
+ /
+ (?: # either a MAC
+ {htype:02x} # ARP HTYPE.
+ -
+ (?P<mac>{re_mac_address.pattern}) # Capture MAC.
+ | # or "default"
+ default
+ )
+ $
+'''

[3]

This is very similar to the re_config_file in pxe.py. Perhaps a common
template could be used to generate them?

+re_config_file = re_config_file.format(
+ htype=ARP_HTYPE.ETHERNET, re_mac_address=re_mac_address)
+re_config_file = re.compile(re_config_file, re.VERBOSE)

[4]

Copied from boot.py - should be common.

[5]

+ def get_reader(self, backend, kernel_params, **extra):
+ """Render a configuration file as a unicode string.
+
+ :param backend: requesting backend
+ :param kernel_params: An instance of `KernelParameters`.
+ :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.
+ """
+ # Due to the path prefix, all requested files from the client will
+ # contain that prefix. Lets return the correct file, removing the
+ # prefix from the path
+ if 'path' in extra:
+ path = extra['path']
+ path = path.replace(self.path_prefix, '', 1)
+ target_path = backend.base.descendant(path.split('/'))
+ return FilesystemReader(target_path)
+
+ # Return empty config for PowerNV local. PowerNV fails to
+ # support the LOCALBOOT flag. Empty config will allow it
+ # to select the first device.
+ if kernel_params.purpose == 'local':
+ return BytesReader("".encode("utf-8"))
+
+ template = self.get_template(
+ kernel_params.purpose, kernel_params.arch,
+ kernel_params.subarch)
+ namespace = self.compose_template_namespace(kernel_params)

Do we ever get to these last lines? Is this missing a return statement?
Can the same lines in pxe.py be factored to a common location?

[7]
+ def install_bootloader(self, destination):
+ """Does nothing. No extra boot files are required. All of the boot
+ files from PXEBootMethod will suffice."""

It seems fragile for PowerEnv to implicitly depend on PXEBootMethod's
behavior rather than explicitly in code identifying either the
dependency on PXEBootMethod or the files it installs.

[8]

+def get_example_path_and_components():
+ """Return a plausible path and its components.
+
+ The path is intended to match `re_config_file`, and the components are
+ the expected groups from a match.
+ """
+ components = {"mac": factory.getRandomMACAddress("-")}
+ config_path = compose_config_path(components["mac"])
+ return config_path, components

I've been told the MAAS convention is to prefix factory methods with
with 'make' rather than 'get'.