You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Qian Zhang <zh...@gmail.com> on 2020/03/11 02:42:32 UTC
Re: Review Request 71953: Updated the test
`ROOT_CGROUPS_CFS_EnableCfs`.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71953/
-----------------------------------------------------------
(Updated March 11, 2020, 10:42 a.m.)
Review request for mesos, Andrei Budnik and Greg Mann.
Changes
-------
Addressed review comment.
Summary (updated)
-----------------
Updated the test `ROOT_CGROUPS_CFS_EnableCfs`.
Bugs: MESOS-10047
https://issues.apache.org/jira/browse/MESOS-10047
Repository: mesos
Description (updated)
-------
This test is renamed to `ROOT_CGROUPS_CFS_CommandTaskNoLimits`, and
besides CFS quota, now it also verifies CPU shares, memory soft and
hard limits and OOM score adjustment.
Diffs (updated)
-----
src/tests/containerizer/cgroups_isolator_tests.cpp f72e6cdab417368e63349915114aeed586e0ef0e
Diff: https://reviews.apache.org/r/71953/diff/4/
Changes: https://reviews.apache.org/r/71953/diff/3-4/
Testing
-------
sudo make check
Thanks,
Qian Zhang
Re: Review Request 71953: Updated the test
`ROOT_CGROUPS_CFS_EnableCfs`.
Posted by Qian Zhang <zh...@gmail.com>.
> On March 16, 2020, 8:44 p.m., Greg Mann wrote:
> > src/tests/containerizer/cgroups_isolator_tests.cpp
> > Lines 465-473 (original), 525-533 (patched)
> > <https://reviews.apache.org/r/71953/diff/5/?file=2214045#file2214045line525>
> >
> > Why are our expectations for CPU consumption changed after your update to the test? Aren't we still setting the CFS quota in the same way?
>
> Qian Zhang wrote:
> That's actually a bug in this test: besides the CPU times consumed by the task, there are also some overhead caused by the executor (e.g. launching executor will also take some CPU times), so in total the actually CPU usage will be a bit higher due to the overhead.
Previously in this test we did add some overhead (0.05) when checking the CPU time, however that overhead is not big enough so this test fails occasionally.
- Qian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71953/#review219956
-----------------------------------------------------------
On March 16, 2020, 5:12 p.m., Qian Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71953/
> -----------------------------------------------------------
>
> (Updated March 16, 2020, 5:12 p.m.)
>
>
> Review request for mesos, Andrei Budnik and Greg Mann.
>
>
> Bugs: MESOS-10047
> https://issues.apache.org/jira/browse/MESOS-10047
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This test is renamed to `ROOT_CGROUPS_CFS_CommandTaskNoLimits`, and
> besides CFS quota, now it also verifies CPU shares, memory soft and
> hard limits and OOM score adjustment.
>
>
> Diffs
> -----
>
> src/tests/containerizer/cgroups_isolator_tests.cpp f72e6cdab417368e63349915114aeed586e0ef0e
>
>
> Diff: https://reviews.apache.org/r/71953/diff/5/
>
>
> Testing
> -------
>
> sudo make check
>
>
> Thanks,
>
> Qian Zhang
>
>
Re: Review Request 71953: Updated the test
`ROOT_CGROUPS_CFS_EnableCfs`.
Posted by Qian Zhang <zh...@gmail.com>.
> On March 16, 2020, 8:44 p.m., Greg Mann wrote:
> > src/tests/containerizer/cgroups_isolator_tests.cpp
> > Lines 465-473 (original), 525-533 (patched)
> > <https://reviews.apache.org/r/71953/diff/5/?file=2214045#file2214045line525>
> >
> > Why are our expectations for CPU consumption changed after your update to the test? Aren't we still setting the CFS quota in the same way?
That's actually a bug in this test: besides the CPU times consumed by the task, there are also some overhead caused by the executor (e.g. launching executor will also take some CPU times), so in total the actually CPU usage will be a bit higher due to the overhead.
- Qian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71953/#review219956
-----------------------------------------------------------
On March 16, 2020, 5:12 p.m., Qian Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71953/
> -----------------------------------------------------------
>
> (Updated March 16, 2020, 5:12 p.m.)
>
>
> Review request for mesos, Andrei Budnik and Greg Mann.
>
>
> Bugs: MESOS-10047
> https://issues.apache.org/jira/browse/MESOS-10047
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This test is renamed to `ROOT_CGROUPS_CFS_CommandTaskNoLimits`, and
> besides CFS quota, now it also verifies CPU shares, memory soft and
> hard limits and OOM score adjustment.
>
>
> Diffs
> -----
>
> src/tests/containerizer/cgroups_isolator_tests.cpp f72e6cdab417368e63349915114aeed586e0ef0e
>
>
> Diff: https://reviews.apache.org/r/71953/diff/5/
>
>
> Testing
> -------
>
> sudo make check
>
>
> Thanks,
>
> Qian Zhang
>
>
Re: Review Request 71953: Updated the test
`ROOT_CGROUPS_CFS_EnableCfs`.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71953/#review219956
-----------------------------------------------------------
src/tests/containerizer/cgroups_isolator_tests.cpp
Lines 498 (patched)
<https://reviews.apache.org/r/71953/#comment308194>
This can probably be EXPECT_EQ?
src/tests/containerizer/cgroups_isolator_tests.cpp
Lines 465-473 (original), 525-533 (patched)
<https://reviews.apache.org/r/71953/#comment308196>
Why are our expectations for CPU consumption changed after your update to the test? Aren't we still setting the CFS quota in the same way?
- Greg Mann
On March 16, 2020, 9:12 a.m., Qian Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71953/
> -----------------------------------------------------------
>
> (Updated March 16, 2020, 9:12 a.m.)
>
>
> Review request for mesos, Andrei Budnik and Greg Mann.
>
>
> Bugs: MESOS-10047
> https://issues.apache.org/jira/browse/MESOS-10047
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This test is renamed to `ROOT_CGROUPS_CFS_CommandTaskNoLimits`, and
> besides CFS quota, now it also verifies CPU shares, memory soft and
> hard limits and OOM score adjustment.
>
>
> Diffs
> -----
>
> src/tests/containerizer/cgroups_isolator_tests.cpp f72e6cdab417368e63349915114aeed586e0ef0e
>
>
> Diff: https://reviews.apache.org/r/71953/diff/5/
>
>
> Testing
> -------
>
> sudo make check
>
>
> Thanks,
>
> Qian Zhang
>
>
Re: Review Request 71953: Updated the test
`ROOT_CGROUPS_CFS_EnableCfs`.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71953/#review219975
-----------------------------------------------------------
Ship it!
Ship It!
- Greg Mann
On March 17, 2020, 7:03 a.m., Qian Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71953/
> -----------------------------------------------------------
>
> (Updated March 17, 2020, 7:03 a.m.)
>
>
> Review request for mesos, Andrei Budnik and Greg Mann.
>
>
> Bugs: MESOS-10047
> https://issues.apache.org/jira/browse/MESOS-10047
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This test is renamed to `ROOT_CGROUPS_CFS_CommandTaskNoLimits`, and
> besides CFS quota, now it also verifies CPU shares, memory soft and
> hard limits and OOM score adjustment.
>
>
> Diffs
> -----
>
> src/tests/containerizer/cgroups_isolator_tests.cpp f72e6cdab417368e63349915114aeed586e0ef0e
>
>
> Diff: https://reviews.apache.org/r/71953/diff/6/
>
>
> Testing
> -------
>
> sudo make check
>
>
> Thanks,
>
> Qian Zhang
>
>
Re: Review Request 71953: Updated the test
`ROOT_CGROUPS_CFS_EnableCfs`.
Posted by Qian Zhang <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71953/
-----------------------------------------------------------
(Updated March 17, 2020, 3:03 p.m.)
Review request for mesos, Andrei Budnik and Greg Mann.
Changes
-------
Addressed review comments.
Bugs: MESOS-10047
https://issues.apache.org/jira/browse/MESOS-10047
Repository: mesos
Description
-------
This test is renamed to `ROOT_CGROUPS_CFS_CommandTaskNoLimits`, and
besides CFS quota, now it also verifies CPU shares, memory soft and
hard limits and OOM score adjustment.
Diffs (updated)
-----
src/tests/containerizer/cgroups_isolator_tests.cpp f72e6cdab417368e63349915114aeed586e0ef0e
Diff: https://reviews.apache.org/r/71953/diff/6/
Changes: https://reviews.apache.org/r/71953/diff/5-6/
Testing
-------
sudo make check
Thanks,
Qian Zhang
Re: Review Request 71953: Updated the test
`ROOT_CGROUPS_CFS_EnableCfs`.
Posted by Qian Zhang <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71953/
-----------------------------------------------------------
(Updated March 16, 2020, 5:12 p.m.)
Review request for mesos, Andrei Budnik and Greg Mann.
Changes
-------
Rebased.
Bugs: MESOS-10047
https://issues.apache.org/jira/browse/MESOS-10047
Repository: mesos
Description
-------
This test is renamed to `ROOT_CGROUPS_CFS_CommandTaskNoLimits`, and
besides CFS quota, now it also verifies CPU shares, memory soft and
hard limits and OOM score adjustment.
Diffs (updated)
-----
src/tests/containerizer/cgroups_isolator_tests.cpp f72e6cdab417368e63349915114aeed586e0ef0e
Diff: https://reviews.apache.org/r/71953/diff/5/
Changes: https://reviews.apache.org/r/71953/diff/4-5/
Testing
-------
sudo make check
Thanks,
Qian Zhang