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

[Chris Marusich]

Hi,

address@hidden (Ludovic Courtès) writes:

> Hi!
>
> Chris Marusich <address@hidden> skribis:
>
>> 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.

Yes, that sounds like the right approach.  I'll do that.

>> 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.

I think we can.  But I think it will require backwards incompatible
changes to the boot parameters file.  Here's why:

Many of the existing procedures in (gnu system grub) take a "file
system" object as input (e.g. the 'grub-configuration-file' procedure).
However, the boot parameters file does not currently contain all the
information that a "file system" object contains.  Here's an example of
what it contains today:

--8<---------------cut here---------------start------------->8---
(boot-parameters
  (version 0)
  (label "GNU with Linux-Libre 4.1.20 (beta)")
  (root-device "root")
  (kernel
    "/gnu/store/zygby8db0adcyj3m6rjflr80jarfy9b5-linux-libre-4.1.20")
  (kernel-arguments ())
  (initrd
    (string-append
      "/gnu/store/hlra3a0g3a14bjvdn3vbagwfvy4nmhn8-base-initrd"
      "/initrd")))
--8<---------------cut here---------------end--------------->8---

To avoid backwards-incompatible changes to the structure of the boot
parameters file, I had originally planned to refactor the procedures in
(gnu system grub) so that I could use them with the limited information
that is contained in the version 0 boot parameters file.  However,
commit 0f65f54e has modified these procedures in a way that makes it
very awkward to refactor the "file system" object out of them.  Now, to
re-use the existing procedures, I believe I will need to add this
missing information (i.e., the information contained in a file system
object) to the boot parameters file, so that I can construct a "file
system" object to pass to those procedures.  Does that sound right to
you?

If I do that, then it will probably be a backwards-incompatible change,
so I will do it in the following way.  I will simply store an entire
"file system" object in the boot parameters file.  I will bump the
version of the boot parameters file from 0 to 1.  To ensure that all new
system generations use version 1, I will update commands like
"reconfigure" to always create a version 1 boot parameters file.  I will
make the new commands (roll-back and switch-generation) refuse to switch
to any system generation which uses version 0 (because it isn't possible
to construct a complete "file system" object from a version 0 boot
parameters file).  I will also update existing commands like
'list-generations' so that they will gracefully handle both versions.

Does this sound like the right approach to you?

> 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’?

I don't mind at all; I'm happy to make any changes that will improve the
quality of the end result.

I've tried using 'git send-email' on GuixSD before, and it didn't work
for me (because a mail transfer agent is not running on my GuixSD
system).  When the new patches are ready, I'll try once more to get it
working.

>> 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?

Yes, I like that better.

>> 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”.

Upon closer inspection, it looks like this procedure,
'grub-configuration-file', actually returns a monadic value (in the
store monad), which "contains" a derivation, which in turn builds the
grub configuration file.  Even in a case like this, where there is so
much indirection, is it appropriate to elide all those details?

If this is the style we should consistently use in our documentation,
then that's fine, and I will happily follow suit in the name of
consistency.  However, as a newcomer to this code base, to gexps, to
derivations, and to monads, in the beginning I was very confused about
how to use this procedure's return value.

If I can think of a good way to make stuff like this more obvious for
newcomers, I'll let you know.  For now, though, I think the best thing
to do is to change my patches to conform to the existing style.

>> 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.

OK.

>> +(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?

That's a good point.  I'll use that name.

>> 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?

The intent of this change was to avoid making backwards-incompatible
changes to the boot parameters file, which only contains the "file
system" device, not a whole "file system" object.  However, going
forward, I will abandon this refactoring in favor of the alternative
plan presented earlier in this email.

>> 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*’.

OK.

>> +        (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?

That makes sense.  I'll add some logic to filter it out.

>> +        (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.  :-)

I didn't know about that procedure!  Both of your suggestions sound good
to me.

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

OK.  I will update the documentation in the next set of patches.

> Thanks a lot for this very useful contribution!

Thank you for your patience.  I'll send the updated patches when they're
ready.

-- 
Chris

signature.asc
Description: PGP signature