[bug#36404] [PATCH 2/5] gnu: Add machine type for deployment specifications.

[Jakob L. Kreuze]

Christopher Lemmer Webber <address@hidden> writes:

> Maybe it would make sense to call it machine-remote-eval to
> distinguish it? I dunno.

Considering the naming used for everything else that '(gnu machine)'
exports, I think that makes more sense. And that way I'll be able to
just import '(gnu remote ssh)' without shadowing 'remote-eval'. I went
ahead and changed it.

> @@ is a (sometimes useful) antipattern.  But in general, if something is
> importing something with @@, it's a good indication that we should just
> be exporting it.  What do you think?

My thinking was that, when we have more than one environment type, @@
could be used with module reflection to get a specific environment's
implementation of 'remote-eval'. But going back to your point in an
earlier email about implementing environments as distinct types rather
than symbols, it would be pretty easy to expose some sort of
'remote-eval' field on those environment types.

> Maybe it wouldn't be so hard to do?
>
> In fact, now that I look at it, we could solve both problems at once:
> there's no need to export deploy-machine and remote-eval if they're
> wrapped in another structure.  Instead, maybe this code could look like:
>
> #+BEGIN_SRC scheme
> (define (remote-eval machine exp)
>
>   "Evaluate EXP, a gexp, on MACHINE. Ensure that all the elements EXP refers 
> to
> are built and deployed to MACHINE beforehand."
>   (let* ((environment (machine-environment machine))
>          (remote-eval (environment-remote-eval environment)))
>     (remote-eval machine exp)))
>
> (define (deploy-machine machine)
>   "Monadic procedure transferring the new system's OS closure to the remote
> MACHINE, activating it on MACHINE and switching MACHINE to the new 
> generation."
>   (let* ((environment (machine-environment machine))
>          (deploy-machine (environment-deploy-machine environment)))
>     (deploy-machine machine)))
> #+END_SRC
>
> Thoughts?

Whoops, wrote the above paragraph before getting here. :]

> Feels like better polymorphism than this is desirable, but I'm not
> sure I have advice on how to do it right now. Probably services
> provide the right form of inspiration.

Are you talking about service extensions? I'm starting to see your point
regarding polymorphism, since SSH would be the backbone for a lot of
these environment types. Does anyone else have suggestions for
implementing that sort of polymorphism?

> Why not just import remote-eval in the define-module?

To avoid a Guile warning about shadowing symbols. This goes away with
the renaming of 'remote-eval' to 'machine-remote-eval', though.

> It's so cool that this works across machines.  Dang!

:)

> Yeah that sounds like it would be bad. But I'm curious... could you
> explain the specific bug it's preventing here? I'd like to know.

You've found something I've overlooked. There wasn't a bug, it's
something I put in since 'guix system' does it when loading the
activation script. But after looking through the 'guix system' code, I
noticed that there's a comment reading "[t]his is necessary to ensure
that 'upgrade-shepherd-services' gets to see the right modules when it
computes derivations with 'gexp->derivation'." Yet, I'm invoking my
version of 'upgrade-shepherd-services' outside of that excursion. I
haven't had any issues with it so far, but then again, I haven't done
much with trying to register new services with 'guix deploy'. I think
it's worth fixing.

> Just to see if I understand it... this is kind of so we can identify
> and "garbage collect" services that don't apply to the new system?

Yep.

>
> I'm a bit unsure from the above code... I'm guessing one of two things
> is happening:
>
>  - Either it's starting services that haven't been started yet, but
>    leaving alone services that are running but which aren't "new"
>  - Or it's restarting services that are currently running
>
> Which is it?  And mind adding a comment explaining it?

The former. I've intentionally avoided restarting services since 'guix
system' warns that "many essential services cannot be meaningfully
restarted." (which is why 'guix system reconfigure' spits out "To
complete the upgrade, run 'herd restart SERVICE' to stop, upgrade, and
restart each service that was not automatically restarted." (which AFAIK
is always none of them)).

> By the way, is there anything about the dependency order in which
> services might need to be restarted to be considered? I'm honestly not
> sure.

I'm not sure either. Would any Shepherd hackers out there care to chime
in?

> So I guess this is derivative of some of the stuff in
> guix/scripts/system.scm. That makes me feel like it would be nice if
> it could be generalized, but I haven't spent enough time with the code
> to figure out if it really can be.
>
> I don't want to block the merge on that desire, though if you agree
> that generalization between those sections of code is desirable, maybe
> add a comment to that effect?

You're right, and I agree 100%. I think I can commit to refactoring out
the common code, albeit after this patch series is merged -- that's
something that deserves its own commit, and it would probably take me
some time to get right anyway.

> This code also looks very similar, but I compared them and I can see
> that they aren't quite the same, at least in that you had to install
> the dynamic-wind. But I get the feeling that it still might be
> possible to generalize them, so could you leave a comment here as
> well? Unless you think it's really not possible to generalize them to
> share code for reasons I'm not yet aware of.

I think it can be generalized. In fact, 'guix system' does with
'save-load-path-excursion' and 'save-environment-excursion'. If I can't
generalize the code from '(gnu machine)' and 'guix system', I'll at
least see about exporting those excursions from 'guix system' (they're
unexported at the moment).

> Seems good from a quick scan, but I'll admit I didn't read these as
> carefully as I did the rest of the code.

I'm not sure it's really worth reading right now, this is the "me way"
of testing everything and I suspect some significant changes are going
to be made.

> This patch looks great overall!  I know it was a lot of work to figure
> out, and I'm impressed by how quickly you came up to speed on it.

Thank you :)

signature.asc
Description: PGP signature