[bug#36555,1/2] guix system: Add 'reconfigure' module.

Hello!

zerodaysfordays@sdf.lonestar.org (Jakob L. Kreuze) skribis:

> Ludovic Courtès <ludo@gnu.org> writes:

[...]

>> Can we remove ‘with-output-to-string’? I’d rather see what’s going on.
>> :-)
>>
>> If that’s too verbose, we can use ‘invoke/quiet’.
>
> I'm not too concerned with verbosity; rather, in the case for 'guix
> deploy', the script's output mixes with the REPL output and that causes
> 'remote-eval' to fail with a match error. I think it would be better to
> continue using 'with-output-to-string', but to preseve its return value
> so we can show it to the user from 'guix deploy' or 'guix system
> reconfigure'. Users of 'guix deploy' would also be able to see the
> script's output this way.

Oh, I see.  So in a way the problem is that ‘remote-eval’ doesn’t do
anything sensible with the output and error ports of that remote
evaluation.

Ultimately we should probably fix (guix inferior) and (guix remote) so
that stdout and stderr are properly transmitted.

In the meantime, what about this patch?
>> It seems that this sort-of inlines parts of ‘shepherd-service-upgrade’
>> but without traversing the service dependency graph to determine the
>> compilete set of obsolete services, no? I feel that we should be
>> reusing ‘shepherd-service-upgrade’ or similar bits. (I realize this is
>> already in ‘master’ for ‘guix deploy’, but since this is going to be
>> shared with ‘guix system’, we’d rather be extra cautious.)
>
> Does 'live-service-requirement' not encompass the full service
> dependency graph? Regardless, I'll look into reusing
> 'shepherd-service-upgrade' as it's well-testsed.

‘live-service-requirement’ gives you the graph of the currently loaded
services, but you also need the target service graph to determine what
to upgrade; that seems to be missing currently.

Thanks,
Ludo’.

Ludovic Courtès <ludo@gnu.org> writes:

> Oh, I see. So in a way the problem is that ‘remote-eval’ doesn’t do
> anything sensible with the output and error ports of that remote
> evaluation.
>
> Ultimately we should probably fix (guix inferior) and (guix remote) so
> that stdout and stderr are properly transmitted.

Thinking about it now, that could make error reporting for 'guix deploy'
less complicated. We'd be able to output the remote's stdout/stderr to
the host's stdout/stderr and be done with it.

> In the meantime, what about this patch?
>
> diff --git a/guix/remote.scm b/guix/remote.scm
> index e503c76167..8ada5c0957 100644
> --- a/guix/remote.scm
> +++ b/guix/remote.scm
> @@ -76,8 +76,14 @@ result to the current output port using the (guix repl) protocol."
>    (with-imported-modules (source-module-closure '((guix repl)))
>      #~(begin
>          (use-modules (guix repl))
> -        (send-repl-response '(primitive-load #$program)
> +
> +        ;; We use CURRENT-OUTPUT-PORT for REPL messages, so redirect PROGRAM's
> +        ;; output to CURRENT-ERROR-PORT so that it does not interfere.
> +        (send-repl-response '(with-output-to-port (current-error-port)
> +                               (lambda ()
> +                                 (primitive-load #$program)))
>                              (current-output-port))
> +
>          (force-output))))
>  
>  (define* (remote-eval exp session

LGTM, thanks!

> ‘live-service-requirement’ gives you the graph of the currently loaded
> services, but you also need the target service graph to determine what
> to upgrade; that seems to be missing currently.

Oh, good catch. Reusing 'shepherd-service-upgrade' is certainly the way
to go, then.

Regards,
Jakob

zerodaysfordays@sdf.lonestar.org (Jakob L. Kreuze) skribis:

> Ludovic Courtès <ludo@gnu.org> writes:

[...]

>> In the meantime, what about this patch?
>>
>> diff --git a/guix/remote.scm b/guix/remote.scm
>> index e503c76167..8ada5c0957 100644
>> --- a/guix/remote.scm
>> +++ b/guix/remote.scm
>> @@ -76,8 +76,14 @@ result to the current output port using the (guix repl) protocol."
>>    (with-imported-modules (source-module-closure '((guix repl)))
>>      #~(begin
>>          (use-modules (guix repl))
>> -        (send-repl-response '(primitive-load #$program)
>> +
>> +        ;; We use CURRENT-OUTPUT-PORT for REPL messages, so redirect PROGRAM's
>> +        ;; output to CURRENT-ERROR-PORT so that it does not interfere.
>> +        (send-repl-response '(with-output-to-port (current-error-port)
>> +                               (lambda ()
>> +                                 (primitive-load #$program)))
>>                              (current-output-port))
>> +
>>          (force-output))))
>>  
>>  (define* (remote-eval exp session
>
> LGTM, thanks!

Cool, pushed as 6f8eb9f1d8bc8660349658602698db36965bba5d.

>> ‘live-service-requirement’ gives you the graph of the currently loaded
>> services, but you also need the target service graph to determine what
>> to upgrade; that seems to be missing currently.
>
> Oh, good catch. Reusing 'shepherd-service-upgrade' is certainly the way
> to go, then.

I think so, which brings us back to the need to de-monadify (guix graph).
:-)

Ludo’.

Ludovic Courtès <ludo@gnu.org> writes:

> I think so, which brings us back to the need to de-monadify (guix
> graph). :-)

Good news, I came up with a way of using 'shepherd-service-upgrade' on
the host side. Stay tuned for v3 of this patch series ;)

Though, I suppose cleaning up the dependencies of '(guix graph)' may be
a good goal to have regardless.

Regards,
Jakob

Hi, all.

Submitting this reroll to ask for some further feedback. Here's a
summary of the more significant changes since v2:

- All of the system tests for the reconfiguration procedures have been
  implemented.
- 'upgrade-services-program' has been completely reimplemented; '(gnu
  machine ssh)' is now capable of (partially) serializing the
  <live-service> objects returned by 'current-services', so we can use
  'shepherd-service-upgrade' to traverse the service dependency graph.
- Procedures in '(guix scripts system reconfigure)' now use
  'program-file' instead of 'gexp->script'. I hadn't realized the
  difference, but this makes invocations of 'remote-eval' a bit cleaner.
- Thanks to Ludovic's patches to '(guix remote)', the reconfiguration
  procedures no longer need to capture output from the
  activation/installation scripts.
- I've removed my awful hack of a solution for handling Shepherd errors
  in 'upgrade-services-program' in favor of handling exceptions on the
  host side. I have some questions about this.
- 'upgrade-services-program' comes after 'install-bootloader-program' in
  'guix deploy' and 'guix system reconfigure' now, as it's the procedure
  most likely to fail trivially.

I still need to handle failed deployments in 'guix deploy'. I suspect
that, for now, it would make sense to implement remote roll-backs and
just roll-back the system on failure, at least until we've have some
dialog about the proper way to do atomic deployments.

My biggest concern at the moment is error handling reporting in the new
'guix system reconfigure'. I'd like to emulate what was done with the
previous version, but I'm at somewhat of a loss for how I'd go about
that, since the error reporting was mixed with the reconfiguration code.
So I'd like to ask for some suggestions: is the best way to catch errors
in '%store-monad' to do what 'with-shepherd-error-handling' does, and
then 'leave' on failure?

Ludovic suggested guarding against 'message-condition' and having the
expression I send to 'remote-eval' return either ('error message) or
('success). Would it make sense to just do this in all of the
reconfiguration procedures? Or is raising exceptions in the
reconfiguration procedures and catching them in the scripts' code the
way to go?

There's also a slight bug in the new 'guix system reconfigure' that I'll
need to figure out. At the moment, it installs a bootloader entry for
all but the newest generation.

Jakob L. Kreuze <zerodaysfordays@sdf.lonestar.org> writes:

> Noted. That should be a relatively small change, so I'll see about
> tackling that in my next revision for this series.

Oh, how naïve I was four days ago. This reroll doesn't address this.
Having the procedures "parameterized by an evaluation procedure" can be
done in so many ways, and I think it would be best I put some serious
thought into which of those ways would be the best. A 'local-eval' would
clearly be much better than what I'm doing at the present in
'system.scm', but the solution I came up with today involved three
layers of 'primitive-load', which I doubt is the way to go about it. I
had the idea to parameterize on a procedure that takes a
'<program-file>' rather than a G-Expression as I was making dinner
tonight, which seems to me like a sound idea, but we'll see if it works
tomorrow when I try to implement it.

Also, it hit me today that the safety checks done in 'guix system
reconfigure' -- 'check-mapped-devices',
'check-file-system-availability', and 'check-initrd-modules' -- should
also be done in 'guix deploy'. It might make sense for me to submit that
change as a separate patch series so the code review for this doesn't
get too complicated, but since we're on the topic of unifying the code
between 'guix deploy' and 'guix system reconfigure', should I perhaps
reimplement those procedures as '<program-file>' objects like everything
else in '(guix scripts system reconfigure)'? They aren't really
effectful, but they concern system reconfiguration.

And, on the same note, should I go ahead and refactor the rest of the
reconfiguration code in 'system.scm' out into '(guix scripts system
reconfigure)'? I mean, this will probably be a separate patch series for
the same reason that the safety checks would be a separate patch series,
and I'll likely do this _after_ I come up with a decent way to
parameterize on an evaluation procedure, but I'd like to know if it's a
good idea or not before going ahead and ripping apart 'system.scm'.

Regards, and TYIA for reviewing this.
Jakob

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                 | 266 ++++++++++-----------------
 gnu/services/herd.scm               |   6 +
 gnu/tests/reconfigure.scm           | 268 ++++++++++++++++++++++++++++
 guix/scripts/system.scm             | 152 +++++-----------
 guix/scripts/system/reconfigure.scm | 122 +++++++++++++
 tests/services.scm                  |   4 -
 8 files changed, 538 insertions(+), 282 deletions(-)
 create mode 100644 gnu/tests/reconfigure.scm
 create mode 100644 guix/scripts/system/reconfigure.scm