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/02/14 01:47:25 UTC
Re: Review Request: Properly cleanup cgroups.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9408/
-----------------------------------------------------------
(Updated Feb. 14, 2013, 12:47 a.m.)
Review request for mesos, Benjamin Hindman and Ben Mahler.
Changes
-------
Isolation modules also properly cleanup executors now!
Summary (updated)
-----------------
Properly cleanup cgroups.
Description
-------
See summary
Diffs (updated)
-----
src/slave/cgroups_isolation_module.hpp 669efa14ba2603764aa68ae19a44e79dbfdec192
src/slave/cgroups_isolation_module.cpp 14f549edaf1b37a6bca8f75309864333ae775e7c
src/slave/process_based_isolation_module.hpp f1817192582e3646f8dcf17934ba7998829e8fd6
src/slave/process_based_isolation_module.cpp 12a579cba56cd3dac384bc7919b0d5537b0e429d
src/tests/balloon_framework_test.sh 93a733f64cfde08349b7781eb3d5e13594c74498
Diff: https://reviews.apache.org/r/9408/diff/
Testing (updated)
-------
make check
Thanks,
Vinod Kone
Re: Review Request: Properly cleanup cgroups.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9408/#review16545
-----------------------------------------------------------
src/slave/cgroups_isolation_module.hpp
<https://reviews.apache.org/r/9408/#comment35112>
Option instead?
src/slave/cgroups_isolation_module.cpp
<https://reviews.apache.org/r/9408/#comment35113>
s/!=0/!= 0/
src/tests/balloon_framework_test.sh
<https://reviews.apache.org/r/9408/#comment35114>
- Ben Mahler
On Feb. 14, 2013, 12:47 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9408/
> -----------------------------------------------------------
>
> (Updated Feb. 14, 2013, 12:47 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Description
> -------
>
> See summary
>
>
> Diffs
> -----
>
> src/slave/cgroups_isolation_module.hpp 669efa14ba2603764aa68ae19a44e79dbfdec192
> src/slave/cgroups_isolation_module.cpp 14f549edaf1b37a6bca8f75309864333ae775e7c
> src/slave/process_based_isolation_module.hpp f1817192582e3646f8dcf17934ba7998829e8fd6
> src/slave/process_based_isolation_module.cpp 12a579cba56cd3dac384bc7919b0d5537b0e429d
> src/tests/balloon_framework_test.sh 93a733f64cfde08349b7781eb3d5e13594c74498
>
> Diff: https://reviews.apache.org/r/9408/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request: Properly cleanup cgroups.
Posted by Ben Mahler <be...@gmail.com>.
> On Feb. 14, 2013, 2:13 a.m., Ben Mahler wrote:
> > src/slave/cgroups_isolation_module.hpp, line 255
> > <https://reviews.apache.org/r/9408/diff/5/?file=258653#file258653line255>
> >
> > Can you use a more descriptive name here?
> >
> > e.g.
> > lock
> > advisoryLock
>
> Vinod Kone wrote:
> i find it weird to name a file descriptor 'lock'. we are placing a lock on the file.
>
> but i agree for the need for a more descriptive name. changed it to lockFile.
>
> n.b: flock places the lock on the file not on the fd.
I see, but we still need to hold the fd right?
"If a process uses open(2) (or similar) to obtain more than one descriptor for the same file, these descriptors are treated independently by flock(). An attempt to lock the file using one of these file descriptors may be denied by a lock that the calling process has already placed via another descriptor."
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9408/#review16557
-----------------------------------------------------------
On Feb. 14, 2013, 7:16 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9408/
> -----------------------------------------------------------
>
> (Updated Feb. 14, 2013, 7:16 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Description
> -------
>
> See summary
>
>
> Diffs
> -----
>
> src/slave/cgroups_isolation_module.hpp 669efa14ba2603764aa68ae19a44e79dbfdec192
> src/slave/cgroups_isolation_module.cpp 14f549edaf1b37a6bca8f75309864333ae775e7c
> src/slave/process_based_isolation_module.hpp f1817192582e3646f8dcf17934ba7998829e8fd6
> src/slave/process_based_isolation_module.cpp 12a579cba56cd3dac384bc7919b0d5537b0e429d
> src/tests/balloon_framework_test.sh 93a733f64cfde08349b7781eb3d5e13594c74498
>
> Diff: https://reviews.apache.org/r/9408/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request: Properly cleanup cgroups.
Posted by Vinod Kone <vi...@gmail.com>.
> On Feb. 14, 2013, 2:13 a.m., Ben Mahler wrote:
> > src/slave/cgroups_isolation_module.hpp, line 255
> > <https://reviews.apache.org/r/9408/diff/5/?file=258653#file258653line255>
> >
> > Can you use a more descriptive name here?
> >
> > e.g.
> > lock
> > advisoryLock
i find it weird to name a file descriptor 'lock'. we are placing a lock on the file.
but i agree for the need for a more descriptive name. changed it to lockFile.
n.b: flock places the lock on the file not on the fd.
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9408/#review16557
-----------------------------------------------------------
On Feb. 14, 2013, 1:38 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9408/
> -----------------------------------------------------------
>
> (Updated Feb. 14, 2013, 1:38 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Description
> -------
>
> See summary
>
>
> Diffs
> -----
>
> src/slave/cgroups_isolation_module.hpp 669efa14ba2603764aa68ae19a44e79dbfdec192
> src/slave/cgroups_isolation_module.cpp 14f549edaf1b37a6bca8f75309864333ae775e7c
> src/slave/process_based_isolation_module.hpp f1817192582e3646f8dcf17934ba7998829e8fd6
> src/slave/process_based_isolation_module.cpp 12a579cba56cd3dac384bc7919b0d5537b0e429d
> src/tests/balloon_framework_test.sh 93a733f64cfde08349b7781eb3d5e13594c74498
>
> Diff: https://reviews.apache.org/r/9408/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request: Properly cleanup cgroups.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9408/#review16557
-----------------------------------------------------------
src/slave/cgroups_isolation_module.hpp
<https://reviews.apache.org/r/9408/#comment35128>
Can you use a more descriptive name here?
e.g.
lock
advisoryLock
- Ben Mahler
On Feb. 14, 2013, 1:38 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9408/
> -----------------------------------------------------------
>
> (Updated Feb. 14, 2013, 1:38 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Description
> -------
>
> See summary
>
>
> Diffs
> -----
>
> src/slave/cgroups_isolation_module.hpp 669efa14ba2603764aa68ae19a44e79dbfdec192
> src/slave/cgroups_isolation_module.cpp 14f549edaf1b37a6bca8f75309864333ae775e7c
> src/slave/process_based_isolation_module.hpp f1817192582e3646f8dcf17934ba7998829e8fd6
> src/slave/process_based_isolation_module.cpp 12a579cba56cd3dac384bc7919b0d5537b0e429d
> src/tests/balloon_framework_test.sh 93a733f64cfde08349b7781eb3d5e13594c74498
>
> Diff: https://reviews.apache.org/r/9408/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request: Properly cleanup cgroups.
Posted by Ben Mahler <be...@gmail.com>.
> On Feb. 20, 2013, 12:54 a.m., Ben Mahler wrote:
> > src/slave/cgroups_isolation_module.cpp, line 437
> > <https://reviews.apache.org/r/9408/diff/6/?file=258837#file258837line437>
> >
> > EXIT is used for messages that the mesos user / operator would read and need to take action in regard to. Given this is for orphaned executors, can you add more information to the message here to describe what could be wrong (so as to guide the user)?
>
> Vinod Kone wrote:
> There are multiple reasons a cgroup 'destroy' fails, as you can see in cgroups::destroy(). And, no, its not because another slave is running (see below). Afaict, it is not possible to tell the reason from the call site. Any ideas?
Ah thanks for explaining, sounds like this is unexpected and therefore should be a CHECK or LOG(FATAL)? Sounds like something we don't anticipate happening so EXIT seems to be the wrong technique to me.
> On Feb. 20, 2013, 12:54 a.m., Ben Mahler wrote:
> > src/slave/cgroups_isolation_module.cpp, line 444
> > <https://reviews.apache.org/r/9408/diff/6/?file=258837#file258837line444>
> >
> > ditto, perhaps an indication that it's likely due to another slave running on this machine?
>
> Vinod Kone wrote:
> Its not possible for another slave to be running, because we acquired the exclusive lock. This could fail if 'fd' is invalid/refers to an object other than file/incorrect descriptor. The strerror() would give the correct reason.
Got it. Ditto then about CHECK or LOG(FATAL)?
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9408/#review16763
-----------------------------------------------------------
On Feb. 19, 2013, 10:56 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9408/
> -----------------------------------------------------------
>
> (Updated Feb. 19, 2013, 10:56 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Description
> -------
>
> See summary
>
>
> Diffs
> -----
>
> src/slave/cgroups_isolation_module.hpp 669efa14ba2603764aa68ae19a44e79dbfdec192
> src/slave/cgroups_isolation_module.cpp 14f549edaf1b37a6bca8f75309864333ae775e7c
> src/slave/process_based_isolation_module.hpp f1817192582e3646f8dcf17934ba7998829e8fd6
> src/slave/process_based_isolation_module.cpp 12a579cba56cd3dac384bc7919b0d5537b0e429d
> src/tests/balloon_framework_test.sh 93a733f64cfde08349b7781eb3d5e13594c74498
>
> Diff: https://reviews.apache.org/r/9408/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request: Properly cleanup cgroups.
Posted by Vinod Kone <vi...@gmail.com>.
> On Feb. 20, 2013, 12:54 a.m., Ben Mahler wrote:
> > src/slave/cgroups_isolation_module.cpp, line 437
> > <https://reviews.apache.org/r/9408/diff/6/?file=258837#file258837line437>
> >
> > EXIT is used for messages that the mesos user / operator would read and need to take action in regard to. Given this is for orphaned executors, can you add more information to the message here to describe what could be wrong (so as to guide the user)?
There are multiple reasons a cgroup 'destroy' fails, as you can see in cgroups::destroy(). And, no, its not because another slave is running (see below). Afaict, it is not possible to tell the reason from the call site. Any ideas?
> On Feb. 20, 2013, 12:54 a.m., Ben Mahler wrote:
> > src/slave/cgroups_isolation_module.cpp, line 444
> > <https://reviews.apache.org/r/9408/diff/6/?file=258837#file258837line444>
> >
> > ditto, perhaps an indication that it's likely due to another slave running on this machine?
Its not possible for another slave to be running, because we acquired the exclusive lock. This could fail if 'fd' is invalid/refers to an object other than file/incorrect descriptor. The strerror() would give the correct reason.
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9408/#review16763
-----------------------------------------------------------
On Feb. 19, 2013, 10:56 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9408/
> -----------------------------------------------------------
>
> (Updated Feb. 19, 2013, 10:56 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Description
> -------
>
> See summary
>
>
> Diffs
> -----
>
> src/slave/cgroups_isolation_module.hpp 669efa14ba2603764aa68ae19a44e79dbfdec192
> src/slave/cgroups_isolation_module.cpp 14f549edaf1b37a6bca8f75309864333ae775e7c
> src/slave/process_based_isolation_module.hpp f1817192582e3646f8dcf17934ba7998829e8fd6
> src/slave/process_based_isolation_module.cpp 12a579cba56cd3dac384bc7919b0d5537b0e429d
> src/tests/balloon_framework_test.sh 93a733f64cfde08349b7781eb3d5e13594c74498
>
> Diff: https://reviews.apache.org/r/9408/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request: Properly cleanup cgroups.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9408/#review16763
-----------------------------------------------------------
src/slave/cgroups_isolation_module.cpp
<https://reviews.apache.org/r/9408/#comment35627>
EXIT is used for messages that the mesos user / operator would read and need to take action in regard to. Given this is for orphaned executors, can you add more information to the message here to describe what could be wrong (so as to guide the user)?
src/slave/cgroups_isolation_module.cpp
<https://reviews.apache.org/r/9408/#comment35628>
ditto, perhaps an indication that it's likely due to another slave running on this machine?
- Ben Mahler
On Feb. 19, 2013, 10:56 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9408/
> -----------------------------------------------------------
>
> (Updated Feb. 19, 2013, 10:56 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Description
> -------
>
> See summary
>
>
> Diffs
> -----
>
> src/slave/cgroups_isolation_module.hpp 669efa14ba2603764aa68ae19a44e79dbfdec192
> src/slave/cgroups_isolation_module.cpp 14f549edaf1b37a6bca8f75309864333ae775e7c
> src/slave/process_based_isolation_module.hpp f1817192582e3646f8dcf17934ba7998829e8fd6
> src/slave/process_based_isolation_module.cpp 12a579cba56cd3dac384bc7919b0d5537b0e429d
> src/tests/balloon_framework_test.sh 93a733f64cfde08349b7781eb3d5e13594c74498
>
> Diff: https://reviews.apache.org/r/9408/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request: Properly cleanup cgroups.
Posted by Vinod Kone <vi...@gmail.com>.
> On Feb. 21, 2013, 8:20 p.m., Benjamin Hindman wrote:
> > src/slave/cgroups_isolation_module.cpp, lines 907-908
> > <https://reviews.apache.org/r/9408/diff/8/?file=260458#file260458line907>
> >
> > So maybe status should an Option, both in the CgroupInfo struct and passed back to the slave?
I thought about that. But the issue is that 'status' is a required field in the ExitedExecutorMessage protobuf. So, a non-existence of status should be converted to "-1" at some point before sending it to the master. One option is to make the field optional, but I guess that makes it backwards incompatible (or rather enforces a "master first then slaves" deploy). Let me know your thoughts?
> On Feb. 21, 2013, 8:20 p.m., Benjamin Hindman wrote:
> > src/slave/slave.cpp, line 1094
> > <https://reviews.apache.org/r/9408/diff/8/?file=260460#file260460line1094>
> >
> > Is this a bug fix?
No. It just has been moved up from shutdownExecutorTimeout().
> On Feb. 21, 2013, 8:20 p.m., Benjamin Hindman wrote:
> > src/slave/slave.cpp, line 1146
> > <https://reviews.apache.org/r/9408/diff/8/?file=260460#file260460line1146>
> >
> > I guess the guarantee that we didn't do this twice was that we couldn't lookup the executor id twice (since we had destroyed the executor on the next line). I like the way you've done it now MUCH better!
More importantly, I think its wrong to remove/gc an executor before it even exited! Because, it could give the wrong impression (for e.g webui) on whether an executor is still running or it could pull an executor's working directory from underneath it while its still running.
> On Feb. 21, 2013, 8:20 p.m., Benjamin Hindman wrote:
> > src/slave/cgroups_isolation_module.cpp, line 902
> > <https://reviews.apache.org/r/9408/diff/8/?file=260458#file260458line902>
> >
> > This one feels a bit gratuitous. Assertions should point out things you might be concerned about getting wrong in the code. The one above says to me: "I only expect to get to _killExecutor with a non-none CgroupInfo if the executor for that cgroup was killed". That tells me that there is some complicated semantics around when/how _killExecutor might get invoked and I should be weary of that if I ever make changes in here. However, this assertion makes me think that there are some complicated semantics around how the cgroup name might change depending on how _killExecutor gets called ... which I don't think is correct thus leaving the wrong message.
Well, my concern is that there is nothing stopping someone from calling _killExecutor() with a different 'cgroup' name than the one that the 'info' corresponds to.
> On Feb. 21, 2013, 8:20 p.m., Benjamin Hindman wrote:
> > src/slave/cgroups_isolation_module.cpp, line 896
> > <https://reviews.apache.org/r/9408/diff/8/?file=260458#file260458line896>
> >
> > I do not understand why you need a copy. And we are generally averse to making copies of things we have pointers to (as it usually implies they have some non-copyable state, otherwise we could avoid pointers all together).
turns out i just need to copy the framework and executor ids when calling unregisterinfo. fixed.
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9408/#review16870
-----------------------------------------------------------
On Feb. 21, 2013, 6:40 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9408/
> -----------------------------------------------------------
>
> (Updated Feb. 21, 2013, 6:40 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Description
> -------
>
> See summary
>
>
> Diffs
> -----
>
> src/slave/cgroups_isolation_module.hpp 669efa14ba2603764aa68ae19a44e79dbfdec192
> src/slave/cgroups_isolation_module.cpp 14f549edaf1b37a6bca8f75309864333ae775e7c
> src/slave/process_based_isolation_module.cpp 12a579cba56cd3dac384bc7919b0d5537b0e429d
> src/slave/slave.cpp d4721c3eb51db87278d05f6fbe2eadb8a3a9b4dd
> src/tests/balloon_framework_test.sh 93a733f64cfde08349b7781eb3d5e13594c74498
>
> Diff: https://reviews.apache.org/r/9408/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request: Properly cleanup cgroups.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9408/#review16870
-----------------------------------------------------------
src/slave/cgroups_isolation_module.hpp
<https://reviews.apache.org/r/9408/#comment35801>
Why is this an option?
src/slave/cgroups_isolation_module.cpp
<https://reviews.apache.org/r/9408/#comment35803>
'None()' (after trunk rebase).
src/slave/cgroups_isolation_module.cpp
<https://reviews.apache.org/r/9408/#comment35802>
PLOG(FATAL)
src/slave/cgroups_isolation_module.cpp
<https://reviews.apache.org/r/9408/#comment35807>
This doesn't feel like an error since you call his with None above in the code! This should probably just be a return (no logging, the logging line above is sufficient).
src/slave/cgroups_isolation_module.cpp
<https://reviews.apache.org/r/9408/#comment35804>
I do not understand why you need a copy. And we are generally averse to making copies of things we have pointers to (as it usually implies they have some non-copyable state, otherwise we could avoid pointers all together).
src/slave/cgroups_isolation_module.cpp
<https://reviews.apache.org/r/9408/#comment35808>
This one feels a bit gratuitous. Assertions should point out things you might be concerned about getting wrong in the code. The one above says to me: "I only expect to get to _killExecutor with a non-none CgroupInfo if the executor for that cgroup was killed". That tells me that there is some complicated semantics around when/how _killExecutor might get invoked and I should be weary of that if I ever make changes in here. However, this assertion makes me think that there are some complicated semantics around how the cgroup name might change depending on how _killExecutor gets called ... which I don't think is correct thus leaving the wrong message.
src/slave/cgroups_isolation_module.cpp
<https://reviews.apache.org/r/9408/#comment35806>
So maybe status should an Option, both in the CgroupInfo struct and passed back to the slave?
src/slave/slave.cpp
<https://reviews.apache.org/r/9408/#comment35810>
Is this a bug fix?
src/slave/slave.cpp
<https://reviews.apache.org/r/9408/#comment35811>
I guess the guarantee that we didn't do this twice was that we couldn't lookup the executor id twice (since we had destroyed the executor on the next line). I like the way you've done it now MUCH better!
src/tests/balloon_framework_test.sh
<https://reviews.apache.org/r/9408/#comment35809>
How about: "This function is not pure, but depends on state from the environment (e.g., ${SLAVE_PID}) because we do not know all possible values when we register this function with 'atexit'".
Related, the last thing I'd like to do here is "declare" MASTER_PID and SLAVE_PID above the cleanup function (above the comment, below the unmount stuff). This should be sufficient:
MASTER_PID=
SLAVE_PID=
As a way of signifying that these are global environment variables that we expect (rather than referencing them in cleanup before they are declared). Sound good?
- Benjamin Hindman
On Feb. 21, 2013, 6:40 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9408/
> -----------------------------------------------------------
>
> (Updated Feb. 21, 2013, 6:40 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Description
> -------
>
> See summary
>
>
> Diffs
> -----
>
> src/slave/cgroups_isolation_module.hpp 669efa14ba2603764aa68ae19a44e79dbfdec192
> src/slave/cgroups_isolation_module.cpp 14f549edaf1b37a6bca8f75309864333ae775e7c
> src/slave/process_based_isolation_module.cpp 12a579cba56cd3dac384bc7919b0d5537b0e429d
> src/slave/slave.cpp d4721c3eb51db87278d05f6fbe2eadb8a3a9b4dd
> src/tests/balloon_framework_test.sh 93a733f64cfde08349b7781eb3d5e13594c74498
>
> Diff: https://reviews.apache.org/r/9408/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request: Properly cleanup cgroups.
Posted by Vinod Kone <vi...@gmail.com>.
> On Feb. 23, 2013, 10:28 p.m., Benjamin Hindman wrote:
> > src/slave/cgroups_isolation_module.cpp, line 561
> > <https://reviews.apache.org/r/9408/diff/9/?file=261098#file261098line561>
> >
> > Can't, since _killExecutor gets used above as well for cleaning up cgroups on startup. Probably means we need two different continuations here, the _killExecutor continuation and the one for just cleaning up the cgroup. Then we don't need to do the Option<CgroupInfo> business or the CHECK to make sure the cgroup name is the same as the info->name()!
added _destroy() continuation for orphaned cgroup destroys.
> On Feb. 23, 2013, 10:28 p.m., Benjamin Hindman wrote:
> > src/slave/cgroups_isolation_module.cpp, line 898
> > <https://reviews.apache.org/r/9408/diff/9/?file=261098#file261098line898>
> >
> > Right, so it's probably fine for now. But we could consider waiting for BOTH the processTerminated event AND the cgroup destroyed event. Futures give us this power! Oh darn, if only the reaper gave us a future instead!
> >
> > How about we add a todo here that says we should actually wait for both events ONCE the reaper returns a future instead.
sg. added a todo.
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9408/#review16989
-----------------------------------------------------------
On Feb. 22, 2013, 2:49 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9408/
> -----------------------------------------------------------
>
> (Updated Feb. 22, 2013, 2:49 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Description
> -------
>
> See summary
>
>
> Diffs
> -----
>
> src/slave/cgroups_isolation_module.hpp 669efa14ba2603764aa68ae19a44e79dbfdec192
> src/slave/cgroups_isolation_module.cpp a2eba6f96f5d8a4b1257571aa29e37c5682aab8d
> src/slave/process_based_isolation_module.cpp 12a579cba56cd3dac384bc7919b0d5537b0e429d
> src/slave/slave.cpp d4721c3eb51db87278d05f6fbe2eadb8a3a9b4dd
> src/tests/balloon_framework_test.sh 93a733f64cfde08349b7781eb3d5e13594c74498
>
> Diff: https://reviews.apache.org/r/9408/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request: Properly cleanup cgroups.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9408/#review16989
-----------------------------------------------------------
Ship it!
src/slave/cgroups_isolation_module.cpp
<https://reviews.apache.org/r/9408/#comment35989>
Yup, I missed this. An option is probably better.
src/slave/cgroups_isolation_module.cpp
<https://reviews.apache.org/r/9408/#comment35990>
Can't, since _killExecutor gets used above as well for cleaning up cgroups on startup. Probably means we need two different continuations here, the _killExecutor continuation and the one for just cleaning up the cgroup. Then we don't need to do the Option<CgroupInfo> business or the CHECK to make sure the cgroup name is the same as the info->name()!
src/slave/cgroups_isolation_module.cpp
<https://reviews.apache.org/r/9408/#comment35991>
Right, so it's probably fine for now. But we could consider waiting for BOTH the processTerminated event AND the cgroup destroyed event. Futures give us this power! Oh darn, if only the reaper gave us a future instead!
How about we add a todo here that says we should actually wait for both events ONCE the reaper returns a future instead.
- Benjamin Hindman
On Feb. 22, 2013, 2:49 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9408/
> -----------------------------------------------------------
>
> (Updated Feb. 22, 2013, 2:49 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Description
> -------
>
> See summary
>
>
> Diffs
> -----
>
> src/slave/cgroups_isolation_module.hpp 669efa14ba2603764aa68ae19a44e79dbfdec192
> src/slave/cgroups_isolation_module.cpp a2eba6f96f5d8a4b1257571aa29e37c5682aab8d
> src/slave/process_based_isolation_module.cpp 12a579cba56cd3dac384bc7919b0d5537b0e429d
> src/slave/slave.cpp d4721c3eb51db87278d05f6fbe2eadb8a3a9b4dd
> src/tests/balloon_framework_test.sh 93a733f64cfde08349b7781eb3d5e13594c74498
>
> Diff: https://reviews.apache.org/r/9408/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request: Properly cleanup cgroups.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9408/#review17062
-----------------------------------------------------------
Hey could you do a rebase? The diff is tough to read now that trunk has changed.
- Ben Mahler
On Feb. 25, 2013, 7:15 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9408/
> -----------------------------------------------------------
>
> (Updated Feb. 25, 2013, 7:15 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Description
> -------
>
> See summary
>
>
> Diffs
> -----
>
> hadoop/mesos/src/java/org/apache/hadoop/mapred/MesosScheduler.java 90970ec112e84fdfb683b9e60dcfc1450278e21a
> include/mesos/mesos.proto 38235157d45bdccb676e5c3241c21b585a6f8801
> src/examples/balloon_framework.cpp 35bc0fe712edbf6130e10e93286c5278625be096
> src/examples/java/TestFramework.java 8417394487a80b439e7d9897c83f0b2c1eb17ff4
> src/examples/long_lived_framework.cpp 04ac678387dd78104b5d42fa1f7b5de1849b0701
> src/examples/python/test_framework.py 269f532733ab82378bc1bd643107e6f9f2945d8b
> src/examples/test_framework.cpp b9ab692414e4df64f176fc1ecd05f24ff089bde0
> src/files/files.cpp 84082ce6dab6c70bb21cd290119cbdd0169be29d
> src/linux/cgroups.cpp e7bdb7442624ac9f77df6ab87de013f39de37d32
> src/master/http.cpp 790961d6890841c456485491516d1991fe35ec16
> src/slave/cgroups_isolation_module.hpp 669efa14ba2603764aa68ae19a44e79dbfdec192
> src/slave/cgroups_isolation_module.cpp a2eba6f96f5d8a4b1257571aa29e37c5682aab8d
> src/slave/http.cpp b94197013ac0d5dd95d6dabb44812905a123184a
> src/slave/slave.hpp e9f7b659ca2860501840b3d01e69915ebd162039
> src/slave/slave.cpp d4721c3eb51db87278d05f6fbe2eadb8a3a9b4dd
> src/state/state.hpp a0f5d75cf1bad325ab0b3c53f0d05825e211ab5f
> src/tests/balloon_framework_test.sh 93a733f64cfde08349b7781eb3d5e13594c74498
> src/webui/master/static/slave_executor.html bd06e69b3c99a0a44993c31429d877914b644a1d
> src/webui/master/static/slave_framework.html c66150ecb794300239c92ceaf684bfd3f1cb007e
> third_party/libprocess/Makefile.am 468df4e1426b725cf1252300134bc13658eae090
> third_party/libprocess/include/process/clock.hpp 1a98c6a8b50fdad5308321413121f86bea135d37
> third_party/libprocess/include/process/defer.hpp f090a4e3b9088c38d0a914cac5f4d2f77b5b3816
> third_party/libprocess/include/process/deferred.hpp 38a6d5ccc5450ee3971ad2e5f58d647bf407ca7b
> third_party/libprocess/include/process/executor.hpp 7f52150d1d59544b1bf07b819f9a0117f8a1b8b6
> third_party/libprocess/include/process/future.hpp 939936720e6cda2a23ac7974311e1c4d93ea5791
> third_party/libprocess/include/process/process.hpp 0a6371813fd5fa585bca2b578404b52a59fc45f0
> third_party/libprocess/include/process/statistics.hpp faefad2e374e08e4e0f88750c4c4eee74bce62d7
> third_party/libprocess/include/stout/owned.hpp PRE-CREATION
> third_party/libprocess/src/process.cpp 878f2e866bcff8ecd743f16b1fd6f211bc5bf78a
> third_party/libprocess/src/statistics.cpp 29d08f05c6ab7b69d587a44aab860b4dfe645b89
> third_party/libprocess/src/tests/process_tests.cpp ecbfa7a0ec2814ebfa87e57ff30bdcdbd1ba93c8
> third_party/libprocess/src/tests/statistics_tests.cpp 0aaab3526618171c7cfbd11d40d614344bcbfd0a
>
> Diff: https://reviews.apache.org/r/9408/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request: Properly cleanup cgroups.
Posted by Vinod Kone <vi...@gmail.com>.
> On Feb. 27, 2013, 7 p.m., Benjamin Hindman wrote:
> > src/slave/cgroups_isolation_module.cpp, line 652
> > <https://reviews.apache.org/r/9408/diff/12/?file=261997#file261997line652>
> >
> > It also gets printed in the slave ...
Since its asynchronous and cgroups destroy may fail, I would also like to have it here.
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9408/#review17159
-----------------------------------------------------------
On Feb. 26, 2013, 12:32 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9408/
> -----------------------------------------------------------
>
> (Updated Feb. 26, 2013, 12:32 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Description
> -------
>
> See summary
>
>
> Diffs
> -----
>
> src/slave/cgroups_isolation_module.hpp a04fc46b15d2741886f5847cadbdc9bed351c588
> src/slave/cgroups_isolation_module.cpp a779de80d13c67e507d7d2ee788fcdaa5e71daca
> src/slave/slave.hpp 7648c33230c1900eda7529045c5df9ccab105d47
> src/slave/slave.cpp 8c2e1bfc363491c681177676f9dfe5f229276f7d
> src/tests/balloon_framework_test.sh 93a733f64cfde08349b7781eb3d5e13594c74498
>
> Diff: https://reviews.apache.org/r/9408/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request: Properly cleanup cgroups.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9408/#review17159
-----------------------------------------------------------
Ship it!
src/slave/cgroups_isolation_module.cpp
<https://reviews.apache.org/r/9408/#comment36337>
Might consider moving this above the destroy call (consider setting it to indicate the "intent"). Given the semantics of libprocess and onAny, and defer, you should be safe, but it's probably better practice to assume you've passed ownership of variables and that you can no longer manipulate them.
src/slave/cgroups_isolation_module.cpp
<https://reviews.apache.org/r/9408/#comment36338>
It also gets printed in the slave ...
- Benjamin Hindman
On Feb. 26, 2013, 12:32 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9408/
> -----------------------------------------------------------
>
> (Updated Feb. 26, 2013, 12:32 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Description
> -------
>
> See summary
>
>
> Diffs
> -----
>
> src/slave/cgroups_isolation_module.hpp a04fc46b15d2741886f5847cadbdc9bed351c588
> src/slave/cgroups_isolation_module.cpp a779de80d13c67e507d7d2ee788fcdaa5e71daca
> src/slave/slave.hpp 7648c33230c1900eda7529045c5df9ccab105d47
> src/slave/slave.cpp 8c2e1bfc363491c681177676f9dfe5f229276f7d
> src/tests/balloon_framework_test.sh 93a733f64cfde08349b7781eb3d5e13594c74498
>
> Diff: https://reviews.apache.org/r/9408/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request: Properly cleanup cgroups.
Posted by Vinod Kone <vi...@gmail.com>.
> On Feb. 27, 2013, 6:43 p.m., Ben Mahler wrote:
> > src/slave/cgroups_isolation_module.cpp, line 946
> > <https://reviews.apache.org/r/9408/diff/12/?file=261997#file261997line946>
> >
> > As a result of this NOTE it sounds like we should kill the sentinel values in favor of using Options, not expecting you to do that but maybe a TODO in the info struct if not already one present.
>
> Vinod Kone wrote:
> As the TODO notes the plan is to actually wait for the process terminated event, before informing the slave. So, need to make this sentinel.
s/need to/no need to/
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9408/#review17156
-----------------------------------------------------------
On Feb. 26, 2013, 12:32 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9408/
> -----------------------------------------------------------
>
> (Updated Feb. 26, 2013, 12:32 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Description
> -------
>
> See summary
>
>
> Diffs
> -----
>
> src/slave/cgroups_isolation_module.hpp a04fc46b15d2741886f5847cadbdc9bed351c588
> src/slave/cgroups_isolation_module.cpp a779de80d13c67e507d7d2ee788fcdaa5e71daca
> src/slave/slave.hpp 7648c33230c1900eda7529045c5df9ccab105d47
> src/slave/slave.cpp 8c2e1bfc363491c681177676f9dfe5f229276f7d
> src/tests/balloon_framework_test.sh 93a733f64cfde08349b7781eb3d5e13594c74498
>
> Diff: https://reviews.apache.org/r/9408/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request: Properly cleanup cgroups.
Posted by Vinod Kone <vi...@gmail.com>.
> On Feb. 27, 2013, 6:43 p.m., Ben Mahler wrote:
> > src/slave/cgroups_isolation_module.cpp, line 946
> > <https://reviews.apache.org/r/9408/diff/12/?file=261997#file261997line946>
> >
> > As a result of this NOTE it sounds like we should kill the sentinel values in favor of using Options, not expecting you to do that but maybe a TODO in the info struct if not already one present.
As the TODO notes the plan is to actually wait for the process terminated event, before informing the slave. So, need to make this sentinel.
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9408/#review17156
-----------------------------------------------------------
On Feb. 26, 2013, 12:32 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9408/
> -----------------------------------------------------------
>
> (Updated Feb. 26, 2013, 12:32 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Description
> -------
>
> See summary
>
>
> Diffs
> -----
>
> src/slave/cgroups_isolation_module.hpp a04fc46b15d2741886f5847cadbdc9bed351c588
> src/slave/cgroups_isolation_module.cpp a779de80d13c67e507d7d2ee788fcdaa5e71daca
> src/slave/slave.hpp 7648c33230c1900eda7529045c5df9ccab105d47
> src/slave/slave.cpp 8c2e1bfc363491c681177676f9dfe5f229276f7d
> src/tests/balloon_framework_test.sh 93a733f64cfde08349b7781eb3d5e13594c74498
>
> Diff: https://reviews.apache.org/r/9408/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request: Properly cleanup cgroups.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9408/#review17156
-----------------------------------------------------------
Ship it!
Looks much cleaner!
src/slave/cgroups_isolation_module.cpp
<https://reviews.apache.org/r/9408/#comment36325>
CHECK_SOME and the message here should maybe say uninitialized instead of invalid, now that we don't use -1?
src/slave/cgroups_isolation_module.cpp
<https://reviews.apache.org/r/9408/#comment36333>
do we want to log the status at the end of this message?
src/slave/cgroups_isolation_module.cpp
<https://reviews.apache.org/r/9408/#comment36327>
Much nicer!
src/slave/cgroups_isolation_module.cpp
<https://reviews.apache.org/r/9408/#comment36331>
As a result of this NOTE it sounds like we should kill the sentinel values in favor of using Options, not expecting you to do that but maybe a TODO in the info struct if not already one present.
src/slave/slave.hpp
<https://reviews.apache.org/r/9408/#comment36326>
thanks!
- Ben Mahler
On Feb. 26, 2013, 12:32 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9408/
> -----------------------------------------------------------
>
> (Updated Feb. 26, 2013, 12:32 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Description
> -------
>
> See summary
>
>
> Diffs
> -----
>
> src/slave/cgroups_isolation_module.hpp a04fc46b15d2741886f5847cadbdc9bed351c588
> src/slave/cgroups_isolation_module.cpp a779de80d13c67e507d7d2ee788fcdaa5e71daca
> src/slave/slave.hpp 7648c33230c1900eda7529045c5df9ccab105d47
> src/slave/slave.cpp 8c2e1bfc363491c681177676f9dfe5f229276f7d
> src/tests/balloon_framework_test.sh 93a733f64cfde08349b7781eb3d5e13594c74498
>
> Diff: https://reviews.apache.org/r/9408/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request: Properly cleanup cgroups.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9408/
-----------------------------------------------------------
(Updated March 1, 2013, 1:53 a.m.)
Review request for mesos, Benjamin Hindman and Ben Mahler.
Changes
-------
Ben's comments and rebased off trunk.
Description
-------
See summary
Diffs (updated)
-----
src/slave/cgroups_isolation_module.hpp a04fc46b15d2741886f5847cadbdc9bed351c588
src/slave/cgroups_isolation_module.cpp a779de80d13c67e507d7d2ee788fcdaa5e71daca
src/slave/slave.hpp 7648c33230c1900eda7529045c5df9ccab105d47
src/slave/slave.cpp c13d268c5e0e107902f64e30304a18128927a571
src/tests/balloon_framework_test.sh 93a733f64cfde08349b7781eb3d5e13594c74498
Diff: https://reviews.apache.org/r/9408/diff/
Testing
-------
make check
Thanks,
Vinod Kone
Re: Review Request: Properly cleanup cgroups.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9408/
-----------------------------------------------------------
(Updated Feb. 26, 2013, 12:32 a.m.)
Review request for mesos, Benjamin Hindman and Ben Mahler.
Changes
-------
rebased.
Description
-------
See summary
Diffs (updated)
-----
src/slave/cgroups_isolation_module.hpp a04fc46b15d2741886f5847cadbdc9bed351c588
src/slave/cgroups_isolation_module.cpp a779de80d13c67e507d7d2ee788fcdaa5e71daca
src/slave/slave.hpp 7648c33230c1900eda7529045c5df9ccab105d47
src/slave/slave.cpp 8c2e1bfc363491c681177676f9dfe5f229276f7d
src/tests/balloon_framework_test.sh 93a733f64cfde08349b7781eb3d5e13594c74498
Diff: https://reviews.apache.org/r/9408/diff/
Testing
-------
make check
Thanks,
Vinod Kone
Re: Review Request: Properly cleanup cgroups.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9408/
-----------------------------------------------------------
(Updated Feb. 25, 2013, 7:15 p.m.)
Review request for mesos, Benjamin Hindman and Ben Mahler.
Changes
-------
fixed log messages.
Description
-------
See summary
Diffs (updated)
-----
hadoop/mesos/src/java/org/apache/hadoop/mapred/MesosScheduler.java 90970ec112e84fdfb683b9e60dcfc1450278e21a
include/mesos/mesos.proto 38235157d45bdccb676e5c3241c21b585a6f8801
src/examples/balloon_framework.cpp 35bc0fe712edbf6130e10e93286c5278625be096
src/examples/java/TestFramework.java 8417394487a80b439e7d9897c83f0b2c1eb17ff4
src/examples/long_lived_framework.cpp 04ac678387dd78104b5d42fa1f7b5de1849b0701
src/examples/python/test_framework.py 269f532733ab82378bc1bd643107e6f9f2945d8b
src/examples/test_framework.cpp b9ab692414e4df64f176fc1ecd05f24ff089bde0
src/files/files.cpp 84082ce6dab6c70bb21cd290119cbdd0169be29d
src/linux/cgroups.cpp e7bdb7442624ac9f77df6ab87de013f39de37d32
src/master/http.cpp 790961d6890841c456485491516d1991fe35ec16
src/slave/cgroups_isolation_module.hpp 669efa14ba2603764aa68ae19a44e79dbfdec192
src/slave/cgroups_isolation_module.cpp a2eba6f96f5d8a4b1257571aa29e37c5682aab8d
src/slave/http.cpp b94197013ac0d5dd95d6dabb44812905a123184a
src/slave/slave.hpp e9f7b659ca2860501840b3d01e69915ebd162039
src/slave/slave.cpp d4721c3eb51db87278d05f6fbe2eadb8a3a9b4dd
src/state/state.hpp a0f5d75cf1bad325ab0b3c53f0d05825e211ab5f
src/tests/balloon_framework_test.sh 93a733f64cfde08349b7781eb3d5e13594c74498
src/webui/master/static/slave_executor.html bd06e69b3c99a0a44993c31429d877914b644a1d
src/webui/master/static/slave_framework.html c66150ecb794300239c92ceaf684bfd3f1cb007e
third_party/libprocess/Makefile.am 468df4e1426b725cf1252300134bc13658eae090
third_party/libprocess/include/process/clock.hpp 1a98c6a8b50fdad5308321413121f86bea135d37
third_party/libprocess/include/process/defer.hpp f090a4e3b9088c38d0a914cac5f4d2f77b5b3816
third_party/libprocess/include/process/deferred.hpp 38a6d5ccc5450ee3971ad2e5f58d647bf407ca7b
third_party/libprocess/include/process/executor.hpp 7f52150d1d59544b1bf07b819f9a0117f8a1b8b6
third_party/libprocess/include/process/future.hpp 939936720e6cda2a23ac7974311e1c4d93ea5791
third_party/libprocess/include/process/process.hpp 0a6371813fd5fa585bca2b578404b52a59fc45f0
third_party/libprocess/include/process/statistics.hpp faefad2e374e08e4e0f88750c4c4eee74bce62d7
third_party/libprocess/include/stout/owned.hpp PRE-CREATION
third_party/libprocess/src/process.cpp 878f2e866bcff8ecd743f16b1fd6f211bc5bf78a
third_party/libprocess/src/statistics.cpp 29d08f05c6ab7b69d587a44aab860b4dfe645b89
third_party/libprocess/src/tests/process_tests.cpp ecbfa7a0ec2814ebfa87e57ff30bdcdbd1ba93c8
third_party/libprocess/src/tests/statistics_tests.cpp 0aaab3526618171c7cfbd11d40d614344bcbfd0a
Diff: https://reviews.apache.org/r/9408/diff/
Testing
-------
make check
Thanks,
Vinod Kone
Re: Review Request: Properly cleanup cgroups.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9408/
-----------------------------------------------------------
(Updated Feb. 24, 2013, 8:39 p.m.)
Review request for mesos, Benjamin Hindman and Ben Mahler.
Changes
-------
bens'.
Description
-------
See summary
Diffs (updated)
-----
src/slave/cgroups_isolation_module.hpp 669efa14ba2603764aa68ae19a44e79dbfdec192
src/slave/cgroups_isolation_module.cpp a2eba6f96f5d8a4b1257571aa29e37c5682aab8d
src/slave/slave.hpp e9f7b659ca2860501840b3d01e69915ebd162039
src/slave/slave.cpp d4721c3eb51db87278d05f6fbe2eadb8a3a9b4dd
src/tests/balloon_framework_test.sh 93a733f64cfde08349b7781eb3d5e13594c74498
Diff: https://reviews.apache.org/r/9408/diff/
Testing
-------
make check
Thanks,
Vinod Kone
Re: Review Request: Properly cleanup cgroups.
Posted by Vinod Kone <vi...@gmail.com>.
> On Feb. 22, 2013, 11:23 p.m., Ben Mahler wrote:
> > src/slave/cgroups_isolation_module.cpp, line 321
> > <https://reviews.apache.org/r/9408/diff/9/?file=261098#file261098line321>
> >
> > Can you use a different continuation here?
> >
> > I see this is why you added the cgroup name and info, but really the continuation for this one is really simple (just LOG(INFO)). This will also simplify the logic inside _killExecutor.
done. added a _destroy().
> On Feb. 22, 2013, 11:23 p.m., Ben Mahler wrote:
> > src/slave/process_based_isolation_module.cpp, line 196
> > <https://reviews.apache.org/r/9408/diff/9/?file=261099#file261099line196>
> >
> > kill the comment now?
good catch. ty.
> On Feb. 22, 2013, 11:23 p.m., Ben Mahler wrote:
> > src/slave/slave.cpp, line 334
> > <https://reviews.apache.org/r/9408/diff/9/?file=261100#file261100line334>
> >
> > s/till/until
> >
> > Can you satisfy the TODO by calling:
> > terminate(isolationModule, false)
> >
> > This will not inject the terminate message in front of the queue, which seems like the right thing to do, right?
Thats not enough. We need to wait until all the asynchronous cgroup destroys complete.
> On Feb. 22, 2013, 11:23 p.m., Ben Mahler wrote:
> > src/slave/slave.cpp, line 1097
> > <https://reviews.apache.org/r/9408/diff/9/?file=261100#file261100line1097>
> >
> > Can you add a TODO where the completedFrameworks is declared in the slave header for me to used the Owned abstraction?
> >
> > (Owned was introduced in the monitoring reviews)
done
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9408/#review16974
-----------------------------------------------------------
On Feb. 22, 2013, 2:49 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9408/
> -----------------------------------------------------------
>
> (Updated Feb. 22, 2013, 2:49 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Description
> -------
>
> See summary
>
>
> Diffs
> -----
>
> src/slave/cgroups_isolation_module.hpp 669efa14ba2603764aa68ae19a44e79dbfdec192
> src/slave/cgroups_isolation_module.cpp a2eba6f96f5d8a4b1257571aa29e37c5682aab8d
> src/slave/process_based_isolation_module.cpp 12a579cba56cd3dac384bc7919b0d5537b0e429d
> src/slave/slave.cpp d4721c3eb51db87278d05f6fbe2eadb8a3a9b4dd
> src/tests/balloon_framework_test.sh 93a733f64cfde08349b7781eb3d5e13594c74498
>
> Diff: https://reviews.apache.org/r/9408/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request: Properly cleanup cgroups.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9408/#review16974
-----------------------------------------------------------
Sorry for not spotting this stuff earlier!
src/slave/cgroups_isolation_module.cpp
<https://reviews.apache.org/r/9408/#comment35970>
Can you use a different continuation here?
I see this is why you added the cgroup name and info, but really the continuation for this one is really simple (just LOG(INFO)). This will also simplify the logic inside _killExecutor.
src/slave/cgroups_isolation_module.cpp
<https://reviews.apache.org/r/9408/#comment35959>
I'm not sure I agree with benh here (or he didn't see this bit in terms of asking why the lock fd was an option).
Now you have a sentinel value (-1) to represent the concept of 'none'. These are the exact semantics abstracted away by the option abstraction.
src/slave/cgroups_isolation_module.cpp
<https://reviews.apache.org/r/9408/#comment35967>
Just pass info, since it includes info->name().
src/slave/cgroups_isolation_module.cpp
<https://reviews.apache.org/r/9408/#comment35965>
Now that you're passing info, can't you remove the cgroup string argument? It now seems redundant and allows a mismatch between info->name and cgroup.
src/slave/cgroups_isolation_module.cpp
<https://reviews.apache.org/r/9408/#comment35962>
since you're no longer making a copy, why introduce a variable?
Seems better to just call the option 'info' and use get() here and below?
src/slave/cgroups_isolation_module.cpp
<https://reviews.apache.org/r/9408/#comment35966>
This check would be eliminated by removing the cgroup argument.
src/slave/process_based_isolation_module.cpp
<https://reviews.apache.org/r/9408/#comment35958>
kill the comment now?
src/slave/slave.cpp
<https://reviews.apache.org/r/9408/#comment35960>
s/till/until
Can you satisfy the TODO by calling:
terminate(isolationModule, false)
This will not inject the terminate message in front of the queue, which seems like the right thing to do, right?
src/slave/slave.cpp
<https://reviews.apache.org/r/9408/#comment35961>
Can you add a TODO where the completedFrameworks is declared in the slave header for me to used the Owned abstraction?
(Owned was introduced in the monitoring reviews)
- Ben Mahler
On Feb. 22, 2013, 2:49 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9408/
> -----------------------------------------------------------
>
> (Updated Feb. 22, 2013, 2:49 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Description
> -------
>
> See summary
>
>
> Diffs
> -----
>
> src/slave/cgroups_isolation_module.hpp 669efa14ba2603764aa68ae19a44e79dbfdec192
> src/slave/cgroups_isolation_module.cpp a2eba6f96f5d8a4b1257571aa29e37c5682aab8d
> src/slave/process_based_isolation_module.cpp 12a579cba56cd3dac384bc7919b0d5537b0e429d
> src/slave/slave.cpp d4721c3eb51db87278d05f6fbe2eadb8a3a9b4dd
> src/tests/balloon_framework_test.sh 93a733f64cfde08349b7781eb3d5e13594c74498
>
> Diff: https://reviews.apache.org/r/9408/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request: Properly cleanup cgroups.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9408/
-----------------------------------------------------------
(Updated Feb. 22, 2013, 2:49 a.m.)
Review request for mesos, Benjamin Hindman and Ben Mahler.
Changes
-------
BenH's comments.
Description
-------
See summary
Diffs (updated)
-----
src/slave/cgroups_isolation_module.hpp 669efa14ba2603764aa68ae19a44e79dbfdec192
src/slave/cgroups_isolation_module.cpp a2eba6f96f5d8a4b1257571aa29e37c5682aab8d
src/slave/process_based_isolation_module.cpp 12a579cba56cd3dac384bc7919b0d5537b0e429d
src/slave/slave.cpp d4721c3eb51db87278d05f6fbe2eadb8a3a9b4dd
src/tests/balloon_framework_test.sh 93a733f64cfde08349b7781eb3d5e13594c74498
Diff: https://reviews.apache.org/r/9408/diff/
Testing
-------
make check
Thanks,
Vinod Kone
Re: Review Request: Properly cleanup cgroups.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9408/
-----------------------------------------------------------
(Updated Feb. 21, 2013, 6:40 a.m.)
Review request for mesos, Benjamin Hindman and Ben Mahler.
Changes
-------
bens'.
Description
-------
See summary
Diffs (updated)
-----
src/slave/cgroups_isolation_module.hpp 669efa14ba2603764aa68ae19a44e79dbfdec192
src/slave/cgroups_isolation_module.cpp 14f549edaf1b37a6bca8f75309864333ae775e7c
src/slave/process_based_isolation_module.cpp 12a579cba56cd3dac384bc7919b0d5537b0e429d
src/slave/slave.cpp d4721c3eb51db87278d05f6fbe2eadb8a3a9b4dd
src/tests/balloon_framework_test.sh 93a733f64cfde08349b7781eb3d5e13594c74498
Diff: https://reviews.apache.org/r/9408/diff/
Testing
-------
make check
Thanks,
Vinod Kone
Re: Review Request: Properly cleanup cgroups.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9408/#review16789
-----------------------------------------------------------
src/slave/cgroups_isolation_module.cpp
<https://reviews.apache.org/r/9408/#comment35671>
keep this.
src/slave/cgroups_isolation_module.cpp
<https://reviews.apache.org/r/9408/#comment35672>
s/exit/fatal/
src/slave/process_based_isolation_module.cpp
<https://reviews.apache.org/r/9408/#comment35670>
don't do in finalize. always cleanup from slave.
- Vinod Kone
On Feb. 19, 2013, 10:56 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9408/
> -----------------------------------------------------------
>
> (Updated Feb. 19, 2013, 10:56 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Description
> -------
>
> See summary
>
>
> Diffs
> -----
>
> src/slave/cgroups_isolation_module.hpp 669efa14ba2603764aa68ae19a44e79dbfdec192
> src/slave/cgroups_isolation_module.cpp 14f549edaf1b37a6bca8f75309864333ae775e7c
> src/slave/process_based_isolation_module.hpp f1817192582e3646f8dcf17934ba7998829e8fd6
> src/slave/process_based_isolation_module.cpp 12a579cba56cd3dac384bc7919b0d5537b0e429d
> src/tests/balloon_framework_test.sh 93a733f64cfde08349b7781eb3d5e13594c74498
>
> Diff: https://reviews.apache.org/r/9408/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request: Properly cleanup cgroups.
Posted by Vinod Kone <vi...@gmail.com>.
> On Feb. 20, 2013, 8:21 p.m., Benjamin Hindman wrote:
> > src/tests/balloon_framework_test.sh, lines 14-15
> > <https://reviews.apache.org/r/9408/diff/7/?file=260009#file260009line14>
> >
> > Can we set this in one line?
i tested several possibilities, but couldn't get it to work with my limited bash fu :/
> On Feb. 20, 2013, 8:21 p.m., Benjamin Hindman wrote:
> > src/slave/process_based_isolation_module.cpp, line 92
> > <https://reviews.apache.org/r/9408/diff/7/?file=260008#file260008line92>
> >
> > This should really be handled by the slave. The isolation module is just responsible for the mechanisms of killing executors, not the policy.
Turns out slave already does this! Though it doesn't wait for the cleanup to be completed :/
I have added a TODO for this now, since it requires a bit more refactoring and this review has already accumulated more changes than it indeed. More importantly the TODO is not imminent because it only affects the tests and not real world semantics.
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9408/#review16784
-----------------------------------------------------------
On Feb. 19, 2013, 10:56 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9408/
> -----------------------------------------------------------
>
> (Updated Feb. 19, 2013, 10:56 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Description
> -------
>
> See summary
>
>
> Diffs
> -----
>
> src/slave/cgroups_isolation_module.hpp 669efa14ba2603764aa68ae19a44e79dbfdec192
> src/slave/cgroups_isolation_module.cpp 14f549edaf1b37a6bca8f75309864333ae775e7c
> src/slave/process_based_isolation_module.hpp f1817192582e3646f8dcf17934ba7998829e8fd6
> src/slave/process_based_isolation_module.cpp 12a579cba56cd3dac384bc7919b0d5537b0e429d
> src/tests/balloon_framework_test.sh 93a733f64cfde08349b7781eb3d5e13594c74498
>
> Diff: https://reviews.apache.org/r/9408/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request: Properly cleanup cgroups.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9408/#review16784
-----------------------------------------------------------
src/slave/process_based_isolation_module.cpp
<https://reviews.apache.org/r/9408/#comment35661>
This should really be handled by the slave. The isolation module is just responsible for the mechanisms of killing executors, not the policy.
src/tests/balloon_framework_test.sh
<https://reviews.apache.org/r/9408/#comment35668>
Can we set this in one line?
src/tests/balloon_framework_test.sh
<https://reviews.apache.org/r/9408/#comment35669>
s/cleanup/function cleanup/
Also, I'd like to see this documented. Why do we need this cleanup function? Maybe a note on why this function isn't pure (i.e., expects environment variables).
- Benjamin Hindman
On Feb. 19, 2013, 10:56 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9408/
> -----------------------------------------------------------
>
> (Updated Feb. 19, 2013, 10:56 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Description
> -------
>
> See summary
>
>
> Diffs
> -----
>
> src/slave/cgroups_isolation_module.hpp 669efa14ba2603764aa68ae19a44e79dbfdec192
> src/slave/cgroups_isolation_module.cpp 14f549edaf1b37a6bca8f75309864333ae775e7c
> src/slave/process_based_isolation_module.hpp f1817192582e3646f8dcf17934ba7998829e8fd6
> src/slave/process_based_isolation_module.cpp 12a579cba56cd3dac384bc7919b0d5537b0e429d
> src/tests/balloon_framework_test.sh 93a733f64cfde08349b7781eb3d5e13594c74498
>
> Diff: https://reviews.apache.org/r/9408/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request: Properly cleanup cgroups.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9408/#review16785
-----------------------------------------------------------
src/slave/cgroups_isolation_module.cpp
<https://reviews.apache.org/r/9408/#comment35662>
CHECK_SOME so that the error string is included
src/slave/cgroups_isolation_module.cpp
<https://reviews.apache.org/r/9408/#comment35663>
ditto
- Ben Mahler
On Feb. 19, 2013, 10:56 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9408/
> -----------------------------------------------------------
>
> (Updated Feb. 19, 2013, 10:56 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Description
> -------
>
> See summary
>
>
> Diffs
> -----
>
> src/slave/cgroups_isolation_module.hpp 669efa14ba2603764aa68ae19a44e79dbfdec192
> src/slave/cgroups_isolation_module.cpp 14f549edaf1b37a6bca8f75309864333ae775e7c
> src/slave/process_based_isolation_module.hpp f1817192582e3646f8dcf17934ba7998829e8fd6
> src/slave/process_based_isolation_module.cpp 12a579cba56cd3dac384bc7919b0d5537b0e429d
> src/tests/balloon_framework_test.sh 93a733f64cfde08349b7781eb3d5e13594c74498
>
> Diff: https://reviews.apache.org/r/9408/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request: Properly cleanup cgroups.
Posted by Vinod Kone <vi...@twitter.com>.
I think doing the cleanup in finalize is always the right thing to do. For
e.g: we don't want cgroups lying around after the last cgroups test is run!
The fact that we do the cgroups cleanup inside initialize() in cgroups
isolation module is just us being defensive against improper shutdowns.
@vinodkone
On Tue, Feb 19, 2013 at 4:14 PM, Benjamin Mahler <bm...@twitter.com>wrote:
> Did we decide whether we want to perform finalize() cleanup?
>
> If so, can you comment on why we need to do it?
>
>
> On Tue, Feb 19, 2013 at 2:56 PM, Vinod Kone <vi...@gmail.com> wrote:
>
>>
>> -----------------------------------------------------------
>>
>> This is an automatically generated e-mail. To reply, visit:
>> https://reviews.apache.org/r/9408/
>> -----------------------------------------------------------
>>
>> (Updated Feb. 19, 2013, 10:56 p.m.)
>>
>>
>> Review request for mesos, Benjamin Hindman and Ben Mahler.
>>
>>
>> Changes
>> -------
>>
>> benm's
>>
>>
>> Description
>> -------
>>
>> See summary
>>
>>
>> Diffs (updated)
>>
>> -----
>>
>> src/slave/cgroups_isolation_module.hpp
>> 669efa14ba2603764aa68ae19a44e79dbfdec192
>> src/slave/cgroups_isolation_module.cpp
>> 14f549edaf1b37a6bca8f75309864333ae775e7c
>> src/slave/process_based_isolation_module.hpp
>> f1817192582e3646f8dcf17934ba7998829e8fd6
>> src/slave/process_based_isolation_module.cpp
>> 12a579cba56cd3dac384bc7919b0d5537b0e429d
>> src/tests/balloon_framework_test.sh
>> 93a733f64cfde08349b7781eb3d5e13594c74498
>>
>> Diff: https://reviews.apache.org/r/9408/diff/
>>
>>
>> Testing
>> -------
>>
>> make check
>>
>>
>> Thanks,
>>
>> Vinod Kone
>>
>>
>
Re: Review Request: Properly cleanup cgroups.
Posted by Benjamin Mahler <bm...@twitter.com>.
Did we decide whether we want to perform finalize() cleanup?
If so, can you comment on why we need to do it?
On Tue, Feb 19, 2013 at 2:56 PM, Vinod Kone <vi...@gmail.com> wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9408/
> -----------------------------------------------------------
>
> (Updated Feb. 19, 2013, 10:56 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Changes
> -------
>
> benm's
>
>
> Description
> -------
>
> See summary
>
>
> Diffs (updated)
> -----
>
> src/slave/cgroups_isolation_module.hpp
> 669efa14ba2603764aa68ae19a44e79dbfdec192
> src/slave/cgroups_isolation_module.cpp
> 14f549edaf1b37a6bca8f75309864333ae775e7c
> src/slave/process_based_isolation_module.hpp
> f1817192582e3646f8dcf17934ba7998829e8fd6
> src/slave/process_based_isolation_module.cpp
> 12a579cba56cd3dac384bc7919b0d5537b0e429d
> src/tests/balloon_framework_test.sh
> 93a733f64cfde08349b7781eb3d5e13594c74498
>
> Diff: https://reviews.apache.org/r/9408/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request: Properly cleanup cgroups.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9408/
-----------------------------------------------------------
(Updated Feb. 19, 2013, 10:56 p.m.)
Review request for mesos, Benjamin Hindman and Ben Mahler.
Changes
-------
benm's
Description
-------
See summary
Diffs (updated)
-----
src/slave/cgroups_isolation_module.hpp 669efa14ba2603764aa68ae19a44e79dbfdec192
src/slave/cgroups_isolation_module.cpp 14f549edaf1b37a6bca8f75309864333ae775e7c
src/slave/process_based_isolation_module.hpp f1817192582e3646f8dcf17934ba7998829e8fd6
src/slave/process_based_isolation_module.cpp 12a579cba56cd3dac384bc7919b0d5537b0e429d
src/tests/balloon_framework_test.sh 93a733f64cfde08349b7781eb3d5e13594c74498
Diff: https://reviews.apache.org/r/9408/diff/
Testing
-------
make check
Thanks,
Vinod Kone
Re: Review Request: Properly cleanup cgroups.
Posted by Vinod Kone <vi...@gmail.com>.
> On Feb. 15, 2013, 1:41 a.m., Ben Mahler wrote:
> > src/slave/cgroups_isolation_module.cpp, line 437
> > <https://reviews.apache.org/r/9408/diff/6/?file=258837#file258837line437>
> >
> > Seems more like a LOG(ERROR) to me, given it's an unexpected error, and we can proceed to remove the rest of the cgroups, right?
I think this is a fatal enough error (un-cleaned cgroups => orphaned executors), that it needs manual intervention.
> On Feb. 15, 2013, 1:41 a.m., Ben Mahler wrote:
> > src/slave/cgroups_isolation_module.cpp, line 444
> > <https://reviews.apache.org/r/9408/diff/6/?file=258837#file258837line444>
> >
> > Ditto. ERROR or FATAL, given we expect to be able to remove the lock, right?
This is not the case when writing googletest style tests. If a test can't properly unlock the file, the subsequent test would fail.
> On Feb. 15, 2013, 1:41 a.m., Ben Mahler wrote:
> > src/slave/process_based_isolation_module.cpp, line 94
> > <https://reviews.apache.org/r/9408/diff/6/?file=258839#file258839line94>
> >
> > Since killExecutor is called from finalize(), can you add a note there to indicate that no dispatches should be added to that method?
> > Even with the note, this still feels a little error prone, should someone change killExecutor.
Good point. Added a note.
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9408/#review16622
-----------------------------------------------------------
On Feb. 14, 2013, 7:16 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9408/
> -----------------------------------------------------------
>
> (Updated Feb. 14, 2013, 7:16 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Description
> -------
>
> See summary
>
>
> Diffs
> -----
>
> src/slave/cgroups_isolation_module.hpp 669efa14ba2603764aa68ae19a44e79dbfdec192
> src/slave/cgroups_isolation_module.cpp 14f549edaf1b37a6bca8f75309864333ae775e7c
> src/slave/process_based_isolation_module.hpp f1817192582e3646f8dcf17934ba7998829e8fd6
> src/slave/process_based_isolation_module.cpp 12a579cba56cd3dac384bc7919b0d5537b0e429d
> src/tests/balloon_framework_test.sh 93a733f64cfde08349b7781eb3d5e13594c74498
>
> Diff: https://reviews.apache.org/r/9408/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request: Properly cleanup cgroups.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9408/#review16622
-----------------------------------------------------------
src/slave/cgroups_isolation_module.cpp
<https://reviews.apache.org/r/9408/#comment35284>
Seems more like a LOG(ERROR) to me, given it's an unexpected error, and we can proceed to remove the rest of the cgroups, right?
src/slave/cgroups_isolation_module.cpp
<https://reviews.apache.org/r/9408/#comment35285>
Ditto. ERROR or FATAL, given we expect to be able to remove the lock, right?
src/slave/process_based_isolation_module.cpp
<https://reviews.apache.org/r/9408/#comment35286>
Since killExecutor is called from finalize(), can you add a note there to indicate that no dispatches should be added to that method?
Even with the note, this still feels a little error prone, should someone change killExecutor.
- Ben Mahler
On Feb. 14, 2013, 7:16 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9408/
> -----------------------------------------------------------
>
> (Updated Feb. 14, 2013, 7:16 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Description
> -------
>
> See summary
>
>
> Diffs
> -----
>
> src/slave/cgroups_isolation_module.hpp 669efa14ba2603764aa68ae19a44e79dbfdec192
> src/slave/cgroups_isolation_module.cpp 14f549edaf1b37a6bca8f75309864333ae775e7c
> src/slave/process_based_isolation_module.hpp f1817192582e3646f8dcf17934ba7998829e8fd6
> src/slave/process_based_isolation_module.cpp 12a579cba56cd3dac384bc7919b0d5537b0e429d
> src/tests/balloon_framework_test.sh 93a733f64cfde08349b7781eb3d5e13594c74498
>
> Diff: https://reviews.apache.org/r/9408/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request: Properly cleanup cgroups.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9408/
-----------------------------------------------------------
(Updated Feb. 14, 2013, 7:16 p.m.)
Review request for mesos, Benjamin Hindman and Ben Mahler.
Changes
-------
benm's
Description
-------
See summary
Diffs (updated)
-----
src/slave/cgroups_isolation_module.hpp 669efa14ba2603764aa68ae19a44e79dbfdec192
src/slave/cgroups_isolation_module.cpp 14f549edaf1b37a6bca8f75309864333ae775e7c
src/slave/process_based_isolation_module.hpp f1817192582e3646f8dcf17934ba7998829e8fd6
src/slave/process_based_isolation_module.cpp 12a579cba56cd3dac384bc7919b0d5537b0e429d
src/tests/balloon_framework_test.sh 93a733f64cfde08349b7781eb3d5e13594c74498
Diff: https://reviews.apache.org/r/9408/diff/
Testing
-------
make check
Thanks,
Vinod Kone
Re: Review Request: Properly cleanup cgroups.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9408/
-----------------------------------------------------------
(Updated Feb. 14, 2013, 1:38 a.m.)
Review request for mesos, Benjamin Hindman and Ben Mahler.
Changes
-------
benm's.
Description
-------
See summary
Diffs (updated)
-----
src/slave/cgroups_isolation_module.hpp 669efa14ba2603764aa68ae19a44e79dbfdec192
src/slave/cgroups_isolation_module.cpp 14f549edaf1b37a6bca8f75309864333ae775e7c
src/slave/process_based_isolation_module.hpp f1817192582e3646f8dcf17934ba7998829e8fd6
src/slave/process_based_isolation_module.cpp 12a579cba56cd3dac384bc7919b0d5537b0e429d
src/tests/balloon_framework_test.sh 93a733f64cfde08349b7781eb3d5e13594c74498
Diff: https://reviews.apache.org/r/9408/diff/
Testing
-------
make check
Thanks,
Vinod Kone