From 60d8dd7aeaf7fc022a1b207012c94180f6732b45 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Thu, 23 Mar 2023 10:06:45 -0400 Subject: [PATCH] Clean up store hierarchy with `IndirectRootStore` See the API doc comments for details. --- src/libstore/build/local-derivation-goal.cc | 4 +- src/libstore/daemon.cc | 5 ++- src/libstore/gc-store.hh | 35 +++++++++++---- src/libstore/gc.cc | 3 +- src/libstore/indirect-root-store.hh | 48 +++++++++++++++++++++ src/libstore/local-fs-store.hh | 16 ++++++- src/libstore/local-store.hh | 13 ++++-- src/libstore/remote-store.cc | 9 ---- src/libstore/remote-store.hh | 2 - src/libstore/ssh-store.cc | 12 ++++-- src/libstore/uds-remote-store.cc | 10 +++++ src/libstore/uds-remote-store.hh | 16 ++++++- 12 files changed, 136 insertions(+), 37 deletions(-) create mode 100644 src/libstore/indirect-root-store.hh diff --git a/src/libstore/build/local-derivation-goal.cc b/src/libstore/build/local-derivation-goal.cc index 214f3d49e..b7a27490c 100644 --- a/src/libstore/build/local-derivation-goal.cc +++ b/src/libstore/build/local-derivation-goal.cc @@ -1,5 +1,5 @@ #include "local-derivation-goal.hh" -#include "gc-store.hh" +#include "indirect-root-store.hh" #include "hook-instance.hh" #include "worker.hh" #include "builtins.hh" @@ -1200,7 +1200,7 @@ struct RestrictedStoreConfig : virtual LocalFSStoreConfig /* A wrapper around LocalStore that only allows building/querying of paths that are in the input closures of the build or were added via recursive Nix calls. */ -struct RestrictedStore : public virtual RestrictedStoreConfig, public virtual LocalFSStore, public virtual GcStore +struct RestrictedStore : public virtual RestrictedStoreConfig, public virtual IndirectRootStore, public virtual GcStore { ref next; diff --git a/src/libstore/daemon.cc b/src/libstore/daemon.cc index ad3dee1a2..8cbf6f044 100644 --- a/src/libstore/daemon.cc +++ b/src/libstore/daemon.cc @@ -7,6 +7,7 @@ #include "store-cast.hh" #include "gc-store.hh" #include "log-store.hh" +#include "indirect-root-store.hh" #include "path-with-outputs.hh" #include "finally.hh" #include "archive.hh" @@ -675,8 +676,8 @@ static void performOp(TunnelLogger * logger, ref store, Path path = absPath(readString(from)); logger->startWork(); - auto & gcStore = require(*store); - gcStore.addIndirectRoot(path); + auto & indirectRootStore = require(*store); + indirectRootStore.addIndirectRoot(path); logger->stopWork(); to << 1; diff --git a/src/libstore/gc-store.hh b/src/libstore/gc-store.hh index 2c26c65c4..ab1059fb1 100644 --- a/src/libstore/gc-store.hh +++ b/src/libstore/gc-store.hh @@ -71,19 +71,36 @@ struct GCResults }; +/** + * Mix-in class for \ref Store "stores" which expose a notion of garbage + * collection. + * + * Garbage collection will allow deleting paths which are not + * transitively "rooted". + * + * The notion of GC roots actually not part of this class. + * + * - The base `Store` class has `Store::addTempRoot()` because for a store + * that doesn't support garbage collection at all, a temporary GC root is + * safely implementable as no-op. + * + * @todo actually this is not so good because stores are *views*. + * Some views have only a no-op temp roots even though others to the + * same store allow triggering GC. For instance one can't add a root + * over ssh, but that doesn't prevent someone from gc-ing that store + * accesed via SSH locally). + * + * - The derived `LocalFSStore` class has `LocalFSStore::addPermRoot`, + * which is not part of this class because it relies on the notion of + * an ambient file system. There are stores (`ssh-ng://`, for one), + * that *do* support garbage collection but *don't* expose any file + * system, and `LocalFSStore::addPermRoot` thus does not make sense + * for them. + */ struct GcStore : public virtual Store { inline static std::string operationName = "Garbage collection"; - /** - * Add an indirect root, which is merely a symlink to `path` from - * `/nix/var/nix/gcroots/auto/`. `path` is supposed - * to be a symlink to a store path. The garbage collector will - * automatically remove the indirect root when it finds that - * `path` has disappeared. - */ - virtual void addIndirectRoot(const Path & path) = 0; - /** * Find the roots of the garbage collector. Each root is a pair * `(link, storepath)` where `link` is the path of the symlink diff --git a/src/libstore/gc.cc b/src/libstore/gc.cc index 20720fb99..26c87391c 100644 --- a/src/libstore/gc.cc +++ b/src/libstore/gc.cc @@ -1,7 +1,6 @@ #include "derivations.hh" #include "globals.hh" #include "local-store.hh" -#include "local-fs-store.hh" #include "finally.hh" #include @@ -50,7 +49,7 @@ void LocalStore::addIndirectRoot(const Path & path) } -Path LocalFSStore::addPermRoot(const StorePath & storePath, const Path & _gcRoot) +Path IndirectRootStore::addPermRoot(const StorePath & storePath, const Path & _gcRoot) { Path gcRoot(canonPath(_gcRoot)); diff --git a/src/libstore/indirect-root-store.hh b/src/libstore/indirect-root-store.hh new file mode 100644 index 000000000..59e45af45 --- /dev/null +++ b/src/libstore/indirect-root-store.hh @@ -0,0 +1,48 @@ +#pragma once +///@file + +#include "local-fs-store.hh" + +namespace nix { + +/** + * Mix-in class for implementing permanent roots as a pair of a direct + * (strong) reference and indirect weak reference to the first + * reference. + * + * See methods for details on the operations it represents. + */ +struct IndirectRootStore : public virtual LocalFSStore +{ + inline static std::string operationName = "Indirect GC roots registration"; + + /** + * Implementation of `LocalFSStore::addPermRoot` where the permanent + * root is a pair of + * + * - The user-facing symlink which all implementations must create + * + * - An additional weak reference known as the "indirect root" that + * points to that symlink. + * + * The garbage collector will automatically remove the indirect root + * when it finds that the symlink has disappeared. + * + * The implementation of this method is concrete, but it delegates + * to `addIndirectRoot()` which is abstract. + */ + Path addPermRoot(const StorePath & storePath, const Path & gcRoot) override final; + + /** + * Add an indirect root, which is a weak reference to the + * user-facing symlink created by `addPermRoot()`. + * + * @param path user-facing and user-controlled symlink to a store + * path. + * + * The form this weak-reference takes is implementation-specific. + */ + virtual void addIndirectRoot(const Path & path) = 0; +}; + +} diff --git a/src/libstore/local-fs-store.hh b/src/libstore/local-fs-store.hh index 2ee2ef0c8..488109501 100644 --- a/src/libstore/local-fs-store.hh +++ b/src/libstore/local-fs-store.hh @@ -40,6 +40,7 @@ class LocalFSStore : public virtual LocalFSStoreConfig, public virtual LogStore { public: + inline static std::string operationName = "Local Filesystem Store"; const static std::string drvsLogDir; @@ -49,9 +50,20 @@ public: ref getFSAccessor() override; /** - * Register a permanent GC root. + * Creates symlink from the `gcRoot` to the `storePath` and + * registers the `gcRoot` as a permanent GC root. The `gcRoot` + * symlink lives outside the store and is created and owned by the + * user. + * + * @param gcRoot The location of the symlink. + * + * @param storePath The store object being rooted. The symlink will + * point to `toRealPath(store.printStorePath(storePath))`. + * + * How the permanent GC root corresponding to this symlink is + * managed is implementation-specific. */ - Path addPermRoot(const StorePath & storePath, const Path & gcRoot); + virtual Path addPermRoot(const StorePath & storePath, const Path & gcRoot) = 0; virtual Path getRealStoreDir() { return realStoreDir; } diff --git a/src/libstore/local-store.hh b/src/libstore/local-store.hh index 2bdd46fa2..4125cacf4 100644 --- a/src/libstore/local-store.hh +++ b/src/libstore/local-store.hh @@ -5,8 +5,7 @@ #include "pathlocks.hh" #include "store-api.hh" -#include "local-fs-store.hh" -#include "gc-store.hh" +#include "indirect-root-store.hh" #include "sync.hh" #include "util.hh" @@ -68,7 +67,9 @@ struct LocalStoreConfig : virtual LocalFSStoreConfig std::string doc() override; }; -class LocalStore : public virtual LocalStoreConfig, public virtual LocalFSStore, public virtual GcStore +class LocalStore : public virtual LocalStoreConfig + , public virtual IndirectRootStore + , public virtual GcStore { private: @@ -209,6 +210,12 @@ private: public: + /** + * Implementation of IndirectRootStore::addIndirectRoot(). + * + * The weak reference merely is a symlink to `path' from + * /nix/var/nix/gcroots/auto/. + */ void addIndirectRoot(const Path & path) override; private: diff --git a/src/libstore/remote-store.cc b/src/libstore/remote-store.cc index 926ccd9d1..21258daec 100644 --- a/src/libstore/remote-store.cc +++ b/src/libstore/remote-store.cc @@ -822,15 +822,6 @@ void RemoteStore::addTempRoot(const StorePath & path) } -void RemoteStore::addIndirectRoot(const Path & path) -{ - auto conn(getConnection()); - conn->to << WorkerProto::Op::AddIndirectRoot << path; - conn.processStderr(); - readInt(conn->from); -} - - Roots RemoteStore::findRoots(bool censor) { auto conn(getConnection()); diff --git a/src/libstore/remote-store.hh b/src/libstore/remote-store.hh index 68ec29e6c..a1ae82a0f 100644 --- a/src/libstore/remote-store.hh +++ b/src/libstore/remote-store.hh @@ -126,8 +126,6 @@ public: void addTempRoot(const StorePath & path) override; - void addIndirectRoot(const Path & path) override; - Roots findRoots(bool censor) override; void collectGarbage(const GCOptions & options, GCResults & results) override; diff --git a/src/libstore/ssh-store.cc b/src/libstore/ssh-store.cc index 0200076c0..9c6c42ef4 100644 --- a/src/libstore/ssh-store.cc +++ b/src/libstore/ssh-store.cc @@ -1,5 +1,6 @@ #include "ssh-store-config.hh" #include "store-api.hh" +#include "local-fs-store.hh" #include "remote-store.hh" #include "remote-store-connection.hh" #include "remote-fs-accessor.hh" @@ -61,7 +62,7 @@ public: std::optional getBuildLogExact(const StorePath & path) override { unsupported("getBuildLogExact"); } -private: +protected: struct Connection : RemoteStore::Connection { @@ -93,9 +94,12 @@ private: ref SSHStore::openConnection() { auto conn = make_ref(); - conn->sshConn = master.startCommand( - fmt("%s --stdio", remoteProgram) - + (remoteStore.get() == "" ? "" : " --store " + shellEscape(remoteStore.get()))); + + std::string command = remoteProgram + " --stdio"; + if (remoteStore.get() != "") + command += " --store " + shellEscape(remoteStore.get()); + + conn->sshConn = master.startCommand(command); conn->to = FdSink(conn->sshConn->in.get()); conn->from = FdSource(conn->sshConn->out.get()); return conn; diff --git a/src/libstore/uds-remote-store.cc b/src/libstore/uds-remote-store.cc index 69dae2da5..99589f8b2 100644 --- a/src/libstore/uds-remote-store.cc +++ b/src/libstore/uds-remote-store.cc @@ -1,4 +1,5 @@ #include "uds-remote-store.hh" +#include "worker-protocol.hh" #include #include @@ -77,6 +78,15 @@ ref UDSRemoteStore::openConnection() } +void UDSRemoteStore::addIndirectRoot(const Path & path) +{ + auto conn(getConnection()); + conn->to << WorkerProto::Op::AddIndirectRoot << path; + conn.processStderr(); + readInt(conn->from); +} + + static RegisterStoreImplementation regUDSRemoteStore; } diff --git a/src/libstore/uds-remote-store.hh b/src/libstore/uds-remote-store.hh index b9d9a39b5..cdb28a001 100644 --- a/src/libstore/uds-remote-store.hh +++ b/src/libstore/uds-remote-store.hh @@ -3,7 +3,7 @@ #include "remote-store.hh" #include "remote-store-connection.hh" -#include "local-fs-store.hh" +#include "indirect-root-store.hh" namespace nix { @@ -21,7 +21,9 @@ struct UDSRemoteStoreConfig : virtual LocalFSStoreConfig, virtual RemoteStoreCon std::string doc() override; }; -class UDSRemoteStore : public virtual UDSRemoteStoreConfig, public virtual LocalFSStore, public virtual RemoteStore +class UDSRemoteStore : public virtual UDSRemoteStoreConfig + , public virtual IndirectRootStore + , public virtual RemoteStore { public: @@ -39,6 +41,16 @@ public: void narFromPath(const StorePath & path, Sink & sink) override { LocalFSStore::narFromPath(path, sink); } + /** + * Implementation of `IndirectRootStore::addIndirectRoot()` which + * delegates to the remote store. + * + * The idea is that the client makes the direct symlink, so it is + * owned managed by the client's user account, and the server makes + * the indirect symlink. + */ + void addIndirectRoot(const Path & path) override; + private: struct Connection : RemoteStore::Connection