You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Jie Yu <yu...@gmail.com> on 2018/08/20 04:34:08 UTC
Review Request 68426: Refactored some cgroups helpers to do verify
from callers.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68426/
-----------------------------------------------------------
Review request for mesos, Benjamin Mahler, Gilbert Song, and James Peach.
Bugs: MESOS-8418
https://issues.apache.org/jira/browse/MESOS-8418
Repository: mesos
Description
-------
Prior to this patch, some of the cgroups helpers call `cgroups::verify`
to make sure the hierarchy and cgroup are valid. This causes some
performance issues for the agent because `cgroups::verify` will read the
entire mount table, and mount table read is expensive if the mount table
is large. See more details in MESOS-8418.
In most of the cases, the verification has already been done when those
cgroups helpers are called. Therefore, it is better to let the caller do
the verification and make those cgroups helpers pure helpers.
Also, the verification is subject to race anyway. The cgroups global
state might change (e.g., operators modify the cgroups) after the
verification is done, making the verification itself not that super
useful.
This patch refactored the cgroups helpers and moved the verifications to
the callers.
Diffs
-----
src/linux/cgroups.hpp 282b8e783e8c70742e4388a19003382bc57db8d5
src/linux/cgroups.cpp 86b0306eee804732a55041d2d82c14db35ec5095
src/linux/systemd.cpp 2444ff4e432e61071c62e8e1ee94d62ac6b96c88
src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 1444c0543540cf61e253579e9e49d8687bd4ed3a
src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpu.cpp 321671f18b72edec1f8ed00f0069b539a698c4af
src/slave/containerizer/mesos/isolators/gpu/isolator.cpp d84f78c99eb118eba1a5bc3e74a2cf57556a325a
src/slave/containerizer/mesos/linux_launcher.cpp e51352af12743cc35f09b9fbb7d770f0108f908c
src/slave/main.cpp 489e87522588be259d382f588b66907ba29f1788
src/tests/containerizer/nested_mesos_containerizer_tests.cpp b80e40b6f4b18e076ebb3c1668f475286c337127
Diff: https://reviews.apache.org/r/68426/diff/1/
Testing
-------
sudo make check
Thanks,
Jie Yu
Re: Review Request 68426: Refactored some cgroups helpers to do
verify from callers.
Posted by James Peach <jp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68426/#review207619
-----------------------------------------------------------
Fix it, then Ship it!
src/linux/cgroups.hpp
Lines 80 (patched)
<https://reviews.apache.org/r/68426/#comment291069>
Why `Option<Error>` here? I think this should be `Try<Nothing>` for consistency with the rest of the cgroup APIs.
src/linux/cgroups.hpp
Lines 165 (patched)
<https://reviews.apache.org/r/68426/#comment291070>
Maybe rephrase for clarity:
```
...
The caller must make sure to remove all cgroups in the hierarchy before unmount.
...
```
src/linux/cgroups.hpp
Line 177 (original), 186 (patched)
<https://reviews.apache.org/r/68426/#comment291071>
"needs"
- James Peach
On Aug. 20, 2018, 4:46 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68426/
> -----------------------------------------------------------
>
> (Updated Aug. 20, 2018, 4:46 a.m.)
>
>
> Review request for mesos, Benjamin Mahler, Gilbert Song, and James Peach.
>
>
> Bugs: MESOS-8418 and MESOS-9081
> https://issues.apache.org/jira/browse/MESOS-8418
> https://issues.apache.org/jira/browse/MESOS-9081
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Prior to this patch, some of the cgroups helpers call `cgroups::verify`
> to make sure the hierarchy and cgroup are valid. This causes some
> performance issues for the agent because `cgroups::verify` will read the
> entire mount table, and mount table read is expensive if the mount table
> is large. See more details in MESOS-8418.
>
> In most of the cases, the verification has already been done when those
> cgroups helpers are called. Therefore, it is better to let the caller do
> the verification and make those cgroups helpers pure helpers.
>
> Also, the verification is subject to race anyway. The cgroups global
> state might change (e.g., operators modify the cgroups) after the
> verification is done, making the verification itself not that super
> useful.
>
> This patch refactored the cgroups helpers and moved the verifications to
> the callers.
>
>
> Diffs
> -----
>
> src/linux/cgroups.hpp 282b8e783e8c70742e4388a19003382bc57db8d5
> src/linux/cgroups.cpp 86b0306eee804732a55041d2d82c14db35ec5095
> src/linux/systemd.cpp 2444ff4e432e61071c62e8e1ee94d62ac6b96c88
> src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 1444c0543540cf61e253579e9e49d8687bd4ed3a
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpu.cpp 321671f18b72edec1f8ed00f0069b539a698c4af
> src/slave/containerizer/mesos/isolators/gpu/isolator.cpp d84f78c99eb118eba1a5bc3e74a2cf57556a325a
> src/slave/containerizer/mesos/linux_launcher.cpp e51352af12743cc35f09b9fbb7d770f0108f908c
> src/slave/main.cpp 489e87522588be259d382f588b66907ba29f1788
> src/tests/containerizer/nested_mesos_containerizer_tests.cpp b80e40b6f4b18e076ebb3c1668f475286c337127
>
>
> Diff: https://reviews.apache.org/r/68426/diff/1/
>
>
> Testing
> -------
>
> sudo make check
>
> I also used the benchmark I developed in https://reviews.apache.org/r/68266/ to see the speedup
>
> Before the patch
> Took 56.xxx secs to launch 1000 containers
> Took 55.xxx secs to destroy 1000 containers
>
> After the patch
> Took 37.xxx secs to launch 1000 containers
> Took 26.xxx secs to destroy 1000 containers
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 68426: Refactored some cgroups helpers to do
verify from callers.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68426/#review207602
-----------------------------------------------------------
Patch looks great!
Reviews applied: [68426]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose --disable-libtool-wrappers' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
On Aug. 20, 2018, 4:46 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68426/
> -----------------------------------------------------------
>
> (Updated Aug. 20, 2018, 4:46 a.m.)
>
>
> Review request for mesos, Benjamin Mahler, Gilbert Song, and James Peach.
>
>
> Bugs: MESOS-8418 and MESOS-9081
> https://issues.apache.org/jira/browse/MESOS-8418
> https://issues.apache.org/jira/browse/MESOS-9081
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Prior to this patch, some of the cgroups helpers call `cgroups::verify`
> to make sure the hierarchy and cgroup are valid. This causes some
> performance issues for the agent because `cgroups::verify` will read the
> entire mount table, and mount table read is expensive if the mount table
> is large. See more details in MESOS-8418.
>
> In most of the cases, the verification has already been done when those
> cgroups helpers are called. Therefore, it is better to let the caller do
> the verification and make those cgroups helpers pure helpers.
>
> Also, the verification is subject to race anyway. The cgroups global
> state might change (e.g., operators modify the cgroups) after the
> verification is done, making the verification itself not that super
> useful.
>
> This patch refactored the cgroups helpers and moved the verifications to
> the callers.
>
>
> Diffs
> -----
>
> src/linux/cgroups.hpp 282b8e783e8c70742e4388a19003382bc57db8d5
> src/linux/cgroups.cpp 86b0306eee804732a55041d2d82c14db35ec5095
> src/linux/systemd.cpp 2444ff4e432e61071c62e8e1ee94d62ac6b96c88
> src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 1444c0543540cf61e253579e9e49d8687bd4ed3a
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpu.cpp 321671f18b72edec1f8ed00f0069b539a698c4af
> src/slave/containerizer/mesos/isolators/gpu/isolator.cpp d84f78c99eb118eba1a5bc3e74a2cf57556a325a
> src/slave/containerizer/mesos/linux_launcher.cpp e51352af12743cc35f09b9fbb7d770f0108f908c
> src/slave/main.cpp 489e87522588be259d382f588b66907ba29f1788
> src/tests/containerizer/nested_mesos_containerizer_tests.cpp b80e40b6f4b18e076ebb3c1668f475286c337127
>
>
> Diff: https://reviews.apache.org/r/68426/diff/1/
>
>
> Testing
> -------
>
> sudo make check
>
> I also used the benchmark I developed in https://reviews.apache.org/r/68266/ to see the speedup
>
> Before the patch
> Took 56.xxx secs to launch 1000 containers
> Took 55.xxx secs to destroy 1000 containers
>
> After the patch
> Took 37.xxx secs to launch 1000 containers
> Took 26.xxx secs to destroy 1000 containers
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 68426: Refactored some cgroups helpers to do
verify from callers.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68426/#review207600
-----------------------------------------------------------
PASS: Mesos patch 68426 was successfully built and tested.
Reviews applied: `['68426']`
All the build artifacts available at: http://dcos-win.westus.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2193/mesos-review-68426
- Mesos Reviewbot Windows
On Aug. 20, 2018, 6:46 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68426/
> -----------------------------------------------------------
>
> (Updated Aug. 20, 2018, 6:46 a.m.)
>
>
> Review request for mesos, Benjamin Mahler, Gilbert Song, and James Peach.
>
>
> Bugs: MESOS-8418 and MESOS-9081
> https://issues.apache.org/jira/browse/MESOS-8418
> https://issues.apache.org/jira/browse/MESOS-9081
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Prior to this patch, some of the cgroups helpers call `cgroups::verify`
> to make sure the hierarchy and cgroup are valid. This causes some
> performance issues for the agent because `cgroups::verify` will read the
> entire mount table, and mount table read is expensive if the mount table
> is large. See more details in MESOS-8418.
>
> In most of the cases, the verification has already been done when those
> cgroups helpers are called. Therefore, it is better to let the caller do
> the verification and make those cgroups helpers pure helpers.
>
> Also, the verification is subject to race anyway. The cgroups global
> state might change (e.g., operators modify the cgroups) after the
> verification is done, making the verification itself not that super
> useful.
>
> This patch refactored the cgroups helpers and moved the verifications to
> the callers.
>
>
> Diffs
> -----
>
> src/linux/cgroups.hpp 282b8e783e8c70742e4388a19003382bc57db8d5
> src/linux/cgroups.cpp 86b0306eee804732a55041d2d82c14db35ec5095
> src/linux/systemd.cpp 2444ff4e432e61071c62e8e1ee94d62ac6b96c88
> src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 1444c0543540cf61e253579e9e49d8687bd4ed3a
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpu.cpp 321671f18b72edec1f8ed00f0069b539a698c4af
> src/slave/containerizer/mesos/isolators/gpu/isolator.cpp d84f78c99eb118eba1a5bc3e74a2cf57556a325a
> src/slave/containerizer/mesos/linux_launcher.cpp e51352af12743cc35f09b9fbb7d770f0108f908c
> src/slave/main.cpp 489e87522588be259d382f588b66907ba29f1788
> src/tests/containerizer/nested_mesos_containerizer_tests.cpp b80e40b6f4b18e076ebb3c1668f475286c337127
>
>
> Diff: https://reviews.apache.org/r/68426/diff/1/
>
>
> Testing
> -------
>
> sudo make check
>
> I also used the benchmark I developed in https://reviews.apache.org/r/68266/ to see the speedup
>
> Before the patch
> Took 56.xxx secs to launch 1000 containers
> Took 55.xxx secs to destroy 1000 containers
>
> After the patch
> Took 37.xxx secs to launch 1000 containers
> Took 26.xxx secs to destroy 1000 containers
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 68426: Refactored some cgroups helpers to do
verify from callers.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68426/#review207647
-----------------------------------------------------------
For posterity, numbers with --enabled-optimize:
```
Before Fix:
Prepared mount 'Took 55.044348852secs to launch 1000 containers
Took 52.265658534secs to destroy 1000 containers
After Fix:
Took 30.683818359secs to launch 1000 containers
Took 22.182255592secs to destroy 1000 containers
```
- Benjamin Mahler
On Aug. 20, 2018, 4:46 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68426/
> -----------------------------------------------------------
>
> (Updated Aug. 20, 2018, 4:46 a.m.)
>
>
> Review request for mesos, Benjamin Mahler, Gilbert Song, and James Peach.
>
>
> Bugs: MESOS-8418 and MESOS-9081
> https://issues.apache.org/jira/browse/MESOS-8418
> https://issues.apache.org/jira/browse/MESOS-9081
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Prior to this patch, some of the cgroups helpers call `cgroups::verify`
> to make sure the hierarchy and cgroup are valid. This causes some
> performance issues for the agent because `cgroups::verify` will read the
> entire mount table, and mount table read is expensive if the mount table
> is large. See more details in MESOS-8418.
>
> In most of the cases, the verification has already been done when those
> cgroups helpers are called. Therefore, it is better to let the caller do
> the verification and make those cgroups helpers pure helpers.
>
> Also, the verification is subject to race anyway. The cgroups global
> state might change (e.g., operators modify the cgroups) after the
> verification is done, making the verification itself not that super
> useful.
>
> This patch refactored the cgroups helpers and moved the verifications to
> the callers.
>
>
> Diffs
> -----
>
> src/linux/cgroups.hpp 282b8e783e8c70742e4388a19003382bc57db8d5
> src/linux/cgroups.cpp 86b0306eee804732a55041d2d82c14db35ec5095
> src/linux/systemd.cpp 2444ff4e432e61071c62e8e1ee94d62ac6b96c88
> src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 1444c0543540cf61e253579e9e49d8687bd4ed3a
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpu.cpp 321671f18b72edec1f8ed00f0069b539a698c4af
> src/slave/containerizer/mesos/isolators/gpu/isolator.cpp d84f78c99eb118eba1a5bc3e74a2cf57556a325a
> src/slave/containerizer/mesos/linux_launcher.cpp e51352af12743cc35f09b9fbb7d770f0108f908c
> src/slave/main.cpp 489e87522588be259d382f588b66907ba29f1788
> src/tests/containerizer/nested_mesos_containerizer_tests.cpp b80e40b6f4b18e076ebb3c1668f475286c337127
>
>
> Diff: https://reviews.apache.org/r/68426/diff/1/
>
>
> Testing
> -------
>
> sudo make check
>
> I also used the benchmark I developed in https://reviews.apache.org/r/68266/ to see the speedup
>
> Before the patch
> Took 56.xxx secs to launch 1000 containers
> Took 55.xxx secs to destroy 1000 containers
>
> After the patch
> Took 37.xxx secs to launch 1000 containers
> Took 26.xxx secs to destroy 1000 containers
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 68426: Refactored some cgroups helpers to do
verify from callers.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68426/
-----------------------------------------------------------
(Updated Aug. 20, 2018, 4:46 a.m.)
Review request for mesos, Benjamin Mahler, Gilbert Song, and James Peach.
Bugs: MESOS-8418 and MESOS-9081
https://issues.apache.org/jira/browse/MESOS-8418
https://issues.apache.org/jira/browse/MESOS-9081
Repository: mesos
Description
-------
Prior to this patch, some of the cgroups helpers call `cgroups::verify`
to make sure the hierarchy and cgroup are valid. This causes some
performance issues for the agent because `cgroups::verify` will read the
entire mount table, and mount table read is expensive if the mount table
is large. See more details in MESOS-8418.
In most of the cases, the verification has already been done when those
cgroups helpers are called. Therefore, it is better to let the caller do
the verification and make those cgroups helpers pure helpers.
Also, the verification is subject to race anyway. The cgroups global
state might change (e.g., operators modify the cgroups) after the
verification is done, making the verification itself not that super
useful.
This patch refactored the cgroups helpers and moved the verifications to
the callers.
Diffs
-----
src/linux/cgroups.hpp 282b8e783e8c70742e4388a19003382bc57db8d5
src/linux/cgroups.cpp 86b0306eee804732a55041d2d82c14db35ec5095
src/linux/systemd.cpp 2444ff4e432e61071c62e8e1ee94d62ac6b96c88
src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 1444c0543540cf61e253579e9e49d8687bd4ed3a
src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpu.cpp 321671f18b72edec1f8ed00f0069b539a698c4af
src/slave/containerizer/mesos/isolators/gpu/isolator.cpp d84f78c99eb118eba1a5bc3e74a2cf57556a325a
src/slave/containerizer/mesos/linux_launcher.cpp e51352af12743cc35f09b9fbb7d770f0108f908c
src/slave/main.cpp 489e87522588be259d382f588b66907ba29f1788
src/tests/containerizer/nested_mesos_containerizer_tests.cpp b80e40b6f4b18e076ebb3c1668f475286c337127
Diff: https://reviews.apache.org/r/68426/diff/1/
Testing (updated)
-------
sudo make check
I also used the benchmark I developed in https://reviews.apache.org/r/68266/ to see the speedup
Before the patch
Took 56.xxx secs to launch 1000 containers
Took 55.xxx secs to destroy 1000 containers
After the patch
Took 37.xxx secs to launch 1000 containers
Took 26.xxx secs to destroy 1000 containers
Thanks,
Jie Yu
Re: Review Request 68426: Refactored some cgroups helpers to do
verify from callers.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68426/
-----------------------------------------------------------
(Updated Aug. 20, 2018, 4:36 a.m.)
Review request for mesos, Benjamin Mahler, Gilbert Song, and James Peach.
Bugs: MESOS-8418 and MESOS-9081
https://issues.apache.org/jira/browse/MESOS-8418
https://issues.apache.org/jira/browse/MESOS-9081
Repository: mesos
Description
-------
Prior to this patch, some of the cgroups helpers call `cgroups::verify`
to make sure the hierarchy and cgroup are valid. This causes some
performance issues for the agent because `cgroups::verify` will read the
entire mount table, and mount table read is expensive if the mount table
is large. See more details in MESOS-8418.
In most of the cases, the verification has already been done when those
cgroups helpers are called. Therefore, it is better to let the caller do
the verification and make those cgroups helpers pure helpers.
Also, the verification is subject to race anyway. The cgroups global
state might change (e.g., operators modify the cgroups) after the
verification is done, making the verification itself not that super
useful.
This patch refactored the cgroups helpers and moved the verifications to
the callers.
Diffs
-----
src/linux/cgroups.hpp 282b8e783e8c70742e4388a19003382bc57db8d5
src/linux/cgroups.cpp 86b0306eee804732a55041d2d82c14db35ec5095
src/linux/systemd.cpp 2444ff4e432e61071c62e8e1ee94d62ac6b96c88
src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 1444c0543540cf61e253579e9e49d8687bd4ed3a
src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpu.cpp 321671f18b72edec1f8ed00f0069b539a698c4af
src/slave/containerizer/mesos/isolators/gpu/isolator.cpp d84f78c99eb118eba1a5bc3e74a2cf57556a325a
src/slave/containerizer/mesos/linux_launcher.cpp e51352af12743cc35f09b9fbb7d770f0108f908c
src/slave/main.cpp 489e87522588be259d382f588b66907ba29f1788
src/tests/containerizer/nested_mesos_containerizer_tests.cpp b80e40b6f4b18e076ebb3c1668f475286c337127
Diff: https://reviews.apache.org/r/68426/diff/1/
Testing
-------
sudo make check
Thanks,
Jie Yu