Comment 19 for bug 575469

Revision history for this message
Colin Watson (cjwatson) wrote :

= Code review =

In general I think this is broadly right; recovery software clearly doesn't belong in /usr, and I can see why it simplifies things to have this guaranteeably start before mountall rather than having to change mountall.

However, the only other 'start on startup' job I see is hostname, and I think we ought to run that even in recovery mode. Any job that's 'start on startup' already has to run with a read-only root filesystem, since that's how the initramfs typically leaves things and it isn't until mountall runs that the root filesystem becomes read-write. Thus, why not simply make friendly-recovery be 'start on starting mountall'? That way, you could drop the grub2 and initramfs-tools changes entirely.

Can we make /usr/share/recovery-mode a symlink to /lib/recovery-mode for compatibility, just in case third-party packages have added entries here? The postinst will need to move any remaining files from /usr/share/recovery-mode/ to /lib/recovery-mode/ and then install a symlink, since dpkg won't replace a directory with a symlink by itself.

I think I'd slightly prefer the Upstart job to look like this:

  script
          if [ -x /lib/recovery-mode/recovery-menu ]; then
                  exec /lib/recovery-mode/recovery-menu
          else
                  exec /sbin/sulogin
          fi
  end script

  post-stop exec initctl emit startup

That way Upstart is directly supervising the process that's actually doing the work; it's a detail but it seems more logical to me.

"Remount / in read/write" should read "Remount / read/write". Also, I think it would be helpful to indicate somewhere in the UI that this enables more recovery mode options, otherwise people will be rather confused why some option they were vaguely looking for doesn't appear. Perhaps check this with a designer?

I feel there ought to be some dependency relation between upstart and friendly-recovery, perhaps Breaks in one direction or both, to make sure partial upgrades don't leave people without recovery mode. This would be simpler if you could take my 'start on starting mountall' suggestion so that we don't also need dependency relations with grub-common and initramfs-tools.

The following items apply only if you decide not to take my 'start on starting mountall' suggestion above:

What happens if friendly-recovery isn't installed? After all, it's only Priority: standard. Can we use *both* single and startup-event=recovery-mode safely; or, failing that, have the relevant grub.d script test for the presence of /lib/recovery-mode/recovery-menu and add either single or startup-event=recovery-mode as appropriate? I think it's important to test this both with and without friendly-recovery installed.

To reduce code duplication, I'd prefer the initramfs-tools code to do something like:

  exec run-init ${rootmnt} ${init} "$@" ${STARTUP_EVENT:+--startup-event=$STARTUP_EVENT} ...

= Feature freeze exception =

On the one hand, it would be lovely to get this in: the bug makes recovery mode unusable in a significant set of situations, and that's serious. On the other, I don't like contemplating something unforeseen making recovery mode worse. So I'm on the fence about this, and would appreciate input from other members of the release team (whom I've subscribed).

I would be happier if it were possible to avoid the grub2 and initramfs-tools changes as discussed above, and confine the change entirely to friendly-recovery and upstart (the latter of which is fair enough since it consists of removing a tentacle of friendly-recovery).