| Message ID | 87muh9q51e.fsf_-_@sdf.lonestar.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | Refactor out common behavior for system reconfiguration. | expand |
zerodaysfordays@sdf.lonestar.org (Jakob L. Kreuze) skribis: > * gnu/tests/reconfigure.scm: New file. > * gnu/local.mk (GNU_SYSTEM_MODULES): Add it. That’s really cool! > + (test-begin "switch-to-system") > + > + (let ((generations-prior (system-generations marionette))) > + (test-assert "script successfully evaluated" > + (marionette-eval > + '(primitive-load #$script) > + marionette)) > + > + (test-equal "script created new generation" > + (length (system-generations marionette)) > + (1+ (length generations-prior)))) Perhaps you could also check the target of /run/current-system, and maybe check things like the set of user accounts (activation code)? > +(define* (run-upgrade-services-test) > + "Run a test of an OS running UPGRADE-SERVICES-PROGRAM, which upgrades the > +Shepherd (PID 1) by unloading obsolete services and loading new services." > + (define os > + (marionette-operating-system > + (simple-operating-system) > + #:imported-modules '((gnu services herd) > + (guix combinators)))) > + > + (define vm (virtual-machine os)) > + > + (define dummy-service > + ;; Shepherd service that does nothing, for the sole purpose of ensuring > + ;; that it is properly installed and started by the script. > + (shepherd-service (provision '(dummy)) > + (start #~(const #t)) > + (stop #~(const #t)) > + (respawn? #f))) > + > + (define (ensure-service-file service) > + "Return the Shepherd service file for SERVICE, after ensuring that it > +exists in the store" No need for docstrings for inner procedures; a comment is enough. > + (test-assert "script started new service" > + (and (not (memq 'dummy services-prior)) > + (memq 'dummy (running-services marionette)))) > + > + (test-assert "script successfully evaluated" > + (marionette-eval > + '(primitive-load #$disable-dummy) > + marionette)) > + > + (test-assert "script stopped new service" ^ s/new/obsolete/, no? Perhaps you could also check for the availability of a “replacement” slot (info "(shepherd) Slots of services") for services that exist both before and after the upgrade? This could be achieved by augmenting (gnu services herd) with a ‘live-service-replacement’ procedure, I think. The rest LGTM! I think you’ve reached the most difficult part of this whole endeavor. The good thing is that, once you’re past this, things will be much easier. Thank you! Ludo’.
Hi, Ludovic! Ludovic Courtès <ludo@gnu.org> writes: > Really nice that it becomes this concise. Yeah, I think (and hope) this is a good sign that we've picked the right abstraction for this :) > I like to avoid exposing constructors so that one cannot “forge” > invalid objects, but let’s see… Should I use @@ for this, perhaps? Aside from one other place in the test suite, it's a one-off use, and the objects are then only used internally. > I wonder it we should just use > > #~(begin (use-modules (guix build utils)) (invoke …)) > > here and in other places. > > That’s probably better longer-term (for example when we switch to > Guile 3, that could ease the transition since the right Guile would be > used) but we can keep it this way and revisit it later. Oh that's a good point, I agree that we should do that. I'll submit a separate patch once this gets merged. > s/remote-exp/exp/ > ... > A leftover? :-) > > These two statements disappeared in the process, but I think they’re > added back by one of the subsequent patches, right? Good catches, thanks! Yes, the code is added back in the commits that follow. > OK, that makes sense here. > > (Once we’ve done that (guix graph) demonadification we discussed > before, perhaps we can perform run ‘shepherd-service-upgrade’ entirely > on the “other side”, and at that point we won’t need to expose the > ‘live-service’ constructor.) The main issue with calling 'shepherd-service-upgrade' on the other side is that we'd need to send over the service objects (the current 'upgrade-services-program' deals with provision symbols rather than the service objects themselves). I'm certain it's possible, it's just easier said than done. I've got time to think it through, though :) > No need to repeat the file name here. > > However there are other changes no mentioned here, for example changes > to the ‘install’ procedure. Could you add them to the log? > > While you’re at it, could you change it to: > > (info (G_ "bootloader successfully installed on '~a'~%") …) > > ? Yep, sure thing. > What happens when ‘install-bootloader’ fails though? We should make > sure that the error is diagnosed, and that the output of > ‘grub-install’ or similar is shown when that happens. > Note that there are now a few places where we call ‘built-derivations’ > without calling ‘show-what-to-build*’ first. That means the UX might > be pretty bad since one has no idea what’s being built. > > Furthermore, that means substitutes may not be up-to-date, leading to > many “updating substitutes” messages and HTTP round trips (as happened > with <https://issues.guix.gnu.org/issue/36509>). > > Last, doing several ‘build-derivations’ call with just a couple of > derivations is less efficient than doing a single call with many > derivations; that also has an impact on the UI, if we were to call > ‘show-what-to-build*’ once for ‘build-derivations’ call. > > What’s your experience with this in practice? I haven't had too many issues with it since the G-Expressions tended to have few inputs, but those are some valid concerns. Would it be better to create derivations for locally-evaluated G-Expressions? For example, with 'program-file' or 'gexp->script'? I thought that evaluating them in-place might be better since that's one fewer store item that needs to be built, but if we were to turn the G-Expression into a derivation, we could add it to the call to 'show-what-to-build*' in 'guix system reconfigure'. > Eventually we should add it to (guix gexp). Yeah, that seems to make more sense. I can move it when I address the above. > Last but not least, make sure to test this on your machine. :-) > > It’s sensitive code that we’d rather not break. Heh, indeed! I've run it several times in a virtual machine, but running it on my desktop is the ultimate "I promise this works, and if it doesn't, I'll eat my hat." I'll do an update on this machine and report back. > Perhaps you could also check the target of /run/current-system, and > maybe check things like the set of user accounts (activation code)? > > Perhaps you could also check for the availability of a “replacement” > slot (info "(shepherd) Slots of services") for services that exist > both before and after the upgrade? This could be achieved by > augmenting (gnu services herd) with a ‘live-service-replacement’ > procedure, I think. Great ideas! In the interest of keeping this patch manageable, I'll submit these improvements separately. > No need for docstrings for inner procedures; a comment is enough. > ... > s/new/obsolete/, no? I can address these in my corrections, though. > I think you’ve reached the most difficult part of this whole endeavor. > The good thing is that, once you’re past this, things will be much > easier. Agreed, I think this gives us a good framework for implementing provisioning etc. Regards, Jakob
zerodaysfordays@sdf.lonestar.org (Jakob L. Kreuze) writes: >> What happens when ‘install-bootloader’ fails though? We should make >> sure that the error is diagnosed, and that the output of >> ‘grub-install’ or similar is shown when that happens. Apologies, forgot to respond to this point. This is handled in 'local-eval'. (guard (c ((message-condition? c) (leave (G_ "failed to install bootloader:~%~a~%") (condition-message c)))) ...
I'm feeling pretty good about this :) jakob@Epsilon ~/Code/guix [env] $ sudo -E ./pre-inst-env guix system reconfigure ~/.config/guix/system/config.scm substitute: updating substitutes from 'https://ci.guix.gnu.org'... 100.0% The following derivation will be built: /gnu/store/327py2dv6xjlm0xanqiqj1paxxx8g1rq-grub.cfg.drv building /gnu/store/327py2dv6xjlm0xanqiqj1paxxx8g1rq-grub.cfg.drv... /gnu/store/h45l455dg3wi6b24m0v8as5wdjskpfsm-system /gnu/store/razfpshw9n33dvm4bp0d2jwpdf4255hf-grub.cfg activating system... making '/gnu/store/h45l455dg3wi6b24m0v8as5wdjskpfsm-system' the current system... setting up setuid programs in '/run/setuid-programs'... populating /etc from /gnu/store/glzrd1cb6ngzwqvnph3q3pbxxjv8nprs-etc... substitute: updating substitutes from 'https://ci.guix.gnu.org'... 100.0% building /gnu/store/8vn3dlcmhri0f3ygfhqavlab2q35q2yn-install-bootloader.scm.drv... guix system: bootloader successfully installed on '/dev/sda' substitute: updating substitutes from 'https://ci.guix.gnu.org'... 100.0% building /gnu/store/43cyy0nnrdr6wg9xzcph6shs4w7gfxi6-upgrade-shepherd-services.scm.drv... shepherd: Evaluating user expression (let* ((services (map primitive-load (?))) # ?) ?). Jakob L. Kreuze (3): guix system: Add 'reconfigure' module. guix system: Reimplement 'reconfigure'. tests: Add reconfigure system test. Makefile.am | 1 + gnu/local.mk | 1 + gnu/machine/ssh.scm | 189 ++------------------ gnu/services/herd.scm | 6 + gnu/tests/reconfigure.scm | 262 ++++++++++++++++++++++++++++ guix/scripts/system.scm | 186 +++++--------------- guix/scripts/system/reconfigure.scm | 237 +++++++++++++++++++++++++ tests/services.scm | 4 - 8 files changed, 560 insertions(+), 326 deletions(-) create mode 100644 gnu/tests/reconfigure.scm create mode 100644 guix/scripts/system/reconfigure.scm
Hello, zerodaysfordays@sdf.lonestar.org (Jakob L. Kreuze) skribis: > Ludovic Courtès <ludo@gnu.org> writes: [...] >> I like to avoid exposing constructors so that one cannot “forge” >> invalid objects, but let’s see… > > Should I use @@ for this, perhaps? No, it’s not any better ;-), but anyway, let’s address this later. >> (Once we’ve done that (guix graph) demonadification we discussed >> before, perhaps we can perform run ‘shepherd-service-upgrade’ entirely >> on the “other side”, and at that point we won’t need to expose the >> ‘live-service’ constructor.) > > The main issue with calling 'shepherd-service-upgrade' on the other side > is that we'd need to send over the service objects (the current > 'upgrade-services-program' deals with provision symbols rather than the > service objects themselves). > > I'm certain it's possible, it's just easier said than done. I've got > time to think it through, though :) Oh, you may be right. :-) >> What happens when ‘install-bootloader’ fails though? We should make >> sure that the error is diagnosed, and that the output of >> ‘grub-install’ or similar is shown when that happens. I think you didn’t answer this specific question; thoughts? >> Note that there are now a few places where we call ‘built-derivations’ >> without calling ‘show-what-to-build*’ first. That means the UX might >> be pretty bad since one has no idea what’s being built. >> >> Furthermore, that means substitutes may not be up-to-date, leading to >> many “updating substitutes” messages and HTTP round trips (as happened >> with <https://issues.guix.gnu.org/issue/36509>). >> >> Last, doing several ‘build-derivations’ call with just a couple of >> derivations is less efficient than doing a single call with many >> derivations; that also has an impact on the UI, if we were to call >> ‘show-what-to-build*’ once for ‘build-derivations’ call. >> >> What’s your experience with this in practice? > > I haven't had too many issues with it since the G-Expressions tended to > have few inputs, but those are some valid concerns. Would it be better > to create derivations for locally-evaluated G-Expressions? For example, > with 'program-file' or 'gexp->script'? I thought that evaluating them > in-place might be better since that's one fewer store item that needs to > be built, but if we were to turn the G-Expression into a derivation, we > could add it to the call to 'show-what-to-build*' in 'guix system > reconfigure'. The number of ‘build-derivations’ calls is the same whether it’s local or distant. What would make a difference is having a single script instead of three—i.e., one program that does: #~(begin (activate-system …) (upgrade-services …) (switch-system …)) I think this program could even be added to the ‘system’ derivation—i.e., as a file next to those in /run/current-system. That way, switching to a system generation would be a matter of running it’s ‘switch’ program. Perhaps this should be our horizon. WDYT? Thanks for your feedback! Ludo’.
Ludovic Courtès <ludo@gnu.org> writes: > I think you didn’t answer this specific question; thoughts? I had a peek at your more recent email, and think you dug up (and commented on) my handling of it, but I'll link [1] just in case. > The number of ‘build-derivations’ calls is the same whether it’s local > or distant. > > What would make a difference is having a single script instead of > three—i.e., one program that does: > > #~(begin > (activate-system …) > (upgrade-services …) > (switch-system …)) > > I think this program could even be added to the ‘system’ > derivation—i.e., as a file next to those in /run/current-system. > > That way, switching to a system generation would be a matter of running > it’s ‘switch’ program. > > Perhaps this should be our horizon. WDYT? I'm a fan of that idea. Having it as a file means we would be able to run activation services on a roll-back. I've added this to my to-do list of patches :) Regards, Jakob [1]: https://lists.gnu.org/archive/html/guix-patches/2019-07/msg00656.html
zerodaysfordays@sdf.lonestar.org (Jakob L. Kreuze) skribis: > Ludovic Courtès <ludo@gnu.org> writes: > >> I think you didn’t answer this specific question; thoughts? > > I had a peek at your more recent email, and think you dug up (and > commented on) my handling of it, but I'll link [1] just in case. Yup, sorry for the confusion! Ludo’.
diff --git a/gnu/local.mk b/gnu/local.mk index 0e17af953..b334d0572 100644 --- a/gnu/local.mk +++ b/gnu/local.mk @@ -592,6 +592,7 @@ GNU_SYSTEM_MODULES = \ %D%/tests/mail.scm \ %D%/tests/messaging.scm \ %D%/tests/networking.scm \ + %D%/tests/reconfigure.scm \ %D%/tests/rsync.scm \ %D%/tests/security-token.scm \ %D%/tests/singularity.scm \ diff --git a/gnu/tests/reconfigure.scm b/gnu/tests/reconfigure.scm new file mode 100644 index 000000000..022492e05 --- /dev/null +++ b/gnu/tests/reconfigure.scm @@ -0,0 +1,263 @@ +;;; GNU Guix --- Functional package management for GNU +;;; Copyright © 2019 Jakob L. Kreuze <zerodaysfordays@sdf.lonestar.org> +;;; +;;; This file is part of GNU Guix. +;;; +;;; GNU Guix is free software; you can redistribute it and/or modify it +;;; under the terms of the GNU General Public License as published by +;;; the Free Software Foundation; either version 3 of the License, or (at +;;; your option) any later version. +;;; +;;; GNU Guix is distributed in the hope that it will be useful, but +;;; WITHOUT ANY WARRANTY; without even the implied warranty of +;;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +;;; GNU General Public License for more details. +;;; +;;; You should have received a copy of the GNU General Public License +;;; along with GNU Guix. If not, see <http://www.gnu.org/licenses/>. + +(define-module (gnu tests reconfigure) + #:use-module (gnu bootloader) + #:use-module (gnu services shepherd) + #:use-module (gnu system vm) + #:use-module (gnu system) + #:use-module (gnu tests) + #:use-module (guix derivations) + #:use-module (guix gexp) + #:use-module (guix monads) + #:use-module (guix scripts system reconfigure) + #:use-module (guix store) + #:export (%test-switch-to-system + %test-upgrade-services + %test-install-bootloader)) + +;;; Commentary: +;;; +;;; Test in-place system reconfiguration: advancing the system generation on a +;;; running instance of the Guix System. +;;; +;;; Code: + +(define* (run-switch-to-system-test) + "Run a test of an OS running SWITCH-SYSTEM-PROGRAM, which creates a new +generation of the system profile." + (define os + (marionette-operating-system + (simple-operating-system) + #:imported-modules '((gnu services herd) + (guix combinators)))) + + (define vm (virtual-machine os)) + + (define (test script) + (with-imported-modules '((gnu build marionette)) + #~(begin + (use-modules (gnu build marionette) + (srfi srfi-64)) + + (define marionette + (make-marionette (list #$vm))) + + (define (system-generations marionette) + "Return the names of the generation symlinks on MARIONETTE." + (marionette-eval + '(begin + (use-modules (ice-9 ftw) + (srfi srfi-1)) + (let* ((profile-dir "/var/guix/profiles/") + (entries (map first (cddr (file-system-tree profile-dir))))) + (remove (lambda (entry) + (member entry '("per-user" "system"))) + entries))) + marionette)) + + (mkdir #$output) + (chdir #$output) + + (test-begin "switch-to-system") + + (let ((generations-prior (system-generations marionette))) + (test-assert "script successfully evaluated" + (marionette-eval + '(primitive-load #$script) + marionette)) + + (test-equal "script created new generation" + (length (system-generations marionette)) + (1+ (length generations-prior)))) + + (test-end) + (exit (= (test-runner-fail-count (test-runner-current)) 0))))) + + (gexp->derivation "switch-to-system" (test (switch-system-program os)))) + +(define* (run-upgrade-services-test) + "Run a test of an OS running UPGRADE-SERVICES-PROGRAM, which upgrades the +Shepherd (PID 1) by unloading obsolete services and loading new services." + (define os + (marionette-operating-system + (simple-operating-system) + #:imported-modules '((gnu services herd) + (guix combinators)))) + + (define vm (virtual-machine os)) + + (define dummy-service + ;; Shepherd service that does nothing, for the sole purpose of ensuring + ;; that it is properly installed and started by the script. + (shepherd-service (provision '(dummy)) + (start #~(const #t)) + (stop #~(const #t)) + (respawn? #f))) + + (define (ensure-service-file service) + "Return the Shepherd service file for SERVICE, after ensuring that it +exists in the store" + (let ((file (shepherd-service-file service))) + (mlet* %store-monad ((store-object (lower-object file)) + (_ (built-derivations (list store-object)))) + (return file)))) + + (define (test enable-dummy disable-dummy) + (with-imported-modules '((gnu build marionette)) + #~(begin + (use-modules (gnu build marionette) + (srfi srfi-64)) + + (define marionette + (make-marionette (list #$vm))) + + (define (running-services marionette) + "Return the names of the running services on MARIONETTE." + (marionette-eval + '(begin + (use-modules (gnu services herd)) + (map live-service-canonical-name (current-services))) + marionette)) + + (mkdir #$output) + (chdir #$output) + + (test-begin "upgrade-services") + + (let ((services-prior (running-services marionette))) + (test-assert "script successfully evaluated" + (marionette-eval + '(primitive-load #$enable-dummy) + marionette)) + + (test-assert "script started new service" + (and (not (memq 'dummy services-prior)) + (memq 'dummy (running-services marionette)))) + + (test-assert "script successfully evaluated" + (marionette-eval + '(primitive-load #$disable-dummy) + marionette)) + + (test-assert "script stopped new service" + (not (memq 'dummy (running-services marionette))))) + + (test-end) + (exit (= (test-runner-fail-count (test-runner-current)) 0))))) + + (mlet* %store-monad ((file (ensure-service-file dummy-service))) + (let ((enable (upgrade-services-program (list file) '(dummy) '() '())) + (disable (upgrade-services-program '() '() '(dummy) '()))) + (gexp->derivation "upgrade-services" (test enable disable))))) + +(define* (run-install-bootloader-test) + "Run a test of an OS running INSTALL-BOOTLOADER-PROGRAM, which installs a +bootloader's configuration file." + (define os + (marionette-operating-system + (simple-operating-system) + #:imported-modules '((gnu services herd) + (guix combinators)))) + + (define vm (virtual-machine os)) + + (define (test script) + (with-imported-modules '((gnu build marionette)) + #~(begin + (use-modules (gnu build marionette) + (ice-9 regex) + (srfi srfi-1) + (srfi srfi-64)) + + (define marionette + (make-marionette (list #$vm))) + + (define (generations-in-grub-cfg marionette) + "Return the system generation paths that have GRUB menu entries." + (let ((grub-cfg (marionette-eval + '(begin + (call-with-input-file "/boot/grub/grub.cfg" + (lambda (port) + (get-string-all port)))) + marionette))) + (map (lambda (parameter) + (second (string-split (match:substring parameter) #\=))) + (list-matches "system=[^ ]*" grub-cfg)))) + + (mkdir #$output) + (chdir #$output) + + (test-begin "install-bootloader") + + + (test-assert "no prior menu entry for system generation" + (not (member #$os (generations-in-grub-cfg marionette)))) + + (test-assert "script successfully evaluated" + (marionette-eval + '(primitive-load #$script) + marionette)) + + (test-assert "menu entry created for system generation" + (member #$os (generations-in-grub-cfg marionette))) + + (test-end) + (exit (= (test-runner-fail-count (test-runner-current)) 0))))) + + (let* ((bootloader ((compose bootloader-configuration-bootloader + operating-system-bootloader) + os)) + ;; The typical use-case for 'install-bootloader-program' is to read + ;; the boot parameters for the existing menu entries on the system, + ;; parse them with 'boot-parameters->menu-entry', and pass the + ;; results to 'operating-system-bootcfg'. However, to obtain boot + ;; parameters, we would need to start the marionette, which we should + ;; ideally avoid doing outside of the 'test' G-Expression. Thus, we + ;; generate a bootloader configuration for the script as if there + ;; were no existing menu entries. In the grand scheme of things, this + ;; matters little -- these tests should not make assertions about the + ;; behavior of 'operating-system-bootcfg'. + (bootcfg (operating-system-bootcfg os '())) + (bootcfg-file (bootloader-configuration-file bootloader))) + (gexp->derivation + "install-bootloader" + ;; Due to the read-only nature of the virtual machines used in the system + ;; test suite, the bootloader installer script is omitted. 'grub-install' + ;; would attempt to write directly to the virtual disk if the + ;; installation script were run. + (test (install-bootloader-program #f #f bootcfg bootcfg-file #f "/"))))) + +(define %test-switch-to-system + (system-test + (name "switch-to-system") + (description "Create a new generation of the system profile.") + (value (run-switch-to-system-test)))) + +(define %test-upgrade-services + (system-test + (name "upgrade-services") + (description "Upgrade the Shepherd by unloading obsolete services and +loading new services.") + (value (run-upgrade-services-test)))) + +(define %test-install-bootloader + (system-test + (name "install-bootloader") + (description "Install a bootloader and its configuration file.") + (value (run-install-bootloader-test))))