-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22980/#review47764
-----------------------------------------------------------
Tim, sorry for getting to your reviews late. I'm really swamped recently. Thank
you for your patience.
I discovered at least three problems in this patch that needs to be addressed.
1) In CgroupsCpushareIsolatorProcess::cleanup, for each 'hierarchy', you'll
gonna call 'cgroups::destroy(hierarchy, info->cgroup,
cgroups::DESTROY_TIMEOUT)'. If cpu,cpuacct are co-mounted, you'll gonna delete
the same cgroup simultaneously by two threads!
2) In CgroupsCpushareIsolatorProcess::isolate, you'll gonna call
'cgroups::assign' twice for the same cgroup. Although this is not a correctness
problem, we should avoid that as much as possible.
3) In CgroupsCpushareIsolatorProcess::recover, removing orphans will be done
twice for each cgroup (although it's not a correctness issue because there will
be no orphan for 'cpuacct' as all orphans will be removed when the control
exits the loop for hierarchies["cpu"]).
I understand that the main trouble here is that our current containerizer code
assumes a very specific cgroups layout (i.e., one subsystem per hierarchy, all
nicely located under a base hierarchy like /sys/fs/cgroup). If a user has
pre-mounted a few subsystems in non-standard locations, the current code will
just fail during 'cgroups::prepare()'.
I really wanna change that. We can solve that iteratively. We probably wanna
change 'cgroups::prepare' a little bit to handle pre-mounted cases. For
example: instead of returning 'path::join(baseHierarchy, subsystem);' all the
time, we can return the hierarchy where the 'subsystem' has already been
pre-mounted (if that's the case). You probably wanna check the
'cgroups::hierarchy(subsystem)' function.
Then, in the cpushare isolator, during
'CgroupsCpushareIsolatorProcess::create', we can still maintain a map
'hierarchies: subsystem -> hierarchy'. In addition, we also keeps a vector
'subsystems': if it's a co-mounted situation, subsystems=['cpu,cpuacct'],
otherwise subsystems=['cpu', 'cpuacct']. In that way, whenever you want to
create/destroy a cgroup, we can do:
foreach (const string& subsystem, subsystems) {
hierarchy = hierarchies[subsystem];
// create/destroy cgroup.
// ...
}
Let me know your thoughts or comments.
- Jie Yu
On July 14, 2014, 8:04 p.m., Timothy St. Clair wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22980/
> -----------------------------------------------------------
>
> (Updated July 14, 2014, 8:04 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Jie Yu, and Vinod Kone.
>
>
> Bugs: MESOS-1195
> https://issues.apache.org/jira/browse/MESOS-1195
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Currently if a controller is mounted outside of mesos perview it errors out.
> This fixes that issue.
>
>
> Diffs
> -----
>
> src/slave/containerizer/isolators/cgroups/cpushare.cpp 3265a80
> src/slave/containerizer/isolators/cgroups/mem.cpp e8d1e35
>
> Diff: https://reviews.apache.org/r/22980/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy St. Clair
>
>
