On 8/13/25 5:17 PM, Xavier Simonart wrote: > Hi Ales, Dumitru > Hi Xavier,
> While reviewing v3, I think that spotted another issue (which was already > present in previous revisions). > Please see below > Thanks for the review! > Thanks > Xavier > > On Tue, Aug 12, 2025 at 4:57 PM Ales Musil <amu...@redhat.com> 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 >> - 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> >> Acked-by: Xavier Simonart <xsimo...@redhat.com> >> --- >> Changes in V3: >> - Rabsed on top of current main. >> - Add Xavier's ack. >> >> 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 01f6de17c..925bcba7c 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 945952c5c..21d074f73 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(); >> > This poll_immediate_wake comes too late: if an netlink error happened (e.g. > in neighbor_exchange), > the poll_immediate_wake must be called within that loop (e.g. > in SET_NEIGHBOR_EXCHANGE_NL_STATUS), > so that ovn-controller immediately wakes up, and runs another loop of I-P, > and hence immediately runs > this node (en_neighbor_exchange_status_run) which will > trigger neighbor_exchange recompute. Good catch! You're right. I should've payed more attention when mimicking the route-exchange mechanism. I'll address it for the next revision. > Same issue exists with route_exchange_status. > I posted a separate patch for route-exchange here: https://patchwork.ozlabs.org/project/ovn/patch/20250813155350.1169298-1-dce...@redhat.com/ Regards, Dumitru _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev