----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51278/#review150361 -----------------------------------------------------------
src/slave/containerizer/mesos/linux_launcher.cpp (line 105) <https://reviews.apache.org/r/51278/#comment218326> = None() is not necessary. src/slave/containerizer/mesos/linux_launcher.cpp (lines 108 - 110) <https://reviews.apache.org/r/51278/#comment218327> Do you still need this function? src/slave/containerizer/mesos/linux_launcher.cpp (lines 257 - 259) <https://reviews.apache.org/r/51278/#comment218328> checkpointed pid? Please update the comment here src/slave/containerizer/mesos/linux_launcher.cpp (line 270) <https://reviews.apache.org/r/51278/#comment218329> `return Failure("Failed to list cgroups under <cgroups_root>: " + cgroups.error());` src/slave/containerizer/mesos/linux_launcher.cpp (lines 276 - 289) <https://reviews.apache.org/r/51278/#comment218330> I suggest we factor out into a helper: ``` ContainerID containerId = getContainerId(cgroup); ``` src/slave/containerizer/mesos/linux_launcher.cpp (lines 308 - 319) <https://reviews.apache.org/r/51278/#comment218331> launcher destroy will be called for those orphans that launcher do not know about as well (e.g., launcher destroy succeeds, but isolator cleanup is not done yet, agent crashes). Therefore, I'd suggest we adjust the destroy interface to return a Future<bool>. 'false' means the container is unknown to the launcher. Let the caller decide what to do. In Mesos containerizer, if the launcher does not know about a container in the destroy path, we probably should just continue the chain. In that way, you don't have to do the reconciliation here. src/slave/containerizer/mesos/linux_launcher.cpp (line 356) <https://reviews.apache.org/r/51278/#comment218332> count returns 'size_t'. So please use xxx.count() != 0 src/slave/containerizer/mesos/linux_launcher.cpp (line 475) <https://reviews.apache.org/r/51278/#comment218333> space after `:` src/slave/containerizer/mesos/linux_launcher.cpp (line 508) <https://reviews.apache.org/r/51278/#comment218334> Ditto. Return a false here src/slave/containerizer/mesos/linux_launcher.cpp (lines 520 - 526) <https://reviews.apache.org/r/51278/#comment218335> I won't worry about this case because the containerizer will make sure cleanup/destroy is only called once for launcher/isolator/provisioner. But I like the fact we remove the container from `containers` map so that other calls to launcher will be properly handled (e.g., status). src/slave/containerizer/mesos/linux_launcher.cpp (line 567) <https://reviews.apache.org/r/51278/#comment218336> using executor_pid here is weird. I think we should rethink how we should `ContainerStatus` when we have nested containers. Should we rename it to `pid`? Sounds like 'executor_pid' is not very precise. Another to consider is: in the future, user might want to get the pid of it's actual process, not the 'init' pid we forked. What should we call that? - Jie Yu On Sept. 25, 2016, 4:08 p.m., Benjamin Hindman wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/51278/ > ----------------------------------------------------------- > > (Updated Sept. 25, 2016, 4:08 p.m.) > > > Review request for mesos, Gilbert Song, Jie Yu, and Kevin Klues. > > > Repository: mesos > > > Description > ------- > > Refactored LinuxLauncher to be nested container aware. > > > Diffs > ----- > > src/slave/containerizer/mesos/launch.hpp > 859990cb85e9e8c06400397256cfc512f0811800 > src/slave/containerizer/mesos/linux_launcher.hpp > 2cd1e4d4b6d9e656c2447a9d3ab8e1f49ba5fffa > src/slave/containerizer/mesos/linux_launcher.cpp > f27757032cc28be0f0067bed045536431dde4d6c > > Diff: https://reviews.apache.org/r/51278/diff/ > > > Testing > ------- > > > Thanks, > > Benjamin Hindman > >
