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

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

[Roel Janssen]

[-- Attachment #1: Type: text/plain, Size: 1044 bytes --]

Dear Guix,

I have an interesting situation where the guix-daemon cannot see all
directories that (may) contain Guix profiles, and therefore is not able
to judge whether a GC root is gone and can be collected, or whether it's
just inaccessible.

To be on the safe side, I'd like to disable garbage collection on this
system.  To achieve this, I wrote the attached patch.  Even though “it
works for me”, I don't think it's good to be added as-is.  At least I
need to figure out how to make the error message “Garbage collection is
disabled.” translatable.

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.

Thank you for your time.

Kind regards,
Roel Janssen


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-guix-daemon-Add-option-to-disable-garbage-collection.patch --]
[-- Type: text/x-patch, Size: 4797 bytes --]

From d842f320f0ee911d7d219bba7baa45240edcbe6d Mon Sep 17 00:00:00 2001
From: Roel Janssen <roel@gnu.org>
Date: Tue, 3 Apr 2018 11:22:16 +0200
Subject: [PATCH] guix-daemon: Add option to disable garbage collection.

* nix/libstore/gc.cc: Return early on deleterious functions.
* nix/libstore/globals.hh (disableGarbageCollection): New settings variable.
* nix/nix-daemon/guix-daemon.cc: Implement new settings variable.
* nix/nix-daemon/nix-daemon.cc: Show appropriate message.
---
 nix/libstore/gc.cc            | 20 ++++++++++++++++++++
 nix/libstore/globals.hh       |  3 +++
 nix/nix-daemon/guix-daemon.cc |  6 ++++++
 nix/nix-daemon/nix-daemon.cc  |  5 +++++
 4 files changed, 34 insertions(+)

diff --git a/nix/libstore/gc.cc b/nix/libstore/gc.cc
index 72eff5242..b811cacd1 100644
--- a/nix/libstore/gc.cc
+++ b/nix/libstore/gc.cc
@@ -393,6 +393,10 @@ bool LocalStore::isActiveTempFile(const GCState & state,
 
 void LocalStore::deleteGarbage(GCState & state, const Path & path)
 {
+    if (settings.disableGarbageCollection) {
+        return;
+    }
+
     unsigned long long bytesFreed;
     deletePath(path, bytesFreed);
     state.results.bytesFreed += bytesFreed;
@@ -401,6 +405,10 @@ void LocalStore::deleteGarbage(GCState & state, const Path & path)
 
 void LocalStore::deletePathRecursive(GCState & state, const Path & path)
 {
+    if (settings.disableGarbageCollection) {
+        return;
+    }
+
     checkInterrupt();
 
     unsigned long long size = 0;
@@ -513,6 +521,10 @@ bool LocalStore::canReachRoot(GCState & state, PathSet & visited, const Path & p
 
 void LocalStore::tryToDelete(GCState & state, const Path & path)
 {
+    if (settings.disableGarbageCollection) {
+        return;
+    }
+
     checkInterrupt();
 
     if (path == linksDir || path == state.trashDir) return;
@@ -552,6 +564,10 @@ void LocalStore::tryToDelete(GCState & state, const Path & path)
    the link count. */
 void LocalStore::removeUnusedLinks(const GCState & state)
 {
+    if (settings.disableGarbageCollection) {
+        return;
+    }
+
     AutoCloseDir dir = opendir(linksDir.c_str());
     if (!dir) throw SysError(format("opening directory `%1%'") % linksDir);
 
@@ -595,6 +611,10 @@ void LocalStore::removeUnusedLinks(const GCState & state)
 
 void LocalStore::collectGarbage(const GCOptions & options, GCResults & results)
 {
+    if (settings.disableGarbageCollection) {
+        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..54e04f218 100644
--- a/nix/libstore/globals.hh
+++ b/nix/libstore/globals.hh
@@ -218,6 +218,9 @@ struct Settings {
     /* Whether the importNative primop should be enabled */
     bool enableImportNative;
 
+    /* Whether the disable garbage collection. */
+    bool disableGarbageCollection;
+
 private:
     SettingsMap settings, overrides;
 
diff --git a/nix/nix-daemon/guix-daemon.cc b/nix/nix-daemon/guix-daemon.cc
index b71b100f6..13811cd99 100644
--- a/nix/nix-daemon/guix-daemon.cc
+++ b/nix/nix-daemon/guix-daemon.cc
@@ -89,6 +89,7 @@ builds derivations on behalf of its clients.");
 #define GUIX_OPT_TIMEOUT 18
 #define GUIX_OPT_MAX_SILENT_TIME 19
 #define GUIX_OPT_LOG_COMPRESSION 20
+#define GUIX_OPT_DISABLE_GC 21
 
 static const struct argp_option options[] =
   {
@@ -133,6 +134,8 @@ static const struct argp_option options[] =
       n_("disable automatic file \"deduplication\" in the store") },
     { "disable-store-optimization", GUIX_OPT_DISABLE_DEDUPLICATION, 0,
       OPTION_ALIAS | OPTION_HIDDEN, NULL },
+    { "disable-gc", GUIX_OPT_DISABLE_GC, 0, 0,
+      n_("disable garbage collection.") },
 
     { "impersonate-linux-2.6", GUIX_OPT_IMPERSONATE_LINUX_26, 0,
 #ifdef HAVE_SYS_PERSONALITY_H
@@ -225,6 +228,9 @@ parse_opt (int key, char *arg, struct argp_state *state)
     case GUIX_OPT_DISABLE_DEDUPLICATION:
       settings.autoOptimiseStore = false;
       break;
+    case GUIX_OPT_DISABLE_GC:
+      settings.disableGarbageCollection = true;
+      break;
     case GUIX_OPT_CACHE_FAILURES:
       settings.cacheFailure = true;
       break;
diff --git a/nix/nix-daemon/nix-daemon.cc b/nix/nix-daemon/nix-daemon.cc
index deb7003d7..5af8ec796 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.disableGarbageCollection) {
+            throw Error("Garbage collection is disabled.");
+            break;
+        }
+
         GCOptions options;
         options.action = (GCOptions::GCAction) readInt(from);
         options.pathsToDelete = readStorePaths<PathSet>(from);
-- 
2.16.1

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

[Ludovic Courtès]

Hello Roel,

Roel Janssen <roel@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?

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?

Thanks,
Ludo’.

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

[Roel Janssen]

Ludovic Courtès <ludovic.courtes@inria.fr> writes:

> Hello Roel,
>
> Roel Janssen <roel@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.

>
> Thanks,
> Ludo’.

Thanks!

Kind regards,
Roel Janssen

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

[Roel Janssen]

[-- Attachment #1: Type: text/plain, Size: 1633 bytes --]


Roel Janssen <roel@gnu.org> writes:

> Ludovic Courtès <ludovic.courtes@inria.fr> writes:
>
>> Hello Roel,
>>
>> Roel Janssen <roel@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


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-guix-daemon-Disable-garbage-collection-for-remote-ho.patch --]
[-- Type: text/x-patch, Size: 2683 bytes --]

From 00f489d6303720c65571fdf0bc9ee810a20f70e0 Mon Sep 17 00:00:00 2001
From: Roel Janssen <roel@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;
-- 
2.16.3

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

[Roel Janssen]

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 <roel@gnu.org> writes:

> Roel Janssen <roel@gnu.org> writes:
>
>> Ludovic Courtès <ludovic.courtes@inria.fr> writes:
>>
>>> Hello Roel,
>>>
>>> Roel Janssen <roel@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 <roel@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;

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

[Ludovic Courtès]

Hello!

Roel Janssen <roel@gnu.org> skribis:

> I'm not sure this made it to the mailing list.  Is the proposed patch
> fine to disable the GC for remote connections?

It did make it to the list, I’ll look at it real soon.

Ludo’.

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

[Ludovic Courtès]

Hello Roel,

Roel Janssen <roel@gnu.org> skribis:

> Roel Janssen <roel@gnu.org> writes:
>
>> Ludovic Courtès <ludovic.courtes@inria.fr> 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 <roel@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.

[...]

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

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

[Roel Janssen]

[-- Attachment #1: Type: text/plain, Size: 4511 bytes --]


Ludovic Courtès <ludovic.courtes@inria.fr> writes:

> Hello Roel,
>
> Roel Janssen <roel@gnu.org> skribis:
>

[...]

>
>> From 00f489d6303720c65571fdf0bc9ee810a20f70e0 Mon Sep 17 00:00:00 2001
>> From: Roel Janssen <roel@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.
>
> [...]
>
>> --- 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.)

Right.  It is indeed unnecessary.  In the updated patch, I no longer do this.

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

Could we please start with a “disable any GC” and start allowing cases
on a case-by-case basis?  The reason I request this, is because it makes
it a lot easier to reason about it from a sysadmin point-of-view.

I'd like to think of it like this: “Garbage collection is effectively
turned off.”.  With the current patch, we can reason about it this way.

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

Right.  I implemented it using a static global variable in
nix-daemon.cc.  This patch gets shorter and shorter. :)

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

I included a test, but I don't know how I can properly run this test.
Could you elaborate on how I can test the test(s)?

Kind regards,
Roel Janssen


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-guix-daemon-Disable-garbage-collection-for-remote-ho.patch --]
[-- Type: text/x-patch, Size: 2554 bytes --]

From b29d3a90e1487ebda5ac5b6bc146f8c95218eab6 Mon Sep 17 00:00:00 2001
From: Roel Janssen <roel@gnu.org>
Date: Thu, 19 Apr 2018 14:01:49 +0200
Subject: [PATCH] guix-daemon: Disable garbage collection for remote hosts.

* nix/nix-daemon/nix-daemon.cc (performOp): Display appropriate error message;
  (acceptConnection): Set isRemoteConnection when connection is over TCP.
* tests/guix-daemon.sh: Add a test for the new behavior.
---
 nix/nix-daemon/nix-daemon.cc | 10 +++++++++-
 tests/guix-daemon.sh         | 14 ++++++++++++++
 2 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/nix/nix-daemon/nix-daemon.cc b/nix/nix-daemon/nix-daemon.cc
index deb7003d7..782e4acfc 100644
--- a/nix/nix-daemon/nix-daemon.cc
+++ b/nix/nix-daemon/nix-daemon.cc
@@ -54,7 +54,9 @@ static FdSink to(STDOUT_FILENO);
 
 bool canSendStderr;
 
-
+/* This variable is used to keep track of whether a connection
+   comes from a host other than the host running guix-daemon. */
+static bool isRemoteConnection;
 
 /* This function is called anytime we want to write something to
    stderr.  If we're in a state where the protocol allows it (i.e.,
@@ -529,6 +531,11 @@ static void performOp(bool trusted, unsigned int clientVersion,
     }
 
     case wopCollectGarbage: {
+        if (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 +941,7 @@ static void acceptConnection(int fdSocket)
                    connection.  Setting these to -1 means: do not change.  */
                 settings.clientUid = clientUid;
 		settings.clientGid = clientGid;
+                isRemoteConnection = (remoteAddr.ss_family != AF_UNIX);
 
                 /* Handle the connection. */
                 from.fd = remote;
diff --git a/tests/guix-daemon.sh b/tests/guix-daemon.sh
index 6f91eb58b..438c79c26 100644
--- a/tests/guix-daemon.sh
+++ b/tests/guix-daemon.sh
@@ -194,6 +194,20 @@ do
     kill "$daemon_pid"
 done
 
+# Make sure garbage collection from a TCP connection does not work.
+
+socket="127.0.0.1:9999"
+guix-daemon --listen="$socket" &
+daemon_pid=$!
+
+output=`GUIX_DAEMON_SOCKET="$socket" guix gc`
+if [[ "$output" != *"GUIX_DAEMON_SOCKET=$socket" ]];
+then
+    exit 1
+fi
+
+kill "$daemon_pid"
+
 # Log compression.
 
 guix-daemon --listen="$socket" --disable-chroot --debug --log-compression=gzip &
-- 
2.17.0

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

[Ludovic Courtès]

Roel Janssen <roel@gnu.org> skribis:

> Ludovic Courtès <ludovic.courtes@inria.fr> writes:

[...]

>>> 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.
>
> Could we please start with a “disable any GC” and start allowing cases
> on a case-by-case basis?

Sure, that’s what I was suggesting.  :-)

>> 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.
>
> I included a test, but I don't know how I can properly run this test.
> Could you elaborate on how I can test the test(s)?

Run:

  make check TESTS=tests/guix-daemon.sh

See
<https://www.gnu.org/software/guix/manual/html_node/Running-the-Test-Suite.html>.

> From b29d3a90e1487ebda5ac5b6bc146f8c95218eab6 Mon Sep 17 00:00:00 2001
> From: Roel Janssen <roel@gnu.org>
> Date: Thu, 19 Apr 2018 14:01:49 +0200
> Subject: [PATCH] guix-daemon: Disable garbage collection for remote hosts.
>
> * nix/nix-daemon/nix-daemon.cc (performOp): Display appropriate error message;
>   (acceptConnection): Set isRemoteConnection when connection is over TCP.

Rather:

* nix/nix-daemon/nix-daemon.cc (isRemoteConnection): New variable.
(performOp): For wopCollectGarbage, throw an error when
isRemoteConnection is set.
(acceptConnection): Set isRemoteConnection when connection is not AF_UNIX.

> +output=`GUIX_DAEMON_SOCKET="$socket" guix gc`
> +if [[ "$output" != *"GUIX_DAEMON_SOCKET=$socket" ]];
> +then
> +    exit 1
> +fi

Perhaps simply check the exit code of ‘guix gc’ and fail if it succeeds?

Like:

  if guix gc; then false; else true; fi

Also please try to avoid Bash-specific constructs like [[ this ]].

Could you send an updated patch?

Thank you,
Ludo’.

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

[Roel Janssen]

[-- Attachment #1: Type: text/plain, Size: 2761 bytes --]


Ludovic Courtès <ludovic.courtes@inria.fr> writes:

> Roel Janssen <roel@gnu.org> skribis:
>
>> Ludovic Courtès <ludovic.courtes@inria.fr> writes:
>
> [...]
>
>>>> 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.
>>
>> Could we please start with a “disable any GC” and start allowing cases
>> on a case-by-case basis?
>
> Sure, that’s what I was suggesting.  :-)
>
>>> 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.
>>
>> I included a test, but I don't know how I can properly run this test.
>> Could you elaborate on how I can test the test(s)?
>
> Run:
>
>   make check TESTS=tests/guix-daemon.sh
>
> See
> <https://www.gnu.org/software/guix/manual/html_node/Running-the-Test-Suite.html>.

That is really nice.  Thanks for pointing to the manual.

>> From b29d3a90e1487ebda5ac5b6bc146f8c95218eab6 Mon Sep 17 00:00:00 2001
>> From: Roel Janssen <roel@gnu.org>
>> Date: Thu, 19 Apr 2018 14:01:49 +0200
>> Subject: [PATCH] guix-daemon: Disable garbage collection for remote hosts.
>>
>> * nix/nix-daemon/nix-daemon.cc (performOp): Display appropriate error message;
>>   (acceptConnection): Set isRemoteConnection when connection is over TCP.
>
> Rather:
>
> * nix/nix-daemon/nix-daemon.cc (isRemoteConnection): New variable.
> (performOp): For wopCollectGarbage, throw an error when
> isRemoteConnection is set.
> (acceptConnection): Set isRemoteConnection when connection is not AF_UNIX.
>
>> +output=`GUIX_DAEMON_SOCKET="$socket" guix gc`
>> +if [[ "$output" != *"GUIX_DAEMON_SOCKET=$socket" ]];
>> +then
>> +    exit 1
>> +fi
>
> Perhaps simply check the exit code of ‘guix gc’ and fail if it succeeds?

Right.

> Like:
>
>   if guix gc; then false; else true; fi
>
> Also please try to avoid Bash-specific constructs like [[ this ]].

Right.

> Could you send an updated patch?

The attached patch should be fine.

Kind regards,
Roel Janssen


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-guix-daemon-Disable-garbage-collection-for-remote-co.patch --]
[-- Type: text/x-patch, Size: 2659 bytes --]

From fcbe7ebb3d205cf7310700e62b78b9aafd94f76f Mon Sep 17 00:00:00 2001
From: Roel Janssen <roel@gnu.org>
Date: Thu, 19 Apr 2018 17:11:30 +0200
Subject: [PATCH] guix-daemon: Disable garbage collection for remote
 connections.

* nix/nix-daemon/nix-daemon.cc (isRemoteConnection): New variable.
  (performOp): For wopCollectGarbage, throw an error when isRemoteConnection
  is set.
  (acceptConnection): Set isRemoteConnection when connection is not AF_UNIX.
* tests/guix-daemon.sh: Add a test for the new behavior.
---
 nix/nix-daemon/nix-daemon.cc | 10 +++++++++-
 tests/guix-daemon.sh         | 14 ++++++++++++++
 2 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/nix/nix-daemon/nix-daemon.cc b/nix/nix-daemon/nix-daemon.cc
index deb7003d7..782e4acfc 100644
--- a/nix/nix-daemon/nix-daemon.cc
+++ b/nix/nix-daemon/nix-daemon.cc
@@ -54,7 +54,9 @@ static FdSink to(STDOUT_FILENO);
 
 bool canSendStderr;
 
-
+/* This variable is used to keep track of whether a connection
+   comes from a host other than the host running guix-daemon. */
+static bool isRemoteConnection;
 
 /* This function is called anytime we want to write something to
    stderr.  If we're in a state where the protocol allows it (i.e.,
@@ -529,6 +531,11 @@ static void performOp(bool trusted, unsigned int clientVersion,
     }
 
     case wopCollectGarbage: {
+        if (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 +941,7 @@ static void acceptConnection(int fdSocket)
                    connection.  Setting these to -1 means: do not change.  */
                 settings.clientUid = clientUid;
 		settings.clientGid = clientGid;
+                isRemoteConnection = (remoteAddr.ss_family != AF_UNIX);
 
                 /* Handle the connection. */
                 from.fd = remote;
diff --git a/tests/guix-daemon.sh b/tests/guix-daemon.sh
index 6f91eb58b..9ae6e0b77 100644
--- a/tests/guix-daemon.sh
+++ b/tests/guix-daemon.sh
@@ -194,6 +194,20 @@ do
     kill "$daemon_pid"
 done
 
+# Make sure garbage collection from a TCP connection does not work.
+
+tcp_socket="127.0.0.1:9999"
+guix-daemon --listen="$tcp_socket" &
+daemon_pid=$!
+
+GUIX_DAEMON_SOCKET="guix://$tcp_socket"
+export GUIX_DAEMON_SOCKET
+
+if guix gc; then false; else true; fi
+
+unset GUIX_DAEMON_SOCKET
+kill "$daemon_pid"
+
 # Log compression.
 
 guix-daemon --listen="$socket" --disable-chroot --debug --log-compression=gzip &
-- 
2.17.0

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

[Ludovic Courtès]

Heya,

Roel Janssen <roel@gnu.org> skribis:

> From fcbe7ebb3d205cf7310700e62b78b9aafd94f76f Mon Sep 17 00:00:00 2001
> From: Roel Janssen <roel@gnu.org>
> Date: Thu, 19 Apr 2018 17:11:30 +0200
> Subject: [PATCH] guix-daemon: Disable garbage collection for remote
>  connections.
>
> * nix/nix-daemon/nix-daemon.cc (isRemoteConnection): New variable.
>   (performOp): For wopCollectGarbage, throw an error when isRemoteConnection
>   is set.
>   (acceptConnection): Set isRemoteConnection when connection is not AF_UNIX.
> * tests/guix-daemon.sh: Add a test for the new behavior.

LGTM, thanks for the quick reply!

Ludo’.

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

[Roel Janssen]

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

> Heya,
>
> Roel Janssen <roel@gnu.org> skribis:
>
>> From fcbe7ebb3d205cf7310700e62b78b9aafd94f76f Mon Sep 17 00:00:00 2001
>> From: Roel Janssen <roel@gnu.org>
>> Date: Thu, 19 Apr 2018 17:11:30 +0200
>> Subject: [PATCH] guix-daemon: Disable garbage collection for remote
>>  connections.
>>
>> * nix/nix-daemon/nix-daemon.cc (isRemoteConnection): New variable.
>>   (performOp): For wopCollectGarbage, throw an error when isRemoteConnection
>>   is set.
>>   (acceptConnection): Set isRemoteConnection when connection is not AF_UNIX.
>> * tests/guix-daemon.sh: Add a test for the new behavior.
>
> LGTM, thanks for the quick reply!

Awesome.  Pushed in 5cefb13dd.

Thanks!

Kind regards,
Roel Janssen

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

[Marius Bakke]

[-- Attachment #1: Type: text/plain, Size: 927 bytes --]

Roel Janssen <roel@gnu.org> writes:

> I included a test, but I don't know how I can properly run this test.
> Could you elaborate on how I can test the test(s)?

[...]

> diff --git a/tests/guix-daemon.sh b/tests/guix-daemon.sh
> index 6f91eb58b..438c79c26 100644
> --- a/tests/guix-daemon.sh
> +++ b/tests/guix-daemon.sh
> @@ -194,6 +194,20 @@ do
>      kill "$daemon_pid"
>  done
>  
> +# Make sure garbage collection from a TCP connection does not work.
> +
> +socket="127.0.0.1:9999"
> +guix-daemon --listen="$socket" &
> +daemon_pid=$!
> +
> +output=`GUIX_DAEMON_SOCKET="$socket" guix gc`
> +if [[ "$output" != *"GUIX_DAEMON_SOCKET=$socket" ]];
> +then
> +    exit 1
> +fi
> +
> +kill "$daemon_pid"
> +
>  # Log compression.
>  
>  guix-daemon --listen="$socket" --disable-chroot --debug --log-compression=gzip &

Use `make check "TESTS=tests/guix-daemon.sh"` to only run this one test.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 487 bytes --]

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

[Adam Van Ymeren]

Just out of curiosity, what is your situation where the daemon can't see all GC roots?  Are you sharing the store over NFS or something?On 3 Apr 2018 6:12 a.m., Roel Janssen <roel@gnu.org> wrote:
>
> Dear Guix, 
>
> I have an interesting situation where the guix-daemon cannot see all 
> directories that (may) contain Guix profiles, and therefore is not able 
> to judge whether a GC root is gone and can be collected, or whether it's 
> just inaccessible. 
>
> To be on the safe side, I'd like to disable garbage collection on this 
> system.  To achieve this, I wrote the attached patch.  Even though “it 
> works for me”, I don't think it's good to be added as-is.  At least I 
> need to figure out how to make the error message “Garbage collection is 
> disabled.” translatable. 
>
> 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. 
>
> Thank you for your time. 
>
> Kind regards, 
> Roel Janssen 
>

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

[Roel Janssen]

Adam Van Ymeren <adam@vany.ca> writes:

> Just out of curiosity, what is your situation where the daemon can't
> see all GC roots?  Are you sharing the store over NFS or something?

Yes.  And the underlying storage system is configured in such a way that
“root” is not allowed to look into user's folders.

Kind regards,
Roel Janssen