Re: Review Request 46158: Completed implementation of the cgroups unified isolator.

[haosdent huang]

> On June 12, 2016, 2:51 p.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp, lines 351-363
> > 
> >
> > Why do we prepare hierarchy here? I think preparing hierarchy for a 
> > specific container is a low-level work which should be handled in 
> > `Subsystem::prepare()` method rather than here. And in that way, we only 
> > need one `for` loop (the following one) rather than 2 here.
> 
> haosdent huang wrote:
> We delegate the create/destory operations of hierarchy to 
> `CgroupsIsolatorProcess`. Because if we do the create/destroy operations of 
> hierarchy in `Subsystem::prepare()` and `Subsystem::cleanup()`, it would 
> create the hierarchy or destroy the hierarchy multiple times when differnt 
> subsystems mount in same hierarchies.
> 
> Qian Zhang wrote:
> My idea is, here we should call each subsystem's `prepare()` (the second 
> `for` loop in this method), and each subsystem's `prepare()` should call its 
> parent class's `prepare()` method (i.e., `Subsystem::prepare()`), and in 
> `Subsystem::prepare()`, we call the virtual method `name()` first to know 
> which subsystem that we are working on, and then ONLY create cgroup for that 
> subsytem. In this way, we keep the common cgroup operations (e.g., create 
> cgroup) only in parent class (e.g., `Subsystem::prepare()`), and we can still 
> do some subsystem specific works in child class (e.g., 
> `MemorySubsystem::prepare()`).
> 
> Currently, `Subsystem::prepare()`, `Subsystem::isolate()` and 
> `Subsystem::cleanup()` are just no-op, I think we should fully utilize them.
> 
> haosdent huang wrote:
> Yes, but it would create/destroy the hierarchy multiple times if we put 
> these operations to `Subsystem`.
> 
> Qian Zhang wrote:
> What did you mean about "create/destroy the hierachy"? Did you mean 
> creating `/sys/fs/cgroup//mesos`? If yes, I think it is a one-time 
> operation which will be only done in `CgroupsIsolatorProcess::create()`, we 
> do not need to call any methods of `Subsystem` at all.
> 
> haosdent huang wrote:
> Oh, not. `/sys/fs/cgroup//mesos` is the root hierachy. Suppose 
> we mount both `cpu` and `cpuacct` into `/sys/fs/cgroup/cpu,cpuacct/mesos`.
> If we delegate the create operation to `Subsystem`, then we would create 
> `/sys/fs/cgroup/cpu,cpuacct/mesos/${container_id}` when 
> `CpuSubsystem::prepare()` and
> `CpuacctSubsystem::prepare()`. This would bring problems.
> 
> Qian Zhang wrote:
> Understood. Then I think we should not **always** create these two 
> subsystems: `CpuSubsystem` and `CpuacctSubsystem`, instead, we should only 
> have one CPU subsystem class `CpuSubsystem` which handles both `cpu` and 
> `cpuacct` subsystems, just like what the original `cgroups/cpu` isolator 
> does. And in the `Subsystem` class, we should have a vector field 
> `subsystems`, for `MemorySubsystem`/`NetClsSubsystem`/`PerfEventSubsystem`, 
> this vector has only 1 element (i.e., `memory`/`net_cls`/`perf_event`), but 
> for `CpuSubsystem`, if cpu and cpuacct are co-mounted at 
> `/sys/fs/cgroup/cpu,cpuacct`, its `subsystems` vector will have 1 elements, 
> and if cpu and cpuacct are mounted separately, it will have 2 elements. And 
> then in `Subsystem::prepare()`, it should go through each element of 
> `subsystems` vector in a `for` loop, and create cgroup accordingly.
> 
> And in the patch https://reviews.apache.org/r/45086/ , I am thinking 
> maybe we should not expose `cgroups/cpuacct` in the flag `--isolation`, how 
> `cpu` and `cpuacct` subsystems are handled should be an internal thing which 
> should be inside of `CpuSubsystem`. That means for user, they should always 
> specify `cgroups/cpu` as before and should not be aware of `cgroups/cpuacct`, 
> and inside of `CpuSubsystem`, we can create 1 subsystem or 2 subsystems based 
> on whether cpu and cpuacct are co-mounted or separately mounted. This should 
> also be a good way to keep the backward compatibility since there is no any 
> changes to the existing user experience.

Hi, @qianzhang We could not do this:

>we should only have one CPU subsystem class CpuSubsystem which handles both 
>cpu and cpuacct subsystems

Because the main purpose of this ticket is try to resolve this problem. We 
manage two different subsystems in `CgroupsCpushareIsolatorProcess` is because 
we could not manage co-mounted subsystems in different isolators before. And 
this bring some tricky problems. So we try to solve this problem and create 
this epic. As you see, not only `cpu` and `cpuacct` could co-mounted under same 
hierarchy, we could mount `mem`, `perf_event` on `/sys/fs/cgroup/cpu,cpuacct` 
as well. And in cgroup-v2, all subsystems would be co-mounted under only one 
hierarchy, so we try to refactor all current cgroups isolators into one unified 
isolator. And make it fits the co-mounted subsystems case better.


- haosdent

Re: Review Request 46158: Completed implementation of the cgroups unified isolator.

[Qian Zhang]

> On June 12, 2016, 10:51 p.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp, lines 351-363
> > 
> >
> > Why do we prepare hierarchy here? I think preparing hierarchy for a 
> > specific container is a low-level work which should be handled in 
> > `Subsystem::prepare()` method rather than here. And in that way, we only 
> > need one `for` loop (the following one) rather than 2 here.
> 
> haosdent huang wrote:
> We delegate the create/destory operations of hierarchy to 
> `CgroupsIsolatorProcess`. Because if we do the create/destroy operations of 
> hierarchy in `Subsystem::prepare()` and `Subsystem::cleanup()`, it would 
> create the hierarchy or destroy the hierarchy multiple times when differnt 
> subsystems mount in same hierarchies.
> 
> Qian Zhang wrote:
> My idea is, here we should call each subsystem's `prepare()` (the second 
> `for` loop in this method), and each subsystem's `prepare()` should call its 
> parent class's `prepare()` method (i.e., `Subsystem::prepare()`), and in 
> `Subsystem::prepare()`, we call the virtual method `name()` first to know 
> which subsystem that we are working on, and then ONLY create cgroup for that 
> subsytem. In this way, we keep the common cgroup operations (e.g., create 
> cgroup) only in parent class (e.g., `Subsystem::prepare()`), and we can still 
> do some subsystem specific works in child class (e.g., 
> `MemorySubsystem::prepare()`).
> 
> Currently, `Subsystem::prepare()`, `Subsystem::isolate()` and 
> `Subsystem::cleanup()` are just no-op, I think we should fully utilize them.
> 
> haosdent huang wrote:
> Yes, but it would create/destroy the hierarchy multiple times if we put 
> these operations to `Subsystem`.
> 
> Qian Zhang wrote:
> What did you mean about "create/destroy the hierachy"? Did you mean 
> creating `/sys/fs/cgroup//mesos`? If yes, I think it is a one-time 
> operation which will be only done in `CgroupsIsolatorProcess::create()`, we 
> do not need to call any methods of `Subsystem` at all.
> 
> haosdent huang wrote:
> Oh, not. `/sys/fs/cgroup//mesos` is the root hierachy. Suppose 
> we mount both `cpu` and `cpuacct` into `/sys/fs/cgroup/cpu,cpuacct/mesos`.
> If we delegate the create operation to `Subsystem`, then we would create 
> `/sys/fs/cgroup/cpu,cpuacct/mesos/${container_id}` when 
> `CpuSubsystem::prepare()` and
> `CpuacctSubsystem::prepare()`. This would bring problems.

Understood. Then I think we should not **always** create these two subsystems: 
`CpuSubsystem` and `CpuacctSubsystem`, instead, we should only have one CPU 
subsystem class `CpuSubsystem` which handles both `cpu` and `cpuacct` 
subsystems, just like what the original `cgroups/cpu` isolator does. And in the 
`Subsystem` class, we should have a vector field `subsystems`, for 
`MemorySubsystem`/`NetClsSubsystem`/`PerfEventSubsystem`, this vector has only 
1 element (i.e., `memory`/`net_cls`/`perf_event`), but for `CpuSubsystem`, if 
cpu and cpuacct are co-mounted at `/sys/fs/cgroup/cpu,cpuacct`, its 
`subsystems` vector will have 1 elements, and if cpu and cpuacct are mounted 
separately, it will have 2 elements. And then in `Subsystem::prepare()`, it 
should go through each element of `subsystems` vector in a `for` loop, and 
create cgroup accordingly.

And in the patch https://reviews.apache.org/r/45086/ , I am thinking maybe we 
should not expose `cgroups/cpuacct` in the flag `--isolation`, how `cpu` and 
`cpuacct` subsystems are handled should be an internal thing which should be 
inside of `CpuSubsystem`. That means for user, they should always specify 
`cgroups/cpu` as before and should not be aware of `cgroups/cpuacct`, and 
inside of `CpuSubsystem`, we can create 1 subsystem or 2 subsystems based on 
whether cpu and cpuacct are co-mounted or separately mounted. This should also 
be a good way to keep the backward compatibility since there is no any changes 
to the existing user experience.


- Qian


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


On June 19, 2016, 11:55 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46158/
> ---
> 
> (Updated June 19, 2016, 11:55 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Guangya Liu, Ian Downes, Jie Yu, 
> Kevin Klues, and Qian Zhang.
> 
> 
> Bugs: MESOS-5041
> https://issues.apache.org/jira/browse/MESOS-5041
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Completed implementation of the cgroups unified isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/

Re: Review Request 46158: Completed implementation of the cgroups unified isolator.

[haosdent huang]

> On June 12, 2016, 2:51 p.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp, lines 351-363
> > 
> >
> > Why do we prepare hierarchy here? I think preparing hierarchy for a 
> > specific container is a low-level work which should be handled in 
> > `Subsystem::prepare()` method rather than here. And in that way, we only 
> > need one `for` loop (the following one) rather than 2 here.
> 
> haosdent huang wrote:
> We delegate the create/destory operations of hierarchy to 
> `CgroupsIsolatorProcess`. Because if we do the create/destroy operations of 
> hierarchy in `Subsystem::prepare()` and `Subsystem::cleanup()`, it would 
> create the hierarchy or destroy the hierarchy multiple times when differnt 
> subsystems mount in same hierarchies.
> 
> Qian Zhang wrote:
> My idea is, here we should call each subsystem's `prepare()` (the second 
> `for` loop in this method), and each subsystem's `prepare()` should call its 
> parent class's `prepare()` method (i.e., `Subsystem::prepare()`), and in 
> `Subsystem::prepare()`, we call the virtual method `name()` first to know 
> which subsystem that we are working on, and then ONLY create cgroup for that 
> subsytem. In this way, we keep the common cgroup operations (e.g., create 
> cgroup) only in parent class (e.g., `Subsystem::prepare()`), and we can still 
> do some subsystem specific works in child class (e.g., 
> `MemorySubsystem::prepare()`).
> 
> Currently, `Subsystem::prepare()`, `Subsystem::isolate()` and 
> `Subsystem::cleanup()` are just no-op, I think we should fully utilize them.
> 
> haosdent huang wrote:
> Yes, but it would create/destroy the hierarchy multiple times if we put 
> these operations to `Subsystem`.
> 
> Qian Zhang wrote:
> What did you mean about "create/destroy the hierachy"? Did you mean 
> creating `/sys/fs/cgroup//mesos`? If yes, I think it is a one-time 
> operation which will be only done in `CgroupsIsolatorProcess::create()`, we 
> do not need to call any methods of `Subsystem` at all.

Oh, not. `/sys/fs/cgroup//mesos` is the root hierachy. Suppose we 
mount both `cpu` and `cpuacct` into `/sys/fs/cgroup/cpu,cpuacct/mesos`.
If we delegate the create operation to `Subsystem`, then we would create 
`/sys/fs/cgroup/cpu,cpuacct/mesos/${container_id}` when 
`CpuSubsystem::prepare()` and
`CpuacctSubsystem::prepare()`. This would bring problems.


- haosdent


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


On June 19, 2016, 3:55 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46158/
> ---
> 
> (Updated June 19, 2016, 3:55 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Guangya Liu, Ian Downes, Jie Yu, 
> Kevin Klues, and Qian Zhang.
> 
> 
> Bugs: MESOS-5041
> https://issues.apache.org/jira/browse/MESOS-5041
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Completed implementation of the cgroups unified isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46158/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 46158: Completed implementation of the cgroups unified isolator.

[Qian Zhang]

> On June 12, 2016, 10:51 p.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp, lines 351-363
> > 
> >
> > Why do we prepare hierarchy here? I think preparing hierarchy for a 
> > specific container is a low-level work which should be handled in 
> > `Subsystem::prepare()` method rather than here. And in that way, we only 
> > need one `for` loop (the following one) rather than 2 here.
> 
> haosdent huang wrote:
> We delegate the create/destory operations of hierarchy to 
> `CgroupsIsolatorProcess`. Because if we do the create/destroy operations of 
> hierarchy in `Subsystem::prepare()` and `Subsystem::cleanup()`, it would 
> create the hierarchy or destroy the hierarchy multiple times when differnt 
> subsystems mount in same hierarchies.
> 
> Qian Zhang wrote:
> My idea is, here we should call each subsystem's `prepare()` (the second 
> `for` loop in this method), and each subsystem's `prepare()` should call its 
> parent class's `prepare()` method (i.e., `Subsystem::prepare()`), and in 
> `Subsystem::prepare()`, we call the virtual method `name()` first to know 
> which subsystem that we are working on, and then ONLY create cgroup for that 
> subsytem. In this way, we keep the common cgroup operations (e.g., create 
> cgroup) only in parent class (e.g., `Subsystem::prepare()`), and we can still 
> do some subsystem specific works in child class (e.g., 
> `MemorySubsystem::prepare()`).
> 
> Currently, `Subsystem::prepare()`, `Subsystem::isolate()` and 
> `Subsystem::cleanup()` are just no-op, I think we should fully utilize them.
> 
> haosdent huang wrote:
> Yes, but it would create/destroy the hierarchy multiple times if we put 
> these operations to `Subsystem`.

What did you mean about "create/destroy the hierachy"? Did you mean creating 
`/sys/fs/cgroup//mesos`? If yes, I think it is a one-time operation 
which will be only done in `CgroupsIsolatorProcess::create()`, we do not need 
to call any methods of `Subsystem` at all.


- Qian


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


On June 19, 2016, 11:55 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46158/
> ---
> 
> (Updated June 19, 2016, 11:55 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Guangya Liu, Ian Downes, Jie Yu, 
> Kevin Klues, and Qian Zhang.
> 
> 
> Bugs: MESOS-5041
> https://issues.apache.org/jira/browse/MESOS-5041
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Completed implementation of the cgroups unified isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46158/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 46158: Completed implementation of the cgroups unified isolator.

[haosdent huang]

> On June 12, 2016, 2:51 p.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp, lines 351-363
> > 
> >
> > Why do we prepare hierarchy here? I think preparing hierarchy for a 
> > specific container is a low-level work which should be handled in 
> > `Subsystem::prepare()` method rather than here. And in that way, we only 
> > need one `for` loop (the following one) rather than 2 here.
> 
> haosdent huang wrote:
> We delegate the create/destory operations of hierarchy to 
> `CgroupsIsolatorProcess`. Because if we do the create/destroy operations of 
> hierarchy in `Subsystem::prepare()` and `Subsystem::cleanup()`, it would 
> create the hierarchy or destroy the hierarchy multiple times when differnt 
> subsystems mount in same hierarchies.
> 
> Qian Zhang wrote:
> My idea is, here we should call each subsystem's `prepare()` (the second 
> `for` loop in this method), and each subsystem's `prepare()` should call its 
> parent class's `prepare()` method (i.e., `Subsystem::prepare()`), and in 
> `Subsystem::prepare()`, we call the virtual method `name()` first to know 
> which subsystem that we are working on, and then ONLY create cgroup for that 
> subsytem. In this way, we keep the common cgroup operations (e.g., create 
> cgroup) only in parent class (e.g., `Subsystem::prepare()`), and we can still 
> do some subsystem specific works in child class (e.g., 
> `MemorySubsystem::prepare()`).
> 
> Currently, `Subsystem::prepare()`, `Subsystem::isolate()` and 
> `Subsystem::cleanup()` are just no-op, I think we should fully utilize them.

Yes, but it would create/destroy the hierarchy multiple times if we put these 
operations to `Subsystem`.


- haosdent


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


On June 19, 2016, 3:55 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46158/
> ---
> 
> (Updated June 19, 2016, 3:55 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Guangya Liu, Ian Downes, Jie Yu, 
> Kevin Klues, and Qian Zhang.
> 
> 
> Bugs: MESOS-5041
> https://issues.apache.org/jira/browse/MESOS-5041
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Completed implementation of the cgroups unified isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46158/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 46158: Completed implementation of the cgroups unified isolator.

[Qian Zhang]

> On June 12, 2016, 10:51 p.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp, lines 351-363
> > 
> >
> > Why do we prepare hierarchy here? I think preparing hierarchy for a 
> > specific container is a low-level work which should be handled in 
> > `Subsystem::prepare()` method rather than here. And in that way, we only 
> > need one `for` loop (the following one) rather than 2 here.
> 
> haosdent huang wrote:
> We delegate the create/destory operations of hierarchy to 
> `CgroupsIsolatorProcess`. Because if we do the create/destroy operations of 
> hierarchy in `Subsystem::prepare()` and `Subsystem::cleanup()`, it would 
> create the hierarchy or destroy the hierarchy multiple times when differnt 
> subsystems mount in same hierarchies.

My idea is, here we should call each subsystem's `prepare()` (the second `for` 
loop in this method), and each subsystem's `prepare()` should call its parent 
class's `prepare()` method (i.e., `Subsystem::prepare()`), and in 
`Subsystem::prepare()`, we call the virtual method `name()` first to know which 
subsystem that we are working on, and then ONLY create cgroup for that 
subsytem. In this way, we keep the common cgroup operations (e.g., create 
cgroup) only in parent class (e.g., `Subsystem::prepare()`), and we can still 
do some subsystem specific works in child class (e.g., 
`MemorySubsystem::prepare()`).

Currently, `Subsystem::prepare()`, `Subsystem::isolate()` and 
`Subsystem::cleanup()` are just no-op, I think we should fully utilize them.


- Qian


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


On June 19, 2016, 11:55 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46158/
> ---
> 
> (Updated June 19, 2016, 11:55 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Guangya Liu, Ian Downes, Jie Yu, 
> Kevin Klues, and Qian Zhang.
> 
> 
> Bugs: MESOS-5041
> https://issues.apache.org/jira/browse/MESOS-5041
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Completed implementation of the cgroups unified isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46158/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 46158: Completed implementation of the cgroups unified isolator.

[haosdent huang]

> On June 11, 2016, 2:31 p.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp, line 141
> > 
> >
> > I would suggest to rename this method to `prepareHierarchy()`,  the 
> > word `get` usually means this is a read-only method, but I think this is 
> > not a read-only method, we may do some write operations, e.g., mount 
> > subsystem, create root cgroup.
> > 
> > And I know there is already a method in this class named 
> > `prepareHierarchy()`, I would suggest to rename it to `createCgroup()` 
> > because the intent of that method is to create cgroup for a specific 
> > container, right?

Make sense!


- haosdent


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


On June 19, 2016, 3:55 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46158/
> ---
> 
> (Updated June 19, 2016, 3:55 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Guangya Liu, Ian Downes, Jie Yu, 
> Kevin Klues, and Qian Zhang.
> 
> 
> Bugs: MESOS-5041
> https://issues.apache.org/jira/browse/MESOS-5041
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Completed implementation of the cgroups unified isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46158/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 46158: Completed implementation of the cgroups unified isolator.

[haosdent huang]

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

(Updated June 19, 2016, 3:55 p.m.)


Review request for mesos, Gilbert Song, Guangya Liu, Ian Downes, Jie Yu, Kevin 
Klues, and Qian Zhang.


Changes
---

Address @qianzhang's comments.


Bugs: MESOS-5041
https://issues.apache.org/jira/browse/MESOS-5041


Repository: mesos


Description
---

Completed implementation of the cgroups unified isolator.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp PRE-CREATION 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 46158: Completed implementation of the cgroups unified isolator.

[haosdent huang]

> On June 15, 2016, 1:12 p.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp, line 74
> > 
> >
> > Can you please clarfiy why we want to have `user` as a field of `Info` 
> > class? Basically, I do not think `Info` class should have such field.

I think a complete container info structure for cgroup should indicate which 
user it belongs to, so I add this before. Could remove it here.


> On June 15, 2016, 1:12 p.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp, line 425
> > 
> >
> > Can you please clarify why we need the `pid` field in the `Info` class? 
> > This is the only place I see we assign value to `Info::pid`, but it seems 
> > we never use this field? Basically, I do not think we should have such 
> > field in `Info` class.

I think a complete container info structure for cgroup should contain the pid 
isolated, so I add this before. Could remove it here.


> On June 15, 2016, 1:12 p.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp, line 511
> > 
> >
> > Can you please clarify why we need the `resources` field in the `Info` 
> > class? This is the only place I see we assign value to `Info::resources`, 
> > but it seems we never use this field? Basically, I do not think we should 
> > have such field in `Info` class.

I think a complete container info structure for cgroup should contain the 
resources allocated, so I add this before. Could remove it here.


- haosdent


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


On June 19, 2016, 3:34 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46158/
> ---
> 
> (Updated June 19, 2016, 3:34 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Guangya Liu, Ian Downes, Jie Yu, 
> Kevin Klues, and Qian Zhang.
> 
> 
> Bugs: MESOS-5041
> https://issues.apache.org/jira/browse/MESOS-5041
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Completed implementation of the cgroups unified isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46158/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 46158: Completed implementation of the cgroups unified isolator.

[haosdent huang]

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

(Updated June 19, 2016, 3:34 p.m.)


Review request for mesos, Gilbert Song, Guangya Liu, Ian Downes, Jie Yu, Kevin 
Klues, and Qian Zhang.


Changes
---

Address @qianzhang's comments.


Bugs: MESOS-5041
https://issues.apache.org/jira/browse/MESOS-5041


Repository: mesos


Description
---

Completed implementation of the cgroups unified isolator.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp PRE-CREATION 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 46158: Completed implementation of the cgroups unified isolator.

[haosdent huang]

> On June 12, 2016, 2:51 p.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp, lines 351-363
> > 
> >
> > Why do we prepare hierarchy here? I think preparing hierarchy for a 
> > specific container is a low-level work which should be handled in 
> > `Subsystem::prepare()` method rather than here. And in that way, we only 
> > need one `for` loop (the following one) rather than 2 here.

We delegate the create/destory operations of hierarchy to 
`CgroupsIsolatorProcess`. Because if we do the create/destroy operations of 
hierarchy in `Subsystem::prepare()` and `Subsystem::cleanup()`, it would create 
the hierarchy or destroy the hierarchy multiple times when differnt subsystems 
mount in same hierarchies.


> On June 12, 2016, 2:51 p.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp, lines 428-445
> > 
> >
> > Again I think assigning container to its own cgroup should be handled 
> > in `Subsystem::isolate()` method. In that way, we only need one `for` loop 
> > (the following one) rather than 2 here.

Same reason as above, suppose we mount multiple subsystems into same hierarchy, 
isolate would be executed multiple times.


> On June 12, 2016, 2:51 p.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp, lines 592-600
> > 
> >
> > Again I think destroying container's cgroup should be handled in 
> > `Subsystem::cleanup()` method.

Same reason as above, suppose we mount multiple subsystems into same hierarchy, 
the hierarchy would be destroyed multiple times.


- haosdent


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


On June 19, 2016, 3:34 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46158/
> ---
> 
> (Updated June 19, 2016, 3:34 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Guangya Liu, Ian Downes, Jie Yu, 
> Kevin Klues, and Qian Zhang.
> 
> 
> Bugs: MESOS-5041
> https://issues.apache.org/jira/browse/MESOS-5041
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Completed implementation of the cgroups unified isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46158/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 46158: Completed implementation of the cgroups unified isolator.

[haosdent huang]

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

(Updated June 19, 2016, 2:55 p.m.)


Review request for mesos, Gilbert Song, Guangya Liu, Ian Downes, Jie Yu, Kevin 
Klues, and Qian Zhang.


Changes
---

Rebase.


Bugs: MESOS-5041
https://issues.apache.org/jira/browse/MESOS-5041


Repository: mesos


Description
---

Completed implementation of the cgroups unified isolator.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp PRE-CREATION 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 46158: Completed implementation of the cgroups unified isolator.

[haosdent huang]

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

(Updated June 19, 2016, 10:21 a.m.)


Review request for mesos, Gilbert Song, Guangya Liu, Ian Downes, Jie Yu, and 
Kevin Klues.


Changes
---

Rebase.


Bugs: MESOS-5041
https://issues.apache.org/jira/browse/MESOS-5041


Repository: mesos


Description
---

Completed implementation of the cgroups unified isolator.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp PRE-CREATION 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 46158: Completed implementation of the cgroups unified isolator.

[Qian Zhang]

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




src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp (line 74)


Can you please clarfiy why we want to have `user` as a field of `Info` 
class? Basically, I do not think `Info` class should have such field.



src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp (line 425)


Can you please clarify why we need the `pid` field in the `Info` class? 
This is the only place I see we assign value to `Info::pid`, but it seems we 
never use this field? Basically, I do not think we should have such field in 
`Info` class.



src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp (line 511)


Can you please clarify why we need the `resources` field in the `Info` 
class? This is the only place I see we assign value to `Info::resources`, but 
it seems we never use this field? Basically, I do not think we should have such 
field in `Info` class.


- Qian Zhang


On April 16, 2016, 6:14 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46158/
> ---
> 
> (Updated April 16, 2016, 6:14 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Guangya Liu, Ian Downes, Jie Yu, and 
> Kevin Klues.
> 
> 
> Bugs: MESOS-5041
> https://issues.apache.org/jira/browse/MESOS-5041
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Completed implementation of the cgroups unified isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46158/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 46158: Completed implementation of the cgroups unified isolator.

[Qian Zhang]

> On June 11, 2016, 10:31 p.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp, line 131
> > 
> >
> > I think there is a special case that we need to handle: In the OS using 
> > systemd (e.g., RHEL 7.1), `cpu` and `cpuacct` subsystems are co-mounted, 
> > like this:
> > ```
> > ls -la /sys/fs/cgroup/
> > ...
> > lrwxrwxrwx.  1 root root  11 Dec 28 15:12 cpu -> cpu,cpuacct
> > lrwxrwxrwx.  1 root root  11 Dec 28 15:12 cpuacct -> cpu,cpuacct
> > drwxr-xr-x.  4 root root   0 Jan  2 17:01 cpu,cpuacct
> > ...
> > ```
> > That means in this `hierarchies` hashmap, the values of the two keys 
> > `cpu` and `cpuacct` are same (both of them are 
> > `/sys/fs/cgroup/cpu,cpuacct`), this will cause an issue in 
> > `CgroupsIsolatorProcess::prepare()`: In this method, we will call 
> > `prepareHierarchy()` which will check if the cgroup for the container to be 
> > creatd exists or not, if not, create the cgroup, if yes, return an Error. 
> > For the OS using systemd, I think we will always get the Error since for 
> > both `cpu` and `cpuacct` subsystems, we will try to create cgroup in the 
> > same location, i.e., `/sys/fs/cgroup/cpu,cpuacct/mesos/`.
> > 
> > You can take a look at the following code in the existing 
> > `CgroupsCpushareIsolatorProcess` class for how we handle this case. 
> > https://github.com/apache/mesos/blob/0.28.2/src/slave/containerizer/mesos/isolators/cgroups/cpushare.cpp#L94:L144
> 
> haosdent huang wrote:
> Because we use
> ```
> foreach (const string& hierarchy, info->subsystems.keys()) {
> ```
> this problem would not happen.
> 
> Suppose both `cpu` and `cpuacct` mount to `/sys/fs/cgroup/cpu,cpuacct`. 
> The `info->subsystems.keys()` only have one `/sys/fs/cgroup/cpu,cpuacct` here.

Ha, I got it, `multihashmap::keys()` does the trick which will return a `set`, 
so there will never be two `/sys/fs/cgroup/cpu,cpuacct`, thanks haosdent!


- Qian


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


On April 16, 2016, 6:14 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46158/
> ---
> 
> (Updated April 16, 2016, 6:14 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Guangya Liu, Ian Downes, Jie Yu, and 
> Kevin Klues.
> 
> 
> Bugs: MESOS-5041
> https://issues.apache.org/jira/browse/MESOS-5041
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Completed implementation of the cgroups unified isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46158/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 46158: Completed implementation of the cgroups unified isolator.

[Qian Zhang]

> On June 2, 2016, 10:58 p.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp, lines 105-106
> > 
> >
> > Since this is a multiple lines code, I think you need to add a newline 
> > after that, please check the following code as a reference:
> > 
> > https://github.com/apache/mesos/blob/0.28.1/src/slave/containerizer/mesos/isolators/network/port_mapping.cpp#L285;L288
> > And you may need to do this change for all other places like this.
> > 
> > And what about just name this variable as `subsystem`?
> 
> haosdent huang wrote:
> For the style, I think both 
> 
> ```
> string subsystemName =
>   strings::remove(type, cgroupsPrefix, strings::Mode::ANY);
> ```
> ```
> string subsystemName = strings::remove(
> type, cgroupsPrefix, strings::Mode::ANY);
> ```
> 
> are match mesos style, refer to the mesos style document 
> https://github.com/apache/mesos/blob/master/docs/c%2B%2B-style-guide.md#continuation
> 
> 
> I think use `subsystem` make bring confusing because it is the name of 
> subsystem, instead of an instance of `Subsystem`.

Sorry, I did not make it clear. My point is we need to change:
```
string subsystemName =
  strings::remove(type, cgroupsPrefix, strings::Mode::ANY);
if (hierarchies.contains(subsystemName)) {
  // Skip when the subsystem exists.
  continue;
}
```
to:
```
string subsystemName =
  strings::remove(type, cgroupsPrefix, strings::Mode::ANY);

if (hierarchies.contains(subsystemName)) {
  // Skip when the subsystem exists.
  continue;
}
```
You can refer to: 
https://github.com/apache/mesos/blob/master/docs/c%2B%2B-style-guide.md#empty-lines
 :-)


- Qian


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


On April 16, 2016, 6:14 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46158/
> ---
> 
> (Updated April 16, 2016, 6:14 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Guangya Liu, Ian Downes, Jie Yu, and 
> Kevin Klues.
> 
> 
> Bugs: MESOS-5041
> https://issues.apache.org/jira/browse/MESOS-5041
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Completed implementation of the cgroups unified isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46158/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 46158: Completed implementation of the cgroups unified isolator.

[haosdent huang]

> On June 11, 2016, 2:31 p.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp, line 131
> > 
> >
> > I think there is a special case that we need to handle: In the OS using 
> > systemd (e.g., RHEL 7.1), `cpu` and `cpuacct` subsystems are co-mounted, 
> > like this:
> > ```
> > ls -la /sys/fs/cgroup/
> > ...
> > lrwxrwxrwx.  1 root root  11 Dec 28 15:12 cpu -> cpu,cpuacct
> > lrwxrwxrwx.  1 root root  11 Dec 28 15:12 cpuacct -> cpu,cpuacct
> > drwxr-xr-x.  4 root root   0 Jan  2 17:01 cpu,cpuacct
> > ...
> > ```
> > That means in this `hierarchies` hashmap, the values of the two keys 
> > `cpu` and `cpuacct` are same (both of them are 
> > `/sys/fs/cgroup/cpu,cpuacct`), this will cause an issue in 
> > `CgroupsIsolatorProcess::prepare()`: In this method, we will call 
> > `prepareHierarchy()` which will check if the cgroup for the container to be 
> > creatd exists or not, if not, create the cgroup, if yes, return an Error. 
> > For the OS using systemd, I think we will always get the Error since for 
> > both `cpu` and `cpuacct` subsystems, we will try to create cgroup in the 
> > same location, i.e., `/sys/fs/cgroup/cpu,cpuacct/mesos/`.
> > 
> > You can take a look at the following code in the existing 
> > `CgroupsCpushareIsolatorProcess` class for how we handle this case. 
> > https://github.com/apache/mesos/blob/0.28.2/src/slave/containerizer/mesos/isolators/cgroups/cpushare.cpp#L94:L144

Because we use
```
foreach (const string& hierarchy, info->subsystems.keys()) {
```
this problem would not happen.

Suppose both `cpu` and `cpuacct` mount to `/sys/fs/cgroup/cpu,cpuacct`. The 
`info->subsystems.keys()` only have one `/sys/fs/cgroup/cpu,cpuacct` here.


- haosdent


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


On April 16, 2016, 10:14 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46158/
> ---
> 
> (Updated April 16, 2016, 10:14 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Guangya Liu, Ian Downes, Jie Yu, and 
> Kevin Klues.
> 
> 
> Bugs: MESOS-5041
> https://issues.apache.org/jira/browse/MESOS-5041
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Completed implementation of the cgroups unified isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46158/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 46158: Completed implementation of the cgroups unified isolator.

[haosdent huang]

> On June 2, 2016, 2:58 p.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp, line 97
> > 
> >
> > Why make it a static variable? I think 
> > `CgroupsIsolatorProcess::create()` will not be called more than once, right?

Nice catch, it only called when initialzation. Let me remvoe static.


> On June 2, 2016, 2:58 p.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp, lines 105-106
> > 
> >
> > Since this is a multiple lines code, I think you need to add a newline 
> > after that, please check the following code as a reference:
> > 
> > https://github.com/apache/mesos/blob/0.28.1/src/slave/containerizer/mesos/isolators/network/port_mapping.cpp#L285;L288
> > And you may need to do this change for all other places like this.
> > 
> > And what about just name this variable as `subsystem`?

For the style, I think both 

```
string subsystemName =
  strings::remove(type, cgroupsPrefix, strings::Mode::ANY);
```
```
string subsystemName = strings::remove(
type, cgroupsPrefix, strings::Mode::ANY);
```

are match mesos style, refer to the mesos style document 
https://github.com/apache/mesos/blob/master/docs/c%2B%2B-style-guide.md#continuation


I think use `subsystem` make bring confusing because it is the name of 
subsystem, instead of an instance of `Subsystem`.


> On June 2, 2016, 2:58 p.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp, line 106
> > 
> >
> > I would suggest to use `string::PREFIX` since we are sure `cgroups/` is 
> > the prefix.

Make sense, let me use PREFIX.


- haosdent


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


On April 16, 2016, 10:14 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46158/
> ---
> 
> (Updated April 16, 2016, 10:14 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Guangya Liu, Ian Downes, Jie Yu, and 
> Kevin Klues.
> 
> 
> Bugs: MESOS-5041
> https://issues.apache.org/jira/browse/MESOS-5041
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Completed implementation of the cgroups unified isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46158/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 46158: Completed implementation of the cgroups unified isolator.

[Qian Zhang]

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




src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp (lines 351 - 363)


Why do we prepare hierarchy here? I think preparing hierarchy for a 
specific container is a low-level work which should be handled in 
`Subsystem::prepare()` method rather than here. And in that way, we only need 
one `for` loop (the following one) rather than 2 here.



src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp (lines 388 - 390)


I am wondering if we really need this method. I think If we introduce a 
static member `hashmap> infos` in the class 
`Subsystem` as I mentioned in this comment 
https://reviews.apache.org/r/46158/#comment201273, then we may not need this 
`CgroupsIsolatorProcess::_prepare()` method, because in 
`CgroupsIsolatorProcess::watch()`, we can directly watch the `limitation` of 
the Info object in that static `infos` hashmap, and in each Subsystem (e.g., in 
`MemorySubsystem::oom()`), we can directly trigger this `limiation`.



src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp (lines 428 - 445)


Again I think assigning container to its own cgroup should be handled in 
`Subsystem::isolate()` method. In that way, we only need one `for` loop (the 
following one) rather than 2 here.



src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp (lines 592 - 600)


Again I think destroying container's cgroup should be handled in 
`Subsystem::cleanup()` method.


- Qian Zhang


On April 16, 2016, 6:14 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46158/
> ---
> 
> (Updated April 16, 2016, 6:14 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Guangya Liu, Ian Downes, Jie Yu, and 
> Kevin Klues.
> 
> 
> Bugs: MESOS-5041
> https://issues.apache.org/jira/browse/MESOS-5041
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Completed implementation of the cgroups unified isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46158/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 46158: Completed implementation of the cgroups unified isolator.

[Qian Zhang]

> On June 6, 2016, 10:15 p.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp, lines 79-80
> > 
> >
> > So we create subsystem for each container, suppose 
> > `"--isolation=cgroups/mem"` and there are 100 containers created in an 
> > agent, we will create 100 `MemorySubsystem` objects, right? It is a bit 
> > overhead since the only differences between these 100 objects are just 
> > `"containerId"` and `"cgroup"`. Can we only create one subsystem object for 
> > each enabled subystem specified in `"--isolation"` and reuse it for all the 
> > containers?

More thinkings: Instead of creating subsytem objects (e.g., `new 
MemorySubsystem`) in `CgroupsIsolatorProcess::Info::create()`, I think maybe we 
can create only ONE subystem object for each subsystem in 
`CgroupsIsolatorProcess::create()`, e.g., if the agent is started with 
`--isolation=cgroups/mem,cgroups/net_cls`, then we create 2 subsystem objects 
for `MemorySubsystem` and `NetClsSubsystem` respectively in 
`CgroupsIsolatorProcess::create()`, these two subsystem objects will be 
responsible for all the containers in the agent.

In the class `Subsystem`, we can introduce a static member `static 
hashmap> infos;` which will be shared by all 
the subsystem objects we created in `CgroupsIsolatorProcess::create()` and 
introduce a static method `static void Subsystem::addInfo(...)`, and then in 
`CgroupsIsolatorProcess::prepare()`, we create an `Info` object for the 
container, call `Subsystem::addInfo()` to add the `Info` object into 
`Subsystem::infos`, and then call the `prepare()` method of each subsystem 
object.

In this way, we only need to create one subsystem object which can handle all 
the containers in the agent. And another benefit is, the following members do 
need to be static anymore:
```
NetClsSubsystem::handleManager
PerfEventSubsystem::handleManager
```


- Qian


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


On April 16, 2016, 6:14 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46158/
> ---
> 
> (Updated April 16, 2016, 6:14 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Guangya Liu, Ian Downes, Jie Yu, and 
> Kevin Klues.
> 
> 
> Bugs: MESOS-5041
> https://issues.apache.org/jira/browse/MESOS-5041
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Completed implementation of the cgroups unified isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46158/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 46158: Completed implementation of the cgroups unified isolator.

[Qian Zhang]

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




src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp (line 131)


I think there is a special case that we need to handle: In the OS using 
systemd (e.g., RHEL 7.1), `cpu` and `cpuacct` subsystems are co-mounted, like 
this:
```
ls -la /sys/fs/cgroup/
...
lrwxrwxrwx.  1 root root  11 Dec 28 15:12 cpu -> cpu,cpuacct
lrwxrwxrwx.  1 root root  11 Dec 28 15:12 cpuacct -> cpu,cpuacct
drwxr-xr-x.  4 root root   0 Jan  2 17:01 cpu,cpuacct
...
```
That means in this `hierarchies` hashmap, the values of the two keys `cpu` 
and `cpuacct` are same (both of them are `/sys/fs/cgroup/cpu,cpuacct`), this 
will cause an issue in `CgroupsIsolatorProcess::prepare()`: In this method, we 
will call `prepareHierarchy()` which will check if the cgroup for the container 
to be creatd exists or not, if not, create the cgroup, if yes, return an Error. 
For the OS using systemd, I think we will always get the Error since for both 
`cpu` and `cpuacct` subsystems, we will try to create cgroup in the same 
location, i.e., `/sys/fs/cgroup/cpu,cpuacct/mesos/`.

You can take a look at the following code in the existing 
`CgroupsCpushareIsolatorProcess` class for how we handle this case. 
https://github.com/apache/mesos/blob/0.28.2/src/slave/containerizer/mesos/isolators/cgroups/cpushare.cpp#L94:L144



src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp (line 141)


I would suggest to rename this method to `prepareHierarchy()`,  the word 
`get` usually means this is a read-only method, but I think this is not a 
read-only method, we may do some write operations, e.g., mount subsystem, 
create root cgroup.

And I know there is already a method in this class named 
`prepareHierarchy()`, I would suggest to rename it to `createCgroup()` because 
the intent of that method is to create cgroup for a specific container, right?



src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp (line 153)


I think it is a bit strange to name it as `cgroupName`, actually it is 
subsystem, right?



src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp (line 288)


I would suggest to rename this method to `createCgroup()` since its intent 
is to create cgroup for a specific container.


- Qian Zhang


On April 16, 2016, 6:14 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46158/
> ---
> 
> (Updated April 16, 2016, 6:14 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Guangya Liu, Ian Downes, Jie Yu, and 
> Kevin Klues.
> 
> 
> Bugs: MESOS-5041
> https://issues.apache.org/jira/browse/MESOS-5041
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Completed implementation of the cgroups unified isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46158/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 46158: Completed implementation of the cgroups unified isolator.

[Qian Zhang]

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




src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp (lines 76 - 78)


I think these 3 lines can be merged into 1 line which will not exceed 80 
chars.



src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp (lines 368 - 370)


If we return from here, then I think we will not have chance to destroy the 
cgroups that we created for this container in `prepareHierarchy()`. The reason 
is, here we have not put `info` into `infos`, so in 
`CgroupsIsolatorProcess::cleanup()`, we will not do anything for this container 
since `infos` does not contains its `info`.
So I would suggest to do `infos[containerId] = info;` right after we get a 
newly created `info` from `info::create()`


- Qian Zhang


On April 16, 2016, 6:14 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46158/
> ---
> 
> (Updated April 16, 2016, 6:14 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Guangya Liu, Ian Downes, Jie Yu, and 
> Kevin Klues.
> 
> 
> Bugs: MESOS-5041
> https://issues.apache.org/jira/browse/MESOS-5041
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Completed implementation of the cgroups unified isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46158/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 46158: Completed implementation of the cgroups unified isolator.

[Qian Zhang]

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




src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp (lines 79 - 80)


So we create subsystem for each container, suppose 
`"--isolation=cgroups/mem"` and there are 100 containers created in an agent, 
we will create 100 `MemorySubsystem` objects, right? It is a bit overhead since 
the only differences between these 100 objects are just `"containerId"` and 
`"cgroup"`. Can we only create one subsystem object for each enabled subystem 
specified in `"--isolation"` and reuse it for all the containers?



src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp (lines 159 - 164)


I would suggest to merge them into:
```
return cgroups::prepare(
rootHierarchy,
cgroupName.get(),
rootCgroup);
```


- Qian Zhang


On April 16, 2016, 6:14 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46158/
> ---
> 
> (Updated April 16, 2016, 6:14 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Guangya Liu, Ian Downes, Jie Yu, and 
> Kevin Klues.
> 
> 
> Bugs: MESOS-5041
> https://issues.apache.org/jira/browse/MESOS-5041
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Completed implementation of the cgroups unified isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46158/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 46158: Completed implementation of the cgroups unified isolator.

[Qian Zhang]

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




src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp (line 97)


Why make it a static variable? I think `CgroupsIsolatorProcess::create()` 
will not be called more than once, right?



src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp (lines 105 - 106)


Since this is a multiple lines code, I think you need to add a newline 
after that, please check the following code as a reference:

https://github.com/apache/mesos/blob/0.28.1/src/slave/containerizer/mesos/isolators/network/port_mapping.cpp#L285;L288
And you may need to do this change for all other places like this.

And what about just name this variable as `subsystem`?



src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp (line 106)


I would suggest to use `string::PREFIX` since we are sure `cgroups/` is the 
prefix.



src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp (line 146)


s/internal/internally/



src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp (line 148)


s/different with/be different from/



src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp (line 149)


s/is belongs to/belongs to/



src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp (line 155)


s/subsystem/Subsystem/


- Qian Zhang


On April 16, 2016, 6:14 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46158/
> ---
> 
> (Updated April 16, 2016, 6:14 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Guangya Liu, Ian Downes, Jie Yu, and 
> Kevin Klues.
> 
> 
> Bugs: MESOS-5041
> https://issues.apache.org/jira/browse/MESOS-5041
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Completed implementation of the cgroups unified isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46158/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 46158: Completed implementation of the cgroups unified isolator.

[haosdent huang]

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

(Updated April 16, 2016, 10:14 a.m.)


Review request for mesos, Gilbert Song, Guangya Liu, Ian Downes, Jie Yu, and 
Kevin Klues.


Changes
---

Rebase.


Bugs: MESOS-5041
https://issues.apache.org/jira/browse/MESOS-5041


Repository: mesos


Description
---

Completed implementation of the cgroups unified isolator.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp PRE-CREATION 

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


Testing
---


Thanks,

haosdent huang

Review Request 46158: Completed implementation of the cgroups unified isolator.

[haosdent huang]

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

Review request for mesos, Gilbert Song, Guangya Liu, Ian Downes, Jie Yu, and 
Kevin Klues.


Bugs: MESOS-5041
https://issues.apache.org/jira/browse/MESOS-5041


Repository: mesos


Description
---

Completed implementation of the cgroups unified isolator.


Diffs
-

  src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp PRE-CREATION 

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


Testing
---


Thanks,

haosdent huang