----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/16149/#review33918 -----------------------------------------------------------
Ship it! src/slave/containerizer/cgroups_launcher.hpp <https://reviews.apache.org/r/16149/#comment63678> Kill the newline. src/slave/containerizer/cgroups_launcher.cpp <https://reviews.apache.org/r/16149/#comment63683> Why is this an instance field? src/slave/containerizer/cgroups_launcher.cpp <https://reviews.apache.org/r/16149/#comment63682> I know the original CgroupsIsolator had lots of CHECKs and EXITs but we should really start transitioning them over to Errors. src/slave/containerizer/cgroups_launcher.cpp <https://reviews.apache.org/r/16149/#comment63684> Same comments in other reviews as to why not returning Errors. src/slave/containerizer/cgroups_launcher.cpp <https://reviews.apache.org/r/16149/#comment63685> Ignore EINTR? src/slave/containerizer/cgroups_launcher.cpp <https://reviews.apache.org/r/16149/#comment63687> I'm confused how you guarantee that this process has the same session as 'pgid' as required by 'setpgid'? I'm guessing this code is not exercised anywhere (including tests) since we don't fork extra processes in a container. src/slave/containerizer/cgroups_launcher.cpp <https://reviews.apache.org/r/16149/#comment63686> I don't think 'perror' is signal safe but I also think that's okay. src/slave/containerizer/cgroups_launcher.cpp <https://reviews.apache.org/r/16149/#comment63688> Ignore EINTR? src/slave/containerizer/cgroups_launcher.cpp <https://reviews.apache.org/r/16149/#comment63689> Why '_exit' here but 'abort' above? I noticed this in the last review too. src/slave/containerizer/cgroups_launcher.cpp <https://reviews.apache.org/r/16149/#comment63690> This feels like a bit of a hack. Why not 'return -1' or even an 'Error("unreachable")' or use 'UNREACHABLE()' rather than give the illusion that 'inChild' is actually supposed to return anything? If you did want 'inChild' to return something you should really be doing 'exit(inChild())'. src/slave/containerizer/cgroups_launcher.cpp <https://reviews.apache.org/r/16149/#comment63691> & src/slave/containerizer/launcher.hpp <https://reviews.apache.org/r/16149/#comment63679> Above the stout headers please. src/slave/containerizer/launcher.cpp <https://reviews.apache.org/r/16149/#comment63692> See comments above re: session. src/slave/containerizer/launcher.cpp <https://reviews.apache.org/r/16149/#comment63680> See comments above. src/slave/containerizer/launcher.cpp <https://reviews.apache.org/r/16149/#comment63681> Space after 'container'. src/slave/containerizer/launcher.cpp <https://reviews.apache.org/r/16149/#comment63693> I don't understand your comment. Do you mean killtree will fail if the "root" process isn't found? The code below seems a bit strange: if there is only the root pid than os::pids will return a non-empty set and we'll just execute killtree as normal so it's unclear what doing os::pids first bought us. If the root process no longer exists but some other processes from the session still exist then calling os::pids will return a non-empty set but then calling killtree will fail and we'll leave some processes running. It seems like we want to try and do killtree and maybe also try and kill all processes in the process group and session? src/slave/containerizer/launcher.cpp <https://reviews.apache.org/r/16149/#comment63694> What do you mean by parent here? src/slave/containerizer/launcher.cpp <https://reviews.apache.org/r/16149/#comment63695> We should not be able to reap any granchildren, only each 'tree.process.pid' in the returned 'trees' from 'killtree'. - Benjamin Hindman On Feb. 7, 2014, 7:09 a.m., Ian Downes wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/16149/ > ----------------------------------------------------------- > > (Updated Feb. 7, 2014, 7:09 a.m.) > > > Review request for mesos, Benjamin Hindman, Ben Mahler, Chi Zhang, Niklas > Nielsen, samya, and Jason Dusek. > > > Repository: mesos-git > > > Description > ------- > > Launcher interface and MesosLauncher to support MesosContainerizers. > > Launchers handle the lifecycle of the executor process (and descendants). > > > Diffs > ----- > > src/Makefile.am c30706846bca1fa3287291e39f46a23713ad1ba4 > src/slave/containerizer/cgroups_launcher.hpp PRE-CREATION > src/slave/containerizer/cgroups_launcher.cpp PRE-CREATION > src/slave/containerizer/launcher.hpp PRE-CREATION > src/slave/containerizer/launcher.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/16149/diff/ > > > Testing > ------- > > > Thanks, > > Ian Downes > >
