mesos-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Hindman <b...@berkeley.edu>
Subject Re: Review Request: Add an API for destroying a cgroup (atomically kill all the processes in it).
Date Tue, 03 Jul 2012 15:53:50 GMT
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 <yujie.jay@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 <yujie.jay@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 <benh@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/>
>>>
>>
>>
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message