> On Jan. 28, 2014, 7:13 p.m., Vinod Kone wrote: > > src/slave/containerizer/cgroups_launcher.cpp, lines 162-165 > > <https://reviews.apache.org/r/16149/diff/7/?file=451161#file451161line162> > > > > Why is this an error? > > > > If the cgroup was removed but the slave died before it got the signal > > from the containerizer, wouldn't we be here? > > > > I think the right thing to do here is to just skip it.
Good point; I've amended it to skip this case. I also commented how the containerizer will react (monitor pid -> destroy) > On Jan. 28, 2014, 7:13 p.m., Vinod Kone wrote: > > src/slave/containerizer/cgroups_launcher.cpp, line 197 > > <https://reviews.apache.org/r/16149/diff/7/?file=451161#file451161line197> > > > > How is it possible that a cgroup for this container already exists? If > > it does, shouldn't we return an error? Launchers support forking multiple processes for a container. This will be used in the near future. PosixLauncher will put each new process into the same session, CgroupsLauncher will put each new process into the same freezer cgroup. > On Jan. 28, 2014, 7:13 p.m., Vinod Kone wrote: > > src/slave/containerizer/cgroups_launcher.cpp, line 207 > > <https://reviews.apache.org/r/16149/diff/7/?file=451161#file451161line207> > > > > s/it's/its/ it has -> it's > On Jan. 28, 2014, 7:13 p.m., Vinod Kone wrote: > > src/slave/containerizer/cgroups_launcher.cpp, line 264 > > <https://reviews.apache.org/r/16149/diff/7/?file=451161#file451161line264> > > > > The older semantics was to also do a setsid() here. Why the regression? Assumption is that sessions are not necessary when the processes are contained in a cgroup. > On Jan. 28, 2014, 7:13 p.m., Vinod Kone wrote: > > src/slave/containerizer/launcher.cpp, lines 45-47 > > <https://reviews.apache.org/r/16149/diff/7/?file=451163#file451163line45> > > > > ditto. see the comment in cgroupslauncher::recover(). This case is a little different: MesosContainerizer only tries to recover containers with pids and skips those without so it is an error to try to recover a container without a pid. In the CgroupsLauncher it was possible that there was a pid but the cgroup had been cleaned up so there it was correct to change to skip. > On Jan. 28, 2014, 7:13 p.m., Vinod Kone wrote: > > src/slave/containerizer/launcher.cpp, lines 49-51 > > <https://reviews.apache.org/r/16149/diff/7/?file=451163#file451163line49> > > > > How is this possible? If there is pid wrapping? If yes, please add a > > comment. Technically it could happen with just the right timing of executor exiting and new executor with same pid and slave dying after new but before hearing about old. Regardless, the launcher cannot support this so it's considered an error. comments added. - Ian ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/16149/#review33012 ----------------------------------------------------------- On Jan. 27, 2014, 3:02 a.m., Ian Downes wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/16149/ > ----------------------------------------------------------- > > (Updated Jan. 27, 2014, 3:02 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 cf0c8c66e8dd21c2f5a2da22e5d5adb056353454 > 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 > >
