Re: [PATCH] guix-daemon: Add option to disable garbage collection.

[Ludovic Courtès]

Hello Roel,

Roel Janssen <address@hidden> skribis:

> Roel Janssen <address@hidden> writes:
>
>> Ludovic Courtès <address@hidden> writes:

[...]

>>> In this case, I thought guix-daemon could explicitly check whether the
>>> peer is remote, and disable GC in that case.  That is, ‘guix gc’ would
>>> still work locally on the machine that runs guix-daemon, but it would no
>>> longer work remotely.
>>>
>>> How does that sound?
>>
>> That sounds like it solves our use-case, but only because in our
>> case the access to the machine running guix-daemon is limited.
>>
>> So, even though I'm not sure how to implement this, your solution is
>> fine with me.
>
> I implemented the solution in the attached patch.  When a connection
> does not come from the UNIX socket, it is treated as “remote”.  So,
> local TCP connections would also be treated as “remote”.

Excellent!

> I assumed ‘collectGarbage()’ is the entry point for all garbage collection,
> is that correct?

Yes.

> From 00f489d6303720c65571fdf0bc9ee810a20f70e0 Mon Sep 17 00:00:00 2001
> From: Roel Janssen <address@hidden>
> Date: Wed, 11 Apr 2018 09:52:11 +0200
> Subject: [PATCH] guix-daemon: Disable garbage collection for remote hosts.
>
> * nix/libstore/gc.cc (collectGarbage): Check for remote connections.
> * nix/libstore/globals.hh: Add isRemoteConnection setting.
> * nix/nix-daemon/nix-daemon.cc (performOp): Display appropriate error message;
>   (acceptConnection): Set isRemoteConnection when connection is over TCP.

[...]

> --- a/nix/libstore/gc.cc
> +++ b/nix/libstore/gc.cc
> @@ -595,6 +595,10 @@ void LocalStore::removeUnusedLinks(const GCState & state)
>  
>  void LocalStore::collectGarbage(const GCOptions & options, GCResults & 
> results)
>  {
> +    if (settings.isRemoteConnection) {
> +        return;
> +    }

I think this is unnecessary since the daemon already checks for that.
(It’s also nicer to keep ‘LocalStore’ unaware of the connection
details.)

> diff --git a/nix/nix-daemon/nix-daemon.cc b/nix/nix-daemon/nix-daemon.cc
> index deb7003d7..65770ba95 100644
> --- a/nix/nix-daemon/nix-daemon.cc
> +++ b/nix/nix-daemon/nix-daemon.cc
> @@ -529,6 +529,11 @@ static void performOp(bool trusted, unsigned int 
> clientVersion,
>      }
>  
>      case wopCollectGarbage: {
> +        if (settings.isRemoteConnection) {
> +            throw Error("Garbage collection is disabled for remote hosts.");
> +            break;
> +        }
>          GCOptions options;
>          options.action = (GCOptions::GCAction) readInt(from);
>          options.pathsToDelete = readStorePaths<PathSet>(from);

I was wondering if we would like to allow some of the ‘GCAction’ values,
but maybe it’s better to disallow them altogether like this code does.

> @@ -934,6 +939,7 @@ static void acceptConnection(int fdSocket)
>                     connection.  Setting these to -1 means: do not change.  */
>                  settings.clientUid = clientUid;
>               settings.clientGid = clientGid;
> +                settings.isRemoteConnection = (remoteAddr.ss_family != 
> AF_UNIX);

I think you can make ‘isRemoteConnection’ a static global variable in
nix-daemon.cc instead of adding it to ‘Settings’.  So it would do
something like:

--8<---------------cut here---------------start------------->8---
        /* Fork a child to handle the connection. */
        startProcess([&]() {
                close(fdSocket);

                /* Background the daemon. */
                if (setsid() == -1)
                    throw SysError(format("creating a new session"));

                /* Restore normal handling of SIGCHLD. */
                setSigChldAction(false);

                /* For debugging, stuff the pid into argv[1]. */
                if (clientPid != -1 && argvSaved[1]) {
                    string processName = std::to_string(clientPid);
                    strncpy(argvSaved[1], processName.c_str(), 
strlen(argvSaved[1]));
                }

                isRemoteConnection = …;  /*  <– this is the new line */

                /* Store the client's user and group for this connection. This
                   has to be done in the forked process since it is per
                   connection.  Setting these to -1 means: do not change.  */
                settings.clientUid = clientUid;
                settings.clientGid = clientGid;
--8<---------------cut here---------------end--------------->8---

Last thing: could you add a couple of tests?  tests/guix-daemon.sh
already has tests for ‘--listen’, so you could take inspiration from
those.

Thank you!

Ludo’.