You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Ben Mahler <be...@gmail.com> on 2013/08/29 23:03:46 UTC
Review Request 13903: Added a cgroups::memory namespace.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13903/
-----------------------------------------------------------
Review request for mesos, Benjamin Hindman, Jie Yu, and Vinod Kone.
Repository: mesos-git
Description
-------
I've adjusted the CgroupsIsolator code to use these new functions.
Note that this always sets the soft_limit_in_bytes. This should have been done in the first place, so let me know if you see a reason to conditionally set the soft limit as before.
Diffs
-----
src/linux/cgroups.hpp 3989712b5b76bc960f03fdf020a4e5b735ba5d80
src/linux/cgroups.cpp b97a89cafab12aa642645508ad465f714ee08f1b
src/slave/cgroups_isolator.cpp 676768e6b8bd13820467309814845257a9c47e02
Diff: https://reviews.apache.org/r/13903/diff/
Testing
-------
make check
Thanks,
Ben Mahler
Re: Review Request 13903: Added a cgroups::memory namespace.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13903/#review25808
-----------------------------------------------------------
src/linux/cgroups.hpp
<https://reviews.apache.org/r/13903/#comment50340>
I'll switch these to not be prefixed with set_ and use overloading instead to distinguish the reads vs writes.
- Ben Mahler
On Aug. 29, 2013, 9:03 p.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/13903/
> -----------------------------------------------------------
>
> (Updated Aug. 29, 2013, 9:03 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Jie Yu, and Vinod Kone.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> I've adjusted the CgroupsIsolator code to use these new functions.
>
> Note that this always sets the soft_limit_in_bytes. This should have been done in the first place, so let me know if you see a reason to conditionally set the soft limit as before.
>
>
> Diffs
> -----
>
> src/linux/cgroups.hpp 3989712b5b76bc960f03fdf020a4e5b735ba5d80
> src/linux/cgroups.cpp b97a89cafab12aa642645508ad465f714ee08f1b
> src/slave/cgroups_isolator.cpp 676768e6b8bd13820467309814845257a9c47e02
>
> Diff: https://reviews.apache.org/r/13903/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request 13903: Added a cgroups::memory namespace.
Posted by Ben Mahler <be...@gmail.com>.
> On Aug. 29, 2013, 10:13 p.m., Jie Yu wrote:
> > src/slave/cgroups_isolator.cpp, line 1226
> > <https://reviews.apache.org/r/13903/diff/1/?file=346415#file346415line1226>
> >
> > Do you plan to introduce: cgroups::memory::stat() as well in the future?
Definitely!
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13903/#review25762
-----------------------------------------------------------
On Aug. 29, 2013, 9:03 p.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/13903/
> -----------------------------------------------------------
>
> (Updated Aug. 29, 2013, 9:03 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Jie Yu, and Vinod Kone.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> I've adjusted the CgroupsIsolator code to use these new functions.
>
> Note that this always sets the soft_limit_in_bytes. This should have been done in the first place, so let me know if you see a reason to conditionally set the soft limit as before.
>
>
> Diffs
> -----
>
> src/linux/cgroups.hpp 3989712b5b76bc960f03fdf020a4e5b735ba5d80
> src/linux/cgroups.cpp b97a89cafab12aa642645508ad465f714ee08f1b
> src/slave/cgroups_isolator.cpp 676768e6b8bd13820467309814845257a9c47e02
>
> Diff: https://reviews.apache.org/r/13903/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request 13903: Added a cgroups::memory namespace.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13903/#review25762
-----------------------------------------------------------
Ship it!
Ship It!
src/slave/cgroups_isolator.cpp
<https://reviews.apache.org/r/13903/#comment50281>
Do you plan to introduce: cgroups::memory::stat() as well in the future?
- Jie Yu
On Aug. 29, 2013, 9:03 p.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/13903/
> -----------------------------------------------------------
>
> (Updated Aug. 29, 2013, 9:03 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Jie Yu, and Vinod Kone.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> I've adjusted the CgroupsIsolator code to use these new functions.
>
> Note that this always sets the soft_limit_in_bytes. This should have been done in the first place, so let me know if you see a reason to conditionally set the soft limit as before.
>
>
> Diffs
> -----
>
> src/linux/cgroups.hpp 3989712b5b76bc960f03fdf020a4e5b735ba5d80
> src/linux/cgroups.cpp b97a89cafab12aa642645508ad465f714ee08f1b
> src/slave/cgroups_isolator.cpp 676768e6b8bd13820467309814845257a9c47e02
>
> Diff: https://reviews.apache.org/r/13903/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request 13903: Added a cgroups::memory namespace.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13903/#review25879
-----------------------------------------------------------
Ship it!
- Vinod Kone
On Sept. 4, 2013, 2:43 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/13903/
> -----------------------------------------------------------
>
> (Updated Sept. 4, 2013, 2:43 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Vinod Kone.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> I've adjusted the CgroupsIsolator code to use these new functions.
>
> Note that this always sets the soft_limit_in_bytes. This should have been done in the first place, so let me know if you see a reason to conditionally set the soft limit as before.
>
>
> Diffs
> -----
>
> src/linux/cgroups.hpp 3989712b5b76bc960f03fdf020a4e5b735ba5d80
> src/linux/cgroups.cpp b97a89cafab12aa642645508ad465f714ee08f1b
> src/slave/cgroups_isolator.cpp 676768e6b8bd13820467309814845257a9c47e02
>
> Diff: https://reviews.apache.org/r/13903/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request 13903: Added a cgroups::memory namespace.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13903/
-----------------------------------------------------------
(Updated Sept. 4, 2013, 2:43 a.m.)
Review request for mesos, Benjamin Hindman and Vinod Kone.
Changes
-------
Vinod / benh reviews.
Repository: mesos-git
Description
-------
I've adjusted the CgroupsIsolator code to use these new functions.
Note that this always sets the soft_limit_in_bytes. This should have been done in the first place, so let me know if you see a reason to conditionally set the soft limit as before.
Diffs (updated)
-----
src/linux/cgroups.hpp 3989712b5b76bc960f03fdf020a4e5b735ba5d80
src/linux/cgroups.cpp b97a89cafab12aa642645508ad465f714ee08f1b
src/slave/cgroups_isolator.cpp 676768e6b8bd13820467309814845257a9c47e02
Diff: https://reviews.apache.org/r/13903/diff/
Testing
-------
make check
Thanks,
Ben Mahler
Re: Review Request 13903: Added a cgroups::memory namespace.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13903/#review25865
-----------------------------------------------------------
Ship it!
src/linux/cgroups.hpp
<https://reviews.apache.org/r/13903/#comment50431>
const &
src/linux/cgroups.hpp
<https://reviews.apache.org/r/13903/#comment50432>
const &
- Benjamin Hindman
On Aug. 31, 2013, 12:29 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/13903/
> -----------------------------------------------------------
>
> (Updated Aug. 31, 2013, 12:29 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Vinod Kone.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> I've adjusted the CgroupsIsolator code to use these new functions.
>
> Note that this always sets the soft_limit_in_bytes. This should have been done in the first place, so let me know if you see a reason to conditionally set the soft limit as before.
>
>
> Diffs
> -----
>
> src/linux/cgroups.hpp 3989712b5b76bc960f03fdf020a4e5b735ba5d80
> src/linux/cgroups.cpp b97a89cafab12aa642645508ad465f714ee08f1b
> src/slave/cgroups_isolator.cpp 676768e6b8bd13820467309814845257a9c47e02
>
> Diff: https://reviews.apache.org/r/13903/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request 13903: Added a cgroups::memory namespace.
Posted by Vinod Kone <vi...@gmail.com>.
> On Aug. 31, 2013, 5:55 a.m., Vinod Kone wrote:
> > src/slave/cgroups_isolator.cpp, line 1044
> > <https://reviews.apache.org/r/13903/diff/2/?file=347000#file347000line1044>
> >
> > Why not just do
> >
> > Bytes mem = Megabytes((uint64_t) resource.scalar().value()); ?
>
> Ben Mahler wrote:
> This would lose accuracy!
>
> Take the example of mem == 0.5:
> Megabytes((uint64_t) 0.5) == Megabytes(0)
good point!
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13903/#review25815
-----------------------------------------------------------
On Sept. 4, 2013, 2:43 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/13903/
> -----------------------------------------------------------
>
> (Updated Sept. 4, 2013, 2:43 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Vinod Kone.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> I've adjusted the CgroupsIsolator code to use these new functions.
>
> Note that this always sets the soft_limit_in_bytes. This should have been done in the first place, so let me know if you see a reason to conditionally set the soft limit as before.
>
>
> Diffs
> -----
>
> src/linux/cgroups.hpp 3989712b5b76bc960f03fdf020a4e5b735ba5d80
> src/linux/cgroups.cpp b97a89cafab12aa642645508ad465f714ee08f1b
> src/slave/cgroups_isolator.cpp 676768e6b8bd13820467309814845257a9c47e02
>
> Diff: https://reviews.apache.org/r/13903/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request 13903: Added a cgroups::memory namespace.
Posted by Benjamin Hindman <be...@berkeley.edu>.
> On Aug. 31, 2013, 5:55 a.m., Vinod Kone wrote:
> > src/linux/cgroups.hpp, line 386
> > <https://reviews.apache.org/r/13903/diff/2/?file=346998#file346998line386>
> >
> > I'm not sure I like the snake cased function names. We rarely do this (if ever) in our code base. I understand you wanted to match the control file names, but why not have simple function names instead?
> >
> > How about limit(), softLimit() and usage()?
> >
> > The 'bytes' in the function name is redundant because we have 'Bytes' as input/return.
> >
> > Thoughts?
> >
Just like with flags, I like the idea of naming the functions to match the underlying file names. I think this will pay off in the long run.
> On Aug. 31, 2013, 5:55 a.m., Vinod Kone wrote:
> > src/slave/cgroups_isolator.cpp, lines 1061-1068
> > <https://reviews.apache.org/r/13903/diff/2/?file=347000#file347000line1061>
> >
> > I recommend not doing this big change (always setting soft limit) as part of this review. Lets keep this review to use the new memory abstraction.
> >
> > You can split the soft limit change into another review. This way it would be easy to revert if things do not work as expected.
+1 to delaying this to a later review.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13903/#review25815
-----------------------------------------------------------
On Aug. 31, 2013, 12:29 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/13903/
> -----------------------------------------------------------
>
> (Updated Aug. 31, 2013, 12:29 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Vinod Kone.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> I've adjusted the CgroupsIsolator code to use these new functions.
>
> Note that this always sets the soft_limit_in_bytes. This should have been done in the first place, so let me know if you see a reason to conditionally set the soft limit as before.
>
>
> Diffs
> -----
>
> src/linux/cgroups.hpp 3989712b5b76bc960f03fdf020a4e5b735ba5d80
> src/linux/cgroups.cpp b97a89cafab12aa642645508ad465f714ee08f1b
> src/slave/cgroups_isolator.cpp 676768e6b8bd13820467309814845257a9c47e02
>
> Diff: https://reviews.apache.org/r/13903/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request 13903: Added a cgroups::memory namespace.
Posted by Ben Mahler <be...@gmail.com>.
> On Aug. 31, 2013, 5:55 a.m., Vinod Kone wrote:
> > src/linux/cgroups.hpp, line 386
> > <https://reviews.apache.org/r/13903/diff/2/?file=346998#file346998line386>
> >
> > I'm not sure I like the snake cased function names. We rarely do this (if ever) in our code base. I understand you wanted to match the control file names, but why not have simple function names instead?
> >
> > How about limit(), softLimit() and usage()?
> >
> > The 'bytes' in the function name is redundant because we have 'Bytes' as input/return.
> >
> > Thoughts?
> >
>
> Benjamin Hindman wrote:
> Just like with flags, I like the idea of naming the functions to match the underlying file names. I think this will pay off in the long run.
I've dropped this one in favor of keeping the methods consistent with the control filenames.
Adding on to benh's comment, I think this also makes it very clear as to what controls are being manipulated, which in turn forces the user to read the cgroups documentation to understand how to use the control.
We could hide this as you suggested but I prefer to mirror the controls for now in favor of being as explicit as possible (cgroups are complicated so I'm hesitant to hide too many of the details through our API). Let me know what you think!
> On Aug. 31, 2013, 5:55 a.m., Vinod Kone wrote:
> > src/slave/cgroups_isolator.cpp, line 1044
> > <https://reviews.apache.org/r/13903/diff/2/?file=347000#file347000line1044>
> >
> > Why not just do
> >
> > Bytes mem = Megabytes((uint64_t) resource.scalar().value()); ?
This would lose accuracy!
Take the example of mem == 0.5:
Megabytes((uint64_t) 0.5) == Megabytes(0)
> On Aug. 31, 2013, 5:55 a.m., Vinod Kone wrote:
> > src/slave/cgroups_isolator.cpp, line 1066
> > <https://reviews.apache.org/r/13903/diff/2/?file=347000#file347000line1066>
> >
> > s/memory.soft_limit_in_bytes/memory soft limit/
Dropped these given the function names mirror the controls.
> On Aug. 31, 2013, 5:55 a.m., Vinod Kone wrote:
> > src/slave/cgroups_isolator.cpp, line 1227
> > <https://reviews.apache.org/r/13903/diff/2/?file=347000#file347000line1227>
> >
> > Why no logging on error like you did above?
> On Aug. 31, 2013, 5:55 a.m., Vinod Kone wrote:
> > src/slave/cgroups_isolator.cpp, lines 1061-1068
> > <https://reviews.apache.org/r/13903/diff/2/?file=347000#file347000line1061>
> >
> > I recommend not doing this big change (always setting soft limit) as part of this review. Lets keep this review to use the new memory abstraction.
> >
> > You can split the soft limit change into another review. This way it would be easy to revert if things do not work as expected.
>
> Benjamin Hindman wrote:
> +1 to delaying this to a later review.
This is a bug! But.. it's pretty harmless and I understand your concerns to minimize change so I will revert my fix.
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13903/#review25815
-----------------------------------------------------------
On Aug. 31, 2013, 12:29 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/13903/
> -----------------------------------------------------------
>
> (Updated Aug. 31, 2013, 12:29 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Vinod Kone.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> I've adjusted the CgroupsIsolator code to use these new functions.
>
> Note that this always sets the soft_limit_in_bytes. This should have been done in the first place, so let me know if you see a reason to conditionally set the soft limit as before.
>
>
> Diffs
> -----
>
> src/linux/cgroups.hpp 3989712b5b76bc960f03fdf020a4e5b735ba5d80
> src/linux/cgroups.cpp b97a89cafab12aa642645508ad465f714ee08f1b
> src/slave/cgroups_isolator.cpp 676768e6b8bd13820467309814845257a9c47e02
>
> Diff: https://reviews.apache.org/r/13903/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request 13903: Added a cgroups::memory namespace.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13903/#review25815
-----------------------------------------------------------
src/linux/cgroups.hpp
<https://reviews.apache.org/r/13903/#comment50354>
I'm not sure I like the snake cased function names. We rarely do this (if ever) in our code base. I understand you wanted to match the control file names, but why not have simple function names instead?
How about limit(), softLimit() and usage()?
The 'bytes' in the function name is redundant because we have 'Bytes' as input/return.
Thoughts?
src/slave/cgroups_isolator.cpp
<https://reviews.apache.org/r/13903/#comment50355>
Why not just do
Bytes mem = Megabytes((uint64_t) resource.scalar().value()); ?
src/slave/cgroups_isolator.cpp
<https://reviews.apache.org/r/13903/#comment50363>
I recommend not doing this big change (always setting soft limit) as part of this review. Lets keep this review to use the new memory abstraction.
You can split the soft limit change into another review. This way it would be easy to revert if things do not work as expected.
src/slave/cgroups_isolator.cpp
<https://reviews.apache.org/r/13903/#comment50358>
s/memory.soft_limit_in_bytes/memory soft limit/
src/slave/cgroups_isolator.cpp
<https://reviews.apache.org/r/13903/#comment50359>
s/memory.soft_limit_in_bytes/memory soft limit/
src/slave/cgroups_isolator.cpp
<https://reviews.apache.org/r/13903/#comment50360>
s/because/is because/.
Also this sentence doesn't follow correctly from the previous sentence. How about phrasing it as it was before?
src/slave/cgroups_isolator.cpp
<https://reviews.apache.org/r/13903/#comment50361>
s/memory.limit_in_bytes/memory limit/
src/slave/cgroups_isolator.cpp
<https://reviews.apache.org/r/13903/#comment50357>
s/memory.limit_in_bytes/memory limit/
src/slave/cgroups_isolator.cpp
<https://reviews.apache.org/r/13903/#comment50356>
s/memory.usage_in_bytes/memory usage/
src/slave/cgroups_isolator.cpp
<https://reviews.apache.org/r/13903/#comment50362>
Why no logging on error like you did above?
- Vinod Kone
On Aug. 31, 2013, 12:29 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/13903/
> -----------------------------------------------------------
>
> (Updated Aug. 31, 2013, 12:29 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Vinod Kone.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> I've adjusted the CgroupsIsolator code to use these new functions.
>
> Note that this always sets the soft_limit_in_bytes. This should have been done in the first place, so let me know if you see a reason to conditionally set the soft limit as before.
>
>
> Diffs
> -----
>
> src/linux/cgroups.hpp 3989712b5b76bc960f03fdf020a4e5b735ba5d80
> src/linux/cgroups.cpp b97a89cafab12aa642645508ad465f714ee08f1b
> src/slave/cgroups_isolator.cpp 676768e6b8bd13820467309814845257a9c47e02
>
> Diff: https://reviews.apache.org/r/13903/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request 13903: Added a cgroups::memory namespace.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13903/
-----------------------------------------------------------
(Updated Aug. 31, 2013, 12:29 a.m.)
Review request for mesos, Benjamin Hindman and Vinod Kone.
Changes
-------
Small naming change.
Repository: mesos-git
Description
-------
I've adjusted the CgroupsIsolator code to use these new functions.
Note that this always sets the soft_limit_in_bytes. This should have been done in the first place, so let me know if you see a reason to conditionally set the soft limit as before.
Diffs (updated)
-----
src/linux/cgroups.hpp 3989712b5b76bc960f03fdf020a4e5b735ba5d80
src/linux/cgroups.cpp b97a89cafab12aa642645508ad465f714ee08f1b
src/slave/cgroups_isolator.cpp 676768e6b8bd13820467309814845257a9c47e02
Diff: https://reviews.apache.org/r/13903/diff/
Testing
-------
make check
Thanks,
Ben Mahler