Merge lp://qastaging/~mvo/snappy/maybe-move-syncbootfiles into lp://qastaging/~snappy-dev/snappy/snappy-moved-to-github

I'm +1 on this, if it always needs to be done and nothing intermediate can be done with the resulting call, it should be internal. Thanks

This is wrong I'm afraid: bootloader.SyncBootFiles() must be called *before* DownloadUpdate() - see the comment above the call to s.partition.SyncBootloaderFiles() in systemimage.go:Install().

Essentially, the code needs to copy the existing kernel+initrd+dtb files when preparing to switch rootfs. This must happen before s-i is involved to guarantee that there actually is a kernel+initrd to boot the "other" rootfs from (since although both partitions are provisioned on a u-boot system, only /boot/uboot/a/ is populated. And if "snappy update" does _not_ install newer kernel/initrd/dtb files, the system wouldn't boot.

If we do the copy afterwards (as this branch does), we will silently switch back to the old kernel+initrd+dtb.

FTR, I'm not against making the calls internal, but the implementation here would break u-boot systems.

Given jodh's comment, that should be set as a code comment and the reordering as well.

Thanks for the review! Great that this was found before it could do harm.

Thanks again, I'm having trouble understanding the issue here right now, could you help me?

The way I read the code in toggleBootloaderRootfs() (which is called indirectly from systemimage.go via ToggleNextBoot) is this:

1 stuff is downloaded via proxy.DownloadUpdate() and the ubuntu-core-upgrader runs
2 SyncBootFiles is called and it copies /boot/uboot/a -> /boot/uboot/b
3 ToggleRootFS() is called and that switches to "b"
4 HandleAssets is called which looks at /writable/cache/hardware.yaml and may override /boot/uboot/b/ again.

In the current code (1) and (2) are switched, what is happening in (1) that breaks (2).

My understanding was that HandleAssets() in (4) would copy the downloaded stuff from (1) into /boot/uboot/b/ (if there is anything). So I guess my disconnect is that I don't quite understand what is happening in (1), i.e. whats the role of s-i (or maybe ubuntu-core-upgrader) that breaks when SyncBootFiles() is not called before s-i is run?

s.partition.SyncBootloaderFiles() must happen before s.proxy.DownloadUpdate() because there is no guarantee that an s-i update will include a new kernel+initrd combo.

The current code does effectively:

1) cp /boot/uboot/a/* to /boot/uboot/b/.
2) download, and unpack new s-i.
3) toggle bootloader config to boot from 'b'.

So, if (2) does *NOT* include a kernel+initrd, the system will still boot 'b' using the original kernel+initrd. If we remove (1), the system is unbootable in that scenario. But if (2) does include a new kernel+initrd, great - that gets installed by HandleAssets().

But imagine we changed the logic to:

1) download, and unpack new s-i.
2) cp /boot/uboot/a/* to /boot/uboot/b/.
3) toggle bootloader config to boot from 'b'.

If the new s-i in (1) did contain a new kernel+initrd, you've just overwritten them with the old ('a' partition) versions.

Aha, my understanding was that the copy to /boot/uboot/b happens in handleAssets which is called after syncbootloader files. Is that where my disconnect is? I. E. Ubuntu-core-upgrader is what copies kernel,initrd to /boot/uboot/b?

On 18 February 2015 18:41:32 CET, James Hunt <email address hidden> wrote:
>s.partition.SyncBootloaderFiles() must happen before
>s.proxy.DownloadUpdate() because there is no guarantee that an s-i
>update will include a new kernel+initrd combo.
>
>The current code does effectively:
>
>1) cp /boot/uboot/a/* to /boot/uboot/b/.
>2) download, and unpack new s-i.
>3) toggle bootloader config to boot from 'b'.
>
>So, if (2) does *NOT* include a kernel+initrd, the system will still
>boot 'b' using the original kernel+initrd. If we remove (1), the system
>is unbootable in that scenario. But if (2) does include a new
>kernel+initrd, great - that gets installed by HandleAssets().
>
>But imagine we changed the logic to:
>
>1) download, and unpack new s-i.
>2) cp /boot/uboot/a/* to /boot/uboot/b/.
>3) toggle bootloader config to boot from 'b'.
>
>If the new s-i in (1) did contain a new kernel+initrd, you've just
>overwritten them with the old ('a' partition) versions.
>
>
>
>
>
>
>
>--
>https://code.launchpad.net/~mvo/snappy/maybe-move-syncbootfiles/+merge/249832
>You are the owner of lp:~mvo/snappy/maybe-move-syncbootfiles.

--
Sent from my Android phone with K-9 Mail. Please excuse my brevity.

HandleAssets copies any newer kernel/initrd/dtb files after the update *if* the new s-i provides them. So, there are potentially 2 copies but the first one has to happen before the upgrade in case there is nothing for HandleAssets to do.

Right, the SyncBootFiles() call is still there and it will copy /boot/uboot/a -> /boot/uboot/b so if there is no new kernel the one from /boot/uboot/a will be used. Its just partition-internal now and the order is slightly different, i.e. its run after the download, but AIUI the download itself will not modify /boot/uboot/{a,b} so that would be ok? Then there is handleAssets which is also run and may override the /boot/uboot/b kernel with a newer version, that has not changed.

After a verbal chat, the confusion has now been cleared... :-)

I've just reviewed this code a little more carefully, and it is in fact correct. Although the SyncBootFiles() happens after the upgrader has run, that is actually safe since the upgrader doesn't modify the boot partition (currently). Hence, we can legitimately call:

1) upgrader.
2) SyncBootFiles().
3) HandleAssets().

I still feel that from a defensive programming perspective though, it is safer to handle the sync before the upgrader runs (at the expense of a single export), just in case the behaviour of the upgrader changes (say if we ever support in-place upgrades). However, given sufficiently good tests [1], such a change should be catchable to ensure such a bug doesn't escape into the wild.

[1] - that checksum the assets on the boot partition and asserts that those relating to the "current" rootfs haven't changed post-upgrade.