bmahler commented on code in PR #539:
URL: https://github.com/apache/mesos/pull/539#discussion_r1543762327
##########
src/slave/main.cpp:
##########
@@ -76,6 +76,13 @@
#ifdef __linux__
#include "linux/cgroups.hpp"
#include "linux/systemd.hpp"
+
+#ifdef ENABLE_CGROUPS_V2
+#include "linux/cgroups2.hpp"
+#endif // ENABLE_CGROUPS_V2
+
+#include "slave/containerizer/mesos/isolators/cgroups2/cgroups2.hpp"
Review Comment:
ifdef guard this too?
##########
src/slave/containerizer/mesos/containerizer.cpp:
##########
@@ -363,6 +368,21 @@ Try<MesosContainerizer*> MesosContainerizer::create(
Shared<Provisioner> provisioner = _provisioner->share();
+#ifdef __linux__
+ // Initialize either the cgroups v2 or cgroups v1 isolator, based on what
+ // is available on the host machine.
+ auto cgroupsIsolatorSelector = [] (const Flags& flags) -> Try<Isolator*> {
+ #ifdef ENABLE_CGROUPS_V2
+ Try<bool> supported =
+ mesos::internal::slave::cgroups2_isolator::supported();
+ if (supported.isSome() && *supported) {
+ return Cgroups2IsolatorProcess::create(flags);
+ }
+ #endif // ENABLE_CGROUPS_V2
+
+ return CgroupsIsolatorProcess::create(flags);
+ };
Review Comment:
Hm.. it seems like mounted is already sufficient? Ditto for the main code?
```suggestion
#ifdef __linux__
// Initialize either the cgroups v2 or cgroups v1 isolator, based on what
// is available on the host machine.
auto cgroupsIsolatorSelector = [] (const Flags& flags) -> Try<Isolator*> {
#ifdef ENABLE_CGROUPS_V2
Try<bool> mounted = cgroups2::mounted();
if (mounted.isError() {
return Error("Failed to determine if cgroups2 is mounted": +
mounted.error());
}
if (*mounted) {
return Cgroups2IsolatorProcess::create(flags);
}
#endif // ENABLE_CGROUPS_V2
return CgroupsIsolatorProcess::create(flags);
};
```
##########
src/slave/main.cpp:
##########
@@ -147,6 +155,144 @@ const char* malloc_conf = "narenas:4";
#ifdef __linux__
+
+#ifdef ENABLE_CGROUPS_V2
+// Log any processes inside of a cgroup.
+static Try<Nothing> checkForProcesses(const string& cgroup)
+{
+ Try<set<pid_t>> processes = cgroups2::processes(cgroup);
+ if (processes.isError()) {
+ return Error(
+ "Failed to check for existing processes in cgroup '" + cgroup + "': "
+ + processes.error());
+ }
+
+ if (!processes->empty()) {
+ vector<string> infos;
+ foreach (pid_t pid, *processes) {
+ Result<os::Process> proc = os::process(pid);
+
+ // Print the command if it's available.
+ if (proc.isSome()) {
+ infos.push_back(stringify(pid) + " '" + proc->command + "'");
+ } else {
+ infos.push_back(stringify(pid));
+ }
+ }
+
+ LOG(INFO) << "Found process(es) in the cgroup '" << cgroup << "'. "
+ << "Consider checking the following process(es) listed in "
+ << path::join("/sys/fs/cgroup", cgroup, "cgroup.procs")
+ << ":\n" << strings::join("\n", infos);
+ }
+
+ return Nothing();
+}
+// Initialize Mesos cgroups for cgroups v2.
Review Comment:
double newline here
##########
src/slave/main.cpp:
##########
@@ -147,6 +155,144 @@ const char* malloc_conf = "narenas:4";
#ifdef __linux__
+
+#ifdef ENABLE_CGROUPS_V2
+// Log any processes inside of a cgroup.
+static Try<Nothing> checkForProcesses(const string& cgroup)
Review Comment:
maybe logProcessesInCgroup?
##########
src/slave/main.cpp:
##########
@@ -406,13 +541,29 @@ int main(int argc, char** argv)
}
#ifdef __linux__
- // Move the agent process into its own cgroup for each of the specified
- // subsystems if necessary before the process is initialized.
if (flags.agent_subsystems.isSome()) {
- Try<Nothing> assign = assignCgroups(flags);
- if (assign.isError()) {
- EXIT(EXIT_FAILURE) << assign.error();
- }
+ // Use the cgroups v2 isolator if it is supported. Otherwise, use
+ // the cgroups v1 isolator.
+ do {
+ #ifdef ENABLE_CGROUPS_V2
+ Try<bool> supported = slave::cgroups2_isolator::supported();
+
+ if (supported.isSome() && *supported) {
+ Try<Nothing> initCgroups2 = initializeCgroups2(flags);
+ if (initCgroups2.isError()) {
+ EXIT(EXIT_FAILURE) << initCgroups2.error();
+ }
+
+ break;
+ }
+ #endif // ENABLE_CGROUPS_V2
+ // Move the agent process into its own cgroup for each of the specified
+ // subsystems if necessary before the process is initialized.
+ Try<Nothing> assign = assignCgroups(flags);
+ if (assign.isError()) {
+ EXIT(EXIT_FAILURE) << assign.error();
+ }
+ } while (false);
Review Comment:
yeah maybe a lambda + early return rather than a loop + break?
##########
src/slave/main.cpp:
##########
@@ -147,6 +155,144 @@ const char* malloc_conf = "narenas:4";
#ifdef __linux__
+
+#ifdef ENABLE_CGROUPS_V2
+// Log any processes inside of a cgroup.
+static Try<Nothing> checkForProcesses(const string& cgroup)
+{
+ Try<set<pid_t>> processes = cgroups2::processes(cgroup);
+ if (processes.isError()) {
+ return Error(
+ "Failed to check for existing processes in cgroup '" + cgroup + "': "
+ + processes.error());
+ }
+
+ if (!processes->empty()) {
+ vector<string> infos;
+ foreach (pid_t pid, *processes) {
+ Result<os::Process> proc = os::process(pid);
+
+ // Print the command if it's available.
+ if (proc.isSome()) {
+ infos.push_back(stringify(pid) + " '" + proc->command + "'");
+ } else {
+ infos.push_back(stringify(pid));
+ }
+ }
+
+ LOG(INFO) << "Found process(es) in the cgroup '" << cgroup << "'. "
+ << "Consider checking the following process(es) listed in "
+ << path::join("/sys/fs/cgroup", cgroup, "cgroup.procs")
+ << ":\n" << strings::join("\n", infos);
+ }
+
+ return Nothing();
+}
+// Initialize Mesos cgroups for cgroups v2.
+//
+// Ensures that cgroups v2 is available, correctly mounted, and all of the
+// requested controllers are available. If correctly setup, the requested
+// controllers are enabled in the root cgroup, and the Mesos Agent is moved
+// into its own cgroup.
+//
+// If there are any processes in the cgroups that are created, we assume there
+// was an error in cleaning up a previous run and an error is returned.
+//
+// Creates cgroups:
+// /<root> Top-level cgroup for the Mesos agent. Has all of the
+// requested subsystems enabled.
Review Comment:
let's only use "controllers" in v2 code
##########
src/slave/main.cpp:
##########
@@ -147,6 +155,144 @@ const char* malloc_conf = "narenas:4";
#ifdef __linux__
+
+#ifdef ENABLE_CGROUPS_V2
+// Log any processes inside of a cgroup.
+static Try<Nothing> checkForProcesses(const string& cgroup)
+{
+ Try<set<pid_t>> processes = cgroups2::processes(cgroup);
+ if (processes.isError()) {
+ return Error(
+ "Failed to check for existing processes in cgroup '" + cgroup + "': "
+ + processes.error());
+ }
+
+ if (!processes->empty()) {
+ vector<string> infos;
+ foreach (pid_t pid, *processes) {
+ Result<os::Process> proc = os::process(pid);
+
+ // Print the command if it's available.
+ if (proc.isSome()) {
+ infos.push_back(stringify(pid) + " '" + proc->command + "'");
+ } else {
+ infos.push_back(stringify(pid));
+ }
+ }
+
+ LOG(INFO) << "Found process(es) in the cgroup '" << cgroup << "'. "
+ << "Consider checking the following process(es) listed in "
+ << path::join("/sys/fs/cgroup", cgroup, "cgroup.procs")
+ << ":\n" << strings::join("\n", infos);
+ }
+
+ return Nothing();
+}
+// Initialize Mesos cgroups for cgroups v2.
+//
+// Ensures that cgroups v2 is available, correctly mounted, and all of the
+// requested controllers are available. If correctly setup, the requested
+// controllers are enabled in the root cgroup, and the Mesos Agent is moved
+// into its own cgroup.
+//
+// If there are any processes in the cgroups that are created, we assume there
+// was an error in cleaning up a previous run and an error is returned.
Review Comment:
looks like this comment isn't accurate? we should probably fix the other
comment that went stale as well as the code evolved to only log the processes
and not bail
--
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]
