> On Aug. 27, 2015, 7:02 p.m., Jie Yu wrote: > > src/linux/cgroups.cpp, lines 1776-1791 > > <https://reviews.apache.org/r/36620/diff/13/?file=1039336#file1039336line1776> > > > > Could you please introduce a new function under cgroups namespace and > > put this logic there: > > > > ``` > > // Kill all processes in the given cgroup. > > Future<Nothing> cgroups::kill( > > const string& hierarchy, > > const string& cgroup) > > { > > ... > > if (freezerCheckError.isNone()) { > > } else { > > } > > > > return ...; > > } > > ``` > > > > You may want to rename the exsiting `cgroups::kill(hierarchy, cgroup, > > signal)` to `cgroups::signal`. > > Timothy Chen wrote: > Hi jie, thanks for chiming in. This should be ready after the comments > are addressed. I'll try to merge this as well when it's done. I'll ping Joerg > about this. > > Joerg Schad wrote: > Hi Jie, > I am not sure whether I should move the entire logic further into > cgroups::kill. Both path (freezer vs Non-freezer) have distinct logic on how > to destroy the cgroups, which in my understanding makes sense to have in > seperate TasksKiller entities. Secondly cgroups::kill currently is little > more than trying to kill the cgroup, when moving the logic in here > cgroups::kill would have to call cgroups::freeze or call the iterative logic. > Secondly, in my understanding the interface here is the destroy() call. I > actually would like to refactor this piece of code in general, but this is > outside the scope of this patch. If we want to discuss this further -or > misunderstand something here- I would be happy to discuss it further! > > Jie Yu wrote: > Hi Joerg, > > I think you misunderstood what I propose here. I am not asking you to > merge the logics of TasksKiller and FreezerTasksKiller. I think it's great > that you separate these two Processes. What I am sugguesting here is that you > pull the code from 1777-1790 to a separate function (like the following). We > may want to reuse that function in Mesos containerizer launcher in the future > (i.e., not rely on freezer). It also makes the logics in Destroyer cleaner. > Let me know your thoughts! Thank you for taking on this! > > ``` > virtual void initialize() > { > // Stop when no one cares. > promise.future().onDiscard(lambda::bind( > static_cast<void (*)(const UPID&, bool)>(terminate), self(), true)); > > // Kill tasks in the given cgroups in parallel. Use collect mechanism to > // wait until all kill processes finish. > foreach (const string& cgroup, cgroups) { > killers.push_back(cgroups:kill(hierarchy, cgroup)); > } > > collect(killers) > .onAny(defer(self(), &Destroyer::killed, lambda::_1)); > } > > > // Kill all processes in the given cgroup. > Future<Nothing> cgroups::kill( > const string& hierarchy, > const string& cgroup) > { > Future<Nothing> future; > > // Use the freezer subsystem if available. > Option<Error> freezerCheckError = > verify(hierarchy, cgroup, "freezer.state"); > > if (freezerCheckError.isNone()) { > internal::FreezerTasksKiller* killer = > new internal::FreezerTasksKiller(hierarchy, cgroup); > > future = killer->future(); > > spawn(killer, true); > } else { > internal::TasksKiller* killer = > new internal::TasksKiller(hierarchy, cgroup); > > future = killer->future(); > > spawn(killer, true); > } > > return future; > } > ```
As the renaming seems to be controversial (https://reviews.apache.org/r/37894/), I would right now introduce cgroups::killProcesses. Does that work for you? - Joerg ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36620/#review96734 ----------------------------------------------------------- On Aug. 28, 2015, 3:48 p.m., Joerg Schad wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/36620/ > ----------------------------------------------------------- > > (Updated Aug. 28, 2015, 3:48 p.m.) > > > Review request for mesos, Benjamin Hindman, Till Toenshoff, and Timothy Chen. > > > Bugs: MESOS-3086 > https://issues.apache.org/jira/browse/MESOS-3086 > > > Repository: mesos > > > Description > ------- > > Added Non-Freezeer Task Killer. > > > Diffs > ----- > > src/linux/cgroups.cpp 6ef42ed1bc719f334d1ac6e90919a1bc1840d31f > > Diff: https://reviews.apache.org/r/36620/diff/ > > > Testing > ------- > > sudo make check > + manual tests > > > Thanks, > > Joerg Schad > >
