----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17567/#review38504 -----------------------------------------------------------
include/mesos/mesos.proto <https://reviews.apache.org/r/17567/#comment71469> Just convert the Termination struct in Containerizer to a protobuf and use it here. Maybe do split that into its own review first? include/mesos/mesos.proto <https://reviews.apache.org/r/17567/#comment71470> does this need to be a protobuf? src/examples/python/test_containerizer.py <https://reviews.apache.org/r/17567/#comment70714> Add some comments on what this method does. src/examples/python/test_containerizer.py <https://reviews.apache.org/r/17567/#comment70717> indent by 4. we are trying to follow PEP8. src/examples/python/test_containerizer.py <https://reviews.apache.org/r/17567/#comment70713> Can you print 'mesos_executor' instead? src/examples/python/test_containerizer.py <https://reviews.apache.org/r/17567/#comment71484> So the launch waits for the executor to exit? That seems wrong. Can it just fork the process and pass the pid back in the return protobuf? src/examples/python/test_containerizer.py <https://reviews.apache.org/r/17567/#comment71485> What happens if the slave dies here before it gets a chance to checkpoint the pid? Does the executor stay up forever? src/examples/python/test_containerizer.py <https://reviews.apache.org/r/17567/#comment70731> You should also be catching OSError and ValueError exceptions. src/examples/python/test_containerizer.py <https://reviews.apache.org/r/17567/#comment70724> s/0/proc.returncode/ ? src/examples/python/test_containerizer.py <https://reviews.apache.org/r/17567/#comment70715> new line between external components. src/examples/python/test_containerizer.py <https://reviews.apache.org/r/17567/#comment70733> comments. src/examples/python/test_containerizer.py <https://reviews.apache.org/r/17567/#comment70706> This should print the valid methods. Create a usage() (or use() since usage is already defined) method that can be called here and everywhere else the arguments are invalid. src/examples/python/test_containerizer.py <https://reviews.apache.org/r/17567/#comment70707> Store this in 'methods' dict so that it can be used in usage(). src/examples/python/test_containerizer.py <https://reviews.apache.org/r/17567/#comment70708> this could be if method not in methods: ... src/slave/containerizer/external_containerizer.hpp <https://reviews.apache.org/r/17567/#comment70817> These comments already exit on the Containerizer interface. Can we get rid of them here to avoid duplication? src/slave/containerizer/external_containerizer.hpp <https://reviews.apache.org/r/17567/#comment70816> Why would the *executor* assume anything? src/slave/containerizer/external_containerizer.hpp <https://reviews.apache.org/r/17567/#comment70830> Why tuple instead of a struct? src/slave/containerizer/external_containerizer.hpp <https://reviews.apache.org/r/17567/#comment70819> s/process//. src/slave/containerizer/external_containerizer.hpp <https://reviews.apache.org/r/17567/#comment70818> s/Running/Container/ ? src/slave/containerizer/external_containerizer.hpp <https://reviews.apache.org/r/17567/#comment70827> Can all this be stuck into the Container/Running struct above? src/slave/containerizer/external_containerizer.hpp <https://reviews.apache.org/r/17567/#comment70829> s/exitCode/status/ ? src/slave/containerizer/external_containerizer.hpp <https://reviews.apache.org/r/17567/#comment70828> Comment? src/slave/containerizer/external_containerizer.hpp <https://reviews.apache.org/r/17567/#comment70754> Why do we need these overloads? src/slave/containerizer/external_containerizer.hpp <https://reviews.apache.org/r/17567/#comment70832> new line. src/slave/containerizer/external_containerizer.hpp <https://reviews.apache.org/r/17567/#comment70831> new line. src/slave/containerizer/external_containerizer.hpp <https://reviews.apache.org/r/17567/#comment70834> s/initializing/initialization/ ? src/slave/containerizer/external_containerizer.cpp <https://reviews.apache.org/r/17567/#comment70753> Why VLOG? src/slave/containerizer/external_containerizer.cpp <https://reviews.apache.org/r/17567/#comment70748> Why VLOG instead of LOG(INFO)? LOG(INFO) << "Launching container '" << containerId << "'"; src/slave/containerizer/external_containerizer.cpp <https://reviews.apache.org/r/17567/#comment70750> only set HADOOP_HOME if the flag is non-empty. src/slave/containerizer/external_containerizer.cpp <https://reviews.apache.org/r/17567/#comment70752> What is the reason for having defaults in 2 places, the slave and the external containerizer? src/slave/containerizer/external_containerizer.cpp <https://reviews.apache.org/r/17567/#comment71482> This seems a bit unfortunate. How about creating a protobuf that wraps all the arguments that are needed for launch (and other methods) and passing it on to the external containerizer? src/slave/containerizer/external_containerizer.cpp <https://reviews.apache.org/r/17567/#comment71483> so launch() exec()s the executor? if not this seems to be the pid of the external containerizer script and not the executor. src/slave/containerizer/external_containerizer.cpp <https://reviews.apache.org/r/17567/#comment71490> Looks like this is the pid of the containerizer script and not the container? src/slave/containerizer/external_containerizer.cpp <https://reviews.apache.org/r/17567/#comment71491> Is this from the posix isolator? If yes, can we refactor the code so that we can reuse. Maybe in its own review? src/slave/containerizer/external_containerizer.cpp <https://reviews.apache.org/r/17567/#comment71480> Instead of closing the file descriptor, how about just writing the length of the protobuf first and then serialized protobuf. That way the read end could know the exact size of data to read. This is how we do serialization and deserialization of protobufs to files. See protobuf::write() and protobuf::read(). src/slave/containerizer/external_containerizer.cpp <https://reviews.apache.org/r/17567/#comment71481> If we do the above and if we have MESOS-995, can you just leverage subprocess here? src/slave/flags.hpp <https://reviews.apache.org/r/17567/#comment70737> s/task/task, when using external containerizer/ ? - Vinod Kone On March 25, 2014, 6:55 p.m., Till Toenshoff wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/17567/ > ----------------------------------------------------------- > > (Updated March 25, 2014, 6:55 p.m.) > > > Review request for mesos, Ian Downes, Niklas Nielsen, and Vinod Kone. > > > Bugs: MESOS-816 > https://issues.apache.org/jira/browse/MESOS-816 > > > Repository: mesos-git > > > Description > ------- > > This patch adds the so-called external containerizer. This > containerizer delegates all containerizer calls directly to > an external containerizer program (which can be specified on > start-up). Few calls have internal fall-back implementations > such as wait(), destroy() and usage(). > > The protocol for the interactions with the external program > is as follows: > > COMMAND (ADDITIONAL-PARAMETERS) < INPUT-PROTO > RESULT-PROTO > > launch (ContainerID, --mesos-executor, <path>) < TaskInfo > ExternalStatus > update (ContainerID) < ResourceArray > ExternalStatus > usage (ContainerID) > ResourceStatistics > wait (ContainerID) > ExternalTermination > destroy (ContainerID) > ExternalStatus > > When protocol buffers need to be provided, the Mesos side of > the external containerizer implementation will serialize the > protos on stdin and vice-versa for reading protos on stdout as > drafted in the above scheme. > > > Diffs > ----- > > configure.ac 5404dc2 > include/mesos/mesos.proto 37f8a7f > src/Makefile.am 0775a0d > src/examples/python/test-containerizer.in PRE-CREATION > src/examples/python/test_containerizer.py PRE-CREATION > src/slave/containerizer/containerizer.cpp 6de091e > src/slave/containerizer/external_containerizer.hpp PRE-CREATION > src/slave/containerizer/external_containerizer.cpp PRE-CREATION > src/slave/flags.hpp c9a627b > src/tests/external_containerizer_test.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/17567/diff/ > > > Testing > ------- > > make check and functional testing. > > > Thanks, > > Till Toenshoff > >
