You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Abhishek Dasgupta <a1...@linux.vnet.ibm.com> on 2016/03/07 07:22:58 UTC
Review Request 44439: Added device support in cgroups abstraction.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44439/
-----------------------------------------------------------
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
Re: Review Request 44439: Added device support in cgroups abstraction.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44439/#review123519
-----------------------------------------------------------
Bad patch!
Reviews applied: [44439, 44796, 44797]
Failed command: ./support/apply-review.sh -n -r 44796
Error:
2016-03-14 22:32:01 URL:https://reviews.apache.org/r/44796/diff/raw/ [8734/8734] -> "44796.patch" [1]
error: patch failed: 3rdparty/libprocess/3rdparty/stout/include/Makefile.am:21
error: 3rdparty/libprocess/3rdparty/stout/include/Makefile.am: patch does not apply
Full log: https://builds.apache.org/job/mesos-reviewbot/11998/console
- Mesos ReviewBot
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
>
>
Re: Review Request 44439: Added device support in cgroups abstraction.
Posted by Kevin Klues <kl...@gmail.com>.
-----------------------------------------------------------
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
>
>
Re: Review Request 44439: Added device support in cgroups abstraction.
Posted by Abhishek Dasgupta <a1...@linux.vnet.ibm.com>.
-----------------------------------------------------------
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 (updated)
-----
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
Re: Review Request 44439: Added device support in cgroups abstraction.
Posted by Kevin Klues <kl...@gmail.com>.
> 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
>
>
Re: Review Request 44439: Added device support in cgroups abstraction.
Posted by Abhishek Dasgupta <a1...@linux.vnet.ibm.com>.
> 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?
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?
- Abhishek
-----------------------------------------------------------
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
>
>
Re: Review Request 44439: Added device support in cgroups abstraction.
Posted by Abhishek Dasgupta <a1...@linux.vnet.ibm.com>.
> On March 9, 2016, 1:58 a.m., Ben Mahler wrote:
> > src/tests/containerizer/cgroups_tests.cpp, lines 1278-1309
> > <https://reviews.apache.org/r/44439/diff/2/?file=1282331#file1282331line1278>
> >
> > 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.
Hi,
I actually ran these tests. And here is the output:
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from CgroupsAnyHierarchyDevicesTest
[ RUN ] CgroupsAnyHierarchyDevicesTest.ROOT_CGROUPS_Devices
[ OK ] CgroupsAnyHierarchyDevicesTest.ROOT_CGROUPS_Devices (10 ms)
[----------] 1 test from CgroupsAnyHierarchyDevicesTest (10 ms total)
[----------] Global test environment tear-down
[==========] 1 test from 1 test case ran. (31 ms total)
[ PASSED ] 1 test.
make[3]: Leaving directory '/home/abhishek/test/mesos/build/src'
make[2]: Leaving directory '/home/abhishek/test/mesos/build/src'
devices.list can't be changed by writing on it. It is to be changed by writing in devices.deny or device.allow. I followed this logic developing the code. Please, contradict me if I am wrong in any assumption/deduction.
- Abhishek
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44439/#review122646
-----------------------------------------------------------
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
>
>
Re: Review Request 44439: Added device support in cgroups abstraction.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
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
>
>
Re: Review Request 44439: Added device support in cgroups abstraction.
Posted by Abhishek Dasgupta <a1...@linux.vnet.ibm.com>.
-----------------------------------------------------------
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 (updated)
-----
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