> On 5/26/20 5:23 AM, Han Zhou wrote: > > > > > > On Mon, May 25, 2020 at 2:55 PM Lorenzo Bianconi > > <lorenzo.bianc...@redhat.com <mailto:lorenzo.bianc...@redhat.com>> wrote: > >> > >> Introduce 150-priority logical flows for each NAT rule that can > >> be handled in a distributed manner to manage ARP request locally > >> for FIP traffic. In particular set reg1 and eth.src to NAT external > >> ip and NAT external mac respectivelly and do not distribute ARP > >> traffic using FIP > >> > >> Signed-off-by: Lorenzo Bianconi <lorenzo.bianc...@redhat.com > > <mailto:lorenzo.bianc...@redhat.com>> > >> --- > >> northd/ovn-northd.8.xml | 12 ++++++++++++ > >> northd/ovn-northd.c | 18 +++++++++++++++++- > >> tests/ovn.at <http://ovn.at> | 28 +++++++++++++++++++++++++--- > >> tests/system-ovn.at <http://system-ovn.at> | 28 > > ++++++++++++++++++++++++++++ > >> 4 files changed, 82 insertions(+), 4 deletions(-) > >> > >> diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml > >> index 9423cbfd1..5e37d9758 100644 > >> --- a/northd/ovn-northd.8.xml > >> +++ b/northd/ovn-northd.8.xml > >> @@ -2875,6 +2875,18 @@ icmp4 { > >> </p> > >> > >> <ul> > >> + <li> > >> + For each NAT rule in the OVN Northbound database that can > >> + be handled in a distributed manner, a priorirty-150 logical > >> + flow with match <code>ip.src == <var>A</var> && > >> + is_chassis_resident(<var>B</var>)</code>, where <var>A</var> > >> + is NAT logical ip and <var>B</var> is NAT logical port. > >> + IP traffic matching the above rule will be managed locally > >> + setting <code>reg1</code> to <var>C</var> and > > <code>eth.src</code> > >> + to <var>D</var>, where <var>C</var> is NAT external ip and > >> + <var>D</var> is NAT external mac. > >> + </li> > >> + > >> <li> > >> For each NAT rule in the OVN Northbound database that can > >> be handled in a distributed manner, a priority-100 logical > >> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c > >> index 7ae5e45da..20f0ee2e4 100644 > >> --- a/northd/ovn-northd.c > >> +++ b/northd/ovn-northd.c > >> @@ -9181,8 +9181,24 @@ build_lrouter_flows(struct hmap *datapaths, > > struct hmap *ports, > >> /* Ingress Gateway Redirect Table: For NAT on a distributed > >> * router, add flows that are specific to a NAT rule. These > >> * flows indicate the presence of an applicable NAT rule that > >> - * can be applied in a distributed manner. */ > >> + * can be applied in a distributed manner. > >> + * Moreover add logical flows to avoid ARP distributing > > when the > >> + * chassis has a direct connection to the underlay > > network through > >> + * a localnet port > > > > I think the two flows may be combined, right? Firstly, the match > > "outport == DGW" is missing from the new flow. Secondly, the > > "is_chassis_resident()" seems to be missing from the original flow. If > > this understanding is correct, the both flows just have same match > > condition, so the actions should be combined as well. > > > > Hi Lorenzo, Han, > > I agree that both flows should have the same match and that actions > should be combined. > > I'm not sure however if we can ever hit these flows if > is_chassis_resident(logical_port) == false. If I understand correctly we > can't, but just to be safe it's probably fine to add the > is_chassis_resident(logical_port) check. > > > In addition, I'd suggest to add more clear comment to state the reason > > why setting reg1 and eth.src. For example: it is done to make sure when > > ARP request is triggered (in next stage), the ARP generated can have the > > eth.src = xxx and src IP as yyy, so that it can bypass the flow that > > redirect the ARP request to GW node ... > > > > Thanks, > > Han > > > >> + */ > >> if (distributed) { > >> + ds_clear(&match); > >> + ds_clear(&actions); > >> + ds_put_format(&match, > >> + "ip%s.src == %s && > > is_chassis_resident(\"%s\")", > >> + is_v6 ? "6" : "4", nat->logical_ip, > >> + nat->logical_port); > >> + ds_put_format(&actions, "eth.src = %s; %sreg1 = %s; > > next;", > >> + nat->external_mac, is_v6 ? "xx" : "", > >> + nat->external_ip); > >> + ovn_lflow_add(lflows, od, S_ROUTER_IN_GW_REDIRECT, 150, > >> + ds_cstr(&match), ds_cstr(&actions)); > >> + > >> ds_clear(&match); > >> ds_put_format(&match, "ip%s.src == %s && outport == %s", > >> is_v6 ? "6" : "4", > >> diff --git a/tests/ovn.at <http://ovn.at> b/tests/ovn.at <http://ovn.at> > >> index 8fa1a7e1b..15b40ca1e 100644 > >> --- a/tests/ovn.at <http://ovn.at> > >> +++ b/tests/ovn.at <http://ovn.at> > >> @@ -14943,9 +14943,14 @@ ovs-vsctl -- add-port br-int hv2-vif1 -- \ > >> set interface hv2-vif1 external-ids:iface-id=sw1-p0 \ > >> options:tx_pcap=hv2/vif1-tx.pcap \ > >> options:rxq_pcap=hv2/vif1-rx.pcap \ > >> - ofport-request=1 > >> + ofport-request=2 > >> +ovs-vsctl -- add-port br-int hv2-vif2 -- \ > >> + set interface hv2-vif2 external-ids:iface-id=sw0-p1 \ > >> + options:tx_pcap=hv2/vif2-tx.pcap \ > >> + options:rxq_pcap=hv2/vif2-rx.pcap \ > >> + ofport-request=3 > >> > >> -ovn-nbctl create Logical_Router name=lr0 options:chassis=hv1 > >> +ovn-nbctl create Logical_Router name=lr0 > >> ovn-nbctl ls-add sw0 > >> ovn-nbctl ls-add sw1 > >> > >> @@ -14954,13 +14959,16 @@ ovn-nbctl lsp-add sw0 rp-sw0 -- set > > Logical_Switch_Port rp-sw0 \ > >> type=router options:router-port=sw0 \ > >> -- lsp-set-addresses rp-sw0 router > >> > >> -ovn-nbctl lrp-add lr0 sw1 00:00:02:01:02:03 172.16.1.1/24 > > <http://172.16.1.1/24> 2002:0:0:0:0:0:0:1/64 > >> +ovn-nbctl lrp-add lr0 sw1 00:00:02:01:02:03 172.16.1.1/24 > > <http://172.16.1.1/24> 2002:0:0:0:0:0:0:1/64 \ > >> + -- set Logical_Router_Port sw1 options:redirect-chassis="hv2" > >> ovn-nbctl lsp-add sw1 rp-sw1 -- set Logical_Switch_Port rp-sw1 \ > >> type=router options:router-port=sw1 \ > >> -- lsp-set-addresses rp-sw1 router > >> > >> ovn-nbctl lsp-add sw0 sw0-p0 \ > >> -- lsp-set-addresses sw0-p0 "f0:00:00:01:02:03 192.168.1.2 2001::2" > >> +ovn-nbctl lsp-add sw0 sw0-p1 \ > >> + -- lsp-set-addresses sw0-p1 "f0:00:00:11:02:03 192.168.1.3 2001::3" > >> > >> ovn-nbctl lsp-add sw1 sw1-p0 \ > >> -- lsp-set-addresses sw1-p0 unknown > >> @@ -15006,6 +15014,20 @@ send_na 2 1 $dst_mac $router_mac1 $dst_ip6 > > $router_ip6 > >> > >> OVN_CHECK_PACKETS([hv2/vif1-tx.pcap], [expected]) > >> > >> +# Create FIP on sw0-p0, add a route on logical router pipeline and > >> +# ARP request for a unkwon destination is sent using FIP MAC/IP > >> +ovn-nbctl lr-nat-add lr0 dnat_and_snat 172.16.1.2 192.168.1.3 sw0-p1 > > f0:00:00:01:02:04 > >> +ovn-nbctl lr-route-add lr0 172.16.2.0/24 <http://172.16.2.0/24> > > 172.16.1.11 > >> + > >> +dst_ip=$(ip_to_hex 172 16 2 10) > >> +fip_ip=$(ip_to_hex 172 16 1 2) > >> +src_ip=$(ip_to_hex 192 168 1 3) > >> +gw_router=$(ip_to_hex 172 16 1 11) > >> +send_icmp_packet 2 2 f00000110203 $router_mac0 $src_ip $dst_ip 0000 $data > >> +echo $(get_arp_req f00000010204 $fip_ip $gw_router) >> expected > >> + > >> +OVN_CHECK_PACKETS([hv2/vif1-tx.pcap], [expected]) > >> + > >> OVN_CLEANUP([hv1],[hv2]) > >> AT_CLEANUP > >> > >> diff --git a/tests/system-ovn.at <http://system-ovn.at> > > b/tests/system-ovn.at <http://system-ovn.at> > >> index 9ae6c6b1f..99a0ee07b 100644 > >> --- a/tests/system-ovn.at <http://system-ovn.at> > >> +++ b/tests/system-ovn.at <http://system-ovn.at> > >> @@ -2747,6 +2747,17 @@ ADD_VETH(alice1, alice1, br-int, "172.16.1.2/24 > > <http://172.16.1.2/24>", "f0:00:00:01:02:05", \ > >> ovn-nbctl lsp-add alice alice1 \ > >> -- lsp-set-addresses alice1 "f0:00:00:01:02:05 172.16.1.2" > >> > >> +# Add external network > >> +ADD_NAMESPACES(ext-net) > >> +ip link add alice-ext netns alice1 type veth peer name ext-veth netns > > ext-net > >> +ip -n ext-net link set dev ext-veth up > >> +ip -n ext-net addr add 10.0.0.1/24 <http://10.0.0.1/24> dev ext-veth > >> +ip -n ext-net route add default via 10.0.0.2 > >> + > >> +ip -n alice1 link set dev alice-ext up > >> +ip -n alice1 addr add 10.0.0.2/24 <http://10.0.0.2/24> dev alice-ext > >> +ip netns exec alice1 sysctl -w net.ipv4.conf.all.forwarding=1 > >> + > > Shouldn't we use NS_CHECK_EXEC() instead of "ip -n"/"ip netns"?
Hi Dumitru, thx for the review. I guess we can use NS_EXEC instead, right? Regards, Lorenzo > > Regards, > Dumitru > > >> # Add DNAT rules > >> AT_CHECK([ovn-nbctl lr-nat-add R1 dnat_and_snat 172.16.1.3 > > 192.168.1.2 foo1 00:00:02:02:03:04]) > >> AT_CHECK([ovn-nbctl lr-nat-add R1 dnat_and_snat 172.16.1.4 > > 192.168.1.3 foo2 00:00:02:02:03:05]) > >> @@ -2754,6 +2765,9 @@ AT_CHECK([ovn-nbctl lr-nat-add R1 dnat_and_snat > > 172.16.1.4 192.168.1.3 foo2 00:0 > >> # Add a SNAT rule > >> AT_CHECK([ovn-nbctl lr-nat-add R1 snat 172.16.1.1 192.168.0.0/16 > > <http://192.168.0.0/16>]) > >> > >> +# Add default route to ext-net > >> +AT_CHECK([ovn-nbctl lr-route-add R1 10.0.0.0/24 <http://10.0.0.0/24> > > 172.16.1.2]) > >> + > >> ovn-nbctl --wait=hv sync > >> OVS_WAIT_UNTIL([ovs-ofctl dump-flows br-int | grep > > 'nat(src=172.16.1.1)']) > >> > >> @@ -2797,6 +2811,20 @@ sed -e 's/zone=[[0-9]]*/zone=<cleared>/'], [0], > > [dnl > >> > > > > icmp,orig=(src=192.168.2.2,dst=172.16.1.2,id=<cleared>,type=8,code=0),reply=(src=172.16.1.2,dst=172.16.1.1,id=<cleared>,type=0,code=0),zone=<cleared> > >> ]) > >> > >> +# Try to ping external network > >> +NS_CHECK_EXEC([ext-net], [tcpdump -n -c 3 -i ext-veth dst 172.16.1.3 > > and icmp > ext-net.pcap &]) > >> +sleep 1 > >> +AT_CHECK([ovn-nbctl lr-nat-del R1 snat]) > >> +NS_CHECK_EXEC([foo1], [ping -q -c 3 -i 0.3 -w 2 10.0.0.1 | > > FORMAT_PING], \ > >> +[0], [dnl > >> +3 packets transmitted, 3 received, 0% packet loss, time 0ms > >> +]) > >> + > >> +OVS_WAIT_UNTIL([ > >> + total_pkts=$(cat ext-net.pcap | wc -l) > >> + test "${total_pkts}" = "3" > >> +]) > >> + > >> OVS_APP_EXIT_AND_WAIT([ovn-controller]) > >> > >> as ovn-sb > >> -- > >> 2.26.2 > >> > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
