Hi Ales, Dumitru Thanks for the patch. Only one nit below Acked-by: Xavier Simonart <xsimo...@redhat.com>
Thanks Xavier On Wed, Aug 6, 2025 at 8:27 PM Ales Musil via dev <ovs-dev@openvswitch.org> wrote: > From: Dumitru Ceara <dce...@redhat.com> > > This enhances ovn-controller with (incremental-processing) support for > managing (Linux) neighbor table entries in the context of integrating > with EVPN control planes (e.g., FRR). > > The following diagram depicts the dependencies between the newly added > nodes and how they interact with the rest of the existing I-P engine > nodes in ovn-controller: > > <SB-DATA> > | > | > v > +----------+----------+ +--------------------+ > | en_neighbor | | en_host_if_monitor | > +----------+----------+ +----------+---------+ > | | > | +------------+ > | | +--------------------------+ > | | | en_neighbor_table_notify | > | | +----------+---------------+ > | | | > | | +-------------------+ > | | | > | | | > +-----------------------------+ > | | | | en_neighbor_exchange_status > | > | | | > +----------+------------------+ > | | | | > | v v | > | +-------+------+-------+ | > +----> en_neighbor_exchange <----------------+ > +-----------+----------+ > | > v > +-----------+----------+ > | en_controller_output | > +----------------------+ > > Where: > - <SB-DATA>: > represents to-be-added input built from OVN Southbound contents > (e.g., VNI information in SB.Datapath_Binding records used for > EVPN integration with the fabric). > - en_neighbor: > - builds the set of (Linux) interfaces that need to be monitored > (for if-index and neighbor table changes) > - en_host_if_monitor: > - triggers depending nodes (i.e., en_neighbor_exchange) to run if > relevant if-index <-> if-name mapping changes have been detected > nit: I found this slightly confusing, as from the code below I think that an interface name *change* is not handled/supported (on an interface name change, I think that only a RTM_NEWLINK is received). Not sure we want to support name changes, so comment could be ... 'relevant if-index <-> if-name mapping have been added or deleted'' > - en_neighbor_table_notify: > - triggers depending nodes (i.e., en_neighbor_exchange) to run if > relevant (Linux) neighbor table changes have been detected > - en_neighbor_exchange_status: > - triggers en_neighbor_exchange to run if a previous neighbor-related > Netlink operation has failed > - en_neighbor_exchange: > - reconciles the (Linux) neighbor table state for relevant interfaces, > that is, it learns routing control plane (e.g., FRR) added > neighbors and it installs neighbor entries for FDB/MAC_Binding > records that OVN needs to advertise to the fabric. > > NOTE: the code has two "TODO GLUE" markers where subsequent commits > should plug the implementation of: > - building inputs for the en_neighbor node from <SB-DATA> > - dynamically learning remote EVPN VTEPs > - installing OpenFlow rules for remote FDB/neighbor records learned from > the fabric > > Reported-at: https://issues.redhat.com/browse/FDP-1416 > Signed-off-by: Dumitru Ceara <dce...@redhat.com> > --- > Changes in V2: > - Added CLEAR_NEIGHBOR_EXCHANGE_NL_STATUS() to neighbor_exchange_run(). > - Added missing neighbor_table_notify_wait() call. > --- > controller/automake.mk | 3 + > controller/neighbor-exchange-stub.c | 30 ++++ > controller/neighbor-exchange.c | 85 ++++++++++ > controller/neighbor-exchange.h | 36 +++++ > controller/neighbor.c | 10 ++ > controller/ovn-controller.c | 240 +++++++++++++++++++++++++++- > 6 files changed, 402 insertions(+), 2 deletions(-) > create mode 100644 controller/neighbor-exchange-stub.c > create mode 100644 controller/neighbor-exchange.c > create mode 100644 controller/neighbor-exchange.h > > diff --git a/controller/automake.mk b/controller/automake.mk > index 6eca5e8ed..37cfd4396 100644 > --- a/controller/automake.mk > +++ b/controller/automake.mk > @@ -64,6 +64,7 @@ controller_ovn_controller_SOURCES = \ > controller/route.c \ > controller/garp_rarp.h \ > controller/garp_rarp.c \ > + controller/neighbor-exchange.h \ > controller/neighbor-table-notify.h \ > controller/host-if-monitor.h > > @@ -72,6 +73,7 @@ controller_ovn_controller_SOURCES += \ > controller/host-if-monitor.c \ > controller/neighbor-exchange-netlink.h \ > controller/neighbor-exchange-netlink.c \ > + controller/neighbor-exchange.c \ > controller/neighbor-table-notify.c \ > controller/route-exchange-netlink.h \ > controller/route-exchange-netlink.c \ > @@ -80,6 +82,7 @@ controller_ovn_controller_SOURCES += \ > else > controller_ovn_controller_SOURCES += \ > controller/host-if-monitor-stub.c \ > + controller/neighbor-exchange-stub.c \ > controller/neighbor-table-notify-stub.c \ > controller/route-exchange-stub.c \ > controller/route-table-notify-stub.c > diff --git a/controller/neighbor-exchange-stub.c > b/controller/neighbor-exchange-stub.c > new file mode 100644 > index 000000000..a42df84c2 > --- /dev/null > +++ b/controller/neighbor-exchange-stub.c > @@ -0,0 +1,30 @@ > +/* 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 "neighbor-exchange.h" > + > +void > +neighbor_exchange_run(const struct neighbor_exchange_ctx_in *ctx_in > OVS_UNUSED, > + struct neighbor_exchange_ctx_out *ctx_out > OVS_UNUSED) > +{ > +} > + > +int > +neighbor_exchange_status_run(void) > +{ > + return 0; > +} > diff --git a/controller/neighbor-exchange.c > b/controller/neighbor-exchange.c > new file mode 100644 > index 000000000..8e3bf3ecb > --- /dev/null > +++ b/controller/neighbor-exchange.c > @@ -0,0 +1,85 @@ > +/* 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 "host-if-monitor.h" > +#include "neighbor.h" > +#include "neighbor-exchange.h" > +#include "neighbor-exchange-netlink.h" > +#include "neighbor-table-notify.h" > +#include "vec.h" > + > +/* Last neighbor_exchange netlink operation. */ > +static int neighbor_exchange_nl_status; > + > +#define CLEAR_NEIGHBOR_EXCHANGE_NL_STATUS() \ > + do { \ > + neighbor_exchange_nl_status = 0; \ > + } while (0) > + > +#define SET_NEIGHBOR_EXCHANGE_NL_STATUS(error) \ > + do { \ > + if (!neighbor_exchange_nl_status) { \ > + neighbor_exchange_nl_status = (error); \ > + } \ > + } while (0) > + > +void > +neighbor_exchange_run(const struct neighbor_exchange_ctx_in *n_ctx_in, > + struct neighbor_exchange_ctx_out *n_ctx_out) > +{ > + struct neighbor_interface_monitor *nim; > + > + struct sset if_names = SSET_INITIALIZER(&if_names); > + VECTOR_FOR_EACH (n_ctx_in->monitored_interfaces, nim) { > + sset_add(&if_names, nim->if_name); > + } > + host_if_monitor_update_watches(&if_names); > + sset_destroy(&if_names); > + > + CLEAR_NEIGHBOR_EXCHANGE_NL_STATUS(); > + VECTOR_FOR_EACH (n_ctx_in->monitored_interfaces, nim) { > + int32_t if_index = host_if_monitor_ifname_toindex(nim->if_name); > + > + if (!if_index) { > + continue; > + } > + > + struct vector received_neighbors = > + VECTOR_EMPTY_INITIALIZER(struct ne_nl_received_neigh); > + SET_NEIGHBOR_EXCHANGE_NL_STATUS( > + ne_nl_sync_neigh(nim->family, if_index, > &nim->announced_neighbors, > + &received_neighbors) > + ); > + > + /* XXX: TODO GLUE: sync received neighbors to: > + * - SB: for remote vtep entries > + * https://issues.redhat.com/browse/FDP-1385 > + * - in memory table for remote neighbor entries > + * https://issues.redhat.com/browse/FDP-1387 > + */ > + > + > neighbor_table_add_watch_request(&n_ctx_out->neighbor_table_watches, > + if_index, nim->if_name); > + vector_destroy(&received_neighbors); > + } > +} > + > +int > +neighbor_exchange_status_run(void) > +{ > + return neighbor_exchange_nl_status; > +} > diff --git a/controller/neighbor-exchange.h > b/controller/neighbor-exchange.h > new file mode 100644 > index 000000000..afbbe9811 > --- /dev/null > +++ b/controller/neighbor-exchange.h > @@ -0,0 +1,36 @@ > +/* 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_EXCHANGE_H > +#define NEIGHBOR_EXCHANGE_H 1 > + > +#include "lib/sset.h" > +#include "openvswitch/hmap.h" > + > +struct neighbor_exchange_ctx_in { > + /* Contains struct neighbor_interface_monitor pointers. */ > + const struct vector *monitored_interfaces; > +}; > + > +struct neighbor_exchange_ctx_out { > + /* Contains struct neighbor_table_watch_request. */ > + struct hmap neighbor_table_watches; > +}; > + > +void neighbor_exchange_run(const struct neighbor_exchange_ctx_in *, > + struct neighbor_exchange_ctx_out *); > +int neighbor_exchange_status_run(void); > + > +#endif /* NEIGHBOR_EXCHANGE_H */ > diff --git a/controller/neighbor.c b/controller/neighbor.c > index 150372fb2..2e48b104d 100644 > --- a/controller/neighbor.c > +++ b/controller/neighbor.c > @@ -52,6 +52,16 @@ neighbor_run(struct neighbor_ctx_in *n_ctx_in > OVS_UNUSED, > struct neighbor_ctx_out *n_ctx_out OVS_UNUSED) > { > /* XXX: Not implemented yet. */ > + > + /* XXX: TODO GLUE: get information from (n_ctx_in) SB (runtime-data) > about: > + * - local datapath vni (and listen on br-$vni, lo-$vni and > vxlan-$vni) > + * for which we want to enable neighbor monitoring > + * https://issues.redhat.com/browse/FDP-1385 > + * - what FDB/neighbor entries to advertise > + * https://issues.redhat.com/browse/FDP-1389 > + * > + * And populate that in n_ctx_out. > + */ > } > > void > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > index 5303c6551..24b86aa86 100644 > --- a/controller/ovn-controller.c > +++ b/controller/ovn-controller.c > @@ -94,6 +94,10 @@ > #include "route-exchange.h" > #include "route-table-notify.h" > #include "garp_rarp.h" > +#include "host-if-monitor.h" > +#include "neighbor.h" > +#include "neighbor-exchange.h" > +#include "neighbor-table-notify.h" > > VLOG_DEFINE_THIS_MODULE(main); > > @@ -4984,6 +4988,14 @@ controller_output_garp_rarp_handler(struct > engine_node *node OVS_UNUSED, > return EN_HANDLED_UPDATED; > } > > +static enum engine_input_handler_result > +controller_output_neighbor_exchange_handler( > + struct engine_node *node OVS_UNUSED, > + void *data OVS_UNUSED) > +{ > + return EN_HANDLED_UPDATED; > +} > + > /* Handles sbrec_chassis changes. > * If a new chassis is added or removed return false, so that > * flows are recomputed. For any updates, there is no need for > @@ -5657,6 +5669,200 @@ garp_rarp_runtime_data_handler(struct engine_node > *node, void *data OVS_UNUSED) > return EN_HANDLED_UNCHANGED; > } > > +struct ed_type_host_if_monitor { > + /* For incremental processing this could be tracked per interface in > + * the future. */ > + bool changed; > +}; > + > +static void * > +en_host_if_monitor_init(struct engine_node *node OVS_UNUSED, > + struct engine_arg *arg OVS_UNUSED) > +{ > + struct ed_type_host_if_monitor *hifm = xmalloc(sizeof *hifm); > + > + *hifm = (struct ed_type_host_if_monitor) { > + .changed = true, > + }; > + return hifm; > +} > + > +static void > +en_host_if_monitor_cleanup(void *data OVS_UNUSED) > +{ > +} > + > +static enum engine_node_state > +en_host_if_monitor_run(struct engine_node *node OVS_UNUSED, void *data) > +{ > + struct ed_type_host_if_monitor *hifm = data; > + enum engine_node_state state; > + if (hifm->changed) { > + state = EN_UPDATED; > + } else { > + state = EN_UNCHANGED; > + } > + hifm->changed = false; > + return state; > +} > + > +struct ed_type_neighbor { > + /* Contains struct neighbor_interface_monitor pointers. */ > + struct vector monitored_interfaces; > +}; > + > +static void * > +en_neighbor_init(struct engine_node *node OVS_UNUSED, > + struct engine_arg *arg OVS_UNUSED) > +{ > + struct ed_type_neighbor *data = xzalloc(sizeof *data); > + > + *data = (struct ed_type_neighbor) { > + .monitored_interfaces = > + VECTOR_EMPTY_INITIALIZER(struct neighbor_interface_monitor *), > + }; > + return data; > +} > + > +static void > +en_neighbor_cleanup(void *data) > +{ > + struct ed_type_neighbor *ne_data = data; > + > + neighbor_cleanup(&ne_data->monitored_interfaces); > + vector_destroy(&ne_data->monitored_interfaces); > +} > + > +static enum engine_node_state > +en_neighbor_run(struct engine_node *node OVS_UNUSED, void *data) > +{ > + struct ed_type_neighbor *ne_data = data; > + struct neighbor_ctx_in n_ctx_in = { > + }; > + > + struct neighbor_ctx_out n_ctx_out = { > + .monitored_interfaces = &ne_data->monitored_interfaces, > + }; > + > + neighbor_cleanup(&ne_data->monitored_interfaces); > + neighbor_run(&n_ctx_in, &n_ctx_out); > + > + /* XXX: This should return EN_UPDATED once we actually process real SB > + * changes, i.e.: > + * > + * return EN_UPDATED; > + */ > + > + return EN_UNCHANGED; > +} > + > +struct ed_type_neighbor_table_notify { > + /* For incremental processing this could be tracked per interface in > + * the future. */ > + bool changed; > +}; > + > +static void * > +en_neighbor_table_notify_init(struct engine_node *node OVS_UNUSED, > + struct engine_arg *arg OVS_UNUSED) > +{ > + struct ed_type_neighbor_table_notify *ntn = xmalloc(sizeof *ntn); > + > + *ntn = (struct ed_type_neighbor_table_notify) { > + .changed = true, > + }; > + return ntn; > +} > + > +static void > +en_neighbor_table_notify_cleanup(void *data OVS_UNUSED) > +{ > +} > + > +static enum engine_node_state > +en_neighbor_table_notify_run(struct engine_node *node OVS_UNUSED, > + void *data) > +{ > + struct ed_type_neighbor_table_notify *ntn = data; > + enum engine_node_state state; > + if (ntn->changed) { > + state = EN_UPDATED; > + } else { > + state = EN_UNCHANGED; > + } > + ntn->changed = false; > + return state; > +} > + > +static void * > +en_neighbor_exchange_init(struct engine_node *node OVS_UNUSED, > + struct engine_arg *arg OVS_UNUSED) > +{ > + return NULL; > +} > + > +static void > +en_neighbor_exchange_cleanup(void *data OVS_UNUSED) > +{ > +} > + > +static enum engine_node_state > +en_neighbor_exchange_run(struct engine_node *node, void *data OVS_UNUSED) > +{ > + const struct ed_type_neighbor *neighbor_data = > + engine_get_input_data("neighbor", node); > + > + struct neighbor_exchange_ctx_in n_ctx_in = { > + .monitored_interfaces = &neighbor_data->monitored_interfaces, > + }; > + struct neighbor_exchange_ctx_out n_ctx_out = { > + .neighbor_table_watches = > + HMAP_INITIALIZER(&n_ctx_out.neighbor_table_watches), > + }; > + > + neighbor_exchange_run(&n_ctx_in, &n_ctx_out); > + > neighbor_table_notify_update_watches(&n_ctx_out.neighbor_table_watches); > + > + > neighbor_table_watch_request_cleanup(&n_ctx_out.neighbor_table_watches); > + hmap_destroy(&n_ctx_out.neighbor_table_watches); > + > + return EN_UPDATED; > +} > + > +struct ed_type_neighbor_exchange_status { > + bool netlink_trigger_run; > +}; > + > +static void * > +en_neighbor_exchange_status_init(struct engine_node *node OVS_UNUSED, > + struct engine_arg *arg OVS_UNUSED) > +{ > + return xzalloc(sizeof(struct ed_type_neighbor_exchange_status)); > +} > + > +static void > +en_neighbor_exchange_status_cleanup(void *data OVS_UNUSED) > +{ > +} > + > +static enum engine_node_state > +en_neighbor_exchange_status_run(struct engine_node *node OVS_UNUSED, > + void *data OVS_UNUSED) > +{ > + struct ed_type_neighbor_exchange_status *res = data; > + enum engine_node_state state; > + > + if (res->netlink_trigger_run) { > + state = EN_UPDATED; > + poll_immediate_wake(); > + } else { > + state = EN_UNCHANGED; > + } > + res->netlink_trigger_run = false; > + > + return state; > +} > + > /* Returns false if the northd internal version stored in SB_Global > * and ovn-controller internal version don't match. > */ > @@ -5966,6 +6172,11 @@ main(int argc, char *argv[]) > ENGINE_NODE(route_exchange); > ENGINE_NODE(route_exchange_status); > ENGINE_NODE(garp_rarp); > + ENGINE_NODE(host_if_monitor); > + ENGINE_NODE(neighbor); > + ENGINE_NODE(neighbor_table_notify); > + ENGINE_NODE(neighbor_exchange); > + ENGINE_NODE(neighbor_exchange_status); > > #define SB_NODE(NAME) ENGINE_NODE_SB(NAME); > SB_NODES > @@ -6191,6 +6402,12 @@ main(int argc, char *argv[]) > engine_add_input(&en_garp_rarp, &en_runtime_data, > garp_rarp_runtime_data_handler); > > + engine_add_input(&en_neighbor_exchange, &en_neighbor, NULL); > + engine_add_input(&en_neighbor_exchange, &en_host_if_monitor, NULL); > + engine_add_input(&en_neighbor_exchange, &en_neighbor_table_notify, > NULL); > + engine_add_input(&en_neighbor_exchange, &en_neighbor_exchange_status, > + NULL); > + > engine_add_input(&en_controller_output, &en_dns_cache, > NULL); > engine_add_input(&en_controller_output, &en_lflow_output, > @@ -6210,6 +6427,9 @@ main(int argc, char *argv[]) > engine_add_input(&en_controller_output, &en_acl_id, > controller_output_acl_id_handler); > > + engine_add_input(&en_controller_output, &en_neighbor_exchange, > + controller_output_neighbor_exchange_handler); > + > struct engine_arg engine_arg = { > .sb_idl = ovnsb_idl_loop.idl, > .ovs_idl = ovs_idl_loop.idl, > @@ -6557,9 +6777,23 @@ main(int argc, char *argv[]) > engine_get_internal_data(&en_route_table_notify); > rtn->changed = route_table_notify_run(); > > - struct ed_type_route_exchange_status *res = > + struct ed_type_host_if_monitor *hifm = > + engine_get_internal_data(&en_host_if_monitor); > + hifm->changed = host_if_monitor_run(); > + > + struct ed_type_neighbor_table_notify *ntn = > + > engine_get_internal_data(&en_neighbor_table_notify); > + ntn->changed = neighbor_table_notify_run(); > + > + struct ed_type_route_exchange_status *rt_res = > > engine_get_internal_data(&en_route_exchange_status); > - res->netlink_trigger_run = > !!route_exchange_status_run(); > + rt_res->netlink_trigger_run = > + !!route_exchange_status_run(); > + > + struct ed_type_neighbor_exchange_status *neigh_res = > + > engine_get_internal_data(&en_neighbor_exchange_status); > + neigh_res->netlink_trigger_run = > + !!neighbor_exchange_status_run(); > > stopwatch_start(CONTROLLER_LOOP_STOPWATCH_NAME, > time_msec()); > @@ -6871,6 +7105,8 @@ main(int argc, char *argv[]) > > binding_wait(); > route_table_notify_wait(); > + host_if_monitor_wait(); > + neighbor_table_notify_wait(); > } > > unixctl_server_run(unixctl); > -- > 2.50.1 > > _______________________________________________ > dev mailing list > d...@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
