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/03/07 04:20:44 UTC
Review Request: Added isolation module tests for resource usage.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9797/
-----------------------------------------------------------
Review request for mesos, Benjamin Hindman and Vinod Kone.
Description
-------
Finally some non-external isolation module tests!
Diffs
-----
src/Makefile.am f7242a7fb55de3d1df4930126345274124331d57
src/tests/environment.cpp 7c8c2c60ec77c732bf32292088cf5f4cb879ed89
src/tests/isolator_tests.cpp PRE-CREATION
src/tests/utils.hpp 0d4edad4787f92f86b1486b6b510f46a62d7d541
Diff: https://reviews.apache.org/r/9797/diff/
Testing
-------
make check
Thanks,
Ben Mahler
Re: Review Request: Added isolation module tests for resource usage.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9797/
-----------------------------------------------------------
(Updated April 12, 2013, 10:42 p.m.)
Review request for mesos, Benjamin Hindman and Vinod Kone.
Changes
-------
This now uses the new testing abstractions, and disables the test on OSX.
Description
-------
Finally some non-external isolation module tests!
Diffs (updated)
-----
src/Makefile.am 0de362e4b0cd8a2bc771facfc7cce17cbe8d8fd5
src/tests/isolator.hpp f885ccb44e809383e658f45d9a03eda174cf2d72
src/tests/isolator_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/9797/diff/
Testing
-------
make check
sudo ./bin/mesos-tests.sh --gtest_filter="Isolator*Usage" --verbose --gtest_break_on_failure --gtest_repeat=60
Thanks,
Ben Mahler
Re: Review Request: Added isolation module tests for resource usage.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9797/
-----------------------------------------------------------
(Updated March 30, 2013, 10:26 p.m.)
Review request for mesos, Benjamin Hindman and Vinod Kone.
Changes
-------
Rebased off trunk, no need for review.
Description
-------
Finally some non-external isolation module tests!
Diffs (updated)
-----
src/Makefile.am 0ef1eac19ff6f366e2590eed371958db8fd62289
src/tests/isolator_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/9797/diff/
Testing
-------
make check
sudo ./bin/mesos-tests.sh --gtest_filter="Isolator*Usage" --verbose --gtest_break_on_failure --gtest_repeat=60
Thanks,
Ben Mahler
Re: Review Request: Added isolation module tests for resource usage.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9797/
-----------------------------------------------------------
(Updated March 28, 2013, 9:26 p.m.)
Review request for mesos, Benjamin Hindman and Vinod Kone.
Changes
-------
Benh review, rebased off trunk.
Description
-------
Finally some non-external isolation module tests!
Diffs (updated)
-----
src/Makefile.am 847a54bb374f1961a06691ec41592f0bdbfa95e6
src/tests/isolator_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/9797/diff/
Testing (updated)
-------
make check
sudo ./bin/mesos-tests.sh --gtest_filter="Isolator*Usage" --verbose --gtest_break_on_failure --gtest_repeat=60
Thanks,
Ben Mahler
Re: Review Request: Added isolation module tests for resource usage.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9797/#review17821
-----------------------------------------------------------
src/tests/utils.hpp
<https://reviews.apache.org/r/9797/#comment37791>
The work_dir technically is the sandbox, since masters and slaves that use the work_dir will put their stuff in directories under the work_dir.
- Benjamin Hindman
On March 13, 2013, 12:21 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9797/
> -----------------------------------------------------------
>
> (Updated March 13, 2013, 12:21 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Vinod Kone.
>
>
> Description
> -------
>
> Finally some non-external isolation module tests!
>
>
> Diffs
> -----
>
> src/Makefile.am f7242a7fb55de3d1df4930126345274124331d57
> src/tests/environment.cpp e71c2cf9f45f6cd7d7305a11da701042bd6e22c6
> src/tests/isolator_tests.cpp PRE-CREATION
> src/tests/utils.hpp d3efa58ef62383af9eb051b23feb950ba6a4f4e3
>
> Diff: https://reviews.apache.org/r/9797/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request: Added isolation module tests for resource usage.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9797/#review17818
-----------------------------------------------------------
src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/9797/#comment37780>
I think you can make this work more clearly with a 'do { ... } while (waited < Seconds(5));' and then put the EXPECT_GT after the loop.
- Benjamin Hindman
On March 13, 2013, 12:21 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9797/
> -----------------------------------------------------------
>
> (Updated March 13, 2013, 12:21 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Vinod Kone.
>
>
> Description
> -------
>
> Finally some non-external isolation module tests!
>
>
> Diffs
> -----
>
> src/Makefile.am f7242a7fb55de3d1df4930126345274124331d57
> src/tests/environment.cpp e71c2cf9f45f6cd7d7305a11da701042bd6e22c6
> src/tests/isolator_tests.cpp PRE-CREATION
> src/tests/utils.hpp d3efa58ef62383af9eb051b23feb950ba6a4f4e3
>
> Diff: https://reviews.apache.org/r/9797/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request: Added isolation module tests for resource usage.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9797/#review17806
-----------------------------------------------------------
Ship it!
can you rebase this off trunk?
- Vinod Kone
On March 13, 2013, 12:21 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9797/
> -----------------------------------------------------------
>
> (Updated March 13, 2013, 12:21 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Vinod Kone.
>
>
> Description
> -------
>
> Finally some non-external isolation module tests!
>
>
> Diffs
> -----
>
> src/Makefile.am f7242a7fb55de3d1df4930126345274124331d57
> src/tests/environment.cpp e71c2cf9f45f6cd7d7305a11da701042bd6e22c6
> src/tests/isolator_tests.cpp PRE-CREATION
> src/tests/utils.hpp d3efa58ef62383af9eb051b23feb950ba6a4f4e3
>
> Diff: https://reviews.apache.org/r/9797/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request: Added isolation module tests for resource usage.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9797/
-----------------------------------------------------------
(Updated March 13, 2013, 12:21 a.m.)
Review request for mesos, Benjamin Hindman and Vinod Kone.
Changes
-------
Updated comments.
Description
-------
Finally some non-external isolation module tests!
Diffs (updated)
-----
src/Makefile.am f7242a7fb55de3d1df4930126345274124331d57
src/tests/environment.cpp e71c2cf9f45f6cd7d7305a11da701042bd6e22c6
src/tests/isolator_tests.cpp PRE-CREATION
src/tests/utils.hpp d3efa58ef62383af9eb051b23feb950ba6a4f4e3
Diff: https://reviews.apache.org/r/9797/diff/
Testing
-------
make check
Thanks,
Ben Mahler
Re: Review Request: Added isolation module tests for resource usage.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9797/
-----------------------------------------------------------
(Updated March 12, 2013, 9:36 p.m.)
Review request for mesos, Benjamin Hindman and Vinod Kone.
Changes
-------
Rebased off trunk, vinod's review.
Description
-------
Finally some non-external isolation module tests!
Diffs (updated)
-----
src/Makefile.am f7242a7fb55de3d1df4930126345274124331d57
src/tests/environment.cpp e71c2cf9f45f6cd7d7305a11da701042bd6e22c6
src/tests/isolator_tests.cpp PRE-CREATION
src/tests/utils.hpp d3efa58ef62383af9eb051b23feb950ba6a4f4e3
Diff: https://reviews.apache.org/r/9797/diff/
Testing
-------
make check
Thanks,
Ben Mahler
Re: Review Request: Added isolation module tests for resource usage.
Posted by Ben Mahler <be...@gmail.com>.
> On March 8, 2013, 9:48 p.m., Vinod Kone wrote:
> > src/tests/isolator_tests.cpp, lines 180-182
> > <https://reviews.apache.org/r/9797/diff/1-2/?file=267815#file267815line180>
> >
> > I'm confused. Where did these numbers come from!?
> >
> > How can you guarantee the stats are going to be greater than these?
I've now made the test even more robust! So it will start the child 'top' process and allow up to 5 seconds for the cpu times to be induced.
Note that this test will fail on OSX, until I fix the child process monitoring to handle OSX as well.
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9797/#review17631
-----------------------------------------------------------
On March 8, 2013, 2:28 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9797/
> -----------------------------------------------------------
>
> (Updated March 8, 2013, 2:28 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Vinod Kone.
>
>
> Description
> -------
>
> Finally some non-external isolation module tests!
>
>
> Diffs
> -----
>
> src/Makefile.am f7242a7fb55de3d1df4930126345274124331d57
> src/tests/environment.cpp bda72ae0310e74015f3fbde50fecbf6515276e81
> src/tests/isolator_tests.cpp PRE-CREATION
> src/tests/utils.hpp 0d4edad4787f92f86b1486b6b510f46a62d7d541
>
> Diff: https://reviews.apache.org/r/9797/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request: Added isolation module tests for resource usage.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9797/#review17631
-----------------------------------------------------------
src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/9797/#comment37390>
I'm confused. Where did these numbers come from!?
How can you guarantee the stats are going to be greater than these?
src/tests/utils.hpp
<https://reviews.apache.org/r/9797/#comment37382>
s/it/them/
src/tests/utils.hpp
<https://reviews.apache.org/r/9797/#comment37385>
Using CHECK's in tests is bad, because 1) TearDown* methods won't be called and 2) no other tests will be run.
Typically, a test failure should only abort the current test and proceed with other tests.
tl:dr; s/CHECK_SOME/ASSERT/
src/tests/utils.hpp
<https://reviews.apache.org/r/9797/#comment37386>
ditto
src/tests/utils.hpp
<https://reviews.apache.org/r/9797/#comment37387>
ditto
src/tests/utils.hpp
<https://reviews.apache.org/r/9797/#comment37388>
ditto
- Vinod Kone
On March 8, 2013, 2:28 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9797/
> -----------------------------------------------------------
>
> (Updated March 8, 2013, 2:28 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Vinod Kone.
>
>
> Description
> -------
>
> Finally some non-external isolation module tests!
>
>
> Diffs
> -----
>
> src/Makefile.am f7242a7fb55de3d1df4930126345274124331d57
> src/tests/environment.cpp bda72ae0310e74015f3fbde50fecbf6515276e81
> src/tests/isolator_tests.cpp PRE-CREATION
> src/tests/utils.hpp 0d4edad4787f92f86b1486b6b510f46a62d7d541
>
> Diff: https://reviews.apache.org/r/9797/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request: Added isolation module tests for resource usage.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9797/
-----------------------------------------------------------
(Updated March 8, 2013, 2:28 a.m.)
Review request for mesos, Benjamin Hindman and Vinod Kone.
Changes
-------
Reviews, rebased off trunk.
Description
-------
Finally some non-external isolation module tests!
Diffs (updated)
-----
src/Makefile.am f7242a7fb55de3d1df4930126345274124331d57
src/tests/environment.cpp bda72ae0310e74015f3fbde50fecbf6515276e81
src/tests/isolator_tests.cpp PRE-CREATION
src/tests/utils.hpp 0d4edad4787f92f86b1486b6b510f46a62d7d541
Diff: https://reviews.apache.org/r/9797/diff/
Testing
-------
make check
Thanks,
Ben Mahler
Re: Review Request: Added isolation module tests for resource usage.
Posted by Ben Mahler <be...@gmail.com>.
> On March 7, 2013, 6:46 p.m., Vinod Kone wrote:
> >
Thanks for the review!
> On March 7, 2013, 6:46 p.m., Vinod Kone wrote:
> > src/tests/isolator_tests.cpp, line 167
> > <https://reviews.apache.org/r/9797/diff/1/?file=267815#file267815line167>
> >
> > I see now why you set the 'id' in utils.hpp.
> >
> > The right way to do this is, do an EXPECT on scheduler's registered call and grab the framework id.
Ah, I see, seems easier if we didn't test that logic everywhere and made it easier on test writers by having the default include an id =/
> On March 7, 2013, 6:46 p.m., Vinod Kone wrote:
> > src/tests/isolator_tests.cpp, line 182
> > <https://reviews.apache.org/r/9797/diff/1/?file=267815#file267815line182>
> >
> > s/codes/code/
Changed to "a non-zero exit code".
> On March 7, 2013, 6:46 p.m., Vinod Kone wrote:
> > src/tests/utils.hpp, line 154
> > <https://reviews.apache.org/r/9797/diff/1/?file=267816#file267816line154>
> >
> > when a framework doesn't have an id set, the id is given by the master when it first registers. when it fails over, a framework uses this id to re-register with master.
> >
> > tldr; revert this.
Ah, I see.
> On March 7, 2013, 6:46 p.m., Vinod Kone wrote:
> > src/tests/environment.cpp, line 108
> > <https://reviews.apache.org/r/9797/diff/1/?file=267814#file267814line108>
> >
> > Why the TODO instead of fixing it? This is going to break Jenkins if it gets submittted asis?
> >
> > Also, call it CgroupsIsolationModule till we get around to renaming it.
Agreed, I just wanted to rebase this off of: https://reviews.apache.org/r/9798/
Benh is currently in the process of a rename.
> On March 7, 2013, 6:46 p.m., Vinod Kone wrote:
> > src/tests/isolator_tests.cpp, lines 173-175
> > <https://reviews.apache.org/r/9797/diff/1/?file=267815#file267815line173>
> >
> > Umm. This doesn't really test that you are collecting usage from the executor process tree :/
> >
> > In other words, this test wouldn't fail even if you would've run it with the previous code?
You're right, thanks for checking me on that!
This test does fail before the descendant usage was aggregated, but only because the system time was *sometimes* zero. So, I'm guilty.
I've made this more robust (for cpu times), thanks for the push!
Now, without child process usage:
cpu_user_time: 0.01
cpu_system_time: 0.01
memory_rss: 10825728
With child process usage:
cpu_user_time: 0.23
cpu_system_time: 0.28
memory_rss: 13058048
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9797/#review17557
-----------------------------------------------------------
On March 7, 2013, 3:20 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9797/
> -----------------------------------------------------------
>
> (Updated March 7, 2013, 3:20 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Vinod Kone.
>
>
> Description
> -------
>
> Finally some non-external isolation module tests!
>
>
> Diffs
> -----
>
> src/Makefile.am f7242a7fb55de3d1df4930126345274124331d57
> src/tests/environment.cpp 7c8c2c60ec77c732bf32292088cf5f4cb879ed89
> src/tests/isolator_tests.cpp PRE-CREATION
> src/tests/utils.hpp 0d4edad4787f92f86b1486b6b510f46a62d7d541
>
> Diff: https://reviews.apache.org/r/9797/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request: Added isolation module tests for resource usage.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9797/#review17557
-----------------------------------------------------------
src/tests/environment.cpp
<https://reviews.apache.org/r/9797/#comment37224>
Why the TODO instead of fixing it? This is going to break Jenkins if it gets submittted asis?
Also, call it CgroupsIsolationModule till we get around to renaming it.
src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/9797/#comment37225>
Reorder these includes.
src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/9797/#comment37230>
why not give it a name?
src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/9797/#comment37232>
How does this file get cleaned up after the test?
You should clean this up in TearDown() method of your fixture.
src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/9797/#comment37233>
Could you comment on what these arguments mean?
src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/9797/#comment37234>
I see now why you set the 'id' in utils.hpp.
The right way to do this is, do an EXPECT on scheduler's registered call and grab the framework id.
src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/9797/#comment37237>
Umm. This doesn't really test that you are collecting usage from the executor process tree :/
In other words, this test wouldn't fail even if you would've run it with the previous code?
src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/9797/#comment37238>
s/codes/code/
src/tests/utils.hpp
<https://reviews.apache.org/r/9797/#comment37228>
when a framework doesn't have an id set, the id is given by the master when it first registers. when it fails over, a framework uses this id to re-register with master.
tldr; revert this.
- Vinod Kone
On March 7, 2013, 3:20 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9797/
> -----------------------------------------------------------
>
> (Updated March 7, 2013, 3:20 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Vinod Kone.
>
>
> Description
> -------
>
> Finally some non-external isolation module tests!
>
>
> Diffs
> -----
>
> src/Makefile.am f7242a7fb55de3d1df4930126345274124331d57
> src/tests/environment.cpp 7c8c2c60ec77c732bf32292088cf5f4cb879ed89
> src/tests/isolator_tests.cpp PRE-CREATION
> src/tests/utils.hpp 0d4edad4787f92f86b1486b6b510f46a62d7d541
>
> Diff: https://reviews.apache.org/r/9797/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request: Added isolation module tests for resource usage.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9797/#review17523
-----------------------------------------------------------
Ship it!
src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/9797/#comment37195>
One of us will still have to get these ones though! ;)
- Benjamin Hindman
On March 7, 2013, 3:20 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9797/
> -----------------------------------------------------------
>
> (Updated March 7, 2013, 3:20 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Vinod Kone.
>
>
> Description
> -------
>
> Finally some non-external isolation module tests!
>
>
> Diffs
> -----
>
> src/Makefile.am f7242a7fb55de3d1df4930126345274124331d57
> src/tests/environment.cpp 7c8c2c60ec77c732bf32292088cf5f4cb879ed89
> src/tests/isolator_tests.cpp PRE-CREATION
> src/tests/utils.hpp 0d4edad4787f92f86b1486b6b510f46a62d7d541
>
> Diff: https://reviews.apache.org/r/9797/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Ben Mahler
>
>