mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jie Yu <yujie....@gmail.com>
Subject Re: Review Request 53837: Added a per container io switchboard server process.
Date Wed, 23 Nov 2016 19:28:10 GMT

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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message