You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Till Toenshoff <to...@me.com> on 2014/05/28 22:26:56 UTC

Re: Review Request 21966: Fixed ExternalContainerizer fd leak and premature close.

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

(Updated May 28, 2014, 8:26 p.m.)


Review request for mesos and Benjamin Hindman.


Changes
-------

Added fix for nasty io::splice issue in connection with a premature os::close. 


Summary (updated)
-----------------

Fixed ExternalContainerizer fd leak and premature close.


Repository: mesos-git


Description
-------

See summary.


Diffs (updated)
-----

  src/slave/containerizer/external_containerizer.cpp b39c845 

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


Testing (updated)
-------

make check as well as Deimos (ECP) intergration tests


Thanks,

Till Toenshoff

Re: Review Request 21966: Fixed ExternalContainerizer fd leak and premature close.

Posted by Till Toenshoff <to...@me.com>.

> On May 28, 2014, 10:58 p.m., Ben Mahler wrote:
> > Quite the tricky bug!
> > 
> > Is this issue also present in the MesosContainerizer? If so, we should probably fix that in this review as well or at least have a ticket to block 0.19.0.

Ticket is up and set for blocking:
https://issues.apache.org/jira/browse/MESOS-1431


> On May 28, 2014, 10:58 p.m., Ben Mahler wrote:
> > src/slave/containerizer/external_containerizer.cpp, lines 1133-1139
> > <https://reviews.apache.org/r/21966/diff/2/?file=597812#file597812line1133>
> >
> >     Wasn't there a separate review already for this cloexec issue?

I had bundled these as I had not gotten any reviews on the old version yet, sorry for triggering that confusion.


> On May 28, 2014, 10:58 p.m., Ben Mahler wrote:
> > src/slave/containerizer/external_containerizer.cpp, line 1148
> > <https://reviews.apache.org/r/21966/diff/2/?file=597812#file597812line1148>
> >
> >     errno at this point is potentially tainted by the call to os::close

Ow, good point. I will be switching over to io::redirect, hence get rid of this.


> On May 28, 2014, 10:58 p.m., Ben Mahler wrote:
> > src/slave/containerizer/external_containerizer.cpp, line 1152
> > <https://reviews.apache.org/r/21966/diff/2/?file=597812#file597812line1152>
> >
> >     Why not have two onAny's instead of the custom '_invoke' function?
> >     
> >     e.g.
> >     
> >       .onAny(bind(&os::close, fd))
> >       .onAny(bind(&os::close, err.get())

Again, good point. I had that extra callback in there for better debugging. 


- Till


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


On May 28, 2014, 8:26 p.m., Till Toenshoff wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21966/
> -----------------------------------------------------------
> 
> (Updated May 28, 2014, 8:26 p.m.)
> 
> 
> Review request for mesos and Benjamin Hindman.
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> See summary.
> 
> 
> Diffs
> -----
> 
>   src/slave/containerizer/external_containerizer.cpp b39c845 
> 
> Diff: https://reviews.apache.org/r/21966/diff/
> 
> 
> Testing
> -------
> 
> make check as well as Deimos (ECP) intergration tests
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 21966: Fixed ExternalContainerizer fd leak and premature close.

Posted by Till Toenshoff <to...@me.com>.

> On May 28, 2014, 10:58 p.m., Ben Mahler wrote:
> > src/slave/containerizer/external_containerizer.cpp, lines 1141-1144
> > <https://reviews.apache.org/r/21966/diff/2/?file=597812#file597812line1141>
> >
> >     Wasn't there an issue more accurately related to libev polling the prematurely closed file descriptor? It might be good to spell that out here for posterity!

Due to the switch towards io::redirect, this comment is not needed anymore. For making sure other devs are aware of this danger, MESOS-1431 was created.


- Till


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


On May 29, 2014, 2:31 a.m., Till Toenshoff wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21966/
> -----------------------------------------------------------
> 
> (Updated May 29, 2014, 2:31 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Ben Mahler.
> 
> 
> Bugs: MESOS-1431
>     https://issues.apache.org/jira/browse/MESOS-1431
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> See summary.
> 
> 
> Diffs
> -----
> 
>   src/slave/containerizer/external_containerizer.cpp b39c845 
> 
> Diff: https://reviews.apache.org/r/21966/diff/
> 
> 
> Testing
> -------
> 
> make check as well as Deimos (ECP) intergration tests
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 21966: Fixed ExternalContainerizer fd leak and premature close.

Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21966/#review44193
-----------------------------------------------------------


Quite the tricky bug!

Is this issue also present in the MesosContainerizer? If so, we should probably fix that in this review as well or at least have a ticket to block 0.19.0.


src/slave/containerizer/external_containerizer.cpp
<https://reviews.apache.org/r/21966/#comment78518>

    Wasn't there a separate review already for this cloexec issue?



src/slave/containerizer/external_containerizer.cpp
<https://reviews.apache.org/r/21966/#comment78515>

    Wasn't there an issue more accurately related to libev polling the prematurely closed file descriptor? It might be good to spell that out here for posterity!



src/slave/containerizer/external_containerizer.cpp
<https://reviews.apache.org/r/21966/#comment78516>

    errno at this point is potentially tainted by the call to os::close



src/slave/containerizer/external_containerizer.cpp
<https://reviews.apache.org/r/21966/#comment78517>

    Why not have two onAny's instead of the custom '_invoke' function?
    
    e.g.
    
      .onAny(bind(&os::close, fd))
      .onAny(bind(&os::close, err.get())


- Ben Mahler


On May 28, 2014, 8:26 p.m., Till Toenshoff wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21966/
> -----------------------------------------------------------
> 
> (Updated May 28, 2014, 8:26 p.m.)
> 
> 
> Review request for mesos and Benjamin Hindman.
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> See summary.
> 
> 
> Diffs
> -----
> 
>   src/slave/containerizer/external_containerizer.cpp b39c845 
> 
> Diff: https://reviews.apache.org/r/21966/diff/
> 
> 
> Testing
> -------
> 
> make check as well as Deimos (ECP) intergration tests
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 21966: Fixed ExternalContainerizer fd leak and premature close.

Posted by Till Toenshoff <to...@me.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21966/
-----------------------------------------------------------

(Updated May 29, 2014, 2:55 a.m.)


Review request for mesos, Benjamin Hindman and Ben Mahler.


Changes
-------

Addressed comments.


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


Repository: mesos-git


Description
-------

See summary.


Diffs (updated)
-----

  src/slave/containerizer/external_containerizer.cpp b39c845 

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


Testing
-------

make check as well as Deimos (ECP) intergration tests


Thanks,

Till Toenshoff

Re: Review Request 21966: Fixed ExternalContainerizer fd leak and premature close.

Posted by Till Toenshoff <to...@me.com>.

> On May 29, 2014, 2:37 a.m., Benjamin Hindman wrote:
> > src/slave/containerizer/external_containerizer.cpp, line 1125
> > <https://reviews.apache.org/r/21966/diff/3/?file=598213#file598213line1125>
> >
> >     You could not bother setting the close-on-exec flag and just close it immediately after doing the io::redirect rather than via an onAny. It's slightly more exposed to a fork but not too much (and it's just less code in general). You could move the TODO up to the open then.

Aye, makes sense - will fix that here and in the MC RR as well.


- Till


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


On May 29, 2014, 2:31 a.m., Till Toenshoff wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21966/
> -----------------------------------------------------------
> 
> (Updated May 29, 2014, 2:31 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Ben Mahler.
> 
> 
> Bugs: MESOS-1431
>     https://issues.apache.org/jira/browse/MESOS-1431
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> See summary.
> 
> 
> Diffs
> -----
> 
>   src/slave/containerizer/external_containerizer.cpp b39c845 
> 
> Diff: https://reviews.apache.org/r/21966/diff/
> 
> 
> Testing
> -------
> 
> make check as well as Deimos (ECP) intergration tests
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 21966: Fixed ExternalContainerizer fd leak and premature close.

Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21966/#review44227
-----------------------------------------------------------

Ship it!



src/slave/containerizer/external_containerizer.cpp
<https://reviews.apache.org/r/21966/#comment78563>

    You could not bother setting the close-on-exec flag and just close it immediately after doing the io::redirect rather than via an onAny. It's slightly more exposed to a fork but not too much (and it's just less code in general). You could move the TODO up to the open then.


- Benjamin Hindman


On May 29, 2014, 2:31 a.m., Till Toenshoff wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21966/
> -----------------------------------------------------------
> 
> (Updated May 29, 2014, 2:31 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Ben Mahler.
> 
> 
> Bugs: MESOS-1431
>     https://issues.apache.org/jira/browse/MESOS-1431
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> See summary.
> 
> 
> Diffs
> -----
> 
>   src/slave/containerizer/external_containerizer.cpp b39c845 
> 
> Diff: https://reviews.apache.org/r/21966/diff/
> 
> 
> Testing
> -------
> 
> make check as well as Deimos (ECP) intergration tests
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 21966: Fixed ExternalContainerizer fd leak and premature close.

Posted by Till Toenshoff <to...@me.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21966/
-----------------------------------------------------------

(Updated May 29, 2014, 2:31 a.m.)


Review request for mesos, Benjamin Hindman and Ben Mahler.


Changes
-------

Added tracking ticket.


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


Repository: mesos-git


Description
-------

See summary.


Diffs
-----

  src/slave/containerizer/external_containerizer.cpp b39c845 

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


Testing
-------

make check as well as Deimos (ECP) intergration tests


Thanks,

Till Toenshoff

Re: Review Request 21966: Fixed ExternalContainerizer fd leak and premature close.

Posted by Till Toenshoff <to...@me.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21966/
-----------------------------------------------------------

(Updated May 29, 2014, 2:30 a.m.)


Review request for mesos, Benjamin Hindman and Ben Mahler.


Changes
-------

Added BenM


Repository: mesos-git


Description
-------

See summary.


Diffs
-----

  src/slave/containerizer/external_containerizer.cpp b39c845 

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


Testing
-------

make check as well as Deimos (ECP) intergration tests


Thanks,

Till Toenshoff

Re: Review Request 21966: Fixed ExternalContainerizer fd leak and premature close.

Posted by Till Toenshoff <to...@me.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21966/
-----------------------------------------------------------

(Updated May 29, 2014, 2:29 a.m.)


Review request for mesos and Benjamin Hindman.


Changes
-------

Switched over to io::redirect.


Repository: mesos-git


Description
-------

See summary.


Diffs (updated)
-----

  src/slave/containerizer/external_containerizer.cpp b39c845 

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


Testing
-------

make check as well as Deimos (ECP) intergration tests


Thanks,

Till Toenshoff