[ovs-dev] [PATCH v3 ovn] controller: make garp_max_timeout configurable
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
---
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.
+ external_ids:garp-max-timeout
+
+When used, this configuration value specifies the maximum timeout
+(in seconds) between two consecutive GARP packets sent by
+ovn-controller.
+ovn-controller by default sends just 4 GARP packets
+with an exponential backoff timeout.
+Setting external_ids:garp-max-timeout allows to
+cap for the exponential backoff used by ovn-controller
+to send GARPs packets.
+
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;
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_n
Re: [ovs-dev] [PATCH v3 ovn] controller: make garp_max_timeout configurable
On Thu, Aug 24, 2023 at 6:36 PM Lorenzo Bianconi <
lorenzo.bianc...@redhat.com> 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
> ---
>
Hi Lorenzo,
I have a couple more comments, sorry for not including those in the
previous review.
> 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.
>
> + external_ids:garp-max-timeout
>
nit: We should specify the unit in the name e.g. "garp-max-timeout-s".
> +
> +When used, this configuration value specifies the maximum timeout
> +(in seconds) between two consecutive GARP packets sent by
> +ovn-controller.
> +ovn-controller by default sends just 4 GARP packets
> +with an exponential backoff timeout.
> +Setting external_ids:garp-max-timeout allows to
> +cap for the exponential backoff used by
> ovn-controller
> +to send GARPs packets.
> +
>
>
>
> 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_
Re: [ovs-dev] [PATCH v3 ovn] controller: make garp_max_timeout configurable
> On Thu, Aug 24, 2023 at 6:36 PM Lorenzo Bianconi < > lorenzo.bianc...@redhat.com> 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 > > --- > > > 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. > > > > + external_ids:garp-max-timeout > > > > nit: We should specify the unit in the name e.g. "garp-max-timeout-s". > > > > + > > +When used, this configuration value specifies the maximum timeout > > +(in seconds) between two consecutive GARP packets sent by > > +ovn-controller. > > +ovn-controller by default sends just 4 GARP packets > > +with an exponential backoff timeout. > > +Setting external_ids:garp-max-timeout allows to > > +cap for the exponential backoff used by > > ovn-controller > > +to send GARPs packets. > > + > > > > > > > > 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, > >
