Merge lp://staging/~ltrager/curtin/lp1640519 into lp://staging/~curtin-dev/curtin/trunk

Thanks for tackling this Lee!

I do fear that the strategy of copying flash-kernel code won't be very maintainable, as the platforms supported by a given flash-kernel version and the curtin in-use will quickly fall out of sync. Note that flash-kernel is a package that is frequently updated, even in stable releases.

My recommendation would be to keep flash-kernel installed on all arm64 systems. This should be safe - flash-kernel now knows to ignore uefi-based systems, and those are the only supported systems that do not require flash-kernel.

Alternatively, I'd suggest querying flash-kernel at runtime by sourcing it's /usr/share/flash-kernel/functions and using get_machine()/check_supported() to determine if it is supported by flash-kernel. This would avoid the overhead of having flash-kernel installed on UEFI systems. Though, that overhead is so low that I personally wouldn't bother.

\o/ MP to review! Thanks Lee!

I'd like some unittest for the machine_uses_uboot,

Something else to think about is that curthooks methods are *directly* available to
writers of curthooks in custom images, so I'd like to name it _machine_uses_uboot.

I dislike re-implementing this in python when the shell code is going to be present so we can get an up-to-date list of machines, but flash-kernel doesn't appear to expose the many subcommands that it uses.

Thanks for the reviews! I took Ryan's suggestion to install flash-kernel in the ephemeral environment during detection. It only does this if the system architecture contains 'arm' and is not UEFI.

Thanks Lee - I agree that querying flash-kernel would be a more robust solution. I'm not sure which sets of problems you are trying to solve, and which ones are ancillary, so here's some more background/questions.

flash-kernel installs a kernel hook that runs /usr/sbin/flash-kernel on every kernel upgrade. This has caused a problem in the past, because flash-kernel will return an error on systems it does not support, causing the apt upgrade process to error. We've therefore historically had code that tries to recognize the system, and only install flash-kernel if it should run. In short - flash-kernel was the problem there, not u-boot-tools. u-boot-tools can be installed on any system innocuously. If that's the problem you are trying to address (which I don't believe is a problem anymore), then I'd suggest removing flash-kernel after this test.

Today, installing flash-kernel and/or u-boot-tools on any MAAS-supported system should be fine. The only reason I see to selectively install u-boot-tools is to save install space. If space is a concern, then this helps to address that. You could also remove flash-kernel if unneeded to save more space.

If space isn't an issue, and we believe that installing flash-kernel everywhere is safe these days, then the simplest fix might be to just install flash-kernel/u-boot-tools on all arm systems.

If you do stick with the system-matching approach, you might want to consider a more generic "optional package" install method than "if flash-kernel-supported then install u-boot-tools". flash-kernel can tell you what additional packages it requires on the target platform:

machine="$(get_cpuinfo_hardware)"
get_machine_field "$machine" "Required-Packages"

This way you would only install u-boot-tools on platforms that really need it, and also support other required packages than u-boot-tools.

On Mon, Nov 14, 2016 at 1:29 PM, dann frazier <email address hidden>
wrote:

> Thanks Lee - I agree that querying flash-kernel would be a more robust
> solution. I'm not sure which sets of problems you are trying to solve, and
> which ones are ancillary, so here's some more background/questions.
>
> flash-kernel installs a kernel hook that runs /usr/sbin/flash-kernel on
> every kernel upgrade. This has caused a problem in the past, because
> flash-kernel will return an error on systems it does not support, causing
> the apt upgrade process to error. We've therefore historically had code
> that tries to recognize the system, and only install flash-kernel if it
> should run. In short - flash-kernel was the problem there, not
> u-boot-tools. u-boot-tools can be installed on any system innocuously. If
> that's the problem you are trying to address (which I don't believe is a
> problem anymore), then I'd suggest removing flash-kernel after this test.
>
> Today, installing flash-kernel and/or u-boot-tools on any MAAS-supported
> system should be fine. The only reason I see to selectively install
> u-boot-tools is to save install space. If space is a concern, then this
> helps to address that. You could also remove flash-kernel if unneeded to
> save more space.
>
> If space isn't an issue, and we believe that installing flash-kernel
> everywhere is safe these days, then the simplest fix might be to just
> install flash-kernel/u-boot-tools on all arm systems.
>

Are there arm systems which *dont* or *wont* use u-boot-tools and
flash-kernel? If so, are they certified for MAAS? Unless we have a
special case, I'd push for just always installing it.

>
> If you do stick with the system-matching approach, you might want to
> consider a more generic "optional package" install method than "if
> flash-kernel-supported then install u-boot-tools". flash-kernel can tell
> you what additional packages it requires on the target platform:
>
> machine="$(get_cpuinfo_hardware)"
> get_machine_field "$machine" "Required-Packages"
>
> This way you would only install u-boot-tools on platforms that really need
> it, and also support other required packages than u-boot-tools.
>

Yeah, that's nicer I think. Will "Requires-Packages" list if flask-kernel
is required? If so, then it does look reasonably tidy

1) install flash kernel in the ephemeral
2) run flash-kernel to detect supported machine
3) if supported, install packages in "Required-Packages" list in-target

It is somewhat unfortunate that the ephemeral environment for arm* doesn't
have flash-kernel always installed since it'd be nice
to not have to download and install that each deploy. But maybe that's a
separate bug once we get this one solved.

--
> https://code.launchpad.net/~ltrager/curtin/lp1640519/+merge/310604
> Your team curtin developers is requested to review the proposed merge of
> lp:~ltrager/curtin/lp1640519 into lp:curtin.
>

Thanks for the updated review. On deployments I think its better for Curtin/MAAS to only install packages in the target install that are really needed. As such sticking with using flash-kernel to detect if additional dependencies are needed seems to be the better path. I've updated the MP to ask flash-kernel what packages are needed for the target machine instead of just assuming that u-boot-tools are always needed. Flash kernel doesn't list itself as a needed dependency so that is now also installed in the target system.

The ephemeral environment that MAAS uses is now the image that is being deployed. That image is the one that is published at cloud-images.ubuntu.com. My understanding is that we don't want flash-kernel in that image which is why Curtin has to install it in the ephemeral environment.

It seems like we are missing the case where flash-kernel is required, but no additional required packages are needed. This would be when flash-kernel's check_supported() returns 0 but get_machine_field('Required-Packages') returns non-zero.

On Tue, Nov 15, 2016 at 2:10 PM, dann frazier <email address hidden>
wrote:

>
>
> Diff comments:
>
> > === modified file 'curtin/commands/curthooks.py'
> > --- curtin/commands/curthooks.py 2016-08-22 16:20:23 +0000
> > +++ curtin/commands/curthooks.py 2016-11-15 19:48:03 +0000
> > @@ -173,6 +173,32 @@
> > mapping = copy.deepcopy(KERNEL_MAPPING)
> > config.merge_config(mapping, kernel_cfg.get('mapping', {}))
> >
> > + # Machines using flash-kernel may need additional dependencies
> installed
> > + # before running. Run those checks in the ephemeral environment so
> the
> > + # target only has required packages installed. See LP:1640519
> > + if not util.is_uefi_bootable() and 'arm' in util.get_architecture():
> > + util.install_packages(['flash-kernel'])
> > + try:
> > + fk_packages, _ = util.subp(
> > + [
> > + 'export FK_DIR=/usr/share/flash-kernel;'
> > + '. ${FK_DIR}/functions;'
> > + 'check_supported;'
>
> check_supported requires $machine as an argument. I don't know how
> util.subp() works - but, if this is all just running in a shell, I don't
> see how "check_supported"'s result is being checked anywhere.
>
> TBH, I'd expect 3 separat util.subp calls here. One to get the machine.
> One to check_supported $machine. Then, if supported, one to
> get_machine_field.
>

It can run shell code; so we should just write it inline in shell and use
textwrap.dedent()

fk_script = textwrap.dedent("""
export FK_DIR=/usr/share/flask-kernel
source ${FK_DIR}/functions
machine="$(get_cpuinfo_hardware)";
check_supported $machine || exit 1
get_machine_field "${machine}" "Required-packages" ||:;
""")

Then call that with sh or bash:

out, err = util.subp(['sh', '-c', fk_script], capture=True)

>
> > + 'machine="$(get_cpuinfo_hardware)";'
> > + 'get_machine_field "${machine}" "Required-packages"
> ||:;'
> > + ],
> > + capture=True, shell=True)
> > + except util.ProcessExecutionError:
> > + # check_supported gives a non-zero return code when the
> machine
> > + # isn't supported by flash-kernel. get_machine_field also
> returns
> > + # a non-zero return code when no additional packages are
> required.
> > + # It's ignored so flash-kernel is installed if its
> supported even
> > + # if it doesn't require additional packages.
> > + pass
> > + else:
> > + util.install_packages(
> > + ['flash-kernel'] + fk_packages.split(), target=target)
> > +
> > if kernel_package:
> > util.install_packages([kernel_package], target=target)
> > return
>
>
> --
> https://code.launchpad.net/~ltrager/curtin/lp1640519/+merge/310604
> Your team curtin developers is requested to review the proposed merge of
> lp:~ltrager/curtin/lp1640519 into lp:curtin.
>

Thanks for catching the missing argument. I fixed it and tested it on hardware today. flash-kernel and u-boot-tools are now both installed before the kernel installation runs. flash-kernel was actually already in the image, only u-boot-tools was missing.

The shell=True argument of util.subp works similar to what you are describing. The arguments are executed as a script and like a script they stop executing as soon as they encounter a non-zero return code. This raises an exception and the installation is skipped.

So the solution here is to install falsh-kernel in the host in order to determine if the guest needs it.

Thats less than ideal in that we leave a change on the host. It happens to not matter much to maas's use case, since the host is ephemeral. The second thing about that, is that it probably makes sense to do that up front as other dependencies are handled. see curtin/deps/ for how that is done, and ultimately i think we'd want the dependency added to the packaging too.

i know thats more painful than you'd like.

there is one change necessary below, you're not determining the difference between failure and success.

One other comment, seems like it would be useful to have flash-kernel actually more straight forwardly support this.

flash-kernel list-needed-if-necessary

or

flash-kernel install-if-necessary

something like that.

Scott, thanks for the review. I realized that flash-kernel is already in the base image from CPC. While that means we won't be installing it I added it to curtin/deps as you suggested. I don't think we can put a package dependency on flash-kernel as its only available on ARM right now. I've also moved the flash-kernel script into its own helper as you suggested.

I'm hesitant to modify flash-kernel right now as it will mean backporting to all supported releases and adding versioning to curtin/deps.

Not being able to use something everywhere right now is not a reason for not fixing a bug.

We don't have to use it, but someone else could. And in two years no one will care about xenial

On November 16, 2016 6:08:02 PM EST, Lee Trager <email address hidden> wrote:
>Scott, thanks for the review. I realized that flash-kernel is already
>in the base image from CPC. While that means we won't be installing it
>I added it to curtin/deps as you suggested. I don't think we can put a
>package dependency on flash-kernel as its only available on ARM right
>now. I've also moved the flash-kernel script into its own helper as you
>suggested.
>
>I'm hesitant to modify flash-kernel right now as it will mean
>backporting to all supported releases and adding versioning to
>curtin/deps.
>--
>https://code.launchpad.net/~ltrager/curtin/lp1640519/+merge/310604
>You are reviewing the proposed merge of lp:~ltrager/curtin/lp1640519
>into lp:curtin.

I changed the shebang to /bin/sh and fixed the tests as requested.

I've also posted https://code.launchpad.net/~ltrager/flash-kernel/get_machine_deps/+merge/311210 to add the machine deps as a command line option to flash-kernel.

I ended up running into another bug while testing. Curtin updates the initrd a second time on ARM but wasn't mounting /dev which caused update-initramfs to fail. Instead of calling util.subp update_initramfs now uses util.ChrootableTarget which automatically mounts /dev, /sys, and /proc. After making that change I was able to deploy Xenial on an ARM host.

> I ended up running into another bug while testing. Curtin updates the initrd a
> second time on ARM but wasn't mounting /dev which caused update-initramfs to
> fail. Instead of calling util.subp update_initramfs now uses
> util.ChrootableTarget which automatically mounts /dev, /sys, and /proc. After
> making that change I was able to deploy Xenial on an ARM host.

Nice catch! That warrants another unittest in the commands curthooks.

Updated to test update_initramfs. Let me know if we need anything else before landing.

Thanks for updating.

I've added https://code.launchpad.net/~ltrager/flash-kernel/get_machine_deps/+merge/311210 to remove u-boot-tools from zesty+ images even for v2.

I think we want to add 'flash-kernel' to debian/control's Depends, as
  flash-kernel [arm64 armhf]

I think thats the right syntax. more info at https://www.debian.org/doc/debian-policy/ch-relationships.html

The first comment below is really minor... but changing the way that works makes unit test easier.

I've added get_flash_kernel_pkgs as you suggested and updated the unit tests. I also looked into adding flash-kernel to debian/control's Depends but kept getting errors that I can't add arch requirements on an all arch package. From a MAAS point of view the package depends doesn't really matter, its installed on the region and given to the target machine over the metadata service.

yeah, i understand the arch-all thing. that does make sense as Depends is generated at build-time, not evaluated at run-time. So our options would then be to make different arch packages or live with it. :-(.

From MAAS point of view, 'curtin' isnt actually even installed, just the python-curtin or python3-curtin. the idea was that:
  apt-get install curtin
woudl get you all your dependencies needed to run curtin
but just getting python-curtin doesnt really get you dependencies at all.

So, I guess for the moment we live with it.

Thanks for the changes.
I'm ok with this as it is, but one comment in line.

I've updated get_flash_kernel_pkgs to accept arch and uefi as arguments to allow for easier testing and updated the tests to use the new functionality.

looks great. thank you.