Re: Review Request 44974: Added device support in cgroups abstraction.

[Ben Mahler]

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


Fix it, then Ship it!




Thanks! Kevin and I went over this and made some adjustments to the comments 
and error formatting. Will commit shortly.


src/linux/cgroups.hpp (line 625)


s/subsytem/subsystem/ and end with a period here



src/linux/cgroups.hpp (lines 628 - 629)


How about s/ListEntry/Entry/ ? I was a bit confused about whether 
"whitelist" in this context meant that this isn't used for the "deny" file, for 
example.



src/linux/cgroups.hpp (lines 634 - 664)


I'm not sure if we need some of these comments, for example I can tell 
pretty easily from 'Access' that it represents the access permissions and it is 
part of a whitelist entry because it is contained within the ListEntry class. 
Ditto for selector.


- Ben Mahler


On March 21, 2016, 6:06 a.m., Abhishek Dasgupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44974/
> ---
> 
> (Updated March 21, 2016, 6:06 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 
> 
> Diff: https://reviews.apache.org/r/44974/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Abhishek Dasgupta
> 
>

Re: Review Request 44974: Added device support in cgroups abstraction.

[Abhishek Dasgupta]

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

(Updated March 21, 2016, 6:06 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 (updated)
---

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 

Diff: https://reviews.apache.org/r/44974/diff/


Testing
---

make check


Thanks,

Abhishek Dasgupta

Re: Review Request 44974: Added device support in cgroups abstraction.

[Kevin Klues]

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



For reference, Ahishek and I talked offline, and he modified his review based 
off of a branch I pushed out to github:
https://github.com/klueska-mesosphere/mesos/commits/r/a10gupta/cgroups-devices

The old reviews that this review is based on are:
https://reviews.apache.org/r/44439/
https://reviews.apache.org/r/44796/

- Kevin Klues


On March 17, 2016, 7:21 p.m., Abhishek Dasgupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44974/
> ---
> 
> (Updated March 17, 2016, 7:21 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 
> 
> Diff: https://reviews.apache.org/r/44974/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Abhishek Dasgupta
> 
>

Re: Review Request 44974: Added device support in cgroups abstraction.

[Kevin Klues]

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




src/linux/cgroups.hpp (line 645)


Because a selector major/minor value can either bo the '*' character or an 
unsinged integer, I'd probably change my constructors to reflect this. I.e.:

```
Selector(Type type, unsigned int major, unsigned int minor);
Selector(Type type, char minor, unsigned int minor);
Selector(Type type, unsigned int major, char minor);
Selector(Type type, char major, char minor);
```

And then just stringify them under the hood to store them as strings for a 
common representation.



src/linux/cgroups.hpp (lines 660 - 662)


I would probably just call these read/write/mknod instead of adding 
"device" as a prefix.



src/linux/cgroups.hpp (lines 665 - 666)


I see you copied my comment, but changed the name of the class to 
DeviceEntry :)

We should probaly update one or the other.

I am personaly in favor of simply 'Device', but I'll like @bmahler and 
@nnielsen chime in on their preferences.



src/linux/cgroups.hpp (lines 671 - 673)


We typically don't write accessor's like this.

Also, we don't space things out like this.  We use single spaces between 
the different elements.



src/linux/cgroups.hpp (lines 675 - 684)


I know we kept going back and forth about this offline, but in the end, I 
think it makes sense to just make all of this public and non-const. I'd even go 
so far as to say we just make this a struct instead of a class.

The original reasoning behind making this constructor private and the 
members const was to prevent someone from inputting wrong values for the 
major/minor numbers at initialization (because these values can either be the 
'*' character or an unsigned integer).  I think this is actually better handled 
in the Selector() constructor, as mentioned above.

Moreover, even if they have bad values, this will all be caught by errors 
when reading/writing to the `devices.allow`, `devices.deny`, and `devices.list` 
files.



src/linux/cgroups.hpp (lines 687 - 694)


For the use case of the GPU, I also need `operator==` for all of these 
structs.  They are implemented in my github branch I showed you before:


https://github.com/klueska-mesosphere/mesos/commits/r/a10gupta/cgroups-devices



src/linux/cgroups.cpp (lines 2426 - 2441)


There's probably no need for this extra level of indirection through the 
`type` variable.  I would also consider using a `switch` statement since these 
are all enums.

```
  switch (selector.deviceType) {
case DeviceEntry::Selector::ALL:
  stream << "a";
  break;
case DeviceEntry::Selector::BLOCK:
  stream << "b";
  break;
case DeviceEntry::Selector::CHARACTER:
  stream << "c";
  break;
default:
  LOG(ERROR) << "Unable to parse device type: " + selector.deviceType;
  return stream;
  }

  return stream << " "
<< selector.deviceMajor
<< ":"
<< selector.deviceMinor;
```



src/linux/cgroups.cpp (line 2459)


The style guieds require two newlines between functions in a `.cpp` file. 
Could you do a pass to fix this up?



src/linux/cgroups.cpp (lines 2463 - 2465)


You shouldn't have to call `stringify()` around the `selector` and `access` 
fields because you have already provided `operator<<` operators for them.



src/linux/cgroups.cpp (lines 2523 - 2526)


I would probably move this block up to line 2509.  We typically try to 
error out on cases we know are error conditions as early in the code as 
possible.  

In this case, we know we require 3 arguments as soon as we determine that 
the first token is not an "a", so we shoud error out right away.



src/linux/cgroups.cpp (line 2539)


I would probably reverse this to say:

```
if (deviceNumbers[0] != "*" major.isError())
```

The reason being that the only reason we care about the error is because 
the `deviceNumber != "*"`.



src/linux/cgroups.cpp (line 2546)


Same comment as above.

Re: Review Request 44974: Added device support in cgroups abstraction.

[Kevin Klues]

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




src/linux/cgroups.hpp (line 629)


I talked with Ben (who actually has commit rights), and he'd prefer if this 
was just called `ListEntry`. So it becomes `cgroups::devices::ListEntry` when 
it actually gets used.



src/linux/cgroups.hpp (lines 633 - 655)


I talked to Ben (who actually has commit rights), and he wants us to just 
smash all of these structs down into a collection of fields with no 
constructors.  He also suggested using an Option for the major/minor 
numbers i.e.:

```
  struct Selector
  {
enum Type
{
  ALL,
  BLOCK,
  CHARACTER
};

Type type;
Option major;
Option minor;
  };
```

This way we can attempt use the `Option<>` type to determine if the value 
is an integer or not. If `major/minor.isNone()`, we assume `major/minor == 
"*"`. If `major/minor.isSome()`, we use its `dev_t` value via 
`major/minor.get()`.

The `dev_t` type is defined in `linux/types.h`.



src/linux/cgroups.hpp (lines 659 - 668)


Likewise, Ben would like to remove all constructors here.



src/linux/cgroups.hpp (lines 673 - 681)


And no constructors here either.



src/linux/cgroups.cpp (lines 2427 - 2438)


According to the style, `case` entries should be indented by 2 spaces.



src/linux/cgroups.cpp (lines 2499 - 2562)


These will go away.



src/linux/cgroups.cpp (lines 2663 - 2697)


These will go away.


- Kevin Klues


On March 18, 2016, 9:41 a.m., Abhishek Dasgupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44974/
> ---
> 
> (Updated March 18, 2016, 9:41 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 
> 
> Diff: https://reviews.apache.org/r/44974/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Abhishek Dasgupta
> 
>

Re: Review Request 44974: Added device support in cgroups abstraction.

[Abhishek Dasgupta]

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

(Updated March 18, 2016, 9:41 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 

Diff: https://reviews.apache.org/r/44974/diff/


Testing
---

make check


Thanks,

Abhishek Dasgupta

Re: Review Request 44974: Added device support in cgroups abstraction.

[Kevin Klues]

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



I ran the review by Ben again and sat with him to rework it to his 
satisfaction. The commit can be found on this branch:
https://github.com/klueska-mesosphere/mesos/commits/r/a10gupta/cgroups-devices

If you just apply the patch as is, Ben says he will get it committed early next 
week.

- Kevin Klues


On March 18, 2016, 9:41 a.m., Abhishek Dasgupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44974/
> ---
> 
> (Updated March 18, 2016, 9:41 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 
> 
> Diff: https://reviews.apache.org/r/44974/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Abhishek Dasgupta
> 
>