On Tue, Nov 12, 2019 at 1:56 AM Han Zhou <[email protected]> wrote: > > > > On Mon, Nov 11, 2019 at 11:03 AM Dumitru Ceara <[email protected]> wrote: > > > > On Mon, Nov 11, 2019 at 6:11 PM Han Zhou <[email protected]> wrote: > > > > > > 1. Would there be problem for VLAN backed logical router use case, since > > > a chassis MAC is used as src MAC to send packets from router ports? > > > 2. How about checking if tpa == spa to make sure GARP is always flooded? > > > (not directly supported by OF) > > > > This would've been nice to have in OF :) > > > > > > > > > > > On Mon, Nov 11, 2019 at 5:32 AM Dumitru Ceara <[email protected]> wrote: > > > > > > > > On Sat, Nov 9, 2019 at 8:35 AM Han Zhou <[email protected]> wrote: > > > > > > > > > > > > > > > > > > > > On Fri, Nov 8, 2019 at 6:38 AM Dumitru Ceara <[email protected]> > > > > > wrote: > > > > > > > > > > > > ARP request and ND NS packets for router owned IPs were being > > > > > > flooded in the complete L2 domain (using the MC_FLOOD multicast > > > > > > group). > > > > > > However this creates a scaling issue in scenarios where aggregation > > > > > > logical switches are connected to more logical routers (~350). The > > > > > > logical pipelines of all routers would have to be executed before > > > > > > the > > > > > > packet is finally replied to by a single router, the owner of the IP > > > > > > address. > > > > > > > > > > > > This commit limits the broadcast domain by bypassing the L2 Lookup > > > > > > stage > > > > > > for ARP requests that will be replied by a single router. The > > > > > > packets > > > > > > are forwarded only to the router port that owns the target IP > > > > > > address. > > > > > > > > > > > > IPs that are owned by the routers and for which this fix applies > > > > > > are: > > > > > > - IP addresses configured on the router ports. > > > > > > - VIPs. > > > > > > - NAT IPs. > > > > > > > > > > > > This commit also fixes function get_router_load_balancer_ips() which > > > > > > was incorrectly returning a single address_family even though the > > > > > > IP set could contain a mix of IPv4 and IPv6 addresses. > > > > > > > > > > > > Reported-at: https://bugzilla.redhat.com/1756945 > > > > > > Reported-by: Anil Venkata <[email protected]> > > > > > > Signed-off-by: Dumitru Ceara <[email protected]> > > > > > > > > > > > > --- > > > > > > v6: > > > > > > - Address Han's comments: > > > > > > - remove flooding of ARPs targeting OVN owned IP addresses. > > > > > > - update ovn-architecture documentation. > > > > > > - rename ARP handling functions. > > > > > > - Adapt "ovn -- 3 HVs, 3 LS, 3 lports/LS, 1 LR" autotest to take > > > > > > into > > > > > > account the new way of forwarding ARPs. > > > > > > - Also, properly deal with ARP packets on VLAN-backed networks. > > > > > > > > > > I am confused by this additional VLAN related change. VLAN is just > > > > > handled as bridged logical switch where a localnet port is used as > > > > > *inport*. It seems to me no difference from regular localnet port no > > > > > matter with or without VLAN tag. When an ARP request is going through > > > > > the ingress pipeline of the bridged logical switch, the logic of > > > > > bypassing the flooding added by this patch should just apply, right? > > > > > It is also very common scenario that the *aggregation switch* for the > > > > > routers is an external physical network backed by VLAN. I think the > > > > > purpose of this patch is to make sure scenario scale. Did I > > > > > misunderstand anything here? > > > > > > > > Hi Han, > > > > > > > > The reason behind it was that with VLAN backed networks when packets > > > > move between hypervisors without going through geneve we rerun the > > > > ingress pipeline. That would mean that the flows I added for self > > > > originated (G)ARP packets won't be hit for ARP requests originated by > > > > ovn-controller on a remote hypervisor: > > > > > > > > priority=80 match: "inport=<patch-port-to-lr>, arp.op == 1" action: > > > > "outport=MC_FLOOD" > > > > > > > > But instead we would hit: > > > > priority=75 match: "arp.op == 1 && arp.tpa == <OVN-owned-IP" action: > > > > "outport=<patch-port-to-lr>" and never send flood the packet out on > > > > the second HV. > > > > > > > > > > Thanks for the explain. Now I understand the problem. > > > > > > > You're right, the way I added the check for all VLAN packets is not OK > > > > as we fall back to the old behavior too often. However, I'm not sure > > > > what the best option is. Do you think it's fine if I change the > > > > priority 80 flow to the following? > > > > > > > > priority=80 match: "eth.src={lrp.ea, lr.nat.external_macs} && arp.op > > > > == 1" action: "outport=MC_FLOOD" > > > > > > > > The idea would be to identify self originated ARP requests by matching > > > > the source mac instead of logical ingress port (as this might not be > > > > present). I tried it locally and it works fine but we do need to add > > > > more flows than before. > > > > > > > > > > Would this work when "ovn-chassis-mac-mappings" is configured for VLAN > > > backed logical routers? We might end up adding chassis unique MACs, too? > > > > I think it will work fine because when ovn-chassis-mac-mappings is > > configured we add an entry in table OFTABLE_PHY_TO_LOG to match on > > CHASSIS_MAC_TO_ROUTER_MAC_CONJID (comparing eth.src to all > > ovn-chassis-macs) to rewrite the eth.src address with that of the > > router port. > > > > > > > > Alternatively, I think we may change the priority 80 flow to match for > > > each OVN router owned IP: arp.tpa == IP && arp.spa == IP ... Would this > > > ensure OVN router generated ARPs are flooded? > > > > > > > I considered this too but because a router port can have an > > "unlimited" number of networks configured I decided to go for MAC to > > minimize the number of flows. > > > > Also, if we match on eth.src we can create a single priority 80 logical > > flow: > > priority=80 match: "eth.src={lrp.ea, lr.nat.external_macs} && arp.op > > == 1" action: "outport=MC_FLOOD" > > > > Whereas if we match on each of the IPs we need individual flows: > > priority=80 match: "arp.tpa == IP && arp.spa == IP && arp.op == 1" > > action: "outport=MC_FLOOD" > > > > In the end they translate to the same number of OF entries but we > > store less logical flows in the database. > > > > Do you think there's anything else missing? > > Ok. I agree that adding chassis unique MACs should be ok and it is likely to > result in less number of flows than using IP. Please let me know when you > send the new version.
Hi Han, I sent a v7 split in series: - patch1: the get_router_load_balancer_ips() fix as it is independent from the ARP/ND broadcast limiting - patch2: the ARP/ND broadcast domain limiting https://patchwork.ozlabs.org/project/openvswitch/list/?series=142268 Thanks, Dumitru > > > > > Thanks, > > Dumitru > > > > > > > > > > > > > > > > In addition, the below macro definition may be renamed to > > > > > FLAGBIT_..., because it is for the bits of MFF_LOG_FLAGS, which is > > > > > one of the pre-defined logical fields, instead of for the > > > > > MFF_LOG_REG0-9 registers. > > > > > > > > > > > > +#define REGBIT_NOT_VXLAN "flags[1] == 0" > > > > > > +#define REGBIT_NOT_VLAN "flags[7] == 0" > > > > > > + > > > > > > > > > > The other part looks good to me. Thanks for simply the patch. > > > > > > > > > > Han > > > > > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
