Merge lp:~jamesodhunt/snappy/bug-1412737 into lp:~snappy-dev/snappy/snappy-moved-to-github

[James Hunt]

Make snappy on grub systems detect a broken boot and fall back to the other root partition on power cycling.

Now that we have a single shared boot partition for grub, we can use it
to store bootloader variables that store state (as is already done for
snappy u-boot systems).

Further, we no longer need to re-install grub from within the chroot to
tell grub which rootfs to boot from since the variables suffice.

Note though that snappy still needs to call update-grub from within the
chroot since that chroot has the correct grub config files for that
particular rootfs (/etc/grub.d/*, etc).

Since the grub code is run from within a chroot (where the "other"
rootfs is mounted), it might _appear_ that the logic is inverted, but
this is not the case since the grub snippet queries the chroot
environment only to set up the grub menu (the grub snippet should work
correctly outside a chroot, although it would not be valid to do this on
a snappy system).

Specifically, the grub snippet performs 2 functions:

1) Create the grub menuentry's for the 2 rootfs's *at update-grub time*
   and within the context of the "other" chroot.

2) Arrange for grub to toggle the rootfs *at boot time*, based on the
   value of the grub environment block variables.

Remaining work:

- update the logic in ubuntu-core-snappy to also mount the current rootfs r/o at
  /writable/cache/system within the chroot (since we are planning to ship 2 implementations
  of snappy for the next release and the behaviour needs to match).

_________

* etc/grub.d/09_snappy: Grub snippet that:
  - Creates menuentry's for both rootfs's.
  - Boots using the rootfs specified by the snappy_ab variable
    (which replaces the boot-by-uuid config auto-generated by
     grub-install [which is no longer called]).
  - Toggles the rootfs if the last boot failed (LP: #1412737).
* partition/bootloader_grub.go:
  - ToggleRootFS: Remove call to grub-install now that
    ubuntu-device-flash creates the shared boot partition for grub.
* partition/partition.go:
  - bindmountRequiredFilesystems(): Call new function
    bindmountThisRootfsRO() so that menu entries can be generated for
    for both rootfs's on grub systems.

[James Hunt (jamesodhunt)]

It is also worth pointing out here that the included etc/grub.d/09_snappy makes /etc/grub.d/10_linux redundant. The fact that 10_linux will be run as part of update-grub does not cause problems - it simply creates an additional menuentry in /boot/grub/grub.cfg. So we could consider removing /etc/grub.d/10_linux from the images via a follow-on livecd-rootfs change.

[James Hunt (jamesodhunt)]

Changes for the snappy python implementation are here:

lp:~jamesodhunt/ubuntu/vivid/ubuntu-core-snappy/mount-current-rootfs-in-chroot-for-bug-1412737 (I'll raise an MP once this branch lands).

[Michael Vogt (mvo)]

Just a quick note without having looked at the code itself yet.

I feel a bit uneasy about the amount of code in 09_snappy that looks like its duplication from 10_linux. I wonder if there is anything we could do here? Maybe share code with 10_linux, maybe even generate from snappy-go directly instead of via shell (snappy-go knows about the partition layout for example already).

The other thing that does not feel ideal is the fact that we chroot and then bindmount the original back. The chroot also seems to not allow lsblk to work. I wonder if we could do something like make update-grub honor a GRUB_SYSCONF_DIR=/path/to/other/etc environment.

This may all be crazy, but I wanted to throw it out as ideas anyway :)

But in any case, merging this into trunk and running:
$ ./run-checks
fails for me so that needs addressing.

[James Hunt (jamesodhunt)]

Hi Michael,

> I feel a bit uneasy about the amount of code in 09_snappy that looks
> like its duplication from 10_linux.
> I wonder if there is anything we
> could do here? Maybe share code with 10_linux,
Well, yes 09_snappy is currently a very cleaned up version of 10_linux :)

I understand the concerns. Regarding 09_snappy and 10_linux, my plan is
to do the following (and get it upstreamed) to minimise the delta we
have to maintain:

1) Move the functions in 09_snappy to /usr/lib/grub/grub-mkconfig_lib
   (or maybe /usr/lib/grub/grub-lib-linux as it is linux-specific).
2) Add linux_entry() to grub-lib-linux.
3) Make linux_entry() call linux_entry_ext().
4) Rework 10_linux to call the functions in grub-lib-linux.

Assuming this can be upstreamed, we could then collapse 09_snappy down
an ~75 line script.

> maybe even generate from
> snappy-go directly instead of via shell (snappy-go knows about the
> partition layout for example already).
I'm not super happy about the amount of shell in grub either tbh.
However, that is the current standard it seems and I'm trying to conform
to that. I like the idea of snappy-go generating the config but would
value input from others as to the safety of such an approach given the
number of other scripts and config files grub has here and there; if we
It feels rather dangerous to start generating the config entirely on our
own as we should be able to rely on grub library routines to handle any
internal grub changes. If we go it alone, that seems higher risk than
simply writing a new grub snippet such as this MP adds.

> The other thing that does not feel ideal is the fact that we chroot and
> then bindmount the original back.
Yes, the chroot'ing is a pita but is required to:

1) Ensure we're calling the correct grub commands for the "other"
   rootfs.
2) Ensure that the command(s) in (1) have the correct config
(/etc/grub.d/* and /etc/default/grub).

> The chroot also seems to not allow lsblk to work.
Right - I haven't looked at the lsblk source, but it fails in a chroot,
only reporting "/".

>I wonder if we could do something like make update-grub
> honor a GRUB_SYSCONF_DIR=/path/to/other/etc environment.
That would be neat if upstream would agree to that. Again, if we're only
going to make these changes locally for snappy, which is worse? A delta
on grub itself or a delta on a packaged grub config file? Also, would
that variable be sufficient (I don't know yet I'm afraid).

> This may all be crazy, but I wanted to throw it out as ideas anyway :)
Definately good to get it into the open! :)

> But in any case, merging this into trunk and running: $ ./run-checks
> fails for me so that needs addressing.
Oops, sorry! I still cannot run the snappy tests due to the click issue
so once the new versions lands in vivid I'll fix that.

I think that if this branch is unlikely to land today (and that's
looking very likely :-), that we should postpone landing it (in any
form) until after the next promotion.

[Michael Vogt (mvo)]

Just a quite note for the "./run-checks". The new click is in the beta PPA since yesterday. You can also run the partition tests by doing: "go test ./partition", this part of the code does not use click so that should work.

[Sergio Schvezov (sergiusens)]

I just want to note that etc/grub.d/09_snappy will be created in both variants (u-boot and grub) if it sits in here.

Also added a couple of inline comments.

review: Abstain

[James Hunt (jamesodhunt)]

Thanks - added rc check.

[Alexander Sack (asac)]

whats the state of this one? Are we doing this? Is this zombied?

[James Hunt (jamesodhunt)]

We still need the code to be reviewed by someone familiar with the grub package. The current behaviour:

If snappy fails to boot (hangs in early boot):

- uboot systems: will revert automatically to the "other" rootfs on next boot.

- grub systems: no rootfs switch happens on next boot due to this branch not having landed yet. A work-around currently would be enter the grub menu and modifying the cmdline to specify manually "root=system-a" or "root=system-b" (the user would need to try both as they won't know which is the current default).

Note that as well as bringing grub systems up to feature parity with uboot system, this MP also adds "boot by label" ("root=system-a") for simplicity.

[Colin Watson (cjwatson)]

The duplication isn't especially pleasant, but it also isn't a huge problem: this is well-contained within snappy and so isn't going to break anything else. And I certainly agree with James that generating this entirely from Go code and thus ignoring all of GRUB's library functions is unwise. The rest looks reasonable enough from a skim-read.

review: Approve

[Alexander Sack (asac)]

whats the plan for testing this? I think if we knew how to test this manually or even better automatically, we should land and get miles on this asap.

[James Hunt (jamesodhunt)]

A manual testcase is here: https://trello.com/c/GhuxMYQX/105-bootloader-failure-mode-testing

[Michael Vogt (mvo)]

Top-level approving based on the ACK from Colin.

[Snappy Tarmac (snappydevtarmac)]

Attempt to merge into lp:snappy failed due to conflicts:

text conflict in debian/ubuntu-snappy.install
text conflict in partition/bootloader_grub.go

[Sergio Schvezov (sergiusens)]

I don't see any a/b layout handling here, can you explain how that will work?

review: Needs Information

[James Hunt (jamesodhunt)]

That's handled at the very end of etc/grub.d/09_snappy (see "Toggle rootfs if previous boot failed.").

(I'm currently reworking the snappy-go changes on this branch so it's no longer ready to land, hence the status change).

[Sergio Schvezov (sergiusens)]

On jueves 26 de marzo de 2015 10h'33:50 ART, James Hunt wrote:
> That's handled at the very end of etc/grub.d/09_snappy (see
> "Toggle rootfs if previous boot failed.").
>
> (I'm currently reworking the snappy-go changes on this branch
> so it's no longer ready to land, hence the status change).

So the kernel lives in system-a and system-b and not system-boot?

[James Hunt (jamesodhunt)]

Currently snappy systems using grub mirror standard Ubuntu systems, so yes.

[James Hunt (jamesodhunt)]

Branch updated but not tested due to current devel-proposed issues.

[James Hunt (jamesodhunt)]

Tested .go changes performing a "snappy-go update".

[lp:~jamesodhunt/snappy/bug-1412737]

186. By James Hunt on 2015-03-27

* Sync with lp:snappy.

[Michael Vogt (mvo)]

I have some inline comments, but I think its more important to land this than to do any of this so I will approve and we can followup in a different MP.

review: Approve

=== modified file 'debian/ubuntu-snappy.install'
--- debian/ubuntu-snappy.install	2015-03-25 20:44:44 +0000
+++ debian/ubuntu-snappy.install	2015-03-27 08:58:19 +0000
@@ -1,2 +1,3 @@
 debian/*.service /lib/systemd/system/
+etc
 debian/*.timer /lib/systemd/system/

=== added directory 'etc'
=== added directory 'etc/grub.d'
=== added file 'etc/grub.d/09_snappy'
--- etc/grub.d/09_snappy	1970-01-01 00:00:00 +0000
+++ etc/grub.d/09_snappy	2015-03-27 08:58:19 +0000
@@ -0,0 +1,410 @@
+#!/bin/sh
+#---------------------------------------------------------------------
+# Summary: Grub bootloader logic for Ubuntu Snappy systems.
+# Description: This is a heavily modified "10_linux" grub snippet that
+#   deals with Snappy dual-rootfs systems.
+#
+# XXX: Note that this script is called from within a chroot environment
+# on snappy systems!
+#
+#---------------------------------------------------------------------
+
+set -e
+
+prefix="/usr"
+exec_prefix="/usr"
+datarootdir="/usr/share"
+
+# Utility functions
+. "${datarootdir}/grub/grub-mkconfig_lib"
+
+# Globals
+machine=`uname -m`
+
+SNAPPY_OS="Ubuntu Core Snappy"
+SNAPPY_TYPE=simple
+SNAPPY_ARGS="${GRUB_CMDLINE_LINUX} ${GRUB_CMDLINE_LINUX_DEFAULT}"
+
+#---------------------------------------------------------------------
+
+# Display message and exit
+die()
+{
+    msg="$*"
+    echo "ERROR: $msg" >&2
+    exit 1
+}
+
+# Create a grub menu entry by writing a menuentry to stdout.
+linux_entry_ext()
+{
+  local name="$1"
+  local os="$2"
+  local version="$3"
+  local type="$4"
+  local args="$5"
+  local device="$6"
+  local kernel="$7"
+  local initrd="$8"
+
+  local boot_device_id=
+  local prepare_root_cache=
+  local prepare_boot_cache=
+
+  if [ -z "$boot_device_id" ]; then
+      boot_device_id="$(grub_get_device_id "${device}")"
+  fi
+
+  echo "menuentry '$name' ${CLASS} \$menuentry_id_option 'gnulinux-simple-$boot_device_id' {" | sed "s/^/$submenu_indentation/"
+
+  if [ "$quick_boot" = 1 ]; then
+      echo "	recordfail" | sed "s/^/$submenu_indentation/"
+  fi
+  if [ x$type != xrecovery ] ; then
+      save_default_entry | grub_add_tab
+  fi
+
+  # Use ELILO's generic "efifb" when it's known to be available.
+  # FIXME: We need an interface to select vesafb in case efifb can't be used.
+  if [ "x$GRUB_GFXPAYLOAD_LINUX" = x ]; then
+      echo "	load_video" | sed "s/^/$submenu_indentation/"
+  else
+      if [ "x$GRUB_GFXPAYLOAD_LINUX" != xtext ]; then
+	  echo "	load_video" | sed "s/^/$submenu_indentation/"
+      fi
+  fi
+  if ([ "$ubuntu_recovery" = 0 ] || [ x$type != xrecovery ]) && \
+     ([ "x$GRUB_GFXPAYLOAD_LINUX" != x ] || [ "$gfxpayload_dynamic" = 1 ]); then
+      echo "	gfxmode \$linux_gfx_mode" | sed "s/^/$submenu_indentation/"
+  fi
+
+  echo "	insmod gzio" | sed "s/^/$submenu_indentation/"
+  echo "	if [ x\$grub_platform = xxen ]; then insmod xzio; insmod lzopio; fi" | sed "s/^/$submenu_indentation/"
+
+  # device may be a label (LABEL=name), so convert back to full path
+  label_name=$(echo "$device"|sed 's/^LABEL=//g')
+  if [ "$device" = "$label_name" ]
+  then
+    device_path="$device"
+  else
+    # found a label
+    device_path=$(get_partition_from_label "$label_name")
+  fi
+
+  if [ x$dirname = x/ ]; then
+    if [ -z "${prepare_root_cache}" ]; then
+
+      prepare_root_cache="$(prepare_grub_to_access_device ${device_path} | grub_add_tab)"
+    fi
+    printf '%s\n' "${prepare_root_cache}" | sed "s/^/$submenu_indentation/"
+  else
+    if [ -z "${prepare_boot_cache}" ]; then
+      prepare_boot_cache="$(prepare_grub_to_access_device ${device_path} | grub_add_tab)"
+    fi
+    printf '%s\n' "${prepare_boot_cache}" | sed "s/^/$submenu_indentation/"
+  fi
+
+  if [ x"$quiet_boot" = x0 ] || [ x"$type" != xsimple ]; then
+    message="$(gettext_printf "Loading Linux %s ..." ${version})"
+    sed "s/^/$submenu_indentation/" << EOF
+	echo	'$(echo "$message" | grub_quote)'
+EOF
+  fi
+
+    sed "s/^/$submenu_indentation/" << EOF
+	linux ${kernel} root=${device} ro ${args}
+EOF
+
+  if test -n "${initrd}"; then
+    # TRANSLATORS: ramdisk isn't identifier. Should be translated.
+    if [ x"$quiet_boot" = x0 ] || [ x"$type" != xsimple ]; then
+      message="$(gettext_printf "Loading initial ramdisk ...")"
+      sed "s/^/$submenu_indentation/" << EOF
+	echo	'$(echo "$message" | grub_quote)'
+EOF
+    fi
+    sed "s/^/$submenu_indentation/" << EOF
+	initrd ${initrd}
+EOF
+  fi
+
+  sed "s/^/$submenu_indentation/" << EOF
+}
+EOF
+}
+
+# Returns a list of the currently available kernels.
+# $1: If set, look for kernel below "$1/boot/".
+get_kernels()
+{
+    local prefix_dir="$1"
+    local list
+
+    case "x$machine" in
+    xi?86 | xx86_64)
+    list=`for i in $prefix_dir/boot/vmlinuz-* \
+                         $prefix_dir/vmlinuz-* \
+                         $prefix_dir/boot/kernel-* ; do
+                  if grub_file_is_not_garbage "$i" ; then echo -n "$i " ; fi
+              done` ;;
+    *) 
+    list=`for i in $prefix_dir/boot/vmlinuz-* \
+                         $prefix_dir/boot/vmlinux-* \
+                         $prefix_dir/vmlinuz-* \
+                         $prefix_dir/vmlinux-* \
+                         $prefix_dir/boot/kernel-* ; do
+                  if grub_file_is_not_garbage "$i" ; then echo -n "$i " ; fi
+         done` ;;
+    esac
+    echo "$list"
+}
+
+# Returns the path to the initrd for the kernel specified by $1.
+# $1: kernel version.
+# $2: directory to look in.
+get_initrd()
+{
+  local version="$1"
+  local dir="$2"
+
+  local alt_version=`echo $version | sed -e "s,\.old$,,g"`
+  local initrd=
+  local i=
+
+  for i in "initrd.img-${version}" "initrd-${version}.img" "initrd-${version}.gz" \
+	   "initrd-${version}" "initramfs-${version}.img" \
+	   "initrd.img-${alt_version}" "initrd-${alt_version}.img" \
+	   "initrd-${alt_version}" "initramfs-${alt_version}.img" \
+	   "initramfs-genkernel-${version}" \
+	   "initramfs-genkernel-${alt_version}" \
+	   "initramfs-genkernel-${GENKERNEL_ARCH}-${version}" \
+	   "initramfs-genkernel-${GENKERNEL_ARCH}-${alt_version}"; do
+    if test -e "${dir}/${i}" ; then
+      initrd="${dir}/${i}"
+      break
+    fi
+  done
+  echo "$initrd"
+}
+
+# Determine full path to disk partition given a filesystem label.
+get_partition_from_label()
+{
+    local label="$1"
+    local part=
+    local path=
+
+    [ -n "$label" ] || grub_warn "need FS label"
+
+    part=$(find /dev -name "$label"|tail -1)
+    [ -z "$part" ] && return
+    path=$(readlink -f "$part")
+    [ -n "$path" ] && echo "$path"
+}
+
+# Return the partition label for the given partition device.
+# $1: full path to partition device.
+get_label_from_device()
+{
+    local root="$1"
+
+    local label=
+    local std_label=
+    local label_rootfs=
+
+    for std_label in system-a system-b; do
+        label_rootfs=$(findfs "PARTLABEL=$std_label" 2>/dev/null || :)
+        if [ "$label_rootfs" = "$root" ]; then
+            label="$std_label"
+            break
+        fi
+    done
+
+    echo "$label"
+}
+
+# Return the full path to the device corresponding to the given
+# partition label.
+#
+# $1: partition label.
+get_device_from_label()
+{
+    local label="$1"
+    local device=
+
+    device=$(findfs "PARTLABEL=$label" 2>/dev/null || :)
+    echo "$device"
+}
+
+# Given a rootfs label, return the rootfs label corresponding to the
+# "other" rootfs partition.
+get_other_rootfs_label()
+{
+    local label="$1"
+
+    if [ "$label" = "system-a" ]; then
+        echo "system-b"
+    else
+        echo "system-a"
+    fi
+}
+
+# Given a mountpoint, return the corresponding device path
+# $1: mountpoint.
+get_device_from_mountpoint()
+{
+    local mountpoint="$1"
+    local device=
+
+    # XXX: Parse mount output rather than looking in /proc/mounts to
+    # avoid seeing the mounts outside the chroot.
+    device=$(/bin/mount | grep "^/dev/.* on ${mountpoint}[[:space:]]" 2>/dev/null |\
+        awk '{print $1}' || :)
+
+    echo "$device"
+}
+
+# Convert a partition label name to a menuentry name
+make_name()
+{
+    local label="$1"
+
+    echo "$SNAPPY_OS $label rootfs" | grub_quote
+}
+
+# Arrange for a grub menuentry to be created for the given partition.
+#
+# $1: full path to rootfs partition device
+# $2: partition label associated with $1
+# $3: mountpoint of $1.
+handle_menu_entry()
+{
+    local rootfs_device="$1"
+    local label="$2"
+    local mountpoint="$3"
+
+    local name=
+    local device=
+    local mount_prefix=
+    local list=
+    local linux=
+    local basename=
+    local dirname=
+    local rel_dirname=
+    local version=
+    local initrd=
+
+    # boot by label
+    device="LABEL=$label"
+
+    name=$(make_name "$label")
+
+    # avoid double-leading slashes and the subsequent need to call
+    # 'readlink -f'.
+    if [ "$mountpoint" = "/" ]; then
+        mount_prefix=""
+    else
+        mount_prefix="$mountpoint"
+    fi
+    list=$(get_kernels "$mount_prefix")
+
+    linux=$(version_find_latest "$list")
+    basename=$(basename "$linux")
+    dirname=$(dirname "$linux")
+    rel_dirname=$(make_system_path_relative_to_its_root "$dirname")
+    version=$(echo "$basename" | sed -e "s,^[^0-9]*-,,g")
+
+    initrd=$(get_initrd "$version" "$dirname")
+
+    # convert the path to the mounted "other" rootfs back to a
+    # a standard one by removing the mountpoint prefix.
+    if [ "$mountpoint" != "/" ]; then
+        linux=$(echo "$linux" | sed "s!^${mountpoint}!!g")
+        initrd=$(echo "$initrd" | sed "s!^${mountpoint}!!g")
+    fi
+
+    # Create menu entries for the 2 snappy rootfs's.
+    linux_entry_ext "$name" "$SNAPPY_OS" "$version" \
+        "$SNAPPY_TYPE" "$SNAPPY_ARGS" "$device" \
+        "$linux" "$initrd"
+}
+
+#---------------------------------------------------------------------
+# main
+
+case "$machine" in
+    i?86) GENKERNEL_ARCH="x86" ;;
+    mips|mips64) GENKERNEL_ARCH="mips" ;;
+    mipsel|mips64el) GENKERNEL_ARCH="mipsel" ;;
+    arm*) GENKERNEL_ARCH="arm" ;;
+    *) GENKERNEL_ARCH="$machine" ;;
+esac
+
+# Determine which partition label is being used for the current root
+# directory. This is slightly convoluted but required since this code
+# runs within a chroot environment (where lsblk does not work).
+#
+# XXX: Note that since this code is run from within a chroot (where the
+# "other" rootfs is mounted), it might appear that the logic is
+# inverted. However, the code below simply 
+this_mountpoint="/"
+this_root=$(get_device_from_mountpoint "$this_mountpoint")
+[ -z "$this_root" ] && {
+    die "cannot determine rootfs for $this_mountpoint"
+}
+
+this_label=$(get_label_from_device "$this_root")
+[ -z "$this_label" ] && {
+    die "cannot determine partition label for rootfs $this_root"
+}
+
+handle_menu_entry "$this_root" "$this_label" "$this_mountpoint"
+
+other_mountpoint="/writable/cache/system"
+if ! $(mountpoint -q "$other_mountpoint"); then
+    die "$other_mountpoint is not a mountpoint"
+fi
+
+other_label=$(get_other_rootfs_label "$this_label")
+
+other_root=$(get_device_from_label "$other_label")
+[ -z "$other_root" ] && {
+    die "cannot determine rootfs"
+}
+
+handle_menu_entry "$other_root" "$other_label" "$other_mountpoint"
+
+# Toggle rootfs if previous boot failed.
+#
+# Since grub sets recordfail, if it is _already_ set when grub starts
+# and we're in try mode, the previous boot must have failed to unset
+# recordfail, hence toggle the rootfs.
+sed "s/^/$submenu_indentation/" << EOF
+  if [ -z "\$snappy_mode" ]; then
+      set snappy_mode=default
+      save_env snappy_mode
+  fi
+  if [ -z "\$snappy_ab" ]; then
+      set snappy_ab=a
+      save_env snappy_ab
+  fi
+
+  if [ "\$snappy_mode" = "try" ]; then
+    if [ "\$recordfail" = "1" ]; then
+        if [ "\$snappy_ab" = "a" ]; then
+            set default="$(make_name system-b)"
+            set snappy_ab=b
+        else
+            set snappy_ab=a
+            set default="$(make_name system-a)"
+        fi
+        save_env snappy_ab
+    fi
+  else
+      if [ "\$snappy_ab" = "a" ]; then
+          set default="$(make_name system-a)"
+      else
+          set default="$(make_name system-b)"
+      fi
+  fi
+EOF

=== modified file 'partition/bootloader_grub.go'
--- partition/bootloader_grub.go	2015-03-26 09:05:27 +0000
+++ partition/bootloader_grub.go	2015-03-27 08:58:19 +0000
@@ -30,9 +30,8 @@
 	bootloaderGrubConfigFileReal = "/boot/grub/grub.cfg"
 	bootloaderGrubEnvFileReal    = "/boot/grub/grubenv"
 
-	bootloaderGrubEnvCmdReal     = "/usr/bin/grub-editenv"
-	bootloaderGrubInstallCmdReal = "/usr/sbin/grub-install"
-	bootloaderGrubUpdateCmdReal  = "/usr/sbin/update-grub"
+	bootloaderGrubEnvCmdReal    = "/usr/bin/grub-editenv"
+	bootloaderGrubUpdateCmdReal = "/usr/sbin/update-grub"
 )
 
 // var to make it testable
@@ -41,9 +40,8 @@
 	bootloaderGrubConfigFile = bootloaderGrubConfigFileReal
 	bootloaderGrubEnvFile    = bootloaderGrubEnvFileReal
 
-	bootloaderGrubEnvCmd     = bootloaderGrubEnvCmdReal
-	bootloaderGrubInstallCmd = bootloaderGrubInstallCmdReal
-	bootloaderGrubUpdateCmd  = bootloaderGrubUpdateCmdReal
+	bootloaderGrubEnvCmd    = bootloaderGrubEnvCmdReal
+	bootloaderGrubUpdateCmd = bootloaderGrubUpdateCmdReal
 )
 
 type grub struct {
@@ -54,7 +52,7 @@
 
 // newGrub create a new Grub bootloader object
 func newGrub(partition *Partition) bootLoader {
-	if !helpers.FileExists(bootloaderGrubConfigFile) || !helpers.FileExists(bootloaderGrubInstallCmd) {
+	if !helpers.FileExists(bootloaderGrubConfigFile) || !helpers.FileExists(bootloaderGrubUpdateCmd) {
 		return nil
 	}
 	b := newBootLoader(partition)
@@ -74,18 +72,9 @@
 //
 // Approach:
 //
-// Re-install grub each time the rootfs is toggled by running
-// grub-install chrooted into the other rootfs. Also update the grub
-// configuration.
+// Update the grub configuration.
 func (g *grub) ToggleRootFS() (err error) {
 
-	other := g.partition.otherRootPartition()
-
-	// install grub
-	if err := runInChroot(g.partition.MountTarget(), bootloaderGrubInstallCmd, other.parentName); err != nil {
-		return err
-	}
-
 	// create the grub config
 	if err := runInChroot(g.partition.MountTarget(), bootloaderGrubUpdateCmd); err != nil {
 		return err

=== modified file 'partition/bootloader_grub_test.go'
--- partition/bootloader_grub_test.go	2015-03-26 09:12:58 +0000
+++ partition/bootloader_grub_test.go	2015-03-27 08:58:19 +0000
@@ -38,7 +38,6 @@
 	// these files just needs to exist
 	mockGrubFile(c, bootloaderGrubConfigFile, 0644)
 	mockGrubFile(c, bootloaderGrubEnvFile, 0644)
-	mockGrubFile(c, bootloaderGrubInstallCmd, 0755)
 	mockGrubFile(c, bootloaderGrubUpdateCmd, 0755)
 
 	// do not run commands for real
@@ -94,20 +93,18 @@
 	mp := singleCommand{"/bin/mountpoint", "/writable/cache/system"}
 	c.Assert(allCommands[0], DeepEquals, mp)
 
-	expectedGrubInstall := singleCommand{"/usr/sbin/chroot", "/writable/cache/system", bootloaderGrubInstallCmd, "/dev/sda"}
-	c.Assert(allCommands[1], DeepEquals, expectedGrubInstall)
-
 	expectedGrubUpdate := singleCommand{"/usr/sbin/chroot", "/writable/cache/system", bootloaderGrubUpdateCmd}
-	c.Assert(allCommands[2], DeepEquals, expectedGrubUpdate)
+	c.Assert(allCommands[1], DeepEquals, expectedGrubUpdate)
 
 	expectedGrubSet := singleCommand{bootloaderGrubEnvCmd, bootloaderGrubEnvFile, "set", "snappy_mode=try"}
-	c.Assert(allCommands[3], DeepEquals, expectedGrubSet)
+	c.Assert(allCommands[2], DeepEquals, expectedGrubSet)
 
 	// the https://developer.ubuntu.com/en/snappy/porting guide says
 	// we always use the short names
 	expectedGrubSet = singleCommand{bootloaderGrubEnvCmd, bootloaderGrubEnvFile, "set", "snappy_ab=b"}
-	c.Assert(allCommands[4], DeepEquals, expectedGrubSet)
+	c.Assert(allCommands[3], DeepEquals, expectedGrubSet)
 
+	c.Assert(len(allCommands), Equals, 4)
 }
 
 func mockGrubEditenvList(cmd ...string) (string, error) {

=== modified file 'partition/partition.go'
--- partition/partition.go	2015-03-26 09:05:27 +0000
+++ partition/partition.go	2015-03-27 08:58:19 +0000
@@ -15,12 +15,6 @@
  *
  */
 
-// TODO:
-//
-// - logging
-// - SNAPPY_DEBUG
-// - locking (sync.Mutex)
-
 // Package partition manipulate snappy disk partitions
 package partition
 
@@ -32,6 +26,7 @@
 	"os/signal"
 	"path"
 	"regexp"
+	"sort"
 	"strings"
 	"syscall"
 
@@ -108,17 +103,6 @@
 // to the disk (such as uBoot, MLO)
 const flashAssetsDir = "flashtool-assets"
 
-//--------------------------------------------------------------------
-// FIXME: Globals
-
-// list of current mounts that this module has created
-var mounts []string
-
-// list of current bindmounts this module has created
-var bindMounts []string
-
-//--------------------------------------------------------------------
-
 // MountOption represents how the partition should be mounted, currently
 // RO (read-only) and RW (read-write) are supported
 type MountOption int
@@ -144,6 +128,24 @@
 	RunWithOther(rw MountOption, f func(otherRoot string) (err error)) (err error)
 }
 
+// mountEntry represents a mount this package has created.
+type mountEntry struct {
+	source string
+	target string
+
+	options string
+
+	// true if target refers to a bind mount. We could derive this
+	// from options, but this field saves the effort.
+	bindMount bool
+}
+
+// mountEntryArray represents an array of mountEntry objects.
+type mountEntryArray []mountEntry
+
+// current mounts that this package has created.
+var mounts mountEntryArray
+
 // Partition is the type to interact with the partition
 type Partition struct {
 	// all partitions
@@ -179,6 +181,37 @@
 	Bootloader      bootloaderName `yaml:"bootloader"`
 }
 
+// Len is part of the sort interface, required to allow sort to work
+// with an array of Mount objects.
+func (mounts mountEntryArray) Len() int {
+	return len(mounts)
+}
+
+// Less is part of the sort interface, required to allow sort to work
+// with an array of Mount objects.
+func (mounts mountEntryArray) Less(i, j int) bool {
+	return mounts[i].target < mounts[j].target
+}
+
+// Swap is part of the sort interface, required to allow sort to work
+// with an array of Mount objects.
+func (mounts mountEntryArray) Swap(i, j int) {
+	mounts[i], mounts[j] = mounts[j], mounts[i]
+}
+
+// removeMountByTarget removes the Mount specified by the target from
+// the global mounts array.
+func removeMountByTarget(mnts mountEntryArray, target string) (results mountEntryArray) {
+
+	for _, m := range mnts {
+		if m.target != target {
+			results = append(results, m)
+		}
+	}
+
+	return results
+}
+
 func init() {
 	if os.Getenv("SNAPPY_DEBUG") != "" {
 		debug = true
@@ -190,20 +223,34 @@
 	}
 }
 
-func undoMounts(mounts []string) (err error) {
+// undoMounts unmounts all mounts this package has mounted optionally
+// only unmounting bind mounts and leaving all remaining mounts.
+func undoMounts(bindMountsOnly bool) error {
+
+	mountsCopy := make(mountEntryArray, len(mounts), cap(mounts))
+	copy(mountsCopy, mounts)
+
+	// reverse sort to ensure unmounts are handled in the correct
+	// order.
+	sort.Sort(sort.Reverse(mountsCopy))
+
 	// Iterate backwards since we want a reverse-sorted list of
 	// mounts to ensure we can unmount in order.
-	for i := range mounts {
-		if err := unmountAndRemoveFromGlobalMountList(mounts[len(mounts)-i-1]); err != nil {
+	for _, mount := range mountsCopy {
+		if bindMountsOnly && !mount.bindMount {
+			continue
+		}
+
+		if err := unmountAndRemoveFromGlobalMountList(mount.target); err != nil {
 			return err
 		}
 	}
 
-	return err
+	return nil
 }
 
 func signalHandler(sig os.Signal) {
-	err := undoMounts(mounts)
+	err := undoMounts(false)
 	if err != nil {
 		// FIXME: use logger
 		fmt.Fprintf(os.Stderr, "ERROR: failed to unmount: %s", err)
@@ -257,45 +304,31 @@
 	return runCommand(args...)
 }
 
-// Mount the given directory and add it to the "mounts" slice
-func mountAndAddToGlobalMountList(source, target, options string) (err error) {
-	err = mount(source, target, options)
+// Mount the given directory and add it to the global mounts slice
+func mountAndAddToGlobalMountList(m mountEntry) (err error) {
+
+	err = mount(m.source, m.target, m.options)
 	if err == nil {
-		mounts = append(mounts, target)
+		mounts = append(mounts, m)
 	}
 
 	return err
 }
 
-// Remove the given string from the string slice
-func stringSliceRemove(slice []string, needle string) (res []string) {
-	// FIXME: so this is golang slice remove?!?! really?
-	if pos := stringInSlice(slice, needle); pos >= 0 {
-		slice = append(slice[:pos], slice[pos+1:]...)
-	}
-	return slice
-}
-
-// FIXME: would it make sense to rename to something like
-//         "UmountAndRemoveFromMountList" to indicate it has side-effects?
 // Unmount the given directory and remove it from the global "mounts" slice
 func unmountAndRemoveFromGlobalMountList(target string) (err error) {
 	err = runCommand("/bin/umount", target)
-	if err == nil {
-		mounts = stringSliceRemove(mounts, target)
-	}
-
-	return err
-}
-
-func bindmountAndAddToGlobalMountList(source, target string) (err error) {
-	err = mountAndAddToGlobalMountList(source, target, "bind")
-
-	if err == nil {
-		bindMounts = append(bindMounts, target)
-	}
-
-	return err
+	if err != nil {
+		return err
+
+	}
+
+	results := removeMountByTarget(mounts, target)
+
+	// Update global
+	mounts = results
+
+	return nil
 }
 
 // Run fsck(8) on specified device.
@@ -409,9 +442,12 @@
 	return partitions, nil
 }
 
+var makeDirectory = func(path string, mode os.FileMode) error {
+	return os.MkdirAll(path, mode)
+}
+
 func (p *Partition) makeMountPoint() (err error) {
-
-	return os.MkdirAll(p.MountTarget(), dirMode)
+	return makeDirectory(p.MountTarget(), dirMode)
 }
 
 // New creates a new partition type
@@ -643,19 +679,31 @@
 
 	other = p.otherRootPartition()
 
+	m := mountEntry{source: other.device, target: p.MountTarget()}
+
 	if readOnly {
-		err = mountAndAddToGlobalMountList(other.device, p.MountTarget(), "ro")
+		m.options = "ro"
+		err = mountAndAddToGlobalMountList(m)
 	} else {
-		err = fsck(other.device)
+		err = fsck(m.source)
 		if err != nil {
 			return err
 		}
-		err = mountAndAddToGlobalMountList(other.device, p.MountTarget(), "")
+		err = mountAndAddToGlobalMountList(m)
 	}
 
 	return err
 }
 
+// Create a read-only bindmount of the currently-mounted rootfs at the
+// specified mountpoint location (which must already exist).
+func (p *Partition) bindmountThisRootfsRO(target string) (err error) {
+	return mountAndAddToGlobalMountList(mountEntry{source: "/",
+		target:    target,
+		options:   "bind,ro",
+		bindMount: true})
+}
+
 // Ensure the other partition is mounted read-only.
 func (p *Partition) ensureOtherMountedRO() (err error) {
 	mountpoint := p.MountTarget()
@@ -690,7 +738,9 @@
 			return err
 		}
 
-		return mount(other.device, p.MountTarget(), "")
+		return mountAndAddToGlobalMountList(mountEntry{
+			source: other.device,
+			target: p.MountTarget()})
 	}
 	// r/w -> r/o: no fsck required.
 	return mount(other.device, p.MountTarget(), "remount,ro")
@@ -724,18 +774,36 @@
 	for _, fs := range requiredChrootMounts {
 		target := path.Join(p.MountTarget(), fs)
 
-		err := bindmountAndAddToGlobalMountList(fs, target)
+		err := mountAndAddToGlobalMountList(mountEntry{source: fs,
+			target:    target,
+			options:   "bind",
+			bindMount: true})
 		if err != nil {
 			return err
 		}
 	}
 
-	return nil
+	// Grub also requires access to both rootfs's when run from
+	// within a chroot (to allow it to create menu entries for
+	// both), so bindmount the real rootfs.
+	targetInChroot := path.Join(p.MountTarget(), p.MountTarget())
+
+	// FIXME: we should really remove this after the unmount
+
+	if err = makeDirectory(targetInChroot, dirMode); err != nil {
+		return err
+	}
+
+	return p.bindmountThisRootfsRO(targetInChroot)
 }
 
 // Undo the effects of BindmountRequiredFilesystems()
 func (p *Partition) unmountRequiredFilesystems() (err error) {
-	return undoMounts(bindMounts)
+	if err = undoMounts(true); err != nil {
+		return err
+	}
+
+	return nil
 }
 
 func (p *Partition) toggleBootloaderRootfs() (err error) {

=== modified file 'partition/partition_test.go'
--- partition/partition_test.go	2015-03-26 09:12:58 +0000
+++ partition/partition_test.go	2015-03-27 08:58:19 +0000
@@ -42,6 +42,10 @@
 	return err
 }
 
+func mockMakeDirectory(path string, mode os.FileMode) error {
+	return nil
+}
+
 func (s *PartitionTestSuite) SetUpTest(c *C) {
 	s.tempdir = c.MkDir()
 	runLsblk = mockRunLsblkDualSnappy
@@ -50,13 +54,14 @@
 	bootloaderGrubDir = filepath.Join(s.tempdir, "boot", "grub")
 	bootloaderGrubConfigFile = filepath.Join(bootloaderGrubDir, "grub.cfg")
 	bootloaderGrubEnvFile = filepath.Join(bootloaderGrubDir, "grubenv")
-	bootloaderGrubInstallCmd = filepath.Join(s.tempdir, "grub-install")
 	bootloaderGrubUpdateCmd = filepath.Join(s.tempdir, "update-grub")
 
 	// and uboot
 	bootloaderUbootDir = filepath.Join(s.tempdir, "boot", "uboot")
 	bootloaderUbootConfigFile = filepath.Join(bootloaderUbootDir, "uEnv.txt")
 	bootloaderUbootEnvFile = filepath.Join(bootloaderUbootDir, "uEnv.txt")
+
+	c.Assert(mounts, DeepEquals, mountEntryArray(nil))
 }
 
 func (s *PartitionTestSuite) TearDownTest(c *C) {
@@ -70,7 +75,6 @@
 	// grub vars
 	bootloaderGrubConfigFile = bootloaderGrubConfigFileReal
 	bootloaderGrubEnvFile = bootloaderGrubEnvFileReal
-	bootloaderGrubInstallCmd = bootloaderGrubInstallCmdReal
 	bootloaderGrubUpdateCmd = bootloaderGrubUpdateCmdReal
 
 	// uboot vars
@@ -78,6 +82,7 @@
 	bootloaderUbootConfigFile = bootloaderUbootConfigFileReal
 	bootloaderUbootEnvFile = bootloaderUbootEnvFileReal
 
+	c.Assert(mounts, DeepEquals, mountEntryArray(nil))
 }
 
 func makeHardwareYaml(c *C, hardwareYaml string) (outPath string) {
@@ -127,6 +132,46 @@
 	return strings.Split(dualData, "\n"), err
 }
 
+func (s *PartitionTestSuite) TestMountEntryArray(c *C) {
+	mea := mountEntryArray{}
+
+	c.Assert(mea.Len(), Equals, 0)
+
+	me := mountEntry{source: "/dev",
+		target:    "/dev",
+		options:   "bind",
+		bindMount: true}
+
+	mea = append(mea, me)
+	c.Assert(mea.Len(), Equals, 1)
+
+	me = mountEntry{source: "/foo",
+		target:    "/foo",
+		options:   "",
+		bindMount: false}
+
+	mea = append(mea, me)
+	c.Assert(mea.Len(), Equals, 2)
+
+	c.Assert(mea.Less(0, 1), Equals, true)
+	c.Assert(mea.Less(1, 0), Equals, false)
+
+	mea.Swap(0, 1)
+	c.Assert(mea.Less(0, 1), Equals, false)
+	c.Assert(mea.Less(1, 0), Equals, true)
+
+	results := removeMountByTarget(mea, "invalid")
+
+	// No change expected
+	c.Assert(results, DeepEquals, mea)
+
+	results = removeMountByTarget(mea, "/dev")
+
+	c.Assert(len(results), Equals, 1)
+	c.Assert(results[0], Equals, mountEntry{source: "/foo",
+		target: "/foo", options: "", bindMount: false})
+}
+
 func (s *PartitionTestSuite) TestSnappyDualRoot(c *C) {
 	p := New()
 	c.Assert(p.dualRootPartitions(), Equals, true)
@@ -173,7 +218,10 @@
 }
 
 func (s *PartitionTestSuite) TestRunWithOtherDualParitionRWFuncErr(c *C) {
+	c.Assert(mounts, DeepEquals, mountEntryArray(nil))
+
 	runCommand = mockRunCommand
+	makeDirectory = mockMakeDirectory
 
 	p := New()
 	err := p.RunWithOther(RW, func(otherRoot string) (err error) {
@@ -186,8 +234,16 @@
 
 	// ensure cleanup happend
 
-	// FIXME: mounts is global
-	c.Assert(mounts, DeepEquals, []string{})
+	// FIXME: mounts are global
+	expected := mountEntry{source: "/dev/sda4",
+		target: "/writable/cache/system", options: "", bindMount: false}
+
+	// At program exit, "other" should still be mounted
+	c.Assert(mounts, DeepEquals, mountEntryArray{expected})
+
+	undoMounts(false)
+
+	c.Assert(mounts, DeepEquals, mountEntryArray(nil))
 }
 
 func (s *PartitionTestSuite) TestRunWithOtherSingleParitionRO(c *C) {
@@ -235,48 +291,94 @@
 	c.Assert(p, NotNil)
 
 	p.mountOtherRootfs(false)
-	c.Assert(mounts, DeepEquals, []string{p.MountTarget()})
+	expected := mountEntry{source: "/dev/sda4",
+		target: "/writable/cache/system", options: "", bindMount: false}
+
+	c.Assert(mounts, DeepEquals, mountEntryArray{expected})
+
 	p.unmountOtherRootfs()
-	c.Assert(mounts, DeepEquals, []string{})
-}
-
-func (s *PartitionTestSuite) TestStringSliceRemoveExisting(c *C) {
-	haystack := []string{"one", "two", "three"}
-
-	newSlice := stringSliceRemove(haystack, "two")
-	c.Assert(newSlice, DeepEquals, []string{"one", "three"})
-
-	// note here that haystack is no longer "valid", i.e. the
-	// underlying array is modified (which is fine)
-	c.Assert(haystack, DeepEquals, []string{"one", "three", "three"})
-}
-
-func (s *PartitionTestSuite) TestStringSliceRemoveNoexistingNoOp(c *C) {
-	haystack := []string{"6", "28", "496", "8128"}
-	newSlice := stringSliceRemove(haystack, "99")
-	c.Assert(newSlice, DeepEquals, []string{"6", "28", "496", "8128"})
-}
-
-func (s *PartitionTestSuite) TestUndoMounts(c *C) {
+	c.Assert(mounts, DeepEquals, mountEntryArray(nil))
+}
+
+func (s *PartitionTestSuite) TestUnmountRequiredFilesystems(c *C) {
 	runCommand = mockRunCommand
 	s.makeFakeGrubEnv(c)
 
 	p := New()
 	c.Assert(c, NotNil)
 
-	// FIXME: mounts is global
-	c.Assert(mounts, DeepEquals, []string{})
 	p.bindmountRequiredFilesystems()
-	c.Assert(mounts, DeepEquals, []string{
-		p.MountTarget() + "/dev",
-		p.MountTarget() + "/proc",
-		p.MountTarget() + "/sys",
-		p.MountTarget() + "/boot/efi",
+	c.Assert(mounts, DeepEquals, mountEntryArray{
+		mountEntry{source: "/dev", target: p.MountTarget() + "/dev",
+			options: "bind", bindMount: true},
+
+		mountEntry{source: "/proc", target: p.MountTarget() + "/proc",
+			options: "bind", bindMount: true},
+
+		mountEntry{source: "/sys", target: p.MountTarget() + "/sys",
+			options: "bind", bindMount: true},
+		mountEntry{source: "/boot/efi", target: p.MountTarget() + "/boot/efi",
+			options: "bind", bindMount: true},
+
 		// this comes from the grub bootloader via AdditionalBindMounts
-		p.MountTarget() + "/boot/grub",
+		mountEntry{source: "/boot/grub", target: p.MountTarget() + "/boot/grub",
+			options: "bind", bindMount: true},
+
+		// Required to allow grub inside the chroot to access
+		// the "current" rootfs outside the chroot (used
+		// to generate the grub menuitems).
+		mountEntry{source: "/",
+			target:  p.MountTarget() + p.MountTarget(),
+			options: "bind,ro", bindMount: true},
 	})
 	p.unmountRequiredFilesystems()
-	c.Assert(mounts, DeepEquals, []string{})
+	c.Assert(mounts, DeepEquals, mountEntryArray(nil))
+}
+
+func (s *PartitionTestSuite) TestUndoMounts(c *C) {
+	runCommand = mockRunCommand
+
+	p := New()
+	c.Assert(c, NotNil)
+
+	err := p.remountOther(RW)
+	c.Assert(err, IsNil)
+
+	p.bindmountRequiredFilesystems()
+	c.Assert(mounts, DeepEquals, mountEntryArray{
+
+		mountEntry{source: "/dev/sda4", target: "/writable/cache/system",
+			options: "", bindMount: false},
+
+		mountEntry{source: "/dev", target: p.MountTarget() + "/dev",
+			options: "bind", bindMount: true},
+
+		mountEntry{source: "/proc", target: p.MountTarget() + "/proc",
+			options: "bind", bindMount: true},
+
+		mountEntry{source: "/sys", target: p.MountTarget() + "/sys",
+			options: "bind", bindMount: true},
+
+		mountEntry{source: "/boot/efi", target: p.MountTarget() + "/boot/efi",
+			options: "bind", bindMount: true},
+
+		mountEntry{source: "/",
+			target:  p.MountTarget() + p.MountTarget(),
+			options: "bind,ro", bindMount: true},
+	})
+
+	// should leave non-bind mounts
+	undoMounts(true)
+
+	c.Assert(mounts, DeepEquals, mountEntryArray{
+		mountEntry{source: "/dev/sda4", target: "/writable/cache/system",
+			options: "", bindMount: false},
+	})
+
+	// should unmount everything
+	undoMounts(false)
+
+	c.Assert(mounts, DeepEquals, mountEntryArray(nil))
 }
 
 func mockRunLsblkNoSnappy() (output []string, err error) {
@@ -361,6 +463,9 @@
 	c.Assert(err, IsNil)
 	c.Assert(b.ToggleRootFSCalled, Equals, true)
 	c.Assert(b.HandleAssetsCalled, Equals, true)
+
+	p.unmountOtherRootfs()
+	c.Assert(mounts, DeepEquals, mountEntryArray(nil))
 }
 
 func (s *PartitionTestSuite) TestMarkBootSuccessful(c *C) {