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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
