You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Till Toenshoff <to...@me.com> on 2014/03/16 23:02:44 UTC
Re: Review Request 17567: External Containerizer
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17567/
-----------------------------------------------------------
(Updated March 16, 2014, 10:02 p.m.)
Review request for mesos, Adam B, Benjamin Hindman, Ben Mahler, Ian Downes, Niklas Nielsen, and Vinod Kone.
Changes
-------
Addressed many comments.
Summary (updated)
-----------------
External Containerizer
Bugs: MESOS-816
https://issues.apache.org/jira/browse/MESOS-816
Repository: mesos-git
Description (updated)
-------
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 (updated)
-----
configure.ac 9a6de87
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 d0a1023
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
Re: Review Request 17567: External Containerizer
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17567/#review37334
-----------------------------------------------------------
Bad patch!
Reviews applied: [19162]
Failed command: git apply --index 19162.patch
Error:
error: patch failed: 3rdparty/libprocess/include/process/subprocess.hpp:69
error: 3rdparty/libprocess/include/process/subprocess.hpp: patch does not apply
- Mesos ReviewBot
On March 16, 2014, 10:02 p.m., Till Toenshoff wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17567/
> -----------------------------------------------------------
>
> (Updated March 16, 2014, 10:02 p.m.)
>
>
> Review request for mesos, Adam B, Benjamin Hindman, Ben Mahler, 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 9a6de87
> 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 d0a1023
> 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
>
>
Re: Review Request 17567: Added External Containerizer.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17567/#review38067
-----------------------------------------------------------
Bad patch!
Reviews applied: [18403, 17567]
Failed command: make -j3 check GTEST_FILTER='' >/dev/null
Error:
ev.c:1531:31: warning: 'ev_default_loop_ptr' initialized and declared 'extern' [enabled by default]
ev.c: In function 'evpipe_write':
ev.c:2160:17: warning: ignoring return value of 'write', declared with attribute warn_unused_result [-Wunused-result]
ev.c:2172:17: warning: ignoring return value of 'write', declared with attribute warn_unused_result [-Wunused-result]
ev.c: In function 'pipecb':
ev.c:2193:16: warning: ignoring return value of 'read', declared with attribute warn_unused_result [-Wunused-result]
ev.c:2207:16: warning: ignoring return value of 'read', declared with attribute warn_unused_result [-Wunused-result]
In file included from /usr/include/c++/4.6/ext/hash_set:61:0,
from src/glog/stl_logging.h:54,
from src/stl_logging_unittest.cc:34:
/usr/include/c++/4.6/backward/backward_warning.h:33:2: warning: #warning This file includes at least one deprecated or antiquated header which may be removed without further notice at a future date. Please use a non-deprecated interface with equivalent functionality instead. For a listing of replacement headers and interfaces, consult the file backward_warning.h. To disable this warning use -Wno-deprecated. [-Wcpp]
In file included from src/utilities.h:73:0,
from src/googletest.h:38,
from src/stl_logging_unittest.cc:48:
src/base/mutex.h:137:0: warning: "_XOPEN_SOURCE" redefined [enabled by default]
/usr/include/features.h:166:0: note: this is the location of the previous definition
warning: no files found matching 'Makefile' under directory 'docs'
warning: no files found matching 'indexsidebar.html' under directory 'docs'
zip_safe flag not set; analyzing archive contents...
slave/slave.cpp:3473:25: error: no 'Try<Nothing> mesos::internal::slave::Executor::checkValidity(const mesos::ExecutorInfo&, const mesos::TaskInfo&)' member function declared in class 'mesos::internal::slave::Executor'
make[2]: *** [slave/libmesos_no_3rdparty_la-slave.lo] Error 1
make[2]: *** Waiting for unfinished jobs....
make[1]: *** [check] Error 2
make: *** [check-recursive] Error 1
- Mesos ReviewBot
On March 21, 2014, 4:11 a.m., Till Toenshoff wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17567/
> -----------------------------------------------------------
>
> (Updated March 21, 2014, 4:11 a.m.)
>
>
> Review request for mesos, Adam B, Benjamin Hindman, Ben Mahler, 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
>
>
Re: Review Request 17567: Added External Containerizer.
Posted by Till Toenshoff <to...@me.com>.
> On March 31, 2014, 8:03 p.m., Vinod Kone wrote:
> > include/mesos/mesos.proto, line 498
> > <https://reviews.apache.org/r/17567/diff/15/?file=531795#file531795line498>
> >
> > Just convert the Termination struct in Containerizer to a protobuf and use it here.
> >
> > Maybe do split that into its own review first?
Yes, good idea.
Proposed https://reviews.apache.org/r/19901/ but did not yet adapt this RR towards it.
> On March 31, 2014, 8:03 p.m., Vinod Kone wrote:
> > src/slave/containerizer/external_containerizer.cpp, lines 661-681
> > <https://reviews.apache.org/r/17567/diff/15/?file=531801#file531801line661>
> >
> > Is this from the posix isolator? If yes, can we refactor the code so that we can reuse. Maybe in its own review?
672 - 681 are indeed from the posix isolator. I am a bit unsure if refactoring those two case decisions with the setters would be an improvement. Would you please have a second look?
- Till
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17567/#review38504
-----------------------------------------------------------
On April 2, 2014, 2:02 a.m., Till Toenshoff wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17567/
> -----------------------------------------------------------
>
> (Updated April 2, 2014, 2:02 a.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.
>
>
> NOTE: This is currently work in progress to cover all comments.
>
>
> 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
>
>
Re: Review Request 17567: Added External Containerizer.
Posted by Till Toenshoff <to...@me.com>.
> On March 31, 2014, 8:03 p.m., Vinod Kone wrote:
> > src/slave/containerizer/external_containerizer.cpp, line 1135
> > <https://reviews.apache.org/r/17567/diff/15/?file=531801#file531801line1135>
> >
> > If we do the above and if we have MESOS-995, can you just leverage subprocess here?
> >
Almost, yes. Let me quote from the header:
// TODO(tillt): Remove and use process::subprocess once that has
// become flexible enough to support all features needed:
// - in-child-context commands (chdir) after fork and before exec.
// - direct execution without the invocation of /bin/sh.
// - external ownership of pipe file descriptors.
Additionally, we are missing a setsid within the child-context to prevent a slave suicide once the external containerizer process gets terminated.
For the reasoning:
chdir() is clear and would be covered by MESOS-995, which is great.
Direct execution is needed as we currently are depending on proper pipe close escalation. If /bin/sh is used to invoke and when its child process is closing a pipe, the shell has to close its end as well. In some tests I did, I found out that this is not always happening, depending on the shell in use - specifically dash on ubuntu appears to not do that in certain configurations.
External ownership of the pipe handles is needed as the current way of signaling a complete transmission of those protobufs is done via detection of an EOF. Subprocess assumes to have full control and does assume the pipes to be open until the process terminates.
Now circling back to your earlier comment on prepending the protobufs by their size to signal a complete transmission. That might ease the process as we could remove the need for "direct execution" and "external pipe ownership". So the only remaining reason to not use subprocess then would be the missing setsid().
> On March 31, 2014, 8:03 p.m., Vinod Kone wrote:
> > src/slave/flags.hpp, line 207
> > <https://reviews.apache.org/r/17567/diff/15/?file=531802#file531802line207>
> >
> > s/task/task, when using external containerizer/ ?
Yes, that is better.
> On March 31, 2014, 8:03 p.m., Vinod Kone wrote:
> > src/slave/containerizer/external_containerizer.cpp, line 163
> > <https://reviews.apache.org/r/17567/diff/15/?file=531801#file531801line163>
> >
> > Why VLOG?
At some point I refactored the log-levels to reduce things to a minimum (on INFO-level) - may have gone too far :).
> On March 31, 2014, 8:03 p.m., Vinod Kone wrote:
> > src/examples/python/test_containerizer.py, lines 54-56
> > <https://reviews.apache.org/r/17567/diff/15/?file=531798#file531798line54>
> >
> > What happens if the slave dies here before it gets a chance to checkpoint the pid? Does the executor stay up forever?
Is that still a problem if we can leverage the new logic which can clean up even though the executor is not in place?
> On March 31, 2014, 8:03 p.m., Vinod Kone wrote:
> > src/examples/python/test_containerizer.py, lines 54-56
> > <https://reviews.apache.org/r/17567/diff/15/?file=531798#file531798line54>
> >
> > 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?
That way, we would have to do reap two processes, the external containerizer script as well as the launched process (executor). Totally doable for sure, but I am a bit unsure how this would improve the results. Are you mostly aiming for the comment below?
> On March 31, 2014, 8:03 p.m., Vinod Kone wrote:
> > src/slave/containerizer/external_containerizer.hpp, lines 119-120
> > <https://reviews.apache.org/r/17567/diff/15/?file=531800#file531800line119>
> >
> > Why would the *executor* assume anything?
> On March 31, 2014, 8:03 p.m., Vinod Kone wrote:
> > src/slave/containerizer/external_containerizer.cpp, lines 1125-1126
> > <https://reviews.apache.org/r/17567/diff/15/?file=531801#file531801line1125>
> >
> > 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().
> >
> >
> >
I like this approach. Will play with this a bit to see if there is something we are missing.
> On March 31, 2014, 8:03 p.m., Vinod Kone wrote:
> > src/slave/containerizer/external_containerizer.cpp, lines 284-287
> > <https://reviews.apache.org/r/17567/diff/15/?file=531801#file531801line284>
> >
> > What is the reason for having defaults in 2 places, the slave and the external containerizer?
If no default image was supplied to the slave (via flags) and no image is supplied with the command, then something else has to be used. We have no control over that "something" and hence this log-output. Does that make sense?
> On March 31, 2014, 8:03 p.m., Vinod Kone wrote:
> > src/slave/containerizer/external_containerizer.cpp, line 270
> > <https://reviews.apache.org/r/17567/diff/15/?file=531801#file531801line270>
> >
> > only set HADOOP_HOME if the flag is non-empty.
> On March 31, 2014, 8:03 p.m., Vinod Kone wrote:
> > src/slave/containerizer/external_containerizer.hpp, lines 158-171
> > <https://reviews.apache.org/r/17567/diff/15/?file=531800#file531800line158>
> >
> > Can all this be stuck into the Container/Running struct above?
That is possible but I felt this would be bit of a stretch as both are semantically different.
Sandbox is capturing information of a process to be launched, hence it is populated before the actual launch. Running/Container is capturing a information of a successfully launched process.
When putting both into a single structure, I would need to introduce additional state checks.
For example, this:
if (!containers.contains(containerId)) {
return Failure("Container '" + containerId.value() + "' not running");
}
Needed to be changed towards something like this:
if (!containers.contains(containerId) || containers[containerId].pid == 0) {
return Failure("Container '" + containerId.value() + "' not running");
}
Would you prefer that?
> On March 31, 2014, 8:03 p.m., Vinod Kone wrote:
> > src/slave/containerizer/external_containerizer.cpp, line 331
> > <https://reviews.apache.org/r/17567/diff/15/?file=531801#file531801line331>
> >
> > so launch() exec()s the executor? if not this seems to be the pid of the external containerizer script and not the executor.
Yes, it launches the external containerizer script which in turn takes care of launching an executor/command.
> On March 31, 2014, 8:03 p.m., Vinod Kone wrote:
> > src/slave/containerizer/external_containerizer.cpp, line 648
> > <https://reviews.apache.org/r/17567/diff/15/?file=531801#file531801line648>
> >
> > Looks like this is the pid of the containerizer script and not the container?
Yes, see above.
> On March 31, 2014, 8:03 p.m., Vinod Kone wrote:
> > include/mesos/mesos.proto, line 508
> > <https://reviews.apache.org/r/17567/diff/15/?file=531795#file531795line508>
> >
> > does this need to be a protobuf?
My idea was to adhere to our general communication scheme by using a protobuf - even though this one currently only contains a string.
Would you prefer using a 0 terminated string without wrapping protobuf instead?
> On March 31, 2014, 8:03 p.m., Vinod Kone wrote:
> > src/slave/containerizer/external_containerizer.hpp, lines 142-144
> > <https://reviews.apache.org/r/17567/diff/15/?file=531800#file531800line142>
> >
> > Why tuple instead of a struct?
I am using this for the await on tuple overload of process. Enhancing the comment to point that out, is that ok?
> On March 31, 2014, 8:03 p.m., Vinod Kone wrote:
> > src/examples/python/test_containerizer.py, line 28
> > <https://reviews.apache.org/r/17567/diff/15/?file=531798#file531798line28>
> >
> > indent by 4. we are trying to follow PEP8.
We should also fix that in all other python scripts.
- Till
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17567/#review38504
-----------------------------------------------------------
On April 2, 2014, 2:02 a.m., Till Toenshoff wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17567/
> -----------------------------------------------------------
>
> (Updated April 2, 2014, 2:02 a.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.
>
>
> NOTE: This is currently work in progress to cover all comments.
>
>
> 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
>
>
Re: Review Request 17567: Added External Containerizer.
Posted by Till Toenshoff <to...@me.com>.
> On March 31, 2014, 8:03 p.m., Vinod Kone wrote:
> > include/mesos/mesos.proto, line 508
> > <https://reviews.apache.org/r/17567/diff/15/?file=531795#file531795line508>
> >
> > does this need to be a protobuf?
>
> Till Toenshoff wrote:
> My idea was to adhere to our general communication scheme by using a protobuf - even though this one currently only contains a string.
>
> Would you prefer using a 0 terminated string without wrapping protobuf instead?
>
Added optional executor pid as a return value from launch into this protobuf.
> On March 31, 2014, 8:03 p.m., Vinod Kone wrote:
> > src/examples/python/test_containerizer.py, lines 54-56
> > <https://reviews.apache.org/r/17567/diff/15/?file=531798#file531798line54>
> >
> > 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?
>
> Till Toenshoff wrote:
> That way, we would have to do reap two processes, the external containerizer script as well as the launched process (executor). Totally doable for sure, but I am a bit unsure how this would improve the results. Are you mostly aiming for the comment below?
Changed this the way you have proposed and it makes much more sense now. Thanks Vinod!
> On March 31, 2014, 8:03 p.m., Vinod Kone wrote:
> > src/slave/containerizer/external_containerizer.cpp, line 331
> > <https://reviews.apache.org/r/17567/diff/15/?file=531801#file531801line331>
> >
> > so launch() exec()s the executor? if not this seems to be the pid of the external containerizer script and not the executor.
>
> Till Toenshoff wrote:
> Yes, it launches the external containerizer script which in turn takes care of launching an executor/command.
Updated this. For checkpointing and wait, the executor pid is used.
> On March 31, 2014, 8:03 p.m., Vinod Kone wrote:
> > src/slave/containerizer/external_containerizer.cpp, line 648
> > <https://reviews.apache.org/r/17567/diff/15/?file=531801#file531801line648>
> >
> > Looks like this is the pid of the containerizer script and not the container?
>
> Till Toenshoff wrote:
> Yes, see above.
Adapted towards using the executor pid.
> On March 31, 2014, 8:03 p.m., Vinod Kone wrote:
> > include/mesos/mesos.proto, line 498
> > <https://reviews.apache.org/r/17567/diff/15/?file=531795#file531795line498>
> >
> > Just convert the Termination struct in Containerizer to a protobuf and use it here.
> >
> > Maybe do split that into its own review first?
>
> Till Toenshoff wrote:
> Yes, good idea.
>
> Proposed https://reviews.apache.org/r/19901/ but did not yet adapt this RR towards it.
Now adapted and rebased on top of 19901.
> On March 31, 2014, 8:03 p.m., Vinod Kone wrote:
> > src/slave/containerizer/external_containerizer.cpp, line 1135
> > <https://reviews.apache.org/r/17567/diff/15/?file=531801#file531801line1135>
> >
> > If we do the above and if we have MESOS-995, can you just leverage subprocess here?
> >
>
> Till Toenshoff wrote:
> Almost, yes. Let me quote from the header:
> // TODO(tillt): Remove and use process::subprocess once that has
> // become flexible enough to support all features needed:
> // - in-child-context commands (chdir) after fork and before exec.
> // - direct execution without the invocation of /bin/sh.
> // - external ownership of pipe file descriptors.
>
> Additionally, we are missing a setsid within the child-context to prevent a slave suicide once the external containerizer process gets terminated.
>
> For the reasoning:
> chdir() is clear and would be covered by MESOS-995, which is great.
>
> Direct execution is needed as we currently are depending on proper pipe close escalation. If /bin/sh is used to invoke and when its child process is closing a pipe, the shell has to close its end as well. In some tests I did, I found out that this is not always happening, depending on the shell in use - specifically dash on ubuntu appears to not do that in certain configurations.
>
> External ownership of the pipe handles is needed as the current way of signaling a complete transmission of those protobufs is done via detection of an EOF. Subprocess assumes to have full control and does assume the pipes to be open until the process terminates.
>
>
> Now circling back to your earlier comment on prepending the protobufs by their size to signal a complete transmission. That might ease the process as we could remove the need for "direct execution" and "external pipe ownership". So the only remaining reason to not use subprocess then would be the missing setsid().
>
>
> Till Toenshoff wrote:
> So now we are only missing setsid() to fully rely upon subprocess. Will propose an overload as soon as I addressed all other comments.
Now back again on subprocess, glad we got this working - feels much better to rely upon a standard component for such error-prone task.
- Till
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17567/#review38504
-----------------------------------------------------------
On April 4, 2014, 8:39 a.m., Till Toenshoff wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17567/
> -----------------------------------------------------------
>
> (Updated April 4, 2014, 8:39 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, and Niklas Nielsen.
>
>
> 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) < ExternalTask > ExternalStatus
> update (ContainerID) < ResourceArray > ExternalStatus
> usage (ContainerID) > ResourceStatistics
> wait (ContainerID) > Termination
> 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 c1de6d7
> include/mesos/mesos.proto 37f8a7f
> src/Makefile.am 95f133d
> src/examples/python/test-containerizer.in PRE-CREATION
> src/examples/python/test_containerizer.py PRE-CREATION
> src/slave/containerizer/containerizer.cpp 344872a
> src/slave/containerizer/external_containerizer.hpp PRE-CREATION
> src/slave/containerizer/external_containerizer.cpp PRE-CREATION
> src/slave/flags.hpp d5c54c0
> 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
>
>
Re: Review Request 17567: Added External Containerizer.
Posted by Till Toenshoff <to...@me.com>.
> On March 31, 2014, 8:03 p.m., Vinod Kone wrote:
> > src/slave/containerizer/external_containerizer.cpp, lines 293-295
> > <https://reviews.apache.org/r/17567/diff/15/?file=531801#file531801line293>
> >
> > 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?
Packed this into ExternalTask which wraps TaskInfo and the mesos-executor path.
> On March 31, 2014, 8:03 p.m., Vinod Kone wrote:
> > src/slave/containerizer/external_containerizer.cpp, lines 1125-1126
> > <https://reviews.apache.org/r/17567/diff/15/?file=531801#file531801line1125>
> >
> > 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().
> >
> >
> >
>
> Till Toenshoff wrote:
> I like this approach. Will play with this a bit to see if there is something we are missing.
Now implemented that in the latest revision.
> On March 31, 2014, 8:03 p.m., Vinod Kone wrote:
> > src/slave/containerizer/external_containerizer.cpp, line 1135
> > <https://reviews.apache.org/r/17567/diff/15/?file=531801#file531801line1135>
> >
> > If we do the above and if we have MESOS-995, can you just leverage subprocess here?
> >
>
> Till Toenshoff wrote:
> Almost, yes. Let me quote from the header:
> // TODO(tillt): Remove and use process::subprocess once that has
> // become flexible enough to support all features needed:
> // - in-child-context commands (chdir) after fork and before exec.
> // - direct execution without the invocation of /bin/sh.
> // - external ownership of pipe file descriptors.
>
> Additionally, we are missing a setsid within the child-context to prevent a slave suicide once the external containerizer process gets terminated.
>
> For the reasoning:
> chdir() is clear and would be covered by MESOS-995, which is great.
>
> Direct execution is needed as we currently are depending on proper pipe close escalation. If /bin/sh is used to invoke and when its child process is closing a pipe, the shell has to close its end as well. In some tests I did, I found out that this is not always happening, depending on the shell in use - specifically dash on ubuntu appears to not do that in certain configurations.
>
> External ownership of the pipe handles is needed as the current way of signaling a complete transmission of those protobufs is done via detection of an EOF. Subprocess assumes to have full control and does assume the pipes to be open until the process terminates.
>
>
> Now circling back to your earlier comment on prepending the protobufs by their size to signal a complete transmission. That might ease the process as we could remove the need for "direct execution" and "external pipe ownership". So the only remaining reason to not use subprocess then would be the missing setsid().
>
So now we are only missing setsid() to fully rely upon subprocess. Will propose an overload as soon as I addressed all other comments.
- Till
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17567/#review38504
-----------------------------------------------------------
On April 2, 2014, 2:02 a.m., Till Toenshoff wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17567/
> -----------------------------------------------------------
>
> (Updated April 2, 2014, 2:02 a.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.
>
>
> NOTE: This is currently work in progress to cover all comments.
>
>
> 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
>
>
Re: Review Request 17567: Added External Containerizer.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
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
>
>
Re: Review Request 17567: Added External Containerizer.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17567/#review39099
-----------------------------------------------------------
Bad patch!
Reviews applied: [17567]
Failed command: git apply --index 17567.patch
Error:
error: patch failed: src/slave/flags.hpp:196
error: src/slave/flags.hpp: patch does not apply
- Mesos ReviewBot
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
>
>
Re: Review Request 17567: Added External Containerizer.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17567/#review39395
-----------------------------------------------------------
Bad patch!
Reviews applied: [19795, 18403, 17567]
Failed command: git apply --index 17567.patch
Error:
error: patch failed: src/slave/flags.hpp:196
error: src/slave/flags.hpp: patch does not apply
- Mesos ReviewBot
On April 3, 2014, 3:36 a.m., Till Toenshoff wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17567/
> -----------------------------------------------------------
>
> (Updated April 3, 2014, 3:36 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, and Niklas Nielsen.
>
>
> 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) < ExternalTask > 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.
>
>
> NOTE: This is currently work in progress to cover all comments.
>
>
> 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
>
>
Re: Review Request 17567: Added External Containerizer.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17567/#review39528
-----------------------------------------------------------
Bad patch!
Reviews applied: [19795, 18403, 19901, 19795]
Failed command: git apply --index 19795.patch
Error:
error: patch failed: src/slave/http.cpp:144
error: src/slave/http.cpp: patch does not apply
error: patch failed: src/slave/slave.hpp:93
error: src/slave/slave.hpp: patch does not apply
error: patch failed: src/slave/slave.cpp:30
error: src/slave/slave.cpp: patch does not apply
- Mesos ReviewBot
On April 4, 2014, 8:39 a.m., Till Toenshoff wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17567/
> -----------------------------------------------------------
>
> (Updated April 4, 2014, 8:39 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, and Niklas Nielsen.
>
>
> 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) < ExternalTask > ExternalStatus
> update (ContainerID) < ResourceArray > ExternalStatus
> usage (ContainerID) > ResourceStatistics
> wait (ContainerID) > Termination
> 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 c1de6d7
> include/mesos/mesos.proto 37f8a7f
> src/Makefile.am 95f133d
> src/examples/python/test-containerizer.in PRE-CREATION
> src/examples/python/test_containerizer.py PRE-CREATION
> src/slave/containerizer/containerizer.cpp 344872a
> src/slave/containerizer/external_containerizer.hpp PRE-CREATION
> src/slave/containerizer/external_containerizer.cpp PRE-CREATION
> src/slave/flags.hpp d5c54c0
> 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
>
>
Re: Review Request 17567: Added External Containerizer.
Posted by Till Toenshoff <to...@me.com>.
> On April 28, 2014, 7:08 a.m., Benjamin Hindman wrote:
> > I've made the minor style cleanups mentioned in this review so I'll be committing this now.
Thank you so much Ben - especially for doing my work and fixing these remaining issues.
> On April 28, 2014, 7:08 a.m., Benjamin Hindman wrote:
> > src/slave/containerizer/external_containerizer.cpp, line 745
> > <https://reviews.apache.org/r/17567/diff/23/?file=569035#file569035line745>
> >
> > This needs to call out to the external containerizer program! This could return nothing after a slave recovers!
Great point indeed. Will propose a subsequent patch. One question though, what about discrepancies;
Suppose the ECP returns containerIDs A and B whereas the internal state of the EC knows about containerIDs A, B and C. I will need to check if that is actually a reasonable scenario but there is potential for a challenge here :).
> On April 28, 2014, 7:08 a.m., Benjamin Hindman wrote:
> > src/slave/containerizer/external_containerizer.cpp, lines 284-286
> > <https://reviews.apache.org/r/17567/diff/23/?file=569035#file569035line284>
> >
> > Okay, while I agree that there isn't too much we need to do, my guess is that we need to at least fill up the list of containers so that subsequent calls to things like 'wait', 'usage', 'update', etc. will work!
Will propose an RR for this.
- Till
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17567/#review41569
-----------------------------------------------------------
On April 26, 2014, 1:14 a.m., Till Toenshoff wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17567/
> -----------------------------------------------------------
>
> (Updated April 26, 2014, 1:14 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, and Niklas Nielsen.
>
>
> 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).
>
> The protocol for the interactions with the external program
> is as follows:
>
> COMMAND < INPUT-PROTO > RESULT-PROTO
>
> launch < Launch
> update < Update
> usage < Usage > ResourceStatistics
> wait < Wait > Termination
> destroy < Destroy
>
> 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 c964452
> src/Makefile.am 364d63b
> src/examples/python/test-containerizer.in PRE-CREATION
> src/examples/python/test_containerizer.py PRE-CREATION
> src/slave/containerizer/containerizer.hpp 9a50fba
> src/slave/containerizer/containerizer.cpp 344872a
> src/slave/containerizer/external_containerizer.hpp PRE-CREATION
> src/slave/containerizer/external_containerizer.cpp PRE-CREATION
> src/slave/containerizer/mesos_containerizer.cpp f5df979
> src/slave/flags.hpp d5c54c0
> 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
>
>
Re: Review Request 17567: Added External Containerizer.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17567/#review41569
-----------------------------------------------------------
Ship it!
I've made the minor style cleanups mentioned in this review so I'll be committing this now.
src/examples/python/test_containerizer.py
<https://reviews.apache.org/r/17567/#comment75075>
Minor nit: s/container/argv0/
src/examples/python/test_containerizer.py
<https://reviews.apache.org/r/17567/#comment75089>
Minor nit: s/container/argv0/
src/slave/containerizer/external_containerizer.hpp
<https://reviews.apache.org/r/17567/#comment75090>
This is now dead code!
src/slave/containerizer/external_containerizer.cpp
<https://reviews.apache.org/r/17567/#comment75096>
We actually wrap these just like other expressions (e.g., boolean expressions), see http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Boolean_Expressions#Boolean_Expressions.
src/slave/containerizer/external_containerizer.cpp
<https://reviews.apache.org/r/17567/#comment75099>
Okay, while I agree that there isn't too much we need to do, my guess is that we need to at least fill up the list of containers so that subsequent calls to things like 'wait', 'usage', 'update', etc. will work!
src/slave/containerizer/external_containerizer.cpp
<https://reviews.apache.org/r/17567/#comment75094>
This needs to call out to the external containerizer program! This could return nothing after a slave recovers!
src/slave/containerizer/external_containerizer.cpp
<https://reviews.apache.org/r/17567/#comment75092>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Boolean_Expressions#Boolean_Expressions
src/slave/containerizer/external_containerizer.cpp
<https://reviews.apache.org/r/17567/#comment75091>
+4 here.
src/slave/containerizer/external_containerizer.cpp
<https://reviews.apache.org/r/17567/#comment75093>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Boolean_Expressions#Boolean_Expressions
src/tests/external_containerizer_test.cpp
<https://reviews.apache.org/r/17567/#comment75109>
const &
src/tests/external_containerizer_test.cpp
<https://reviews.apache.org/r/17567/#comment75110>
+2 here.
src/tests/external_containerizer_test.cpp
<https://reviews.apache.org/r/17567/#comment75111>
CopyFrom (not your bug, but don't hesitate to clean this up when you copy/paste).
- Benjamin Hindman
On April 26, 2014, 1:14 a.m., Till Toenshoff wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17567/
> -----------------------------------------------------------
>
> (Updated April 26, 2014, 1:14 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, and Niklas Nielsen.
>
>
> 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).
>
> The protocol for the interactions with the external program
> is as follows:
>
> COMMAND < INPUT-PROTO > RESULT-PROTO
>
> launch < Launch
> update < Update
> usage < Usage > ResourceStatistics
> wait < Wait > Termination
> destroy < Destroy
>
> 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 c964452
> src/Makefile.am 364d63b
> src/examples/python/test-containerizer.in PRE-CREATION
> src/examples/python/test_containerizer.py PRE-CREATION
> src/slave/containerizer/containerizer.hpp 9a50fba
> src/slave/containerizer/containerizer.cpp 344872a
> src/slave/containerizer/external_containerizer.hpp PRE-CREATION
> src/slave/containerizer/external_containerizer.cpp PRE-CREATION
> src/slave/containerizer/mesos_containerizer.cpp f5df979
> src/slave/flags.hpp d5c54c0
> 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
>
>
Re: Review Request 17567: Added External Containerizer.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17567/#review41564
-----------------------------------------------------------
Bad patch!
Reviews applied: [17567]
Failed command: git apply --index 17567.patch
Error:
error: patch failed: src/slave/flags.hpp:204
error: src/slave/flags.hpp: patch does not apply
- Mesos ReviewBot
On April 26, 2014, 1:14 a.m., Till Toenshoff wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17567/
> -----------------------------------------------------------
>
> (Updated April 26, 2014, 1:14 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, and Niklas Nielsen.
>
>
> 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).
>
> The protocol for the interactions with the external program
> is as follows:
>
> COMMAND < INPUT-PROTO > RESULT-PROTO
>
> launch < Launch
> update < Update
> usage < Usage > ResourceStatistics
> wait < Wait > Termination
> destroy < Destroy
>
> 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 c964452
> src/Makefile.am 364d63b
> src/examples/python/test-containerizer.in PRE-CREATION
> src/examples/python/test_containerizer.py PRE-CREATION
> src/slave/containerizer/containerizer.hpp 9a50fba
> src/slave/containerizer/containerizer.cpp 344872a
> src/slave/containerizer/external_containerizer.hpp PRE-CREATION
> src/slave/containerizer/external_containerizer.cpp PRE-CREATION
> src/slave/containerizer/mesos_containerizer.cpp f5df979
> src/slave/flags.hpp d5c54c0
> 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
>
>
Re: Review Request 17567: Added External Containerizer.
Posted by Till Toenshoff <to...@me.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17567/
-----------------------------------------------------------
(Updated April 26, 2014, 1:14 a.m.)
Review request for mesos, Benjamin Hindman, Ian Downes, and Niklas Nielsen.
Changes
-------
Addressed remaining comments. Ready for next review - ty!
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).
The protocol for the interactions with the external program
is as follows:
COMMAND < INPUT-PROTO > RESULT-PROTO
launch < Launch
update < Update
usage < Usage > ResourceStatistics
wait < Wait > Termination
destroy < Destroy
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 (updated)
-----
configure.ac c964452
src/Makefile.am 364d63b
src/examples/python/test-containerizer.in PRE-CREATION
src/examples/python/test_containerizer.py PRE-CREATION
src/slave/containerizer/containerizer.hpp 9a50fba
src/slave/containerizer/containerizer.cpp 344872a
src/slave/containerizer/external_containerizer.hpp PRE-CREATION
src/slave/containerizer/external_containerizer.cpp PRE-CREATION
src/slave/containerizer/mesos_containerizer.cpp f5df979
src/slave/flags.hpp d5c54c0
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
Re: Review Request 17567: Added External Containerizer.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17567/#review41406
-----------------------------------------------------------
Bad patch!
Reviews applied: [20080, 20668]
Failed command: git apply --index 20668.patch
Error:
error: patch failed: include/mesos/containerizer/containerizer.proto:25
error: include/mesos/containerizer/containerizer.proto: patch does not apply
- Mesos ReviewBot
On April 24, 2014, 8:14 p.m., Till Toenshoff wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17567/
> -----------------------------------------------------------
>
> (Updated April 24, 2014, 8:14 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, and Niklas Nielsen.
>
>
> 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).
>
> The protocol for the interactions with the external program
> is as follows:
>
> COMMAND < INPUT-PROTO > RESULT-PROTO
>
> launch < Launch
> update < Update
> usage < Usage > ResourceStatistics
> wait < Wait > Termination
> destroy < Destroy
>
> 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 c964452
> src/Makefile.am 364d63b
> src/examples/python/test-containerizer.in PRE-CREATION
> src/examples/python/test_containerizer.py PRE-CREATION
> src/slave/containerizer/containerizer.hpp 9a50fba
> src/slave/containerizer/containerizer.cpp 344872a
> src/slave/containerizer/external_containerizer.hpp PRE-CREATION
> src/slave/containerizer/external_containerizer.cpp PRE-CREATION
> src/slave/containerizer/mesos_containerizer.cpp 722f3fe
> src/slave/flags.hpp d5c54c0
> 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
>
>
Re: Review Request 17567: Added External Containerizer.
Posted by Till Toenshoff <to...@me.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17567/
-----------------------------------------------------------
(Updated April 24, 2014, 8:14 p.m.)
Review request for mesos, Benjamin Hindman, Ian Downes, and Niklas Nielsen.
Changes
-------
Addressed more comments.
Bugs: MESOS-816
https://issues.apache.org/jira/browse/MESOS-816
Repository: mesos-git
Description (updated)
-------
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).
The protocol for the interactions with the external program
is as follows:
COMMAND < INPUT-PROTO > RESULT-PROTO
launch < Launch
update < Update
usage < Usage > ResourceStatistics
wait < Wait > Termination
destroy < Destroy
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 (updated)
-----
configure.ac c964452
src/Makefile.am 364d63b
src/examples/python/test-containerizer.in PRE-CREATION
src/examples/python/test_containerizer.py PRE-CREATION
src/slave/containerizer/containerizer.hpp 9a50fba
src/slave/containerizer/containerizer.cpp 344872a
src/slave/containerizer/external_containerizer.hpp PRE-CREATION
src/slave/containerizer/external_containerizer.cpp PRE-CREATION
src/slave/containerizer/mesos_containerizer.cpp 722f3fe
src/slave/flags.hpp d5c54c0
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
Re: Review Request 17567: Added External Containerizer.
Posted by Till Toenshoff <to...@me.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17567/
-----------------------------------------------------------
(Updated April 24, 2014, 2:40 a.m.)
Review request for mesos, Benjamin Hindman, Ian Downes, and Niklas Nielsen.
Changes
-------
Addressed most comments.
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).
The protocol for the interactions with the external program
is as follows:
COMMAND < INPUT-PROTO > RESULT-PROTO
launch < Launch
update < Update
usage < ContainerID > ResourceStatistics
wait < ContainerID > Termination
destroy < ContainerID
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 (updated)
-----
configure.ac c964452
include/mesos/containerizer/containerizer.proto 6ecd82e
src/Makefile.am 364d63b
src/examples/python/test-containerizer.in PRE-CREATION
src/examples/python/test_containerizer.py PRE-CREATION
src/slave/containerizer/containerizer.hpp 9a50fba
src/slave/containerizer/containerizer.cpp 344872a
src/slave/containerizer/external_containerizer.hpp PRE-CREATION
src/slave/containerizer/external_containerizer.cpp PRE-CREATION
src/slave/containerizer/mesos_containerizer.cpp 722f3fe
src/slave/flags.hpp d5c54c0
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
Re: Review Request 17567: Added External Containerizer.
Posted by Till Toenshoff <to...@me.com>.
> On April 19, 2014, 9:33 p.m., Benjamin Hindman wrote:
> > src/examples/python/test_containerizer.py, line 50
> > <https://reviews.apache.org/r/17567/diff/20/?file=562057#file562057line50>
> >
> > s/use/usage/?
That function name is already used.
> On April 19, 2014, 9:33 p.m., Benjamin Hindman wrote:
> > src/slave/containerizer/external_containerizer.hpp, lines 48-50
> > <https://reviews.apache.org/r/17567/diff/20/?file=562060#file562060line48>
> >
> > Just FYI, if we ever decide to modify the interface of Containerizer::usage,wait,destroy then we'll effectively break external containerizer programs. If we actually had a containerizer::Usage,Wait,Destroy protobuf then we could likely add optional fields to them as a way of extending the interface in the future.
That is a very good point I did not see earlier - adding these to containerizer.proto now.
> On April 19, 2014, 9:33 p.m., Benjamin Hindman wrote:
> > src/slave/containerizer/external_containerizer.hpp, lines 219-223
> > <https://reviews.apache.org/r/17567/diff/20/?file=562060#file562060line219>
> >
> > Just like 'isDone', let's pull this into the .cpp file directly (i.e., not a member of ExternalContainerizerProcess). The big benefit here is that it's helpful to a code reader to know that this function only relies on it's parameters. Making this a static function helps a bit with this, but then we still have to think about the static members and we "pollute" the header file with this non-interface facing function. Make sense?
Yes, indeed a much better design.
> On April 19, 2014, 9:33 p.m., Benjamin Hindman wrote:
> > src/slave/containerizer/external_containerizer.cpp, lines 229-230
> > <https://reviews.apache.org/r/17567/diff/20/?file=562061#file562061line229>
> >
> > My guess is that an external containerizer program will want more than just the path to mesos-executor but also the path to mesos-fetcher in order to fetch packages? Perhaps this should be 'mesos_libexec_directory' instead (which just equals flags.launcher_dir)? Or something else to describe where mesos-* programs are installed?
Yes, good point.
> On April 19, 2014, 9:33 p.m., Benjamin Hindman wrote:
> > src/slave/containerizer/external_containerizer.cpp, line 679
> > <https://reviews.apache.org/r/17567/diff/20/?file=562061#file562061line679>
> >
> > In case you didn't know about it, there's also VLOG_IF:
> >
> > VLOG_IF(sandbox.user.isSome(), 2) << "user: " << sandbox.user.get();
Neat, ty
> On April 19, 2014, 9:33 p.m., Benjamin Hindman wrote:
> > src/examples/python/test_containerizer.py, line 80
> > <https://reviews.apache.org/r/17567/diff/20/?file=562057#file562057line80>
> >
> > Why not use sys.stdout.write like you use sys.stdin.read above?
Historic reasons that do no apply in this version anymore. Fixing....
> On April 19, 2014, 9:33 p.m., Benjamin Hindman wrote:
> > src/examples/python/test_containerizer.py, line 218
> > <https://reviews.apache.org/r/17567/diff/20/?file=562057#file562057line218>
> >
> > Do you ever actually destroy the process? Will tests leave around orphans?
Good point, fixing that.
> On April 19, 2014, 9:33 p.m., Benjamin Hindman wrote:
> > src/slave/containerizer/external_containerizer.hpp, line 179
> > <https://reviews.apache.org/r/17567/diff/20/?file=562060#file562060line179>
> >
> > Did you mean s/processes/containers/ ?
Yes :)
> On April 19, 2014, 9:33 p.m., Benjamin Hindman wrote:
> > src/slave/containerizer/external_containerizer.hpp, lines 213-215
> > <https://reviews.apache.org/r/17567/diff/20/?file=562060#file562060line213>
> >
> > This doesn't rely on anything from ExternalContainerizerProcess, so let's pull it out and put it in the .cpp as a static function.
Aye
> On April 19, 2014, 9:33 p.m., Benjamin Hindman wrote:
> > src/slave/containerizer/external_containerizer.hpp, line 269
> > <https://reviews.apache.org/r/17567/diff/20/?file=562060#file562060line269>
> >
> > It would be nice to move 'containerExecutorInfo' into ExternalContainerizer as a static function. Then uses of it (for example in tests) read as:
> > ExternalContainerizer::containerExecutorInfo.
> >
> > That's more explicit because you see that you're getting an ExecutorInfo as deemed by the ExternalContainerizer.
Aye
> On April 19, 2014, 9:33 p.m., Benjamin Hindman wrote:
> > src/slave/containerizer/external_containerizer.cpp, lines 51-52
> > <https://reviews.apache.org/r/17567/diff/20/?file=562061#file562061line51>
> >
> > This doesn't look like it's used?
Orphaned, ty!
> On April 19, 2014, 9:33 p.m., Benjamin Hindman wrote:
> > src/slave/containerizer/external_containerizer.cpp, lines 184-185
> > <https://reviews.apache.org/r/17567/diff/20/?file=562061#file562061line184>
> >
> > FYI, our style is to keep '+' on previous line and indent as though it was a continued argument. Can you please do a quick pass and clean this up everywhere? Thanks Till!
return Failure("Cannot start already running container '" +
containerId.value() + "'");
It is, as a continued argument gets indented matching the argument start, correct?
> On April 19, 2014, 9:33 p.m., Benjamin Hindman wrote:
> > src/slave/containerizer/external_containerizer.cpp, lines 196-198
> > <https://reviews.apache.org/r/17567/diff/20/?file=562061#file562061line196>
> >
> > Would this be generally useful to move to 'executorEnvironment'? How about a TODO to do this in the future?
Yes.
> On April 19, 2014, 9:33 p.m., Benjamin Hindman wrote:
> > src/slave/containerizer/external_containerizer.cpp, lines 205-206
> > <https://reviews.apache.org/r/17567/diff/20/?file=562061#file562061line205>
> >
> > My two cents, the default container image would be better shared as an environment variable, i.e., MESOS_DEFAULT_CONTAINER_IMAGE. My thinking here is that the external containerizer program might actually like to know whether or not a default image was included in the task and if we set it then it will have no way of knowing. Is deimos actually assuming this will happen right now? We could always change this in the future but it'll be breaking if deimos assumes this now. Thoughts?
An environment variable seems to be fine for this purpose.
> On April 19, 2014, 9:33 p.m., Benjamin Hindman wrote:
> > src/slave/containerizer/external_containerizer.cpp, lines 249-250
> > <https://reviews.apache.org/r/17567/diff/20/?file=562061#file562061line249>
> >
> > So what happens if the slave discards the returned future? I guess in these semantics the subprocess might stay running forever (if there was a bug). How about a TODO above each of these calls to 'then' that suggests adding an 'onDiscard' callback? Oh how I wish we had C++11 lambdas! :-/
Fixing this specific case by inserting an onAny which checks if the future is ready. If that future is not ready (discarded, failed), then this callback will terminate the given containerId. And yes, proper lambdas would be such a code savior.
> On April 19, 2014, 9:33 p.m., Benjamin Hindman wrote:
> > src/slave/containerizer/external_containerizer.cpp, line 276
> > <https://reviews.apache.org/r/17567/diff/20/?file=562061#file562061line276>
> >
> > The 'isDone' function is a little misleading to me as the future is definitely done since you did a 'then' above. In fact, I was a little surprised that '_launch' wasn't taking a Future<Option<int>> instead of an Option<int>.
> >
> > Given the comment at the declaration of 'isDone' (in the header) I wonder if a better name here might be 'validate'? We used 'verify' in src/linux/cgroups.cpp code and an Option<Error> instead of a Try<Nothing>. That pattern was pretty clear to read:
> >
> > Option<Error> error = validate(status);
> > if (error.isSome()) {
> > return Failure(error.get());
> > }
> >
> > Taking a step back a bit, maybe you wanted '_launch' to be invoked for an 'onAny' not a 'then'? That still doesn't change too much in my mind, you still know that the future is "done", you just don't know if the future is a failure or not, hence the suggestion for 'validate' or 'verify'.
Yes, much better.
> On April 19, 2014, 9:33 p.m., Benjamin Hindman wrote:
> > src/slave/containerizer/external_containerizer.cpp, line 287
> > <https://reviews.apache.org/r/17567/diff/20/?file=562061#file562061line287>
> >
> > I think the biggest question I have architecturally here is why the external containerizer program isn't returning the ExecutorInfo (and thus, also determining what the ExecutorInfo should even be). In this case it's especially weird because the ExecutorInfo that gets created is never actually passed to the external containerizer program! It looks like maybe this was done because we needed an ExecutorInfo in order to call 'executorEnvironment', but we could have easily refactored that. Maybe for now we should add a TODO here ...
Adapted towards latest related commits.
> On April 19, 2014, 9:33 p.m., Benjamin Hindman wrote:
> > src/slave/containerizer/external_containerizer.cpp, line 300
> > <https://reviews.apache.org/r/17567/diff/20/?file=562061#file562061line300>
> >
> > What if a launch fails!? IIUC a failure would mean that 'launched' stays in pending forever because we do a 'then' in 'launch' which means '_launch' will not get invoked on a failure ... thus 'launched' will stay pending forever.
Should be fixed now.
> On April 19, 2014, 9:33 p.m., Benjamin Hindman wrote:
> > src/slave/containerizer/external_containerizer.cpp, line 580
> > <https://reviews.apache.org/r/17567/diff/20/?file=562061#file562061line580>
> >
> > So what I understand here is that terminate is really just about killing the subprocess that was invoked in order to wait on a container, but passing 'destroy' to the external containerizer program is really what does the termination/cleanup of the container. The comments here make it sound like you're actually terminating the container when you're just killing the process that is waiting from the 'wait' subcommand. Just want to make sure that is what you expect as well, and if that's the case then how about we clean up some of the comments here (and where 'terminate' gets called as well)? In fact, what about calling this 'unwait' instead of 'terminate' since that's being explicit about what it's really doing?
> >
> > Note that an external containerizer program might try and do something fancy when/if it gets a signal here (at least if it wasn't a SIGKILL and it could react to this signal), but I don't think we should tell people to rely on this, but instead to rely on getting a 'destroy' in order to actually terminate/cleanup a container. Make sense? Or did you have something else in mind?
Correct, it is basically an unwait and that is what I shall rename it to.
> On April 19, 2014, 9:33 p.m., Benjamin Hindman wrote:
> > src/slave/containerizer/external_containerizer.cpp, line 607
> > <https://reviews.apache.org/r/17567/diff/20/?file=562061#file562061line607>
> >
> > Just so I understand correctly, the reason you're not calling 'cleanup' here is because you assume that __wait will get invoked and you'll call cleanup then? That might be a nice comment to leave behind for somebody! ;)
Aye.
- Till
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17567/#review40828
-----------------------------------------------------------
On April 24, 2014, 2:40 a.m., Till Toenshoff wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17567/
> -----------------------------------------------------------
>
> (Updated April 24, 2014, 2:40 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, and Niklas Nielsen.
>
>
> 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).
>
> The protocol for the interactions with the external program
> is as follows:
>
> COMMAND < INPUT-PROTO > RESULT-PROTO
>
> launch < Launch
> update < Update
> usage < ContainerID > ResourceStatistics
> wait < ContainerID > Termination
> destroy < ContainerID
>
> 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 c964452
> include/mesos/containerizer/containerizer.proto 6ecd82e
> src/Makefile.am 364d63b
> src/examples/python/test-containerizer.in PRE-CREATION
> src/examples/python/test_containerizer.py PRE-CREATION
> src/slave/containerizer/containerizer.hpp 9a50fba
> src/slave/containerizer/containerizer.cpp 344872a
> src/slave/containerizer/external_containerizer.hpp PRE-CREATION
> src/slave/containerizer/external_containerizer.cpp PRE-CREATION
> src/slave/containerizer/mesos_containerizer.cpp 722f3fe
> src/slave/flags.hpp d5c54c0
> 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
>
>
Re: Review Request 17567: Added External Containerizer.
Posted by Till Toenshoff <to...@me.com>.
> On April 19, 2014, 9:33 p.m., Benjamin Hindman wrote:
> > src/slave/containerizer/external_containerizer.cpp, lines 184-185
> > <https://reviews.apache.org/r/17567/diff/20/?file=562061#file562061line184>
> >
> > FYI, our style is to keep '+' on previous line and indent as though it was a continued argument. Can you please do a quick pass and clean this up everywhere? Thanks Till!
>
> Till Toenshoff wrote:
> return Failure("Cannot start already running container '" +
> containerId.value() + "'");
>
> It is, as a continued argument gets indented matching the argument start, correct?
>
Ok, got that wrong for some reason. The correct way should be:
return Failure("Cannot start already running container '" +
containerId.value() + "'");
Continued arguments always get 4 chars indention.
> On April 19, 2014, 9:33 p.m., Benjamin Hindman wrote:
> > src/slave/containerizer/external_containerizer.hpp, lines 48-50
> > <https://reviews.apache.org/r/17567/diff/20/?file=562060#file562060line48>
> >
> > Just FYI, if we ever decide to modify the interface of Containerizer::usage,wait,destroy then we'll effectively break external containerizer programs. If we actually had a containerizer::Usage,Wait,Destroy protobuf then we could likely add optional fields to them as a way of extending the interface in the future.
>
> Till Toenshoff wrote:
> That is a very good point I did not see earlier - adding these to containerizer.proto now.
https://reviews.apache.org/r/20668/
> On April 19, 2014, 9:33 p.m., Benjamin Hindman wrote:
> > src/tests/external_containerizer_test.cpp, lines 215-217
> > <https://reviews.apache.org/r/17567/diff/20/?file=562063#file562063line215>
> >
> > The way that you've done this totally works, but I wanted to suggest an alternative that we use other places because it's fairly extensible and very much in the 'mocking' mindset.
> >
> > Rather than extend ExternalContainerizer with a TestExternalContainerizer make it a MockExternalContainerizer and mock the 'launch' method:
> >
> > class MockExternalContainerizer : public ExternalContainerizer
> > {
> > public:
> > MockExternalContainerizer()
> > {
> > // Set up defaults for mocked methods.
> > // NOTE: See TestContainerizer::setup for why we use
> > // 'EXPECT_CALL' and 'WillRepeatedly' here instead of
> > // 'ON_CALL' and 'WillByDefault'.
> > EXPECT_CALL(*this, launch(_, _, _, _, _, _, _))
> > .WillRepeatedly(Invoke(this, &ExternalContainerizer::launch));
> > }
> >
> > MOCK_METHOD7(
> > launch,
> > process::Future<Nothing>(
> > const ContainerID&,
> > const ExecutorInfo&,
> > const std::string&,
> > const Option<std::string>&,
> > const SlaveID&,
> > const process::PID<slave::Slave>&,
> > bool checkpoint));
> > };
> >
> > Now you can set up an expectation to get the launch in the test body! That is, up by 'driver.launchTasks' you can do (or some variation):
> >
> > // Expect the launch but don't do anything.
> > Future<Nothing> launch;
> > EXPECT_CALL(containerizer, launch(_, _, _, _, _, _, _))
> > .WillOnce(DoAll(FutureSatisfy(&launch),
> > Invoke(&containerizer, &ExternalContainerizer::launch)));
> >
> >
> > And later you can wait in the test until you know the launch has occurred:
> >
> > AWAIT_READY(launch);
> >
> > And in your case since you wanted the container ID you could explicitly pull that out in the expectation:
> >
> > Future<ContainerID> containerId;
> > EXPECT_CALL(containerizer, launch(_, _, _, _, _, _, _))
> > .WillOnce(DoAll(FutureArg<0>(&containerId),
> > Invoke(&containerizer, &ExternalContainerizer::launch)));
> >
> > ...
> >
> > AWAIT_READY(containerId);
> >
> > Kind of cool right!? See https://reviews.apache.org/r/20179 for how this is technique is being used other places.
While this indeed is really cool, it also seems to be rather fragile when it comes to mocking overloaded functions (Launch in its two new variants). It might be worth investigating the details here but for now, I ended up introducing a proxy function within the Mocked class to get the invocations working without causing infinite recursion - not beautiful but working fine.
Thanks Niklas for coming up with this workaround!
> On April 19, 2014, 9:33 p.m., Benjamin Hindman wrote:
> > src/tests/external_containerizer_test.cpp, lines 226-230
> > <https://reviews.apache.org/r/17567/diff/20/?file=562063#file562063line226>
> >
> > Just so I'm clear, we're returning dummy data right!? Maybe we should add a TODO here that captures that and also add a TODO in the Python code that captures that we should eventually use mesos-usage! (After I get it committed, will do that pronto and it will be another utility we might want beyond just mesos-executor as mentioned before).
Aye
- Till
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17567/#review40828
-----------------------------------------------------------
On April 24, 2014, 8:14 p.m., Till Toenshoff wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17567/
> -----------------------------------------------------------
>
> (Updated April 24, 2014, 8:14 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, and Niklas Nielsen.
>
>
> 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).
>
> The protocol for the interactions with the external program
> is as follows:
>
> COMMAND < INPUT-PROTO > RESULT-PROTO
>
> launch < Launch
> update < Update
> usage < Usage > ResourceStatistics
> wait < Wait > Termination
> destroy < Destroy
>
> 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 c964452
> src/Makefile.am 364d63b
> src/examples/python/test-containerizer.in PRE-CREATION
> src/examples/python/test_containerizer.py PRE-CREATION
> src/slave/containerizer/containerizer.hpp 9a50fba
> src/slave/containerizer/containerizer.cpp 344872a
> src/slave/containerizer/external_containerizer.hpp PRE-CREATION
> src/slave/containerizer/external_containerizer.cpp PRE-CREATION
> src/slave/containerizer/mesos_containerizer.cpp 722f3fe
> src/slave/flags.hpp d5c54c0
> 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
>
>
Re: Review Request 17567: Added External Containerizer.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17567/#review40828
-----------------------------------------------------------
src/examples/python/test_containerizer.py
<https://reviews.apache.org/r/17567/#comment74083>
s/use/usage/?
src/examples/python/test_containerizer.py
<https://reviews.apache.org/r/17567/#comment74082>
Why not use sys.stdout.write like you use sys.stdin.read above?
src/examples/python/test_containerizer.py
<https://reviews.apache.org/r/17567/#comment74086>
ResourceArray?
src/examples/python/test_containerizer.py
<https://reviews.apache.org/r/17567/#comment74087>
sleep?
src/examples/python/test_containerizer.py
<https://reviews.apache.org/r/17567/#comment74085>
Do you ever actually destroy the process? Will tests leave around orphans?
src/slave/containerizer/external_containerizer.hpp
<https://reviews.apache.org/r/17567/#comment74073>
Just FYI, if we ever decide to modify the interface of Containerizer::usage,wait,destroy then we'll effectively break external containerizer programs. If we actually had a containerizer::Usage,Wait,Destroy protobuf then we could likely add optional fields to them as a way of extending the interface in the future.
src/slave/containerizer/external_containerizer.hpp
<https://reviews.apache.org/r/17567/#comment74010>
Did you mean s/processes/containers/ ?
src/slave/containerizer/external_containerizer.hpp
<https://reviews.apache.org/r/17567/#comment74074>
This doesn't rely on anything from ExternalContainerizerProcess, so let's pull it out and put it in the .cpp as a static function.
src/slave/containerizer/external_containerizer.hpp
<https://reviews.apache.org/r/17567/#comment74075>
Just like 'isDone', let's pull this into the .cpp file directly (i.e., not a member of ExternalContainerizerProcess). The big benefit here is that it's helpful to a code reader to know that this function only relies on it's parameters. Making this a static function helps a bit with this, but then we still have to think about the static members and we "pollute" the header file with this non-interface facing function. Make sense?
src/slave/containerizer/external_containerizer.hpp
<https://reviews.apache.org/r/17567/#comment74081>
It would be nice to move 'containerExecutorInfo' into ExternalContainerizer as a static function. Then uses of it (for example in tests) read as:
ExternalContainerizer::containerExecutorInfo.
That's more explicit because you see that you're getting an ExecutorInfo as deemed by the ExternalContainerizer.
src/slave/containerizer/external_containerizer.cpp
<https://reviews.apache.org/r/17567/#comment74012>
This doesn't look like it's used?
src/slave/containerizer/external_containerizer.cpp
<https://reviews.apache.org/r/17567/#comment74014>
Newline between these, just like for includes (and below for 'using tuples::tuple;' too please). Thanks!
src/slave/containerizer/external_containerizer.cpp
<https://reviews.apache.org/r/17567/#comment74024>
s/MergeFrom/CopyFrom/
Whenever you know you want the entire protobuf just copy as it's more explicit. Would you mind doing a sweep of this review and updating as appropriate? Thanks Till!
src/slave/containerizer/external_containerizer.cpp
<https://reviews.apache.org/r/17567/#comment74020>
Why not do this first?
src/slave/containerizer/external_containerizer.cpp
<https://reviews.apache.org/r/17567/#comment74022>
FYI, our style is to keep '+' on previous line and indent as though it was a continued argument. Can you please do a quick pass and clean this up everywhere? Thanks Till!
src/slave/containerizer/external_containerizer.cpp
<https://reviews.apache.org/r/17567/#comment74026>
Would this be generally useful to move to 'executorEnvironment'? How about a TODO to do this in the future?
src/slave/containerizer/external_containerizer.cpp
<https://reviews.apache.org/r/17567/#comment74027>
My two cents, the default container image would be better shared as an environment variable, i.e., MESOS_DEFAULT_CONTAINER_IMAGE. My thinking here is that the external containerizer program might actually like to know whether or not a default image was included in the task and if we set it then it will have no way of knowing. Is deimos actually assuming this will happen right now? We could always change this in the future but it'll be breaking if deimos assumes this now. Thoughts?
src/slave/containerizer/external_containerizer.cpp
<https://reviews.apache.org/r/17567/#comment74028>
My guess is that an external containerizer program will want more than just the path to mesos-executor but also the path to mesos-fetcher in order to fetch packages? Perhaps this should be 'mesos_libexec_directory' instead (which just equals flags.launcher_dir)? Or something else to describe where mesos-* programs are installed?
src/slave/containerizer/external_containerizer.cpp
<https://reviews.apache.org/r/17567/#comment74079>
So what happens if the slave discards the returned future? I guess in these semantics the subprocess might stay running forever (if there was a bug). How about a TODO above each of these calls to 'then' that suggests adding an 'onDiscard' callback? Oh how I wish we had C++11 lambdas! :-/
src/slave/containerizer/external_containerizer.cpp
<https://reviews.apache.org/r/17567/#comment74080>
The 'isDone' function is a little misleading to me as the future is definitely done since you did a 'then' above. In fact, I was a little surprised that '_launch' wasn't taking a Future<Option<int>> instead of an Option<int>.
Given the comment at the declaration of 'isDone' (in the header) I wonder if a better name here might be 'validate'? We used 'verify' in src/linux/cgroups.cpp code and an Option<Error> instead of a Try<Nothing>. That pattern was pretty clear to read:
Option<Error> error = validate(status);
if (error.isSome()) {
return Failure(error.get());
}
Taking a step back a bit, maybe you wanted '_launch' to be invoked for an 'onAny' not a 'then'? That still doesn't change too much in my mind, you still know that the future is "done", you just don't know if the future is a failure or not, hence the suggestion for 'validate' or 'verify'.
src/slave/containerizer/external_containerizer.cpp
<https://reviews.apache.org/r/17567/#comment74030>
I think the biggest question I have architecturally here is why the external containerizer program isn't returning the ExecutorInfo (and thus, also determining what the ExecutorInfo should even be). In this case it's especially weird because the ExecutorInfo that gets created is never actually passed to the external containerizer program! It looks like maybe this was done because we needed an ExecutorInfo in order to call 'executorEnvironment', but we could have easily refactored that. Maybe for now we should add a TODO here ...
src/slave/containerizer/external_containerizer.cpp
<https://reviews.apache.org/r/17567/#comment74032>
What if a launch fails!? IIUC a failure would mean that 'launched' stays in pending forever because we do a 'then' in 'launch' which means '_launch' will not get invoked on a failure ... thus 'launched' will stay pending forever.
src/slave/containerizer/external_containerizer.cpp
<https://reviews.apache.org/r/17567/#comment74068>
Can we s/p/read/ for future readability please?
src/slave/containerizer/external_containerizer.cpp
<https://reviews.apache.org/r/17567/#comment74069>
To make things easier to read we try and put things like 'result<' on a newline here. Also, are you sure that you need the explicit template instantiation? You do this below as well.
src/slave/containerizer/external_containerizer.cpp
<https://reviews.apache.org/r/17567/#comment74070>
This needs to be in an 'else' please otherwise we're going to crash the slave when we do 'termination.get()'!
src/slave/containerizer/external_containerizer.cpp
<https://reviews.apache.org/r/17567/#comment74076>
src/slave/containerizer/external_containerizer.cpp
<https://reviews.apache.org/r/17567/#comment74067>
I know that you're just following the semantics in MesosContainerizer, but this might bite us in the future. I think a LOG(WARNING) here might be nice.
src/slave/containerizer/external_containerizer.cpp
<https://reviews.apache.org/r/17567/#comment74064>
Can we call this s/p/read/ instead? It's just a bit more readable for others groking the code.
src/slave/containerizer/external_containerizer.cpp
<https://reviews.apache.org/r/17567/#comment74065>
These two look like they're _almost_ doing the same thing.
src/slave/containerizer/external_containerizer.cpp
<https://reviews.apache.org/r/17567/#comment74066>
So what I understand here is that terminate is really just about killing the subprocess that was invoked in order to wait on a container, but passing 'destroy' to the external containerizer program is really what does the termination/cleanup of the container. The comments here make it sound like you're actually terminating the container when you're just killing the process that is waiting from the 'wait' subcommand. Just want to make sure that is what you expect as well, and if that's the case then how about we clean up some of the comments here (and where 'terminate' gets called as well)? In fact, what about calling this 'unwait' instead of 'terminate' since that's being explicit about what it's really doing?
Note that an external containerizer program might try and do something fancy when/if it gets a signal here (at least if it wasn't a SIGKILL and it could react to this signal), but I don't think we should tell people to rely on this, but instead to rely on getting a 'destroy' in order to actually terminate/cleanup a container. Make sense? Or did you have something else in mind?
src/slave/containerizer/external_containerizer.cpp
<https://reviews.apache.org/r/17567/#comment74071>
Just so I understand correctly, the reason you're not calling 'cleanup' here is because you assume that __wait will get invoked and you'll call cleanup then? That might be a nice comment to leave behind for somebody! ;)
src/slave/containerizer/external_containerizer.cpp
<https://reviews.apache.org/r/17567/#comment74063>
I think asking for a death signal from the kernel (on Linux) is a good idea here. See below where I describe this in more detail.
src/slave/containerizer/external_containerizer.cpp
<https://reviews.apache.org/r/17567/#comment74046>
CHECK_SOME
src/slave/containerizer/external_containerizer.cpp
<https://reviews.apache.org/r/17567/#comment74047>
src/slave/containerizer/external_containerizer.cpp
<https://reviews.apache.org/r/17567/#comment74061>
src/slave/containerizer/external_containerizer.cpp
<https://reviews.apache.org/r/17567/#comment74035>
s/if(/if (/
src/slave/containerizer/external_containerizer.cpp
<https://reviews.apache.org/r/17567/#comment74062>
In case you didn't know about it, there's also VLOG_IF:
VLOG_IF(sandbox.user.isSome(), 2) << "user: " << sandbox.user.get();
src/slave/containerizer/external_containerizer.cpp
<https://reviews.apache.org/r/17567/#comment74036>
src/slave/containerizer/external_containerizer.cpp
<https://reviews.apache.org/r/17567/#comment74037>
ErrnoError? But why not:
return Error("Failed to chown work directory: " + chown.error());
src/slave/containerizer/external_containerizer.cpp
<https://reviews.apache.org/r/17567/#comment74045>
What happens if the slave process dies? The 'wait' call will run "forever", so for a container that outlives 100 slave restarts it would be unfortunate to have 100 subprocesses waiting on 'wait'. Maybe this isn't too bad, but we could easily avoid this on Linux by having the kernel send us a death signal when our parent dies using 'prctl(PR_SET_PDEATHSIG, ..)' in our 'setup' function (since it's preserved across exec* calls). I'm happy to have you circle back to this, but let's definitely add a TODO and not forget about it. ;)
src/slave/containerizer/external_containerizer.cpp
<https://reviews.apache.org/r/17567/#comment74039>
No need to wrap this in a 'string()'. There are some places where it's needed, but not everywhere. Could you do a quick pass of your code and clean this up as necessary? I think there are a few more places you use 'string' but don't need to. Thanks Till!
src/slave/containerizer/external_containerizer.cpp
<https://reviews.apache.org/r/17567/#comment74040>
Wrapping the additional error message in '(error: ...)' is not something that we've done in the code base. Instead, the expectation is that "chained" errors will just be of the form:
outer: inner: more inner: even more inner
Not a big deal, but if would be great if you could do a sweep and clean this up so we can keep the code base consistent. Thanks Till!
src/slave/containerizer/external_containerizer.cpp
<https://reviews.apache.org/r/17567/#comment74041>
src/slave/containerizer/external_containerizer.cpp
<https://reviews.apache.org/r/17567/#comment74034>
s/w/write/?
src/slave/containerizer/external_containerizer.cpp
<https://reviews.apache.org/r/17567/#comment74016>
Indentation.
src/slave/containerizer/external_containerizer.cpp
<https://reviews.apache.org/r/17567/#comment74015>
src/tests/external_containerizer_test.cpp
<https://reviews.apache.org/r/17567/#comment74090>
+2
src/tests/external_containerizer_test.cpp
<https://reviews.apache.org/r/17567/#comment74088>
You shouldn't need 'string()' here. I called this one out because it's not in an 'Error(' or 'Failure(' and depending on how you did your search to clean up the other ones I wasn't sure if you'd miss this one. ;)
src/tests/external_containerizer_test.cpp
<https://reviews.apache.org/r/17567/#comment74089>
You shouldn't need 'string()' here. I called this one out because it's not in an 'Error(' or 'Failure(' and depending on how you did your search to clean up the other ones I wasn't sure if you'd miss this one. ;)
src/tests/external_containerizer_test.cpp
<https://reviews.apache.org/r/17567/#comment74092>
The way that you've done this totally works, but I wanted to suggest an alternative that we use other places because it's fairly extensible and very much in the 'mocking' mindset.
Rather than extend ExternalContainerizer with a TestExternalContainerizer make it a MockExternalContainerizer and mock the 'launch' method:
class MockExternalContainerizer : public ExternalContainerizer
{
public:
MockExternalContainerizer()
{
// Set up defaults for mocked methods.
// NOTE: See TestContainerizer::setup for why we use
// 'EXPECT_CALL' and 'WillRepeatedly' here instead of
// 'ON_CALL' and 'WillByDefault'.
EXPECT_CALL(*this, launch(_, _, _, _, _, _, _))
.WillRepeatedly(Invoke(this, &ExternalContainerizer::launch));
}
MOCK_METHOD7(
launch,
process::Future<Nothing>(
const ContainerID&,
const ExecutorInfo&,
const std::string&,
const Option<std::string>&,
const SlaveID&,
const process::PID<slave::Slave>&,
bool checkpoint));
};
Now you can set up an expectation to get the launch in the test body! That is, up by 'driver.launchTasks' you can do (or some variation):
// Expect the launch but don't do anything.
Future<Nothing> launch;
EXPECT_CALL(containerizer, launch(_, _, _, _, _, _, _))
.WillOnce(DoAll(FutureSatisfy(&launch),
Invoke(&containerizer, &ExternalContainerizer::launch)));
And later you can wait in the test until you know the launch has occurred:
AWAIT_READY(launch);
And in your case since you wanted the container ID you could explicitly pull that out in the expectation:
Future<ContainerID> containerId;
EXPECT_CALL(containerizer, launch(_, _, _, _, _, _, _))
.WillOnce(DoAll(FutureArg<0>(&containerId),
Invoke(&containerizer, &ExternalContainerizer::launch)));
...
AWAIT_READY(containerId);
Kind of cool right!? See https://reviews.apache.org/r/20179 for how this is technique is being used other places.
src/tests/external_containerizer_test.cpp
<https://reviews.apache.org/r/17567/#comment74091>
Just so I'm clear, we're returning dummy data right!? Maybe we should add a TODO here that captures that and also add a TODO in the Python code that captures that we should eventually use mesos-usage! (After I get it committed, will do that pronto and it will be another utility we might want beyond just mesos-executor as mentioned before).
- Benjamin Hindman
On April 18, 2014, 3:36 a.m., Till Toenshoff wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17567/
> -----------------------------------------------------------
>
> (Updated April 18, 2014, 3:36 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, and Niklas Nielsen.
>
>
> 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).
>
> The protocol for the interactions with the external program
> is as follows:
>
> COMMAND < INPUT-PROTO > RESULT-PROTO
>
> launch < Launch
> update < Update
> usage < ContainerID > ResourceStatistics
> wait < ContainerID > Termination
> destroy < ContainerID
>
> 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 c1de6d7
> src/Makefile.am 560b4c7
> src/examples/python/test-containerizer.in PRE-CREATION
> src/examples/python/test_containerizer.py PRE-CREATION
> src/slave/containerizer/containerizer.hpp d9ae326
> src/slave/containerizer/containerizer.cpp 344872a
> src/slave/containerizer/external_containerizer.hpp PRE-CREATION
> src/slave/containerizer/external_containerizer.cpp PRE-CREATION
> src/slave/flags.hpp d5c54c0
> 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
>
>
Re: Review Request 17567: Added External Containerizer.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17567/#review40741
-----------------------------------------------------------
Bad patch!
Reviews applied: [19795, 19795]
Failed command: git apply --index 19795.patch
Error:
error: patch failed: src/slave/http.cpp:144
error: src/slave/http.cpp: patch does not apply
error: patch failed: src/slave/slave.hpp:93
error: src/slave/slave.hpp: patch does not apply
error: patch failed: src/slave/slave.cpp:30
error: src/slave/slave.cpp: patch does not apply
error: patch failed: src/tests/containerizer.cpp:163
error: src/tests/containerizer.cpp: patch does not apply
- Mesos ReviewBot
On April 18, 2014, 3:36 a.m., Till Toenshoff wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17567/
> -----------------------------------------------------------
>
> (Updated April 18, 2014, 3:36 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, and Niklas Nielsen.
>
>
> 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).
>
> The protocol for the interactions with the external program
> is as follows:
>
> COMMAND < INPUT-PROTO > RESULT-PROTO
>
> launch < Launch
> update < Update
> usage < ContainerID > ResourceStatistics
> wait < ContainerID > Termination
> destroy < ContainerID
>
> 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 c1de6d7
> src/Makefile.am 560b4c7
> src/examples/python/test-containerizer.in PRE-CREATION
> src/examples/python/test_containerizer.py PRE-CREATION
> src/slave/containerizer/containerizer.hpp d9ae326
> src/slave/containerizer/containerizer.cpp 344872a
> src/slave/containerizer/external_containerizer.hpp PRE-CREATION
> src/slave/containerizer/external_containerizer.cpp PRE-CREATION
> src/slave/flags.hpp d5c54c0
> 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
>
>
Re: Review Request 17567: Added External Containerizer.
Posted by Till Toenshoff <to...@me.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17567/
-----------------------------------------------------------
(Updated April 18, 2014, 3:36 a.m.)
Review request for mesos, Benjamin Hindman, Ian Downes, and Niklas Nielsen.
Changes
-------
- removed fallbacks
- now using recordio for protobuf serializing
- removed containerID parameter
- now using Launch and Update protobufs
- rebased on dependencies
Bugs: MESOS-816
https://issues.apache.org/jira/browse/MESOS-816
Repository: mesos-git
Description (updated)
-------
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).
The protocol for the interactions with the external program
is as follows:
COMMAND < INPUT-PROTO > RESULT-PROTO
launch < Launch
update < Update
usage < ContainerID > ResourceStatistics
wait < ContainerID > Termination
destroy < ContainerID
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 (updated)
-----
configure.ac c1de6d7
src/Makefile.am 560b4c7
src/examples/python/test-containerizer.in PRE-CREATION
src/examples/python/test_containerizer.py PRE-CREATION
src/slave/containerizer/containerizer.hpp d9ae326
src/slave/containerizer/containerizer.cpp 344872a
src/slave/containerizer/external_containerizer.hpp PRE-CREATION
src/slave/containerizer/external_containerizer.cpp PRE-CREATION
src/slave/flags.hpp d5c54c0
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
Re: Review Request 17567: Added External Containerizer.
Posted by Till Toenshoff <to...@me.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17567/
-----------------------------------------------------------
(Updated April 4, 2014, 8:39 a.m.)
Review request for mesos, Benjamin Hindman, Ian Downes, and Niklas Nielsen.
Changes
-------
Rebased on top of all dependencies. Please review.
Bugs: MESOS-816
https://issues.apache.org/jira/browse/MESOS-816
Repository: mesos-git
Description (updated)
-------
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) < ExternalTask > ExternalStatus
update (ContainerID) < ResourceArray > ExternalStatus
usage (ContainerID) > ResourceStatistics
wait (ContainerID) > Termination
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 (updated)
-----
configure.ac c1de6d7
include/mesos/mesos.proto 37f8a7f
src/Makefile.am 95f133d
src/examples/python/test-containerizer.in PRE-CREATION
src/examples/python/test_containerizer.py PRE-CREATION
src/slave/containerizer/containerizer.cpp 344872a
src/slave/containerizer/external_containerizer.hpp PRE-CREATION
src/slave/containerizer/external_containerizer.cpp PRE-CREATION
src/slave/flags.hpp d5c54c0
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
Re: Review Request 17567: Added External Containerizer.
Posted by Till Toenshoff <to...@me.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17567/
-----------------------------------------------------------
(Updated April 3, 2014, 3:36 a.m.)
Review request for mesos, Benjamin Hindman, Ian Downes, and Niklas Nielsen.
Changes
-------
Now reaping the executor pid. Enhanced ExternalStatus to support holding the optional executor pid returned by launch.
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) < ExternalTask > 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.
NOTE: This is currently work in progress to cover all comments.
Diffs (updated)
-----
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
Re: Review Request 17567: Added External Containerizer.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17567/#review39392
-----------------------------------------------------------
Bad patch!
Reviews applied: [17567]
Failed command: git apply --index 17567.patch
Error:
error: patch failed: src/slave/flags.hpp:196
error: src/slave/flags.hpp: patch does not apply
- Mesos ReviewBot
On April 3, 2014, 1:58 a.m., Till Toenshoff wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17567/
> -----------------------------------------------------------
>
> (Updated April 3, 2014, 1:58 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, and Niklas Nielsen.
>
>
> 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) < ExternalTask > 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.
>
>
> NOTE: This is currently work in progress to cover all comments.
>
>
> 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
>
>
Re: Review Request 17567: Added External Containerizer.
Posted by Till Toenshoff <to...@me.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17567/
-----------------------------------------------------------
(Updated April 3, 2014, 1:58 a.m.)
Review request for mesos, Benjamin Hindman, Ian Downes, and Niklas Nielsen.
Bugs: MESOS-816
https://issues.apache.org/jira/browse/MESOS-816
Repository: mesos-git
Description (updated)
-------
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) < ExternalTask > 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.
NOTE: This is currently work in progress to cover all comments.
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
Re: Review Request 17567: Added External Containerizer.
Posted by Till Toenshoff <to...@me.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17567/
-----------------------------------------------------------
(Updated April 3, 2014, 1:57 a.m.)
Review request for mesos, Benjamin Hindman, Ian Downes, and Niklas Nielsen.
Changes
-------
Now prefixing protobuf transmission by the message size. Removed additional launch parameter and wrapped it into a new protobuf (ExternalTask).
Bugs: MESOS-816
https://issues.apache.org/jira/browse/MESOS-816
Repository: mesos-git
Description (updated)
-------
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) < 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.
NOTE: This is currently work in progress to cover all comments.
Diffs (updated)
-----
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
Re: Review Request 17567: Added External Containerizer.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17567/#review39245
-----------------------------------------------------------
Bad patch!
Reviews applied: [19795, 18403, 17567]
Failed command: git apply --index 17567.patch
Error:
error: patch failed: src/slave/flags.hpp:196
error: src/slave/flags.hpp: patch does not apply
- Mesos ReviewBot
On April 2, 2014, 2:02 a.m., Till Toenshoff wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17567/
> -----------------------------------------------------------
>
> (Updated April 2, 2014, 2:02 a.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.
>
>
> NOTE: This is currently work in progress to cover all comments.
>
>
> 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
>
>
Re: Review Request 17567: Added External Containerizer.
Posted by Till Toenshoff <to...@me.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17567/
-----------------------------------------------------------
(Updated April 2, 2014, 2:02 a.m.)
Review request for mesos, Ian Downes, Niklas Nielsen, and Vinod Kone.
Changes
-------
Addressed some comments. Needs rebase and some more comment-addressing.
Bugs: MESOS-816
https://issues.apache.org/jira/browse/MESOS-816
Repository: mesos-git
Description (updated)
-------
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.
NOTE: This is currently work in progress to cover all comments.
Diffs (updated)
-----
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
Re: Review Request 17567: Added External Containerizer.
Posted by Benjamin Mahler <be...@gmail.com>.
Vinod graciously offered to help with this so I can focus on the Registrar
work! I won't be looking at this Till ;)
On Tue, Mar 25, 2014 at 11:55 AM, Till Toenshoff <to...@me.com> 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.
>
>
> Changes
> -------
>
> Me, Niklas and Ian will shepherd this. -- Vinod.
>
>
> 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
>
>
Re: Review Request 17567: Added External Containerizer.
Posted by Till Toenshoff <to...@me.com>.
-----------------------------------------------------------
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.
Changes
-------
Me, Niklas and Ian will shepherd this. -- Vinod.
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
Re: Review Request 17567: Added External Containerizer.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17567/#review38287
-----------------------------------------------------------
Patch looks great!
Reviews applied: [18403, 17567]
All tests passed.
- Mesos ReviewBot
On March 21, 2014, 8:22 p.m., Till Toenshoff wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17567/
> -----------------------------------------------------------
>
> (Updated March 21, 2014, 8:22 p.m.)
>
>
> Review request for mesos, Adam B, Benjamin Hindman, Ben Mahler, 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
>
>
Re: Review Request 17567: Added External Containerizer.
Posted by Till Toenshoff <to...@me.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17567/
-----------------------------------------------------------
(Updated March 21, 2014, 8:22 p.m.)
Review request for mesos, Adam B, Benjamin Hindman, Ben Mahler, Ian Downes, Niklas Nielsen, and Vinod Kone.
Changes
-------
Added setsid for child process - got lost in some refactorings.
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 (updated)
-----
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
Re: Review Request 17567: Added External Containerizer.
Posted by Till Toenshoff <to...@me.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17567/#review38052
-----------------------------------------------------------
I would kindly like to ask for shepherding on this - nominating Vinod/Niklas/BenM in collaboration with Ian.
- Till Toenshoff
On March 21, 2014, 4:11 a.m., Till Toenshoff wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17567/
> -----------------------------------------------------------
>
> (Updated March 21, 2014, 4:11 a.m.)
>
>
> Review request for mesos, Adam B, Benjamin Hindman, Ben Mahler, 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
>
>
Re: Review Request 17567: Added External Containerizer.
Posted by Till Toenshoff <to...@me.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17567/
-----------------------------------------------------------
(Updated March 21, 2014, 4:11 a.m.)
Review request for mesos, Adam B, Benjamin Hindman, Ben Mahler, Ian Downes, Niklas Nielsen, and Vinod Kone.
Changes
-------
Reverted adoption of Subprocess due to multiple issues arising out of its use.
Summary (updated)
-----------------
Added External Containerizer.
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 (updated)
-----
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
Re: Review Request 17567: External Containerizer
Posted by Till Toenshoff <to...@me.com>.
> On March 17, 2014, 4:49 p.m., Niklas Nielsen wrote:
> > src/tests/external_containerizer_test.cpp, lines 223-230
> > <https://reviews.apache.org/r/17567/diff/13/?file=521581#file521581line223>
> >
> > This was based on isolator tests in the containerizer patch set interim; We should be able to use/reuse the same logic as the current isolator_tests.cpp.
Yes, the current ExternalContainerizer tests are not sufficient and need to be updated and enhanced. I would however like to move that into subsequent reviews after having landed this one. It is noted as a TODO right now.
- Till
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17567/#review37391
-----------------------------------------------------------
On March 16, 2014, 10:02 p.m., Till Toenshoff wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17567/
> -----------------------------------------------------------
>
> (Updated March 16, 2014, 10:02 p.m.)
>
>
> Review request for mesos, Adam B, Benjamin Hindman, Ben Mahler, 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 9a6de87
> 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 d0a1023
> 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
>
>
Re: Review Request 17567: External Containerizer
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17567/#review37391
-----------------------------------------------------------
src/slave/containerizer/external_containerizer.cpp
<https://reviews.apache.org/r/17567/#comment68908>
Kill newline
src/tests/external_containerizer_test.cpp
<https://reviews.apache.org/r/17567/#comment68906>
This was based on isolator tests in the containerizer patch set interim; We should be able to use/reuse the same logic as the current isolator_tests.cpp.
src/tests/external_containerizer_test.cpp
<https://reviews.apache.org/r/17567/#comment68905>
Please delete this :) It's a leftover from early iterations.
- Niklas Nielsen
On March 16, 2014, 3:02 p.m., Till Toenshoff wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17567/
> -----------------------------------------------------------
>
> (Updated March 16, 2014, 3:02 p.m.)
>
>
> Review request for mesos, Adam B, Benjamin Hindman, Ben Mahler, 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 9a6de87
> 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 d0a1023
> 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
>
>