You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Kevin Klues <kl...@gmail.com> on 2017/01/21 01:03:17 UTC
Review Request 55810: Fixed bug allowing IOSwitchboard::connect()
after container destruction.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55810/
-----------------------------------------------------------
Review request for mesos, Anand Mazumdar, Benjamin Bannier, Greg Mann, Jie Yu, and Vinod Kone.
Bugs: MESOS-6948
https://issues.apache.org/jira/browse/MESOS-6948
Repository: mesos
Description
-------
Fixed bug allowing IOSwitchboard::connect() after container destruction.
Diffs
-----
src/slave/containerizer/mesos/io/switchboard.cpp 4b134f8420e1ef5cd139e0e4b8b08b1ff32e3a30
Diff: https://reviews.apache.org/r/55810/diff/
Testing
-------
GTEST_FILTER="ContentType/AgentAPITest.LaunchNestedContainerSession/1" src/mesos-tests --gtest_repeat=-1 --gtest_break_on_failure
Thanks,
Kevin Klues
Re: Review Request 55810: Fixed bug allowing IOSwitchboard::connect()
after container destruction.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55810/#review162551
-----------------------------------------------------------
Ship it!
With this fix I couldn't reproduce the error I was seeing in 1000+ test iterations testing both parameterizations of this test.
- Benjamin Bannier
On Jan. 21, 2017, 2:03 a.m., Kevin Klues wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55810/
> -----------------------------------------------------------
>
> (Updated Jan. 21, 2017, 2:03 a.m.)
>
>
> Review request for mesos, Anand Mazumdar, Benjamin Bannier, Greg Mann, Jie Yu, and Vinod Kone.
>
>
> Bugs: MESOS-6948
> https://issues.apache.org/jira/browse/MESOS-6948
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Fixed bug allowing IOSwitchboard::connect() after container destruction.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/io/switchboard.cpp 4b134f8420e1ef5cd139e0e4b8b08b1ff32e3a30
>
> Diff: https://reviews.apache.org/r/55810/diff/
>
>
> Testing
> -------
>
> GTEST_FILTER="ContentType/AgentAPITest.LaunchNestedContainerSession/1" src/mesos-tests --gtest_repeat=-1 --gtest_break_on_failure
>
>
> Thanks,
>
> Kevin Klues
>
>
Re: Review Request 55810: Fixed bug allowing IOSwitchboard::connect()
after container destruction.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55810/#review162553
-----------------------------------------------------------
Patch looks great!
Reviews applied: [55810]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
On Jan. 21, 2017, 1:03 a.m., Kevin Klues wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55810/
> -----------------------------------------------------------
>
> (Updated Jan. 21, 2017, 1:03 a.m.)
>
>
> Review request for mesos, Anand Mazumdar, Benjamin Bannier, Greg Mann, Jie Yu, and Vinod Kone.
>
>
> Bugs: MESOS-6948
> https://issues.apache.org/jira/browse/MESOS-6948
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Fixed bug allowing IOSwitchboard::connect() after container destruction.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/io/switchboard.cpp 4b134f8420e1ef5cd139e0e4b8b08b1ff32e3a30
>
> Diff: https://reviews.apache.org/r/55810/diff/
>
>
> Testing
> -------
>
> GTEST_FILTER="ContentType/AgentAPITest.LaunchNestedContainerSession/1" src/mesos-tests --gtest_repeat=-1 --gtest_break_on_failure
>
>
> Thanks,
>
> Kevin Klues
>
>
Re: Review Request 55810: Fixed bug allowing IOSwitchboard::connect()
after container destruction.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55810/#review162864
-----------------------------------------------------------
Ship it!
Ship It!
- Vinod Kone
On Jan. 24, 2017, 10:45 p.m., Kevin Klues wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55810/
> -----------------------------------------------------------
>
> (Updated Jan. 24, 2017, 10:45 p.m.)
>
>
> Review request for mesos, Anand Mazumdar, Benjamin Bannier, Greg Mann, Jie Yu, and Vinod Kone.
>
>
> Bugs: MESOS-6948
> https://issues.apache.org/jira/browse/MESOS-6948
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Fixed bug allowing IOSwitchboard::connect() after container destruction.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/io/switchboard.cpp 4b134f8420e1ef5cd139e0e4b8b08b1ff32e3a30
>
> Diff: https://reviews.apache.org/r/55810/diff/
>
>
> Testing
> -------
>
> GTEST_FILTER="ContentType/AgentAPITest.LaunchNestedContainerSession/1" src/mesos-tests --gtest_repeat=-1 --gtest_break_on_failure
>
>
> Thanks,
>
> Kevin Klues
>
>
Re: Review Request 55810: Fixed bug allowing IOSwitchboard::connect()
after container destruction.
Posted by Vinod Kone <vi...@gmail.com>.
> On Jan. 24, 2017, 11:04 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/io/switchboard.cpp, lines 761-762
> > <https://reviews.apache.org/r/55810/diff/1/?file=1611305#file1611305line761>
> >
> > Hum, then, it's possible that we install two timers and sending sigterm twice?
yes, and i think that's safe?
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55810/#review162684
-----------------------------------------------------------
On Jan. 24, 2017, 10:45 p.m., Kevin Klues wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55810/
> -----------------------------------------------------------
>
> (Updated Jan. 24, 2017, 10:45 p.m.)
>
>
> Review request for mesos, Anand Mazumdar, Benjamin Bannier, Greg Mann, Jie Yu, and Vinod Kone.
>
>
> Bugs: MESOS-6948
> https://issues.apache.org/jira/browse/MESOS-6948
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Fixed bug allowing IOSwitchboard::connect() after container destruction.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/io/switchboard.cpp 4b134f8420e1ef5cd139e0e4b8b08b1ff32e3a30
>
> Diff: https://reviews.apache.org/r/55810/diff/
>
>
> Testing
> -------
>
> GTEST_FILTER="ContentType/AgentAPITest.LaunchNestedContainerSession/1" src/mesos-tests --gtest_repeat=-1 --gtest_break_on_failure
>
>
> Thanks,
>
> Kevin Klues
>
>
Re: Review Request 55810: Fixed bug allowing IOSwitchboard::connect()
after container destruction.
Posted by Kevin Klues <kl...@gmail.com>.
> On Jan. 24, 2017, 11:04 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/io/switchboard.cpp, lines 761-762
> > <https://reviews.apache.org/r/55810/diff/1/?file=1611305#file1611305line761>
> >
> > Hum, then, it's possible that we install two timers and sending sigterm twice?
>
> Vinod Kone wrote:
> yes, and i think that's safe?
Yeah, Vinod and I talked about this because I raised the same issue with him when we were discussing the change in person. One way to avoid this would be to introduce a `state` field to our info struct and transition it into the `DESTROYING` state once cleanup has been called the first time, but not fully remove it from the infos list. We could then change the check above to:
```
if (!infos.contains(containerId) || infos[containerId].state == DESTROYING) {
return Nothing();
}
```
In the end, we decided not to make this change for now because it should be safe to start the timer multiple times (and even run the body of the lambda multiple times) without breaking anything. We would just be doing extra, unnecessary work.
- Kevin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55810/#review162684
-----------------------------------------------------------
On Jan. 24, 2017, 10:45 p.m., Kevin Klues wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55810/
> -----------------------------------------------------------
>
> (Updated Jan. 24, 2017, 10:45 p.m.)
>
>
> Review request for mesos, Anand Mazumdar, Benjamin Bannier, Greg Mann, Jie Yu, and Vinod Kone.
>
>
> Bugs: MESOS-6948
> https://issues.apache.org/jira/browse/MESOS-6948
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Fixed bug allowing IOSwitchboard::connect() after container destruction.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/io/switchboard.cpp 4b134f8420e1ef5cd139e0e4b8b08b1ff32e3a30
>
> Diff: https://reviews.apache.org/r/55810/diff/
>
>
> Testing
> -------
>
> GTEST_FILTER="ContentType/AgentAPITest.LaunchNestedContainerSession/1" src/mesos-tests --gtest_repeat=-1 --gtest_break_on_failure
>
>
> Thanks,
>
> Kevin Klues
>
>
Re: Review Request 55810: Fixed bug allowing IOSwitchboard::connect()
after container destruction.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55810/#review162684
-----------------------------------------------------------
src/slave/containerizer/mesos/io/switchboard.cpp
<https://reviews.apache.org/r/55810/#comment234028>
Hum, then, it's possible that we install two timers and sending sigterm twice?
- Jie Yu
On Jan. 24, 2017, 10:45 p.m., Kevin Klues wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55810/
> -----------------------------------------------------------
>
> (Updated Jan. 24, 2017, 10:45 p.m.)
>
>
> Review request for mesos, Anand Mazumdar, Benjamin Bannier, Greg Mann, Jie Yu, and Vinod Kone.
>
>
> Bugs: MESOS-6948
> https://issues.apache.org/jira/browse/MESOS-6948
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Fixed bug allowing IOSwitchboard::connect() after container destruction.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/io/switchboard.cpp 4b134f8420e1ef5cd139e0e4b8b08b1ff32e3a30
>
> Diff: https://reviews.apache.org/r/55810/diff/
>
>
> Testing
> -------
>
> GTEST_FILTER="ContentType/AgentAPITest.LaunchNestedContainerSession/1" src/mesos-tests --gtest_repeat=-1 --gtest_break_on_failure
>
>
> Thanks,
>
> Kevin Klues
>
>
Re: Review Request 55810: Fixed bug allowing IOSwitchboard::connect()
after container destruction.
Posted by Kevin Klues <kl...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55810/
-----------------------------------------------------------
(Updated Jan. 24, 2017, 10:45 p.m.)
Review request for mesos, Anand Mazumdar, Benjamin Bannier, Greg Mann, Jie Yu, and Vinod Kone.
Changes
-------
Updated comment.
Bugs: MESOS-6948
https://issues.apache.org/jira/browse/MESOS-6948
Repository: mesos
Description
-------
Fixed bug allowing IOSwitchboard::connect() after container destruction.
Diffs (updated)
-----
src/slave/containerizer/mesos/io/switchboard.cpp 4b134f8420e1ef5cd139e0e4b8b08b1ff32e3a30
Diff: https://reviews.apache.org/r/55810/diff/
Testing
-------
GTEST_FILTER="ContentType/AgentAPITest.LaunchNestedContainerSession/1" src/mesos-tests --gtest_repeat=-1 --gtest_break_on_failure
Thanks,
Kevin Klues