You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by "DevinLeamy (via GitHub)" <gi...@apache.org> on 2024/03/21 21:55:56 UTC
[PR] [cgroups2] Introduces an interface to read and assign processes. [mesos]
DevinLeamy opened a new pull request, #532:
URL: https://github.com/apache/mesos/pull/532
`cgroup.procs` lists all of the processes inside of a cgroup, one per line. There may be duplicates. A single process can be moved into a cgroup by writing its process ID into `cgroup.procs`.
Here we introduce:
- Reading the process ids of the processes inside of a cgroup, and
- Assigning a process to a cgroup by process ID.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@mesos.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [cgroups2] Introduces an interface to read and assign processes. [mesos]
Posted by "DevinLeamy (via GitHub)" <gi...@apache.org>.
DevinLeamy commented on code in PR #532:
URL: https://github.com/apache/mesos/pull/532#discussion_r1536001471
##########
src/tests/containerizer/cgroups2_tests.cpp:
##########
@@ -123,6 +123,43 @@ TEST_F(Cgroups2Test, CGROUPS2_Path)
EXPECT_EQ("/sys/fs/cgroup/foo/bar", cgroups2::path("foo/bar"));
}
+
+TEST_F(Cgroups2Test, CGROUPS_Path)
Review Comment:
Removed the `ROOT_CGROUPS2_AssignProcessToCgroup` test and updated the test name.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@mesos.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [cgroups2] Introduces an interface to read and assign processes. [mesos]
Posted by "bmahler (via GitHub)" <gi...@apache.org>.
bmahler commented on code in PR #532:
URL: https://github.com/apache/mesos/pull/532#discussion_r1535810980
##########
src/linux/cgroups2.cpp:
##########
@@ -350,6 +350,47 @@ Try<string> cgroup(pid_t pid)
}
+Try<set<pid_t>> processes(const string& cgroup)
+{
+ if (!cgroups2::exists(cgroup)) {
+ return Error("Cgroup '" + cgroup + "' does not exist");
+ }
+
+ Try<string> contents = cgroups2::read<string>(cgroup, control::PROCESSES);
+ if (contents.isError()) {
+ return Error(
+ "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")) {
Review Comment:
maybe s/_pid/line/?
##########
src/linux/cgroups2.cpp:
##########
@@ -350,6 +350,47 @@ Try<string> cgroup(pid_t pid)
}
+Try<set<pid_t>> processes(const string& cgroup)
+{
+ if (!cgroups2::exists(cgroup)) {
+ return Error("Cgroup '" + cgroup + "' does not exist");
+ }
+
+ Try<string> contents = cgroups2::read<string>(cgroup, control::PROCESSES);
+ if (contents.isError()) {
+ return Error(
+ "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")) {
Review Comment:
we can re-use trimmed here?
##########
src/linux/cgroups2.cpp:
##########
@@ -350,6 +350,47 @@ Try<string> cgroup(pid_t pid)
}
+Try<set<pid_t>> processes(const string& cgroup)
+{
+ if (!cgroups2::exists(cgroup)) {
+ return Error("Cgroup '" + cgroup + "' does not exist");
+ }
+
+ Try<string> contents = cgroups2::read<string>(cgroup, control::PROCESSES);
+ if (contents.isError()) {
+ return Error(
+ "Failed to read cgroup.procs in '" + cgroup + "': " + contents.error());
+ }
+
+ string trimmed = strings::trim(*contents);
Review Comment:
hm.. a bit sad that we have to copy the whole string in the typical case where there are many processes in the cgroup, just to handle the case where it's a single newline character?
in general, could we not be handling the empty cases with a cheap check?
```
if (contents->empty() || contents->at(0) == '\n') {
return set<pid_t>();
}
```
are there actually more cases to handle? strangely they don't show the empty case:
```
Format
------
All interface files should be in one of the following formats whenever
possible::
New-line separated values
(when only one value can be written at once)
VAL0\n
VAL1\n
...
Space separated values
(when read-only or multiple values can be written at once)
VAL0 VAL1 ...\n
Flat keyed
KEY0 VAL0\n
KEY1 VAL1\n
...
Nested keyed
KEY0 SUB_KEY0=VAL00 SUB_KEY1=VAL01...
KEY1 SUB_KEY0=VAL10 SUB_KEY1=VAL11...
...
For a writable file, the format for writing should generally match
reading; however, controllers may allow omitting later fields or
implement restricted shortcuts for most common use cases.
For both flat and nested keyed files, only the values for a single key
can be written at a time. For nested keyed files, the sub key pairs
may be specified in any order and not all pairs have to be specified.
```
how is crun handling these cases efficiently?
##########
src/linux/cgroups2.hpp:
##########
@@ -76,6 +76,15 @@ Try<Nothing> move_process(const std::string& cgroup, pid_t pid);
Try<std::string> cgroup(pid_t pid);
+// Get the processes inside of a cgroup.
+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);
Review Comment:
there's the same function already with cgroups2::move_process..? assign or assign_process does seem like a better name, but did you mean to add two of the same function?
##########
src/linux/cgroups2.cpp:
##########
@@ -350,6 +350,47 @@ Try<string> cgroup(pid_t pid)
}
+Try<set<pid_t>> processes(const string& cgroup)
+{
+ if (!cgroups2::exists(cgroup)) {
+ return Error("Cgroup '" + cgroup + "' does not exist");
+ }
+
+ Try<string> contents = cgroups2::read<string>(cgroup, control::PROCESSES);
+ if (contents.isError()) {
+ return Error(
+ "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));
Review Comment:
we shouldn't need another trim here, given we've already split on newlines?
##########
src/linux/cgroups2.cpp:
##########
@@ -350,6 +350,47 @@ Try<string> cgroup(pid_t pid)
}
+Try<set<pid_t>> processes(const string& cgroup)
+{
+ if (!cgroups2::exists(cgroup)) {
+ return Error("Cgroup '" + cgroup + "' does not exist");
+ }
+
+ Try<string> contents = cgroups2::read<string>(cgroup, control::PROCESSES);
+ if (contents.isError()) {
+ return Error(
+ "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));
+ if (pid.isError()) {
+ return Error("Failed to parse pid: " + pid.error());
Review Comment:
we can include the string here to show which one failed to parse
##########
src/tests/containerizer/cgroups2_tests.cpp:
##########
@@ -123,6 +123,43 @@ TEST_F(Cgroups2Test, CGROUPS2_Path)
EXPECT_EQ("/sys/fs/cgroup/foo/bar", cgroups2::path("foo/bar"));
}
+
+TEST_F(Cgroups2Test, CGROUPS_Path)
Review Comment:
This test is about cgroups2::processes and process assignment, not path?
If this is fixed to be s/CGROUPS/CGROUPS2/ then this clashes with the existing CGROUPS2_Path test?
There's already ROOT_CGROUPS2_AssignProcessToCgroup which seems to be about the same as this test, minus cgroups2::processes?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@mesos.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [cgroups2] Introduces an interface to read and assign processes. [mesos]
Posted by "bmahler (via GitHub)" <gi...@apache.org>.
bmahler closed pull request #532: [cgroups2] Introduces an interface to read and assign processes.
URL: https://github.com/apache/mesos/pull/532
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@mesos.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [cgroups2] Introduces an interface to read and assign processes. [mesos]
Posted by "DevinLeamy (via GitHub)" <gi...@apache.org>.
DevinLeamy commented on code in PR #532:
URL: https://github.com/apache/mesos/pull/532#discussion_r1536002096
##########
src/linux/cgroups2.cpp:
##########
@@ -350,6 +350,47 @@ Try<string> cgroup(pid_t pid)
}
+Try<set<pid_t>> processes(const string& cgroup)
+{
+ if (!cgroups2::exists(cgroup)) {
+ return Error("Cgroup '" + cgroup + "' does not exist");
+ }
+
+ Try<string> contents = cgroups2::read<string>(cgroup, control::PROCESSES);
+ if (contents.isError()) {
+ return Error(
+ "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));
Review Comment:
Was doing it to be "safe". Removed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@mesos.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [cgroups2] Introduces an interface to read and assign processes. [mesos]
Posted by "DevinLeamy (via GitHub)" <gi...@apache.org>.
DevinLeamy commented on code in PR #532:
URL: https://github.com/apache/mesos/pull/532#discussion_r1535998268
##########
src/linux/cgroups2.hpp:
##########
@@ -76,6 +76,15 @@ Try<Nothing> move_process(const std::string& cgroup, pid_t pid);
Try<std::string> cgroup(pid_t pid);
+// Get the processes inside of a cgroup.
+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);
Review Comment:
Did not mean to have two. `assign` is the name used by the initial cgroups so I think that's the name we should use. I've opted to replace `move_process` with assign.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@mesos.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org