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/27 14:30:35 UTC
[PR] [cgroups2] Added path helpers that define the structure of the cgroup… [mesos]
DevinLeamy opened a new pull request, #537:
URL: https://github.com/apache/mesos/pull/537
…2 filesystem.
Because of the unified hierarchy and internal processes constraint in cgroups v2, a new cgroup directory structure is used for cgroups in cgroups v2.
Here we introduce paths helpers and lay out the directory structure of the cgroup2 filesystem, used in cgroups v2.
--
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] Added path helpers that define the structure of the cgroup2 filesystem [mesos]
Posted by "DevinLeamy (via GitHub)" <gi...@apache.org>.
DevinLeamy commented on code in PR #537:
URL: https://github.com/apache/mesos/pull/537#discussion_r1541237582
##########
src/slave/containerizer/mesos/paths.cpp:
##########
@@ -654,6 +654,30 @@ Option<ContainerID> parseCgroupPath(
return current;
}
+namespace cgroups2 {
+
+string agent(const string& root, bool leaf)
+{
+ if (leaf) { return path::join(root, "agent", "leaf"); }
+ else { return path::join(root, "agent"); }
Review Comment:
IMO this looks nicer than using the ternary operator or removing the `else`, because the paths line up. Ditto below.
--
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] Added path helpers that define the structure of the cgroup2 filesystem [mesos]
Posted by "DevinLeamy (via GitHub)" <gi...@apache.org>.
DevinLeamy commented on code in PR #537:
URL: https://github.com/apache/mesos/pull/537#discussion_r1541270546
##########
src/slave/containerizer/mesos/paths.cpp:
##########
@@ -654,6 +654,30 @@ Option<ContainerID> parseCgroupPath(
return current;
}
+namespace cgroups2 {
+
Review Comment:
Could have additionally added:
```cpp
// Path to the top-level Mesos cgroup. All global resource constraints
// are impose from this cgroup.
std::string root(const std::string& root)
{
return root;
}
```
But given that it takes the flag called `cgroups_root` and simply just returns its parameter I didn't see much of a point.
--
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] Added path helpers that define the structure of the cgroup2 filesystem [mesos]
Posted by "bmahler (via GitHub)" <gi...@apache.org>.
bmahler closed pull request #537: [cgroups2] Added path helpers that define the structure of the cgroup2 filesystem
URL: https://github.com/apache/mesos/pull/537
--
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] Added path helpers that define the structure of the cgroup2 filesystem [mesos]
Posted by "bmahler (via GitHub)" <gi...@apache.org>.
bmahler commented on code in PR #537:
URL: https://github.com/apache/mesos/pull/537#discussion_r1542037947
##########
src/tests/containerizer/mesos_containerizer_paths_tests.cpp:
##########
@@ -95,6 +95,51 @@ TEST(MesosContainerizerPathsTest, BuildPathJoinMode)
path::join(parent.value(), separator, child.value()));
}
+TEST(MesosContainerizerPathsTest, CGROUPS2_Cgroups2Paths)
+{
+ namespace cgroups2 = mesos::internal::slave::containerizer::paths::cgroups2;
Review Comment:
indentation?
##########
src/tests/containerizer/mesos_containerizer_paths_tests.cpp:
##########
@@ -95,6 +95,51 @@ TEST(MesosContainerizerPathsTest, BuildPathJoinMode)
path::join(parent.value(), separator, child.value()));
}
+TEST(MesosContainerizerPathsTest, CGROUPS2_Cgroups2Paths)
+{
+ namespace cgroups2 = mesos::internal::slave::containerizer::paths::cgroups2;
+
+ const string& ROOT = "mesos";
+
+ ContainerID parent;
+ parent.set_value("parent");
+
+ ContainerID child1;
+ child1.set_value("child2");
+ child1.mutable_parent()->CopyFrom(parent);
+
+ ContainerID child2;
+ child2.set_value("child2");
+ child2.mutable_parent()->CopyFrom(child1);
+
+ EXPECT_EQ("mesos/agent", cgroups2::agent(ROOT));
+ EXPECT_EQ("mesos/agent/leaf", cgroups2::agent(ROOT, true));
+
+ EXPECT_EQ(
+ path::join(ROOT, parent.value()), cgroups2::container(ROOT, parent));
+ EXPECT_EQ(path::join(
+ ROOT, parent.value(), "leaf"), cgroups2::container(ROOT, parent, true));
+
+ EXPECT_EQ(
+ path::join(ROOT, parent.value(), "mesos", child1.value()),
+ cgroups2::container(ROOT, child1));
+ EXPECT_EQ(
+ path::join(ROOT, parent.value(), "mesos", child1.value(), "leaf"),
+ cgroups2::container(ROOT, child1, true));
+
+ EXPECT_EQ(
+ path::join(
+ ROOT,
+ parent.value(), "mesos", child1.value(), "mesos", child2.value()),
+ cgroups2::container(ROOT, child2));
Review Comment:
```
EXPECT_EQ("mesos/parent/mesos/child1/mesos/child2",
cgroups2::container("mesos", child2));
```
##########
src/slave/containerizer/mesos/paths.hpp:
##########
@@ -286,6 +286,44 @@ Option<ContainerID> parseCgroupPath(
const std::string& cgroupsRoot,
const std::string& cgroup);
+// All cgroups v2 paths are either leaf or non-leaf paths. Leaf paths end
+// in `/leaf`, contain processes, and may impose resource constraints. Non-leaf
+// paths do not contain processes and may impose resource constraints.
+// This is done to avoid the restrictions of the "Internal Process Contraint".
+// More information: https://docs.kernel.org/admin-guide/cgroup-v2.html#no-internal-process-constraint
+//
+// The cgroup2 file system has the structure:
+// <MOUNT>/<root>/agent Mesos Agent constraints
+// <MOUNT>/<root>/agent/leaf Mesos Agent process
+//
+// <MOUNT>/<root>/<id> Container constraints
+// <MOUNT>/<root>/<id>/leaf Container process
+//
+// <MOUNT>/<root>/<id>/mesos/<id2> Nested container constraints
+// <MOUNT>/<root>/<id>/mesos/<id2>/leaf Nested container process
+//
+// For every new level of nesting, `/mesos/<idN>` is added to the path.
+// Note: We assume these nested containers are not configured to "share"
+// cgroups with their parent, in which case they would use the same cgroup
+// paths as their parent.
Review Comment:
can probably remove this? I think in v2 the share_cgroups flag should probably be aliased to a new name like shared_isolation, since:
* this is what the user cares about, and
* creating the cgroups lets us get stats across the executor, nested containers, even though they aren't isolated from each other, which is nice
--
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