You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Ian Downes <ia...@gmail.com> on 2015/05/16 03:14:11 UTC
Review Request 34310: Use IDLE scheduling for revocable CPU in cgroups
isolator.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34310/
-----------------------------------------------------------
Review request for mesos, Joris Van Remoortere, Niklas Nielsen, and Vinod Kone.
Bugs: MESOS-2652
https://issues.apache.org/jira/browse/MESOS-2652
Repository: mesos
Description
-------
Use IDLE scheduling for revocable CPU in cgroups isolator.
Diffs
-----
src/slave/containerizer/isolators/cgroups/cpushare.hpp ff4a9dbdb1b655e71bf87dcee8fe62433d396f52
src/slave/containerizer/isolators/cgroups/cpushare.cpp 6a5b2b5c6e2844fe1a10815956569194b6f56681
Diff: https://reviews.apache.org/r/34310/diff/
Testing
-------
Thanks,
Ian Downes
Re: Review Request 34310: Use IDLE scheduling for revocable CPU in
cgroups isolator.
Posted by Timothy Chen <tn...@apache.org>.
> On May 16, 2015, 4:46 a.m., Timothy Chen wrote:
> > src/slave/containerizer/isolators/cgroups/cpushare.cpp, line 345
> > <https://reviews.apache.org/r/34310/diff/1/?file=961963#file961963line345>
> >
> > What if the same set of resources contains both revocable and non-revocable resources?
>
> Ian Downes wrote:
> Hmm, I presumed that any amount of revocable CPU means the container is treated as revocable. Thoughts?
I believe what you said it's true, I think it just wasn't obvious to me when I read the code. I wonder if we have anywhere that states this or we should state it here?
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34310/#review84024
-----------------------------------------------------------
On May 19, 2015, 7:58 p.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34310/
> -----------------------------------------------------------
>
> (Updated May 19, 2015, 7:58 p.m.)
>
>
> Review request for mesos, Joris Van Remoortere, Niklas Nielsen, and Vinod Kone.
>
>
> Bugs: MESOS-2652
> https://issues.apache.org/jira/browse/MESOS-2652
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Use IDLE scheduling for revocable CPU in cgroups isolator.
>
>
> Diffs
> -----
>
> src/slave/containerizer/isolators/cgroups/cpushare.hpp ff4a9dbdb1b655e71bf87dcee8fe62433d396f52
> src/slave/containerizer/isolators/cgroups/cpushare.cpp 6a5b2b5c6e2844fe1a10815956569194b6f56681
>
> Diff: https://reviews.apache.org/r/34310/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 34310: Use IDLE scheduling for revocable CPU in
cgroups isolator.
Posted by Ian Downes <ia...@gmail.com>.
> On May 15, 2015, 9:46 p.m., Timothy Chen wrote:
> > src/slave/containerizer/isolators/cgroups/cpushare.cpp, line 345
> > <https://reviews.apache.org/r/34310/diff/1/?file=961963#file961963line345>
> >
> > What if the same set of resources contains both revocable and non-revocable resources?
Hmm, I presumed that any amount of revocable CPU means the container is treated as revocable. Thoughts?
- Ian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34310/#review84024
-----------------------------------------------------------
On May 18, 2015, 10:33 a.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34310/
> -----------------------------------------------------------
>
> (Updated May 18, 2015, 10:33 a.m.)
>
>
> Review request for mesos, Joris Van Remoortere, Niklas Nielsen, and Vinod Kone.
>
>
> Bugs: MESOS-2652
> https://issues.apache.org/jira/browse/MESOS-2652
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Use IDLE scheduling for revocable CPU in cgroups isolator.
>
>
> Diffs
> -----
>
> src/slave/containerizer/isolators/cgroups/cpushare.hpp ff4a9dbdb1b655e71bf87dcee8fe62433d396f52
> src/slave/containerizer/isolators/cgroups/cpushare.cpp 6a5b2b5c6e2844fe1a10815956569194b6f56681
>
> Diff: https://reviews.apache.org/r/34310/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 34310: Use IDLE scheduling for revocable CPU in
cgroups isolator.
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34310/#review84024
-----------------------------------------------------------
src/slave/containerizer/isolators/cgroups/cpushare.cpp
<https://reviews.apache.org/r/34310/#comment135147>
What if the same set of resources contains both revocable and non-revocable resources?
- Timothy Chen
On May 16, 2015, 1:14 a.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34310/
> -----------------------------------------------------------
>
> (Updated May 16, 2015, 1:14 a.m.)
>
>
> Review request for mesos, Joris Van Remoortere, Niklas Nielsen, and Vinod Kone.
>
>
> Bugs: MESOS-2652
> https://issues.apache.org/jira/browse/MESOS-2652
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Use IDLE scheduling for revocable CPU in cgroups isolator.
>
>
> Diffs
> -----
>
> src/slave/containerizer/isolators/cgroups/cpushare.hpp ff4a9dbdb1b655e71bf87dcee8fe62433d396f52
> src/slave/containerizer/isolators/cgroups/cpushare.cpp 6a5b2b5c6e2844fe1a10815956569194b6f56681
>
> Diff: https://reviews.apache.org/r/34310/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 34310: Use IDLE scheduling for revocable CPU in
cgroups isolator.
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34310/#review84745
-----------------------------------------------------------
Ship it!
Modulo making the resources field optional
src/slave/containerizer/isolators/cgroups/cpushare.cpp
<https://reviews.apache.org/r/34310/#comment136090>
Per my review for 34309 - how about printing the pid too?
- Niklas Nielsen
On May 19, 2015, 3:43 p.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34310/
> -----------------------------------------------------------
>
> (Updated May 19, 2015, 3:43 p.m.)
>
>
> Review request for mesos, Joris Van Remoortere, Niklas Nielsen, and Vinod Kone.
>
>
> Bugs: MESOS-2652
> https://issues.apache.org/jira/browse/MESOS-2652
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Use IDLE scheduling for revocable CPU in cgroups isolator.
>
>
> Diffs
> -----
>
> src/slave/containerizer/isolators/cgroups/cpushare.hpp ff4a9dbdb1b655e71bf87dcee8fe62433d396f52
> src/slave/containerizer/isolators/cgroups/cpushare.cpp 6a5b2b5c6e2844fe1a10815956569194b6f56681
>
> Diff: https://reviews.apache.org/r/34310/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 34310: Use IDLE scheduling for revocable CPU in
cgroups isolator.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34310/#review84417
-----------------------------------------------------------
Patch looks great!
Reviews applied: [34308, 34309, 34310]
All tests passed.
- Mesos ReviewBot
On May 19, 2015, 10:43 p.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34310/
> -----------------------------------------------------------
>
> (Updated May 19, 2015, 10:43 p.m.)
>
>
> Review request for mesos, Joris Van Remoortere, Niklas Nielsen, and Vinod Kone.
>
>
> Bugs: MESOS-2652
> https://issues.apache.org/jira/browse/MESOS-2652
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Use IDLE scheduling for revocable CPU in cgroups isolator.
>
>
> Diffs
> -----
>
> src/slave/containerizer/isolators/cgroups/cpushare.hpp ff4a9dbdb1b655e71bf87dcee8fe62433d396f52
> src/slave/containerizer/isolators/cgroups/cpushare.cpp 6a5b2b5c6e2844fe1a10815956569194b6f56681
>
> Diff: https://reviews.apache.org/r/34310/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 34310: Use IDLE scheduling for revocable CPU in
cgroups isolator.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34310/#review85810
-----------------------------------------------------------
Patch looks great!
Reviews applied: [34308, 34309, 34310]
All tests passed.
- Mesos ReviewBot
On May 29, 2015, 7:57 p.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34310/
> -----------------------------------------------------------
>
> (Updated May 29, 2015, 7:57 p.m.)
>
>
> Review request for mesos, Joris Van Remoortere, Niklas Nielsen, and Vinod Kone.
>
>
> Bugs: MESOS-2652
> https://issues.apache.org/jira/browse/MESOS-2652
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Use IDLE scheduling for revocable CPU in cgroups isolator.
>
>
> Diffs
> -----
>
> src/slave/containerizer/isolators/cgroups/cpushare.hpp ff4a9dbdb1b655e71bf87dcee8fe62433d396f52
> src/slave/containerizer/isolators/cgroups/cpushare.cpp e3b5e2aa8ba06eec82b929144627d044f6b19d9d
> src/slave/flags.hpp 944ed794b6dc8a2d3712145a15b6b27cebf26a22
> src/slave/flags.cpp 6b7c61ea1a9f3b5d401d2aec935188a1b70c1738
> src/tests/isolator_tests.cpp 24c71b7906a92bdc84a38e88d6084ab09e3cf2ab
>
> Diff: https://reviews.apache.org/r/34310/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 34310: Use IDLE scheduling for revocable CPU in
cgroups isolator.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34310/#review85799
-----------------------------------------------------------
Ship it!
- Vinod Kone
On May 29, 2015, 7:57 p.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34310/
> -----------------------------------------------------------
>
> (Updated May 29, 2015, 7:57 p.m.)
>
>
> Review request for mesos, Joris Van Remoortere, Niklas Nielsen, and Vinod Kone.
>
>
> Bugs: MESOS-2652
> https://issues.apache.org/jira/browse/MESOS-2652
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Use IDLE scheduling for revocable CPU in cgroups isolator.
>
>
> Diffs
> -----
>
> src/slave/containerizer/isolators/cgroups/cpushare.hpp ff4a9dbdb1b655e71bf87dcee8fe62433d396f52
> src/slave/containerizer/isolators/cgroups/cpushare.cpp e3b5e2aa8ba06eec82b929144627d044f6b19d9d
> src/slave/flags.hpp 944ed794b6dc8a2d3712145a15b6b27cebf26a22
> src/slave/flags.cpp 6b7c61ea1a9f3b5d401d2aec935188a1b70c1738
> src/tests/isolator_tests.cpp 24c71b7906a92bdc84a38e88d6084ab09e3cf2ab
>
> Diff: https://reviews.apache.org/r/34310/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 34310: Use IDLE scheduling for revocable CPU in
cgroups isolator.
Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34310/
-----------------------------------------------------------
(Updated May 29, 2015, 12:57 p.m.)
Review request for mesos, Joris Van Remoortere, Niklas Nielsen, and Vinod Kone.
Changes
-------
Added flag to control setting of low priority for revocable cpu; default is enabled. Added test.
Bugs: MESOS-2652
https://issues.apache.org/jira/browse/MESOS-2652
Repository: mesos
Description
-------
Use IDLE scheduling for revocable CPU in cgroups isolator.
Diffs (updated)
-----
src/slave/containerizer/isolators/cgroups/cpushare.hpp ff4a9dbdb1b655e71bf87dcee8fe62433d396f52
src/slave/containerizer/isolators/cgroups/cpushare.cpp e3b5e2aa8ba06eec82b929144627d044f6b19d9d
src/slave/flags.hpp 944ed794b6dc8a2d3712145a15b6b27cebf26a22
src/slave/flags.cpp 6b7c61ea1a9f3b5d401d2aec935188a1b70c1738
src/tests/isolator_tests.cpp 24c71b7906a92bdc84a38e88d6084ab09e3cf2ab
Diff: https://reviews.apache.org/r/34310/diff/
Testing
-------
Thanks,
Ian Downes
Re: Review Request 34310: Use IDLE scheduling for revocable CPU in
cgroups isolator.
Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34310/
-----------------------------------------------------------
(Updated May 19, 2015, 3:43 p.m.)
Review request for mesos, Joris Van Remoortere, Niklas Nielsen, and Vinod Kone.
Bugs: MESOS-2652
https://issues.apache.org/jira/browse/MESOS-2652
Repository: mesos
Description
-------
Use IDLE scheduling for revocable CPU in cgroups isolator.
Diffs
-----
src/slave/containerizer/isolators/cgroups/cpushare.hpp ff4a9dbdb1b655e71bf87dcee8fe62433d396f52
src/slave/containerizer/isolators/cgroups/cpushare.cpp 6a5b2b5c6e2844fe1a10815956569194b6f56681
Diff: https://reviews.apache.org/r/34310/diff/
Testing
-------
Thanks,
Ian Downes
Re: Review Request 34310: Use IDLE scheduling for revocable CPU in
cgroups isolator.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34310/#review84402
-----------------------------------------------------------
Bad patch!
Reviews applied: [34308, 34309, 34308]
Failed command: ./support/apply-review.sh -n -r 34308
Error:
2015-05-19 22:07:20 URL:https://reviews.apache.org/r/34308/diff/raw/ [1126/1126] -> "34308.patch" [1]
error: patch failed: include/mesos/resources.hpp:189
error: include/mesos/resources.hpp: patch does not apply
Failed to apply patch
- Mesos ReviewBot
On May 19, 2015, 7:58 p.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34310/
> -----------------------------------------------------------
>
> (Updated May 19, 2015, 7:58 p.m.)
>
>
> Review request for mesos, Joris Van Remoortere, Niklas Nielsen, and Vinod Kone.
>
>
> Bugs: MESOS-2652
> https://issues.apache.org/jira/browse/MESOS-2652
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Use IDLE scheduling for revocable CPU in cgroups isolator.
>
>
> Diffs
> -----
>
> src/slave/containerizer/isolators/cgroups/cpushare.hpp ff4a9dbdb1b655e71bf87dcee8fe62433d396f52
> src/slave/containerizer/isolators/cgroups/cpushare.cpp 6a5b2b5c6e2844fe1a10815956569194b6f56681
>
> Diff: https://reviews.apache.org/r/34310/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 34310: Use IDLE scheduling for revocable CPU in
cgroups isolator.
Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34310/
-----------------------------------------------------------
(Updated May 19, 2015, 12:58 p.m.)
Review request for mesos, Joris Van Remoortere, Niklas Nielsen, and Vinod Kone.
Bugs: MESOS-2652
https://issues.apache.org/jira/browse/MESOS-2652
Repository: mesos
Description
-------
Use IDLE scheduling for revocable CPU in cgroups isolator.
Diffs (updated)
-----
src/slave/containerizer/isolators/cgroups/cpushare.hpp ff4a9dbdb1b655e71bf87dcee8fe62433d396f52
src/slave/containerizer/isolators/cgroups/cpushare.cpp 6a5b2b5c6e2844fe1a10815956569194b6f56681
Diff: https://reviews.apache.org/r/34310/diff/
Testing
-------
Thanks,
Ian Downes
Re: Review Request 34310: Use IDLE scheduling for revocable CPU in
cgroups isolator.
Posted by Ian Downes <ia...@gmail.com>.
> On May 19, 2015, 9:57 a.m., Niklas Nielsen wrote:
> > src/slave/containerizer/isolators/cgroups/cpushare.cpp, lines 341-343
> > <https://reviews.apache.org/r/34310/diff/2/?file=963184#file963184line341>
> >
> > We need to enforce that invariant in the slave then. Can we create a JIRA for this?
https://issues.apache.org/jira/browse/MESOS-2753
- Ian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34310/#review84338
-----------------------------------------------------------
On May 19, 2015, 3:43 p.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34310/
> -----------------------------------------------------------
>
> (Updated May 19, 2015, 3:43 p.m.)
>
>
> Review request for mesos, Joris Van Remoortere, Niklas Nielsen, and Vinod Kone.
>
>
> Bugs: MESOS-2652
> https://issues.apache.org/jira/browse/MESOS-2652
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Use IDLE scheduling for revocable CPU in cgroups isolator.
>
>
> Diffs
> -----
>
> src/slave/containerizer/isolators/cgroups/cpushare.hpp ff4a9dbdb1b655e71bf87dcee8fe62433d396f52
> src/slave/containerizer/isolators/cgroups/cpushare.cpp 6a5b2b5c6e2844fe1a10815956569194b6f56681
>
> Diff: https://reviews.apache.org/r/34310/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 34310: Use IDLE scheduling for revocable CPU in
cgroups isolator.
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34310/#review84338
-----------------------------------------------------------
LGTM - Can we wire up a test for this?
src/slave/containerizer/isolators/cgroups/cpushare.cpp
<https://reviews.apache.org/r/34310/#comment135556>
We need to enforce that invariant in the slave then. Can we create a JIRA for this?
- Niklas Nielsen
On May 18, 2015, 1:49 p.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34310/
> -----------------------------------------------------------
>
> (Updated May 18, 2015, 1:49 p.m.)
>
>
> Review request for mesos, Joris Van Remoortere, Niklas Nielsen, and Vinod Kone.
>
>
> Bugs: MESOS-2652
> https://issues.apache.org/jira/browse/MESOS-2652
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Use IDLE scheduling for revocable CPU in cgroups isolator.
>
>
> Diffs
> -----
>
> src/slave/containerizer/isolators/cgroups/cpushare.hpp ff4a9dbdb1b655e71bf87dcee8fe62433d396f52
> src/slave/containerizer/isolators/cgroups/cpushare.cpp 6a5b2b5c6e2844fe1a10815956569194b6f56681
>
> Diff: https://reviews.apache.org/r/34310/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 34310: Use IDLE scheduling for revocable CPU in
cgroups isolator.
Posted by Ian Downes <ia...@gmail.com>.
> On May 19, 2015, 12:42 p.m., Vinod Kone wrote:
> > My high leve comment:
> >
> > Lets add the following check/invariant in the master:
> >
> > "An executor and all its tasks should all use regular resources or should all use revocable resources".
> >
> > We can relax this policy in the future when we have better QoS mechanisms in place. Does that make sense?
https://issues.apache.org/jira/browse/MESOS-2753
- Ian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34310/#review84371
-----------------------------------------------------------
On May 19, 2015, 3:43 p.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34310/
> -----------------------------------------------------------
>
> (Updated May 19, 2015, 3:43 p.m.)
>
>
> Review request for mesos, Joris Van Remoortere, Niklas Nielsen, and Vinod Kone.
>
>
> Bugs: MESOS-2652
> https://issues.apache.org/jira/browse/MESOS-2652
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Use IDLE scheduling for revocable CPU in cgroups isolator.
>
>
> Diffs
> -----
>
> src/slave/containerizer/isolators/cgroups/cpushare.hpp ff4a9dbdb1b655e71bf87dcee8fe62433d396f52
> src/slave/containerizer/isolators/cgroups/cpushare.cpp 6a5b2b5c6e2844fe1a10815956569194b6f56681
>
> Diff: https://reviews.apache.org/r/34310/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 34310: Use IDLE scheduling for revocable CPU in
cgroups isolator.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34310/#review84371
-----------------------------------------------------------
My high leve comment:
Lets add the following check/invariant in the master:
"An executor and all its tasks should all use regular resources or should all use revocable resources".
We can relax this policy in the future when we have better QoS mechanisms in place. Does that make sense?
src/slave/containerizer/isolators/cgroups/cpushare.hpp
<https://reviews.apache.org/r/34310/#comment135584>
Optional? because this only gets set/updated in update().
- Vinod Kone
On May 18, 2015, 8:49 p.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34310/
> -----------------------------------------------------------
>
> (Updated May 18, 2015, 8:49 p.m.)
>
>
> Review request for mesos, Joris Van Remoortere, Niklas Nielsen, and Vinod Kone.
>
>
> Bugs: MESOS-2652
> https://issues.apache.org/jira/browse/MESOS-2652
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Use IDLE scheduling for revocable CPU in cgroups isolator.
>
>
> Diffs
> -----
>
> src/slave/containerizer/isolators/cgroups/cpushare.hpp ff4a9dbdb1b655e71bf87dcee8fe62433d396f52
> src/slave/containerizer/isolators/cgroups/cpushare.cpp 6a5b2b5c6e2844fe1a10815956569194b6f56681
>
> Diff: https://reviews.apache.org/r/34310/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 34310: Use IDLE scheduling for revocable CPU in
cgroups isolator.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34310/#review84218
-----------------------------------------------------------
Bad patch!
Reviews applied: [34308, 34308]
Failed command: ./support/apply-review.sh -n -r 34308
Error:
2015-05-18 21:03:45 URL:https://reviews.apache.org/r/34308/diff/raw/ [1126/1126] -> "34308.patch" [1]
error: patch failed: include/mesos/resources.hpp:189
error: include/mesos/resources.hpp: patch does not apply
Failed to apply patch
- Mesos ReviewBot
On May 18, 2015, 8:49 p.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34310/
> -----------------------------------------------------------
>
> (Updated May 18, 2015, 8:49 p.m.)
>
>
> Review request for mesos, Joris Van Remoortere, Niklas Nielsen, and Vinod Kone.
>
>
> Bugs: MESOS-2652
> https://issues.apache.org/jira/browse/MESOS-2652
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Use IDLE scheduling for revocable CPU in cgroups isolator.
>
>
> Diffs
> -----
>
> src/slave/containerizer/isolators/cgroups/cpushare.hpp ff4a9dbdb1b655e71bf87dcee8fe62433d396f52
> src/slave/containerizer/isolators/cgroups/cpushare.cpp 6a5b2b5c6e2844fe1a10815956569194b6f56681
>
> Diff: https://reviews.apache.org/r/34310/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 34310: Use IDLE scheduling for revocable CPU in
cgroups isolator.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34310/#review84353
-----------------------------------------------------------
src/slave/containerizer/isolators/cgroups/cpushare.cpp
<https://reviews.apache.org/r/34310/#comment135566>
Do you need a flag to control this? I would imagine some users might prefer not setting the scheduling policy for predictability and relying on the QoS controller to kill the revokable containers if resources used by PR tasks exceeds certain threshold.
Instead of adding a new slave flag, do you think it makes sense to pull this logic into a separate isolator (e.g., cpu/revokable_idle)?
- Jie Yu
On May 18, 2015, 8:49 p.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34310/
> -----------------------------------------------------------
>
> (Updated May 18, 2015, 8:49 p.m.)
>
>
> Review request for mesos, Joris Van Remoortere, Niklas Nielsen, and Vinod Kone.
>
>
> Bugs: MESOS-2652
> https://issues.apache.org/jira/browse/MESOS-2652
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Use IDLE scheduling for revocable CPU in cgroups isolator.
>
>
> Diffs
> -----
>
> src/slave/containerizer/isolators/cgroups/cpushare.hpp ff4a9dbdb1b655e71bf87dcee8fe62433d396f52
> src/slave/containerizer/isolators/cgroups/cpushare.cpp 6a5b2b5c6e2844fe1a10815956569194b6f56681
>
> Diff: https://reviews.apache.org/r/34310/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 34310: Use IDLE scheduling for revocable CPU in
cgroups isolator.
Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34310/
-----------------------------------------------------------
(Updated May 18, 2015, 1:49 p.m.)
Review request for mesos, Joris Van Remoortere, Niklas Nielsen, and Vinod Kone.
Changes
-------
Addressed comments.
Bugs: MESOS-2652
https://issues.apache.org/jira/browse/MESOS-2652
Repository: mesos
Description
-------
Use IDLE scheduling for revocable CPU in cgroups isolator.
Diffs (updated)
-----
src/slave/containerizer/isolators/cgroups/cpushare.hpp ff4a9dbdb1b655e71bf87dcee8fe62433d396f52
src/slave/containerizer/isolators/cgroups/cpushare.cpp 6a5b2b5c6e2844fe1a10815956569194b6f56681
Diff: https://reviews.apache.org/r/34310/diff/
Testing
-------
Thanks,
Ian Downes
Re: Review Request 34310: Use IDLE scheduling for revocable CPU in
cgroups isolator.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34310/#review84213
-----------------------------------------------------------
Bad patch!
Reviews applied: [34308, 34309, 34308]
Failed command: ./support/apply-review.sh -n -r 34308
Error:
2015-05-18 20:30:01 URL:https://reviews.apache.org/r/34308/diff/raw/ [1838/1838] -> "34308.patch" [1]
error: patch failed: include/mesos/resources.hpp:105
error: include/mesos/resources.hpp: patch does not apply
Failed to apply patch
- Mesos ReviewBot
On May 18, 2015, 5:33 p.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34310/
> -----------------------------------------------------------
>
> (Updated May 18, 2015, 5:33 p.m.)
>
>
> Review request for mesos, Joris Van Remoortere, Niklas Nielsen, and Vinod Kone.
>
>
> Bugs: MESOS-2652
> https://issues.apache.org/jira/browse/MESOS-2652
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Use IDLE scheduling for revocable CPU in cgroups isolator.
>
>
> Diffs
> -----
>
> src/slave/containerizer/isolators/cgroups/cpushare.hpp ff4a9dbdb1b655e71bf87dcee8fe62433d396f52
> src/slave/containerizer/isolators/cgroups/cpushare.cpp 6a5b2b5c6e2844fe1a10815956569194b6f56681
>
> Diff: https://reviews.apache.org/r/34310/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 34310: Use IDLE scheduling for revocable CPU in
cgroups isolator.
Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34310/
-----------------------------------------------------------
(Updated May 18, 2015, 10:33 a.m.)
Review request for mesos, Joris Van Remoortere, Niklas Nielsen, and Vinod Kone.
Bugs: MESOS-2652
https://issues.apache.org/jira/browse/MESOS-2652
Repository: mesos
Description
-------
Use IDLE scheduling for revocable CPU in cgroups isolator.
Diffs
-----
src/slave/containerizer/isolators/cgroups/cpushare.hpp ff4a9dbdb1b655e71bf87dcee8fe62433d396f52
src/slave/containerizer/isolators/cgroups/cpushare.cpp 6a5b2b5c6e2844fe1a10815956569194b6f56681
Diff: https://reviews.apache.org/r/34310/diff/
Testing
-------
Thanks,
Ian Downes
Re: Review Request 34310: Use IDLE scheduling for revocable CPU in
cgroups isolator.
Posted by Ben Mahler <be...@gmail.com>.
> On May 16, 2015, 8:03 p.m., Joris Van Remoortere wrote:
> > Hi Ian,
> >
> > I'm wondering about the `Note` regarding only setting the scheduling policy to IDLE if the initial resources are revocable. I think this exposes many scenarios where the isolator will seem `enabled` to the operator, but won't actually be isolating correctly.
> > I don't think it is uncommon to launch an executor with non-revocable resources, and then later launch BE tasks with revocable resources. This pattern would always lead to the isolator being in-effective right?
> >
> > I am not super familiar with the rules around the isolators, but why can we not adjust the policy within the update() call as opposed to in the isolate() function?
>
> Ian Downes wrote:
> This is not a limitation of the isolator interface but a complexity of setting the scheduling policy because it's task rather than cgroup based. We'd need to ensure we set it on all tasks and forking children. One approach could be to briefly freeze the cgroup and set the scheduling policy but I'm concerned that if the slave dies during the freeze/set/unfreeze operation then we've frozen a running container. Otherwise, we need some hackery as used in os:killtree() to ensure we get every task.
>
> We need to do it in isolate() regardless because that's the initial forked child which will later exec the executor and we may never get a subsequent update(). I'll look at adding it also to the update() call to capture updates to the resources after the executor has launched.
>
> I presume we also want to support the reverse update from having some revocable to having no revocable CPU, i.e., SCHED_IDLE -> SCHED_OTHER?
Why does it need to be as complicated as os::killtree? If we rely on cgroups to provide the list of processes, we shouldn't have to do the stopping and tree traversal logic. Just have to iterate on cgroup::processes until everything is set, no? Are you worried about the fork-bomb case where we iterate forever, unless the freezer is eventually used?
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34310/#review84039
-----------------------------------------------------------
On May 29, 2015, 7:57 p.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34310/
> -----------------------------------------------------------
>
> (Updated May 29, 2015, 7:57 p.m.)
>
>
> Review request for mesos, Joris Van Remoortere, Niklas Nielsen, and Vinod Kone.
>
>
> Bugs: MESOS-2652
> https://issues.apache.org/jira/browse/MESOS-2652
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Use IDLE scheduling for revocable CPU in cgroups isolator.
>
>
> Diffs
> -----
>
> src/slave/containerizer/isolators/cgroups/cpushare.hpp ff4a9dbdb1b655e71bf87dcee8fe62433d396f52
> src/slave/containerizer/isolators/cgroups/cpushare.cpp e3b5e2aa8ba06eec82b929144627d044f6b19d9d
> src/slave/flags.hpp 944ed794b6dc8a2d3712145a15b6b27cebf26a22
> src/slave/flags.cpp 6b7c61ea1a9f3b5d401d2aec935188a1b70c1738
> src/tests/isolator_tests.cpp 24c71b7906a92bdc84a38e88d6084ab09e3cf2ab
>
> Diff: https://reviews.apache.org/r/34310/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 34310: Use IDLE scheduling for revocable CPU in
cgroups isolator.
Posted by Ian Downes <ia...@gmail.com>.
> On May 16, 2015, 1:03 p.m., Joris Van Remoortere wrote:
> > Hi Ian,
> >
> > I'm wondering about the `Note` regarding only setting the scheduling policy to IDLE if the initial resources are revocable. I think this exposes many scenarios where the isolator will seem `enabled` to the operator, but won't actually be isolating correctly.
> > I don't think it is uncommon to launch an executor with non-revocable resources, and then later launch BE tasks with revocable resources. This pattern would always lead to the isolator being in-effective right?
> >
> > I am not super familiar with the rules around the isolators, but why can we not adjust the policy within the update() call as opposed to in the isolate() function?
>
> Ian Downes wrote:
> This is not a limitation of the isolator interface but a complexity of setting the scheduling policy because it's task rather than cgroup based. We'd need to ensure we set it on all tasks and forking children. One approach could be to briefly freeze the cgroup and set the scheduling policy but I'm concerned that if the slave dies during the freeze/set/unfreeze operation then we've frozen a running container. Otherwise, we need some hackery as used in os:killtree() to ensure we get every task.
>
> We need to do it in isolate() regardless because that's the initial forked child which will later exec the executor and we may never get a subsequent update(). I'll look at adding it also to the update() call to capture updates to the resources after the executor has launched.
>
> I presume we also want to support the reverse update from having some revocable to having no revocable CPU, i.e., SCHED_IDLE -> SCHED_OTHER?
>
> Ben Mahler wrote:
> Why does it need to be as complicated as os::killtree? If we rely on cgroups to provide the list of processes, we shouldn't have to do the stopping and tree traversal logic. Just have to iterate on cgroup::processes until everything is set, no? Are you worried about the fork-bomb case where we iterate forever, unless the freezer is eventually used?
Yeah, we can get the process list (actually, better to use the task list) from the cpu cgroup and iterate on that until set. Modulo fork-bombs it will complete in a "small" number of iterations because once set all children inherit it.
I'm happy to do this rather than supporting only revocable cpu being initially set.
- Ian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34310/#review84039
-----------------------------------------------------------
On May 29, 2015, 12:57 p.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34310/
> -----------------------------------------------------------
>
> (Updated May 29, 2015, 12:57 p.m.)
>
>
> Review request for mesos, Joris Van Remoortere, Niklas Nielsen, and Vinod Kone.
>
>
> Bugs: MESOS-2652
> https://issues.apache.org/jira/browse/MESOS-2652
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Use IDLE scheduling for revocable CPU in cgroups isolator.
>
>
> Diffs
> -----
>
> src/slave/containerizer/isolators/cgroups/cpushare.hpp ff4a9dbdb1b655e71bf87dcee8fe62433d396f52
> src/slave/containerizer/isolators/cgroups/cpushare.cpp e3b5e2aa8ba06eec82b929144627d044f6b19d9d
> src/slave/flags.hpp 944ed794b6dc8a2d3712145a15b6b27cebf26a22
> src/slave/flags.cpp 6b7c61ea1a9f3b5d401d2aec935188a1b70c1738
> src/tests/isolator_tests.cpp 24c71b7906a92bdc84a38e88d6084ab09e3cf2ab
>
> Diff: https://reviews.apache.org/r/34310/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 34310: Use IDLE scheduling for revocable CPU in
cgroups isolator.
Posted by Ian Downes <ia...@gmail.com>.
> On May 16, 2015, 1:03 p.m., Joris Van Remoortere wrote:
> > Hi Ian,
> >
> > I'm wondering about the `Note` regarding only setting the scheduling policy to IDLE if the initial resources are revocable. I think this exposes many scenarios where the isolator will seem `enabled` to the operator, but won't actually be isolating correctly.
> > I don't think it is uncommon to launch an executor with non-revocable resources, and then later launch BE tasks with revocable resources. This pattern would always lead to the isolator being in-effective right?
> >
> > I am not super familiar with the rules around the isolators, but why can we not adjust the policy within the update() call as opposed to in the isolate() function?
This is not a limitation of the isolator interface but a complexity of setting the scheduling policy because it's task rather than cgroup based. We'd need to ensure we set it on all tasks and forking children. One approach could be to briefly freeze the cgroup and set the scheduling policy but I'm concerned that if the slave dies during the freeze/set/unfreeze operation then we've frozen a running container. Otherwise, we need some hackery as used in os:killtree() to ensure we get every task.
We need to do it in isolate() regardless because that's the initial forked child which will later exec the executor and we may never get a subsequent update(). I'll look at adding it also to the update() call to capture updates to the resources after the executor has launched.
I presume we also want to support the reverse update from having some revocable to having no revocable CPU, i.e., SCHED_IDLE -> SCHED_OTHER?
- Ian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34310/#review84039
-----------------------------------------------------------
On May 18, 2015, 10:33 a.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34310/
> -----------------------------------------------------------
>
> (Updated May 18, 2015, 10:33 a.m.)
>
>
> Review request for mesos, Joris Van Remoortere, Niklas Nielsen, and Vinod Kone.
>
>
> Bugs: MESOS-2652
> https://issues.apache.org/jira/browse/MESOS-2652
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Use IDLE scheduling for revocable CPU in cgroups isolator.
>
>
> Diffs
> -----
>
> src/slave/containerizer/isolators/cgroups/cpushare.hpp ff4a9dbdb1b655e71bf87dcee8fe62433d396f52
> src/slave/containerizer/isolators/cgroups/cpushare.cpp 6a5b2b5c6e2844fe1a10815956569194b6f56681
>
> Diff: https://reviews.apache.org/r/34310/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 34310: Use IDLE scheduling for revocable CPU in
cgroups isolator.
Posted by Joris Van Remoortere <jo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34310/#review84039
-----------------------------------------------------------
Hi Ian,
I'm wondering about the `Note` regarding only setting the scheduling policy to IDLE if the initial resources are revocable. I think this exposes many scenarios where the isolator will seem `enabled` to the operator, but won't actually be isolating correctly.
I don't think it is uncommon to launch an executor with non-revocable resources, and then later launch BE tasks with revocable resources. This pattern would always lead to the isolator being in-effective right?
I am not super familiar with the rules around the isolators, but why can we not adjust the policy within the update() call as opposed to in the isolate() function?
src/slave/containerizer/isolators/cgroups/cpushare.cpp
<https://reviews.apache.org/r/34310/#comment135169>
Do you actually use anything provided by this header explicitly in this file? I don't think we need to include it again here.
src/slave/containerizer/isolators/cgroups/cpushare.cpp
<https://reviews.apache.org/r/34310/#comment135170>
Should we add the container id to this error message?
src/slave/containerizer/isolators/cgroups/cpushare.cpp
<https://reviews.apache.org/r/34310/#comment135171>
If this succeeds, should we log an INFO message? I think this is a significant enough system change to warrant some debugging info.
- Joris Van Remoortere
On May 16, 2015, 1:14 a.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34310/
> -----------------------------------------------------------
>
> (Updated May 16, 2015, 1:14 a.m.)
>
>
> Review request for mesos, Joris Van Remoortere, Niklas Nielsen, and Vinod Kone.
>
>
> Bugs: MESOS-2652
> https://issues.apache.org/jira/browse/MESOS-2652
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Use IDLE scheduling for revocable CPU in cgroups isolator.
>
>
> Diffs
> -----
>
> src/slave/containerizer/isolators/cgroups/cpushare.hpp ff4a9dbdb1b655e71bf87dcee8fe62433d396f52
> src/slave/containerizer/isolators/cgroups/cpushare.cpp 6a5b2b5c6e2844fe1a10815956569194b6f56681
>
> Diff: https://reviews.apache.org/r/34310/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>