mesos-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jie Yu" <yujie....@gmail.com>
Subject Re: Review Request 22980: Enable support for existing cgroups, don't always try to create.
Date Tue, 15 Jul 2014 06:03:10 GMT

-----------------------------------------------------------
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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message