Re: Review Request 63280: Made the containerizer launch be explicit about O_CLOEXEC.

[Gilbert Song]

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


Ship it!




Ship It!

- Gilbert Song


On Aug. 17, 2018, 11:14 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63280/
> ---
> 
> (Updated Aug. 17, 2018, 11:14 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Benjamin Bannier, Benjamin 
> Mahler, Gilbert Song, Greg Mann, and Jie Yu.
> 
> 
> Bugs: MESOS-8128
> https://issues.apache.org/jira/browse/MESOS-8128
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Since the containerizer launch depends on the inherited pipe to signal
> the forked child, be explicit about manipulating O_CLOEXEC on the pipe
> file descriptors. Make sure to close the pipe on the error paths.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 98129d006cda9b65804b518619b6addc8990410a 
>   src/slave/containerizer/mesos/launcher.cpp 
> ed41a14892bdfa74d28bd0541ca188b86f8f96f8 
>   src/slave/containerizer/mesos/linux_launcher.cpp 
> d1f8d3f56a9ef3aa98012c9395e74732e49c264d 
> 
> 
> Diff: https://reviews.apache.org/r/63280/diff/4/
> 
> 
> Testing
> ---
> 
> sudo make check (Fedora 28)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 63280: Made the containerizer launch be explicit about O_CLOEXEC.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On Aug. 17, 2018, 6:14 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63280/
> ---
> 
> (Updated Aug. 17, 2018, 6:14 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Benjamin Bannier, Benjamin 
> Mahler, Gilbert Song, Greg Mann, and Jie Yu.
> 
> 
> Bugs: MESOS-8128
> https://issues.apache.org/jira/browse/MESOS-8128
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Since the containerizer launch depends on the inherited pipe to signal
> the forked child, be explicit about manipulating O_CLOEXEC on the pipe
> file descriptors. Make sure to close the pipe on the error paths.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 98129d006cda9b65804b518619b6addc8990410a 
>   src/slave/containerizer/mesos/launcher.cpp 
> ed41a14892bdfa74d28bd0541ca188b86f8f96f8 
>   src/slave/containerizer/mesos/linux_launcher.cpp 
> d1f8d3f56a9ef3aa98012c9395e74732e49c264d 
> 
> 
> Diff: https://reviews.apache.org/r/63280/diff/4/
> 
> 
> Testing
> ---
> 
> sudo make check (Fedora 28)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 63280: Made the containerizer launch be explicit about O_CLOEXEC.

[James Peach]

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

(Updated Aug. 17, 2018, 6:14 p.m.)


Review request for mesos, Andrew Schwartzmeyer, Benjamin Bannier, Benjamin 
Mahler, Gilbert Song, Greg Mann, and Jie Yu.


Bugs: MESOS-8128
https://issues.apache.org/jira/browse/MESOS-8128


Repository: mesos


Description
---

Since the containerizer launch depends on the inherited pipe to signal
the forked child, be explicit about manipulating O_CLOEXEC on the pipe
file descriptors. Make sure to close the pipe on the error paths.


Diffs (updated)
-

  src/slave/containerizer/mesos/containerizer.cpp 
98129d006cda9b65804b518619b6addc8990410a 
  src/slave/containerizer/mesos/launcher.cpp 
ed41a14892bdfa74d28bd0541ca188b86f8f96f8 
  src/slave/containerizer/mesos/linux_launcher.cpp 
d1f8d3f56a9ef3aa98012c9395e74732e49c264d 


Diff: https://reviews.apache.org/r/63280/diff/4/

Changes: https://reviews.apache.org/r/63280/diff/3-4/


Testing
---

sudo make check (Fedora 28)


Thanks,

James Peach

Re: Review Request 63280: Made the containerizer launch be explicit about O_CLOEXEC.

[James Peach]

> On May 24, 2018, 10:36 p.m., Andrew Schwartzmeyer wrote:
> > src/slave/containerizer/mesos/containerizer.cpp
> > Lines 1820 (patched)
> > 
> >
> > Shouldn't we also close the parent's copy of the read end of the pipe 
> > immediately (and unconditionally) after forking? (Similar to what we do for 
> > the in/out/err pipes.)
> 
> James Peach wrote:
> The parent still needs this pipe. However, as @jieyu points out offline, 
> this is still racey, so the right approach is to find some way to apply the 
> `UNSET_CLOEXEC` child hook.

Updated to `UNSET_CLOEXEC` the whitelisted FDs.


- James


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


On May 24, 2018, 9:16 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63280/
> ---
> 
> (Updated May 24, 2018, 9:16 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Benjamin Bannier, Benjamin 
> Mahler, Greg Mann, and Jie Yu.
> 
> 
> Bugs: MESOS-8128
> https://issues.apache.org/jira/browse/MESOS-8128
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Since the containerizer launch depends on the inherited pipe to signal
> the forked child, be explicit about setting O_CLOEXEC on the pipe file
> descriptors. Make sure to close the pipe on the error paths.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 100e3bbda543d87808da9ff6bea42da5099ea8c5 
> 
> 
> Diff: https://reviews.apache.org/r/63280/diff/2/
> 
> 
> Testing
> ---
> 
> make check (Fedora 25)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 63280: Made the containerizer launch be explicit about O_CLOEXEC.

[James Peach]

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

(Updated Aug. 17, 2018, 5:54 p.m.)


Review request for mesos, Andrew Schwartzmeyer, Benjamin Bannier, Benjamin 
Mahler, Greg Mann, and Jie Yu.


Changes
---

Rebased.


Bugs: MESOS-8128
https://issues.apache.org/jira/browse/MESOS-8128


Repository: mesos


Description (updated)
---

Since the containerizer launch depends on the inherited pipe to signal
the forked child, be explicit about manipulating O_CLOEXEC on the pipe
file descriptors. Make sure to close the pipe on the error paths.


Diffs (updated)
-

  src/slave/containerizer/mesos/containerizer.cpp 
98129d006cda9b65804b518619b6addc8990410a 
  src/slave/containerizer/mesos/launcher.cpp 
ed41a14892bdfa74d28bd0541ca188b86f8f96f8 
  src/slave/containerizer/mesos/linux_launcher.cpp 
d1f8d3f56a9ef3aa98012c9395e74732e49c264d 


Diff: https://reviews.apache.org/r/63280/diff/3/

Changes: https://reviews.apache.org/r/63280/diff/2-3/


Testing
---

make check (Fedora 25)


Thanks,

James Peach

Re: Review Request 63280: Made the containerizer launch be explicit about O_CLOEXEC.

[Gilbert Song]

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



Could we rebase?

- Gilbert Song


On May 24, 2018, 2:16 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63280/
> ---
> 
> (Updated May 24, 2018, 2:16 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Benjamin Bannier, Benjamin 
> Mahler, Greg Mann, and Jie Yu.
> 
> 
> Bugs: MESOS-8128
> https://issues.apache.org/jira/browse/MESOS-8128
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Since the containerizer launch depends on the inherited pipe to signal
> the forked child, be explicit about setting O_CLOEXEC on the pipe file
> descriptors. Make sure to close the pipe on the error paths.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 100e3bbda543d87808da9ff6bea42da5099ea8c5 
> 
> 
> Diff: https://reviews.apache.org/r/63280/diff/2/
> 
> 
> Testing
> ---
> 
> make check (Fedora 25)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 63280: Made the containerizer launch be explicit about O_CLOEXEC.

[James Peach]

> On May 24, 2018, 10:36 p.m., Andrew Schwartzmeyer wrote:
> > src/slave/containerizer/mesos/containerizer.cpp
> > Lines 1820 (patched)
> > 
> >
> > Shouldn't we also close the parent's copy of the read end of the pipe 
> > immediately (and unconditionally) after forking? (Similar to what we do for 
> > the in/out/err pipes.)

The parent still needs this pipe. However, as @jieyu points out offline, this 
is still racey, so the right approach is to find some way to apply the 
`UNSET_CLOEXEC` child hook.


- James


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


On May 24, 2018, 9:16 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63280/
> ---
> 
> (Updated May 24, 2018, 9:16 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Benjamin Bannier, Benjamin 
> Mahler, Greg Mann, and Jie Yu.
> 
> 
> Bugs: MESOS-8128
> https://issues.apache.org/jira/browse/MESOS-8128
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Since the containerizer launch depends on the inherited pipe to signal
> the forked child, be explicit about setting O_CLOEXEC on the pipe file
> descriptors. Make sure to close the pipe on the error paths.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 100e3bbda543d87808da9ff6bea42da5099ea8c5 
> 
> 
> Diff: https://reviews.apache.org/r/63280/diff/2/
> 
> 
> Testing
> ---
> 
> make check (Fedora 25)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 63280: Made the containerizer launch be explicit about O_CLOEXEC.

[Andrew Schwartzmeyer]

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




src/slave/containerizer/mesos/containerizer.cpp
Lines 1815-1817 (original), 1818-1823 (patched)


Just a heads up, this chain is making related changes: 
https://reviews.apache.org/r/67288/



src/slave/containerizer/mesos/containerizer.cpp
Lines 1820 (patched)


Shouldn't we also close the parent's copy of the read end of the pipe 
immediately (and unconditionally) after forking? (Similar to what we do for the 
in/out/err pipes.)


- Andrew Schwartzmeyer


On May 24, 2018, 2:16 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63280/
> ---
> 
> (Updated May 24, 2018, 2:16 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Benjamin Bannier, Benjamin 
> Mahler, Greg Mann, and Jie Yu.
> 
> 
> Bugs: MESOS-8128
> https://issues.apache.org/jira/browse/MESOS-8128
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Since the containerizer launch depends on the inherited pipe to signal
> the forked child, be explicit about setting O_CLOEXEC on the pipe file
> descriptors. Make sure to close the pipe on the error paths.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 100e3bbda543d87808da9ff6bea42da5099ea8c5 
> 
> 
> Diff: https://reviews.apache.org/r/63280/diff/2/
> 
> 
> Testing
> ---
> 
> make check (Fedora 25)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 63280: Made the containerizer launch be explicit about O_CLOEXEC.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 63280 was successfully built and tested.

Reviews applied: `['63270', '63280']`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/63280

- Mesos Reviewbot Windows


On Oct. 24, 2017, 11:29 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63280/
> ---
> 
> (Updated Oct. 24, 2017, 11:29 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Greg Mann, and Jie Yu.
> 
> 
> Bugs: MESOS-8128
> https://issues.apache.org/jira/browse/MESOS-8128
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Since the containerizer launch depends on the inherited pipe to signal
> the forked child, be explicit about setting O_CLOEXEC on the pipe file
> descriptors. Make sure to close the pipe on the error paths.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 100e3bbda543d87808da9ff6bea42da5099ea8c5 
> 
> 
> Diff: https://reviews.apache.org/r/63280/diff/1/
> 
> 
> Testing
> ---
> 
> make check (Fedora 25)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 63280: Made the containerizer launch be explicit about O_CLOEXEC.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [63270, 63280]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose --disable-libtool-wrappers' ENVIRONMENT='GLOG_v=1 
MESOS_VERBOSE=1'; ./support/docker-build.sh

- Mesos Reviewbot


On Oct. 24, 2017, 11:29 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63280/
> ---
> 
> (Updated Oct. 24, 2017, 11:29 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Greg Mann, and Jie Yu.
> 
> 
> Bugs: MESOS-8128
> https://issues.apache.org/jira/browse/MESOS-8128
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Since the containerizer launch depends on the inherited pipe to signal
> the forked child, be explicit about setting O_CLOEXEC on the pipe file
> descriptors. Make sure to close the pipe on the error paths.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 100e3bbda543d87808da9ff6bea42da5099ea8c5 
> 
> 
> Diff: https://reviews.apache.org/r/63280/diff/1/
> 
> 
> Testing
> ---
> 
> make check (Fedora 25)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 63280: Made the containerizer launch be explicit about O_CLOEXEC.

[Mesos Reviewbot Windows]

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



FAIL: The file 'C:DCOSmesosbuild-outputlogsapply-review-63270-stdout.log' 
already exists.

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/63280

- Mesos Reviewbot Windows


On Oct. 24, 2017, 11:29 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63280/
> ---
> 
> (Updated Oct. 24, 2017, 11:29 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Greg Mann, and Jie Yu.
> 
> 
> Bugs: MESOS-8128
> https://issues.apache.org/jira/browse/MESOS-8128
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Since the containerizer launch depends on the inherited pipe to signal
> the forked child, be explicit about setting O_CLOEXEC on the pipe file
> descriptors. Make sure to close the pipe on the error paths.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 100e3bbda543d87808da9ff6bea42da5099ea8c5 
> 
> 
> Diff: https://reviews.apache.org/r/63280/diff/1/
> 
> 
> Testing
> ---
> 
> make check (Fedora 25)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Review Request 63280: Made the containerizer launch be explicit about O_CLOEXEC.

[James Peach]

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

Review request for mesos, Benjamin Bannier, Greg Mann, and Jie Yu.


Bugs: MESOS-8128
https://issues.apache.org/jira/browse/MESOS-8128


Repository: mesos


Description
---

Since the containerizer launch depends on the inherited pipe to signal
the forked child, be explicit about setting O_CLOEXEC on the pipe file
descriptors. Make sure to close the pipe on the error paths.


Diffs
-

  src/slave/containerizer/mesos/containerizer.cpp 
100e3bbda543d87808da9ff6bea42da5099ea8c5 


Diff: https://reviews.apache.org/r/63280/diff/1/


Testing
---

make check (Fedora 25)


Thanks,

James Peach