This is an automated email from the ASF dual-hosted git repository.
bmahler pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/mesos.git
The following commit(s) were added to refs/heads/master by this push:
new 89d636d06 [cgroups2] Addressing comments from #532
89d636d06 is described below
commit 89d636d06f598c39f5e0637e7bc6329560e56676
Author: Devin Leamy <[email protected]>
AuthorDate: Fri Mar 22 16:43:42 2024 -0400
[cgroups2] Addressing comments from #532
Commit 60283dfed06a5fff4ecedaba4e67674dc01aa16e was merged before the
PR comments were addressed. This commit address the comments.
- We remove `cgroups2::move_process` in favor of `cgroups2::assign`,
which is consistent with the `cgroups` v1 function naming.
- We remove a duplicate test for assigning processes to cgroups.
- We remove a string copy when fetching the processes PIDs in a cgroup.
This closes #533
---
src/linux/cgroups2.cpp | 32 ++++++----------------
src/linux/cgroups2.hpp | 11 ++------
src/tests/containerizer/cgroups2_tests.cpp | 44 ++++++------------------------
3 files changed, 21 insertions(+), 66 deletions(-)
diff --git a/src/linux/cgroups2.cpp b/src/linux/cgroups2.cpp
index 310119bc8..de79c7fc1 100644
--- a/src/linux/cgroups2.cpp
+++ b/src/linux/cgroups2.cpp
@@ -312,12 +312,10 @@ Try<Nothing> destroy(const string& cgroup)
}
-Try<Nothing> move_process(const string& cgroup, pid_t pid)
+Try<Nothing> assign(const string& cgroup, pid_t pid)
{
- const string path = cgroups2::path(cgroup);
-
- if (!os::exists(path)) {
- return Error("There does not exist a cgroup at '" + path + "'");
+ if (!cgroups2::exists(cgroup)) {
+ return Error("Cgroup '" + cgroup + "' does not exist");
}
return cgroups2::write(cgroup, control::PROCESSES, stringify(pid));
@@ -369,16 +367,14 @@ Try<set<pid_t>> processes(const string& cgroup)
"Failed to read cgroup.procs in '" + cgroup + "': " +
contents.error());
}
- string trimmed = strings::trim(*contents);
- if (trimmed.empty()) {
- return set<pid_t>();
- }
-
set<pid_t> pids;
- foreach (const string& _pid, strings::split(strings::trim(*contents), "\n"))
{
- Try<pid_t> pid = numify<pid_t>(strings::trim(_pid));
+ foreach (const string& line, strings::split(*contents, "\n")) {
+ if (line.empty()) continue;
+
+ Try<pid_t> pid = numify<pid_t>(line);
if (pid.isError()) {
- return Error("Failed to parse pid: " + pid.error());
+ return Error(
+ "Failed to parse line '" + line + "' as a pid: " + pid.error());
}
pids.insert(*pid);
@@ -388,16 +384,6 @@ Try<set<pid_t>> processes(const string& cgroup)
}
-Try<Nothing> assign(const string& cgroup, pid_t pid)
-{
- if (!cgroups2::exists(cgroup)) {
- return Error("Cgroup '" + cgroup + "' does not exist");
- }
-
- return cgroups2::write(cgroup, control::PROCESSES, stringify(pid));
-}
-
-
string path(const string& cgroup)
{
return path::join(cgroups2::MOUNT_POINT, cgroup);
diff --git a/src/linux/cgroups2.hpp b/src/linux/cgroups2.hpp
index 9c9b9ceef..1913a750a 100644
--- a/src/linux/cgroups2.hpp
+++ b/src/linux/cgroups2.hpp
@@ -66,9 +66,9 @@ Try<Nothing> create(const std::string& cgroup, bool recursive
= false);
Try<Nothing> destroy(const std::string& cgroup);
-// Moves a process into a cgroup, by PID. Errors if the cgroup does not exist.
-// If the process is already in the cgroup, this operation is a NOP.
-Try<Nothing> move_process(const std::string& cgroup, pid_t pid);
+// Assign a process to a cgroup, by PID, removing the process from its
+// current cgroup. Returns an error if the cgroup does not exist.
+Try<Nothing> assign(const std::string& cgroup, pid_t pid);
// Get the cgroup that a process is part of, returns a relative path off of
@@ -80,11 +80,6 @@ Try<std::string> cgroup(pid_t pid);
Try<std::set<pid_t>> processes(const std::string& cgroup);
-// Assign a process to a cgroup, by PID. This removes the process from its
-// current cgroup.
-Try<Nothing> assign(const std::string& cgroup, pid_t pid);
-
-
// Get the absolute of a cgroup. The cgroup provided should not start with '/'.
std::string path(const std::string& cgroup);
diff --git a/src/tests/containerizer/cgroups2_tests.cpp
b/src/tests/containerizer/cgroups2_tests.cpp
index f49a4cf39..39ea2a6b3 100644
--- a/src/tests/containerizer/cgroups2_tests.cpp
+++ b/src/tests/containerizer/cgroups2_tests.cpp
@@ -68,6 +68,14 @@ TEST_F(Cgroups2Test, ROOT_CGROUPS2_Enabled)
}
+TEST_F(Cgroups2Test, CGROUPS2_Path)
+{
+ EXPECT_EQ("/sys/fs/cgroup/", cgroups2::path(cgroups2::ROOT_CGROUP));
+ EXPECT_EQ("/sys/fs/cgroup/foo", cgroups2::path("foo"));
+ EXPECT_EQ("/sys/fs/cgroup/foo/bar", cgroups2::path("foo/bar"));
+}
+
+
TEST_F(Cgroups2Test, ROOT_CGROUPS2_AvailableSubsystems)
{
@@ -90,41 +98,7 @@ TEST_F(Cgroups2Test, ROOT_CGROUPS2_AvailableSubsystems)
}
-TEST_F(Cgroups2Test, ROOT_CGROUPS2_AssignProcessToCgroup)
-{
- ASSERT_SOME(cgroups2::create(TEST_CGROUP));
-
- pid_t pid = ::fork();
- ASSERT_NE(-1, pid);
-
- if (pid == 0) {
- // In child process, wait for kill signal.
- while (true) { sleep(1); }
-
- SAFE_EXIT(
- EXIT_FAILURE, "Error, child should be killed before reaching here");
- }
-
- // Add the forked child to the cgroup and check that its 'cgroup' membership
- // is correct.
- EXPECT_SOME(cgroups2::move_process(TEST_CGROUP, pid));
- EXPECT_SOME_EQ(TEST_CGROUP, cgroups2::cgroup(pid));
-
- // Kill the child process.
- ASSERT_NE(-1, ::kill(pid, SIGKILL));
- AWAIT_EXPECT_WTERMSIG_EQ(SIGKILL, process::reap(pid));
-}
-
-
-TEST_F(Cgroups2Test, CGROUPS2_Path)
-{
- EXPECT_EQ("/sys/fs/cgroup/", cgroups2::path(cgroups2::ROOT_CGROUP));
- EXPECT_EQ("/sys/fs/cgroup/foo", cgroups2::path("foo"));
- EXPECT_EQ("/sys/fs/cgroup/foo/bar", cgroups2::path("foo/bar"));
-}
-
-
-TEST_F(Cgroups2Test, CGROUPS_Path)
+TEST_F(Cgroups2Test, ROOT_CGROUPS2_AssignProcesses)
{
Try<set<pid_t>> pids = cgroups2::processes(cgroups2::ROOT_CGROUP);
