[ovs-dev,v5,1/4,ovn] OVN: Do not replace router port mac on gateway chassis.
diff mbox series

Message ID 1565898039-26467-2-git-send-email-ankur.sharma@nutanix.com
State New
Headers show
Series
  • OVN: Vlan backed DVR, enable N-S packet flow
Related show

Commit Message

Ankur Sharma Aug. 15, 2019, 7:40 p.m. UTC
With 795d7f24ce0e2ed5454e193a059451d237289542 we have added
support for E-W routing on vlan backed networks by replacing
router port macs with chassis macs.

This replacement of router port mac need NOT be done on
gateway chassis for following reasons:

a. For N-S traffic, gateway chassis will respond to ARP
for the router port (to which it is attached) and
traffic will be using router port mac as destination mac.

b. Chassis redirect port is a centralized version of distributed
router port, hence we need not replace its mac with chassis mac
on the resident chassis.

This patch addresses the same.

Signed-off-by: Ankur Sharma <ankur.sharma@nutanix.com>
---
 controller/lport.c    |  20 ++++
 controller/lport.h    |   6 +
 controller/physical.c |  18 ++-
 controller/pinctrl.c  |  20 +---
 tests/ovn.at          | 313 ++++++++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 356 insertions(+), 21 deletions(-)

Comments

Numan Siddique Aug. 16, 2019, 12:45 p.m. UTC | #1
On Fri, Aug 16, 2019 at 1:11 AM Ankur Sharma <ankur.sharma@nutanix.com>
wrote:

> With 795d7f24ce0e2ed5454e193a059451d237289542 we have added
> support for E-W routing on vlan backed networks by replacing
> router port macs with chassis macs.
>
> This replacement of router port mac need NOT be done on
> gateway chassis for following reasons:
>
> a. For N-S traffic, gateway chassis will respond to ARP
> for the router port (to which it is attached) and
> traffic will be using router port mac as destination mac.
>
> b. Chassis redirect port is a centralized version of distributed
> router port, hence we need not replace its mac with chassis mac
> on the resident chassis.
>
> This patch addresses the same.
>
> Signed-off-by: Ankur Sharma <ankur.sharma@nutanix.com>
> ---
>  controller/lport.c    |  20 ++++
>  controller/lport.h    |   6 +
>  controller/physical.c |  18 ++-
>  controller/pinctrl.c  |  20 +---
>  tests/ovn.at          | 313
> ++++++++++++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 356 insertions(+), 21 deletions(-)
>
> diff --git a/controller/lport.c b/controller/lport.c
> index 792c825..478fcfd 100644
> --- a/controller/lport.c
> +++ b/controller/lport.c
> @@ -17,6 +17,7 @@
>
>  #include "lib/sset.h"
>  #include "lport.h"
> +#include "ha-chassis.h"
>  #include "hash.h"
>  #include "openvswitch/vlog.h"
>  #include "lib/ovn-sb-idl.h"
> @@ -64,6 +65,25 @@ lport_lookup_by_key(struct ovsdb_idl_index
> *sbrec_datapath_binding_by_key,
>      return retval;
>  }
>
> +bool
> +lport_is_chassis_resident(struct ovsdb_idl_index
> *sbrec_port_binding_by_name,
> +                          const struct sbrec_chassis *chassis,
> +                          const struct sset *active_tunnels,
> +                          const char *port_name)
> +{
> +    const struct sbrec_port_binding *pb
> +        = lport_lookup_by_name(sbrec_port_binding_by_name, port_name);
> +    if (!pb || !pb->chassis) {
> +        return false;
> +    }
> +    if (strcmp(pb->type, "chassisredirect")) {
> +        return pb->chassis == chassis;
> +    } else {
> +        return ha_chassis_group_is_active(pb->ha_chassis_group,
> +                                          active_tunnels, chassis);
> +    }
> +}
> +
>  const struct sbrec_datapath_binding *
>  datapath_lookup_by_key(struct ovsdb_idl_index
> *sbrec_datapath_binding_by_key,
>                         uint64_t dp_key)
> diff --git a/controller/lport.h b/controller/lport.h
> index 2d4bb71..345efc1 100644
> --- a/controller/lport.h
> +++ b/controller/lport.h
> @@ -23,6 +23,7 @@ struct sbrec_chassis;
>  struct sbrec_datapath_binding;
>  struct sbrec_multicast_group;
>  struct sbrec_port_binding;
> +struct sset;
>
>
>  /* Database indexes.
> @@ -48,5 +49,10 @@ const struct sbrec_datapath_binding
> *datapath_lookup_by_key(
>  const struct sbrec_multicast_group *mcgroup_lookup_by_dp_name(
>      struct ovsdb_idl_index *sbrec_multicast_group_by_name_datapath,
>      const struct sbrec_datapath_binding *, const char *name);
> +bool
> +lport_is_chassis_resident(struct ovsdb_idl_index
> *sbrec_port_binding_by_name,
> +                          const struct sbrec_chassis *chassis,
> +                          const struct sset *active_tunnels,
> +                          const char *port_name);
>
>  #endif /* controller/lport.h */
> diff --git a/controller/physical.c b/controller/physical.c
> index a05962b..5068785 100644
> --- a/controller/physical.c
> +++ b/controller/physical.c
> @@ -228,9 +228,12 @@ get_zone_ids(const struct sbrec_port_binding *binding,
>  }
>
>  static void
> -put_replace_router_port_mac_flows(const struct
> +put_replace_router_port_mac_flows(struct ovsdb_idl_index
> +                                  *sbrec_port_binding_by_name,
> +                                  const struct
>                                    sbrec_port_binding *localnet_port,
>                                    const struct sbrec_chassis *chassis,
> +                                  const struct sset *active_tunnels,
>                                    const struct hmap *local_datapaths,
>                                    struct ofpbuf *ofpacts_p,
>                                    ofp_port_t ofport,
> @@ -270,6 +273,16 @@ put_replace_router_port_mac_flows(const struct
>          struct eth_addr router_port_mac;
>          struct match match;
>          struct ofpact_mac *replace_mac;
> +        char *cr_peer_name = xasprintf("cr-%s",
> rport_binding->logical_port);
> +        if (lport_is_chassis_resident(sbrec_port_binding_by_name,
> +                                      chassis, active_tunnels,
> +                                      cr_peer_name)) {
> +            /* If a router port's chassisredirect port is
> +             * resident on this chassis, then we need not do mac replace.
> */
> +            free(cr_peer_name);
> +            continue;
> +        }
> +        free(cr_peer_name);
>
>          /* Table 65, priority 150.
>           * =======================
> @@ -787,7 +800,8 @@ consider_port_binding(struct ovsdb_idl_index
> *sbrec_port_binding_by_name,
>                          &match, ofpacts_p, &binding->header_.uuid);
>
>          if (!strcmp(binding->type, "localnet")) {
> -            put_replace_router_port_mac_flows(binding, chassis,
> +            put_replace_router_port_mac_flows(sbrec_port_binding_by_name,
> +                                              binding, chassis,
> active_tunnels,
>                                                local_datapaths, ofpacts_p,
>                                                ofport, flow_table);
>          }
> diff --git a/controller/pinctrl.c b/controller/pinctrl.c
> index f27718f..e8abe0b 100644
> --- a/controller/pinctrl.c
> +++ b/controller/pinctrl.c
> @@ -3755,24 +3755,6 @@ get_localnet_vifs_l3gwports(
>      sbrec_port_binding_index_destroy_row(target);
>  }
>
> -static bool
> -pinctrl_is_chassis_resident(struct ovsdb_idl_index
> *sbrec_port_binding_by_name,
> -                            const struct sbrec_chassis *chassis,
> -                            const struct sset *active_tunnels,
> -                            const char *port_name)
> -{
> -    const struct sbrec_port_binding *pb
> -        = lport_lookup_by_name(sbrec_port_binding_by_name, port_name);
> -    if (!pb || !pb->chassis) {
> -        return false;
> -    }
> -    if (strcmp(pb->type, "chassisredirect")) {
> -        return pb->chassis == chassis;
> -    } else {
> -        return ha_chassis_group_is_active(pb->ha_chassis_group,
> -                                          active_tunnels, chassis);
> -    }
> -}
>
>  /* Extracts the mac, IPv4 and IPv6 addresses, and logical port from
>   * 'addresses' which should be of the format 'MAC [IP1 IP2 ..]
> @@ -3853,7 +3835,7 @@ consider_nat_address(struct ovsdb_idl_index
> *sbrec_port_binding_by_name,
>      char *lport = NULL;
>      if (!extract_addresses_with_port(nat_address, laddrs, &lport)
>          || (!lport && !strcmp(pb->type, "patch"))
> -        || (lport && !pinctrl_is_chassis_resident(
> +        || (lport && !lport_is_chassis_resident(
>                  sbrec_port_binding_by_name, chassis,
>                  active_tunnels, lport))) {
>          destroy_lport_addresses(laddrs);
> diff --git a/tests/ovn.at b/tests/ovn.at
> index 71eb390..1613c0b 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -29,6 +29,12 @@ m4_define([OVN_CHECK_PACKETS],
>    [ovn_check_packets__ "$1" "$2"
>     AT_CHECK([sort $rcv_text], [0], [expout])])
>
> +m4_define([OVN_CHECK_PACKETS_REMOVE_BROADCAST],
> +  [ovn_check_packets__ "$1" "$2"
> +   echo "received_text=$rcv_text"
> +   sed -i '/ffffffffffff/d' $rcv_text
> +   AT_CHECK([sort $rcv_text], [0], [expout])])
> +
>  AT_BANNER([OVN components])
>
>  AT_SETUP([ovn -- lexer])
> @@ -15009,3 +15015,310 @@ on_exit 'kill $(cat ovn-nbctl.pid)'
>  AT_CHECK([ovn-nbctl -u $sockfile show])
>
>  AT_CLEANUP
> +
> +
> +AT_SETUP([ovn -- 2 HVs, 2 lports/HV, localnet ports, DVR N-S ARP
> handling])
> +ovn_start
> +
> +# In this test cases we create 3 switches, all connected to same
> +# physical network (through br-phys on each HV). LS1 and LS2 have
> +# 1 VIF each. Each HV has 1 VIF port. The first digit
> +# of VIF port name indicates the hypervisor it is bound to, e.g.
> +# lp23 means VIF 3 on hv2.
> +#
> +# All the switches are connected to a logical router "router".
> +#
> +# Each switch's VLAN tag and their logical switch ports are:
> +#   - ls1:
> +#       - tagged with VLAN 101
> +#       - ports: lp11
> +#   - ls2:
> +#       - tagged with VLAN 201
> +#       - ports: lp22
> +#   - ls-underlay:
> +#       - tagged with VLAN 1000
> +# Note: a localnet port is created for each switch to connect to
> +# physical network.
> +
> +for i in 1 2; do
> +    ls_name=ls$i
> +    ovn-nbctl ls-add $ls_name
> +    ln_port_name=ln$i
> +    if test $i -eq 1; then
> +        ovn-nbctl lsp-add $ls_name $ln_port_name "" 101
> +    elif test $i -eq 2; then
> +        ovn-nbctl lsp-add $ls_name $ln_port_name "" 201
> +    fi
> +    ovn-nbctl lsp-set-addresses $ln_port_name unknown
> +    ovn-nbctl lsp-set-type $ln_port_name localnet
> +    ovn-nbctl lsp-set-options $ln_port_name network_name=phys
> +done
> +
> +# lsp_to_ls LSP
> +#
> +# Prints the name of the logical switch that contains LSP.
> +lsp_to_ls () {
> +    case $1 in dnl (
> +        lp?[[11]]) echo ls1 ;; dnl (
> +        lp?[[12]]) echo ls2 ;; dnl (
> +        *) AT_FAIL_IF([:]) ;;
> +    esac
> +}
> +
> +vif_to_hv () {
> +    case $1 in dnl (
> +        vif[[1]]?) echo hv1 ;; dnl (
> +        vif[[2]]?) echo hv2 ;; dnl (
> +        vif?[[north]]?) echo hv4 ;; dnl (
> +        *) AT_FAIL_IF([:]) ;;
> +    esac
> +}
> +
> +ip_to_hex() {
> +       printf "%02x%02x%02x%02x" "$@"
> +}
> +
> +net_add n1
> +for i in 1 2; do
> +    sim_add hv$i
> +    as hv$i
> +    ovs-vsctl add-br br-phys
> +    ovs-vsctl set open . external-ids:ovn-bridge-mappings=phys:br-phys
> +    ovs-vsctl set open .
> external-ids:ovn-chassis-mac-mappings="phys:aa:bb:cc:dd:ee:$i$i"
> +    ovn_attach n1 br-phys 192.168.0.$i
> +
> +    ovs-vsctl add-port br-int vif$i$i -- \
> +        set Interface vif$i$i external-ids:iface-id=lp$i$i \
> +                              options:tx_pcap=hv$i/vif$i$i-tx.pcap \
> +                              options:rxq_pcap=hv$i/vif$i$i-rx.pcap \
> +                              ofport-request=$i$i
> +
> +    lsp_name=lp$i$i
> +    ls_name=$(lsp_to_ls $lsp_name)
> +
> +    ovn-nbctl lsp-add $ls_name $lsp_name
> +    ovn-nbctl lsp-set-addresses $lsp_name "f0:00:00:00:00:$i$i
> 192.168.$i.$i"
> +    ovn-nbctl lsp-set-port-security $lsp_name f0:00:00:00:00:$i$i
> +
> +    OVS_WAIT_UNTIL([test x`ovn-nbctl lsp-get-up $lsp_name` = xup])
> +
> +done
> +
> +ovn-nbctl ls-add ls-underlay
> +ovn-nbctl lsp-add ls-underlay ln3 "" 1000
> +ovn-nbctl lsp-set-addresses ln3 unknown
> +ovn-nbctl lsp-set-type ln3 localnet
> +ovn-nbctl lsp-set-options ln3 network_name=phys
> +
> +ovn-nbctl ls-add ls-north
> +ovn-nbctl lsp-add ls-north ln4 "" 1000
> +ovn-nbctl lsp-set-addresses ln4 unknown
> +ovn-nbctl lsp-set-type ln4 localnet
> +ovn-nbctl lsp-set-options ln4 network_name=phys
> +
> +# Add a VM on ls-north
> +ovn-nbctl lsp-add ls-north lp-north
> +ovn-nbctl lsp-set-addresses lp-north "f0:f0:00:00:00:11 172.31.0.10"
> +ovn-nbctl lsp-set-port-security lp-north f0:f0:00:00:00:11
> +
> +# Add 3rd hypervisor
> +sim_add hv3
> +as hv3 ovs-vsctl add-br br-phys
> +as hv3 ovs-vsctl set open . external-ids:ovn-bridge-mappings=phys:br-phys
> +as hv3 ovs-vsctl set open .
> external-ids:ovn-chassis-mac-mappings="phys:aa:bb:cc:dd:ee:33"
> +as hv3 ovn_attach n1 br-phys 192.168.0.3
> +
> +# Add 4th hypervisor
> +sim_add hv4
> +as hv4 ovs-vsctl add-br br-phys
> +as hv4 ovs-vsctl set open . external-ids:ovn-bridge-mappings=phys:br-phys
> +as hv4 ovs-vsctl set open .
> external-ids:ovn-chassis-mac-mappings="phys:aa:bb:cc:dd:ee:44"
> +as hv4 ovn_attach n1 br-phys 192.168.0.4
> +
> +as hv4 ovs-vsctl add-port br-int vif-north -- \
> +        set Interface vif-north external-ids:iface-id=lp-north \
> +                              options:tx_pcap=hv4/vif-north-tx.pcap \
> +                              options:rxq_pcap=hv4/vif-north-rx.pcap \
> +                              ofport-request=44
> +
> +ovn-nbctl lr-add router
> +ovn-nbctl lrp-add router router-to-ls1 00:00:01:01:02:03 192.168.1.3/24
> +ovn-nbctl <http://192.168.1.3/24+ovn-nbctl> lrp-add router router-to-ls2
> 00:00:01:01:02:05 192.168.2.3/24
> +ovn-nbctl <http://192.168.2.3/24+ovn-nbctl> lrp-add router
> router-to-underlay 00:00:01:01:02:07 172.31.0.1/24
> +
> +ovn-nbctl lsp-add ls1 ls1-to-router -- set Logical_Switch_Port
> ls1-to-router type=router \
> +          options:router-port=router-to-ls1 -- lsp-set-addresses
> ls1-to-router router
> +ovn-nbctl lsp-add ls2 ls2-to-router -- set Logical_Switch_Port
> ls2-to-router type=router \
> +          options:router-port=router-to-ls2 -- lsp-set-addresses
> ls2-to-router router
> +ovn-nbctl lsp-add ls-underlay underlay-to-router -- set
> Logical_Switch_Port \
> +                              underlay-to-router type=router \
> +                              options:router-port=router-to-underlay \
> +                              -- lsp-set-addresses underlay-to-router
> router
> +
> +
> +OVN_POPULATE_ARP
> +
> +# lsp_to_ls LSP
> +#
> +# Prints the name of the logical switch that contains LSP.
> +lsp_to_ls () {
> +    case $1 in dnl (
> +        lp?[[11]]) echo ls1 ;; dnl (
> +        lp?[[12]]) echo ls2 ;; dnl (
> +        *) AT_FAIL_IF([:]) ;;
> +    esac
> +}
> +
> +vif_to_ls () {
> +    case $1 in dnl (
> +        vif?[[11]]) echo ls1 ;; dnl (
> +        vif?[[12]]) echo ls2 ;; dnl (
> +        vif-north) echo ls-north ;; dnl (
> +        *) AT_FAIL_IF([:]) ;;
> +    esac
> +}
> +
> +hv_to_num () {
> +    case $1 in dnl (
> +        hv1) echo 1 ;; dnl (
> +        hv2) echo 2 ;; dnl (
> +        hv3) echo 3 ;; dnl (
> +        hv4) echo 4 ;; dnl (
> +        *) AT_FAIL_IF([:]) ;;
> +    esac
> +}
> +
> +vif_to_num () {
> +    case $1 in dnl (
> +        vif22) echo 22 ;; dnl (
> +        vif21) echo 21 ;; dnl (
> +        vif11) echo 11 ;; dnl (
> +        *) AT_FAIL_IF([:]) ;;
> +    esac
> +}
> +
> +vif_to_hv () {
> +    case $1 in dnl (
> +        vif[[1]]?) echo hv1 ;; dnl (
> +        vif[[2]]?) echo hv2 ;; dnl (
> +        vif-north) echo hv4 ;; dnl (
> +        *) AT_FAIL_IF([:]) ;;
> +    esac
> +}
> +
> +vif_to_lrp () {
> +    echo router-to-`vif_to_ls $1`
> +}
> +
> +ip_to_hex() {
> +       printf "%02x%02x%02x%02x" "$@"
> +}
> +
> +# Dump a bunch of info helpful for debugging if there's a failure.
> +
> +echo "------ OVN dump ------"
> +ovn-nbctl show
> +ovn-sbctl show
> +ovn-sbctl list port_binding
> +ovn-sbctl list mac_binding
> +
> +echo "------ hv1 dump ------"
> +as hv1 ovs-vsctl show
> +as hv1 ovs-vsctl list Open_Vswitch
> +
> +echo "------ hv2 dump ------"
> +as hv2 ovs-vsctl show
> +as hv2 ovs-vsctl list Open_Vswitch
> +
> +echo "------ hv3 dump ------"
> +as hv3 ovs-vsctl show
> +as hv3 ovs-vsctl list Open_Vswitch
> +
> +echo "------ hv4 dump ------"
> +as hv4 ovs-vsctl show
> +as hv4 ovs-vsctl list Open_Vswitch
> +
> +# test_arp INPORT SHA SPA TPA [REPLY_HA]
> +#
> +# Causes a packet to be received on INPORT.  The packet is an ARP
> +# request with SHA, SPA, and TPA as specified.  If REPLY_HA is provided,
> then
> +# it should be the hardware address of the target to expect to receive in
> an
> +# ARP reply; otherwise no reply is expected.
> +#
> +# INPORT is an logical switch port number, e.g. 11 for vif11.
> +# SHA and REPLY_HA are each 12 hex digits.
> +# SPA and TPA are each 8 hex digits.
> +test_arp() {
> +    local inport=$1 sha=$2 spa=$3 tpa=$4 reply_ha=$5
> +    local
> request=ffffffffffff${sha}08060001080006040001${sha}${spa}ffffffffffff${tpa}
> +    hv=`vif_to_hv $inport`
> +    as $hv ovs-appctl netdev-dummy/receive $inport $request
> +
> +    if test X$reply_ha = X; then
> +        # Expect to receive the broadcast ARP on the other logical switch
> ports
> +        # if no reply is expected.
> +        local i j
> +        for i in 1 2 3; do
> +            for j in 1 2 3; do
> +                if test $i$j != $inport; then
> +                    echo $request >> $i$j.expected
> +                fi
> +            done
> +        done
> +    else
> +        # Expect to receive the reply, if any.
> +        local
> reply=${sha}${reply_ha}08060001080006040002${reply_ha}${tpa}${sha}${spa}
> +        local
> reply_vid=${sha}${reply_ha}810003e808060001080006040002${reply_ha}${tpa}${sha}${spa}
> +        echo $reply_vid >> ${inport}_vid.expected
> +        echo $reply >> $inport.expected
> +    fi
> +}
> +
> +sip=`ip_to_hex 172 31 0 10`
> +tip=`ip_to_hex 172 31 0 1`
> +
> +# Set a hypervisor as gateway chassis, for router port 172.31.0.1
> +ovn-nbctl lrp-set-gateway-chassis router-to-underlay hv3
> +ovn-nbctl --wait=sb sync
> +sleep 2
> +
> +test_arp vif-north f0f000000011 $sip $tip 000001010207
> +
> +sleep 1
>

Hi Ankur,

Thanks for addressing most of the comments.
I see that the comment on sleep is not addressed.
Is it possible to address that ? -
https://mail.openvswitch.org/pipermail/ovs-dev/2019-August/361380.html

In the patch 4, there is sleep 5. I would suggest to remove this sleep 5
and think of a better way.

Other than the comments related to the tests, the series LGTM.

Thanks
Numan


> +
> +# Confirm that vif-north gets a single ARP reply
> +OVN_CHECK_PACKETS_REMOVE_BROADCAST([hv4/vif-north-tx.pcap],
> [vif-north.expected])
> +
> +# Confirm that only redirect chassis allowed arp resolution.
> +OVN_CHECK_PACKETS_REMOVE_BROADCAST([hv3/br-phys_n1-tx.pcap],
> [vif-north_vid.expected])
> +AT_CHECK([grep 000001010207 hv3/br-phys_n1-tx.packets | wc -l], [0], [[1
> +]])
> +
> +# Confirm that other OVN chassis did not generate ARP reply.
> +$PYTHON "$top_srcdir/utilities/ovs-pcap.in" hv1/br-phys_n1-tx.pcap >
> hv1/br-phys_n1-tx.packets
> +$PYTHON "$top_srcdir/utilities/ovs-pcap.in" hv2/br-phys_n1-tx.pcap >
> hv2/br-phys_n1-tx.packets
> +
> +AT_CHECK([grep 000001010207 hv1/br-phys_n1-tx.packets | wc -l], [0], [[0
> +]])
> +AT_CHECK([grep 000001010207 hv2/br-phys_n1-tx.packets | wc -l], [0], [[0
> +]])
> +
> +echo "----------- Post Traffic hv1 dump -----------"
> +as hv1 ovs-ofctl -O OpenFlow13 dump-flows br-int
> +as hv1 ovs-appctl fdb/show br-phys
> +
> +echo "----------- Post Traffic hv2 dump -----------"
> +as hv2 ovs-ofctl -O OpenFlow13 dump-flows br-int
> +as hv2 ovs-appctl fdb/show br-phys
> +
> +echo "----------- Post Traffic hv3 dump -----------"
> +as hv3 ovs-ofctl -O OpenFlow13 dump-flows br-int
> +as hv3 ovs-appctl fdb/show br-phys
> +
> +echo "----------- Post Traffic hv4 dump -----------"
> +as hv4 ovs-ofctl -O OpenFlow13 dump-flows br-int
> +as hv4 ovs-appctl fdb/show br-phys
> +
> +OVN_CLEANUP([hv1],[hv2],[hv3],[hv4])
> +
> +AT_CLEANUP
> --
> 1.8.3.1
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
Ankur Sharma Aug. 17, 2019, 12:37 a.m. UTC | #2
Hi Numan,

My bad, I missed the sleep related comments.
Fixed the same in v6, please take a look.

Appreciate your help.

Regards,
Ankur

From: Numan Siddique <nusiddiq@redhat.com>
Sent: Friday, August 16, 2019 5:46 AM
To: Ankur Sharma <ankur.sharma@nutanix.com>
Cc: ovs-dev@openvswitch.org
Subject: Re: [ovs-dev] [PATCH v5 1/4 ovn] OVN: Do not replace router port mac on gateway chassis.



On Fri, Aug 16, 2019 at 1:11 AM Ankur Sharma <ankur.sharma@nutanix.com<mailto:ankur.sharma@nutanix.com>> wrote:
With 795d7f24ce0e2ed5454e193a059451d237289542 we have added
support for E-W routing on vlan backed networks by replacing
router port macs with chassis macs.

This replacement of router port mac need NOT be done on
gateway chassis for following reasons:

a. For N-S traffic, gateway chassis will respond to ARP
for the router port (to which it is attached) and
traffic will be using router port mac as destination mac.

b. Chassis redirect port is a centralized version of distributed
router port, hence we need not replace its mac with chassis mac
on the resident chassis.

This patch addresses the same.

Signed-off-by: Ankur Sharma <ankur.sharma@nutanix.com<mailto:ankur.sharma@nutanix.com>>
---
 controller/lport.c    |  20 ++++
 controller/lport.h    |   6 +
 controller/physical.c |  18 ++-
 controller/pinctrl.c  |  20 +---
 tests/ovn.at [ovn.at]<https://urldefense.proofpoint.com/v2/url?u=http-3A__ovn.at&d=DwMFaQ&c=s883GpUCOChKOHiocYtGcg&r=mZwX9gFQgeJHzTg-68aCJgsODyUEVsHGFOfL90J6MJY&m=8lRHQF9NAseA2tKXK0ON2WZL5FFfm-oWD7DK0rjdjAw&s=w3LIeruJk1CnRw6SHnGnzwMdJnPWOwznG2nFPkaQM0E&e=>          | 313 ++++++++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 356 insertions(+), 21 deletions(-)

diff --git a/controller/lport.c b/controller/lport.c
index 792c825..478fcfd 100644
--- a/controller/lport.c
+++ b/controller/lport.c
@@ -17,6 +17,7 @@

 #include "lib/sset.h"
 #include "lport.h"
+#include "ha-chassis.h"
 #include "hash.h"
 #include "openvswitch/vlog.h"
 #include "lib/ovn-sb-idl.h"
@@ -64,6 +65,25 @@ lport_lookup_by_key(struct ovsdb_idl_index *sbrec_datapath_binding_by_key,
     return retval;
 }

+bool
+lport_is_chassis_resident(struct ovsdb_idl_index *sbrec_port_binding_by_name,
+                          const struct sbrec_chassis *chassis,
+                          const struct sset *active_tunnels,
+                          const char *port_name)
+{
+    const struct sbrec_port_binding *pb
+        = lport_lookup_by_name(sbrec_port_binding_by_name, port_name);
+    if (!pb || !pb->chassis) {
+        return false;
+    }
+    if (strcmp(pb->type, "chassisredirect")) {
+        return pb->chassis == chassis;
+    } else {
+        return ha_chassis_group_is_active(pb->ha_chassis_group,
+                                          active_tunnels, chassis);
+    }
+}
+
 const struct sbrec_datapath_binding *
 datapath_lookup_by_key(struct ovsdb_idl_index *sbrec_datapath_binding_by_key,
                        uint64_t dp_key)
diff --git a/controller/lport.h b/controller/lport.h
index 2d4bb71..345efc1 100644
--- a/controller/lport.h
+++ b/controller/lport.h
@@ -23,6 +23,7 @@ struct sbrec_chassis;
 struct sbrec_datapath_binding;
 struct sbrec_multicast_group;
 struct sbrec_port_binding;
+struct sset;


 /* Database indexes.
@@ -48,5 +49,10 @@ const struct sbrec_datapath_binding *datapath_lookup_by_key(
 const struct sbrec_multicast_group *mcgroup_lookup_by_dp_name(
     struct ovsdb_idl_index *sbrec_multicast_group_by_name_datapath,
     const struct sbrec_datapath_binding *, const char *name);
+bool
+lport_is_chassis_resident(struct ovsdb_idl_index *sbrec_port_binding_by_name,
+                          const struct sbrec_chassis *chassis,
+                          const struct sset *active_tunnels,
+                          const char *port_name);

 #endif /* controller/lport.h */
diff --git a/controller/physical.c b/controller/physical.c
index a05962b..5068785 100644
--- a/controller/physical.c
+++ b/controller/physical.c
@@ -228,9 +228,12 @@ get_zone_ids(const struct sbrec_port_binding *binding,
 }

 static void
-put_replace_router_port_mac_flows(const struct
+put_replace_router_port_mac_flows(struct ovsdb_idl_index
+                                  *sbrec_port_binding_by_name,
+                                  const struct
                                   sbrec_port_binding *localnet_port,
                                   const struct sbrec_chassis *chassis,
+                                  const struct sset *active_tunnels,
                                   const struct hmap *local_datapaths,
                                   struct ofpbuf *ofpacts_p,
                                   ofp_port_t ofport,
@@ -270,6 +273,16 @@ put_replace_router_port_mac_flows(const struct
         struct eth_addr router_port_mac;
         struct match match;
         struct ofpact_mac *replace_mac;
+        char *cr_peer_name = xasprintf("cr-%s", rport_binding->logical_port);
+        if (lport_is_chassis_resident(sbrec_port_binding_by_name,
+                                      chassis, active_tunnels,
+                                      cr_peer_name)) {
+            /* If a router port's chassisredirect port is
+             * resident on this chassis, then we need not do mac replace. */
+            free(cr_peer_name);
+            continue;
+        }
+        free(cr_peer_name);

         /* Table 65, priority 150.
          * =======================
@@ -787,7 +800,8 @@ consider_port_binding(struct ovsdb_idl_index *sbrec_port_binding_by_name,
                         &match, ofpacts_p, &binding->header_.uuid);

         if (!strcmp(binding->type, "localnet")) {
-            put_replace_router_port_mac_flows(binding, chassis,
+            put_replace_router_port_mac_flows(sbrec_port_binding_by_name,
+                                              binding, chassis, active_tunnels,
                                               local_datapaths, ofpacts_p,
                                               ofport, flow_table);
         }
diff --git a/controller/pinctrl.c b/controller/pinctrl.c
index f27718f..e8abe0b 100644
--- a/controller/pinctrl.c
+++ b/controller/pinctrl.c
@@ -3755,24 +3755,6 @@ get_localnet_vifs_l3gwports(
     sbrec_port_binding_index_destroy_row(target);
 }

-static bool
-pinctrl_is_chassis_resident(struct ovsdb_idl_index *sbrec_port_binding_by_name,
-                            const struct sbrec_chassis *chassis,
-                            const struct sset *active_tunnels,
-                            const char *port_name)
-{
-    const struct sbrec_port_binding *pb
-        = lport_lookup_by_name(sbrec_port_binding_by_name, port_name);
-    if (!pb || !pb->chassis) {
-        return false;
-    }
-    if (strcmp(pb->type, "chassisredirect")) {
-        return pb->chassis == chassis;
-    } else {
-        return ha_chassis_group_is_active(pb->ha_chassis_group,
-                                          active_tunnels, chassis);
-    }
-}

 /* Extracts the mac, IPv4 and IPv6 addresses, and logical port from
  * 'addresses' which should be of the format 'MAC [IP1 IP2 ..]
@@ -3853,7 +3835,7 @@ consider_nat_address(struct ovsdb_idl_index *sbrec_port_binding_by_name,
     char *lport = NULL;
     if (!extract_addresses_with_port(nat_address, laddrs, &lport)
         || (!lport && !strcmp(pb->type, "patch"))
-        || (lport && !pinctrl_is_chassis_resident(
+        || (lport && !lport_is_chassis_resident(
                 sbrec_port_binding_by_name, chassis,
                 active_tunnels, lport))) {
         destroy_lport_addresses(laddrs);
diff --git a/tests/ovn.at [ovn.at]<https://urldefense.proofpoint.com/v2/url?u=http-3A__ovn.at&d=DwMFaQ&c=s883GpUCOChKOHiocYtGcg&r=mZwX9gFQgeJHzTg-68aCJgsODyUEVsHGFOfL90J6MJY&m=8lRHQF9NAseA2tKXK0ON2WZL5FFfm-oWD7DK0rjdjAw&s=w3LIeruJk1CnRw6SHnGnzwMdJnPWOwznG2nFPkaQM0E&e=> b/tests/ovn.at [ovn.at]<https://urldefense.proofpoint.com/v2/url?u=http-3A__ovn.at&d=DwMFaQ&c=s883GpUCOChKOHiocYtGcg&r=mZwX9gFQgeJHzTg-68aCJgsODyUEVsHGFOfL90J6MJY&m=8lRHQF9NAseA2tKXK0ON2WZL5FFfm-oWD7DK0rjdjAw&s=w3LIeruJk1CnRw6SHnGnzwMdJnPWOwznG2nFPkaQM0E&e=>
index 71eb390..1613c0b 100644
--- a/tests/ovn.at [ovn.at]<https://urldefense.proofpoint.com/v2/url?u=http-3A__ovn.at&d=DwMFaQ&c=s883GpUCOChKOHiocYtGcg&r=mZwX9gFQgeJHzTg-68aCJgsODyUEVsHGFOfL90J6MJY&m=8lRHQF9NAseA2tKXK0ON2WZL5FFfm-oWD7DK0rjdjAw&s=w3LIeruJk1CnRw6SHnGnzwMdJnPWOwznG2nFPkaQM0E&e=>
+++ b/tests/ovn.at [ovn.at]<https://urldefense.proofpoint.com/v2/url?u=http-3A__ovn.at&d=DwMFaQ&c=s883GpUCOChKOHiocYtGcg&r=mZwX9gFQgeJHzTg-68aCJgsODyUEVsHGFOfL90J6MJY&m=8lRHQF9NAseA2tKXK0ON2WZL5FFfm-oWD7DK0rjdjAw&s=w3LIeruJk1CnRw6SHnGnzwMdJnPWOwznG2nFPkaQM0E&e=>
@@ -29,6 +29,12 @@ m4_define([OVN_CHECK_PACKETS],
   [ovn_check_packets__ "$1" "$2"
    AT_CHECK([sort $rcv_text], [0], [expout])])

+m4_define([OVN_CHECK_PACKETS_REMOVE_BROADCAST],
+  [ovn_check_packets__ "$1" "$2"
+   echo "received_text=$rcv_text"
+   sed -i '/ffffffffffff/d' $rcv_text
+   AT_CHECK([sort $rcv_text], [0], [expout])])
+
 AT_BANNER([OVN components])

 AT_SETUP([ovn -- lexer])
@@ -15009,3 +15015,310 @@ on_exit 'kill $(cat ovn-nbctl.pid)'
 AT_CHECK([ovn-nbctl -u $sockfile show])

 AT_CLEANUP
+
+
+AT_SETUP([ovn -- 2 HVs, 2 lports/HV, localnet ports, DVR N-S ARP handling])
+ovn_start
+
+# In this test cases we create 3 switches, all connected to same
+# physical network (through br-phys on each HV). LS1 and LS2 have
+# 1 VIF each. Each HV has 1 VIF port. The first digit
+# of VIF port name indicates the hypervisor it is bound to, e.g.
+# lp23 means VIF 3 on hv2.
+#
+# All the switches are connected to a logical router "router".
+#
+# Each switch's VLAN tag and their logical switch ports are:
+#   - ls1:
+#       - tagged with VLAN 101
+#       - ports: lp11
+#   - ls2:
+#       - tagged with VLAN 201
+#       - ports: lp22
+#   - ls-underlay:
+#       - tagged with VLAN 1000
+# Note: a localnet port is created for each switch to connect to
+# physical network.
+
+for i in 1 2; do
+    ls_name=ls$i
+    ovn-nbctl ls-add $ls_name
+    ln_port_name=ln$i
+    if test $i -eq 1; then
+        ovn-nbctl lsp-add $ls_name $ln_port_name "" 101
+    elif test $i -eq 2; then
+        ovn-nbctl lsp-add $ls_name $ln_port_name "" 201
+    fi
+    ovn-nbctl lsp-set-addresses $ln_port_name unknown
+    ovn-nbctl lsp-set-type $ln_port_name localnet
+    ovn-nbctl lsp-set-options $ln_port_name network_name=phys
+done
+
+# lsp_to_ls LSP
+#
+# Prints the name of the logical switch that contains LSP.
+lsp_to_ls () {
+    case $1 in dnl (
+        lp?[[11]]) echo ls1 ;; dnl (
+        lp?[[12]]) echo ls2 ;; dnl (
+        *) AT_FAIL_IF([:]) ;;
+    esac
+}
+
+vif_to_hv () {
+    case $1 in dnl (
+        vif[[1]]?) echo hv1 ;; dnl (
+        vif[[2]]?) echo hv2 ;; dnl (
+        vif?[[north]]?) echo hv4 ;; dnl (
+        *) AT_FAIL_IF([:]) ;;
+    esac
+}
+
+ip_to_hex() {
+       printf "%02x%02x%02x%02x" "$@"
+}
+
+net_add n1
+for i in 1 2; do
+    sim_add hv$i
+    as hv$i
+    ovs-vsctl add-br br-phys
+    ovs-vsctl set open . external-ids:ovn-bridge-mappings=phys:br-phys
+    ovs-vsctl set open . external-ids:ovn-chassis-mac-mappings="phys:aa:bb:cc:dd:ee:$i$i"
+    ovn_attach n1 br-phys 192.168.0.$i
+
+    ovs-vsctl add-port br-int vif$i$i -- \
+        set Interface vif$i$i external-ids:iface-id=lp$i$i \
+                              options:tx_pcap=hv$i/vif$i$i-tx.pcap \
+                              options:rxq_pcap=hv$i/vif$i$i-rx.pcap \
+                              ofport-request=$i$i
+
+    lsp_name=lp$i$i
+    ls_name=$(lsp_to_ls $lsp_name)
+
+    ovn-nbctl lsp-add $ls_name $lsp_name
+    ovn-nbctl lsp-set-addresses $lsp_name "f0:00:00:00:00:$i$i 192.168.$i.$i"
+    ovn-nbctl lsp-set-port-security $lsp_name f0:00:00:00:00:$i$i
+
+    OVS_WAIT_UNTIL([test x`ovn-nbctl lsp-get-up $lsp_name` = xup])
+
+done
+
+ovn-nbctl ls-add ls-underlay
+ovn-nbctl lsp-add ls-underlay ln3 "" 1000
+ovn-nbctl lsp-set-addresses ln3 unknown
+ovn-nbctl lsp-set-type ln3 localnet
+ovn-nbctl lsp-set-options ln3 network_name=phys
+
+ovn-nbctl ls-add ls-north
+ovn-nbctl lsp-add ls-north ln4 "" 1000
+ovn-nbctl lsp-set-addresses ln4 unknown
+ovn-nbctl lsp-set-type ln4 localnet
+ovn-nbctl lsp-set-options ln4 network_name=phys
+
+# Add a VM on ls-north
+ovn-nbctl lsp-add ls-north lp-north
+ovn-nbctl lsp-set-addresses lp-north "f0:f0:00:00:00:11 172.31.0.10"
+ovn-nbctl lsp-set-port-security lp-north f0:f0:00:00:00:11
+
+# Add 3rd hypervisor
+sim_add hv3
+as hv3 ovs-vsctl add-br br-phys
+as hv3 ovs-vsctl set open . external-ids:ovn-bridge-mappings=phys:br-phys
+as hv3 ovs-vsctl set open . external-ids:ovn-chassis-mac-mappings="phys:aa:bb:cc:dd:ee:33"
+as hv3 ovn_attach n1 br-phys 192.168.0.3
+
+# Add 4th hypervisor
+sim_add hv4
+as hv4 ovs-vsctl add-br br-phys
+as hv4 ovs-vsctl set open . external-ids:ovn-bridge-mappings=phys:br-phys
+as hv4 ovs-vsctl set open . external-ids:ovn-chassis-mac-mappings="phys:aa:bb:cc:dd:ee:44"
+as hv4 ovn_attach n1 br-phys 192.168.0.4
+
+as hv4 ovs-vsctl add-port br-int vif-north -- \
+        set Interface vif-north external-ids:iface-id=lp-north \
+                              options:tx_pcap=hv4/vif-north-tx.pcap \
+                              options:rxq_pcap=hv4/vif-north-rx.pcap \
+                              ofport-request=44
+
+ovn-nbctl lr-add router
+ovn-nbctl lrp-add router router-to-ls1 00:00:01:01:02:03 192.168.1.3/24
+ovn-nbctl [192.168.1.3]<https://urldefense.proofpoint.com/v2/url?u=http-3A__192.168.1.3_24-2Bovn-2Dnbctl&d=DwMFaQ&c=s883GpUCOChKOHiocYtGcg&r=mZwX9gFQgeJHzTg-68aCJgsODyUEVsHGFOfL90J6MJY&m=8lRHQF9NAseA2tKXK0ON2WZL5FFfm-oWD7DK0rjdjAw&s=6tHzia-2yI1lnnjpILts0EiAgvvpHEAfnRV5C10lsP0&e=> lrp-add router router-to-ls2 00:00:01:01:02:05 192.168.2.3/24
+ovn-nbctl [192.168.2.3]<https://urldefense.proofpoint.com/v2/url?u=http-3A__192.168.2.3_24-2Bovn-2Dnbctl&d=DwMFaQ&c=s883GpUCOChKOHiocYtGcg&r=mZwX9gFQgeJHzTg-68aCJgsODyUEVsHGFOfL90J6MJY&m=8lRHQF9NAseA2tKXK0ON2WZL5FFfm-oWD7DK0rjdjAw&s=WU5KjQQKv5XYKS_FW-g7fs0Kob6-j--q38Vr-PaMM6g&e=> lrp-add router router-to-underlay 00:00:01:01:02:07 172.31.0.1/24 [172.31.0.1]<https://urldefense.proofpoint.com/v2/url?u=http-3A__172.31.0.1_24&d=DwMFaQ&c=s883GpUCOChKOHiocYtGcg&r=mZwX9gFQgeJHzTg-68aCJgsODyUEVsHGFOfL90J6MJY&m=8lRHQF9NAseA2tKXK0ON2WZL5FFfm-oWD7DK0rjdjAw&s=EL6Rm6HUq4t0WDpco9GqQIS3YEd7Zj_KhbCH4M1D4Vw&e=>
+
+ovn-nbctl lsp-add ls1 ls1-to-router -- set Logical_Switch_Port ls1-to-router type=router \
+          options:router-port=router-to-ls1 -- lsp-set-addresses ls1-to-router router
+ovn-nbctl lsp-add ls2 ls2-to-router -- set Logical_Switch_Port ls2-to-router type=router \
+          options:router-port=router-to-ls2 -- lsp-set-addresses ls2-to-router router
+ovn-nbctl lsp-add ls-underlay underlay-to-router -- set Logical_Switch_Port \
+                              underlay-to-router type=router \
+                              options:router-port=router-to-underlay \
+                              -- lsp-set-addresses underlay-to-router router
+
+
+OVN_POPULATE_ARP
+
+# lsp_to_ls LSP
+#
+# Prints the name of the logical switch that contains LSP.
+lsp_to_ls () {
+    case $1 in dnl (
+        lp?[[11]]) echo ls1 ;; dnl (
+        lp?[[12]]) echo ls2 ;; dnl (
+        *) AT_FAIL_IF([:]) ;;
+    esac
+}
+
+vif_to_ls () {
+    case $1 in dnl (
+        vif?[[11]]) echo ls1 ;; dnl (
+        vif?[[12]]) echo ls2 ;; dnl (
+        vif-north) echo ls-north ;; dnl (
+        *) AT_FAIL_IF([:]) ;;
+    esac
+}
+
+hv_to_num () {
+    case $1 in dnl (
+        hv1) echo 1 ;; dnl (
+        hv2) echo 2 ;; dnl (
+        hv3) echo 3 ;; dnl (
+        hv4) echo 4 ;; dnl (
+        *) AT_FAIL_IF([:]) ;;
+    esac
+}
+
+vif_to_num () {
+    case $1 in dnl (
+        vif22) echo 22 ;; dnl (
+        vif21) echo 21 ;; dnl (
+        vif11) echo 11 ;; dnl (
+        *) AT_FAIL_IF([:]) ;;
+    esac
+}
+
+vif_to_hv () {
+    case $1 in dnl (
+        vif[[1]]?) echo hv1 ;; dnl (
+        vif[[2]]?) echo hv2 ;; dnl (
+        vif-north) echo hv4 ;; dnl (
+        *) AT_FAIL_IF([:]) ;;
+    esac
+}
+
+vif_to_lrp () {
+    echo router-to-`vif_to_ls $1`
+}
+
+ip_to_hex() {
+       printf "%02x%02x%02x%02x" "$@"
+}
+
+# Dump a bunch of info helpful for debugging if there's a failure.
+
+echo "------ OVN dump ------"
+ovn-nbctl show
+ovn-sbctl show
+ovn-sbctl list port_binding
+ovn-sbctl list mac_binding
+
+echo "------ hv1 dump ------"
+as hv1 ovs-vsctl show
+as hv1 ovs-vsctl list Open_Vswitch
+
+echo "------ hv2 dump ------"
+as hv2 ovs-vsctl show
+as hv2 ovs-vsctl list Open_Vswitch
+
+echo "------ hv3 dump ------"
+as hv3 ovs-vsctl show
+as hv3 ovs-vsctl list Open_Vswitch
+
+echo "------ hv4 dump ------"
+as hv4 ovs-vsctl show
+as hv4 ovs-vsctl list Open_Vswitch
+
+# test_arp INPORT SHA SPA TPA [REPLY_HA]
+#
+# Causes a packet to be received on INPORT.  The packet is an ARP
+# request with SHA, SPA, and TPA as specified.  If REPLY_HA is provided, then
+# it should be the hardware address of the target to expect to receive in an
+# ARP reply; otherwise no reply is expected.
+#
+# INPORT is an logical switch port number, e.g. 11 for vif11.
+# SHA and REPLY_HA are each 12 hex digits.
+# SPA and TPA are each 8 hex digits.
+test_arp() {
+    local inport=$1 sha=$2 spa=$3 tpa=$4 reply_ha=$5
+    local request=ffffffffffff${sha}08060001080006040001${sha}${spa}ffffffffffff${tpa}
+    hv=`vif_to_hv $inport`
+    as $hv ovs-appctl netdev-dummy/receive $inport $request
+
+    if test X$reply_ha = X; then
+        # Expect to receive the broadcast ARP on the other logical switch ports
+        # if no reply is expected.
+        local i j
+        for i in 1 2 3; do
+            for j in 1 2 3; do
+                if test $i$j != $inport; then
+                    echo $request >> $i$j.expected
+                fi
+            done
+        done
+    else
+        # Expect to receive the reply, if any.
+        local reply=${sha}${reply_ha}08060001080006040002${reply_ha}${tpa}${sha}${spa}
+        local reply_vid=${sha}${reply_ha}810003e808060001080006040002${reply_ha}${tpa}${sha}${spa}
+        echo $reply_vid >> ${inport}_vid.expected
+        echo $reply >> $inport.expected
+    fi
+}
+
+sip=`ip_to_hex 172 31 0 10`
+tip=`ip_to_hex 172 31 0 1`
+
+# Set a hypervisor as gateway chassis, for router port 172.31.0.1
+ovn-nbctl lrp-set-gateway-chassis router-to-underlay hv3
+ovn-nbctl --wait=sb sync
+sleep 2
+
+test_arp vif-north f0f000000011 $sip $tip 000001010207
+
+sleep 1

Hi Ankur,

Thanks for addressing most of the comments.
I see that the comment on sleep is not addressed.
Is it possible to address that ? - https://mail.openvswitch.org/pipermail/ovs-dev/2019-August/361380.html [mail.openvswitch.org]<https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_pipermail_ovs-2Ddev_2019-2DAugust_361380.html&d=DwMFaQ&c=s883GpUCOChKOHiocYtGcg&r=mZwX9gFQgeJHzTg-68aCJgsODyUEVsHGFOfL90J6MJY&m=8lRHQF9NAseA2tKXK0ON2WZL5FFfm-oWD7DK0rjdjAw&s=y84LxUEJ21NGZE5vkJ5qAqJrhLamTD1Ky4Rfr1QJwt0&e=>

In the patch 4, there is sleep 5. I would suggest to remove this sleep 5 and think of a better way.

Other than the comments related to the tests, the series LGTM.

Thanks
Numan

+
+# Confirm that vif-north gets a single ARP reply
+OVN_CHECK_PACKETS_REMOVE_BROADCAST([hv4/vif-north-tx.pcap], [vif-north.expected])
+
+# Confirm that only redirect chassis allowed arp resolution.
+OVN_CHECK_PACKETS_REMOVE_BROADCAST([hv3/br-phys_n1-tx.pcap], [vif-north_vid.expected])
+AT_CHECK([grep 000001010207 hv3/br-phys_n1-tx.packets | wc -l], [0], [[1
+]])
+
+# Confirm that other OVN chassis did not generate ARP reply.
+$PYTHON "$top_srcdir/utilities/ovs-pcap.in [ovs-pcap.in]<https://urldefense.proofpoint.com/v2/url?u=http-3A__ovs-2Dpcap.in&d=DwMFaQ&c=s883GpUCOChKOHiocYtGcg&r=mZwX9gFQgeJHzTg-68aCJgsODyUEVsHGFOfL90J6MJY&m=8lRHQF9NAseA2tKXK0ON2WZL5FFfm-oWD7DK0rjdjAw&s=kcSy2E_5iT1j383saryywnWrCesuyOqggseo79Gpsho&e=>" hv1/br-phys_n1-tx.pcap > hv1/br-phys_n1-tx.packets
+$PYTHON "$top_srcdir/utilities/ovs-pcap.in [ovs-pcap.in]<https://urldefense.proofpoint.com/v2/url?u=http-3A__ovs-2Dpcap.in&d=DwMFaQ&c=s883GpUCOChKOHiocYtGcg&r=mZwX9gFQgeJHzTg-68aCJgsODyUEVsHGFOfL90J6MJY&m=8lRHQF9NAseA2tKXK0ON2WZL5FFfm-oWD7DK0rjdjAw&s=kcSy2E_5iT1j383saryywnWrCesuyOqggseo79Gpsho&e=>" hv2/br-phys_n1-tx.pcap > hv2/br-phys_n1-tx.packets
+
+AT_CHECK([grep 000001010207 hv1/br-phys_n1-tx.packets | wc -l], [0], [[0
+]])
+AT_CHECK([grep 000001010207 hv2/br-phys_n1-tx.packets | wc -l], [0], [[0
+]])
+
+echo "----------- Post Traffic hv1 dump -----------"
+as hv1 ovs-ofctl -O OpenFlow13 dump-flows br-int
+as hv1 ovs-appctl fdb/show br-phys
+
+echo "----------- Post Traffic hv2 dump -----------"
+as hv2 ovs-ofctl -O OpenFlow13 dump-flows br-int
+as hv2 ovs-appctl fdb/show br-phys
+
+echo "----------- Post Traffic hv3 dump -----------"
+as hv3 ovs-ofctl -O OpenFlow13 dump-flows br-int
+as hv3 ovs-appctl fdb/show br-phys
+
+echo "----------- Post Traffic hv4 dump -----------"
+as hv4 ovs-ofctl -O OpenFlow13 dump-flows br-int
+as hv4 ovs-appctl fdb/show br-phys
+
+OVN_CLEANUP([hv1],[hv2],[hv3],[hv4])
+
+AT_CLEANUP
--
1.8.3.1
Ankur Sharma Aug. 22, 2019, 7:21 p.m. UTC | #3
Hi Numan,

Submitted a v6 last week. Awaiting your feedback on it, please feel free to let me know if additional changes are needed.
Thanks

Regards,
Ankur
From: Ankur Sharma
Sent: Friday, August 16, 2019 5:37 PM
To: Numan Siddique <nusiddiq@redhat.com>
Cc: ovs-dev@openvswitch.org
Subject: RE: [ovs-dev] [PATCH v5 1/4 ovn] OVN: Do not replace router port mac on gateway chassis.

Hi Numan,

My bad, I missed the sleep related comments.
Fixed the same in v6, please take a look.

Appreciate your help.

Regards,
Ankur

From: Numan Siddique <nusiddiq@redhat.com<mailto:nusiddiq@redhat.com>>
Sent: Friday, August 16, 2019 5:46 AM
To: Ankur Sharma <ankur.sharma@nutanix.com<mailto:ankur.sharma@nutanix.com>>
Cc: ovs-dev@openvswitch.org<mailto:ovs-dev@openvswitch.org>
Subject: Re: [ovs-dev] [PATCH v5 1/4 ovn] OVN: Do not replace router port mac on gateway chassis.



On Fri, Aug 16, 2019 at 1:11 AM Ankur Sharma <ankur.sharma@nutanix.com<mailto:ankur.sharma@nutanix.com>> wrote:
With 795d7f24ce0e2ed5454e193a059451d237289542 we have added
support for E-W routing on vlan backed networks by replacing
router port macs with chassis macs.

This replacement of router port mac need NOT be done on
gateway chassis for following reasons:

a. For N-S traffic, gateway chassis will respond to ARP
for the router port (to which it is attached) and
traffic will be using router port mac as destination mac.

b. Chassis redirect port is a centralized version of distributed
router port, hence we need not replace its mac with chassis mac
on the resident chassis.

This patch addresses the same.

Signed-off-by: Ankur Sharma <ankur.sharma@nutanix.com<mailto:ankur.sharma@nutanix.com>>
---
 controller/lport.c    |  20 ++++
 controller/lport.h    |   6 +
 controller/physical.c |  18 ++-
 controller/pinctrl.c  |  20 +---
 tests/ovn.at [ovn.at]<https://urldefense.proofpoint.com/v2/url?u=http-3A__ovn.at&d=DwMFaQ&c=s883GpUCOChKOHiocYtGcg&r=mZwX9gFQgeJHzTg-68aCJgsODyUEVsHGFOfL90J6MJY&m=8lRHQF9NAseA2tKXK0ON2WZL5FFfm-oWD7DK0rjdjAw&s=w3LIeruJk1CnRw6SHnGnzwMdJnPWOwznG2nFPkaQM0E&e=>          | 313 ++++++++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 356 insertions(+), 21 deletions(-)

diff --git a/controller/lport.c b/controller/lport.c
index 792c825..478fcfd 100644
--- a/controller/lport.c
+++ b/controller/lport.c
@@ -17,6 +17,7 @@

 #include "lib/sset.h"
 #include "lport.h"
+#include "ha-chassis.h"
 #include "hash.h"
 #include "openvswitch/vlog.h"
 #include "lib/ovn-sb-idl.h"
@@ -64,6 +65,25 @@ lport_lookup_by_key(struct ovsdb_idl_index *sbrec_datapath_binding_by_key,
     return retval;
 }

+bool
+lport_is_chassis_resident(struct ovsdb_idl_index *sbrec_port_binding_by_name,
+                          const struct sbrec_chassis *chassis,
+                          const struct sset *active_tunnels,
+                          const char *port_name)
+{
+    const struct sbrec_port_binding *pb
+        = lport_lookup_by_name(sbrec_port_binding_by_name, port_name);
+    if (!pb || !pb->chassis) {
+        return false;
+    }
+    if (strcmp(pb->type, "chassisredirect")) {
+        return pb->chassis == chassis;
+    } else {
+        return ha_chassis_group_is_active(pb->ha_chassis_group,
+                                          active_tunnels, chassis);
+    }
+}
+
 const struct sbrec_datapath_binding *
 datapath_lookup_by_key(struct ovsdb_idl_index *sbrec_datapath_binding_by_key,
                        uint64_t dp_key)
diff --git a/controller/lport.h b/controller/lport.h
index 2d4bb71..345efc1 100644
--- a/controller/lport.h
+++ b/controller/lport.h
@@ -23,6 +23,7 @@ struct sbrec_chassis;
 struct sbrec_datapath_binding;
 struct sbrec_multicast_group;
 struct sbrec_port_binding;
+struct sset;


 /* Database indexes.
@@ -48,5 +49,10 @@ const struct sbrec_datapath_binding *datapath_lookup_by_key(
 const struct sbrec_multicast_group *mcgroup_lookup_by_dp_name(
     struct ovsdb_idl_index *sbrec_multicast_group_by_name_datapath,
     const struct sbrec_datapath_binding *, const char *name);
+bool
+lport_is_chassis_resident(struct ovsdb_idl_index *sbrec_port_binding_by_name,
+                          const struct sbrec_chassis *chassis,
+                          const struct sset *active_tunnels,
+                          const char *port_name);

 #endif /* controller/lport.h */
diff --git a/controller/physical.c b/controller/physical.c
index a05962b..5068785 100644
--- a/controller/physical.c
+++ b/controller/physical.c
@@ -228,9 +228,12 @@ get_zone_ids(const struct sbrec_port_binding *binding,
 }

 static void
-put_replace_router_port_mac_flows(const struct
+put_replace_router_port_mac_flows(struct ovsdb_idl_index
+                                  *sbrec_port_binding_by_name,
+                                  const struct
                                   sbrec_port_binding *localnet_port,
                                   const struct sbrec_chassis *chassis,
+                                  const struct sset *active_tunnels,
                                   const struct hmap *local_datapaths,
                                   struct ofpbuf *ofpacts_p,
                                   ofp_port_t ofport,
@@ -270,6 +273,16 @@ put_replace_router_port_mac_flows(const struct
         struct eth_addr router_port_mac;
         struct match match;
         struct ofpact_mac *replace_mac;
+        char *cr_peer_name = xasprintf("cr-%s", rport_binding->logical_port);
+        if (lport_is_chassis_resident(sbrec_port_binding_by_name,
+                                      chassis, active_tunnels,
+                                      cr_peer_name)) {
+            /* If a router port's chassisredirect port is
+             * resident on this chassis, then we need not do mac replace. */
+            free(cr_peer_name);
+            continue;
+        }
+        free(cr_peer_name);

         /* Table 65, priority 150.
          * =======================
@@ -787,7 +800,8 @@ consider_port_binding(struct ovsdb_idl_index *sbrec_port_binding_by_name,
                         &match, ofpacts_p, &binding->header_.uuid);

         if (!strcmp(binding->type, "localnet")) {
-            put_replace_router_port_mac_flows(binding, chassis,
+            put_replace_router_port_mac_flows(sbrec_port_binding_by_name,
+                                              binding, chassis, active_tunnels,
                                               local_datapaths, ofpacts_p,
                                               ofport, flow_table);
         }
diff --git a/controller/pinctrl.c b/controller/pinctrl.c
index f27718f..e8abe0b 100644
--- a/controller/pinctrl.c
+++ b/controller/pinctrl.c
@@ -3755,24 +3755,6 @@ get_localnet_vifs_l3gwports(
     sbrec_port_binding_index_destroy_row(target);
 }

-static bool
-pinctrl_is_chassis_resident(struct ovsdb_idl_index *sbrec_port_binding_by_name,
-                            const struct sbrec_chassis *chassis,
-                            const struct sset *active_tunnels,
-                            const char *port_name)
-{
-    const struct sbrec_port_binding *pb
-        = lport_lookup_by_name(sbrec_port_binding_by_name, port_name);
-    if (!pb || !pb->chassis) {
-        return false;
-    }
-    if (strcmp(pb->type, "chassisredirect")) {
-        return pb->chassis == chassis;
-    } else {
-        return ha_chassis_group_is_active(pb->ha_chassis_group,
-                                          active_tunnels, chassis);
-    }
-}

 /* Extracts the mac, IPv4 and IPv6 addresses, and logical port from
  * 'addresses' which should be of the format 'MAC [IP1 IP2 ..]
@@ -3853,7 +3835,7 @@ consider_nat_address(struct ovsdb_idl_index *sbrec_port_binding_by_name,
     char *lport = NULL;
     if (!extract_addresses_with_port(nat_address, laddrs, &lport)
         || (!lport && !strcmp(pb->type, "patch"))
-        || (lport && !pinctrl_is_chassis_resident(
+        || (lport && !lport_is_chassis_resident(
                 sbrec_port_binding_by_name, chassis,
                 active_tunnels, lport))) {
         destroy_lport_addresses(laddrs);
diff --git a/tests/ovn.at [ovn.at]<https://urldefense.proofpoint.com/v2/url?u=http-3A__ovn.at&d=DwMFaQ&c=s883GpUCOChKOHiocYtGcg&r=mZwX9gFQgeJHzTg-68aCJgsODyUEVsHGFOfL90J6MJY&m=8lRHQF9NAseA2tKXK0ON2WZL5FFfm-oWD7DK0rjdjAw&s=w3LIeruJk1CnRw6SHnGnzwMdJnPWOwznG2nFPkaQM0E&e=> b/tests/ovn.at [ovn.at]<https://urldefense.proofpoint.com/v2/url?u=http-3A__ovn.at&d=DwMFaQ&c=s883GpUCOChKOHiocYtGcg&r=mZwX9gFQgeJHzTg-68aCJgsODyUEVsHGFOfL90J6MJY&m=8lRHQF9NAseA2tKXK0ON2WZL5FFfm-oWD7DK0rjdjAw&s=w3LIeruJk1CnRw6SHnGnzwMdJnPWOwznG2nFPkaQM0E&e=>
index 71eb390..1613c0b 100644
--- a/tests/ovn.at [ovn.at]<https://urldefense.proofpoint.com/v2/url?u=http-3A__ovn.at&d=DwMFaQ&c=s883GpUCOChKOHiocYtGcg&r=mZwX9gFQgeJHzTg-68aCJgsODyUEVsHGFOfL90J6MJY&m=8lRHQF9NAseA2tKXK0ON2WZL5FFfm-oWD7DK0rjdjAw&s=w3LIeruJk1CnRw6SHnGnzwMdJnPWOwznG2nFPkaQM0E&e=>
+++ b/tests/ovn.at [ovn.at]<https://urldefense.proofpoint.com/v2/url?u=http-3A__ovn.at&d=DwMFaQ&c=s883GpUCOChKOHiocYtGcg&r=mZwX9gFQgeJHzTg-68aCJgsODyUEVsHGFOfL90J6MJY&m=8lRHQF9NAseA2tKXK0ON2WZL5FFfm-oWD7DK0rjdjAw&s=w3LIeruJk1CnRw6SHnGnzwMdJnPWOwznG2nFPkaQM0E&e=>
@@ -29,6 +29,12 @@ m4_define([OVN_CHECK_PACKETS],
   [ovn_check_packets__ "$1" "$2"
    AT_CHECK([sort $rcv_text], [0], [expout])])

+m4_define([OVN_CHECK_PACKETS_REMOVE_BROADCAST],
+  [ovn_check_packets__ "$1" "$2"
+   echo "received_text=$rcv_text"
+   sed -i '/ffffffffffff/d' $rcv_text
+   AT_CHECK([sort $rcv_text], [0], [expout])])
+
 AT_BANNER([OVN components])

 AT_SETUP([ovn -- lexer])
@@ -15009,3 +15015,310 @@ on_exit 'kill $(cat ovn-nbctl.pid)'
 AT_CHECK([ovn-nbctl -u $sockfile show])

 AT_CLEANUP
+
+
+AT_SETUP([ovn -- 2 HVs, 2 lports/HV, localnet ports, DVR N-S ARP handling])
+ovn_start
+
+# In this test cases we create 3 switches, all connected to same
+# physical network (through br-phys on each HV). LS1 and LS2 have
+# 1 VIF each. Each HV has 1 VIF port. The first digit
+# of VIF port name indicates the hypervisor it is bound to, e.g.
+# lp23 means VIF 3 on hv2.
+#
+# All the switches are connected to a logical router "router".
+#
+# Each switch's VLAN tag and their logical switch ports are:
+#   - ls1:
+#       - tagged with VLAN 101
+#       - ports: lp11
+#   - ls2:
+#       - tagged with VLAN 201
+#       - ports: lp22
+#   - ls-underlay:
+#       - tagged with VLAN 1000
+# Note: a localnet port is created for each switch to connect to
+# physical network.
+
+for i in 1 2; do
+    ls_name=ls$i
+    ovn-nbctl ls-add $ls_name
+    ln_port_name=ln$i
+    if test $i -eq 1; then
+        ovn-nbctl lsp-add $ls_name $ln_port_name "" 101
+    elif test $i -eq 2; then
+        ovn-nbctl lsp-add $ls_name $ln_port_name "" 201
+    fi
+    ovn-nbctl lsp-set-addresses $ln_port_name unknown
+    ovn-nbctl lsp-set-type $ln_port_name localnet
+    ovn-nbctl lsp-set-options $ln_port_name network_name=phys
+done
+
+# lsp_to_ls LSP
+#
+# Prints the name of the logical switch that contains LSP.
+lsp_to_ls () {
+    case $1 in dnl (
+        lp?[[11]]) echo ls1 ;; dnl (
+        lp?[[12]]) echo ls2 ;; dnl (
+        *) AT_FAIL_IF([:]) ;;
+    esac
+}
+
+vif_to_hv () {
+    case $1 in dnl (
+        vif[[1]]?) echo hv1 ;; dnl (
+        vif[[2]]?) echo hv2 ;; dnl (
+        vif?[[north]]?) echo hv4 ;; dnl (
+        *) AT_FAIL_IF([:]) ;;
+    esac
+}
+
+ip_to_hex() {
+       printf "%02x%02x%02x%02x" "$@"
+}
+
+net_add n1
+for i in 1 2; do
+    sim_add hv$i
+    as hv$i
+    ovs-vsctl add-br br-phys
+    ovs-vsctl set open . external-ids:ovn-bridge-mappings=phys:br-phys
+    ovs-vsctl set open . external-ids:ovn-chassis-mac-mappings="phys:aa:bb:cc:dd:ee:$i$i"
+    ovn_attach n1 br-phys 192.168.0.$i
+
+    ovs-vsctl add-port br-int vif$i$i -- \
+        set Interface vif$i$i external-ids:iface-id=lp$i$i \
+                              options:tx_pcap=hv$i/vif$i$i-tx.pcap \
+                              options:rxq_pcap=hv$i/vif$i$i-rx.pcap \
+                              ofport-request=$i$i
+
+    lsp_name=lp$i$i
+    ls_name=$(lsp_to_ls $lsp_name)
+
+    ovn-nbctl lsp-add $ls_name $lsp_name
+    ovn-nbctl lsp-set-addresses $lsp_name "f0:00:00:00:00:$i$i 192.168.$i.$i"
+    ovn-nbctl lsp-set-port-security $lsp_name f0:00:00:00:00:$i$i
+
+    OVS_WAIT_UNTIL([test x`ovn-nbctl lsp-get-up $lsp_name` = xup])
+
+done
+
+ovn-nbctl ls-add ls-underlay
+ovn-nbctl lsp-add ls-underlay ln3 "" 1000
+ovn-nbctl lsp-set-addresses ln3 unknown
+ovn-nbctl lsp-set-type ln3 localnet
+ovn-nbctl lsp-set-options ln3 network_name=phys
+
+ovn-nbctl ls-add ls-north
+ovn-nbctl lsp-add ls-north ln4 "" 1000
+ovn-nbctl lsp-set-addresses ln4 unknown
+ovn-nbctl lsp-set-type ln4 localnet
+ovn-nbctl lsp-set-options ln4 network_name=phys
+
+# Add a VM on ls-north
+ovn-nbctl lsp-add ls-north lp-north
+ovn-nbctl lsp-set-addresses lp-north "f0:f0:00:00:00:11 172.31.0.10"
+ovn-nbctl lsp-set-port-security lp-north f0:f0:00:00:00:11
+
+# Add 3rd hypervisor
+sim_add hv3
+as hv3 ovs-vsctl add-br br-phys
+as hv3 ovs-vsctl set open . external-ids:ovn-bridge-mappings=phys:br-phys
+as hv3 ovs-vsctl set open . external-ids:ovn-chassis-mac-mappings="phys:aa:bb:cc:dd:ee:33"
+as hv3 ovn_attach n1 br-phys 192.168.0.3
+
+# Add 4th hypervisor
+sim_add hv4
+as hv4 ovs-vsctl add-br br-phys
+as hv4 ovs-vsctl set open . external-ids:ovn-bridge-mappings=phys:br-phys
+as hv4 ovs-vsctl set open . external-ids:ovn-chassis-mac-mappings="phys:aa:bb:cc:dd:ee:44"
+as hv4 ovn_attach n1 br-phys 192.168.0.4
+
+as hv4 ovs-vsctl add-port br-int vif-north -- \
+        set Interface vif-north external-ids:iface-id=lp-north \
+                              options:tx_pcap=hv4/vif-north-tx.pcap \
+                              options:rxq_pcap=hv4/vif-north-rx.pcap \
+                              ofport-request=44
+
+ovn-nbctl lr-add router
+ovn-nbctl lrp-add router router-to-ls1 00:00:01:01:02:03 192.168.1.3/24
+ovn-nbctl [192.168.1.3]<https://urldefense.proofpoint.com/v2/url?u=http-3A__192.168.1.3_24-2Bovn-2Dnbctl&d=DwMFaQ&c=s883GpUCOChKOHiocYtGcg&r=mZwX9gFQgeJHzTg-68aCJgsODyUEVsHGFOfL90J6MJY&m=8lRHQF9NAseA2tKXK0ON2WZL5FFfm-oWD7DK0rjdjAw&s=6tHzia-2yI1lnnjpILts0EiAgvvpHEAfnRV5C10lsP0&e=> lrp-add router router-to-ls2 00:00:01:01:02:05 192.168.2.3/24
+ovn-nbctl [192.168.2.3]<https://urldefense.proofpoint.com/v2/url?u=http-3A__192.168.2.3_24-2Bovn-2Dnbctl&d=DwMFaQ&c=s883GpUCOChKOHiocYtGcg&r=mZwX9gFQgeJHzTg-68aCJgsODyUEVsHGFOfL90J6MJY&m=8lRHQF9NAseA2tKXK0ON2WZL5FFfm-oWD7DK0rjdjAw&s=WU5KjQQKv5XYKS_FW-g7fs0Kob6-j--q38Vr-PaMM6g&e=> lrp-add router router-to-underlay 00:00:01:01:02:07 172.31.0.1/24 [172.31.0.1]<https://urldefense.proofpoint.com/v2/url?u=http-3A__172.31.0.1_24&d=DwMFaQ&c=s883GpUCOChKOHiocYtGcg&r=mZwX9gFQgeJHzTg-68aCJgsODyUEVsHGFOfL90J6MJY&m=8lRHQF9NAseA2tKXK0ON2WZL5FFfm-oWD7DK0rjdjAw&s=EL6Rm6HUq4t0WDpco9GqQIS3YEd7Zj_KhbCH4M1D4Vw&e=>
+
+ovn-nbctl lsp-add ls1 ls1-to-router -- set Logical_Switch_Port ls1-to-router type=router \
+          options:router-port=router-to-ls1 -- lsp-set-addresses ls1-to-router router
+ovn-nbctl lsp-add ls2 ls2-to-router -- set Logical_Switch_Port ls2-to-router type=router \
+          options:router-port=router-to-ls2 -- lsp-set-addresses ls2-to-router router
+ovn-nbctl lsp-add ls-underlay underlay-to-router -- set Logical_Switch_Port \
+                              underlay-to-router type=router \
+                              options:router-port=router-to-underlay \
+                              -- lsp-set-addresses underlay-to-router router
+
+
+OVN_POPULATE_ARP
+
+# lsp_to_ls LSP
+#
+# Prints the name of the logical switch that contains LSP.
+lsp_to_ls () {
+    case $1 in dnl (
+        lp?[[11]]) echo ls1 ;; dnl (
+        lp?[[12]]) echo ls2 ;; dnl (
+        *) AT_FAIL_IF([:]) ;;
+    esac
+}
+
+vif_to_ls () {
+    case $1 in dnl (
+        vif?[[11]]) echo ls1 ;; dnl (
+        vif?[[12]]) echo ls2 ;; dnl (
+        vif-north) echo ls-north ;; dnl (
+        *) AT_FAIL_IF([:]) ;;
+    esac
+}
+
+hv_to_num () {
+    case $1 in dnl (
+        hv1) echo 1 ;; dnl (
+        hv2) echo 2 ;; dnl (
+        hv3) echo 3 ;; dnl (
+        hv4) echo 4 ;; dnl (
+        *) AT_FAIL_IF([:]) ;;
+    esac
+}
+
+vif_to_num () {
+    case $1 in dnl (
+        vif22) echo 22 ;; dnl (
+        vif21) echo 21 ;; dnl (
+        vif11) echo 11 ;; dnl (
+        *) AT_FAIL_IF([:]) ;;
+    esac
+}
+
+vif_to_hv () {
+    case $1 in dnl (
+        vif[[1]]?) echo hv1 ;; dnl (
+        vif[[2]]?) echo hv2 ;; dnl (
+        vif-north) echo hv4 ;; dnl (
+        *) AT_FAIL_IF([:]) ;;
+    esac
+}
+
+vif_to_lrp () {
+    echo router-to-`vif_to_ls $1`
+}
+
+ip_to_hex() {
+       printf "%02x%02x%02x%02x" "$@"
+}
+
+# Dump a bunch of info helpful for debugging if there's a failure.
+
+echo "------ OVN dump ------"
+ovn-nbctl show
+ovn-sbctl show
+ovn-sbctl list port_binding
+ovn-sbctl list mac_binding
+
+echo "------ hv1 dump ------"
+as hv1 ovs-vsctl show
+as hv1 ovs-vsctl list Open_Vswitch
+
+echo "------ hv2 dump ------"
+as hv2 ovs-vsctl show
+as hv2 ovs-vsctl list Open_Vswitch
+
+echo "------ hv3 dump ------"
+as hv3 ovs-vsctl show
+as hv3 ovs-vsctl list Open_Vswitch
+
+echo "------ hv4 dump ------"
+as hv4 ovs-vsctl show
+as hv4 ovs-vsctl list Open_Vswitch
+
+# test_arp INPORT SHA SPA TPA [REPLY_HA]
+#
+# Causes a packet to be received on INPORT.  The packet is an ARP
+# request with SHA, SPA, and TPA as specified.  If REPLY_HA is provided, then
+# it should be the hardware address of the target to expect to receive in an
+# ARP reply; otherwise no reply is expected.
+#
+# INPORT is an logical switch port number, e.g. 11 for vif11.
+# SHA and REPLY_HA are each 12 hex digits.
+# SPA and TPA are each 8 hex digits.
+test_arp() {
+    local inport=$1 sha=$2 spa=$3 tpa=$4 reply_ha=$5
+    local request=ffffffffffff${sha}08060001080006040001${sha}${spa}ffffffffffff${tpa}
+    hv=`vif_to_hv $inport`
+    as $hv ovs-appctl netdev-dummy/receive $inport $request
+
+    if test X$reply_ha = X; then
+        # Expect to receive the broadcast ARP on the other logical switch ports
+        # if no reply is expected.
+        local i j
+        for i in 1 2 3; do
+            for j in 1 2 3; do
+                if test $i$j != $inport; then
+                    echo $request >> $i$j.expected
+                fi
+            done
+        done
+    else
+        # Expect to receive the reply, if any.
+        local reply=${sha}${reply_ha}08060001080006040002${reply_ha}${tpa}${sha}${spa}
+        local reply_vid=${sha}${reply_ha}810003e808060001080006040002${reply_ha}${tpa}${sha}${spa}
+        echo $reply_vid >> ${inport}_vid.expected
+        echo $reply >> $inport.expected
+    fi
+}
+
+sip=`ip_to_hex 172 31 0 10`
+tip=`ip_to_hex 172 31 0 1`
+
+# Set a hypervisor as gateway chassis, for router port 172.31.0.1
+ovn-nbctl lrp-set-gateway-chassis router-to-underlay hv3
+ovn-nbctl --wait=sb sync
+sleep 2
+
+test_arp vif-north f0f000000011 $sip $tip 000001010207
+
+sleep 1

Hi Ankur,

Thanks for addressing most of the comments.
I see that the comment on sleep is not addressed.
Is it possible to address that ? - https://mail.openvswitch.org/pipermail/ovs-dev/2019-August/361380.html [mail.openvswitch.org]<https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_pipermail_ovs-2Ddev_2019-2DAugust_361380.html&d=DwMFaQ&c=s883GpUCOChKOHiocYtGcg&r=mZwX9gFQgeJHzTg-68aCJgsODyUEVsHGFOfL90J6MJY&m=8lRHQF9NAseA2tKXK0ON2WZL5FFfm-oWD7DK0rjdjAw&s=y84LxUEJ21NGZE5vkJ5qAqJrhLamTD1Ky4Rfr1QJwt0&e=>

In the patch 4, there is sleep 5. I would suggest to remove this sleep 5 and think of a better way.

Other than the comments related to the tests, the series LGTM.

Thanks
Numan

+
+# Confirm that vif-north gets a single ARP reply
+OVN_CHECK_PACKETS_REMOVE_BROADCAST([hv4/vif-north-tx.pcap], [vif-north.expected])
+
+# Confirm that only redirect chassis allowed arp resolution.
+OVN_CHECK_PACKETS_REMOVE_BROADCAST([hv3/br-phys_n1-tx.pcap], [vif-north_vid.expected])
+AT_CHECK([grep 000001010207 hv3/br-phys_n1-tx.packets | wc -l], [0], [[1
+]])
+
+# Confirm that other OVN chassis did not generate ARP reply.
+$PYTHON "$top_srcdir/utilities/ovs-pcap.in [ovs-pcap.in]<https://urldefense.proofpoint.com/v2/url?u=http-3A__ovs-2Dpcap.in&d=DwMFaQ&c=s883GpUCOChKOHiocYtGcg&r=mZwX9gFQgeJHzTg-68aCJgsODyUEVsHGFOfL90J6MJY&m=8lRHQF9NAseA2tKXK0ON2WZL5FFfm-oWD7DK0rjdjAw&s=kcSy2E_5iT1j383saryywnWrCesuyOqggseo79Gpsho&e=>" hv1/br-phys_n1-tx.pcap > hv1/br-phys_n1-tx.packets
+$PYTHON "$top_srcdir/utilities/ovs-pcap.in [ovs-pcap.in]<https://urldefense.proofpoint.com/v2/url?u=http-3A__ovs-2Dpcap.in&d=DwMFaQ&c=s883GpUCOChKOHiocYtGcg&r=mZwX9gFQgeJHzTg-68aCJgsODyUEVsHGFOfL90J6MJY&m=8lRHQF9NAseA2tKXK0ON2WZL5FFfm-oWD7DK0rjdjAw&s=kcSy2E_5iT1j383saryywnWrCesuyOqggseo79Gpsho&e=>" hv2/br-phys_n1-tx.pcap > hv2/br-phys_n1-tx.packets
+
+AT_CHECK([grep 000001010207 hv1/br-phys_n1-tx.packets | wc -l], [0], [[0
+]])
+AT_CHECK([grep 000001010207 hv2/br-phys_n1-tx.packets | wc -l], [0], [[0
+]])
+
+echo "----------- Post Traffic hv1 dump -----------"
+as hv1 ovs-ofctl -O OpenFlow13 dump-flows br-int
+as hv1 ovs-appctl fdb/show br-phys
+
+echo "----------- Post Traffic hv2 dump -----------"
+as hv2 ovs-ofctl -O OpenFlow13 dump-flows br-int
+as hv2 ovs-appctl fdb/show br-phys
+
+echo "----------- Post Traffic hv3 dump -----------"
+as hv3 ovs-ofctl -O OpenFlow13 dump-flows br-int
+as hv3 ovs-appctl fdb/show br-phys
+
+echo "----------- Post Traffic hv4 dump -----------"
+as hv4 ovs-ofctl -O OpenFlow13 dump-flows br-int
+as hv4 ovs-appctl fdb/show br-phys
+
+OVN_CLEANUP([hv1],[hv2],[hv3],[hv4])
+
+AT_CLEANUP
--
1.8.3.1
Numan Siddique Aug. 23, 2019, 1:40 p.m. UTC | #4
On Fri, Aug 23, 2019, 12:52 AM Ankur Sharma <ankur.sharma@nutanix.com>
wrote:

> Hi Numan,
>
> Submitted a v6 last week. Awaiting your feedback on it, please feel free
> to let me know if additional changes are needed.
>
> Thanks
>

Hi Ankit,

Yes it's on top of my head to review these patches. I will take a look and
come back to you if there are any comments.

Thanks
Numan


> Regards,
>
> Ankur
>
> *From:* Ankur Sharma
> *Sent:* Friday, August 16, 2019 5:37 PM
> *To:* Numan Siddique <nusiddiq@redhat.com>
> *Cc:* ovs-dev@openvswitch.org
> *Subject:* RE: [ovs-dev] [PATCH v5 1/4 ovn] OVN: Do not replace router
> port mac on gateway chassis.
>
>
>
> Hi Numan,
>
> My bad, I missed the sleep related comments.
> Fixed the same in v6, please take a look.
>
> Appreciate your help.
>
> Regards,
>
> Ankur
>
>
>
> *From:* Numan Siddique <nusiddiq@redhat.com>
> *Sent:* Friday, August 16, 2019 5:46 AM
> *To:* Ankur Sharma <ankur.sharma@nutanix.com>
> *Cc:* ovs-dev@openvswitch.org
> *Subject:* Re: [ovs-dev] [PATCH v5 1/4 ovn] OVN: Do not replace router
> port mac on gateway chassis.
>
>
>
>
>
>
>
> On Fri, Aug 16, 2019 at 1:11 AM Ankur Sharma <ankur.sharma@nutanix.com>
> wrote:
>
> With 795d7f24ce0e2ed5454e193a059451d237289542 we have added
> support for E-W routing on vlan backed networks by replacing
> router port macs with chassis macs.
>
> This replacement of router port mac need NOT be done on
> gateway chassis for following reasons:
>
> a. For N-S traffic, gateway chassis will respond to ARP
> for the router port (to which it is attached) and
> traffic will be using router port mac as destination mac.
>
> b. Chassis redirect port is a centralized version of distributed
> router port, hence we need not replace its mac with chassis mac
> on the resident chassis.
>
> This patch addresses the same.
>
> Signed-off-by: Ankur Sharma <ankur.sharma@nutanix.com>
> ---
>  controller/lport.c    |  20 ++++
>  controller/lport.h    |   6 +
>  controller/physical.c |  18 ++-
>  controller/pinctrl.c  |  20 +---
>  tests/ovn.at [ovn.at]
> <https://urldefense.proofpoint.com/v2/url?u=http-3A__ovn.at&d=DwMFaQ&c=s883GpUCOChKOHiocYtGcg&r=mZwX9gFQgeJHzTg-68aCJgsODyUEVsHGFOfL90J6MJY&m=8lRHQF9NAseA2tKXK0ON2WZL5FFfm-oWD7DK0rjdjAw&s=w3LIeruJk1CnRw6SHnGnzwMdJnPWOwznG2nFPkaQM0E&e=>
>         | 313 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 356 insertions(+), 21 deletions(-)
>
> diff --git a/controller/lport.c b/controller/lport.c
> index 792c825..478fcfd 100644
> --- a/controller/lport.c
> +++ b/controller/lport.c
> @@ -17,6 +17,7 @@
>
>  #include "lib/sset.h"
>  #include "lport.h"
> +#include "ha-chassis.h"
>  #include "hash.h"
>  #include "openvswitch/vlog.h"
>  #include "lib/ovn-sb-idl.h"
> @@ -64,6 +65,25 @@ lport_lookup_by_key(struct ovsdb_idl_index
> *sbrec_datapath_binding_by_key,
>      return retval;
>  }
>
> +bool
> +lport_is_chassis_resident(struct ovsdb_idl_index
> *sbrec_port_binding_by_name,
> +                          const struct sbrec_chassis *chassis,
> +                          const struct sset *active_tunnels,
> +                          const char *port_name)
> +{
> +    const struct sbrec_port_binding *pb
> +        = lport_lookup_by_name(sbrec_port_binding_by_name, port_name);
> +    if (!pb || !pb->chassis) {
> +        return false;
> +    }
> +    if (strcmp(pb->type, "chassisredirect")) {
> +        return pb->chassis == chassis;
> +    } else {
> +        return ha_chassis_group_is_active(pb->ha_chassis_group,
> +                                          active_tunnels, chassis);
> +    }
> +}
> +
>  const struct sbrec_datapath_binding *
>  datapath_lookup_by_key(struct ovsdb_idl_index
> *sbrec_datapath_binding_by_key,
>                         uint64_t dp_key)
> diff --git a/controller/lport.h b/controller/lport.h
> index 2d4bb71..345efc1 100644
> --- a/controller/lport.h
> +++ b/controller/lport.h
> @@ -23,6 +23,7 @@ struct sbrec_chassis;
>  struct sbrec_datapath_binding;
>  struct sbrec_multicast_group;
>  struct sbrec_port_binding;
> +struct sset;
>
>
>  /* Database indexes.
> @@ -48,5 +49,10 @@ const struct sbrec_datapath_binding
> *datapath_lookup_by_key(
>  const struct sbrec_multicast_group *mcgroup_lookup_by_dp_name(
>      struct ovsdb_idl_index *sbrec_multicast_group_by_name_datapath,
>      const struct sbrec_datapath_binding *, const char *name);
> +bool
> +lport_is_chassis_resident(struct ovsdb_idl_index
> *sbrec_port_binding_by_name,
> +                          const struct sbrec_chassis *chassis,
> +                          const struct sset *active_tunnels,
> +                          const char *port_name);
>
>  #endif /* controller/lport.h */
> diff --git a/controller/physical.c b/controller/physical.c
> index a05962b..5068785 100644
> --- a/controller/physical.c
> +++ b/controller/physical.c
> @@ -228,9 +228,12 @@ get_zone_ids(const struct sbrec_port_binding *binding,
>  }
>
>  static void
> -put_replace_router_port_mac_flows(const struct
> +put_replace_router_port_mac_flows(struct ovsdb_idl_index
> +                                  *sbrec_port_binding_by_name,
> +                                  const struct
>                                    sbrec_port_binding *localnet_port,
>                                    const struct sbrec_chassis *chassis,
> +                                  const struct sset *active_tunnels,
>                                    const struct hmap *local_datapaths,
>                                    struct ofpbuf *ofpacts_p,
>                                    ofp_port_t ofport,
> @@ -270,6 +273,16 @@ put_replace_router_port_mac_flows(const struct
>          struct eth_addr router_port_mac;
>          struct match match;
>          struct ofpact_mac *replace_mac;
> +        char *cr_peer_name = xasprintf("cr-%s",
> rport_binding->logical_port);
> +        if (lport_is_chassis_resident(sbrec_port_binding_by_name,
> +                                      chassis, active_tunnels,
> +                                      cr_peer_name)) {
> +            /* If a router port's chassisredirect port is
> +             * resident on this chassis, then we need not do mac replace.
> */
> +            free(cr_peer_name);
> +            continue;
> +        }
> +        free(cr_peer_name);
>
>          /* Table 65, priority 150.
>           * =======================
> @@ -787,7 +800,8 @@ consider_port_binding(struct ovsdb_idl_index
> *sbrec_port_binding_by_name,
>                          &match, ofpacts_p, &binding->header_.uuid);
>
>          if (!strcmp(binding->type, "localnet")) {
> -            put_replace_router_port_mac_flows(binding, chassis,
> +            put_replace_router_port_mac_flows(sbrec_port_binding_by_name,
> +                                              binding, chassis,
> active_tunnels,
>                                                local_datapaths, ofpacts_p,
>                                                ofport, flow_table);
>          }
> diff --git a/controller/pinctrl.c b/controller/pinctrl.c
> index f27718f..e8abe0b 100644
> --- a/controller/pinctrl.c
> +++ b/controller/pinctrl.c
> @@ -3755,24 +3755,6 @@ get_localnet_vifs_l3gwports(
>      sbrec_port_binding_index_destroy_row(target);
>  }
>
> -static bool
> -pinctrl_is_chassis_resident(struct ovsdb_idl_index
> *sbrec_port_binding_by_name,
> -                            const struct sbrec_chassis *chassis,
> -                            const struct sset *active_tunnels,
> -                            const char *port_name)
> -{
> -    const struct sbrec_port_binding *pb
> -        = lport_lookup_by_name(sbrec_port_binding_by_name, port_name);
> -    if (!pb || !pb->chassis) {
> -        return false;
> -    }
> -    if (strcmp(pb->type, "chassisredirect")) {
> -        return pb->chassis == chassis;
> -    } else {
> -        return ha_chassis_group_is_active(pb->ha_chassis_group,
> -                                          active_tunnels, chassis);
> -    }
> -}
>
>  /* Extracts the mac, IPv4 and IPv6 addresses, and logical port from
>   * 'addresses' which should be of the format 'MAC [IP1 IP2 ..]
> @@ -3853,7 +3835,7 @@ consider_nat_address(struct ovsdb_idl_index
> *sbrec_port_binding_by_name,
>      char *lport = NULL;
>      if (!extract_addresses_with_port(nat_address, laddrs, &lport)
>          || (!lport && !strcmp(pb->type, "patch"))
> -        || (lport && !pinctrl_is_chassis_resident(
> +        || (lport && !lport_is_chassis_resident(
>                  sbrec_port_binding_by_name, chassis,
>                  active_tunnels, lport))) {
>          destroy_lport_addresses(laddrs);
> diff --git a/tests/ovn.at [ovn.at]
> <https://urldefense.proofpoint.com/v2/url?u=http-3A__ovn.at&d=DwMFaQ&c=s883GpUCOChKOHiocYtGcg&r=mZwX9gFQgeJHzTg-68aCJgsODyUEVsHGFOfL90J6MJY&m=8lRHQF9NAseA2tKXK0ON2WZL5FFfm-oWD7DK0rjdjAw&s=w3LIeruJk1CnRw6SHnGnzwMdJnPWOwznG2nFPkaQM0E&e=>
> b/tests/ovn.at [ovn.at]
> <https://urldefense.proofpoint.com/v2/url?u=http-3A__ovn.at&d=DwMFaQ&c=s883GpUCOChKOHiocYtGcg&r=mZwX9gFQgeJHzTg-68aCJgsODyUEVsHGFOfL90J6MJY&m=8lRHQF9NAseA2tKXK0ON2WZL5FFfm-oWD7DK0rjdjAw&s=w3LIeruJk1CnRw6SHnGnzwMdJnPWOwznG2nFPkaQM0E&e=>
> index 71eb390..1613c0b 100644
> --- a/tests/ovn.at [ovn.at]
> <https://urldefense.proofpoint.com/v2/url?u=http-3A__ovn.at&d=DwMFaQ&c=s883GpUCOChKOHiocYtGcg&r=mZwX9gFQgeJHzTg-68aCJgsODyUEVsHGFOfL90J6MJY&m=8lRHQF9NAseA2tKXK0ON2WZL5FFfm-oWD7DK0rjdjAw&s=w3LIeruJk1CnRw6SHnGnzwMdJnPWOwznG2nFPkaQM0E&e=>
> +++ b/tests/ovn.at [ovn.at]
> <https://urldefense.proofpoint.com/v2/url?u=http-3A__ovn.at&d=DwMFaQ&c=s883GpUCOChKOHiocYtGcg&r=mZwX9gFQgeJHzTg-68aCJgsODyUEVsHGFOfL90J6MJY&m=8lRHQF9NAseA2tKXK0ON2WZL5FFfm-oWD7DK0rjdjAw&s=w3LIeruJk1CnRw6SHnGnzwMdJnPWOwznG2nFPkaQM0E&e=>
> @@ -29,6 +29,12 @@ m4_define([OVN_CHECK_PACKETS],
>    [ovn_check_packets__ "$1" "$2"
>     AT_CHECK([sort $rcv_text], [0], [expout])])
>
> +m4_define([OVN_CHECK_PACKETS_REMOVE_BROADCAST],
> +  [ovn_check_packets__ "$1" "$2"
> +   echo "received_text=$rcv_text"
> +   sed -i '/ffffffffffff/d' $rcv_text
> +   AT_CHECK([sort $rcv_text], [0], [expout])])
> +
>  AT_BANNER([OVN components])
>
>  AT_SETUP([ovn -- lexer])
> @@ -15009,3 +15015,310 @@ on_exit 'kill $(cat ovn-nbctl.pid)'
>  AT_CHECK([ovn-nbctl -u $sockfile show])
>
>  AT_CLEANUP
> +
> +
> +AT_SETUP([ovn -- 2 HVs, 2 lports/HV, localnet ports, DVR N-S ARP
> handling])
> +ovn_start
> +
> +# In this test cases we create 3 switches, all connected to same
> +# physical network (through br-phys on each HV). LS1 and LS2 have
> +# 1 VIF each. Each HV has 1 VIF port. The first digit
> +# of VIF port name indicates the hypervisor it is bound to, e.g.
> +# lp23 means VIF 3 on hv2.
> +#
> +# All the switches are connected to a logical router "router".
> +#
> +# Each switch's VLAN tag and their logical switch ports are:
> +#   - ls1:
> +#       - tagged with VLAN 101
> +#       - ports: lp11
> +#   - ls2:
> +#       - tagged with VLAN 201
> +#       - ports: lp22
> +#   - ls-underlay:
> +#       - tagged with VLAN 1000
> +# Note: a localnet port is created for each switch to connect to
> +# physical network.
> +
> +for i in 1 2; do
> +    ls_name=ls$i
> +    ovn-nbctl ls-add $ls_name
> +    ln_port_name=ln$i
> +    if test $i -eq 1; then
> +        ovn-nbctl lsp-add $ls_name $ln_port_name "" 101
> +    elif test $i -eq 2; then
> +        ovn-nbctl lsp-add $ls_name $ln_port_name "" 201
> +    fi
> +    ovn-nbctl lsp-set-addresses $ln_port_name unknown
> +    ovn-nbctl lsp-set-type $ln_port_name localnet
> +    ovn-nbctl lsp-set-options $ln_port_name network_name=phys
> +done
> +
> +# lsp_to_ls LSP
> +#
> +# Prints the name of the logical switch that contains LSP.
> +lsp_to_ls () {
> +    case $1 in dnl (
> +        lp?[[11]]) echo ls1 ;; dnl (
> +        lp?[[12]]) echo ls2 ;; dnl (
> +        *) AT_FAIL_IF([:]) ;;
> +    esac
> +}
> +
> +vif_to_hv () {
> +    case $1 in dnl (
> +        vif[[1]]?) echo hv1 ;; dnl (
> +        vif[[2]]?) echo hv2 ;; dnl (
> +        vif?[[north]]?) echo hv4 ;; dnl (
> +        *) AT_FAIL_IF([:]) ;;
> +    esac
> +}
> +
> +ip_to_hex() {
> +       printf "%02x%02x%02x%02x" "$@"
> +}
> +
> +net_add n1
> +for i in 1 2; do
> +    sim_add hv$i
> +    as hv$i
> +    ovs-vsctl add-br br-phys
> +    ovs-vsctl set open . external-ids:ovn-bridge-mappings=phys:br-phys
> +    ovs-vsctl set open .
> external-ids:ovn-chassis-mac-mappings="phys:aa:bb:cc:dd:ee:$i$i"
> +    ovn_attach n1 br-phys 192.168.0.$i
> +
> +    ovs-vsctl add-port br-int vif$i$i -- \
> +        set Interface vif$i$i external-ids:iface-id=lp$i$i \
> +                              options:tx_pcap=hv$i/vif$i$i-tx.pcap \
> +                              options:rxq_pcap=hv$i/vif$i$i-rx.pcap \
> +                              ofport-request=$i$i
> +
> +    lsp_name=lp$i$i
> +    ls_name=$(lsp_to_ls $lsp_name)
> +
> +    ovn-nbctl lsp-add $ls_name $lsp_name
> +    ovn-nbctl lsp-set-addresses $lsp_name "f0:00:00:00:00:$i$i
> 192.168.$i.$i"
> +    ovn-nbctl lsp-set-port-security $lsp_name f0:00:00:00:00:$i$i
> +
> +    OVS_WAIT_UNTIL([test x`ovn-nbctl lsp-get-up $lsp_name` = xup])
> +
> +done
> +
> +ovn-nbctl ls-add ls-underlay
> +ovn-nbctl lsp-add ls-underlay ln3 "" 1000
> +ovn-nbctl lsp-set-addresses ln3 unknown
> +ovn-nbctl lsp-set-type ln3 localnet
> +ovn-nbctl lsp-set-options ln3 network_name=phys
> +
> +ovn-nbctl ls-add ls-north
> +ovn-nbctl lsp-add ls-north ln4 "" 1000
> +ovn-nbctl lsp-set-addresses ln4 unknown
> +ovn-nbctl lsp-set-type ln4 localnet
> +ovn-nbctl lsp-set-options ln4 network_name=phys
> +
> +# Add a VM on ls-north
> +ovn-nbctl lsp-add ls-north lp-north
> +ovn-nbctl lsp-set-addresses lp-north "f0:f0:00:00:00:11 172.31.0.10"
> +ovn-nbctl lsp-set-port-security lp-north f0:f0:00:00:00:11
> +
> +# Add 3rd hypervisor
> +sim_add hv3
> +as hv3 ovs-vsctl add-br br-phys
> +as hv3 ovs-vsctl set open . external-ids:ovn-bridge-mappings=phys:br-phys
> +as hv3 ovs-vsctl set open .
> external-ids:ovn-chassis-mac-mappings="phys:aa:bb:cc:dd:ee:33"
> +as hv3 ovn_attach n1 br-phys 192.168.0.3
> +
> +# Add 4th hypervisor
> +sim_add hv4
> +as hv4 ovs-vsctl add-br br-phys
> +as hv4 ovs-vsctl set open . external-ids:ovn-bridge-mappings=phys:br-phys
> +as hv4 ovs-vsctl set open .
> external-ids:ovn-chassis-mac-mappings="phys:aa:bb:cc:dd:ee:44"
> +as hv4 ovn_attach n1 br-phys 192.168.0.4
> +
> +as hv4 ovs-vsctl add-port br-int vif-north -- \
> +        set Interface vif-north external-ids:iface-id=lp-north \
> +                              options:tx_pcap=hv4/vif-north-tx.pcap \
> +                              options:rxq_pcap=hv4/vif-north-rx.pcap \
> +                              ofport-request=44
> +
> +ovn-nbctl lr-add router
> +ovn-nbctl lrp-add router router-to-ls1 00:00:01:01:02:03 192.168.1.3/24
> +ovn-nbctl [192.168.1.3]
> <https://urldefense.proofpoint.com/v2/url?u=http-3A__192.168.1.3_24-2Bovn-2Dnbctl&d=DwMFaQ&c=s883GpUCOChKOHiocYtGcg&r=mZwX9gFQgeJHzTg-68aCJgsODyUEVsHGFOfL90J6MJY&m=8lRHQF9NAseA2tKXK0ON2WZL5FFfm-oWD7DK0rjdjAw&s=6tHzia-2yI1lnnjpILts0EiAgvvpHEAfnRV5C10lsP0&e=>
> lrp-add router router-to-ls2 00:00:01:01:02:05 192.168.2.3/24
> +ovn-nbctl [192.168.2.3]
> <https://urldefense.proofpoint.com/v2/url?u=http-3A__192.168.2.3_24-2Bovn-2Dnbctl&d=DwMFaQ&c=s883GpUCOChKOHiocYtGcg&r=mZwX9gFQgeJHzTg-68aCJgsODyUEVsHGFOfL90J6MJY&m=8lRHQF9NAseA2tKXK0ON2WZL5FFfm-oWD7DK0rjdjAw&s=WU5KjQQKv5XYKS_FW-g7fs0Kob6-j--q38Vr-PaMM6g&e=>
> lrp-add router router-to-underlay 00:00:01:01:02:07 172.31.0.1/24
> [172.31.0.1]
> <https://urldefense.proofpoint.com/v2/url?u=http-3A__172.31.0.1_24&d=DwMFaQ&c=s883GpUCOChKOHiocYtGcg&r=mZwX9gFQgeJHzTg-68aCJgsODyUEVsHGFOfL90J6MJY&m=8lRHQF9NAseA2tKXK0ON2WZL5FFfm-oWD7DK0rjdjAw&s=EL6Rm6HUq4t0WDpco9GqQIS3YEd7Zj_KhbCH4M1D4Vw&e=>
> +
> +ovn-nbctl lsp-add ls1 ls1-to-router -- set Logical_Switch_Port
> ls1-to-router type=router \
> +          options:router-port=router-to-ls1 -- lsp-set-addresses
> ls1-to-router router
> +ovn-nbctl lsp-add ls2 ls2-to-router -- set Logical_Switch_Port
> ls2-to-router type=router \
> +          options:router-port=router-to-ls2 -- lsp-set-addresses
> ls2-to-router router
> +ovn-nbctl lsp-add ls-underlay underlay-to-router -- set
> Logical_Switch_Port \
> +                              underlay-to-router type=router \
> +                              options:router-port=router-to-underlay \
> +                              -- lsp-set-addresses underlay-to-router
> router
> +
> +
> +OVN_POPULATE_ARP
> +
> +# lsp_to_ls LSP
> +#
> +# Prints the name of the logical switch that contains LSP.
> +lsp_to_ls () {
> +    case $1 in dnl (
> +        lp?[[11]]) echo ls1 ;; dnl (
> +        lp?[[12]]) echo ls2 ;; dnl (
> +        *) AT_FAIL_IF([:]) ;;
> +    esac
> +}
> +
> +vif_to_ls () {
> +    case $1 in dnl (
> +        vif?[[11]]) echo ls1 ;; dnl (
> +        vif?[[12]]) echo ls2 ;; dnl (
> +        vif-north) echo ls-north ;; dnl (
> +        *) AT_FAIL_IF([:]) ;;
> +    esac
> +}
> +
> +hv_to_num () {
> +    case $1 in dnl (
> +        hv1) echo 1 ;; dnl (
> +        hv2) echo 2 ;; dnl (
> +        hv3) echo 3 ;; dnl (
> +        hv4) echo 4 ;; dnl (
> +        *) AT_FAIL_IF([:]) ;;
> +    esac
> +}
> +
> +vif_to_num () {
> +    case $1 in dnl (
> +        vif22) echo 22 ;; dnl (
> +        vif21) echo 21 ;; dnl (
> +        vif11) echo 11 ;; dnl (
> +        *) AT_FAIL_IF([:]) ;;
> +    esac
> +}
> +
> +vif_to_hv () {
> +    case $1 in dnl (
> +        vif[[1]]?) echo hv1 ;; dnl (
> +        vif[[2]]?) echo hv2 ;; dnl (
> +        vif-north) echo hv4 ;; dnl (
> +        *) AT_FAIL_IF([:]) ;;
> +    esac
> +}
> +
> +vif_to_lrp () {
> +    echo router-to-`vif_to_ls $1`
> +}
> +
> +ip_to_hex() {
> +       printf "%02x%02x%02x%02x" "$@"
> +}
> +
> +# Dump a bunch of info helpful for debugging if there's a failure.
> +
> +echo "------ OVN dump ------"
> +ovn-nbctl show
> +ovn-sbctl show
> +ovn-sbctl list port_binding
> +ovn-sbctl list mac_binding
> +
> +echo "------ hv1 dump ------"
> +as hv1 ovs-vsctl show
> +as hv1 ovs-vsctl list Open_Vswitch
> +
> +echo "------ hv2 dump ------"
> +as hv2 ovs-vsctl show
> +as hv2 ovs-vsctl list Open_Vswitch
> +
> +echo "------ hv3 dump ------"
> +as hv3 ovs-vsctl show
> +as hv3 ovs-vsctl list Open_Vswitch
> +
> +echo "------ hv4 dump ------"
> +as hv4 ovs-vsctl show
> +as hv4 ovs-vsctl list Open_Vswitch
> +
> +# test_arp INPORT SHA SPA TPA [REPLY_HA]
> +#
> +# Causes a packet to be received on INPORT.  The packet is an ARP
> +# request with SHA, SPA, and TPA as specified.  If REPLY_HA is provided,
> then
> +# it should be the hardware address of the target to expect to receive in
> an
> +# ARP reply; otherwise no reply is expected.
> +#
> +# INPORT is an logical switch port number, e.g. 11 for vif11.
> +# SHA and REPLY_HA are each 12 hex digits.
> +# SPA and TPA are each 8 hex digits.
> +test_arp() {
> +    local inport=$1 sha=$2 spa=$3 tpa=$4 reply_ha=$5
> +    local
> request=ffffffffffff${sha}08060001080006040001${sha}${spa}ffffffffffff${tpa}
> +    hv=`vif_to_hv $inport`
> +    as $hv ovs-appctl netdev-dummy/receive $inport $request
> +
> +    if test X$reply_ha = X; then
> +        # Expect to receive the broadcast ARP on the other logical switch
> ports
> +        # if no reply is expected.
> +        local i j
> +        for i in 1 2 3; do
> +            for j in 1 2 3; do
> +                if test $i$j != $inport; then
> +                    echo $request >> $i$j.expected
> +                fi
> +            done
> +        done
> +    else
> +        # Expect to receive the reply, if any.
> +        local
> reply=${sha}${reply_ha}08060001080006040002${reply_ha}${tpa}${sha}${spa}
> +        local
> reply_vid=${sha}${reply_ha}810003e808060001080006040002${reply_ha}${tpa}${sha}${spa}
> +        echo $reply_vid >> ${inport}_vid.expected
> +        echo $reply >> $inport.expected
> +    fi
> +}
> +
> +sip=`ip_to_hex 172 31 0 10`
> +tip=`ip_to_hex 172 31 0 1`
> +
> +# Set a hypervisor as gateway chassis, for router port 172.31.0.1
> +ovn-nbctl lrp-set-gateway-chassis router-to-underlay hv3
> +ovn-nbctl --wait=sb sync
> +sleep 2
> +
> +test_arp vif-north f0f000000011 $sip $tip 000001010207
> +
> +sleep 1
>
>
>
> Hi Ankur,
>
>
>
> Thanks for addressing most of the comments.
>
> I see that the comment on sleep is not addressed.
>
> Is it possible to address that ? - https://mail.openvswitch.org/pipermail/ovs-dev/2019-August/361380.html
> [mail.openvswitch.org]
> <https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_pipermail_ovs-2Ddev_2019-2DAugust_361380.html&d=DwMFaQ&c=s883GpUCOChKOHiocYtGcg&r=mZwX9gFQgeJHzTg-68aCJgsODyUEVsHGFOfL90J6MJY&m=8lRHQF9NAseA2tKXK0ON2WZL5FFfm-oWD7DK0rjdjAw&s=y84LxUEJ21NGZE5vkJ5qAqJrhLamTD1Ky4Rfr1QJwt0&e=>
>
>
>
> In the patch 4, there is sleep 5. I would suggest to remove this sleep 5
> and think of a better way.
>
>
>
> Other than the comments related to the tests, the series LGTM.
>
>
>
> Thanks
>
> Numan
>
>
>
> +
> +# Confirm that vif-north gets a single ARP reply
> +OVN_CHECK_PACKETS_REMOVE_BROADCAST([hv4/vif-north-tx.pcap],
> [vif-north.expected])
> +
> +# Confirm that only redirect chassis allowed arp resolution.
> +OVN_CHECK_PACKETS_REMOVE_BROADCAST([hv3/br-phys_n1-tx.pcap],
> [vif-north_vid.expected])
> +AT_CHECK([grep 000001010207 hv3/br-phys_n1-tx.packets | wc -l], [0], [[1
> +]])
> +
> +# Confirm that other OVN chassis did not generate ARP reply.
> +$PYTHON "$top_srcdir/utilities/ovs-pcap.in [ovs-pcap.in]
> <https://urldefense.proofpoint.com/v2/url?u=http-3A__ovs-2Dpcap.in&d=DwMFaQ&c=s883GpUCOChKOHiocYtGcg&r=mZwX9gFQgeJHzTg-68aCJgsODyUEVsHGFOfL90J6MJY&m=8lRHQF9NAseA2tKXK0ON2WZL5FFfm-oWD7DK0rjdjAw&s=kcSy2E_5iT1j383saryywnWrCesuyOqggseo79Gpsho&e=>"
> hv1/br-phys_n1-tx.pcap > hv1/br-phys_n1-tx.packets
> +$PYTHON "$top_srcdir/utilities/ovs-pcap.in [ovs-pcap.in]
> <https://urldefense.proofpoint.com/v2/url?u=http-3A__ovs-2Dpcap.in&d=DwMFaQ&c=s883GpUCOChKOHiocYtGcg&r=mZwX9gFQgeJHzTg-68aCJgsODyUEVsHGFOfL90J6MJY&m=8lRHQF9NAseA2tKXK0ON2WZL5FFfm-oWD7DK0rjdjAw&s=kcSy2E_5iT1j383saryywnWrCesuyOqggseo79Gpsho&e=>"
> hv2/br-phys_n1-tx.pcap > hv2/br-phys_n1-tx.packets
> +
> +AT_CHECK([grep 000001010207 hv1/br-phys_n1-tx.packets | wc -l], [0], [[0
> +]])
> +AT_CHECK([grep 000001010207 hv2/br-phys_n1-tx.packets | wc -l], [0], [[0
> +]])
> +
> +echo "----------- Post Traffic hv1 dump -----------"
> +as hv1 ovs-ofctl -O OpenFlow13 dump-flows br-int
> +as hv1 ovs-appctl fdb/show br-phys
> +
> +echo "----------- Post Traffic hv2 dump -----------"
> +as hv2 ovs-ofctl -O OpenFlow13 dump-flows br-int
> +as hv2 ovs-appctl fdb/show br-phys
> +
> +echo "----------- Post Traffic hv3 dump -----------"
> +as hv3 ovs-ofctl -O OpenFlow13 dump-flows br-int
> +as hv3 ovs-appctl fdb/show br-phys
> +
> +echo "----------- Post Traffic hv4 dump -----------"
> +as hv4 ovs-ofctl -O OpenFlow13 dump-flows br-int
> +as hv4 ovs-appctl fdb/show br-phys
> +
> +OVN_CLEANUP([hv1],[hv2],[hv3],[hv4])
> +
> +AT_CLEANUP
> --
> 1.8.3.1
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> [mail.openvswitch.org]
> <https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwMFaQ&c=s883GpUCOChKOHiocYtGcg&r=mZwX9gFQgeJHzTg-68aCJgsODyUEVsHGFOfL90J6MJY&m=8lRHQF9NAseA2tKXK0ON2WZL5FFfm-oWD7DK0rjdjAw&s=SXdSIo1J13UcK2nLI2fXQ9vGcAxzognm3oD9f5F17Eo&e=>
>
>

Patch
diff mbox series

diff --git a/controller/lport.c b/controller/lport.c
index 792c825..478fcfd 100644
--- a/controller/lport.c
+++ b/controller/lport.c
@@ -17,6 +17,7 @@ 
 
 #include "lib/sset.h"
 #include "lport.h"
+#include "ha-chassis.h"
 #include "hash.h"
 #include "openvswitch/vlog.h"
 #include "lib/ovn-sb-idl.h"
@@ -64,6 +65,25 @@  lport_lookup_by_key(struct ovsdb_idl_index *sbrec_datapath_binding_by_key,
     return retval;
 }
 
+bool
+lport_is_chassis_resident(struct ovsdb_idl_index *sbrec_port_binding_by_name,
+                          const struct sbrec_chassis *chassis,
+                          const struct sset *active_tunnels,
+                          const char *port_name)
+{
+    const struct sbrec_port_binding *pb
+        = lport_lookup_by_name(sbrec_port_binding_by_name, port_name);
+    if (!pb || !pb->chassis) {
+        return false;
+    }
+    if (strcmp(pb->type, "chassisredirect")) {
+        return pb->chassis == chassis;
+    } else {
+        return ha_chassis_group_is_active(pb->ha_chassis_group,
+                                          active_tunnels, chassis);
+    }
+}
+
 const struct sbrec_datapath_binding *
 datapath_lookup_by_key(struct ovsdb_idl_index *sbrec_datapath_binding_by_key,
                        uint64_t dp_key)
diff --git a/controller/lport.h b/controller/lport.h
index 2d4bb71..345efc1 100644
--- a/controller/lport.h
+++ b/controller/lport.h
@@ -23,6 +23,7 @@  struct sbrec_chassis;
 struct sbrec_datapath_binding;
 struct sbrec_multicast_group;
 struct sbrec_port_binding;
+struct sset;
 
 
 /* Database indexes.
@@ -48,5 +49,10 @@  const struct sbrec_datapath_binding *datapath_lookup_by_key(
 const struct sbrec_multicast_group *mcgroup_lookup_by_dp_name(
     struct ovsdb_idl_index *sbrec_multicast_group_by_name_datapath,
     const struct sbrec_datapath_binding *, const char *name);
+bool
+lport_is_chassis_resident(struct ovsdb_idl_index *sbrec_port_binding_by_name,
+                          const struct sbrec_chassis *chassis,
+                          const struct sset *active_tunnels,
+                          const char *port_name);
 
 #endif /* controller/lport.h */
diff --git a/controller/physical.c b/controller/physical.c
index a05962b..5068785 100644
--- a/controller/physical.c
+++ b/controller/physical.c
@@ -228,9 +228,12 @@  get_zone_ids(const struct sbrec_port_binding *binding,
 }
 
 static void
-put_replace_router_port_mac_flows(const struct
+put_replace_router_port_mac_flows(struct ovsdb_idl_index
+                                  *sbrec_port_binding_by_name,
+                                  const struct
                                   sbrec_port_binding *localnet_port,
                                   const struct sbrec_chassis *chassis,
+                                  const struct sset *active_tunnels,
                                   const struct hmap *local_datapaths,
                                   struct ofpbuf *ofpacts_p,
                                   ofp_port_t ofport,
@@ -270,6 +273,16 @@  put_replace_router_port_mac_flows(const struct
         struct eth_addr router_port_mac;
         struct match match;
         struct ofpact_mac *replace_mac;
+        char *cr_peer_name = xasprintf("cr-%s", rport_binding->logical_port);
+        if (lport_is_chassis_resident(sbrec_port_binding_by_name,
+                                      chassis, active_tunnels,
+                                      cr_peer_name)) {
+            /* If a router port's chassisredirect port is
+             * resident on this chassis, then we need not do mac replace. */
+            free(cr_peer_name);
+            continue;
+        }
+        free(cr_peer_name);
 
         /* Table 65, priority 150.
          * =======================
@@ -787,7 +800,8 @@  consider_port_binding(struct ovsdb_idl_index *sbrec_port_binding_by_name,
                         &match, ofpacts_p, &binding->header_.uuid);
 
         if (!strcmp(binding->type, "localnet")) {
-            put_replace_router_port_mac_flows(binding, chassis,
+            put_replace_router_port_mac_flows(sbrec_port_binding_by_name,
+                                              binding, chassis, active_tunnels,
                                               local_datapaths, ofpacts_p,
                                               ofport, flow_table);
         }
diff --git a/controller/pinctrl.c b/controller/pinctrl.c
index f27718f..e8abe0b 100644
--- a/controller/pinctrl.c
+++ b/controller/pinctrl.c
@@ -3755,24 +3755,6 @@  get_localnet_vifs_l3gwports(
     sbrec_port_binding_index_destroy_row(target);
 }
 
-static bool
-pinctrl_is_chassis_resident(struct ovsdb_idl_index *sbrec_port_binding_by_name,
-                            const struct sbrec_chassis *chassis,
-                            const struct sset *active_tunnels,
-                            const char *port_name)
-{
-    const struct sbrec_port_binding *pb
-        = lport_lookup_by_name(sbrec_port_binding_by_name, port_name);
-    if (!pb || !pb->chassis) {
-        return false;
-    }
-    if (strcmp(pb->type, "chassisredirect")) {
-        return pb->chassis == chassis;
-    } else {
-        return ha_chassis_group_is_active(pb->ha_chassis_group,
-                                          active_tunnels, chassis);
-    }
-}
 
 /* Extracts the mac, IPv4 and IPv6 addresses, and logical port from
  * 'addresses' which should be of the format 'MAC [IP1 IP2 ..]
@@ -3853,7 +3835,7 @@  consider_nat_address(struct ovsdb_idl_index *sbrec_port_binding_by_name,
     char *lport = NULL;
     if (!extract_addresses_with_port(nat_address, laddrs, &lport)
         || (!lport && !strcmp(pb->type, "patch"))
-        || (lport && !pinctrl_is_chassis_resident(
+        || (lport && !lport_is_chassis_resident(
                 sbrec_port_binding_by_name, chassis,
                 active_tunnels, lport))) {
         destroy_lport_addresses(laddrs);
diff --git a/tests/ovn.at b/tests/ovn.at
index 71eb390..1613c0b 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -29,6 +29,12 @@  m4_define([OVN_CHECK_PACKETS],
   [ovn_check_packets__ "$1" "$2"
    AT_CHECK([sort $rcv_text], [0], [expout])])
 
+m4_define([OVN_CHECK_PACKETS_REMOVE_BROADCAST],
+  [ovn_check_packets__ "$1" "$2"
+   echo "received_text=$rcv_text"
+   sed -i '/ffffffffffff/d' $rcv_text
+   AT_CHECK([sort $rcv_text], [0], [expout])])
+
 AT_BANNER([OVN components])
 
 AT_SETUP([ovn -- lexer])
@@ -15009,3 +15015,310 @@  on_exit 'kill $(cat ovn-nbctl.pid)'
 AT_CHECK([ovn-nbctl -u $sockfile show])
 
 AT_CLEANUP
+
+
+AT_SETUP([ovn -- 2 HVs, 2 lports/HV, localnet ports, DVR N-S ARP handling])
+ovn_start
+
+# In this test cases we create 3 switches, all connected to same
+# physical network (through br-phys on each HV). LS1 and LS2 have
+# 1 VIF each. Each HV has 1 VIF port. The first digit
+# of VIF port name indicates the hypervisor it is bound to, e.g.
+# lp23 means VIF 3 on hv2.
+#
+# All the switches are connected to a logical router "router".
+#
+# Each switch's VLAN tag and their logical switch ports are:
+#   - ls1:
+#       - tagged with VLAN 101
+#       - ports: lp11
+#   - ls2:
+#       - tagged with VLAN 201
+#       - ports: lp22
+#   - ls-underlay:
+#       - tagged with VLAN 1000
+# Note: a localnet port is created for each switch to connect to
+# physical network.
+
+for i in 1 2; do
+    ls_name=ls$i
+    ovn-nbctl ls-add $ls_name
+    ln_port_name=ln$i
+    if test $i -eq 1; then
+        ovn-nbctl lsp-add $ls_name $ln_port_name "" 101
+    elif test $i -eq 2; then
+        ovn-nbctl lsp-add $ls_name $ln_port_name "" 201
+    fi
+    ovn-nbctl lsp-set-addresses $ln_port_name unknown
+    ovn-nbctl lsp-set-type $ln_port_name localnet
+    ovn-nbctl lsp-set-options $ln_port_name network_name=phys
+done
+
+# lsp_to_ls LSP
+#
+# Prints the name of the logical switch that contains LSP.
+lsp_to_ls () {
+    case $1 in dnl (
+        lp?[[11]]) echo ls1 ;; dnl (
+        lp?[[12]]) echo ls2 ;; dnl (
+        *) AT_FAIL_IF([:]) ;;
+    esac
+}
+
+vif_to_hv () {
+    case $1 in dnl (
+        vif[[1]]?) echo hv1 ;; dnl (
+        vif[[2]]?) echo hv2 ;; dnl (
+        vif?[[north]]?) echo hv4 ;; dnl (
+        *) AT_FAIL_IF([:]) ;;
+    esac
+}
+
+ip_to_hex() {
+       printf "%02x%02x%02x%02x" "$@"
+}
+
+net_add n1
+for i in 1 2; do
+    sim_add hv$i
+    as hv$i
+    ovs-vsctl add-br br-phys
+    ovs-vsctl set open . external-ids:ovn-bridge-mappings=phys:br-phys
+    ovs-vsctl set open . external-ids:ovn-chassis-mac-mappings="phys:aa:bb:cc:dd:ee:$i$i"
+    ovn_attach n1 br-phys 192.168.0.$i
+
+    ovs-vsctl add-port br-int vif$i$i -- \
+        set Interface vif$i$i external-ids:iface-id=lp$i$i \
+                              options:tx_pcap=hv$i/vif$i$i-tx.pcap \
+                              options:rxq_pcap=hv$i/vif$i$i-rx.pcap \
+                              ofport-request=$i$i
+
+    lsp_name=lp$i$i
+    ls_name=$(lsp_to_ls $lsp_name)
+
+    ovn-nbctl lsp-add $ls_name $lsp_name
+    ovn-nbctl lsp-set-addresses $lsp_name "f0:00:00:00:00:$i$i 192.168.$i.$i"
+    ovn-nbctl lsp-set-port-security $lsp_name f0:00:00:00:00:$i$i
+
+    OVS_WAIT_UNTIL([test x`ovn-nbctl lsp-get-up $lsp_name` = xup])
+
+done
+
+ovn-nbctl ls-add ls-underlay
+ovn-nbctl lsp-add ls-underlay ln3 "" 1000
+ovn-nbctl lsp-set-addresses ln3 unknown
+ovn-nbctl lsp-set-type ln3 localnet
+ovn-nbctl lsp-set-options ln3 network_name=phys
+
+ovn-nbctl ls-add ls-north
+ovn-nbctl lsp-add ls-north ln4 "" 1000
+ovn-nbctl lsp-set-addresses ln4 unknown
+ovn-nbctl lsp-set-type ln4 localnet
+ovn-nbctl lsp-set-options ln4 network_name=phys
+
+# Add a VM on ls-north
+ovn-nbctl lsp-add ls-north lp-north
+ovn-nbctl lsp-set-addresses lp-north "f0:f0:00:00:00:11 172.31.0.10"
+ovn-nbctl lsp-set-port-security lp-north f0:f0:00:00:00:11
+
+# Add 3rd hypervisor
+sim_add hv3
+as hv3 ovs-vsctl add-br br-phys
+as hv3 ovs-vsctl set open . external-ids:ovn-bridge-mappings=phys:br-phys
+as hv3 ovs-vsctl set open . external-ids:ovn-chassis-mac-mappings="phys:aa:bb:cc:dd:ee:33"
+as hv3 ovn_attach n1 br-phys 192.168.0.3
+
+# Add 4th hypervisor
+sim_add hv4
+as hv4 ovs-vsctl add-br br-phys
+as hv4 ovs-vsctl set open . external-ids:ovn-bridge-mappings=phys:br-phys
+as hv4 ovs-vsctl set open . external-ids:ovn-chassis-mac-mappings="phys:aa:bb:cc:dd:ee:44"
+as hv4 ovn_attach n1 br-phys 192.168.0.4
+
+as hv4 ovs-vsctl add-port br-int vif-north -- \
+        set Interface vif-north external-ids:iface-id=lp-north \
+                              options:tx_pcap=hv4/vif-north-tx.pcap \
+                              options:rxq_pcap=hv4/vif-north-rx.pcap \
+                              ofport-request=44
+
+ovn-nbctl lr-add router
+ovn-nbctl lrp-add router router-to-ls1 00:00:01:01:02:03 192.168.1.3/24
+ovn-nbctl lrp-add router router-to-ls2 00:00:01:01:02:05 192.168.2.3/24
+ovn-nbctl lrp-add router router-to-underlay 00:00:01:01:02:07 172.31.0.1/24
+
+ovn-nbctl lsp-add ls1 ls1-to-router -- set Logical_Switch_Port ls1-to-router type=router \
+          options:router-port=router-to-ls1 -- lsp-set-addresses ls1-to-router router
+ovn-nbctl lsp-add ls2 ls2-to-router -- set Logical_Switch_Port ls2-to-router type=router \
+          options:router-port=router-to-ls2 -- lsp-set-addresses ls2-to-router router
+ovn-nbctl lsp-add ls-underlay underlay-to-router -- set Logical_Switch_Port \
+                              underlay-to-router type=router \
+                              options:router-port=router-to-underlay \
+                              -- lsp-set-addresses underlay-to-router router
+
+
+OVN_POPULATE_ARP
+
+# lsp_to_ls LSP
+#
+# Prints the name of the logical switch that contains LSP.
+lsp_to_ls () {
+    case $1 in dnl (
+        lp?[[11]]) echo ls1 ;; dnl (
+        lp?[[12]]) echo ls2 ;; dnl (
+        *) AT_FAIL_IF([:]) ;;
+    esac
+}
+
+vif_to_ls () {
+    case $1 in dnl (
+        vif?[[11]]) echo ls1 ;; dnl (
+        vif?[[12]]) echo ls2 ;; dnl (
+        vif-north) echo ls-north ;; dnl (
+        *) AT_FAIL_IF([:]) ;;
+    esac
+}
+
+hv_to_num () {
+    case $1 in dnl (
+        hv1) echo 1 ;; dnl (
+        hv2) echo 2 ;; dnl (
+        hv3) echo 3 ;; dnl (
+        hv4) echo 4 ;; dnl (
+        *) AT_FAIL_IF([:]) ;;
+    esac
+}
+
+vif_to_num () {
+    case $1 in dnl (
+        vif22) echo 22 ;; dnl (
+        vif21) echo 21 ;; dnl (
+        vif11) echo 11 ;; dnl (
+        *) AT_FAIL_IF([:]) ;;
+    esac
+}
+
+vif_to_hv () {
+    case $1 in dnl (
+        vif[[1]]?) echo hv1 ;; dnl (
+        vif[[2]]?) echo hv2 ;; dnl (
+        vif-north) echo hv4 ;; dnl (
+        *) AT_FAIL_IF([:]) ;;
+    esac
+}
+
+vif_to_lrp () {
+    echo router-to-`vif_to_ls $1`
+}
+
+ip_to_hex() {
+       printf "%02x%02x%02x%02x" "$@"
+}
+
+# Dump a bunch of info helpful for debugging if there's a failure.
+
+echo "------ OVN dump ------"
+ovn-nbctl show
+ovn-sbctl show
+ovn-sbctl list port_binding
+ovn-sbctl list mac_binding
+
+echo "------ hv1 dump ------"
+as hv1 ovs-vsctl show
+as hv1 ovs-vsctl list Open_Vswitch
+
+echo "------ hv2 dump ------"
+as hv2 ovs-vsctl show
+as hv2 ovs-vsctl list Open_Vswitch
+
+echo "------ hv3 dump ------"
+as hv3 ovs-vsctl show
+as hv3 ovs-vsctl list Open_Vswitch
+
+echo "------ hv4 dump ------"
+as hv4 ovs-vsctl show
+as hv4 ovs-vsctl list Open_Vswitch
+
+# test_arp INPORT SHA SPA TPA [REPLY_HA]
+#
+# Causes a packet to be received on INPORT.  The packet is an ARP
+# request with SHA, SPA, and TPA as specified.  If REPLY_HA is provided, then
+# it should be the hardware address of the target to expect to receive in an
+# ARP reply; otherwise no reply is expected.
+#
+# INPORT is an logical switch port number, e.g. 11 for vif11.
+# SHA and REPLY_HA are each 12 hex digits.
+# SPA and TPA are each 8 hex digits.
+test_arp() {
+    local inport=$1 sha=$2 spa=$3 tpa=$4 reply_ha=$5
+    local request=ffffffffffff${sha}08060001080006040001${sha}${spa}ffffffffffff${tpa}
+    hv=`vif_to_hv $inport`
+    as $hv ovs-appctl netdev-dummy/receive $inport $request
+
+    if test X$reply_ha = X; then
+        # Expect to receive the broadcast ARP on the other logical switch ports
+        # if no reply is expected.
+        local i j
+        for i in 1 2 3; do
+            for j in 1 2 3; do
+                if test $i$j != $inport; then
+                    echo $request >> $i$j.expected
+                fi
+            done
+        done
+    else
+        # Expect to receive the reply, if any.
+        local reply=${sha}${reply_ha}08060001080006040002${reply_ha}${tpa}${sha}${spa}
+        local reply_vid=${sha}${reply_ha}810003e808060001080006040002${reply_ha}${tpa}${sha}${spa}
+        echo $reply_vid >> ${inport}_vid.expected
+        echo $reply >> $inport.expected
+    fi
+}
+
+sip=`ip_to_hex 172 31 0 10`
+tip=`ip_to_hex 172 31 0 1`
+
+# Set a hypervisor as gateway chassis, for router port 172.31.0.1
+ovn-nbctl lrp-set-gateway-chassis router-to-underlay hv3
+ovn-nbctl --wait=sb sync
+sleep 2
+
+test_arp vif-north f0f000000011 $sip $tip 000001010207
+
+sleep 1
+
+# Confirm that vif-north gets a single ARP reply
+OVN_CHECK_PACKETS_REMOVE_BROADCAST([hv4/vif-north-tx.pcap], [vif-north.expected])
+
+# Confirm that only redirect chassis allowed arp resolution.
+OVN_CHECK_PACKETS_REMOVE_BROADCAST([hv3/br-phys_n1-tx.pcap], [vif-north_vid.expected])
+AT_CHECK([grep 000001010207 hv3/br-phys_n1-tx.packets | wc -l], [0], [[1
+]])
+
+# Confirm that other OVN chassis did not generate ARP reply.
+$PYTHON "$top_srcdir/utilities/ovs-pcap.in" hv1/br-phys_n1-tx.pcap > hv1/br-phys_n1-tx.packets
+$PYTHON "$top_srcdir/utilities/ovs-pcap.in" hv2/br-phys_n1-tx.pcap > hv2/br-phys_n1-tx.packets
+
+AT_CHECK([grep 000001010207 hv1/br-phys_n1-tx.packets | wc -l], [0], [[0
+]])
+AT_CHECK([grep 000001010207 hv2/br-phys_n1-tx.packets | wc -l], [0], [[0
+]])
+
+echo "----------- Post Traffic hv1 dump -----------"
+as hv1 ovs-ofctl -O OpenFlow13 dump-flows br-int
+as hv1 ovs-appctl fdb/show br-phys
+
+echo "----------- Post Traffic hv2 dump -----------"
+as hv2 ovs-ofctl -O OpenFlow13 dump-flows br-int
+as hv2 ovs-appctl fdb/show br-phys
+
+echo "----------- Post Traffic hv3 dump -----------"
+as hv3 ovs-ofctl -O OpenFlow13 dump-flows br-int
+as hv3 ovs-appctl fdb/show br-phys
+
+echo "----------- Post Traffic hv4 dump -----------"
+as hv4 ovs-ofctl -O OpenFlow13 dump-flows br-int
+as hv4 ovs-appctl fdb/show br-phys
+
+OVN_CLEANUP([hv1],[hv2],[hv3],[hv4])
+
+AT_CLEANUP