You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Chi Zhang <ch...@gmail.com> on 2015/03/02 22:36:01 UTC
Re: Review Request 31503: (2/4) Add the Classifier action
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31503/#review74806
-----------------------------------------------------------
src/linux/routing/filter/action.hpp
<https://reviews.apache.org/r/31503/#comment121568>
The name 'classifier' is used in filter.hpp; I feel like also using it for action might lead to confusion.
src/linux/routing/filter/internal.hpp
<https://reviews.apache.org/r/31503/#comment121573>
"Failed to set the classifier _action_" (thats why suggested using a different name :)
src/linux/routing/filter/internal.hpp
<https://reviews.apache.org/r/31503/#comment121576>
If the classifier action was set but setting terminal had an error. What would happen? is rewinding necessary?
src/linux/routing/filter/internal.hpp
<https://reviews.apache.org/r/31503/#comment121577>
The function comment looks like you are not doing anything with any non-u32 filters but returning error. Maybe document what you are doing with basic filters as well?
src/linux/routing/filter/internal.hpp
<https://reviews.apache.org/r/31503/#comment121570>
Be consistent with other err msgs? :)
"Failed to do XXX: errMsg"
src/linux/routing/filter/ip.hpp
<https://reviews.apache.org/r/31503/#comment121579>
Let's give this public interface some comments like other functions in this file?
src/tests/routing_tests.cpp
<https://reviews.apache.org/r/31503/#comment121601>
If you can have a way to programmatically determine the existence of this condition, this test should be checked before enabling (environemnt.cpp has control for filtering tests; sometimes we also do it in the test fixture).
If that's impossible, you should print out error messages to explain this and give instructions to disable this test manually. src/tests has examples for that.
EXPECT(condition) << errMsg;
Also could you provide a link to the email thread or webpage where this bug is discussed / fixed in upstream for context (like how we document kernel issues in port_mapping.cpp)
src/tests/routing_tests.cpp
<https://reviews.apache.org/r/31503/#comment121602>
kill this line
src/tests/routing_tests.cpp
<https://reviews.apache.org/r/31503/#comment121604>
kill this line.
- Chi Zhang
On Feb. 27, 2015, 9:39 p.m., Cong Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31503/
> -----------------------------------------------------------
>
> (Updated Feb. 27, 2015, 9:39 p.m.)
>
>
> Review request for mesos, Chi Zhang, Ian Downes, and Jie Yu.
>
>
> Bugs: MESOS-2422
> https://issues.apache.org/jira/browse/MESOS-2422
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Introduce a new action which saves the classid for a specified filter. Similar to the u32 terminal action, it is not a real action defined by TC, it is just for setting a filter attribute. The classid or flowid is used to match the fq_codel flows.
>
>
> Diffs
> -----
>
> src/linux/routing/filter/action.hpp 410c15ad20ffa694bdb4c4187f1544ef9617ebac
> src/linux/routing/filter/arp.hpp fa0ea6f93218fb3bee9f435f0268022d5edc5e11
> src/linux/routing/filter/arp.cpp bf19264ccc83e8a8b8c2affe53779ac070472925
> src/linux/routing/filter/icmp.hpp 7b478c4e23a2fb0895021ce5fb30bdf78131b871
> src/linux/routing/filter/icmp.cpp 86bd67b71a590b88344adbe10fd1b44ea1b5148d
> src/linux/routing/filter/internal.hpp 8a6c0c0390c4ed119a11f0f808e0a244f7734c45
> src/linux/routing/filter/ip.hpp 932ed4bbf57e60261ff3b48ae242a7dd5e1f4260
> src/linux/routing/filter/ip.cpp 922a732c3543a072674208b123fdfadbef2b15f2
> src/tests/routing_tests.cpp 3cda6ab8c1ad24e4b7d0b9aeda2abc595fc857a5
>
> Diff: https://reviews.apache.org/r/31503/diff/
>
>
> Testing
> -------
>
> Run the testcase.
>
>
> Thanks,
>
> Cong Wang
>
>
Re: Review Request 31503: (2/4) Add the Classifier action
Posted by Chi Zhang <ch...@gmail.com>.
> On March 2, 2015, 9:36 p.m., Chi Zhang wrote:
> > src/tests/routing_tests.cpp, line 446
> > <https://reviews.apache.org/r/31503/diff/1/?file=879078#file879078line446>
> >
> > If you can have a way to programmatically determine the existence of this condition, this test should be checked before enabling (environemnt.cpp has control for filtering tests; sometimes we also do it in the test fixture).
> >
> > If that's impossible, you should print out error messages to explain this and give instructions to disable this test manually. src/tests has examples for that.
> >
> > EXPECT(condition) << errMsg;
> >
> > Also could you provide a link to the email thread or webpage where this bug is discussed / fixed in upstream for context (like how we document kernel issues in port_mapping.cpp)
>
> Cong Wang wrote:
> Hmm, it is possible, I will check how much effort we will need to detect this. At least I can just add a TODO saying we could detect this kernel bug.
>
> The kernel changelog of that commit already has these information I believe, or you mean I should add a web link to in the comment of the code?
Really just a nit: we did something like this in port_mapping.cpp:
// We disable 'rp_filter' and 'send_redirects' for host loopback
// interface (lo) to work around a kernel bug, which was only
// recently addressed in upstream in the following 3 commits.
// https://git.kernel.org/cgit/linux/kernel/git/davem/net.git/:
// 6a662719c9868b3d6c7d26b3a085f0cd3cc15e64
// 0d5edc68739f1c1e0519acbea1d3f0c1882a15d7
// e374c618b1465f0292047a9f4c244bd71ab5f1f0
- Chi
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31503/#review74806
-----------------------------------------------------------
On Feb. 27, 2015, 9:39 p.m., Cong Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31503/
> -----------------------------------------------------------
>
> (Updated Feb. 27, 2015, 9:39 p.m.)
>
>
> Review request for mesos, Chi Zhang, Ian Downes, and Jie Yu.
>
>
> Bugs: MESOS-2422
> https://issues.apache.org/jira/browse/MESOS-2422
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Introduce a new action which saves the classid for a specified filter. Similar to the u32 terminal action, it is not a real action defined by TC, it is just for setting a filter attribute. The classid or flowid is used to match the fq_codel flows.
>
>
> Diffs
> -----
>
> src/linux/routing/filter/action.hpp 410c15ad20ffa694bdb4c4187f1544ef9617ebac
> src/linux/routing/filter/arp.hpp fa0ea6f93218fb3bee9f435f0268022d5edc5e11
> src/linux/routing/filter/arp.cpp bf19264ccc83e8a8b8c2affe53779ac070472925
> src/linux/routing/filter/icmp.hpp 7b478c4e23a2fb0895021ce5fb30bdf78131b871
> src/linux/routing/filter/icmp.cpp 86bd67b71a590b88344adbe10fd1b44ea1b5148d
> src/linux/routing/filter/internal.hpp 8a6c0c0390c4ed119a11f0f808e0a244f7734c45
> src/linux/routing/filter/ip.hpp 932ed4bbf57e60261ff3b48ae242a7dd5e1f4260
> src/linux/routing/filter/ip.cpp 922a732c3543a072674208b123fdfadbef2b15f2
> src/tests/routing_tests.cpp 3cda6ab8c1ad24e4b7d0b9aeda2abc595fc857a5
>
> Diff: https://reviews.apache.org/r/31503/diff/
>
>
> Testing
> -------
>
> Run the testcase.
>
>
> Thanks,
>
> Cong Wang
>
>
Re: Review Request 31503: (2/4) Add the Classifier action
Posted by Cong Wang <cw...@twopensource.com>.
> On March 2, 2015, 9:36 p.m., Chi Zhang wrote:
> > src/linux/routing/filter/internal.hpp, line 274
> > <https://reviews.apache.org/r/31503/diff/1/?file=879075#file879075line274>
> >
> > The function comment looks like you are not doing anything with any non-u32 filters but returning error. Maybe document what you are doing with basic filters as well?
Ah, comment is out of date, fixed now.
- Cong
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31503/#review74806
-----------------------------------------------------------
On Feb. 27, 2015, 9:39 p.m., Cong Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31503/
> -----------------------------------------------------------
>
> (Updated Feb. 27, 2015, 9:39 p.m.)
>
>
> Review request for mesos, Chi Zhang, Ian Downes, and Jie Yu.
>
>
> Bugs: MESOS-2422
> https://issues.apache.org/jira/browse/MESOS-2422
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Introduce a new action which saves the classid for a specified filter. Similar to the u32 terminal action, it is not a real action defined by TC, it is just for setting a filter attribute. The classid or flowid is used to match the fq_codel flows.
>
>
> Diffs
> -----
>
> src/linux/routing/filter/action.hpp 410c15ad20ffa694bdb4c4187f1544ef9617ebac
> src/linux/routing/filter/arp.hpp fa0ea6f93218fb3bee9f435f0268022d5edc5e11
> src/linux/routing/filter/arp.cpp bf19264ccc83e8a8b8c2affe53779ac070472925
> src/linux/routing/filter/icmp.hpp 7b478c4e23a2fb0895021ce5fb30bdf78131b871
> src/linux/routing/filter/icmp.cpp 86bd67b71a590b88344adbe10fd1b44ea1b5148d
> src/linux/routing/filter/internal.hpp 8a6c0c0390c4ed119a11f0f808e0a244f7734c45
> src/linux/routing/filter/ip.hpp 932ed4bbf57e60261ff3b48ae242a7dd5e1f4260
> src/linux/routing/filter/ip.cpp 922a732c3543a072674208b123fdfadbef2b15f2
> src/tests/routing_tests.cpp 3cda6ab8c1ad24e4b7d0b9aeda2abc595fc857a5
>
> Diff: https://reviews.apache.org/r/31503/diff/
>
>
> Testing
> -------
>
> Run the testcase.
>
>
> Thanks,
>
> Cong Wang
>
>
Re: Review Request 31503: (2/4) Add the Classifier action
Posted by Cong Wang <cw...@twopensource.com>.
> On March 2, 2015, 9:36 p.m., Chi Zhang wrote:
> > src/tests/routing_tests.cpp, line 446
> > <https://reviews.apache.org/r/31503/diff/1/?file=879078#file879078line446>
> >
> > If you can have a way to programmatically determine the existence of this condition, this test should be checked before enabling (environemnt.cpp has control for filtering tests; sometimes we also do it in the test fixture).
> >
> > If that's impossible, you should print out error messages to explain this and give instructions to disable this test manually. src/tests has examples for that.
> >
> > EXPECT(condition) << errMsg;
> >
> > Also could you provide a link to the email thread or webpage where this bug is discussed / fixed in upstream for context (like how we document kernel issues in port_mapping.cpp)
Hmm, it is possible, I will check how much effort we will need to detect this. At least I can just add a TODO saying we could detect this kernel bug.
The kernel changelog of that commit already has these information I believe, or you mean I should add a web link to in the comment of the code?
- Cong
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31503/#review74806
-----------------------------------------------------------
On Feb. 27, 2015, 9:39 p.m., Cong Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31503/
> -----------------------------------------------------------
>
> (Updated Feb. 27, 2015, 9:39 p.m.)
>
>
> Review request for mesos, Chi Zhang, Ian Downes, and Jie Yu.
>
>
> Bugs: MESOS-2422
> https://issues.apache.org/jira/browse/MESOS-2422
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Introduce a new action which saves the classid for a specified filter. Similar to the u32 terminal action, it is not a real action defined by TC, it is just for setting a filter attribute. The classid or flowid is used to match the fq_codel flows.
>
>
> Diffs
> -----
>
> src/linux/routing/filter/action.hpp 410c15ad20ffa694bdb4c4187f1544ef9617ebac
> src/linux/routing/filter/arp.hpp fa0ea6f93218fb3bee9f435f0268022d5edc5e11
> src/linux/routing/filter/arp.cpp bf19264ccc83e8a8b8c2affe53779ac070472925
> src/linux/routing/filter/icmp.hpp 7b478c4e23a2fb0895021ce5fb30bdf78131b871
> src/linux/routing/filter/icmp.cpp 86bd67b71a590b88344adbe10fd1b44ea1b5148d
> src/linux/routing/filter/internal.hpp 8a6c0c0390c4ed119a11f0f808e0a244f7734c45
> src/linux/routing/filter/ip.hpp 932ed4bbf57e60261ff3b48ae242a7dd5e1f4260
> src/linux/routing/filter/ip.cpp 922a732c3543a072674208b123fdfadbef2b15f2
> src/tests/routing_tests.cpp 3cda6ab8c1ad24e4b7d0b9aeda2abc595fc857a5
>
> Diff: https://reviews.apache.org/r/31503/diff/
>
>
> Testing
> -------
>
> Run the testcase.
>
>
> Thanks,
>
> Cong Wang
>
>