Review Request 13038: Use clone instead of fork to create the executor process

[Eric Biederman <eb...@xmission.com>]

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

Review request for mesos, Benjamin Hindman, Ben Mahler, Ian Downes, and Vinod Kone.


Repository: mesos-git


Description
-------

Use clone instead of fork to create the executor process

In preparation for taking advantage of the linux namespaces replace the fork
before the exec of the executor with a call to clone.

For the moment this preserves the existing behavior exactly, but this provides
the opportunity for creating namespaces to isolate the executor with at the
when the process for the executor is created.

clone(3) requires an extra stack and for the code to run in the child
to be passed as a function.  I would prefer to build a wrapper of
clone that doesn't require these unneeded shenanigans but glibc caches
the processes current pid, and only invalidates the cache if you use
one of glibc's wrappers of clone, fork and vfork. Sigh.


Diffs
-----

  src/slave/cgroups_isolator.hpp e86062e 
  src/slave/cgroups_isolator.cpp 0faf7d5 

Diff: https://reviews.apache.org/r/13038/diff/


Testing
-------

make -j8 check

And all of the tests pass.


Thanks,

Eric Biederman

Re: Review Request 13038: Use clone instead of fork to create the executor process

[Vinod Kone <vi...@gmail.com>]

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



src/slave/cgroups_isolator.hpp
<https://reviews.apache.org/r/13038/#comment48213>

    Hmm. I'm not sure I like this struct. For one some of the variables are in (or should be in) CgroupInfo. More importantly, with clone cant the child process simply access this data from the shared memory? Was there a reason you explicitly passed it via a struct?



src/slave/cgroups_isolator.cpp
<https://reviews.apache.org/r/13038/#comment48212>

    why 16384?



src/slave/cgroups_isolator.cpp
<https://reviews.apache.org/r/13038/#comment48132>

    any particular reason you pulled out executorId into a variable?



src/slave/cgroups_isolator.cpp
<https://reviews.apache.org/r/13038/#comment48131>

    new line.


- Vinod Kone


On July 29, 2013, 10:22 p.m., Eric Biederman wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/13038/
> -----------------------------------------------------------
> 
> (Updated July 29, 2013, 10:22 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Ben Mahler, Ian Downes, and Vinod Kone.
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> Use clone instead of fork to create the executor process
> 
> In preparation for taking advantage of the linux namespaces replace the fork
> before the exec of the executor with a call to clone.
> 
> For the moment this preserves the existing behavior exactly, but this provides
> the opportunity for creating namespaces to isolate the executor with at the
> when the process for the executor is created.
> 
> clone(3) requires an extra stack and for the code to run in the child
> to be passed as a function.  I would prefer to build a wrapper of
> clone that doesn't require these unneeded shenanigans but glibc caches
> the processes current pid, and only invalidates the cache if you use
> one of glibc's wrappers of clone, fork and vfork. Sigh.
> 
> 
> Diffs
> -----
> 
>   src/slave/cgroups_isolator.hpp e86062e 
>   src/slave/cgroups_isolator.cpp 0faf7d5 
> 
> Diff: https://reviews.apache.org/r/13038/diff/
> 
> 
> Testing
> -------
> 
> make -j8 check
> 
> And all of the tests pass.
> 
> 
> Thanks,
> 
> Eric Biederman
> 
>