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

[Ludovic Courtès]

Hi Chris,

Chris Marusich  skribis:

> l...@gnu.org (Ludovic Courtès) writes:
>> We can now avoid monadic procedures by using the declarative counterpart
>> of the monadic API.  That is, we could write:
>>
>>   (define (grub-configuration-file …)  ;normal proc
>> (computed-file "grub.cfg" builder))
>>
>> instead of:
>>
>>   (define (grub-configuration-file …)  ;monadic proc
>> (gexp->derivation "grub.cfg" builder))
>>
>> I would welcome such changes.
>>
>
> That's an interesting idea.  However, in this case, I think we need to
> pass the build options (from the parsed command-line arguments) along
> somehow.  How should we set the build options when using the declarative
> 'computed-file' procedure?  It seems like the most obvious way would be
> to pass the build options in as arguments to the 'computed-file'
> procedure, but is there a better way?

Which build options?  There’s a direct correspondence between, say,
‘gexp->derivation’ and ‘computed-file’ and the arguments are essentially
the same.  Unless I’m overlooking something (again!), the change I
suggest above is a mechanical change.

HTH,
Ludo’.

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

[Chris Marusich]

Hi Ludo,

l...@gnu.org (Ludovic Courtès) writes:

> Hello,
>
> Chris Marusich  skribis:
>
>> l...@gnu.org (Ludovic Courtès) writes:
>
> [...]
>
>>> 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.
>
> Good point.  This ‘store-fs’ argument was added in response to
> .
>
>> Here's an example of what it contains today:
>>
>> (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")))
>>
>> 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?
>
> Yes, I think so.
>
> More precisely, I think we need to add a ‘device’ field to ,
> which could be the UUID or label of the device where the kernel and
> initrd are to be found, or #f, in which case grub.cfg would contain a
> “search --file” command (instead of “search --label” or “search
> --fs-uuid”).
>
> Correspondingly, we’d add a ‘device’ (or ‘boot-device’?) field to
> .  The key is that ‘device’ can be different from
> ‘root-device’ because the store and root devices can be different from
> one another.
>
> That way we could remove the ‘store-fs’ parameter of
> ‘grub-configuration-file’ since that information would now be contained
> in each .
>

That sounds promising!  I'll try that approach.

>
>> 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?
>
> I think we don’t need to bump the version number: ‘read-boot-parameters’
> can simply do what it currently does for ‘initrd’ and
> ‘kernel-arguments’, which is to provide a default value when they’re
> missing.  Here the default value could be ‘root-device’.

I think you're probably right about this, too.  I'll try it that way.

>
>> 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.
>
> AFAICT an MTA is not needed.
>

I'll let you know if it works!

>
 -  "Return the GRUB configuration file corresponding to CONFIG, a
 - object, and where the store is available at 
 STORE-FS, a
 - 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  object, and where the store is 
 available at
 +STORE-FS, a  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 t

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

[Ludovic Courtès]

Hello,

Chris Marusich  skribis:

> l...@gnu.org (Ludovic Courtès) writes:

[...]

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

Good point.  This ‘store-fs’ argument was added in response to
.

> Here's an example of what it contains today:
>
> (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")))
>
> 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?

Yes, I think so.

More precisely, I think we need to add a ‘device’ field to ,
which could be the UUID or label of the device where the kernel and
initrd are to be found, or #f, in which case grub.cfg would contain a
“search --file” command (instead of “search --label” or “search
--fs-uuid”).

Correspondingly, we’d add a ‘device’ (or ‘boot-device’?) field to
.  The key is that ‘device’ can be different from
‘root-device’ because the store and root devices can be different from
one another.

That way we could remove the ‘store-fs’ parameter of
‘grub-configuration-file’ since that information would now be contained
in each .

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

I think we don’t need to bump the version number: ‘read-boot-parameters’
can simply do what it currently does for ‘initrd’ and
‘kernel-arguments’, which is to provide a default value when they’re
missing.  Here the default value could be ‘root-device’.

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

AFAICT an MTA is not needed.

>>> -  "Return the GRUB configuration file corresponding to CONFIG, a
>>> - object, and where the store is available at STORE-FS, 
>>> a
>>> - 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  object, and where the store is available 
>>> at
>>> +STORE-FS, a  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 v

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

[Chris Marusich]

Hi,

l...@gnu.org (Ludovic Courtès) writes:

> Hi!
>
> Chris Marusich  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 
>> 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 
>> 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

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

[Ludovic Courtès]

Hi!

Chris Marusich  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 
> 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 
> 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 
> 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 
> 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
> - object, and where the store is available at STORE-FS, a
> - object.  OLD-ENTRIES is taken to be a list of menu entries
> -correspondi