----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44439/#review122646 -----------------------------------------------------------
src/linux/cgroups.hpp (lines 628 - 633) <https://reviews.apache.org/r/44439/#comment184759> Hm.. we probably do not want to add this, because there is already a `cgroups::create` and the caller is free to compose `cgroups::create` along with additional calls to `cgroups::devices::*` as appropriate. src/linux/cgroups.hpp (lines 635 - 654) <https://reviews.apache.org/r/44439/#comment184766> The naming convention in this file is to mirror the cgroups controls, so these would be: ``` cgroups::devices::allow(...) cgroups::devices::deny(...) cgroups::devices::list(...) ``` The other reason we've added these helpers is to provide better types to make the code more readable, notice how the memory controls and cpu controls in this file use Bytes and Duration, respectively. Here, I think we need to do something a bit more involved. To be more specific, if I read the caller code: ``` cgroups::devices::allow( hierarchy, cgroup, "c 1:3 rm"); ``` It's not easy for the reader to understand what `"c 1:3 rm"` means. So adding types here would help, for example: ``` // This is /dev/null. cgroups::device::Selector selector; devices.type = CHARACTER; devices.major = 1; devices.minor = ALL; cgroups::device::Access access; access.read = true; access.write = true; access.mknod = true; Try<Nothing> allow = cgroups::devices::allow( hierarchy, cgroup, selector, access); ``` For cgroups::devices::list, Kevin and I played around and weren't able to see the contents of the file change at all, did you have any luck getting different results out of the devices.list file? src/tests/containerizer/cgroups_tests.cpp (lines 1278 - 1309) <https://reviews.apache.org/r/44439/#comment184760> Does this test pass? Did you make sure this test ran? Note that you have to run the tests under 'sudo' on Linux for this test to run because it has the ROOT and CGROUPS strings in the test name. - Ben Mahler On March 7, 2016, 6:27 a.m., Abhishek Dasgupta wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/44439/ > ----------------------------------------------------------- > > (Updated March 7, 2016, 6:27 a.m.) > > > Review request for mesos, Ben Mahler, Felix Abecassis, Kevin Klues, and > Niklas Nielsen. > > > Bugs: MESOS-3368 > https://issues.apache.org/jira/browse/MESOS-3368 > > > Repository: mesos > > > Description > ------- > > There are some helper methods added to support device cgroups in cgroups > abstraction to aid isolators controlling access to devices. > > > Diffs > ----- > > src/linux/cgroups.hpp 51ccefd1fd2d0989b9bd31342d3d1e3701f88ed2 > src/linux/cgroups.cpp df18ed46a2a96871f67c7eb4233c3b4c27b7aa1c > src/tests/containerizer/cgroups_tests.cpp > acaed9b3f8a04964092cef413133834d0cf5a145 > > Diff: https://reviews.apache.org/r/44439/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Abhishek Dasgupta > >
