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 2013/12/10 03:21:05 UTC
Re: Review Request 16147: Containerizer
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16147/
-----------------------------------------------------------
(Updated Dec. 10, 2013, 2:21 a.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, Chi Zhang, Niklas Nielsen, samya, and Jason Dusek.
Repository: mesos-git
Description
-------
The proposed Containerizer interface is to replace the existing Isolator.
One ContainerizerProcess has been written:
MesosContainerizerProcess - implements containerizeration internally using a Launcher and one or more Isolators (following review)
The intent is to also support a generic ExternalContainerizerProcess that can delegate containerizeration by making external calls. Other Containerizers could interface with specific external containerization techniques such as Docker or LXC.
Diffs
-----
src/slave/container/containerizer.hpp PRE-CREATION
src/slave/container/containerizer.cpp PRE-CREATION
src/slave/container/mesos_containerizer.hpp PRE-CREATION
src/slave/container/mesos_containerizer.cpp PRE-CREATION
src/slave/slave.hpp 2d093a3
src/slave/slave.cpp 91afe03
src/tests/test_containerizer.hpp PRE-CREATION
Diff: https://reviews.apache.org/r/16147/diff/
Testing
-------
Thanks,
Ian Downes
Re: Review Request 16147: Containerizer
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16147/#review30225
-----------------------------------------------------------
Partial review. One of the main things that has come up is the lack of documentation which makes it a bit tricky to figure out how these new interfaces work.
src/slave/container/containerizer.hpp
<https://reviews.apache.org/r/16147/#comment57835>
Is this exposed in the header file for testing? Why is a Containerizer injected with a ContainerizerProcess? What is a Mesos Containerizer and what might other implementations be?
Reading the code I find answers to these questions but these are good things to document for others.
src/slave/container/containerizer.hpp
<https://reviews.apache.org/r/16147/#comment57848>
Why is this destructor pure virtual?
src/slave/container/containerizer.hpp
<https://reviews.apache.org/r/16147/#comment57849>
start / terminate here feel more like create / destroy, since we're dealing with containers?
src/slave/container/containerizer.hpp
<https://reviews.apache.org/r/16147/#comment57834>
Please add documentation.
src/slave/container/containerizer.hpp
<https://reviews.apache.org/r/16147/#comment57853>
What will create be used for? (i.e. documentation is needed).
src/slave/container/containerizer.hpp
<https://reviews.apache.org/r/16147/#comment57836>
Why is this the responsibility of the Containerizer?
src/slave/container/containerizer.cpp
<https://reviews.apache.org/r/16147/#comment57851>
End comments with periods please. :)
src/slave/container/containerizer.cpp
<https://reviews.apache.org/r/16147/#comment57850>
Should these be TODOs?
src/slave/container/containerizer.cpp
<https://reviews.apache.org/r/16147/#comment57852>
TODO formatting
src/slave/container/mesos_containerizer.hpp
<https://reviews.apache.org/r/16147/#comment57854>
What is this?
src/slave/container/mesos_containerizer.cpp
<https://reviews.apache.org/r/16147/#comment57872>
Declare this above MesosContainerizerProcess::usage which is the only place it is needed.
src/slave/container/mesos_containerizer.cpp
<https://reviews.apache.org/r/16147/#comment57868>
Why not just declare this on one line up here:
Future<Nothing> _nothing() { return Nothing(); }
src/slave/container/mesos_containerizer.cpp
<https://reviews.apache.org/r/16147/#comment57869>
Ditto:
Future<Nothing> _failure(const string& message) { return Failure(message); }
src/slave/container/mesos_containerizer.cpp
<https://reviews.apache.org/r/16147/#comment57837>
?
src/slave/container/mesos_containerizer.cpp
<https://reviews.apache.org/r/16147/#comment57856>
?
src/slave/container/mesos_containerizer.cpp
<https://reviews.apache.org/r/16147/#comment57861>
"could not be"?
src/slave/container/mesos_containerizer.cpp
<https://reviews.apache.org/r/16147/#comment57862>
"could not be"?
src/slave/container/mesos_containerizer.cpp
<https://reviews.apache.org/r/16147/#comment57863>
self() and Self:: don't work here?
e.g.
.then(defer(self(), &Self::_wait, containerId))
src/slave/container/mesos_containerizer.cpp
<https://reviews.apache.org/r/16147/#comment57864>
The onFailed line is effectively a no-op. It's returned failed future is dropped when the callback is executed, see Future<T>::onFailed and Future<T>::fail for the mechanics of this.
src/slave/container/mesos_containerizer.cpp
<https://reviews.apache.org/r/16147/#comment57860>
Kill these for consistency with the rest of our code.
src/slave/container/mesos_containerizer.cpp
<https://reviews.apache.org/r/16147/#comment57865>
This is supposed to be the final recover call to the isolator but it is assuming all of the asynchronous calls from _recoverIsolators have completed at this point, which is incorrect.
We don't typically rely on sentinel values like this, would it be simpler to have a single call to recover? This way, the launcher and isolators can clean up as part of that single call, rather than having to rely on a sentinel value.
src/slave/container/mesos_containerizer.cpp
<https://reviews.apache.org/r/16147/#comment57866>
Ditto for self() and Self::, and everywhere else in this file.
src/slave/container/mesos_containerizer.cpp
<https://reviews.apache.org/r/16147/#comment57867>
It looks like there are two reasons _recover exists:
1. Convert from list<Nothing> to Nothing.
2. Wrap the underlying future failure message with "Containerizer recovery failed", I assume you wanted to append the failure message..?
I think the containerizer recovery failed part can be prepended by the caller (similar to how our os:: functions do not mention themselves in the returned error message. In this case, you could just re-use your _nothing function, right?
src/slave/container/mesos_containerizer.cpp
<https://reviews.apache.org/r/16147/#comment57873>
Try to declare continuations in the order they are used. Ideally after reading this chain I can continue downwards in this file and read _prepare, _fork, _isolate, _exec, and _startFailed in that order, if possible.
src/slave/container/mesos_containerizer.cpp
<https://reviews.apache.org/r/16147/#comment57875>
This is a no-op. Did you want a .then?
src/slave/container/mesos_containerizer.cpp
<https://reviews.apache.org/r/16147/#comment57874>
This return is dropped, please refer to my other comment about onFailed.
src/slave/container/mesos_containerizer.cpp
<https://reviews.apache.org/r/16147/#comment57878>
Does this need to be a method? It seems like the chain in start can just call launcher->fork and .then launcher->wait?
src/slave/container/mesos_containerizer.cpp
<https://reviews.apache.org/r/16147/#comment57857>
This checkpointPath function seems more complicated than just passing in the slave id as we currently do in Isolator, what was the motivation?
src/slave/container/mesos_containerizer.cpp
<https://reviews.apache.org/r/16147/#comment57870>
You could also store the result of lambda::bind as _nothing so that you don't have to keep binding it.
src/slave/container/mesos_containerizer.cpp
<https://reviews.apache.org/r/16147/#comment57871>
If we can't update a resource we terminate the container? Please add a comment as to why.
src/slave/container/mesos_containerizer.cpp
<https://reviews.apache.org/r/16147/#comment57858>
process::collect is all or nothing in terms of retrieving results, in that a single failure will result in an overall failure. Just want to make you aware of that in case you want the semantics of process::await.
src/slave/container/mesos_containerizer.cpp
<https://reviews.apache.org/r/16147/#comment57859>
This is impossible since you are using .then. The argument to _usage from lambda::_1 will be list<RS>, not Future<list<RS> >, I suspect this is compiling due to the fact that the Future constructor is implicit.
src/slave/slave.hpp
<https://reviews.apache.org/r/16147/#comment57838>
Is this a legitimate TODO? If so, can you make it more clear?
src/slave/slave.hpp
<https://reviews.apache.org/r/16147/#comment57839>
I'm not sure the idea of a "terminated container" makes sense, this function still looks to be dealing with executor termination so perhaps the old name is more appropriate.
src/slave/slave.hpp
<https://reviews.apache.org/r/16147/#comment57840>
Just a general note about reviews that may not be applicable here but I'd like to mention to reinforce it:
Fixing typos and doing cleanup are always nice :) but doing many of them in a large change increases the difficulty of the review, so it's good to try to split these out into separate reviews in the future. Also, the typo fixing review would have the benefit of being reviewed and committed immediately.
This is why we (benh) wrote post-reviews ;)
src/slave/slave.hpp
<https://reviews.apache.org/r/16147/#comment57841>
Our format for TODOs is to use a ':' after the name and treat it as a sentence, so s/this/This/
src/slave/slave.cpp
<https://reviews.apache.org/r/16147/#comment57842>
What if the flags cannot be parsed? Seems Containerizer::resources should be returning a Try so that the slave can EXIT(1) when the flags cannot be parsed.
src/slave/slave.cpp
<https://reviews.apache.org/r/16147/#comment57843>
Why did you leave this here?
src/slave/slave.cpp
<https://reviews.apache.org/r/16147/#comment57844>
Can you clean this up? Were these intended for the reviewers to address?
src/slave/slave.cpp
<https://reviews.apache.org/r/16147/#comment57845>
Kill the std:: prefixes since this is inside a cpp file.
src/slave/slave.cpp
<https://reviews.apache.org/r/16147/#comment57846>
string& ?
src/slave/slave.cpp
<https://reviews.apache.org/r/16147/#comment57847>
Why are we monitoring before we've created the container? We should only start monitoring once the container is fully created (which we can do once the container->start future is satisfied, right?)
Is this diff incomplete? I don't see a .start() method on the ResourceMonitor.
src/tests/test_containerizer.hpp
<https://reviews.apache.org/r/16147/#comment57855>
Is this supposed to be start()?
- Ben Mahler
On Dec. 11, 2013, 4:05 a.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16147/
> -----------------------------------------------------------
>
> (Updated Dec. 11, 2013, 4:05 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, Chi Zhang, Niklas Nielsen, samya, and Jason Dusek.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> The proposed Containerizer interface is to replace the existing Isolator.
>
> One ContainerizerProcess has been written:
> MesosContainerizerProcess - implements containerizeration internally using a Launcher and one or more Isolators (following review)
>
> The intent is to also support a generic ExternalContainerizerProcess that can delegate containerizeration by making external calls. Other Containerizers could interface with specific external containerization techniques such as Docker or LXC.
>
>
> Diffs
> -----
>
> src/slave/container/containerizer.hpp PRE-CREATION
> src/slave/container/containerizer.cpp PRE-CREATION
> src/slave/container/mesos_containerizer.hpp PRE-CREATION
> src/slave/container/mesos_containerizer.cpp PRE-CREATION
> src/slave/slave.hpp 71fa4f0
> src/slave/slave.cpp 75d9e5d
> src/tests/test_containerizer.hpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/16147/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 16147: Containerizer
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16147/#review30208
-----------------------------------------------------------
src/slave/slave.hpp
<https://reviews.apache.org/r/16147/#comment57822>
I think it's okay to not use Owned here just like we're not using it for MasterDetector, Files, etc.
- Benjamin Hindman
On Dec. 11, 2013, 4:05 a.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16147/
> -----------------------------------------------------------
>
> (Updated Dec. 11, 2013, 4:05 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, Chi Zhang, Niklas Nielsen, samya, and Jason Dusek.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> The proposed Containerizer interface is to replace the existing Isolator.
>
> One ContainerizerProcess has been written:
> MesosContainerizerProcess - implements containerizeration internally using a Launcher and one or more Isolators (following review)
>
> The intent is to also support a generic ExternalContainerizerProcess that can delegate containerizeration by making external calls. Other Containerizers could interface with specific external containerization techniques such as Docker or LXC.
>
>
> Diffs
> -----
>
> src/slave/container/containerizer.hpp PRE-CREATION
> src/slave/container/containerizer.cpp PRE-CREATION
> src/slave/container/mesos_containerizer.hpp PRE-CREATION
> src/slave/container/mesos_containerizer.cpp PRE-CREATION
> src/slave/slave.hpp 71fa4f0
> src/slave/slave.cpp 75d9e5d
> src/tests/test_containerizer.hpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/16147/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 16147: Containerizer
Posted by Ian Downes <ia...@gmail.com>.
> On Jan. 13, 2014, 6:52 a.m., Benjamin Hindman wrote:
> > src/slave/container/containerizer.cpp, line 19
> > <https://reviews.apache.org/r/16147/diff/4/?file=401861#file401861line19>
> >
> > Not used?
Used for ContainerizerProcess::create implementation which will appear in next update.
> On Jan. 13, 2014, 6:52 a.m., Benjamin Hindman wrote:
> > src/slave/monitor.hpp, line 135
> > <https://reviews.apache.org/r/16147/diff/4/?file=401868#file401868line135>
> >
> > Why hashmap to std::map?
Relic from when I hadn't defined hash_value(ContainerID) I think.
> On Jan. 13, 2014, 6:52 a.m., Benjamin Hindman wrote:
> > src/slave/monitor.cpp, line 117
> > <https://reviews.apache.org/r/16147/diff/4/?file=401869#file401869line117>
> >
> > We should then be using 'path::join' here too.
I'll defer these sort of refactors to [~bmahler]'s monitor changes.
> On Jan. 13, 2014, 6:52 a.m., Benjamin Hindman wrote:
> > src/slave/monitor.cpp, line 118
> > <https://reviews.apache.org/r/16147/diff/4/?file=401869#file401869line118>
> >
> > And then 'path::join' here too, and everywhere else. Given this, it would be much cleaner to have a helper which did everything for us:
> >
> > // Helper to create a namespaced statistic name given an ExecutorInfo
> > // and the statistic. We namespace the statistics like a path.
> > string name(const ExecutorInfo& info, const std::string& statistic)
> > {
> > return path::join(
> > info.framework_id().value(),
> > info.executor_id().value,
> > statistic);
> > }
> >
> > Then, rather than pulling out a "prefix" and appending the statistic name everywhere we could do:
> >
> > name(executorInfo, CPU_USAGE)
> >
> > For example, assuming we had pulled out an 'info' representing the ExecutorInfo:
> >
> > ::statistics->set(
> > "monitor",
> > name(info, CPUS_USER_TIME_SECS),
> > statistics.cpus_user_time_secs(),
> > time);
> >
> > Is there any reason why we need to be using 'monitor' as the context as well?
I'll defer these sort of refactors to [~bmahler]'s monitor changes.
> On Jan. 13, 2014, 6:52 a.m., Benjamin Hindman wrote:
> > src/slave/slave.cpp, line 2161
> > <https://reviews.apache.org/r/16147/diff/4/?file=401874#file401874line2161>
> >
> > You're missing a ' after frameworkId. Also, do you want to print out the reason monitoring termination failed (i.e., termination.failure())? Also, where do we document that a failed or discarded termination future implies the executor has actually terminated? Do we need, or want, to force the slave to destroy the container? Let's be explicit about the semantics here. Even more generally, does executor termination == container destroyed?
I've added a comments to make it explicit that the intended semantics were the containerized context is automatically destroyed when the executor terminates.
However, I'm not 100% sure this is the preferred semantics, particularly for other containerizers? Comments?
> On Jan. 13, 2014, 6:52 a.m., Benjamin Hindman wrote:
> > src/slave/slave.cpp, line 2699
> > <https://reviews.apache.org/r/16147/diff/4/?file=401874#file401874line2699>
> >
> > Can you do:
> >
> > .then(defer(lambda::bind(&Containerizer::recover, containerizer, state)));
> >
> > Or also:
> >
> > .then(lambda::bind(&Containerizer::recover, containerizer, state));
> >
> > The defer version is preferred but not strictly necessary.
Neither of these are recognized and compile. See this review for a minimal example https://reviews.apache.org/r/13782/
- Ian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16147/#review31605
-----------------------------------------------------------
On Dec. 21, 2013, 6:50 a.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16147/
> -----------------------------------------------------------
>
> (Updated Dec. 21, 2013, 6:50 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, Chi Zhang, Niklas Nielsen, samya, and Jason Dusek.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> The proposed Containerizer interface is to replace the existing Isolator.
>
> One ContainerizerProcess has been written:
> MesosContainerizerProcess - implements containerizeration internally using a Launcher and one or more Isolators (following review)
>
> The intent is to also support a generic ExternalContainerizerProcess that can delegate containerizeration by making external calls. Other Containerizers could interface with specific external containerization techniques such as Docker or LXC.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 655f86757487ddbe551fdcf53eb793e773ecdd34
> src/Makefile.am 42dafbcd6e2d4f67d2705c5a2bea6ec6a0f4fcc1
> src/common/type_utils.hpp 3b05751a9a27055dd19e4abb79197f53e5554fe1
> src/launcher/launcher.hpp 104fe812ddd2b75b14ec0b40d192f58cdba3603a
> src/launcher/launcher.cpp d5ab66704429a95eeb8eda5188e33d8e691221af
> src/launcher/main.cpp de64609905ee63096c0173fe7e64a1eafea5d6bf
> src/local/local.cpp 83a7f913afb1441e9137c7fcec8fd81815714905
> src/slave/cgroups_isolator.hpp e86062e9abaaa263c32c55e9dbfefd700f605886
> src/slave/cgroups_isolator.cpp 80155a3f0cd684a6aad9a37f61c5204d86032735
> src/slave/container/containerizer.hpp PRE-CREATION
> src/slave/container/containerizer.cpp PRE-CREATION
> src/slave/container/mesos_containerizer.hpp PRE-CREATION
> src/slave/container/mesos_containerizer.cpp PRE-CREATION
> src/slave/http.cpp 3316fdf6d1dfb97a1695a47c754773835fbdd3fb
> src/slave/isolator.hpp fc13089664e136936ed30ae4a5a11ab55d7719dc
> src/slave/isolator.cpp c9643cf9c5657bc142482a71fb161233bffb3b9f
> src/slave/main.cpp e0cae7b205c2599e05c4db990cc9c8e9e3673c37
> src/slave/monitor.hpp 52568ad8ec566f7cf36c249c76d798d44eacb578
> src/slave/monitor.cpp a931c4f35a8793c66ee03de82f0e0a21b92f8ffa
> src/slave/paths.hpp 8f80b931a896cd7e317658147e864410558d5028
> src/slave/process_isolator.hpp 4ae093fe65775a2b9bec42071961dd58aa0c3d8b
> src/slave/process_isolator.cpp 0bc698f04f7c8eaad166dc9d646e13310129dd01
> src/slave/slave.hpp 71fa4f0d0ccde8cdc023b18c6a4d7eb7478fd0cf
> src/slave/slave.cpp 75d9e5dfce9f546fb528d9f0fcf8ba127ac40b9c
> src/slave/state.hpp f06cba8b9b197ea40e6c255e158d920b4ba602ff
> src/slave/state.cpp bf267b5ea23a7427c7cb05ab4e98af2e44d9cc43
> src/slave/status_update_manager.hpp 06ea4659cdd24cb1b82f389f404566ba14a663fb
> src/slave/status_update_manager.cpp f7a0c40f1b1a8cfab828998146b8f446a0569251
> src/tests/test_containerizer.hpp PRE-CREATION
> src/tests/test_containerizer.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/16147/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 16147: Containerizer
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16147/#review31605
-----------------------------------------------------------
src/slave/container/containerizer.cpp
<https://reviews.apache.org/r/16147/#comment60189>
Not used?
src/slave/container/containerizer.cpp
<https://reviews.apache.org/r/16147/#comment60188>
None of these headers appear to be used.
src/slave/container/containerizer.cpp
<https://reviews.apache.org/r/16147/#comment60190>
Not used?
src/slave/container/containerizer.cpp
<https://reviews.apache.org/r/16147/#comment60184>
A comment in the header that the Containerizer will spawn the process (and terminate and wait for it in the destructor) would be nice.
src/slave/container/containerizer.cpp
<https://reviews.apache.org/r/16147/#comment60185>
Complete sentences please.
src/slave/container/containerizer.cpp
<https://reviews.apache.org/r/16147/#comment60186>
Complete sentences please.
src/slave/container/containerizer.cpp
<https://reviews.apache.org/r/16147/#comment60187>
Formatting is off here.
src/slave/container/containerizer.cpp
<https://reviews.apache.org/r/16147/#comment60191>
In the future, use Error("unimplemented") instead. ;)
src/slave/container/mesos_containerizer.hpp
<https://reviews.apache.org/r/16147/#comment60193>
Use const please for arguments.
src/slave/container/mesos_containerizer.cpp
<https://reviews.apache.org/r/16147/#comment60194>
Let's not use the '_' prefix unless it's actually a continuation.
src/slave/container/mesos_containerizer.cpp
<https://reviews.apache.org/r/16147/#comment60195>
Let's call this 'prepare', and s/_fork/fork/ and others too. Let's reserve the '_' prefix for continuations of the same function rather than chained functions.
src/slave/container/mesos_containerizer.cpp
<https://reviews.apache.org/r/16147/#comment60196>
Oooh, I like the use of CommandInfo! Can we s/scripts/commands/ everywhere? I like the semantics of an isolator might provide extra commands that need to get run!
src/slave/container/mesos_containerizer.cpp
<https://reviews.apache.org/r/16147/#comment60197>
s/lamba/lambda/
src/slave/container/mesos_containerizer.cpp
<https://reviews.apache.org/r/16147/#comment60198>
Maybe include the container ID that was unknown?
src/slave/container/mesos_containerizer.cpp
<https://reviews.apache.org/r/16147/#comment60199>
Maybe include the container ID that was unknown?
src/slave/container/mesos_containerizer.cpp
<https://reviews.apache.org/r/16147/#comment60201>
Is there something else we can print to know why or what we are skipping?
src/slave/container/mesos_containerizer.cpp
<https://reviews.apache.org/r/16147/#comment60203>
Is there any reason to not include the launcher's termination message too? Maybe you should just append all messages together? If nothing more some comments about why you made the decisions you made would be great.
src/slave/container/mesos_containerizer.cpp
<https://reviews.apache.org/r/16147/#comment60202>
s/has been limited in/has reached it's limit for/
src/slave/main.cpp
<https://reviews.apache.org/r/16147/#comment60181>
Wrap please.
src/slave/main.cpp
<https://reviews.apache.org/r/16147/#comment60182>
Wrap please.
src/slave/monitor.hpp
<https://reviews.apache.org/r/16147/#comment60168>
Why hashmap to std::map?
src/slave/monitor.cpp
<https://reviews.apache.org/r/16147/#comment60165>
Not your bug, but we should be using 'path::join' instead please.
src/slave/monitor.cpp
<https://reviews.apache.org/r/16147/#comment60169>
If you use a hashmap as the original code did you could use 'contains'.
src/slave/monitor.cpp
<https://reviews.apache.org/r/16147/#comment60166>
We should then be using 'path::join' here too.
src/slave/monitor.cpp
<https://reviews.apache.org/r/16147/#comment60167>
And then 'path::join' here too, and everywhere else. Given this, it would be much cleaner to have a helper which did everything for us:
// Helper to create a namespaced statistic name given an ExecutorInfo
// and the statistic. We namespace the statistics like a path.
string name(const ExecutorInfo& info, const std::string& statistic)
{
return path::join(
info.framework_id().value(),
info.executor_id().value,
statistic);
}
Then, rather than pulling out a "prefix" and appending the statistic name everywhere we could do:
name(executorInfo, CPU_USAGE)
For example, assuming we had pulled out an 'info' representing the ExecutorInfo:
::statistics->set(
"monitor",
name(info, CPUS_USER_TIME_SECS),
statistics.cpus_user_time_secs(),
time);
Is there any reason why we need to be using 'monitor' as the context as well?
src/slave/slave.cpp
<https://reviews.apache.org/r/16147/#comment60175>
Any reason not to do this call up above where you deleted the code and then keep the info.mutable_resources()->MergeFrom(resources.get()); in the same place?
src/slave/slave.cpp
<https://reviews.apache.org/r/16147/#comment60174>
s/MergeFrom/CopyFrom/
src/slave/slave.cpp
<https://reviews.apache.org/r/16147/#comment60176>
Please wrap.
src/slave/slave.cpp
<https://reviews.apache.org/r/16147/#comment60177>
If we aren't going to use 'future', let's not even put it in the signature (and kill the lambda::_1 below).
src/slave/slave.cpp
<https://reviews.apache.org/r/16147/#comment60178>
You're missing a ' after frameworkId. Also, do you want to print out the reason monitoring termination failed (i.e., termination.failure())? Also, where do we document that a failed or discarded termination future implies the executor has actually terminated? Do we need, or want, to force the slave to destroy the container? Let's be explicit about the semantics here. Even more generally, does executor termination == container destroyed?
src/slave/slave.cpp
<https://reviews.apache.org/r/16147/#comment60179>
Wrap please.
src/slave/slave.cpp
<https://reviews.apache.org/r/16147/#comment60180>
Can you do:
.then(defer(lambda::bind(&Containerizer::recover, containerizer, state)));
Or also:
.then(lambda::bind(&Containerizer::recover, containerizer, state));
The defer version is preferred but not strictly necessary.
src/slave/slave.cpp
<https://reviews.apache.org/r/16147/#comment60171>
Option<string> user = slave->flags.switch_user ? info.user() : None();
In fact, you can just use the right hand side where ever you use 'user'.
src/slave/slave.cpp
<https://reviews.apache.org/r/16147/#comment60170>
This is not doing what you expect, this is returning a new Option that is "some" with value info.user().
src/slave/slave.cpp
<https://reviews.apache.org/r/16147/#comment60172>
Kill the helper and just create and populate the map right here, it's less lines of code (the helper doesn't really help).
src/slave/slave.cpp
<https://reviews.apache.org/r/16147/#comment60173>
Wrap please.
- Benjamin Hindman
On Dec. 21, 2013, 6:50 a.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16147/
> -----------------------------------------------------------
>
> (Updated Dec. 21, 2013, 6:50 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, Chi Zhang, Niklas Nielsen, samya, and Jason Dusek.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> The proposed Containerizer interface is to replace the existing Isolator.
>
> One ContainerizerProcess has been written:
> MesosContainerizerProcess - implements containerizeration internally using a Launcher and one or more Isolators (following review)
>
> The intent is to also support a generic ExternalContainerizerProcess that can delegate containerizeration by making external calls. Other Containerizers could interface with specific external containerization techniques such as Docker or LXC.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 655f86757487ddbe551fdcf53eb793e773ecdd34
> src/Makefile.am 42dafbcd6e2d4f67d2705c5a2bea6ec6a0f4fcc1
> src/common/type_utils.hpp 3b05751a9a27055dd19e4abb79197f53e5554fe1
> src/launcher/launcher.hpp 104fe812ddd2b75b14ec0b40d192f58cdba3603a
> src/launcher/launcher.cpp d5ab66704429a95eeb8eda5188e33d8e691221af
> src/launcher/main.cpp de64609905ee63096c0173fe7e64a1eafea5d6bf
> src/local/local.cpp 83a7f913afb1441e9137c7fcec8fd81815714905
> src/slave/cgroups_isolator.hpp e86062e9abaaa263c32c55e9dbfefd700f605886
> src/slave/cgroups_isolator.cpp 80155a3f0cd684a6aad9a37f61c5204d86032735
> src/slave/container/containerizer.hpp PRE-CREATION
> src/slave/container/containerizer.cpp PRE-CREATION
> src/slave/container/mesos_containerizer.hpp PRE-CREATION
> src/slave/container/mesos_containerizer.cpp PRE-CREATION
> src/slave/http.cpp 3316fdf6d1dfb97a1695a47c754773835fbdd3fb
> src/slave/isolator.hpp fc13089664e136936ed30ae4a5a11ab55d7719dc
> src/slave/isolator.cpp c9643cf9c5657bc142482a71fb161233bffb3b9f
> src/slave/main.cpp e0cae7b205c2599e05c4db990cc9c8e9e3673c37
> src/slave/monitor.hpp 52568ad8ec566f7cf36c249c76d798d44eacb578
> src/slave/monitor.cpp a931c4f35a8793c66ee03de82f0e0a21b92f8ffa
> src/slave/paths.hpp 8f80b931a896cd7e317658147e864410558d5028
> src/slave/process_isolator.hpp 4ae093fe65775a2b9bec42071961dd58aa0c3d8b
> src/slave/process_isolator.cpp 0bc698f04f7c8eaad166dc9d646e13310129dd01
> src/slave/slave.hpp 71fa4f0d0ccde8cdc023b18c6a4d7eb7478fd0cf
> src/slave/slave.cpp 75d9e5dfce9f546fb528d9f0fcf8ba127ac40b9c
> src/slave/state.hpp f06cba8b9b197ea40e6c255e158d920b4ba602ff
> src/slave/state.cpp bf267b5ea23a7427c7cb05ab4e98af2e44d9cc43
> src/slave/status_update_manager.hpp 06ea4659cdd24cb1b82f389f404566ba14a663fb
> src/slave/status_update_manager.cpp f7a0c40f1b1a8cfab828998146b8f446a0569251
> src/tests/test_containerizer.hpp PRE-CREATION
> src/tests/test_containerizer.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/16147/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 16147: Containerizer (part 1)
Posted by Ian Downes <ia...@gmail.com>.
> On Jan. 14, 2014, 4:29 p.m., Jason Dusek wrote:
> > src/slave/container/containerizer.hpp, line 84
> > <https://reviews.apache.org/r/16147/diff/4/?file=401860#file401860line84>
> >
> > If Containerizer::launch() had the resources, it could simplify working with many external containerizers, which usually allow one to specify all the resources to be used (memory, filesystems, CPU) up front.
>
> Ian Downes wrote:
> The executor's resources are included in ExecutorInfo. Addition/removal of task resources comes in through update() as tasks are assigned to/completed by the executor.
>
> Are you asking if launch() can have the combined resources for the executor and (first) task?
>
> Are you also implying that external containerizers require up front specification because they can't modify the resource after launch?
We will add a launch(taskInfo) shortly.
- Ian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16147/#review31730
-----------------------------------------------------------
On Jan. 24, 2014, 6:25 p.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16147/
> -----------------------------------------------------------
>
> (Updated Jan. 24, 2014, 6:25 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, Chi Zhang, Niklas Nielsen, samya, and Jason Dusek.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> The proposed Containerizer interface is to replace the existing Isolator.
>
> One ContainerizerProcess has been written:
> MesosContainerizerProcess - implements containerizeration internally using a Launcher and one or more Isolators (following review)
>
> The intent is to also support a generic ExternalContainerizerProcess that can delegate containerizeration by making external calls. Other Containerizers could interface with specific external containerization techniques such as Docker or LXC.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 655f86757487ddbe551fdcf53eb793e773ecdd34
> src/Makefile.am cf0c8c66e8dd21c2f5a2da22e5d5adb056353454
> src/common/type_utils.hpp 3b05751a9a27055dd19e4abb79197f53e5554fe1
> src/launcher/launcher.hpp 104fe812ddd2b75b14ec0b40d192f58cdba3603a
> src/launcher/launcher.cpp d5ab66704429a95eeb8eda5188e33d8e691221af
> src/launcher/main.cpp de64609905ee63096c0173fe7e64a1eafea5d6bf
> src/local/local.cpp 83a7f913afb1441e9137c7fcec8fd81815714905
> src/slave/cgroups_isolator.hpp e86062e9abaaa263c32c55e9dbfefd700f605886
> src/slave/cgroups_isolator.cpp 80155a3f0cd684a6aad9a37f61c5204d86032735
> src/slave/container/containerizer.hpp PRE-CREATION
> src/slave/container/containerizer.cpp PRE-CREATION
> src/slave/container/mesos_containerizer.hpp PRE-CREATION
> src/slave/container/mesos_containerizer.cpp PRE-CREATION
> src/slave/flags.hpp 827b2d0d6dc8fa279f3187a09e5dc6d4799d17cd
> src/slave/http.cpp c8357e214d2adf2cd712072f58d07b07badb79dc
> src/slave/isolator.hpp 9634535d8c746597b4bb6e278587a1b9ca8f1608
> src/slave/isolator.cpp c9643cf9c5657bc142482a71fb161233bffb3b9f
> src/slave/main.cpp e0cae7b205c2599e05c4db990cc9c8e9e3673c37
> src/slave/monitor.hpp 52568ad8ec566f7cf36c249c76d798d44eacb578
> src/slave/monitor.cpp a931c4f35a8793c66ee03de82f0e0a21b92f8ffa
> src/slave/paths.hpp b1a56a346ca84e7e71c31386602264c5097ef1cf
> src/slave/process_isolator.hpp 4ae093fe65775a2b9bec42071961dd58aa0c3d8b
> src/slave/process_isolator.cpp 0bc698f04f7c8eaad166dc9d646e13310129dd01
> src/slave/slave.hpp 38ba7b6d6c35f0668d2f021a1285363cbf4367f4
> src/slave/slave.cpp 8b83dacad802a5434bf76ca075b2b8113ed14a84
> src/slave/state.hpp 78b20ffae1f2f629d851784302648e3bf5349321
> src/slave/state.cpp 93cde0c2efbbe5387448631a02e64b20a3d021c1
> src/slave/status_update_manager.hpp 06ea4659cdd24cb1b82f389f404566ba14a663fb
> src/slave/status_update_manager.cpp 03f5eafefd6ed748bfd4511d654c23c7b460db66
>
> Diff: https://reviews.apache.org/r/16147/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 16147: Containerizer (part 1)
Posted by Ian Downes <ia...@gmail.com>.
> On Jan. 14, 2014, 4:29 p.m., Jason Dusek wrote:
> > src/slave/container/containerizer.hpp, line 84
> > <https://reviews.apache.org/r/16147/diff/4/?file=401860#file401860line84>
> >
> > If Containerizer::launch() had the resources, it could simplify working with many external containerizers, which usually allow one to specify all the resources to be used (memory, filesystems, CPU) up front.
The executor's resources are included in ExecutorInfo. Addition/removal of task resources comes in through update() as tasks are assigned to/completed by the executor.
Are you asking if launch() can have the combined resources for the executor and (first) task?
Are you also implying that external containerizers require up front specification because they can't modify the resource after launch?
- Ian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16147/#review31730
-----------------------------------------------------------
On Jan. 21, 2014, 7:36 p.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16147/
> -----------------------------------------------------------
>
> (Updated Jan. 21, 2014, 7:36 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, Chi Zhang, Niklas Nielsen, samya, and Jason Dusek.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> The proposed Containerizer interface is to replace the existing Isolator.
>
> One ContainerizerProcess has been written:
> MesosContainerizerProcess - implements containerizeration internally using a Launcher and one or more Isolators (following review)
>
> The intent is to also support a generic ExternalContainerizerProcess that can delegate containerizeration by making external calls. Other Containerizers could interface with specific external containerization techniques such as Docker or LXC.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 655f86757487ddbe551fdcf53eb793e773ecdd34
> src/Makefile.am cf0c8c66e8dd21c2f5a2da22e5d5adb056353454
> src/common/type_utils.hpp 3b05751a9a27055dd19e4abb79197f53e5554fe1
> src/launcher/launcher.hpp 104fe812ddd2b75b14ec0b40d192f58cdba3603a
> src/launcher/launcher.cpp d5ab66704429a95eeb8eda5188e33d8e691221af
> src/launcher/main.cpp de64609905ee63096c0173fe7e64a1eafea5d6bf
> src/local/local.cpp 83a7f913afb1441e9137c7fcec8fd81815714905
> src/slave/cgroups_isolator.hpp e86062e9abaaa263c32c55e9dbfefd700f605886
> src/slave/cgroups_isolator.cpp 80155a3f0cd684a6aad9a37f61c5204d86032735
> src/slave/container/containerizer.hpp PRE-CREATION
> src/slave/container/containerizer.cpp PRE-CREATION
> src/slave/container/external_containerizer.hpp PRE-CREATION
> src/slave/container/mesos_containerizer.hpp PRE-CREATION
> src/slave/container/mesos_containerizer.cpp PRE-CREATION
> src/slave/flags.hpp 827b2d0d6dc8fa279f3187a09e5dc6d4799d17cd
> src/slave/http.cpp c8357e214d2adf2cd712072f58d07b07badb79dc
> src/slave/isolator.hpp 9634535d8c746597b4bb6e278587a1b9ca8f1608
> src/slave/isolator.cpp c9643cf9c5657bc142482a71fb161233bffb3b9f
> src/slave/main.cpp e0cae7b205c2599e05c4db990cc9c8e9e3673c37
> src/slave/monitor.hpp 52568ad8ec566f7cf36c249c76d798d44eacb578
> src/slave/monitor.cpp a931c4f35a8793c66ee03de82f0e0a21b92f8ffa
> src/slave/paths.hpp b1a56a346ca84e7e71c31386602264c5097ef1cf
> src/slave/process_isolator.hpp 4ae093fe65775a2b9bec42071961dd58aa0c3d8b
> src/slave/process_isolator.cpp 0bc698f04f7c8eaad166dc9d646e13310129dd01
> src/slave/slave.hpp 38ba7b6d6c35f0668d2f021a1285363cbf4367f4
> src/slave/slave.cpp 8b83dacad802a5434bf76ca075b2b8113ed14a84
> src/slave/state.hpp 78b20ffae1f2f629d851784302648e3bf5349321
> src/slave/state.cpp 93cde0c2efbbe5387448631a02e64b20a3d021c1
> src/slave/status_update_manager.hpp 06ea4659cdd24cb1b82f389f404566ba14a663fb
> src/slave/status_update_manager.cpp 03f5eafefd6ed748bfd4511d654c23c7b460db66
>
> Diff: https://reviews.apache.org/r/16147/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 16147: Containerizer
Posted by Jason Dusek <ja...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16147/#review31730
-----------------------------------------------------------
src/slave/container/containerizer.hpp
<https://reviews.apache.org/r/16147/#comment60463>
If Containerizer::launch() had the resources, it could simplify working with many external containerizers, which usually allow one to specify all the resources to be used (memory, filesystems, CPU) up front.
- Jason Dusek
On Dec. 21, 2013, 6:50 a.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16147/
> -----------------------------------------------------------
>
> (Updated Dec. 21, 2013, 6:50 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, Chi Zhang, Niklas Nielsen, samya, and Jason Dusek.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> The proposed Containerizer interface is to replace the existing Isolator.
>
> One ContainerizerProcess has been written:
> MesosContainerizerProcess - implements containerizeration internally using a Launcher and one or more Isolators (following review)
>
> The intent is to also support a generic ExternalContainerizerProcess that can delegate containerizeration by making external calls. Other Containerizers could interface with specific external containerization techniques such as Docker or LXC.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 655f86757487ddbe551fdcf53eb793e773ecdd34
> src/Makefile.am 42dafbcd6e2d4f67d2705c5a2bea6ec6a0f4fcc1
> src/common/type_utils.hpp 3b05751a9a27055dd19e4abb79197f53e5554fe1
> src/launcher/launcher.hpp 104fe812ddd2b75b14ec0b40d192f58cdba3603a
> src/launcher/launcher.cpp d5ab66704429a95eeb8eda5188e33d8e691221af
> src/launcher/main.cpp de64609905ee63096c0173fe7e64a1eafea5d6bf
> src/local/local.cpp 83a7f913afb1441e9137c7fcec8fd81815714905
> src/slave/cgroups_isolator.hpp e86062e9abaaa263c32c55e9dbfefd700f605886
> src/slave/cgroups_isolator.cpp 80155a3f0cd684a6aad9a37f61c5204d86032735
> src/slave/container/containerizer.hpp PRE-CREATION
> src/slave/container/containerizer.cpp PRE-CREATION
> src/slave/container/mesos_containerizer.hpp PRE-CREATION
> src/slave/container/mesos_containerizer.cpp PRE-CREATION
> src/slave/http.cpp 3316fdf6d1dfb97a1695a47c754773835fbdd3fb
> src/slave/isolator.hpp fc13089664e136936ed30ae4a5a11ab55d7719dc
> src/slave/isolator.cpp c9643cf9c5657bc142482a71fb161233bffb3b9f
> src/slave/main.cpp e0cae7b205c2599e05c4db990cc9c8e9e3673c37
> src/slave/monitor.hpp 52568ad8ec566f7cf36c249c76d798d44eacb578
> src/slave/monitor.cpp a931c4f35a8793c66ee03de82f0e0a21b92f8ffa
> src/slave/paths.hpp 8f80b931a896cd7e317658147e864410558d5028
> src/slave/process_isolator.hpp 4ae093fe65775a2b9bec42071961dd58aa0c3d8b
> src/slave/process_isolator.cpp 0bc698f04f7c8eaad166dc9d646e13310129dd01
> src/slave/slave.hpp 71fa4f0d0ccde8cdc023b18c6a4d7eb7478fd0cf
> src/slave/slave.cpp 75d9e5dfce9f546fb528d9f0fcf8ba127ac40b9c
> src/slave/state.hpp f06cba8b9b197ea40e6c255e158d920b4ba602ff
> src/slave/state.cpp bf267b5ea23a7427c7cb05ab4e98af2e44d9cc43
> src/slave/status_update_manager.hpp 06ea4659cdd24cb1b82f389f404566ba14a663fb
> src/slave/status_update_manager.cpp f7a0c40f1b1a8cfab828998146b8f446a0569251
> src/tests/test_containerizer.hpp PRE-CREATION
> src/tests/test_containerizer.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/16147/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 16147: Containerizer (part 1)
Posted by Ian Downes <ia...@gmail.com>.
> On Jan. 21, 2014, 11:39 p.m., Benjamin Hindman wrote:
> > src/slave/container/containerizer.hpp, line 153
> > <https://reviews.apache.org/r/16147/diff/6/?file=425462#file425462line153>
> >
> > Doing a fetch should not block. We should probably have some abstractions around doing the operations required for a fetch asynchronously. Worst case, we should do the fetching out of process and have an abstraction for doing that asynchronously. Doing fetch out of process might make sense anyway because then one could integrate with their own caching or storage layers more easily.
Moved fetch/extract out to an external fetcher which can be executed in a Subprocess.
> On Jan. 21, 2014, 11:39 p.m., Benjamin Hindman wrote:
> > src/slave/container/mesos_containerizer.cpp, lines 528-529
> > <https://reviews.apache.org/r/16147/diff/6/?file=425466#file425466line528>
> >
> > The flow around container termination seems a bit flawed. If you're asking the launche to destroy a container AND setting the promise below that means that the slave is possibly finding out about a terminated container before it's actually completely destroyed. I think the launcher should have something like 'wait' as well that the containerizer uses to gate when it returns to the slave. In fact, you might be able to make Containerizer::wait be a simple wrapper around Launcher::wait.
Reworked flow so all destroy paths merge at MesosContainerizer::destroy() and promise is set after all processes have been killed, the isolators have cleaned up, and the executor pid has been reaped.
> On Jan. 21, 2014, 11:39 p.m., Benjamin Hindman wrote:
> > src/slave/slave.cpp, line 2041
> > <https://reviews.apache.org/r/16147/diff/6/?file=425478#file425478line2041>
> >
> > What are the ramifications of the overcommit with respect to other containers? Should we be moving this closer to the acceptance of the task (i.e., in the master?).
>
> Ian Downes wrote:
> Previously, the (initial) overcommit was done in the cgroups isolator where default values for memory and cpu were used if unspecified. I think better than adding an allocation is (a) if an executor is specified then its resources must be too, and (b) if no executor is specified then the task resources are used. Thoughts?
We now include the first task's resources when launching a new executor so this code has been pruned.
- Ian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16147/#review32409
-----------------------------------------------------------
On Jan. 31, 2014, 11:51 p.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16147/
> -----------------------------------------------------------
>
> (Updated Jan. 31, 2014, 11:51 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, Chi Zhang, Niklas Nielsen, samya, and Jason Dusek.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> The proposed Containerizer interface is to replace the existing Isolator.
>
> One ContainerizerProcess has been written:
> MesosContainerizerProcess - implements containerizeration internally using a Launcher and one or more Isolators (following review)
>
> The intent is to also support a generic ExternalContainerizerProcess that can delegate containerizeration by making external calls. Other Containerizers could interface with specific external containerization techniques such as Docker or LXC.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/Makefile.am 5d5a76021900bf27113fe5b730cfd2b6a537d132
> 3rdparty/libprocess/3rdparty/stout/include/stout/format.hpp 3eadaefad1945a2cf55ed67e5ee02eaffce76fea
> 3rdparty/libprocess/3rdparty/stout/include/stout/list.hpp 60f0c8c67317872883857a1cd8f39d6d6a45d0c1
> 3rdparty/libprocess/3rdparty/stout/include/stout/strings.hpp 08428b8f38f08674743ec0d07ec53bee8649fadd
> 3rdparty/libprocess/Makefile.am 40f01a7b3803696ccca440c8326e1d6d7c377459
> 3rdparty/libprocess/include/process/statistics.hpp a4f1db3a8a219c39193a1d237477f0350e47e681
> 3rdparty/libprocess/include/process/timeseries.hpp b368b9bb9cd63fb331b2c137dcde759d0ee59ee6
> 3rdparty/libprocess/src/process.cpp bc7a1c5890df7c763b0b140b47966e457e519208
> 3rdparty/libprocess/src/statistics.cpp 75aac4074d33cb5054da6c8b0bd4a890c2eaf80e
> 3rdparty/libprocess/src/tests/statistics_tests.cpp 3521bd565dae8fcbba464f2539b3b14a37a037f0
> 3rdparty/libprocess/src/tests/timeseries_tests.cpp fd906b869e8bbf5c07c67c5319dec82681e811b7
> CHANGELOG 905c48996c37b33253deb00c156682fd1b14eedf
> configure.ac aa6ee45bc477dd81b6f0cb389ef9f16f8216d6df
> docs/home.md df8e05a1aed4040325ae4258f05f7cbda6dcdb8e
> docs/logging-and-debugging.md 1f037d6cdbc9e76976a796961c300d8842a16601
> docs/powered-by-mesos.md 6df9490c3df28f2097927d3e707a62099fe2b460
> docs/upgrades.md dac0ef8ac62e97e6dc88b0e1a42e042a8d775f0c
> include/mesos/mesos.proto 1503e73f0d299f8efb209e0b10966c9a7f8db5f8
> src/Makefile.am d58b46e99e0a041cf2a26abe44bbd1504a9539c0
> src/common/type_utils.hpp fe6bf71d689f7bfd8b6ae1b8fab9b2e76e28e7a8
> src/launcher/fetcher.cpp PRE-CREATION
> src/launcher/launcher.hpp 104fe812ddd2b75b14ec0b40d192f58cdba3603a
> src/launcher/launcher.cpp d5ab66704429a95eeb8eda5188e33d8e691221af
> src/launcher/main.cpp de64609905ee63096c0173fe7e64a1eafea5d6bf
> src/local/local.cpp 83a7f913afb1441e9137c7fcec8fd81815714905
> src/log/recover.cpp ee38acd280fab53b8584f68ea9dfbb496820d022
> src/master/constants.hpp cdaaad060d4ee777f8b0838b63c0fd031da861ea
> src/master/constants.cpp 8a48bbbe1d452919562b21f9b73680c42271a207
> src/master/contender.hpp 0048ee026afc7034a09dd71f0a65c0fb55e8c77f
> src/master/contender.cpp e3b0737195aba42c805a05c96b054cec1471b502
> src/master/detector.hpp 277c9d907120a9ba30948d8ab4baaa2586293a62
> src/master/detector.cpp 2b169c551affe7acd2feac7806a27b46eb99bb88
> src/master/master.hpp 99b81811d596a777ee83dce7b157c1b11c064fab
> src/master/master.cpp c7d9186b9c22fb40b05aa6d5bc6f2d38fb7f73ea
> src/slave/cgroups_isolator.hpp e86062e9abaaa263c32c55e9dbfefd700f605886
> src/slave/cgroups_isolator.cpp 80155a3f0cd684a6aad9a37f61c5204d86032735
> src/slave/containerizer/containerizer.hpp PRE-CREATION
> src/slave/containerizer/containerizer.cpp PRE-CREATION
> src/slave/containerizer/mesos_containerizer.hpp PRE-CREATION
> src/slave/containerizer/mesos_containerizer.cpp PRE-CREATION
> src/slave/flags.hpp 827b2d0d6dc8fa279f3187a09e5dc6d4799d17cd
> src/slave/http.cpp c8357e214d2adf2cd712072f58d07b07badb79dc
> src/slave/isolator.hpp 9634535d8c746597b4bb6e278587a1b9ca8f1608
> src/slave/isolator.cpp c9643cf9c5657bc142482a71fb161233bffb3b9f
> src/slave/main.cpp e0cae7b205c2599e05c4db990cc9c8e9e3673c37
> src/slave/monitor.hpp b677410e20eeafa7eac9acb4e687af7c8d53809f
> src/slave/monitor.cpp bb3723e967b0723bc2b925019b6e9c4170dcc071
> src/slave/paths.hpp 70ee0f31ebb294011e5ec6b05ce3ad28b2c4bacc
> src/slave/process_isolator.hpp 4ae093fe65775a2b9bec42071961dd58aa0c3d8b
> src/slave/process_isolator.cpp 0bc698f04f7c8eaad166dc9d646e13310129dd01
> src/slave/slave.hpp b8acc1866353f4863aee5e2d3db4ac1f9d2a48a5
> src/slave/slave.cpp aeecca0b8631c4b947d505a99d705f909d5986f7
> src/slave/state.hpp 78b20ffae1f2f629d851784302648e3bf5349321
> src/slave/state.cpp 6c382cd30602ede60b7d526acc272adc16285a93
> src/slave/status_update_manager.hpp 06ea4659cdd24cb1b82f389f404566ba14a663fb
> src/slave/status_update_manager.cpp 03f5eafefd6ed748bfd4511d654c23c7b460db66
> src/tests/group_tests.cpp 5d4240c5fb32db97a8a0920d90af5400a0900dfd
> src/tests/log_tests.cpp e493af4f2f2435efe168d07acd267b61afd37fe4
> src/tests/master_contender_detector_tests.cpp 5223200ee59b9ce0e1d228320e8ad9e84a567288
> src/tests/master_tests.cpp f1486cecec2fdc1d56624de150bc8cb18df76b65
> src/tests/monitor_tests.cpp 7988c90b4b383bd822d1294a7f0318fbd9160dc6
> src/tests/slave_recovery_tests.cpp 999e59857fd5a02de24bd15c9253d7a42df816ac
> src/tests/zookeeper_tests.cpp 615338adf2a66bdf2d8c48e63783f054612bb958
> src/webui/master/static/js/controllers.js afb24fb9c2184772f7314162f5637dbabaa2ab94
> src/webui/master/static/js/services.js 8403aad9b2586ac3c1bea3f75ca38f6f907087a2
> src/webui/master/static/slave.html 09bfcf29a700168938f4ec4fa42ca8383fa55511
> src/webui/master/static/slave_executor.html 7c66405090f46f89bdd29806a58c05dc76c0ad23
> src/webui/master/static/slave_framework.html 90eff2f3b4de08f98283b27e1635ba23a855e488
> src/zookeeper/contender.hpp 65292450d8c800d3e6a7e8e4db7c8d48bc79a79d
> src/zookeeper/contender.cpp 6710da4e64fc0a43c1eabfc0f39fb0133c13df14
> src/zookeeper/detector.hpp 73235c0f2a339e27fa86f3bcb5e9a7a08e96f7f8
> src/zookeeper/detector.cpp e186e5110341dd2fb94ddf50fed1b2819f70afb4
> src/zookeeper/group.hpp e51ebb2cf5f09a633462c101f913ee8272be9a6c
> src/zookeeper/group.cpp ecb6c002e8194b8d67e262826d988f747414f9f3
> src/zookeeper/zookeeper.hpp f50aca6e7035c8084c3e76fd56b9d1ef7f9d9902
> support/tag.sh 3977f4fce0200069f2265bfd8a03a611c2fa1849
>
> Diff: https://reviews.apache.org/r/16147/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 16147: Containerizer (part 1)
Posted by Ian Downes <ia...@gmail.com>.
> On Jan. 21, 2014, 11:39 p.m., Benjamin Hindman wrote:
> > src/slave/slave.cpp, line 2041
> > <https://reviews.apache.org/r/16147/diff/6/?file=425478#file425478line2041>
> >
> > What are the ramifications of the overcommit with respect to other containers? Should we be moving this closer to the acceptance of the task (i.e., in the master?).
Previously, the (initial) overcommit was done in the cgroups isolator where default values for memory and cpu were used if unspecified. I think better than adding an allocation is (a) if an executor is specified then its resources must be too, and (b) if no executor is specified then the task resources are used. Thoughts?
> On Jan. 21, 2014, 11:39 p.m., Benjamin Hindman wrote:
> > src/slave/container/mesos_containerizer.cpp, line 255
> > <https://reviews.apache.org/r/16147/diff/6/?file=425466#file425466line255>
> >
> > Can we inline this here please?
It's used by both the mesos containerizer and the test containerizer. I'll inline in both if desired?
> On Jan. 21, 2014, 11:39 p.m., Benjamin Hindman wrote:
> > src/slave/slave.cpp, line 3013
> > <https://reviews.apache.org/r/16147/diff/6/?file=425478#file425478line3013>
> >
> > Can you not use 'Some(info.user())' here?
I don't think the ternary operator can know the two types are the same/convertible?
> On Jan. 21, 2014, 11:39 p.m., Benjamin Hindman wrote:
> > src/slave/slave.cpp, line 2246
> > <https://reviews.apache.org/r/16147/diff/6/?file=425478#file425478line2246>
> >
> > Since the API call is 'destroy' should we s/killed/destroy/?
This is meant to indicate the executor was killed (e.g. by oom) rather than the container being destroyed (through the api).
> On Jan. 21, 2014, 11:39 p.m., Benjamin Hindman wrote:
> > src/slave/state.cpp, line 340
> > <https://reviews.apache.org/r/16147/diff/6/?file=425480#file425480line340>
> >
> > Why don't you need to use UUID::fromString anymore? Also below? Will this work with older versions?
I believe this is ok - latest is a Result<string> from the dir name so an old 'latest' that was actually a UUID will be read as a string and ContainerID is just a string.
- Ian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16147/#review32409
-----------------------------------------------------------
On Jan. 21, 2014, 7:36 p.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16147/
> -----------------------------------------------------------
>
> (Updated Jan. 21, 2014, 7:36 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, Chi Zhang, Niklas Nielsen, samya, and Jason Dusek.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> The proposed Containerizer interface is to replace the existing Isolator.
>
> One ContainerizerProcess has been written:
> MesosContainerizerProcess - implements containerizeration internally using a Launcher and one or more Isolators (following review)
>
> The intent is to also support a generic ExternalContainerizerProcess that can delegate containerizeration by making external calls. Other Containerizers could interface with specific external containerization techniques such as Docker or LXC.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 655f86757487ddbe551fdcf53eb793e773ecdd34
> src/Makefile.am cf0c8c66e8dd21c2f5a2da22e5d5adb056353454
> src/common/type_utils.hpp 3b05751a9a27055dd19e4abb79197f53e5554fe1
> src/launcher/launcher.hpp 104fe812ddd2b75b14ec0b40d192f58cdba3603a
> src/launcher/launcher.cpp d5ab66704429a95eeb8eda5188e33d8e691221af
> src/launcher/main.cpp de64609905ee63096c0173fe7e64a1eafea5d6bf
> src/local/local.cpp 83a7f913afb1441e9137c7fcec8fd81815714905
> src/slave/cgroups_isolator.hpp e86062e9abaaa263c32c55e9dbfefd700f605886
> src/slave/cgroups_isolator.cpp 80155a3f0cd684a6aad9a37f61c5204d86032735
> src/slave/container/containerizer.hpp PRE-CREATION
> src/slave/container/containerizer.cpp PRE-CREATION
> src/slave/container/external_containerizer.hpp PRE-CREATION
> src/slave/container/mesos_containerizer.hpp PRE-CREATION
> src/slave/container/mesos_containerizer.cpp PRE-CREATION
> src/slave/flags.hpp 827b2d0d6dc8fa279f3187a09e5dc6d4799d17cd
> src/slave/http.cpp c8357e214d2adf2cd712072f58d07b07badb79dc
> src/slave/isolator.hpp 9634535d8c746597b4bb6e278587a1b9ca8f1608
> src/slave/isolator.cpp c9643cf9c5657bc142482a71fb161233bffb3b9f
> src/slave/main.cpp e0cae7b205c2599e05c4db990cc9c8e9e3673c37
> src/slave/monitor.hpp 52568ad8ec566f7cf36c249c76d798d44eacb578
> src/slave/monitor.cpp a931c4f35a8793c66ee03de82f0e0a21b92f8ffa
> src/slave/paths.hpp b1a56a346ca84e7e71c31386602264c5097ef1cf
> src/slave/process_isolator.hpp 4ae093fe65775a2b9bec42071961dd58aa0c3d8b
> src/slave/process_isolator.cpp 0bc698f04f7c8eaad166dc9d646e13310129dd01
> src/slave/slave.hpp 38ba7b6d6c35f0668d2f021a1285363cbf4367f4
> src/slave/slave.cpp 8b83dacad802a5434bf76ca075b2b8113ed14a84
> src/slave/state.hpp 78b20ffae1f2f629d851784302648e3bf5349321
> src/slave/state.cpp 93cde0c2efbbe5387448631a02e64b20a3d021c1
> src/slave/status_update_manager.hpp 06ea4659cdd24cb1b82f389f404566ba14a663fb
> src/slave/status_update_manager.cpp 03f5eafefd6ed748bfd4511d654c23c7b460db66
>
> Diff: https://reviews.apache.org/r/16147/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 16147: Containerizer (part 1)
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16147/#review32409
-----------------------------------------------------------
Ship it!
This is looking really good. There are some minor cleanups that need to be done and some code motion around MesosContainerizerProcess::terminated and fetching but barring those things I think we're ready to get this committed!
src/Makefile.am
<https://reviews.apache.org/r/16147/#comment61207>
Why 'container' instead of 'containerizer'? Also, any reason not to just s/mesos_containerizer/mesos/
src/local/local.cpp
<https://reviews.apache.org/r/16147/#comment61209>
Any reason not to just do Containerizer::create and avoid the extra code here? Since you subclass Containerizer with TestContainerizer as well you shouldn't need to inject TestContainerizerProcess into TestContainerizer either (that's the whole point of subclassing Containerizer with TestContainerizer!).
src/slave/container/containerizer.hpp
<https://reviews.apache.org/r/16147/#comment61232>
Why not make these const? Are these non-const for assignability? If so, please comment.
src/slave/container/containerizer.hpp
<https://reviews.apache.org/r/16147/#comment61233>
Add a comment! ;)
src/slave/container/containerizer.hpp
<https://reviews.apache.org/r/16147/#comment61283>
Doing a fetch should not block. We should probably have some abstractions around doing the operations required for a fetch asynchronously. Worst case, we should do the fetching out of process and have an abstraction for doing that asynchronously. Doing fetch out of process might make sense anyway because then one could integrate with their own caching or storage layers more easily.
src/slave/container/containerizer.cpp
<https://reviews.apache.org/r/16147/#comment61238>
Kill the else and put at top-level.
src/slave/container/containerizer.cpp
<https://reviews.apache.org/r/16147/#comment61208>
Please add a newline above this.
src/slave/container/external_containerizer.hpp
<https://reviews.apache.org/r/16147/#comment61239>
Do we want this just yet? I don't think it's clear what we want just yet here, so no need to add it just yet.
src/slave/container/mesos_containerizer.cpp
<https://reviews.apache.org/r/16147/#comment61236>
Can we inline this here please?
src/slave/container/mesos_containerizer.cpp
<https://reviews.apache.org/r/16147/#comment61237>
I guess this is in a later review? ;)
src/slave/container/mesos_containerizer.cpp
<https://reviews.apache.org/r/16147/#comment61282>
The flow around container termination seems a bit flawed. If you're asking the launche to destroy a container AND setting the promise below that means that the slave is possibly finding out about a terminated container before it's actually completely destroyed. I think the launcher should have something like 'wait' as well that the containerizer uses to gate when it returns to the slave. In fact, you might be able to make Containerizer::wait be a simple wrapper around Launcher::wait.
src/slave/slave.cpp
<https://reviews.apache.org/r/16147/#comment61212>
Is this error possibly because of bad user flags? If so, it would be nice not to print out a stack trace but instead use something like EXIT(1).
src/slave/slave.cpp
<https://reviews.apache.org/r/16147/#comment61217>
What are the ramifications of the overcommit with respect to other containers? Should we be moving this closer to the acceptance of the task (i.e., in the master?).
src/slave/slave.cpp
<https://reviews.apache.org/r/16147/#comment61219>
Can you include the frameworkId and executorId as well please?
src/slave/slave.cpp
<https://reviews.apache.org/r/16147/#comment61220>
Can we move Termination into the Containerizer class? It would be nice to read Containerizer::Termination here.
src/slave/slave.cpp
<https://reviews.apache.org/r/16147/#comment61221>
Since the API call is 'destroy' should we s/killed/destroy/?
src/slave/slave.cpp
<https://reviews.apache.org/r/16147/#comment61224>
Can you not use 'Some(info.user())' here?
src/slave/state.cpp
<https://reviews.apache.org/r/16147/#comment61226>
Comment needs an update?
src/slave/state.cpp
<https://reviews.apache.org/r/16147/#comment61227>
Why don't you need to use UUID::fromString anymore? Also below? Will this work with older versions?
src/slave/status_update_manager.cpp
<https://reviews.apache.org/r/16147/#comment61229>
Wrap please!
- Benjamin Hindman
On Jan. 21, 2014, 7:36 p.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16147/
> -----------------------------------------------------------
>
> (Updated Jan. 21, 2014, 7:36 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, Chi Zhang, Niklas Nielsen, samya, and Jason Dusek.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> The proposed Containerizer interface is to replace the existing Isolator.
>
> One ContainerizerProcess has been written:
> MesosContainerizerProcess - implements containerizeration internally using a Launcher and one or more Isolators (following review)
>
> The intent is to also support a generic ExternalContainerizerProcess that can delegate containerizeration by making external calls. Other Containerizers could interface with specific external containerization techniques such as Docker or LXC.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 655f86757487ddbe551fdcf53eb793e773ecdd34
> src/Makefile.am cf0c8c66e8dd21c2f5a2da22e5d5adb056353454
> src/common/type_utils.hpp 3b05751a9a27055dd19e4abb79197f53e5554fe1
> src/launcher/launcher.hpp 104fe812ddd2b75b14ec0b40d192f58cdba3603a
> src/launcher/launcher.cpp d5ab66704429a95eeb8eda5188e33d8e691221af
> src/launcher/main.cpp de64609905ee63096c0173fe7e64a1eafea5d6bf
> src/local/local.cpp 83a7f913afb1441e9137c7fcec8fd81815714905
> src/slave/cgroups_isolator.hpp e86062e9abaaa263c32c55e9dbfefd700f605886
> src/slave/cgroups_isolator.cpp 80155a3f0cd684a6aad9a37f61c5204d86032735
> src/slave/container/containerizer.hpp PRE-CREATION
> src/slave/container/containerizer.cpp PRE-CREATION
> src/slave/container/external_containerizer.hpp PRE-CREATION
> src/slave/container/mesos_containerizer.hpp PRE-CREATION
> src/slave/container/mesos_containerizer.cpp PRE-CREATION
> src/slave/flags.hpp 827b2d0d6dc8fa279f3187a09e5dc6d4799d17cd
> src/slave/http.cpp c8357e214d2adf2cd712072f58d07b07badb79dc
> src/slave/isolator.hpp 9634535d8c746597b4bb6e278587a1b9ca8f1608
> src/slave/isolator.cpp c9643cf9c5657bc142482a71fb161233bffb3b9f
> src/slave/main.cpp e0cae7b205c2599e05c4db990cc9c8e9e3673c37
> src/slave/monitor.hpp 52568ad8ec566f7cf36c249c76d798d44eacb578
> src/slave/monitor.cpp a931c4f35a8793c66ee03de82f0e0a21b92f8ffa
> src/slave/paths.hpp b1a56a346ca84e7e71c31386602264c5097ef1cf
> src/slave/process_isolator.hpp 4ae093fe65775a2b9bec42071961dd58aa0c3d8b
> src/slave/process_isolator.cpp 0bc698f04f7c8eaad166dc9d646e13310129dd01
> src/slave/slave.hpp 38ba7b6d6c35f0668d2f021a1285363cbf4367f4
> src/slave/slave.cpp 8b83dacad802a5434bf76ca075b2b8113ed14a84
> src/slave/state.hpp 78b20ffae1f2f629d851784302648e3bf5349321
> src/slave/state.cpp 93cde0c2efbbe5387448631a02e64b20a3d021c1
> src/slave/status_update_manager.hpp 06ea4659cdd24cb1b82f389f404566ba14a663fb
> src/slave/status_update_manager.cpp 03f5eafefd6ed748bfd4511d654c23c7b460db66
>
> Diff: https://reviews.apache.org/r/16147/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 16147: Containerizer (part 1)
Posted by Ian Downes <ia...@gmail.com>.
> On Jan. 24, 2014, 1:59 a.m., Adam B wrote:
> > src/slave/container/containerizer.cpp, line 58
> > <https://reviews.apache.org/r/16147/diff/6/?file=425463#file425463line58>
> >
> > Do you need to use "process::" since you're "using namespace process" above?
This code disappeared when I refactored the containerizer api.
> On Jan. 24, 2014, 1:59 a.m., Adam B wrote:
> > src/slave/container/containerizer.cpp, lines 139-145
> > <https://reviews.apache.org/r/16147/diff/6/?file=425463#file425463line139>
> >
> > If resources = parsed.get(), and !resources.cpus().isSome(), then how can parsed.get().cpus().isSome() be true?
> > Maybe I'm missing something here about Resources and Try<>, but it looks like unreachable code to me. Ditto for mem.
Good catch!
> On Jan. 24, 2014, 1:59 a.m., Adam B wrote:
> > src/slave/container/containerizer.cpp, lines 292-293
> > <https://reviews.apache.org/r/16147/diff/6/?file=425463#file425463line292>
> >
> > PosixLauncher can be used on Linux too, right?
Changed this so the cgroups launcher is used if any cgroups isolators are specified, otherwise posix launcher is used.
> On Jan. 24, 2014, 1:59 a.m., Adam B wrote:
> > src/slave/container/mesos_containerizer.hpp, lines 45-48
> > <https://reviews.apache.org/r/16147/diff/6/?file=425465#file425465line45>
> >
> > And if state.isNone(), it terminates/cleans-up all executors? Or recovers all?
Terminated. Added comment in containerizer.hpp .
- Ian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16147/#review32670
-----------------------------------------------------------
On Jan. 24, 2014, 6:25 p.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16147/
> -----------------------------------------------------------
>
> (Updated Jan. 24, 2014, 6:25 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, Chi Zhang, Niklas Nielsen, samya, and Jason Dusek.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> The proposed Containerizer interface is to replace the existing Isolator.
>
> One ContainerizerProcess has been written:
> MesosContainerizerProcess - implements containerizeration internally using a Launcher and one or more Isolators (following review)
>
> The intent is to also support a generic ExternalContainerizerProcess that can delegate containerizeration by making external calls. Other Containerizers could interface with specific external containerization techniques such as Docker or LXC.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 655f86757487ddbe551fdcf53eb793e773ecdd34
> src/Makefile.am cf0c8c66e8dd21c2f5a2da22e5d5adb056353454
> src/common/type_utils.hpp 3b05751a9a27055dd19e4abb79197f53e5554fe1
> src/launcher/launcher.hpp 104fe812ddd2b75b14ec0b40d192f58cdba3603a
> src/launcher/launcher.cpp d5ab66704429a95eeb8eda5188e33d8e691221af
> src/launcher/main.cpp de64609905ee63096c0173fe7e64a1eafea5d6bf
> src/local/local.cpp 83a7f913afb1441e9137c7fcec8fd81815714905
> src/slave/cgroups_isolator.hpp e86062e9abaaa263c32c55e9dbfefd700f605886
> src/slave/cgroups_isolator.cpp 80155a3f0cd684a6aad9a37f61c5204d86032735
> src/slave/container/containerizer.hpp PRE-CREATION
> src/slave/container/containerizer.cpp PRE-CREATION
> src/slave/container/mesos_containerizer.hpp PRE-CREATION
> src/slave/container/mesos_containerizer.cpp PRE-CREATION
> src/slave/flags.hpp 827b2d0d6dc8fa279f3187a09e5dc6d4799d17cd
> src/slave/http.cpp c8357e214d2adf2cd712072f58d07b07badb79dc
> src/slave/isolator.hpp 9634535d8c746597b4bb6e278587a1b9ca8f1608
> src/slave/isolator.cpp c9643cf9c5657bc142482a71fb161233bffb3b9f
> src/slave/main.cpp e0cae7b205c2599e05c4db990cc9c8e9e3673c37
> src/slave/monitor.hpp 52568ad8ec566f7cf36c249c76d798d44eacb578
> src/slave/monitor.cpp a931c4f35a8793c66ee03de82f0e0a21b92f8ffa
> src/slave/paths.hpp b1a56a346ca84e7e71c31386602264c5097ef1cf
> src/slave/process_isolator.hpp 4ae093fe65775a2b9bec42071961dd58aa0c3d8b
> src/slave/process_isolator.cpp 0bc698f04f7c8eaad166dc9d646e13310129dd01
> src/slave/slave.hpp 38ba7b6d6c35f0668d2f021a1285363cbf4367f4
> src/slave/slave.cpp 8b83dacad802a5434bf76ca075b2b8113ed14a84
> src/slave/state.hpp 78b20ffae1f2f629d851784302648e3bf5349321
> src/slave/state.cpp 93cde0c2efbbe5387448631a02e64b20a3d021c1
> src/slave/status_update_manager.hpp 06ea4659cdd24cb1b82f389f404566ba14a663fb
> src/slave/status_update_manager.cpp 03f5eafefd6ed748bfd4511d654c23c7b460db66
>
> Diff: https://reviews.apache.org/r/16147/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 16147: Containerizer (part 1)
Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16147/#review32670
-----------------------------------------------------------
Finally got around to reviewing the new containerizer files. Looks great, only some minor comments.
src/slave/container/containerizer.hpp
<https://reviews.apache.org/r/16147/#comment61582>
Comment what the map<string, string> return value represents, please? Looks like environment variables mapped to their values.
src/slave/container/containerizer.cpp
<https://reviews.apache.org/r/16147/#comment61583>
Do you need to use "process::" since you're "using namespace process" above?
src/slave/container/containerizer.cpp
<https://reviews.apache.org/r/16147/#comment61594>
Lost some TODOs in the move that might be useful:
// TODO(benh): Move this computation into Flags as the "default".
// TODO(vinod): Move some of this computation into Resources.
src/slave/container/containerizer.cpp
<https://reviews.apache.org/r/16147/#comment61585>
If resources = parsed.get(), and !resources.cpus().isSome(), then how can parsed.get().cpus().isSome() be true?
Maybe I'm missing something here about Resources and Try<>, but it looks like unreachable code to me. Ditto for mem.
src/slave/container/containerizer.cpp
<https://reviews.apache.org/r/16147/#comment61589>
Lost the old comment, including BenH's TODO:
// Leave 1 GB free if we have more than 1 GB, otherwise, use all!
// TODO(benh): Have better default scheme (e.g., % of mem not
// greater than 1 GB?)
Shouldn't we put it back in?
src/slave/container/containerizer.cpp
<https://reviews.apache.org/r/16147/#comment61597>
resources += Resources::parse(...? Like you did for cpu/mem. Same applies to 'ports' below.
I'd like to see the style consistent for each of these four resources.
src/slave/container/containerizer.cpp
<https://reviews.apache.org/r/16147/#comment61600>
Could include the default translation (e.g. process -> "posix/cpu,posix/mem") in log/warn message, so a user knows what to replace their deprecated isolation options with.
src/slave/container/containerizer.cpp
<https://reviews.apache.org/r/16147/#comment61613>
Will isolator.error() include type in its string? Seems useful.
src/slave/container/containerizer.cpp
<https://reviews.apache.org/r/16147/#comment61601>
PosixLauncher can be used on Linux too, right?
src/slave/container/containerizer.cpp
<https://reviews.apache.org/r/16147/#comment61615>
Might want to give a different error message from below, to indicate an unrecognized-file-extension error as opposed to the command failing.
src/slave/container/containerizer.cpp
<https://reviews.apache.org/r/16147/#comment61616>
s/htfp/hftp/
src/slave/container/containerizer.cpp
<https://reviews.apache.org/r/16147/#comment61617>
from <uri> to <path>?
src/slave/container/containerizer.cpp
<https://reviews.apache.org/r/16147/#comment61620>
Shouldn't path::join be "making it: <local>" and not uri (the original relative path)?
src/slave/container/containerizer.cpp
<https://reviews.apache.org/r/16147/#comment61619>
More specific error message?
src/slave/container/containerizer.cpp
<https://reviews.apache.org/r/16147/#comment61621>
s/uri/local/ ?
src/slave/container/mesos_containerizer.hpp
<https://reviews.apache.org/r/16147/#comment61623>
And if state.isNone(), it terminates/cleans-up all executors? Or recovers all?
src/slave/container/mesos_containerizer.hpp
<https://reviews.apache.org/r/16147/#comment61624>
There were talks (with BenH) of moving the Reaper into the launcher, but that can be a later review/change
- Adam B
On Jan. 21, 2014, 11:36 a.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16147/
> -----------------------------------------------------------
>
> (Updated Jan. 21, 2014, 11:36 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, Chi Zhang, Niklas Nielsen, samya, and Jason Dusek.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> The proposed Containerizer interface is to replace the existing Isolator.
>
> One ContainerizerProcess has been written:
> MesosContainerizerProcess - implements containerizeration internally using a Launcher and one or more Isolators (following review)
>
> The intent is to also support a generic ExternalContainerizerProcess that can delegate containerizeration by making external calls. Other Containerizers could interface with specific external containerization techniques such as Docker or LXC.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 655f86757487ddbe551fdcf53eb793e773ecdd34
> src/Makefile.am cf0c8c66e8dd21c2f5a2da22e5d5adb056353454
> src/common/type_utils.hpp 3b05751a9a27055dd19e4abb79197f53e5554fe1
> src/launcher/launcher.hpp 104fe812ddd2b75b14ec0b40d192f58cdba3603a
> src/launcher/launcher.cpp d5ab66704429a95eeb8eda5188e33d8e691221af
> src/launcher/main.cpp de64609905ee63096c0173fe7e64a1eafea5d6bf
> src/local/local.cpp 83a7f913afb1441e9137c7fcec8fd81815714905
> src/slave/cgroups_isolator.hpp e86062e9abaaa263c32c55e9dbfefd700f605886
> src/slave/cgroups_isolator.cpp 80155a3f0cd684a6aad9a37f61c5204d86032735
> src/slave/container/containerizer.hpp PRE-CREATION
> src/slave/container/containerizer.cpp PRE-CREATION
> src/slave/container/external_containerizer.hpp PRE-CREATION
> src/slave/container/mesos_containerizer.hpp PRE-CREATION
> src/slave/container/mesos_containerizer.cpp PRE-CREATION
> src/slave/flags.hpp 827b2d0d6dc8fa279f3187a09e5dc6d4799d17cd
> src/slave/http.cpp c8357e214d2adf2cd712072f58d07b07badb79dc
> src/slave/isolator.hpp 9634535d8c746597b4bb6e278587a1b9ca8f1608
> src/slave/isolator.cpp c9643cf9c5657bc142482a71fb161233bffb3b9f
> src/slave/main.cpp e0cae7b205c2599e05c4db990cc9c8e9e3673c37
> src/slave/monitor.hpp 52568ad8ec566f7cf36c249c76d798d44eacb578
> src/slave/monitor.cpp a931c4f35a8793c66ee03de82f0e0a21b92f8ffa
> src/slave/paths.hpp b1a56a346ca84e7e71c31386602264c5097ef1cf
> src/slave/process_isolator.hpp 4ae093fe65775a2b9bec42071961dd58aa0c3d8b
> src/slave/process_isolator.cpp 0bc698f04f7c8eaad166dc9d646e13310129dd01
> src/slave/slave.hpp 38ba7b6d6c35f0668d2f021a1285363cbf4367f4
> src/slave/slave.cpp 8b83dacad802a5434bf76ca075b2b8113ed14a84
> src/slave/state.hpp 78b20ffae1f2f629d851784302648e3bf5349321
> src/slave/state.cpp 93cde0c2efbbe5387448631a02e64b20a3d021c1
> src/slave/status_update_manager.hpp 06ea4659cdd24cb1b82f389f404566ba14a663fb
> src/slave/status_update_manager.cpp 03f5eafefd6ed748bfd4511d654c23c7b460db66
>
> Diff: https://reviews.apache.org/r/16147/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 16147: Containerizer (part 1)
Posted by Ian Downes <ia...@gmail.com>.
> On Jan. 22, 2014, 3:25 a.m., Adam B wrote:
> > src/local/local.cpp, line 83
> > <https://reviews.apache.org/r/16147/diff/6/?file=425459#file425459line83>
> >
> > I wonder if we should we be mapping ContainerID -> Slave* instead?
This map is storing Containerizers that have been created on the heap and need to be cleaned up.
> On Jan. 22, 2014, 3:25 a.m., Adam B wrote:
> > src/local/local.cpp, lines 155-159
> > <https://reviews.apache.org/r/16147/diff/6/?file=425459#file425459line155>
> >
> > BenH/Niklas may have already mentioned this, but why should anybody need to create a ContainerizerProcess before creating the Containerizer? Maybe make a factory for this?
Squashed so need for creating process and wrapping.
> On Jan. 22, 2014, 3:25 a.m., Adam B wrote:
> > src/slave/monitor.cpp, lines 76-77
> > <https://reviews.apache.org/r/16147/diff/6/?file=425473#file425473line76>
> >
> > Why does publish() take a prefix string instead of just taking the ContainerID/ExecutorInfo and then generating the prefix itself?
This will be part of Ben Mahler's cleanup.
> On Jan. 22, 2014, 3:25 a.m., Adam B wrote:
> > src/slave/slave.cpp, line 2074
> > <https://reviews.apache.org/r/16147/diff/6/?file=425478#file425478line2074>
> >
> > Can we name 'future' something that indicates what the future is for/from?
I tend to agree but this matches argument naming for other methods in the slave.
> On Jan. 22, 2014, 3:25 a.m., Adam B wrote:
> > src/slave/slave.cpp, line 2718
> > <https://reviews.apache.org/r/16147/diff/6/?file=425478#file425478line2718>
> >
> > Why not just "defer(containerizer, &Containerizer::recover, state)"?
This isn't currently supported, will be done alongside C++11 support.
- Ian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16147/#review32418
-----------------------------------------------------------
On Jan. 24, 2014, 6:25 p.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16147/
> -----------------------------------------------------------
>
> (Updated Jan. 24, 2014, 6:25 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, Chi Zhang, Niklas Nielsen, samya, and Jason Dusek.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> The proposed Containerizer interface is to replace the existing Isolator.
>
> One ContainerizerProcess has been written:
> MesosContainerizerProcess - implements containerizeration internally using a Launcher and one or more Isolators (following review)
>
> The intent is to also support a generic ExternalContainerizerProcess that can delegate containerizeration by making external calls. Other Containerizers could interface with specific external containerization techniques such as Docker or LXC.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 655f86757487ddbe551fdcf53eb793e773ecdd34
> src/Makefile.am cf0c8c66e8dd21c2f5a2da22e5d5adb056353454
> src/common/type_utils.hpp 3b05751a9a27055dd19e4abb79197f53e5554fe1
> src/launcher/launcher.hpp 104fe812ddd2b75b14ec0b40d192f58cdba3603a
> src/launcher/launcher.cpp d5ab66704429a95eeb8eda5188e33d8e691221af
> src/launcher/main.cpp de64609905ee63096c0173fe7e64a1eafea5d6bf
> src/local/local.cpp 83a7f913afb1441e9137c7fcec8fd81815714905
> src/slave/cgroups_isolator.hpp e86062e9abaaa263c32c55e9dbfefd700f605886
> src/slave/cgroups_isolator.cpp 80155a3f0cd684a6aad9a37f61c5204d86032735
> src/slave/container/containerizer.hpp PRE-CREATION
> src/slave/container/containerizer.cpp PRE-CREATION
> src/slave/container/mesos_containerizer.hpp PRE-CREATION
> src/slave/container/mesos_containerizer.cpp PRE-CREATION
> src/slave/flags.hpp 827b2d0d6dc8fa279f3187a09e5dc6d4799d17cd
> src/slave/http.cpp c8357e214d2adf2cd712072f58d07b07badb79dc
> src/slave/isolator.hpp 9634535d8c746597b4bb6e278587a1b9ca8f1608
> src/slave/isolator.cpp c9643cf9c5657bc142482a71fb161233bffb3b9f
> src/slave/main.cpp e0cae7b205c2599e05c4db990cc9c8e9e3673c37
> src/slave/monitor.hpp 52568ad8ec566f7cf36c249c76d798d44eacb578
> src/slave/monitor.cpp a931c4f35a8793c66ee03de82f0e0a21b92f8ffa
> src/slave/paths.hpp b1a56a346ca84e7e71c31386602264c5097ef1cf
> src/slave/process_isolator.hpp 4ae093fe65775a2b9bec42071961dd58aa0c3d8b
> src/slave/process_isolator.cpp 0bc698f04f7c8eaad166dc9d646e13310129dd01
> src/slave/slave.hpp 38ba7b6d6c35f0668d2f021a1285363cbf4367f4
> src/slave/slave.cpp 8b83dacad802a5434bf76ca075b2b8113ed14a84
> src/slave/state.hpp 78b20ffae1f2f629d851784302648e3bf5349321
> src/slave/state.cpp 93cde0c2efbbe5387448631a02e64b20a3d021c1
> src/slave/status_update_manager.hpp 06ea4659cdd24cb1b82f389f404566ba14a663fb
> src/slave/status_update_manager.cpp 03f5eafefd6ed748bfd4511d654c23c7b460db66
>
> Diff: https://reviews.apache.org/r/16147/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 16147: Containerizer (part 1)
Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16147/#review32418
-----------------------------------------------------------
Please bear with me. I'm still pretty new, so my comments/questions may be naive/trivial. Here goes!
src/Makefile.am
<https://reviews.apache.org/r/16147/#comment61263>
Alpha order?
src/local/local.cpp
<https://reviews.apache.org/r/16147/#comment61270>
I wonder if we should we be mapping ContainerID -> Slave* instead?
src/local/local.cpp
<https://reviews.apache.org/r/16147/#comment61264>
s/isolator/containerizer/
Or is this comment even valid anymore? BenH?
src/local/local.cpp
<https://reviews.apache.org/r/16147/#comment61265>
BenH/Niklas may have already mentioned this, but why should anybody need to create a ContainerizerProcess before creating the Containerizer? Maybe make a factory for this?
src/slave/flags.hpp
<https://reviews.apache.org/r/16147/#comment61271>
So "posix/cpu,posix/mem" is the default, but the help string suggests "cgroups/cpu,cgroups/mem", even if not on linux?
src/slave/monitor.hpp
<https://reviews.apache.org/r/16147/#comment61272>
Alpha order: map < string
src/slave/monitor.hpp
<https://reviews.apache.org/r/16147/#comment61273>
s/executor/container/
src/slave/monitor.hpp
<https://reviews.apache.org/r/16147/#comment61274>
s/executor/container/
src/slave/monitor.cpp
<https://reviews.apache.org/r/16147/#comment61276>
Why does publish() take a prefix string instead of just taking the ContainerID/ExecutorInfo and then generating the prefix itself?
src/slave/slave.cpp
<https://reviews.apache.org/r/16147/#comment61289>
Can we name 'future' something that indicates what the future is for/from?
src/slave/slave.cpp
<https://reviews.apache.org/r/16147/#comment61311>
Double space: "of executor"
src/slave/slave.cpp
<https://reviews.apache.org/r/16147/#comment61317>
Why not just "defer(containerizer, &Containerizer::recover, state)"?
src/slave/slave.cpp
<https://reviews.apache.org/r/16147/#comment61318>
Why use executor->containerId when we have a local containerId variable? Same for executor->info vs. executorInfo, executor->directory vs. directory, etc.?
- Adam B
On Jan. 21, 2014, 11:36 a.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16147/
> -----------------------------------------------------------
>
> (Updated Jan. 21, 2014, 11:36 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, Chi Zhang, Niklas Nielsen, samya, and Jason Dusek.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> The proposed Containerizer interface is to replace the existing Isolator.
>
> One ContainerizerProcess has been written:
> MesosContainerizerProcess - implements containerizeration internally using a Launcher and one or more Isolators (following review)
>
> The intent is to also support a generic ExternalContainerizerProcess that can delegate containerizeration by making external calls. Other Containerizers could interface with specific external containerization techniques such as Docker or LXC.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 655f86757487ddbe551fdcf53eb793e773ecdd34
> src/Makefile.am cf0c8c66e8dd21c2f5a2da22e5d5adb056353454
> src/common/type_utils.hpp 3b05751a9a27055dd19e4abb79197f53e5554fe1
> src/launcher/launcher.hpp 104fe812ddd2b75b14ec0b40d192f58cdba3603a
> src/launcher/launcher.cpp d5ab66704429a95eeb8eda5188e33d8e691221af
> src/launcher/main.cpp de64609905ee63096c0173fe7e64a1eafea5d6bf
> src/local/local.cpp 83a7f913afb1441e9137c7fcec8fd81815714905
> src/slave/cgroups_isolator.hpp e86062e9abaaa263c32c55e9dbfefd700f605886
> src/slave/cgroups_isolator.cpp 80155a3f0cd684a6aad9a37f61c5204d86032735
> src/slave/container/containerizer.hpp PRE-CREATION
> src/slave/container/containerizer.cpp PRE-CREATION
> src/slave/container/external_containerizer.hpp PRE-CREATION
> src/slave/container/mesos_containerizer.hpp PRE-CREATION
> src/slave/container/mesos_containerizer.cpp PRE-CREATION
> src/slave/flags.hpp 827b2d0d6dc8fa279f3187a09e5dc6d4799d17cd
> src/slave/http.cpp c8357e214d2adf2cd712072f58d07b07badb79dc
> src/slave/isolator.hpp 9634535d8c746597b4bb6e278587a1b9ca8f1608
> src/slave/isolator.cpp c9643cf9c5657bc142482a71fb161233bffb3b9f
> src/slave/main.cpp e0cae7b205c2599e05c4db990cc9c8e9e3673c37
> src/slave/monitor.hpp 52568ad8ec566f7cf36c249c76d798d44eacb578
> src/slave/monitor.cpp a931c4f35a8793c66ee03de82f0e0a21b92f8ffa
> src/slave/paths.hpp b1a56a346ca84e7e71c31386602264c5097ef1cf
> src/slave/process_isolator.hpp 4ae093fe65775a2b9bec42071961dd58aa0c3d8b
> src/slave/process_isolator.cpp 0bc698f04f7c8eaad166dc9d646e13310129dd01
> src/slave/slave.hpp 38ba7b6d6c35f0668d2f021a1285363cbf4367f4
> src/slave/slave.cpp 8b83dacad802a5434bf76ca075b2b8113ed14a84
> src/slave/state.hpp 78b20ffae1f2f629d851784302648e3bf5349321
> src/slave/state.cpp 93cde0c2efbbe5387448631a02e64b20a3d021c1
> src/slave/status_update_manager.hpp 06ea4659cdd24cb1b82f389f404566ba14a663fb
> src/slave/status_update_manager.cpp 03f5eafefd6ed748bfd4511d654c23c7b460db66
>
> Diff: https://reviews.apache.org/r/16147/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 16147: Containerizer (part 1)
Posted by Ian Downes <ia...@gmail.com>.
> On Jan. 28, 2014, 9:16 a.m., Vinod Kone wrote:
> > src/launcher/fetcher.cpp, lines 41-49
> > <https://reviews.apache.org/r/16147/diff/8/?file=451132#file451132line41>
> >
> > could we just do 'xf' and let tar figure out the compression?
This was copied straight from the old launcher, but yes. I had thought gnu tar required the flags but I checked now and it confirmed it doesn't
> On Jan. 28, 2014, 9:16 a.m., Vinod Kone wrote:
> > src/launcher/fetcher.cpp, line 216
> > <https://reviews.apache.org/r/16147/diff/8/?file=451132#file451132line216>
> >
> > You can kill "uri.has_executable()".
This relies on the optional executable defaulting to false - should we make that explicit in the proto file?
> On Jan. 28, 2014, 9:16 a.m., Vinod Kone wrote:
> > src/launcher/fetcher.cpp, line 222
> > <https://reviews.apache.org/r/16147/diff/8/?file=451132#file451132line222>
> >
> > Why the extraction in an else loop? Was this the old behavior? mesos.proto doesn't say that it is only extracted if it's executable.
The old code was:
if (executable) { chmod(); }
if (archive) { extract(); }
But what does it mean for a file to be both executable and an archive!? If it's executable then I don't believe we want to extract it. Is this not correct?
Technically, it could be both, e.g., a self-extracting archive, but we don't want to extract these with tar/unzip.
> On Jan. 28, 2014, 9:16 a.m., Vinod Kone wrote:
> > src/launcher/fetcher.cpp, line 230
> > <https://reviews.apache.org/r/16147/diff/8/?file=451132#file451132line230>
> >
> > s/a/an/ ?
Not for user because it's consonant sounding.
> On Jan. 28, 2014, 9:16 a.m., Vinod Kone wrote:
> > src/local/local.cpp, line 153
> > <https://reviews.apache.org/r/16147/diff/8/?file=451136#file451136line153>
> >
> > s/isolator/containerizer/ ?
> >
> > Do you know what is this TODO about?
No, I don't. [~benh], can you please elaborate?
> On Jan. 28, 2014, 9:16 a.m., Vinod Kone wrote:
> > src/slave/containerizer/containerizer.hpp, lines 77-78
> > <https://reviews.apache.org/r/16147/diff/8/?file=451139#file451139line77>
> >
> > Isn't this containerizer interface?
> >
> > s/enabled/enable/
I meant make it non-static so each containerizer implementation determines the resources. I've reworded the TODO to make this clearer.
> On Jan. 28, 2014, 9:16 a.m., Vinod Kone wrote:
> > src/slave/containerizer/containerizer.cpp, line 225
> > <https://reviews.apache.org/r/16147/diff/8/?file=451140#file451140line225>
> >
> > Should there be a check here for user specifying cgroups isolation on non-linux boxes?
The cgroups isolators are conditionally included in the map so specifying them on a non-linux platform will result in a 'not found' error. I've updated the message to include "unsupported isolators".
> On Jan. 28, 2014, 9:16 a.m., Vinod Kone wrote:
> > src/slave/containerizer/containerizer.cpp, lines 251-253
> > <https://reviews.apache.org/r/16147/diff/8/?file=451140#file451140line251>
> >
> > No need for "+"s to concatenate!?
adjacent string literals are automatically concatenated.
> On Jan. 28, 2014, 9:16 a.m., Vinod Kone wrote:
> > src/slave/containerizer/mesos_containerizer.cpp, line 209
> > <https://reviews.apache.org/r/16147/diff/8/?file=451142#file451142line209>
> >
> > Depending on how you fix the above, this should/could not be a CHECK either.
Kept this here because recover() should not try to _recover() any executors without pids.
> On Jan. 28, 2014, 9:16 a.m., Vinod Kone wrote:
> > src/slave/slave.hpp, line 266
> > <https://reviews.apache.org/r/16147/diff/8/?file=451153#file451153line266>
> >
> > not yours but could you s/recoveR/recover/
I originally had this in and [~bmahler] scolded me for included random fixes ;-)
> On Jan. 28, 2014, 9:16 a.m., Vinod Kone wrote:
> > src/slave/slave.cpp, lines 2041-2046
> > <https://reviews.apache.org/r/16147/diff/8/?file=451154#file451154line2041>
> >
> > With my recent change to include the first tasks resources when launching an executor do we still need this?
Removed and I'll rebase.
> On Jan. 28, 2014, 9:16 a.m., Vinod Kone wrote:
> > src/slave/slave.cpp, line 2081
> > <https://reviews.apache.org/r/16147/diff/8/?file=451154#file451154line2081>
> >
> > Wouldn't the containerizer destroy it automatically in this case?
No, the current semantics are that a container isn't destroyed until you actually call destroy(). Should this be revised?
- Ian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16147/#review32969
-----------------------------------------------------------
On Jan. 27, 2014, 3 a.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16147/
> -----------------------------------------------------------
>
> (Updated Jan. 27, 2014, 3 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, Chi Zhang, Niklas Nielsen, samya, and Jason Dusek.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> The proposed Containerizer interface is to replace the existing Isolator.
>
> One ContainerizerProcess has been written:
> MesosContainerizerProcess - implements containerizeration internally using a Launcher and one or more Isolators (following review)
>
> The intent is to also support a generic ExternalContainerizerProcess that can delegate containerizeration by making external calls. Other Containerizers could interface with specific external containerization techniques such as Docker or LXC.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 655f86757487ddbe551fdcf53eb793e773ecdd34
> src/Makefile.am cf0c8c66e8dd21c2f5a2da22e5d5adb056353454
> src/common/type_utils.hpp 3b05751a9a27055dd19e4abb79197f53e5554fe1
> src/launcher/fetcher.cpp PRE-CREATION
> src/launcher/launcher.hpp 104fe812ddd2b75b14ec0b40d192f58cdba3603a
> src/launcher/launcher.cpp d5ab66704429a95eeb8eda5188e33d8e691221af
> src/launcher/main.cpp de64609905ee63096c0173fe7e64a1eafea5d6bf
> src/local/local.cpp 83a7f913afb1441e9137c7fcec8fd81815714905
> src/slave/cgroups_isolator.hpp e86062e9abaaa263c32c55e9dbfefd700f605886
> src/slave/cgroups_isolator.cpp 80155a3f0cd684a6aad9a37f61c5204d86032735
> src/slave/containerizer/containerizer.hpp PRE-CREATION
> src/slave/containerizer/containerizer.cpp PRE-CREATION
> src/slave/containerizer/mesos_containerizer.hpp PRE-CREATION
> src/slave/containerizer/mesos_containerizer.cpp PRE-CREATION
> src/slave/flags.hpp 827b2d0d6dc8fa279f3187a09e5dc6d4799d17cd
> src/slave/http.cpp c8357e214d2adf2cd712072f58d07b07badb79dc
> src/slave/isolator.hpp 9634535d8c746597b4bb6e278587a1b9ca8f1608
> src/slave/isolator.cpp c9643cf9c5657bc142482a71fb161233bffb3b9f
> src/slave/main.cpp e0cae7b205c2599e05c4db990cc9c8e9e3673c37
> src/slave/monitor.hpp 52568ad8ec566f7cf36c249c76d798d44eacb578
> src/slave/monitor.cpp a931c4f35a8793c66ee03de82f0e0a21b92f8ffa
> src/slave/paths.hpp b1a56a346ca84e7e71c31386602264c5097ef1cf
> src/slave/process_isolator.hpp 4ae093fe65775a2b9bec42071961dd58aa0c3d8b
> src/slave/process_isolator.cpp 0bc698f04f7c8eaad166dc9d646e13310129dd01
> src/slave/slave.hpp 38ba7b6d6c35f0668d2f021a1285363cbf4367f4
> src/slave/slave.cpp 8b83dacad802a5434bf76ca075b2b8113ed14a84
> src/slave/state.hpp 78b20ffae1f2f629d851784302648e3bf5349321
> src/slave/state.cpp 93cde0c2efbbe5387448631a02e64b20a3d021c1
> src/slave/status_update_manager.hpp 06ea4659cdd24cb1b82f389f404566ba14a663fb
> src/slave/status_update_manager.cpp 03f5eafefd6ed748bfd4511d654c23c7b460db66
>
> Diff: https://reviews.apache.org/r/16147/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 16147: Containerizer (part 1)
Posted by Benjamin Hindman <be...@berkeley.edu>.
> On Jan. 28, 2014, 9:16 a.m., Vinod Kone wrote:
> > src/local/local.cpp, line 153
> > <https://reviews.apache.org/r/16147/diff/8/?file=451136#file451136line153>
> >
> > s/isolator/containerizer/ ?
> >
> > Do you know what is this TODO about?
>
> Ian Downes wrote:
> No, I don't. [~benh], can you please elaborate?
The TODO referred to the fact that we have to pass in a 'local=true' bool when creating the slave instead of the isolator. Now that we pass the flag directly to the containerizer this TODO can be removed.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16147/#review32969
-----------------------------------------------------------
On Jan. 27, 2014, 3 a.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16147/
> -----------------------------------------------------------
>
> (Updated Jan. 27, 2014, 3 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, Chi Zhang, Niklas Nielsen, samya, and Jason Dusek.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> The proposed Containerizer interface is to replace the existing Isolator.
>
> One ContainerizerProcess has been written:
> MesosContainerizerProcess - implements containerizeration internally using a Launcher and one or more Isolators (following review)
>
> The intent is to also support a generic ExternalContainerizerProcess that can delegate containerizeration by making external calls. Other Containerizers could interface with specific external containerization techniques such as Docker or LXC.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 655f86757487ddbe551fdcf53eb793e773ecdd34
> src/Makefile.am cf0c8c66e8dd21c2f5a2da22e5d5adb056353454
> src/common/type_utils.hpp 3b05751a9a27055dd19e4abb79197f53e5554fe1
> src/launcher/fetcher.cpp PRE-CREATION
> src/launcher/launcher.hpp 104fe812ddd2b75b14ec0b40d192f58cdba3603a
> src/launcher/launcher.cpp d5ab66704429a95eeb8eda5188e33d8e691221af
> src/launcher/main.cpp de64609905ee63096c0173fe7e64a1eafea5d6bf
> src/local/local.cpp 83a7f913afb1441e9137c7fcec8fd81815714905
> src/slave/cgroups_isolator.hpp e86062e9abaaa263c32c55e9dbfefd700f605886
> src/slave/cgroups_isolator.cpp 80155a3f0cd684a6aad9a37f61c5204d86032735
> src/slave/containerizer/containerizer.hpp PRE-CREATION
> src/slave/containerizer/containerizer.cpp PRE-CREATION
> src/slave/containerizer/mesos_containerizer.hpp PRE-CREATION
> src/slave/containerizer/mesos_containerizer.cpp PRE-CREATION
> src/slave/flags.hpp 827b2d0d6dc8fa279f3187a09e5dc6d4799d17cd
> src/slave/http.cpp c8357e214d2adf2cd712072f58d07b07badb79dc
> src/slave/isolator.hpp 9634535d8c746597b4bb6e278587a1b9ca8f1608
> src/slave/isolator.cpp c9643cf9c5657bc142482a71fb161233bffb3b9f
> src/slave/main.cpp e0cae7b205c2599e05c4db990cc9c8e9e3673c37
> src/slave/monitor.hpp 52568ad8ec566f7cf36c249c76d798d44eacb578
> src/slave/monitor.cpp a931c4f35a8793c66ee03de82f0e0a21b92f8ffa
> src/slave/paths.hpp b1a56a346ca84e7e71c31386602264c5097ef1cf
> src/slave/process_isolator.hpp 4ae093fe65775a2b9bec42071961dd58aa0c3d8b
> src/slave/process_isolator.cpp 0bc698f04f7c8eaad166dc9d646e13310129dd01
> src/slave/slave.hpp 38ba7b6d6c35f0668d2f021a1285363cbf4367f4
> src/slave/slave.cpp 8b83dacad802a5434bf76ca075b2b8113ed14a84
> src/slave/state.hpp 78b20ffae1f2f629d851784302648e3bf5349321
> src/slave/state.cpp 93cde0c2efbbe5387448631a02e64b20a3d021c1
> src/slave/status_update_manager.hpp 06ea4659cdd24cb1b82f389f404566ba14a663fb
> src/slave/status_update_manager.cpp 03f5eafefd6ed748bfd4511d654c23c7b460db66
>
> Diff: https://reviews.apache.org/r/16147/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 16147: Containerizer (part 1)
Posted by Vinod Kone <vi...@gmail.com>.
> On Jan. 28, 2014, 9:16 a.m., Vinod Kone wrote:
> > src/launcher/fetcher.cpp, line 222
> > <https://reviews.apache.org/r/16147/diff/8/?file=451132#file451132line222>
> >
> > Why the extraction in an else loop? Was this the old behavior? mesos.proto doesn't say that it is only extracted if it's executable.
>
> Ian Downes wrote:
> The old code was:
>
> if (executable) { chmod(); }
> if (archive) { extract(); }
>
> But what does it mean for a file to be both executable and an archive!? If it's executable then I don't believe we want to extract it. Is this not correct?
>
> Technically, it could be both, e.g., a self-extracting archive, but we don't want to extract these with tar/unzip.
>
>
I see. Could add a comment on the CommandInfo proto reflecting this?
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16147/#review32969
-----------------------------------------------------------
On Jan. 27, 2014, 3 a.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16147/
> -----------------------------------------------------------
>
> (Updated Jan. 27, 2014, 3 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, Chi Zhang, Niklas Nielsen, samya, and Jason Dusek.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> The proposed Containerizer interface is to replace the existing Isolator.
>
> One ContainerizerProcess has been written:
> MesosContainerizerProcess - implements containerizeration internally using a Launcher and one or more Isolators (following review)
>
> The intent is to also support a generic ExternalContainerizerProcess that can delegate containerizeration by making external calls. Other Containerizers could interface with specific external containerization techniques such as Docker or LXC.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 655f86757487ddbe551fdcf53eb793e773ecdd34
> src/Makefile.am cf0c8c66e8dd21c2f5a2da22e5d5adb056353454
> src/common/type_utils.hpp 3b05751a9a27055dd19e4abb79197f53e5554fe1
> src/launcher/fetcher.cpp PRE-CREATION
> src/launcher/launcher.hpp 104fe812ddd2b75b14ec0b40d192f58cdba3603a
> src/launcher/launcher.cpp d5ab66704429a95eeb8eda5188e33d8e691221af
> src/launcher/main.cpp de64609905ee63096c0173fe7e64a1eafea5d6bf
> src/local/local.cpp 83a7f913afb1441e9137c7fcec8fd81815714905
> src/slave/cgroups_isolator.hpp e86062e9abaaa263c32c55e9dbfefd700f605886
> src/slave/cgroups_isolator.cpp 80155a3f0cd684a6aad9a37f61c5204d86032735
> src/slave/containerizer/containerizer.hpp PRE-CREATION
> src/slave/containerizer/containerizer.cpp PRE-CREATION
> src/slave/containerizer/mesos_containerizer.hpp PRE-CREATION
> src/slave/containerizer/mesos_containerizer.cpp PRE-CREATION
> src/slave/flags.hpp 827b2d0d6dc8fa279f3187a09e5dc6d4799d17cd
> src/slave/http.cpp c8357e214d2adf2cd712072f58d07b07badb79dc
> src/slave/isolator.hpp 9634535d8c746597b4bb6e278587a1b9ca8f1608
> src/slave/isolator.cpp c9643cf9c5657bc142482a71fb161233bffb3b9f
> src/slave/main.cpp e0cae7b205c2599e05c4db990cc9c8e9e3673c37
> src/slave/monitor.hpp 52568ad8ec566f7cf36c249c76d798d44eacb578
> src/slave/monitor.cpp a931c4f35a8793c66ee03de82f0e0a21b92f8ffa
> src/slave/paths.hpp b1a56a346ca84e7e71c31386602264c5097ef1cf
> src/slave/process_isolator.hpp 4ae093fe65775a2b9bec42071961dd58aa0c3d8b
> src/slave/process_isolator.cpp 0bc698f04f7c8eaad166dc9d646e13310129dd01
> src/slave/slave.hpp 38ba7b6d6c35f0668d2f021a1285363cbf4367f4
> src/slave/slave.cpp 8b83dacad802a5434bf76ca075b2b8113ed14a84
> src/slave/state.hpp 78b20ffae1f2f629d851784302648e3bf5349321
> src/slave/state.cpp 93cde0c2efbbe5387448631a02e64b20a3d021c1
> src/slave/status_update_manager.hpp 06ea4659cdd24cb1b82f389f404566ba14a663fb
> src/slave/status_update_manager.cpp 03f5eafefd6ed748bfd4511d654c23c7b460db66
>
> Diff: https://reviews.apache.org/r/16147/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 16147: Containerizer (part 1)
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16147/#review32969
-----------------------------------------------------------
Looks great!
My main concern is that there seems to be a bug in recover() when an executor dies before checkpointing forked pid.
src/launcher/fetcher.cpp
<https://reviews.apache.org/r/16147/#comment62057>
could we just do 'xf' and let tar figure out the compression?
src/launcher/fetcher.cpp
<https://reviews.apache.org/r/16147/#comment62055>
not properly aligned.
src/launcher/fetcher.cpp
<https://reviews.apache.org/r/16147/#comment62056>
not properly aligned.
src/launcher/fetcher.cpp
<https://reviews.apache.org/r/16147/#comment62059>
CHECK(pos != std::string::npos)
<< "blah....";
src/launcher/fetcher.cpp
<https://reviews.apache.org/r/16147/#comment62060>
You can kill "uri.has_executable()".
src/launcher/fetcher.cpp
<https://reviews.apache.org/r/16147/#comment62061>
s/chmod"/chmod: "<< fetched.get()/
src/launcher/fetcher.cpp
<https://reviews.apache.org/r/16147/#comment62062>
Why the extraction in an else loop? Was this the old behavior? mesos.proto doesn't say that it is only extracted if it's executable.
src/launcher/fetcher.cpp
<https://reviews.apache.org/r/16147/#comment62064>
include the filename here.
src/launcher/fetcher.cpp
<https://reviews.apache.org/r/16147/#comment62063>
s/a/an/ ?
src/launcher/fetcher.cpp
<https://reviews.apache.org/r/16147/#comment62065>
include the filename here.
src/local/local.cpp
<https://reviews.apache.org/r/16147/#comment62066>
s/isolator/containerizer/ ?
Do you know what is this TODO about?
src/slave/containerizer/containerizer.hpp
<https://reviews.apache.org/r/16147/#comment62089>
Isn't this containerizer interface?
s/enabled/enable/
src/slave/containerizer/containerizer.hpp
<https://reviews.apache.org/r/16147/#comment62091>
s/Termination/'Termination'/
s/terminates/terminates,/ ?
src/slave/containerizer/containerizer.hpp
<https://reviews.apache.org/r/16147/#comment62093>
new line.
src/slave/containerizer/containerizer.hpp
<https://reviews.apache.org/r/16147/#comment62094>
new line.
src/slave/containerizer/containerizer.hpp
<https://reviews.apache.org/r/16147/#comment62095>
new line.
src/slave/containerizer/containerizer.cpp
<https://reviews.apache.org/r/16147/#comment62096>
why not a hashmap?
src/slave/containerizer/containerizer.cpp
<https://reviews.apache.org/r/16147/#comment62097>
Should there be a check here for user specifying cgroups isolation on non-linux boxes?
src/slave/containerizer/containerizer.cpp
<https://reviews.apache.org/r/16147/#comment62099>
No need for "+"s to concatenate!?
src/slave/containerizer/mesos_containerizer.hpp
<https://reviews.apache.org/r/16147/#comment62100>
new line.
src/slave/containerizer/mesos_containerizer.hpp
<https://reviews.apache.org/r/16147/#comment62103>
s/destroy()/'destroy()'/
src/slave/containerizer/mesos_containerizer.hpp
<https://reviews.apache.org/r/16147/#comment62106>
s/_destroy()/'_destroy()'/
src/slave/containerizer/mesos_containerizer.cpp
<https://reviews.apache.org/r/16147/#comment62109>
process = new MesosContainerizerProcess(
flags, local, launcher, isolators);
src/slave/containerizer/mesos_containerizer.cpp
<https://reviews.apache.org/r/16147/#comment62114>
I haven't yet seen where the forked pid checkpointing is done, so I might be missing something here.
But according to the old semantics this should not be a return!
It is entirely possible that an executor died before it got a chance to checkpoint its forked pid.
src/slave/containerizer/mesos_containerizer.cpp
<https://reviews.apache.org/r/16147/#comment62116>
Depending on how you fix the above, this should/could not be a CHECK either.
src/slave/containerizer/mesos_containerizer.cpp
<https://reviews.apache.org/r/16147/#comment62117>
Do you need the if loop? How about doing straight up trim?
src/slave/flags.hpp
<https://reviews.apache.org/r/16147/#comment62067>
s/isolation/Flags::isolation/ ?
src/slave/monitor.hpp
<https://reviews.apache.org/r/16147/#comment62068>
s/executor/container/
src/slave/monitor.hpp
<https://reviews.apache.org/r/16147/#comment62069>
s/executor/container/
src/slave/slave.hpp
<https://reviews.apache.org/r/16147/#comment62073>
not yours but could you s/recoveR/recover/
src/slave/slave.hpp
<https://reviews.apache.org/r/16147/#comment62074>
Is this called after _recover but before __recover? Not looking ahead at the implementation, this seems to warrant a comment either way.
src/slave/slave.cpp
<https://reviews.apache.org/r/16147/#comment62075>
s/."/: " << resources.error()/
src/slave/slave.cpp
<https://reviews.apache.org/r/16147/#comment62077>
how about the following to make it consistent with how you did it elsewhere.
statusUpdateManager->update(
update,
info.id(),
executor->id,
executor->containerId)
.onAny(...)
src/slave/slave.cpp
<https://reviews.apache.org/r/16147/#comment62079>
With my recent change to include the first tasks resources when launching an executor do we still need this?
src/slave/slave.cpp
<https://reviews.apache.org/r/16147/#comment62080>
s/container"/container " << containerId/
src/slave/slave.cpp
<https://reviews.apache.org/r/16147/#comment62081>
Why the quotes here but not above?
src/slave/slave.cpp
<https://reviews.apache.org/r/16147/#comment62092>
Wouldn't the containerizer destroy it automatically in this case?
src/slave/slave.cpp
<https://reviews.apache.org/r/16147/#comment62082>
Could you wrap termination.isReady && termination.get().killed insidd "()"
src/slave/slave.cpp
<https://reviews.apache.org/r/16147/#comment62083>
Are you missing termination.isReady() check here?
src/slave/slave.cpp
<https://reviews.apache.org/r/16147/#comment62085>
This cannot happen anymore because we call wait below correct?
src/slave/slave.cpp
<https://reviews.apache.org/r/16147/#comment62086>
s/launchExecutor()/'launchExecutor()'/
- Vinod Kone
On Jan. 27, 2014, 3 a.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16147/
> -----------------------------------------------------------
>
> (Updated Jan. 27, 2014, 3 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, Chi Zhang, Niklas Nielsen, samya, and Jason Dusek.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> The proposed Containerizer interface is to replace the existing Isolator.
>
> One ContainerizerProcess has been written:
> MesosContainerizerProcess - implements containerizeration internally using a Launcher and one or more Isolators (following review)
>
> The intent is to also support a generic ExternalContainerizerProcess that can delegate containerizeration by making external calls. Other Containerizers could interface with specific external containerization techniques such as Docker or LXC.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 655f86757487ddbe551fdcf53eb793e773ecdd34
> src/Makefile.am cf0c8c66e8dd21c2f5a2da22e5d5adb056353454
> src/common/type_utils.hpp 3b05751a9a27055dd19e4abb79197f53e5554fe1
> src/launcher/fetcher.cpp PRE-CREATION
> src/launcher/launcher.hpp 104fe812ddd2b75b14ec0b40d192f58cdba3603a
> src/launcher/launcher.cpp d5ab66704429a95eeb8eda5188e33d8e691221af
> src/launcher/main.cpp de64609905ee63096c0173fe7e64a1eafea5d6bf
> src/local/local.cpp 83a7f913afb1441e9137c7fcec8fd81815714905
> src/slave/cgroups_isolator.hpp e86062e9abaaa263c32c55e9dbfefd700f605886
> src/slave/cgroups_isolator.cpp 80155a3f0cd684a6aad9a37f61c5204d86032735
> src/slave/containerizer/containerizer.hpp PRE-CREATION
> src/slave/containerizer/containerizer.cpp PRE-CREATION
> src/slave/containerizer/mesos_containerizer.hpp PRE-CREATION
> src/slave/containerizer/mesos_containerizer.cpp PRE-CREATION
> src/slave/flags.hpp 827b2d0d6dc8fa279f3187a09e5dc6d4799d17cd
> src/slave/http.cpp c8357e214d2adf2cd712072f58d07b07badb79dc
> src/slave/isolator.hpp 9634535d8c746597b4bb6e278587a1b9ca8f1608
> src/slave/isolator.cpp c9643cf9c5657bc142482a71fb161233bffb3b9f
> src/slave/main.cpp e0cae7b205c2599e05c4db990cc9c8e9e3673c37
> src/slave/monitor.hpp 52568ad8ec566f7cf36c249c76d798d44eacb578
> src/slave/monitor.cpp a931c4f35a8793c66ee03de82f0e0a21b92f8ffa
> src/slave/paths.hpp b1a56a346ca84e7e71c31386602264c5097ef1cf
> src/slave/process_isolator.hpp 4ae093fe65775a2b9bec42071961dd58aa0c3d8b
> src/slave/process_isolator.cpp 0bc698f04f7c8eaad166dc9d646e13310129dd01
> src/slave/slave.hpp 38ba7b6d6c35f0668d2f021a1285363cbf4367f4
> src/slave/slave.cpp 8b83dacad802a5434bf76ca075b2b8113ed14a84
> src/slave/state.hpp 78b20ffae1f2f629d851784302648e3bf5349321
> src/slave/state.cpp 93cde0c2efbbe5387448631a02e64b20a3d021c1
> src/slave/status_update_manager.hpp 06ea4659cdd24cb1b82f389f404566ba14a663fb
> src/slave/status_update_manager.cpp 03f5eafefd6ed748bfd4511d654c23c7b460db66
>
> Diff: https://reviews.apache.org/r/16147/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 16147: Containerizer (part 1)
Posted by Ian Downes <ia...@gmail.com>.
> On Feb. 7, 2014, 12:55 a.m., Niklas Nielsen wrote:
> > src/slave/containerizer/mesos_containerizer.cpp, line 238
> > <https://reviews.apache.org/r/16147/diff/8-11/?file=451142#file451142line238>
> >
> > This is unfortunately not enough to silence gcc version 4.8.1 (Ubuntu/Linaro 4.8.1-10ubuntu9).
> > Think you actually need to read the value:
> >
> > ssize_t written = ...
> > (void)written;
this fix crept into a subsequent review.
- Ian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16147/#review33880
-----------------------------------------------------------
On Feb. 6, 2014, 5:34 a.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16147/
> -----------------------------------------------------------
>
> (Updated Feb. 6, 2014, 5:34 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, Chi Zhang, Niklas Nielsen, samya, and Jason Dusek.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> The proposed Containerizer interface is to replace the existing Isolator.
>
> One ContainerizerProcess has been written:
> MesosContainerizerProcess - implements containerizeration internally using a Launcher and one or more Isolators (following review)
>
> The intent is to also support a generic ExternalContainerizerProcess that can delegate containerizeration by making external calls. Other Containerizers could interface with specific external containerization techniques such as Docker or LXC.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 1503e73f0d299f8efb209e0b10966c9a7f8db5f8
> src/Makefile.am c30706846bca1fa3287291e39f46a23713ad1ba4
> src/common/type_utils.hpp fe6bf71d689f7bfd8b6ae1b8fab9b2e76e28e7a8
> src/launcher/fetcher.cpp PRE-CREATION
> src/launcher/launcher.hpp 104fe812ddd2b75b14ec0b40d192f58cdba3603a
> src/launcher/launcher.cpp d5ab66704429a95eeb8eda5188e33d8e691221af
> src/launcher/main.cpp de64609905ee63096c0173fe7e64a1eafea5d6bf
> src/local/local.cpp 83a7f913afb1441e9137c7fcec8fd81815714905
> src/slave/cgroups_isolator.hpp 1a66dc630f0857db48b7cfb316fbbb15518fcec2
> src/slave/cgroups_isolator.cpp ef7dd682e5462d0158d6ea6654246d77000e9778
> src/slave/containerizer/containerizer.hpp PRE-CREATION
> src/slave/containerizer/containerizer.cpp PRE-CREATION
> src/slave/containerizer/mesos_containerizer.hpp PRE-CREATION
> src/slave/containerizer/mesos_containerizer.cpp PRE-CREATION
> src/slave/flags.hpp 827b2d0d6dc8fa279f3187a09e5dc6d4799d17cd
> src/slave/http.cpp c4f598faf6807214608cc89a6d9cf665133f95f3
> src/slave/isolator.hpp 9634535d8c746597b4bb6e278587a1b9ca8f1608
> src/slave/isolator.cpp c9643cf9c5657bc142482a71fb161233bffb3b9f
> src/slave/main.cpp e0cae7b205c2599e05c4db990cc9c8e9e3673c37
> src/slave/monitor.hpp b677410e20eeafa7eac9acb4e687af7c8d53809f
> src/slave/monitor.cpp bb3723e967b0723bc2b925019b6e9c4170dcc071
> src/slave/paths.hpp 70ee0f31ebb294011e5ec6b05ce3ad28b2c4bacc
> src/slave/process_isolator.hpp bc52f33d8e83fe026be712c8b15e689eb23dd65c
> src/slave/process_isolator.cpp 09cb9968c914a095b5737b70bf5044865fc3a2c4
> src/slave/slave.hpp 891c874397f5add9b70432c40c152f7f19922e34
> src/slave/slave.cpp a97b1d531d05e63e8ebaa474f526c110f5d573d4
> src/slave/state.hpp 78b20ffae1f2f629d851784302648e3bf5349321
> src/slave/state.cpp 6c382cd30602ede60b7d526acc272adc16285a93
> src/slave/status_update_manager.hpp 06ea4659cdd24cb1b82f389f404566ba14a663fb
> src/slave/status_update_manager.cpp 03f5eafefd6ed748bfd4511d654c23c7b460db66
>
> Diff: https://reviews.apache.org/r/16147/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 16147: Containerizer (part 1)
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16147/#review33880
-----------------------------------------------------------
src/launcher/fetcher.cpp
<https://reviews.apache.org/r/16147/#comment63574>
Wrap line.
src/slave/containerizer/mesos_containerizer.cpp
<https://reviews.apache.org/r/16147/#comment63575>
Wrap line.
src/slave/containerizer/mesos_containerizer.cpp
<https://reviews.apache.org/r/16147/#comment63573>
This is unfortunately not enough to silence gcc version 4.8.1 (Ubuntu/Linaro 4.8.1-10ubuntu9).
Think you actually need to read the value:
ssize_t written = ...
(void)written;
src/slave/containerizer/mesos_containerizer.cpp
<https://reviews.apache.org/r/16147/#comment63577>
Wrap line.
- Niklas Nielsen
On Feb. 5, 2014, 9:34 p.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16147/
> -----------------------------------------------------------
>
> (Updated Feb. 5, 2014, 9:34 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, Chi Zhang, Niklas Nielsen, samya, and Jason Dusek.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> The proposed Containerizer interface is to replace the existing Isolator.
>
> One ContainerizerProcess has been written:
> MesosContainerizerProcess - implements containerizeration internally using a Launcher and one or more Isolators (following review)
>
> The intent is to also support a generic ExternalContainerizerProcess that can delegate containerizeration by making external calls. Other Containerizers could interface with specific external containerization techniques such as Docker or LXC.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 1503e73f0d299f8efb209e0b10966c9a7f8db5f8
> src/Makefile.am c30706846bca1fa3287291e39f46a23713ad1ba4
> src/common/type_utils.hpp fe6bf71d689f7bfd8b6ae1b8fab9b2e76e28e7a8
> src/launcher/fetcher.cpp PRE-CREATION
> src/launcher/launcher.hpp 104fe812ddd2b75b14ec0b40d192f58cdba3603a
> src/launcher/launcher.cpp d5ab66704429a95eeb8eda5188e33d8e691221af
> src/launcher/main.cpp de64609905ee63096c0173fe7e64a1eafea5d6bf
> src/local/local.cpp 83a7f913afb1441e9137c7fcec8fd81815714905
> src/slave/cgroups_isolator.hpp 1a66dc630f0857db48b7cfb316fbbb15518fcec2
> src/slave/cgroups_isolator.cpp ef7dd682e5462d0158d6ea6654246d77000e9778
> src/slave/containerizer/containerizer.hpp PRE-CREATION
> src/slave/containerizer/containerizer.cpp PRE-CREATION
> src/slave/containerizer/mesos_containerizer.hpp PRE-CREATION
> src/slave/containerizer/mesos_containerizer.cpp PRE-CREATION
> src/slave/flags.hpp 827b2d0d6dc8fa279f3187a09e5dc6d4799d17cd
> src/slave/http.cpp c4f598faf6807214608cc89a6d9cf665133f95f3
> src/slave/isolator.hpp 9634535d8c746597b4bb6e278587a1b9ca8f1608
> src/slave/isolator.cpp c9643cf9c5657bc142482a71fb161233bffb3b9f
> src/slave/main.cpp e0cae7b205c2599e05c4db990cc9c8e9e3673c37
> src/slave/monitor.hpp b677410e20eeafa7eac9acb4e687af7c8d53809f
> src/slave/monitor.cpp bb3723e967b0723bc2b925019b6e9c4170dcc071
> src/slave/paths.hpp 70ee0f31ebb294011e5ec6b05ce3ad28b2c4bacc
> src/slave/process_isolator.hpp bc52f33d8e83fe026be712c8b15e689eb23dd65c
> src/slave/process_isolator.cpp 09cb9968c914a095b5737b70bf5044865fc3a2c4
> src/slave/slave.hpp 891c874397f5add9b70432c40c152f7f19922e34
> src/slave/slave.cpp a97b1d531d05e63e8ebaa474f526c110f5d573d4
> src/slave/state.hpp 78b20ffae1f2f629d851784302648e3bf5349321
> src/slave/state.cpp 6c382cd30602ede60b7d526acc272adc16285a93
> src/slave/status_update_manager.hpp 06ea4659cdd24cb1b82f389f404566ba14a663fb
> src/slave/status_update_manager.cpp 03f5eafefd6ed748bfd4511d654c23c7b460db66
>
> Diff: https://reviews.apache.org/r/16147/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 16147: Containerizer (part 1)
Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16147/
-----------------------------------------------------------
(Updated Feb. 12, 2014, 12:17 a.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, Chi Zhang, Niklas Nielsen, samya, and Jason Dusek.
Changes
-------
Adressed Ben's comments.
Repository: mesos-git
Description
-------
The proposed Containerizer interface is to replace the existing Isolator.
One ContainerizerProcess has been written:
MesosContainerizerProcess - implements containerizeration internally using a Launcher and one or more Isolators (following review)
The intent is to also support a generic ExternalContainerizerProcess that can delegate containerizeration by making external calls. Other Containerizers could interface with specific external containerization techniques such as Docker or LXC.
Diffs (updated)
-----
include/mesos/mesos.proto 7079e032223c2537e966e38755ed42ac69a663f0
src/Makefile.am c30706846bca1fa3287291e39f46a23713ad1ba4
src/common/type_utils.hpp b8fc573e86775708b6a142217823b2d9985ceda9
src/launcher/fetcher.cpp PRE-CREATION
src/launcher/launcher.hpp 104fe812ddd2b75b14ec0b40d192f58cdba3603a
src/launcher/launcher.cpp d5ab66704429a95eeb8eda5188e33d8e691221af
src/launcher/main.cpp de64609905ee63096c0173fe7e64a1eafea5d6bf
src/local/local.cpp e650de91acb0fc8448f76d5a658353ab84e7661c
src/slave/cgroups_isolator.hpp 1a66dc630f0857db48b7cfb316fbbb15518fcec2
src/slave/cgroups_isolator.cpp ef7dd682e5462d0158d6ea6654246d77000e9778
src/slave/containerizer/containerizer.hpp PRE-CREATION
src/slave/containerizer/containerizer.cpp PRE-CREATION
src/slave/containerizer/mesos_containerizer.hpp PRE-CREATION
src/slave/containerizer/mesos_containerizer.cpp PRE-CREATION
src/slave/flags.hpp 827b2d0d6dc8fa279f3187a09e5dc6d4799d17cd
src/slave/http.cpp c4f598faf6807214608cc89a6d9cf665133f95f3
src/slave/isolator.hpp 9634535d8c746597b4bb6e278587a1b9ca8f1608
src/slave/isolator.cpp c9643cf9c5657bc142482a71fb161233bffb3b9f
src/slave/main.cpp 44020af30a76c8092f5b280f86a1f2865efe0233
src/slave/monitor.hpp b677410e20eeafa7eac9acb4e687af7c8d53809f
src/slave/monitor.cpp bb3723e967b0723bc2b925019b6e9c4170dcc071
src/slave/paths.hpp 70ee0f31ebb294011e5ec6b05ce3ad28b2c4bacc
src/slave/process_isolator.hpp bc52f33d8e83fe026be712c8b15e689eb23dd65c
src/slave/process_isolator.cpp 09cb9968c914a095b5737b70bf5044865fc3a2c4
src/slave/slave.hpp 2ddadb411f8b2f91a4a5038664f424d4eadfbb87
src/slave/slave.cpp 213df86084e51ab8407c6f2c2bd24ba718560faa
src/slave/state.hpp 78b20ffae1f2f629d851784302648e3bf5349321
src/slave/state.cpp 6c382cd30602ede60b7d526acc272adc16285a93
src/slave/status_update_manager.hpp 06ea4659cdd24cb1b82f389f404566ba14a663fb
src/slave/status_update_manager.cpp 03f5eafefd6ed748bfd4511d654c23c7b460db66
Diff: https://reviews.apache.org/r/16147/diff/
Testing
-------
Thanks,
Ian Downes
Re: Review Request 16147: Containerizer (part 1)
Posted by TILL TOENSHOFF <to...@me.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16147/#review34025
-----------------------------------------------------------
src/slave/containerizer/containerizer.hpp
<https://reviews.apache.org/r/16147/#comment63977>
Would you care to explain on why you chose to make the Containerizer declarations of the commands (launch/wait/...) pure virtual instead of having the ContainerizerProcess implementations being pure virtual?
If I understand things correctly, then Containerizer (as in MesosContainerizer) will be doing dispatches and not much more, correct?
- TILL TOENSHOFF
On Feb. 7, 2014, 3:35 a.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16147/
> -----------------------------------------------------------
>
> (Updated Feb. 7, 2014, 3:35 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, Chi Zhang, Niklas Nielsen, samya, and Jason Dusek.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> The proposed Containerizer interface is to replace the existing Isolator.
>
> One ContainerizerProcess has been written:
> MesosContainerizerProcess - implements containerizeration internally using a Launcher and one or more Isolators (following review)
>
> The intent is to also support a generic ExternalContainerizerProcess that can delegate containerizeration by making external calls. Other Containerizers could interface with specific external containerization techniques such as Docker or LXC.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 1503e73f0d299f8efb209e0b10966c9a7f8db5f8
> src/Makefile.am c30706846bca1fa3287291e39f46a23713ad1ba4
> src/common/type_utils.hpp fe6bf71d689f7bfd8b6ae1b8fab9b2e76e28e7a8
> src/launcher/fetcher.cpp PRE-CREATION
> src/launcher/launcher.hpp 104fe812ddd2b75b14ec0b40d192f58cdba3603a
> src/launcher/launcher.cpp d5ab66704429a95eeb8eda5188e33d8e691221af
> src/launcher/main.cpp de64609905ee63096c0173fe7e64a1eafea5d6bf
> src/local/local.cpp 83a7f913afb1441e9137c7fcec8fd81815714905
> src/slave/cgroups_isolator.hpp 1a66dc630f0857db48b7cfb316fbbb15518fcec2
> src/slave/cgroups_isolator.cpp ef7dd682e5462d0158d6ea6654246d77000e9778
> src/slave/containerizer/containerizer.hpp PRE-CREATION
> src/slave/containerizer/containerizer.cpp PRE-CREATION
> src/slave/containerizer/mesos_containerizer.hpp PRE-CREATION
> src/slave/containerizer/mesos_containerizer.cpp PRE-CREATION
> src/slave/flags.hpp 827b2d0d6dc8fa279f3187a09e5dc6d4799d17cd
> src/slave/http.cpp c4f598faf6807214608cc89a6d9cf665133f95f3
> src/slave/isolator.hpp 9634535d8c746597b4bb6e278587a1b9ca8f1608
> src/slave/isolator.cpp c9643cf9c5657bc142482a71fb161233bffb3b9f
> src/slave/main.cpp 6a7131635218181bd85beb7f097c2718f16fa734
> src/slave/monitor.hpp b677410e20eeafa7eac9acb4e687af7c8d53809f
> src/slave/monitor.cpp bb3723e967b0723bc2b925019b6e9c4170dcc071
> src/slave/paths.hpp 70ee0f31ebb294011e5ec6b05ce3ad28b2c4bacc
> src/slave/process_isolator.hpp bc52f33d8e83fe026be712c8b15e689eb23dd65c
> src/slave/process_isolator.cpp 09cb9968c914a095b5737b70bf5044865fc3a2c4
> src/slave/slave.hpp 891c874397f5add9b70432c40c152f7f19922e34
> src/slave/slave.cpp 1c0502e4b88719f18ecb0caface17b8511e9257b
> src/slave/state.hpp 78b20ffae1f2f629d851784302648e3bf5349321
> src/slave/state.cpp 6c382cd30602ede60b7d526acc272adc16285a93
> src/slave/status_update_manager.hpp 06ea4659cdd24cb1b82f389f404566ba14a663fb
> src/slave/status_update_manager.cpp 03f5eafefd6ed748bfd4511d654c23c7b460db66
>
> Diff: https://reviews.apache.org/r/16147/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 16147: Containerizer (part 1)
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16147/#review33866
-----------------------------------------------------------
Ship it!
src/launcher/fetcher.cpp
<https://reviews.apache.org/r/16147/#comment63529>
s/code/status/
src/launcher/fetcher.cpp
<https://reviews.apache.org/r/16147/#comment63530>
We use "exit status" to stay consistent with POSIX and libc, i.e., you check the status via WEXITSTATUS, not WEXITCODE. ;)
src/launcher/fetcher.cpp
<https://reviews.apache.org/r/16147/#comment63533>
s/uri/URI/
src/launcher/fetcher.cpp
<https://reviews.apache.org/r/16147/#comment63534>
s/Fetching uri/Fetching URI/
src/launcher/fetcher.cpp
<https://reviews.apache.org/r/16147/#comment63536>
I know this one isn't yours but if you see them please capitalize acronyms.
src/launcher/fetcher.cpp
<https://reviews.apache.org/r/16147/#comment63540>
Option<std::string> user = os::hasenv("MESOS_USER")
? Option<std::string>(os::getenv("MESOS_USER"))
: None();
Also, just to be clear, 'Some' doesn't work here?
src/launcher/fetcher.cpp
<https://reviews.apache.org/r/16147/#comment63539>
return 0 (this function returns an int so we should return something).
src/slave/containerizer/containerizer.hpp
<https://reviews.apache.org/r/16147/#comment63597>
IIUC, this has moved into mesos-fetcher, please kill.
src/slave/containerizer/containerizer.hpp
<https://reviews.apache.org/r/16147/#comment63598>
IIUC, this has moved into mesos-fetcher, please kill.
src/slave/containerizer/containerizer.hpp
<https://reviews.apache.org/r/16147/#comment63599>
IIUC, this has moved into mesos-fetcher, please kill.
src/slave/containerizer/containerizer.hpp
<https://reviews.apache.org/r/16147/#comment63600>
const &
src/slave/containerizer/containerizer.cpp
<https://reviews.apache.org/r/16147/#comment63601>
Please break the includes into directory "depths", even if it means more '#ifdef __linux__' are needed.
src/slave/containerizer/containerizer.cpp
<https://reviews.apache.org/r/16147/#comment63602>
I don't think this is used anymore.
src/slave/containerizer/containerizer.cpp
<https://reviews.apache.org/r/16147/#comment63604>
Moving it into the Containerizer makes the most sense to me, so feel free to kill these TODOs.
src/slave/containerizer/containerizer.cpp
<https://reviews.apache.org/r/16147/#comment63596>
s/flags.resources.isSome() ? flags.resources.get() : ""/flags.resources.get("")/
src/slave/containerizer/containerizer.cpp
<https://reviews.apache.org/r/16147/#comment63606>
The 'if' statement way of doing this before was far more readable.
src/slave/containerizer/containerizer.cpp
<https://reviews.apache.org/r/16147/#comment63621>
It's very non-standard to call a function on a libprocess directly (i.e., not dispatch). How about adding factory functions for each Isolator (i.e., Posix*Isolator::create(flags), Cgroups*Isolator::create(flags)) that returns a Try<Isolator*>? This would be an easy cleanup in the isolators. They could still use 'Isolator(Owned<IsolatorProcess>(process))'.
src/slave/containerizer/mesos_containerizer.hpp
<https://reviews.apache.org/r/16147/#comment63624>
virtual
src/slave/containerizer/mesos_containerizer.hpp
<https://reviews.apache.org/r/16147/#comment63625>
Please put all the instance variables below all the functions!
src/slave/containerizer/mesos_containerizer.hpp
<https://reviews.apache.org/r/16147/#comment63627>
This appears to have been killed?
src/slave/containerizer/mesos_containerizer.hpp
<https://reviews.apache.org/r/16147/#comment63629>
const&
src/slave/containerizer/mesos_containerizer.cpp
<https://reviews.apache.org/r/16147/#comment63630>
Please put this one after the other "slave/*" with a newline in between.
src/slave/containerizer/mesos_containerizer.cpp
<https://reviews.apache.org/r/16147/#comment63633>
There are some helpers in stout in fatal.hpp that were subsumed by EXIT. I think we could put an async signal safe 'fatal' there that could be shared by others. How about a TODO?
src/slave/containerizer/mesos_containerizer.cpp
<https://reviews.apache.org/r/16147/#comment63634>
Do you want to ignore EINTR?
src/slave/flags.hpp
<https://reviews.apache.org/r/16147/#comment63587>
s/be/been/
Also, please put the \n back.
src/slave/flags.hpp
<https://reviews.apache.org/r/16147/#comment63588>
If this is no longer accessed please make it an Option.
src/slave/monitor.cpp
<https://reviews.apache.org/r/16147/#comment63590>
Please indent +4 here.
src/slave/monitor.cpp
<https://reviews.apache.org/r/16147/#comment63593>
This should all fit on one line now.
src/slave/slave.cpp
<https://reviews.apache.org/r/16147/#comment63677>
Could you include a comment that explains the semantics around a failed or discarded future here? Are we guaranteed that container has been destroyed?
src/slave/containerizer/mesos_containerizer.hpp
<https://reviews.apache.org/r/16147/#comment63659>
const &
src/slave/containerizer/mesos_containerizer.cpp
<https://reviews.apache.org/r/16147/#comment63639>
I think it would be nice to replace stout's fatal with this async safe version since EXIT is a much better replacement in other circumstances. Maybe a TODO for now?
src/slave/containerizer/mesos_containerizer.cpp
<https://reviews.apache.org/r/16147/#comment63635>
What's with the '----'?
src/slave/containerizer/mesos_containerizer.cpp
<https://reviews.apache.org/r/16147/#comment63637>
I'm assuming that you're not including the error because the concatenation is not async signal safe? It would be great to comment on that at the top of the function. At the same time, calls like os::chown (and os::su, os::chdir, etc) likely do some potentially dangerous stuff (unless you checked, in which case comment on that too please), so concatenating the error message might not be such a big deal. The tradeoff here is that the process crashes, but it was already exiting anyway! And if I had to choose between the potential crash and some log message showing "Failed to chown work directory: No such user" versus just "Failed to chown work directory" the former would be that would be immensely more useful.
src/slave/containerizer/mesos_containerizer.cpp
<https://reviews.apache.org/r/16147/#comment63638>
s/Environment_Variable/Environment::Variable/
src/slave/containerizer/mesos_containerizer.cpp
<https://reviews.apache.org/r/16147/#comment63640>
Why not clean up and return a Failure?
src/slave/containerizer/mesos_containerizer.cpp
<https://reviews.apache.org/r/16147/#comment63642>
A small comment that you're assuming these rarely fail and therefore doing them in a check would be great. Also doesn't 'execute' above make sure it closes both ends of the pipe no matter what? What's this doing?
src/slave/containerizer/mesos_containerizer.cpp
<https://reviews.apache.org/r/16147/#comment63643>
If 'prepare' fails, do you clean up 'promises' and 'resources'? I think the semantics here are that the slave is expected to invoke 'destroy' if this future fails but it's not obvious so a comment would be good. Also, is there any reason why you didn't want to just do the destroy preemptively yourself?
src/slave/containerizer/mesos_containerizer.cpp
<https://reviews.apache.org/r/16147/#comment63644>
s/executor/URIs/
src/slave/containerizer/mesos_containerizer.cpp
<https://reviews.apache.org/r/16147/#comment63645>
s/executor/URIs/
src/slave/containerizer/mesos_containerizer.cpp
<https://reviews.apache.org/r/16147/#comment63646>
s/code/status/
src/slave/containerizer/mesos_containerizer.cpp
<https://reviews.apache.org/r/16147/#comment63647>
s/mesos-launcher/mesos-fetcher/
src/slave/containerizer/mesos_containerizer.cpp
<https://reviews.apache.org/r/16147/#comment63648>
s/-launcher/-fetcher/
src/slave/containerizer/mesos_containerizer.cpp
<https://reviews.apache.org/r/16147/#comment63649>
s/executor/URIs/
src/slave/containerizer/mesos_containerizer.cpp
<https://reviews.apache.org/r/16147/#comment63650>
Not your bug but the indentation here is incorrect.
src/slave/containerizer/mesos_containerizer.cpp
<https://reviews.apache.org/r/16147/#comment63651>
s|env|/usr/bin/env|
src/slave/containerizer/mesos_containerizer.cpp
<https://reviews.apache.org/r/16147/#comment63652>
Subprocess should really take a map of environment variables. ;)
src/slave/containerizer/mesos_containerizer.cpp
<https://reviews.apache.org/r/16147/#comment63653>
You're leaking a file descriptor.
src/slave/containerizer/mesos_containerizer.cpp
<https://reviews.apache.org/r/16147/#comment63654>
You're leaking a file descriptor.
src/slave/containerizer/mesos_containerizer.cpp
<https://reviews.apache.org/r/16147/#comment63655>
Leaking a file descriptor.
src/slave/containerizer/mesos_containerizer.cpp
<https://reviews.apache.org/r/16147/#comment63656>
Leaking a file descriptor.
src/slave/containerizer/mesos_containerizer.cpp
<https://reviews.apache.org/r/16147/#comment63657>
s/out/err/
src/slave/containerizer/mesos_containerizer.cpp
<https://reviews.apache.org/r/16147/#comment63658>
Should 'statuses' be cleaned up? Perhaps this relates to my cleanup concerns above?
src/slave/containerizer/mesos_containerizer.cpp
<https://reviews.apache.org/r/16147/#comment63660>
Ignore EINTR?
src/slave/containerizer/mesos_containerizer.cpp
<https://reviews.apache.org/r/16147/#comment63661>
Include string error of errno?
src/slave/containerizer/mesos_containerizer.cpp
<https://reviews.apache.org/r/16147/#comment63662>
s/.get()->/->/
src/slave/containerizer/mesos_containerizer.cpp
<https://reviews.apache.org/r/16147/#comment63663>
Why is this one discarded but the others are failed?
src/slave/containerizer/mesos_containerizer.cpp
<https://reviews.apache.org/r/16147/#comment63664>
This seems serious enough to have a JIRA ticket to follow up on this.
src/slave/containerizer/mesos_containerizer.cpp
<https://reviews.apache.org/r/16147/#comment63668>
& (I guess I missed it above.)
src/slave/containerizer/mesos_containerizer.cpp
<https://reviews.apache.org/r/16147/#comment63676>
This seems like a scary place to be ... stuff is definitely "leaking" here right? Also, did you want to clean up 'promises', 'statuses', etc.?
src/slave/containerizer/mesos_containerizer.cpp
<https://reviews.apache.org/r/16147/#comment63665>
s/.get()->/->/
src/slave/containerizer/mesos_containerizer.cpp
<https://reviews.apache.org/r/16147/#comment63666>
Why "unknown error" instead of "future discarded" or "discarded future"?
src/slave/containerizer/mesos_containerizer.cpp
<https://reviews.apache.org/r/16147/#comment63675>
The call to Isolator::watch is always going to require another thread to run 'watch' so even if the limitation is ready it's highly unlikely the future will be ready. I think you're better off only having the limitation message if 'limited' gets invoked below (and saving it until '__destroy' gets invoked).
src/slave/containerizer/mesos_containerizer.cpp
<https://reviews.apache.org/r/16147/#comment63672>
s/.get()->/->/
src/slave/containerizer/mesos_containerizer.cpp
<https://reviews.apache.org/r/16147/#comment63673>
Kill this. ;)
- Benjamin Hindman
On Feb. 7, 2014, 3:35 a.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16147/
> -----------------------------------------------------------
>
> (Updated Feb. 7, 2014, 3:35 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, Chi Zhang, Niklas Nielsen, samya, and Jason Dusek.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> The proposed Containerizer interface is to replace the existing Isolator.
>
> One ContainerizerProcess has been written:
> MesosContainerizerProcess - implements containerizeration internally using a Launcher and one or more Isolators (following review)
>
> The intent is to also support a generic ExternalContainerizerProcess that can delegate containerizeration by making external calls. Other Containerizers could interface with specific external containerization techniques such as Docker or LXC.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 1503e73f0d299f8efb209e0b10966c9a7f8db5f8
> src/Makefile.am c30706846bca1fa3287291e39f46a23713ad1ba4
> src/common/type_utils.hpp fe6bf71d689f7bfd8b6ae1b8fab9b2e76e28e7a8
> src/launcher/fetcher.cpp PRE-CREATION
> src/launcher/launcher.hpp 104fe812ddd2b75b14ec0b40d192f58cdba3603a
> src/launcher/launcher.cpp d5ab66704429a95eeb8eda5188e33d8e691221af
> src/launcher/main.cpp de64609905ee63096c0173fe7e64a1eafea5d6bf
> src/local/local.cpp 83a7f913afb1441e9137c7fcec8fd81815714905
> src/slave/cgroups_isolator.hpp 1a66dc630f0857db48b7cfb316fbbb15518fcec2
> src/slave/cgroups_isolator.cpp ef7dd682e5462d0158d6ea6654246d77000e9778
> src/slave/containerizer/containerizer.hpp PRE-CREATION
> src/slave/containerizer/containerizer.cpp PRE-CREATION
> src/slave/containerizer/mesos_containerizer.hpp PRE-CREATION
> src/slave/containerizer/mesos_containerizer.cpp PRE-CREATION
> src/slave/flags.hpp 827b2d0d6dc8fa279f3187a09e5dc6d4799d17cd
> src/slave/http.cpp c4f598faf6807214608cc89a6d9cf665133f95f3
> src/slave/isolator.hpp 9634535d8c746597b4bb6e278587a1b9ca8f1608
> src/slave/isolator.cpp c9643cf9c5657bc142482a71fb161233bffb3b9f
> src/slave/main.cpp 6a7131635218181bd85beb7f097c2718f16fa734
> src/slave/monitor.hpp b677410e20eeafa7eac9acb4e687af7c8d53809f
> src/slave/monitor.cpp bb3723e967b0723bc2b925019b6e9c4170dcc071
> src/slave/paths.hpp 70ee0f31ebb294011e5ec6b05ce3ad28b2c4bacc
> src/slave/process_isolator.hpp bc52f33d8e83fe026be712c8b15e689eb23dd65c
> src/slave/process_isolator.cpp 09cb9968c914a095b5737b70bf5044865fc3a2c4
> src/slave/slave.hpp 891c874397f5add9b70432c40c152f7f19922e34
> src/slave/slave.cpp 1c0502e4b88719f18ecb0caface17b8511e9257b
> src/slave/state.hpp 78b20ffae1f2f629d851784302648e3bf5349321
> src/slave/state.cpp 6c382cd30602ede60b7d526acc272adc16285a93
> src/slave/status_update_manager.hpp 06ea4659cdd24cb1b82f389f404566ba14a663fb
> src/slave/status_update_manager.cpp 03f5eafefd6ed748bfd4511d654c23c7b460db66
>
> Diff: https://reviews.apache.org/r/16147/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 16147: Containerizer (part 1)
Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16147/
-----------------------------------------------------------
(Updated Feb. 7, 2014, 3:35 a.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, Chi Zhang, Niklas Nielsen, samya, and Jason Dusek.
Changes
-------
Addressed Vinod's comments.
Repository: mesos-git
Description
-------
The proposed Containerizer interface is to replace the existing Isolator.
One ContainerizerProcess has been written:
MesosContainerizerProcess - implements containerizeration internally using a Launcher and one or more Isolators (following review)
The intent is to also support a generic ExternalContainerizerProcess that can delegate containerizeration by making external calls. Other Containerizers could interface with specific external containerization techniques such as Docker or LXC.
Diffs (updated)
-----
include/mesos/mesos.proto 1503e73f0d299f8efb209e0b10966c9a7f8db5f8
src/Makefile.am c30706846bca1fa3287291e39f46a23713ad1ba4
src/common/type_utils.hpp fe6bf71d689f7bfd8b6ae1b8fab9b2e76e28e7a8
src/launcher/fetcher.cpp PRE-CREATION
src/launcher/launcher.hpp 104fe812ddd2b75b14ec0b40d192f58cdba3603a
src/launcher/launcher.cpp d5ab66704429a95eeb8eda5188e33d8e691221af
src/launcher/main.cpp de64609905ee63096c0173fe7e64a1eafea5d6bf
src/local/local.cpp 83a7f913afb1441e9137c7fcec8fd81815714905
src/slave/cgroups_isolator.hpp 1a66dc630f0857db48b7cfb316fbbb15518fcec2
src/slave/cgroups_isolator.cpp ef7dd682e5462d0158d6ea6654246d77000e9778
src/slave/containerizer/containerizer.hpp PRE-CREATION
src/slave/containerizer/containerizer.cpp PRE-CREATION
src/slave/containerizer/mesos_containerizer.hpp PRE-CREATION
src/slave/containerizer/mesos_containerizer.cpp PRE-CREATION
src/slave/flags.hpp 827b2d0d6dc8fa279f3187a09e5dc6d4799d17cd
src/slave/http.cpp c4f598faf6807214608cc89a6d9cf665133f95f3
src/slave/isolator.hpp 9634535d8c746597b4bb6e278587a1b9ca8f1608
src/slave/isolator.cpp c9643cf9c5657bc142482a71fb161233bffb3b9f
src/slave/main.cpp 6a7131635218181bd85beb7f097c2718f16fa734
src/slave/monitor.hpp b677410e20eeafa7eac9acb4e687af7c8d53809f
src/slave/monitor.cpp bb3723e967b0723bc2b925019b6e9c4170dcc071
src/slave/paths.hpp 70ee0f31ebb294011e5ec6b05ce3ad28b2c4bacc
src/slave/process_isolator.hpp bc52f33d8e83fe026be712c8b15e689eb23dd65c
src/slave/process_isolator.cpp 09cb9968c914a095b5737b70bf5044865fc3a2c4
src/slave/slave.hpp 891c874397f5add9b70432c40c152f7f19922e34
src/slave/slave.cpp 1c0502e4b88719f18ecb0caface17b8511e9257b
src/slave/state.hpp 78b20ffae1f2f629d851784302648e3bf5349321
src/slave/state.cpp 6c382cd30602ede60b7d526acc272adc16285a93
src/slave/status_update_manager.hpp 06ea4659cdd24cb1b82f389f404566ba14a663fb
src/slave/status_update_manager.cpp 03f5eafefd6ed748bfd4511d654c23c7b460db66
Diff: https://reviews.apache.org/r/16147/diff/
Testing
-------
Thanks,
Ian Downes
Re: Review Request 16147: Containerizer (part 1)
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16147/#review33867
-----------------------------------------------------------
Ship it!
src/launcher/fetcher.cpp
<https://reviews.apache.org/r/16147/#comment63535>
s/bool/Nothing/ ?
src/slave/containerizer/containerizer.cpp
<https://reviews.apache.org/r/16147/#comment63542>
can you confirm how the tests passed with this bug?
src/slave/containerizer/mesos_containerizer.cpp
<https://reviews.apache.org/r/16147/#comment63543>
match subprocess.
src/slave/containerizer/mesos_containerizer.cpp
<https://reviews.apache.org/r/16147/#comment63549>
put the error message in the Failure and kill logging here?
src/slave/containerizer/mesos_containerizer.cpp
<https://reviews.apache.org/r/16147/#comment63550>
ditto.
src/slave/containerizer/mesos_containerizer.cpp
<https://reviews.apache.org/r/16147/#comment63556>
include error.
src/slave/containerizer/mesos_containerizer.cpp
<https://reviews.apache.org/r/16147/#comment63557>
ditto.
src/slave/containerizer/mesos_containerizer.cpp
<https://reviews.apache.org/r/16147/#comment63558>
ditto.
src/slave/containerizer/mesos_containerizer.cpp
<https://reviews.apache.org/r/16147/#comment63559>
s/optionally/optional/
src/slave/flags.hpp
<https://reviews.apache.org/r/16147/#comment63537>
s/be/been/
src/slave/monitor.hpp
<https://reviews.apache.org/r/16147/#comment63538>
s/containeri/container/
- Vinod Kone
On Feb. 6, 2014, 5:34 a.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16147/
> -----------------------------------------------------------
>
> (Updated Feb. 6, 2014, 5:34 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, Chi Zhang, Niklas Nielsen, samya, and Jason Dusek.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> The proposed Containerizer interface is to replace the existing Isolator.
>
> One ContainerizerProcess has been written:
> MesosContainerizerProcess - implements containerizeration internally using a Launcher and one or more Isolators (following review)
>
> The intent is to also support a generic ExternalContainerizerProcess that can delegate containerizeration by making external calls. Other Containerizers could interface with specific external containerization techniques such as Docker or LXC.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 1503e73f0d299f8efb209e0b10966c9a7f8db5f8
> src/Makefile.am c30706846bca1fa3287291e39f46a23713ad1ba4
> src/common/type_utils.hpp fe6bf71d689f7bfd8b6ae1b8fab9b2e76e28e7a8
> src/launcher/fetcher.cpp PRE-CREATION
> src/launcher/launcher.hpp 104fe812ddd2b75b14ec0b40d192f58cdba3603a
> src/launcher/launcher.cpp d5ab66704429a95eeb8eda5188e33d8e691221af
> src/launcher/main.cpp de64609905ee63096c0173fe7e64a1eafea5d6bf
> src/local/local.cpp 83a7f913afb1441e9137c7fcec8fd81815714905
> src/slave/cgroups_isolator.hpp 1a66dc630f0857db48b7cfb316fbbb15518fcec2
> src/slave/cgroups_isolator.cpp ef7dd682e5462d0158d6ea6654246d77000e9778
> src/slave/containerizer/containerizer.hpp PRE-CREATION
> src/slave/containerizer/containerizer.cpp PRE-CREATION
> src/slave/containerizer/mesos_containerizer.hpp PRE-CREATION
> src/slave/containerizer/mesos_containerizer.cpp PRE-CREATION
> src/slave/flags.hpp 827b2d0d6dc8fa279f3187a09e5dc6d4799d17cd
> src/slave/http.cpp c4f598faf6807214608cc89a6d9cf665133f95f3
> src/slave/isolator.hpp 9634535d8c746597b4bb6e278587a1b9ca8f1608
> src/slave/isolator.cpp c9643cf9c5657bc142482a71fb161233bffb3b9f
> src/slave/main.cpp e0cae7b205c2599e05c4db990cc9c8e9e3673c37
> src/slave/monitor.hpp b677410e20eeafa7eac9acb4e687af7c8d53809f
> src/slave/monitor.cpp bb3723e967b0723bc2b925019b6e9c4170dcc071
> src/slave/paths.hpp 70ee0f31ebb294011e5ec6b05ce3ad28b2c4bacc
> src/slave/process_isolator.hpp bc52f33d8e83fe026be712c8b15e689eb23dd65c
> src/slave/process_isolator.cpp 09cb9968c914a095b5737b70bf5044865fc3a2c4
> src/slave/slave.hpp 891c874397f5add9b70432c40c152f7f19922e34
> src/slave/slave.cpp a97b1d531d05e63e8ebaa474f526c110f5d573d4
> src/slave/state.hpp 78b20ffae1f2f629d851784302648e3bf5349321
> src/slave/state.cpp 6c382cd30602ede60b7d526acc272adc16285a93
> src/slave/status_update_manager.hpp 06ea4659cdd24cb1b82f389f404566ba14a663fb
> src/slave/status_update_manager.cpp 03f5eafefd6ed748bfd4511d654c23c7b460db66
>
> Diff: https://reviews.apache.org/r/16147/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 16147: Containerizer (part 1)
Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16147/
-----------------------------------------------------------
(Updated Feb. 6, 2014, 5:34 a.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, Chi Zhang, Niklas Nielsen, samya, and Jason Dusek.
Changes
-------
Splice output from mesos-fetcher subprocess to log files in the executor work directory.
Repository: mesos-git
Description
-------
The proposed Containerizer interface is to replace the existing Isolator.
One ContainerizerProcess has been written:
MesosContainerizerProcess - implements containerizeration internally using a Launcher and one or more Isolators (following review)
The intent is to also support a generic ExternalContainerizerProcess that can delegate containerizeration by making external calls. Other Containerizers could interface with specific external containerization techniques such as Docker or LXC.
Diffs (updated)
-----
include/mesos/mesos.proto 1503e73f0d299f8efb209e0b10966c9a7f8db5f8
src/Makefile.am c30706846bca1fa3287291e39f46a23713ad1ba4
src/common/type_utils.hpp fe6bf71d689f7bfd8b6ae1b8fab9b2e76e28e7a8
src/launcher/fetcher.cpp PRE-CREATION
src/launcher/launcher.hpp 104fe812ddd2b75b14ec0b40d192f58cdba3603a
src/launcher/launcher.cpp d5ab66704429a95eeb8eda5188e33d8e691221af
src/launcher/main.cpp de64609905ee63096c0173fe7e64a1eafea5d6bf
src/local/local.cpp 83a7f913afb1441e9137c7fcec8fd81815714905
src/slave/cgroups_isolator.hpp 1a66dc630f0857db48b7cfb316fbbb15518fcec2
src/slave/cgroups_isolator.cpp ef7dd682e5462d0158d6ea6654246d77000e9778
src/slave/containerizer/containerizer.hpp PRE-CREATION
src/slave/containerizer/containerizer.cpp PRE-CREATION
src/slave/containerizer/mesos_containerizer.hpp PRE-CREATION
src/slave/containerizer/mesos_containerizer.cpp PRE-CREATION
src/slave/flags.hpp 827b2d0d6dc8fa279f3187a09e5dc6d4799d17cd
src/slave/http.cpp c4f598faf6807214608cc89a6d9cf665133f95f3
src/slave/isolator.hpp 9634535d8c746597b4bb6e278587a1b9ca8f1608
src/slave/isolator.cpp c9643cf9c5657bc142482a71fb161233bffb3b9f
src/slave/main.cpp e0cae7b205c2599e05c4db990cc9c8e9e3673c37
src/slave/monitor.hpp b677410e20eeafa7eac9acb4e687af7c8d53809f
src/slave/monitor.cpp bb3723e967b0723bc2b925019b6e9c4170dcc071
src/slave/paths.hpp 70ee0f31ebb294011e5ec6b05ce3ad28b2c4bacc
src/slave/process_isolator.hpp bc52f33d8e83fe026be712c8b15e689eb23dd65c
src/slave/process_isolator.cpp 09cb9968c914a095b5737b70bf5044865fc3a2c4
src/slave/slave.hpp 891c874397f5add9b70432c40c152f7f19922e34
src/slave/slave.cpp a97b1d531d05e63e8ebaa474f526c110f5d573d4
src/slave/state.hpp 78b20ffae1f2f629d851784302648e3bf5349321
src/slave/state.cpp 6c382cd30602ede60b7d526acc272adc16285a93
src/slave/status_update_manager.hpp 06ea4659cdd24cb1b82f389f404566ba14a663fb
src/slave/status_update_manager.cpp 03f5eafefd6ed748bfd4511d654c23c7b460db66
Diff: https://reviews.apache.org/r/16147/diff/
Testing
-------
Thanks,
Ian Downes
Re: Review Request 16147: Containerizer (part 1)
Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16147/
-----------------------------------------------------------
(Updated Feb. 1, 2014, 4:01 a.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, Chi Zhang, Niklas Nielsen, samya, and Jason Dusek.
Changes
-------
Rebase and some fetcher clean up.
Repository: mesos-git
Description
-------
The proposed Containerizer interface is to replace the existing Isolator.
One ContainerizerProcess has been written:
MesosContainerizerProcess - implements containerizeration internally using a Launcher and one or more Isolators (following review)
The intent is to also support a generic ExternalContainerizerProcess that can delegate containerizeration by making external calls. Other Containerizers could interface with specific external containerization techniques such as Docker or LXC.
Diffs (updated)
-----
include/mesos/mesos.proto 1503e73f0d299f8efb209e0b10966c9a7f8db5f8
src/Makefile.am c30706846bca1fa3287291e39f46a23713ad1ba4
src/common/type_utils.hpp fe6bf71d689f7bfd8b6ae1b8fab9b2e76e28e7a8
src/launcher/fetcher.cpp PRE-CREATION
src/launcher/launcher.hpp 104fe812ddd2b75b14ec0b40d192f58cdba3603a
src/launcher/launcher.cpp d5ab66704429a95eeb8eda5188e33d8e691221af
src/launcher/main.cpp de64609905ee63096c0173fe7e64a1eafea5d6bf
src/local/local.cpp 83a7f913afb1441e9137c7fcec8fd81815714905
src/slave/cgroups_isolator.hpp 1a66dc630f0857db48b7cfb316fbbb15518fcec2
src/slave/cgroups_isolator.cpp 690ae817a6c9ddda90395ffc38d0c93c47be7f1a
src/slave/containerizer/containerizer.hpp PRE-CREATION
src/slave/containerizer/containerizer.cpp PRE-CREATION
src/slave/containerizer/mesos_containerizer.hpp PRE-CREATION
src/slave/containerizer/mesos_containerizer.cpp PRE-CREATION
src/slave/flags.hpp 827b2d0d6dc8fa279f3187a09e5dc6d4799d17cd
src/slave/http.cpp c4f598faf6807214608cc89a6d9cf665133f95f3
src/slave/isolator.hpp 9634535d8c746597b4bb6e278587a1b9ca8f1608
src/slave/isolator.cpp c9643cf9c5657bc142482a71fb161233bffb3b9f
src/slave/main.cpp e0cae7b205c2599e05c4db990cc9c8e9e3673c37
src/slave/monitor.hpp b677410e20eeafa7eac9acb4e687af7c8d53809f
src/slave/monitor.cpp bb3723e967b0723bc2b925019b6e9c4170dcc071
src/slave/paths.hpp 70ee0f31ebb294011e5ec6b05ce3ad28b2c4bacc
src/slave/process_isolator.hpp bc52f33d8e83fe026be712c8b15e689eb23dd65c
src/slave/process_isolator.cpp 748d9c2e19b9545aa4b4959825399aeb5dc45469
src/slave/slave.hpp 891c874397f5add9b70432c40c152f7f19922e34
src/slave/slave.cpp a97b1d531d05e63e8ebaa474f526c110f5d573d4
src/slave/state.hpp 78b20ffae1f2f629d851784302648e3bf5349321
src/slave/state.cpp 6c382cd30602ede60b7d526acc272adc16285a93
src/slave/status_update_manager.hpp 06ea4659cdd24cb1b82f389f404566ba14a663fb
src/slave/status_update_manager.cpp 03f5eafefd6ed748bfd4511d654c23c7b460db66
Diff: https://reviews.apache.org/r/16147/diff/
Testing
-------
Thanks,
Ian Downes
Re: Review Request 16147: Containerizer (part 1)
Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16147/
-----------------------------------------------------------
(Updated Jan. 31, 2014, 11:51 p.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, Chi Zhang, Niklas Nielsen, samya, and Jason Dusek.
Changes
-------
Addressed Vinod's comments.
Repository: mesos-git
Description
-------
The proposed Containerizer interface is to replace the existing Isolator.
One ContainerizerProcess has been written:
MesosContainerizerProcess - implements containerizeration internally using a Launcher and one or more Isolators (following review)
The intent is to also support a generic ExternalContainerizerProcess that can delegate containerizeration by making external calls. Other Containerizers could interface with specific external containerization techniques such as Docker or LXC.
Diffs (updated)
-----
3rdparty/libprocess/3rdparty/stout/Makefile.am 5d5a76021900bf27113fe5b730cfd2b6a537d132
3rdparty/libprocess/3rdparty/stout/include/stout/format.hpp 3eadaefad1945a2cf55ed67e5ee02eaffce76fea
3rdparty/libprocess/3rdparty/stout/include/stout/list.hpp 60f0c8c67317872883857a1cd8f39d6d6a45d0c1
3rdparty/libprocess/3rdparty/stout/include/stout/strings.hpp 08428b8f38f08674743ec0d07ec53bee8649fadd
3rdparty/libprocess/Makefile.am 40f01a7b3803696ccca440c8326e1d6d7c377459
3rdparty/libprocess/include/process/statistics.hpp a4f1db3a8a219c39193a1d237477f0350e47e681
3rdparty/libprocess/include/process/timeseries.hpp b368b9bb9cd63fb331b2c137dcde759d0ee59ee6
3rdparty/libprocess/src/process.cpp bc7a1c5890df7c763b0b140b47966e457e519208
3rdparty/libprocess/src/statistics.cpp 75aac4074d33cb5054da6c8b0bd4a890c2eaf80e
3rdparty/libprocess/src/tests/statistics_tests.cpp 3521bd565dae8fcbba464f2539b3b14a37a037f0
3rdparty/libprocess/src/tests/timeseries_tests.cpp fd906b869e8bbf5c07c67c5319dec82681e811b7
CHANGELOG 905c48996c37b33253deb00c156682fd1b14eedf
configure.ac aa6ee45bc477dd81b6f0cb389ef9f16f8216d6df
docs/home.md df8e05a1aed4040325ae4258f05f7cbda6dcdb8e
docs/logging-and-debugging.md 1f037d6cdbc9e76976a796961c300d8842a16601
docs/powered-by-mesos.md 6df9490c3df28f2097927d3e707a62099fe2b460
docs/upgrades.md dac0ef8ac62e97e6dc88b0e1a42e042a8d775f0c
include/mesos/mesos.proto 1503e73f0d299f8efb209e0b10966c9a7f8db5f8
src/Makefile.am d58b46e99e0a041cf2a26abe44bbd1504a9539c0
src/common/type_utils.hpp fe6bf71d689f7bfd8b6ae1b8fab9b2e76e28e7a8
src/launcher/fetcher.cpp PRE-CREATION
src/launcher/launcher.hpp 104fe812ddd2b75b14ec0b40d192f58cdba3603a
src/launcher/launcher.cpp d5ab66704429a95eeb8eda5188e33d8e691221af
src/launcher/main.cpp de64609905ee63096c0173fe7e64a1eafea5d6bf
src/local/local.cpp 83a7f913afb1441e9137c7fcec8fd81815714905
src/log/recover.cpp ee38acd280fab53b8584f68ea9dfbb496820d022
src/master/constants.hpp cdaaad060d4ee777f8b0838b63c0fd031da861ea
src/master/constants.cpp 8a48bbbe1d452919562b21f9b73680c42271a207
src/master/contender.hpp 0048ee026afc7034a09dd71f0a65c0fb55e8c77f
src/master/contender.cpp e3b0737195aba42c805a05c96b054cec1471b502
src/master/detector.hpp 277c9d907120a9ba30948d8ab4baaa2586293a62
src/master/detector.cpp 2b169c551affe7acd2feac7806a27b46eb99bb88
src/master/master.hpp 99b81811d596a777ee83dce7b157c1b11c064fab
src/master/master.cpp c7d9186b9c22fb40b05aa6d5bc6f2d38fb7f73ea
src/slave/cgroups_isolator.hpp e86062e9abaaa263c32c55e9dbfefd700f605886
src/slave/cgroups_isolator.cpp 80155a3f0cd684a6aad9a37f61c5204d86032735
src/slave/containerizer/containerizer.hpp PRE-CREATION
src/slave/containerizer/containerizer.cpp PRE-CREATION
src/slave/containerizer/mesos_containerizer.hpp PRE-CREATION
src/slave/containerizer/mesos_containerizer.cpp PRE-CREATION
src/slave/flags.hpp 827b2d0d6dc8fa279f3187a09e5dc6d4799d17cd
src/slave/http.cpp c8357e214d2adf2cd712072f58d07b07badb79dc
src/slave/isolator.hpp 9634535d8c746597b4bb6e278587a1b9ca8f1608
src/slave/isolator.cpp c9643cf9c5657bc142482a71fb161233bffb3b9f
src/slave/main.cpp e0cae7b205c2599e05c4db990cc9c8e9e3673c37
src/slave/monitor.hpp b677410e20eeafa7eac9acb4e687af7c8d53809f
src/slave/monitor.cpp bb3723e967b0723bc2b925019b6e9c4170dcc071
src/slave/paths.hpp 70ee0f31ebb294011e5ec6b05ce3ad28b2c4bacc
src/slave/process_isolator.hpp 4ae093fe65775a2b9bec42071961dd58aa0c3d8b
src/slave/process_isolator.cpp 0bc698f04f7c8eaad166dc9d646e13310129dd01
src/slave/slave.hpp b8acc1866353f4863aee5e2d3db4ac1f9d2a48a5
src/slave/slave.cpp aeecca0b8631c4b947d505a99d705f909d5986f7
src/slave/state.hpp 78b20ffae1f2f629d851784302648e3bf5349321
src/slave/state.cpp 6c382cd30602ede60b7d526acc272adc16285a93
src/slave/status_update_manager.hpp 06ea4659cdd24cb1b82f389f404566ba14a663fb
src/slave/status_update_manager.cpp 03f5eafefd6ed748bfd4511d654c23c7b460db66
src/tests/group_tests.cpp 5d4240c5fb32db97a8a0920d90af5400a0900dfd
src/tests/log_tests.cpp e493af4f2f2435efe168d07acd267b61afd37fe4
src/tests/master_contender_detector_tests.cpp 5223200ee59b9ce0e1d228320e8ad9e84a567288
src/tests/master_tests.cpp f1486cecec2fdc1d56624de150bc8cb18df76b65
src/tests/monitor_tests.cpp 7988c90b4b383bd822d1294a7f0318fbd9160dc6
src/tests/slave_recovery_tests.cpp 999e59857fd5a02de24bd15c9253d7a42df816ac
src/tests/zookeeper_tests.cpp 615338adf2a66bdf2d8c48e63783f054612bb958
src/webui/master/static/js/controllers.js afb24fb9c2184772f7314162f5637dbabaa2ab94
src/webui/master/static/js/services.js 8403aad9b2586ac3c1bea3f75ca38f6f907087a2
src/webui/master/static/slave.html 09bfcf29a700168938f4ec4fa42ca8383fa55511
src/webui/master/static/slave_executor.html 7c66405090f46f89bdd29806a58c05dc76c0ad23
src/webui/master/static/slave_framework.html 90eff2f3b4de08f98283b27e1635ba23a855e488
src/zookeeper/contender.hpp 65292450d8c800d3e6a7e8e4db7c8d48bc79a79d
src/zookeeper/contender.cpp 6710da4e64fc0a43c1eabfc0f39fb0133c13df14
src/zookeeper/detector.hpp 73235c0f2a339e27fa86f3bcb5e9a7a08e96f7f8
src/zookeeper/detector.cpp e186e5110341dd2fb94ddf50fed1b2819f70afb4
src/zookeeper/group.hpp e51ebb2cf5f09a633462c101f913ee8272be9a6c
src/zookeeper/group.cpp ecb6c002e8194b8d67e262826d988f747414f9f3
src/zookeeper/zookeeper.hpp f50aca6e7035c8084c3e76fd56b9d1ef7f9d9902
support/tag.sh 3977f4fce0200069f2265bfd8a03a611c2fa1849
Diff: https://reviews.apache.org/r/16147/diff/
Testing
-------
Thanks,
Ian Downes
Re: Review Request 16147: Containerizer (part 1)
Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16147/
-----------------------------------------------------------
(Updated Jan. 27, 2014, 3 a.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, Chi Zhang, Niklas Nielsen, samya, and Jason Dusek.
Changes
-------
Addressed almost all comments, including:
1. Fixed destroy flow to ensure Termination is only set when container is completely destroyed.
2. Moved Termination struct inside Containerizer.
3. Executor is fetched using an external binary, mesos-fetcher. This will be made asynchronous using Subprocess.
4. Reverted use of semaphore to pipes for synchronizing child processes.
Repository: mesos-git
Description
-------
The proposed Containerizer interface is to replace the existing Isolator.
One ContainerizerProcess has been written:
MesosContainerizerProcess - implements containerizeration internally using a Launcher and one or more Isolators (following review)
The intent is to also support a generic ExternalContainerizerProcess that can delegate containerizeration by making external calls. Other Containerizers could interface with specific external containerization techniques such as Docker or LXC.
Diffs (updated)
-----
include/mesos/mesos.proto 655f86757487ddbe551fdcf53eb793e773ecdd34
src/Makefile.am cf0c8c66e8dd21c2f5a2da22e5d5adb056353454
src/common/type_utils.hpp 3b05751a9a27055dd19e4abb79197f53e5554fe1
src/launcher/fetcher.cpp PRE-CREATION
src/launcher/launcher.hpp 104fe812ddd2b75b14ec0b40d192f58cdba3603a
src/launcher/launcher.cpp d5ab66704429a95eeb8eda5188e33d8e691221af
src/launcher/main.cpp de64609905ee63096c0173fe7e64a1eafea5d6bf
src/local/local.cpp 83a7f913afb1441e9137c7fcec8fd81815714905
src/slave/cgroups_isolator.hpp e86062e9abaaa263c32c55e9dbfefd700f605886
src/slave/cgroups_isolator.cpp 80155a3f0cd684a6aad9a37f61c5204d86032735
src/slave/containerizer/containerizer.hpp PRE-CREATION
src/slave/containerizer/containerizer.cpp PRE-CREATION
src/slave/containerizer/mesos_containerizer.hpp PRE-CREATION
src/slave/containerizer/mesos_containerizer.cpp PRE-CREATION
src/slave/flags.hpp 827b2d0d6dc8fa279f3187a09e5dc6d4799d17cd
src/slave/http.cpp c8357e214d2adf2cd712072f58d07b07badb79dc
src/slave/isolator.hpp 9634535d8c746597b4bb6e278587a1b9ca8f1608
src/slave/isolator.cpp c9643cf9c5657bc142482a71fb161233bffb3b9f
src/slave/main.cpp e0cae7b205c2599e05c4db990cc9c8e9e3673c37
src/slave/monitor.hpp 52568ad8ec566f7cf36c249c76d798d44eacb578
src/slave/monitor.cpp a931c4f35a8793c66ee03de82f0e0a21b92f8ffa
src/slave/paths.hpp b1a56a346ca84e7e71c31386602264c5097ef1cf
src/slave/process_isolator.hpp 4ae093fe65775a2b9bec42071961dd58aa0c3d8b
src/slave/process_isolator.cpp 0bc698f04f7c8eaad166dc9d646e13310129dd01
src/slave/slave.hpp 38ba7b6d6c35f0668d2f021a1285363cbf4367f4
src/slave/slave.cpp 8b83dacad802a5434bf76ca075b2b8113ed14a84
src/slave/state.hpp 78b20ffae1f2f629d851784302648e3bf5349321
src/slave/state.cpp 93cde0c2efbbe5387448631a02e64b20a3d021c1
src/slave/status_update_manager.hpp 06ea4659cdd24cb1b82f389f404566ba14a663fb
src/slave/status_update_manager.cpp 03f5eafefd6ed748bfd4511d654c23c7b460db66
Diff: https://reviews.apache.org/r/16147/diff/
Testing
-------
Thanks,
Ian Downes
Re: Review Request 16147: Containerizer (part 1)
Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16147/
-----------------------------------------------------------
(Updated Jan. 26, 2014, 8:18 a.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, Chi Zhang, Niklas Nielsen, samya, and Jason Dusek.
Repository: mesos-git
Description
-------
The proposed Containerizer interface is to replace the existing Isolator.
One ContainerizerProcess has been written:
MesosContainerizerProcess - implements containerizeration internally using a Launcher and one or more Isolators (following review)
The intent is to also support a generic ExternalContainerizerProcess that can delegate containerizeration by making external calls. Other Containerizers could interface with specific external containerization techniques such as Docker or LXC.
Diffs
-----
include/mesos/mesos.proto 655f86757487ddbe551fdcf53eb793e773ecdd34
src/Makefile.am cf0c8c66e8dd21c2f5a2da22e5d5adb056353454
src/common/type_utils.hpp 3b05751a9a27055dd19e4abb79197f53e5554fe1
src/launcher/launcher.hpp 104fe812ddd2b75b14ec0b40d192f58cdba3603a
src/launcher/launcher.cpp d5ab66704429a95eeb8eda5188e33d8e691221af
src/launcher/main.cpp de64609905ee63096c0173fe7e64a1eafea5d6bf
src/local/local.cpp 83a7f913afb1441e9137c7fcec8fd81815714905
src/slave/cgroups_isolator.hpp e86062e9abaaa263c32c55e9dbfefd700f605886
src/slave/cgroups_isolator.cpp 80155a3f0cd684a6aad9a37f61c5204d86032735
src/slave/container/containerizer.hpp PRE-CREATION
src/slave/container/containerizer.cpp PRE-CREATION
src/slave/container/mesos_containerizer.hpp PRE-CREATION
src/slave/container/mesos_containerizer.cpp PRE-CREATION
src/slave/flags.hpp 827b2d0d6dc8fa279f3187a09e5dc6d4799d17cd
src/slave/http.cpp c8357e214d2adf2cd712072f58d07b07badb79dc
src/slave/isolator.hpp 9634535d8c746597b4bb6e278587a1b9ca8f1608
src/slave/isolator.cpp c9643cf9c5657bc142482a71fb161233bffb3b9f
src/slave/main.cpp e0cae7b205c2599e05c4db990cc9c8e9e3673c37
src/slave/monitor.hpp 52568ad8ec566f7cf36c249c76d798d44eacb578
src/slave/monitor.cpp a931c4f35a8793c66ee03de82f0e0a21b92f8ffa
src/slave/paths.hpp b1a56a346ca84e7e71c31386602264c5097ef1cf
src/slave/process_isolator.hpp 4ae093fe65775a2b9bec42071961dd58aa0c3d8b
src/slave/process_isolator.cpp 0bc698f04f7c8eaad166dc9d646e13310129dd01
src/slave/slave.hpp 38ba7b6d6c35f0668d2f021a1285363cbf4367f4
src/slave/slave.cpp 8b83dacad802a5434bf76ca075b2b8113ed14a84
src/slave/state.hpp 78b20ffae1f2f629d851784302648e3bf5349321
src/slave/state.cpp 93cde0c2efbbe5387448631a02e64b20a3d021c1
src/slave/status_update_manager.hpp 06ea4659cdd24cb1b82f389f404566ba14a663fb
src/slave/status_update_manager.cpp 03f5eafefd6ed748bfd4511d654c23c7b460db66
Diff: https://reviews.apache.org/r/16147/diff/
Testing
-------
Thanks,
Ian Downes
Re: Review Request 16147: Containerizer (part 1)
Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16147/
-----------------------------------------------------------
(Updated Jan. 24, 2014, 6:25 p.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, Chi Zhang, Niklas Nielsen, samya, and Jason Dusek.
Changes
-------
Adressed some comments.
Repository: mesos-git
Description
-------
The proposed Containerizer interface is to replace the existing Isolator.
One ContainerizerProcess has been written:
MesosContainerizerProcess - implements containerizeration internally using a Launcher and one or more Isolators (following review)
The intent is to also support a generic ExternalContainerizerProcess that can delegate containerizeration by making external calls. Other Containerizers could interface with specific external containerization techniques such as Docker or LXC.
Diffs (updated)
-----
include/mesos/mesos.proto 655f86757487ddbe551fdcf53eb793e773ecdd34
src/Makefile.am cf0c8c66e8dd21c2f5a2da22e5d5adb056353454
src/common/type_utils.hpp 3b05751a9a27055dd19e4abb79197f53e5554fe1
src/launcher/launcher.hpp 104fe812ddd2b75b14ec0b40d192f58cdba3603a
src/launcher/launcher.cpp d5ab66704429a95eeb8eda5188e33d8e691221af
src/launcher/main.cpp de64609905ee63096c0173fe7e64a1eafea5d6bf
src/local/local.cpp 83a7f913afb1441e9137c7fcec8fd81815714905
src/slave/cgroups_isolator.hpp e86062e9abaaa263c32c55e9dbfefd700f605886
src/slave/cgroups_isolator.cpp 80155a3f0cd684a6aad9a37f61c5204d86032735
src/slave/container/containerizer.hpp PRE-CREATION
src/slave/container/containerizer.cpp PRE-CREATION
src/slave/container/mesos_containerizer.hpp PRE-CREATION
src/slave/container/mesos_containerizer.cpp PRE-CREATION
src/slave/flags.hpp 827b2d0d6dc8fa279f3187a09e5dc6d4799d17cd
src/slave/http.cpp c8357e214d2adf2cd712072f58d07b07badb79dc
src/slave/isolator.hpp 9634535d8c746597b4bb6e278587a1b9ca8f1608
src/slave/isolator.cpp c9643cf9c5657bc142482a71fb161233bffb3b9f
src/slave/main.cpp e0cae7b205c2599e05c4db990cc9c8e9e3673c37
src/slave/monitor.hpp 52568ad8ec566f7cf36c249c76d798d44eacb578
src/slave/monitor.cpp a931c4f35a8793c66ee03de82f0e0a21b92f8ffa
src/slave/paths.hpp b1a56a346ca84e7e71c31386602264c5097ef1cf
src/slave/process_isolator.hpp 4ae093fe65775a2b9bec42071961dd58aa0c3d8b
src/slave/process_isolator.cpp 0bc698f04f7c8eaad166dc9d646e13310129dd01
src/slave/slave.hpp 38ba7b6d6c35f0668d2f021a1285363cbf4367f4
src/slave/slave.cpp 8b83dacad802a5434bf76ca075b2b8113ed14a84
src/slave/state.hpp 78b20ffae1f2f629d851784302648e3bf5349321
src/slave/state.cpp 93cde0c2efbbe5387448631a02e64b20a3d021c1
src/slave/status_update_manager.hpp 06ea4659cdd24cb1b82f389f404566ba14a663fb
src/slave/status_update_manager.cpp 03f5eafefd6ed748bfd4511d654c23c7b460db66
Diff: https://reviews.apache.org/r/16147/diff/
Testing
-------
Thanks,
Ian Downes
Re: Review Request 16147: Containerizer (part 1)
Posted by Ian Downes <ia...@gmail.com>.
> On Jan. 22, 2014, 12:01 a.m., Niklas Nielsen wrote:
> > src/slave/container/mesos_containerizer.cpp, line 169
> > <https://reviews.apache.org/r/16147/diff/6/?file=425466#file425466line169>
> >
> > Is this async signal safe?
No, turns out it isn't so I'll reimplement this functionality with pipes.
- Ian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16147/#review32209
-----------------------------------------------------------
On Jan. 24, 2014, 6:25 p.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16147/
> -----------------------------------------------------------
>
> (Updated Jan. 24, 2014, 6:25 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, Chi Zhang, Niklas Nielsen, samya, and Jason Dusek.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> The proposed Containerizer interface is to replace the existing Isolator.
>
> One ContainerizerProcess has been written:
> MesosContainerizerProcess - implements containerizeration internally using a Launcher and one or more Isolators (following review)
>
> The intent is to also support a generic ExternalContainerizerProcess that can delegate containerizeration by making external calls. Other Containerizers could interface with specific external containerization techniques such as Docker or LXC.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 655f86757487ddbe551fdcf53eb793e773ecdd34
> src/Makefile.am cf0c8c66e8dd21c2f5a2da22e5d5adb056353454
> src/common/type_utils.hpp 3b05751a9a27055dd19e4abb79197f53e5554fe1
> src/launcher/launcher.hpp 104fe812ddd2b75b14ec0b40d192f58cdba3603a
> src/launcher/launcher.cpp d5ab66704429a95eeb8eda5188e33d8e691221af
> src/launcher/main.cpp de64609905ee63096c0173fe7e64a1eafea5d6bf
> src/local/local.cpp 83a7f913afb1441e9137c7fcec8fd81815714905
> src/slave/cgroups_isolator.hpp e86062e9abaaa263c32c55e9dbfefd700f605886
> src/slave/cgroups_isolator.cpp 80155a3f0cd684a6aad9a37f61c5204d86032735
> src/slave/container/containerizer.hpp PRE-CREATION
> src/slave/container/containerizer.cpp PRE-CREATION
> src/slave/container/mesos_containerizer.hpp PRE-CREATION
> src/slave/container/mesos_containerizer.cpp PRE-CREATION
> src/slave/flags.hpp 827b2d0d6dc8fa279f3187a09e5dc6d4799d17cd
> src/slave/http.cpp c8357e214d2adf2cd712072f58d07b07badb79dc
> src/slave/isolator.hpp 9634535d8c746597b4bb6e278587a1b9ca8f1608
> src/slave/isolator.cpp c9643cf9c5657bc142482a71fb161233bffb3b9f
> src/slave/main.cpp e0cae7b205c2599e05c4db990cc9c8e9e3673c37
> src/slave/monitor.hpp 52568ad8ec566f7cf36c249c76d798d44eacb578
> src/slave/monitor.cpp a931c4f35a8793c66ee03de82f0e0a21b92f8ffa
> src/slave/paths.hpp b1a56a346ca84e7e71c31386602264c5097ef1cf
> src/slave/process_isolator.hpp 4ae093fe65775a2b9bec42071961dd58aa0c3d8b
> src/slave/process_isolator.cpp 0bc698f04f7c8eaad166dc9d646e13310129dd01
> src/slave/slave.hpp 38ba7b6d6c35f0668d2f021a1285363cbf4367f4
> src/slave/slave.cpp 8b83dacad802a5434bf76ca075b2b8113ed14a84
> src/slave/state.hpp 78b20ffae1f2f629d851784302648e3bf5349321
> src/slave/state.cpp 93cde0c2efbbe5387448631a02e64b20a3d021c1
> src/slave/status_update_manager.hpp 06ea4659cdd24cb1b82f389f404566ba14a663fb
> src/slave/status_update_manager.cpp 03f5eafefd6ed748bfd4511d654c23c7b460db66
>
> Diff: https://reviews.apache.org/r/16147/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 16147: Containerizer (part 1)
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16147/#review32209
-----------------------------------------------------------
Ship it!
Good stuff! A couple of style nits and request for comments :)
src/Makefile.am
<https://reviews.apache.org/r/16147/#comment60924>
The '\''s should be aligned. The tabstop for Makefiles is 8.
src/Makefile.am
<https://reviews.apache.org/r/16147/#comment60925>
Again, can you indent the '\' to align with the others.
src/Makefile.am
<https://reviews.apache.org/r/16147/#comment60926>
And here.
src/Makefile.am
<https://reviews.apache.org/r/16147/#comment60927>
And here.
src/local/local.cpp
<https://reviews.apache.org/r/16147/#comment61053>
Wrap line.
src/local/local.cpp
<https://reviews.apache.org/r/16147/#comment61054>
Wrap line.
src/local/local.cpp
<https://reviews.apache.org/r/16147/#comment61055>
Wrap line.
src/slave/container/containerizer.hpp
<https://reviews.apache.org/r/16147/#comment61010>
s/class/struct/
src/slave/container/containerizer.hpp
<https://reviews.apache.org/r/16147/#comment60933>
Kill new line?
src/slave/container/containerizer.hpp
<https://reviews.apache.org/r/16147/#comment61235>
Can you merge this fetch() into the one above? Or are you using this fetch() somewhere else? At least the two sharing name and having different semantics was confused us a bit :)
src/slave/container/containerizer.hpp
<https://reviews.apache.org/r/16147/#comment60932>
Kill newline?
src/slave/container/containerizer.cpp
<https://reviews.apache.org/r/16147/#comment61240>
Can we skip this for now? We'll extend ::create() with the pluggable containerizer review.
src/slave/container/containerizer.cpp
<https://reviews.apache.org/r/16147/#comment61211>
Not yours, but can we wrap this way:
bool chmodded =
os::chmod(fetched.get(), S_IRWXU | S_IRGRP | S_IXGRP | S_IROTH | S_IXOTH);
or another way pointed out by the style guide?
src/slave/container/external_containerizer.hpp
<https://reviews.apache.org/r/16147/#comment61013>
Can we move ExternalContainerizer to its own review i.e. kill this file from this review?
src/slave/container/mesos_containerizer.cpp
<https://reviews.apache.org/r/16147/#comment61245>
Why is the semaphore optional?
src/slave/container/mesos_containerizer.cpp
<https://reviews.apache.org/r/16147/#comment61249>
Is this async signal safe?
src/slave/container/mesos_containerizer.cpp
<https://reviews.apache.org/r/16147/#comment61243>
Can you wrap and indent? So it becomes:
const map<string, string>& env = executorEnvironment(
executorInfo,
directory,
slaveId,
slavePid,
checkpoint);
src/slave/container/mesos_containerizer.cpp
<https://reviews.apache.org/r/16147/#comment61248>
Why a semaphore instead of a pipe?
src/slave/container/mesos_containerizer.cpp
<https://reviews.apache.org/r/16147/#comment61242>
Same wrap and indent here.
src/slave/container/mesos_containerizer.cpp
<https://reviews.apache.org/r/16147/#comment61250>
Can you add a comment on the flow below? This will also highlight your naming scheme.
src/slave/container/mesos_containerizer.cpp
<https://reviews.apache.org/r/16147/#comment61244>
Does it make sense to delegate reaping to the launcher?
src/slave/container/mesos_containerizer.cpp
<https://reviews.apache.org/r/16147/#comment60930>
The usual wrap is before ':'
src/slave/monitor.hpp
<https://reviews.apache.org/r/16147/#comment60928>
s/exectorInfo/executorInfo/
src/slave/slave.cpp
<https://reviews.apache.org/r/16147/#comment61213>
Mind change this to CopyFrom too?
src/slave/slave.cpp
<https://reviews.apache.org/r/16147/#comment61215>
Wrap this please.
src/slave/slave.cpp
<https://reviews.apache.org/r/16147/#comment61222>
Can you indent on new line? So it becomes:
const string& path = paths::getExecutorRunPath(
flags.work_dir,
info.id(),
framework->id,
executor->id,
executor->containerId);
src/slave/slave.cpp
<https://reviews.apache.org/r/16147/#comment61223>
Wrap please.
src/slave/slave.cpp
<https://reviews.apache.org/r/16147/#comment61225>
Indent as mentioned above.
src/slave/state.cpp
<https://reviews.apache.org/r/16147/#comment61228>
Not yours, but can you bring to new line and indent? So it becomes:
return Error(
"Failed to recover" ...
" of executor "' ...
src/slave/state.cpp
<https://reviews.apache.org/r/16147/#comment61056>
Wrap line.
src/slave/status_update_manager.cpp
<https://reviews.apache.org/r/16147/#comment61230>
Can you swap "->" with "therefore". Or otherwise spell the log string out?
- Niklas Nielsen
On Jan. 21, 2014, 11:36 a.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16147/
> -----------------------------------------------------------
>
> (Updated Jan. 21, 2014, 11:36 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, Chi Zhang, Niklas Nielsen, samya, and Jason Dusek.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> The proposed Containerizer interface is to replace the existing Isolator.
>
> One ContainerizerProcess has been written:
> MesosContainerizerProcess - implements containerizeration internally using a Launcher and one or more Isolators (following review)
>
> The intent is to also support a generic ExternalContainerizerProcess that can delegate containerizeration by making external calls. Other Containerizers could interface with specific external containerization techniques such as Docker or LXC.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 655f86757487ddbe551fdcf53eb793e773ecdd34
> src/Makefile.am cf0c8c66e8dd21c2f5a2da22e5d5adb056353454
> src/common/type_utils.hpp 3b05751a9a27055dd19e4abb79197f53e5554fe1
> src/launcher/launcher.hpp 104fe812ddd2b75b14ec0b40d192f58cdba3603a
> src/launcher/launcher.cpp d5ab66704429a95eeb8eda5188e33d8e691221af
> src/launcher/main.cpp de64609905ee63096c0173fe7e64a1eafea5d6bf
> src/local/local.cpp 83a7f913afb1441e9137c7fcec8fd81815714905
> src/slave/cgroups_isolator.hpp e86062e9abaaa263c32c55e9dbfefd700f605886
> src/slave/cgroups_isolator.cpp 80155a3f0cd684a6aad9a37f61c5204d86032735
> src/slave/container/containerizer.hpp PRE-CREATION
> src/slave/container/containerizer.cpp PRE-CREATION
> src/slave/container/external_containerizer.hpp PRE-CREATION
> src/slave/container/mesos_containerizer.hpp PRE-CREATION
> src/slave/container/mesos_containerizer.cpp PRE-CREATION
> src/slave/flags.hpp 827b2d0d6dc8fa279f3187a09e5dc6d4799d17cd
> src/slave/http.cpp c8357e214d2adf2cd712072f58d07b07badb79dc
> src/slave/isolator.hpp 9634535d8c746597b4bb6e278587a1b9ca8f1608
> src/slave/isolator.cpp c9643cf9c5657bc142482a71fb161233bffb3b9f
> src/slave/main.cpp e0cae7b205c2599e05c4db990cc9c8e9e3673c37
> src/slave/monitor.hpp 52568ad8ec566f7cf36c249c76d798d44eacb578
> src/slave/monitor.cpp a931c4f35a8793c66ee03de82f0e0a21b92f8ffa
> src/slave/paths.hpp b1a56a346ca84e7e71c31386602264c5097ef1cf
> src/slave/process_isolator.hpp 4ae093fe65775a2b9bec42071961dd58aa0c3d8b
> src/slave/process_isolator.cpp 0bc698f04f7c8eaad166dc9d646e13310129dd01
> src/slave/slave.hpp 38ba7b6d6c35f0668d2f021a1285363cbf4367f4
> src/slave/slave.cpp 8b83dacad802a5434bf76ca075b2b8113ed14a84
> src/slave/state.hpp 78b20ffae1f2f629d851784302648e3bf5349321
> src/slave/state.cpp 93cde0c2efbbe5387448631a02e64b20a3d021c1
> src/slave/status_update_manager.hpp 06ea4659cdd24cb1b82f389f404566ba14a663fb
> src/slave/status_update_manager.cpp 03f5eafefd6ed748bfd4511d654c23c7b460db66
>
> Diff: https://reviews.apache.org/r/16147/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 16147: Containerizer (part 1)
Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16147/
-----------------------------------------------------------
(Updated Jan. 21, 2014, 7:36 p.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, Chi Zhang, Niklas Nielsen, samya, and Jason Dusek.
Repository: mesos-git
Description
-------
The proposed Containerizer interface is to replace the existing Isolator.
One ContainerizerProcess has been written:
MesosContainerizerProcess - implements containerizeration internally using a Launcher and one or more Isolators (following review)
The intent is to also support a generic ExternalContainerizerProcess that can delegate containerizeration by making external calls. Other Containerizers could interface with specific external containerization techniques such as Docker or LXC.
Diffs
-----
include/mesos/mesos.proto 655f86757487ddbe551fdcf53eb793e773ecdd34
src/Makefile.am cf0c8c66e8dd21c2f5a2da22e5d5adb056353454
src/common/type_utils.hpp 3b05751a9a27055dd19e4abb79197f53e5554fe1
src/launcher/launcher.hpp 104fe812ddd2b75b14ec0b40d192f58cdba3603a
src/launcher/launcher.cpp d5ab66704429a95eeb8eda5188e33d8e691221af
src/launcher/main.cpp de64609905ee63096c0173fe7e64a1eafea5d6bf
src/local/local.cpp 83a7f913afb1441e9137c7fcec8fd81815714905
src/slave/cgroups_isolator.hpp e86062e9abaaa263c32c55e9dbfefd700f605886
src/slave/cgroups_isolator.cpp 80155a3f0cd684a6aad9a37f61c5204d86032735
src/slave/container/containerizer.hpp PRE-CREATION
src/slave/container/containerizer.cpp PRE-CREATION
src/slave/container/external_containerizer.hpp PRE-CREATION
src/slave/container/mesos_containerizer.hpp PRE-CREATION
src/slave/container/mesos_containerizer.cpp PRE-CREATION
src/slave/flags.hpp 827b2d0d6dc8fa279f3187a09e5dc6d4799d17cd
src/slave/http.cpp c8357e214d2adf2cd712072f58d07b07badb79dc
src/slave/isolator.hpp 9634535d8c746597b4bb6e278587a1b9ca8f1608
src/slave/isolator.cpp c9643cf9c5657bc142482a71fb161233bffb3b9f
src/slave/main.cpp e0cae7b205c2599e05c4db990cc9c8e9e3673c37
src/slave/monitor.hpp 52568ad8ec566f7cf36c249c76d798d44eacb578
src/slave/monitor.cpp a931c4f35a8793c66ee03de82f0e0a21b92f8ffa
src/slave/paths.hpp b1a56a346ca84e7e71c31386602264c5097ef1cf
src/slave/process_isolator.hpp 4ae093fe65775a2b9bec42071961dd58aa0c3d8b
src/slave/process_isolator.cpp 0bc698f04f7c8eaad166dc9d646e13310129dd01
src/slave/slave.hpp 38ba7b6d6c35f0668d2f021a1285363cbf4367f4
src/slave/slave.cpp 8b83dacad802a5434bf76ca075b2b8113ed14a84
src/slave/state.hpp 78b20ffae1f2f629d851784302648e3bf5349321
src/slave/state.cpp 93cde0c2efbbe5387448631a02e64b20a3d021c1
src/slave/status_update_manager.hpp 06ea4659cdd24cb1b82f389f404566ba14a663fb
src/slave/status_update_manager.cpp 03f5eafefd6ed748bfd4511d654c23c7b460db66
Diff: https://reviews.apache.org/r/16147/diff/
Testing
-------
Thanks,
Ian Downes
Re: Review Request 16147: Containerizer (part 1)
Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16147/
-----------------------------------------------------------
(Updated Jan. 21, 2014, 7:33 p.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, Chi Zhang, Niklas Nielsen, samya, and Jason Dusek.
Summary (updated)
-----------------
Containerizer (part 1)
Repository: mesos-git
Description
-------
The proposed Containerizer interface is to replace the existing Isolator.
One ContainerizerProcess has been written:
MesosContainerizerProcess - implements containerizeration internally using a Launcher and one or more Isolators (following review)
The intent is to also support a generic ExternalContainerizerProcess that can delegate containerizeration by making external calls. Other Containerizers could interface with specific external containerization techniques such as Docker or LXC.
Diffs
-----
include/mesos/mesos.proto 655f86757487ddbe551fdcf53eb793e773ecdd34
src/Makefile.am cf0c8c66e8dd21c2f5a2da22e5d5adb056353454
src/common/type_utils.hpp 3b05751a9a27055dd19e4abb79197f53e5554fe1
src/launcher/launcher.hpp 104fe812ddd2b75b14ec0b40d192f58cdba3603a
src/launcher/launcher.cpp d5ab66704429a95eeb8eda5188e33d8e691221af
src/launcher/main.cpp de64609905ee63096c0173fe7e64a1eafea5d6bf
src/local/local.cpp 83a7f913afb1441e9137c7fcec8fd81815714905
src/slave/cgroups_isolator.hpp e86062e9abaaa263c32c55e9dbfefd700f605886
src/slave/cgroups_isolator.cpp 80155a3f0cd684a6aad9a37f61c5204d86032735
src/slave/container/containerizer.hpp PRE-CREATION
src/slave/container/containerizer.cpp PRE-CREATION
src/slave/container/external_containerizer.hpp PRE-CREATION
src/slave/container/mesos_containerizer.hpp PRE-CREATION
src/slave/container/mesos_containerizer.cpp PRE-CREATION
src/slave/flags.hpp 827b2d0d6dc8fa279f3187a09e5dc6d4799d17cd
src/slave/http.cpp c8357e214d2adf2cd712072f58d07b07badb79dc
src/slave/isolator.hpp 9634535d8c746597b4bb6e278587a1b9ca8f1608
src/slave/isolator.cpp c9643cf9c5657bc142482a71fb161233bffb3b9f
src/slave/main.cpp e0cae7b205c2599e05c4db990cc9c8e9e3673c37
src/slave/monitor.hpp 52568ad8ec566f7cf36c249c76d798d44eacb578
src/slave/monitor.cpp a931c4f35a8793c66ee03de82f0e0a21b92f8ffa
src/slave/paths.hpp b1a56a346ca84e7e71c31386602264c5097ef1cf
src/slave/process_isolator.hpp 4ae093fe65775a2b9bec42071961dd58aa0c3d8b
src/slave/process_isolator.cpp 0bc698f04f7c8eaad166dc9d646e13310129dd01
src/slave/slave.hpp 38ba7b6d6c35f0668d2f021a1285363cbf4367f4
src/slave/slave.cpp 8b83dacad802a5434bf76ca075b2b8113ed14a84
src/slave/state.hpp 78b20ffae1f2f629d851784302648e3bf5349321
src/slave/state.cpp 93cde0c2efbbe5387448631a02e64b20a3d021c1
src/slave/status_update_manager.hpp 06ea4659cdd24cb1b82f389f404566ba14a663fb
src/slave/status_update_manager.cpp 03f5eafefd6ed748bfd4511d654c23c7b460db66
Diff: https://reviews.apache.org/r/16147/diff/
Testing
-------
Thanks,
Ian Downes
Re: Review Request 16147: Containerizer
Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16147/
-----------------------------------------------------------
(Updated Jan. 17, 2014, 1:36 a.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, Chi Zhang, Niklas Nielsen, samya, and Jason Dusek.
Repository: mesos-git
Description
-------
The proposed Containerizer interface is to replace the existing Isolator.
One ContainerizerProcess has been written:
MesosContainerizerProcess - implements containerizeration internally using a Launcher and one or more Isolators (following review)
The intent is to also support a generic ExternalContainerizerProcess that can delegate containerizeration by making external calls. Other Containerizers could interface with specific external containerization techniques such as Docker or LXC.
Diffs (updated)
-----
include/mesos/mesos.proto 655f86757487ddbe551fdcf53eb793e773ecdd34
src/Makefile.am cf0c8c66e8dd21c2f5a2da22e5d5adb056353454
src/common/type_utils.hpp 3b05751a9a27055dd19e4abb79197f53e5554fe1
src/launcher/launcher.hpp 104fe812ddd2b75b14ec0b40d192f58cdba3603a
src/launcher/launcher.cpp d5ab66704429a95eeb8eda5188e33d8e691221af
src/launcher/main.cpp de64609905ee63096c0173fe7e64a1eafea5d6bf
src/local/local.cpp 83a7f913afb1441e9137c7fcec8fd81815714905
src/slave/cgroups_isolator.hpp e86062e9abaaa263c32c55e9dbfefd700f605886
src/slave/cgroups_isolator.cpp 80155a3f0cd684a6aad9a37f61c5204d86032735
src/slave/container/containerizer.hpp PRE-CREATION
src/slave/container/containerizer.cpp PRE-CREATION
src/slave/container/external_containerizer.hpp PRE-CREATION
src/slave/container/mesos_containerizer.hpp PRE-CREATION
src/slave/container/mesos_containerizer.cpp PRE-CREATION
src/slave/flags.hpp 827b2d0d6dc8fa279f3187a09e5dc6d4799d17cd
src/slave/http.cpp c8357e214d2adf2cd712072f58d07b07badb79dc
src/slave/isolator.hpp 9634535d8c746597b4bb6e278587a1b9ca8f1608
src/slave/isolator.cpp c9643cf9c5657bc142482a71fb161233bffb3b9f
src/slave/main.cpp e0cae7b205c2599e05c4db990cc9c8e9e3673c37
src/slave/monitor.hpp 52568ad8ec566f7cf36c249c76d798d44eacb578
src/slave/monitor.cpp a931c4f35a8793c66ee03de82f0e0a21b92f8ffa
src/slave/paths.hpp b1a56a346ca84e7e71c31386602264c5097ef1cf
src/slave/process_isolator.hpp 4ae093fe65775a2b9bec42071961dd58aa0c3d8b
src/slave/process_isolator.cpp 0bc698f04f7c8eaad166dc9d646e13310129dd01
src/slave/slave.hpp 38ba7b6d6c35f0668d2f021a1285363cbf4367f4
src/slave/slave.cpp 8b83dacad802a5434bf76ca075b2b8113ed14a84
src/slave/state.hpp 78b20ffae1f2f629d851784302648e3bf5349321
src/slave/state.cpp 93cde0c2efbbe5387448631a02e64b20a3d021c1
src/slave/status_update_manager.hpp 06ea4659cdd24cb1b82f389f404566ba14a663fb
src/slave/status_update_manager.cpp 03f5eafefd6ed748bfd4511d654c23c7b460db66
Diff: https://reviews.apache.org/r/16147/diff/
Testing
-------
Thanks,
Ian Downes
Re: Review Request 16147: Containerizer
Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16147/
-----------------------------------------------------------
(Updated Jan. 17, 2014, 1:32 a.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, Chi Zhang, Niklas Nielsen, samya, and Jason Dusek.
Changes
-------
Rebased + various clean ups.
Repository: mesos-git
Description
-------
The proposed Containerizer interface is to replace the existing Isolator.
One ContainerizerProcess has been written:
MesosContainerizerProcess - implements containerizeration internally using a Launcher and one or more Isolators (following review)
The intent is to also support a generic ExternalContainerizerProcess that can delegate containerizeration by making external calls. Other Containerizers could interface with specific external containerization techniques such as Docker or LXC.
Diffs (updated)
-----
include/mesos/mesos.proto 655f86757487ddbe551fdcf53eb793e773ecdd34
src/Makefile.am cf0c8c66e8dd21c2f5a2da22e5d5adb056353454
src/common/type_utils.hpp 3b05751a9a27055dd19e4abb79197f53e5554fe1
src/launcher/launcher.hpp 104fe812ddd2b75b14ec0b40d192f58cdba3603a
src/launcher/launcher.cpp d5ab66704429a95eeb8eda5188e33d8e691221af
src/launcher/main.cpp de64609905ee63096c0173fe7e64a1eafea5d6bf
src/local/local.cpp 83a7f913afb1441e9137c7fcec8fd81815714905
src/slave/cgroups_isolator.hpp e86062e9abaaa263c32c55e9dbfefd700f605886
src/slave/cgroups_isolator.cpp 80155a3f0cd684a6aad9a37f61c5204d86032735
src/slave/container/containerizer.hpp PRE-CREATION
src/slave/container/containerizer.cpp PRE-CREATION
src/slave/container/external_containerizer.hpp PRE-CREATION
src/slave/container/mesos_containerizer.hpp PRE-CREATION
src/slave/container/mesos_containerizer.cpp PRE-CREATION
src/slave/flags.hpp 827b2d0d6dc8fa279f3187a09e5dc6d4799d17cd
src/slave/http.cpp c8357e214d2adf2cd712072f58d07b07badb79dc
src/slave/isolator.hpp 9634535d8c746597b4bb6e278587a1b9ca8f1608
src/slave/isolator.cpp c9643cf9c5657bc142482a71fb161233bffb3b9f
src/slave/main.cpp e0cae7b205c2599e05c4db990cc9c8e9e3673c37
src/slave/monitor.hpp 52568ad8ec566f7cf36c249c76d798d44eacb578
src/slave/monitor.cpp a931c4f35a8793c66ee03de82f0e0a21b92f8ffa
src/slave/paths.hpp b1a56a346ca84e7e71c31386602264c5097ef1cf
src/slave/process_isolator.hpp 4ae093fe65775a2b9bec42071961dd58aa0c3d8b
src/slave/process_isolator.cpp 0bc698f04f7c8eaad166dc9d646e13310129dd01
src/slave/slave.hpp 38ba7b6d6c35f0668d2f021a1285363cbf4367f4
src/slave/slave.cpp 8b83dacad802a5434bf76ca075b2b8113ed14a84
src/slave/state.hpp 78b20ffae1f2f629d851784302648e3bf5349321
src/slave/state.cpp 93cde0c2efbbe5387448631a02e64b20a3d021c1
src/slave/status_update_manager.hpp 06ea4659cdd24cb1b82f389f404566ba14a663fb
src/slave/status_update_manager.cpp 03f5eafefd6ed748bfd4511d654c23c7b460db66
Diff: https://reviews.apache.org/r/16147/diff/
Testing
-------
Thanks,
Ian Downes
Re: Review Request 16147: Containerizer
Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16147/
-----------------------------------------------------------
(Updated Dec. 21, 2013, 6:50 a.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, Chi Zhang, Niklas Nielsen, samya, and Jason Dusek.
Changes
-------
Fixed compilation on linux. Addressed Niklas' comments.
Repository: mesos-git
Description
-------
The proposed Containerizer interface is to replace the existing Isolator.
One ContainerizerProcess has been written:
MesosContainerizerProcess - implements containerizeration internally using a Launcher and one or more Isolators (following review)
The intent is to also support a generic ExternalContainerizerProcess that can delegate containerizeration by making external calls. Other Containerizers could interface with specific external containerization techniques such as Docker or LXC.
Diffs (updated)
-----
include/mesos/mesos.proto 655f86757487ddbe551fdcf53eb793e773ecdd34
src/Makefile.am 42dafbcd6e2d4f67d2705c5a2bea6ec6a0f4fcc1
src/common/type_utils.hpp 3b05751a9a27055dd19e4abb79197f53e5554fe1
src/launcher/launcher.hpp 104fe812ddd2b75b14ec0b40d192f58cdba3603a
src/launcher/launcher.cpp d5ab66704429a95eeb8eda5188e33d8e691221af
src/launcher/main.cpp de64609905ee63096c0173fe7e64a1eafea5d6bf
src/local/local.cpp 83a7f913afb1441e9137c7fcec8fd81815714905
src/slave/cgroups_isolator.hpp e86062e9abaaa263c32c55e9dbfefd700f605886
src/slave/cgroups_isolator.cpp 80155a3f0cd684a6aad9a37f61c5204d86032735
src/slave/container/containerizer.hpp PRE-CREATION
src/slave/container/containerizer.cpp PRE-CREATION
src/slave/container/mesos_containerizer.hpp PRE-CREATION
src/slave/container/mesos_containerizer.cpp PRE-CREATION
src/slave/http.cpp 3316fdf6d1dfb97a1695a47c754773835fbdd3fb
src/slave/isolator.hpp fc13089664e136936ed30ae4a5a11ab55d7719dc
src/slave/isolator.cpp c9643cf9c5657bc142482a71fb161233bffb3b9f
src/slave/main.cpp e0cae7b205c2599e05c4db990cc9c8e9e3673c37
src/slave/monitor.hpp 52568ad8ec566f7cf36c249c76d798d44eacb578
src/slave/monitor.cpp a931c4f35a8793c66ee03de82f0e0a21b92f8ffa
src/slave/paths.hpp 8f80b931a896cd7e317658147e864410558d5028
src/slave/process_isolator.hpp 4ae093fe65775a2b9bec42071961dd58aa0c3d8b
src/slave/process_isolator.cpp 0bc698f04f7c8eaad166dc9d646e13310129dd01
src/slave/slave.hpp 71fa4f0d0ccde8cdc023b18c6a4d7eb7478fd0cf
src/slave/slave.cpp 75d9e5dfce9f546fb528d9f0fcf8ba127ac40b9c
src/slave/state.hpp f06cba8b9b197ea40e6c255e158d920b4ba602ff
src/slave/state.cpp bf267b5ea23a7427c7cb05ab4e98af2e44d9cc43
src/slave/status_update_manager.hpp 06ea4659cdd24cb1b82f389f404566ba14a663fb
src/slave/status_update_manager.cpp f7a0c40f1b1a8cfab828998146b8f446a0569251
src/tests/test_containerizer.hpp PRE-CREATION
src/tests/test_containerizer.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/16147/diff/
Testing
-------
Thanks,
Ian Downes
Re: Review Request 16147: Containerizer
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16147/#review30692
-----------------------------------------------------------
Hi Ian!
Great progress - Here are a few breaking issues (will follow up with a style review later on).
A side note: I see that cgroups_isolator.cpp has been removed. It should also be removed from src/Makefile.am.
src/slave/container/containerizer.hpp
<https://reviews.apache.org/r/16147/#comment58797>
~ContainerizerProcess() needs an implementation.
src/slave/container/mesos_containerizer.hpp
<https://reviews.apache.org/r/16147/#comment58792>
The ContainerizerProcess defines launch(). Should this be launch, or should ContainerizerProcess use start()? :)
src/slave/container/mesos_containerizer.hpp
<https://reviews.apache.org/r/16147/#comment58793>
Same as launch/start - destroy() is defined in ContainerizerProcess.
- Niklas Nielsen
On Dec. 19, 2013, 7:59 a.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16147/
> -----------------------------------------------------------
>
> (Updated Dec. 19, 2013, 7:59 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, Chi Zhang, Niklas Nielsen, samya, and Jason Dusek.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> The proposed Containerizer interface is to replace the existing Isolator.
>
> One ContainerizerProcess has been written:
> MesosContainerizerProcess - implements containerizeration internally using a Launcher and one or more Isolators (following review)
>
> The intent is to also support a generic ExternalContainerizerProcess that can delegate containerizeration by making external calls. Other Containerizers could interface with specific external containerization techniques such as Docker or LXC.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 655f86757487ddbe551fdcf53eb793e773ecdd34
> src/Makefile.am 42dafbcd6e2d4f67d2705c5a2bea6ec6a0f4fcc1
> src/common/type_utils.hpp 3b05751a9a27055dd19e4abb79197f53e5554fe1
> src/launcher/launcher.hpp 104fe812ddd2b75b14ec0b40d192f58cdba3603a
> src/launcher/launcher.cpp d5ab66704429a95eeb8eda5188e33d8e691221af
> src/launcher/main.cpp de64609905ee63096c0173fe7e64a1eafea5d6bf
> src/local/local.cpp 83a7f913afb1441e9137c7fcec8fd81815714905
> src/slave/cgroups_isolator.hpp e86062e9abaaa263c32c55e9dbfefd700f605886
> src/slave/cgroups_isolator.cpp 80155a3f0cd684a6aad9a37f61c5204d86032735
> src/slave/container/containerizer.hpp PRE-CREATION
> src/slave/container/containerizer.cpp PRE-CREATION
> src/slave/container/mesos_containerizer.hpp PRE-CREATION
> src/slave/container/mesos_containerizer.cpp PRE-CREATION
> src/slave/http.cpp 3316fdf6d1dfb97a1695a47c754773835fbdd3fb
> src/slave/isolator.hpp fc13089664e136936ed30ae4a5a11ab55d7719dc
> src/slave/isolator.cpp c9643cf9c5657bc142482a71fb161233bffb3b9f
> src/slave/main.cpp e0cae7b205c2599e05c4db990cc9c8e9e3673c37
> src/slave/monitor.hpp 52568ad8ec566f7cf36c249c76d798d44eacb578
> src/slave/monitor.cpp a931c4f35a8793c66ee03de82f0e0a21b92f8ffa
> src/slave/paths.hpp 8f80b931a896cd7e317658147e864410558d5028
> src/slave/process_isolator.hpp 4ae093fe65775a2b9bec42071961dd58aa0c3d8b
> src/slave/process_isolator.cpp 0bc698f04f7c8eaad166dc9d646e13310129dd01
> src/slave/slave.hpp 71fa4f0d0ccde8cdc023b18c6a4d7eb7478fd0cf
> src/slave/slave.cpp 75d9e5dfce9f546fb528d9f0fcf8ba127ac40b9c
> src/slave/state.hpp f06cba8b9b197ea40e6c255e158d920b4ba602ff
> src/slave/state.cpp bf267b5ea23a7427c7cb05ab4e98af2e44d9cc43
> src/slave/status_update_manager.hpp 06ea4659cdd24cb1b82f389f404566ba14a663fb
> src/slave/status_update_manager.cpp f7a0c40f1b1a8cfab828998146b8f446a0569251
> src/tests/test_containerizer.hpp PRE-CREATION
> src/tests/test_containerizer.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/16147/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 16147: Containerizer
Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16147/
-----------------------------------------------------------
(Updated Dec. 19, 2013, 7:59 a.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, Chi Zhang, Niklas Nielsen, samya, and Jason Dusek.
Changes
-------
Addressed comments and added changes in other necessary files.
Repository: mesos-git
Description
-------
The proposed Containerizer interface is to replace the existing Isolator.
One ContainerizerProcess has been written:
MesosContainerizerProcess - implements containerizeration internally using a Launcher and one or more Isolators (following review)
The intent is to also support a generic ExternalContainerizerProcess that can delegate containerizeration by making external calls. Other Containerizers could interface with specific external containerization techniques such as Docker or LXC.
Diffs (updated)
-----
include/mesos/mesos.proto 655f86757487ddbe551fdcf53eb793e773ecdd34
src/Makefile.am 42dafbcd6e2d4f67d2705c5a2bea6ec6a0f4fcc1
src/common/type_utils.hpp 3b05751a9a27055dd19e4abb79197f53e5554fe1
src/launcher/launcher.hpp 104fe812ddd2b75b14ec0b40d192f58cdba3603a
src/launcher/launcher.cpp d5ab66704429a95eeb8eda5188e33d8e691221af
src/launcher/main.cpp de64609905ee63096c0173fe7e64a1eafea5d6bf
src/local/local.cpp 83a7f913afb1441e9137c7fcec8fd81815714905
src/slave/cgroups_isolator.hpp e86062e9abaaa263c32c55e9dbfefd700f605886
src/slave/cgroups_isolator.cpp 80155a3f0cd684a6aad9a37f61c5204d86032735
src/slave/container/containerizer.hpp PRE-CREATION
src/slave/container/containerizer.cpp PRE-CREATION
src/slave/container/mesos_containerizer.hpp PRE-CREATION
src/slave/container/mesos_containerizer.cpp PRE-CREATION
src/slave/http.cpp 3316fdf6d1dfb97a1695a47c754773835fbdd3fb
src/slave/isolator.hpp fc13089664e136936ed30ae4a5a11ab55d7719dc
src/slave/isolator.cpp c9643cf9c5657bc142482a71fb161233bffb3b9f
src/slave/main.cpp e0cae7b205c2599e05c4db990cc9c8e9e3673c37
src/slave/monitor.hpp 52568ad8ec566f7cf36c249c76d798d44eacb578
src/slave/monitor.cpp a931c4f35a8793c66ee03de82f0e0a21b92f8ffa
src/slave/paths.hpp 8f80b931a896cd7e317658147e864410558d5028
src/slave/process_isolator.hpp 4ae093fe65775a2b9bec42071961dd58aa0c3d8b
src/slave/process_isolator.cpp 0bc698f04f7c8eaad166dc9d646e13310129dd01
src/slave/slave.hpp 71fa4f0d0ccde8cdc023b18c6a4d7eb7478fd0cf
src/slave/slave.cpp 75d9e5dfce9f546fb528d9f0fcf8ba127ac40b9c
src/slave/state.hpp f06cba8b9b197ea40e6c255e158d920b4ba602ff
src/slave/state.cpp bf267b5ea23a7427c7cb05ab4e98af2e44d9cc43
src/slave/status_update_manager.hpp 06ea4659cdd24cb1b82f389f404566ba14a663fb
src/slave/status_update_manager.cpp f7a0c40f1b1a8cfab828998146b8f446a0569251
src/tests/test_containerizer.hpp PRE-CREATION
src/tests/test_containerizer.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/16147/diff/
Testing
-------
Thanks,
Ian Downes
Re: Review Request 16147: Containerizer
Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16147/
-----------------------------------------------------------
(Updated Dec. 11, 2013, 4:05 a.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, Chi Zhang, Niklas Nielsen, samya, and Jason Dusek.
Changes
-------
Rebased master.
Repository: mesos-git
Description
-------
The proposed Containerizer interface is to replace the existing Isolator.
One ContainerizerProcess has been written:
MesosContainerizerProcess - implements containerizeration internally using a Launcher and one or more Isolators (following review)
The intent is to also support a generic ExternalContainerizerProcess that can delegate containerizeration by making external calls. Other Containerizers could interface with specific external containerization techniques such as Docker or LXC.
Diffs (updated)
-----
src/slave/container/containerizer.hpp PRE-CREATION
src/slave/container/containerizer.cpp PRE-CREATION
src/slave/container/mesos_containerizer.hpp PRE-CREATION
src/slave/container/mesos_containerizer.cpp PRE-CREATION
src/slave/slave.hpp 71fa4f0
src/slave/slave.cpp 75d9e5d
src/tests/test_containerizer.hpp PRE-CREATION
Diff: https://reviews.apache.org/r/16147/diff/
Testing
-------
Thanks,
Ian Downes