[ovs-dev] [PATCH ovn 2/2] northd: manage ARP request locally for FIP traffic

Dumitru Ceara dceara at redhat.com
Tue May 26 15:46:28 UTC 2020 

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.bianconi at redhat.com <mailto:lorenzo.bianconi at 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.bianconi at redhat.com
>>> <mailto:lorenzo.bianconi at 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().

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

More information about the dev mailing list