Hello there,
I'm not sure this made it to the mailing list. Is the proposed patch
fine to disable the GC for remote connections?
Thanks!
Kind regards,
Roel Janssen
Roel Janssen <r...@gnu.org> writes:
> Roel Janssen <r...@gnu.org> writes:
>
>> Ludovic Courtès <ludovic.cour...@inria.fr> writes:
>>
>>> Hello Roel,
>>>
>>> Roel Janssen <r...@gnu.org> skribis:
>>>
>>>> The patch adds a “disableGarbageCollection” boolean variable to the
>>>> guix-daemon settings, and on each occasion where a store item may be
>>>> deleted, it checks this option.
>>>>
>>>> This option can be set using “--disable-gc”.
>>>>
>>>> It would be great if someone could review this and discuss whether
>>>> this is the right way to implement such a feature. And to point out
>>>> what else would be needed to include this option in guix-daemon.
>>>
>>> I suppose the use case is when guix-daemon runs on a machine and is
>>> accessed over TCP/IP (with GUIX_DAEMON_SOCKET=guix://…) from other
>>> machines, right?
>>
>> That's right.
>>
>>> 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”.
>
> I assumed ‘collectGarbage()’ is the entry point for all garbage collection,
> is that correct?
>
> Kind regards,
> Roel Janssen
>
> From 00f489d6303720c65571fdf0bc9ee810a20f70e0 Mon Sep 17 00:00:00 2001
> From: Roel Janssen <r...@gnu.org>
> 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.
> ---
> nix/libstore/gc.cc | 4 ++++
> nix/libstore/globals.hh | 4 ++++
> nix/nix-daemon/nix-daemon.cc | 6 ++++++
> 3 files changed, 14 insertions(+)
>
> diff --git a/nix/libstore/gc.cc b/nix/libstore/gc.cc
> index 72eff5242..1bc6eedb5 100644
> --- 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;
> + }
> +
> GCState state(results);
> state.options = options;
> state.trashDir = settings.nixStore + "/trash";
> diff --git a/nix/libstore/globals.hh b/nix/libstore/globals.hh
> index 1293625e1..83efbcd50 100644
> --- a/nix/libstore/globals.hh
> +++ b/nix/libstore/globals.hh
> @@ -81,6 +81,10 @@ struct Settings {
> uid_t clientUid;
> gid_t clientGid;
>
> + /* Whether the connection comes from a host other than the host running
> + guix-daemon. */
> + bool isRemoteConnection;
> +
> /* Whether, if we cannot realise the known closure corresponding
> to a derivation, we should try to normalise the derivation
> instead. */
> 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);
> @@ -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);
>
> /* Handle the connection. */
> from.fd = remote;
