diff mbox

Update: Implementing guix system rollback / switch-generation

Message ID 87d1mnserm.fsf@gmail.com
State New
Headers show

Commit Message

Chris Marusich July 9, 2016, 2:53 a.m. UTC
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,

Comments

Ludovic Courtès July 12, 2016, 3:30 p.m. UTC | #1
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’.
Chris Marusich July 13, 2016, 6:40 a.m. UTC | #2
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.
diff mbox

Patch

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."
 
 
 ;;;
+;;; Roll-back.
+;;;
+(define (roll-back-system)
+  "Roll back the system profile to its previous generation."
+  (switch-to-previous-generation* %system-profile))
+
+;;;
+;;; 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))))
+
+;;;
 ;;; 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