Skip to content

allow setting cgroup memory limits via both nix.conf and pre-build hooks #7388

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
cleverca22 wants to merge 1 commit into
base: master
Choose a base branch
from
Open

allow setting cgroup memory limits via both nix.conf and pre-build hooks #7388

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/libstore/build-result.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ struct BuildResult

/* User and system CPU time the build took. */
std::optional<std::chrono::microseconds> cpuUser, cpuSystem;
std::optional<uint64_t> memoryHigh;

bool success()
{
Expand Down
20 changes: 19 additions & 1 deletion src/libstore/build/local-derivation-goal.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,7 @@ void LocalDerivationGoal::killSandbox(bool getStats)
if (getStats) {
buildResult.cpuUser = stats.cpuUser;
buildResult.cpuSystem = stats.cpuSystem;
buildResult.memoryHigh = stats.memoryHigh;
}
#else
abort();
Expand Down Expand Up @@ -415,6 +416,7 @@ void LocalDerivationGoal::startBuilder()
throw Error("cannot determine cgroup name from /proc/self/cgroup");

auto ourCgroupPath = canonPath("/sys/fs/cgroup/" + ourCgroup);
ourCgroupPath = dirOf(ourCgroupPath);
Copy link
Member

@edolstra edolstra Dec 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is very confusing naming. Doing dirOf means it isn't our cgroup, but the parent's. And it's not clear that we want to be putting stuff in the parent cgroup, since it causes nix to break out of whatever resource controls are in place for its own cgroup. It especially wouldn't be nice if Nix builds escape from system.slice/nix-daemon.service.

Copy link
Contributor Author

@cleverca22 cleverca22 Dec 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i had modified things so the daemon runs in system.slice/nix-daemon.service/actual-daemon

the problem is that you cant certain things on system.slice/nix-daemon.service if it contains any process, everything must be in a child c-group

i'm not sure what the best solution to that is, and this currently needs an extra script around nix-daemon to move it to a child cgroup


if (!pathExists(ourCgroupPath))
throw Error("expected cgroup directory '%s'", ourCgroupPath);
Expand Down Expand Up @@ -720,6 +722,15 @@ void LocalDerivationGoal::startBuilder()
chownToBuilder(*cgroup);
chownToBuilder(*cgroup + "/cgroup.procs");
chownToBuilder(*cgroup + "/cgroup.threads");
auto parentCgroup = dirOf(*cgroup);
writeFile(parentCgroup + "/cgroup.subtree_control", "+memory");
Copy link
Member

@edolstra edolstra Dec 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will fail if the cgroup does not have memory in cgroup.controllers, or if it's not a leaf cgroup or one that only has pids in its children (see the "no internal processes" rule in cgroups(7)). In particular, this may cause non-root chroot builds to fail (i.e. --store /path/to/my/nix as a non-root user) since we can't count on memory being enabled.

It's not clear what the best solution is, but some options:

  • When nix-daemon starts, it can create a sub-cgroup (e.g. system.slice/nix-daemon.service/manager) for itself and move into it, and create sub-cgroups for the builds in the original cgroup (e.g. system.slice/nix-daemon.service/<build>). That way the original cgroup doesn't violate the "no internal processes" rule.
  • That doesn't work for the non-root case, since the original cgroup will likely have other processes in it (e.g. user.slice/user-1000.slice/user@1000.service/app.slice/app-org.kde.konsole-7edbbf6bbf864ef9a259f86f31b0fd90.scope). So we can't easily convert it into a cgroup that doesn't violate the "no internal processes" rule. I guess we could search upwards for the nearest parent that has the memory controller enabled and has no internal processes (e.g. /sys/fs/cgroup/user.slice/user-1000.slice/user@1000.service/app.slice in this example) but that's a bit naughty...
  • Allow the parent cgroup for builds to be specified in nix.conf.

Whatever the solution, Nix shouldn't fail if writing to cgroup.subtree_control fails.

writeFile(*cgroup + "/memory.oom.group", "1");
if (settings.memoryHigh) {
writeFile(*cgroup + "/memory.high", fmt("%d", settings.memoryHigh));
}
if (settings.memoryMax) {
writeFile(*cgroup + "/memory.max", fmt("%d", settings.memoryMax));
}
//chownToBuilder(*cgroup + "/cgroup.subtree_control");
}

Expand Down Expand Up @@ -751,7 +762,14 @@ void LocalDerivationGoal::startBuilder()
stExtraChrootDirs
};
auto state = stBegin;
auto lines = runProgram(settings.preBuildHook, false, args);
auto hookEnv = getEnv();
if (cgroup) hookEnv["NIX_CGROUP"] = *cgroup;
auto res = runProgram(RunOptions {.program = settings.preBuildHook, .searchPath = false, .args = args, .environment = hookEnv, .input = {} });

if (!statusOk(res.first))
throw ExecError(res.first, "program '%1%' %2%", settings.preBuildHook, statusToString(res.first));

auto lines = res.second;
auto lastPos = std::string::size_type{0};
for (auto nlPos = lines.find('\n'); nlPos != std::string::npos;
nlPos = lines.find('\n', lastPos))
Expand Down
5 changes: 5 additions & 0 deletions src/libstore/cgroup.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,11 @@ static CgroupStats destroyCgroup(const Path & cgroup, bool returnStats)
}
}

auto memoryhighPath = cgroup + "/memory.high";
if (pathExists(memoryhighPath)) {
stats.memoryHigh = string2Int<uint64_t>(trim(readFile(memoryhighPath)));
Copy link
Member

@edolstra edolstra Dec 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indentation is off here.

}
debug("memory events %s", readFile(cgroup + "/memory.events"));
}

if (rmdir(cgroup.c_str()) == -1)
Expand Down
1 change: 1 addition & 0 deletions src/libstore/cgroup.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ std::map<std::string, std::string> getCgroups(const Path & cgroupFile);
struct CgroupStats
{
std::optional<std::chrono::microseconds> cpuUser, cpuSystem;
std::optional<uint64_t> memoryHigh;
};

/* Destroy the cgroup denoted by 'path'. The postcondition is that
Expand Down
16 changes: 16 additions & 0 deletions src/libstore/globals.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -313,6 +313,22 @@ public:
`uid-range` system feature.
)"
};

Setting<uint64_t> memoryHigh{
this, 0, "memory-high",
R"(
sets the cgroup memory.high limit, causing the build to throttle if exceeded
depends on use-cgroups=true
Comment on lines +320 to +321
Copy link
Member

@edolstra edolstra Dec 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs proper capitalization/spelling/Markdown formatting.

)"
};

Setting<uint64_t> memoryMax{
this, 0, "memory-max",
R"(
sets the cgroup memory.max limit, causing the build to be killed if exceeded
depends on use-cgroups=true
Comment on lines +320 to +329
Copy link
Member

@SuperSandro2000 SuperSandro2000 Dec 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am missing some documentation here in what relation to each other you should set memory-high and memory-max. I would assume something like memory-high 3GB and memory-max 4GB?

Copy link
Contributor Author

@cleverca22 cleverca22 Dec 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

depends on how much a specific derivation needs, the goal is to keep track of how much memory each derivation is using, and not schedule too many on one machine
then use cgroups to enforce that it stays within the reserved amounts

)"
};
#endif

Setting<bool> impersonateLinux26{this, false, "impersonate-linux-26",
Expand Down
1 change: 1 addition & 0 deletions src/nix/build.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ nlohmann::json builtPathsWithResultToJSON(const std::vector<BuiltPathWithResult>
j["cpuUser"] = ((double) b.result->cpuUser->count()) / 1000000;
if (b.result->cpuSystem)
j["cpuSystem"] = ((double) b.result->cpuSystem->count()) / 1000000;
if (b.result->memoryHigh) j["memoryHigh"] = *b.result->memoryHigh;
}
res.push_back(j);
}, b.path.raw());
Expand Down