From 46b31880458f0cbea2a45a42734d1ac76c9ea88f Mon Sep 17 00:00:00 2001 From: Parker Hoyes Date: Mon, 26 Aug 2024 13:50:22 +0000 Subject: [PATCH 1/4] Move daemon process into sub-cgroup The daemon process is now moved into a new sub-cgroup called nix-daemon when the daemon starts. This is necessary to abide by the no-processes-in-inner-nodes rule, because the service cgroup becomes an inner node when the child cgroups for the build are created (see LocalDerivationGoal::startBuilder()). See #9675 --- .../unix/build/local-derivation-goal.cc | 21 ++++++------ src/libutil/linux/cgroup.cc | 32 +++++++++++++++++++ src/libutil/linux/cgroup.hh | 9 ++++++ src/nix/unix/daemon.cc | 25 +++++++++++++++ 4 files changed, 75 insertions(+), 12 deletions(-) diff --git a/src/libstore/unix/build/local-derivation-goal.cc b/src/libstore/unix/build/local-derivation-goal.cc index 01a133766..d55278a52 100644 --- a/src/libstore/unix/build/local-derivation-goal.cc +++ b/src/libstore/unix/build/local-derivation-goal.cc @@ -444,25 +444,22 @@ void LocalDerivationGoal::startBuilder() #if __linux__ experimentalFeatureSettings.require(Xp::Cgroups); + /* If we're running from the daemon, then this will return the + root cgroup of the service. Otherwise, it will return the + current cgroup. */ + auto rootCgroup = getRootCgroup(); auto cgroupFS = getCgroupFS(); if (!cgroupFS) throw Error("cannot determine the cgroups file system"); - - auto ourCgroups = getCgroups("/proc/self/cgroup"); - auto ourCgroup = ourCgroups[""]; - if (ourCgroup == "") - throw Error("cannot determine cgroup name from /proc/self/cgroup"); - - auto ourCgroupPath = canonPath(*cgroupFS + "/" + ourCgroup); - - if (!pathExists(ourCgroupPath)) - throw Error("expected cgroup directory '%s'", ourCgroupPath); + auto rootCgroupPath = canonPath(*cgroupFS + "/" + rootCgroup); + if (!pathExists(rootCgroupPath)) + throw Error("expected cgroup directory '%s'", rootCgroupPath); static std::atomic counter{0}; cgroup = buildUser - ? fmt("%s/nix-build-uid-%d", ourCgroupPath, buildUser->getUID()) - : fmt("%s/nix-build-pid-%d-%d", ourCgroupPath, getpid(), counter++); + ? fmt("%s/nix-build-uid-%d", rootCgroupPath, buildUser->getUID()) + : fmt("%s/nix-build-pid-%d-%d", rootCgroupPath, getpid(), counter++); debug("using cgroup '%s'", *cgroup); diff --git a/src/libutil/linux/cgroup.cc b/src/libutil/linux/cgroup.cc index 140ff4566..706f0f159 100644 --- a/src/libutil/linux/cgroup.cc +++ b/src/libutil/linux/cgroup.cc @@ -6,6 +6,7 @@ #include #include +#include #include #include #include @@ -144,4 +145,35 @@ CgroupStats destroyCgroup(const Path & cgroup) return destroyCgroup(cgroup, true); } +std::string getCurrentCgroup() +{ + auto cgroupFS = getCgroupFS(); + if (!cgroupFS) + throw Error("cannot determine the cgroups file system"); + + auto ourCgroups = getCgroups("/proc/self/cgroup"); + auto ourCgroup = ourCgroups[""]; + if (ourCgroup == "") + throw Error("cannot determine cgroup name from /proc/self/cgroup"); + return ourCgroup; +} + +static std::optional rootCgroup; +static std::mutex rootCgroupMutex; + +std::string getRootCgroup() +{ + { + std::lock_guard guard(rootCgroupMutex); + if (rootCgroup) + return *rootCgroup; + } + auto current = getCurrentCgroup(); + std::lock_guard guard(rootCgroupMutex); + if (rootCgroup) + return *rootCgroup; + rootCgroup = current; + return current; +} + } diff --git a/src/libutil/linux/cgroup.hh b/src/libutil/linux/cgroup.hh index 783a0ab87..87d135ba6 100644 --- a/src/libutil/linux/cgroup.hh +++ b/src/libutil/linux/cgroup.hh @@ -25,4 +25,13 @@ struct CgroupStats */ CgroupStats destroyCgroup(const Path & cgroup); +std::string getCurrentCgroup(); + +/** + * Get the cgroup that should be used as the parent when creating new + * sub-cgroups. The first time this is called, the current cgroup will be + * returned, and then all subsequent calls will return the original cgroup. + */ +std::string getRootCgroup(); + } diff --git a/src/nix/unix/daemon.cc b/src/nix/unix/daemon.cc index 66d8dbcf0..746963a01 100644 --- a/src/nix/unix/daemon.cc +++ b/src/nix/unix/daemon.cc @@ -33,6 +33,10 @@ #include #include +#if __linux__ +#include "cgroup.hh" +#endif + #if __APPLE__ || __FreeBSD__ #include #endif @@ -312,6 +316,27 @@ static void daemonLoop(std::optional forceTrustClientOpt) // Get rid of children automatically; don't let them become zombies. setSigChldAction(true); + #if __linux__ + if (settings.useCgroups) { + experimentalFeatureSettings.require(Xp::Cgroups); + + // This also sets the root cgroup to the current one. + auto rootCgroup = getRootCgroup(); + auto cgroupFS = getCgroupFS(); + if (!cgroupFS) + throw Error("cannot determine the cgroups file system"); + auto rootCgroupPath = canonPath(*cgroupFS + "/" + rootCgroup); + if (!pathExists(rootCgroupPath)) + throw Error("expected cgroup directory '%s'", rootCgroupPath); + auto daemonCgroupPath = rootCgroupPath + "/nix-daemon"; + // Create new sub-cgroup for the daemon. + if (mkdir(daemonCgroupPath.c_str(), 0755) != 0 && errno != EEXIST) + throw SysError("creating cgroup '%s'", daemonCgroupPath); + // Move daemon into the new cgroup. + writeFile(daemonCgroupPath + "/cgroup.procs", fmt("%d", getpid())); + } + #endif + // Loop accepting connections. while (1) { From 4c88deef38678154f3043ca2e0f4bd84a3c6cbc5 Mon Sep 17 00:00:00 2001 From: Parker Hoyes Date: Tue, 3 Sep 2024 17:27:56 +0000 Subject: [PATCH 2/4] Add tests for daemon with cgroups --- tests/nixos/cgroups/default.nix | 40 +++++++++++++++++++++++++++++++++ tests/nixos/cgroups/hang.nix | 10 +++++++++ tests/nixos/default.nix | 2 ++ 3 files changed, 52 insertions(+) create mode 100644 tests/nixos/cgroups/default.nix create mode 100644 tests/nixos/cgroups/hang.nix diff --git a/tests/nixos/cgroups/default.nix b/tests/nixos/cgroups/default.nix new file mode 100644 index 000000000..b8febbf4b --- /dev/null +++ b/tests/nixos/cgroups/default.nix @@ -0,0 +1,40 @@ +{ nixpkgs, ... }: + +{ + name = "cgroups"; + + nodes = + { + host = + { config, pkgs, ... }: + { virtualisation.additionalPaths = [ pkgs.stdenvNoCC ]; + nix.extraOptions = + '' + extra-experimental-features = nix-command auto-allocate-uids cgroups + extra-system-features = uid-range + ''; + nix.settings.use-cgroups = true; + nix.nixPath = [ "nixpkgs=${nixpkgs}" ]; + }; + }; + + testScript = { nodes }: '' + start_all() + + host.wait_for_unit("multi-user.target") + + # Start build in background + host.execute("NIX_REMOTE=daemon nix build --auto-allocate-uids --file ${./hang.nix} >&2 &") + service = "/sys/fs/cgroup/system.slice/nix-daemon.service" + + # Wait for cgroups to be created + host.succeed(f"until [ -e {service}/nix-daemon ]; do sleep 1; done", timeout=30) + host.succeed(f"until [ -e {service}/nix-build-uid-* ]; do sleep 1; done", timeout=30) + + # Check that there aren't processes where there shouldn't be, and that there are where there should be + host.succeed(f'[ -z "$(cat {service}/cgroup.procs)" ]') + host.succeed(f'[ -n "$(cat {service}/nix-daemon/cgroup.procs)" ]') + host.succeed(f'[ -n "$(cat {service}/nix-build-uid-*/cgroup.procs)" ]') + ''; + +} diff --git a/tests/nixos/cgroups/hang.nix b/tests/nixos/cgroups/hang.nix new file mode 100644 index 000000000..cefe2d031 --- /dev/null +++ b/tests/nixos/cgroups/hang.nix @@ -0,0 +1,10 @@ +{ }: + +with import {}; + +runCommand "hang" + { requiredSystemFeatures = "uid-range"; + } + '' + sleep infinity + '' diff --git a/tests/nixos/default.nix b/tests/nixos/default.nix index 40d29b371..62fc6b10f 100644 --- a/tests/nixos/default.nix +++ b/tests/nixos/default.nix @@ -157,4 +157,6 @@ in s3-binary-cache-store = runNixOSTestFor "x86_64-linux" ./s3-binary-cache-store.nix; fsync = runNixOSTestFor "x86_64-linux" ./fsync.nix; + + cgroups = runNixOSTestFor "x86_64-linux" ./cgroups; } From 03484641a1c712b6d10f636e7dbb5280c1668c4b Mon Sep 17 00:00:00 2001 From: Parker Hoyes Date: Wed, 4 Sep 2024 18:11:16 +0000 Subject: [PATCH 3/4] Simplify getRootCgroup() Static local initializers are atomic in C++. --- src/libutil/linux/cgroup.cc | 17 ++--------------- 1 file changed, 2 insertions(+), 15 deletions(-) diff --git a/src/libutil/linux/cgroup.cc b/src/libutil/linux/cgroup.cc index 706f0f159..ad3e8a017 100644 --- a/src/libutil/linux/cgroup.cc +++ b/src/libutil/linux/cgroup.cc @@ -6,7 +6,6 @@ #include #include -#include #include #include #include @@ -158,22 +157,10 @@ std::string getCurrentCgroup() return ourCgroup; } -static std::optional rootCgroup; -static std::mutex rootCgroupMutex; - std::string getRootCgroup() { - { - std::lock_guard guard(rootCgroupMutex); - if (rootCgroup) - return *rootCgroup; - } - auto current = getCurrentCgroup(); - std::lock_guard guard(rootCgroupMutex); - if (rootCgroup) - return *rootCgroup; - rootCgroup = current; - return current; + static std::string rootCgroup = getCurrentCgroup(); + return rootCgroup; } } From bd6ae2f3b929e8bf280692f3cae36bce7f688b4f Mon Sep 17 00:00:00 2001 From: Parker Hoyes Date: Wed, 4 Sep 2024 19:10:31 +0000 Subject: [PATCH 4/4] Use getCurrentCgroup() in getMaxCPU() --- src/libutil/current-process.cc | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/libutil/current-process.cc b/src/libutil/current-process.cc index 0bc46d746..ed1c1ca6c 100644 --- a/src/libutil/current-process.cc +++ b/src/libutil/current-process.cc @@ -32,11 +32,7 @@ unsigned int getMaxCPU() auto cgroupFS = getCgroupFS(); if (!cgroupFS) return 0; - auto cgroups = getCgroups("/proc/self/cgroup"); - auto cgroup = cgroups[""]; - if (cgroup == "") return 0; - - auto cpuFile = *cgroupFS + "/" + cgroup + "/cpu.max"; + auto cpuFile = *cgroupFS + "/" + getCurrentCgroup() + "/cpu.max"; auto cpuMax = readFile(cpuFile); auto cpuMaxParts = tokenizeString>(cpuMax, " \n");