You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Benjamin Hindman <be...@berkeley.edu> on 2012/07/02 07:18:31 UTC
Re: Review Request: Add an API for destroying a cgroup (atomically kill all
the processes in it).
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5402/#review8781
-----------------------------------------------------------
src/linux/cgroups.hpp
<https://reviews.apache.org/r/5402/#comment18586>
const & for the Option<double>.
src/linux/cgroups.hpp
<https://reviews.apache.org/r/5402/#comment18587>
Ditto.
src/linux/cgroups.cpp
<https://reviews.apache.org/r/5402/#comment18588>
virtual.
src/linux/cgroups.cpp
<https://reviews.apache.org/r/5402/#comment18589>
This method is also virtual -- also, any reason not to keep this protected?
src/linux/cgroups.cpp
<https://reviews.apache.org/r/5402/#comment18591>
Just like the 'EventListener', this should be a call to terminate.
src/linux/cgroups.cpp
<https://reviews.apache.org/r/5402/#comment18597>
Why not pass timeout into the constructor and put all this logic in 'initialize'?
src/linux/cgroups.cpp
<https://reviews.apache.org/r/5402/#comment18602>
Rather than keep this state, this is a great opportunity to fire off the 'killTasks' requests in parallel, then do a 'collect' on the futures!
src/linux/cgroups.cpp
<https://reviews.apache.org/r/5402/#comment18592>
terminate
src/linux/cgroups.cpp
<https://reviews.apache.org/r/5402/#comment18593>
terminate
src/linux/cgroups.cpp
<https://reviews.apache.org/r/5402/#comment18601>
const &
src/linux/cgroups.cpp
<https://reviews.apache.org/r/5402/#comment18594>
terminate
src/linux/cgroups.cpp
<https://reviews.apache.org/r/5402/#comment18595>
terminate
src/linux/cgroups.cpp
<https://reviews.apache.org/r/5402/#comment18596>
This function can be removed.
src/linux/cgroups.cpp
<https://reviews.apache.org/r/5402/#comment18599>
Just return the future, no need for the Try too.
src/linux/cgroups.cpp
<https://reviews.apache.org/r/5402/#comment18598>
See comment above, this extra dispatch can be removed.
- Benjamin Hindman
On June 21, 2012, 7:42 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5402/
> -----------------------------------------------------------
>
> (Updated June 21, 2012, 7:42 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Vinod Kone.
>
>
> Description
> -------
>
> This patch leverages the freezer subsystem in cgroups to kill all the processes in a cgroup atomically.
>
> The main idea is to freeze all the processes in a cgroup first, then send kill signal to all the proceses. This avoids the need of walking the proc process tree to kill all processes associated with an executor. In fact, the original killtree solution assumes that the user processes haven't blocked the SIGSTOP signal, which may not be true in some cases.
>
>
> 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/5402/diff/
>
>
> Testing
> -------
>
> On Linux machine, make check.
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request: Add an API to atomically kill all tasks in a cgroup.
Posted by Jie Yu <yu...@gmail.com>.
> On July 2, 2012, 5:18 a.m., Benjamin Hindman wrote:
> > src/linux/cgroups.cpp, line 615
> > <https://reviews.apache.org/r/5402/diff/5/?file=115747#file115747line615>
> >
> > Rather than keep this state, this is a great opportunity to fire off the 'killTasks' requests in parallel, then do a 'collect' on the futures!
Good idea! I will do it in the destroyer patch (the current patch only contains atomic kill stuff).
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5402/#review8781
-----------------------------------------------------------
On July 5, 2012, 10:07 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5402/
> -----------------------------------------------------------
>
> (Updated July 5, 2012, 10:07 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Vinod Kone.
>
>
> Description
> -------
>
> This patch leverages the freezer subsystem in cgroups to kill all the processes in a cgroup atomically.
>
> The main idea is to freeze all the processes in a cgroup first, then send kill signal to all the proceses. This avoids the need of walking the proc process tree to kill all processes associated with an executor. In fact, the original killtree solution assumes that the user processes haven't blocked the SIGSTOP signal, which may not be true in some cases.
>
>
> 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/5402/diff/
>
>
> Testing
> -------
>
> On Linux machine, make check.
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request: Add an API for destroying a cgroup (atomically
kill all the processes in it).
Posted by Jie Yu <yu...@gmail.com>.
Yes. In that case, timeout is actually controlled by its caller.
- Jie
On Tue, Jul 3, 2012 at 8:53 AM, Benjamin Hindman <be...@berkeley.edu> wrote:
> As in, if the user discards the future, the destroy/freeze operation will
> be aborted?
>
> This sounds fine to me.
>
>
>
> On Mon, Jul 2, 2012 at 6:11 PM, Jie Yu <yu...@gmail.com> wrote:
>
>> Ben,
>>
>> I plan to remove the timeout parameter and let user decide when to cancel
>> the destroy (or freeze) operation.
>>
>> What do you think?
>>
>> - Jie
>>
>>
>> On Mon, Jul 2, 2012 at 5:34 PM, Jie Yu <yu...@gmail.com> wrote:
>>
>>> Ben,
>>>
>>> This patch depends on the freezer patch. Could you please review the
>>> freezer utils patch first? Thanks! (I will address the comments in this
>>> review anyway).
>>>
>>> https://reviews.apache.org/r/5401/
>>>
>>> - Jie
>>>
>>>
>>> On Sun, Jul 1, 2012 at 10:18 PM, Benjamin Hindman <be...@berkeley.edu>wrote:
>>>
>>>> This is an automatically generated e-mail. To reply, visit:
>>>> https://reviews.apache.org/r/5402/
>>>> src/linux/cgroups.hpp<https://reviews.apache.org/r/5402/diff/5/?file=115746#file115746line169> (Diff
>>>> revision 5)
>>>>
>>>> Try<process::Future<bool> > destroyCgroup(
>>>>
>>>> 169
>>>>
>>>> Option<double> timeout = Option<double>::none());
>>>>
>>>> const & for the Option<double>.
>>>>
>>>>
>>>> src/linux/cgroups.hpp<https://reviews.apache.org/r/5402/diff/5/?file=115746#file115746line273> (Diff
>>>> revision 5)
>>>>
>>>> Try<process::Future<bool> > killTasks(
>>>>
>>>> 273
>>>>
>>>> Option<double> timeout = Option<double>::none());
>>>>
>>>> Ditto.
>>>>
>>>>
>>>> src/linux/cgroups.cpp<https://reviews.apache.org/r/5402/diff/5/?file=115747#file115747line601> (Diff
>>>> revision 5)
>>>>
>>>> public:
>>>>
>>>> 601
>>>>
>>>> ~CgroupDestroyer() {}
>>>>
>>>> virtual.
>>>>
>>>>
>>>> src/linux/cgroups.cpp<https://reviews.apache.org/r/5402/diff/5/?file=115747#file115747line603> (Diff
>>>> revision 5)
>>>>
>>>> public:
>>>>
>>>> 603
>>>>
>>>> void initialize()
>>>>
>>>> This method is also virtual -- also, any reason not to keep this protected?
>>>>
>>>>
>>>> src/linux/cgroups.cpp<https://reviews.apache.org/r/5402/diff/5/?file=115747#file115747line606> (Diff
>>>> revision 5)
>>>>
>>>> public:
>>>>
>>>> 606
>>>>
>>>> state().onDiscarded(process::defer(this, &CgroupDestroyer::stop));
>>>>
>>>> Just like the 'EventListener', this should be a call to terminate.
>>>>
>>>>
>>>> src/linux/cgroups.cpp<https://reviews.apache.org/r/5402/diff/5/?file=115747#file115747line613> (Diff
>>>> revision 5)
>>>>
>>>> public:
>>>>
>>>> 613
>>>>
>>>> void start(Option<double> timeout)
>>>>
>>>> Why not pass timeout into the constructor and put all this logic in 'initialize'?
>>>>
>>>>
>>>> src/linux/cgroups.cpp<https://reviews.apache.org/r/5402/diff/5/?file=115747#file115747line615> (Diff
>>>> revision 5)
>>>>
>>>> public:
>>>>
>>>> 615
>>>>
>>>> if (index >= cgroups.size()){
>>>>
>>>> Rather than keep this state, this is a great opportunity to fire off the 'killTasks' requests in parallel, then do a 'collect' on the futures!
>>>>
>>>>
>>>> src/linux/cgroups.cpp<https://reviews.apache.org/r/5402/diff/5/?file=115747#file115747line617> (Diff
>>>> revision 5)
>>>>
>>>> public:
>>>>
>>>> 617
>>>>
>>>> process::dispatch(this, &CgroupDestroyer::stop);
>>>>
>>>> terminate
>>>>
>>>>
>>>> src/linux/cgroups.cpp<https://reviews.apache.org/r/5402/diff/5/?file=115747#file115747line627> (Diff
>>>> revision 5)
>>>>
>>>> public:
>>>>
>>>> 627
>>>>
>>>> process::dispatch(this, &CgroupDestroyer::stop);
>>>>
>>>> terminate
>>>>
>>>>
>>>> src/linux/cgroups.cpp<https://reviews.apache.org/r/5402/diff/5/?file=115747#file115747line642> (Diff
>>>> revision 5)
>>>>
>>>> public:
>>>>
>>>> 642
>>>>
>>>> Option<double> timeout)
>>>>
>>>> const &
>>>>
>>>>
>>>> src/linux/cgroups.cpp<https://reviews.apache.org/r/5402/diff/5/?file=115747#file115747line648> (Diff
>>>> revision 5)
>>>>
>>>> public:
>>>>
>>>> 648
>>>>
>>>> process::dispatch(this, &CgroupDestroyer::stop);
>>>>
>>>> terminate
>>>>
>>>>
>>>> src/linux/cgroups.cpp<https://reviews.apache.org/r/5402/diff/5/?file=115747#file115747line655> (Diff
>>>> revision 5)
>>>>
>>>> public:
>>>>
>>>> 655
>>>>
>>>> process::dispatch(this, &CgroupDestroyer::stop);
>>>>
>>>> terminate
>>>>
>>>>
>>>> src/linux/cgroups.cpp<https://reviews.apache.org/r/5402/diff/5/?file=115747#file115747line659> (Diff
>>>> revision 5)
>>>>
>>>> public:
>>>>
>>>> 659
>>>>
>>>> void stop()
>>>>
>>>> This function can be removed.
>>>>
>>>>
>>>> src/linux/cgroups.cpp<https://reviews.apache.org/r/5402/diff/5/?file=115747#file115747line674> (Diff
>>>> revision 5)
>>>>
>>>> public:
>>>>
>>>> 674
>>>>
>>>> Try<process::Future<bool> > destroyCgroup(const std::string& hierarchy,
>>>>
>>>> Just return the future, no need for the Try too.
>>>>
>>>>
>>>> src/linux/cgroups.cpp<https://reviews.apache.org/r/5402/diff/5/?file=115747#file115747line705> (Diff
>>>> revision 5)
>>>>
>>>> public:
>>>>
>>>> 705
>>>>
>>>> process::dispatch(destroyer, &internal::CgroupDestroyer::start, timeout);
>>>>
>>>> See comment above, this extra dispatch can be removed.
>>>>
>>>>
>>>> - Benjamin
>>>>
>>>> On June 21st, 2012, 7:42 p.m., Jie Yu wrote:
>>>> Review request for mesos, Benjamin Hindman and Vinod Kone.
>>>> By Jie Yu.
>>>>
>>>> *Updated June 21, 2012, 7:42 p.m.*
>>>> Description
>>>>
>>>> This patch leverages the freezer subsystem in cgroups to kill all the processes in a cgroup atomically.
>>>>
>>>> The main idea is to freeze all the processes in a cgroup first, then send kill signal to all the proceses. This avoids the need of walking the proc process tree to kill all processes associated with an executor. In fact, the original killtree solution assumes that the user processes haven't blocked the SIGSTOP signal, which may not be true in some cases.
>>>>
>>>> Testing
>>>>
>>>> On Linux machine, make check.
>>>>
>>>> Diffs
>>>>
>>>> - src/linux/cgroups.hpp (PRE-CREATION)
>>>> - src/linux/cgroups.cpp (PRE-CREATION)
>>>> - src/tests/cgroups_tests.cpp (PRE-CREATION)
>>>>
>>>> View Diff <https://reviews.apache.org/r/5402/diff/>
>>>>
>>>
>>>
>>
>
Re: Review Request: Add an API for destroying a cgroup (atomically
kill all the processes in it).
Posted by Benjamin Hindman <be...@berkeley.edu>.
As in, if the user discards the future, the destroy/freeze operation will
be aborted?
This sounds fine to me.
On Mon, Jul 2, 2012 at 6:11 PM, Jie Yu <yu...@gmail.com> wrote:
> Ben,
>
> I plan to remove the timeout parameter and let user decide when to cancel
> the destroy (or freeze) operation.
>
> What do you think?
>
> - Jie
>
>
> On Mon, Jul 2, 2012 at 5:34 PM, Jie Yu <yu...@gmail.com> wrote:
>
>> Ben,
>>
>> This patch depends on the freezer patch. Could you please review the
>> freezer utils patch first? Thanks! (I will address the comments in this
>> review anyway).
>>
>> https://reviews.apache.org/r/5401/
>>
>> - Jie
>>
>>
>> On Sun, Jul 1, 2012 at 10:18 PM, Benjamin Hindman <be...@berkeley.edu>wrote:
>>
>>> This is an automatically generated e-mail. To reply, visit:
>>> https://reviews.apache.org/r/5402/
>>> src/linux/cgroups.hpp<https://reviews.apache.org/r/5402/diff/5/?file=115746#file115746line169> (Diff
>>> revision 5)
>>>
>>> Try<process::Future<bool> > destroyCgroup(
>>>
>>> 169
>>>
>>> Option<double> timeout = Option<double>::none());
>>>
>>> const & for the Option<double>.
>>>
>>>
>>> src/linux/cgroups.hpp<https://reviews.apache.org/r/5402/diff/5/?file=115746#file115746line273> (Diff
>>> revision 5)
>>>
>>> Try<process::Future<bool> > killTasks(
>>>
>>> 273
>>>
>>> Option<double> timeout = Option<double>::none());
>>>
>>> Ditto.
>>>
>>>
>>> src/linux/cgroups.cpp<https://reviews.apache.org/r/5402/diff/5/?file=115747#file115747line601> (Diff
>>> revision 5)
>>>
>>> public:
>>>
>>> 601
>>>
>>> ~CgroupDestroyer() {}
>>>
>>> virtual.
>>>
>>>
>>> src/linux/cgroups.cpp<https://reviews.apache.org/r/5402/diff/5/?file=115747#file115747line603> (Diff
>>> revision 5)
>>>
>>> public:
>>>
>>> 603
>>>
>>> void initialize()
>>>
>>> This method is also virtual -- also, any reason not to keep this protected?
>>>
>>>
>>> src/linux/cgroups.cpp<https://reviews.apache.org/r/5402/diff/5/?file=115747#file115747line606> (Diff
>>> revision 5)
>>>
>>> public:
>>>
>>> 606
>>>
>>> state().onDiscarded(process::defer(this, &CgroupDestroyer::stop));
>>>
>>> Just like the 'EventListener', this should be a call to terminate.
>>>
>>>
>>> src/linux/cgroups.cpp<https://reviews.apache.org/r/5402/diff/5/?file=115747#file115747line613> (Diff
>>> revision 5)
>>>
>>> public:
>>>
>>> 613
>>>
>>> void start(Option<double> timeout)
>>>
>>> Why not pass timeout into the constructor and put all this logic in 'initialize'?
>>>
>>>
>>> src/linux/cgroups.cpp<https://reviews.apache.org/r/5402/diff/5/?file=115747#file115747line615> (Diff
>>> revision 5)
>>>
>>> public:
>>>
>>> 615
>>>
>>> if (index >= cgroups.size()){
>>>
>>> Rather than keep this state, this is a great opportunity to fire off the 'killTasks' requests in parallel, then do a 'collect' on the futures!
>>>
>>>
>>> src/linux/cgroups.cpp<https://reviews.apache.org/r/5402/diff/5/?file=115747#file115747line617> (Diff
>>> revision 5)
>>>
>>> public:
>>>
>>> 617
>>>
>>> process::dispatch(this, &CgroupDestroyer::stop);
>>>
>>> terminate
>>>
>>>
>>> src/linux/cgroups.cpp<https://reviews.apache.org/r/5402/diff/5/?file=115747#file115747line627> (Diff
>>> revision 5)
>>>
>>> public:
>>>
>>> 627
>>>
>>> process::dispatch(this, &CgroupDestroyer::stop);
>>>
>>> terminate
>>>
>>>
>>> src/linux/cgroups.cpp<https://reviews.apache.org/r/5402/diff/5/?file=115747#file115747line642> (Diff
>>> revision 5)
>>>
>>> public:
>>>
>>> 642
>>>
>>> Option<double> timeout)
>>>
>>> const &
>>>
>>>
>>> src/linux/cgroups.cpp<https://reviews.apache.org/r/5402/diff/5/?file=115747#file115747line648> (Diff
>>> revision 5)
>>>
>>> public:
>>>
>>> 648
>>>
>>> process::dispatch(this, &CgroupDestroyer::stop);
>>>
>>> terminate
>>>
>>>
>>> src/linux/cgroups.cpp<https://reviews.apache.org/r/5402/diff/5/?file=115747#file115747line655> (Diff
>>> revision 5)
>>>
>>> public:
>>>
>>> 655
>>>
>>> process::dispatch(this, &CgroupDestroyer::stop);
>>>
>>> terminate
>>>
>>>
>>> src/linux/cgroups.cpp<https://reviews.apache.org/r/5402/diff/5/?file=115747#file115747line659> (Diff
>>> revision 5)
>>>
>>> public:
>>>
>>> 659
>>>
>>> void stop()
>>>
>>> This function can be removed.
>>>
>>>
>>> src/linux/cgroups.cpp<https://reviews.apache.org/r/5402/diff/5/?file=115747#file115747line674> (Diff
>>> revision 5)
>>>
>>> public:
>>>
>>> 674
>>>
>>> Try<process::Future<bool> > destroyCgroup(const std::string& hierarchy,
>>>
>>> Just return the future, no need for the Try too.
>>>
>>>
>>> src/linux/cgroups.cpp<https://reviews.apache.org/r/5402/diff/5/?file=115747#file115747line705> (Diff
>>> revision 5)
>>>
>>> public:
>>>
>>> 705
>>>
>>> process::dispatch(destroyer, &internal::CgroupDestroyer::start, timeout);
>>>
>>> See comment above, this extra dispatch can be removed.
>>>
>>>
>>> - Benjamin
>>>
>>> On June 21st, 2012, 7:42 p.m., Jie Yu wrote:
>>> Review request for mesos, Benjamin Hindman and Vinod Kone.
>>> By Jie Yu.
>>>
>>> *Updated June 21, 2012, 7:42 p.m.*
>>> Description
>>>
>>> This patch leverages the freezer subsystem in cgroups to kill all the processes in a cgroup atomically.
>>>
>>> The main idea is to freeze all the processes in a cgroup first, then send kill signal to all the proceses. This avoids the need of walking the proc process tree to kill all processes associated with an executor. In fact, the original killtree solution assumes that the user processes haven't blocked the SIGSTOP signal, which may not be true in some cases.
>>>
>>> Testing
>>>
>>> On Linux machine, make check.
>>>
>>> Diffs
>>>
>>> - src/linux/cgroups.hpp (PRE-CREATION)
>>> - src/linux/cgroups.cpp (PRE-CREATION)
>>> - src/tests/cgroups_tests.cpp (PRE-CREATION)
>>>
>>> View Diff <https://reviews.apache.org/r/5402/diff/>
>>>
>>
>>
>
Re: Review Request: Add an API for destroying a cgroup (atomically
kill all the processes in it).
Posted by Jie Yu <yu...@gmail.com>.
Ben,
I plan to remove the timeout parameter and let user decide when to cancel
the destroy (or freeze) operation.
What do you think?
- Jie
On Mon, Jul 2, 2012 at 5:34 PM, Jie Yu <yu...@gmail.com> wrote:
> Ben,
>
> This patch depends on the freezer patch. Could you please review the
> freezer utils patch first? Thanks! (I will address the comments in this
> review anyway).
>
> https://reviews.apache.org/r/5401/
>
> - Jie
>
>
> On Sun, Jul 1, 2012 at 10:18 PM, Benjamin Hindman <be...@berkeley.edu>wrote:
>
>> This is an automatically generated e-mail. To reply, visit:
>> https://reviews.apache.org/r/5402/
>> src/linux/cgroups.hpp<https://reviews.apache.org/r/5402/diff/5/?file=115746#file115746line169> (Diff
>> revision 5)
>>
>> Try<process::Future<bool> > destroyCgroup(
>>
>> 169
>>
>> Option<double> timeout = Option<double>::none());
>>
>> const & for the Option<double>.
>>
>>
>> src/linux/cgroups.hpp<https://reviews.apache.org/r/5402/diff/5/?file=115746#file115746line273> (Diff
>> revision 5)
>>
>> Try<process::Future<bool> > killTasks(
>>
>> 273
>>
>> Option<double> timeout = Option<double>::none());
>>
>> Ditto.
>>
>>
>> src/linux/cgroups.cpp<https://reviews.apache.org/r/5402/diff/5/?file=115747#file115747line601> (Diff
>> revision 5)
>>
>> public:
>>
>> 601
>>
>> ~CgroupDestroyer() {}
>>
>> virtual.
>>
>>
>> src/linux/cgroups.cpp<https://reviews.apache.org/r/5402/diff/5/?file=115747#file115747line603> (Diff
>> revision 5)
>>
>> public:
>>
>> 603
>>
>> void initialize()
>>
>> This method is also virtual -- also, any reason not to keep this protected?
>>
>>
>> src/linux/cgroups.cpp<https://reviews.apache.org/r/5402/diff/5/?file=115747#file115747line606> (Diff
>> revision 5)
>>
>> public:
>>
>> 606
>>
>> state().onDiscarded(process::defer(this, &CgroupDestroyer::stop));
>>
>> Just like the 'EventListener', this should be a call to terminate.
>>
>>
>> src/linux/cgroups.cpp<https://reviews.apache.org/r/5402/diff/5/?file=115747#file115747line613> (Diff
>> revision 5)
>>
>> public:
>>
>> 613
>>
>> void start(Option<double> timeout)
>>
>> Why not pass timeout into the constructor and put all this logic in 'initialize'?
>>
>>
>> src/linux/cgroups.cpp<https://reviews.apache.org/r/5402/diff/5/?file=115747#file115747line615> (Diff
>> revision 5)
>>
>> public:
>>
>> 615
>>
>> if (index >= cgroups.size()){
>>
>> Rather than keep this state, this is a great opportunity to fire off the 'killTasks' requests in parallel, then do a 'collect' on the futures!
>>
>>
>> src/linux/cgroups.cpp<https://reviews.apache.org/r/5402/diff/5/?file=115747#file115747line617> (Diff
>> revision 5)
>>
>> public:
>>
>> 617
>>
>> process::dispatch(this, &CgroupDestroyer::stop);
>>
>> terminate
>>
>>
>> src/linux/cgroups.cpp<https://reviews.apache.org/r/5402/diff/5/?file=115747#file115747line627> (Diff
>> revision 5)
>>
>> public:
>>
>> 627
>>
>> process::dispatch(this, &CgroupDestroyer::stop);
>>
>> terminate
>>
>>
>> src/linux/cgroups.cpp<https://reviews.apache.org/r/5402/diff/5/?file=115747#file115747line642> (Diff
>> revision 5)
>>
>> public:
>>
>> 642
>>
>> Option<double> timeout)
>>
>> const &
>>
>>
>> src/linux/cgroups.cpp<https://reviews.apache.org/r/5402/diff/5/?file=115747#file115747line648> (Diff
>> revision 5)
>>
>> public:
>>
>> 648
>>
>> process::dispatch(this, &CgroupDestroyer::stop);
>>
>> terminate
>>
>>
>> src/linux/cgroups.cpp<https://reviews.apache.org/r/5402/diff/5/?file=115747#file115747line655> (Diff
>> revision 5)
>>
>> public:
>>
>> 655
>>
>> process::dispatch(this, &CgroupDestroyer::stop);
>>
>> terminate
>>
>>
>> src/linux/cgroups.cpp<https://reviews.apache.org/r/5402/diff/5/?file=115747#file115747line659> (Diff
>> revision 5)
>>
>> public:
>>
>> 659
>>
>> void stop()
>>
>> This function can be removed.
>>
>>
>> src/linux/cgroups.cpp<https://reviews.apache.org/r/5402/diff/5/?file=115747#file115747line674> (Diff
>> revision 5)
>>
>> public:
>>
>> 674
>>
>> Try<process::Future<bool> > destroyCgroup(const std::string& hierarchy,
>>
>> Just return the future, no need for the Try too.
>>
>>
>> src/linux/cgroups.cpp<https://reviews.apache.org/r/5402/diff/5/?file=115747#file115747line705> (Diff
>> revision 5)
>>
>> public:
>>
>> 705
>>
>> process::dispatch(destroyer, &internal::CgroupDestroyer::start, timeout);
>>
>> See comment above, this extra dispatch can be removed.
>>
>>
>> - Benjamin
>>
>> On June 21st, 2012, 7:42 p.m., Jie Yu wrote:
>> Review request for mesos, Benjamin Hindman and Vinod Kone.
>> By Jie Yu.
>>
>> *Updated June 21, 2012, 7:42 p.m.*
>> Description
>>
>> This patch leverages the freezer subsystem in cgroups to kill all the processes in a cgroup atomically.
>>
>> The main idea is to freeze all the processes in a cgroup first, then send kill signal to all the proceses. This avoids the need of walking the proc process tree to kill all processes associated with an executor. In fact, the original killtree solution assumes that the user processes haven't blocked the SIGSTOP signal, which may not be true in some cases.
>>
>> Testing
>>
>> On Linux machine, make check.
>>
>> Diffs
>>
>> - src/linux/cgroups.hpp (PRE-CREATION)
>> - src/linux/cgroups.cpp (PRE-CREATION)
>> - src/tests/cgroups_tests.cpp (PRE-CREATION)
>>
>> View Diff <https://reviews.apache.org/r/5402/diff/>
>>
>
>
Re: Review Request: Add an API for destroying a cgroup (atomically
kill all the processes in it).
Posted by Jie Yu <yu...@gmail.com>.
Ben,
This patch depends on the freezer patch. Could you please review the
freezer utils patch first? Thanks! (I will address the comments in this
review anyway).
https://reviews.apache.org/r/5401/
- Jie
On Sun, Jul 1, 2012 at 10:18 PM, Benjamin Hindman <be...@berkeley.edu> wrote:
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5402/
> src/linux/cgroups.hpp<https://reviews.apache.org/r/5402/diff/5/?file=115746#file115746line169> (Diff
> revision 5)
>
> Try<process::Future<bool> > destroyCgroup(
>
> 169
>
> Option<double> timeout = Option<double>::none());
>
> const & for the Option<double>.
>
>
> src/linux/cgroups.hpp<https://reviews.apache.org/r/5402/diff/5/?file=115746#file115746line273> (Diff
> revision 5)
>
> Try<process::Future<bool> > killTasks(
>
> 273
>
> Option<double> timeout = Option<double>::none());
>
> Ditto.
>
>
> src/linux/cgroups.cpp<https://reviews.apache.org/r/5402/diff/5/?file=115747#file115747line601> (Diff
> revision 5)
>
> public:
>
> 601
>
> ~CgroupDestroyer() {}
>
> virtual.
>
>
> src/linux/cgroups.cpp<https://reviews.apache.org/r/5402/diff/5/?file=115747#file115747line603> (Diff
> revision 5)
>
> public:
>
> 603
>
> void initialize()
>
> This method is also virtual -- also, any reason not to keep this protected?
>
>
> src/linux/cgroups.cpp<https://reviews.apache.org/r/5402/diff/5/?file=115747#file115747line606> (Diff
> revision 5)
>
> public:
>
> 606
>
> state().onDiscarded(process::defer(this, &CgroupDestroyer::stop));
>
> Just like the 'EventListener', this should be a call to terminate.
>
>
> src/linux/cgroups.cpp<https://reviews.apache.org/r/5402/diff/5/?file=115747#file115747line613> (Diff
> revision 5)
>
> public:
>
> 613
>
> void start(Option<double> timeout)
>
> Why not pass timeout into the constructor and put all this logic in 'initialize'?
>
>
> src/linux/cgroups.cpp<https://reviews.apache.org/r/5402/diff/5/?file=115747#file115747line615> (Diff
> revision 5)
>
> public:
>
> 615
>
> if (index >= cgroups.size()){
>
> Rather than keep this state, this is a great opportunity to fire off the 'killTasks' requests in parallel, then do a 'collect' on the futures!
>
>
> src/linux/cgroups.cpp<https://reviews.apache.org/r/5402/diff/5/?file=115747#file115747line617> (Diff
> revision 5)
>
> public:
>
> 617
>
> process::dispatch(this, &CgroupDestroyer::stop);
>
> terminate
>
>
> src/linux/cgroups.cpp<https://reviews.apache.org/r/5402/diff/5/?file=115747#file115747line627> (Diff
> revision 5)
>
> public:
>
> 627
>
> process::dispatch(this, &CgroupDestroyer::stop);
>
> terminate
>
>
> src/linux/cgroups.cpp<https://reviews.apache.org/r/5402/diff/5/?file=115747#file115747line642> (Diff
> revision 5)
>
> public:
>
> 642
>
> Option<double> timeout)
>
> const &
>
>
> src/linux/cgroups.cpp<https://reviews.apache.org/r/5402/diff/5/?file=115747#file115747line648> (Diff
> revision 5)
>
> public:
>
> 648
>
> process::dispatch(this, &CgroupDestroyer::stop);
>
> terminate
>
>
> src/linux/cgroups.cpp<https://reviews.apache.org/r/5402/diff/5/?file=115747#file115747line655> (Diff
> revision 5)
>
> public:
>
> 655
>
> process::dispatch(this, &CgroupDestroyer::stop);
>
> terminate
>
>
> src/linux/cgroups.cpp<https://reviews.apache.org/r/5402/diff/5/?file=115747#file115747line659> (Diff
> revision 5)
>
> public:
>
> 659
>
> void stop()
>
> This function can be removed.
>
>
> src/linux/cgroups.cpp<https://reviews.apache.org/r/5402/diff/5/?file=115747#file115747line674> (Diff
> revision 5)
>
> public:
>
> 674
>
> Try<process::Future<bool> > destroyCgroup(const std::string& hierarchy,
>
> Just return the future, no need for the Try too.
>
>
> src/linux/cgroups.cpp<https://reviews.apache.org/r/5402/diff/5/?file=115747#file115747line705> (Diff
> revision 5)
>
> public:
>
> 705
>
> process::dispatch(destroyer, &internal::CgroupDestroyer::start, timeout);
>
> See comment above, this extra dispatch can be removed.
>
>
> - Benjamin
>
> On June 21st, 2012, 7:42 p.m., Jie Yu wrote:
> Review request for mesos, Benjamin Hindman and Vinod Kone.
> By Jie Yu.
>
> *Updated June 21, 2012, 7:42 p.m.*
> Description
>
> This patch leverages the freezer subsystem in cgroups to kill all the processes in a cgroup atomically.
>
> The main idea is to freeze all the processes in a cgroup first, then send kill signal to all the proceses. This avoids the need of walking the proc process tree to kill all processes associated with an executor. In fact, the original killtree solution assumes that the user processes haven't blocked the SIGSTOP signal, which may not be true in some cases.
>
> Testing
>
> On Linux machine, make check.
>
> Diffs
>
> - src/linux/cgroups.hpp (PRE-CREATION)
> - src/linux/cgroups.cpp (PRE-CREATION)
> - src/tests/cgroups_tests.cpp (PRE-CREATION)
>
> View Diff <https://reviews.apache.org/r/5402/diff/>
>