Re: Patches to implement system roll-back and switch-generation

[Ludovic Courtès]

Hi!

Chris Marusich <address@hidden> skribis:

> Here are some patches which, when applied to
> 1df00601b280db1cdfe0fc8d539ee6c6c726c355, make it possible to switch
> system generations using two new "guix system" subcommands: "roll-back"
> and "switch-generation".  These commands currently just rebuild the
> grub.cfg file with a new default entry.  As a result, if you want to
> roll back or switch to another system generation, you don't have to
> manually select a previous version every single time you boot, which is
> nice.

Awesome!

> I believe my patch does NOT yet make the regenerated grub.cfg a GC root,
> so it is possible that after rolling back or switching generations,
> invoking GC might clean up some things you'd rather keep around (like
> the grub background image file).  Please be careful not to try this
> patch on a machine you care about; please use a VM instead.  I've
> verified that it works in a VM.

Indeed.  I think you’ll need to extract and reuse the logic in
‘install-grub*’ that installs the GC root.

> Unfortunately, these patches do not apply cleanly to the current master
> branch because of commit 0f65f54ebd76324653fd5506a7dab42ee44d9255.  This
> commit has thrown a wrench in my plans.  When I try to rebase onto
> master, there are multiple conflicts, and I am not sure yet what the
> best way to resolve them will be.  In any case, I think it will be more
> productive to ask for feedback now, rather than to continue on my own
> down uncertain paths.

Sorry about that!  Hopefully we can work around the conflicts.

Overall the patches LGTM.  You’re going to hate me for that, but could
you please add ChangeLog-style commit logs?  Also, could you send the
revised patches using ‘git send-email’?

> From 1741e8a66be3d7e5f647796f434689b0a61e1122 Mon Sep 17 00:00:00 2001
> From: Chris Marusich <address@hidden>
> Date: Tue, 12 Jul 2016 22:58:03 -0700
> Subject: [PATCH 1/9] Improve docstring: switch-to-generation
>
> ---
>  guix/profiles.scm | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/guix/profiles.scm b/guix/profiles.scm
> index 4a2ba1c..38f69dd 100644
> --- a/guix/profiles.scm
> +++ b/guix/profiles.scm
> @@ -1011,7 +1011,9 @@ that fails."
>  
>  (define (switch-to-generation profile number)
>    "Atomically switch PROFILE to the generation NUMBER.  Return the number of
> -the generation that was current before switching."
> +the generation that was current before switching.  Raise a
> +&profile-not-found-error when the profile does not exist.  Raise a
> +&missing-generation-error when the generation does not exist."
>    (let ((current    (generation-number profile))
>          (generation (generation-file-name profile number)))

OK.

> From 9f554133be83f06d5a3d0bfc713331e59eb2116c Mon Sep 17 00:00:00 2001
> From: Chris Marusich <address@hidden>
> Date: Tue, 12 Jul 2016 22:53:10 -0700
> Subject: [PATCH 2/9] Refactor: extract procedure:
>  relative-generation-spec->number

OK.

> From 42b1f00ce802745fbdc3b9bc5be38ccd47c0af33 Mon Sep 17 00:00:00 2001
> From: Chris Marusich <address@hidden>
> Date: Sat, 16 Jul 2016 15:53:22 -0700
> Subject: [PATCH 3/9] Rename previous-grub-entries to grub-entries
>
> This procedure actually returns an entry for every generation of the profile,
> so its name is confusing if it suggests that it only returns "previous"
> entries.

OK!  Maybe ‘profile-grub-entries’ would work too, to suggest that it’s
stateful?

> From a440eb18eaa6c2fe12d91db1c9d62e79823e7ad0 Mon Sep 17 00:00:00 2001
> From: Chris Marusich <address@hidden>
> Date: Mon, 1 Aug 2016 07:51:38 -0700
> Subject: [PATCH 4/9] Fix docstrings: these procedures return derivations
>
> ---
>  gnu/system.scm      | 4 ++--
>  gnu/system/grub.scm | 8 ++++----
>  2 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/gnu/system.scm b/gnu/system.scm
> index 7edb018..1d1ed5e 100644
> --- a/gnu/system.scm
> +++ b/gnu/system.scm
> @@ -718,8 +718,8 @@ listed in OS.  The C library expects to find it under
>    (store-file-system (operating-system-file-systems os)))
>  
>  (define* (operating-system-grub.cfg os #:optional (old-entries '()))
> -  "Return the GRUB configuration file for OS.  Use OLD-ENTRIES to populate 
> the
> -\"old entries\" menu."
> +  "Return a derivation which builds the GRUB configuration file for OS.  Use
> +OLD-ENTRIES to populate the \"old entries\" menu."
>    (mlet* %store-monad
>        ((system      (operating-system-derivation os))
>         (root-fs ->  (operating-system-root-file-system os))
> diff --git a/gnu/system/grub.scm b/gnu/system/grub.scm
> index 4592747..c426d5f 100644
> --- a/gnu/system/grub.scm
> +++ b/gnu/system/grub.scm
> @@ -239,10 +239,10 @@ code."
>                                    #:key
>                                    (system (%current-system))
>                                    (old-entries '()))
> -  "Return the GRUB configuration file corresponding to CONFIG, a
> -<grub-configuration> object, and where the store is available at STORE-FS, a
> -<file-system> object.  OLD-ENTRIES is taken to be a list of menu entries
> -corresponding to old generations of the system."
> +  "Return a derivation which builds the GRUB configuration file corresponding
> +to CONFIG, a <grub-configuration> object, and where the store is available at
> +STORE-FS, a <file-system> object.  OLD-ENTRIES is taken to be a list of menu
> +entries corresponding to old generations of the system."

OK, although I often write “Return something” when that really means
“Return a derivation that builds something”.

> From 227ffc6c34c7bef29a39b2745865ac25c28a7e74 Mon Sep 17 00:00:00 2001
> From: Chris Marusich <address@hidden>
> Date: Mon, 1 Aug 2016 08:46:48 -0700
> Subject: [PATCH 5/9] Factor out procedure: device->title
>
> ---
>  gnu/build/file-systems.scm | 31 ++++++++++++++++++-------------
>  1 file changed, 18 insertions(+), 13 deletions(-)
>
> diff --git a/gnu/build/file-systems.scm b/gnu/build/file-systems.scm
> index f1fccbd..4a8acd5 100644
> --- a/gnu/build/file-systems.scm
> +++ b/gnu/build/file-systems.scm
> @@ -38,6 +38,7 @@
>              find-partition-by-uuid
>              find-partition-by-luks-uuid
>              canonicalize-device-spec
> +            device->title
>  
>              uuid->string
>              string->uuid
> @@ -364,19 +365,6 @@ the following:
>      ;; this long.
>      20)
>  
> -  (define canonical-title
> -    ;; The realm of canonicalization.
> -    (if (eq? title 'any)
> -        (if (string? spec)
> -            ;; The "--root=SPEC" kernel command-line option always provides a
> -            ;; string, but the string can represent a device, a UUID, or a
> -            ;; label.  So check for all three.
> -            (cond ((string-prefix? "/" spec) 'device)
> -                  ((string->uuid spec) 'uuid)
> -                  (else 'label))
> -            'uuid)
> -        title))
> -
>    (define (resolve find-partition spec fmt)
>      (let loop ((count 0))
>        (let ((device (find-partition spec)))
> @@ -391,6 +379,10 @@ the following:
>                    (sleep 1)
>                    (loop (+ 1 count))))))))
>  
> +  (define canonical-title (if (eq? title 'any)

Put the “(if” on the next line.

> +(define (device->title device)
> +  "Guess the title for the given DEVICE, which must be a device parameter 
> from
> +a <file-system> object.  As a special case, when the DEVICE is a UUID, it may
> +be specified as a string."

What about calling it ‘inferred-device-title’ instead, since it’s not
just a conversion?

> From 1ade872ffae08ded1b8dae2fca05fee33ac0c69f Mon Sep 17 00:00:00 2001
> From: Chris Marusich <address@hidden>
> Date: Mon, 1 Aug 2016 08:57:08 -0700
> Subject: [PATCH 6/9] gnu/system and gnu/system/grub: use root-fs-device, not
>  root-fs

[...]

> -(define (grub-root-search root-fs file)
> -  "Return the GRUB 'search' command to look for ROOT-FS, which contains FILE,
> +(define (grub-root-search root-fs-device file)
> +  "Return the GRUB 'search' command to look for ROOT-FS-DEVICE, which 
> contains FILE,
>  a gexp.  The result is a gexp that can be inserted in the grub.cfg-generation
>  code."
> -  (case (file-system-title root-fs)
> -    ;; Preferably refer to ROOT-FS by its UUID or label.  This is more
> -    ;; efficient and less ambiguous, see <>.
> +  (case (device->title root-fs-device)
> +    ;; Preferably refer to ROOT-FS-DEVICE by its UUID or label.  This is more
> +    ;; efficient and less ambiguous.

I’m not convinced by this patch.  What’s the rationale?  To better deal
with cases where the title is 'any?

> From 55cf900abf6dba10ed640d24433cc1ca23e4acf2 Mon Sep 17 00:00:00 2001
> From: Chris Marusich <address@hidden>
> Date: Tue, 2 Aug 2016 21:28:21 -0700
> Subject: [PATCH 7/9] gnu/build/install: Factor out procedure:
>  install-grub-config

OK.

> From 059e79ab26f5e8ee60b60f5df01907300346c8e2 Mon Sep 17 00:00:00 2001
> From: Chris Marusich <address@hidden>
> Date: Thu, 28 Jul 2016 02:31:38 -0700
> Subject: [PATCH 8/9] grub-entries: take a list of numbers on input

OK.

> From b5816897c6db7984f678963dabe8ae58c6947677 Mon Sep 17 00:00:00 2001
> From: Chris Marusich <address@hidden>
> Date: Wed, 3 Aug 2016 00:41:01 -0700
> Subject: [PATCH 9/9] Implement switch-generation and roll-back
>
> ---
>  guix/scripts/system.scm | 87 
> ++++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 83 insertions(+), 4 deletions(-)
>
> diff --git a/guix/scripts/system.scm b/guix/scripts/system.scm
> index f450c9a..5c72808 100644
> --- a/guix/scripts/system.scm
> +++ b/guix/scripts/system.scm
> @@ -408,6 +408,57 @@ NUMBERS, which is a list of generation numbers."
>  
>
>  ;;;
> +;;; Roll-back.
> +;;;
> +(define (roll-back-system store)
> +  "Roll back the system profile to its previous generation."
> +  (switch-to-system-generation store "-1"))
> +
> +;;;
> +;;; Switch generations.
> +;;;
> +(define (switch-to-system-generation store spec)
> +  "Switch the system profile to the generation specified by SPEC, and
> +re-install grub with a grub configuration file that uses the specified system
> +generation as its default entry."
> +  (let ((number (relative-generation-spec->number %system-profile spec)))
> +      (if number
> +          (begin
> +            (reinstall-grub store number)
> +            (switch-to-generation* %system-profile number))
> +          (leave (_ "cannot switch to system generation '~a'~%") spec))))
> +
> +(define (reinstall-grub store number)
> +  "Re-install grub for existing system profile generation NUMBER."
> +  (unless-file-not-found
> +   (let*
> +       ((generation (generation-file-name %system-profile number))

No newline after ‘let*’.

> +        (file (string-append generation "/parameters"))
> +        (params (call-with-input-file file read-boot-parameters))
> +        ;; We assume that the root file system contains the store.  If this
> +        ;; assumption is ever false, then problems might occur.
> +        (root-device (boot-parameters-root-device params))
> +        ;; Generate a new grub configuration which uses default values for
> +        ;; just about everything.  If the specified system generation was
> +        ;; built from an operating system configuration file that contained
> +        ;; non-default values in its grub-configuration, then the grub
> +        ;; configuration we generate here will be different.  However, even
> +        ;; if that is the case, this default configuration will be good
> +        ;; enough to enable the user to boot into the specified generation.
> +        (grub-config (grub-configuration (device root-device)))
> +        ;; Make the specified system generation the default entry.
> +        (entries (grub-entries %system-profile (list number)))
> +        (old-entries (grub-entries))

Should we remove NUMBER from OLD-ENTRIES?

> +        (grub.cfg-derivation (run-with-store store
> +                    (grub-configuration-file grub-config
> +                                             root-device
> +                                             entries
> +                                             #:old-entries old-entries))))
> +     (build-derivations store (list grub.cfg-derivation))

Add call to ‘show-what-to-build’ before.  I’d remove “-derivation” from
the identifier, that’s not very helpful IMO.  :-)

> +     (install-grub-config (derivation->output-path grub.cfg-derivation) 
> "/"))))

And yeah, GRUB.CFG must be registered as a GC root before we can safely
apply this patch.

The rest LGTM.  We’ll also need a few lines describing these actions in
guix.texi.

Thanks a lot for this very useful contribution!

Ludo’.