Re: Review Request 49814: Implemented `CgroupsIsolatorProcess::create`.

[haosdent huang]

> On July 24, 2016, 5:50 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp, lines 78-84
> > 
> >
> > I just realize that the old `cgroups/cpu` isolator actually handles two 
> > subsystems: cpu and cpuacct. Therefore, I suggest we use the following 
> > logic:
> > ```
> > // Multipmap: isolator name -> subsystem name.
> > multihashmap table = {
> >   {"cpu", "cpu"},
> >   {"cpu", "cpuacct"},
> >   {"mem", "memory"},
> >   ...
> > };
> > 
> > foreach (const string& _isolator?...) {
> >   if (!strings::startsWith(_isolator, "cgroups/")) {
> > continue;
> >   }
> >   
> >   string isolator = strings::remove(
> >   isolator,
> >   "cgroups/",
> >   strings::mode::PREFIX);
> >   
> >   foreach (const string& subsystemName, table.get(isolator)) {
> > if (hierarchies.contains(subsystemName)) {
> >   continue;
> > }
> > 
> > Try hierarchy = cgroups::prepare(
> > flags.cgroups_hierarchy,
> > subsystemName,
> > flags.cgroups_root);
> > 
> > ...
> >   }
> > }
> > ```
> 
> haosdent huang wrote:
> Do we ask user to pass `cgroups/cpuacct` exactly in `isolation` flag 
> eventually?
> 
> Qian Zhang wrote:
> With the logic suggested by Jie, I think user does not need to specify 
> `cgroups/cpuacct` in `--isolation` flag, just `cgroups/cpu` should be enough.

Yes, have removed `cgroups/cpuacct` in `--isolation` flag.


- haosdent


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


On July 25, 2016, 2:35 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49814/
> ---
> 
> (Updated July 25, 2016, 2:35 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, Qian Zhang, and Zhengju Sha.
> 
> 
> Bugs: MESOS-5041
> https://issues.apache.org/jira/browse/MESOS-5041
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented `CgroupsIsolatorProcess::create`.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 
> 4a9f55bf3b217405bf90943f27a976422877a99e 
> 
> Diff: https://reviews.apache.org/r/49814/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 49814: Implemented `CgroupsIsolatorProcess::create`.

[Qian Zhang]

> On July 25, 2016, 1:50 a.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp, lines 78-84
> > 
> >
> > I just realize that the old `cgroups/cpu` isolator actually handles two 
> > subsystems: cpu and cpuacct. Therefore, I suggest we use the following 
> > logic:
> > ```
> > // Multipmap: isolator name -> subsystem name.
> > multihashmap table = {
> >   {"cpu", "cpu"},
> >   {"cpu", "cpuacct"},
> >   {"mem", "memory"},
> >   ...
> > };
> > 
> > foreach (const string& _isolator?...) {
> >   if (!strings::startsWith(_isolator, "cgroups/")) {
> > continue;
> >   }
> >   
> >   string isolator = strings::remove(
> >   isolator,
> >   "cgroups/",
> >   strings::mode::PREFIX);
> >   
> >   foreach (const string& subsystemName, table.get(isolator)) {
> > if (hierarchies.contains(subsystemName)) {
> >   continue;
> > }
> > 
> > Try hierarchy = cgroups::prepare(
> > flags.cgroups_hierarchy,
> > subsystemName,
> > flags.cgroups_root);
> > 
> > ...
> >   }
> > }
> > ```
> 
> haosdent huang wrote:
> Do we ask user to pass `cgroups/cpuacct` exactly in `isolation` flag 
> eventually?

With the logic suggested by Jie, I think user does not need to specify 
`cgroups/cpuacct` in `--isolation` flag, just `cgroups/cpu` should be enough.


- Qian


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


On July 25, 2016, 10:35 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49814/
> ---
> 
> (Updated July 25, 2016, 10:35 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, Qian Zhang, and Zhengju Sha.
> 
> 
> Bugs: MESOS-5041
> https://issues.apache.org/jira/browse/MESOS-5041
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented `CgroupsIsolatorProcess::create`.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 
> 4a9f55bf3b217405bf90943f27a976422877a99e 
> 
> Diff: https://reviews.apache.org/r/49814/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 49814: Implemented `CgroupsIsolatorProcess::create`.

[haosdent huang]

> On July 25, 2016, 1:46 p.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp, lines 82-86
> > 
> >
> > I think besides preparing the hierarchy, we also need to ensure no 
> > other subsystem is attached to the hierarchy, see the following code as an 
> > example:
> > 
> > https://github.com/apache/mesos/blob/1.0.0-rc4/src/slave/containerizer/mesos/isolators/cgroups/mem.cpp#L95:L105
> > 
> > Maybe you can do it in each subsystem's `load()` method.
> 
> haosdent huang wrote:
> We don't need add this check because there may be multiple subsystems 
> attach to a same hierarchy. For example, if `cpu` and `cpuacct` attached to 
> the same hierarchy `/a`, this check would failed.
> 
> Qian Zhang wrote:
> Understood, but I think we should still be able to handle that case with 
> the code below in the `load()` method of both `cpu` and `cpuacct` subsystems:
> ```
> // Ensure that no other subsystem is attached to the hierarchy.
> Try _subsystems = cgroups::subsystems(hierarchy.get());
> if (_subsystems.isError()) {
>   return Error(
>   "Failed to get the list of attached subsystems for hierarchy " +
>   hierarchy.get());
> } else if (_subsystems.get().size() != 1) {
>   foreach (const string& subsystem, subsystems.get()) {
> if (subsystem != "cpu" && subsystem != "cpuacct") {
>   return Error(
>   "Unexpected subsystems found attached to the hierarchy " +
>   hierarchy.get());
> }
> }
> ```
> 
> You can take a look at the following code as an reference:
> 
> https://github.com/apache/mesos/blob/1.0.0-rc4/src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp#L267:L273
> 
> haosdent huang wrote:
> Still could not, suppose this case, we mount `cpu`, `cpuacct`, `memory` 
> on the same hierarchy path `/a`. Then the check above would fail.
> 
> Qian Zhang wrote:
> I see, you are talking about cgroup v2 that we are going to support in 
> future, right?

No, we could do it no matter cgroup-v1 or cgroup-v2. For example,

```
# Clear the enviroment.
[root@localhost tmp]# cgclear

# Mount `cpuacct,cpu,memory` in the same hierarchy.
[root@localhost tmp]# mount -t cgroup -o 'cpuacct,cpu,memory' cgroup /tmp/a

# Verfied that `cpuacct,cpu,memory` mount successfully.
[root@localhost tmp]# cat /proc/mounts |grep cpuacct
cgroup /tmp/a cgroup rw,relatime,memory,cpuacct,cpu 0 0


[root@localhost tmp]# ls /tmp/a
cgroup.event_control  cpuacct.usage_percpu  cpu.rt_runtime_us  
memory.force_empty memory.memsw.limit_in_bytes  memory.oom_control  
memory.usage_in_bytes  tasks
cgroup.procs  cpu.cfs_period_us cpu.shares 
memory.limit_in_bytes  memory.memsw.max_usage_in_bytes  
memory.soft_limit_in_bytes  memory.use_hierarchy
cpuacct.stat  cpu.cfs_quota_us  cpu.stat   
memory.max_usage_in_bytes  memory.memsw.usage_in_bytes  memory.stat 
notify_on_release
cpuacct.usage cpu.rt_period_us  memory.failcnt 
memory.memsw.failcnt   memory.move_charge_at_immigrate  memory.swappiness   
release_agent
```


- haosdent


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


On July 25, 2016, 2:35 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49814/
> ---
> 
> (Updated July 25, 2016, 2:35 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, Qian Zhang, and Zhengju Sha.
> 
> 
> Bugs: MESOS-5041
> https://issues.apache.org/jira/browse/MESOS-5041
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented `CgroupsIsolatorProcess::create`.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 
> 4a9f55bf3b217405bf90943f27a976422877a99e 
> 
> Diff: https://reviews.apache.org/r/49814/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 49814: Implemented `CgroupsIsolatorProcess::create`.

[Qian Zhang]

> On July 25, 2016, 9:46 p.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp, lines 82-86
> > 
> >
> > I think besides preparing the hierarchy, we also need to ensure no 
> > other subsystem is attached to the hierarchy, see the following code as an 
> > example:
> > 
> > https://github.com/apache/mesos/blob/1.0.0-rc4/src/slave/containerizer/mesos/isolators/cgroups/mem.cpp#L95:L105
> > 
> > Maybe you can do it in each subsystem's `load()` method.
> 
> haosdent huang wrote:
> We don't need add this check because there may be multiple subsystems 
> attach to a same hierarchy. For example, if `cpu` and `cpuacct` attached to 
> the same hierarchy `/a`, this check would failed.
> 
> Qian Zhang wrote:
> Understood, but I think we should still be able to handle that case with 
> the code below in the `load()` method of both `cpu` and `cpuacct` subsystems:
> ```
> // Ensure that no other subsystem is attached to the hierarchy.
> Try _subsystems = cgroups::subsystems(hierarchy.get());
> if (_subsystems.isError()) {
>   return Error(
>   "Failed to get the list of attached subsystems for hierarchy " +
>   hierarchy.get());
> } else if (_subsystems.get().size() != 1) {
>   foreach (const string& subsystem, subsystems.get()) {
> if (subsystem != "cpu" && subsystem != "cpuacct") {
>   return Error(
>   "Unexpected subsystems found attached to the hierarchy " +
>   hierarchy.get());
> }
> }
> ```
> 
> You can take a look at the following code as an reference:
> 
> https://github.com/apache/mesos/blob/1.0.0-rc4/src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp#L267:L273
> 
> haosdent huang wrote:
> Still could not, suppose this case, we mount `cpu`, `cpuacct`, `memory` 
> on the same hierarchy path `/a`. Then the check above would fail.

I see, you are talking about cgroup v2 that we are going to support in future, 
right?


- Qian


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


On July 25, 2016, 10:35 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49814/
> ---
> 
> (Updated July 25, 2016, 10:35 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, Qian Zhang, and Zhengju Sha.
> 
> 
> Bugs: MESOS-5041
> https://issues.apache.org/jira/browse/MESOS-5041
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented `CgroupsIsolatorProcess::create`.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 
> 4a9f55bf3b217405bf90943f27a976422877a99e 
> 
> Diff: https://reviews.apache.org/r/49814/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 49814: Implemented `CgroupsIsolatorProcess::create`.

[Qian Zhang]

> On July 25, 2016, 9:46 p.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp, lines 82-86
> > 
> >
> > I think besides preparing the hierarchy, we also need to ensure no 
> > other subsystem is attached to the hierarchy, see the following code as an 
> > example:
> > 
> > https://github.com/apache/mesos/blob/1.0.0-rc4/src/slave/containerizer/mesos/isolators/cgroups/mem.cpp#L95:L105
> > 
> > Maybe you can do it in each subsystem's `load()` method.
> 
> haosdent huang wrote:
> We don't need add this check because there may be multiple subsystems 
> attach to a same hierarchy. For example, if `cpu` and `cpuacct` attached to 
> the same hierarchy `/a`, this check would failed.

Understood, but I think we should still be able to handle that case with the 
code below in the `load()` method of both `cpu` and `cpuacct` subsystems:
```
// Ensure that no other subsystem is attached to the hierarchy.
Try _subsystems = cgroups::subsystems(hierarchy.get());
if (_subsystems.isError()) {
  return Error(
  "Failed to get the list of attached subsystems for hierarchy " +
  hierarchy.get());
} else if (_subsystems.get().size() != 1) {
  foreach (const string& subsystem, subsystems.get()) {
if (subsystem != "cpu" && subsystem != "cpuacct") {
  return Error(
  "Unexpected subsystems found attached to the hierarchy " +
  hierarchy.get());
}
}
```

You can take a look at the following code as an reference:
https://github.com/apache/mesos/blob/1.0.0-rc4/src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp#L267:L273


- Qian


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


On July 25, 2016, 10:35 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49814/
> ---
> 
> (Updated July 25, 2016, 10:35 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, Qian Zhang, and Zhengju Sha.
> 
> 
> Bugs: MESOS-5041
> https://issues.apache.org/jira/browse/MESOS-5041
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented `CgroupsIsolatorProcess::create`.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 
> 4a9f55bf3b217405bf90943f27a976422877a99e 
> 
> Diff: https://reviews.apache.org/r/49814/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 49814: Implemented `CgroupsIsolatorProcess::create`.

[haosdent huang]

> On July 25, 2016, 1:46 p.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp, lines 82-86
> > 
> >
> > I think besides preparing the hierarchy, we also need to ensure no 
> > other subsystem is attached to the hierarchy, see the following code as an 
> > example:
> > 
> > https://github.com/apache/mesos/blob/1.0.0-rc4/src/slave/containerizer/mesos/isolators/cgroups/mem.cpp#L95:L105
> > 
> > Maybe you can do it in each subsystem's `load()` method.

We don't need add this check because there may be multiple subsystems attach to 
a same hierarchy. For example, if `cpu` and `cpuacct` attached to the same 
hierarchy `/a`, this check would failed.


- haosdent


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


On July 25, 2016, 2:35 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49814/
> ---
> 
> (Updated July 25, 2016, 2:35 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, Qian Zhang, and Zhengju Sha.
> 
> 
> Bugs: MESOS-5041
> https://issues.apache.org/jira/browse/MESOS-5041
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented `CgroupsIsolatorProcess::create`.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 
> 4a9f55bf3b217405bf90943f27a976422877a99e 
> 
> Diff: https://reviews.apache.org/r/49814/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 49814: Implemented `CgroupsIsolatorProcess::create`.

[Qian Zhang]

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




src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp (lines 82 - 86)


I think besides preparing the hierarchy, we also need to ensure no other 
subsystem is attached to the hierarchy, see the following code as an example:

https://github.com/apache/mesos/blob/1.0.0-rc4/src/slave/containerizer/mesos/isolators/cgroups/mem.cpp#L95:L105

Maybe you can do it in each subsystem's `load()` method.


- Qian Zhang


On July 25, 2016, 10:35 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49814/
> ---
> 
> (Updated July 25, 2016, 10:35 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, Qian Zhang, and Zhengju Sha.
> 
> 
> Bugs: MESOS-5041
> https://issues.apache.org/jira/browse/MESOS-5041
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented `CgroupsIsolatorProcess::create`.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 
> 4a9f55bf3b217405bf90943f27a976422877a99e 
> 
> Diff: https://reviews.apache.org/r/49814/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 49814: Implemented `CgroupsIsolatorProcess::create`.

[haosdent huang]

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

(Updated July 25, 2016, 2:21 a.m.)


Review request for mesos, Gilbert Song, Jie Yu, Qian Zhang, and Zhengju Sha.


Changes
---

Address @jieyu's comments.


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


Repository: mesos


Description
---

Implemented `CgroupsIsolatorProcess::create`.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 
4a9f55bf3b217405bf90943f27a976422877a99e 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 49814: Implemented `CgroupsIsolatorProcess::create`.

[haosdent huang]

> On July 24, 2016, 5:50 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp, lines 78-84
> > 
> >
> > I just realize that the old `cgroups/cpu` isolator actually handles two 
> > subsystems: cpu and cpuacct. Therefore, I suggest we use the following 
> > logic:
> > ```
> > // Multipmap: isolator name -> subsystem name.
> > multihashmap table = {
> >   {"cpu", "cpu"},
> >   {"cpu", "cpuacct"},
> >   {"mem", "memory"},
> >   ...
> > };
> > 
> > foreach (const string& _isolator?...) {
> >   if (!strings::startsWith(_isolator, "cgroups/")) {
> > continue;
> >   }
> >   
> >   string isolator = strings::remove(
> >   isolator,
> >   "cgroups/",
> >   strings::mode::PREFIX);
> >   
> >   foreach (const string& subsystemName, table.get(isolator)) {
> > if (hierarchies.contains(subsystemName)) {
> >   continue;
> > }
> > 
> > Try hierarchy = cgroups::prepare(
> > flags.cgroups_hierarchy,
> > subsystemName,
> > flags.cgroups_root);
> > 
> > ...
> >   }
> > }
> > ```

Do we ask user to pass `cgroups/cpuacct` exactly in `isolation` flag eventually?


- haosdent


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


On July 24, 2016, 12:52 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49814/
> ---
> 
> (Updated July 24, 2016, 12:52 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, Qian Zhang, and Zhengju Sha.
> 
> 
> Bugs: MESOS-5041
> https://issues.apache.org/jira/browse/MESOS-5041
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented `CgroupsIsolatorProcess::create`.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 
> 4a9f55bf3b217405bf90943f27a976422877a99e 
> 
> Diff: https://reviews.apache.org/r/49814/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 49814: Implemented `CgroupsIsolatorProcess::create`.

[Jie Yu]

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




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


I just realize that the old `cgroups/cpu` isolator actually handles two 
subsystems: cpu and cpuacct. Therefore, I suggest we use the following logic:
```
// Multipmap: isolator name -> subsystem name.
multihashmap table = {
  {"cpu", "cpu"},
  {"cpu", "cpuacct"},
  {"mem", "memory"},
  ...
};

foreach (const string& _isolator?...) {
  if (!strings::startsWith(_isolator, "cgroups/")) {
continue;
  }
  
  string isolator = strings::remove(
  isolator,
  "cgroups/",
  strings::mode::PREFIX);
  
  foreach (const string& subsystemName, table.get(isolator)) {
if (hierarchies.contains(subsystemName)) {
  continue;
}

Try hierarchy = cgroups::prepare(
flags.cgroups_hierarchy,
subsystemName,
flags.cgroups_root);

...
  }
}
```


- Jie Yu


On July 24, 2016, 12:52 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49814/
> ---
> 
> (Updated July 24, 2016, 12:52 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, Qian Zhang, and Zhengju Sha.
> 
> 
> Bugs: MESOS-5041
> https://issues.apache.org/jira/browse/MESOS-5041
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented `CgroupsIsolatorProcess::create`.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 
> 4a9f55bf3b217405bf90943f27a976422877a99e 
> 
> Diff: https://reviews.apache.org/r/49814/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 49814: Implemented `CgroupsIsolatorProcess::create`.

[haosdent huang]

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

(Updated July 24, 2016, 12:52 p.m.)


Review request for mesos, Gilbert Song, Jie Yu, Qian Zhang, and Zhengju Sha.


Changes
---

Update the comment in code.


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


Repository: mesos


Description
---

Implemented `CgroupsIsolatorProcess::create`.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 
4a9f55bf3b217405bf90943f27a976422877a99e 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 49814: Implemented `CgroupsIsolatorProcess::create`.

[Jie Yu]

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


Fix it, then Ship it!





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


I would simply just kill this temp var and inline `"cgroups/"` in the 
following code.



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


Please add a comment about what is key and what is value since it's not 
quite obvious.



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


s/type/isolator/



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


I would simply inline `prepareHierarchy` here. Is this helper used 
elsewhere?



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


s/create/subsystem/



src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp (lines 112 - 117)


I'd suggest we use the same subsystem name as that in the kernel. We can 
deprecate some existing name (e.g., use cgroups/memory forward and deprecate 
cgroups/mem).



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


Please avoid using non-POD global variables because it might cause issues 
during tear down. Simply remove `static` here.


- Jie Yu


On July 21, 2016, 7:04 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49814/
> ---
> 
> (Updated July 21, 2016, 7:04 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, Qian Zhang, and Zhengju Sha.
> 
> 
> Bugs: MESOS-5041
> https://issues.apache.org/jira/browse/MESOS-5041
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented `CgroupsIsolatorProcess::create`.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp 
> c57baec88437f68886702a40ec8a6a6458546119 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 
> 4a9f55bf3b217405bf90943f27a976422877a99e 
> 
> Diff: https://reviews.apache.org/r/49814/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 49814: Implemented `CgroupsIsolatorProcess::create`.

[haosdent huang]

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

(Updated July 21, 2016, 7:04 p.m.)


Review request for mesos, Gilbert Song, Jie Yu, Qian Zhang, and Zhengju Sha.


Changes
---

Address @qianzhang's comments.


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


Repository: mesos


Description
---

Implemented `CgroupsIsolatorProcess::create`.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp 
c57baec88437f68886702a40ec8a6a6458546119 
  src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 
4a9f55bf3b217405bf90943f27a976422877a99e 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 49814: Implemented `CgroupsIsolatorProcess::create`.

[haosdent huang]

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

(Updated July 20, 2016, 6:36 p.m.)


Review request for mesos, Gilbert Song, Jie Yu, Qian Zhang, and Zhengju Sha.


Changes
---

Address @qianzhang's comments.


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


Repository: mesos


Description
---

Implemented `CgroupsIsolatorProcess::create`.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp 
c57baec88437f68886702a40ec8a6a6458546119 
  src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 
4a9f55bf3b217405bf90943f27a976422877a99e 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 49814: Implemented `CgroupsIsolatorProcess::create`.

[Qian Zhang]

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


Fix it, then Ship it!





src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp (lines 83 - 85)


I would suggest to make the naming of these 3 parameters consistent, i.e., 
all starting with underscore. You can take the method below it as an example :-)



src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp (lines 108 - 110)


Ditto



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


s/used/use/



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


s/by/be/


- Qian Zhang


On July 15, 2016, 11:31 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49814/
> ---
> 
> (Updated July 15, 2016, 11:31 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, Qian Zhang, and Zhengju Sha.
> 
> 
> Bugs: MESOS-5041
> https://issues.apache.org/jira/browse/MESOS-5041
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented `CgroupsIsolatorProcess::create`.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp 
> c57baec88437f68886702a40ec8a6a6458546119 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 
> 4a9f55bf3b217405bf90943f27a976422877a99e 
> 
> Diff: https://reviews.apache.org/r/49814/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 49814: Implemented `CgroupsIsolatorProcess::create`.

[haosdent huang]

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

(Updated July 14, 2016, 5:12 p.m.)


Review request for mesos, Gilbert Song, Jie Yu, Qian Zhang, and Zhengju Sha.


Changes
---

Update ticket field.


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


Repository: mesos


Description
---

Implemented `CgroupsIsolatorProcess::create`.


Diffs
-

  src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp 
c57baec88437f68886702a40ec8a6a6458546119 
  src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 
4a9f55bf3b217405bf90943f27a976422877a99e 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 49814: Implemented `CgroupsIsolatorProcess::create`.

[haosdent huang]

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

(Updated July 14, 2016, 5:12 p.m.)


Review request for mesos, Gilbert Song, Jie Yu, Qian Zhang, and Zhengju Sha.


Changes
---

Rebase.


Repository: mesos


Description
---

Implemented `CgroupsIsolatorProcess::create`.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp 
c57baec88437f68886702a40ec8a6a6458546119 
  src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 
4a9f55bf3b217405bf90943f27a976422877a99e 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 49814: Implemented `CgroupsIsolatorProcess::create`.

[haosdent huang]

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

(Updated July 14, 2016, 4:44 p.m.)


Review request for mesos, Gilbert Song, Jie Yu, Qian Zhang, and Zhengju Sha.


Changes
---

Rebase.


Repository: mesos


Description
---

Implemented `CgroupsIsolatorProcess::create`.


Diffs (updated)
-

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

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 49814: Implemented `CgroupsIsolatorProcess::create`.

[haosdent huang]

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

(Updated July 13, 2016, 6:42 p.m.)


Review request for mesos, Gilbert Song, Jie Yu, and Qian Zhang.


Changes
---

Rebase and address comments.


Repository: mesos


Description
---

Implemented `CgroupsIsolatorProcess::create`.


Diffs (updated)
-

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

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 49814: Implemented `CgroupsIsolatorProcess::create`.

[Qian Zhang]

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




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


Should be "// Create subsytem"? I guess `Subsystem::initialize()` is the 
one to initialize the subsystem?


- Qian Zhang


On July 9, 2016, 6:16 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49814/
> ---
> 
> (Updated July 9, 2016, 6:16 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, and Qian Zhang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented `CgroupsIsolatorProcess::create`.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/49814/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 49814: Implemented `CgroupsIsolatorProcess::create`.

[haosdent huang]

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

(Updated July 9, 2016, 10:16 a.m.)


Review request for mesos, Gilbert Song, Jie Yu, and Qian Zhang.


Changes
---

Rebase.


Repository: mesos


Description
---

Implemented `CgroupsIsolatorProcess::create`.


Diffs (updated)
-

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

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


Testing
---


Thanks,

haosdent huang

Review Request 49814: Implemented `CgroupsIsolatorProcess::create`.

[haosdent huang]

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

Review request for mesos, Gilbert Song, Jie Yu, and Qian Zhang.


Repository: mesos


Description
---

Implemented `CgroupsIsolatorProcess::create`.


Diffs
-

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

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


Testing
---


Thanks,

haosdent huang