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 Mon, 14 Mar 2016 23:40:04 GMT

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



As mentioned in https://reviews.apache.org/r/44476/ this patch should probably be partially
combined with it and then split out into tow pieces:

1) The new API code
2) The tests.

Other comments below.


src/linux/cgroups.hpp (line 34)
<https://reviews.apache.org/r/44439/#comment185766>

    This should go away, given the feedback in the previous patch.



src/linux/cgroups.hpp (lines 629 - 635)
<https://reviews.apache.org/r/44439/#comment185767>

    This function probably shouldn't do the work of checking the list for an entry.  Instead,
it should return a vector of whitelist entries (i.e. `vector<DeviceEntry>`) which the
user can parse as he sees fit. The comment should cahnge to match.



src/linux/cgroups.cpp (lines 2426 - 2427)
<https://reviews.apache.org/r/44439/#comment185773>

    Given the feedback on the previous patch, this should probably take a `DeviceEntry`, not
a `Selector` and `Access` parameter separately.
    
    I realize this is contary to what we suggested previously, but we've had a bit more time
to think about it now, and we think this makes the msot sense.



src/linux/cgroups.cpp (lines 2429 - 2437)
<https://reviews.apache.org/r/44439/#comment185772>

    As per the comment above, we should probably jsut return a vector here.  Let the code
outside of this decide what to do with the list.



src/linux/cgroups.cpp (lines 2443 - 2444)
<https://reviews.apache.org/r/44439/#comment185768>

    Same comment as above.



src/linux/cgroups.cpp (lines 2450 - 2451)
<https://reviews.apache.org/r/44439/#comment185770>

    We should probably put a blank line between thses two statements for consistency with
other code.



src/linux/cgroups.cpp (lines 2461 - 2462)
<https://reviews.apache.org/r/44439/#comment185769>

    Same comment as above.



src/linux/cgroups.cpp (lines 2468 - 2469)
<https://reviews.apache.org/r/44439/#comment185771>

    Same comment as above.



src/tests/containerizer/cgroups_tests.cpp (line 1284)
<https://reviews.apache.org/r/44439/#comment185782>

    Can you put a comment here about why we need to deny all by default for this to work (I
know why this is necessary, but it may not be clear for others trying to walk through all
the cases)?
    
    For example, without first denying all, the following fails:
    
    * write allow all
    * check /dev/console
      FAIL
    
    This will fail because of the way the devices.list file works. It only actually prints
out entries **explicitly** written to it previously.
    
    This means that writing "allow all" followed by "check /dev/null" will always fail, even
though we obviously have access.
    
    Unfortunately, this is just the nature of the interface and due "whitelisting" instead
of "blacklisting".



src/tests/containerizer/cgroups_tests.cpp (lines 1286 - 1287)
<https://reviews.apache.org/r/44439/#comment185778>

    Update throughout with `DeviceEntry`



src/tests/containerizer/cgroups_tests.cpp (line 1287)
<https://reviews.apache.org/r/44439/#comment185780>

    I would probably not use any sort of default constructor for either `Selector` or `Access`.
 It is much more readable to see the actual parameters passed.



src/tests/containerizer/cgroups_tests.cpp (lines 1289 - 1293)
<https://reviews.apache.org/r/44439/#comment185779>

    This test will probably have to change given the new semantics of `devices::list()`


- Kevin Klues


On March 14, 2016, 6:25 p.m., Abhishek Dasgupta wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44439/
> -----------------------------------------------------------
> 
> (Updated March 14, 2016, 6:25 p.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