----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53837/#review156684 -----------------------------------------------------------
This is a partial review. Will continue to review the rest while you're addressing the existing issues. src/slave/containerizer/mesos/containerizer.cpp (lines 1400 - 1411) <https://reviews.apache.org/r/53837/#comment226923> hum, that means we cannot use attach with local mode? I think we should pass 'local' to io switchboard prepare. 'local' simply means redirect the container io to stdio (in fact, maybe we should have a local logger?). However, we should still be allowed to do attach. In local mode, we probably want to run io switchboard as an actor in the same linux process, rather than forking a new process. Thoughts? src/slave/containerizer/mesos/containerizer.cpp (lines 2386 - 2387) <https://reviews.apache.org/r/53837/#comment226907> See my comments below. I'd suggest we don't do wait/destroy for io switchboard yet. src/slave/containerizer/mesos/io_switchboard/io_switchboard.cpp (line 153) <https://reviews.apache.org/r/53837/#comment226922> ContainerLogger src/slave/containerizer/mesos/io_switchboard/io_switchboard.cpp (line 155) <https://reviews.apache.org/r/53837/#comment226921> Looks like the defer here is not necessary. src/slave/containerizer/mesos/io_switchboard/io_switchboard.cpp (line 166) <https://reviews.apache.org/r/53837/#comment227041> Realized an FD leak in logger code. This is not yours because the original code has the same leak. When we return error below, we need to make sure that fds in loggerInfo are closed. src/slave/containerizer/mesos/io_switchboard/io_switchboard.cpp (lines 335 - 341) <https://reviews.apache.org/r/53837/#comment226902> So this is a TODO for agent restart case? What's the plan moving forward? Looks like previously, we don't handle logger process destory and wait as well. I'd suggest we don't do this optimization yet. Moving forward, do we plan to move the processes (including io switchboard and logger) to the container's cgroup? If that's the case, maybe it makes sense to make this an isolator (wrapping ioswitch board and logger, e.g., 'posix/io' isolator) which is always enabled (like filesystem or network isolator). The subprocess io info can be part of the ContainerLaunchInfo. If we put those processes in the same cgroup, we don't have to worry about destroy/wait anymore. Thoughts? - 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 > >
