On Fri, Jul 7, 2017 at 11:10 PM, Russell Bryant <russ...@ovn.org> wrote:
> On Wed, Jul 5, 2017 at 9:45 AM, Miguel Angel Ajo <majop...@redhat.com>
> wrote:
> > From: Venkata Anil Kommaddi <vkomm...@redhat.com>
> >
> > This patch extends gratuitous ARP support for NAT addresses so that it
> > applies to centralized NAT rules on a HA router.
> > Gratuitous ARP packets for centralized NAT rules on a HA router
> > are only generated on the active gateway chassis.
> >
> > Signed-off-by: Anil Venkata <vkomm...@redhat.com>
> > ---
> > ovn/controller/ovn-controller.c | 3 +-
> > ovn/controller/pinctrl.c | 45 ++++++++++++---
> > ovn/controller/pinctrl.h | 5 +-
> > tests/ovn.at | 121 ++++++++++++++++++++++++++++++
> ++++++++++
> > 4 files changed, 165 insertions(+), 9 deletions(-)
> >
> > diff --git a/ovn/controller/ovn-controller.c b/ovn/controller/ovn-
> controller.c
> > index 1418a15..0360c4c 100644
> > --- a/ovn/controller/ovn-controller.c
> > +++ b/ovn/controller/ovn-controller.c
> > @@ -655,7 +655,8 @@ main(int argc, char *argv[])
> > enum mf_field_id mff_ovn_geneve = ofctrl_run(br_int,
> >
> &pending_ct_zones);
> >
> > - pinctrl_run(&ctx, &lports, br_int, chassis,
> &local_datapaths);
> > + pinctrl_run(&ctx, &lports, br_int, chassis, &chassis_index,
> > + &local_datapaths, &active_tunnels);
> > update_ct_zones(&local_lports, &local_datapaths, &ct_zones,
> > ct_zone_bitmap, &pending_ct_zones);
> > if (ctx.ovs_idl_txn) {
> > diff --git a/ovn/controller/pinctrl.c b/ovn/controller/pinctrl.c
> > index 660233a..b7bcee6 100644
> > --- a/ovn/controller/pinctrl.c
> > +++ b/ovn/controller/pinctrl.c
> > @@ -23,6 +23,7 @@
> > #include "dirs.h"
> > #include "dp-packet.h"
> > #include "flow.h"
> > +#include "gchassis.h"
> > #include "lport.h"
> > #include "nx-match.h"
> > #include "ovn-controller.h"
> > @@ -72,7 +73,9 @@ static void send_garp_wait(void);
> > static void send_garp_run(const struct ovsrec_bridge *,
> > const struct sbrec_chassis *,
> > const struct lport_index *lports,
> > - struct hmap *local_datapaths);
> > + const struct chassis_index *chassis_index,
> > + struct hmap *local_datapaths,
> > + struct sset *active_tunnels);
> > static void pinctrl_handle_nd_na(const struct flow *ip_flow,
> > const struct match *md,
> > struct ofpbuf *userdata);
> > @@ -1016,7 +1019,9 @@ void
> > pinctrl_run(struct controller_ctx *ctx, const struct lport_index
> *lports,
> > const struct ovsrec_bridge *br_int,
> > const struct sbrec_chassis *chassis,
> > - struct hmap *local_datapaths)
> > + const struct chassis_index *chassis_index,
> > + struct hmap *local_datapaths,
> > + struct sset *active_tunnels)
> > {
> > char *target = xasprintf("unix:%s/%s.mgmt", ovs_rundir(),
> br_int->name);
> > if (strcmp(target, rconn_get_target(swconn))) {
> > @@ -1051,7 +1056,8 @@ pinctrl_run(struct controller_ctx *ctx, const
> struct lport_index *lports,
> > }
> >
> > run_put_mac_bindings(ctx, lports);
> > - send_garp_run(br_int, chassis, lports, local_datapaths);
> > + send_garp_run(br_int, chassis, lports, chassis_index,
> local_datapaths,
> > + active_tunnels);
> > }
> >
> > void
> > @@ -1490,11 +1496,26 @@ get_localnet_vifs_l3gwports(const struct
> ovsrec_bridge *br_int,
> > static bool
> > pinctrl_is_chassis_resident(const struct lport_index *lports,
> > const struct sbrec_chassis *chassis,
> > + const struct chassis_index *chassis_index,
> > + struct sset *active_tunnels,
> > const char *port_name)
> > {
> > const struct sbrec_port_binding *pb
> > = lport_lookup_by_name(lports, port_name);
> > - return pb && pb->chassis && pb->chassis == chassis;
> > + if (!pb || !pb->chassis) {
> > + return false;
> > + }
> > + if (strcmp(pb->type, "chassisredirect")) {
> > + return pb->chassis == chassis;
> > + } else {
> > + struct ovs_list *gateway_chassis =
> > + gateway_chassis_get_ordered(pb, chassis_index);
> > + bool active = gateway_chassis_is_active(gateway_chassis,
> > + chassis,
> > + active_tunnels);
> > + gateway_chassis_destroy(gateway_chassis);
> > + return active;
>
> This seems to be the core of this patch, but why is this change
> necessary? If we update the port binding chassis column to always be
> the active gateway_chassis, the original code would have the same
> result, right? What am I missing?
>
>
Good observation, may be we need to make the commit message more clear
about this, specially since I needed around 1 minute to remember why did we
have to get there.
The reasoning behind this patch is that only relying on the port binding
status from SBDB has the issue that when connection has been lost we won't
be able to update SBDB to release the port binding, or get the update that
other host has taken over it. And since the bfd status comes from the local
openvswitch database, we will be able to react anyway, stop sending gARPs,
(and removing all the other openflow rules in the case if
is_chassis_resident patch).
> > + }
> > }
> >
> > /* Extracts the mac, IPv4 and IPv6 addresses, and logical port from
> > @@ -1569,13 +1590,16 @@ consider_nat_address(const char *nat_address,
> > struct sset *nat_address_keys,
> > const struct lport_index *lports,
> > const struct sbrec_chassis *chassis,
> > + const struct chassis_index *chassis_index,
> > + struct sset *active_tunnels,
> > struct shash *nat_addresses)
> > {
> > struct lport_addresses *laddrs = xmalloc(sizeof *laddrs);
> > char *lport = NULL;
> > if (!extract_addresses_with_port(nat_address, laddrs, &lport)
> > || (!lport && !strcmp(pb->type, "patch"))
> > - || (lport && !pinctrl_is_chassis_resident(lports, chassis,
> lport))) {
> > + || (lport && !pinctrl_is_chassis_resident(
> > + lports, chassis, chassis_index, active_tunnels, lport))) {
> > destroy_lport_addresses(laddrs);
> > free(laddrs);
> > free(lport);
> > @@ -1598,6 +1622,8 @@ get_nat_addresses_and_keys(struct sset
> *nat_address_keys,
> > struct sset *local_l3gw_ports,
> > const struct lport_index *lports,
> > const struct sbrec_chassis *chassis,
> > + const struct chassis_index *chassis_index,
> > + struct sset *active_tunnels,
> > struct shash *nat_addresses)
> > {
> > const char *gw_port;
> > @@ -1612,6 +1638,7 @@ get_nat_addresses_and_keys(struct sset
> *nat_address_keys,
> > for (int i = 0; i < pb->n_nat_addresses; i++) {
> > consider_nat_address(pb->nat_addresses[i], pb,
> > nat_address_keys, lports, chassis,
> > + chassis_index, active_tunnels,
> > nat_addresses);
> > }
> > } else {
> > @@ -1622,6 +1649,7 @@ get_nat_addresses_and_keys(struct sset
> *nat_address_keys,
> > if (nat_addresses_options) {
> > consider_nat_address(nat_addresses_options, pb,
> > nat_address_keys, lports, chassis,
> > + chassis_index, active_tunnels,
> > nat_addresses);
> > }
> > }
> > @@ -1638,7 +1666,9 @@ static void
> > send_garp_run(const struct ovsrec_bridge *br_int,
> > const struct sbrec_chassis *chassis,
> > const struct lport_index *lports,
> > - struct hmap *local_datapaths)
> > + const struct chassis_index *chassis_index,
> > + struct hmap *local_datapaths,
> > + struct sset *active_tunnels)
> > {
> > struct sset localnet_vifs = SSET_INITIALIZER(&localnet_vifs);
> > struct sset local_l3gw_ports = SSET_INITIALIZER(&local_l3gw_ports);
> > @@ -1652,7 +1682,8 @@ send_garp_run(const struct ovsrec_bridge *br_int,
> > &localnet_vifs, &localnet_ofports,
> &local_l3gw_ports);
> >
> > get_nat_addresses_and_keys(&nat_ip_keys, &local_l3gw_ports, lports,
> > - chassis, &nat_addresses);
> > + chassis, chassis_index, active_tunnels,
> > + &nat_addresses);
> > /* For deleted ports and deleted nat ips, remove from
> send_garp_data. */
> > struct shash_node *iter, *next;
> > SHASH_FOR_EACH_SAFE (iter, next, &send_garp_data) {
> > diff --git a/ovn/controller/pinctrl.h b/ovn/controller/pinctrl.h
> > index 230580b..913c170 100644
> > --- a/ovn/controller/pinctrl.h
> > +++ b/ovn/controller/pinctrl.h
> > @@ -19,8 +19,10 @@
> >
> > #include <stdint.h>
> >
> > +#include "lib/sset.h"
> > #include "openvswitch/meta-flow.h"
> >
> > +struct chassis_index;
> > struct controller_ctx;
> > struct hmap;
> > struct lport_index;
> > @@ -30,7 +32,8 @@ struct sbrec_chassis;
> > void pinctrl_init(void);
> > void pinctrl_run(struct controller_ctx *, const struct lport_index *,
> > const struct ovsrec_bridge *, const struct
> sbrec_chassis *,
> > - struct hmap *local_datapaths);
> > + const struct chassis_index *, struct hmap
> *local_datapaths,
> > + struct sset *active_tunnels);
> > void pinctrl_wait(struct controller_ctx *);
> > void pinctrl_destroy(void);
> >
> > diff --git a/tests/ovn.at b/tests/ovn.at
> > index 398afee..4282328 100644
> > --- a/tests/ovn.at
> > +++ b/tests/ovn.at
> > @@ -8000,3 +8000,124 @@ OVN_CLEANUP([gw1],[gw2],[hv1],[hv2])
> >
> > AT_CLEANUP
> >
> > +AT_SETUP([ovn -- send gratuitous ARP for NAT rules on HA distributed
> router])
> > +AT_SKIP_IF([test $HAVE_PYTHON = no])
> > +ovn_start
> > +ovn-nbctl ls-add ls0
> > +ovn-nbctl ls-add ls1
> > +ovn-nbctl create Logical_Router name=lr0
> > +ovn-nbctl lrp-add lr0 lrp0 f0:00:00:00:00:01 192.168.0.100/24
> > +
> > +ovn-nbctl --id=@gc0 create Gateway_Chassis \
> > + name=outside_gw1 chassis_name=hv2 priority=10 -- \
> > + --id=@gc1 create Gateway_Chassis \
> > + name=outside_gw2 chassis_name=hv3 priority=1 -- \
> > + set Logical_Router_Port lrp0 'gateway_chassis=[@gc0,@gc1]'
> > +
> > +ovn-nbctl lsp-add ls0 lrp0-rp -- set Logical_Switch_Port lrp0-rp \
> > + type=router options:router-port=lrp0 addresses="router"
> > +ovn-nbctl lrp-add lr0 lrp1 f0:00:00:00:00:02 10.0.0.1/24
> > +ovn-nbctl lsp-add ls1 lrp1-rp -- set Logical_Switch_Port lrp1-rp \
> > + type=router options:router-port=lrp1 addresses="router"
> > +
> > +# Add NAT rules
> > +AT_CHECK([ovn-nbctl lr-nat-add lr0 snat 192.168.0.100 10.0.0.0/24])
> > +
> > +net_add n1
> > +sim_add hv1
> > +as hv1
> > +ovs-vsctl add-br br-phys
> > +ovn_attach n1 br-phys 192.168.0.1
> > +AT_CHECK([ovs-vsctl set Open_vSwitch . external-ids:ovn-bridge-
> mappings=physnet1:br-phys])
> > +AT_CHECK([ovs-vsctl add-port br-phys snoopvif -- set Interface snoopvif
> options:tx_pcap=hv1/snoopvif-tx.pcap options:rxq_pcap=hv1/snoopvif-
> rx.pcap])
> > +
> > +sim_add hv2
> > +as hv2
> > +ovs-vsctl add-br br-phys
> > +ovn_attach n1 br-phys 192.168.0.2
> > +AT_CHECK([as hv2 ovs-vsctl set Open_vSwitch . external-ids:ovn-bridge-
> mappings=physnet1:br-phys])
> > +
> > +sim_add hv3
> > +as hv3
> > +ovs-vsctl add-br br-phys
> > +ovn_attach n1 br-phys 192.168.0.3
> > +AT_CHECK([as hv3 ovs-vsctl set Open_vSwitch . external-ids:ovn-bridge-
> mappings=physnet1:br-phys])
> > +
> > +# Create a localnet port.
> > +AT_CHECK([ovn-nbctl lsp-add ls0 ln_port])
> > +AT_CHECK([ovn-nbctl lsp-set-addresses ln_port unknown])
> > +AT_CHECK([ovn-nbctl lsp-set-type ln_port localnet])
> > +AT_CHECK([ovn-nbctl lsp-set-options ln_port network_name=physnet1])
> > +
> > +# wait for earlier changes to take effect
> > +AT_CHECK([ovn-nbctl --timeout=3 --wait=sb sync], [0], [ignore])
> > +
> > +reset_pcap_file() {
> > + local iface=$1
> > + local pcap_file=$2
> > + ovs-vsctl -- set Interface $iface options:tx_pcap=dummy-tx.pcap \
> > +options:rxq_pcap=dummy-rx.pcap
> > + rm -f ${pcap_file}*.pcap
> > + ovs-vsctl -- set Interface $iface options:tx_pcap=${pcap_file}-tx.pcap
> \
> > +options:rxq_pcap=${pcap_file}-rx.pcap
> > +}
> > +
> > +as hv1 reset_pcap_file snoopvif hv1/snoopvif
> > +as hv2 reset_pcap_file br-phys_n1 hv2/br-phys_n1
> > +as hv3 reset_pcap_file br-phys_n1 hv3/br-phys_n1
> > +# add nat-addresses option
> > +ovn-nbctl --wait=sb lsp-set-options lrp0-rp router-port=lrp0
> nat-addresses="router"
> > +
> > +# Wait for packets to be received through hv2.
> > +OVS_WAIT_UNTIL([test `wc -c < "hv1/snoopvif-tx.pcap"` -ge 100])
> > +trim_zeros() {
> > + sed 's/\(00\)\{1,\}$//'
> > +}
> > +
> > +garp="fffffffffffff0000000000108060001080006040001f00000000001c0a8
> 0064000000000000c0a80064"
> > +echo $garp >> expout
> > +echo $garp >> expout
> > +$PYTHON "$top_srcdir/utilities/ovs-pcap.in" hv1/snoopvif-tx.pcap |
> trim_zeros > hv1_snoop_tx
> > +echo "packets on hv1-snoopvif:"
> > +cat hv1_snoop_tx
> > +AT_CHECK([sort hv1_snoop_tx], [0], [expout])
> > +$PYTHON "$top_srcdir/utilities/ovs-pcap.in" hv2/br-phys_n1-tx.pcap |
> trim_zeros > hv2_br_phys_tx
> > +echo "packets on hv2 br-phys tx"
> > +cat hv2_br_phys_tx
> > +AT_CHECK([grep $garp hv2_br_phys_tx | sort], [0], [expout])
> > +$PYTHON "$top_srcdir/utilities/ovs-pcap.in" hv3/br-phys_n1-tx.pcap |
> trim_zeros > hv3_br_phys_tx
> > +echo "packets on hv3 br-phys tx"
> > +cat hv3_br_phys_tx
> > +AT_CHECK([grep $garp hv3_br_phys_tx | sort], [0], [])
> > +
> > +
> > +# at this point, we invert the priority of the gw chassis between hv2
> and hv3
> > +
> > +ovn-nbctl --wait=hv \
> > + --id=@gc0 create Gateway_Chassis \
> > + name=outside_gw1 chassis_name=hv2 priority=1 -- \
> > + --id=@gc1 create Gateway_Chassis \
> > + name=outside_gw2 chassis_name=hv3 priority=10 -- \
> > + set Logical_Router_Port lrp0 'gateway_chassis=[@gc0,@gc1]'
> > +
> > +
> > +as hv1 reset_pcap_file snoopvif hv1/snoopvif
> > +as hv2 reset_pcap_file br-phys_n1 hv2/br-phys_n1
> > +as hv3 reset_pcap_file br-phys_n1 hv3/br-phys_n1
> > +
> > +# Wait for packets to be received.
> > +OVS_WAIT_UNTIL([test `wc -c < "hv1/snoopvif-tx.pcap"` -ge 100])
> > +trim_zeros() {
> > + sed 's/\(00\)\{1,\}$//'
> > +}
> > +
> > +$PYTHON "$top_srcdir/utilities/ovs-pcap.in" hv1/snoopvif-tx.pcap |
> trim_zeros > hv1_snoopvif_tx
> > +AT_CHECK([sort hv1_snoopvif_tx], [0], [expout])
> > +$PYTHON "$top_srcdir/utilities/ovs-pcap.in" hv3/br-phys_n1-tx.pcap |
> trim_zeros > hv3_br_phys_tx
> > +AT_CHECK([grep $garp hv3_br_phys_tx | sort], [0], [expout])
> > +$PYTHON "$top_srcdir/utilities/ovs-pcap.in" hv2/br-phys_n1-tx.pcap |
> trim_zeros > hv2_br_phys_tx
> > +AT_CHECK([grep $garp hv2_br_phys_tx | sort], [0], [])
> > +OVN_CLEANUP([hv1],[hv2],[hv3])
> > +
> > +AT_CLEANUP
> > +
> > --
> > 1.8.3.1
> >
> > _______________________________________________
> > dev mailing list
> > d...@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
>
> --
> Russell Bryant
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
