Update: Implementing guix system rollback / switch-generation

Update: Implementing guix system rollback / switch-generation

[Chris Marusich]

[-- Attachment #1.1: Type: text/plain, Size: 2014 bytes --]

Hi Guix,

I've attached a preliminary patch which adds rudimentary roll-back and
switch-generation commands to "guix system".  Currently, the commands
just flip symlinks, which is part of the first milestone that Ludo
suggested in the following thread:

https://lists.gnu.org/archive/html/guix-devel/2016-06/msg00178.html

Please let me know what you think.  After getting feedback, I intend to
follow up a more complete patch that contains the following additional
changes:

* Regenerate grub.cfg, to complete the first milestone.  I'm not sure
  exactly how this will play out, but I imagine I'll need to use the
  store somehow to get it done.

* Use a properly formatted ChangeLog-style Git commit message.

* Mention these new commands in the manual.

* Add tests.  I'm not new to automated testing, but I haven't done it
  using autoconf/make or in Guile, so tips here would be welcome!

* Add these new commands to the emacs interface.

I've tested the changes on my bare metal GuixSD installation.  I would
have used a VM, but my system has trouble launching VMs.  I first
verified manually that the roll-back command switched the profile's
symlink to the previous generation, by examining the output of
list-generations and ls.  I then verified, in the same way, that the
switch-generation command switched the profile's symlink to the
specified generation.  I verified that switch-generation works for
arbitrary numbers and relative numbers (note that a negative relative
number must be preceded by the special "--" argument to prevent it from
being interpreted as an option).  I also verified that if the user
attempts to switch to a nonexistent generation (or to roll back from
generation 1 in the case of "guix system"), an error is raised without
modifying anything.  I repeated this manual verification process for
both the "guix system" and "guix package" commands, since my changes
involve some refactoring in "guix package".

Thank you,

-- 
Chris


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: 0001-Implement-system-roll-back-and-switch-generation-com.patch --]
[-- Type: text/x-patch, Size: 8824 bytes --]

From 00f8b1ccea6c80e37535cd16b05bafbb9cf3686b Mon Sep 17 00:00:00 2001
From: Chris Marusich <cmmarusich@gmail.com>
Date: Fri, 8 Jul 2016 13:49:25 -0700
Subject: [PATCH] Implement system roll-back and switch-generation commands

Right now, they just switch symlinks.
---
 guix/profiles.scm        | 21 ++++++++++++++++-
 guix/scripts/package.scm |  7 +-----
 guix/scripts/system.scm  | 59 +++++++++++++++++++++++++++++++++++++-----------
 guix/ui.scm              |  8 +++++++
 4 files changed, 75 insertions(+), 20 deletions(-)

diff --git a/guix/profiles.scm b/guix/profiles.scm
index 90c4332..9b0ce7f 100644
--- a/guix/profiles.scm
+++ b/guix/profiles.scm
@@ -90,11 +90,13 @@
             generation-number
             generation-numbers
             profile-generations
+            relative-generation-spec->number
             relative-generation
             previous-generation-number
             generation-time
             generation-file-name
             switch-to-generation
+            switch-to-previous-generation
             roll-back
             delete-generation))
 
@@ -896,6 +898,21 @@ former profiles were found."
         '()
         generations)))
 
+(define (relative-generation-spec->number profile spec)
+  "Return PROFILE's generation specified by SPEC, which is a string.  The SPEC
+may be a N, -N, or +N, where N is a number.  If the spec is N, then the number
+returned is N.  If it is -N, then the number returned is the profile's current
+generation number minus N.  If it is +N, then the number returned is the
+profile's current generation number plus N.  Return #f if there is no such
+generation."
+  (let ((number (string->number spec)))
+    (and number
+         (case (string-ref spec 0)
+           ((#\+ #\-)
+            (relative-generation profile number))
+           (else number)))))
+
+
 (define* (relative-generation profile shift #:optional
                               (current (generation-number profile)))
   "Return PROFILE's generation shifted from the CURRENT generation by SHIFT.
@@ -939,7 +956,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)))
     (cond ((not (file-exists? profile))
diff --git a/guix/scripts/package.scm b/guix/scripts/package.scm
index e2e3709..8df5145 100644
--- a/guix/scripts/package.scm
+++ b/guix/scripts/package.scm
@@ -743,12 +743,7 @@ processed, #f otherwise."
                                    #:key dry-run?)
   "Switch PROFILE to the generation specified by SPEC."
   (unless dry-run?
-    (let* ((number (string->number spec))
-           (number (and number
-                        (case (string-ref spec 0)
-                          ((#\+ #\-)
-                           (relative-generation profile number))
-                          (else number)))))
+    (let ((number (relative-generation-spec->number profile spec)))
       (if number
           (switch-to-generation* profile number)
           (leave (_ "cannot switch to generation '~a'~%") spec)))))
diff --git a/guix/scripts/system.scm b/guix/scripts/system.scm
index dd1e534..3548d54 100644
--- a/guix/scripts/system.scm
+++ b/guix/scripts/system.scm
@@ -418,6 +418,23 @@ it atomically, and then run OS's activation script."
 
 \f
 ;;;
+;;; Roll-back.
+;;;
+(define (roll-back-system)
+  "Roll back the system profile to its previous generation."
+  (switch-to-previous-generation* %system-profile))
+\f
+;;;
+;;; Switch generations.
+;;;
+(define (switch-to-system-generation spec)
+  "Switch the system profile to the generation specified by SPEC."
+  (let ((number (relative-generation-spec->number %system-profile spec)))
+      (if number
+          (switch-to-generation* %system-profile number)
+          (leave (_ "cannot switch to system generation '~a'~%") spec))))
+\f
+;;;
 ;;; Graphs.
 ;;;
 
@@ -649,31 +666,36 @@ building anything."
 ;;;
 
 (define (show-help)
-  (display (_ "Usage: guix system [OPTION] ACTION [FILE]
-Build the operating system declared in FILE according to ACTION.\n"))
+  (display (_ "Usage: guix system [OPTION ...] ACTION [ARG ...] [FILE]
+Build the operating system declared in FILE according to ACTION.
+Some ACTIONS support additional ARGS.\n"))
   (newline)
   (display (_ "The valid values for ACTION are:\n"))
   (newline)
   (display (_ "\
-   reconfigure      switch to a new operating system configuration\n"))
+   reconfigure       switch to a new operating system configuration\n"))
+  (display (_ "\
+   roll-back         switch to the previous operating system configuration\n"))
   (display (_ "\
-   list-generations list the system generations\n"))
+   switch-generation switch to a generation matching a pattern\n"))
   (display (_ "\
-   build            build the operating system without installing anything\n"))
+   list-generations  list the system generations\n"))
   (display (_ "\
-   container        build a container that shares the host's store\n"))
+   build             build the operating system without installing anything\n"))
   (display (_ "\
-   vm               build a virtual machine image that shares the host's store\n"))
+   container         build a container that shares the host's store\n"))
   (display (_ "\
-   vm-image         build a freestanding virtual machine image\n"))
+   vm                build a virtual machine image that shares the host's store\n"))
   (display (_ "\
-   disk-image       build a disk image, suitable for a USB stick\n"))
+   vm-image          build a freestanding virtual machine image\n"))
   (display (_ "\
-   init             initialize a root file system to run GNU\n"))
+   disk-image        build a disk image, suitable for a USB stick\n"))
   (display (_ "\
-   extension-graph  emit the service extension graph in Dot format\n"))
+   init              initialize a root file system to run GNU\n"))
   (display (_ "\
-   shepherd-graph   emit the graph of shepherd services in Dot format\n"))
+   extension-graph   emit the service extension graph in Dot format\n"))
+  (display (_ "\
+   shepherd-graph    emit the graph of shepherd services in Dot format\n"))
 
   (show-build-options-help)
   (display (_ "
@@ -824,6 +846,16 @@ argument list and OPTS is the option alist."
                       ((pattern) pattern)
                       (x (leave (_ "wrong number of arguments~%"))))))
        (list-generations pattern)))
+    ((roll-back)
+     (let ((pattern (match args
+                      (() "")
+                      (x (leave (_ "wrong number of arguments~%"))))))
+       (roll-back-system)))
+    ((switch-generation)
+     (let ((pattern (match args
+                      ((pattern) pattern)
+                      (x (leave (_ "wrong number of arguments~%"))))))
+       (switch-to-system-generation pattern)))
     (else
      (process-action command args opts))))
 
@@ -835,7 +867,8 @@ argument list and OPTS is the option alist."
         (let ((action (string->symbol arg)))
           (case action
             ((build container vm vm-image disk-image reconfigure init
-              extension-graph shepherd-graph list-generations)
+              extension-graph shepherd-graph list-generations roll-back
+              switch-generation)
              (alist-cons 'action action result))
             (else (leave (_ "~a: unknown action~%") action))))))
 
diff --git a/guix/ui.scm b/guix/ui.scm
index 4d1b65c..fcf403c 100644
--- a/guix/ui.scm
+++ b/guix/ui.scm
@@ -90,6 +90,7 @@
             display-generation
             display-profile-content
             roll-back*
+            switch-to-previous-generation*
             switch-to-generation*
             delete-generation*
             run-guix-command
@@ -1095,6 +1096,13 @@ way."
         (roll-back store profile))
     display-generation-change))
 
+(define (switch-to-previous-generation* profile)
+  "Like switch-to-previous-generation, but display what is happening."
+  (call-with-values
+      (lambda ()
+        (switch-to-previous-generation profile))
+    display-generation-change))
+
 (define (switch-to-generation* profile number)
   "Like 'switch-generation', but display what is happening."
   (let ((previous (switch-to-generation profile number)))
-- 
2.7.3


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: Update: Implementing guix system rollback / switch-generation

[Ludovic Courtès]

Hello Chris,

Chris Marusich <cmmarusich@gmail.com> skribis:

> I've attached a preliminary patch which adds rudimentary roll-back and
> switch-generation commands to "guix system".  Currently, the commands
> just flip symlinks, which is part of the first milestone that Ludo
> suggested in the following thread:
>
> https://lists.gnu.org/archive/html/guix-devel/2016-06/msg00178.html

Awesome!

> Please let me know what you think.  After getting feedback, I intend to
> follow up a more complete patch that contains the following additional
> changes:
>
> * Regenerate grub.cfg, to complete the first milestone.  I'm not sure
>   exactly how this will play out, but I imagine I'll need to use the
>   store somehow to get it done.
>
> * Use a properly formatted ChangeLog-style Git commit message.
>
> * Mention these new commands in the manual.
>
> * Add tests.  I'm not new to automated testing, but I haven't done it
>   using autoconf/make or in Guile, so tips here would be welcome!
>
> * Add these new commands to the emacs interface.

Sounds good to me.

Automated tests for this will be a bit difficult because we don’t have
any ‘guix system’ tests yet.  I think this should be done in the new
system test infrastructure.

However, since you’ve done extensive manual testing, this part shouldn’t
block you.  We can add it at a later point.

> I've tested the changes on my bare metal GuixSD installation.  I would
> have used a VM, but my system has trouble launching VMs.  I first
> verified manually that the roll-back command switched the profile's
> symlink to the previous generation, by examining the output of
> list-generations and ls.  I then verified, in the same way, that the
> switch-generation command switched the profile's symlink to the
> specified generation.  I verified that switch-generation works for
> arbitrary numbers and relative numbers (note that a negative relative
> number must be preceded by the special "--" argument to prevent it from
> being interpreted as an option).  I also verified that if the user
> attempts to switch to a nonexistent generation (or to roll back from
> generation 1 in the case of "guix system"), an error is raised without
> modifying anything.  I repeated this manual verification process for
> both the "guix system" and "guix package" commands, since my changes
> involve some refactoring in "guix package".

Perfect.

> +(define (relative-generation-spec->number profile spec)
> +  "Return PROFILE's generation specified by SPEC, which is a string.  The SPEC
> +may be a N, -N, or +N, where N is a number.  If the spec is N, then the number
> +returned is N.  If it is -N, then the number returned is the profile's current
> +generation number minus N.  If it is +N, then the number returned is the
> +profile's current generation number plus N.  Return #f if there is no such
> +generation."
> +  (let ((number (string->number spec)))
> +    (and number
> +         (case (string-ref spec 0)
> +           ((#\+ #\-)
> +            (relative-generation profile number))
> +           (else number)))))

(Refactored from (guix scripts package).)  Could you make this
refactoring in a separate patch?  It LGTM.

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

Could you add the docstring in a separate patch?

(Isolating changes like this can be tedious but it simplifies review and
bisecting should a regression be introduced.)

>  (define (show-help)
> -  (display (_ "Usage: guix system [OPTION] ACTION [FILE]
> -Build the operating system declared in FILE according to ACTION.\n"))
> +  (display (_ "Usage: guix system [OPTION ...] ACTION [ARG ...] [FILE]
> +Build the operating system declared in FILE according to ACTION.
> +Some ACTIONS support additional ARGS.\n"))
>    (newline)
>    (display (_ "The valid values for ACTION are:\n"))
>    (newline)
>    (display (_ "\
> -   reconfigure      switch to a new operating system configuration\n"))
> +   reconfigure       switch to a new operating system configuration\n"))
> +  (display (_ "\
> +   roll-back         switch to the previous operating system configuration\n"))
>    (display (_ "\
> -   list-generations list the system generations\n"))
> +   switch-generation switch to a generation matching a pattern\n"))
>    (display (_ "\
> -   build            build the operating system without installing anything\n"))
> +   list-generations  list the system generations\n"))
>    (display (_ "\
> -   container        build a container that shares the host's store\n"))
> +   build             build the operating system without installing anything\n"))
>    (display (_ "\
> -   vm               build a virtual machine image that shares the host's store\n"))
> +   container         build a container that shares the host's store\n"))
>    (display (_ "\
> -   vm-image         build a freestanding virtual machine image\n"))
> +   vm                build a virtual machine image that shares the host's store\n"))
>    (display (_ "\
> -   disk-image       build a disk image, suitable for a USB stick\n"))
> +   vm-image          build a freestanding virtual machine image\n"))
>    (display (_ "\
> -   init             initialize a root file system to run GNU\n"))
> +   disk-image        build a disk image, suitable for a USB stick\n"))
>    (display (_ "\
> -   extension-graph  emit the service extension graph in Dot format\n"))
> +   init              initialize a root file system to run GNU\n"))
>    (display (_ "\
> -   shepherd-graph   emit the graph of shepherd services in Dot format\n"))
> +   extension-graph   emit the service extension graph in Dot format\n"))
> +  (display (_ "\
> +   shepherd-graph    emit the graph of shepherd services in Dot format\n"))

Not sure what happened here.  :-)  Please avoid reformatting such
strings; they are translated so changing them makes translations stale.

Otherwise looks like Milestone #1 is already close to completion!

Thank you,
Ludo’.

Re: Update: Implementing guix system rollback / switch-generation

[Chris Marusich]

[-- Attachment #1: Type: text/plain, Size: 1293 bytes --]

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

> Automated tests for this will be a bit difficult because we don’t have
> any ‘guix system’ tests yet.  I think this should be done in the new
> system test infrastructure.
>
> However, since you’ve done extensive manual testing, this part shouldn’t
> block you.  We can add it at a later point.

OK. I saw your blog post on Savannah about the system test
infrastructure; I'm excited to play around with that!

> (Refactored from (guix scripts package).)  Could you make this
> refactoring in a separate patch?  It LGTM.
> ...
> (Isolating changes like this can be tedious but it simplifies review and
> bisecting should a regression be introduced.)

This makes sense.  I've split them out.  I'll keep this in mind going
forward.

> Not sure what happened here.  :-)  Please avoid reformatting such
> strings; they are translated so changing them makes translations stale.

I intended to modify the alignment of the second column, but the diff
certainly looks ugly, and I forgot that these strings get translated.
Good point; I will not change the existing strings.

> Otherwise looks like Milestone #1 is already close to completion!

Thank you for your help!  Hopefully the rest will be quick.

-- 
Chris

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]