The function behavior would be unexpected in cases when there is
BFD flap between two gateway chassis. This would result in two
chassis claiming that the port binding should be claimed by them.
This leads to multiple problems like unsent gARPs. The gARP problem
on its own was solved [0], but unfortunately reintroduced [1] in
slightly different form.
The conclusion from the investigation is that the function in
the current form is easy to misuse. To prevent that use
only SB state to determine which chassis has actually claimed
the port binding. This is the desired behavior in cases where
this function was used previously.
The code that determines which chassis should claim port binding
based on BFD status remains unchanged, thus the behavior during
failover should be the same with minimizing the impact for BFD
flapping cases.
[0] 289ec19b01ad ("pinctrl: Fix missing garp.")
[1] 05527bd6ccdb ("controller: Extract garp_rarp to engine node.").
Signed-off-by: Ales Musil <[email protected]>
---
controller/garp_rarp.c | 8 +++-----
controller/lport.c | 19 +++++++------------
controller/lport.h | 3 ---
controller/ovn-controller.c | 11 ++---------
controller/physical.c | 3 +--
controller/pinctrl.c | 22 ++++++++--------------
controller/pinctrl.h | 1 -
controller/route.c | 5 +----
controller/route.h | 2 --
9 files changed, 22 insertions(+), 52 deletions(-)
diff --git a/controller/garp_rarp.c b/controller/garp_rarp.c
index ef377e26b..551d8303f 100644
--- a/controller/garp_rarp.c
+++ b/controller/garp_rarp.c
@@ -17,6 +17,7 @@
#include <config.h>
#include "controller/local_data.h"
+#include "lport.h"
#include "mac-binding-index.h"
#include "openvswitch/hmap.h"
#include "openvswitch/vlog.h"
@@ -147,11 +148,7 @@ consider_nat_address(struct ovsdb_idl_index
*sbrec_port_binding_by_name,
{
struct lport_addresses *laddrs = xmalloc(sizeof *laddrs);
char *lport = NULL;
- const struct sbrec_port_binding *cr_pb = NULL;
bool rc = extract_addresses_with_port(nat_address, laddrs, &lport);
- if (lport) {
- cr_pb = lport_lookup_by_name(sbrec_port_binding_by_name, lport);
- }
if (!rc
|| (!lport && !strcmp(pb->type, "patch"))) {
destroy_lport_addresses(laddrs);
@@ -160,7 +157,8 @@ consider_nat_address(struct ovsdb_idl_index
*sbrec_port_binding_by_name,
return;
}
if (lport) {
- if (!cr_pb || (cr_pb->chassis != chassis)) {
+ if (!lport_is_chassis_resident(sbrec_port_binding_by_name,
+ chassis, lport)) {
sset_add(non_local_lports, lport);
destroy_lport_addresses(laddrs);
free(laddrs);
diff --git a/controller/lport.c b/controller/lport.c
index 92de375b5..1ea57a5ca 100644
--- a/controller/lport.c
+++ b/controller/lport.c
@@ -65,35 +65,30 @@ lport_lookup_by_key(struct ovsdb_idl_index
*sbrec_datapath_binding_by_key,
bool
lport_pb_is_chassis_resident(const struct sbrec_chassis *chassis,
- const struct sset *active_tunnels,
const struct sbrec_port_binding *pb)
{
if (!pb || !pb->chassis) {
return false;
}
- if (strcmp(pb->type, "chassisredirect")) {
- return pb->chassis == chassis;
- } else {
- return ha_chassis_group_is_active(pb->ha_chassis_group,
- active_tunnels, chassis);
- }
+
+ /* Note we relay on SB to provide the information, this is needed in case
+ * of flapping BFD when it's not detected by one side. */
+ return pb->chassis == chassis;
}
bool
lport_is_chassis_resident(struct ovsdb_idl_index *sbrec_port_binding_by_name,
const struct sbrec_chassis *chassis,
- const struct sset *active_tunnels,
const char *port_name)
{
const struct sbrec_port_binding *pb
= lport_lookup_by_name(sbrec_port_binding_by_name, port_name);
- return lport_pb_is_chassis_resident(chassis, active_tunnels, pb);
+ return lport_pb_is_chassis_resident(chassis, pb);
}
bool
lport_is_local(struct ovsdb_idl_index *sbrec_port_binding_by_name,
const struct sbrec_chassis *chassis,
- const struct sset *active_tunnels,
const char *port_name)
{
const struct sbrec_port_binding *pb = lport_lookup_by_name(
@@ -103,14 +98,14 @@ lport_is_local(struct ovsdb_idl_index
*sbrec_port_binding_by_name,
return false;
}
- if (lport_pb_is_chassis_resident(chassis, active_tunnels, pb)) {
+ if (lport_pb_is_chassis_resident(chassis, pb)) {
return true;
}
const struct sbrec_port_binding *cr_pb =
lport_get_cr_port(sbrec_port_binding_by_name, pb, NULL);
- return lport_pb_is_chassis_resident(chassis, active_tunnels, cr_pb);
+ return lport_pb_is_chassis_resident(chassis, cr_pb);
}
const struct sbrec_port_binding *
diff --git a/controller/lport.h b/controller/lport.h
index 8b1809a27..6d48301d2 100644
--- a/controller/lport.h
+++ b/controller/lport.h
@@ -62,15 +62,12 @@ const struct sbrec_multicast_group
*mcgroup_lookup_by_dp_name(
bool
lport_is_chassis_resident(struct ovsdb_idl_index *sbrec_port_binding_by_name,
const struct sbrec_chassis *chassis,
- const struct sset *active_tunnels,
const char *port_name);
bool lport_pb_is_chassis_resident(const struct sbrec_chassis *chassis,
- const struct sset *active_tunnels,
const struct sbrec_port_binding *pb);
bool lport_is_local(struct ovsdb_idl_index *sbrec_port_binding_by_name,
const struct sbrec_chassis *chassis,
- const struct sset *active_tunnels,
const char *port_name);
const struct sbrec_port_binding *lport_get_peer(
const struct sbrec_port_binding *,
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
index 7940091da..e0dc754a7 100644
--- a/controller/ovn-controller.c
+++ b/controller/ovn-controller.c
@@ -5062,7 +5062,6 @@ en_route_run(struct engine_node *node, void *data)
.sbrec_port_binding_by_name = sbrec_port_binding_by_name,
.chassis = chassis,
.dynamic_routing_port_mapping = dynamic_routing_port_mapping,
- .active_tunnels = &rt_data->active_tunnels,
.local_datapaths = &rt_data->local_datapaths,
.local_bindings = &rt_data->lbinding_data.bindings,
};
@@ -5164,7 +5163,6 @@ route_runtime_data_handler(struct engine_node *node, void
*data)
struct tracked_lport *lport = shash_node->data;
if (route_exchange_find_port(sbrec_port_binding_by_name, chassis,
- &rt_data->active_tunnels,
lport->pb)) {
/* XXX: Until we get I-P support for route exchange we need to
* request recompute. */
@@ -5222,8 +5220,6 @@ route_sb_port_binding_data_handler(struct engine_node
*node, void *data)
engine_ovsdb_node_get_index(
engine_get_input("SB_port_binding", node),
"name");
- struct ed_type_runtime_data *rt_data =
- engine_get_input_data("runtime_data", node);
/* There are the following cases where we need to handle updates to the
@@ -5246,8 +5242,8 @@ route_sb_port_binding_data_handler(struct engine_node
*node, void *data)
return EN_UNHANDLED;
}
- if (route_exchange_find_port(sbrec_port_binding_by_name, chassis,
- &rt_data->active_tunnels, sbrec_pb)) {
+ if (route_exchange_find_port(sbrec_port_binding_by_name,
+ chassis, sbrec_pb)) {
/* XXX: Until we get I-P support for route exchange we need to
* request recompute. */
return EN_UNHANDLED;
@@ -5556,7 +5552,6 @@ garp_rarp_sb_port_binding_handler(struct engine_node
*node,
if (sset_contains(&data->non_local_lports, pb->logical_port) &&
lport_is_chassis_resident(sbrec_port_binding_by_name, chassis,
- &rt_data->active_tunnels,
pb->logical_port)) {
/* XXX: actually handle this incrementally. */
return EN_UNHANDLED;
@@ -5564,7 +5559,6 @@ garp_rarp_sb_port_binding_handler(struct engine_node
*node,
if (sset_contains(&data->local_lports, pb->logical_port) &&
!lport_is_chassis_resident(sbrec_port_binding_by_name, chassis,
- &rt_data->active_tunnels,
pb->logical_port)) {
/* XXX: actually handle this incrementally. */
return EN_UNHANDLED;
@@ -6650,7 +6644,6 @@ main(int argc, char *argv[])
ovnsb_idl_loop.idl),
chassis,
&runtime_data->local_datapaths,
- &runtime_data->active_tunnels,
&runtime_data->local_active_ports_ipv6_pd,
&runtime_data->local_active_ports_ras,
ovsrec_open_vswitch_table_get(
diff --git a/controller/physical.c b/controller/physical.c
index 65b4c7335..1cc4173b2 100644
--- a/controller/physical.c
+++ b/controller/physical.c
@@ -872,8 +872,7 @@ put_replace_router_port_mac_flows(const struct physical_ctx
*ctx,
struct ofpact_mac *replace_mac;
char *cr_peer_name = xasprintf("cr-%s", rport_binding->logical_port);
if (lport_is_chassis_resident(ctx->sbrec_port_binding_by_name,
- ctx->chassis, ctx->active_tunnels,
- cr_peer_name)) {
+ ctx->chassis, cr_peer_name)) {
/* If a router port's chassisredirect port is
* resident on this chassis, then we need not do mac replace. */
free(cr_peer_name);
diff --git a/controller/pinctrl.c b/controller/pinctrl.c
index 545f1b174..d4f4da731 100644
--- a/controller/pinctrl.c
+++ b/controller/pinctrl.c
@@ -366,8 +366,7 @@ pinctrl_handle_bfd_msg(struct rconn *swconn, const struct
flow *ip_flow,
static void bfd_monitor_run(struct ovsdb_idl_txn *ovnsb_idl_txn,
const struct sbrec_bfd_table *bfd_table,
struct ovsdb_idl_index *sbrec_port_binding_by_name,
- const struct sbrec_chassis *chassis,
- const struct sset *active_tunnels)
+ const struct sbrec_chassis *chassis)
OVS_REQUIRES(pinctrl_mutex);
static void init_fdb_entries(void);
static void destroy_fdb_entries(void);
@@ -1434,7 +1433,6 @@ prepare_ipv6_prefixd(struct ovsdb_idl_txn *ovnsb_idl_txn,
struct ovsdb_idl_index *sbrec_port_binding_by_name,
const struct shash *local_active_ports_ipv6_pd,
const struct sbrec_chassis *chassis,
- const struct sset *active_tunnels,
const struct hmap *local_datapaths)
OVS_REQUIRES(pinctrl_mutex)
{
@@ -1463,9 +1461,8 @@ prepare_ipv6_prefixd(struct ovsdb_idl_txn *ovnsb_idl_txn,
}
char *redirect_name = xasprintf("cr-%s", pb->logical_port);
- bool resident = lport_is_chassis_resident(
- sbrec_port_binding_by_name, chassis, active_tunnels,
- redirect_name);
+ bool resident = lport_is_chassis_resident(sbrec_port_binding_by_name,
+ chassis, redirect_name);
free(redirect_name);
if ((strcmp(pb->type, "l3gateway") || pb->chassis != chassis) &&
!resident) {
@@ -4070,7 +4067,6 @@ pinctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn,
const struct sbrec_ecmp_nexthop_table *ecmp_nh_table,
const struct sbrec_chassis *chassis,
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 ovsrec_open_vswitch_table *ovs_table,
@@ -4086,7 +4082,7 @@ pinctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn,
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,
- active_tunnels, local_datapaths);
+ local_datapaths);
controller_event_run(ovnsb_idl_txn, ce_table, chassis);
ip_mcast_sync(ovnsb_idl_txn, chassis, local_datapaths,
sbrec_datapath_binding_by_key,
@@ -4101,7 +4097,7 @@ pinctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn,
sync_svc_monitors(ovnsb_idl_txn, svc_mon_table, sbrec_port_binding_by_name,
chassis);
bfd_monitor_run(ovnsb_idl_txn, bfd_table, sbrec_port_binding_by_name,
- chassis, active_tunnels);
+ chassis);
run_put_fdbs(ovnsb_idl_txn, sbrec_port_binding_by_key,
sbrec_datapath_binding_by_key, sbrec_fdb_by_dp_key_mac);
run_activated_ports(ovnsb_idl_txn, sbrec_datapath_binding_by_key,
@@ -7749,8 +7745,7 @@ static void
bfd_monitor_run(struct ovsdb_idl_txn *ovnsb_idl_txn,
const struct sbrec_bfd_table *bfd_table,
struct ovsdb_idl_index *sbrec_port_binding_by_name,
- const struct sbrec_chassis *chassis,
- const struct sset *active_tunnels)
+ const struct sbrec_chassis *chassis)
OVS_REQUIRES(pinctrl_mutex)
{
struct bfd_entry *entry;
@@ -7782,9 +7777,8 @@ bfd_monitor_run(struct ovsdb_idl_txn *ovnsb_idl_txn,
}
char *redirect_name = xasprintf("cr-%s", pb->logical_port);
- bool resident = lport_is_chassis_resident(
- sbrec_port_binding_by_name, chassis, active_tunnels,
- redirect_name);
+ bool resident = lport_is_chassis_resident(sbrec_port_binding_by_name,
+ chassis, redirect_name);
free(redirect_name);
if ((strcmp(pb->type, "l3gateway") || pb->chassis != chassis) &&
!resident) {
diff --git a/controller/pinctrl.h b/controller/pinctrl.h
index f977d1168..80384ac9b 100644
--- a/controller/pinctrl.h
+++ b/controller/pinctrl.h
@@ -57,7 +57,6 @@ void pinctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn,
const struct sbrec_ecmp_nexthop_table *,
const struct sbrec_chassis *chassis,
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 ovsrec_open_vswitch_table *ovs_table,
diff --git a/controller/route.c b/controller/route.c
index 2ea53a287..7615f3f59 100644
--- a/controller/route.c
+++ b/controller/route.c
@@ -52,7 +52,6 @@ advertise_route_hash(const struct in6_addr *dst, unsigned int
plen)
const struct sbrec_port_binding*
route_exchange_find_port(struct ovsdb_idl_index *sbrec_port_binding_by_name,
const struct sbrec_chassis *chassis,
- const struct sset *active_tunnels,
const struct sbrec_port_binding *pb)
{
if (!pb) {
@@ -69,7 +68,7 @@ route_exchange_find_port(struct ovsdb_idl_index
*sbrec_port_binding_by_name,
return NULL;
}
- if (!lport_pb_is_chassis_resident(chassis, active_tunnels, cr_pb)) {
+ if (!lport_pb_is_chassis_resident(chassis, cr_pb)) {
return NULL;
}
@@ -171,7 +170,6 @@ route_run(struct route_ctx_in *r_ctx_in,
const struct sbrec_port_binding *repb =
route_exchange_find_port(r_ctx_in->sbrec_port_binding_by_name,
r_ctx_in->chassis,
- r_ctx_in->active_tunnels,
local_peer);
if (!repb) {
continue;
@@ -257,7 +255,6 @@ route_run(struct route_ctx_in *r_ctx_in,
if (route->tracked_port) {
if (lport_is_local(r_ctx_in->sbrec_port_binding_by_name,
r_ctx_in->chassis,
- r_ctx_in->active_tunnels,
route->tracked_port->logical_port)) {
priority = PRIORITY_LOCAL_BOUND;
sset_add(r_ctx_out->tracked_ports_local,
diff --git a/controller/route.h b/controller/route.h
index 11016d818..09aff89ff 100644
--- a/controller/route.h
+++ b/controller/route.h
@@ -35,7 +35,6 @@ struct route_ctx_in {
struct ovsdb_idl_index *sbrec_port_binding_by_name;
const struct sbrec_chassis *chassis;
const char *dynamic_routing_port_mapping;
- const struct sset *active_tunnels;
const struct hmap *local_datapaths;
struct shash *local_bindings;
};
@@ -81,7 +80,6 @@ struct advertise_route_entry {
const struct sbrec_port_binding *route_exchange_find_port(
struct ovsdb_idl_index *sbrec_port_binding_by_name,
const struct sbrec_chassis *chassis,
- const struct sset *active_tunnels,
const struct sbrec_port_binding *pb);
uint32_t advertise_route_hash(const struct in6_addr *dst, unsigned int plen);
void route_run(struct route_ctx_in *, struct route_ctx_out *);
--
2.49.0
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
