You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Ian Downes <ia...@gmail.com> on 2013/12/21 07:54:20 UTC

Review Request 16432: Forth part of isolation refactor - cgroup isolators

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

Review request for mesos, Benjamin Hindman, Chi Zhang, and samya.


Repository: mesos-git


Description
-------

Forth part of isolation refactor - cgroup isolators

CPU and Memory isolators using Linux cgroups.

This code was written by Chi Zhang <cz...@twitter.com> and based on the original cgroups_isolator.


Diffs
-----

  src/Makefile.am 42dafbcd6e2d4f67d2705c5a2bea6ec6a0f4fcc1 
  src/slave/container/isolators/cgroups/cgroups.hpp PRE-CREATION 
  src/slave/container/isolators/cgroups/cgroups.cpp PRE-CREATION 
  src/slave/container/isolators/cgroups/cpushare.hpp PRE-CREATION 
  src/slave/container/isolators/cgroups/cpushare.cpp PRE-CREATION 
  src/slave/container/isolators/cgroups/mem.hpp PRE-CREATION 
  src/slave/container/isolators/cgroups/mem.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/16432/diff/


Testing
-------


Thanks,

Ian Downes

Re: Review Request 16432: Containerizer - cgroup isolators (part 4)

Posted by Chi Zhang <ch...@gmail.com>.

> On Jan. 23, 2014, 7:32 p.m., Niklas Nielsen wrote:
> > Again, good stuff :) A few easy style nits/questions.

Hi Niklas,

Thanks for taking the time to review this. A new version has been pushed up and will be incorporated into Ian's branch and reflected here. I can't close issues here since I am not the original reporter.

In summary:
- all line-wraps and indentation comments are fixed. I also did another run to capture similar issues in the code.
- The 3 LOG(FATAL)s in cgroups_helper::setup are ok because they are in the initialization path of an isolator. We can't do much after that, for example, if the memcg mount point couldn't be set up. I fixed the ones in ::prepare/::oomListen to LOG(ERROR) since an error in dealing with a container shouldn't halt the system as you pointed out. 
- constants.[h/c]pp have global scope while those constants are only used in their respective files, so I thought it could be okay to leave them in the implementation files, at least for now?
- I believe I've fixed all other small issues.


- Chi


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


On Jan. 21, 2014, 7:35 p.m., Ian Downes wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16432/
> -----------------------------------------------------------
> 
> (Updated Jan. 21, 2014, 7:35 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Chi Zhang, samya, and Vinod Kone.
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> Forth part of isolation refactor - cgroup isolators
> 
> CPU and Memory isolators using Linux cgroups.
> 
> This code was written by Chi Zhang <cz...@twitter.com> and based on the original cgroups_isolator.
> 
> 
> Diffs
> -----
> 
>   src/slave/container/isolators/cgroups/cgroups.hpp PRE-CREATION 
>   src/slave/container/isolators/cgroups/cgroups.cpp PRE-CREATION 
>   src/slave/container/isolators/cgroups/cpushare.hpp PRE-CREATION 
>   src/slave/container/isolators/cgroups/cpushare.cpp PRE-CREATION 
>   src/slave/container/isolators/cgroups/mem.hpp PRE-CREATION 
>   src/slave/container/isolators/cgroups/mem.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/16432/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Ian Downes
> 
>

Re: Review Request 16432: Containerizer - cgroup isolators (part 4)

Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16432/#review32647
-----------------------------------------------------------

Ship it!


Again, good stuff :) A few easy style nits/questions.


src/slave/container/isolators/cgroups/cgroups.hpp
<https://reviews.apache.org/r/16432/#comment61529>

    Can these be set in the initializer list?



src/slave/container/isolators/cgroups/cgroups.hpp
<https://reviews.apache.org/r/16432/#comment61530>

    Wrap and indent :)



src/slave/container/isolators/cgroups/cgroups.hpp
<https://reviews.apache.org/r/16432/#comment61531>

    This should fit on one line.



src/slave/container/isolators/cgroups/cgroups.cpp
<https://reviews.apache.org/r/16432/#comment61532>

    Does it make sense to use path::join?



src/slave/container/isolators/cgroups/cgroups.cpp
<https://reviews.apache.org/r/16432/#comment61533>

    Any particular need to do LOG(FATAL)?



src/slave/container/isolators/cgroups/cgroups.cpp
<https://reviews.apache.org/r/16432/#comment61534>

    Same here.



src/slave/container/isolators/cgroups/cgroups.cpp
<https://reviews.apache.org/r/16432/#comment61535>

    And here.



src/slave/container/isolators/cgroups/cgroups.cpp
<https://reviews.apache.org/r/16432/#comment61555>

    Wrap please.



src/slave/container/isolators/cgroups/cgroups.cpp
<https://reviews.apache.org/r/16432/#comment61551>

    CHECK's are usually wrapped and indented:
    
      CHECK_SOME(exists)
        << "Failed to determine if '" << info->croup
        << "' cgroup already exists in the hierarchy at '"
        << hierarchy << "': " << exists.error();



src/slave/container/isolators/cgroups/cgroups.cpp
<https://reviews.apache.org/r/16432/#comment61536>

    Any particular need for LOG(FATAL)?



src/slave/container/isolators/cgroups/cgroups.cpp
<https://reviews.apache.org/r/16432/#comment61539>

    Fix indentation :)



src/slave/container/isolators/cgroups/cpushare.hpp
<https://reviews.apache.org/r/16432/#comment61528>

    Wrap and indent.



src/slave/container/isolators/cgroups/cpushare.cpp
<https://reviews.apache.org/r/16432/#comment61540>

    Also here, does it make sense to move to constants.hpp?



src/slave/container/isolators/cgroups/cpushare.cpp
<https://reviews.apache.org/r/16432/#comment61554>

    Wrap please :)



src/slave/container/isolators/cgroups/cpushare.cpp
<https://reviews.apache.org/r/16432/#comment61552>

    Wrap please.



src/slave/container/isolators/cgroups/cpushare.cpp
<https://reviews.apache.org/r/16432/#comment61553>

    Wrap please.



src/slave/container/isolators/cgroups/cpushare.cpp
<https://reviews.apache.org/r/16432/#comment61549>

    How about wrapping after '=' ?
    
      Try<Nothing> write =
          cgroups::write(hierarchy, info->cgroup, "cpu.shares", stringify(shares));



src/slave/container/isolators/cgroups/cpushare.cpp
<https://reviews.apache.org/r/16432/#comment61556>

    Wrap please.



src/slave/container/isolators/cgroups/mem.hpp
<https://reviews.apache.org/r/16432/#comment61550>

    This could fit on a single line.



src/slave/container/isolators/cgroups/mem.cpp
<https://reviews.apache.org/r/16432/#comment61537>

    How about moving this to constants.hpp?



src/slave/container/isolators/cgroups/mem.cpp
<https://reviews.apache.org/r/16432/#comment61548>

    How about wrapping after '='?
    
      Try<Nothing> write =
          cgroups::write(hierarchy, info->cgroup, control, stringify(limitInBytes));



src/slave/container/isolators/cgroups/mem.cpp
<https://reviews.apache.org/r/16432/#comment61542>

    Does it make sense to move this to os.hpp in stout?



src/slave/container/isolators/cgroups/mem.cpp
<https://reviews.apache.org/r/16432/#comment61557>

    Wrap please.



src/slave/container/isolators/cgroups/mem.cpp
<https://reviews.apache.org/r/16432/#comment61546>

    Any good reason for LOG(FATAL)?



src/slave/container/isolators/cgroups/mem.cpp
<https://reviews.apache.org/r/16432/#comment61545>

    With the usual wrapping, shouldn't this be:
    
          LOG(ERROR) << "Failed to numify 'memory.limit_in_bytes': "
                     << bytes.error();
    ?



src/slave/container/isolators/cgroups/mem.cpp
<https://reviews.apache.org/r/16432/#comment61543>

    Same here: 
          LOG(ERROR) << "Failed to numify 'memory.usage_in_bytes': "
                     << bytes.error();
    ?


- Niklas Nielsen


On Jan. 21, 2014, 11:35 a.m., Ian Downes wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16432/
> -----------------------------------------------------------
> 
> (Updated Jan. 21, 2014, 11:35 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Chi Zhang, samya, and Vinod Kone.
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> Forth part of isolation refactor - cgroup isolators
> 
> CPU and Memory isolators using Linux cgroups.
> 
> This code was written by Chi Zhang <cz...@twitter.com> and based on the original cgroups_isolator.
> 
> 
> Diffs
> -----
> 
>   src/slave/container/isolators/cgroups/cgroups.hpp PRE-CREATION 
>   src/slave/container/isolators/cgroups/cgroups.cpp PRE-CREATION 
>   src/slave/container/isolators/cgroups/cpushare.hpp PRE-CREATION 
>   src/slave/container/isolators/cgroups/cpushare.cpp PRE-CREATION 
>   src/slave/container/isolators/cgroups/mem.hpp PRE-CREATION 
>   src/slave/container/isolators/cgroups/mem.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/16432/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Ian Downes
> 
>

Re: Review Request 16432: Containerizer - cgroup isolators (part 4)

Posted by Chi Zhang <ch...@gmail.com>.

> On Jan. 22, 2014, 12:22 a.m., Ian Downes wrote:
> > src/slave/container/isolators/cgroups/cpushare.cpp, line 305
> > <https://reviews.apache.org/r/16432/diff/3/?file=425495#file425495line305>
> >
> >     Also need to destroy the cpuacct cgroup for the container.

This is fixed and incorporated into Ian's branch.


- Chi


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


On Jan. 21, 2014, 7:35 p.m., Ian Downes wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16432/
> -----------------------------------------------------------
> 
> (Updated Jan. 21, 2014, 7:35 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Chi Zhang, samya, and Vinod Kone.
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> Forth part of isolation refactor - cgroup isolators
> 
> CPU and Memory isolators using Linux cgroups.
> 
> This code was written by Chi Zhang <cz...@twitter.com> and based on the original cgroups_isolator.
> 
> 
> Diffs
> -----
> 
>   src/slave/container/isolators/cgroups/cgroups.hpp PRE-CREATION 
>   src/slave/container/isolators/cgroups/cgroups.cpp PRE-CREATION 
>   src/slave/container/isolators/cgroups/cpushare.hpp PRE-CREATION 
>   src/slave/container/isolators/cgroups/cpushare.cpp PRE-CREATION 
>   src/slave/container/isolators/cgroups/mem.hpp PRE-CREATION 
>   src/slave/container/isolators/cgroups/mem.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/16432/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Ian Downes
> 
>

Re: Review Request 16432: Containerizer - cgroup isolators (part 4)

Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16432/#review32447
-----------------------------------------------------------



src/slave/container/isolators/cgroups/cpushare.cpp
<https://reviews.apache.org/r/16432/#comment61290>

    Also need to destroy the cpuacct cgroup for the container.


- Ian Downes


On Jan. 21, 2014, 7:35 p.m., Ian Downes wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16432/
> -----------------------------------------------------------
> 
> (Updated Jan. 21, 2014, 7:35 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Chi Zhang, samya, and Vinod Kone.
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> Forth part of isolation refactor - cgroup isolators
> 
> CPU and Memory isolators using Linux cgroups.
> 
> This code was written by Chi Zhang <cz...@twitter.com> and based on the original cgroups_isolator.
> 
> 
> Diffs
> -----
> 
>   src/slave/container/isolators/cgroups/cgroups.hpp PRE-CREATION 
>   src/slave/container/isolators/cgroups/cgroups.cpp PRE-CREATION 
>   src/slave/container/isolators/cgroups/cpushare.hpp PRE-CREATION 
>   src/slave/container/isolators/cgroups/cpushare.cpp PRE-CREATION 
>   src/slave/container/isolators/cgroups/mem.hpp PRE-CREATION 
>   src/slave/container/isolators/cgroups/mem.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/16432/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Ian Downes
> 
>

Re: Review Request 16432: Containerizer - cgroup isolators (part 4)

Posted by Ian Downes <ia...@gmail.com>.

> On Jan. 28, 2014, 1:46 a.m., Adam B wrote:
> > src/slave/containerizer/isolators/cgroups/cpushare.hpp, line 90
> > <https://reviews.apache.org/r/16432/diff/4/?file=451180#file451180line90>
> >
> >     Just because cgroups uses "cpuacct", does that mean we need to keep that archaic name? I think cpuAccountHierarchy would be easier to read. Or cpuShareHierarchy if you'd rather.

I'd prefer to keep it matching the cgroup it refers to for consistency but don't feel super strongly. Others?


> On Jan. 28, 2014, 1:46 a.m., Adam B wrote:
> > src/slave/containerizer/isolators/cgroups/cpushare.cpp, line 361
> > <https://reviews.apache.org/r/16432/diff/4/?file=451181#file451181line361>
> >
> >     Newline at end of file

Not required for cpp and not used for other files in codebase.


> On Jan. 28, 2014, 1:46 a.m., Adam B wrote:
> > src/slave/containerizer/isolators/cgroups/mem.cpp, line 420
> > <https://reviews.apache.org/r/16432/diff/4/?file=451183#file451183line420>
> >
> >     What if read.isNone()?
> >     If you assign this to 'usage' above the read.isSome() check, you could use it in the numify call instead of calling trim(read.get()) again.

Code removed.


> On Jan. 28, 2014, 1:46 a.m., Adam B wrote:
> > src/slave/containerizer/isolators/cgroups/mem.cpp, line 431
> > <https://reviews.apache.org/r/16432/diff/4/?file=451183#file451183line431>
> >
> >     What if 'usage' wasn't able to be read above (isNone)?

Code removed.


- Ian


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


On Jan. 27, 2014, 3:03 a.m., Ian Downes wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16432/
> -----------------------------------------------------------
> 
> (Updated Jan. 27, 2014, 3:03 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Chi Zhang, samya, and Vinod Kone.
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> Forth part of isolation refactor - cgroup isolators
> 
> CPU and Memory isolators using Linux cgroups.
> 
> This code was written by Chi Zhang <cz...@twitter.com> and based on the original cgroups_isolator.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am cf0c8c66e8dd21c2f5a2da22e5d5adb056353454 
>   src/slave/containerizer/isolators/cgroups/cgroups.hpp PRE-CREATION 
>   src/slave/containerizer/isolators/cgroups/cgroups.cpp PRE-CREATION 
>   src/slave/containerizer/isolators/cgroups/cpushare.hpp PRE-CREATION 
>   src/slave/containerizer/isolators/cgroups/cpushare.cpp PRE-CREATION 
>   src/slave/containerizer/isolators/cgroups/mem.hpp PRE-CREATION 
>   src/slave/containerizer/isolators/cgroups/mem.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/16432/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Ian Downes
> 
>

Re: Review Request 16432: Containerizer - cgroup isolators (part 4)

Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16432/#review32889
-----------------------------------------------------------

Ship it!


Looks good, just some minor nits.


src/slave/containerizer/isolators/cgroups/cgroups.cpp
<https://reviews.apache.org/r/16432/#comment61881>

    Why not assign "path::join(root, "test")" to a variable, if it's going to be used 3 times in this function?
    Isn't this test already performed in the cgroups_launcher? Do we really need it in both? (I guess that's what your TODO in the launcher is about.)



src/slave/containerizer/isolators/cgroups/cgroups.cpp
<https://reviews.apache.org/r/16432/#comment61883>

    Do you also want to log the info->cgroup string as above?



src/slave/containerizer/isolators/cgroups/cgroups.cpp
<https://reviews.apache.org/r/16432/#comment61884>

    s/cgorups/cgroups/



src/slave/containerizer/isolators/cgroups/cpushare.hpp
<https://reviews.apache.org/r/16432/#comment61886>

    Shouldn't need both <mesos/resources.hpp> and "mesos/resources.hpp"



src/slave/containerizer/isolators/cgroups/cpushare.hpp
<https://reviews.apache.org/r/16432/#comment61930>

    Please use 'subsystem' instead of 'subsystem_' for the member variable.



src/slave/containerizer/isolators/cgroups/cpushare.hpp
<https://reviews.apache.org/r/16432/#comment61887>

    Just because cgroups uses "cpuacct", does that mean we need to keep that archaic name? I think cpuAccountHierarchy would be easier to read. Or cpuShareHierarchy if you'd rather.



src/slave/containerizer/isolators/cgroups/cpushare.cpp
<https://reviews.apache.org/r/16432/#comment61890>

    Go ahead and use local variable foo_ and foo = foo_.get() for cpuacctHierarchy here like you did above for hierarchy_.
    We prefer not to use this-> to workaround shadowed variables.



src/slave/containerizer/isolators/cgroups/cpushare.cpp
<https://reviews.apache.org/r/16432/#comment61894>

    Please comment somewhere that CFS stands for "Completely Fair Scheduler". My first google search for "cfs cgroups" suggested "Chronic Fatigue Syndrome" support groups. :P



src/slave/containerizer/isolators/cgroups/cpushare.cpp
<https://reviews.apache.org/r/16432/#comment61901>

    isNone() instead of !isSome()?



src/slave/containerizer/isolators/cgroups/cpushare.cpp
<https://reviews.apache.org/r/16432/#comment61913>

    No need for all this Option<>:: nonsense, you can just use "= Some(pid);", or even just "= pid;"



src/slave/containerizer/isolators/cgroups/cpushare.cpp
<https://reviews.apache.org/r/16432/#comment61917>

    s/hierarchy/cpuacctHierarchy/



src/slave/containerizer/isolators/cgroups/cpushare.cpp
<https://reviews.apache.org/r/16432/#comment61919>

    First "CHECK_NOTNULL(infos[containerId]);" ?



src/slave/containerizer/isolators/cgroups/cpushare.cpp
<https://reviews.apache.org/r/16432/#comment61918>

    isNone() instead of !isSome()



src/slave/containerizer/isolators/cgroups/cpushare.cpp
<https://reviews.apache.org/r/16432/#comment61922>

    Shadows the identical "double cpus = resources.cpus().get();" on line 249; you can probably remove this line and use the preexisting 'cpus' variable



src/slave/containerizer/isolators/cgroups/cpushare.cpp
<https://reviews.apache.org/r/16432/#comment61923>

    Can you chain these futures together with .then() to ensure that we destroy both of these hierarchies and delete the containerId/info from infos, even if the futures aren't immediately ready?



src/slave/containerizer/isolators/cgroups/cpushare.cpp
<https://reviews.apache.org/r/16432/#comment61926>

    Newline at end of file



src/slave/containerizer/isolators/cgroups/mem.hpp
<https://reviews.apache.org/r/16432/#comment61925>

    Apache License statement?



src/slave/containerizer/isolators/cgroups/mem.hpp
<https://reviews.apache.org/r/16432/#comment61928>

    alpha-order



src/slave/containerizer/isolators/cgroups/mem.hpp
<https://reviews.apache.org/r/16432/#comment61931>

    s/subsystem_/subsystem/



src/slave/containerizer/isolators/cgroups/mem.hpp
<https://reviews.apache.org/r/16432/#comment61932>

    If you want Doxygen to pick this up, you should either use triple-slash "/// @param foo bar" or double-star:
    /**
     * Description.
     * @param foo bar
     */



src/slave/containerizer/isolators/cgroups/mem.cpp
<https://reviews.apache.org/r/16432/#comment61933>

    alpha-order



src/slave/containerizer/isolators/cgroups/mem.cpp
<https://reviews.apache.org/r/16432/#comment61935>

    isNone



src/slave/containerizer/isolators/cgroups/mem.cpp
<https://reviews.apache.org/r/16432/#comment61941>

    isNone



src/slave/containerizer/isolators/cgroups/mem.cpp
<https://reviews.apache.org/r/16432/#comment61942>

    = pid;



src/slave/containerizer/isolators/cgroups/mem.cpp
<https://reviews.apache.org/r/16432/#comment61944>

    CHECK_NOTNULL(infos[containerId]); first?



src/slave/containerizer/isolators/cgroups/mem.cpp
<https://reviews.apache.org/r/16432/#comment61945>

    isNone()



src/slave/containerizer/isolators/cgroups/mem.cpp
<https://reviews.apache.org/r/16432/#comment61946>

    Please put the TODO on a new line



src/slave/containerizer/isolators/cgroups/mem.cpp
<https://reviews.apache.org/r/16432/#comment61947>

    Would it make sense to change the '<=' to '<'? I'd think you could still use limit_in_bytes if the new limit == current.



src/slave/containerizer/isolators/cgroups/mem.cpp
<https://reviews.apache.org/r/16432/#comment61948>

    Use .then() to ensure the proper cleanup occurs, even if destroy isn't immediately ready.



src/slave/containerizer/isolators/cgroups/mem.cpp
<https://reviews.apache.org/r/16432/#comment61968>

    What if read.isNone()?
    If you assign this to 'usage' above the read.isSome() check, you could use it in the numify call instead of calling trim(read.get()) again.



src/slave/containerizer/isolators/cgroups/mem.cpp
<https://reviews.apache.org/r/16432/#comment61970>

    What if 'usage' wasn't able to be read above (isNone)?



src/slave/containerizer/isolators/cgroups/mem.cpp
<https://reviews.apache.org/r/16432/#comment61971>

    Technically, it's ending namespace slave, then internal, mesos.


- Adam B


On Jan. 26, 2014, 7:03 p.m., Ian Downes wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16432/
> -----------------------------------------------------------
> 
> (Updated Jan. 26, 2014, 7:03 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Chi Zhang, samya, and Vinod Kone.
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> Forth part of isolation refactor - cgroup isolators
> 
> CPU and Memory isolators using Linux cgroups.
> 
> This code was written by Chi Zhang <cz...@twitter.com> and based on the original cgroups_isolator.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am cf0c8c66e8dd21c2f5a2da22e5d5adb056353454 
>   src/slave/containerizer/isolators/cgroups/cgroups.hpp PRE-CREATION 
>   src/slave/containerizer/isolators/cgroups/cgroups.cpp PRE-CREATION 
>   src/slave/containerizer/isolators/cgroups/cpushare.hpp PRE-CREATION 
>   src/slave/containerizer/isolators/cgroups/cpushare.cpp PRE-CREATION 
>   src/slave/containerizer/isolators/cgroups/mem.hpp PRE-CREATION 
>   src/slave/containerizer/isolators/cgroups/mem.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/16432/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Ian Downes
> 
>

Re: Review Request 16432: Containerizer - cgroup isolators (part 4)

Posted by Ian Downes <ia...@gmail.com>.

> On Jan. 28, 2014, 8:51 p.m., Vinod Kone wrote:
> > src/slave/containerizer/isolators/cgroups/cgroups.cpp, line 121
> > <https://reviews.apache.org/r/16432/diff/4/?file=451179#file451179line121>
> >
> >     +1
> >     
> >     How about moving this file up to "containerizer/cgroups.cpp" so that it can be used by both launcher and isolators?

Will be fixed later.


- Ian


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


On Feb. 6, 2014, 5:37 a.m., Ian Downes wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16432/
> -----------------------------------------------------------
> 
> (Updated Feb. 6, 2014, 5:37 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Chi Zhang, samya, and Vinod Kone.
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> Forth part of isolation refactor - cgroup isolators
> 
> CPU and Memory isolators using Linux cgroups.
> 
> This code was written by Chi Zhang <cz...@twitter.com> and based on the original cgroups_isolator.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am c30706846bca1fa3287291e39f46a23713ad1ba4 
>   src/slave/containerizer/isolators/cgroups/cgroups.hpp PRE-CREATION 
>   src/slave/containerizer/isolators/cgroups/cgroups.cpp PRE-CREATION 
>   src/slave/containerizer/isolators/cgroups/cpushare.hpp PRE-CREATION 
>   src/slave/containerizer/isolators/cgroups/cpushare.cpp PRE-CREATION 
>   src/slave/containerizer/isolators/cgroups/mem.hpp PRE-CREATION 
>   src/slave/containerizer/isolators/cgroups/mem.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/16432/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Ian Downes
> 
>

Re: Review Request 16432: Containerizer - cgroup isolators (part 4)

Posted by Ian Downes <ia...@gmail.com>.

> On Jan. 28, 2014, 8:51 p.m., Vinod Kone wrote:
> > src/slave/containerizer/isolators/cgroups/mem.hpp, line 43
> > <https://reviews.apache.org/r/16432/diff/4/?file=451182#file451182line43>
> >
> >     Why is this public?

Removed.


- Ian


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


On Jan. 27, 2014, 3:03 a.m., Ian Downes wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16432/
> -----------------------------------------------------------
> 
> (Updated Jan. 27, 2014, 3:03 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Chi Zhang, samya, and Vinod Kone.
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> Forth part of isolation refactor - cgroup isolators
> 
> CPU and Memory isolators using Linux cgroups.
> 
> This code was written by Chi Zhang <cz...@twitter.com> and based on the original cgroups_isolator.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am cf0c8c66e8dd21c2f5a2da22e5d5adb056353454 
>   src/slave/containerizer/isolators/cgroups/cgroups.hpp PRE-CREATION 
>   src/slave/containerizer/isolators/cgroups/cgroups.cpp PRE-CREATION 
>   src/slave/containerizer/isolators/cgroups/cpushare.hpp PRE-CREATION 
>   src/slave/containerizer/isolators/cgroups/cpushare.cpp PRE-CREATION 
>   src/slave/containerizer/isolators/cgroups/mem.hpp PRE-CREATION 
>   src/slave/containerizer/isolators/cgroups/mem.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/16432/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Ian Downes
> 
>

Re: Review Request 16432: Containerizer - cgroup isolators (part 4)

Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16432/#review33029
-----------------------------------------------------------


This has some issues with not properly doing clean up in an asynchronous manner. See Adam's comments. This got be fixed.

Also, I see a lot of CHECKs in this code where a return of Error() or Failure() would've been appropriate. Fix those too?


src/slave/containerizer/isolators/cgroups/cgroups.hpp
<https://reviews.apache.org/r/16432/#comment62185>

    Move this up to the initializer list.



src/slave/containerizer/isolators/cgroups/cgroups.hpp
<https://reviews.apache.org/r/16432/#comment62186>

    const?



src/slave/containerizer/isolators/cgroups/cgroups.hpp
<https://reviews.apache.org/r/16432/#comment62188>

    these should be in reverse order.



src/slave/containerizer/isolators/cgroups/cgroups.cpp
<https://reviews.apache.org/r/16432/#comment62195>

    +1
    
    How about moving this file up to "containerizer/cgroups.cpp" so that it can be used by both launcher and isolators?



src/slave/containerizer/isolators/cgroups/cgroups.cpp
<https://reviews.apache.org/r/16432/#comment62201>

    parameters should be aligned.



src/slave/containerizer/isolators/cgroups/cgroups.cpp
<https://reviews.apache.org/r/16432/#comment62202>

    parameters should be properly aligned.



src/slave/containerizer/isolators/cgroups/cgroups.cpp
<https://reviews.apache.org/r/16432/#comment62203>

    alignment.



src/slave/containerizer/isolators/cgroups/cgroups.cpp
<https://reviews.apache.org/r/16432/#comment62196>

    Reverse order.



src/slave/containerizer/isolators/cgroups/cpushare.hpp
<https://reviews.apache.org/r/16432/#comment62211>

    Why is this a public method?



src/slave/containerizer/isolators/cgroups/cpushare.hpp
<https://reviews.apache.org/r/16432/#comment62206>

    I wager this is because there is already a method by the same name. This has been our convention for name clashes.



src/slave/containerizer/isolators/cgroups/cpushare.hpp
<https://reviews.apache.org/r/16432/#comment62207>

    I wager this is because there is already a method by the same name. This has been our convention for name clashes.



src/slave/containerizer/isolators/cgroups/cpushare.cpp
<https://reviews.apache.org/r/16432/#comment62215>

    I actually wonder if it's cleaner to just have this class maintain a "vector<string> subsystems" instead of just "string subsystem" and inject cpuacct subsystem here.



src/slave/containerizer/isolators/cgroups/cpushare.cpp
<https://reviews.apache.org/r/16432/#comment62216>

    +1



src/slave/containerizer/isolators/cgroups/cpushare.cpp
<https://reviews.apache.org/r/16432/#comment62217>

    Please include the container id in the Failure message.
    
    More importantly, I don't think this should be Failure. See my comment in an earlier review on Launcher.



src/slave/containerizer/isolators/cgroups/cpushare.cpp
<https://reviews.apache.org/r/16432/#comment62218>

    How about
    
    CpushareCgroupInfo* info = 
      new CpushareCgroupInfo(containerId, flags.cgroup_root);



src/slave/containerizer/isolators/cgroups/cpushare.cpp
<https://reviews.apache.org/r/16432/#comment62224>

    you can combine these
    
    CpushareCgroupInfo* info = CHECK_NOTNULL(infos[containerId]);



src/slave/containerizer/isolators/cgroups/cpushare.cpp
<https://reviews.apache.org/r/16432/#comment62225>

    ditto. you can combine these.



src/slave/containerizer/isolators/cgroups/cpushare.cpp
<https://reviews.apache.org/r/16432/#comment62227>

    +1. kill this line.



src/slave/containerizer/isolators/cgroups/cpushare.cpp
<https://reviews.apache.org/r/16432/#comment62228>

    combine these two.



src/slave/containerizer/isolators/cgroups/cpushare.cpp
<https://reviews.apache.org/r/16432/#comment62229>

    ditto. combine these two.



src/slave/containerizer/isolators/cgroups/cpushare.cpp
<https://reviews.apache.org/r/16432/#comment62230>

    +1



src/slave/containerizer/isolators/cgroups/mem.hpp
<https://reviews.apache.org/r/16432/#comment62231>

    Why is this public?



src/slave/containerizer/isolators/cgroups/mem.cpp
<https://reviews.apache.org/r/16432/#comment62232>

    ditto. should not be a failure.



src/slave/containerizer/isolators/cgroups/mem.cpp
<https://reviews.apache.org/r/16432/#comment62234>

    MemCgroupInfo* info =
      new MemCgroupInfo(containerId, flags.cgroups_root);



src/slave/containerizer/isolators/cgroups/mem.cpp
<https://reviews.apache.org/r/16432/#comment62235>

    combine these two.



src/slave/containerizer/isolators/cgroups/mem.cpp
<https://reviews.apache.org/r/16432/#comment62238>

    combine these two.



src/slave/containerizer/isolators/cgroups/mem.cpp
<https://reviews.apache.org/r/16432/#comment62239>

    +1



src/slave/containerizer/isolators/cgroups/mem.cpp
<https://reviews.apache.org/r/16432/#comment62240>

    combine these two.



src/slave/containerizer/isolators/cgroups/mem.cpp
<https://reviews.apache.org/r/16432/#comment62242>

    s/ERROR/FATAL/ ?


- Vinod Kone


On Jan. 27, 2014, 3:03 a.m., Ian Downes wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16432/
> -----------------------------------------------------------
> 
> (Updated Jan. 27, 2014, 3:03 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Chi Zhang, samya, and Vinod Kone.
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> Forth part of isolation refactor - cgroup isolators
> 
> CPU and Memory isolators using Linux cgroups.
> 
> This code was written by Chi Zhang <cz...@twitter.com> and based on the original cgroups_isolator.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am cf0c8c66e8dd21c2f5a2da22e5d5adb056353454 
>   src/slave/containerizer/isolators/cgroups/cgroups.hpp PRE-CREATION 
>   src/slave/containerizer/isolators/cgroups/cgroups.cpp PRE-CREATION 
>   src/slave/containerizer/isolators/cgroups/cpushare.hpp PRE-CREATION 
>   src/slave/containerizer/isolators/cgroups/cpushare.cpp PRE-CREATION 
>   src/slave/containerizer/isolators/cgroups/mem.hpp PRE-CREATION 
>   src/slave/containerizer/isolators/cgroups/mem.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/16432/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Ian Downes
> 
>

Re: Review Request 16432: Containerizer - cgroup isolators (part 4)

Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16432/#review33920
-----------------------------------------------------------

Ship it!



src/slave/containerizer/isolators/cgroups/cgroups.hpp
<https://reviews.apache.org/r/16432/#comment63705>

    What's with snake case?



src/slave/containerizer/isolators/cgroups/cgroups.hpp
<https://reviews.apache.org/r/16432/#comment63738>

    It looks like 'setup' is basically "(1) make sure cgroups are supported then (2) make sure hierarchy is mounted with subsystem and (3) make sure there is a root cgroup". I imagine that could just be moved into our cgroups library and then it can be used by the launcher too.



src/slave/containerizer/isolators/cgroups/cgroups.hpp
<https://reviews.apache.org/r/16432/#comment63737>

    It seems like these two can and should just be functions on CgroupInfo.



src/slave/containerizer/isolators/cgroups/cgroups.hpp
<https://reviews.apache.org/r/16432/#comment63706>

    Spelling (and snake case).



src/slave/containerizer/isolators/cgroups/cgroups.hpp
<https://reviews.apache.org/r/16432/#comment63707>

    Kill newline (and looking back I realized you've done this in a fair number of files, please kill trailing newlines elsewhere too).



src/slave/containerizer/isolators/cgroups/cpushare.hpp
<https://reviews.apache.org/r/16432/#comment63708>

    virtual



src/slave/containerizer/isolators/cgroups/cpushare.hpp
<https://reviews.apache.org/r/16432/#comment63709>

    virtual



src/slave/containerizer/isolators/cgroups/cpushare.hpp
<https://reviews.apache.org/r/16432/#comment63710>

    virtual



src/slave/containerizer/isolators/cgroups/cpushare.hpp
<https://reviews.apache.org/r/16432/#comment63711>

    const?



src/slave/containerizer/isolators/cgroups/cpushare.cpp
<https://reviews.apache.org/r/16432/#comment63712>

    Newline between these two please.



src/slave/containerizer/isolators/cgroups/cpushare.cpp
<https://reviews.apache.org/r/16432/#comment63713>

    s/flags_/_flags/



src/slave/containerizer/isolators/cgroups/cpushare.cpp
<https://reviews.apache.org/r/16432/#comment63714>

    You shouldn't need to wrap this with 'string'.



src/slave/containerizer/isolators/cgroups/cpushare.cpp
<https://reviews.apache.org/r/16432/#comment63716>

    s/exiting/exited/



src/slave/containerizer/isolators/cgroups/cpushare.cpp
<https://reviews.apache.org/r/16432/#comment63717>

    Wrap at 80 columns please.



src/slave/containerizer/isolators/cgroups/cpushare.cpp
<https://reviews.apache.org/r/16432/#comment63727>

    Anymore cleanup need to be done in this case?



src/slave/containerizer/isolators/cgroups/cpushare.cpp
<https://reviews.apache.org/r/16432/#comment63728>

    See comment above.



src/slave/containerizer/isolators/cgroups/cpushare.cpp
<https://reviews.apache.org/r/16432/#comment63718>

    You can do the CHECK_NOTNULL from above right here.



src/slave/containerizer/isolators/cgroups/cpushare.cpp
<https://reviews.apache.org/r/16432/#comment63719>

    You could fire these off in parallel and do a collect too.



src/slave/containerizer/isolators/cgroups/mem.hpp
<https://reviews.apache.org/r/16432/#comment63720>

    virtual



src/slave/containerizer/isolators/cgroups/mem.hpp
<https://reviews.apache.org/r/16432/#comment63721>

    virtual



src/slave/containerizer/isolators/cgroups/mem.hpp
<https://reviews.apache.org/r/16432/#comment63722>

    virtual



src/slave/containerizer/isolators/cgroups/mem.hpp
<https://reviews.apache.org/r/16432/#comment63723>

    Add a newline above please.



src/slave/containerizer/isolators/cgroups/mem.hpp
<https://reviews.apache.org/r/16432/#comment63724>

    const?



src/slave/containerizer/isolators/cgroups/mem.cpp
<https://reviews.apache.org/r/16432/#comment63725>

    Newline between these please.



src/slave/containerizer/isolators/cgroups/mem.cpp
<https://reviews.apache.org/r/16432/#comment63726>

    Don't think 'string' is needed here.



src/slave/containerizer/isolators/cgroups/mem.cpp
<https://reviews.apache.org/r/16432/#comment63729>

    Anymore cleanup need to be done here?



src/slave/containerizer/isolators/cgroups/mem.cpp
<https://reviews.apache.org/r/16432/#comment63730>

    s/ */* /



src/slave/containerizer/isolators/cgroups/mem.cpp
<https://reviews.apache.org/r/16432/#comment63731>

    Move CHECK_NOTNULL down here from above.



src/slave/containerizer/isolators/cgroups/mem.cpp
<https://reviews.apache.org/r/16432/#comment63732>

    s/ */* /



src/slave/containerizer/isolators/cgroups/mem.cpp
<https://reviews.apache.org/r/16432/#comment63733>

    Shouldn't need 'string' here.



src/slave/containerizer/isolators/cgroups/mem.cpp
<https://reviews.apache.org/r/16432/#comment63734>

    Please put the newline back above this comment.



src/slave/containerizer/isolators/cgroups/mem.cpp
<https://reviews.apache.org/r/16432/#comment63735>

    Please wrap at 80 columns.



src/slave/containerizer/isolators/cgroups/mem.cpp
<https://reviews.apache.org/r/16432/#comment63736>

    Please wrap at 80 columns.


- Benjamin Hindman


On Feb. 7, 2014, 3:38 a.m., Ian Downes wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16432/
> -----------------------------------------------------------
> 
> (Updated Feb. 7, 2014, 3:38 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Chi Zhang, samya, and Vinod Kone.
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> Forth part of isolation refactor - cgroup isolators
> 
> CPU and Memory isolators using Linux cgroups.
> 
> This code was written by Chi Zhang <cz...@twitter.com> and based on the original cgroups_isolator.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am c30706846bca1fa3287291e39f46a23713ad1ba4 
>   src/slave/containerizer/isolators/cgroups/cgroups.hpp PRE-CREATION 
>   src/slave/containerizer/isolators/cgroups/cgroups.cpp PRE-CREATION 
>   src/slave/containerizer/isolators/cgroups/cpushare.hpp PRE-CREATION 
>   src/slave/containerizer/isolators/cgroups/cpushare.cpp PRE-CREATION 
>   src/slave/containerizer/isolators/cgroups/mem.hpp PRE-CREATION 
>   src/slave/containerizer/isolators/cgroups/mem.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/16432/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Ian Downes
> 
>

Re: Review Request 16432: Containerizer - cgroup isolators (part 4)

Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16432/
-----------------------------------------------------------

(Updated Feb. 12, 2014, 12:21 a.m.)


Review request for mesos, Benjamin Hindman, Chi Zhang, samya, and Vinod Kone.


Changes
-------

Adressed Ben's comments.


Repository: mesos-git


Description
-------

Forth part of isolation refactor - cgroup isolators

CPU and Memory isolators using Linux cgroups.

This code was written by Chi Zhang <cz...@twitter.com> and based on the original cgroups_isolator.


Diffs (updated)
-----

  src/Makefile.am c30706846bca1fa3287291e39f46a23713ad1ba4 
  src/slave/containerizer/isolators/cgroups/cpushare.hpp PRE-CREATION 
  src/slave/containerizer/isolators/cgroups/cpushare.cpp PRE-CREATION 
  src/slave/containerizer/isolators/cgroups/mem.hpp PRE-CREATION 
  src/slave/containerizer/isolators/cgroups/mem.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/16432/diff/


Testing
-------


Thanks,

Ian Downes

Re: Review Request 16432: Containerizer - cgroup isolators (part 4)

Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16432/
-----------------------------------------------------------

(Updated Feb. 7, 2014, 3:38 a.m.)


Review request for mesos, Benjamin Hindman, Chi Zhang, samya, and Vinod Kone.


Changes
-------

Addressed Vinod's comments.


Repository: mesos-git


Description
-------

Forth part of isolation refactor - cgroup isolators

CPU and Memory isolators using Linux cgroups.

This code was written by Chi Zhang <cz...@twitter.com> and based on the original cgroups_isolator.


Diffs (updated)
-----

  src/Makefile.am c30706846bca1fa3287291e39f46a23713ad1ba4 
  src/slave/containerizer/isolators/cgroups/cgroups.hpp PRE-CREATION 
  src/slave/containerizer/isolators/cgroups/cgroups.cpp PRE-CREATION 
  src/slave/containerizer/isolators/cgroups/cpushare.hpp PRE-CREATION 
  src/slave/containerizer/isolators/cgroups/cpushare.cpp PRE-CREATION 
  src/slave/containerizer/isolators/cgroups/mem.hpp PRE-CREATION 
  src/slave/containerizer/isolators/cgroups/mem.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/16432/diff/


Testing
-------


Thanks,

Ian Downes

Re: Review Request 16432: Containerizer - cgroup isolators (part 4)

Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16432/#review33865
-----------------------------------------------------------

Ship it!



src/slave/containerizer/isolators/cgroups/cgroups.cpp
<https://reviews.apache.org/r/16432/#comment63522>

    does it fit on the same line?



src/slave/containerizer/isolators/cgroups/cgroups.cpp
<https://reviews.apache.org/r/16432/#comment63523>

    ditto.



src/slave/containerizer/isolators/cgroups/cpushare.cpp
<https://reviews.apache.org/r/16432/#comment63524>

    s/VLOG/LOG(INFO)/ ?



src/slave/containerizer/isolators/cgroups/mem.cpp
<https://reviews.apache.org/r/16432/#comment63528>

    ditto.


- Vinod Kone


On Feb. 6, 2014, 5:37 a.m., Ian Downes wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16432/
> -----------------------------------------------------------
> 
> (Updated Feb. 6, 2014, 5:37 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Chi Zhang, samya, and Vinod Kone.
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> Forth part of isolation refactor - cgroup isolators
> 
> CPU and Memory isolators using Linux cgroups.
> 
> This code was written by Chi Zhang <cz...@twitter.com> and based on the original cgroups_isolator.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am c30706846bca1fa3287291e39f46a23713ad1ba4 
>   src/slave/containerizer/isolators/cgroups/cgroups.hpp PRE-CREATION 
>   src/slave/containerizer/isolators/cgroups/cgroups.cpp PRE-CREATION 
>   src/slave/containerizer/isolators/cgroups/cpushare.hpp PRE-CREATION 
>   src/slave/containerizer/isolators/cgroups/cpushare.cpp PRE-CREATION 
>   src/slave/containerizer/isolators/cgroups/mem.hpp PRE-CREATION 
>   src/slave/containerizer/isolators/cgroups/mem.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/16432/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Ian Downes
> 
>

Re: Review Request 16432: Containerizer - cgroup isolators (part 4)

Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16432/
-----------------------------------------------------------

(Updated Feb. 6, 2014, 5:37 a.m.)


Review request for mesos, Benjamin Hindman, Chi Zhang, samya, and Vinod Kone.


Changes
-------

Minor cleanups.


Repository: mesos-git


Description
-------

Forth part of isolation refactor - cgroup isolators

CPU and Memory isolators using Linux cgroups.

This code was written by Chi Zhang <cz...@twitter.com> and based on the original cgroups_isolator.


Diffs
-----

  src/Makefile.am c30706846bca1fa3287291e39f46a23713ad1ba4 
  src/slave/containerizer/isolators/cgroups/cgroups.hpp PRE-CREATION 
  src/slave/containerizer/isolators/cgroups/cgroups.cpp PRE-CREATION 
  src/slave/containerizer/isolators/cgroups/cpushare.hpp PRE-CREATION 
  src/slave/containerizer/isolators/cgroups/cpushare.cpp PRE-CREATION 
  src/slave/containerizer/isolators/cgroups/mem.hpp PRE-CREATION 
  src/slave/containerizer/isolators/cgroups/mem.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/16432/diff/


Testing
-------


Thanks,

Ian Downes

Re: Review Request 16432: Containerizer - cgroup isolators (part 4)

Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16432/
-----------------------------------------------------------

(Updated Feb. 1, 2014, 4:06 a.m.)


Review request for mesos, Benjamin Hindman, Chi Zhang, samya, and Vinod Kone.


Changes
-------

Rebased and addressed comments.


Repository: mesos-git


Description
-------

Forth part of isolation refactor - cgroup isolators

CPU and Memory isolators using Linux cgroups.

This code was written by Chi Zhang <cz...@twitter.com> and based on the original cgroups_isolator.


Diffs (updated)
-----

  src/Makefile.am c30706846bca1fa3287291e39f46a23713ad1ba4 
  src/slave/containerizer/isolators/cgroups/cgroups.hpp PRE-CREATION 
  src/slave/containerizer/isolators/cgroups/cgroups.cpp PRE-CREATION 
  src/slave/containerizer/isolators/cgroups/cpushare.hpp PRE-CREATION 
  src/slave/containerizer/isolators/cgroups/cpushare.cpp PRE-CREATION 
  src/slave/containerizer/isolators/cgroups/mem.hpp PRE-CREATION 
  src/slave/containerizer/isolators/cgroups/mem.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/16432/diff/


Testing
-------


Thanks,

Ian Downes

Re: Review Request 16432: Containerizer - cgroup isolators (part 4)

Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16432/
-----------------------------------------------------------

(Updated Jan. 27, 2014, 3:03 a.m.)


Review request for mesos, Benjamin Hindman, Chi Zhang, samya, and Vinod Kone.


Changes
-------

Addressed review comments.

- This code updated by Chi Zhang.


Repository: mesos-git


Description
-------

Forth part of isolation refactor - cgroup isolators

CPU and Memory isolators using Linux cgroups.

This code was written by Chi Zhang <cz...@twitter.com> and based on the original cgroups_isolator.


Diffs (updated)
-----

  src/Makefile.am cf0c8c66e8dd21c2f5a2da22e5d5adb056353454 
  src/slave/containerizer/isolators/cgroups/cgroups.hpp PRE-CREATION 
  src/slave/containerizer/isolators/cgroups/cgroups.cpp PRE-CREATION 
  src/slave/containerizer/isolators/cgroups/cpushare.hpp PRE-CREATION 
  src/slave/containerizer/isolators/cgroups/cpushare.cpp PRE-CREATION 
  src/slave/containerizer/isolators/cgroups/mem.hpp PRE-CREATION 
  src/slave/containerizer/isolators/cgroups/mem.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/16432/diff/


Testing
-------


Thanks,

Ian Downes

Re: Review Request 16432: Containerizer - cgroup isolators (part 4)

Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16432/
-----------------------------------------------------------

(Updated Jan. 21, 2014, 7:35 p.m.)


Review request for mesos, Benjamin Hindman, Chi Zhang, samya, and Vinod Kone.


Summary (updated)
-----------------

Containerizer - cgroup isolators (part 4)


Repository: mesos-git


Description
-------

Forth part of isolation refactor - cgroup isolators

CPU and Memory isolators using Linux cgroups.

This code was written by Chi Zhang <cz...@twitter.com> and based on the original cgroups_isolator.


Diffs
-----

  src/slave/container/isolators/cgroups/cgroups.hpp PRE-CREATION 
  src/slave/container/isolators/cgroups/cgroups.cpp PRE-CREATION 
  src/slave/container/isolators/cgroups/cpushare.hpp PRE-CREATION 
  src/slave/container/isolators/cgroups/cpushare.cpp PRE-CREATION 
  src/slave/container/isolators/cgroups/mem.hpp PRE-CREATION 
  src/slave/container/isolators/cgroups/mem.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/16432/diff/


Testing
-------


Thanks,

Ian Downes

Re: Review Request 16432: Fourth part of isolation refactor - cgroup isolators

Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16432/
-----------------------------------------------------------

(Updated Jan. 21, 2014, 7:26 p.m.)


Review request for mesos, Benjamin Hindman, Chi Zhang, samya, and Vinod Kone.


Changes
-------

Add Vinod.


Repository: mesos-git


Description
-------

Forth part of isolation refactor - cgroup isolators

CPU and Memory isolators using Linux cgroups.

This code was written by Chi Zhang <cz...@twitter.com> and based on the original cgroups_isolator.


Diffs
-----

  src/slave/container/isolators/cgroups/cgroups.hpp PRE-CREATION 
  src/slave/container/isolators/cgroups/cgroups.cpp PRE-CREATION 
  src/slave/container/isolators/cgroups/cpushare.hpp PRE-CREATION 
  src/slave/container/isolators/cgroups/cpushare.cpp PRE-CREATION 
  src/slave/container/isolators/cgroups/mem.hpp PRE-CREATION 
  src/slave/container/isolators/cgroups/mem.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/16432/diff/


Testing
-------


Thanks,

Ian Downes

Re: Review Request 16432: Fourth part of isolation refactor - cgroup isolators

Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16432/
-----------------------------------------------------------

(Updated Jan. 17, 2014, 1:36 a.m.)


Review request for mesos, Benjamin Hindman, Chi Zhang, and samya.


Repository: mesos-git


Description
-------

Forth part of isolation refactor - cgroup isolators

CPU and Memory isolators using Linux cgroups.

This code was written by Chi Zhang <cz...@twitter.com> and based on the original cgroups_isolator.


Diffs (updated)
-----

  src/slave/container/isolators/cgroups/cgroups.hpp PRE-CREATION 
  src/slave/container/isolators/cgroups/cgroups.cpp PRE-CREATION 
  src/slave/container/isolators/cgroups/cpushare.hpp PRE-CREATION 
  src/slave/container/isolators/cgroups/cpushare.cpp PRE-CREATION 
  src/slave/container/isolators/cgroups/mem.hpp PRE-CREATION 
  src/slave/container/isolators/cgroups/mem.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/16432/diff/


Testing
-------


Thanks,

Ian Downes

Re: Review Request 16432: Fourth part of isolation refactor - cgroup isolators

Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16432/
-----------------------------------------------------------

(Updated Jan. 17, 2014, 1:34 a.m.)


Review request for mesos, Benjamin Hindman, Chi Zhang, and samya.


Repository: mesos-git


Description
-------

Forth part of isolation refactor - cgroup isolators

CPU and Memory isolators using Linux cgroups.

This code was written by Chi Zhang <cz...@twitter.com> and based on the original cgroups_isolator.


Diffs
-----

  src/slave/container/isolators/cgroups/cgroups.hpp PRE-CREATION 
  src/slave/container/isolators/cgroups/cgroups.cpp PRE-CREATION 
  src/slave/container/isolators/cgroups/cpushare.hpp PRE-CREATION 
  src/slave/container/isolators/cgroups/cpushare.cpp PRE-CREATION 
  src/slave/container/isolators/cgroups/mem.hpp PRE-CREATION 
  src/slave/container/isolators/cgroups/mem.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/16432/diff/


Testing
-------


Thanks,

Ian Downes

Re: Review Request 16432: Fourth part of isolation refactor - cgroup isolators

Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16432/
-----------------------------------------------------------

(Updated Jan. 17, 2014, 1:33 a.m.)


Review request for mesos, Benjamin Hindman, Chi Zhang, and samya.


Changes
-------

Rebased plus implementation of recover() and other fixes.


Repository: mesos-git


Description
-------

Forth part of isolation refactor - cgroup isolators

CPU and Memory isolators using Linux cgroups.

This code was written by Chi Zhang <cz...@twitter.com> and based on the original cgroups_isolator.


Diffs (updated)
-----

  src/slave/container/isolators/cgroups/cgroups.hpp PRE-CREATION 
  src/slave/container/isolators/cgroups/cgroups.cpp PRE-CREATION 
  src/slave/container/isolators/cgroups/cpushare.hpp PRE-CREATION 
  src/slave/container/isolators/cgroups/cpushare.cpp PRE-CREATION 
  src/slave/container/isolators/cgroups/mem.hpp PRE-CREATION 
  src/slave/container/isolators/cgroups/mem.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/16432/diff/


Testing
-------


Thanks,

Ian Downes

Re: Review Request 16432: Fourth part of isolation refactor - cgroup isolators

Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16432/
-----------------------------------------------------------

(Updated Dec. 21, 2013, 6:57 a.m.)


Review request for mesos, Benjamin Hindman, Chi Zhang, and samya.


Summary (updated)
-----------------

Fourth part of isolation refactor - cgroup isolators


Repository: mesos-git


Description
-------

Forth part of isolation refactor - cgroup isolators

CPU and Memory isolators using Linux cgroups.

This code was written by Chi Zhang <cz...@twitter.com> and based on the original cgroups_isolator.


Diffs
-----

  src/Makefile.am 42dafbcd6e2d4f67d2705c5a2bea6ec6a0f4fcc1 
  src/slave/container/isolators/cgroups/cgroups.hpp PRE-CREATION 
  src/slave/container/isolators/cgroups/cgroups.cpp PRE-CREATION 
  src/slave/container/isolators/cgroups/cpushare.hpp PRE-CREATION 
  src/slave/container/isolators/cgroups/cpushare.cpp PRE-CREATION 
  src/slave/container/isolators/cgroups/mem.hpp PRE-CREATION 
  src/slave/container/isolators/cgroups/mem.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/16432/diff/


Testing
-------


Thanks,

Ian Downes