Hi Ilya,
On 8/14/25 2:35 PM, Ilya Maximets wrote:
> On 8/12/25 4:56 PM, Ales Musil via dev wrote:
>> From: Dumitru Ceara <dce...@redhat.com>
>>
>> For each (Linux) interface that's interesting for the OVN control plane,
>> watch for neighbor (fdb/arp) changes. This allows us to reconcile on
>> changed neighbor entries from outside routing agents (e.g., FRR).
>>
>> This is the neighbor equivalent of the support added for route tables in
>> 673d90f1173f ("controller: Watch for route changes.").
>>
>> Signed-off-by: Dumitru Ceara <dce...@redhat.com>
>> ---
>> V3:
>> - renamed fdb_table_change() to neighbor_table_change()
>> - added tests
>>
>> Changes in V2:
>> - fixed up log messages
>> - changed code to make OVN only manage neighbor entries with VLAN 0
>> - added advertise_neigh_find() helper
>> ---
>> Makefile.am | 5 +-
>> controller/automake.mk | 5 +-
>> controller/neighbor-table-notify-stub.c | 57 ++++++
>> controller/neighbor-table-notify.c | 244 ++++++++++++++++++++++++
>> controller/neighbor-table-notify.h | 45 +++++
>> controller/test-ovn-netlink.c | 41 ++++
>> tests/automake.mk | 1 +
>> tests/system-ovn-netlink.at | 55 ++++++
>> 8 files changed, 451 insertions(+), 2 deletions(-)
>> create mode 100644 controller/neighbor-table-notify-stub.c
>> create mode 100644 controller/neighbor-table-notify.c
>> create mode 100644 controller/neighbor-table-notify.h
>>
>> diff --git a/Makefile.am b/Makefile.am
>> index ea98fb5fb..d8e24d0ab 100644
>> --- a/Makefile.am
>> +++ b/Makefile.am
>> @@ -350,13 +350,16 @@ check-tabs:
>> fi
>> .PHONY: check-tabs
>>
>> +# NOTE: test-ovn-netlink.c excluded due to use of system() to execute
>> +# ip neigh / bridge fdb commands provided as arguments by test suite.
>> ALL_LOCAL += thread-safety-check
>> thread-safety-check:
>> @cd $(srcdir); \
>> if test -e .git && (git --version) >/dev/null 2>&1 && \
>> grep -n -f build-aux/thread-safety-blacklist \
>> `git ls-files | grep -v $(submodules) | grep '\.[ch]$$'` /dev/null \
>> - | $(EGREP) -v ':[ ]*/?\*'; \
>> + | $(EGREP) -v ':[ ]*/?\*' \
>> + | $(EGREP) -v '^controller/test-ovn-netlink.c'; \
>> then \
>> echo "See above for list of calls to functions that are"; \
>> echo "blacklisted due to thread safety issues"; \
>> diff --git a/controller/automake.mk b/controller/automake.mk
>> index 3eb45475c..6af6ee2a9 100644
>> --- a/controller/automake.mk
>> +++ b/controller/automake.mk
>> @@ -63,18 +63,21 @@ controller_ovn_controller_SOURCES = \
>> controller/route.h \
>> controller/route.c \
>> controller/garp_rarp.h \
>> - controller/garp_rarp.c
>> + controller/garp_rarp.c \
>> + controller/neighbor-table-notify.h
>>
>> if HAVE_NETLINK
>> controller_ovn_controller_SOURCES += \
>> controller/neighbor-exchange-netlink.h \
>> controller/neighbor-exchange-netlink.c \
>> + controller/neighbor-table-notify.c \
>> controller/route-exchange-netlink.h \
>> controller/route-exchange-netlink.c \
>> controller/route-exchange.c \
>> controller/route-table-notify.c
>> else
>> controller_ovn_controller_SOURCES += \
>> + controller/neighbor-table-notify-stub.c \
>> controller/route-exchange-stub.c \
>> controller/route-table-notify-stub.c
>> endif
>> diff --git a/controller/neighbor-table-notify-stub.c
>> b/controller/neighbor-table-notify-stub.c
>> new file mode 100644
>> index 000000000..bb4fe5991
>> --- /dev/null
>> +++ b/controller/neighbor-table-notify-stub.c
>> @@ -0,0 +1,57 @@
>> +/* Copyright (c) 2025, Red Hat, Inc.
>> + *
>> + * Licensed under the Apache License, Version 2.0 (the "License");
>> + * you may not use this file except in compliance with the License.
>> + * You may obtain a copy of the License at:
>> + *
>> + * http://www.apache.org/licenses/LICENSE-2.0
>> + *
>> + * Unless required by applicable law or agreed to in writing, software
>> + * distributed under the License is distributed on an "AS IS" BASIS,
>> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
>> + * See the License for the specific language governing permissions and
>> + * limitations under the License.
>> + */
>> +
>> +#include <config.h>
>> +
>> +#include <stdbool.h>
>> +
>> +#include "openvswitch/compiler.h"
>> +#include "neighbor-table-notify.h"
>> +
>> +bool
>> +neighbor_table_notify_run(void)
>> +{
>> + return false;
>> +}
>> +
>> +void
>> +neighbor_table_notify_wait(void)
>> +{
>> +}
>> +
>> +void
>> +neighbor_table_add_watch_request(
>> + struct hmap *neighbor_table_watches OVS_UNUSED,
>> + int32_t if_index OVS_UNUSED,
>> + const char *if_name OVS_UNUSED)
>> +{
>> +}
>> +
>> +void
>> +neighbor_table_watch_request_cleanup(
>> + struct hmap *neighbor_table_watches OVS_UNUSED)
>> +{
>> +}
>> +
>> +void
>> +neighbor_table_notify_update_watches(
>> + const struct hmap *neighbor_table_watches OVS_UNUSED)
>> +{
>> +}
>> +
>> +void
>> +neighbor_table_notify_destroy(void)
>> +{
>> +}
>> diff --git a/controller/neighbor-table-notify.c
>> b/controller/neighbor-table-notify.c
>> new file mode 100644
>> index 000000000..dd0b320c4
>> --- /dev/null
>> +++ b/controller/neighbor-table-notify.c
>> @@ -0,0 +1,244 @@
>> +/* Copyright (c) 2025, Red Hat, Inc.
>> + *
>> + * Licensed under the Apache License, Version 2.0 (the "License");
>> + * you may not use this file except in compliance with the License.
>> + * You may obtain a copy of the License at:
>> + *
>> + * http://www.apache.org/licenses/LICENSE-2.0
>> + *
>> + * Unless required by applicable law or agreed to in writing, software
>> + * distributed under the License is distributed on an "AS IS" BASIS,
>> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
>> + * See the License for the specific language governing permissions and
>> + * limitations under the License.
>> + */
>> +
>> +#include <config.h>
>> +
>> +#include <linux/rtnetlink.h>
>> +#include <net/if.h>
>> +
>> +#include "hash.h"
>> +#include "hmapx.h"
>> +#include "lib/util.h"
>> +#include "netlink-notifier.h"
>> +#include "openvswitch/vlog.h"
>> +
>> +#include "neighbor-exchange-netlink.h"
>> +#include "neighbor-table-notify.h"
>> +
>> +VLOG_DEFINE_THIS_MODULE(neighbor_table_notify);
>> +
>> +struct neighbor_table_watch_request {
>> + struct hmap_node node;
>> + int32_t if_index;
>> + char if_name[IFNAMSIZ + 1];
>> +};
>> +
>> +struct neighbor_table_watch_entry {
>> + struct hmap_node node;
>> + int32_t if_index;
>> + char if_name[IFNAMSIZ + 1];
>> +};
>> +
>> +static struct hmap watches = HMAP_INITIALIZER(&watches);
>> +static bool any_neighbor_table_changed;
>> +static struct ne_table_msg nln_nmsg_change;
>> +
>> +static struct nln *nl_neighbor_handle;
>> +static struct nln_notifier *nl_neighbor_notifier;
>> +
>> +static void neighbor_table_change(const void *change_, void *aux);
>> +
>> +static void
>> +neighbor_table_register_notifiers(void)
>> +{
>> + VLOG_INFO("Adding neighbor table watchers.");
>> + ovs_assert(!nl_neighbor_handle);
>> +
>> + nl_neighbor_handle = nln_create(NETLINK_ROUTE, ne_table_parse,
>> + &nln_nmsg_change);
>> + ovs_assert(nl_neighbor_handle);
>> +
>> + nl_neighbor_notifier =
>> + nln_notifier_create(nl_neighbor_handle, RTNLGRP_NEIGH,
>> + neighbor_table_change, NULL);
>> + if (!nl_neighbor_notifier) {
>> + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
>> + VLOG_WARN_RL(&rl, "Failed to create neighbor table watcher.");
>> + }
>> +}
>> +
>> +static void
>> +neighbor_table_deregister_notifiers(void)
>> +{
>> + VLOG_INFO("Removing neighbor table watchers.");
>> + ovs_assert(nl_neighbor_handle);
>> +
>> + nln_notifier_destroy(nl_neighbor_notifier);
>> + nln_destroy(nl_neighbor_handle);
>> + nl_neighbor_notifier = NULL;
>> + nl_neighbor_handle = NULL;
>> +}
>> +
>> +static uint32_t
>> +neighbor_table_notify_hash_watch(int32_t if_index)
>> +{
>> + /* To allow lookups triggered by netlink messages, don't include the
>> + * if_name in the hash. The netlink updates only include if_index. */
>> + return hash_int(if_index, 0);
>> +}
>> +
>> +static void
>> +add_watch_entry(int32_t if_index, const char *if_name)
>> +{
>> + VLOG_INFO("Registering new neighbor table watcher "
>> + "for if_index %s (%"PRId32").",
>> + if_name, if_index);
>> +
>> + struct neighbor_table_watch_entry *we;
>> + uint32_t hash = neighbor_table_notify_hash_watch(if_index);
>> + we = xzalloc(sizeof *we);
>> + we->if_index = if_index;
>> + ovs_strzcpy(we->if_name, if_name, IFNAMSIZ + 1);
>
> nit: sizeof wr->if_name.
>
Ack.
>> + hmap_insert(&watches, &we->node, hash);
>> +
>> + if (!nl_neighbor_handle) {
>> + neighbor_table_register_notifiers();
>> + }
>> +}
>> +
>> +static void
>> +remove_watch_entry(struct neighbor_table_watch_entry *we)
>> +{
>> + VLOG_INFO("Removing neighbor table watcher for table %s (%"PRId32").",
>
> "for table" ?
>
Ah, copy/paste. :D
> Also, should these be debug logs, maybe?
>
Yeah, that's maybe better, I'll change it.
>> + we->if_name, we->if_index);
>> + hmap_remove(&watches, &we->node);
>> + free(we);
>> +
>> + if (hmap_is_empty(&watches)) {
>> + neighbor_table_deregister_notifiers();
>> + }
>> +}
>> +
>> +bool
>> +neighbor_table_notify_run(void)
>> +{
>> + any_neighbor_table_changed = false;
>> +
>> + if (nl_neighbor_handle) {
>> + nln_run(nl_neighbor_handle);
>> + }
>> +
>> + return any_neighbor_table_changed;
>> +}
>> +
>> +void
>> +neighbor_table_notify_wait(void)
>> +{
>> + if (nl_neighbor_handle) {
>> + nln_wait(nl_neighbor_handle);
>> + }
>> +}
>> +
>> +void
>> +neighbor_table_add_watch_request(struct hmap *neighbor_table_watches,
>> + int32_t if_index, const char *if_name)
>> +{
>> + struct neighbor_table_watch_request *wr = xzalloc(sizeof *wr);
>> +
>> + wr->if_index = if_index;
>> + ovs_strzcpy(wr->if_name, if_name, IFNAMSIZ + 1);
>
> nit: sizeof wr->if_name.
>
Ack.
>> + hmap_insert(neighbor_table_watches, &wr->node,
>> + neighbor_table_notify_hash_watch(wr->if_index));
>> +}
>> +
>> +void
>> +neighbor_table_watch_request_cleanup(struct hmap *neighbor_table_watches)
>> +{
>> + struct neighbor_table_watch_request *wr;
>> + HMAP_FOR_EACH_POP (wr, node, neighbor_table_watches) {
>> + free(wr);
>> + }
>> +}
>> +
>> +static struct neighbor_table_watch_entry *
>> +find_watch_entry(int32_t if_index, const char *if_name)
>> +{
>> + struct neighbor_table_watch_entry *we;
>> + uint32_t hash = neighbor_table_notify_hash_watch(if_index);
>> + HMAP_FOR_EACH_WITH_HASH (we, node, hash, &watches) {
>> + if (if_index == we->if_index && !strcmp(if_name, we->if_name)) {
>> + return we;
>> + }
>> + }
>> + return NULL;
>> +}
>> +
>> +static struct neighbor_table_watch_entry *
>> +find_watch_entry_by_if_index(int32_t if_index)
>> +{
>> + struct neighbor_table_watch_entry *we;
>> + uint32_t hash = neighbor_table_notify_hash_watch(if_index);
>> + HMAP_FOR_EACH_WITH_HASH (we, node, hash, &watches) {
>> + if (if_index == we->if_index) {
>> + return we;
>> + }
>> + }
>> + return NULL;
>> +}
>> +
>> +void
>> +neighbor_table_notify_update_watches(const struct hmap
>> *neighbor_table_watches)
>> +{
>> + struct hmapx sync_watches = HMAPX_INITIALIZER(&sync_watches);
>> + struct neighbor_table_watch_entry *we;
>> + HMAP_FOR_EACH (we, node, &watches) {
>> + hmapx_add(&sync_watches, we);
>> + }
>> +
>> + struct neighbor_table_watch_request *wr;
>> + HMAP_FOR_EACH (wr, node, neighbor_table_watches) {
>> + we = find_watch_entry(wr->if_index, wr->if_name);
>> + if (we) {
>> + hmapx_find_and_delete(&sync_watches, we);
>> + } else {
>> + add_watch_entry(wr->if_index, wr->if_name);
>> + }
>> + }
>> +
>> + struct hmapx_node *node;
>> + HMAPX_FOR_EACH (node, &sync_watches) {
>> + remove_watch_entry(node->data);
>> + }
>> +
>> + hmapx_destroy(&sync_watches);
>> +}
>> +
>> +void
>> +neighbor_table_notify_destroy(void)
>> +{
>> + struct neighbor_table_watch_entry *we;
>> + HMAP_FOR_EACH_SAFE (we, node, &watches) {
>> + remove_watch_entry(we);
>> + }
>> +}
>> +
>> +static void
>> +neighbor_table_change(const void *change_, void *aux OVS_UNUSED)
>> +{
>> + /* We currently track whether at least one recent neighbor table change
>> + * was detected. If that's the case already there's no need to
>> + * continue. */
>> + if (any_neighbor_table_changed) {
>> + return;
>> + }
>> +
>> + const struct ne_table_msg *change = change_;
>> +
>> + if (change && !ne_is_ovn_owned(&change->nd)) {
>> + if (find_watch_entry_by_if_index(change->nd.if_index)) {
>> + any_neighbor_table_changed = true;
>> + }
>> + }
>> +}
>> diff --git a/controller/neighbor-table-notify.h
>> b/controller/neighbor-table-notify.h
>> new file mode 100644
>> index 000000000..9f21271cc
>> --- /dev/null
>> +++ b/controller/neighbor-table-notify.h
>> @@ -0,0 +1,45 @@
>> +/* Copyright (c) 2025, Red Hat, Inc.
>> + *
>> + * Licensed under the Apache License, Version 2.0 (the "License");
>> + * you may not use this file except in compliance with the License.
>> + * You may obtain a copy of the License at:
>> + *
>> + * http://www.apache.org/licenses/LICENSE-2.0
>> + *
>> + * Unless required by applicable law or agreed to in writing, software
>> + * distributed under the License is distributed on an "AS IS" BASIS,
>> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
>> + * See the License for the specific language governing permissions and
>> + * limitations under the License.
>> + */
>> +
>> +#ifndef NEIGHBOR_TABLE_NOTIFY_H
>> +#define NEIGHBOR_TABLE_NOTIFY_H 1
>> +
>> +#include <stdbool.h>
>> +#include "openvswitch/hmap.h"
>> +
>> +/* Returns true if any neighbor table has changed enough that we need
>> + * to learn new neighbor entries. */
>> +bool neighbor_table_notify_run(void);
>> +void neighbor_table_notify_wait(void);
>> +
>> +/* Add a watch request to the hmap. The hmap should later be passed to
>> + * neighbor_table_notify_update_watches*/
>> +void neighbor_table_add_watch_request(struct hmap *neighbor_table_watches,
>> + int32_t if_index, const char
>> *if_name);
>> +
>> +/* Cleanup all watch request in the provided hmap that where added using
>> + * neighbor_table_add_watch_request. */
>> +void neighbor_table_watch_request_cleanup(
>> + struct hmap *neighbor_table_watches);
>> +
>> +/* Updates the list of neighbor table watches that are currently active.
>> + * hmap should contain struct neighbor_table_watch_request */
>> +void neighbor_table_notify_update_watches(
>> + const struct hmap *neighbor_table_watches);
>> +
>> +/* Cleans up all neighbor table watches. */
>> +void neighbor_table_notify_destroy(void);
>> +
>> +#endif /* NEIGHBOR_TABLE_NOTIFY_H */
>> diff --git a/controller/test-ovn-netlink.c b/controller/test-ovn-netlink.c
>> index 4134d9f0a..a26e2d478 100644
>> --- a/controller/test-ovn-netlink.c
>> +++ b/controller/test-ovn-netlink.c
>> @@ -21,6 +21,7 @@
>> #include "tests/test-utils.h"
>>
>> #include "neighbor-exchange-netlink.h"
>> +#include "neighbor-table-notify.h"
>> #include "neighbor.h"
>>
>> static void
>> @@ -103,12 +104,52 @@ done:
>> vector_destroy(&received_neighbors);
>> }
>>
>> +static void
>> +test_neighbor_table_notify(struct ovs_cmdl_context *ctx)
>> +{
>> + unsigned int shift = 1;
>> +
>> + const char *if_name = test_read_value(ctx, shift++, "if_name");
>> + if (!if_name) {
>> + return;
>> + }
>> +
>> + unsigned int if_index;
>> + if (!test_read_uint_value(ctx, shift++, "if_index", &if_index)) {
>> + return;
>> + }
>> +
>> + const char *cmd = test_read_value(ctx, shift++, "shell_command");
>> + if (!cmd) {
>> + return;
>> + }
>> +
>> + const char *notify = test_read_value(ctx, shift++, "should_notify");
>> + bool expect_notify = notify && !strcmp(notify, "true");
>> +
>> + struct hmap table_watches = HMAP_INITIALIZER(&table_watches);
>> + neighbor_table_add_watch_request(&table_watches, if_index, if_name);
>> + neighbor_table_notify_update_watches(&table_watches);
>> +
>> + neighbor_table_notify_run();
>> + neighbor_table_notify_wait();
>> +
>> + int rc = system(cmd);
>> + if (rc) {
>> + exit(rc);
>> + }
>> + ovs_assert(neighbor_table_notify_run() == expect_notify);
>> + neighbor_table_watch_request_cleanup(&table_watches);
>> +}
>> +
>> static void
>> test_ovn_netlink(int argc, char *argv[])
>> {
>> set_program_name(argv[0]);
>> static const struct ovs_cmdl_command commands[] = {
>> {"neighbor-sync", NULL, 2, INT_MAX, test_neighbor_sync, OVS_RO},
>> + {"neighbor-table-notify", NULL, 3, 4,
>> + test_neighbor_table_notify, OVS_RO},
>> {NULL, NULL, 0, 0, NULL, OVS_RO},
>> };
>> struct ovs_cmdl_context ctx;
>> diff --git a/tests/automake.mk b/tests/automake.mk
>> index b2db67e99..ba6b6d7ba 100644
>> --- a/tests/automake.mk
>> +++ b/tests/automake.mk
>> @@ -320,6 +320,7 @@ if HAVE_NETLINK
>> tests_ovstest_LDADD += \
>> controller/neighbor.$(OBJEXT) \
>> controller/neighbor-exchange-netlink.$(OBJEXT) \
>> + controller/neighbor-table-notify.$(OBJEXT) \
>> controller/route-exchange-netlink.$(OBJEXT) \
>> controller/test-ovn-netlink.$(OBJEXT)
>> endif
>> diff --git a/tests/system-ovn-netlink.at b/tests/system-ovn-netlink.at
>> index 6a21c0e56..068dfdc63 100644
>> --- a/tests/system-ovn-netlink.at
>> +++ b/tests/system-ovn-netlink.at
>> @@ -176,3 +176,58 @@ OVN_NEIGH_EQUAL([br-test], [nud noarp], [20.20.20], [dnl
>> OVN_NEIGH_V6_EQUAL([br-test], [nud noarp], [20::], [dnl
>> 20::20 lladdr 00:00:00:00:20:00 NOARP])
>> AT_CLEANUP
>> +
>> +AT_SETUP([sync netlink neighbors - table notify])
>> +AT_KEYWORDS([netlink-neighbors])
>> +
>> +check ip link add br-test type bridge
>> +on_exit 'ip link del br-test'
>> +check ip link set br-test address 00:00:00:00:00:01
>> +check ip address add dev br-test 10.10.10.1/24
>> +check ip link set dev br-test up
>> +
>> +check ip link add lo-test type dummy
>> +on_exit 'ip link del lo-test'
>> +check ip link set lo-test master br-test
>> +check ip link set lo-test address 00:00:00:00:00:02
>> +check ip link set dev lo-test up
>> +lo_if_index=$(netlink_if_index lo-test)
>> +
>> +check ip link add br-test-unused type bridge
>> +on_exit 'ip link del br-test-unused'
>> +check ip link set br-test-unused address 00:00:00:00:00:03
>> +check ip address add dev br-test-unused 20.20.20.1/24
>> +check ip link set dev br-test-unused up
>> +
>> +check ip link add lo-test-unused type dummy
>> +on_exit 'ip link del lo-test-unused'
>> +check ip link set lo-test-unused master br-test-unused
>> +check ip link set lo-test-unused address 00:00:00:00:00:04
>> +check ip link set dev lo-test-unused up
>> +
>> +dnl Should notify if an entry is added to a bridge port monitored by OVN.
>> +check ovstest test-ovn-netlink neighbor-table-notify lo-test $lo_if_index \
>> + 'bridge fdb add 00:00:00:00:00:05 dev lo-test' \
>> + true
>> +
>> +dnl Should NOT notify if an entry is added to a bridge port that's not
>> +dnl monitored by OVN.
>> +check ovstest test-ovn-netlink neighbor-table-notify lo-test $lo_if_index \
>> + 'bridge fdb add 00:00:00:00:00:05 dev lo-test-unused' \
>> + false
>> +
>> +br_if_index=$(netlink_if_index br-test)
>> +dnl Should notify if an entry is added to a bridge that's monitored by
>> +dnl OVN.
>> +check ovstest test-ovn-netlink neighbor-table-notify br-test $br_if_index \
>> + 'ip neigh add 10.10.10.10 lladdr 00:00:00:00:10:00 \
>> + dev br-test extern_learn' \
>> + true
>> +
>> +dnl Should NOT notify if an entry is added to a bridge that's not monitored
>> by
>> +dnl OVN.
>> +check ovstest test-ovn-netlink neighbor-table-notify br-test $br_if_index \
>> + 'ip neigh add 20.20.20.20 lladdr 00:00:00:00:20:00 \
>> + dev br-test-unused extern_learn' \
>> + false
>> +AT_CLEANUP
>
Regards,
Dumitru
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
