----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31505/#review83690 -----------------------------------------------------------
src/slave/containerizer/isolators/network/port_mapping.hpp <https://reviews.apache.org/r/31505/#comment134860> Please add some comments about what this is. Consider using hashset instead of std::set. src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/31505/#comment134858> Could you please add some comments about what this is? For example, what is container min flowid? What is host flowid? Why ICMP and ARP are sharing the same flowid? src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/31505/#comment134733> What does that mean? What do you want to revise? You probably want to restructure the TODO like the following: ``` TODO(cwang): Maybe we can continue .... See details in MESOS-2370. ``` src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/31505/#comment134859> The comments here is a little confusing. The flowIDs here is actually freeFlowIds, right? How about just calling it freeFlowIds? src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/31505/#comment134863> We define variables when we want to use them. So you don't have to declare this variable here. src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/31505/#comment134866> Can you move this code block after getting the vethClassifiers? src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/31505/#comment134862> No need to use `filter::` prefix for `filter::Filter` as it's already included. src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/31505/#comment134864> s/flowClassifiers/eth0EgressFilters/ src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/31505/#comment134865> s/classifiers/vethIngressClassifiers/ src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/31505/#comment134875> You probably also want to do some sanity check here to make sure all port ranges from a container points to the same flow on eth0 egress. Also, PortRange supports hashing, so you can probably pre-create a hashmap<PortRange, classid> mapping and lookup the hashmap later: ``` // Construct a port range to classid mapping from host eth0 egress. // TODO: Consider moving the following to a common place so that we // don't duplicate this logic for each container. Result<vector<Filter<ip::Classifier>>> eth0EgressFilters = ip::filters(eth0, fq_codel::HANDLE); ... hashmap<PortRange, uint16_t> classids; foreach (const Filter<ip::Classifier>& filter, eth0EgressFilters.get()) { const Option<PortRange> sourcePorts = filter.classifier().sourcePorts(); const Option<uint16_t> classid = filter.classifier().classid(); if (sourcePorts.isNone()) { return Error("Missing source ports for filters on egress of " + eth0); } if (classid.isNone()) { return Error("Missing classid for filters on egress of " + eth0); } if (classids.contains(range) { return Error( "Duplicated port range " + stringify(range) + " detected on egress of " + eth0); } classids[range.get()] = classid.get(); } ... IntervalSet<uint16_t> nonEphemeralPorts; IntervalSet<uint16_t> ephemeralPorts; Option<uint16_t> classid; foreach (...) { ... if (classids.contains(sourcePorts.get()) { if (classid.isNone()) { classid = classids.get(sourcePorts.get()); } else if (classid != classids.get(sourcePorts.get())) { return Error("A container is associated with multiple flows on eth0 egress"); } } else if (classid.isSome()) { // This is the case where some port range of a container is assigned // to a flow while some isn't. This could happen if slave crashes // while those filters are created. However, this is OK for us because // ... LOG(WARNING) << "..."; } ... } ``` src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/31505/#comment134885> info->flowId = getNextFlowId(); src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/31505/#comment134886> s/flowId/info->flowId/ src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/31505/#comment134889> Please passing in flowId and make 'addHostIPFilters' accept an Option<uint16_t> flowId. src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/31505/#comment134890> Shouldn't this be Priority(ICMP_FILTER_PRIORITY, NORMAL)? src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/31505/#comment134892> Add a space before '=' src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/31505/#comment134891> Ditto here. src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/31505/#comment134894> Priority(DEFAULT_FILTER_PRIORITY, NORMAL); ? src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/31505/#comment134901> Do you need a new flow here? I would imagine to reuse the existing flow if exists? src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/31505/#comment134871> Could you please add a comment here saying that all IP packets from a container will be assigned a *single* flow on host eth0. src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/31505/#comment134899> Why is that? I would imagine some port range still points the original flow? src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/31505/#comment134870> s/const// We typically don't use const for primitive type parameters. src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/31505/#comment134896> Priority(IP_FILTER_PRIORITY, NORMAL)? - Jie Yu On May 12, 2015, 12:14 a.m., Cong Wang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/31505/ > ----------------------------------------------------------- > > (Updated May 12, 2015, 12:14 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/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 > >
