[ovs-dev] [PATCH] ovn-controller: Inject GARPs to logical switch pipeline to update neighbors

Daniel Alvarez Sanchez dalvarez at redhat.com
Mon Dec 3 17:58:20 UTC 2018


This patch is making test "ovn -- vlan traffic for external network
with distributed router gateway port" fail at [0].
The reason seems to be that now we're sending GARPs so n_packets is
not 0 anymore for the flow that the test is checking:

./ovn.at:8816: as hv1 ovs-ofctl dump-flows br-int table=65 | grep
"priority=100,reg15=0x1,metadata=0x2" \
| grep actions=clone | grep -v n_packets=0 | wc -l

cookie=0x0, duration=6.454s, table=65, n_packets=2, n_bytes=84,
idle_age=3, priority=100,reg15=0x1,metadata=0x2
actions=clone(ct_clear,load:0->NXM_NX_REG11[],load:0->NXM_NX_REG12[],load:0->NXM_NX_REG13[],load:0x2->NXM_NX_REG11[],load:0x7->NXM_NX_REG12[],load:0x1->OXM_OF_METADATA[],load:0x1->NXM_NX_REG14[],load:0->NXM_NX_REG10[],load:0->NXM_NX_REG15[],load:0->NXM_NX_REG0[],load:0->NXM_NX_REG1[],load:0->NXM_NX_REG2[],load:0->NXM_NX_REG3[],load:0->NXM_NX_REG4[],load:0->NXM_NX_REG5[],load:0->NXM_NX_REG6[],load:0->NXM_NX_REG7[],load:0->NXM_NX_REG8[],load:0->NXM_NX_REG9[],load:0->NXM_OF_IN_PORT[],resubmit(,8))

As you can see n_packets is 2 at this point. I'm waiting for inputs
here from authors of the patch [1] (Numan, Anil and Guru) or other
folks. This is the only test failing and if we would expect 2 packets
instead of comment out that particular line, the rest of the test
passes.

Thanks a lot!
Daniel

[0] https://github.com/openvswitch/ovs/blob/master/tests/ovn.at#L8816
[1] https://github.com/openvswitch/ovs/commit/85706c34d53d4810f54bec1de662392a3c06a996
On Mon, Dec 3, 2018 at 6:54 PM Daniel Alvarez <dalvarez at redhat.com> wrote:
>
> Prior to this patch, GARPs announcing NAT addresses or new VIFs
> were sent out to localnet ofport through an output action.
> This can lead to problems since local datapaths won't get those
> GARPs and ovn-controller won't update MAC_Binding entries (as
> upstream switch will not send back the GARP to this port hence
> other logical routers won't update their neighbours).
>
> This patch is changing the behavior so that GARPs get injected
> to OVN pipeline of the external switch. This way, they'll get
> broadcasted to local pipelines and also sent out to the external
> network through the localnet port.
>
> Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2018-October/047604.html
> Signed-off-by: Daniel Alvarez <dalvarez at redhat.com>
> ---
>  ovn/controller/pinctrl.c |  62 ++++++++++--------------
>  tests/ovn.at             | 100 +++++++++++++++++++++++++++++++++++++++
>  2 files changed, 125 insertions(+), 37 deletions(-)
>
> diff --git a/ovn/controller/pinctrl.c b/ovn/controller/pinctrl.c
> index 56539a891..3e8af41cc 100644
> --- a/ovn/controller/pinctrl.c
> +++ b/ovn/controller/pinctrl.c
> @@ -2019,8 +2019,8 @@ struct garp_data {
>      ovs_be32 ipv4;               /* Ipv4 address of port. */
>      long long int announce_time; /* Next announcement in ms. */
>      int backoff;                 /* Backoff for the next announcement. */
> -    ofp_port_t ofport;           /* ofport used to output this GARP. */
> -    int tag;                     /* VLAN tag of this GARP packet, or -1. */
> +    uint32_t dp_key;             /* Datapath used to output this GARP. */
> +    uint32_t port_key;           /* Port to inject the GARP into. */
>  };
>
>  /* Contains GARPs to be sent. */
> @@ -2043,37 +2043,24 @@ destroy_send_garps(void)
>  }
>
>  static void
> -add_garp(const char *name, ofp_port_t ofport, int tag,
> -         const struct eth_addr ea, ovs_be32 ip)
> +add_garp(const char *name, const struct eth_addr ea, ovs_be32 ip,
> +         uint32_t dp_key, uint32_t port_key)
>  {
>      struct garp_data *garp = xmalloc(sizeof *garp);
>      garp->ea = ea;
>      garp->ipv4 = ip;
>      garp->announce_time = time_msec() + 1000;
>      garp->backoff = 1;
> -    garp->ofport = ofport;
> -    garp->tag = tag;
> +    garp->dp_key = dp_key;
> +    garp->port_key = port_key;
>      shash_add(&send_garp_data, name, garp);
>  }
>
>  /* Add or update a vif for which GARPs need to be announced. */
>  static void
>  send_garp_update(const struct sbrec_port_binding *binding_rec,
> -                 struct simap *localnet_ofports,
> -                 const struct hmap *local_datapaths,
>                   struct shash *nat_addresses)
>  {
> -    /* Find the localnet ofport to send this GARP. */
> -    struct local_datapath *ld
> -        = get_local_datapath(local_datapaths,
> -                             binding_rec->datapath->tunnel_key);
> -    if (!ld || !ld->localnet_port) {
> -        return;
> -    }
> -    ofp_port_t ofport = u16_to_ofp(simap_get(localnet_ofports,
> -                                             ld->localnet_port->logical_port));
> -    int tag = ld->localnet_port->n_tag ? *ld->localnet_port->tag : -1;
> -
>      volatile struct garp_data *garp = NULL;
>      /* Update GARP for NAT IP if it exists.  Consider port bindings with type
>       * "l3gateway" for logical switch ports attached to gateway routers, and
> @@ -2090,11 +2077,13 @@ send_garp_update(const struct sbrec_port_binding *binding_rec,
>                                                  laddrs->ipv4_addrs[i].addr_s);
>                  garp = shash_find_data(&send_garp_data, name);
>                  if (garp) {
> -                    garp->ofport = ofport;
> -                    garp->tag = tag;
> +                    garp->dp_key = binding_rec->datapath->tunnel_key;
> +                    garp->port_key = binding_rec->tunnel_key;
>                  } else {
> -                    add_garp(name, ofport, tag, laddrs->ea,
> -                             laddrs->ipv4_addrs[i].addr);
> +                    add_garp(name, laddrs->ea,
> +                             laddrs->ipv4_addrs[i].addr,
> +                             binding_rec->datapath->tunnel_key,
> +                             binding_rec->tunnel_key);
>                  }
>                  free(name);
>              }
> @@ -2107,7 +2096,8 @@ send_garp_update(const struct sbrec_port_binding *binding_rec,
>      /* Update GARP for vif if it exists. */
>      garp = shash_find_data(&send_garp_data, binding_rec->logical_port);
>      if (garp) {
> -        garp->ofport = ofport;
> +        garp->dp_key = binding_rec->datapath->tunnel_key;
> +        garp->port_key = binding_rec->tunnel_key;
>          return;
>      }
>
> @@ -2120,8 +2110,9 @@ send_garp_update(const struct sbrec_port_binding *binding_rec,
>              continue;
>          }
>
> -        add_garp(binding_rec->logical_port, ofport, tag,
> -                 laddrs.ea, laddrs.ipv4_addrs[0].addr);
> +        add_garp(binding_rec->logical_port,
> +                 laddrs.ea, laddrs.ipv4_addrs[0].addr,
> +                 binding_rec->datapath->tunnel_key, binding_rec->tunnel_key);
>
>          destroy_lport_addresses(&laddrs);
>          break;
> @@ -2150,16 +2141,15 @@ send_garp(struct garp_data *garp, long long int current_time)
>      compose_arp(&packet, ARP_OP_REQUEST, garp->ea, eth_addr_zero,
>                  true, garp->ipv4, garp->ipv4);
>
> -    /* Compose a GARP request packet's vlan if exist. */
> -    if (garp->tag >= 0) {
> -        eth_push_vlan(&packet, htons(ETH_TYPE_VLAN), htons(garp->tag));
> -    }
> -
> -    /* Compose actions.  The garp request is output on localnet ofport. */
> +    /* Inject GARP request. */
>      uint64_t ofpacts_stub[4096 / 8];
>      struct ofpbuf ofpacts = OFPBUF_STUB_INITIALIZER(ofpacts_stub);
>      enum ofp_version version = rconn_get_version(swconn);
> -    ofpact_put_OUTPUT(&ofpacts)->port = garp->ofport;
> +    put_load(garp->dp_key, MFF_LOG_DATAPATH, 0, 64, &ofpacts);
> +    put_load(garp->port_key, MFF_LOG_INPORT, 0, 32, &ofpacts);
> +    struct ofpact_resubmit *resubmit = ofpact_put_RESUBMIT(&ofpacts);
> +    resubmit->in_port = OFPP_CONTROLLER;
> +    resubmit->table_id = OFTABLE_LOG_INGRESS_PIPELINE;
>
>      struct ofputil_packet_out po = {
>          .packet = dp_packet_data(&packet),
> @@ -2489,8 +2479,7 @@ send_garp_run(struct ovsdb_idl_index *sbrec_chassis_by_name,
>          const struct sbrec_port_binding *pb = lport_lookup_by_name(
>              sbrec_port_binding_by_name, iface_id);
>          if (pb) {
> -            send_garp_update(pb, &localnet_ofports, local_datapaths,
> -                             &nat_addresses);
> +            send_garp_update(pb, &nat_addresses);
>          }
>      }
>
> @@ -2500,8 +2489,7 @@ send_garp_run(struct ovsdb_idl_index *sbrec_chassis_by_name,
>          const struct sbrec_port_binding *pb
>              = lport_lookup_by_name(sbrec_port_binding_by_name, gw_port);
>          if (pb) {
> -            send_garp_update(pb, &localnet_ofports, local_datapaths,
> -                             &nat_addresses);
> +            send_garp_update(pb, &nat_addresses);
>          }
>      }
>
> diff --git a/tests/ovn.at b/tests/ovn.at
> index 2db3f675a..466bfc6ed 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -11727,3 +11727,103 @@ OVN_CHECK_PACKETS([hv2/vif1-tx.pcap], [expected])
>
>  OVN_CLEANUP([hv1],[hv2])
>  AT_CLEANUP
> +
> +AT_SETUP([ovn -- neighbor update on same HV])
> +AT_SKIP_IF([test $HAVE_PYTHON = no])
> +ovn_start
> +
> +# Logical network:
> +# A public switch (pub) with a localnet port connected to two LRs (lr0 and lr1)
> +# each with a distributed gateway port.
> +# Two VMs: lp0 on sw0 connected to lr0
> +#          lp1 on sw1 connected to lr1
> +#
> +# This test adds a floating IP to each VM so when they are bound to the same
> +# hypervisor, it checks that the GARP sent by ovn-controller causes the
> +# MAC_Binding entries to be updated properly on each logical router.
> +# It will also capture packets on the physical interface to make sure that the
> +# GARPs have been sent out to the external network as well.
> +
> +# Create logical switches
> +ovn-nbctl ls-add sw0
> +ovn-nbctl ls-add sw1
> +ovn-nbctl ls-add pub
> +
> +# Created localnet port on public switch
> +ovn-nbctl lsp-add pub ln-pub
> +ovn-nbctl lsp-set-type ln-pub localnet
> +ovn-nbctl lsp-set-addresses ln-pub unknown
> +ovn-nbctl lsp-set-options ln-pub network_name=phys
> +
> +# Create logical routers and connect them to public switch
> +ovn-nbctl create Logical_Router name=lr0
> +ovn-nbctl create Logical_Router name=lr1
> +
> +ovn-nbctl lrp-add lr0 lr0-pub f0:00:00:00:00:01 172.24.4.220/24
> +ovn-nbctl lsp-add pub pub-lr0 -- set Logical_Switch_Port pub-lr0 \
> +    type=router options:router-port=lr0-pub options:nat-addresses="router" addresses="router"
> +ovn-nbctl lrp-add lr1 lr1-pub f0:00:00:00:01:01 172.24.4.221/24
> +ovn-nbctl lsp-add pub pub-lr1 -- set Logical_Switch_Port pub-lr1 \
> +    type=router options:router-port=lr1-pub options:nat-addresses="router" addresses="router"
> +
> +ovn-nbctl lrp-set-gateway-chassis lr0-pub hv1 10
> +ovn-nbctl lrp-set-gateway-chassis lr1-pub hv1 10
> +
> +# Connect sw0 and sw1 to lr0 and lr1
> +ovn-nbctl lrp-add lr0 lr0-sw0 00:00:00:00:ff:01 10.0.0.254/24
> +ovn-nbctl lsp-add sw0 sw0-lr0 -- set Logical_Switch_Port sw0-lr0 type=router \
> +    options:router-port=lr0-sw0 addresses="router"
> +ovn-nbctl lrp-add lr1 lr1-sw1 00:00:00:00:ff:02 20.0.0.254/24
> +ovn-nbctl lsp-add sw1 sw1-lr1 -- set Logical_Switch_Port sw1-lr1 type=router \
> +    options:router-port=lr1-sw1 addresses="router"
> +
> +
> +# Add SNAT rules
> +ovn-nbctl lr-nat-add lr0 snat 172.24.4.220 10.0.0.0/24
> +ovn-nbctl lr-nat-add lr1 snat 172.24.4.221 20.0.0.0/24
> +
> +net_add n1
> +sim_add hv1
> +as hv1
> +ovs-vsctl add-br br-phys
> +ovn_attach n1 br-phys 172.24.4.1
> +ovs-vsctl set open . external-ids:ovn-bridge-mappings=phys:br-phys
> +
> +ovs-vsctl add-port br-int vif0 -- set Interface vif0 external-ids:iface-id=lp0
> +ovs-vsctl add-port br-int vif1 -- set Interface vif1 external-ids:iface-id=lp1
> +
> +ovn-nbctl lsp-add sw0 lp0
> +ovn-nbctl lsp-add sw1 lp1
> +ovn-nbctl lsp-set-addresses lp0 "50:54:00:00:00:01 10.0.0.10"
> +ovn-nbctl lsp-set-addresses lp1 "50:54:00:00:00:02 20.0.0.10"
> +
> +OVS_WAIT_UNTIL([test x`ovn-nbctl lsp-get-up lp0` = xup])
> +OVS_WAIT_UNTIL([test x`ovn-nbctl lsp-get-up lp1` = xup])
> +
> +# Create two floating IPs, one for each VIF
> +ovn-nbctl lr-nat-add lr0 dnat_and_snat 172.24.4.100 10.0.0.10
> +ovn-nbctl lr-nat-add lr1 dnat_and_snat 172.24.4.200 20.0.0.10
> +
> +# Check that the MAC_Binding entries have been properly created
> +OVS_WAIT_UNTIL([test `ovn-sbctl find mac_binding logical_port="lr0-pub" ip="172.24.4.200" | wc -l` -gt 0])
> +OVS_WAIT_UNTIL([test `ovn-sbctl find mac_binding logical_port="lr1-pub" ip="172.24.4.100" | wc -l` -gt 0])
> +
> +# Check that the GARPs went also to the external physical network
> +# Wait until at least 4 packets have arrived and copy them to a separate file as
> +# more GARPs are expected in the capture in order to avoid race conditions.
> +OVS_WAIT_UNTIL([test `$PYTHON "$top_srcdir/utilities/ovs-pcap.in" hv1/br-phys-tx.pcap | wc -l` -gt 4])
> +$PYTHON "$top_srcdir/utilities/ovs-pcap.in" hv1/br-phys-tx.pcap | head -n4 > hv1/br-phys-tx4.pcap
> +
> +# GARP for lp0 172.24.4.100 on lr0-pub MAC (f0:00:00:00:00:01)
> +echo "fffffffffffff0000000000108060001080006040001f00000000001ac180464000000000000ac180464" > expout
> +# GARP for 172.24.4.220 on lr0-pub (f0:00:00:00:00:01)
> +echo "fffffffffffff0000000000108060001080006040001f00000000001ac1804dc000000000000ac1804dc" >> expout
> +# GARP for lp1 172.24.4.200 on lr1-pub MAC (f0:00:00:00:01:01)
> +echo "fffffffffffff0000000010108060001080006040001f00000000101ac1804c8000000000000ac1804c8" >> expout
> +# GARP for 172.24.4.221 on lr1-pub (f0:00:00:00:01:01)
> +echo "fffffffffffff0000000010108060001080006040001f00000000101ac1804dd000000000000ac1804dd" >> expout
> +AT_CHECK([sort hv1/br-phys-tx4.pcap], [0], [expout])
> +#OVN_CHECK_PACKETS([hv1/br-phys-tx4.pcap], [br-phys.expected])
> +
> +OVN_CLEANUP([hv1])
> +AT_CLEANUP
> --
>


More information about the dev mailing list