On Wed, Dec 5, 2018 at 5:03 AM Han Zhou <[email protected]> wrote: > On Tue, Dec 4, 2018 at 10:14 AM Daniel Alvarez <[email protected]> > 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 <[email protected]> > > --- > > > > v1->v2 > > Fix VLAN tests to account for the GARPs received by the local pipeline. > > Remove localnet_ofports parameter as it's not used anymore. > > > > ovn/controller/pinctrl.c | 80 +++++++++---------------- > > tests/ovn.at | 124 +++++++++++++++++++++++++++++++++------ > > 2 files changed, 135 insertions(+), 69 deletions(-) > > > > diff --git a/ovn/controller/pinctrl.c b/ovn/controller/pinctrl.c > > index 56539a891..3cd2ad718 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), > > @@ -2194,7 +2184,6 @@ get_localnet_vifs_l3gwports( > > const struct sbrec_chassis *chassis, > > const struct hmap *local_datapaths, > > struct sset *localnet_vifs, > > - struct simap *localnet_ofports, > > struct sset *local_l3gw_ports) > > { > > for (int i = 0; i < br_int->n_ports; i++) { > > @@ -2209,20 +2198,14 @@ get_localnet_vifs_l3gwports( > > } > > const char *localnet = smap_get(&port_rec->external_ids, > > "ovn-localnet-port"); > > + if (localnet) { > > + continue; > > + } > > for (int j = 0; j < port_rec->n_interfaces; j++) { > > const struct ovsrec_interface *iface_rec = > port_rec->interfaces[j]; > > if (!iface_rec->n_ofport) { > > continue; > > } > > - /* Get localnet port with its ofport. */ > > - if (localnet) { > > - int64_t ofport = iface_rec->ofport[0]; > > - if (ofport < 1 || ofport > ofp_to_u16(OFPP_MAX)) { > > - continue; > > - } > > - simap_put(localnet_ofports, localnet, ofport); > > - continue; > > - } > > /* Get localnet vif. */ > > const char *iface_id = smap_get(&iface_rec->external_ids, > > "iface-id"); > > @@ -2458,7 +2441,6 @@ send_garp_run(struct ovsdb_idl_index > *sbrec_chassis_by_name, > > struct sset localnet_vifs = SSET_INITIALIZER(&localnet_vifs); > > struct sset local_l3gw_ports = SSET_INITIALIZER(&local_l3gw_ports); > > struct sset nat_ip_keys = SSET_INITIALIZER(&nat_ip_keys); > > - struct simap localnet_ofports = > SIMAP_INITIALIZER(&localnet_ofports); > > struct shash nat_addresses; > > > > shash_init(&nat_addresses); > > @@ -2466,8 +2448,7 @@ send_garp_run(struct ovsdb_idl_index > *sbrec_chassis_by_name, > > get_localnet_vifs_l3gwports(sbrec_port_binding_by_datapath, > > sbrec_port_binding_by_name, > > br_int, chassis, local_datapaths, > > - &localnet_vifs, &localnet_ofports, > > - &local_l3gw_ports); > > + &localnet_vifs, &local_l3gw_ports); > > > > get_nat_addresses_and_keys(sbrec_chassis_by_name, > > sbrec_port_binding_by_name, > > @@ -2489,8 +2470,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 +2480,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); > > } > > } > > > > @@ -2516,7 +2495,6 @@ send_garp_run(struct ovsdb_idl_index > *sbrec_chassis_by_name, > > } > > sset_destroy(&localnet_vifs); > > sset_destroy(&local_l3gw_ports); > > - simap_destroy(&localnet_ofports); > > > > SHASH_FOR_EACH_SAFE (iter, next, &nat_addresses) { > > struct lport_addresses *laddrs = iter->data; > > diff --git a/tests/ovn.at b/tests/ovn.at > > index 2db3f675a..975229af7 100644 > > --- a/tests/ovn.at > > +++ b/tests/ovn.at > > @@ -8761,21 +8761,7 @@ src_mac="f00000010203" > > dst_mac="000001010203" > > > > > packet=${foo_mac}${foo1_mac}08004500001c0000000040110000${foo1_ip}${dst_ip}0035111100080000 > > > > -as hv1 ovs-appctl netdev-dummy/receive hv1-vif1 $packet > > -sleep 2 > > - > > -# ARP request packet for nexthop_ip to expect at outside1 > > > > -arp_request=ffffffffffff${gw_mac}08060001080006040001${gw_mac}${gw_ip}000000000000${nexthop_ip} > > -echo $arp_request >> hv3-vif1.expected > > -cat hv3-vif1.expected > expout > > -$PYTHON "$top_srcdir/utilities/ovs-pcap.in" hv3/vif1-tx.pcap | grep > ${nexthop_ip} | uniq > hv3-vif1 > > -AT_CHECK([sort hv3-vif1], [0], [expout]) > > - > > -# Send ARP reply from outside1 back to the router > > -reply_mac="f00000010204" > > > > -arp_reply=${gw_mac}${nexthop_mac}08060001080006040002${nexthop_mac}${nexthop_ip}${gw_mac}${gw_ip} > > - > > -as hv3 ovs-appctl netdev-dummy/receive hv3-vif1 $arp_reply > > +# Wait for GARPs announcing gw IP to arrive > > OVS_WAIT_UNTIL([ > > test `as hv2 ovs-ofctl dump-flows br-int | grep table=66 | \ > > grep actions=mod_dl_dst:f0:00:00:01:02:04 | wc -l` -eq 1 > > @@ -8806,15 +8792,17 @@ options:rxq_pcap=${pcap_file}-rx.pcap > > as hv1 reset_pcap_file br-ex_n2 hv1/br-ex_n2 > > as hv3 reset_pcap_file hv3-vif1 hv3/vif1 > > sleep 2 > > +# Take note of how many packets arrived on the VLAN switch before > generating > > +# further traffic > > +n_packets=`as hv1 ovs-ofctl dump-flows br-int table=65 | grep > "priority=100,reg15=0x1,metadata=0x2" | grep actions=clone | sed > 's/.*n_packets=\([[0-9]]\+\),.*/\1/'` > > as hv1 ovs-appctl netdev-dummy/receive hv1-vif1 $packet > > sleep 2 > > > > # On hv1, the packet should not go from vlan switch pipleline to router > > -# pipleine > > +# pipeline > > as hv1 ovs-ofctl dump-flows br-int > > - > > AT_CHECK([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], [0], [[0 > > +| grep actions=clone | grep -v n_packets=$n_packets | wc -l], [0], [[0 > > ]]) > > > > # On hv1, table 32 check that no packet goes via the tunnel port > > @@ -11727,3 +11715,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 > > -- > > 2.17.2 (Apple Git-113) > > > > _______________________________________________ > > dev mailing list > > [email protected] > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > Thanks for the quick update. > > Acked-by: Han Zhou <[email protected]> >
Acked-by: Numan Siddique <[email protected]> Numan > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
