-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31505/#review84998
-----------------------------------------------------------


Two high level comments:

1) We need a slave flag to control if turning on this feature or not
2) Do you think we should at least add some unit test for this? For example, 
verify that the filters are properly set up?


src/linux/routing/filter/filter.hpp
<https://reviews.apache.org/r/31505/#comment136460>

    These two can be merged since `classid` is already Optional. So please 
remove the first constructor and change the callsites accordingly.



src/linux/routing/filter/filter.hpp
<https://reviews.apache.org/r/31505/#comment136461>

    These two can be merged too. Remove the latter and updates the callsites.



src/slave/containerizer/isolators/network/port_mapping.cpp
<https://reviews.apache.org/r/31505/#comment136470>

    Please add a NOTE: prefix here for the comments.



src/slave/containerizer/isolators/network/port_mapping.cpp
<https://reviews.apache.org/r/31505/#comment136465>

    We should call it flowIds:
    ```
    flowIds[sourcePorts.get()] = classid.get().secondary();
    ```



src/slave/containerizer/isolators/network/port_mapping.cpp
<https://reviews.apache.org/r/31505/#comment136468>

    s/classid/flowId/



src/slave/containerizer/isolators/network/port_mapping.cpp
<https://reviews.apache.org/r/31505/#comment136466>

    'classid' here could be None, right? It'll cause classid.get() to crash. So 
please move it to the end of this function:
    
    ```
    if (flowId.isSome()) {
      info->flowId = flowId.get();
      freeFlowIds.erase(flowId.get());
    }
    ```



src/slave/containerizer/isolators/network/port_mapping.cpp
<https://reviews.apache.org/r/31505/#comment136469>

    Kill this blank line.



src/slave/containerizer/isolators/network/port_mapping.cpp
<https://reviews.apache.org/r/31505/#comment136476>

    Rest of the host packets...



src/slave/containerizer/isolators/network/port_mapping.cpp
<https://reviews.apache.org/r/31505/#comment136480>

    Could you please move this right above `set<string> targets` and add a LOG 
as well?



src/slave/containerizer/isolators/network/port_mapping.cpp
<https://reviews.apache.org/r/31505/#comment136482>

    s/Host/host/
    
    We use camelCase for varibles. Please fix it here and everywhere else.



src/slave/containerizer/isolators/network/port_mapping.cpp
<https://reviews.apache.org/r/31505/#comment136481>

    Please add a blank line above.


- Jie Yu


On May 17, 2015, 12:27 a.m., Cong Wang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31505/
> -----------------------------------------------------------
> 
> (Updated May 17, 2015, 12:27 a.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
> -------
> 
> Currently we do nothing on the host egress side. By default, kernel uses its 
> own hash function to classify the packets to different TX queues (if the 
> hardware interface supports multiqueue). So packets coming out of different 
> containers could end up queueing in the same TX queue, in this case we saw 
> buffer bloat on some TX queue caused packet drops.
> 
> We need to isolation the egress traffic so that containers will not have 
> interference with each other. The number of hardware TX queues is limited by 
> hardware interface, usually not enough to map our container in 1:1 way, 
> therefore we need some software solution. We choose fq_codel and use tc 
> filters to classify packets coming out of different containers to different 
> fq_codel flows, and the codel algorithm on each flow could also help us to 
> reduce the buffer bloat. Note when the packets leave fq_codel, they still 
> share the physical TX queue(s), this is however (almost) beyond what we can 
> control, we have to rely on the kernel behavior.
> 
> TODO: get some performance numbers
> 
> 
> Diffs
> -----
> 
>   src/linux/routing/filter/filter.hpp 
> 024582cf8274da2e5bcccc4ebd00fcf83d6930e2 
>   src/linux/routing/filter/ip.hpp 62bb5f89d35fc9622fca303aa01a789fcfbf2f76 
>   src/linux/routing/filter/ip.cpp 0d25e2d4f0ac4c3f151ad40e796dbdbc54dba9fa 
>   src/slave/containerizer/isolators/network/port_mapping.hpp 
> c72fb47f60f40cda8d84a10497b9133f83cf018e 
>   src/slave/containerizer/isolators/network/port_mapping.cpp 
> a4abaff30bb4646b1b1edfdbbc243c9e3f6851df 
> 
> Diff: https://reviews.apache.org/r/31505/diff/
> 
> 
> Testing
> -------
> 
> Manually start two mesos containers with netperf running side.
> 
> 
> Thanks,
> 
> Cong Wang
> 
>

Reply via email to