Merge lp:~racb/maas/multi-soc-ephemerals into lp:maas/trunk

[Robie Basak]

Add multi-SoC support to ephemeral image import

If ephemeral image tarballs contain a subarch/ directory, then import the
linux and initrd.gz files for each subarch as TFTP commissioning images into
MAAS.

If subarch/ does not exist, behave exactly the same as before.

This allows armhf SoCs that require different kernels to all be supported
by a single armhf ephemeral image with multiple kernels installed within it.

I'd like this to end up in a Precise SRU, so that we can enable other ARM server systems based on non-highbank SoCs as they become available.

There are no flag days. We can generate ephemeral images with subarch/ directories whenever we need. When a MAAS installation uses the updated import script and the ephemeral image has a subarch/ directory, it'll all just start working.

[Robie Basak (racb)]

Example of subarch ephemeral image entries:

quantal-ephemeral-maas-armhf-initrd
quantal-ephemeral-maas-armhf-vmlinuz
quantal-ephemeral-maas-armhf.img
subarch/
subarch/armadaxp/
subarch/armadaxp/initrd.gz
subarch/armadaxp/linux
subarch/highbank/
subarch/highbank/initrd.gz
subarch/highbank/linux

I went with initrd.gz as this is what seems to be used internally within maas-import-ephemerals. However, note that the expectation of a file ending in ".gz" is that gunzip will work on it, but in the case of armadaxp, there is an extra "uInitrd" wrapper (created by u-boot's mkimage) that breaks this expectation. Using "initrd" instead of "initrd.gz" universally would be better, but it is possibly not practical to make this change everywhere now.

The alternative would be to ship it as uInitrd and uImage inside subarch/armadaxp and have maas-import-ephemerals rename it as needed.

[Gavin Panella (allenap)]

Thanks for doing this Robie. However, it's untested, and shell script. I'm very reluctant to +1 any more shell script into production parts of MAAS, tested or not, but without tests it's got no hope. Can you move this logic into Python - or just about anything other than shell script - such that it can also be tested without having to drive pencils into our temples?

review: Needs Fixing

[Scott Moser (smoser)]

some things:
 * use more quotes. example 'cp $src/subarch/$subarch/initrd.gz' needs to be quoted to avoid shell expansion (space in 'src' for example).
 * generally i think that $(subcmd) is used here rather than `subcmd`. just try to keep consistent.
 * uniq_major_arches uses global 'arch' destroying its contents. needs to declare 'local' for arch.
 * subarches needs 'local' on variables.
 * 'mv' on line 40 can be made (i think) to avoid the race condition of "check then move".
   see '--target-directory' and '--no-target-directory'

generally looks reasonable and most of hte above are nitpick. and i'm not opposed to writing these things in python.

[Gavin Panella (allenap)]

Despite the dread I feel, I'll +1 this because we don't have the means
right now to refactor with confidence, nor do we have the time to go
through indeterminable QA iterations after a rewrite (it might be fine
on the first go, but we can't have much confidence in that).

I assume this will be well exercised before it gets SRU'd?

[1]

+    for arch in "$@"; do echo $arch; done|cut -f1 -d/|uniq

You could save a process here:

    for arch in "$@"; do echo "${arch%%/*}"; done | uniq

[2]

+            $major_arch/*) echo "$candidate"|cut -f2 -d/ ;;

Similar thing to [1] here:

            $major_arch/*) echo "${candidate#*/}" ;;

[3]

+    for arch in `uniq_major_arches $ARCHES`; do
...
+        for subarch in `subarches $arch $ARCHES`; do

Beware that errors from these new functions won't do anything; command
substitution hides errors. I think they're simple enough that won't
fail, but it's worth knowing for the future.

[4]

+    for arch in "$@"; do echo $arch; done|cut -f1 -d/|uniq

Fwiw, if $arch is -e, -E or -n, echo will absorb it as flag. Afaik
there's no way to avoid this, other than to use printf:

    for arch in "$@"; do printf "%s\n" $arch; done|cut -f1 -d/|uniq

Which gives me an idea of how to make this simpler:

    printf "%s\n" "$@" | uniq

review: Approve

[lp:~racb/maas/multi-soc-ephemerals]

1355. By Robie Basak on 2012-11-23

Quote all shell variables

This excludes ARCHES, as it is whitespace-separated. As a result, any
arch with a space in the name is broken anyway.

1356. By Robie Basak on 2012-11-23

Use $() instead of ``

1357. By Robie Basak on 2012-11-23

Declare local variables to avoid clobbering globals

1358. By Robie Basak on 2012-11-23

Use parameter expansions instead of cut

[Robie Basak (racb)]

Thanks for the reviews!

I've made all requested changes except:

Gavin> printf; as discussed, we couldn't find this use documented anywhere so I have left the echo instead.

Scott> "check then move" race; this race exists for all the other checks and moves in the script already, and I didn't want to break the existing pattern as I felt that would make the code less readable for no real gain. Multiple concurrent calls to maas-import-ephemerals are already broken anyway, so I'm not sure that we need to care about that race. I'll merge unless you object?

[Julian Edwards (julian-edwards)]

JFLI!

=== modified file 'scripts/maas-import-ephemerals'
--- scripts/maas-import-ephemerals	2012-11-08 14:37:46 +0000
+++ scripts/maas-import-ephemerals	2012-11-23 11:03:21 +0000
@@ -91,6 +91,32 @@
 }
 
 
+uniq_major_arches() {
+    # print on stdout a uniq set of arches out of the list of arch/subarch
+    # pairs supplied in $@
+    # Eg. armhf/highbank armhf/armadaxp i386/generic amd64/generic ->
+    #           "armhf\ni386\namd64\n"
+    local arch
+    for arch in "$@"; do echo "${arch%%/*}"; done|uniq
+}
+
+
+subarches() {
+    # print on stdout a list of subarches available for a given major arch $1
+    # given a list of arch/subarch pairs in $2-
+    # Eg. armhf armhf/highbank armhf/armadaxp i386/generic ->
+    #           "highbank\narmadaxp\n"
+    local major_arch="$1" candidate
+    shift
+    for candidate in "$@"; do
+        case "$candidate" in
+            "$major_arch"/*) echo "${candidate#*/}" ;;
+            *) ;;
+        esac
+    done
+}
+
+
 query_remote() {
     # query /query data at CLOUD_IMAGES_ARCHIVE
     # returns 7 values prefixed with 'r_'
@@ -238,6 +264,9 @@
     [ -z "$initrd" ] || mv "$initrd" "$wd/initrd.gz" ||
         { error "failed to move extracted initrd to $wd/initrd.gz"; return 1; }
 
+    [ ! -d "$exdir/subarch" ] || mv "$exdir/subarch" "$wd/" ||
+        { error "failed to move extracted subarch to $wd/subarch"; return 1; }
+
     rm -Rf "$exdir" || { error "failed to cleanup extraction dir"; return 1; }
     { [ -z "$rmtar" ] || rm "$rmtar"; } ||
         { error "failed to remove temporary tarball $rmtar"; return 1; }
@@ -304,10 +333,16 @@
     # deletes it.
     tmpdir="$(mktemp -d)"
 
-    copy_first_available "$src/linux" "$src/kernel" "$tmpdir/linux" ||
-        return 1
-    copy_first_available "$src/initrd.gz" "$src/initrd" "$tmpdir/initrd.gz" ||
-        return 1
+    if [ -f "$src/subarch/$subarch/linux" -a \
+            -f "$src/subarch/$subarch/initrd.gz" ]; then
+        cp "$src/subarch/$subarch/linux" "$tmpdir/linux" || return 1
+        cp "$src/subarch/$subarch/initrd.gz" "$tmpdir/initrd.gz" || return 1
+    else
+        copy_first_available "$src/linux" "$src/kernel" "$tmpdir/linux" ||
+            return 1
+        copy_first_available "$src/initrd.gz" "$src/initrd" "$tmpdir/initrd.gz" ||
+            return 1
+    fi
 
     local cmd out=""
     cmd=( maas-provision install-pxe-image
@@ -361,10 +396,7 @@
 
 updates=0
 for release in $RELEASES; do
-    for arch_tuple in $ARCHES; do
-        arch="${arch_tuple%%/*}"
-        subarch="${arch_tuple#*/}"
-
+    for arch in $(uniq_major_arches $ARCHES); do
         query_local "$arch" "$release" "$BUILD_NAME" ||
             fail "failed to query local for $release/$arch"
         query_remote "$arch" "$release" "$BUILD_NAME" ||
@@ -405,11 +437,13 @@
             name="${l_name}"
         fi
 
-        # Even if there was no need to update the image, we make sure it
-        # gets installed.
-        debug 1 "adding images for $release/$arch/$subarch to maas"
-        install_tftp_image "$fpfinal_d" "$arch" "$subarch" "$release" ||
-            fail "failed to install ftp image [$info]"
+        for subarch in $(subarches "$arch" $ARCHES); do
+                # Even if there was no need to update the image, we make sure
+                # it gets installed.
+                debug 1 "adding images for $release/$arch/$subarch to maas"
+                install_tftp_image "$fpfinal_d" "$arch" "$subarch" "$release" ||
+                    fail "failed to install tftp image [$info]"
+        done
 
         target_name="${TARGET_NAME_PREFIX}${name}"
         rel_tgt="../${final_d}/tgt.conf"