On Tue, May 26, 2020 at 9:16 PM Dumitru Ceara <dce...@redhat.com> wrote:
> On 5/26/20 5:39 PM, Lorenzo Bianconi wrote: > >> 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? > > > > I guess we could but we don't use NS_EXEC() anywhere in the > system-ovn.at tests. Also I don't see any harm in the additional checks > done by NS_CHECK_EXEC(). > I'm not sure in this particular case, but in this patch [1] which I submitted a while back, I couldn't use NS_CHECK_EXEC() and hence used "ip netns " directly. I wanted to do "nc 10.0.0.4 84 2> r" using NS_CHECK_EXEC() but it failed for some reason. [1] - https://github.com/ovn-org/ovn/commit/f792b1a00b439a949e3b7aae4951f8513340c1a1#diff-eaa18a11c5b23c901f9a4dd849c58db6R3802 > Thanks, > Dumitru > > > 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 > > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
