Re: [PR] [cgroups2][ebpf] Configure device access permissions. [mesos]
bmahler closed pull request #519: [cgroups2][ebpf] Configure device access permissions. URL: https://github.com/apache/mesos/pull/519 -- 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-unsubscr...@mesos.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [cgroups2] Addressing comments from #532 [mesos]
bmahler commented on code in PR #533:
URL: https://github.com/apache/mesos/pull/533#discussion_r1536201006
##
src/tests/containerizer/cgroups2_tests.cpp:
##
@@ -90,41 +90,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"));
-}
Review Comment:
why remove this?
--
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-unsubscr...@mesos.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [cgroups2] Addressing comments from #532 [mesos]
bmahler closed pull request #533: [cgroups2] Addressing comments from #532 URL: https://github.com/apache/mesos/pull/533 -- 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-unsubscr...@mesos.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [cgroups2] Introduces an interface to read and assign processes. [mesos]
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-unsubscr...@mesos.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [cgroups2] Introduces an interface to read and assign processes. [mesos]
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 cgroup(pid_t pid)
}
+Try> processes(const string& cgroup)
+{
+ if (!cgroups2::exists(cgroup)) {
+return Error("Cgroup '" + cgroup + "' does not exist");
+ }
+
+ Try contents = cgroups2::read(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();
+ }
+
+ set pids;
+ foreach (const string& _pid, strings::split(strings::trim(*contents), "\n"))
{
+Try pid = numify(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-unsubscr...@mesos.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [cgroups2] Introduces an interface to read and assign processes. [mesos]
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-unsubscr...@mesos.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [cgroups2] Introduces an interface to read and assign processes. [mesos]
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 move_process(const std::string& cgroup, pid_t pid); Try cgroup(pid_t pid); +// Get the processes inside of a cgroup. +Try> processes(const std::string& cgroup); + + +// Assign a process to a cgroup, by PID. This removes the process from its +// current cgroup. +Try 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-unsubscr...@mesos.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [cgroups2] Introduces an interface to read and assign processes. [mesos]
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 cgroup(pid_t pid)
}
+Try> processes(const string& cgroup)
+{
+ if (!cgroups2::exists(cgroup)) {
+return Error("Cgroup '" + cgroup + "' does not exist");
+ }
+
+ Try contents = cgroups2::read(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();
+ }
+
+ set 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 cgroup(pid_t pid)
}
+Try> processes(const string& cgroup)
+{
+ if (!cgroups2::exists(cgroup)) {
+return Error("Cgroup '" + cgroup + "' does not exist");
+ }
+
+ Try contents = cgroups2::read(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();
+ }
+
+ set 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 cgroup(pid_t pid)
}
+Try> processes(const string& cgroup)
+{
+ if (!cgroups2::exists(cgroup)) {
+return Error("Cgroup '" + cgroup + "' does not exist");
+ }
+
+ Try contents = cgroups2::read(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();
}
```
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 move_process(const std::string& cgroup, pid_t
pid);
Try cgroup(pid_t pid);
+// Get the processes inside of a cgroup.
+Try> processes(const std::string& cgroup);
+
+
+// Assign a process to a cgroup, by PID. This removes the process from its
+// current cgroup.
+Try 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 cgroup(pid_t pid)
}
+Try> processes(const string& cgroup)
+{
+ if (!cgroups2::exists(cgroup)) {
+return Error("Cgroup '" + cgroup + "' does not exist");
+ }
+
+ Try contents = cgroups2::read(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();
+ }
+
+ set pids;
+ foreach (const string& _pid, strings::split(strings::trim(*contents), "\n"))
{
+Try pid = numify(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 cgroup(pid_t pid)
}
+Try> processes(const string&
