You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Chi Zhang <ch...@gmail.com> on 2015/02/21 02:12:12 UTC
Review Request 31250: Expose the number of processes and threads in a
container when cgroup is enabled.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31250/
-----------------------------------------------------------
Review request for mesos and Ian Downes.
Bugs: mesos-2103
https://issues.apache.org/jira/browse/mesos-2103
Repository: mesos
Description
-------
Ian, as discussed, I put the logic in cpu isolator for now until linux launcher sees more and more need to report isolator-independent cgroup usage.
Diffs
-----
include/mesos/mesos.proto 507845c493f65e154214fc7e562206e452990469
src/slave/containerizer/isolators/cgroups/cpushare.cpp e724b128f204e64a41561679c2797f81f64d1ab7
src/tests/containerizer_tests.cpp 06788493c263e623644f8d4df8cca44a74800ebb
Diff: https://reviews.apache.org/r/31250/diff/
Testing
-------
added a new test. the test is end-to-end so that future replacement of the actual logic won't require a change.
Thanks,
Chi Zhang
Re: Review Request 31250: Expose the number of processes and threads
in a container when cgroup is enabled.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31250/#review73760
-----------------------------------------------------------
Patch looks great!
Reviews applied: [31250]
All tests passed.
- Mesos ReviewBot
On Feb. 24, 2015, 12:45 a.m., Chi Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31250/
> -----------------------------------------------------------
>
> (Updated Feb. 24, 2015, 12:45 a.m.)
>
>
> Review request for mesos and Ian Downes.
>
>
> Bugs: mesos-2103
> https://issues.apache.org/jira/browse/mesos-2103
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Ian, as discussed, I put the logic in cpu isolator for now until linux launcher sees more and more need to report isolator-independent cgroup usage.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 9df972d750ce1e4a81d2e96cc508d6f83cad2fc8
> src/slave/containerizer/isolators/cgroups/cpushare.cpp b6df23963f20e5f14f484d449733655d46cd6884
> src/tests/isolator_tests.cpp f7323965b94543bfda78afc4f60fc677f4900ecd
>
> Diff: https://reviews.apache.org/r/31250/diff/
>
>
> Testing
> -------
>
> added a new test. the test is end-to-end so that future replacement of the actual logic won't require a change.
>
>
> Thanks,
>
> Chi Zhang
>
>
Re: Review Request 31250: Expose the number of processes and threads
in a container when cgroup is enabled.
Posted by Chi Zhang <ch...@gmail.com>.
> On March 3, 2015, 8:19 p.m., Jie Yu wrote:
> > src/slave/containerizer/isolators/cgroups/cpushare.cpp, lines 426-431
> > <https://reviews.apache.org/r/31250/diff/3/?file=883399#file883399line426>
> >
> > Can you add a slave flag so that we can turn it off if needed?
updated it.
- Chi
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31250/#review75026
-----------------------------------------------------------
On March 4, 2015, 1:47 a.m., Chi Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31250/
> -----------------------------------------------------------
>
> (Updated March 4, 2015, 1:47 a.m.)
>
>
> Review request for mesos and Ian Downes.
>
>
> Bugs: mesos-2103
> https://issues.apache.org/jira/browse/mesos-2103
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Ian, as discussed, I put the logic in cpu isolator for now until linux launcher sees more and more need to report isolator-independent cgroup usage.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 9df972d750ce1e4a81d2e96cc508d6f83cad2fc8
> src/slave/containerizer/isolators/cgroups/cpushare.cpp d4967687241ddf50a85beb961d691a8974f24fea
> src/slave/flags.hpp 56b25caf3901b38bdecb50310e8bcae0b114efa8
> src/tests/isolator_tests.cpp f7323965b94543bfda78afc4f60fc677f4900ecd
>
> Diff: https://reviews.apache.org/r/31250/diff/
>
>
> Testing
> -------
>
> added a new test. the test is end-to-end so that future replacement of the actual logic won't require a change.
>
>
> Thanks,
>
> Chi Zhang
>
>
Re: Review Request 31250: Expose the number of processes and threads
in a container when cgroup is enabled.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31250/#review75026
-----------------------------------------------------------
src/slave/containerizer/isolators/cgroups/cpushare.cpp
<https://reviews.apache.org/r/31250/#comment121957>
Can you add a slave flag so that we can turn it off if needed?
- Jie Yu
On March 3, 2015, 7:37 p.m., Chi Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31250/
> -----------------------------------------------------------
>
> (Updated March 3, 2015, 7:37 p.m.)
>
>
> Review request for mesos and Ian Downes.
>
>
> Bugs: mesos-2103
> https://issues.apache.org/jira/browse/mesos-2103
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Ian, as discussed, I put the logic in cpu isolator for now until linux launcher sees more and more need to report isolator-independent cgroup usage.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 9df972d750ce1e4a81d2e96cc508d6f83cad2fc8
> src/slave/containerizer/isolators/cgroups/cpushare.cpp d4967687241ddf50a85beb961d691a8974f24fea
> src/tests/isolator_tests.cpp f7323965b94543bfda78afc4f60fc677f4900ecd
>
> Diff: https://reviews.apache.org/r/31250/diff/
>
>
> Testing
> -------
>
> added a new test. the test is end-to-end so that future replacement of the actual logic won't require a change.
>
>
> Thanks,
>
> Chi Zhang
>
>
Re: Review Request 31250: Expose the number of processes and threads
in a container when cgroup is enabled.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31250/#review75192
-----------------------------------------------------------
Patch looks great!
Reviews applied: [31250]
All tests passed.
- Mesos ReviewBot
On March 4, 2015, 1:47 a.m., Chi Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31250/
> -----------------------------------------------------------
>
> (Updated March 4, 2015, 1:47 a.m.)
>
>
> Review request for mesos and Ian Downes.
>
>
> Bugs: mesos-2103
> https://issues.apache.org/jira/browse/mesos-2103
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Ian, as discussed, I put the logic in cpu isolator for now until linux launcher sees more and more need to report isolator-independent cgroup usage.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 9df972d750ce1e4a81d2e96cc508d6f83cad2fc8
> src/slave/containerizer/isolators/cgroups/cpushare.cpp d4967687241ddf50a85beb961d691a8974f24fea
> src/slave/flags.hpp 56b25caf3901b38bdecb50310e8bcae0b114efa8
> src/tests/isolator_tests.cpp f7323965b94543bfda78afc4f60fc677f4900ecd
>
> Diff: https://reviews.apache.org/r/31250/diff/
>
>
> Testing
> -------
>
> added a new test. the test is end-to-end so that future replacement of the actual logic won't require a change.
>
>
> Thanks,
>
> Chi Zhang
>
>
Re: Review Request 31250: Expose the number of processes and threads
in a container when cgroup is enabled.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31250/#review75465
-----------------------------------------------------------
Patch looks great!
Reviews applied: [31250]
All tests passed.
- Mesos ReviewBot
On March 6, 2015, 2 a.m., Chi Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31250/
> -----------------------------------------------------------
>
> (Updated March 6, 2015, 2 a.m.)
>
>
> Review request for mesos and Ian Downes.
>
>
> Bugs: mesos-2103
> https://issues.apache.org/jira/browse/mesos-2103
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Ian, as discussed, I put the logic in cpu isolator for now until linux launcher sees more and more need to report isolator-independent cgroup usage.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 9df972d750ce1e4a81d2e96cc508d6f83cad2fc8
> src/slave/containerizer/isolators/cgroups/cpushare.cpp d4967687241ddf50a85beb961d691a8974f24fea
> src/slave/flags.hpp 56b25caf3901b38bdecb50310e8bcae0b114efa8
> src/tests/isolator_tests.cpp f7323965b94543bfda78afc4f60fc677f4900ecd
>
> Diff: https://reviews.apache.org/r/31250/diff/
>
>
> Testing
> -------
>
> added a new test. the test is end-to-end so that future replacement of the actual logic won't require a change.
>
>
> Thanks,
>
> Chi Zhang
>
>
Re: Review Request 31250: Expose the number of processes and threads
in a container when cgroup is enabled.
Posted by Chi Zhang <ch...@gmail.com>.
> On March 6, 2015, 5:03 p.m., Dominic Hamon wrote:
> > src/tests/isolator_tests.cpp, line 577
> > <https://reviews.apache.org/r/31250/diff/2/?file=873351#file873351line577>
> >
> > 0U .. processes() and threads() are unsigned.
fixed.
> On March 6, 2015, 5:03 p.m., Dominic Hamon wrote:
> > src/tests/isolator_tests.cpp, line 583
> > <https://reviews.apache.org/r/31250/diff/2/?file=873351#file873351line583>
> >
> > vector<string> argv{"sh", "-c", "sleep 1000"};
> >
> > initializer lists!
kept the old way to remain consistent with rest of the file. Will keep a note on this for 'fresh' files.
> On March 6, 2015, 5:03 p.m., Dominic Hamon wrote:
> > src/tests/isolator_tests.cpp, line 586
> > <https://reviews.apache.org/r/31250/diff/2/?file=873351#file873351line586>
> >
> > this is flaky. what if the below takes longer than expected? or someone thinks that changing this value to be smaller will speed up the test?
> >
> > maybe 'while true; do sleep 1s; done;' is better?
fixed. thanks for the suggestion.
> On March 6, 2015, 5:03 p.m., Dominic Hamon wrote:
> > src/tests/isolator_tests.cpp, line 602
> > <https://reviews.apache.org/r/31250/diff/2/?file=873351#file873351line602>
> >
> > s/> >/>>/ throughout.
fixed.
> On March 6, 2015, 5:03 p.m., Dominic Hamon wrote:
> > src/slave/flags.hpp, line 229
> > <https://reviews.apache.org/r/31250/diff/5/?file=886975#file886975line229>
> >
> > having this in slave makes me uncomfortable. it really should be somewhere like the isolator given that it's accessed only there.
I agree. with slave getting bigger and bigger, we are gonna have all these minor feature flags in there. So far this is the only portal for slave flags.
- Chi
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31250/#review73835
-----------------------------------------------------------
On March 6, 2015, 6:36 p.m., Chi Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31250/
> -----------------------------------------------------------
>
> (Updated March 6, 2015, 6:36 p.m.)
>
>
> Review request for mesos and Ian Downes.
>
>
> Bugs: mesos-2103
> https://issues.apache.org/jira/browse/mesos-2103
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Expose the number of processes and threads in a container when cgroup is enabled.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 9df972d750ce1e4a81d2e96cc508d6f83cad2fc8
> src/slave/containerizer/isolators/cgroups/cpushare.cpp d4967687241ddf50a85beb961d691a8974f24fea
> src/slave/flags.hpp 56b25caf3901b38bdecb50310e8bcae0b114efa8
> src/tests/isolator_tests.cpp f7323965b94543bfda78afc4f60fc677f4900ecd
>
> Diff: https://reviews.apache.org/r/31250/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Chi Zhang
>
>
Re: Review Request 31250: Expose the number of processes and threads
in a container when cgroup is enabled.
Posted by Jie Yu <yu...@gmail.com>.
> On March 6, 2015, 5:03 p.m., Dominic Hamon wrote:
> > src/slave/flags.hpp, line 229
> > <https://reviews.apache.org/r/31250/diff/5/?file=886975#file886975line229>
> >
> > having this in slave makes me uncomfortable. it really should be somewhere like the isolator given that it's accessed only there.
>
> Chi Zhang wrote:
> I agree. with slave getting bigger and bigger, we are gonna have all these minor feature flags in there. So far this is the only portal for slave flags.
I agree as well. Right now, we use the prefix `cgroups` to indicate that it's a cgroups isolator related flag.
Ideally, if our flag parsing can have similar grouping and subcommand features in argparse of Python, that'll be awesome.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31250/#review73835
-----------------------------------------------------------
On March 6, 2015, 6:36 p.m., Chi Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31250/
> -----------------------------------------------------------
>
> (Updated March 6, 2015, 6:36 p.m.)
>
>
> Review request for mesos and Ian Downes.
>
>
> Bugs: mesos-2103
> https://issues.apache.org/jira/browse/mesos-2103
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Expose the number of processes and threads in a container when cgroup is enabled.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 9df972d750ce1e4a81d2e96cc508d6f83cad2fc8
> src/slave/containerizer/isolators/cgroups/cpushare.cpp d4967687241ddf50a85beb961d691a8974f24fea
> src/slave/flags.hpp 56b25caf3901b38bdecb50310e8bcae0b114efa8
> src/tests/isolator_tests.cpp f7323965b94543bfda78afc4f60fc677f4900ecd
>
> Diff: https://reviews.apache.org/r/31250/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Chi Zhang
>
>
Re: Review Request 31250: Expose the number of processes and threads
in a container when cgroup is enabled.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31250/#review73835
-----------------------------------------------------------
src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/31250/#comment120240>
0U .. processes() and threads() are unsigned.
src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/31250/#comment120242>
vector<string> argv{"sh", "-c", "sleep 1000"};
initializer lists!
src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/31250/#comment120244>
this is flaky. what if the below takes longer than expected? or someone thinks that changing this value to be smaller will speed up the test?
maybe 'while true; do sleep 1s; done;' is better?
src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/31250/#comment120243>
s/> >/>>/ throughout.
src/slave/flags.hpp
<https://reviews.apache.org/r/31250/#comment122593>
having this in slave makes me uncomfortable. it really should be somewhere like the isolator given that it's accessed only there.
- Dominic Hamon
On March 5, 2015, 6 p.m., Chi Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31250/
> -----------------------------------------------------------
>
> (Updated March 5, 2015, 6 p.m.)
>
>
> Review request for mesos and Ian Downes.
>
>
> Bugs: mesos-2103
> https://issues.apache.org/jira/browse/mesos-2103
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Ian, as discussed, I put the logic in cpu isolator for now until linux launcher sees more and more need to report isolator-independent cgroup usage.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 9df972d750ce1e4a81d2e96cc508d6f83cad2fc8
> src/slave/containerizer/isolators/cgroups/cpushare.cpp d4967687241ddf50a85beb961d691a8974f24fea
> src/slave/flags.hpp 56b25caf3901b38bdecb50310e8bcae0b114efa8
> src/tests/isolator_tests.cpp f7323965b94543bfda78afc4f60fc677f4900ecd
>
> Diff: https://reviews.apache.org/r/31250/diff/
>
>
> Testing
> -------
>
> added a new test. the test is end-to-end so that future replacement of the actual logic won't require a change.
>
>
> Thanks,
>
> Chi Zhang
>
>
Re: Review Request 31250: Expose the number of processes and threads
in a container when cgroup is enabled.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31250/#review75548
-----------------------------------------------------------
Patch looks great!
Reviews applied: [31250]
All tests passed.
- Mesos ReviewBot
On March 6, 2015, 6:36 p.m., Chi Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31250/
> -----------------------------------------------------------
>
> (Updated March 6, 2015, 6:36 p.m.)
>
>
> Review request for mesos and Ian Downes.
>
>
> Bugs: mesos-2103
> https://issues.apache.org/jira/browse/mesos-2103
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Expose the number of processes and threads in a container when cgroup is enabled.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 9df972d750ce1e4a81d2e96cc508d6f83cad2fc8
> src/slave/containerizer/isolators/cgroups/cpushare.cpp d4967687241ddf50a85beb961d691a8974f24fea
> src/slave/flags.hpp 56b25caf3901b38bdecb50310e8bcae0b114efa8
> src/tests/isolator_tests.cpp f7323965b94543bfda78afc4f60fc677f4900ecd
>
> Diff: https://reviews.apache.org/r/31250/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Chi Zhang
>
>
Re: Review Request 31250: Expose the number of processes and threads
in a container when cgroup is enabled.
Posted by Chi Zhang <ch...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31250/
-----------------------------------------------------------
(Updated March 6, 2015, 6:36 p.m.)
Review request for mesos and Ian Downes.
Bugs: mesos-2103
https://issues.apache.org/jira/browse/mesos-2103
Repository: mesos
Description (updated)
-------
Expose the number of processes and threads in a container when cgroup is enabled.
Diffs (updated)
-----
include/mesos/mesos.proto 9df972d750ce1e4a81d2e96cc508d6f83cad2fc8
src/slave/containerizer/isolators/cgroups/cpushare.cpp d4967687241ddf50a85beb961d691a8974f24fea
src/slave/flags.hpp 56b25caf3901b38bdecb50310e8bcae0b114efa8
src/tests/isolator_tests.cpp f7323965b94543bfda78afc4f60fc677f4900ecd
Diff: https://reviews.apache.org/r/31250/diff/
Testing (updated)
-------
Thanks,
Chi Zhang
Re: Review Request 31250: Expose the number of processes and threads
in a container when cgroup is enabled.
Posted by Chi Zhang <ch...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31250/
-----------------------------------------------------------
(Updated March 6, 2015, 2 a.m.)
Review request for mesos and Ian Downes.
Bugs: mesos-2103
https://issues.apache.org/jira/browse/mesos-2103
Repository: mesos
Description
-------
Ian, as discussed, I put the logic in cpu isolator for now until linux launcher sees more and more need to report isolator-independent cgroup usage.
Diffs (updated)
-----
include/mesos/mesos.proto 9df972d750ce1e4a81d2e96cc508d6f83cad2fc8
src/slave/containerizer/isolators/cgroups/cpushare.cpp d4967687241ddf50a85beb961d691a8974f24fea
src/slave/flags.hpp 56b25caf3901b38bdecb50310e8bcae0b114efa8
src/tests/isolator_tests.cpp f7323965b94543bfda78afc4f60fc677f4900ecd
Diff: https://reviews.apache.org/r/31250/diff/
Testing
-------
added a new test. the test is end-to-end so that future replacement of the actual logic won't require a change.
Thanks,
Chi Zhang
Re: Review Request 31250: Expose the number of processes and threads
in a container when cgroup is enabled.
Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31250/#review75370
-----------------------------------------------------------
Ship it!
src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/31250/#comment122375>
the other optional statistics default to false, is this safe to enable by default?
src/slave/flags.hpp
<https://reviews.apache.org/r/31250/#comment122372>
should we follow the naming of the flags for enabling additional network related statistics, i.e., we already have "network_enable_socket_statistics_summary" and "network_enable_socket_statistics_details", so perhaps something like "cgroups_cpu_enable_pids_and_tids_count"?
- Ian Downes
On March 3, 2015, 5:47 p.m., Chi Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31250/
> -----------------------------------------------------------
>
> (Updated March 3, 2015, 5:47 p.m.)
>
>
> Review request for mesos and Ian Downes.
>
>
> Bugs: mesos-2103
> https://issues.apache.org/jira/browse/mesos-2103
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Ian, as discussed, I put the logic in cpu isolator for now until linux launcher sees more and more need to report isolator-independent cgroup usage.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 9df972d750ce1e4a81d2e96cc508d6f83cad2fc8
> src/slave/containerizer/isolators/cgroups/cpushare.cpp d4967687241ddf50a85beb961d691a8974f24fea
> src/slave/flags.hpp 56b25caf3901b38bdecb50310e8bcae0b114efa8
> src/tests/isolator_tests.cpp f7323965b94543bfda78afc4f60fc677f4900ecd
>
> Diff: https://reviews.apache.org/r/31250/diff/
>
>
> Testing
> -------
>
> added a new test. the test is end-to-end so that future replacement of the actual logic won't require a change.
>
>
> Thanks,
>
> Chi Zhang
>
>
Re: Review Request 31250: Expose the number of processes and threads
in a container when cgroup is enabled.
Posted by Chi Zhang <ch...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31250/
-----------------------------------------------------------
(Updated March 4, 2015, 1:47 a.m.)
Review request for mesos and Ian Downes.
Bugs: mesos-2103
https://issues.apache.org/jira/browse/mesos-2103
Repository: mesos
Description
-------
Ian, as discussed, I put the logic in cpu isolator for now until linux launcher sees more and more need to report isolator-independent cgroup usage.
Diffs (updated)
-----
include/mesos/mesos.proto 9df972d750ce1e4a81d2e96cc508d6f83cad2fc8
src/slave/containerizer/isolators/cgroups/cpushare.cpp d4967687241ddf50a85beb961d691a8974f24fea
src/slave/flags.hpp 56b25caf3901b38bdecb50310e8bcae0b114efa8
src/tests/isolator_tests.cpp f7323965b94543bfda78afc4f60fc677f4900ecd
Diff: https://reviews.apache.org/r/31250/diff/
Testing
-------
added a new test. the test is end-to-end so that future replacement of the actual logic won't require a change.
Thanks,
Chi Zhang
Re: Review Request 31250: Expose the number of processes and threads
in a container when cgroup is enabled.
Posted by Chi Zhang <ch...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31250/
-----------------------------------------------------------
(Updated March 3, 2015, 7:37 p.m.)
Review request for mesos and Ian Downes.
Changes
-------
addressed comments.
Bugs: mesos-2103
https://issues.apache.org/jira/browse/mesos-2103
Repository: mesos
Description
-------
Ian, as discussed, I put the logic in cpu isolator for now until linux launcher sees more and more need to report isolator-independent cgroup usage.
Diffs (updated)
-----
include/mesos/mesos.proto 9df972d750ce1e4a81d2e96cc508d6f83cad2fc8
src/slave/containerizer/isolators/cgroups/cpushare.cpp d4967687241ddf50a85beb961d691a8974f24fea
src/tests/isolator_tests.cpp f7323965b94543bfda78afc4f60fc677f4900ecd
Diff: https://reviews.apache.org/r/31250/diff/
Testing
-------
added a new test. the test is end-to-end so that future replacement of the actual logic won't require a change.
Thanks,
Chi Zhang
Re: Review Request 31250: Expose the number of processes and threads
in a container when cgroup is enabled.
Posted by Chi Zhang <ch...@gmail.com>.
> On March 3, 2015, 6:17 p.m., Ian Downes wrote:
> > src/tests/isolator_tests.cpp, lines 607-611
> > <https://reviews.apache.org/r/31250/diff/2/?file=873351#file873351line607>
> >
> > Is the earlier test for pids=[], tids=[] redundant?
CpuIsolator creates a cgroup for the container in ::create, so the check above was intended to make sure it's empty right after creation. I've added a comment there.
This check could make sure nothing happens to the cgroup between prepare and isolate (the forking, and so on), and with the check right after isolate that isolate increases the number of pids and tids.
- Chi
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31250/#review74837
-----------------------------------------------------------
On March 3, 2015, 7:37 p.m., Chi Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31250/
> -----------------------------------------------------------
>
> (Updated March 3, 2015, 7:37 p.m.)
>
>
> Review request for mesos and Ian Downes.
>
>
> Bugs: mesos-2103
> https://issues.apache.org/jira/browse/mesos-2103
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Ian, as discussed, I put the logic in cpu isolator for now until linux launcher sees more and more need to report isolator-independent cgroup usage.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 9df972d750ce1e4a81d2e96cc508d6f83cad2fc8
> src/slave/containerizer/isolators/cgroups/cpushare.cpp d4967687241ddf50a85beb961d691a8974f24fea
> src/tests/isolator_tests.cpp f7323965b94543bfda78afc4f60fc677f4900ecd
>
> Diff: https://reviews.apache.org/r/31250/diff/
>
>
> Testing
> -------
>
> added a new test. the test is end-to-end so that future replacement of the actual logic won't require a change.
>
>
> Thanks,
>
> Chi Zhang
>
>
Re: Review Request 31250: Expose the number of processes and threads
in a container when cgroup is enabled.
Posted by Chi Zhang <ch...@gmail.com>.
> On March 3, 2015, 6:17 p.m., Ian Downes wrote:
> > src/tests/isolator_tests.cpp, lines 592-594
> > <https://reviews.apache.org/r/31250/diff/2/?file=873351#file873351line592>
> >
> > Can you use Subprocess::PIPE here so the process will terminate if the parent exits?
dropped as per discussion offline.
- Chi
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31250/#review74837
-----------------------------------------------------------
On March 4, 2015, 1:47 a.m., Chi Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31250/
> -----------------------------------------------------------
>
> (Updated March 4, 2015, 1:47 a.m.)
>
>
> Review request for mesos and Ian Downes.
>
>
> Bugs: mesos-2103
> https://issues.apache.org/jira/browse/mesos-2103
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Ian, as discussed, I put the logic in cpu isolator for now until linux launcher sees more and more need to report isolator-independent cgroup usage.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 9df972d750ce1e4a81d2e96cc508d6f83cad2fc8
> src/slave/containerizer/isolators/cgroups/cpushare.cpp d4967687241ddf50a85beb961d691a8974f24fea
> src/slave/flags.hpp 56b25caf3901b38bdecb50310e8bcae0b114efa8
> src/tests/isolator_tests.cpp f7323965b94543bfda78afc4f60fc677f4900ecd
>
> Diff: https://reviews.apache.org/r/31250/diff/
>
>
> Testing
> -------
>
> added a new test. the test is end-to-end so that future replacement of the actual logic won't require a change.
>
>
> Thanks,
>
> Chi Zhang
>
>
Re: Review Request 31250: Expose the number of processes and threads
in a container when cgroup is enabled.
Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31250/#review74837
-----------------------------------------------------------
Ship it!
src/slave/containerizer/isolators/cgroups/cpushare.cpp
<https://reviews.apache.org/r/31250/#comment121896>
s/opeation/operation/
src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/31250/#comment121617>
give it some memory too?
src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/31250/#comment121619>
Can you use Subprocess::PIPE here so the process will terminate if the parent exits?
src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/31250/#comment121620>
Is the earlier test for pids=[], tids=[] redundant?
src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/31250/#comment121890>
s/Process/Process count/
- Ian Downes
On Feb. 23, 2015, 4:45 p.m., Chi Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31250/
> -----------------------------------------------------------
>
> (Updated Feb. 23, 2015, 4:45 p.m.)
>
>
> Review request for mesos and Ian Downes.
>
>
> Bugs: mesos-2103
> https://issues.apache.org/jira/browse/mesos-2103
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Ian, as discussed, I put the logic in cpu isolator for now until linux launcher sees more and more need to report isolator-independent cgroup usage.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 9df972d750ce1e4a81d2e96cc508d6f83cad2fc8
> src/slave/containerizer/isolators/cgroups/cpushare.cpp b6df23963f20e5f14f484d449733655d46cd6884
> src/tests/isolator_tests.cpp f7323965b94543bfda78afc4f60fc677f4900ecd
>
> Diff: https://reviews.apache.org/r/31250/diff/
>
>
> Testing
> -------
>
> added a new test. the test is end-to-end so that future replacement of the actual logic won't require a change.
>
>
> Thanks,
>
> Chi Zhang
>
>
Re: Review Request 31250: Expose the number of processes and threads
in a container when cgroup is enabled.
Posted by Chi Zhang <ch...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31250/
-----------------------------------------------------------
(Updated Feb. 24, 2015, 12:45 a.m.)
Review request for mesos and Ian Downes.
Changes
-------
addressed comments.
Bugs: mesos-2103
https://issues.apache.org/jira/browse/mesos-2103
Repository: mesos
Description
-------
Ian, as discussed, I put the logic in cpu isolator for now until linux launcher sees more and more need to report isolator-independent cgroup usage.
Diffs (updated)
-----
include/mesos/mesos.proto 9df972d750ce1e4a81d2e96cc508d6f83cad2fc8
src/slave/containerizer/isolators/cgroups/cpushare.cpp b6df23963f20e5f14f484d449733655d46cd6884
src/tests/isolator_tests.cpp f7323965b94543bfda78afc4f60fc677f4900ecd
Diff: https://reviews.apache.org/r/31250/diff/
Testing
-------
added a new test. the test is end-to-end so that future replacement of the actual logic won't require a change.
Thanks,
Chi Zhang
Re: Review Request 31250: Expose the number of processes and threads
in a container when cgroup is enabled.
Posted by Dominic Hamon <dh...@twopensource.com>.
> On Feb. 23, 2015, 9:43 a.m., Ian Downes wrote:
> > src/slave/containerizer/isolators/cgroups/cpushare.cpp, line 421
> > <https://reviews.apache.org/r/31250/diff/1/?file=871370#file871370line421>
> >
> > It's hitting the kernel: I'm not sure how it internally keeps track of the processes/threads in a cgroup, I would hope that it's not a list but....
>
> Chi Zhang wrote:
> it is a list. left a comment in the code.
so this is O(N log(N)) to read the string, iterate through it and insert into the set.
what is N? same for threads below.
- Dominic
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31250/#review73591
-----------------------------------------------------------
On March 5, 2015, 6 p.m., Chi Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31250/
> -----------------------------------------------------------
>
> (Updated March 5, 2015, 6 p.m.)
>
>
> Review request for mesos and Ian Downes.
>
>
> Bugs: mesos-2103
> https://issues.apache.org/jira/browse/mesos-2103
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Ian, as discussed, I put the logic in cpu isolator for now until linux launcher sees more and more need to report isolator-independent cgroup usage.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 9df972d750ce1e4a81d2e96cc508d6f83cad2fc8
> src/slave/containerizer/isolators/cgroups/cpushare.cpp d4967687241ddf50a85beb961d691a8974f24fea
> src/slave/flags.hpp 56b25caf3901b38bdecb50310e8bcae0b114efa8
> src/tests/isolator_tests.cpp f7323965b94543bfda78afc4f60fc677f4900ecd
>
> Diff: https://reviews.apache.org/r/31250/diff/
>
>
> Testing
> -------
>
> added a new test. the test is end-to-end so that future replacement of the actual logic won't require a change.
>
>
> Thanks,
>
> Chi Zhang
>
>
Re: Review Request 31250: Expose the number of processes and threads
in a container when cgroup is enabled.
Posted by Chi Zhang <ch...@gmail.com>.
> On Feb. 23, 2015, 5:43 p.m., Ian Downes wrote:
> > src/tests/containerizer_tests.cpp, line 427
> > <https://reviews.apache.org/r/31250/diff/1/?file=871371#file871371line427>
> >
> > This is hard-coded to true already and is soon to be removed, I believe. Can you check if this is all necessary?
I had to add it because otherwise resources from non-checkpointing slaves will be considered not useable by checkpointing frameworks.
> On Feb. 23, 2015, 5:43 p.m., Ian Downes wrote:
> > src/slave/containerizer/isolators/cgroups/cpushare.cpp, line 421
> > <https://reviews.apache.org/r/31250/diff/1/?file=871370#file871370line421>
> >
> > It's hitting the kernel: I'm not sure how it internally keeps track of the processes/threads in a cgroup, I would hope that it's not a list but....
it is a list. left a comment in the code.
- Chi
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31250/#review73591
-----------------------------------------------------------
On Feb. 24, 2015, 12:45 a.m., Chi Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31250/
> -----------------------------------------------------------
>
> (Updated Feb. 24, 2015, 12:45 a.m.)
>
>
> Review request for mesos and Ian Downes.
>
>
> Bugs: mesos-2103
> https://issues.apache.org/jira/browse/mesos-2103
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Ian, as discussed, I put the logic in cpu isolator for now until linux launcher sees more and more need to report isolator-independent cgroup usage.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 9df972d750ce1e4a81d2e96cc508d6f83cad2fc8
> src/slave/containerizer/isolators/cgroups/cpushare.cpp b6df23963f20e5f14f484d449733655d46cd6884
> src/tests/isolator_tests.cpp f7323965b94543bfda78afc4f60fc677f4900ecd
>
> Diff: https://reviews.apache.org/r/31250/diff/
>
>
> Testing
> -------
>
> added a new test. the test is end-to-end so that future replacement of the actual logic won't require a change.
>
>
> Thanks,
>
> Chi Zhang
>
>
Re: Review Request 31250: Expose the number of processes and threads
in a container when cgroup is enabled.
Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31250/#review73591
-----------------------------------------------------------
src/slave/containerizer/isolators/cgroups/cpushare.cpp
<https://reviews.apache.org/r/31250/#comment119988>
s/itto/it to/
src/slave/containerizer/isolators/cgroups/cpushare.cpp
<https://reviews.apache.org/r/31250/#comment119990>
It's hitting the kernel: I'm not sure how it internally keeps track of the processes/threads in a cgroup, I would hope that it's not a list but....
src/tests/containerizer_tests.cpp
<https://reviews.apache.org/r/31250/#comment119991>
This is hard-coded to true already and is soon to be removed, I believe. Can you check if this is all necessary?
src/tests/containerizer_tests.cpp
<https://reviews.apache.org/r/31250/#comment119993>
Why is this a containerizer test - could you not test just the cpu isolator without bringing up an entire cluster?
src/tests/containerizer_tests.cpp
<https://reviews.apache.org/r/31250/#comment119994>
what's the purpose of the while loop - can't you just block on the last sleep?
- Ian Downes
On Feb. 20, 2015, 5:12 p.m., Chi Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31250/
> -----------------------------------------------------------
>
> (Updated Feb. 20, 2015, 5:12 p.m.)
>
>
> Review request for mesos and Ian Downes.
>
>
> Bugs: mesos-2103
> https://issues.apache.org/jira/browse/mesos-2103
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Ian, as discussed, I put the logic in cpu isolator for now until linux launcher sees more and more need to report isolator-independent cgroup usage.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 507845c493f65e154214fc7e562206e452990469
> src/slave/containerizer/isolators/cgroups/cpushare.cpp e724b128f204e64a41561679c2797f81f64d1ab7
> src/tests/containerizer_tests.cpp 06788493c263e623644f8d4df8cca44a74800ebb
>
> Diff: https://reviews.apache.org/r/31250/diff/
>
>
> Testing
> -------
>
> added a new test. the test is end-to-end so that future replacement of the actual logic won't require a change.
>
>
> Thanks,
>
> Chi Zhang
>
>
Re: Review Request 31250: Expose the number of processes and threads
in a container when cgroup is enabled.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31250/#review73381
-----------------------------------------------------------
Patch looks great!
Reviews applied: [31250]
All tests passed.
- Mesos ReviewBot
On Feb. 21, 2015, 1:12 a.m., Chi Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31250/
> -----------------------------------------------------------
>
> (Updated Feb. 21, 2015, 1:12 a.m.)
>
>
> Review request for mesos and Ian Downes.
>
>
> Bugs: mesos-2103
> https://issues.apache.org/jira/browse/mesos-2103
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Ian, as discussed, I put the logic in cpu isolator for now until linux launcher sees more and more need to report isolator-independent cgroup usage.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 507845c493f65e154214fc7e562206e452990469
> src/slave/containerizer/isolators/cgroups/cpushare.cpp e724b128f204e64a41561679c2797f81f64d1ab7
> src/tests/containerizer_tests.cpp 06788493c263e623644f8d4df8cca44a74800ebb
>
> Diff: https://reviews.apache.org/r/31250/diff/
>
>
> Testing
> -------
>
> added a new test. the test is end-to-end so that future replacement of the actual logic won't require a change.
>
>
> Thanks,
>
> Chi Zhang
>
>
Re: Review Request 31250: Expose the number of processes and threads
in a container when cgroup is enabled.
Posted by Chi Zhang <ch...@gmail.com>.
> On Feb. 21, 2015, 1:15 a.m., Dominic Hamon wrote:
> > src/slave/containerizer/isolators/cgroups/cpushare.cpp, line 421
> > <https://reviews.apache.org/r/31250/diff/1/?file=871370#file871370line421>
> >
> > does this scale linearly with number of processes?
it does. :(
This is the only interface we have. I left a comment in the code.
- Chi
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31250/#review73360
-----------------------------------------------------------
On Feb. 24, 2015, 12:45 a.m., Chi Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31250/
> -----------------------------------------------------------
>
> (Updated Feb. 24, 2015, 12:45 a.m.)
>
>
> Review request for mesos and Ian Downes.
>
>
> Bugs: mesos-2103
> https://issues.apache.org/jira/browse/mesos-2103
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Ian, as discussed, I put the logic in cpu isolator for now until linux launcher sees more and more need to report isolator-independent cgroup usage.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 9df972d750ce1e4a81d2e96cc508d6f83cad2fc8
> src/slave/containerizer/isolators/cgroups/cpushare.cpp b6df23963f20e5f14f484d449733655d46cd6884
> src/tests/isolator_tests.cpp f7323965b94543bfda78afc4f60fc677f4900ecd
>
> Diff: https://reviews.apache.org/r/31250/diff/
>
>
> Testing
> -------
>
> added a new test. the test is end-to-end so that future replacement of the actual logic won't require a change.
>
>
> Thanks,
>
> Chi Zhang
>
>
Re: Review Request 31250: Expose the number of processes and threads
in a container when cgroup is enabled.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31250/#review73360
-----------------------------------------------------------
include/mesos/mesos.proto
<https://reviews.apache.org/r/31250/#comment119665>
uint64? i hope we don't have more than 2^32 processes in a container!
src/slave/containerizer/isolators/cgroups/cpushare.cpp
<https://reviews.apache.org/r/31250/#comment119667>
s/> >/>>/
src/slave/containerizer/isolators/cgroups/cpushare.cpp
<https://reviews.apache.org/r/31250/#comment119670>
does this scale linearly with number of processes?
src/slave/containerizer/isolators/cgroups/cpushare.cpp
<https://reviews.apache.org/r/31250/#comment119671>
does this scale linearly with number of threads?
- Dominic Hamon
On Feb. 20, 2015, 5:12 p.m., Chi Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31250/
> -----------------------------------------------------------
>
> (Updated Feb. 20, 2015, 5:12 p.m.)
>
>
> Review request for mesos and Ian Downes.
>
>
> Bugs: mesos-2103
> https://issues.apache.org/jira/browse/mesos-2103
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Ian, as discussed, I put the logic in cpu isolator for now until linux launcher sees more and more need to report isolator-independent cgroup usage.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 507845c493f65e154214fc7e562206e452990469
> src/slave/containerizer/isolators/cgroups/cpushare.cpp e724b128f204e64a41561679c2797f81f64d1ab7
> src/tests/containerizer_tests.cpp 06788493c263e623644f8d4df8cca44a74800ebb
>
> Diff: https://reviews.apache.org/r/31250/diff/
>
>
> Testing
> -------
>
> added a new test. the test is end-to-end so that future replacement of the actual logic won't require a change.
>
>
> Thanks,
>
> Chi Zhang
>
>