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