bmahler commented on code in PR #571:
URL: https://github.com/apache/mesos/pull/571#discussion_r1578197877
##########
src/linux/cgroups2.cpp:
##########
@@ -381,41 +381,66 @@ Try<Nothing> kill(const std::string& cgroup)
}
-Try<Nothing> destroy(const string& cgroup)
+Future<Nothing> destroy(const string& cgroup)
{
if (!cgroups2::exists(cgroup)) {
- return Error("Cgroup '" + cgroup + "' does not exist");
+ return Failure("Cgroup '" + cgroup + "' does not exist");
}
// To destroy a subtree of cgroups we first kill all of the processes inside
// of the cgroup and then remove all of the cgroup directories, removing
// the most deeply nested directories first.
+
Try<Nothing> kill = cgroups2::kill(cgroup);
if (kill.isError()) {
- return Error("Failed to kill processes in cgroup: " + kill.error());
+ return Failure("Failed to kill processes in cgroup: " + kill.error());
}
- Try<set<string>> cgroups = cgroups2::get(cgroup);
- if (cgroups.isError()) {
- return Error("Failed to get nested cgroups: " + cgroups.error());
- }
+ // Wait until all of the processes have been killed.
+ int retries = 50;
+ Future<Nothing> processes = loop(
+ []() { return process::after(Milliseconds(10)); },
Review Comment:
can you reduce this to 1 millisecond?
##########
src/linux/cgroups2.cpp:
##########
@@ -381,41 +381,66 @@ Try<Nothing> kill(const std::string& cgroup)
}
-Try<Nothing> destroy(const string& cgroup)
+Future<Nothing> destroy(const string& cgroup)
{
if (!cgroups2::exists(cgroup)) {
- return Error("Cgroup '" + cgroup + "' does not exist");
+ return Failure("Cgroup '" + cgroup + "' does not exist");
}
// To destroy a subtree of cgroups we first kill all of the processes inside
// of the cgroup and then remove all of the cgroup directories, removing
// the most deeply nested directories first.
+
Try<Nothing> kill = cgroups2::kill(cgroup);
if (kill.isError()) {
- return Error("Failed to kill processes in cgroup: " + kill.error());
+ return Failure("Failed to kill processes in cgroup: " + kill.error());
}
- Try<set<string>> cgroups = cgroups2::get(cgroup);
- if (cgroups.isError()) {
- return Error("Failed to get nested cgroups: " + cgroups.error());
- }
+ // Wait until all of the processes have been killed.
+ int retries = 50;
+ Future<Nothing> processes = loop(
+ []() { return process::after(Milliseconds(10)); },
+ [=](const Nothing&) mutable -> Future<ControlFlow<Nothing>> {
+ Try<set<pid_t>> pids = cgroups2::processes(cgroup, true);
+ if (pids.isError()) {
+ return Failure("Failed to fetch pids in cgroup: " + pids.error());
+ }
- vector<string> sorted(
- std::make_move_iterator(cgroups->begin()),
- std::make_move_iterator(cgroups->end()));
- sorted.push_back(cgroup);
- std::sort(sorted.rbegin(), sorted.rend());
+ if (pids->size() == 0) {
+ return Break();
+ }
- foreach (const string& cgroup, sorted) {
- const string path = cgroups2::path(cgroup);
- Try<Nothing> rmdir = os::rmdir(path, false);
- if (rmdir.isError()) {
- return Error(
- "Failed to remove directory '" + path + "': " + rmdir.error());
+ --retries;
+ if (retries == 0) {
+ return Failure("Failed to kill all processes in cgroup");
+ }
Review Comment:
ditto here, maybe this should just Break?
##########
src/linux/cgroups2.cpp:
##########
@@ -381,41 +381,66 @@ Try<Nothing> kill(const std::string& cgroup)
}
-Try<Nothing> destroy(const string& cgroup)
+Future<Nothing> destroy(const string& cgroup)
{
if (!cgroups2::exists(cgroup)) {
- return Error("Cgroup '" + cgroup + "' does not exist");
+ return Failure("Cgroup '" + cgroup + "' does not exist");
}
// To destroy a subtree of cgroups we first kill all of the processes inside
// of the cgroup and then remove all of the cgroup directories, removing
// the most deeply nested directories first.
+
Try<Nothing> kill = cgroups2::kill(cgroup);
if (kill.isError()) {
- return Error("Failed to kill processes in cgroup: " + kill.error());
+ return Failure("Failed to kill processes in cgroup: " + kill.error());
}
- Try<set<string>> cgroups = cgroups2::get(cgroup);
- if (cgroups.isError()) {
- return Error("Failed to get nested cgroups: " + cgroups.error());
- }
+ // Wait until all of the processes have been killed.
+ int retries = 50;
+ Future<Nothing> processes = loop(
+ []() { return process::after(Milliseconds(10)); },
+ [=](const Nothing&) mutable -> Future<ControlFlow<Nothing>> {
+ Try<set<pid_t>> pids = cgroups2::processes(cgroup, true);
+ if (pids.isError()) {
+ return Failure("Failed to fetch pids in cgroup: " + pids.error());
+ }
- vector<string> sorted(
- std::make_move_iterator(cgroups->begin()),
- std::make_move_iterator(cgroups->end()));
- sorted.push_back(cgroup);
- std::sort(sorted.rbegin(), sorted.rend());
+ if (pids->size() == 0) {
+ return Break();
+ }
- foreach (const string& cgroup, sorted) {
- const string path = cgroups2::path(cgroup);
- Try<Nothing> rmdir = os::rmdir(path, false);
- if (rmdir.isError()) {
- return Error(
- "Failed to remove directory '" + path + "': " + rmdir.error());
+ --retries;
+ if (retries == 0) {
+ return Failure("Failed to kill all processes in cgroup");
+ }
+
+ return Continue();
+ });
+
+ return processes.then([=]() -> Future<Nothing> {
+ Try<set<string>> cgroups = cgroups2::get(cgroup);
+ if (cgroups.isError()) {
+ return Failure("Failed to get nested cgroups: " + cgroups.error());
}
- }
- return Nothing();
+ vector<string> sorted(
+ std::make_move_iterator(cgroups->begin()),
+ std::make_move_iterator(cgroups->end()));
+ sorted.push_back(cgroup);
+ std::sort(sorted.rbegin(), sorted.rend());
+
+ foreach (const string& cgroup, sorted) {
+ const string path = cgroups2::path(cgroup);
+ Try<Nothing> rmdir = os::rmdir(path, false);
Review Comment:
hm.. ideally we ignore ENOENT here, but looking at os::rmdir, it doesn't care
might be worth just using ::rmdir directly here and looking at ENOENT, since
os::rmdir is mainly providing recursion support and cross OS support, which we
don't care about here
##########
src/linux/cgroups2.cpp:
##########
@@ -381,41 +381,66 @@ Try<Nothing> kill(const std::string& cgroup)
}
-Try<Nothing> destroy(const string& cgroup)
+Future<Nothing> destroy(const string& cgroup)
{
if (!cgroups2::exists(cgroup)) {
- return Error("Cgroup '" + cgroup + "' does not exist");
+ return Failure("Cgroup '" + cgroup + "' does not exist");
}
// To destroy a subtree of cgroups we first kill all of the processes inside
// of the cgroup and then remove all of the cgroup directories, removing
// the most deeply nested directories first.
+
Try<Nothing> kill = cgroups2::kill(cgroup);
if (kill.isError()) {
- return Error("Failed to kill processes in cgroup: " + kill.error());
+ return Failure("Failed to kill processes in cgroup: " + kill.error());
}
- Try<set<string>> cgroups = cgroups2::get(cgroup);
- if (cgroups.isError()) {
- return Error("Failed to get nested cgroups: " + cgroups.error());
- }
+ // Wait until all of the processes have been killed.
+ int retries = 50;
+ Future<Nothing> processes = loop(
+ []() { return process::after(Milliseconds(10)); },
+ [=](const Nothing&) mutable -> Future<ControlFlow<Nothing>> {
+ Try<set<pid_t>> pids = cgroups2::processes(cgroup, true);
+ if (pids.isError()) {
+ return Failure("Failed to fetch pids in cgroup: " + pids.error());
+ }
Review Comment:
maybe this should just Break? or if this returns Failure, then we likely
want an onAny rather than a .then below, which will complicate things
##########
src/linux/cgroups2.cpp:
##########
@@ -381,41 +381,66 @@ Try<Nothing> kill(const std::string& cgroup)
}
-Try<Nothing> destroy(const string& cgroup)
+Future<Nothing> destroy(const string& cgroup)
{
if (!cgroups2::exists(cgroup)) {
- return Error("Cgroup '" + cgroup + "' does not exist");
+ return Failure("Cgroup '" + cgroup + "' does not exist");
}
// To destroy a subtree of cgroups we first kill all of the processes inside
// of the cgroup and then remove all of the cgroup directories, removing
// the most deeply nested directories first.
+
Try<Nothing> kill = cgroups2::kill(cgroup);
if (kill.isError()) {
- return Error("Failed to kill processes in cgroup: " + kill.error());
+ return Failure("Failed to kill processes in cgroup: " + kill.error());
}
- Try<set<string>> cgroups = cgroups2::get(cgroup);
- if (cgroups.isError()) {
- return Error("Failed to get nested cgroups: " + cgroups.error());
- }
+ // Wait until all of the processes have been killed.
+ int retries = 50;
+ Future<Nothing> processes = loop(
+ []() { return process::after(Milliseconds(10)); },
+ [=](const Nothing&) mutable -> Future<ControlFlow<Nothing>> {
+ Try<set<pid_t>> pids = cgroups2::processes(cgroup, true);
+ if (pids.isError()) {
+ return Failure("Failed to fetch pids in cgroup: " + pids.error());
+ }
- vector<string> sorted(
- std::make_move_iterator(cgroups->begin()),
- std::make_move_iterator(cgroups->end()));
- sorted.push_back(cgroup);
- std::sort(sorted.rbegin(), sorted.rend());
+ if (pids->size() == 0) {
+ return Break();
+ }
Review Comment:
`pids->empty()`
--
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]
