> On Thu, Aug 24, 2023 at 6:36 PM Lorenzo Bianconi <
> [email protected]> wrote:
>
> > When using VLAN backed networks and OVN routers leveraging the
> > 'ovn-chassis-mac-mappings' option for east-west traffic, the eth.src field
> > is
> > replaced by the chassis mac address in order to not expose the router mac
> > address from different nodes and confuse the TOR switch. However doing so
> > the TOR switch is not able to learn the port/mac bindings for routed E/W
> > traffic and it is force to always flood it. Fix this issue adding the
> > capability to configure a given timeout for garp sent by ovn-controller
> > and not disable it after the exponential backoff in order to keep
> > refreshing the entries in TOR swtich fdb table.
> > More into about the issue can be found here [0].
> >
> > [0]
> > https://mail.openvswitch.org/pipermail/ovs-discuss/2020-September/050678.html
> > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2087779
> >
> > Signed-off-by: Lorenzo Bianconi <[email protected]>
> > ---
> >
> Hi Lorenzo,
>
> I have a couple more comments, sorry for not including those in the
> previous review.
Hi Ales,
no, worries and thx for the review.
I will fix the comments below in v4.
Regards,
Lorenzo
>
>
> > Changes since v2:
> > - cap exponential backoff timeout to garp_max_timeout
> >
> > Changes since v1:
> > - add uni-test
> > - add documentation
> > ---
> > controller/ovn-controller.8.xml | 11 ++++++
> > controller/ovn-controller.c | 4 +-
> > controller/pinctrl.c | 70 +++++++++++++++++++++++++++------
> > controller/pinctrl.h | 4 +-
> > tests/ovn.at | 16 ++++++++
> > 5 files changed, 91 insertions(+), 14 deletions(-)
> >
> > diff --git a/controller/ovn-controller.8.xml
> > b/controller/ovn-controller.8.xml
> > index 0883d8da9..831df4481 100644
> > --- a/controller/ovn-controller.8.xml
> > +++ b/controller/ovn-controller.8.xml
> > @@ -365,6 +365,17 @@
> > heplful to pin source outer IP for the tunnel when multiple
> > interfaces
> > are used on the host for overlay traffic.
> > </dd>
> > + <dt><code>external_ids:garp-max-timeout</code></dt>
> >
>
> nit: We should specify the unit in the name e.g. "garp-max-timeout-s".
>
>
> > + <dd>
> > + When used, this configuration value specifies the maximum timeout
> > + (in seconds) between two consecutive GARP packets sent by
> > + <code>ovn-controller</code>.
> > + <code>ovn-controller</code> by default sends just 4 GARP packets
> > + with an exponential backoff timeout.
> > + Setting <code>external_ids:garp-max-timeout</code> allows to
> > + cap for the exponential backoff used by
> > <code>ovn-controller</code>
> > + to send GARPs packets.
> > + </dd>
> > </dl>
> >
> > <p>
> > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> > index 0c975dc5e..1ae2f6cbe 100644
> > --- a/controller/ovn-controller.c
> > +++ b/controller/ovn-controller.c
> > @@ -5447,7 +5447,9 @@ main(int argc, char *argv[])
> > &runtime_data->local_datapaths,
> > &runtime_data->active_tunnels,
> >
> > &runtime_data->local_active_ports_ipv6_pd,
> > -
> > &runtime_data->local_active_ports_ras);
> > + &runtime_data->local_active_ports_ras,
> > + ovsrec_open_vswitch_table_get(
> > + ovs_idl_loop.idl));
> > stopwatch_stop(PINCTRL_RUN_STOPWATCH_NAME,
> > time_msec());
> > mirror_run(ovs_idl_txn,
> > diff --git a/controller/pinctrl.c b/controller/pinctrl.c
> > index bed90fe0b..850c53c58 100644
> > --- a/controller/pinctrl.c
> > +++ b/controller/pinctrl.c
> > @@ -165,6 +165,7 @@ VLOG_DEFINE_THIS_MODULE(pinctrl);
> > static struct ovs_mutex pinctrl_mutex = OVS_MUTEX_INITIALIZER;
> > static struct seq *pinctrl_handler_seq;
> > static struct seq *pinctrl_main_seq;
> > +static long long int garp_rarp_max_timeout = LLONG_MAX;
> >
>
> By setting the default value to 16000 (maybe even making that #define so it
> is really clear) and introducing something like "static bool
> garp_rarp_continuous = false;" we can simplify the code below
> significantly.
>
>
> >
> > static void *pinctrl_handler(void *arg);
> >
> > @@ -227,7 +228,8 @@ static void send_garp_rarp_prepare(
> > const struct ovsrec_bridge *,
> > const struct sbrec_chassis *,
> > const struct hmap *local_datapaths,
> > - const struct sset *active_tunnels)
> > + const struct sset *active_tunnels,
> > + const struct ovsrec_open_vswitch_table *ovs_table)
> > OVS_REQUIRES(pinctrl_mutex);
> > static void send_garp_rarp_run(struct rconn *swconn,
> > long long int *send_garp_rarp_time)
> > @@ -3492,7 +3494,8 @@ pinctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn,
> > const struct hmap *local_datapaths,
> > const struct sset *active_tunnels,
> > const struct shash *local_active_ports_ipv6_pd,
> > - const struct shash *local_active_ports_ras)
> > + const struct shash *local_active_ports_ras,
> > + const struct ovsrec_open_vswitch_table *ovs_table)
> > {
> > ovs_mutex_lock(&pinctrl_mutex);
> > run_put_mac_bindings(ovnsb_idl_txn, sbrec_datapath_binding_by_key,
> > @@ -3503,7 +3506,7 @@ pinctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn,
> > send_garp_rarp_prepare(ovnsb_idl_txn, sbrec_port_binding_by_datapath,
> > sbrec_port_binding_by_name,
> > sbrec_mac_binding_by_lport_ip, br_int, chassis,
> > - local_datapaths, active_tunnels);
> > + local_datapaths, active_tunnels, ovs_table);
> > prepare_ipv6_ras(local_active_ports_ras, sbrec_port_binding_by_name);
> > prepare_ipv6_prefixd(ovnsb_idl_txn, sbrec_port_binding_by_name,
> > local_active_ports_ipv6_pd, chassis,
> > @@ -4379,7 +4382,8 @@ struct garp_rarp_data {
> > struct eth_addr ea; /* Ethernet address of port. */
> > ovs_be32 ipv4; /* Ipv4 address of port. */
> > long long int announce_time; /* Next announcement in ms. */
> > - int backoff; /* Backoff for the next announcement. */
> > + int backoff; /* Backoff timeout for the next
> > + * announcement (in msecs). */
> > uint32_t dp_key; /* Datapath used to output this GARP. */
> > uint32_t port_key; /* Port to inject the GARP into. */
> > };
> > @@ -4408,7 +4412,7 @@ add_garp_rarp(const char *name, const struct
> > eth_addr ea, ovs_be32 ip,
> > garp_rarp->ea = ea;
> > garp_rarp->ipv4 = ip;
> > garp_rarp->announce_time = time_msec() + 1000;
> > - garp_rarp->backoff = 1;
> > + garp_rarp->backoff = 1000; /* msec. */
> > garp_rarp->dp_key = dp_key;
> > garp_rarp->port_key = port_key;
> > shash_add(&send_garp_rarp_data, name, garp_rarp);
> > @@ -4424,7 +4428,8 @@ send_garp_rarp_update(struct ovsdb_idl_txn
> > *ovnsb_idl_txn,
> > struct ovsdb_idl_index
> > *sbrec_mac_binding_by_lport_ip,
> > const struct hmap *local_datapaths,
> > const struct sbrec_port_binding *binding_rec,
> > - struct shash *nat_addresses)
> > + struct shash *nat_addresses,
> > + long long int garp_max_timeout)
> > {
> > volatile struct garp_rarp_data *garp_rarp = NULL;
> >
> > @@ -4450,6 +4455,11 @@ send_garp_rarp_update(struct ovsdb_idl_txn
> > *ovnsb_idl_txn,
> > if (garp_rarp) {
> > garp_rarp->dp_key = binding_rec->datapath->tunnel_key;
> > garp_rarp->port_key = binding_rec->tunnel_key;
> > + if (garp_max_timeout != garp_rarp_max_timeout) {
> > + /* reset backoff */
> > + garp_rarp->announce_time = time_msec() + 1000;
> > + garp_rarp->backoff = 1000; /* msec. */
> > + }
> > } else {
> > add_garp_rarp(name, laddrs->ea,
> > laddrs->ipv4_addrs[i].addr,
> > @@ -4474,6 +4484,11 @@ send_garp_rarp_update(struct ovsdb_idl_txn
> > *ovnsb_idl_txn,
> > if (garp_rarp) {
> > garp_rarp->dp_key =
> > binding_rec->datapath->tunnel_key;
> > garp_rarp->port_key = binding_rec->tunnel_key;
> > + if (garp_max_timeout != garp_rarp_max_timeout) {
> > + /* reset backoff */
> > + garp_rarp->announce_time = time_msec() + 1000;
> > + garp_rarp->backoff = 1000; /* msec. */
> > + }
> > } else {
> > add_garp_rarp(name, laddrs->ea,
> > 0,
> > binding_rec->datapath->tunnel_key,
> > @@ -4493,6 +4508,11 @@ send_garp_rarp_update(struct ovsdb_idl_txn
> > *ovnsb_idl_txn,
> > if (garp_rarp) {
> > garp_rarp->dp_key = binding_rec->datapath->tunnel_key;
> > garp_rarp->port_key = binding_rec->tunnel_key;
> > + if (garp_max_timeout != garp_rarp_max_timeout) {
> > + /* reset backoff */
> > + garp_rarp->announce_time = time_msec() + 1000;
> > + garp_rarp->backoff = 1000; /* msec. */
> > + }
> >
>
> For the resets we would just need to check the continuous bool, we should
> also reset it into the configured value.
>
> return;
> > }
> >
> > @@ -4578,13 +4598,22 @@ send_garp_rarp(struct rconn *swconn, struct
> > garp_rarp_data *garp_rarp,
> > ofpbuf_uninit(&ofpacts);
> >
> > /* Set the next announcement. At most 5 announcements are sent for a
> > - * vif. */
> > - if (garp_rarp->backoff < 16) {
> > + * vif if garp_rarp_max_timeout is not specified otherwise cap the max
> > + * timeout to garp_rarp_max_timeout. */
> > + if (garp_rarp_max_timeout != LLONG_MAX) {
> > + if (garp_rarp->backoff < garp_rarp_max_timeout) {
> > + garp_rarp->backoff *= 2;
> > + } else {
> > + garp_rarp->backoff = garp_rarp_max_timeout;
> > + }
> > + garp_rarp->announce_time = current_time + garp_rarp->backoff;
> > + } else if (garp_rarp->backoff < 16000) {
> > garp_rarp->backoff *= 2;
> > - garp_rarp->announce_time = current_time + garp_rarp->backoff *
> > 1000;
> > + garp_rarp->announce_time = current_time + garp_rarp->backoff;
> > } else {
> > garp_rarp->announce_time = LLONG_MAX;
> > }
> >
>
> With the continuous flag all of this can be simplified into:
>
> if (garp_rarp_continuous || garp_rarp->backoff < garp_rarp_max_timeout)
> {
> garp_rarp->announce_time = current_time + garp_rarp->backoff;
> } else {
> garp_rarp->announce_time = LLONG_MAX;
> }
> garp_rarp->backoff = MIN(garp_rarp_max_timeout, garp_rarp->backoff * 2);
>
>
> > +
> > return garp_rarp->announce_time;
> > }
> >
> > @@ -5881,13 +5910,26 @@ send_garp_rarp_prepare(struct ovsdb_idl_txn
> > *ovnsb_idl_txn,
> > const struct ovsrec_bridge *br_int,
> > const struct sbrec_chassis *chassis,
> > const struct hmap *local_datapaths,
> > - const struct sset *active_tunnels)
> > + const struct sset *active_tunnels,
> > + const struct ovsrec_open_vswitch_table *ovs_table)
> > OVS_REQUIRES(pinctrl_mutex)
> > {
> > 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 shash nat_addresses;
> > + unsigned long long garp_max_timeout = LLONG_MAX;
> > + const struct ovsrec_open_vswitch *cfg =
> > + ovsrec_open_vswitch_table_first(ovs_table);
> > + if (cfg) {
> > + garp_max_timeout = smap_get_ullong(
> > + &cfg->external_ids, "garp-max-timeout", LLONG_MAX);
> > + if (!garp_max_timeout) {
> > + garp_max_timeout = LLONG_MAX;
> > + } else if (garp_max_timeout != LLONG_MAX) {
> > + garp_max_timeout *= 1000; /* convert to msec. */
> > + }
> > + }
> >
>
> This could be simplified to:
>
> garp_max_timeout = smap_get_ullong(&cfg->external_ids,
> "garp-max-timeout", 0) * 1000;
> garp_rarp_continuous = !!garp_max_timeout;
> if (!garp_max_timeout) {
> garp_max_timeout = 16000;
> }
>
>
>
> >
> > shash_init(&nat_addresses);
> >
> > @@ -5918,7 +5960,8 @@ send_garp_rarp_prepare(struct ovsdb_idl_txn
> > *ovnsb_idl_txn,
> > if (pb) {
> > send_garp_rarp_update(ovnsb_idl_txn,
> > sbrec_mac_binding_by_lport_ip,
> > - local_datapaths, pb, &nat_addresses);
> > + local_datapaths, pb, &nat_addresses,
> > + garp_max_timeout);
> > }
> > }
> >
> > @@ -5929,7 +5972,8 @@ send_garp_rarp_prepare(struct ovsdb_idl_txn
> > *ovnsb_idl_txn,
> > = lport_lookup_by_name(sbrec_port_binding_by_name, gw_port);
> > if (pb) {
> > send_garp_rarp_update(ovnsb_idl_txn,
> > sbrec_mac_binding_by_lport_ip,
> > - local_datapaths, pb, &nat_addresses);
> > + local_datapaths, pb, &nat_addresses,
> > + garp_max_timeout);
> > }
> > }
> >
> > @@ -5947,6 +5991,8 @@ send_garp_rarp_prepare(struct ovsdb_idl_txn
> > *ovnsb_idl_txn,
> > shash_destroy(&nat_addresses);
> >
> > sset_destroy(&nat_ip_keys);
> > +
> > + garp_rarp_max_timeout = garp_max_timeout;
> > }
> >
> > static bool
> > diff --git a/controller/pinctrl.h b/controller/pinctrl.h
> > index 279a49fbc..23343f097 100644
> > --- a/controller/pinctrl.h
> > +++ b/controller/pinctrl.h
> > @@ -30,6 +30,7 @@ struct ovsdb_idl;
> > struct ovsdb_idl_index;
> > struct ovsdb_idl_txn;
> > struct ovsrec_bridge;
> > +struct ovsrec_open_vswitch_table;
> > struct sbrec_chassis;
> > struct sbrec_dns_table;
> > struct sbrec_controller_event_table;
> > @@ -57,7 +58,8 @@ void pinctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn,
> > const struct hmap *local_datapaths,
> > const struct sset *active_tunnels,
> > const struct shash *local_active_ports_ipv6_pd,
> > - const struct shash *local_active_ports_ras);
> > + const struct shash *local_active_ports_ras,
> > + const struct ovsrec_open_vswitch_table *ovs_table);
> > void pinctrl_wait(struct ovsdb_idl_txn *ovnsb_idl_txn);
> > void pinctrl_destroy(void);
> > void pinctrl_set_br_int_name(const char *br_int_name);
> > diff --git a/tests/ovn.at b/tests/ovn.at
> > index 2cf5d5169..1c8f8a340 100644
> > --- a/tests/ovn.at
> > +++ b/tests/ovn.at
> > @@ -9094,6 +9094,7 @@ AT_CLEANUP
> >
> > OVN_FOR_EACH_NORTHD([
> > AT_SETUP([send gratuitous arp for l3gateway only on selected chassis])
> > +AT_SKIP_IF([test $HAVE_TCPDUMP = no])
> > ovn_start
> >
> > # Create logical switch
> > @@ -9183,6 +9184,21 @@ sleep 2
> > OVN_CHECK_PACKETS_CONTAIN([hv2/snoopvif-tx.pcap], [arp_expected])
> > OVN_CHECK_PACKETS([hv1/snoopvif-tx.pcap], [empty_expected])
> >
> > +# Temporarily remove lr0 chassis
> > +AT_CHECK([ovn-nbctl --wait=hv remove logical_router lr0 options chassis])
> > +
> > +as hv1 reset_pcap_file snoopvif hv1/snoopvif
> > +as hv2 reset_pcap_file snoopvif hv2/snoopvif
> > +
> > +AT_CHECK([ovn-nbctl --wait=hv set logical_router lr0 options:chassis=hv1])
> > +# set garp max timeout to 32s
> >
>
> nit: s/32s/2s/
>
>
> > +AT_CHECK([as hv1 ovs-vsctl set Open_vSwitch .
> > external-ids:garp-max-timeout=2])
> > +
> > +OVS_WAIT_UNTIL([
> > +n_arp=$(tcpdump -c 10 -ner hv1/snoopvif-tx.pcap arp | wc -l)
> > +test "$n_arp" = 10
> > +])
> > +
> > OVN_CLEANUP([hv1],[hv2])
> >
> > AT_CLEANUP
> > --
> > 2.41.0
> >
> >
> Thanks,
> Ales
>
> --
>
> Ales Musil
>
> Senior Software Engineer - OVN Core
>
> Red Hat EMEA <https://www.redhat.com>
>
> [email protected] IM: amusil
> <https://red.ht/sig>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
