Fix installer restart.

Fix installer restart.

[Mathieu Othacehe]

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


Hello,

Here are two patches that should fix the "Installer fails when
restarted" issue.

For memory, the issue is caused by the store overlay created at
cow-store service start that I was unable to remove. Failing to remove
this overlay, caused the target device to be seen as busy by the kernel
and made further umount calls to fail[1].

I also fixed the "installation failed dialog". There are now two
options:
* "Resume": let the user resume the installation from any step (from the
installation menu)
* "Retry": restart the installer process.

WDYT?

Thanks,

Mathieu


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-installer-Remove-the-cow-store-overlay-after-system-.patch --]
[-- Type: text/x-diff, Size: 2507 bytes --]

From f02ef650526973af55cf1629de1cc48e888c714f Mon Sep 17 00:00:00 2001
From: Mathieu Othacehe <m.othacehe@gmail.com>
Date: Mon, 10 Feb 2020 14:43:05 +0100
Subject: [PATCH 1/2] installer: Remove the cow-store overlay after system
 installation.

* gnu/installer/final.scm (umount-cow-store): New procedure ...,
(install-system): ... used here, to remove the store overlay so that the
target device is not seen as busy during further umount calls.
---
 gnu/installer/final.scm | 25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

diff --git a/gnu/installer/final.scm b/gnu/installer/final.scm
index 1b85900912..b969ec3676 100644
--- a/gnu/installer/final.scm
+++ b/gnu/installer/final.scm
@@ -23,6 +23,7 @@
   #:use-module (gnu installer utils)
   #:use-module (gnu installer user)
   #:use-module (gnu services herd)
+  #:use-module (guix build syscalls)
   #:use-module (guix build utils)
   #:use-module (gnu build accounts)
   #:use-module ((gnu system shadow) #:prefix sys:)
@@ -96,6 +97,15 @@ USERS."
   (write-passwd password (string-append etc "/passwd"))
   (write-shadow shadow (string-append etc "/shadow")))
 
+(define (umount-cow-store)
+  "Remove the store overlay and the bind-mount on /tmp created by the
+cow-store service."
+  (let ((tmp-dir "/remove"))
+    (mkdir-p tmp-dir)
+    (mount (%store-directory) tmp-dir "" MS_MOVE)
+    (umount tmp-dir)
+    (umount "/tmp")))
+
 (define* (install-system locale #:key (users '()))
   "Create /etc/shadow and /etc/passwd on the installation target for USERS.
 Start COW-STORE service on target directory and launch guix install command in
@@ -114,5 +124,16 @@ or #f.  Return #t on success and #f on failure."
     ;; passwords that we've put in there.
     (create-user-database users (%installer-target-dir))
 
-    (start-service 'cow-store (list (%installer-target-dir)))
-    (run-shell-command install-command #:locale locale)))
+    (dynamic-wind
+      (lambda ()
+        (start-service 'cow-store (list (%installer-target-dir))))
+      (lambda ()
+        (run-shell-command install-command #:locale locale))
+      (lambda ()
+        (stop-service 'cow-store)
+        ;; Remove the store overlay created at cow-store service start.
+        ;; Failing to do that will result in further umount calls to fail
+        ;; because the target device is seen as busy. See:
+        ;; https://lists.gnu.org/archive/html/guix-devel/2018-12/msg00161.html.
+        (umount-cow-store)
+        #f))))
-- 
2.25.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-installer-Fix-installer-restart-dialog.patch --]
[-- Type: text/x-diff, Size: 2340 bytes --]

From 50884fca871cfceb020d63e556c957163fdd6064 Mon Sep 17 00:00:00 2001
From: Mathieu Othacehe <m.othacehe@gmail.com>
Date: Mon, 10 Feb 2020 14:47:56 +0100
Subject: [PATCH 2/2] installer: Fix installer restart dialog.

* gnu/installer/newt/final.scm (run-install-failed-page): Propose between
installer resume or restart. Do actually resume the installation by raising an
&installer-step-abort condition if "Resume" button is pressed. Otherwise, keep
going as the installer will be restarted by login.
* gnu/installer.scm (installer-program): Remove the associated TODO comment.
---
 gnu/installer.scm            |  5 +++--
 gnu/installer/newt/final.scm | 15 +++++++++------
 2 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/gnu/installer.scm b/gnu/installer.scm
index 1676a91801..806f80ec9c 100644
--- a/gnu/installer.scm
+++ b/gnu/installer.scm
@@ -389,8 +389,9 @@ selected keymap."
                      ;; We did it!  Let's reboot!
                      (sync)
                      (stop-service 'root))
-                    (_                            ;installation failed
-                     ;; TODO: Honor the result of 'run-install-failed-page'.
+                    (_
+                     ;; The installation failed, exit so that it is restarted
+                     ;; by login.
                      #f)))
                 (const #f)
                 (lambda (key . args)
diff --git a/gnu/installer/newt/final.scm b/gnu/installer/newt/final.scm
index 061bcd3f78..fb1b2281da 100644
--- a/gnu/installer/newt/final.scm
+++ b/gnu/installer/newt/final.scm
@@ -73,12 +73,15 @@ press the button to reboot."))
   'success)
 
 (define (run-install-failed-page)
-  (choice-window
-   (G_ "Installation failed")
-   (G_ "Restart installer")
-   (G_ "Retry system install")
-   (G_ "The final system installation step failed.  You can retry the \
-last step, or restart the installer.")))
+  (case (choice-window
+         (G_ "Installation failed")
+         (G_ "Resume")
+         (G_ "Restart the installer")
+         (G_ "The final system installation step failed.  You can resume from \
+a specific step, or restart the installer."))
+    ((1) (raise
+          (condition
+           (&installer-step-abort))))))
 
 (define* (run-install-shell locale
                             #:key (users '()))
-- 
2.25.0


[-- Attachment #4: Type: text/plain, Size: 74 bytes --]


[1]: https://lists.gnu.org/archive/html/guix-devel/2018-12/msg00161.html

bug#38447: Fix installer restart.

[Mathieu Othacehe]

This should allow us to close:

https://issues.guix.info/issue/39217
https://issues.guix.info/issue/38447

And other issues I'm not aware of?

Mathieu

bug#39217: Fix installer restart.

[Mathieu Othacehe]

This should allow us to close:

https://issues.guix.info/issue/39217
https://issues.guix.info/issue/38447

And other issues I'm not aware of?

Mathieu

Re: Fix installer restart.

[Mathieu Othacehe]

This should allow us to close:

https://issues.guix.info/issue/39217
https://issues.guix.info/issue/38447

And other issues I'm not aware of?

Mathieu

Re: Fix installer restart.

[Ludovic Courtès]

Hi Mathieu,

Mathieu Othacehe <m.othacehe@gmail.com> skribis:

> Here are two patches that should fix the "Installer fails when
> restarted" issue.

Great!

> I also fixed the "installation failed dialog". There are now two
> options:
> * "Resume": let the user resume the installation from any step (from the
> installation menu)
> * "Retry": restart the installer process.

Nice.

> From f02ef650526973af55cf1629de1cc48e888c714f Mon Sep 17 00:00:00 2001
> From: Mathieu Othacehe <m.othacehe@gmail.com>
> Date: Mon, 10 Feb 2020 14:43:05 +0100
> Subject: [PATCH 1/2] installer: Remove the cow-store overlay after system
>  installation.
>
> * gnu/installer/final.scm (umount-cow-store): New procedure ...,
> (install-system): ... used here, to remove the store overlay so that the
> target device is not seen as busy during further umount calls.

Please add a “Fixes …” line in the log, for future reference.

> +    (dynamic-wind
> +      (lambda ()
> +        (start-service 'cow-store (list (%installer-target-dir))))
> +      (lambda ()
> +        (run-shell-command install-command #:locale locale))
> +      (lambda ()
> +        (stop-service 'cow-store)
> +        ;; Remove the store overlay created at cow-store service start.
> +        ;; Failing to do that will result in further umount calls to fail
> +        ;; because the target device is seen as busy. See:
> +        ;; https://lists.gnu.org/archive/html/guix-devel/2018-12/msg00161.html.
> +        (umount-cow-store)
> +        #f))))

LGTM!

Did you make sure that:

  make check-system TESTS=installed-os

still passes?

> From 50884fca871cfceb020d63e556c957163fdd6064 Mon Sep 17 00:00:00 2001
> From: Mathieu Othacehe <m.othacehe@gmail.com>
> Date: Mon, 10 Feb 2020 14:47:56 +0100
> Subject: [PATCH 2/2] installer: Fix installer restart dialog.
>
> * gnu/installer/newt/final.scm (run-install-failed-page): Propose between
> installer resume or restart. Do actually resume the installation by raising an
> &installer-step-abort condition if "Resume" button is pressed. Otherwise, keep
> going as the installer will be restarted by login.
> * gnu/installer.scm (installer-program): Remove the associated TODO comment.

[…]

> --- a/gnu/installer/newt/final.scm
> +++ b/gnu/installer/newt/final.scm
> @@ -73,12 +73,15 @@ press the button to reboot."))
>    'success)
>  
>  (define (run-install-failed-page)
> -  (choice-window
> -   (G_ "Installation failed")
> -   (G_ "Restart installer")
> -   (G_ "Retry system install")
> -   (G_ "The final system installation step failed.  You can retry the \
> -last step, or restart the installer.")))
> +  (case (choice-window
> +         (G_ "Installation failed")
> +         (G_ "Resume")
> +         (G_ "Restart the installer")
> +         (G_ "The final system installation step failed.  You can resume from \
> +a specific step, or restart the installer."))
> +    ((1) (raise
> +          (condition
> +           (&installer-step-abort))))))

I’d recommend using ‘match’ rather than ‘case’, because ‘case’ silently
returns *unspecified* when it fails to match the value.

Apart from that, LGTM!

Thank you,
Ludo’.

PS: I’m still looking at the installer tests we discussed before FOSDEM,
    but life got in the way so progress is slow!

Re: Fix installer restart.

[Mathieu Othacehe]

Hey,

> Did you make sure that:
>
>   make check-system TESTS=installed-os
>
> still passes?

Yup, still working :)

In the meantime, while testing those patches, I noticed that:

* If you edit the final operating-system configuration with a syntax
error, start the install, then come back to the final page, your changes
are lost. But I'm not sure we can do much about it.

* If you open a TTY4 during "guix init" is running on TTY1, then the
command fails, the cow-store umount will fail because the bash started
in TTY4 will keep the overlay busy. This is a corner-case but it will
happen. Maybe I should check for all processed using the store overlay
and kill them before umounting?

> I’d recommend using ‘match’ rather than ‘case’, because ‘case’ silently
> returns *unspecified* when it fails to match the value.

Sure.

> PS: I’m still looking at the installer tests we discussed before FOSDEM,
>     but life got in the way so progress is slow!

Hehe, same for all of us :)

Anyway, I'll push soon, thanks for the fast review!

Mathieu

bug#39217: Fix installer restart.

[Mathieu Othacehe]

> This should allow us to close:
>
> https://issues.guix.info/issue/39217
> https://issues.guix.info/issue/38447

Fixed by 1d02052067e04d7dd8fd1ec17557ca02a30b9bcf.

Thanks,

Mathieu

Re: Fix installer restart.

[Ludovic Courtès]

Hi,

Mathieu Othacehe <m.othacehe@gmail.com> skribis:

> In the meantime, while testing those patches, I noticed that:
>
> * If you edit the final operating-system configuration with a syntax
> error, start the install, then come back to the final page, your changes
> are lost. But I'm not sure we can do much about it.

Or we could check for a pre-existing config file and use it instead of
re-generating it?

> * If you open a TTY4 during "guix init" is running on TTY1, then the
> command fails, the cow-store umount will fail because the bash started
> in TTY4 will keep the overlay busy. This is a corner-case but it will
> happen. Maybe I should check for all processed using the store overlay
> and kill them before umounting?

Oh, yes.  How would you find all the processes using the overlay though?
Browsing /proc/[0-9]*/cwd & co?

Thank you!

Ludo’.