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