From mboxrd@z Thu Jan 1 00:00:00 1970 From: ludo@gnu.org (Ludovic =?utf-8?Q?Court=C3=A8s?=) Subject: Re: Patches to implement system roll-back and switch-generation Date: Tue, 04 Oct 2016 12:01:03 +0200 Message-ID: <8737kcbfhc.fsf@gnu.org> References: <87a8ep6h6g.fsf@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Return-path: Received: from eggs.gnu.org ([2001:4830:134:3::10]:52165) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1brMX9-0000Ma-Tg for guix-devel@gnu.org; Tue, 04 Oct 2016 06:01:18 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1brMX5-0005as-MH for guix-devel@gnu.org; Tue, 04 Oct 2016 06:01:10 -0400 In-Reply-To: <87a8ep6h6g.fsf@gmail.com> (Chris Marusich's message of "Thu, 29 Sep 2016 23:21:59 -0700") List-Id: "Development of GNU Guix and the GNU System distribution." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: guix-devel-bounces+gcggd-guix-devel=m.gmane.org@gnu.org Sender: "Guix-devel" To: Chris Marusich Cc: guix-devel@gnu.org 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=E2=80=99ll need to extract and reuse the logic in =E2=80=98install-grub*=E2=80=99 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=E2=80=99re going to hate me for that, but co= uld you please add ChangeLog-style commit logs? Also, could you send the revised patches using =E2=80=98git send-email=E2=80=99? > 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." >=20=20 > (define (switch-to-generation profile number) > "Atomically switch PROFILE to the generation NUMBER. Return the numbe= r 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 prof= ile, > so its name is confusing if it suggests that it only returns "previous" > entries. OK! Maybe =E2=80=98profile-grub-entries=E2=80=99 would work too, to sugges= t that it=E2=80=99s 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))) >=20=20 > (define* (operating-system-grub.cfg os #:optional (old-entries '())) > - "Return the GRUB configuration file for OS. Use OLD-ENTRIES to popula= te 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-F= S, 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 correspo= nding > +to CONFIG, a object, and where the store is availab= le 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 =E2=80=9CReturn something=E2=80=9D when that rea= lly means =E2=80=9CReturn a derivation that builds something=E2=80=9D. > From 227ffc6c34c7bef29a39b2745865ac25c28a7e74 Mon Sep 17 00:00:00 2001 > From: Chris Marusich > 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 >=20=20 > uuid->string > string->uuid > @@ -364,19 +365,6 @@ the following: > ;; this long. > 20) >=20=20 > - (define canonical-title > - ;; The realm of canonicalization. > - (if (eq? title 'any) > - (if (string? spec) > - ;; The "--root=3DSPEC" kernel command-line option always pro= vides 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)))))))) >=20=20 > + (define canonical-title (if (eq? title 'any) Put the =E2=80=9C(if=E2=80=9D on the next line. > +(define (device->title device) > + "Guess the title for the given DEVICE, which must be a device paramete= r from > +a object. As a special case, when the DEVICE is a UUID, i= t may > +be specified as a string." What about calling it =E2=80=98inferred-device-title=E2=80=99 instead, sinc= e it=E2=80=99s not just a conversion? > From 1ade872ffae08ded1b8dae2fca05fee33ac0c69f Mon Sep 17 00:00:00 2001 > From: Chris Marusich > 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 co= ntains FILE, > a gexp. The result is a gexp that can be inserted in the grub.cfg-gener= ation > 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=E2=80=99m not convinced by this patch. What=E2=80=99s the rationale? To= better deal with cases where the title is 'any? > From 55cf900abf6dba10ed640d24433cc1ca23e4acf2 Mon Sep 17 00:00:00 2001 > From: Chris Marusich > 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 > 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 > 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." >=20=20 > > ;;; > +;;; 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 s= ystem > +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 =E2=80=98let*=E2=80=99. > + (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 t= his > + ;; assumption is ever false, then problems might occur. > + (root-device (boot-parameters-root-device params)) > + ;; Generate a new grub configuration which uses default values f= or > + ;; just about everything. If the specified system generation was > + ;; built from an operating system configuration file that contai= ned > + ;; non-default values in its grub-configuration, then the grub > + ;; configuration we generate here will be different. However, e= ven > + ;; if that is the case, this default configuration will be good > + ;; enough to enable the user to boot into the specified generati= on. > + (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 =E2=80=98show-what-to-build=E2=80=99 before. I=E2=80=99d remov= e =E2=80=9C-derivation=E2=80=9D from the identifier, that=E2=80=99s 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=E2=80=99ll also need a few lines describing these action= s in guix.texi. Thanks a lot for this very useful contribution! Ludo=E2=80=99.