mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Qian Zhang <zhq527...@gmail.com>
Subject Re: Review Request 69681: Fixed the FD leak if containerizer::_launch() failed or discarded.
Date Mon, 07 Jan 2019 14:04:00 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69681/#review211724
-----------------------------------------------------------




include/mesos/slave/containerizer.hpp
Lines 85 (patched)
<https://reviews.apache.org/r/69681/#comment297279>

    s/Option<int_fd>()/Option<int_fd>::::none()/



include/mesos/slave/containerizer.hpp
Lines 90 (patched)
<https://reviews.apache.org/r/69681/#comment297280>

    Ditto.



src/slave/containerizer/mesos/containerizer.cpp
Lines 1366-1371 (original), 1368-1373 (patched)
<https://reviews.apache.org/r/69681/#comment297284>

    Can we do the `repair` inside this `then()`? Something like:
    ```
          .then(defer(self(), [=](const Option<ContainerIO>& containerIO) {
            return _launch(containerId, containerIO, environment, pidCheckpointPath)
              .repair([&containerIO](
                  const Future<Containerizer::LaunchResult>& result) {
                CHECK(containerIO.isSome());
                closeContainerIoFds(containerIO.get());
                return result;
              });
          }));
    ```
    In this way we do not need the local variable `_containerIO`.



src/slave/containerizer/mesos/utils.cpp
Lines 22 (patched)
<https://reviews.apache.org/r/69681/#comment297282>

    Why do we need this header?



src/slave/containerizer/mesos/utils.cpp
Lines 153-154 (patched)
<https://reviews.apache.org/r/69681/#comment297281>

    We could merge these two lines into one.


- Qian Zhang


On Jan. 7, 2019, 8:22 p.m., Gilbert Song wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69681/
> -----------------------------------------------------------
> 
> (Updated Jan. 7, 2019, 8:22 p.m.)
> 
> 
> Review request for mesos, Andrei Budnik, Jie Yu, Qian Zhang, and Vinod Kone.
> 
> 
> Bugs: MESOS-6632
>     https://issues.apache.org/jira/browse/MESOS-6632
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> For the period between IOSB server is up and container process exec,
> if the containerizer launch returns failure or discarded, the FD used
> for signaling from the container process to the IOSB finish redirect
> will be leaked, which would cause the IOSB stuck at io::redirect
> forever. It would block the containerizer from cleaning up this
> container at IOSB.
> 
> This issue could be exposed if there are frequent check containers
> launch on an agent with heavy loads.
> 
> 
> Diffs
> -----
> 
>   include/mesos/slave/containerizer.hpp c34f1296586ea5d26619605f8f6a5ae9444f1a96 
>   src/slave/containerizer/mesos/containerizer.cpp a5cf2da55c046c5c45e0c2ca3400f64de12de62b

>   src/slave/containerizer/mesos/io/switchboard.cpp c445a8f09d7671d5763281bac9881489b3cc9c39

>   src/slave/containerizer/mesos/utils.hpp bfd07e28c78fc140e395ffccd11d65545bf007fc 
>   src/slave/containerizer/mesos/utils.cpp d9964f0b0e8ad8a41e5f5490a1c28bba9a972ae0 
> 
> 
> Diff: https://reviews.apache.org/r/69681/diff/1/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>


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