-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/74872/#review226255
-----------------------------------------------------------
Ship it!
went over the diff with dleamy locally:
```
diff --git a/src/linux/cgroups2.cpp b/src/linux/cgroups2.cpp
index b4f462c72..b42a012bc 100644
--- a/src/linux/cgroups2.cpp
+++ b/src/linux/cgroups2.cpp
@@ -32,7 +32,8 @@ namespace cgroups2 {
const string FILE_SYSTEM = "cgroup2";
// Mount point for the cgroups2 file system.
-const string ROOT_MOUNT_POINT = "/sys/fs/cgroup";
+const string MOUNT_POINT = "/sys/fs/cgroup";
+
bool enabled()
{
@@ -40,101 +41,82 @@ bool enabled()
return supported.isSome() && *supported;
}
+
Try<Nothing> mount()
{
if (!cgroups2::enabled()) {
- return Error("Mounting the cgroups2 hierarchy failed as cgroups2 "
- "is not enabled");
+ return Error("cgroups2 is not enabled");
}
+
Try<bool> mounted = cgroups2::mounted();
if (mounted.isError()) {
- return Error(mounted.error());
+ return Error("Failed to check if cgroups2 filesystem is mounted: "
+ + mounted.error());
}
if (*mounted) {
- return Error("The cgroup2 file system is already mounted at '" +
- cgroups2::ROOT_MOUNT_POINT + "'");
+ return Error("cgroup2 filesystem is already mounted at"
+ " '" + cgroups2::MOUNT_POINT + "'");
}
- Try<Nothing> mkdir = os::mkdir(cgroups2::ROOT_MOUNT_POINT);
+ Try<Nothing> mkdir = os::mkdir(cgroups2::MOUNT_POINT);
if (mkdir.isError()) {
- return Error("Failed to create cgroups2 directory '" +
- cgroups2::ROOT_MOUNT_POINT +
- "': " + mkdir.error());
+ return Error("Failed to create cgroups2 directory"
+ " '" + cgroups2::MOUNT_POINT + "'"
+ ": " + mkdir.error());
}
- Try<Nothing> result = mesos::internal::fs::mount(
+ return mesos::internal::fs::mount(
None(),
- cgroups2::ROOT_MOUNT_POINT,
+ cgroups2::MOUNT_POINT,
cgroups2::FILE_SYSTEM,
0,
- None()
- );
-
- return Nothing();
+ None());
}
-Try<bool> mounted(const std::string& directory)
-{
- Try<MountTable> mountTable = MountTable::read("/proc/mounts");
- if (mountTable.isError()) {
- return Error("Failed to check if the cgroup2 file system is mounted: " +
- mountTable.error());
- }
-
- const string normalizedPath = *path::normalize(directory);
- foreach (MountTable::Entry entry, mountTable.get().entries) {
- if (entry.type == cgroups2::FILE_SYSTEM && entry.dir == normalizedPath) {
- return true;
- }
- }
-
- return false;
-}
-
-Try<bool> mounted_at_root()
-{
- return mounted(ROOT_MOUNT_POINT);
-}
Try<bool> mounted()
{
Try<MountTable> mountTable = MountTable::read("/proc/mounts");
if (mountTable.isError()) {
- return Error("Failed to check if the cgroup2 file system is mounted: " +
- mountTable.error());
+ return Error("Failed to read /proc/mounts: " + mountTable.error());
}
foreach (MountTable::Entry entry, mountTable.get().entries) {
if (entry.type == cgroups2::FILE_SYSTEM) {
- return true;
+ if (entry.dir == MOUNT_POINT) {
+ return true;
+ }
+ return Error("Found cgroups2 mount at an unexpected location"
+ " '" + entry.dir + "'");
}
}
return false;
}
+
Try<Nothing> unmount()
{
- Try<bool> mounted = cgroups2::mounted_at_root();
+ Try<bool> mounted = cgroups2::mounted();
if (mounted.isError()) {
- return Error("Failed to unmount the cgroup2 hierarchy: " +
mounted.error());
+ return Error("Failed to check if the cgroup2 filesystem is mounted: "
+ + mounted.error());
}
- if (*mounted) {
- return Error("Failed to unmount the cgroup2 hierarchy because it is "
- "not mounted at " + ROOT_MOUNT_POINT);
+ if (!*mounted) {
+ return Error("cgroups2 filesystem is not mounted");
}
- Try<Nothing> result = mesos::internal::fs::unmount(ROOT_MOUNT_POINT);
+ Try<Nothing> result = mesos::internal::fs::unmount(MOUNT_POINT);
if (result.isError()) {
- return Error("Failed to unmount the cgroup2 hierarchy '" +
- cgroups2::ROOT_MOUNT_POINT + "': " + result.error());
+ return Error("Failed to unmount the cgroup2 hierarchy" +
+ " '" + cgroups2::MOUNT_POINT + "': " + result.error());
}
- Try<Nothing> rmdir = os::rmdir(cgroups2::ROOT_MOUNT_POINT);
+ Try<Nothing> rmdir = os::rmdir(cgroups2::MOUNT_POINT);
if (rmdir.isError()) {
return Error(
- "Failed to remove directory '" + cgroups2::ROOT_MOUNT_POINT + "': " +
+ "Failed to remove directory '" + cgroups2::MOUNT_POINT + "': " +
rmdir.error());
}
}
diff --git a/src/linux/cgroups2.hpp b/src/linux/cgroups2.hpp
index d3a1d892f..0c54e464b 100644
--- a/src/linux/cgroups2.hpp
+++ b/src/linux/cgroups2.hpp
@@ -29,10 +29,8 @@ bool enabled();
// the cgroups v2 file system is already mounted.
Try<Nothing> mount();
-// Checks if the cgroup2 file systems is mounted at /sys/fs/cgroup.
-Try<bool> mounted_at_root();
-
-// Checks if the cgroup2 file systems is mounted.
+// Checks if the cgroup2 file systems is mounted at /sys/fs/cgroup,
+// returns an error if the mount is found at an unexpected location.
Try<bool> mounted();
// Unmounts the cgroups2 file system from /sys/fs/cgroup. Errors if
@@ -42,4 +40,4 @@ Try<Nothing> unmount();
} // namespace cgroups2
-#endif // __CGROUPS_V2_HPP__
\ No newline at end of file
+#endif // __CGROUPS_V2_HPP__
```
- Benjamin Mahler
On Feb. 27, 2024, 7:49 p.m., Devin Leamy wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/74872/
> -----------------------------------------------------------
>
> (Updated Feb. 27, 2024, 7:49 p.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Introduces:
> - `cgroups2::mount()`: Mount the cgroup2 hierarchy.
> - `cgroups2::mounted()`: Check if the cgroup2 hierarchy is mounted.
> - `cgroups2::mounted_at_root()`: Check if the cgroup2 hierarchy is mounted
> at /sys/fs/cgroup.
> - `cgroups2::unmount()`: Unmount the cgroup2 hierarchy from /sys/fs/cgroup.
>
> The root mount point for the cgroup2 file system is hard-coded to be
> /sys/fs/cgroup,
> which is the default mount point on most systems.
>
>
> Diffs
> -----
>
> src/linux/cgroups2.hpp 833960d301335541f0bb7ad5c0e05e7222d7bc06
> src/linux/cgroups2.cpp baa13557e31234281da9a40526b45df0444061d4
>
>
> Diff: https://reviews.apache.org/r/74872/diff/4/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Devin Leamy
>
>
