When two gateways fight in ha mode to bind the router port,
there were cases in which one of the gateway almost always
immediately reclaimed the router port, as the grace time was
honored by the other gw. This is also the case since [1] for
the highest priority gw.
In such a case, the garp_rarp module did not detect the change in
the router port binding (as the port bound to a different chassis
in idl was already rebound in runtime_data, potentially during a
recompute.
Hence it was not resetting the garp "count", potentially resulting
in no new garps.
Also, with a fight time of 500 msec there is not enough time for gw
to send garp, so increase the grace time to two seconds.
[1] controller: Do not postpone claim for highest priority chassis.
Fixes: 8ab8570e21ff ("pinctrl: Fix missing garp.")
Reported-at: https://issues.redhat.com/browse/FDP-1418
Signed-off-by: Xavier Simonart <[email protected]>
---
- v2: Updated based on Ales' feedback i.e.
- Deleting node while collecting dgp was strange.
- Deleting node instead of resetting time was overkill.
- This could be I-P.
---
controller/binding.c | 2 +-
controller/garp_rarp.c | 70 +++++++++++++++++++++++++++++--------
controller/garp_rarp.h | 4 +++
controller/if-status.c | 25 +++++++++++++
controller/if-status.h | 2 ++
controller/ovn-controller.c | 13 +++++++
6 files changed, 100 insertions(+), 16 deletions(-)
diff --git a/controller/binding.c b/controller/binding.c
index a2976ca03..74330b256 100644
--- a/controller/binding.c
+++ b/controller/binding.c
@@ -49,7 +49,7 @@ VLOG_DEFINE_THIS_MODULE(binding);
#define OVN_QOS_TYPE "linux-htb"
-#define CLAIM_TIME_THRESHOLD_MS 500
+#define CLAIM_TIME_THRESHOLD_MS 2000
struct claimed_port {
long long int last_claimed;
diff --git a/controller/garp_rarp.c b/controller/garp_rarp.c
index 551d8303f..8eccf10fe 100644
--- a/controller/garp_rarp.c
+++ b/controller/garp_rarp.c
@@ -24,6 +24,7 @@
#include "ovn/lex.h"
#include "garp_rarp.h"
#include "ovn-sb-idl.h"
+#include "if-status.h"
VLOG_DEFINE_THIS_MODULE(garp_rarp);
static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 20);
@@ -33,6 +34,11 @@ static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5,
20);
static bool garp_rarp_data_has_changed = false;
static struct garp_rarp_data garp_rarp_data;
+struct laddrs_port {
+ struct lport_addresses laddrs;
+ char *lport;
+};
+
/* Get localnet vifs, local l3gw ports and ofport for localnet patch ports. */
static void
get_localnet_vifs_l3gwports(
@@ -146,14 +152,15 @@ consider_nat_address(struct ovsdb_idl_index
*sbrec_port_binding_by_name,
struct sset *non_local_lports,
struct sset *local_lports)
{
- struct lport_addresses *laddrs = xmalloc(sizeof *laddrs);
+ struct laddrs_port *laddrs_port = xmalloc(sizeof *laddrs_port);
+ struct lport_addresses *laddrs = &laddrs_port->laddrs;
char *lport = NULL;
bool rc = extract_addresses_with_port(nat_address, laddrs, &lport);
if (!rc
|| (!lport && !strcmp(pb->type, "patch"))) {
destroy_lport_addresses(laddrs);
- free(laddrs);
free(lport);
+ free(laddrs_port);
return;
}
if (lport) {
@@ -161,14 +168,13 @@ consider_nat_address(struct ovsdb_idl_index
*sbrec_port_binding_by_name,
chassis, lport)) {
sset_add(non_local_lports, lport);
destroy_lport_addresses(laddrs);
- free(laddrs);
free(lport);
+ free(laddrs_port);
return;
} else {
sset_add(local_lports, lport);
}
}
- free(lport);
for (size_t i = 0; i < laddrs->n_ipv4_addrs; i++) {
char *name = xasprintf("%s-%s", pb->logical_port,
@@ -181,7 +187,8 @@ consider_nat_address(struct ovsdb_idl_index
*sbrec_port_binding_by_name,
sset_add(nat_address_keys, name);
free(name);
}
- shash_add(nat_addresses, pb->logical_port, laddrs);
+ laddrs_port->lport = lport;
+ shash_add(nat_addresses, pb->logical_port, laddrs_port);
}
static void
@@ -271,9 +278,34 @@ garp_rarp_lookup(const struct eth_addr ea, ovs_be32 ipv4,
uint32_t dp_key,
return NULL;
}
+void
+garp_rarp_node_reset_timers(const char *logical_port)
+{
+ struct garp_rarp_node *grn;
+ CMAP_FOR_EACH (grn, cmap_node, &garp_rarp_data.data) {
+ if (grn->logical_port && !strcmp(grn->logical_port, logical_port)) {
+ atomic_store(&grn->announce_time, time_msec() + 1000);
+ atomic_store(&grn->backoff, 1000);
+ }
+ }
+}
+
+static void
+reset_timers_for_claimed_cr(struct if_status_mgr *mgr)
+{
+ struct sset *claimed_cr = get_claimed_cr(mgr);
+ const char *cr_logical_port;
+ SSET_FOR_EACH_SAFE (cr_logical_port, claimed_cr) {
+ garp_rarp_node_reset_timers(cr_logical_port);
+ sset_delete(claimed_cr, SSET_NODE_FROM_NAME(cr_logical_port));
+ }
+
+}
+
static void
garp_rarp_node_add(const struct eth_addr ea, ovs_be32 ip,
- uint32_t dp_key, uint32_t port_key)
+ uint32_t dp_key, uint32_t port_key,
+ const char *logical_port)
{
struct garp_rarp_node *grn = garp_rarp_lookup(ea, ip, dp_key, port_key);
if (grn) {
@@ -288,6 +320,7 @@ garp_rarp_node_add(const struct eth_addr ea, ovs_be32 ip,
atomic_store(&grn->backoff, 1000); /* msec. */
grn->dp_key = dp_key;
grn->port_key = port_key;
+ grn->logical_port = nullable_xstrdup(logical_port);
grn->stale = false;
cmap_insert(&garp_rarp_data.data, &grn->cmap_node,
garp_rarp_node_hash_struct(grn));
@@ -353,13 +386,15 @@ send_garp_rarp_update(const struct garp_rarp_ctx_in
*r_ctx_in,
* distributed gateway ports. */
if (!strcmp(binding_rec->type, "l3gateway")
|| !strcmp(binding_rec->type, "patch")) {
- struct lport_addresses *laddrs = NULL;
- while ((laddrs = shash_find_and_delete(nat_addresses,
+ struct laddrs_port *laddrs_port = NULL;
+ while ((laddrs_port = shash_find_and_delete(nat_addresses,
binding_rec->logical_port))) {
+ struct lport_addresses *laddrs = &laddrs_port->laddrs;
for (size_t i = 0; i < laddrs->n_ipv4_addrs; i++) {
garp_rarp_node_add(laddrs->ea, laddrs->ipv4_addrs[i].addr,
binding_rec->datapath->tunnel_key,
- binding_rec->tunnel_key);
+ binding_rec->tunnel_key,
+ laddrs_port->lport);
send_garp_locally(r_ctx_in, binding_rec, laddrs->ea,
laddrs->ipv4_addrs[i].addr);
}
@@ -370,10 +405,12 @@ send_garp_rarp_update(const struct garp_rarp_ctx_in
*r_ctx_in,
if (laddrs->n_ipv4_addrs == 0) {
garp_rarp_node_add(laddrs->ea, 0,
binding_rec->datapath->tunnel_key,
- binding_rec->tunnel_key);
+ binding_rec->tunnel_key,
+ laddrs_port->lport);
}
destroy_lport_addresses(laddrs);
- free(laddrs);
+ free(laddrs_port->lport);
+ free(laddrs_port);
}
return;
}
@@ -392,7 +429,7 @@ send_garp_rarp_update(const struct garp_rarp_ctx_in
*r_ctx_in,
garp_rarp_node_add(laddrs.ea, ip,
binding_rec->datapath->tunnel_key,
- binding_rec->tunnel_key);
+ binding_rec->tunnel_key, NULL);
if (ip) {
send_garp_locally(r_ctx_in, binding_rec, laddrs.ea, ip);
}
@@ -424,6 +461,7 @@ garp_rarp_run(struct garp_rarp_ctx_in *r_ctx_in)
grn->stale = true;
}
+ reset_timers_for_claimed_cr(r_ctx_in->mgr);
get_localnet_vifs_l3gwports(r_ctx_in->sbrec_port_binding_by_datapath,
r_ctx_in->chassis,
r_ctx_in->local_datapaths,
@@ -460,10 +498,11 @@ garp_rarp_run(struct garp_rarp_ctx_in *r_ctx_in)
struct shash_node *iter;
SHASH_FOR_EACH_SAFE (iter, &nat_addresses) {
- struct lport_addresses *laddrs = iter->data;
- destroy_lport_addresses(laddrs);
+ struct laddrs_port *laddrs_port = iter->data;
+ destroy_lport_addresses(&laddrs_port->laddrs);
shash_delete(&nat_addresses, iter);
- free(laddrs);
+ free(laddrs_port->lport);
+ free(laddrs_port);
}
shash_destroy(&nat_addresses);
@@ -512,6 +551,7 @@ garp_rarp_data_changed(void) {
void
garp_rarp_node_free(struct garp_rarp_node *garp_rarp)
{
+ free(garp_rarp->logical_port);
free(garp_rarp);
}
diff --git a/controller/garp_rarp.h b/controller/garp_rarp.h
index 38f19e1c0..47dce69fb 100644
--- a/controller/garp_rarp.h
+++ b/controller/garp_rarp.h
@@ -21,6 +21,7 @@
#include "cmap.h"
#include "sset.h"
#include "openvswitch/types.h"
+#include "if-status.h"
/* Contains a single mac and ip address that should be announced. */
struct garp_rarp_node {
@@ -36,6 +37,7 @@ struct garp_rarp_node {
uint32_t port_key; /* Port to inject the GARP into. */
bool stale; /* Used during sync to remove stale
* information. */
+ char *logical_port; /* Name of the cr logical_port, if any */
};
/* Contains all required data for pinctrl to actually send garps. */
@@ -57,6 +59,7 @@ struct garp_rarp_ctx_in {
const struct hmap *local_datapaths;
const struct sset *active_tunnels;
struct ed_type_garp_rarp *data;
+ struct if_status_mgr *mgr;
};
struct ed_type_garp_rarp {
@@ -75,5 +78,6 @@ bool garp_rarp_data_changed(void);
struct ed_type_garp_rarp *garp_rarp_init(void);
void garp_rarp_cleanup(struct ed_type_garp_rarp *);
+void garp_rarp_node_reset_timers(const char *logical_port);
#endif /* GARP_RARP_H */
diff --git a/controller/if-status.c b/controller/if-status.c
index 9b0f2bcdf..5b176b86d 100644
--- a/controller/if-status.c
+++ b/controller/if-status.c
@@ -206,6 +206,10 @@ struct if_status_mgr {
/* All local interfaces, stored per state. */
struct hmapx ifaces_per_state[OIF_MAX];
+ /* All chassisredirect ports bound to different chassis in idl and bound in
+ * this loop. */
+ struct sset claimed_cr;
+
/* Registered ofctrl seqno type for port_binding flow installation. */
size_t iface_seq_type_pb_cfg;
@@ -245,6 +249,7 @@ if_status_mgr_create(void)
hmapx_init(&mgr->ifaces_per_state[i]);
}
shash_init(&mgr->ifaces);
+ sset_init(&mgr->claimed_cr);
shash_init(&mgr->ovn_uninstall_hash);
return mgr;
}
@@ -259,6 +264,8 @@ if_status_mgr_clear(struct if_status_mgr *mgr)
}
ovs_assert(shash_is_empty(&mgr->ifaces));
+ sset_clear(&mgr->claimed_cr);
+
SHASH_FOR_EACH_SAFE (node, &mgr->ovn_uninstall_hash) {
ovn_uninstall_hash_destroy(mgr, node);
}
@@ -274,6 +281,7 @@ if_status_mgr_destroy(struct if_status_mgr *mgr)
{
if_status_mgr_clear(mgr);
shash_destroy(&mgr->ifaces);
+ sset_destroy(&mgr->claimed_cr);
shash_destroy(&mgr->ovn_uninstall_hash);
for (size_t i = 0; i < ARRAY_SIZE(mgr->ifaces_per_state); i++) {
hmapx_destroy(&mgr->ifaces_per_state[i]);
@@ -313,6 +321,10 @@ if_status_mgr_claim_iface(struct if_status_mgr *mgr,
memcpy(&iface->parent_pb_uuid, &parent_pb->header_.uuid,
sizeof(iface->pb_uuid));
}
+ if (pb->chassis && pb->chassis != chassis_rec &&
+ !strcmp(pb->type, "chassisredirect")) {
+ sset_add(&mgr->claimed_cr, pb->logical_port);
+ }
if (!sb_readonly) {
if (bind_type == CAN_BIND_AS_MAIN) {
set_pb_chassis_in_sbrec(pb, chassis_rec, true);
@@ -346,6 +358,18 @@ if_status_mgr_iface_is_present(struct if_status_mgr *mgr,
const char *iface_id)
return !!shash_find_data(&mgr->ifaces, iface_id);
}
+bool
+if_status_reclaimed(struct if_status_mgr *mgr, const char *iface_id)
+{
+ return sset_find(&mgr->claimed_cr, iface_id);
+}
+
+struct sset *
+get_claimed_cr(struct if_status_mgr *mgr)
+{
+ return &mgr->claimed_cr;
+}
+
void
if_status_mgr_release_iface(struct if_status_mgr *mgr, const char *iface_id)
{
@@ -707,6 +731,7 @@ if_status_mgr_run(struct if_status_mgr *mgr,
if_status_mgr_update_bindings(mgr, binding_data, chassis_rec,
iface_table, pb_table,
sb_readonly, ovs_readonly);
+ sset_clear(&mgr->claimed_cr);
}
static void
diff --git a/controller/if-status.h b/controller/if-status.h
index d4f972355..d15ca3008 100644
--- a/controller/if-status.h
+++ b/controller/if-status.h
@@ -67,5 +67,7 @@ bool if_status_mgr_iface_update(const struct if_status_mgr
*mgr,
const struct ovsrec_interface *iface_rec);
bool if_status_is_port_claimed(const struct if_status_mgr *mgr,
const char *iface_id);
+bool if_status_reclaimed(struct if_status_mgr *mgr, const char *iface_id);
+struct sset * get_claimed_cr(struct if_status_mgr *mgr);
# endif /* controller/if-status.h */
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
index 477b3b089..5303c6551 100644
--- a/controller/ovn-controller.c
+++ b/controller/ovn-controller.c
@@ -5459,6 +5459,8 @@ static enum engine_node_state
en_garp_rarp_run(struct engine_node *node, void *data_)
{
struct ed_type_garp_rarp *data = data_;
+ struct controller_engine_ctx *ctrl_ctx =
+ engine_get_context()->client_ctx;
const struct ovsrec_open_vswitch_table *ovs_table =
EN_OVSDB_GET(engine_get_input("OVS_open_vswitch", node));
@@ -5508,6 +5510,7 @@ en_garp_rarp_run(struct engine_node *node, void *data_)
.active_tunnels = &rt_data->active_tunnels,
.local_datapaths = &rt_data->local_datapaths,
.data = data,
+ .mgr = ctrl_ctx->if_mgr,
};
garp_rarp_run(&r_ctx_in);
@@ -5558,6 +5561,7 @@ garp_rarp_sb_port_binding_handler(struct engine_node
*node,
engine_ovsdb_node_get_index(
engine_get_input("SB_port_binding", node),
"name");
+ struct controller_engine_ctx *ctrl_ctx = engine_get_context()->client_ctx;
const struct sbrec_port_binding *pb;
SBREC_PORT_BINDING_TABLE_FOR_EACH_TRACKED (pb, port_binding_table) {
@@ -5586,6 +5590,15 @@ garp_rarp_sb_port_binding_handler(struct engine_node
*node,
/* XXX: actually handle this incrementally. */
return EN_UNHANDLED;
}
+
+ /* If the cr_port was updated, bound to a different chassis in idl
+ * and (re)bound to our chassis in runtime data, make sure to reset
+ * garp timers*/
+ if (sbrec_port_binding_is_updated(pb,
+ SBREC_PORT_BINDING_COL_CHASSIS) &&
+ if_status_reclaimed(ctrl_ctx->if_mgr, pb->logical_port)) {
+ garp_rarp_node_reset_timers(pb->logical_port);
+ }
}
return EN_HANDLED_UNCHANGED;
--
2.47.1
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
