----------------------------------------------------------- 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 > >
