You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Benjamin Hindman <be...@berkeley.edu> on 2014/07/15 00:30:50 UTC
Review Request 23462: Added composing containerizer and --containerizers
flag.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23462/
-----------------------------------------------------------
Review request for mesos, Ian Downes and Jie Yu.
Bugs: MESOS-1527
https://issues.apache.org/jira/browse/MESOS-1527
Repository: mesos-git
Description
-------
Next part of MESOS-1527, see dependent review 23461 for the first phase.
Diffs
-----
src/Makefile.am 2c7bfc57103c47523466c297f2596eaf4eb55c3c
src/slave/containerizer/composing.hpp PRE-CREATION
src/slave/containerizer/composing.cpp PRE-CREATION
src/slave/containerizer/containerizer.cpp 1b71f33b430645300d171eadc9f7de96d28adf60
src/slave/flags.hpp 1fe7b7da5aaa221ed6b94203b1189a80b14edd25
Diff: https://reviews.apache.org/r/23462/diff/
Testing
-------
make check
Thanks,
Benjamin Hindman
Re: Review Request 23462: Added composing containerizer and --containerizers
flag.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23462/#review47907
-----------------------------------------------------------
Ship it!
src/slave/containerizer/composing.cpp
<https://reviews.apache.org/r/23462/#comment84132>
Can you use the following here:
foreach (Containerizer* containerizer, containerizers_) {
...
}
This reads better.
src/slave/containerizer/composing.cpp
<https://reviews.apache.org/r/23462/#comment84142>
Now I see why you are doing this. It's a little hard to follow. And you are relying on the fact that 'containers' and '_containers' are in _sync_ (implicit assumption). How about the following:
Future<Nothing> _recover(const Option<state::SlaveState>& state)
{
list<Future<Nothing> > futures;
foreach (Containerizer* containerizer, containerizers_) {
futures.push_back(
containerizer->containers()
.then(defer(self(), &Self::__recover, lambda::_1, containerizer)));
}
return collect(futures)
.then(lambda::bind(&_nothing));
}
Future<Nothing> __recover(const hashset<ContainerID>& containers, Containerizer* containerizer)
{
containers_[containerizer] = containers;
return Nothing();
}
src/slave/containerizer/composing.cpp
<https://reviews.apache.org/r/23462/#comment84133>
Ditto.
src/slave/containerizer/composing.cpp
<https://reviews.apache.org/r/23462/#comment84143>
Kill white space.
- Jie Yu
On July 14, 2014, 10:30 p.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23462/
> -----------------------------------------------------------
>
> (Updated July 14, 2014, 10:30 p.m.)
>
>
> Review request for mesos, Ian Downes and Jie Yu.
>
>
> Bugs: MESOS-1527
> https://issues.apache.org/jira/browse/MESOS-1527
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Next part of MESOS-1527, see dependent review 23461 for the first phase.
>
>
> Diffs
> -----
>
> src/Makefile.am 2c7bfc57103c47523466c297f2596eaf4eb55c3c
> src/slave/containerizer/composing.hpp PRE-CREATION
> src/slave/containerizer/composing.cpp PRE-CREATION
> src/slave/containerizer/containerizer.cpp 1b71f33b430645300d171eadc9f7de96d28adf60
> src/slave/flags.hpp 1fe7b7da5aaa221ed6b94203b1189a80b14edd25
>
> Diff: https://reviews.apache.org/r/23462/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request 23462: Added composing containerizer and --containerizers
flag.
Posted by Benjamin Hindman <be...@berkeley.edu>.
> On July 16, 2014, 6:33 p.m., Ian Downes wrote:
> > src/slave/containerizer/composing.hpp, line 1
> > <https://reviews.apache.org/r/23462/diff/1/?file=629883#file629883line1>
> >
> > others are *_containerizer, e.g., mesos_containerizer, external_containerizer. What about composing_containerizer for consistency?
The mesos_containerizer.hpp|cpp got moved to /mesos directory, and we should probably do the same with external or rename it without the _containerizer suffix. We also just introduced docker.hpp|cpp rather than docker_containerizer.hpp|cpp too.
> On July 16, 2014, 6:33 p.m., Ian Downes wrote:
> > src/slave/containerizer/composing.cpp, line 260
> > <https://reviews.apache.org/r/23462/diff/1/?file=629884#file629884line260>
> >
> > What happens if --containerizers is changed? i.e., you have running containers that were started with a containerizer that now isn't present? You're only recovering containers for containerizers that you're told to IIUC...?
That's a good point. I've added MESOS-1663 and put a comment in Containerizer::create. This existed even before these changes though, for just ExternalContainerizer versus MesosContainerizer too.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23462/#review47917
-----------------------------------------------------------
On July 14, 2014, 10:30 p.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23462/
> -----------------------------------------------------------
>
> (Updated July 14, 2014, 10:30 p.m.)
>
>
> Review request for mesos, Ian Downes and Jie Yu.
>
>
> Bugs: MESOS-1527
> https://issues.apache.org/jira/browse/MESOS-1527
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Next part of MESOS-1527, see dependent review 23461 for the first phase.
>
>
> Diffs
> -----
>
> src/Makefile.am 2c7bfc57103c47523466c297f2596eaf4eb55c3c
> src/slave/containerizer/composing.hpp PRE-CREATION
> src/slave/containerizer/composing.cpp PRE-CREATION
> src/slave/containerizer/containerizer.cpp 1b71f33b430645300d171eadc9f7de96d28adf60
> src/slave/flags.hpp 1fe7b7da5aaa221ed6b94203b1189a80b14edd25
>
> Diff: https://reviews.apache.org/r/23462/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request 23462: Added composing containerizer and --containerizers
flag.
Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23462/#review47917
-----------------------------------------------------------
Ship it!
src/slave/containerizer/composing.hpp
<https://reviews.apache.org/r/23462/#comment84163>
others are *_containerizer, e.g., mesos_containerizer, external_containerizer. What about composing_containerizer for consistency?
src/slave/containerizer/composing.cpp
<https://reviews.apache.org/r/23462/#comment84158>
What happens if --containerizers is changed? i.e., you have running containers that were started with a containerizer that now isn't present? You're only recovering containers for containerizers that you're told to IIUC...?
src/slave/containerizer/containerizer.cpp
<https://reviews.apache.org/r/23462/#comment84161>
order is incorrect, com < con
src/slave/flags.hpp
<https://reviews.apache.org/r/23462/#comment84154>
Do you want to add something here about the order in which they are tried?
- Ian Downes
On July 14, 2014, 3:30 p.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23462/
> -----------------------------------------------------------
>
> (Updated July 14, 2014, 3:30 p.m.)
>
>
> Review request for mesos, Ian Downes and Jie Yu.
>
>
> Bugs: MESOS-1527
> https://issues.apache.org/jira/browse/MESOS-1527
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Next part of MESOS-1527, see dependent review 23461 for the first phase.
>
>
> Diffs
> -----
>
> src/Makefile.am 2c7bfc57103c47523466c297f2596eaf4eb55c3c
> src/slave/containerizer/composing.hpp PRE-CREATION
> src/slave/containerizer/composing.cpp PRE-CREATION
> src/slave/containerizer/containerizer.cpp 1b71f33b430645300d171eadc9f7de96d28adf60
> src/slave/flags.hpp 1fe7b7da5aaa221ed6b94203b1189a80b14edd25
>
> Diff: https://reviews.apache.org/r/23462/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>