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

[Christopher Lemmer Webber]

Jakob L. Kreuze writes:

> Christopher Lemmer Webber <address@hidden> writes:
>
>> 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?

Right now it looks like you're hard-coding dispatch into the procedure
by doing a case analysis of what type it is, but this doesn't allow us
to extend it.

Here I'd look at how service-type works.  Check out gnu/services.scm
and then some examples of how services are defined in say,
gnu/services/admin.scm or something (eg rotlog-service-type).  I'm not
saying structure it in exactly this way, but that seems to be the right
general pattern to do extensibility in the guix'y way:

 - Have a common outer type (eg <service-type>) which actually
   sets up the structure of this service type
 - Then have the actual records that are specific to the service type
   represented as the service-value.

Section 8.16.2 "Serivce Types and Services" and 6.16.3 "Service
Reference" for details.

Note that I wish there was a way to generalize the ideas behind this
pattern rather than have it be reinvented for everything that needs
them.  This is part of why David and I turned to GOOPS in the initial
prototype implementation; it's a lot of work figuring out how to set up
extensibility in this way, at least for me.  You might want to write a
quick GOOPS version to understand what all the parameters are that are
needed, then convert it to the services way of doing a general structure
that wraps a specific structure.

I suspect you won't need as much composability as services currently
need, so the implementation of whatever this extensibility is is
probably not as complicated as it is for services.

As for how to share the ssh code, maybe just having the building-block
procedures is good enough?

Since all we support, so far, is this kind of ssh'ing, I don't want this
to block the patch though.  It could be that we file this as a bug and
add a TODO above the code for the moment saying "we know this isn't
right/ideal".  However, there is some risk that this could result in
people writing out machine configurations that later break... I dunno.

Thoughts?

>> 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.

Heh :)

>> 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.

Cool.  Yay reviews!

If you remove it, please leave a comment noting the difference between
this and "guix system" and why you thought it was safe to remove.  If it
turns out to not be the case, there's a breadcrumb there to figure out
how to add it back.

>> 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)).

Aha.  Thank you for explaining!  This make ssense.

>> 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?

I guess if you aren't restarting the services, it's no longer a big deal.

>> 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.

Great!

>> 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).

Okay, cool.

>> 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.

Kk.

>> 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 :)

Thank *you*!