> On July 15, 2014, 6:03 a.m., Jie Yu wrote:
> > 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.
Thanks for the feedback, I'll take a look today.
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22980/#review47764
-----------------------------------------------------------
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
>
>
