> On May 22, 2015, 10:09 p.m., Jie Yu wrote: > > 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? > > Jie Yu wrote: > Ping. Did you see these two comments? Seems that the new diff hasn't > resolved these two issues. > > Cong Wang wrote: > Yes. 1) I think we all agree on not adding a new flag in case of > inconsistent behavior; 2) We already have a test case, FqCodelClassifier. > > Jie Yu wrote: > For 1), I think we need a flag so that we can slowly roll out this > feature. Otherwise, if there's a bug, all slave needs to be rebooted. > > For 2), I mean we need to add an integration test in > port_mapping_tests.cpp to exercise the code path in the isolator. The test > you mentioned is a unit test for the routing library stuff. > > Cong Wang wrote: > 1) That depends on what is our fallback solution in case of bugs, > potentially every line of new code needs a flag if not reverting to an old > binary. :) So generally speaking, what is our solution when we hit some bug > during deloyment? If it is reverting to the old version, then adding a flag > just for deloyment is useless. Or if it is turning off some flag, then as I > said every new code would potentially need a flag and some new code can't > have a flag, that means this way doesn't scale. > > 2) In src/tests/port_mapping_tests.cpp, it calls > PortMappingIsolatorProcess::create() which should cover these new code too, > no? > > Jie Yu wrote: > For 1), if the fallback solution is to just to restart the slave, then we > usually don't require a new flag. However, in this case, we will leave > filters on host eth0, and a simple restart of the slave won't clean up those > filters. > > For 2), Yes, we exercise the code of creating those filters, but do we > have ASSERT or EXPECT to check if those filters are properly setup? AFAICT, > even if the filters are not properly setup, the existing tests will still > finish, no? > > Cong Wang wrote: > 1) We only leave the default filters, which is used to catch all > non-matched packets (essentially a kenrel issue as mentioned in comments), so > nothing harms here. > > > 2) Test cases like ContainerICMPExternal check the return value of > ::create() and ::isolate(), so if any of these filters fails to setup, then > they should fail, right? I mean: > > > // Isolate the forked child. > AWAIT_READY(isolator.get()->isolate(containerId, pid.get())); > > Even if not, we should fix these test cases rather adding a new one? > Since they are supposed to catch all tc setup failures? > > Jie Yu wrote: > 1) What if the isolator creates some filters for the container and we > decided to rollback after that due to bug. Do we leave other filters as well? > > 2) How do you capture the case where two filters associated the same > container are assigned to different flows in host eth0 (I remembered that was > a bug in the code previously) using the existing tests?
Oh, you should mention the flag is just for recovery test, it is clearly not for deployment. :) But that means every new feature added to isolate() would have to get a new flag, otherwise not possible to do unit test for recovery with current code, perhaps we just need some internal flags for unit tests only. Note, that also means we have to disable this feature at run-time and continue when kernel doesn't support fq_codel (currently we just fail). - Cong ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31505/#review84998 ----------------------------------------------------------- On May 26, 2015, 8:41 p.m., Cong Wang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/31505/ > ----------------------------------------------------------- > > (Updated May 26, 2015, 8:41 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/linux/routing/filter/basic.cpp 755be7de5c43c2f3b336af6cb47af9c97a21218c > src/linux/routing/filter/filter.hpp > 024582cf8274da2e5bcccc4ebd00fcf83d6930e2 > src/linux/routing/filter/icmp.cpp 60703e727ecab9557911ab0562773771db51db90 > 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 > 49e983edab598e2ac487bb488fdd12840a9e7dfc > > Diff: https://reviews.apache.org/r/31505/diff/ > > > Testing > ------- > > Manually start two mesos containers with netperf running side. > > > Thanks, > > Cong Wang > >
