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. > > 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
