You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Jie Yu <yu...@gmail.com> on 2012/06/18 20:24:28 UTC

Review Request: Add basic APIs to use Linux control groups (cgroups).

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

Review request for mesos, Benjamin Hindman and Vinod Kone.


Description
-------

Add simple utility function to control Linux control groups (cgroups).


Diffs
-----

  src/Makefile.am 8760f59 
  src/linux/cgroups.hpp PRE-CREATION 
  src/linux/cgroups.cpp PRE-CREATION 
  src/tests/cgroups_tests.cpp PRE-CREATION 

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


Testing
-------

On Linux machine, make check.

(Root permission required. It is detected automatically, thanks to the gtest hack).


Thanks,

Jie Yu

Re: Review Request: Add basic APIs to use Linux control groups (cgroups).

Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5394/#review8409
-----------------------------------------------------------



src/linux/cgroups.cpp
<https://reviews.apache.org/r/5394/#comment18091>

    Done.



src/linux/cgroups.cpp
<https://reviews.apache.org/r/5394/#comment18090>

    Done.



src/linux/cgroups.cpp
<https://reviews.apache.org/r/5394/#comment18092>

    Done.



src/linux/cgroups.cpp
<https://reviews.apache.org/r/5394/#comment18093>

    Done.



src/linux/cgroups.cpp
<https://reviews.apache.org/r/5394/#comment18094>

    Done.



src/linux/cgroups.cpp
<https://reviews.apache.org/r/5394/#comment18095>

    Done.



src/linux/cgroups.cpp
<https://reviews.apache.org/r/5394/#comment18096>

    Done. Will refactor the code later (using the new set stuff).



src/linux/cgroups.cpp
<https://reviews.apache.org/r/5394/#comment18097>

    Totally agreed. Refactor this code later.



src/linux/cgroups.cpp
<https://reviews.apache.org/r/5394/#comment18098>

    Done.



src/linux/cgroups.cpp
<https://reviews.apache.org/r/5394/#comment18099>

    Done.



src/linux/cgroups.cpp
<https://reviews.apache.org/r/5394/#comment18100>

    Done.



src/linux/cgroups.cpp
<https://reviews.apache.org/r/5394/#comment18101>

    Done.



src/linux/cgroups.cpp
<https://reviews.apache.org/r/5394/#comment18102>

    Done.


- Jie Yu


On June 19, 2012, 4:52 a.m., Jie Yu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5394/
> -----------------------------------------------------------
> 
> (Updated June 19, 2012, 4:52 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Vinod Kone.
> 
> 
> Description
> -------
> 
> Add simple utility function to control Linux control groups (cgroups).
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am 8760f59 
>   src/linux/cgroups.hpp PRE-CREATION 
>   src/linux/cgroups.cpp PRE-CREATION 
>   src/tests/cgroups_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/5394/diff/
> 
> 
> Testing
> -------
> 
> On Linux machine, make check.
> 
> (Root permission required. It is detected automatically, thanks to the gtest hack).
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request: Add basic APIs to use Linux control groups (cgroups).

Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5394/
-----------------------------------------------------------

(Updated July 3, 2012, 12:05 a.m.)


Review request for mesos, Benjamin Hindman and Vinod Kone.


Changes
-------

Refactor the cgroups tests to have two test fixtures.


Description
-------

Add simple utility function to control Linux control groups (cgroups).


Diffs (updated)
-----

  src/Makefile.am 8760f59 
  src/linux/cgroups.hpp PRE-CREATION 
  src/linux/cgroups.cpp PRE-CREATION 
  src/tests/cgroups_tests.cpp PRE-CREATION 

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


Testing
-------

On Linux machine, make check.

(Root permission required. It is detected automatically, thanks to the gtest hack).


Thanks,

Jie Yu

Re: Review Request: Add basic APIs to use Linux control groups (cgroups).

Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5394/
-----------------------------------------------------------

(Updated June 26, 2012, 4:37 p.m.)


Review request for mesos, Benjamin Hindman and Vinod Kone.


Changes
-------

Based on the new trunk. Fix a coding style issue.


Description
-------

Add simple utility function to control Linux control groups (cgroups).


Diffs (updated)
-----

  src/Makefile.am 8760f59 
  src/linux/cgroups.hpp PRE-CREATION 
  src/linux/cgroups.cpp PRE-CREATION 
  src/tests/cgroups_tests.cpp PRE-CREATION 

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


Testing
-------

On Linux machine, make check.

(Root permission required. It is detected automatically, thanks to the gtest hack).


Thanks,

Jie Yu

Re: Review Request: Add basic APIs to use Linux control groups (cgroups).

Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5394/
-----------------------------------------------------------

(Updated June 20, 2012, 5:37 a.m.)


Review request for mesos, Benjamin Hindman and Vinod Kone.


Changes
-------

Addressed Ben's review feedback.


Description
-------

Add simple utility function to control Linux control groups (cgroups).


Diffs (updated)
-----

  src/Makefile.am 8760f59 
  src/linux/cgroups.hpp PRE-CREATION 
  src/linux/cgroups.cpp PRE-CREATION 
  src/tests/cgroups_tests.cpp PRE-CREATION 

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


Testing
-------

On Linux machine, make check.

(Root permission required. It is detected automatically, thanks to the gtest hack).


Thanks,

Jie Yu

Re: Review Request: Add basic APIs to use Linux control groups (cgroups).

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

Ship it!


Awesome!


src/linux/cgroups.cpp
<https://reviews.apache.org/r/5394/#comment18076>

    Okay, I think it's time to take this out of the mesos::internal namespace, and just put it in the cgroups namespace.



src/linux/cgroups.cpp
<https://reviews.apache.org/r/5394/#comment18075>

    Liking the name better, but given that you're the variable name 'info' and 'infos' everywhere, I'm thinking the right name here is SubsystemInfo.



src/linux/cgroups.cpp
<https://reviews.apache.org/r/5394/#comment18077>

    Let's stick all these _* functions in an cgroups::internal namespace.



src/linux/cgroups.cpp
<https://reviews.apache.org/r/5394/#comment18078>

    s/mountVFS/mount/



src/linux/cgroups.cpp
<https://reviews.apache.org/r/5394/#comment18079>

    s/unmountVFS/unmount/



src/linux/cgroups.cpp
<https://reviews.apache.org/r/5394/#comment18080>

    return !disabled;



src/linux/cgroups.cpp
<https://reviews.apache.org/r/5394/#comment18081>

    Why do we need both cgroups::enabled(subsystems) and cgroups::subsystems()? It seems like if nothing more, one should be implemented in terms of the other.



src/linux/cgroups.cpp
<https://reviews.apache.org/r/5394/#comment18083>

    Add a comment here that you're basically trying to get the intersection of enabled subsystems in the system and mount options. It would be even cooler if we could also have exactly that code, something like:
    
    Try<set<string> > subsystems = cgroups::subsystems();
    
    if (subsystems.isError()) {
     ...;
    }
    
    Try<set<string> > options = hierarchyMountEntry.options();
    
    if (options.isError()) {
     ...;
    }
    
    return sets::intersect(subsystems.get(), options.get());



src/linux/cgroups.cpp
<https://reviews.apache.org/r/5394/#comment18085>

    Now I see why you created this. I think we need some set abstractions! How about a new header, sets.hpp, which provides things like 'intersection', 'union', etc (they can wrap the STL versions appropriately). 



src/linux/cgroups.cpp
<https://reviews.apache.org/r/5394/#comment18084>

    No need for explicit  '== false'.



src/linux/cgroups.cpp
<https://reviews.apache.org/r/5394/#comment18086>

    s/non/none/



src/linux/cgroups.cpp
<https://reviews.apache.org/r/5394/#comment18087>

    while ((node = fts_read(tree) != NULL) {



src/linux/cgroups.cpp
<https://reviews.apache.org/r/5394/#comment18088>

    if (errno != 0) {



src/linux/cgroups.cpp
<https://reviews.apache.org/r/5394/#comment18089>

    if (fts_close(tree) != 0) {


- Benjamin Hindman


On June 19, 2012, 4:52 a.m., Jie Yu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5394/
> -----------------------------------------------------------
> 
> (Updated June 19, 2012, 4:52 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Vinod Kone.
> 
> 
> Description
> -------
> 
> Add simple utility function to control Linux control groups (cgroups).
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am 8760f59 
>   src/linux/cgroups.hpp PRE-CREATION 
>   src/linux/cgroups.cpp PRE-CREATION 
>   src/tests/cgroups_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/5394/diff/
> 
> 
> Testing
> -------
> 
> On Linux machine, make check.
> 
> (Root permission required. It is detected automatically, thanks to the gtest hack).
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request: Add basic APIs to use Linux control groups (cgroups).

Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5394/
-----------------------------------------------------------

(Updated June 19, 2012, 4:52 a.m.)


Review request for mesos, Benjamin Hindman and Vinod Kone.


Changes
-------

Change test names to adapt to the new gtest hack.


Description
-------

Add simple utility function to control Linux control groups (cgroups).


Diffs (updated)
-----

  src/Makefile.am 8760f59 
  src/linux/cgroups.hpp PRE-CREATION 
  src/linux/cgroups.cpp PRE-CREATION 
  src/tests/cgroups_tests.cpp PRE-CREATION 

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


Testing
-------

On Linux machine, make check.

(Root permission required. It is detected automatically, thanks to the gtest hack).


Thanks,

Jie Yu

Re: Review Request: Add basic APIs to use Linux control groups (cgroups).

Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5394/
-----------------------------------------------------------

(Updated June 18, 2012, 6:29 p.m.)


Review request for mesos, Benjamin Hindman and Vinod Kone.


Changes
-------

The newest version.


Description
-------

Add simple utility function to control Linux control groups (cgroups).


Diffs (updated)
-----

  src/Makefile.am 8760f59 
  src/linux/cgroups.hpp PRE-CREATION 
  src/linux/cgroups.cpp PRE-CREATION 
  src/tests/cgroups_tests.cpp PRE-CREATION 

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


Testing
-------

On Linux machine, make check.

(Root permission required. It is detected automatically, thanks to the gtest hack).


Thanks,

Jie Yu