On 9/8/20 2:06 PM, Numan Siddique wrote: > On Tue, Sep 8, 2020 at 4:54 PM Dumitru Ceara <dce...@redhat.com> wrote: >> >> On 9/8/20 12:58 PM, Numan Siddique wrote: >>> On Tue, Sep 8, 2020 at 2:13 PM Dumitru Ceara <dce...@redhat.com> wrote: >>>> >>>> OVN was dropping IP packets destined to IPs owned by logical routers but >>>> only if those IPs are not used for SNAT rules. However, if a packet >>>> doesn't match an existing NAT session and its destination is still a >>>> router owned IP, it can be safely dropped. Otherwise it will trigger an >>>> unnecessary packet-in in stage lr_in_arp_request. >>>> >>>> To achieve that we add flows that drop traffic to router owned IPs in >>>> table lr_in_arp_resolve. >>>> >>>> Reported-by: Tim Rozet <tro...@redhat.com> >>>> Reported-at: https://bugzilla.redhat.com/1876174 >>>> Signed-off-by: Dumitru Ceara <dce...@redhat.com> >>> >>> >>> >>> Hi Dumitru, >>> >>> Thanks for the fix. >>> >>> I have few comments. >>> >>> Suppose we have the below lr and NAT entries >>> >>> router 392b19fe-adf9-4b3d-bf43-040d12240e54 (lr0) >>> port lr0-sw0 >>> mac: "00:00:00:00:ff:01" >>> networks: ["10.0.0.1/24"] >>> port lr0-sw1 >>> mac: "00:00:00:00:ff:02" >>> networks: ["20.0.0.1/24"] >>> port lr0-public >>> mac: "00:00:20:20:12:13" >>> networks: ["172.168.0.100/24"] >>> gateway chassis: [chassis-1] >>> nat 4c26f3f0-0ae0-4c18-9a1e-9d2751389270 >>> external ip: "172.168.0.110" >>> logical ip: "10.0.0.3" >>> type: "dnat_and_snat" >>> nat b9296dc8-ac3a-427a-a97a-a94bf16214b2 >>> external ip: "172.168.0.120" >>> logical ip: "20.0.0.3" >>> type: "dnat_and_snat" >>> nat da28129c-4e81-4e20-865a-768bd88e5a26 >>> external ip: "172.168.0.100" >>> logical ip: "10.0.0.0/24" >>> type: "snat" >>> >>> I see the below lflows added in lr_in_ip_input to drop the pkts for >>> router port IPs >>> >>> table=3 (lr_in_ip_input ), priority=60 , match=(ip4.dst == >>> {10.0.0.1} || ip6.dst == {fe80::200:ff:fe00:ff01}), action=(drop;) >>> table=3 (lr_in_ip_input ), priority=60 , match=(ip4.dst == >>> {20.0.0.1} || ip6.dst == {fe80::200:ff:fe00:ff02}), action=(drop;) >>> >>> So I think there is no need to add the lflows in lr_in_arp_resolve to >>> drop for 10.0.0.1 and 20.0.0.1 again. >>> >>> I think it's sufficient to add lflows in lr_in_arp_resolve to drop the >>> packet only for packets destined to the router ips which have NAT >>> entries. >>> >> >> Hi Numan, >> >> Thanks for the review. >> >> You're right we could try to optimize a bit the number of flows. >> >> However, I didn't want to duplicate the code that builds snat_ips [0] >> and I thought it complicates the already long build_lrouter_flows() >> function if I add flows to stage LR_IN_ARP_RESOLVE in the same place >> where we add flows for LR_IN_IP_INPUT. >> >> Also, the number of IP addresses per router port is usually low so the >> number of redundant logical flows in lr_in_arp_resolve would be low too. >> >> If you really have a strong preference about this, I can try to change >> it though. >> > > Hi Dumitru, > > I was thinking more from the scale perspective. I think this issue is > to address ovn-k8s use case. > Although the gateway router on each node will be configured with NAT > entries and it will be > definitely having very few router ports, but I think on a large scale > setup, the cluster router will have > many router ports and we will see these flows on this router even > though there are no NAT entries > for the cluster router. > > For the openstack deployment too we will see these flows and a logical > router with gateway router port > may have many other router ports connecting to the tenant logical switches. > > It would be good if we address my comment. Or at the least add these > lflows only for routers configured > with NAT entries. >
We could also just add the flows to lr_in_arp_resolve and remove the ones that drop traffic destined to router-owned IPs in lr_in_ip_input. It's a bit ugly because we're forcing this in a stage that should resolve ARP but lr_in_ip_input is too early for traffic that might get NAT-ed. What do you think? > @Han Zhou Do you have comments or concerns with this patch ? > > I am raising these points, because you and Han addressed lflow > explosion issues a while back. > Although in this case, the flows added in this patch do not result in > exponential increase. > Right, here there should be max 1 flow per IP address so it shouldn't create a scale issue. Thanks, Dumitru > Thanks > Numan > > >> Regards, >> Dumitru >> >> [0] https://github.com/ovn-org/ovn/blob/master/northd/ovn-northd.c#L9015 >> >> _______________________________________________ >> dev mailing list >> d...@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
