You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Timothy Chen <tn...@apache.org> on 2014/10/21 00:08:05 UTC
Re: Review Request 26486: Fix containerizer not receiving
destroy/update calls when launching.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26486/
-----------------------------------------------------------
(Updated Oct. 20, 2014, 10:08 p.m.)
Review request for mesos and Benjamin Hindman.
Summary (updated)
-----------------
Fix containerizer not receiving destroy/update calls when launching.
Bugs: MESOS-1884 and MESOS-1915
https://issues.apache.org/jira/browse/MESOS-1884
https://issues.apache.org/jira/browse/MESOS-1915
Repository: mesos-git
Description
-------
Review: https://reviews.apache.org/r/26486
Diffs (updated)
-----
src/Makefile.am c44a9ad47d6e1262949b9049f4ae25b049440d99
src/slave/containerizer/composing.cpp 9022700b628d9746a6a8a17c9fbf1b1988da6fca
src/tests/composing_containerizer_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/26486/diff/
Testing
-------
make check
Thanks,
Timothy Chen
Re: Review Request 26486: Fix containerizer not receiving
destroy/update calls when launching.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26486/#review57455
-----------------------------------------------------------
Patch looks great!
Reviews applied: [26486]
All tests passed.
- Mesos ReviewBot
On Oct. 20, 2014, 10:08 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26486/
> -----------------------------------------------------------
>
> (Updated Oct. 20, 2014, 10:08 p.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Bugs: MESOS-1884 and MESOS-1915
> https://issues.apache.org/jira/browse/MESOS-1884
> https://issues.apache.org/jira/browse/MESOS-1915
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Review: https://reviews.apache.org/r/26486
>
>
> Diffs
> -----
>
> src/Makefile.am c44a9ad47d6e1262949b9049f4ae25b049440d99
> src/slave/containerizer/composing.cpp 9022700b628d9746a6a8a17c9fbf1b1988da6fca
> src/tests/composing_containerizer_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/26486/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 26486: Fix containerizer not receiving
destroy/update calls when launching.
Posted by Timothy Chen <tn...@apache.org>.
> On Oct. 21, 2014, 2:18 a.m., Benjamin Hindman wrote:
> > src/tests/composing_containerizer_tests.cpp, lines 156-157
> > <https://reviews.apache.org/r/26486/diff/3/?file=726515#file726515line156>
> >
> > Cool, let's test this part too! I think all you will need is the following above:
> >
> > EXPECT_CALL(*mockContainerizer2, launch(_, _, _, _, _, _, _, _))
> > .Times(0);
> >
> > And then this at the bottom after AWAIT_READY(destroy);
> >
> >
> > launchPromise.set(false);
> > AWAIT_READY_EQ(false, launch);
I'm actually throwing a failure in the end. I don't think false is the right choice still false signals that we don't support this TaskInfo, but the truth is that it was destroyed!
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26486/#review57522
-----------------------------------------------------------
On Oct. 21, 2014, 4:42 a.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26486/
> -----------------------------------------------------------
>
> (Updated Oct. 21, 2014, 4:42 a.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Bugs: MESOS-1884 and MESOS-1915
> https://issues.apache.org/jira/browse/MESOS-1884
> https://issues.apache.org/jira/browse/MESOS-1915
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Review: https://reviews.apache.org/r/26486
>
>
> Diffs
> -----
>
> src/Makefile.am c44a9ad47d6e1262949b9049f4ae25b049440d99
> src/slave/containerizer/composing.cpp 9022700b628d9746a6a8a17c9fbf1b1988da6fca
> src/tests/composing_containerizer_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/26486/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 26486: Fix containerizer not receiving
destroy/update calls when launching.
Posted by Timothy Chen <tn...@apache.org>.
> On Oct. 21, 2014, 2:18 a.m., Benjamin Hindman wrote:
> > src/slave/containerizer/composing.cpp, line 594
> > <https://reviews.apache.org/r/26486/diff/3/?file=726514#file726514line594>
> >
> > What if destroy is called twice? I think this breaks your invariant?
If the container is already launched, then it's fine since we remove the container from the map. If it's launching, then yes the current code forwads it twice.
I added a check to only forward it once.
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26486/#review57522
-----------------------------------------------------------
On Oct. 20, 2014, 10:08 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26486/
> -----------------------------------------------------------
>
> (Updated Oct. 20, 2014, 10:08 p.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Bugs: MESOS-1884 and MESOS-1915
> https://issues.apache.org/jira/browse/MESOS-1884
> https://issues.apache.org/jira/browse/MESOS-1915
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Review: https://reviews.apache.org/r/26486
>
>
> Diffs
> -----
>
> src/Makefile.am c44a9ad47d6e1262949b9049f4ae25b049440d99
> src/slave/containerizer/composing.cpp 9022700b628d9746a6a8a17c9fbf1b1988da6fca
> src/tests/composing_containerizer_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/26486/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 26486: Fix containerizer not receiving
destroy/update calls when launching.
Posted by Timothy Chen <tn...@apache.org>.
> On Oct. 21, 2014, 2:18 a.m., Benjamin Hindman wrote:
> > src/tests/composing_containerizer_tests.cpp, line 151
> > <https://reviews.apache.org/r/26486/diff/3/?file=726515#file726515line151>
> >
> > EXPECT_PENDING?
Still can't find where it is?
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26486/#review57522
-----------------------------------------------------------
On Oct. 21, 2014, 4:42 a.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26486/
> -----------------------------------------------------------
>
> (Updated Oct. 21, 2014, 4:42 a.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Bugs: MESOS-1884 and MESOS-1915
> https://issues.apache.org/jira/browse/MESOS-1884
> https://issues.apache.org/jira/browse/MESOS-1915
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Review: https://reviews.apache.org/r/26486
>
>
> Diffs
> -----
>
> src/Makefile.am c44a9ad47d6e1262949b9049f4ae25b049440d99
> src/slave/containerizer/composing.cpp 9022700b628d9746a6a8a17c9fbf1b1988da6fca
> src/tests/composing_containerizer_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/26486/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 26486: Fix containerizer not receiving
destroy/update calls when launching.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26486/#review57522
-----------------------------------------------------------
Looking so much cleaner Tim! Thanks for taking it on to improve the original implementation.
src/slave/containerizer/composing.cpp
<https://reviews.apache.org/r/26486/#comment98256>
{ on newline please.
src/slave/containerizer/composing.cpp
<https://reviews.apache.org/r/26486/#comment98254>
Can you add a comment here that your assumption is that even if we've forwarded the destroy to the containerizer we will in fact eventually get the 'launch' response and won't clean up 'containers_' until then.
src/slave/containerizer/composing.cpp
<https://reviews.apache.org/r/26486/#comment98245>
I tend to swap these since I don'k like the idea of deleting something that is still in the container! ;-)
src/slave/containerizer/composing.cpp
<https://reviews.apache.org/r/26486/#comment98253>
Could you make this a ternary if to clean it up?
src/slave/containerizer/composing.cpp
<https://reviews.apache.org/r/26486/#comment98246>
s/ is//g
src/slave/containerizer/composing.cpp
<https://reviews.apache.org/r/26486/#comment98248>
Can we comment on why calling through to destroy is "okay" even if that containerizer is not going to launch the container?
src/slave/containerizer/composing.cpp
<https://reviews.apache.org/r/26486/#comment98247>
Great comment, and here's a wording/language suggestion:
Record the fact that this container was asked to be destroyed so that we won't try and launch this container using any other containerizers in the event the current containerizer has decided it can't launch the container.
src/slave/containerizer/composing.cpp
<https://reviews.apache.org/r/26486/#comment98255>
What if destroy is called twice? I think this breaks your invariant?
src/tests/composing_containerizer_tests.cpp
<https://reviews.apache.org/r/26486/#comment98252>
EXPECT_PENDING?
src/tests/composing_containerizer_tests.cpp
<https://reviews.apache.org/r/26486/#comment98251>
Cool, let's test this part too! I think all you will need is the following above:
EXPECT_CALL(*mockContainerizer2, launch(_, _, _, _, _, _, _, _))
.Times(0);
And then this at the bottom after AWAIT_READY(destroy);
launchPromise.set(false);
AWAIT_READY_EQ(false, launch);
- Benjamin Hindman
On Oct. 20, 2014, 10:08 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26486/
> -----------------------------------------------------------
>
> (Updated Oct. 20, 2014, 10:08 p.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Bugs: MESOS-1884 and MESOS-1915
> https://issues.apache.org/jira/browse/MESOS-1884
> https://issues.apache.org/jira/browse/MESOS-1915
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Review: https://reviews.apache.org/r/26486
>
>
> Diffs
> -----
>
> src/Makefile.am c44a9ad47d6e1262949b9049f4ae25b049440d99
> src/slave/containerizer/composing.cpp 9022700b628d9746a6a8a17c9fbf1b1988da6fca
> src/tests/composing_containerizer_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/26486/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 26486: Fix containerizer not receiving
destroy/update calls when launching.
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26486/
-----------------------------------------------------------
(Updated Oct. 31, 2014, 10:35 p.m.)
Review request for mesos and Benjamin Hindman.
Bugs: MESOS-1884 and MESOS-1915
https://issues.apache.org/jira/browse/MESOS-1884
https://issues.apache.org/jira/browse/MESOS-1915
Repository: mesos-git
Description
-------
Review: https://reviews.apache.org/r/26486
Diffs (updated)
-----
src/Makefile.am 2d72a70
src/slave/containerizer/composing.cpp 9022700
src/tests/composing_containerizer_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/26486/diff/
Testing
-------
make check
Thanks,
Timothy Chen
Re: Review Request 26486: Fix containerizer not receiving
destroy/update calls when launching.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26486/#review57551
-----------------------------------------------------------
Patch looks great!
Reviews applied: [26486]
All tests passed.
- Mesos ReviewBot
On Oct. 21, 2014, 4:42 a.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26486/
> -----------------------------------------------------------
>
> (Updated Oct. 21, 2014, 4:42 a.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Bugs: MESOS-1884 and MESOS-1915
> https://issues.apache.org/jira/browse/MESOS-1884
> https://issues.apache.org/jira/browse/MESOS-1915
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Review: https://reviews.apache.org/r/26486
>
>
> Diffs
> -----
>
> src/Makefile.am c44a9ad47d6e1262949b9049f4ae25b049440d99
> src/slave/containerizer/composing.cpp 9022700b628d9746a6a8a17c9fbf1b1988da6fca
> src/tests/composing_containerizer_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/26486/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 26486: Fix containerizer not receiving
destroy/update calls when launching.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26486/#review59350
-----------------------------------------------------------
Ship it!
src/slave/containerizer/composing.cpp
<https://reviews.apache.org/r/26486/#comment100620>
Move update to previous line and wrap after '(' please.
- Benjamin Hindman
On Oct. 21, 2014, 4:42 a.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26486/
> -----------------------------------------------------------
>
> (Updated Oct. 21, 2014, 4:42 a.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Bugs: MESOS-1884 and MESOS-1915
> https://issues.apache.org/jira/browse/MESOS-1884
> https://issues.apache.org/jira/browse/MESOS-1915
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Review: https://reviews.apache.org/r/26486
>
>
> Diffs
> -----
>
> src/Makefile.am c44a9ad47d6e1262949b9049f4ae25b049440d99
> src/slave/containerizer/composing.cpp 9022700b628d9746a6a8a17c9fbf1b1988da6fca
> src/tests/composing_containerizer_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/26486/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 26486: Fix containerizer not receiving
destroy/update calls when launching.
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26486/
-----------------------------------------------------------
(Updated Oct. 21, 2014, 4:42 a.m.)
Review request for mesos and Benjamin Hindman.
Bugs: MESOS-1884 and MESOS-1915
https://issues.apache.org/jira/browse/MESOS-1884
https://issues.apache.org/jira/browse/MESOS-1915
Repository: mesos-git
Description
-------
Review: https://reviews.apache.org/r/26486
Diffs (updated)
-----
src/Makefile.am c44a9ad47d6e1262949b9049f4ae25b049440d99
src/slave/containerizer/composing.cpp 9022700b628d9746a6a8a17c9fbf1b1988da6fca
src/tests/composing_containerizer_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/26486/diff/
Testing
-------
make check
Thanks,
Timothy Chen