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



src/linux/cgroups.cpp
<https://reviews.apache.org/r/22981/#comment82484>

    I don't think this is going to work.
    
    First off, the cpushare isolator needs to be made aware of the co-mounted 
hierarchy. Otherwise lot of its methods are either going to fail or do 
something wrong (recover(), cleanup(), usage()).
    
    Second, this is being done transparently to the isolator by adding a 
special case to the underlying cgroups library. I think this is breaking the 
abstraction. It should be done the other way.
    
    Here is what I suggest:
    
    In cpushare isolator prepare() figure out if we there is already a 
co-mounted cpu,cpuacct hierarchy. If yes "hierarchies" map should just contain 
one key "cpu,cpuacct". If not it should do what it is doing today.
    
    You would also want to update recover() and update() to make sure they are 
not using hard-coded keys into the map (currently I see that they are using 
hardcoded "cpu" and "cpuacct" keys.)  cleanup() seems to be fine.


- Vinod Kone


On June 25, 2014, 8:01 p.m., Timothy St. Clair wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22981/
> -----------------------------------------------------------
> 
> (Updated June 25, 2014, 8:01 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Vinod Kone.
> 
> 
> Bugs: MESOS-1195
>     https://issues.apache.org/jira/browse/MESOS-1195
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> Enable co-mounting of only the cpu & cpuacct.  Systemd will co-mount by 
> default.
> 
> 
> Diffs
> -----
> 
>   src/linux/cgroups.cpp 5472eb8 
> 
> Diff: https://reviews.apache.org/r/22981/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> + full run of marathon jobs.
> 
> 
> Thanks,
> 
> Timothy St. Clair
> 
>

Reply via email to