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/05/24 00:30:38 UTC
Review Request: Adding interfaces to control cgroups,
mount/unmount file systems.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5211/
-----------------------------------------------------------
Review request for mesos and Benjamin Hindman.
Summary
-------
This patch contains the following work:
1) Adding interfaces to control linux cgroups directly (see src/linux/cgroups.h|cpp)
2) Adding interfaces to mount/unmount file systems on linux (see src/linux/mount.h|cpp)
3) Adding more interfaces in src/linux/proc to examine cgroups info and mount table.
Diffs
-----
src/Makefile.am d6eff8a
src/linux/cgroups.hpp PRE-CREATION
src/linux/cgroups.cpp PRE-CREATION
src/linux/mount.hpp PRE-CREATION
src/linux/mount.cpp PRE-CREATION
src/linux/proc.hpp 2f5c02a
src/linux/proc.cpp ed1beff
src/tests/cgroups_tests.cpp PRE-CREATION
src/tests/mount_tests.cpp PRE-CREATION
src/tests/proc_tests.cpp b9dcb91
Diff: https://reviews.apache.org/r/5211/diff
Testing
-------
Manual tests are done for those operations that need root permission.
Proc related tests are done in google test framework (ProcTest).
On linux machines, make check.
Thanks,
Jie
Re: Review Request: Adding interfaces to control cgroups,
mount/unmount file systems.
Posted by Jie Yu <yu...@gmail.com>.
> On 2012-05-23 23:50:20, Benjamin Hindman wrote:
> > src/linux/cgroups.cpp, line 55
> > <https://reviews.apache.org/r/5211/diff/2/?file=110054#file110054line55>
> >
> > It would be great to include the mount error too (for easier debugging by the human trying to use this stuff). For example, maybe:
> >
> > return Try<bool>::error("Failed to create hierarchy " + hierarchy + ": " + result.error());
> >
> > I think the error will include the "Failed to mount" bit, so no need to repeat that here (in fact, in many cases you can get away with something like 'return Try<bool>::error(result.error());'.
Done.
> On 2012-05-23 23:50:20, Benjamin Hindman wrote:
> > src/linux/cgroups.cpp, line 65
> > <https://reviews.apache.org/r/5211/diff/2/?file=110054#file110054line65>
> >
> > We don't use snake case, except when working with protobuf generated code: s/unmount_rc/result/ is probably better.
> >
> > Also, does it make sense to make the flags passed to unmount default to 0?
Done. Add another interface "unmount" without flags (google c++ style says default values are not recommended).
> On 2012-05-23 23:50:20, Benjamin Hindman wrote:
> > src/linux/cgroups.cpp, line 47
> > <https://reviews.apache.org/r/5211/diff/2/?file=110054#file110054line47>
> >
> > s/mount_rc/result
Done.
> On 2012-05-23 23:50:20, Benjamin Hindman wrote:
> > src/linux/cgroups.hpp, line 110
> > <https://reviews.apache.org/r/5211/diff/2/?file=110053#file110053line110>
> >
> > s/val/value/
Done.
> On 2012-05-23 23:50:20, Benjamin Hindman wrote:
> > src/linux/cgroups.cpp, line 67
> > <https://reviews.apache.org/r/5211/diff/2/?file=110054#file110054line67>
> >
> > Again, the actual unmount error would probably be helpful here.
Done.
> On 2012-05-23 23:50:20, Benjamin Hindman wrote:
> > src/linux/cgroups.cpp, line 111
> > <https://reviews.apache.org/r/5211/diff/2/?file=110054#file110054line111>
> >
> > s/val/value/
Done.
> On 2012-05-23 23:50:20, Benjamin Hindman wrote:
> > src/linux/mount.hpp, line 45
> > <https://reviews.apache.org/r/5211/diff/2/?file=110055#file110055line45>
> >
> > Any reason not to make this a 'const string&'?
The glibc interface uses void * here. I guess in some cases, the parameter is not a string (e.g. mount a very special file system).
> On 2012-05-23 23:50:20, Benjamin Hindman wrote:
> > src/linux/proc.hpp, line 221
> > <https://reviews.apache.org/r/5211/diff/2/?file=110057#file110057line221>
> >
> > Indent is +4 here please.
Done.
> On 2012-05-23 23:50:20, Benjamin Hindman wrote:
> > src/linux/proc.hpp, line 244
> > <https://reviews.apache.org/r/5211/diff/2/?file=110057#file110057line244>
> >
> > No need for the typedef, we prefer explicit types as much as possible.
Done.
> On 2012-05-23 23:50:20, Benjamin Hindman wrote:
> > src/linux/proc.hpp, line 246
> > <https://reviews.apache.org/r/5211/diff/2/?file=110057#file110057line246>
> >
> > You should just call this 'entries'.
Done.
> On 2012-05-23 23:50:20, Benjamin Hindman wrote:
> > src/linux/cgroups.cpp, line 80
> > <https://reviews.apache.org/r/5211/diff/2/?file=110054#file110054line80>
> >
> > This indentation should be +4 please.
Done.
> On 2012-05-23 23:50:20, Benjamin Hindman wrote:
> > src/linux/cgroups.hpp, line 51
> > <https://reviews.apache.org/r/5211/diff/2/?file=110053#file110053line51>
> >
> > s/WH/Wh
Done.
> On 2012-05-23 23:50:20, Benjamin Hindman wrote:
> > src/linux/cgroups.hpp, line 79
> > <https://reviews.apache.org/r/5211/diff/2/?file=110053#file110053line79>
> >
> > Maybe it makes sense to take a third argument that specifies whether to create the hierarchy if it doesn't exist defaulted to true?
I want to keep these interfaces as simple as possible. Other richer functions can be built on top of these primitives. What do you think?
> On 2012-05-23 23:50:20, Benjamin Hindman wrote:
> > src/linux/cgroups.hpp, line 39
> > <https://reviews.apache.org/r/5211/diff/2/?file=110053#file110053line39>
> >
> > What about calling it a 'control' instead? Has the cgroups community adopted any terminology themselves? Is it knob?
Done.
> On 2012-05-23 23:50:20, Benjamin Hindman wrote:
> > src/linux/cgroups.hpp, line 62
> > <https://reviews.apache.org/r/5211/diff/2/?file=110053#file110053line62>
> >
> > This is awesome, but what about two sub-namespaces: 'hierarchies' and 'controls'. E.g., from a clients perspective:
> >
> > cgroups::hierarchies::create(...);
> > cgroups::hierarchies::destroy(...);
> >
> > cgroups::create(hierarchy, cgroup);
> >
> > cgroups::controls::read(hierarchy, cgroup, control);
> > cgroups::controls::write(...);
I prefer to keep the current interfaces. The way you suggested makes me feel that hierarchies are contained in cgroups. But in fact, a cgroup is a node in a hierarchy. What do you think?
> On 2012-05-23 23:50:20, Benjamin Hindman wrote:
> > src/linux/cgroups.cpp, line 78
> > <https://reviews.apache.org/r/5211/diff/2/?file=110054#file110054line78>
> >
> > Why use ::mkdir? If you want to make sure it already doesn't exist, you should just do os::exists first. If it's because you want better error information, it probably makes sense to update os::mkdir and have it return a Try. Either way, you can defer this to a later review but add a comment (either here or in utils.hpp next to mkdir and rmdir and any of the other ones that were insufficient for your use case).
The reason I use ::mkdir (non-recursive version) here is the following: (in fact, we may need to add a non-recursive mkdir in common/utils.hpp)
Each directory in a hierarchy is treated as a cgroup. Say your hierarchy is mounted at "/sys/fs/cgroups". You want to create a cgroup "students/undergrads". If the parent cgroups "students/" does not exists, the recursive version of mkdir (the one I don't use) will create two cgroups: "students/" and "students/undergrads". Consider the situation in which "students/" is created successfully and "students/undergrads" is not. You may want to return an error and remove the cgroup "students/" to make the operation atomic. I don't think such a complex logic should be in these primitive functions (we can always build rich functionalities based on these primitives).
The take away here is: we should add a new function "utils::os::mkdir" in common/utils.hpp, which is just a wrapper of ::mkdir (non-recursieve).
What's you opinion?
> On 2012-05-23 23:50:20, Benjamin Hindman wrote:
> > src/linux/cgroups.hpp, line 35
> > <https://reviews.apache.org/r/5211/diff/2/?file=110053#file110053line35>
> >
> > Is there any reason for the hierarchy? For example, are any controls applied in a hierarchical fashion? Any details like this would be great to add for posterity.
Done.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5211/#review8069
-----------------------------------------------------------
On 2012-05-23 22:30:37, Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5211/
> -----------------------------------------------------------
>
> (Updated 2012-05-23 22:30:37)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Summary
> -------
>
> This patch contains the following work:
>
> 1) Adding interfaces to control linux cgroups directly (see src/linux/cgroups.h|cpp)
>
> 2) Adding interfaces to mount/unmount file systems on linux (see src/linux/mount.h|cpp)
>
> 3) Adding more interfaces in src/linux/proc to examine cgroups info and mount table.
>
>
> Diffs
> -----
>
> src/Makefile.am d6eff8a
> src/linux/cgroups.hpp PRE-CREATION
> src/linux/cgroups.cpp PRE-CREATION
> src/linux/mount.hpp PRE-CREATION
> src/linux/mount.cpp PRE-CREATION
> src/linux/proc.hpp 2f5c02a
> src/linux/proc.cpp ed1beff
> src/tests/cgroups_tests.cpp PRE-CREATION
> src/tests/mount_tests.cpp PRE-CREATION
> src/tests/proc_tests.cpp b9dcb91
>
> Diff: https://reviews.apache.org/r/5211/diff
>
>
> Testing
> -------
>
> Manual tests are done for those operations that need root permission.
>
> Proc related tests are done in google test framework (ProcTest).
>
> On linux machines, make check.
>
>
> Thanks,
>
> Jie
>
>
Re: Review Request: Adding interfaces to control cgroups,
mount/unmount file systems.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5211/#review8069
-----------------------------------------------------------
This is a review for everything but the proc.hpp/cpp stuff (which doesn't look like it's being used by this review anyway, so it might make sense to toss it into a different review).
src/linux/cgroups.hpp
<https://reviews.apache.org/r/5211/#comment17550>
Is there any reason for the hierarchy? For example, are any controls applied in a hierarchical fashion? Any details like this would be great to add for posterity.
src/linux/cgroups.hpp
<https://reviews.apache.org/r/5211/#comment17551>
What about calling it a 'control' instead? Has the cgroups community adopted any terminology themselves? Is it knob?
src/linux/cgroups.hpp
<https://reviews.apache.org/r/5211/#comment17552>
s/WH/Wh
src/linux/cgroups.hpp
<https://reviews.apache.org/r/5211/#comment17562>
This is awesome, but what about two sub-namespaces: 'hierarchies' and 'controls'. E.g., from a clients perspective:
cgroups::hierarchies::create(...);
cgroups::hierarchies::destroy(...);
cgroups::create(hierarchy, cgroup);
cgroups::controls::read(hierarchy, cgroup, control);
cgroups::controls::write(...);
src/linux/cgroups.hpp
<https://reviews.apache.org/r/5211/#comment17563>
Maybe it makes sense to take a third argument that specifies whether to create the hierarchy if it doesn't exist defaulted to true?
src/linux/cgroups.hpp
<https://reviews.apache.org/r/5211/#comment17561>
s/val/value/
src/linux/cgroups.cpp
<https://reviews.apache.org/r/5211/#comment17554>
s/mount_rc/result
src/linux/cgroups.cpp
<https://reviews.apache.org/r/5211/#comment17555>
It would be great to include the mount error too (for easier debugging by the human trying to use this stuff). For example, maybe:
return Try<bool>::error("Failed to create hierarchy " + hierarchy + ": " + result.error());
I think the error will include the "Failed to mount" bit, so no need to repeat that here (in fact, in many cases you can get away with something like 'return Try<bool>::error(result.error());'.
src/linux/cgroups.cpp
<https://reviews.apache.org/r/5211/#comment17556>
We don't use snake case, except when working with protobuf generated code: s/unmount_rc/result/ is probably better.
Also, does it make sense to make the flags passed to unmount default to 0?
src/linux/cgroups.cpp
<https://reviews.apache.org/r/5211/#comment17557>
Again, the actual unmount error would probably be helpful here.
src/linux/cgroups.cpp
<https://reviews.apache.org/r/5211/#comment17558>
Why use ::mkdir? If you want to make sure it already doesn't exist, you should just do os::exists first. If it's because you want better error information, it probably makes sense to update os::mkdir and have it return a Try. Either way, you can defer this to a later review but add a comment (either here or in utils.hpp next to mkdir and rmdir and any of the other ones that were insufficient for your use case).
src/linux/cgroups.cpp
<https://reviews.apache.org/r/5211/#comment17564>
This indentation should be +4 please.
src/linux/cgroups.cpp
<https://reviews.apache.org/r/5211/#comment17559>
s/val/value/
src/linux/mount.hpp
<https://reviews.apache.org/r/5211/#comment17553>
Any reason not to make this a 'const string&'?
src/linux/proc.hpp
<https://reviews.apache.org/r/5211/#comment17565>
Indent is +4 here please.
src/linux/proc.hpp
<https://reviews.apache.org/r/5211/#comment17566>
No need for the typedef, we prefer explicit types as much as possible.
src/linux/proc.hpp
<https://reviews.apache.org/r/5211/#comment17567>
You should just call this 'entries'.
- Benjamin
On 2012-05-23 22:30:37, Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5211/
> -----------------------------------------------------------
>
> (Updated 2012-05-23 22:30:37)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Summary
> -------
>
> This patch contains the following work:
>
> 1) Adding interfaces to control linux cgroups directly (see src/linux/cgroups.h|cpp)
>
> 2) Adding interfaces to mount/unmount file systems on linux (see src/linux/mount.h|cpp)
>
> 3) Adding more interfaces in src/linux/proc to examine cgroups info and mount table.
>
>
> Diffs
> -----
>
> src/Makefile.am d6eff8a
> src/linux/cgroups.hpp PRE-CREATION
> src/linux/cgroups.cpp PRE-CREATION
> src/linux/mount.hpp PRE-CREATION
> src/linux/mount.cpp PRE-CREATION
> src/linux/proc.hpp 2f5c02a
> src/linux/proc.cpp ed1beff
> src/tests/cgroups_tests.cpp PRE-CREATION
> src/tests/mount_tests.cpp PRE-CREATION
> src/tests/proc_tests.cpp b9dcb91
>
> Diff: https://reviews.apache.org/r/5211/diff
>
>
> Testing
> -------
>
> Manual tests are done for those operations that need root permission.
>
> Proc related tests are done in google test framework (ProcTest).
>
> On linux machines, make check.
>
>
> Thanks,
>
> Jie
>
>