----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5509/#review9613 -----------------------------------------------------------
src/slave/cgroups_isolation_module.hpp <https://reviews.apache.org/r/5509/#comment20514> No need to make this virtual (it doesn't need to be virtual in ProcessBasedIsolationModule either). src/slave/cgroups_isolation_module.cpp <https://reviews.apache.org/r/5509/#comment20507> s/DEFAULT_HIERARCHY/HIERARCHY/ src/slave/cgroups_isolation_module.cpp <https://reviews.apache.org/r/5509/#comment20509> Add a newline. src/slave/cgroups_isolation_module.cpp <https://reviews.apache.org/r/5509/#comment20508> s/DEFAULT_SUBSYSTEMS/SUBSYSTEMS/ src/slave/cgroups_isolation_module.cpp <https://reviews.apache.org/r/5509/#comment20510> I'd prefer you looped through and checked each individual subsystem. That way, when one isn't enabled the user can very clearly get some insight into which one. I'd also prefer to see a 'std::list<std::string>' instead of a string with commas (which would require updating the cgroups::enabled and cgroups::busy API to take a list of string instead of a string with list semantics). The flip side of this will be being able to change which subsystems to actually use. src/slave/cgroups_isolation_module.cpp <https://reviews.apache.org/r/5509/#comment20511> Ditto comments above. src/slave/cgroups_isolation_module.cpp <https://reviews.apache.org/r/5509/#comment20512> A more informative comment please (i.e., why should they remove the directory?). src/slave/cgroups_isolation_module.cpp <https://reviews.apache.org/r/5509/#comment20505> No need for the helpers, just have a 'subsystems' variable (and see comment above about it being a std::list<std::string>). src/slave/cgroups_isolation_module.cpp <https://reviews.apache.org/r/5509/#comment20506> Ditto above. src/slave/cgroups_isolation_module.cpp <https://reviews.apache.org/r/5509/#comment20515> This should not be a member function, and you should give it a better name. ;) src/slave/cgroups_isolation_module.cpp <https://reviews.apache.org/r/5509/#comment20516> It would be cool to have a map from resource name/type to subsystem. This would give you two things: the ability to change which subsystems you do isolation for on a per slave basis (amazing!) and a nice clean piece of code which loops through the map and performs the writeControl's as necessary. Assuming all the controls are "scalars" this might make it easy to add controls in the future as well (e.g., on the command line via something like --isolate=[disk, disk.subsystem]). src/slave/cgroups_isolation_module.cpp <https://reviews.apache.org/r/5509/#comment20518> s/listen OOM/listen for OOM/ src/slave/cgroups_isolation_module.cpp <https://reviews.apache.org/r/5509/#comment20519> Ditto above. src/slave/cgroups_isolation_module.cpp <https://reviews.apache.org/r/5509/#comment20521> Kill this log message, add another one if future.isReady() is true if you'd like that says something along the lines of "Successfully destroyed cgroup for executor ...". src/slave/cgroups_isolation_module.cpp <https://reviews.apache.org/r/5509/#comment20520> Is this not a fatal issue? Doesn't this mean that the executor/cgroup might keep running forever? - Benjamin Hindman On July 17, 2012, 7:49 p.m., Jie Yu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/5509/ > ----------------------------------------------------------- > > (Updated July 17, 2012, 7:49 p.m.) > > > Review request for mesos, Benjamin Hindman and Vinod Kone. > > > Description > ------- > > Add the cgroups isolation module. > > > Diffs > ----- > > src/Makefile.am 10f1101 > src/examples/balloon_executor.cpp PRE-CREATION > src/examples/balloon_framework.cpp PRE-CREATION > src/slave/cgroups_isolation_module.hpp PRE-CREATION > src/slave/cgroups_isolation_module.cpp PRE-CREATION > src/slave/isolation_module.cpp 5b7b4a2 > src/tests/cgroups_isolation_tests.cpp PRE-CREATION > src/tests/external/CgroupsIsolation/ROOT_CGROUPS_BalloonFramework.sh > PRE-CREATION > > Diff: https://reviews.apache.org/r/5509/diff/ > > > Testing > ------- > > make check (external test, including OOM tests). > > > Thanks, > > Jie Yu > >
