----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31505/#review74859 -----------------------------------------------------------
Cong, while you are working on the recovery story: there is a caveat that the order you add filters on different place is important otherwise races happen. There is extensive comment in isolate and recover for that. See if your recover logic applies as well. src/slave/containerizer/isolators/network/port_mapping.hpp <https://reviews.apache.org/r/31505/#comment121658> const std::string& eth0 src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/31505/#comment121659> alphebatical order. src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/31505/#comment121660> ditto. src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/31505/#comment121676> Is it the case that the smaller the number, the higher the priority? If that's the case, maybe document that default is used to 'catch' everything else? src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/31505/#comment121678> isn't it more like a CONTAINER_FLOWID_BEGIN? src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/31505/#comment121662> s/revise/Revise/ src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/31505/#comment121663> s/do/Do/ src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/31505/#comment121665> You saved the flow code in ::isolate. Is it unnecessary here? src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/31505/#comment121667> Is it a good idea to update the log msg with flowID here? src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/31505/#comment121666> Any specific reason host traffic has the lowest prioity among these 3? src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/31505/#comment121668> update msg with flow id? src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/31505/#comment121669> ditto src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/31505/#comment121671> The order of recycling flowid and the call to cleanup below, which one should happen first, esp. when cleanup fails? src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/31505/#comment121670> ditto src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/31505/#comment121672> s/revise/Revise/ src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/31505/#comment121673> We don't remove ingress qdisc either here. So to be consistent maybe have only a TODO mentioning the ticket and say it should apply to both ingress and fq_codel? (could you edit that ticket btw to include fq_codel as well, thanks!) src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/31505/#comment121679> don't reuse this counter here? revise the names of the existing ones and add your own counters for your own errors? Otherwise, these counters are useless when where is more than one source? ditto for all the other counters you use. src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/31505/#comment121675> add a period at the end of the line. - Chi Zhang On March 2, 2015, 5:21 p.m., Cong Wang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/31505/ > ----------------------------------------------------------- > > (Updated March 2, 2015, 5:21 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 > ------- > > 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/slave/containerizer/isolators/network/port_mapping.hpp > 8443097b2c79fef5ae0e23a3fb815ffec0318a93 > src/slave/containerizer/isolators/network/port_mapping.cpp > 5227987cdb7b904c2f4bb2bdf5c5d705a435010d > > Diff: https://reviews.apache.org/r/31505/diff/ > > > Testing > ------- > > Manually start two mesos containers with netperf running side. > > > Thanks, > > Cong Wang > >
