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/07/09 21:02:03 UTC
Review Request: Add an API to destroy a cgroup (and all its sub-cgroups).
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5840/
-----------------------------------------------------------
Review request for mesos and Benjamin Hindman.
Description
-------
The patch leverages the atomic task kill patch
https://reviews.apache.org/r/5402/
It can destroy an active cgroup (and all its sub-cgroups) by killing all tasks and remove the cgroups in appropriate order.
It uses the process::collect mechanism to parallelize the killing process.
Diffs
-----
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/5840/diff/
Testing
-------
On linux machines, make check.
Thanks,
Jie Yu
Re: Review Request: Add an API to destroy a cgroup (and all its sub-cgroups).
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5840/#review9911
-----------------------------------------------------------
src/linux/cgroups.cpp
<https://reviews.apache.org/r/5840/#comment21080>
Done.
src/tests/cgroups_tests.cpp
<https://reviews.apache.org/r/5840/#comment21086>
Done.
src/tests/cgroups_tests.cpp
<https://reviews.apache.org/r/5840/#comment21088>
No, if this assignment fails, the child will exit without writing to the pipe. That will cause one of the "ASSERT_NE(-1, ::read(pipes[0], &dummy, sizeof(dummy)));" fail
src/tests/cgroups_tests.cpp
<https://reviews.apache.org/r/5840/#comment21089>
If the assignment fails here, the child will not write the pipe. This will cause one of the pipe read fail in parent.
- Jie Yu
On July 24, 2012, 11:25 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5840/
> -----------------------------------------------------------
>
> (Updated July 24, 2012, 11:25 p.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Description
> -------
>
> The patch leverages the atomic task kill patch
> https://reviews.apache.org/r/5402/
>
> It can destroy an active cgroup (and all its sub-cgroups) by killing all tasks and remove the cgroups in appropriate order.
>
> It uses the process::collect mechanism to parallelize the killing process.
>
>
> Diffs
> -----
>
> src/linux/cgroups.hpp cec9e36
> src/linux/cgroups.cpp 93c140f
> src/tests/cgroups_tests.cpp 7f0f901
> third_party/libprocess/include/process/future.hpp 1da33ad
>
> Diff: https://reviews.apache.org/r/5840/diff/
>
>
> Testing
> -------
>
> On linux machines, make check.
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request: Add an API to destroy a cgroup (and all its sub-cgroups).
Posted by Jie Yu <yu...@gmail.com>.
> On Aug. 6, 2012, 10:52 p.m., Benjamin Hindman wrote:
> > third_party/libprocess/include/process/future.hpp, line 340
> > <https://reviews.apache.org/r/5840/diff/3/?file=128481#file128481line340>
> >
> > Can you please make this a const_iterator too?
Done.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5840/#review9910
-----------------------------------------------------------
On July 24, 2012, 11:25 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5840/
> -----------------------------------------------------------
>
> (Updated July 24, 2012, 11:25 p.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Description
> -------
>
> The patch leverages the atomic task kill patch
> https://reviews.apache.org/r/5402/
>
> It can destroy an active cgroup (and all its sub-cgroups) by killing all tasks and remove the cgroups in appropriate order.
>
> It uses the process::collect mechanism to parallelize the killing process.
>
>
> Diffs
> -----
>
> src/linux/cgroups.hpp cec9e36
> src/linux/cgroups.cpp 93c140f
> src/tests/cgroups_tests.cpp 7f0f901
> third_party/libprocess/include/process/future.hpp 1da33ad
>
> Diff: https://reviews.apache.org/r/5840/diff/
>
>
> Testing
> -------
>
> On linux machines, make check.
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request: Add an API to destroy a cgroup (and all its sub-cgroups).
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5840/#review9910
-----------------------------------------------------------
src/linux/cgroups.cpp
<https://reviews.apache.org/r/5840/#comment21072>
s/finailize/finalize/
src/tests/cgroups_tests.cpp
<https://reviews.apache.org/r/5840/#comment21074>
Why the extra forks? These need a comment.
src/tests/cgroups_tests.cpp
<https://reviews.apache.org/r/5840/#comment21076>
What's the semantics of this with respect to the parent test? Will the parent just wait forever? Or will the test correctly "fail"?
third_party/libprocess/include/process/future.hpp
<https://reviews.apache.org/r/5840/#comment21077>
Can you please make this a const_iterator too?
- Benjamin Hindman
On July 24, 2012, 11:25 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5840/
> -----------------------------------------------------------
>
> (Updated July 24, 2012, 11:25 p.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Description
> -------
>
> The patch leverages the atomic task kill patch
> https://reviews.apache.org/r/5402/
>
> It can destroy an active cgroup (and all its sub-cgroups) by killing all tasks and remove the cgroups in appropriate order.
>
> It uses the process::collect mechanism to parallelize the killing process.
>
>
> Diffs
> -----
>
> src/linux/cgroups.hpp cec9e36
> src/linux/cgroups.cpp 93c140f
> src/tests/cgroups_tests.cpp 7f0f901
> third_party/libprocess/include/process/future.hpp 1da33ad
>
> Diff: https://reviews.apache.org/r/5840/diff/
>
>
> Testing
> -------
>
> On linux machines, make check.
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request: Add an API to destroy a cgroup (and all its sub-cgroups).
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5840/#review9923
-----------------------------------------------------------
Ship it!
Ship It!
- Benjamin Hindman
On Aug. 6, 2012, 11:23 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5840/
> -----------------------------------------------------------
>
> (Updated Aug. 6, 2012, 11:23 p.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Description
> -------
>
> The patch leverages the atomic task kill patch
> https://reviews.apache.org/r/5402/
>
> It can destroy an active cgroup (and all its sub-cgroups) by killing all tasks and remove the cgroups in appropriate order.
>
> It uses the process::collect mechanism to parallelize the killing process.
>
>
> Diffs
> -----
>
> src/linux/cgroups.hpp cec9e36
> src/linux/cgroups.cpp 93c140f
> src/tests/cgroups_tests.cpp 7f0f901
> third_party/libprocess/include/process/future.hpp 60f2070
>
> Diff: https://reviews.apache.org/r/5840/diff/
>
>
> Testing
> -------
>
> On linux machines, make check.
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request: Add an API to destroy a cgroup (and all its sub-cgroups).
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5840/
-----------------------------------------------------------
(Updated Aug. 6, 2012, 11:23 p.m.)
Review request for mesos and Benjamin Hindman.
Changes
-------
Comments from Ben.
Description
-------
The patch leverages the atomic task kill patch
https://reviews.apache.org/r/5402/
It can destroy an active cgroup (and all its sub-cgroups) by killing all tasks and remove the cgroups in appropriate order.
It uses the process::collect mechanism to parallelize the killing process.
Diffs (updated)
-----
src/linux/cgroups.hpp cec9e36
src/linux/cgroups.cpp 93c140f
src/tests/cgroups_tests.cpp 7f0f901
third_party/libprocess/include/process/future.hpp 60f2070
Diff: https://reviews.apache.org/r/5840/diff/
Testing
-------
On linux machines, make check.
Thanks,
Jie Yu
Re: Review Request: Add an API to destroy a cgroup (and all its sub-cgroups).
Posted by Jie Yu <yu...@gmail.com>.
> On Aug. 6, 2012, 11:16 p.m., Benjamin Hindman wrote:
> > src/linux/cgroups.cpp, line 1439
> > <https://reviews.apache.org/r/5840/diff/3/?file=128479#file128479line1439>
> >
> > This is already being done in killTasks, and destroy doesn't explicitly use the freezer subsystem, so let's remove this.
Done.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5840/#review9918
-----------------------------------------------------------
On July 24, 2012, 11:25 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5840/
> -----------------------------------------------------------
>
> (Updated July 24, 2012, 11:25 p.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Description
> -------
>
> The patch leverages the atomic task kill patch
> https://reviews.apache.org/r/5402/
>
> It can destroy an active cgroup (and all its sub-cgroups) by killing all tasks and remove the cgroups in appropriate order.
>
> It uses the process::collect mechanism to parallelize the killing process.
>
>
> Diffs
> -----
>
> src/linux/cgroups.hpp cec9e36
> src/linux/cgroups.cpp 93c140f
> src/tests/cgroups_tests.cpp 7f0f901
> third_party/libprocess/include/process/future.hpp 1da33ad
>
> Diff: https://reviews.apache.org/r/5840/diff/
>
>
> Testing
> -------
>
> On linux machines, make check.
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request: Add an API to destroy a cgroup (and all its sub-cgroups).
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5840/#review9918
-----------------------------------------------------------
src/linux/cgroups.cpp
<https://reviews.apache.org/r/5840/#comment21099>
This is already being done in killTasks, and destroy doesn't explicitly use the freezer subsystem, so let's remove this.
- Benjamin Hindman
On July 24, 2012, 11:25 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5840/
> -----------------------------------------------------------
>
> (Updated July 24, 2012, 11:25 p.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Description
> -------
>
> The patch leverages the atomic task kill patch
> https://reviews.apache.org/r/5402/
>
> It can destroy an active cgroup (and all its sub-cgroups) by killing all tasks and remove the cgroups in appropriate order.
>
> It uses the process::collect mechanism to parallelize the killing process.
>
>
> Diffs
> -----
>
> src/linux/cgroups.hpp cec9e36
> src/linux/cgroups.cpp 93c140f
> src/tests/cgroups_tests.cpp 7f0f901
> third_party/libprocess/include/process/future.hpp 1da33ad
>
> Diff: https://reviews.apache.org/r/5840/diff/
>
>
> Testing
> -------
>
> On linux machines, make check.
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request: Add an API to destroy a cgroup (and all its sub-cgroups).
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5840/
-----------------------------------------------------------
(Updated July 24, 2012, 11:25 p.m.)
Review request for mesos and Benjamin Hindman.
Changes
-------
Address Ben's review comments.
Description
-------
The patch leverages the atomic task kill patch
https://reviews.apache.org/r/5402/
It can destroy an active cgroup (and all its sub-cgroups) by killing all tasks and remove the cgroups in appropriate order.
It uses the process::collect mechanism to parallelize the killing process.
Diffs (updated)
-----
src/linux/cgroups.hpp cec9e36
src/linux/cgroups.cpp 93c140f
src/tests/cgroups_tests.cpp 7f0f901
third_party/libprocess/include/process/future.hpp 1da33ad
Diff: https://reviews.apache.org/r/5840/diff/
Testing
-------
On linux machines, make check.
Thanks,
Jie Yu
Re: Review Request: Add an API to destroy a cgroup (and all its sub-cgroups).
Posted by Jie Yu <yu...@gmail.com>.
> On July 24, 2012, 6:49 p.m., Benjamin Hindman wrote:
> > src/linux/cgroups.cpp, line 1392
> > <https://reviews.apache.org/r/5840/diff/2/?file=123397#file123397line1392>
> >
> > Kill newline.
Done.
> On July 24, 2012, 6:49 p.m., Benjamin Hindman wrote:
> > src/linux/cgroups.cpp, line 1460
> > <https://reviews.apache.org/r/5840/diff/2/?file=123397#file123397line1460>
> >
> > LOG(FATAL)
Done.
> On July 24, 2012, 6:49 p.m., Benjamin Hindman wrote:
> > src/tests/cgroups_tests.cpp, line 566
> > <https://reviews.apache.org/r/5840/diff/2/?file=123398#file123398line566>
> >
> > Did you want this to be a fatal thing? In the tests, use FAIL() << "message " << variable << " here";
Done.
> On July 24, 2012, 6:49 p.m., Benjamin Hindman wrote:
> > src/tests/cgroups_tests.cpp, line 575
> > <https://reviews.apache.org/r/5840/diff/2/?file=123398#file123398line575>
> >
> > Again, in the tests use FAIL().
Done.
> On July 24, 2012, 6:49 p.m., Benjamin Hindman wrote:
> > src/linux/cgroups.cpp, line 1443
> > <https://reviews.apache.org/r/5840/diff/2/?file=123397#file123397line1443>
> >
> > I think there is a 'discard' that takes a collection of futures (maybe a set not a list, but feel free to add the list version, or anything that can be const_iterator'ed for that matter.
Done. Add a list version.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5840/#review9403
-----------------------------------------------------------
On July 16, 2012, 7:19 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5840/
> -----------------------------------------------------------
>
> (Updated July 16, 2012, 7:19 p.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Description
> -------
>
> The patch leverages the atomic task kill patch
> https://reviews.apache.org/r/5402/
>
> It can destroy an active cgroup (and all its sub-cgroups) by killing all tasks and remove the cgroups in appropriate order.
>
> It uses the process::collect mechanism to parallelize the killing process.
>
>
> Diffs
> -----
>
> 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/5840/diff/
>
>
> Testing
> -------
>
> On linux machines, make check.
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request: Add an API to destroy a cgroup (and all its sub-cgroups).
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5840/#review9403
-----------------------------------------------------------
Again, comments from previous reviews apply here.
src/linux/cgroups.cpp
<https://reviews.apache.org/r/5840/#comment20141>
Kill newline.
src/linux/cgroups.cpp
<https://reviews.apache.org/r/5840/#comment20145>
I think there is a 'discard' that takes a collection of futures (maybe a set not a list, but feel free to add the list version, or anything that can be const_iterator'ed for that matter.
src/linux/cgroups.cpp
<https://reviews.apache.org/r/5840/#comment20142>
LOG(FATAL)
src/tests/cgroups_tests.cpp
<https://reviews.apache.org/r/5840/#comment20143>
Did you want this to be a fatal thing? In the tests, use FAIL() << "message " << variable << " here";
src/tests/cgroups_tests.cpp
<https://reviews.apache.org/r/5840/#comment20144>
Again, in the tests use FAIL().
- Benjamin Hindman
On July 16, 2012, 7:19 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5840/
> -----------------------------------------------------------
>
> (Updated July 16, 2012, 7:19 p.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Description
> -------
>
> The patch leverages the atomic task kill patch
> https://reviews.apache.org/r/5402/
>
> It can destroy an active cgroup (and all its sub-cgroups) by killing all tasks and remove the cgroups in appropriate order.
>
> It uses the process::collect mechanism to parallelize the killing process.
>
>
> Diffs
> -----
>
> 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/5840/diff/
>
>
> Testing
> -------
>
> On linux machines, make check.
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request: Add an API to destroy a cgroup (and all its sub-cgroups).
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5840/
-----------------------------------------------------------
(Updated July 16, 2012, 7:19 p.m.)
Review request for mesos and Benjamin Hindman.
Changes
-------
Updated to fit the new trunk.
Description
-------
The patch leverages the atomic task kill patch
https://reviews.apache.org/r/5402/
It can destroy an active cgroup (and all its sub-cgroups) by killing all tasks and remove the cgroups in appropriate order.
It uses the process::collect mechanism to parallelize the killing process.
Diffs (updated)
-----
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/5840/diff/
Testing
-------
On linux machines, make check.
Thanks,
Jie Yu