You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Ian Downes <ia...@gmail.com> on 2014/10/28 19:28:11 UTC
Review Request 27295: os::killtree() supports case when root pid has
exited.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27295/
-----------------------------------------------------------
Review request for mesos, Ben Mahler and Vinod Kone.
Bugs: MESOS-1875
https://issues.apache.org/jira/browse/MESOS-1875
Repository: mesos-git
Description
-------
Improve os::killtree() for the case when the root pid has exited and
it has been instructed to either follow process groups or sessions. It
will now kill all processes (and their trees) with pgid or session
matching the root pid.
Diffs
-----
3rdparty/libprocess/3rdparty/stout/include/stout/os/killtree.hpp fa1950cc8849fcd81d425c651848e05661111078
3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp e9f37dfd3cf54c3ca85d74716b3ee4262b065fad
Diff: https://reviews.apache.org/r/27295/diff/
Testing
-------
Added a test where the root pid exits (and is reaped), leaving behind a reparented subtree. This test initially fails.
+ // -+- child exit 0 [new session and process group leader]
+ // -+- grandchild sleep 100
+ // -+- great grandchild sleep 100
Thanks,
Ian Downes
Re: Review Request 27295: os::killtree() supports case when root pid
has exited.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27295/#review59017
-----------------------------------------------------------
Ship it!
Ship It!
- Vinod Kone
On Oct. 29, 2014, 5:26 p.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27295/
> -----------------------------------------------------------
>
> (Updated Oct. 29, 2014, 5:26 p.m.)
>
>
> Review request for mesos, Ben Mahler and Vinod Kone.
>
>
> Bugs: MESOS-1875
> https://issues.apache.org/jira/browse/MESOS-1875
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Improve os::killtree() for the case when the root pid has exited and
> it has been instructed to either follow process groups or sessions. It
> will now kill all processes (and their trees) with pgid or session
> matching the root pid.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/os/killtree.hpp fa1950cc8849fcd81d425c651848e05661111078
> 3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp e9f37dfd3cf54c3ca85d74716b3ee4262b065fad
>
> Diff: https://reviews.apache.org/r/27295/diff/
>
>
> Testing
> -------
>
> Added a test where the root pid exits (and is reaped), leaving behind a reparented subtree. This test initially fails.
>
> + // -+- child exit 0 [new session and process group leader]
> + // -+- grandchild sleep 100
> + // -+- great grandchild sleep 100
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 27295: os::killtree() supports case when root pid
has exited.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27295/#review59019
-----------------------------------------------------------
Patch looks great!
Reviews applied: [27295]
All tests passed.
- Mesos ReviewBot
On Oct. 29, 2014, 5:26 p.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27295/
> -----------------------------------------------------------
>
> (Updated Oct. 29, 2014, 5:26 p.m.)
>
>
> Review request for mesos, Ben Mahler and Vinod Kone.
>
>
> Bugs: MESOS-1875
> https://issues.apache.org/jira/browse/MESOS-1875
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Improve os::killtree() for the case when the root pid has exited and
> it has been instructed to either follow process groups or sessions. It
> will now kill all processes (and their trees) with pgid or session
> matching the root pid.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/os/killtree.hpp fa1950cc8849fcd81d425c651848e05661111078
> 3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp e9f37dfd3cf54c3ca85d74716b3ee4262b065fad
>
> Diff: https://reviews.apache.org/r/27295/diff/
>
>
> Testing
> -------
>
> Added a test where the root pid exits (and is reaped), leaving behind a reparented subtree. This test initially fails.
>
> + // -+- child exit 0 [new session and process group leader]
> + // -+- grandchild sleep 100
> + // -+- great grandchild sleep 100
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 27295: os::killtree() supports case when root pid
has exited.
Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27295/
-----------------------------------------------------------
(Updated Oct. 29, 2014, 10:26 a.m.)
Review request for mesos, Ben Mahler and Vinod Kone.
Changes
-------
Rebased.
Bugs: MESOS-1875
https://issues.apache.org/jira/browse/MESOS-1875
Repository: mesos-git
Description
-------
Improve os::killtree() for the case when the root pid has exited and
it has been instructed to either follow process groups or sessions. It
will now kill all processes (and their trees) with pgid or session
matching the root pid.
Diffs (updated)
-----
3rdparty/libprocess/3rdparty/stout/include/stout/os/killtree.hpp fa1950cc8849fcd81d425c651848e05661111078
3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp e9f37dfd3cf54c3ca85d74716b3ee4262b065fad
Diff: https://reviews.apache.org/r/27295/diff/
Testing
-------
Added a test where the root pid exits (and is reaped), leaving behind a reparented subtree. This test initially fails.
+ // -+- child exit 0 [new session and process group leader]
+ // -+- grandchild sleep 100
+ // -+- great grandchild sleep 100
Thanks,
Ian Downes
Re: Review Request 27295: os::killtree() supports case when root pid
has exited.
Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27295/
-----------------------------------------------------------
(Updated Oct. 29, 2014, 10:18 a.m.)
Review request for mesos, Ben Mahler and Vinod Kone.
Changes
-------
Addressed Vinod's comments.
Bugs: MESOS-1875
https://issues.apache.org/jira/browse/MESOS-1875
Repository: mesos-git
Description
-------
Improve os::killtree() for the case when the root pid has exited and
it has been instructed to either follow process groups or sessions. It
will now kill all processes (and their trees) with pgid or session
matching the root pid.
Diffs (updated)
-----
3rdparty/libprocess/3rdparty/Makefile.am 0dc2871af7d8c6646080263c71f41db855d50be4
3rdparty/libprocess/3rdparty/stout/Makefile.am 7aac3fdb864e838cdd4e0d817ed4dff14923c69d
3rdparty/libprocess/3rdparty/stout/include/Makefile.am 00a4edd0bcde3f92630fb173aeb4fff8e9139f77
3rdparty/libprocess/3rdparty/stout/include/stout/os/killtree.hpp fa1950cc8849fcd81d425c651848e05661111078
3rdparty/libprocess/3rdparty/stout/tests/os/setns_tests.cpp PRE-CREATION
3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp e9f37dfd3cf54c3ca85d74716b3ee4262b065fad
3rdparty/libprocess/configure.ac 18eb681a5779b15cc5c7b7187ba5da8b4059c530
src/Makefile.am 374f284e1ac839fbcd8a28171b1ff4fbe8a17bd4
src/linux/ns.hpp ec1a5171c8923d8661caab9999d0d73d5140bee4
src/slave/containerizer/isolators/namespaces/pid.hpp 7c40e7730e690e69a3bbef02a46ccb32ebc6badf
src/slave/containerizer/isolators/namespaces/pid.cpp 5a13a6d1249a8aca416080bcb509b703cf48d244
src/slave/containerizer/isolators/network/port_mapping.cpp 14fae1f00050afbc6b99f4aabf868a2d75774b15
src/slave/containerizer/linux_launcher.cpp 10c12039cf684bef9398da72c3eceb9ed8b8b9c9
src/slave/containerizer/mesos/containerizer.cpp d4b08f54d6feb453f3a9d27ca54c867176e62102
src/tests/isolator_tests.cpp 4d03f46cbe20c0e7aff15bab0a26d7738b55aab9
src/tests/ns_tests.cpp 872009fbd5e48f74f7183da085d462fb4b7b61d0
src/tests/setns_test_helper.hpp c6bec95632f7426c76a02eaddbcedade85e17436
src/tests/setns_test_helper.cpp eb8746bd6e75323682d9dbbf989b65e30e3c1cda
src/tests/slave_recovery_tests.cpp 98e059f33b2b9f552b0a091ded97f78c3b217d45
Diff: https://reviews.apache.org/r/27295/diff/
Testing
-------
Added a test where the root pid exits (and is reaped), leaving behind a reparented subtree. This test initially fails.
+ // -+- child exit 0 [new session and process group leader]
+ // -+- grandchild sleep 100
+ // -+- great grandchild sleep 100
Thanks,
Ian Downes
Re: Review Request 27295: os::killtree() supports case when root pid
has exited.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27295/#review58855
-----------------------------------------------------------
Patch looks great!
Reviews applied: [27295]
All tests passed.
- Mesos ReviewBot
On Oct. 28, 2014, 6:28 p.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27295/
> -----------------------------------------------------------
>
> (Updated Oct. 28, 2014, 6:28 p.m.)
>
>
> Review request for mesos, Ben Mahler and Vinod Kone.
>
>
> Bugs: MESOS-1875
> https://issues.apache.org/jira/browse/MESOS-1875
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Improve os::killtree() for the case when the root pid has exited and
> it has been instructed to either follow process groups or sessions. It
> will now kill all processes (and their trees) with pgid or session
> matching the root pid.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/os/killtree.hpp fa1950cc8849fcd81d425c651848e05661111078
> 3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp e9f37dfd3cf54c3ca85d74716b3ee4262b065fad
>
> Diff: https://reviews.apache.org/r/27295/diff/
>
>
> Testing
> -------
>
> Added a test where the root pid exits (and is reaped), leaving behind a reparented subtree. This test initially fails.
>
> + // -+- child exit 0 [new session and process group leader]
> + // -+- grandchild sleep 100
> + // -+- great grandchild sleep 100
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 27295: os::killtree() supports case when root pid
has exited.
Posted by Ian Downes <ia...@gmail.com>.
> On Oct. 28, 2014, 5:40 p.m., Vinod Kone wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/os/killtree.hpp, line 161
> > <https://reviews.apache.org/r/27295/diff/1/?file=735584#file735584line161>
> >
> > Don't you have to check that child is not already in queue because it might be added in #80 or #84?
No, we mark visited pids on #145 so on #127 we'll just continue if the pid has already been visited, i.e., it's okay to add a pid we've already seen or is already in the queue.
> On Oct. 28, 2014, 5:40 p.m., Vinod Kone wrote:
> > 3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp, line 682
> > <https://reviews.apache.org/r/27295/diff/1/?file=735585#file735585line682>
> >
> > #669 seems to indicate that process could be none when we are here? why not have ASSERT_SOME(process) up there instead?
Good catch but it's the isNone() that is erroneous. We wait for it to exit and become zombied so that we can then reap it.
- Ian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27295/#review58845
-----------------------------------------------------------
On Oct. 28, 2014, 11:28 a.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27295/
> -----------------------------------------------------------
>
> (Updated Oct. 28, 2014, 11:28 a.m.)
>
>
> Review request for mesos, Ben Mahler and Vinod Kone.
>
>
> Bugs: MESOS-1875
> https://issues.apache.org/jira/browse/MESOS-1875
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Improve os::killtree() for the case when the root pid has exited and
> it has been instructed to either follow process groups or sessions. It
> will now kill all processes (and their trees) with pgid or session
> matching the root pid.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/os/killtree.hpp fa1950cc8849fcd81d425c651848e05661111078
> 3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp e9f37dfd3cf54c3ca85d74716b3ee4262b065fad
>
> Diff: https://reviews.apache.org/r/27295/diff/
>
>
> Testing
> -------
>
> Added a test where the root pid exits (and is reaped), leaving behind a reparented subtree. This test initially fails.
>
> + // -+- child exit 0 [new session and process group leader]
> + // -+- grandchild sleep 100
> + // -+- great grandchild sleep 100
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 27295: os::killtree() supports case when root pid
has exited.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27295/#review58845
-----------------------------------------------------------
3rdparty/libprocess/3rdparty/stout/include/stout/os/killtree.hpp
<https://reviews.apache.org/r/27295/#comment100001>
s/process/process 'pid'/
3rdparty/libprocess/3rdparty/stout/include/stout/os/killtree.hpp
<https://reviews.apache.org/r/27295/#comment100000>
s/cgroups/groups/ :)
3rdparty/libprocess/3rdparty/stout/include/stout/os/killtree.hpp
<https://reviews.apache.org/r/27295/#comment100122>
Don't you have to check that child is not already in queue because it might be added in #80 or #84?
3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp
<https://reviews.apache.org/r/27295/#comment100128>
#669 seems to indicate that process could be none when we are here? why not have ASSERT_SOME(process) up there instead?
- Vinod Kone
On Oct. 28, 2014, 6:28 p.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27295/
> -----------------------------------------------------------
>
> (Updated Oct. 28, 2014, 6:28 p.m.)
>
>
> Review request for mesos, Ben Mahler and Vinod Kone.
>
>
> Bugs: MESOS-1875
> https://issues.apache.org/jira/browse/MESOS-1875
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Improve os::killtree() for the case when the root pid has exited and
> it has been instructed to either follow process groups or sessions. It
> will now kill all processes (and their trees) with pgid or session
> matching the root pid.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/os/killtree.hpp fa1950cc8849fcd81d425c651848e05661111078
> 3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp e9f37dfd3cf54c3ca85d74716b3ee4262b065fad
>
> Diff: https://reviews.apache.org/r/27295/diff/
>
>
> Testing
> -------
>
> Added a test where the root pid exits (and is reaped), leaving behind a reparented subtree. This test initially fails.
>
> + // -+- child exit 0 [new session and process group leader]
> + // -+- grandchild sleep 100
> + // -+- great grandchild sleep 100
>
>
> Thanks,
>
> Ian Downes
>
>