You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Ian Downes <ia...@gmail.com> on 2014/01/21 20:34:50 UTC
Re: Review Request 16150: Containerizer - isolators (part 3)
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16150/
-----------------------------------------------------------
(Updated Jan. 21, 2014, 7:34 p.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, Chi Zhang, Niklas Nielsen, samya, and Jason Dusek.
Summary (updated)
-----------------
Containerizer - isolators (part 3)
Repository: mesos-git
Description
-------
Isolators perform isolator for the MesosContainerizer
Isolator interface and implementations of Posix CPU and Mem isolators (no isolation, just usage())
Diffs
-----
src/Makefile.am cf0c8c66e8dd21c2f5a2da22e5d5adb056353454
src/slave/container/isolator.hpp PRE-CREATION
src/slave/container/isolator.cpp PRE-CREATION
src/slave/container/isolators/posix.hpp PRE-CREATION
Diff: https://reviews.apache.org/r/16150/diff/
Testing
-------
Thanks,
Ian Downes
Re: Review Request 16150: Containerizer - isolators (part 3)
Posted by Ian Downes <ia...@gmail.com>.
> On Jan. 26, 2014, 8:08 a.m., Adam B wrote:
> > src/slave/container/isolator.cpp, line 99
> > <https://reviews.apache.org/r/16150/diff/4/?file=425490#file425490line99>
> >
> > Needs newline at end of file
Not needed for cpp file, and not done by other files in codebase.
- Ian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16150/#review32792
-----------------------------------------------------------
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/16150/
> -----------------------------------------------------------
>
> (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
> -------
>
> Isolators perform isolator for the MesosContainerizer
>
> Isolator interface and implementations of Posix CPU and Mem isolators (no isolation, just usage())
>
>
> Diffs
> -----
>
> src/Makefile.am cf0c8c66e8dd21c2f5a2da22e5d5adb056353454
> src/slave/containerizer/isolator.hpp PRE-CREATION
> src/slave/containerizer/isolator.cpp PRE-CREATION
> src/slave/containerizer/isolators/posix.hpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/16150/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 16150: Containerizer - isolators (part 3)
Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16150/#review32792
-----------------------------------------------------------
Ship it!
Looks great!
src/slave/container/isolator.hpp
<https://reviews.apache.org/r/16150/#comment61765>
"states" plural
src/slave/container/isolator.hpp
<https://reviews.apache.org/r/16150/#comment61766>
Should the CommandInfo comment go with the isolate() method that returns the CommandInfo?
src/slave/container/isolator.cpp
<https://reviews.apache.org/r/16150/#comment61767>
Needs newline at end of file
src/slave/container/isolators/posix.hpp
<https://reviews.apache.org/r/16150/#comment61769>
Neither parameter is used here. Could you comment why update does nothing?
- Adam B
On Jan. 21, 2014, 11:34 a.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16150/
> -----------------------------------------------------------
>
> (Updated Jan. 21, 2014, 11:34 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, Chi Zhang, Niklas Nielsen, samya, and Jason Dusek.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Isolators perform isolator for the MesosContainerizer
>
> Isolator interface and implementations of Posix CPU and Mem isolators (no isolation, just usage())
>
>
> Diffs
> -----
>
> src/Makefile.am cf0c8c66e8dd21c2f5a2da22e5d5adb056353454
> src/slave/container/isolator.hpp PRE-CREATION
> src/slave/container/isolator.cpp PRE-CREATION
> src/slave/container/isolators/posix.hpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/16150/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 16150: Containerizer - isolators (part 3)
Posted by Ian Downes <ia...@gmail.com>.
> On Jan. 28, 2014, 7:42 p.m., Vinod Kone wrote:
> > src/slave/containerizer/isolators/posix.hpp, line 36
> > <https://reviews.apache.org/r/16150/diff/5/?file=451176#file451176line36>
> >
> > I'm assuming you don't people to directly use PosixIsolatorProcess but rather its subclasses?
> >
> > If yes, should we make its constructor(s) private?
It hasn't implemented Isolator::usage() so it's still abstract. Is that sufficient or should I make the constructor private too?
> On Jan. 28, 2014, 7:42 p.m., Vinod Kone wrote:
> > src/slave/containerizer/isolators/posix.hpp, lines 52-54
> > <https://reviews.apache.org/r/16150/diff/5/?file=451176#file451176line52>
> >
> > As mentioned in earlier reviews, this shouldn't be an error?
MesosContainerizer only tries to recover containers from the isolators and launchers if they have pids.
> On Jan. 28, 2014, 7:42 p.m., Vinod Kone wrote:
> > src/slave/containerizer/isolators/posix.hpp, line 140
> > <https://reviews.apache.org/r/16150/diff/5/?file=451176#file451176line140>
> >
> > What if the pid exits by this point? Should it be a Failure or return empty resource stats?
Yes, should just return empty stats.
- Ian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16150/#review33022
-----------------------------------------------------------
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/16150/
> -----------------------------------------------------------
>
> (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
> -------
>
> Isolators perform isolator for the MesosContainerizer
>
> Isolator interface and implementations of Posix CPU and Mem isolators (no isolation, just usage())
>
>
> Diffs
> -----
>
> src/Makefile.am cf0c8c66e8dd21c2f5a2da22e5d5adb056353454
> src/slave/containerizer/isolator.hpp PRE-CREATION
> src/slave/containerizer/isolator.cpp PRE-CREATION
> src/slave/containerizer/isolators/posix.hpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/16150/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 16150: Containerizer - isolators (part 3)
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16150/#review33022
-----------------------------------------------------------
src/slave/containerizer/isolator.hpp
<https://reviews.apache.org/r/16150/#comment62168>
const?
src/slave/containerizer/isolator.hpp
<https://reviews.apache.org/r/16150/#comment62169>
const ?
src/slave/containerizer/isolators/posix.hpp
<https://reviews.apache.org/r/16150/#comment62174>
I'm assuming you don't people to directly use PosixIsolatorProcess but rather its subclasses?
If yes, should we make its constructor(s) private?
src/slave/containerizer/isolators/posix.hpp
<https://reviews.apache.org/r/16150/#comment62171>
As mentioned in earlier reviews, this shouldn't be an error?
src/slave/containerizer/isolators/posix.hpp
<https://reviews.apache.org/r/16150/#comment62172>
How is this possible? Comment?
src/slave/containerizer/isolators/posix.hpp
<https://reviews.apache.org/r/16150/#comment62182>
What if the pid exits by this point? Should it be a Failure or return empty resource stats?
src/slave/containerizer/isolators/posix.hpp
<https://reviews.apache.org/r/16150/#comment62183>
ditto.
- Vinod Kone
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/16150/
> -----------------------------------------------------------
>
> (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
> -------
>
> Isolators perform isolator for the MesosContainerizer
>
> Isolator interface and implementations of Posix CPU and Mem isolators (no isolation, just usage())
>
>
> Diffs
> -----
>
> src/Makefile.am cf0c8c66e8dd21c2f5a2da22e5d5adb056353454
> src/slave/containerizer/isolator.hpp PRE-CREATION
> src/slave/containerizer/isolator.cpp PRE-CREATION
> src/slave/containerizer/isolators/posix.hpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/16150/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 16150: Containerizer - isolators (part 3)
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16150/#review33864
-----------------------------------------------------------
Ship it!
Ship It!
- Vinod Kone
On Feb. 1, 2014, 4:03 a.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16150/
> -----------------------------------------------------------
>
> (Updated Feb. 1, 2014, 4:03 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, Chi Zhang, Niklas Nielsen, samya, and Jason Dusek.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Isolators perform isolator for the MesosContainerizer
>
> Isolator interface and implementations of Posix CPU and Mem isolators (no isolation, just usage())
>
>
> Diffs
> -----
>
> src/Makefile.am c30706846bca1fa3287291e39f46a23713ad1ba4
> src/slave/containerizer/isolator.hpp PRE-CREATION
> src/slave/containerizer/isolator.cpp PRE-CREATION
> src/slave/containerizer/isolators/posix.hpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/16150/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 16150: Containerizer - isolators (part 3)
Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16150/
-----------------------------------------------------------
(Updated Feb. 12, 2014, 12:20 a.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, Chi Zhang, Niklas Nielsen, samya, and Jason Dusek.
Changes
-------
Adressed Ben's comments.
Repository: mesos-git
Description
-------
Isolators perform isolator for the MesosContainerizer
Isolator interface and implementations of Posix CPU and Mem isolators (no isolation, just usage())
Diffs (updated)
-----
src/Makefile.am c30706846bca1fa3287291e39f46a23713ad1ba4
src/slave/containerizer/isolator.hpp PRE-CREATION
src/slave/containerizer/isolator.cpp PRE-CREATION
src/slave/containerizer/isolators/posix.hpp PRE-CREATION
Diff: https://reviews.apache.org/r/16150/diff/
Testing
-------
Thanks,
Ian Downes
Re: Review Request 16150: Containerizer - isolators (part 3)
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16150/#review33919
-----------------------------------------------------------
Ship it!
src/slave/containerizer/isolator.hpp
<https://reviews.apache.org/r/16150/#comment63700>
Please wrap everything at 80 columns!
src/slave/containerizer/isolator.hpp
<https://reviews.apache.org/r/16150/#comment63697>
Can kill this newline.
src/slave/containerizer/isolator.cpp
<https://reviews.apache.org/r/16150/#comment63698>
FYI, you can also do 'process::spawn(CHECK_NOTNULL(process.get()))'.
src/slave/containerizer/isolator.cpp
<https://reviews.apache.org/r/16150/#comment63699>
Please wrap everything in here at 80 columns.
src/slave/containerizer/isolators/posix.hpp
<https://reviews.apache.org/r/16150/#comment63703>
Wrap at 80 columns pretty pretty please!
src/slave/containerizer/isolators/posix.hpp
<https://reviews.apache.org/r/16150/#comment63701>
The wrapping is usually:
process::Owned<process::Promise<Limitation> > promise(
new process::Promise<Limitation>());
src/slave/containerizer/isolators/posix.hpp
<https://reviews.apache.org/r/16150/#comment63702>
The wrapping is usually:
process::Owned<process::Promise<Limitation> > promise(
new process::Promise<Limitation>());
src/slave/containerizer/isolators/posix.hpp
<https://reviews.apache.org/r/16150/#comment63704>
You can kill the newline.
- Benjamin Hindman
On Feb. 7, 2014, 3:36 a.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16150/
> -----------------------------------------------------------
>
> (Updated Feb. 7, 2014, 3:36 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, Chi Zhang, Niklas Nielsen, samya, and Jason Dusek.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Isolators perform isolator for the MesosContainerizer
>
> Isolator interface and implementations of Posix CPU and Mem isolators (no isolation, just usage())
>
>
> Diffs
> -----
>
> src/Makefile.am c30706846bca1fa3287291e39f46a23713ad1ba4
> src/slave/containerizer/isolator.hpp PRE-CREATION
> src/slave/containerizer/isolator.cpp PRE-CREATION
> src/slave/containerizer/isolators/posix.hpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/16150/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 16150: Containerizer - isolators (part 3)
Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16150/
-----------------------------------------------------------
(Updated Feb. 7, 2014, 3:36 a.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, Chi Zhang, Niklas Nielsen, samya, and Jason Dusek.
Changes
-------
Addressed Vinod's comments.
Repository: mesos-git
Description
-------
Isolators perform isolator for the MesosContainerizer
Isolator interface and implementations of Posix CPU and Mem isolators (no isolation, just usage())
Diffs (updated)
-----
src/Makefile.am c30706846bca1fa3287291e39f46a23713ad1ba4
src/slave/containerizer/isolator.hpp PRE-CREATION
src/slave/containerizer/isolator.cpp PRE-CREATION
src/slave/containerizer/isolators/posix.hpp PRE-CREATION
Diff: https://reviews.apache.org/r/16150/diff/
Testing
-------
Thanks,
Ian Downes
Re: Review Request 16150: Containerizer - isolators (part 3)
Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16150/
-----------------------------------------------------------
(Updated Feb. 1, 2014, 4:03 a.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, Chi Zhang, Niklas Nielsen, samya, and Jason Dusek.
Changes
-------
Rebased and addressed comments.
Repository: mesos-git
Description
-------
Isolators perform isolator for the MesosContainerizer
Isolator interface and implementations of Posix CPU and Mem isolators (no isolation, just usage())
Diffs (updated)
-----
src/Makefile.am c30706846bca1fa3287291e39f46a23713ad1ba4
src/slave/containerizer/isolator.hpp PRE-CREATION
src/slave/containerizer/isolator.cpp PRE-CREATION
src/slave/containerizer/isolators/posix.hpp PRE-CREATION
Diff: https://reviews.apache.org/r/16150/diff/
Testing
-------
Thanks,
Ian Downes
Re: Review Request 16150: Containerizer - isolators (part 3)
Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16150/
-----------------------------------------------------------
(Updated Jan. 27, 2014, 3:02 a.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, Chi Zhang, Niklas Nielsen, samya, and Jason Dusek.
Changes
-------
Addressed review comments.
Repository: mesos-git
Description
-------
Isolators perform isolator for the MesosContainerizer
Isolator interface and implementations of Posix CPU and Mem isolators (no isolation, just usage())
Diffs (updated)
-----
src/Makefile.am cf0c8c66e8dd21c2f5a2da22e5d5adb056353454
src/slave/containerizer/isolator.hpp PRE-CREATION
src/slave/containerizer/isolator.cpp PRE-CREATION
src/slave/containerizer/isolators/posix.hpp PRE-CREATION
Diff: https://reviews.apache.org/r/16150/diff/
Testing
-------
Thanks,
Ian Downes
Re: Review Request 16150: Containerizer - isolators (part 3)
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16150/#review32799
-----------------------------------------------------------
src/slave/container/isolator.hpp
<https://reviews.apache.org/r/16150/#comment61777>
clang emitted a 'hidden virtual overload' error - that's way I mentioned it :)
- Niklas Nielsen
On Jan. 21, 2014, 11:34 a.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16150/
> -----------------------------------------------------------
>
> (Updated Jan. 21, 2014, 11:34 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, Chi Zhang, Niklas Nielsen, samya, and Jason Dusek.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Isolators perform isolator for the MesosContainerizer
>
> Isolator interface and implementations of Posix CPU and Mem isolators (no isolation, just usage())
>
>
> Diffs
> -----
>
> src/Makefile.am cf0c8c66e8dd21c2f5a2da22e5d5adb056353454
> src/slave/container/isolator.hpp PRE-CREATION
> src/slave/container/isolator.cpp PRE-CREATION
> src/slave/container/isolators/posix.hpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/16150/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 16150: Containerizer - isolators (part 3)
Posted by Ian Downes <ia...@gmail.com>.
> On Jan. 23, 2014, 6:39 p.m., Niklas Nielsen wrote:
> > src/slave/container/isolator.hpp, line 108
> > <https://reviews.apache.org/r/16150/diff/4/?file=425489#file425489line108>
> >
> > This initialize overshadows initialize in Process. Can use resolve this with 'using process::ProcessBase::initialize;'?
This won't overshadow process::ProcessBase::initialize()?
- Ian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16150/#review32553
-----------------------------------------------------------
On Jan. 21, 2014, 7:34 p.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16150/
> -----------------------------------------------------------
>
> (Updated Jan. 21, 2014, 7:34 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, Chi Zhang, Niklas Nielsen, samya, and Jason Dusek.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Isolators perform isolator for the MesosContainerizer
>
> Isolator interface and implementations of Posix CPU and Mem isolators (no isolation, just usage())
>
>
> Diffs
> -----
>
> src/Makefile.am cf0c8c66e8dd21c2f5a2da22e5d5adb056353454
> src/slave/container/isolator.hpp PRE-CREATION
> src/slave/container/isolator.cpp PRE-CREATION
> src/slave/container/isolators/posix.hpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/16150/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 16150: Containerizer - isolators (part 3)
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16150/#review32553
-----------------------------------------------------------
Ship it!
src/Makefile.am
<https://reviews.apache.org/r/16150/#comment61433>
Align '\''s
src/slave/container/isolator.hpp
<https://reviews.apache.org/r/16150/#comment61434>
Can you comment this call too?
src/slave/container/isolator.hpp
<https://reviews.apache.org/r/16150/#comment61435>
This one.
src/slave/container/isolator.hpp
<https://reviews.apache.org/r/16150/#comment61436>
This one.
src/slave/container/isolator.hpp
<https://reviews.apache.org/r/16150/#comment61437>
And this one.
src/slave/container/isolator.hpp
<https://reviews.apache.org/r/16150/#comment61438>
This initialize overshadows initialize in Process. Can use resolve this with 'using process::ProcessBase::initialize;'?
- Niklas Nielsen
On Jan. 21, 2014, 11:34 a.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16150/
> -----------------------------------------------------------
>
> (Updated Jan. 21, 2014, 11:34 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, Chi Zhang, Niklas Nielsen, samya, and Jason Dusek.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Isolators perform isolator for the MesosContainerizer
>
> Isolator interface and implementations of Posix CPU and Mem isolators (no isolation, just usage())
>
>
> Diffs
> -----
>
> src/Makefile.am cf0c8c66e8dd21c2f5a2da22e5d5adb056353454
> src/slave/container/isolator.hpp PRE-CREATION
> src/slave/container/isolator.cpp PRE-CREATION
> src/slave/container/isolators/posix.hpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/16150/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>