mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Kevin Klues <klue...@gmail.com>
Subject Re: Review Request 44439: Added device support in cgroups abstraction.
Date Sun, 13 Mar 2016 04:28:05 GMT


> On March 9, 2016, 1:58 a.m., Ben Mahler wrote:
> > src/linux/cgroups.hpp, lines 635-654
> > <https://reviews.apache.org/r/44439/diff/2/?file=1282329#file1282329line635>
> >
> >     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?
> 
> Abhishek Dasgupta wrote:
>     So, here you are suggesting to parse deviceEntry like "c 1:3 rm" into say Selector
class and use the Selector object as argument to cgroups::devices::allow. Right?

The idea is to represent that string as two objects (Selector and Access) instead of just
as a string.  When actually writing to the allow and deny files, you can construct the string
from these objects.  When reading from the the list file to perform a check, you will need
to parse the  string and compare it against the Selector / Access objects that are passed
in.


- Kevin


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44439/#review122646
-----------------------------------------------------------


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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message