----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53837/#review156769 -----------------------------------------------------------
We should probably split the server code out and add unit test for the server. src/slave/containerizer/mesos/io/switchboard.cpp (line 216) <https://reviews.apache.org/r/53837/#comment227043> Should we close the pipe created above? I'd suggest we maintain a list of fds and introduce an inline lambda to close all of them. Similar to what we do in ns::clone. src/slave/containerizer/mesos/io/switchboard.cpp (line 225) <https://reviews.apache.org/r/53837/#comment227044> Ditto. src/slave/containerizer/mesos/io/switchboard.cpp (line 266) <https://reviews.apache.org/r/53837/#comment227073> i'd just call it instead of using namespace: `IOSwitchboardFlags flags` src/slave/containerizer/mesos/io/switchboard.cpp (lines 272 - 273) <https://reviews.apache.org/r/53837/#comment227074> Do you need this one? Can you just inline `{}` in subprocess call below? src/slave/containerizer/mesos/io/switchboard.cpp (lines 291 - 294) <https://reviews.apache.org/r/53837/#comment227065> Instead of closing them in a parent hook which blocks the child, can you just close them in after 'subprocess()' call. src/slave/containerizer/mesos/io/switchboard_main.hpp (line 29) <https://reviews.apache.org/r/53837/#comment227076> I would move `NAME` to IOSwitchboard class. src/slave/containerizer/mesos/io/switchboard_main.hpp (line 30) <https://reviews.apache.org/r/53837/#comment227066> I'd suggest we just use os::mktemp(). Attaching a container id to the socket path means that we might not be able to handle a deep nested container. src/slave/containerizer/mesos/io/switchboard_main.hpp (line 33) <https://reviews.apache.org/r/53837/#comment227078> I'd move this to IOSwitchboard class as well: `IOSwitchboard::Flags` src/slave/containerizer/mesos/io/switchboard_main.cpp (line 70) <https://reviews.apache.org/r/53837/#comment227079> s/Switchboard/IOSwitchboard/ In fact, i'd prefer we move this impl. to switchboard.cpp and let the main.cpp be very simple. In that way, you can unit test those actors. src/slave/containerizer/mesos/io/switchboard_main.cpp (lines 74 - 75) <https://reviews.apache.org/r/53837/#comment227094> I'd suggest we take explicit parameters for this actor, instead of taking flags. Also, given that we will only have one actor here, no need to use Owned pointer for Promise anymore. src/slave/containerizer/mesos/io/switchboard_main.cpp (lines 80 - 83) <https://reviews.apache.org/r/53837/#comment227091> I don't think this is necessary. src/slave/containerizer/mesos/io/switchboard_main.cpp (line 86) <https://reviews.apache.org/r/53837/#comment227092> I would move this to initialize method. src/slave/containerizer/mesos/io/switchboard_main.cpp (line 94) <https://reviews.apache.org/r/53837/#comment227085> Any reason you need a Owned<Socket> here? src/slave/containerizer/mesos/io/switchboard_main.cpp (line 96) <https://reviews.apache.org/r/53837/#comment227088> See my comments above. I'd prefer we use a random tmp dir for the socket file and save a symlink in the runtime dir. This way, we don't need to worry about a too deep nested container. src/slave/containerizer/mesos/io/switchboard_main.cpp (line 110) <https://reviews.apache.org/r/53837/#comment227093> you probably should start the io::splice for stdout and stderr as well here with a defer callback to the receiveOutput method. src/slave/containerizer/mesos/io/switchboard_main.cpp (line 170) <https://reviews.apache.org/r/53837/#comment227084> This is basically io::splice with a registered callback. I am wondering if we should just modify io::splice to take a lambda callback, instead of reinventing the wheel here. src/slave/containerizer/mesos/io/switchboard_main.cpp (lines 297 - 303) <https://reviews.apache.org/r/53837/#comment227083> This should be a child hook of the subprocess call, instead of doing that in main here. src/slave/containerizer/mesos/io/switchboard_main.cpp (lines 320 - 324) <https://reviews.apache.org/r/53837/#comment227090> I'd suggest we simply wait for the termination of the IOSwitchboardServer actor and if it terminates, we terminate the linux process. Intead of rely on the return value of 'run' method, the standard pattern is to have a method 'future' which returns the future of the promise. And then, spawn the process. See src/log/consensus.cpp for examples. Therefore, in IOSwitchboardServer, whenever there is an error, you can simply terminate(self()) and fail the promise. - Jie Yu On Nov. 23, 2016, 2:45 a.m., Kevin Klues wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/53837/ > ----------------------------------------------------------- > > (Updated Nov. 23, 2016, 2:45 a.m.) > > > Review request for mesos, Benjamin Hindman, Jie Yu, and Vinod Kone. > > > Bugs: MESOS-6467 > https://issues.apache.org/jira/browse/MESOS-6467 > > > Repository: mesos > > > Description > ------- > > Currently, this process simply intercepts the stdout/stderr of a > container and writes it to the stdout/stderr FDs set up by the > container logger. We also send the stdout/stderr data to a simple HTTP > server that we launch on a unix domain socket set up by the agent. > Right now this server is just a stub and doesn't do anything useful. > > In future commits, we will expand this HTTP server to handle > 'ATTACH_CONTAINER_INPUT' and 'ATTACH_CONTAINER_OUTPUT' calls on behalf > of a container. It will use the stdout/stderr messages passed to it to > and send that data over the response stream to any clients connected > with an 'ATTACH_CONTAINER_OUTPUT' call. Likewise, it will take any > input streamed in over a 'ATTACH_CONTAINER_INPUT' request and write it > to a container's stdin. > > We don't currently handle recovering access to the io switchboard > server process after agent restarts. We will add that in a subsequent > commit. > > > Diffs > ----- > > src/Makefile.am 5e0b8406f7f624bd8b03ff76b887f20e22fc66e0 > src/slave/containerizer/mesos/containerizer.cpp > e47a120bfbb607cc0cdbdaed934ae15f15666ae3 > src/slave/containerizer/mesos/io/switchboard.hpp PRE-CREATION > src/slave/containerizer/mesos/io/switchboard.cpp PRE-CREATION > src/slave/containerizer/mesos/io/switchboard_main.hpp PRE-CREATION > src/slave/containerizer/mesos/io/switchboard_main.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/53837/diff/ > > > Testing > ------- > > GTEST_FILTER="" make -j check > sudo src/mesos-tests > > > Thanks, > > Kevin Klues > >
