[ovs-dev] [PATCH v3 6/6] ovn: l3ha, gratuitous ARP for HA router

Miguel Angel Ajo Pelayo majopela at redhat.com
Sat Jul 8 18:51:59 UTC 2017


Thanks a lot for the quick and thorough review of the patch series.

On Sat, Jul 8, 2017 at 8:51 PM, Miguel Angel Ajo Pelayo <majopela at redhat.com
> wrote:

>
>
> On Fri, Jul 7, 2017 at 11:10 PM, Russell Bryant <russell at ovn.org> wrote:
>
>> On Wed, Jul 5, 2017 at 9:45 AM, Miguel Angel Ajo <majopela at redhat.com>
>> wrote:
>> > From: Venkata Anil Kommaddi <vkommadi at 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 <vkommadi at 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_p
>> orts);
>> > @@ -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, at 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-mappin
>> gs=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="fffffffffffff0000000000108060001080006040001f00000000
>> 001c0a80064000000000000c0a80064"
>> > +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, at 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
>> > dev at openvswitch.org
>> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
>>
>>
>> --
>> Russell Bryant
>>
>
>


More information about the dev mailing list