The kernel tells us that it could not tell us what changed by reporting a
change with no content, which happens when the receive buffer of the
netlink socket overflows or when a message cannot be parsed. Since the
notifiers share one socket, this is reported for every group.
All three change handlers dropped that report, so the tables the notifiers
track kept whatever state they had built from the messages that did make
it through. Route and neighbor changes in a watched table were silently
never acted upon, and the tracked kernel nexthop table kept entries that
may no longer exist. What saved us most of the time is that any later
recompute reads the tables again, but nothing guarantees one happens.
Record that notifications were missed and let each user of a notifier
resync: the route and neighbor notify nodes report an update whatever
their watches say, and nexthop_exchange dumps the nexthop table again
instead of applying the messages it has.
Fixes: 673d90f1173f ("controller: Watch for route changes.")
Assisted-by: Claude Opus 5, Cursor
Signed-off-by: Han Zhou <[email protected]>
---
controller/ovn-controller.c | 47 +++++++++++++++++++++++--------
controller/ovn-netlink-notifier.c | 42 +++++++++++++++++++++++++--
controller/ovn-netlink-notifier.h | 1 +
3 files changed, 75 insertions(+), 15 deletions(-)
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
index e221f60af118..931fb95dccb3 100644
--- a/controller/ovn-controller.c
+++ b/controller/ovn-controller.c
@@ -5893,11 +5893,20 @@ en_route_table_notify_run(struct engine_node *node
OVS_UNUSED, void *data)
struct vector *msgs;
uint32_t *table_id;
- msgs = ovn_netlink_get_msgs(OVN_NL_NOTIFIER_ROUTE_V4);
- VECTOR_FOR_EACH_PTR (msgs, table_id) {
- if (vector_bsearch(&rtn->watches, table_id, table_id_cmp)) {
- state = EN_UPDATED;
- break;
+ /* We cannot tell whether a table we watch was among the changes we
+ * missed, so assume it was. */
+ if (ovn_netlink_notifier_lost(OVN_NL_NOTIFIER_ROUTE_V4) ||
+ ovn_netlink_notifier_lost(OVN_NL_NOTIFIER_ROUTE_V6)) {
+ state = EN_UPDATED;
+ }
+
+ if (state != EN_UPDATED) {
+ msgs = ovn_netlink_get_msgs(OVN_NL_NOTIFIER_ROUTE_V4);
+ VECTOR_FOR_EACH_PTR (msgs, table_id) {
+ if (vector_bsearch(&rtn->watches, table_id, table_id_cmp)) {
+ state = EN_UPDATED;
+ break;
+ }
}
}
@@ -6537,13 +6546,21 @@ en_neighbor_table_notify_run(struct engine_node *node
OVS_UNUSED,
struct vector *msgs;
struct ne_table_msg *ne_msg;
- msgs = ovn_netlink_get_msgs(OVN_NL_NOTIFIER_NEIGHBOR);
- VECTOR_FOR_EACH_PTR (msgs, ne_msg) {
- if (vector_bsearch(&ntn->watches,
- &ne_msg->nd.if_index,
- if_index_cmp)) {
- state = EN_UPDATED;
- break;
+ /* We cannot tell whether an interface we watch was among the changes we
+ * missed, so assume it was. */
+ if (ovn_netlink_notifier_lost(OVN_NL_NOTIFIER_NEIGHBOR)) {
+ state = EN_UPDATED;
+ }
+
+ if (state != EN_UPDATED) {
+ msgs = ovn_netlink_get_msgs(OVN_NL_NOTIFIER_NEIGHBOR);
+ VECTOR_FOR_EACH_PTR (msgs, ne_msg) {
+ if (vector_bsearch(&ntn->watches,
+ &ne_msg->nd.if_index,
+ if_index_cmp)) {
+ state = EN_UPDATED;
+ break;
+ }
}
}
@@ -6588,6 +6605,12 @@ en_nexthop_exchange_run(struct engine_node *node
OVS_UNUSED, void *data)
vector_clear(&nhe_data->changed_ids);
nhe_data->resynced = false;
+ /* The messages we did get do not describe every change, so the table has
+ * to be read again to find out what it looks like now. */
+ if (ovn_netlink_notifier_lost(OVN_NL_NOTIFIER_NEXTHOP)) {
+ nhe_data->recompute = true;
+ }
+
if (nhe_data->recompute) {
nexthops_destroy(&nhe_data->nexthops);
nexthops_sync(&nhe_data->nexthops);
diff --git a/controller/ovn-netlink-notifier.c
b/controller/ovn-netlink-notifier.c
index ff0d01ca7550..04db42d0e109 100644
--- a/controller/ovn-netlink-notifier.c
+++ b/controller/ovn-netlink-notifier.c
@@ -40,6 +40,10 @@ struct ovn_netlink_notifier {
struct nln_notifier *nln_notifier;
/* Messages received by given notifier. */
struct vector msgs;
+ /* Set when the kernel reported a change we could not read, in which case
+ * 'msgs' does not describe everything that happened and the state derived
+ * from it has to be built again from scratch. */
+ bool lost;
/* Notifier change handler. */
nln_notify_func *change_handler;
/* Name of the notifier. */
@@ -57,6 +61,7 @@ static void ovn_netlink_neighbor_change_handler(const void
*change_,
void *aux);
static void ovn_netlink_nexthop_change_handler(const void *change_,
void *aux);
+static void ovn_netlink_notifier_report_lost(struct ovn_netlink_notifier *);
static struct ovn_netlink_notifier notifiers[OVN_NL_NOTIFIER_MAX] = {
[OVN_NL_NOTIFIER_ROUTE_V4] = {
@@ -115,14 +120,30 @@ ovn_netlink_notifier_parse(struct ofpbuf *buf, void
*change_)
return 0;
}
+/* Records that the kernel told us that something changed without us being
+ * able to tell what, which happens when the receive buffer overflows or when
+ * a message cannot be parsed. It is reported for every group, since the
+ * notifiers share a single socket. */
+static void
+ovn_netlink_notifier_report_lost(struct ovn_netlink_notifier *notifier)
+{
+ static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
+
+ VLOG_INFO_RL(&rl, "Missed %s table notifications, resyncing.",
+ notifier->name);
+ notifier->lost = true;
+}
+
static void
ovn_netlink_route_change_handler(const void *change_, void *aux)
{
+ struct ovn_netlink_notifier *notifier = aux;
+
if (!change_) {
+ ovn_netlink_notifier_report_lost(notifier);
return;
}
- struct ovn_netlink_notifier *notifier = aux;
union ovn_notifier_msg_change *change =
CONST_CAST(union ovn_notifier_msg_change *, change_);
@@ -139,11 +160,13 @@ ovn_netlink_route_change_handler(const void *change_,
void *aux)
static void
ovn_netlink_neighbor_change_handler(const void *change_, void *aux)
{
+ struct ovn_netlink_notifier *notifier = aux;
+
if (!change_) {
+ ovn_netlink_notifier_report_lost(notifier);
return;
}
- struct ovn_netlink_notifier *notifier = aux;
const union ovn_notifier_msg_change *change = change_;
if (!ne_is_ovn_owned(&change->neighbor.nd)) {
@@ -154,11 +177,13 @@ ovn_netlink_neighbor_change_handler(const void *change_,
void *aux)
static void
ovn_netlink_nexthop_change_handler(const void *change_, void *aux)
{
+ struct ovn_netlink_notifier *notifier = aux;
+
if (!change_) {
+ ovn_netlink_notifier_report_lost(notifier);
return;
}
- struct ovn_netlink_notifier *notifier = aux;
const union ovn_notifier_msg_change *change = change_;
vector_push(¬ifier->msgs, &change->nexthop);
}
@@ -238,6 +263,16 @@ ovn_netlink_get_msgs(enum ovn_netlink_notifier_type type)
return ¬ifiers[type].msgs;
}
+/* Returns true if notifications were missed since the last flush, in which
+ * case the messages of 'type' do not describe every change and the caller has
+ * to read the table it tracks again. */
+bool
+ovn_netlink_notifier_lost(enum ovn_netlink_notifier_type type)
+{
+ ovs_assert(type < OVN_NL_NOTIFIER_MAX);
+ return notifiers[type].lost;
+}
+
void
ovn_netlink_notifier_flush(enum ovn_netlink_notifier_type type)
{
@@ -260,6 +295,7 @@ ovn_netlink_notifier_flush(enum ovn_netlink_notifier_type
type)
}
vector_clear(¬ifier->msgs);
+ notifier->lost = false;
}
void
diff --git a/controller/ovn-netlink-notifier.h
b/controller/ovn-netlink-notifier.h
index 208a28d9968e..08a8380e8924 100644
--- a/controller/ovn-netlink-notifier.h
+++ b/controller/ovn-netlink-notifier.h
@@ -31,6 +31,7 @@ enum ovn_netlink_notifier_type {
void ovn_netlink_update_notifier(enum ovn_netlink_notifier_type type,
bool enabled);
struct vector *ovn_netlink_get_msgs(enum ovn_netlink_notifier_type type);
+bool ovn_netlink_notifier_lost(enum ovn_netlink_notifier_type type);
void ovn_netlink_notifier_flush(enum ovn_netlink_notifier_type type);
void ovn_netlink_notifiers_run(void);
void ovn_netlink_notifiers_wait(void);
--
2.38.1
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
