You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Vinod Kone <vi...@gmail.com> on 2013/03/11 05:18:41 UTC
Review Request: Added fixtures to enable templatized isolation module tests
and fixed a bug in cgroups isolation module.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9847/
-----------------------------------------------------------
Review request for mesos, Benjamin Hindman and Ben Mahler.
Description
-------
With the new fixtures, one can implement a typed test, for isolation modules, as follows.
template <typename T>
class SlaveRecoveryTest : public IsolationTest<T>
{
T isolationModule;
};
Diffs
-----
src/slave/cgroups_isolation_module.hpp 11244802b3210ef1a6900b978faf8bbcaa00266c
src/slave/cgroups_isolation_module.cpp 9395d9cd6cf2ac7a720480b778836eb1d704e00d
src/tests/utils.hpp d3efa58ef62383af9eb051b23feb950ba6a4f4e3
Diff: https://reviews.apache.org/r/9847/diff/
Testing
-------
make check
Thanks,
Vinod Kone
Re: Review Request: Added fixtures to enable templatized isolation module
tests and fixed a bug in cgroups isolation module.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9847/#review17700
-----------------------------------------------------------
src/slave/cgroups_isolation_module.hpp
<https://reviews.apache.org/r/9847/#comment37583>
pass 'flags' instead.
- Vinod Kone
On March 11, 2013, 4:18 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9847/
> -----------------------------------------------------------
>
> (Updated March 11, 2013, 4:18 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Description
> -------
>
> With the new fixtures, one can implement a typed test, for isolation modules, as follows.
>
> template <typename T>
> class SlaveRecoveryTest : public IsolationTest<T>
> {
> T isolationModule;
> };
>
>
> Diffs
> -----
>
> src/slave/cgroups_isolation_module.hpp 11244802b3210ef1a6900b978faf8bbcaa00266c
> src/slave/cgroups_isolation_module.cpp 9395d9cd6cf2ac7a720480b778836eb1d704e00d
> src/tests/utils.hpp d3efa58ef62383af9eb051b23feb950ba6a4f4e3
>
> Diff: https://reviews.apache.org/r/9847/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request: Added fixtures to enable templatized isolation module
tests and fixed a bug in cgroups isolation module.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9847/#review17777
-----------------------------------------------------------
src/linux/cgroups.hpp
<https://reviews.apache.org/r/9847/#comment37725>
kill
src/linux/cgroups.hpp
<https://reviews.apache.org/r/9847/#comment37724>
s/cleanup/destroy.
src/linux/cgroups.cpp
<https://reviews.apache.org/r/9847/#comment37726>
else if
src/tests/cgroups_tests.cpp
<https://reviews.apache.org/r/9847/#comment37723>
just call cleanup here.
src/tests/cgroups_tests.cpp
<https://reviews.apache.org/r/9847/#comment37727>
just call cleanup.
- Vinod Kone
On March 12, 2013, 10:21 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9847/
> -----------------------------------------------------------
>
> (Updated March 12, 2013, 10:21 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Description
> -------
>
> With the new fixtures, one can implement a typed test, for isolation modules, as follows.
>
> template <typename T>
> class SlaveRecoveryTest : public IsolationTest<T>
> {
> T isolationModule;
> };
>
>
> Diffs
> -----
>
> src/linux/cgroups.hpp 56c6eb7ea130acd951eabdb42dafae4f2cdcff4d
> src/linux/cgroups.cpp 480ae228a1365afa60c9f88c1f31f48e3a5eb294
> src/slave/cgroups_isolation_module.hpp 11244802b3210ef1a6900b978faf8bbcaa00266c
> src/slave/cgroups_isolation_module.cpp 9395d9cd6cf2ac7a720480b778836eb1d704e00d
> src/tests/cgroups_tests.cpp 65c2bfaec1dd5adc879a206aa97592ae7f0c0074
> src/tests/utils.hpp d3efa58ef62383af9eb051b23feb950ba6a4f4e3
>
> Diff: https://reviews.apache.org/r/9847/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request: Added fixtures to enable templatized isolation module
tests and fixed a bug in cgroups isolation module.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9847/
-----------------------------------------------------------
(Updated March 13, 2013, 6:09 a.m.)
Review request for mesos, Benjamin Hindman and Ben Mahler.
Changes
-------
benh's comments. no need for review.
Description
-------
With the new fixtures, one can implement a typed test, for isolation modules, as follows.
template <typename T>
class SlaveRecoveryTest : public IsolationTest<T>
{
T isolationModule;
};
Diffs (updated)
-----
src/linux/cgroups.hpp 56c6eb7ea130acd951eabdb42dafae4f2cdcff4d
src/linux/cgroups.cpp 480ae228a1365afa60c9f88c1f31f48e3a5eb294
src/slave/cgroups_isolation_module.hpp 11244802b3210ef1a6900b978faf8bbcaa00266c
src/slave/cgroups_isolation_module.cpp 9395d9cd6cf2ac7a720480b778836eb1d704e00d
src/tests/cgroups_tests.cpp 65c2bfaec1dd5adc879a206aa97592ae7f0c0074
src/tests/utils.hpp d3efa58ef62383af9eb051b23feb950ba6a4f4e3
Diff: https://reviews.apache.org/r/9847/diff/
Testing
-------
make check
Thanks,
Vinod Kone
Re: Review Request: Added fixtures to enable templatized isolation module
tests and fixed a bug in cgroups isolation module.
Posted by Vinod Kone <vi...@gmail.com>.
> On March 13, 2013, 5:51 a.m., Benjamin Hindman wrote:
> > src/tests/utils.hpp, line 295
> > <https://reviews.apache.org/r/9847/diff/4/?file=269891#file269891line295>
> >
> > Final question: what's the value in using TEST_CGROUPS_ROOT?
We need this to ensure, we only cleanup cgroup(s) that we create as part of the tests. If there was already a "mesos" cgroup (under /cgroup for example), I don't want the tests to touch it.
> On March 13, 2013, 5:51 a.m., Benjamin Hindman wrote:
> > src/linux/cgroups.hpp, line 337
> > <https://reviews.apache.org/r/9847/diff/4/?file=269886#file269886line337>
> >
> > "... or if we failed to destroy any of the cgroups."
> >
> > So, the comment leaves me wondering: how can you just "remove" a cgroup? Don't you have to kill all the tasks first?
> >
> > My expectation was that destroy always tried to kill all the tasks in a cgroup. The freezer subsystem just made that more reliable. If this isn't the current implementation we should put a big TODO to eventually do it this way.
Added a TODO. Thanks for catching that.
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9847/#review17794
-----------------------------------------------------------
On March 13, 2013, 4:52 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9847/
> -----------------------------------------------------------
>
> (Updated March 13, 2013, 4:52 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Description
> -------
>
> With the new fixtures, one can implement a typed test, for isolation modules, as follows.
>
> template <typename T>
> class SlaveRecoveryTest : public IsolationTest<T>
> {
> T isolationModule;
> };
>
>
> Diffs
> -----
>
> src/linux/cgroups.hpp 56c6eb7ea130acd951eabdb42dafae4f2cdcff4d
> src/linux/cgroups.cpp 480ae228a1365afa60c9f88c1f31f48e3a5eb294
> src/slave/cgroups_isolation_module.hpp 11244802b3210ef1a6900b978faf8bbcaa00266c
> src/slave/cgroups_isolation_module.cpp 9395d9cd6cf2ac7a720480b778836eb1d704e00d
> src/tests/cgroups_tests.cpp 65c2bfaec1dd5adc879a206aa97592ae7f0c0074
> src/tests/utils.hpp d3efa58ef62383af9eb051b23feb950ba6a4f4e3
>
> Diff: https://reviews.apache.org/r/9847/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request: Added fixtures to enable templatized isolation module
tests and fixed a bug in cgroups isolation module.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9847/#review17794
-----------------------------------------------------------
Ship it!
LGTM!
src/linux/cgroups.hpp
<https://reviews.apache.org/r/9847/#comment37745>
"... or if we failed to destroy any of the cgroups."
So, the comment leaves me wondering: how can you just "remove" a cgroup? Don't you have to kill all the tasks first?
My expectation was that destroy always tried to kill all the tasks in a cgroup. The freezer subsystem just made that more reliable. If this isn't the current implementation we should put a big TODO to eventually do it this way.
src/linux/cgroups.hpp
<https://reviews.apache.org/r/9847/#comment37746>
s/cleaning up all/destroying all/
src/tests/utils.hpp
<https://reviews.apache.org/r/9847/#comment37748>
Final question: what's the value in using TEST_CGROUPS_ROOT?
- Benjamin Hindman
On March 13, 2013, 4:52 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9847/
> -----------------------------------------------------------
>
> (Updated March 13, 2013, 4:52 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Description
> -------
>
> With the new fixtures, one can implement a typed test, for isolation modules, as follows.
>
> template <typename T>
> class SlaveRecoveryTest : public IsolationTest<T>
> {
> T isolationModule;
> };
>
>
> Diffs
> -----
>
> src/linux/cgroups.hpp 56c6eb7ea130acd951eabdb42dafae4f2cdcff4d
> src/linux/cgroups.cpp 480ae228a1365afa60c9f88c1f31f48e3a5eb294
> src/slave/cgroups_isolation_module.hpp 11244802b3210ef1a6900b978faf8bbcaa00266c
> src/slave/cgroups_isolation_module.cpp 9395d9cd6cf2ac7a720480b778836eb1d704e00d
> src/tests/cgroups_tests.cpp 65c2bfaec1dd5adc879a206aa97592ae7f0c0074
> src/tests/utils.hpp d3efa58ef62383af9eb051b23feb950ba6a4f4e3
>
> Diff: https://reviews.apache.org/r/9847/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request: Added fixtures to enable templatized isolation module
tests and fixed a bug in cgroups isolation module.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9847/
-----------------------------------------------------------
(Updated March 13, 2013, 4:52 a.m.)
Review request for mesos, Benjamin Hindman and Ben Mahler.
Changes
-------
review comments.
need a ship-it before i commit this.
Description
-------
With the new fixtures, one can implement a typed test, for isolation modules, as follows.
template <typename T>
class SlaveRecoveryTest : public IsolationTest<T>
{
T isolationModule;
};
Diffs (updated)
-----
src/linux/cgroups.hpp 56c6eb7ea130acd951eabdb42dafae4f2cdcff4d
src/linux/cgroups.cpp 480ae228a1365afa60c9f88c1f31f48e3a5eb294
src/slave/cgroups_isolation_module.hpp 11244802b3210ef1a6900b978faf8bbcaa00266c
src/slave/cgroups_isolation_module.cpp 9395d9cd6cf2ac7a720480b778836eb1d704e00d
src/tests/cgroups_tests.cpp 65c2bfaec1dd5adc879a206aa97592ae7f0c0074
src/tests/utils.hpp d3efa58ef62383af9eb051b23feb950ba6a4f4e3
Diff: https://reviews.apache.org/r/9847/diff/
Testing
-------
make check
Thanks,
Vinod Kone
Re: Review Request: Added fixtures to enable templatized isolation module
tests and fixed a bug in cgroups isolation module.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9847/
-----------------------------------------------------------
(Updated March 12, 2013, 10:21 p.m.)
Review request for mesos, Benjamin Hindman and Ben Mahler.
Changes
-------
Moar refactoring. Hopefully this is much cleaner.
--> Moved the cgroups hierarchies back to cgroups_tests.
--> Added 3 more functions to cgroups namespace, that I used in test fixtures.
Description
-------
With the new fixtures, one can implement a typed test, for isolation modules, as follows.
template <typename T>
class SlaveRecoveryTest : public IsolationTest<T>
{
T isolationModule;
};
Diffs (updated)
-----
src/linux/cgroups.hpp 56c6eb7ea130acd951eabdb42dafae4f2cdcff4d
src/linux/cgroups.cpp 480ae228a1365afa60c9f88c1f31f48e3a5eb294
src/slave/cgroups_isolation_module.hpp 11244802b3210ef1a6900b978faf8bbcaa00266c
src/slave/cgroups_isolation_module.cpp 9395d9cd6cf2ac7a720480b778836eb1d704e00d
src/tests/cgroups_tests.cpp 65c2bfaec1dd5adc879a206aa97592ae7f0c0074
src/tests/utils.hpp d3efa58ef62383af9eb051b23feb950ba6a4f4e3
Diff: https://reviews.apache.org/r/9847/diff/
Testing
-------
make check
Thanks,
Vinod Kone
Re: Review Request: Added fixtures to enable templatized isolation module
tests and fixed a bug in cgroups isolation module.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9847/
-----------------------------------------------------------
(Updated March 12, 2013, 6:14 a.m.)
Review request for mesos, Benjamin Hindman and Ben Mahler.
Changes
-------
factored out cgroups setup/cleanup into functions.
Description
-------
With the new fixtures, one can implement a typed test, for isolation modules, as follows.
template <typename T>
class SlaveRecoveryTest : public IsolationTest<T>
{
T isolationModule;
};
Diffs (updated)
-----
src/slave/cgroups_isolation_module.hpp 11244802b3210ef1a6900b978faf8bbcaa00266c
src/slave/cgroups_isolation_module.cpp 9395d9cd6cf2ac7a720480b778836eb1d704e00d
src/tests/utils.hpp d3efa58ef62383af9eb051b23feb950ba6a4f4e3
Diff: https://reviews.apache.org/r/9847/diff/
Testing
-------
make check
Thanks,
Vinod Kone