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