Re: Review Request 34426: Report per-container metrics for network bandwidth throttling
On June 3, 2015, 10:35 p.m., Jie Yu wrote:
src/linux/routing/queueing/internal.hpp, line 320
https://reviews.apache.org/r/34426/diff/11/?file=977689#file977689line320
Why =?
Paul Brett wrote:
The definition for RTNL_TC_STATS_MAX comes from libnl and looks like this:
enum rtnl_tc_stat {
RTNL_TC_PACKETS, /** Number of packets seen */
RTNL_TC_BYTES,/** Total bytes seen */
RTNL_TC_RATE_BPS, /** Current bits/s (rate estimator) */
RTNL_TC_RATE_PPS, /** Current packet/s (rate estimator) */
RTNL_TC_QLEN, /** Current queue length */
RTNL_TC_BACKLOG, /** Current backlog length */
RTNL_TC_DROPS,/** Total number of packets dropped */
RTNL_TC_REQUEUES, /** Total number of requeues */
RTNL_TC_OVERLIMITS, /** Total number of overlimits */
__RTNL_TC_STATS_MAX,
};
#define RTNL_TC_STATS_MAX (__RTNL_TC_STATS_MAX - 1)
So the range is from 0 to RTNL_TC_STATS_MAX inclusive.
Cong Wang wrote:
This looks like a libnl bug for me, however it is too late to fix it...
OK, I added a NOTE for you because this is very unintuitive.
On June 3, 2015, 10:35 p.m., Jie Yu wrote:
src/linux/routing/queueing/statistics.hpp, line 23
https://reviews.apache.org/r/34426/diff/11/?file=977690#file977690line23
Should we get rid of 'queueing' here because this can be used by
filters as well?
Paul Brett wrote:
It should work on filters but we have no use case and no testing to know
if it works. Lets leave it here until that changes.
SGTM.
- Jie
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34426/#review86493
---
On June 4, 2015, 1:19 a.m., Paul Brett wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34426/
---
(Updated June 4, 2015, 1:19 a.m.)
Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang.
Bugs: MESOS-2665
https://issues.apache.org/jira/browse/MESOS-2665
Repository: mesos
Description
---
Report the network statistics from libnl
Diffs
-
src/Makefile.am 66030c4b211ea61e97e62c35ec1821e0958f9787
src/linux/routing/queueing/fq_codel.hpp
49c8df626addcfc841132f821b8697b5e6e3b341
src/linux/routing/queueing/fq_codel.cpp
976c6a7a11d39609e6597daeb1a74c93ac62e4bf
src/linux/routing/queueing/ingress.hpp
2b7e1d3d1c02120195b6697320fb46f085a1ef92
src/linux/routing/queueing/ingress.cpp
e96f547200dc4ad5b2e33fd1ffbd7fb92b955a46
src/linux/routing/queueing/internal.hpp
3713f6a4d3f9a44b142b8150e85cf911e53e34e9
src/linux/routing/queueing/statistics.hpp PRE-CREATION
Diff: https://reviews.apache.org/r/34426/diff/
Testing
---
make check
Thanks,
Paul Brett
Re: Review Request 34426: Report per-container metrics for network bandwidth throttling
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34426/ --- (Updated June 3, 2015, 7:36 p.m.) Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang. Changes --- Rebase and update to match new discipline interface. Bugs: MESOS-2665 https://issues.apache.org/jira/browse/MESOS-2665 Repository: mesos Description --- Report the network statistics from libnl Diffs (updated) - src/Makefile.am f045b89e8590c4775b21980bedff14cc27d31bfb src/linux/routing/queueing/fq_codel.hpp 49c8df626addcfc841132f821b8697b5e6e3b341 src/linux/routing/queueing/fq_codel.cpp 976c6a7a11d39609e6597daeb1a74c93ac62e4bf src/linux/routing/queueing/ingress.hpp 2b7e1d3d1c02120195b6697320fb46f085a1ef92 src/linux/routing/queueing/ingress.cpp e96f547200dc4ad5b2e33fd1ffbd7fb92b955a46 src/linux/routing/queueing/internal.hpp 3713f6a4d3f9a44b142b8150e85cf911e53e34e9 src/linux/routing/queueing/statistics.hpp PRE-CREATION Diff: https://reviews.apache.org/r/34426/diff/ Testing --- make check Thanks, Paul Brett
Re: Review Request 34426: Report per-container metrics for network bandwidth throttling
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34426/#review86493
---
Ship it!
src/linux/routing/queueing/internal.hpp
https://reviews.apache.org/r/34426/#comment138537
I would probably just use 'size_t' here:
```
for (size_t i = 0; i (size_t) RTNL_TC_STATS_MAX; i++) {
...
}
```
src/linux/routing/queueing/internal.hpp
https://reviews.apache.org/r/34426/#comment138539
Why =?
src/linux/routing/queueing/statistics.hpp
https://reviews.apache.org/r/34426/#comment138540
Should we get rid of 'queueing' here because this can be used by filters as
well?
- Jie Yu
On June 3, 2015, 9:19 p.m., Paul Brett wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34426/
---
(Updated June 3, 2015, 9:19 p.m.)
Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang.
Bugs: MESOS-2665
https://issues.apache.org/jira/browse/MESOS-2665
Repository: mesos
Description
---
Report the network statistics from libnl
Diffs
-
src/Makefile.am 66030c4b211ea61e97e62c35ec1821e0958f9787
src/linux/routing/queueing/fq_codel.hpp
49c8df626addcfc841132f821b8697b5e6e3b341
src/linux/routing/queueing/fq_codel.cpp
976c6a7a11d39609e6597daeb1a74c93ac62e4bf
src/linux/routing/queueing/ingress.hpp
2b7e1d3d1c02120195b6697320fb46f085a1ef92
src/linux/routing/queueing/ingress.cpp
e96f547200dc4ad5b2e33fd1ffbd7fb92b955a46
src/linux/routing/queueing/internal.hpp
3713f6a4d3f9a44b142b8150e85cf911e53e34e9
src/linux/routing/queueing/statistics.hpp PRE-CREATION
Diff: https://reviews.apache.org/r/34426/diff/
Testing
---
make check
Thanks,
Paul Brett
Re: Review Request 34426: Report per-container metrics for network bandwidth throttling
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34426/ --- (Updated June 4, 2015, 1:19 a.m.) Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang. Changes --- Incorporated review feedback. Bugs: MESOS-2665 https://issues.apache.org/jira/browse/MESOS-2665 Repository: mesos Description --- Report the network statistics from libnl Diffs (updated) - src/Makefile.am 66030c4b211ea61e97e62c35ec1821e0958f9787 src/linux/routing/queueing/fq_codel.hpp 49c8df626addcfc841132f821b8697b5e6e3b341 src/linux/routing/queueing/fq_codel.cpp 976c6a7a11d39609e6597daeb1a74c93ac62e4bf src/linux/routing/queueing/ingress.hpp 2b7e1d3d1c02120195b6697320fb46f085a1ef92 src/linux/routing/queueing/ingress.cpp e96f547200dc4ad5b2e33fd1ffbd7fb92b955a46 src/linux/routing/queueing/internal.hpp 3713f6a4d3f9a44b142b8150e85cf911e53e34e9 src/linux/routing/queueing/statistics.hpp PRE-CREATION Diff: https://reviews.apache.org/r/34426/diff/ Testing --- make check Thanks, Paul Brett
Re: Review Request 34426: Report per-container metrics for network bandwidth throttling
On June 3, 2015, 10:35 p.m., Jie Yu wrote:
src/linux/routing/queueing/internal.hpp, line 320
https://reviews.apache.org/r/34426/diff/11/?file=977689#file977689line320
Why =?
The definition for RTNL_TC_STATS_MAX comes from libnl and looks like this:
enum rtnl_tc_stat {
RTNL_TC_PACKETS, /** Number of packets seen */
RTNL_TC_BYTES,/** Total bytes seen */
RTNL_TC_RATE_BPS, /** Current bits/s (rate estimator) */
RTNL_TC_RATE_PPS, /** Current packet/s (rate estimator) */
RTNL_TC_QLEN, /** Current queue length */
RTNL_TC_BACKLOG, /** Current backlog length */
RTNL_TC_DROPS,/** Total number of packets dropped */
RTNL_TC_REQUEUES, /** Total number of requeues */
RTNL_TC_OVERLIMITS, /** Total number of overlimits */
__RTNL_TC_STATS_MAX,
};
#define RTNL_TC_STATS_MAX (__RTNL_TC_STATS_MAX - 1)
So the range is from 0 to RTNL_TC_STATS_MAX inclusive.
On June 3, 2015, 10:35 p.m., Jie Yu wrote:
src/linux/routing/queueing/statistics.hpp, line 23
https://reviews.apache.org/r/34426/diff/11/?file=977690#file977690line23
Should we get rid of 'queueing' here because this can be used by
filters as well?
It should work on filters but we have no use case and no testing to know if it
works. Lets leave it here until that changes.
- Paul
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34426/#review86493
---
On June 3, 2015, 9:19 p.m., Paul Brett wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34426/
---
(Updated June 3, 2015, 9:19 p.m.)
Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang.
Bugs: MESOS-2665
https://issues.apache.org/jira/browse/MESOS-2665
Repository: mesos
Description
---
Report the network statistics from libnl
Diffs
-
src/Makefile.am 66030c4b211ea61e97e62c35ec1821e0958f9787
src/linux/routing/queueing/fq_codel.hpp
49c8df626addcfc841132f821b8697b5e6e3b341
src/linux/routing/queueing/fq_codel.cpp
976c6a7a11d39609e6597daeb1a74c93ac62e4bf
src/linux/routing/queueing/ingress.hpp
2b7e1d3d1c02120195b6697320fb46f085a1ef92
src/linux/routing/queueing/ingress.cpp
e96f547200dc4ad5b2e33fd1ffbd7fb92b955a46
src/linux/routing/queueing/internal.hpp
3713f6a4d3f9a44b142b8150e85cf911e53e34e9
src/linux/routing/queueing/statistics.hpp PRE-CREATION
Diff: https://reviews.apache.org/r/34426/diff/
Testing
---
make check
Thanks,
Paul Brett
Re: Review Request 34426: Report per-container metrics for network bandwidth throttling
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34426/ --- (Updated June 2, 2015, 4:31 p.m.) Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang. Changes --- Fix rebase Bugs: MESOS-2665 https://issues.apache.org/jira/browse/MESOS-2665 Repository: mesos Description --- Report the network statistics from libnl Diffs (updated) - src/Makefile.am a5a7306b1ef65ca2b643653779ab76c26dbb5c90 src/linux/routing/queueing/fq_codel.hpp 7de1e31d4c43ac9cbffab1e472ea51140719f900 src/linux/routing/queueing/fq_codel.cpp 4dc2a9d2ed52937f0a78a083980db488c06b45a7 src/linux/routing/queueing/ingress.hpp 84506fecd01522471a7998176c28bea8f1367aed src/linux/routing/queueing/ingress.cpp ae0c38d2215e7fabcc1060e7385484bd455e1699 src/linux/routing/queueing/internal.hpp d43a9fd405af2e59dc57cfc7ba9b5e77cb9f6b70 Diff: https://reviews.apache.org/r/34426/diff/ Testing --- make check Thanks, Paul Brett
Re: Review Request 34426: Report per-container metrics for network bandwidth throttling
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34426/#review86058 --- src/linux/routing/queueing/fq_codel.hpp https://reviews.apache.org/r/34426/#comment137934 Find out what? Drop the clause or be specific, please. If the link doesn't exist is Result more appropriate so it can return None? src/linux/routing/queueing/internal.hpp https://reviews.apache.org/r/34426/#comment137932 +1 If we have a hard requirement for libnl = 3.2.26 then let's not do this. src/linux/routing/queueing/internal.hpp https://reviews.apache.org/r/34426/#comment137929 s/q/Q src/linux/routing/queueing/internal.hpp https://reviews.apache.org/r/34426/#comment137930 ditto src/linux/routing/queueing/internal.hpp https://reviews.apache.org/r/34426/#comment137924 use foreachpair and don't use auto where it's useful to see the type (and it's a short type name). src/linux/routing/queueing/statistics.hpp https://reviews.apache.org/r/34426/#comment137926 constexpr? src/linux/routing/queueing/statistics.cpp https://reviews.apache.org/r/34426/#comment137927 ditto - Ian Downes On May 31, 2015, 12:53 p.m., Paul Brett wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34426/ --- (Updated May 31, 2015, 12:53 p.m.) Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang. Bugs: MESOS-2665 https://issues.apache.org/jira/browse/MESOS-2665 Repository: mesos Description --- Report the network statistics from libnl Diffs - src/Makefile.am 9d1f0d5de2e7647a6677d86f5032ae1b79f1b241 src/linux/routing/queueing/fq_codel.hpp 7de1e31d4c43ac9cbffab1e472ea51140719f900 src/linux/routing/queueing/fq_codel.cpp 64b5c73e8cfa672141ddb663e879c58b4babfdbc src/linux/routing/queueing/ingress.hpp 84506fecd01522471a7998176c28bea8f1367aed src/linux/routing/queueing/ingress.cpp ae0c38d2215e7fabcc1060e7385484bd455e1699 src/linux/routing/queueing/internal.hpp d43a9fd405af2e59dc57cfc7ba9b5e77cb9f6b70 src/linux/routing/queueing/statistics.hpp PRE-CREATION src/linux/routing/queueing/statistics.cpp PRE-CREATION Diff: https://reviews.apache.org/r/34426/diff/ Testing --- make check Thanks, Paul Brett
Re: Review Request 34426: Report per-container metrics for network bandwidth throttling
On June 1, 2015, 6:22 p.m., Chi Zhang wrote: src/linux/routing/queueing/fq_codel.hpp, lines 63-64 https://reviews.apache.org/r/34426/diff/8/?file=975297#file975297line63 nit: can probably save the or an error if we cannot find out Disagree, I am trying to provide guidance between returning Error() and false. - Paul --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34426/#review86022 --- On May 31, 2015, 7:53 p.m., Paul Brett wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34426/ --- (Updated May 31, 2015, 7:53 p.m.) Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang. Bugs: MESOS-2665 https://issues.apache.org/jira/browse/MESOS-2665 Repository: mesos Description --- Report the network statistics from libnl Diffs - src/Makefile.am 9d1f0d5de2e7647a6677d86f5032ae1b79f1b241 src/linux/routing/queueing/fq_codel.hpp 7de1e31d4c43ac9cbffab1e472ea51140719f900 src/linux/routing/queueing/fq_codel.cpp 64b5c73e8cfa672141ddb663e879c58b4babfdbc src/linux/routing/queueing/ingress.hpp 84506fecd01522471a7998176c28bea8f1367aed src/linux/routing/queueing/ingress.cpp ae0c38d2215e7fabcc1060e7385484bd455e1699 src/linux/routing/queueing/internal.hpp d43a9fd405af2e59dc57cfc7ba9b5e77cb9f6b70 src/linux/routing/queueing/statistics.hpp PRE-CREATION src/linux/routing/queueing/statistics.cpp PRE-CREATION Diff: https://reviews.apache.org/r/34426/diff/ Testing --- make check Thanks, Paul Brett
Re: Review Request 34426: Report per-container metrics for network bandwidth throttling
On June 1, 2015, 8:11 p.m., Ian Downes wrote: src/linux/routing/queueing/statistics.hpp, lines 26-34 https://reviews.apache.org/r/34426/diff/8/?file=975302#file975302line26 constexpr? Nice :) - Paul --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34426/#review86058 --- On May 31, 2015, 7:53 p.m., Paul Brett wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34426/ --- (Updated May 31, 2015, 7:53 p.m.) Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang. Bugs: MESOS-2665 https://issues.apache.org/jira/browse/MESOS-2665 Repository: mesos Description --- Report the network statistics from libnl Diffs - src/Makefile.am 9d1f0d5de2e7647a6677d86f5032ae1b79f1b241 src/linux/routing/queueing/fq_codel.hpp 7de1e31d4c43ac9cbffab1e472ea51140719f900 src/linux/routing/queueing/fq_codel.cpp 64b5c73e8cfa672141ddb663e879c58b4babfdbc src/linux/routing/queueing/ingress.hpp 84506fecd01522471a7998176c28bea8f1367aed src/linux/routing/queueing/ingress.cpp ae0c38d2215e7fabcc1060e7385484bd455e1699 src/linux/routing/queueing/internal.hpp d43a9fd405af2e59dc57cfc7ba9b5e77cb9f6b70 src/linux/routing/queueing/statistics.hpp PRE-CREATION src/linux/routing/queueing/statistics.cpp PRE-CREATION Diff: https://reviews.apache.org/r/34426/diff/ Testing --- make check Thanks, Paul Brett
Re: Review Request 34426: Report per-container metrics for network bandwidth throttling
On June 1, 2015, 6:44 p.m., Cong Wang wrote: src/linux/routing/queueing/internal.hpp, line 284 https://reviews.apache.org/r/34426/diff/8/?file=975301#file975301line284 The lastest mesos code already relies on libnl 3.2.26, so not sure how much sense it makes to keep the backward compatibility here. Paul Brett wrote: 3.2.26 is not yet required as we have not pushed the last of the fq_codel changes to head. Lets leave this as a TODO until the dependency is committed to master. rtnl_u32_get_classid() is added in 3.2.26 and it has been already in master branch. - Cong --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34426/#review86040 --- On May 31, 2015, 7:53 p.m., Paul Brett wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34426/ --- (Updated May 31, 2015, 7:53 p.m.) Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang. Bugs: MESOS-2665 https://issues.apache.org/jira/browse/MESOS-2665 Repository: mesos Description --- Report the network statistics from libnl Diffs - src/Makefile.am 9d1f0d5de2e7647a6677d86f5032ae1b79f1b241 src/linux/routing/queueing/fq_codel.hpp 7de1e31d4c43ac9cbffab1e472ea51140719f900 src/linux/routing/queueing/fq_codel.cpp 64b5c73e8cfa672141ddb663e879c58b4babfdbc src/linux/routing/queueing/ingress.hpp 84506fecd01522471a7998176c28bea8f1367aed src/linux/routing/queueing/ingress.cpp ae0c38d2215e7fabcc1060e7385484bd455e1699 src/linux/routing/queueing/internal.hpp d43a9fd405af2e59dc57cfc7ba9b5e77cb9f6b70 src/linux/routing/queueing/statistics.hpp PRE-CREATION src/linux/routing/queueing/statistics.cpp PRE-CREATION Diff: https://reviews.apache.org/r/34426/diff/ Testing --- make check Thanks, Paul Brett
Re: Review Request 34426: Report per-container metrics for network bandwidth throttling
On June 1, 2015, 6:44 p.m., Cong Wang wrote: src/linux/routing/queueing/internal.hpp, line 284 https://reviews.apache.org/r/34426/diff/8/?file=975301#file975301line284 The lastest mesos code already relies on libnl 3.2.26, so not sure how much sense it makes to keep the backward compatibility here. 3.2.26 is not yet required as we have not pushed the last of the fq_codel changes to head. Lets leave this as a TODO until the dependency is committed to master. - Paul --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34426/#review86040 --- On May 31, 2015, 7:53 p.m., Paul Brett wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34426/ --- (Updated May 31, 2015, 7:53 p.m.) Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang. Bugs: MESOS-2665 https://issues.apache.org/jira/browse/MESOS-2665 Repository: mesos Description --- Report the network statistics from libnl Diffs - src/Makefile.am 9d1f0d5de2e7647a6677d86f5032ae1b79f1b241 src/linux/routing/queueing/fq_codel.hpp 7de1e31d4c43ac9cbffab1e472ea51140719f900 src/linux/routing/queueing/fq_codel.cpp 64b5c73e8cfa672141ddb663e879c58b4babfdbc src/linux/routing/queueing/ingress.hpp 84506fecd01522471a7998176c28bea8f1367aed src/linux/routing/queueing/ingress.cpp ae0c38d2215e7fabcc1060e7385484bd455e1699 src/linux/routing/queueing/internal.hpp d43a9fd405af2e59dc57cfc7ba9b5e77cb9f6b70 src/linux/routing/queueing/statistics.hpp PRE-CREATION src/linux/routing/queueing/statistics.cpp PRE-CREATION Diff: https://reviews.apache.org/r/34426/diff/ Testing --- make check Thanks, Paul Brett
Re: Review Request 34426: Report per-container metrics for network bandwidth throttling
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34426/ --- (Updated May 31, 2015, 7:53 p.m.) Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang. Changes --- Brake out the components of the qdisc statistics for easier review. Bugs: MESOS-2665 https://issues.apache.org/jira/browse/MESOS-2665 Repository: mesos Description --- Report the network statistics from libnl Diffs (updated) - src/Makefile.am 9d1f0d5de2e7647a6677d86f5032ae1b79f1b241 src/linux/routing/queueing/fq_codel.hpp 7de1e31d4c43ac9cbffab1e472ea51140719f900 src/linux/routing/queueing/fq_codel.cpp 64b5c73e8cfa672141ddb663e879c58b4babfdbc src/linux/routing/queueing/ingress.hpp 84506fecd01522471a7998176c28bea8f1367aed src/linux/routing/queueing/ingress.cpp ae0c38d2215e7fabcc1060e7385484bd455e1699 src/linux/routing/queueing/internal.hpp d43a9fd405af2e59dc57cfc7ba9b5e77cb9f6b70 src/linux/routing/queueing/statistics.hpp PRE-CREATION src/linux/routing/queueing/statistics.cpp PRE-CREATION Diff: https://reviews.apache.org/r/34426/diff/ Testing --- make check Thanks, Paul Brett
Re: Review Request 34426: Report per-container metrics for network bandwidth throttling
On May 26, 2015, 10:56 p.m., Chi Zhang wrote: src/linux/routing/queueing/fq_codel.cpp, line 80 https://reviews.apache.org/r/34426/diff/7/?file=969377#file969377line80 would egress::ROOT be more consistent? Yes, but it would require adding a namespace to hold a single value. - Paul --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34426/#review85274 --- On May 31, 2015, 7:53 p.m., Paul Brett wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34426/ --- (Updated May 31, 2015, 7:53 p.m.) Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang. Bugs: MESOS-2665 https://issues.apache.org/jira/browse/MESOS-2665 Repository: mesos Description --- Report the network statistics from libnl Diffs - src/Makefile.am 9d1f0d5de2e7647a6677d86f5032ae1b79f1b241 src/linux/routing/queueing/fq_codel.hpp 7de1e31d4c43ac9cbffab1e472ea51140719f900 src/linux/routing/queueing/fq_codel.cpp 64b5c73e8cfa672141ddb663e879c58b4babfdbc src/linux/routing/queueing/ingress.hpp 84506fecd01522471a7998176c28bea8f1367aed src/linux/routing/queueing/ingress.cpp ae0c38d2215e7fabcc1060e7385484bd455e1699 src/linux/routing/queueing/internal.hpp d43a9fd405af2e59dc57cfc7ba9b5e77cb9f6b70 src/linux/routing/queueing/statistics.hpp PRE-CREATION src/linux/routing/queueing/statistics.cpp PRE-CREATION Diff: https://reviews.apache.org/r/34426/diff/ Testing --- make check Thanks, Paul Brett
Re: Review Request 34426: Report per-container metrics for network bandwidth throttling
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34426/ --- (Updated May 22, 2015, 4:31 p.m.) Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang. Changes --- Update to reflect changes in Handle Bugs: MESOS-2665 https://issues.apache.org/jira/browse/MESOS-2665 Repository: mesos Description (updated) --- Report the network statistics from libnl Diffs (updated) - src/linux/routing/queueing/fq_codel.hpp 4f67ab7d64afea96a07dfcf36769a9c667749a00 src/linux/routing/queueing/fq_codel.cpp 02ad8df7814c0e549a9ca9aef39777684e6abdcb src/linux/routing/queueing/ingress.hpp b323a7f6daed828327d6d9e9740df81582e0ba2b src/linux/routing/queueing/ingress.cpp 47c73376097d70819defdee31a6d1e446df6b8ba src/linux/routing/queueing/internal.hpp 7c6c4d3d960b9a4bf44dcf482212317522353d69 src/slave/containerizer/isolators/network/port_mapping.cpp 49e983edab598e2ac487bb488fdd12840a9e7dfc src/tests/port_mapping_tests.cpp b8c2db6d0a02f79d38a21c227575299880980502 src/tests/routing_tests.cpp 6bf5e63aecf2dee36fb8cf3575c0bd2625d29dfa Diff: https://reviews.apache.org/r/34426/diff/ Testing --- make check Thanks, Paul Brett
Re: Review Request 34426: Report per-container metrics for network bandwidth throttling
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34426/#review84830
---
Thanks for the efforts! Here are my main sugguestions for this patch:
1) Please split it out. Let's do step by step.
2) Introduce a top level Decipline (similar to Filter).
Please let me know if you want to chat about that.
src/linux/routing/queueing/ingress.cpp
https://reviews.apache.org/r/34426/#comment136239
Do you have this defined somewhere already?
src/linux/routing/queueing/ingress.cpp
https://reviews.apache.org/r/34426/#comment136251
First of all, I think fixing the ingress handle is OK for now. Second, if
you really want to change this, you should probably do that in a separate patch.
src/linux/routing/queueing/internal.hpp
https://reviews.apache.org/r/34426/#comment136231
These causes unnessary code jumping and it's bad for readability. Could you
please revert this change? Thanks!
src/linux/routing/queueing/internal.hpp
https://reviews.apache.org/r/34426/#comment136241
I prefer keeping this function simple (just getting all the qdiscs). The
filtering can be handled by `getQdisc`.
src/linux/routing/queueing/internal.hpp
https://reviews.apache.org/r/34426/#comment136242
As I mentioned above, Let's keep getQdiscs simple and put all the filtering
logic in the following for loop.
src/linux/routing/queueing/internal.hpp
https://reviews.apache.org/r/34426/#comment136389
Can this be in a separate patch?
src/linux/routing/queueing/internal.hpp
https://reviews.apache.org/r/34426/#comment136393
Hum.. Looking at those interfaces make me feel that we should probably
introduce a top level Discipline class (not the same as the current one, but
similar to Filter):
```
template typename Config
class Discipline
{
Handle parent_;
OptionHandle handle_;
string kind_;
Config config_;
};
```
And the 'create' interface should be:
```
template typename Config
Trybool create(
const std::string _link,
const DisciplineConfig discipline)
{
...
}
```
Let's try not to return the Handle in this patch! Keep this patch smaller
makes it easier for the reviewers!
src/linux/routing/queueing/internal.hpp
https://reviews.apache.org/r/34426/#comment136250
? Please do not sneak in changes like this in an already very huge diff.
src/slave/containerizer/isolators/network/port_mapping.cpp
https://reviews.apache.org/r/34426/#comment136394
Let's still make the ingress::HANDLE fixed so that we don't need to change
port mapping isolator in this patch.
- Jie Yu
On May 22, 2015, 4:31 p.m., Paul Brett wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34426/
---
(Updated May 22, 2015, 4:31 p.m.)
Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang.
Bugs: MESOS-2665
https://issues.apache.org/jira/browse/MESOS-2665
Repository: mesos
Description
---
Report the network statistics from libnl
Diffs
-
src/linux/routing/queueing/fq_codel.hpp
4f67ab7d64afea96a07dfcf36769a9c667749a00
src/linux/routing/queueing/fq_codel.cpp
02ad8df7814c0e549a9ca9aef39777684e6abdcb
src/linux/routing/queueing/ingress.hpp
b323a7f6daed828327d6d9e9740df81582e0ba2b
src/linux/routing/queueing/ingress.cpp
47c73376097d70819defdee31a6d1e446df6b8ba
src/linux/routing/queueing/internal.hpp
7c6c4d3d960b9a4bf44dcf482212317522353d69
src/slave/containerizer/isolators/network/port_mapping.cpp
49e983edab598e2ac487bb488fdd12840a9e7dfc
src/tests/port_mapping_tests.cpp b8c2db6d0a02f79d38a21c227575299880980502
src/tests/routing_tests.cpp 6bf5e63aecf2dee36fb8cf3575c0bd2625d29dfa
Diff: https://reviews.apache.org/r/34426/diff/
Testing
---
make check
Thanks,
Paul Brett
Re: Review Request 34426: Report per-container metrics for network bandwidth throttling
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34426/ --- (Updated May 20, 2015, 11:44 p.m.) Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang. Changes --- Dependent changes are now review in separate commits Summary (updated) - Report per-container metrics for network bandwidth throttling Bugs: MESOS-2665 https://issues.apache.org/jira/browse/MESOS-2665 Repository: mesos Description (updated) --- Report the network statistics Diffs (updated) - include/mesos/mesos.proto 9cc5782256156ed59fd4640091413b76480d939f src/slave/containerizer/isolators/network/port_mapping.hpp c72fb47f60f40cda8d84a10497b9133f83cf018e src/slave/containerizer/isolators/network/port_mapping.cpp 49e983edab598e2ac487bb488fdd12840a9e7dfc Diff: https://reviews.apache.org/r/34426/diff/ Testing --- make check Thanks, Paul Brett
