----------------------------------------------------------- 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) <https://reviews.apache.org/r/46158/#comment202273> 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/<containerID>`. 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) <https://reviews.apache.org/r/46158/#comment202266> 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) <https://reviews.apache.org/r/46158/#comment202109> 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) <https://reviews.apache.org/r/46158/#comment202269> 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 > >
