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 2017/05/15 20:56:24 UTC
Review Request 59294: Optionally scale egress bandwidth with CPU.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59294/
-----------------------------------------------------------
Review request for mesos, Dmitry Zhuk, Ilya Pronin, Jie Yu, and Santhosh Kumar Shanmugham.
Bugs: MESOS-7508
https://issues.apache.org/jira/browse/MESOS-7508
Repository: mesos
Description
-------
Add support to isolators/port_mapping for optionally scaling egress bandwidth with CPU and with minimum and maximum limits.
Diffs
-----
src/slave/containerizer/mesos/isolators/network/port_mapping.hpp 9d38289c7161d5e931053b587d115684ccc44c94
src/slave/containerizer/mesos/isolators/network/port_mapping.cpp cd008aaebcd42554a9a81d2b059269546f59c966
src/slave/flags.hpp e5784ef81ad0720c7ec061ee0b28b8fadae77afd
src/slave/flags.cpp bc63a6a4cb6115b4b4d592e67e34045f52b50d4c
src/tests/containerizer/port_mapping_tests.cpp a528382e8b4831b9c7e8dcc877a5e242909f0cd5
Diff: https://reviews.apache.org/r/59294/diff/1/
Testing
-------
# added a new test
$ make check
Thanks,
Ian Downes
Re: Review Request 59294: Optionally scale egress bandwidth with CPU.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59294/#review176336
-----------------------------------------------------------
Patch looks great!
Reviews applied: [59294]
Passed command: support\windows-build.bat
- Mesos Reviewbot Windows
On May 26, 2017, 6:23 p.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59294/
> -----------------------------------------------------------
>
> (Updated May 26, 2017, 6:23 p.m.)
>
>
> Review request for mesos, Dmitry Zhuk, Ilya Pronin, and Jie Yu.
>
>
> Bugs: MESOS-7508
> https://issues.apache.org/jira/browse/MESOS-7508
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add support to isolators/port_mapping for optionally scaling egress bandwidth with CPU and with minimum and maximum limits.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/network/port_mapping.hpp 9d38289c7161d5e931053b587d115684ccc44c94
> src/slave/containerizer/mesos/isolators/network/port_mapping.cpp cd008aaebcd42554a9a81d2b059269546f59c966
> src/slave/flags.hpp b66995630f89dfb95a6d0cf66efc5d7590e90cbc
> src/slave/flags.cpp 0c8276e425a6a7d22ee68edc6cc25b331635ec44
> src/tests/containerizer/port_mapping_tests.cpp d062f2f6bcf7b44dbcde951cdca23b0a2cd42115
>
>
> Diff: https://reviews.apache.org/r/59294/diff/2/
>
>
> Testing
> -------
>
> # added a new test
> $ make check
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 59294: Optionally scale egress bandwidth with CPU.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59294/#review180440
-----------------------------------------------------------
Patch looks great!
Reviews applied: [60203, 59294]
Passed command: support\windows-build.bat
- Mesos Reviewbot Windows
On June 30, 2017, 11 p.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59294/
> -----------------------------------------------------------
>
> (Updated June 30, 2017, 11 p.m.)
>
>
> Review request for mesos, Dmitry Zhuk, Ilya Pronin, and Jie Yu.
>
>
> Bugs: MESOS-7508
> https://issues.apache.org/jira/browse/MESOS-7508
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add support to isolators/port_mapping for optionally scaling egress bandwidth with CPU and with minimum and maximum limits.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto e2502acc3542d64c2b2b13f8c0363a3c8372e20e
> src/slave/containerizer/mesos/isolators/network/helper.cpp 4ed879dca42f85fc9dd7638e763822cf10fa8405
> src/slave/containerizer/mesos/isolators/network/port_mapping.hpp 9d38289c7161d5e931053b587d115684ccc44c94
> src/slave/containerizer/mesos/isolators/network/port_mapping.cpp cd008aaebcd42554a9a81d2b059269546f59c966
> src/slave/flags.hpp e75c1b4227b443aedf445921b3f2108d930c112c
> src/slave/flags.cpp c84aa6724170bba46b4444be8410b71d42a1626e
> src/tests/containerizer/port_mapping_tests.cpp 16e015a8ac53a4aa5336b60c40228720b5f6910a
>
>
> Diff: https://reviews.apache.org/r/59294/diff/5/
>
>
> Testing
> -------
>
> # added a new test
> $ make check
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 59294: Optionally scale egress bandwidth with CPU.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59294/#review180577
-----------------------------------------------------------
Patch looks great!
Reviews applied: [60203, 59294]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
On June 30, 2017, 11 p.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59294/
> -----------------------------------------------------------
>
> (Updated June 30, 2017, 11 p.m.)
>
>
> Review request for mesos, Dmitry Zhuk, Ilya Pronin, and Jie Yu.
>
>
> Bugs: MESOS-7508
> https://issues.apache.org/jira/browse/MESOS-7508
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add support to isolators/port_mapping for optionally scaling egress bandwidth with CPU and with minimum and maximum limits.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto e2502acc3542d64c2b2b13f8c0363a3c8372e20e
> src/slave/containerizer/mesos/isolators/network/helper.cpp 4ed879dca42f85fc9dd7638e763822cf10fa8405
> src/slave/containerizer/mesos/isolators/network/port_mapping.hpp 9d38289c7161d5e931053b587d115684ccc44c94
> src/slave/containerizer/mesos/isolators/network/port_mapping.cpp cd008aaebcd42554a9a81d2b059269546f59c966
> src/slave/flags.hpp e75c1b4227b443aedf445921b3f2108d930c112c
> src/slave/flags.cpp c84aa6724170bba46b4444be8410b71d42a1626e
> src/tests/containerizer/port_mapping_tests.cpp 16e015a8ac53a4aa5336b60c40228720b5f6910a
>
>
> Diff: https://reviews.apache.org/r/59294/diff/5/
>
>
> Testing
> -------
>
> # added a new test
> $ make check
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 59294: Optionally scale egress bandwidth with CPU.
Posted by Ilya Pronin <ip...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59294/#review180436
-----------------------------------------------------------
Ship it!
Ship It!
- Ilya Pronin
On July 1, 2017, midnight, Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59294/
> -----------------------------------------------------------
>
> (Updated July 1, 2017, midnight)
>
>
> Review request for mesos, Dmitry Zhuk, Ilya Pronin, and Jie Yu.
>
>
> Bugs: MESOS-7508
> https://issues.apache.org/jira/browse/MESOS-7508
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add support to isolators/port_mapping for optionally scaling egress bandwidth with CPU and with minimum and maximum limits.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto e2502acc3542d64c2b2b13f8c0363a3c8372e20e
> src/slave/containerizer/mesos/isolators/network/helper.cpp 4ed879dca42f85fc9dd7638e763822cf10fa8405
> src/slave/containerizer/mesos/isolators/network/port_mapping.hpp 9d38289c7161d5e931053b587d115684ccc44c94
> src/slave/containerizer/mesos/isolators/network/port_mapping.cpp cd008aaebcd42554a9a81d2b059269546f59c966
> src/slave/flags.hpp e75c1b4227b443aedf445921b3f2108d930c112c
> src/slave/flags.cpp c84aa6724170bba46b4444be8410b71d42a1626e
> src/tests/containerizer/port_mapping_tests.cpp 16e015a8ac53a4aa5336b60c40228720b5f6910a
>
>
> Diff: https://reviews.apache.org/r/59294/diff/5/
>
>
> Testing
> -------
>
> # added a new test
> $ make check
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 59294: Optionally scale egress bandwidth with CPU.
Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59294/
-----------------------------------------------------------
(Updated June 30, 2017, 4 p.m.)
Review request for mesos, Dmitry Zhuk, Ilya Pronin, and Jie Yu.
Changes
-------
Addressed comments.
Bugs: MESOS-7508
https://issues.apache.org/jira/browse/MESOS-7508
Repository: mesos
Description
-------
Add support to isolators/port_mapping for optionally scaling egress bandwidth with CPU and with minimum and maximum limits.
Diffs (updated)
-----
include/mesos/mesos.proto e2502acc3542d64c2b2b13f8c0363a3c8372e20e
src/slave/containerizer/mesos/isolators/network/helper.cpp 4ed879dca42f85fc9dd7638e763822cf10fa8405
src/slave/containerizer/mesos/isolators/network/port_mapping.hpp 9d38289c7161d5e931053b587d115684ccc44c94
src/slave/containerizer/mesos/isolators/network/port_mapping.cpp cd008aaebcd42554a9a81d2b059269546f59c966
src/slave/flags.hpp e75c1b4227b443aedf445921b3f2108d930c112c
src/slave/flags.cpp c84aa6724170bba46b4444be8410b71d42a1626e
src/tests/containerizer/port_mapping_tests.cpp 16e015a8ac53a4aa5336b60c40228720b5f6910a
Diff: https://reviews.apache.org/r/59294/diff/5/
Changes: https://reviews.apache.org/r/59294/diff/4-5/
Testing
-------
# added a new test
$ make check
Thanks,
Ian Downes
Re: Review Request 59294: Optionally scale egress bandwidth with CPU.
Posted by Ian Downes <ia...@gmail.com>.
> On June 26, 2017, 6:08 a.m., Ilya Pronin wrote:
> > src/slave/containerizer/mesos/isolators/network/port_mapping.cpp
> > Lines 2209-2210 (original), 2301-2302 (patched)
> > <https://reviews.apache.org/r/59294/diff/2-4/?file=1731976#file1731976line2339>
> >
> > Do we expect the helper to hang? Is this a safeguard for the new code deployment?
Nope, it's not expected to hang but the 10 second timeout is indeed a safety check in case something is amiss. This method is only used during recovery and in tests.
> On June 26, 2017, 6:08 a.m., Ilya Pronin wrote:
> > src/slave/containerizer/mesos/isolators/network/port_mapping.cpp
> > Lines 4315-4317 (patched)
> > <https://reviews.apache.org/r/59294/diff/2-4/?file=1731976#file1731976line4376>
> >
> > If we set `ceil` low, it will prevent heavy CPU users' from bursting. Wouldn't it be more flexible to make `egress_ceil_limit` flag additive instead of absolute?
Additive burst makes it more complicated as you need to at least min(rate + ceil, max) or scale ceil with cpu as well. Instead, I think it's simpler to scale rate by cpu and burst naturally becomes progressively less useful for higher cpu.
- Ian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59294/#review178796
-----------------------------------------------------------
On June 21, 2017, 1:48 p.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59294/
> -----------------------------------------------------------
>
> (Updated June 21, 2017, 1:48 p.m.)
>
>
> Review request for mesos, Dmitry Zhuk, Ilya Pronin, and Jie Yu.
>
>
> Bugs: MESOS-7508
> https://issues.apache.org/jira/browse/MESOS-7508
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add support to isolators/port_mapping for optionally scaling egress bandwidth with CPU and with minimum and maximum limits.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto e2502acc3542d64c2b2b13f8c0363a3c8372e20e
> src/slave/containerizer/mesos/isolators/network/helper.cpp 4ed879dca42f85fc9dd7638e763822cf10fa8405
> src/slave/containerizer/mesos/isolators/network/port_mapping.hpp 9d38289c7161d5e931053b587d115684ccc44c94
> src/slave/containerizer/mesos/isolators/network/port_mapping.cpp cd008aaebcd42554a9a81d2b059269546f59c966
> src/slave/flags.hpp e75c1b4227b443aedf445921b3f2108d930c112c
> src/slave/flags.cpp c84aa6724170bba46b4444be8410b71d42a1626e
> src/tests/containerizer/port_mapping_tests.cpp 16e015a8ac53a4aa5336b60c40228720b5f6910a
>
>
> Diff: https://reviews.apache.org/r/59294/diff/4/
>
>
> Testing
> -------
>
> # added a new test
> $ make check
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 59294: Optionally scale egress bandwidth with CPU.
Posted by Ilya Pronin <ip...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59294/#review178796
-----------------------------------------------------------
src/slave/containerizer/mesos/isolators/network/port_mapping.cpp
Lines 423-427 (original), 424-428 (patched)
<https://reviews.apache.org/r/59294/#comment253023>
I think this flag's semantics needs additional explanation. If it is set, but contains an empty JSON, then an existing HTB class is removed.
src/slave/containerizer/mesos/isolators/network/port_mapping.cpp
Lines 576 (patched)
<https://reviews.apache.org/r/59294/#comment253002>
Style: For functions the opening brace must be on a separate line.
src/slave/containerizer/mesos/isolators/network/port_mapping.cpp
Lines 617 (patched)
<https://reviews.apache.org/r/59294/#comment253003>
Above there's a check that verifies that `rate.isSome()`.
src/slave/containerizer/mesos/isolators/network/port_mapping.cpp
Lines 604-606 (original), 642-644 (patched)
<https://reviews.apache.org/r/59294/#comment253005>
The comment is a bit misleading. We're removing the whole qdisc here, not just a class.
src/slave/containerizer/mesos/isolators/network/port_mapping.cpp
Lines 2209-2210 (original), 2301-2302 (patched)
<https://reviews.apache.org/r/59294/#comment253204>
Do we expect the helper to hang? Is this a safeguard for the new code deployment?
src/slave/containerizer/mesos/isolators/network/port_mapping.cpp
Line 2231 (original), 2318 (patched)
<https://reviews.apache.org/r/59294/#comment253219>
Spelling: s/stdout>/stdout/.
src/slave/containerizer/mesos/isolators/network/port_mapping.cpp
Lines 4315-4317 (patched)
<https://reviews.apache.org/r/59294/#comment253211>
If we set `ceil` low, it will prevent heavy CPU users' from bursting. Wouldn't it be more flexible to make `egress_ceil_limit` flag additive instead of absolute?
- Ilya Pronin
On June 21, 2017, 9:48 p.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59294/
> -----------------------------------------------------------
>
> (Updated June 21, 2017, 9:48 p.m.)
>
>
> Review request for mesos, Dmitry Zhuk, Ilya Pronin, and Jie Yu.
>
>
> Bugs: MESOS-7508
> https://issues.apache.org/jira/browse/MESOS-7508
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add support to isolators/port_mapping for optionally scaling egress bandwidth with CPU and with minimum and maximum limits.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto e2502acc3542d64c2b2b13f8c0363a3c8372e20e
> src/slave/containerizer/mesos/isolators/network/helper.cpp 4ed879dca42f85fc9dd7638e763822cf10fa8405
> src/slave/containerizer/mesos/isolators/network/port_mapping.hpp 9d38289c7161d5e931053b587d115684ccc44c94
> src/slave/containerizer/mesos/isolators/network/port_mapping.cpp cd008aaebcd42554a9a81d2b059269546f59c966
> src/slave/flags.hpp e75c1b4227b443aedf445921b3f2108d930c112c
> src/slave/flags.cpp c84aa6724170bba46b4444be8410b71d42a1626e
> src/tests/containerizer/port_mapping_tests.cpp 16e015a8ac53a4aa5336b60c40228720b5f6910a
>
>
> Diff: https://reviews.apache.org/r/59294/diff/4/
>
>
> Testing
> -------
>
> # added a new test
> $ make check
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 59294: Optionally scale egress bandwidth with CPU.
Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59294/
-----------------------------------------------------------
(Updated June 21, 2017, 1:48 p.m.)
Review request for mesos, Dmitry Zhuk, Ilya Pronin, and Jie Yu.
Changes
-------
Include egress rate limit, burst rate limit, and burst size in ResourceStatistics.
Bugs: MESOS-7508
https://issues.apache.org/jira/browse/MESOS-7508
Repository: mesos
Description
-------
Add support to isolators/port_mapping for optionally scaling egress bandwidth with CPU and with minimum and maximum limits.
Diffs (updated)
-----
include/mesos/mesos.proto e2502acc3542d64c2b2b13f8c0363a3c8372e20e
src/slave/containerizer/mesos/isolators/network/helper.cpp 4ed879dca42f85fc9dd7638e763822cf10fa8405
src/slave/containerizer/mesos/isolators/network/port_mapping.hpp 9d38289c7161d5e931053b587d115684ccc44c94
src/slave/containerizer/mesos/isolators/network/port_mapping.cpp cd008aaebcd42554a9a81d2b059269546f59c966
src/slave/flags.hpp e75c1b4227b443aedf445921b3f2108d930c112c
src/slave/flags.cpp c84aa6724170bba46b4444be8410b71d42a1626e
src/tests/containerizer/port_mapping_tests.cpp 16e015a8ac53a4aa5336b60c40228720b5f6910a
Diff: https://reviews.apache.org/r/59294/diff/4/
Changes: https://reviews.apache.org/r/59294/diff/3-4/
Testing
-------
# added a new test
$ make check
Thanks,
Ian Downes
Re: Review Request 59294: Optionally scale egress bandwidth with CPU.
Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59294/
-----------------------------------------------------------
(Updated June 21, 2017, 10:27 a.m.)
Review request for mesos, Dmitry Zhuk, Ilya Pronin, and Jie Yu.
Changes
-------
Use HTB class introduced in review 60203 to avoid shelling out. Included support for setting burst rate (ceil) and bucket size (cburst).
This is intentionally still in the model of an egress qdisc inside each container.
Bugs: MESOS-7508
https://issues.apache.org/jira/browse/MESOS-7508
Repository: mesos
Description
-------
Add support to isolators/port_mapping for optionally scaling egress bandwidth with CPU and with minimum and maximum limits.
Diffs (updated)
-----
src/slave/containerizer/mesos/isolators/network/helper.cpp 4ed879dca42f85fc9dd7638e763822cf10fa8405
src/slave/containerizer/mesos/isolators/network/port_mapping.hpp 9d38289c7161d5e931053b587d115684ccc44c94
src/slave/containerizer/mesos/isolators/network/port_mapping.cpp cd008aaebcd42554a9a81d2b059269546f59c966
src/slave/flags.hpp e75c1b4227b443aedf445921b3f2108d930c112c
src/slave/flags.cpp c84aa6724170bba46b4444be8410b71d42a1626e
src/tests/containerizer/port_mapping_tests.cpp 16e015a8ac53a4aa5336b60c40228720b5f6910a
Diff: https://reviews.apache.org/r/59294/diff/3/
Changes: https://reviews.apache.org/r/59294/diff/2-3/
Testing
-------
# added a new test
$ make check
Thanks,
Ian Downes
Re: Review Request 59294: Optionally scale egress bandwidth with CPU.
Posted by Ilya Pronin <ip...@twopensource.com>.
> On May 26, 2017, 9:45 p.m., Ilya Pronin wrote:
> > src/slave/containerizer/mesos/isolators/network/port_mapping.hpp
> > Lines 82-85 (patched)
> > <https://reviews.apache.org/r/59294/diff/2/?file=1731975#file1731975line82>
> >
> > Isn't 1Mb = 128KB?
>
> Santhosh Kumar Shanmugham wrote:
> This is correct. 1Mb = 1000Kb = 1000/8 KB = 125KB
When you're using SI units, yes. Stout `Bytes` and `Kilobytes` are using IEC (binary) units. If we want exactly 1 Mb (not Mib), we can use `Bytes(125000)` here.
- Ilya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59294/#review176219
-----------------------------------------------------------
On May 26, 2017, 7:23 p.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59294/
> -----------------------------------------------------------
>
> (Updated May 26, 2017, 7:23 p.m.)
>
>
> Review request for mesos, Dmitry Zhuk, Ilya Pronin, and Jie Yu.
>
>
> Bugs: MESOS-7508
> https://issues.apache.org/jira/browse/MESOS-7508
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add support to isolators/port_mapping for optionally scaling egress bandwidth with CPU and with minimum and maximum limits.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/network/port_mapping.hpp 9d38289c7161d5e931053b587d115684ccc44c94
> src/slave/containerizer/mesos/isolators/network/port_mapping.cpp cd008aaebcd42554a9a81d2b059269546f59c966
> src/slave/flags.hpp b66995630f89dfb95a6d0cf66efc5d7590e90cbc
> src/slave/flags.cpp 0c8276e425a6a7d22ee68edc6cc25b331635ec44
> src/tests/containerizer/port_mapping_tests.cpp d062f2f6bcf7b44dbcde951cdca23b0a2cd42115
>
>
> Diff: https://reviews.apache.org/r/59294/diff/2/
>
>
> Testing
> -------
>
> # added a new test
> $ make check
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 59294: Optionally scale egress bandwidth with CPU.
Posted by Santhosh Kumar Shanmugham <sa...@gmail.com>.
> On May 26, 2017, 1:45 p.m., Ilya Pronin wrote:
> > src/slave/containerizer/mesos/isolators/network/port_mapping.hpp
> > Lines 82-85 (patched)
> > <https://reviews.apache.org/r/59294/diff/2/?file=1731975#file1731975line82>
> >
> > Isn't 1Mb = 128KB?
This is correct. 1Mb = 1000Kb = 1000/8 KB = 125KB
- Santhosh Kumar
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59294/#review176219
-----------------------------------------------------------
On May 26, 2017, 11:23 a.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59294/
> -----------------------------------------------------------
>
> (Updated May 26, 2017, 11:23 a.m.)
>
>
> Review request for mesos, Dmitry Zhuk, Ilya Pronin, and Jie Yu.
>
>
> Bugs: MESOS-7508
> https://issues.apache.org/jira/browse/MESOS-7508
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add support to isolators/port_mapping for optionally scaling egress bandwidth with CPU and with minimum and maximum limits.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/network/port_mapping.hpp 9d38289c7161d5e931053b587d115684ccc44c94
> src/slave/containerizer/mesos/isolators/network/port_mapping.cpp cd008aaebcd42554a9a81d2b059269546f59c966
> src/slave/flags.hpp b66995630f89dfb95a6d0cf66efc5d7590e90cbc
> src/slave/flags.cpp 0c8276e425a6a7d22ee68edc6cc25b331635ec44
> src/tests/containerizer/port_mapping_tests.cpp d062f2f6bcf7b44dbcde951cdca23b0a2cd42115
>
>
> Diff: https://reviews.apache.org/r/59294/diff/2/
>
>
> Testing
> -------
>
> # added a new test
> $ make check
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 59294: Optionally scale egress bandwidth with CPU.
Posted by Ilya Pronin <ip...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59294/#review176219
-----------------------------------------------------------
Fix it, then Ship it!
src/slave/containerizer/mesos/isolators/network/port_mapping.hpp
Lines 82-85 (patched)
<https://reviews.apache.org/r/59294/#comment249560>
Isn't 1Mb = 128KB?
src/slave/containerizer/mesos/isolators/network/port_mapping.hpp
Lines 329 (patched)
<https://reviews.apache.org/r/59294/#comment249562>
What's the point of returning a `const Option`? Should be a `const` method :)
src/slave/containerizer/mesos/isolators/network/port_mapping.cpp
Lines 1720-1723 (patched)
<https://reviews.apache.org/r/59294/#comment249561>
Style: `return` statement is overindented.
src/slave/containerizer/mesos/isolators/network/port_mapping.cpp
Lines 1590-1591 (original), 1728-1729 (patched)
<https://reviews.apache.org/r/59294/#comment249566>
Or if `flags.maximum_egress_rate_limit.isSome()`.
src/slave/containerizer/mesos/isolators/network/port_mapping.cpp
Lines 4210-4213 (original), 4505-4507 (patched)
<https://reviews.apache.org/r/59294/#comment249564>
This empty line is needed.
- Ilya Pronin
On May 26, 2017, 7:23 p.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59294/
> -----------------------------------------------------------
>
> (Updated May 26, 2017, 7:23 p.m.)
>
>
> Review request for mesos, Dmitry Zhuk, Ilya Pronin, and Jie Yu.
>
>
> Bugs: MESOS-7508
> https://issues.apache.org/jira/browse/MESOS-7508
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add support to isolators/port_mapping for optionally scaling egress bandwidth with CPU and with minimum and maximum limits.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/network/port_mapping.hpp 9d38289c7161d5e931053b587d115684ccc44c94
> src/slave/containerizer/mesos/isolators/network/port_mapping.cpp cd008aaebcd42554a9a81d2b059269546f59c966
> src/slave/flags.hpp b66995630f89dfb95a6d0cf66efc5d7590e90cbc
> src/slave/flags.cpp 0c8276e425a6a7d22ee68edc6cc25b331635ec44
> src/tests/containerizer/port_mapping_tests.cpp d062f2f6bcf7b44dbcde951cdca23b0a2cd42115
>
>
> Diff: https://reviews.apache.org/r/59294/diff/2/
>
>
> Testing
> -------
>
> # added a new test
> $ make check
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 59294: Optionally scale egress bandwidth with CPU.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59294/#review176284
-----------------------------------------------------------
Patch looks great!
Reviews applied: [59294]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
On May 26, 2017, 6:23 p.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59294/
> -----------------------------------------------------------
>
> (Updated May 26, 2017, 6:23 p.m.)
>
>
> Review request for mesos, Dmitry Zhuk, Ilya Pronin, and Jie Yu.
>
>
> Bugs: MESOS-7508
> https://issues.apache.org/jira/browse/MESOS-7508
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add support to isolators/port_mapping for optionally scaling egress bandwidth with CPU and with minimum and maximum limits.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/network/port_mapping.hpp 9d38289c7161d5e931053b587d115684ccc44c94
> src/slave/containerizer/mesos/isolators/network/port_mapping.cpp cd008aaebcd42554a9a81d2b059269546f59c966
> src/slave/flags.hpp b66995630f89dfb95a6d0cf66efc5d7590e90cbc
> src/slave/flags.cpp 0c8276e425a6a7d22ee68edc6cc25b331635ec44
> src/tests/containerizer/port_mapping_tests.cpp d062f2f6bcf7b44dbcde951cdca23b0a2cd42115
>
>
> Diff: https://reviews.apache.org/r/59294/diff/2/
>
>
> Testing
> -------
>
> # added a new test
> $ make check
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 59294: Optionally scale egress bandwidth with CPU.
Posted by Ilya Pronin <ip...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59294/#review177027
-----------------------------------------------------------
src/slave/containerizer/mesos/isolators/network/port_mapping.cpp
Lines 2235-2239 (patched)
<https://reviews.apache.org/r/59294/#comment250604>
`Bytes::parse()` handles units as binary units, but AFAIK `tc` is showing rate with SI units. At least without `-iec` option. Won't we think that egress rate is higher than it actually is? Am I missing something here?
- Ilya Pronin
On May 26, 2017, 7:23 p.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59294/
> -----------------------------------------------------------
>
> (Updated May 26, 2017, 7:23 p.m.)
>
>
> Review request for mesos, Dmitry Zhuk, Ilya Pronin, and Jie Yu.
>
>
> Bugs: MESOS-7508
> https://issues.apache.org/jira/browse/MESOS-7508
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add support to isolators/port_mapping for optionally scaling egress bandwidth with CPU and with minimum and maximum limits.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/network/port_mapping.hpp 9d38289c7161d5e931053b587d115684ccc44c94
> src/slave/containerizer/mesos/isolators/network/port_mapping.cpp cd008aaebcd42554a9a81d2b059269546f59c966
> src/slave/flags.hpp b66995630f89dfb95a6d0cf66efc5d7590e90cbc
> src/slave/flags.cpp 0c8276e425a6a7d22ee68edc6cc25b331635ec44
> src/tests/containerizer/port_mapping_tests.cpp d062f2f6bcf7b44dbcde951cdca23b0a2cd42115
>
>
> Diff: https://reviews.apache.org/r/59294/diff/2/
>
>
> Testing
> -------
>
> # added a new test
> $ make check
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 59294: Optionally scale egress bandwidth with CPU.
Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59294/
-----------------------------------------------------------
(Updated May 26, 2017, 11:23 a.m.)
Review request for mesos, Dmitry Zhuk, Ilya Pronin, and Jie Yu.
Bugs: MESOS-7508
https://issues.apache.org/jira/browse/MESOS-7508
Repository: mesos
Description
-------
Add support to isolators/port_mapping for optionally scaling egress bandwidth with CPU and with minimum and maximum limits.
Diffs
-----
src/slave/containerizer/mesos/isolators/network/port_mapping.hpp 9d38289c7161d5e931053b587d115684ccc44c94
src/slave/containerizer/mesos/isolators/network/port_mapping.cpp cd008aaebcd42554a9a81d2b059269546f59c966
src/slave/flags.hpp b66995630f89dfb95a6d0cf66efc5d7590e90cbc
src/slave/flags.cpp 0c8276e425a6a7d22ee68edc6cc25b331635ec44
src/tests/containerizer/port_mapping_tests.cpp d062f2f6bcf7b44dbcde951cdca23b0a2cd42115
Diff: https://reviews.apache.org/r/59294/diff/2/
Testing
-------
# added a new test
$ make check
Thanks,
Ian Downes
Re: Review Request 59294: Optionally scale egress bandwidth with CPU.
Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59294/
-----------------------------------------------------------
(Updated May 26, 2017, 11:22 a.m.)
Review request for .
Bugs: MESOS-7508
https://issues.apache.org/jira/browse/MESOS-7508
Repository: mesos
Description
-------
Add support to isolators/port_mapping for optionally scaling egress bandwidth with CPU and with minimum and maximum limits.
Diffs (updated)
-----
src/slave/containerizer/mesos/isolators/network/port_mapping.hpp 9d38289c7161d5e931053b587d115684ccc44c94
src/slave/containerizer/mesos/isolators/network/port_mapping.cpp cd008aaebcd42554a9a81d2b059269546f59c966
src/slave/flags.hpp b66995630f89dfb95a6d0cf66efc5d7590e90cbc
src/slave/flags.cpp 0c8276e425a6a7d22ee68edc6cc25b331635ec44
src/tests/containerizer/port_mapping_tests.cpp d062f2f6bcf7b44dbcde951cdca23b0a2cd42115
Diff: https://reviews.apache.org/r/59294/diff/2/
Changes: https://reviews.apache.org/r/59294/diff/1-2/
Testing
-------
# added a new test
$ make check
Thanks,
Ian Downes
Re: Review Request 59294: Optionally scale egress bandwidth with CPU.
Posted by Ian Downes <ia...@gmail.com>.
> On May 17, 2017, 2:14 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/network/port_mapping.cpp
> > Lines 586-588 (patched)
> > <https://reviews.apache.org/r/59294/diff/1/?file=1719988#file1719988line586>
> >
> > Instead of shelling out, i'd say we just introduce support in the nl library. IN fact, we already have a patch chain starts here to support that
> > https://reviews.apache.org/r/45605/
Definitely agree that's a better approach but I'd like to get this change in first. I do plan to revive those reviews so that shaping can be moved out to the host side to enable token sharing but that's a much larger code change and also a much harder deploy: this patch will modify running containers in place with the new policy so is easy to deploy.
> On May 17, 2017, 2:14 p.m., Jie Yu wrote:
> > src/slave/flags.cpp
> > Lines 770-786 (patched)
> > <https://reviews.apache.org/r/59294/diff/1/?file=1719990#file1719990line770>
> >
> > This sounds like a heuristic. Any justification why this heuristic? Wondering if label based solution is better? For instance, the isolator will look for a special label of the task/executor. The label specifies the egress rate limit which can override the default rate limit. Something along this line?
> >
> > Then, the custom logic can be injected into a label decrorator, rather than first class it here?
It's not really a heuristic, it's a simple linear model with min/max. The major benefit is that it enables more effective allocation of a host's egress bandwidth without exposing bandwidth as a resource. A fixed egress bandwidth allocates poorly for either a small number of very large containers (underutilizing) or a large number of small containers (overcommitting). Scaling with CPU means a large container can get a larger share of the bandwidth.
I thought about a label based solution but this doesn't work well with a heterogenous cluster. We have a mix of 1G and 10G hosts and we'd like to use different egress_rate_per_cpu depending on the link speed, e.g., 40 Mbps / core for 1G and 120 Mbps / core for 10 G. The scheduler doesn't (and shouldn't) know the specifics of hosts beyond resources so unless we make bandwidth a first class resource I think the logic should be at the isolator. Host bandwidth could be exposed via an agent attribute but that's *really* breaking the resource abstraction.
- Ian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59294/#review175114
-----------------------------------------------------------
On May 15, 2017, 1:56 p.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59294/
> -----------------------------------------------------------
>
> (Updated May 15, 2017, 1:56 p.m.)
>
>
> Review request for mesos, Dmitry Zhuk, Ilya Pronin, Jie Yu, and Santhosh Kumar Shanmugham.
>
>
> Bugs: MESOS-7508
> https://issues.apache.org/jira/browse/MESOS-7508
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add support to isolators/port_mapping for optionally scaling egress bandwidth with CPU and with minimum and maximum limits.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/network/port_mapping.hpp 9d38289c7161d5e931053b587d115684ccc44c94
> src/slave/containerizer/mesos/isolators/network/port_mapping.cpp cd008aaebcd42554a9a81d2b059269546f59c966
> src/slave/flags.hpp e5784ef81ad0720c7ec061ee0b28b8fadae77afd
> src/slave/flags.cpp bc63a6a4cb6115b4b4d592e67e34045f52b50d4c
> src/tests/containerizer/port_mapping_tests.cpp a528382e8b4831b9c7e8dcc877a5e242909f0cd5
>
>
> Diff: https://reviews.apache.org/r/59294/diff/1/
>
>
> Testing
> -------
>
> # added a new test
> $ make check
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 59294: Optionally scale egress bandwidth with CPU.
Posted by Ian Downes <ia...@gmail.com>.
> On May 17, 2017, 2:14 p.m., Jie Yu wrote:
> > src/slave/flags.cpp
> > Lines 770-786 (patched)
> > <https://reviews.apache.org/r/59294/diff/1/?file=1719990#file1719990line770>
> >
> > This sounds like a heuristic. Any justification why this heuristic? Wondering if label based solution is better? For instance, the isolator will look for a special label of the task/executor. The label specifies the egress rate limit which can override the default rate limit. Something along this line?
> >
> > Then, the custom logic can be injected into a label decrorator, rather than first class it here?
>
> Ian Downes wrote:
> It's not really a heuristic, it's a simple linear model with min/max. The major benefit is that it enables more effective allocation of a host's egress bandwidth without exposing bandwidth as a resource. A fixed egress bandwidth allocates poorly for either a small number of very large containers (underutilizing) or a large number of small containers (overcommitting). Scaling with CPU means a large container can get a larger share of the bandwidth.
>
> I thought about a label based solution but this doesn't work well with a heterogenous cluster. We have a mix of 1G and 10G hosts and we'd like to use different egress_rate_per_cpu depending on the link speed, e.g., 40 Mbps / core for 1G and 120 Mbps / core for 10 G. The scheduler doesn't (and shouldn't) know the specifics of hosts beyond resources so unless we make bandwidth a first class resource I think the logic should be at the isolator. Host bandwidth could be exposed via an agent attribute but that's *really* breaking the resource abstraction.
>
> Santhosh Kumar Shanmugham wrote:
> To add to the point - scaling network bandwidth with CPU is typical in the cloud and we are just mirroring the same feature here.
>
> ` Each core is subject to a 2 Gbits/second (Gbps) cap for peak performance. Each additional core increases the network cap, up to a theoretical maximum of 16 Gbps for each virtual machine; however, the actual performance you experience can vary depending on your workload.`
>
> https://cloud.google.com/compute/docs/networks-and-firewalls
>
> Jie Yu wrote:
> > The scheduler doesn't (and shouldn't) know the specifics of hosts beyond resources so unless we make bandwidth a first class resource I think the logic should be at the isolator
>
> Scheduler is not the one that sets the label. The label decrocator on the agent will be the one doing that. Since the label decroator lives in the agent, it can automatically detect the link speed and set the label properly.
The label decorator could work but the slaveRunTaskLabelDecorator is only on task start. One of the key benefits of the posted code is that it will update existing containers when this policy is introduced and then in the future if config is changed, all without restarting containers. We're going to deploy this code to large running clusters, incrementally increasing the config values. Rolling the clusters at each increment is not feasible and we don't want out-of-band tooling separately re-configuring.
- Ian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59294/#review175114
-----------------------------------------------------------
On May 26, 2017, 11:23 a.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59294/
> -----------------------------------------------------------
>
> (Updated May 26, 2017, 11:23 a.m.)
>
>
> Review request for mesos, Dmitry Zhuk, Ilya Pronin, and Jie Yu.
>
>
> Bugs: MESOS-7508
> https://issues.apache.org/jira/browse/MESOS-7508
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add support to isolators/port_mapping for optionally scaling egress bandwidth with CPU and with minimum and maximum limits.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/network/port_mapping.hpp 9d38289c7161d5e931053b587d115684ccc44c94
> src/slave/containerizer/mesos/isolators/network/port_mapping.cpp cd008aaebcd42554a9a81d2b059269546f59c966
> src/slave/flags.hpp b66995630f89dfb95a6d0cf66efc5d7590e90cbc
> src/slave/flags.cpp 0c8276e425a6a7d22ee68edc6cc25b331635ec44
> src/tests/containerizer/port_mapping_tests.cpp d062f2f6bcf7b44dbcde951cdca23b0a2cd42115
>
>
> Diff: https://reviews.apache.org/r/59294/diff/2/
>
>
> Testing
> -------
>
> # added a new test
> $ make check
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 59294: Optionally scale egress bandwidth with CPU.
Posted by Santhosh Kumar Shanmugham <sa...@gmail.com>.
> On May 17, 2017, 2:14 p.m., Jie Yu wrote:
> > src/slave/flags.cpp
> > Lines 770-786 (patched)
> > <https://reviews.apache.org/r/59294/diff/1/?file=1719990#file1719990line770>
> >
> > This sounds like a heuristic. Any justification why this heuristic? Wondering if label based solution is better? For instance, the isolator will look for a special label of the task/executor. The label specifies the egress rate limit which can override the default rate limit. Something along this line?
> >
> > Then, the custom logic can be injected into a label decrorator, rather than first class it here?
>
> Ian Downes wrote:
> It's not really a heuristic, it's a simple linear model with min/max. The major benefit is that it enables more effective allocation of a host's egress bandwidth without exposing bandwidth as a resource. A fixed egress bandwidth allocates poorly for either a small number of very large containers (underutilizing) or a large number of small containers (overcommitting). Scaling with CPU means a large container can get a larger share of the bandwidth.
>
> I thought about a label based solution but this doesn't work well with a heterogenous cluster. We have a mix of 1G and 10G hosts and we'd like to use different egress_rate_per_cpu depending on the link speed, e.g., 40 Mbps / core for 1G and 120 Mbps / core for 10 G. The scheduler doesn't (and shouldn't) know the specifics of hosts beyond resources so unless we make bandwidth a first class resource I think the logic should be at the isolator. Host bandwidth could be exposed via an agent attribute but that's *really* breaking the resource abstraction.
To add to the point - scaling network bandwidth with CPU is typical in the cloud and we are just mirroring the same feature here.
` Each core is subject to a 2 Gbits/second (Gbps) cap for peak performance. Each additional core increases the network cap, up to a theoretical maximum of 16 Gbps for each virtual machine; however, the actual performance you experience can vary depending on your workload.`
https://cloud.google.com/compute/docs/networks-and-firewalls
- Santhosh Kumar
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59294/#review175114
-----------------------------------------------------------
On May 26, 2017, 11:23 a.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59294/
> -----------------------------------------------------------
>
> (Updated May 26, 2017, 11:23 a.m.)
>
>
> Review request for mesos, Dmitry Zhuk, Ilya Pronin, and Jie Yu.
>
>
> Bugs: MESOS-7508
> https://issues.apache.org/jira/browse/MESOS-7508
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add support to isolators/port_mapping for optionally scaling egress bandwidth with CPU and with minimum and maximum limits.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/network/port_mapping.hpp 9d38289c7161d5e931053b587d115684ccc44c94
> src/slave/containerizer/mesos/isolators/network/port_mapping.cpp cd008aaebcd42554a9a81d2b059269546f59c966
> src/slave/flags.hpp b66995630f89dfb95a6d0cf66efc5d7590e90cbc
> src/slave/flags.cpp 0c8276e425a6a7d22ee68edc6cc25b331635ec44
> src/tests/containerizer/port_mapping_tests.cpp d062f2f6bcf7b44dbcde951cdca23b0a2cd42115
>
>
> Diff: https://reviews.apache.org/r/59294/diff/2/
>
>
> Testing
> -------
>
> # added a new test
> $ make check
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 59294: Optionally scale egress bandwidth with CPU.
Posted by Jie Yu <yu...@gmail.com>.
> On May 17, 2017, 9:14 p.m., Jie Yu wrote:
> > src/slave/flags.cpp
> > Lines 770-786 (patched)
> > <https://reviews.apache.org/r/59294/diff/1/?file=1719990#file1719990line770>
> >
> > This sounds like a heuristic. Any justification why this heuristic? Wondering if label based solution is better? For instance, the isolator will look for a special label of the task/executor. The label specifies the egress rate limit which can override the default rate limit. Something along this line?
> >
> > Then, the custom logic can be injected into a label decrorator, rather than first class it here?
>
> Ian Downes wrote:
> It's not really a heuristic, it's a simple linear model with min/max. The major benefit is that it enables more effective allocation of a host's egress bandwidth without exposing bandwidth as a resource. A fixed egress bandwidth allocates poorly for either a small number of very large containers (underutilizing) or a large number of small containers (overcommitting). Scaling with CPU means a large container can get a larger share of the bandwidth.
>
> I thought about a label based solution but this doesn't work well with a heterogenous cluster. We have a mix of 1G and 10G hosts and we'd like to use different egress_rate_per_cpu depending on the link speed, e.g., 40 Mbps / core for 1G and 120 Mbps / core for 10 G. The scheduler doesn't (and shouldn't) know the specifics of hosts beyond resources so unless we make bandwidth a first class resource I think the logic should be at the isolator. Host bandwidth could be exposed via an agent attribute but that's *really* breaking the resource abstraction.
>
> Santhosh Kumar Shanmugham wrote:
> To add to the point - scaling network bandwidth with CPU is typical in the cloud and we are just mirroring the same feature here.
>
> ` Each core is subject to a 2 Gbits/second (Gbps) cap for peak performance. Each additional core increases the network cap, up to a theoretical maximum of 16 Gbps for each virtual machine; however, the actual performance you experience can vary depending on your workload.`
>
> https://cloud.google.com/compute/docs/networks-and-firewalls
> The scheduler doesn't (and shouldn't) know the specifics of hosts beyond resources so unless we make bandwidth a first class resource I think the logic should be at the isolator
Scheduler is not the one that sets the label. The label decrocator on the agent will be the one doing that. Since the label decroator lives in the agent, it can automatically detect the link speed and set the label properly.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59294/#review175114
-----------------------------------------------------------
On May 26, 2017, 6:23 p.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59294/
> -----------------------------------------------------------
>
> (Updated May 26, 2017, 6:23 p.m.)
>
>
> Review request for mesos, Dmitry Zhuk, Ilya Pronin, and Jie Yu.
>
>
> Bugs: MESOS-7508
> https://issues.apache.org/jira/browse/MESOS-7508
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add support to isolators/port_mapping for optionally scaling egress bandwidth with CPU and with minimum and maximum limits.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/network/port_mapping.hpp 9d38289c7161d5e931053b587d115684ccc44c94
> src/slave/containerizer/mesos/isolators/network/port_mapping.cpp cd008aaebcd42554a9a81d2b059269546f59c966
> src/slave/flags.hpp b66995630f89dfb95a6d0cf66efc5d7590e90cbc
> src/slave/flags.cpp 0c8276e425a6a7d22ee68edc6cc25b331635ec44
> src/tests/containerizer/port_mapping_tests.cpp d062f2f6bcf7b44dbcde951cdca23b0a2cd42115
>
>
> Diff: https://reviews.apache.org/r/59294/diff/2/
>
>
> Testing
> -------
>
> # added a new test
> $ make check
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 59294: Optionally scale egress bandwidth with CPU.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59294/#review175114
-----------------------------------------------------------
src/slave/containerizer/mesos/isolators/network/port_mapping.cpp
Lines 586-588 (patched)
<https://reviews.apache.org/r/59294/#comment248474>
Instead of shelling out, i'd say we just introduce support in the nl library. IN fact, we already have a patch chain starts here to support that
https://reviews.apache.org/r/45605/
src/slave/flags.cpp
Lines 770-786 (patched)
<https://reviews.apache.org/r/59294/#comment248771>
This sounds like a heuristic. Any justification why this heuristic? Wondering if label based solution is better? For instance, the isolator will look for a special label of the task/executor. The label specifies the egress rate limit which can override the default rate limit. Something along this line?
Then, the custom logic can be injected into a label decrorator, rather than first class it here?
- Jie Yu
On May 15, 2017, 8:56 p.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59294/
> -----------------------------------------------------------
>
> (Updated May 15, 2017, 8:56 p.m.)
>
>
> Review request for mesos, Dmitry Zhuk, Ilya Pronin, Jie Yu, and Santhosh Kumar Shanmugham.
>
>
> Bugs: MESOS-7508
> https://issues.apache.org/jira/browse/MESOS-7508
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add support to isolators/port_mapping for optionally scaling egress bandwidth with CPU and with minimum and maximum limits.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/network/port_mapping.hpp 9d38289c7161d5e931053b587d115684ccc44c94
> src/slave/containerizer/mesos/isolators/network/port_mapping.cpp cd008aaebcd42554a9a81d2b059269546f59c966
> src/slave/flags.hpp e5784ef81ad0720c7ec061ee0b28b8fadae77afd
> src/slave/flags.cpp bc63a6a4cb6115b4b4d592e67e34045f52b50d4c
> src/tests/containerizer/port_mapping_tests.cpp a528382e8b4831b9c7e8dcc877a5e242909f0cd5
>
>
> Diff: https://reviews.apache.org/r/59294/diff/1/
>
>
> Testing
> -------
>
> # added a new test
> $ make check
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 59294: Optionally scale egress bandwidth with CPU.
Posted by Ilya Pronin <ip...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59294/#review175077
-----------------------------------------------------------
Looks good overall. Just a few comments.
src/slave/containerizer/mesos/isolators/network/port_mapping.hpp
Lines 323 (patched)
<https://reviews.apache.org/r/59294/#comment248423>
Make it a `const` method?
src/slave/containerizer/mesos/isolators/network/port_mapping.cpp
Lines 696-697 (patched)
<https://reviews.apache.org/r/59294/#comment248426>
Maybe add a note here that the logic below is guarded by this check, so someone won't accidently "simplify" it?
src/slave/containerizer/mesos/isolators/network/port_mapping.cpp
Line 1589 (original), 1715-1716 (patched)
<https://reviews.apache.org/r/59294/#comment248427>
Should we also add a check that `minimum_egress_rate_limit <= maximum_egress_rate_limit`? If that's not true egress rate limit will always be set to `maximum_egress_rate_limit`, even if originally it was smaller.
What if `speed < maximum_egress_rate_limit`? Should we provide a warning?
src/slave/containerizer/mesos/isolators/network/port_mapping.cpp
Lines 2176 (patched)
<https://reviews.apache.org/r/59294/#comment248429>
`strings::remove(handle, "0", strings::SUFFIX)`? We use "1:0" handle, but just in case.
src/slave/containerizer/mesos/isolators/network/port_mapping.cpp
Lines 2679-2681 (patched)
<https://reviews.apache.org/r/59294/#comment248438>
Should we also log this when no ephemeral ports were found for the container?
src/slave/containerizer/mesos/isolators/network/port_mapping.cpp
Lines 2680-2681 (patched)
<https://reviews.apache.org/r/59294/#comment248439>
`Try::operator->` can be used instead of `Try::get()`.
src/slave/containerizer/mesos/isolators/network/port_mapping.cpp
Lines 4177-4178 (patched)
<https://reviews.apache.org/r/59294/#comment248443>
What if a container uses `cpus:0.5` and `minimum_egress_rate_limit` is unset? Shouldn't `limit` have some minimum resonable value instead of 0?
src/slave/containerizer/mesos/isolators/network/port_mapping.cpp
Lines 4184 (patched)
<https://reviews.apache.org/r/59294/#comment248446>
There's no need to use `Bytes()` constructor here. The result of `max()` here is already `Bytes`.
src/slave/containerizer/mesos/isolators/network/port_mapping.cpp
Lines 4188 (patched)
<https://reviews.apache.org/r/59294/#comment248447>
Ditto.
- Ilya Pronin
On May 15, 2017, 9:56 p.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59294/
> -----------------------------------------------------------
>
> (Updated May 15, 2017, 9:56 p.m.)
>
>
> Review request for mesos, Dmitry Zhuk, Ilya Pronin, Jie Yu, and Santhosh Kumar Shanmugham.
>
>
> Bugs: MESOS-7508
> https://issues.apache.org/jira/browse/MESOS-7508
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add support to isolators/port_mapping for optionally scaling egress bandwidth with CPU and with minimum and maximum limits.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/network/port_mapping.hpp 9d38289c7161d5e931053b587d115684ccc44c94
> src/slave/containerizer/mesos/isolators/network/port_mapping.cpp cd008aaebcd42554a9a81d2b059269546f59c966
> src/slave/flags.hpp e5784ef81ad0720c7ec061ee0b28b8fadae77afd
> src/slave/flags.cpp bc63a6a4cb6115b4b4d592e67e34045f52b50d4c
> src/tests/containerizer/port_mapping_tests.cpp a528382e8b4831b9c7e8dcc877a5e242909f0cd5
>
>
> Diff: https://reviews.apache.org/r/59294/diff/1/
>
>
> Testing
> -------
>
> # added a new test
> $ make check
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 59294: Optionally scale egress bandwidth with CPU.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59294/#review175071
-----------------------------------------------------------
Patch looks great!
Reviews applied: [59294]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
On May 15, 2017, 8:56 p.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59294/
> -----------------------------------------------------------
>
> (Updated May 15, 2017, 8:56 p.m.)
>
>
> Review request for mesos, Dmitry Zhuk, Ilya Pronin, Jie Yu, and Santhosh Kumar Shanmugham.
>
>
> Bugs: MESOS-7508
> https://issues.apache.org/jira/browse/MESOS-7508
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add support to isolators/port_mapping for optionally scaling egress bandwidth with CPU and with minimum and maximum limits.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/network/port_mapping.hpp 9d38289c7161d5e931053b587d115684ccc44c94
> src/slave/containerizer/mesos/isolators/network/port_mapping.cpp cd008aaebcd42554a9a81d2b059269546f59c966
> src/slave/flags.hpp e5784ef81ad0720c7ec061ee0b28b8fadae77afd
> src/slave/flags.cpp bc63a6a4cb6115b4b4d592e67e34045f52b50d4c
> src/tests/containerizer/port_mapping_tests.cpp a528382e8b4831b9c7e8dcc877a5e242909f0cd5
>
>
> Diff: https://reviews.apache.org/r/59294/diff/1/
>
>
> Testing
> -------
>
> # added a new test
> $ make check
>
>
> Thanks,
>
> Ian Downes
>
>