Hi Ales, I had a look at this, and there is one aspect I have a comment about. I've detailed it below in-line
On Thu, Jul 23, 2026 at 5:18 AM Ales Musil via dev <[email protected]> wrote: > > When a logical switch has dynamic-routing-redistribute=fdb > set, neighbor_collect_mac_to_advertise() only iterated > statically configured port MAC addresses. Dynamically > learned FDB entries from the SB FDB table were completely > ignored. > > Add neighbor_collect_fdb_to_advertise() which iterates FDB > entries for the datapath and advertises each learned MAC as > an EVPN Type-2 MAC-only neighbor. Only FDB entries learned > on VIF ports bound to this chassis or on localnet ports are > included. The per-port dynamic-routing-advertise option is > honored. > > An incremental processing handler for SB FDB changes is > added to trigger a recompute of the neighbor node only when > an FDB entry changes on a datapath that has FDB > advertisement enabled. A set of relevant datapath pointers > is maintained to filter out irrelevant FDB changes. > > Fixes: 32d5252ceb65 ("controller, northd: Add an option to advertise FDB over > EVPN.") > Reported-at: https://redhat.atlassian.net/browse/FDP-4149 > Assisted-by: Claude Opus 4.6, OpenCode > Signed-off-by: Ales Musil <[email protected]> > --- > controller/neighbor.c | 61 +++++++++++++++++++++++++++++++ > controller/neighbor.h | 8 +++++ > controller/ovn-controller.c | 40 +++++++++++++++++++++ > ovn-nb.xml | 8 +++-- > tests/ovn-inc-proc-graph-dump.at | 1 + > tests/system-ovn.at | 62 ++++++++++++++++++++++++++++++++ > 6 files changed, 178 insertions(+), 2 deletions(-) > > diff --git a/controller/neighbor.c b/controller/neighbor.c > index ada9254e0..c06183063 100644 > --- a/controller/neighbor.c > +++ b/controller/neighbor.c > @@ -46,6 +46,9 @@ neighbor_interface_monitor_alloc(enum neighbor_family > family, > static void neighbor_collect_mac_to_advertise( > const struct neighbor_ctx_in *, struct hmap *neighbors, > struct sset *advertised_pbs, const struct sbrec_datapath_binding *); > +static void neighbor_collect_fdb_to_advertise( > + const struct neighbor_ctx_in *, struct hmap *neighbors, > + struct hmapx *fdb_datapaths, struct local_datapath *); > static void neighbor_collect_advertised_mac_bindings( > const struct neighbor_ctx_in *, enum neigh_redistribute_mode mode, > struct hmap *fdb_neighbors, > @@ -176,6 +179,10 @@ neighbor_run(struct neighbor_ctx_in *n_ctx_in, > &lo->announced_neighbors, > n_ctx_out->advertised_pbs, > ld->datapath); > + neighbor_collect_fdb_to_advertise(n_ctx_in, > + &lo->announced_neighbors, > + n_ctx_out->fdb_datapaths, > + ld); > } > /* Advertise every SB Advertised_MAC_Binding row of the datapath as > an > * EVPN Type-2 MAC+IP neighbor; ovn-northd only populates rows that > are > @@ -305,6 +312,60 @@ neighbor_collect_mac_to_advertise(const struct > neighbor_ctx_in *n_ctx_in, > sbrec_port_binding_index_destroy_row(target); > } > > +/* Walks the SB FDB entries for 'dp' and advertises each learned MAC as an > + * EVPN Type-2 MAC-only neighbor (into 'neighbors'). Only FDB entries > + * learned on VIF ports bound to this chassis or on localnet ports are > + * included. The per-port 'dynamic-routing-advertise' option is honored. */ > +static void > +neighbor_collect_fdb_to_advertise(const struct neighbor_ctx_in *n_ctx_in, > + struct hmap *neighbors, > + struct hmapx *fdb_datapaths, > + struct local_datapath *ld) > +{ > + hmapx_add(fdb_datapaths, ld); > + > + const struct sbrec_datapath_binding *dp = ld->datapath; > + struct sbrec_fdb *fdb_target = > + sbrec_fdb_index_init_row(n_ctx_in->sbrec_fdb_by_dp_key); > + sbrec_fdb_index_set_dp_key(fdb_target, dp->tunnel_key); > + > + const struct sbrec_fdb *fdb; > + SBREC_FDB_FOR_EACH_EQUAL (fdb, fdb_target, > + n_ctx_in->sbrec_fdb_by_dp_key) { > + const struct sbrec_port_binding *pb = > + lport_lookup_by_key_with_dp(n_ctx_in->sbrec_pb_by_key, > + dp, fdb->port_key); > + if (!pb) { > + continue; > + } > + > + enum en_lport_type type = get_lport_type(pb); > + if (type == LP_VIF && > + !lport_pb_is_chassis_resident(n_ctx_in->chassis, pb)) { > + continue; > + } > + > + if (type != LP_VIF && type != LP_LOCALNET) { > + continue; > + } > + > + if (!smap_get_bool(&pb->options, "dynamic-routing-advertise", true)) > { > + continue; > + } > + > + struct eth_addr ea; > + if (!eth_addr_from_string(fdb->mac, &ea)) { > + continue; > + } > + > + if (!advertise_neigh_find(neighbors, ea, &in6addr_any)) { > + advertise_neigh_add(neighbors, ea, in6addr_any); > + } > + } > + > + sbrec_fdb_index_destroy_row(fdb_target); > +} > + > /* Walks the SB Advertised_MAC_Binding rows of 'dp' and advertises each as an > * EVPN Type-2 MAC+IP neighbor (into 'neighbors_v4'/'neighbors_v6'). > * ovn-northd only populates a row when the datapath opted into advertising > it > diff --git a/controller/neighbor.h b/controller/neighbor.h > index e3adc87d1..cd158a0f1 100644 > --- a/controller/neighbor.h > +++ b/controller/neighbor.h > @@ -21,6 +21,7 @@ > #include <net/if.h> > #include <stdint.h> > > +#include "hmapx.h" > #include "lib/sset.h" > #include "openvswitch/hmap.h" > > @@ -48,6 +49,10 @@ struct neighbor_ctx_in { > struct ovsdb_idl_index *sbrec_amb_by_dp; > /* Index for Port Binding by name. */ > struct ovsdb_idl_index *sbrec_pb_by_name; > + /* Index for Port Binding by datapath and tunnel key. */ > + struct ovsdb_idl_index *sbrec_pb_by_key; > + /* Index for FDB by dp_key. */ > + struct ovsdb_idl_index *sbrec_fdb_by_dp_key; > const struct sbrec_chassis *chassis; > }; > > @@ -56,6 +61,9 @@ struct neighbor_ctx_out { > struct vector *monitored_interfaces; > /* Contains set of port binding names that are currently advertised. */ > struct sset *advertised_pbs; > + /* Contains 'struct local_datapath' pointers for datapaths with FDB > + * advertisement enabled. */ > + struct hmapx *fdb_datapaths; > }; > > enum neighbor_interface_type { > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > index 5f63c5fec..f1420e727 100644 > --- a/controller/ovn-controller.c > +++ b/controller/ovn-controller.c > @@ -6081,6 +6081,9 @@ struct ed_type_neighbor { > struct vector monitored_interfaces; > /* Contains set of PB names that are currently advertised. */ > struct sset advertised_pbs; > + /* Contains 'struct local_datapath' pointers for datapaths with FDB > + * advertisement enabled. */ > + struct hmapx fdb_datapaths; > }; > > static void * > @@ -6093,6 +6096,7 @@ en_neighbor_init(struct engine_node *node OVS_UNUSED, > .monitored_interfaces = > VECTOR_EMPTY_INITIALIZER(struct neighbor_interface_monitor *), > .advertised_pbs = SSET_INITIALIZER(&data->advertised_pbs), > + .fdb_datapaths = HMAPX_INITIALIZER(&data->fdb_datapaths), > }; > return data; > } > @@ -6105,6 +6109,7 @@ en_neighbor_cleanup(void *data) > neighbor_cleanup(&ne_data->monitored_interfaces); > vector_destroy(&ne_data->monitored_interfaces); > sset_destroy(&ne_data->advertised_pbs); > + hmapx_destroy(&ne_data->fdb_datapaths); > } > > static enum engine_node_state > @@ -6131,6 +6136,14 @@ en_neighbor_run(struct engine_node *node OVS_UNUSED, > void *data) > engine_ovsdb_node_get_index( > engine_get_input("SB_advertised_mac_binding", node), > "datapath"); > + struct ovsdb_idl_index *sbrec_port_binding_by_key = > + engine_ovsdb_node_get_index( > + engine_get_input("SB_port_binding", node), > + "key"); > + struct ovsdb_idl_index *sbrec_fdb_by_dp_key = > + engine_ovsdb_node_get_index( > + engine_get_input("SB_fdb", node), > + "dp_key"); > > const char *chassis_id = get_ovs_chassis_id(ovs_table); > ovs_assert(chassis_id); > @@ -6143,16 +6156,20 @@ en_neighbor_run(struct engine_node *node OVS_UNUSED, > void *data) > .sbrec_pb_by_dp = sbrec_port_binding_by_datapath, > .sbrec_amb_by_dp = sbrec_advertised_mac_binding_by_datapath, > .sbrec_pb_by_name = sbrec_port_binding_by_name, > + .sbrec_pb_by_key = sbrec_port_binding_by_key, > + .sbrec_fdb_by_dp_key = sbrec_fdb_by_dp_key, > .chassis = chassis, > }; > > struct neighbor_ctx_out n_ctx_out = { > .monitored_interfaces = &ne_data->monitored_interfaces, > .advertised_pbs = &ne_data->advertised_pbs, > + .fdb_datapaths = &ne_data->fdb_datapaths, > }; > > neighbor_cleanup(&ne_data->monitored_interfaces); > sset_clear(&ne_data->advertised_pbs); > + hmapx_clear(&ne_data->fdb_datapaths); > neighbor_run(&n_ctx_in, &n_ctx_out); > > return EN_UPDATED; > @@ -6283,6 +6300,27 @@ neighbor_sb_port_binding_handler(struct engine_node > *node, void *data) > return EN_HANDLED_UNCHANGED; > } > > +static enum engine_input_handler_result > +neighbor_sb_fdb_handler(struct engine_node *node, void *data) > +{ > + struct ed_type_neighbor *ne_data = data; > + struct ed_type_runtime_data *rt_data = > + engine_get_input_data("runtime_data", node); > + const struct sbrec_fdb_table *fdb_table = > + EN_OVSDB_GET(engine_get_input("SB_fdb", node)); > + > + const struct sbrec_fdb *fdb; > + SBREC_FDB_TABLE_FOR_EACH_TRACKED (fdb, fdb_table) { > + struct local_datapath *ld = > + get_local_datapath(&rt_data->local_datapaths, fdb->dp_key); > + if (ld && hmapx_contains(&ne_data->fdb_datapaths, ld)) { Initially, I thought there might be a problem if en_runtime_data added a new local_datapath to rt_data->local_datapaths. The new local_datapath will definitely not be in ne_data->fdb_datapaths, meaning that we might skip advertising dynamic FDB data on the new local_datapath. Having taken a close look, I think there won't be any issues with how this is written, for now at least. The reason is that neighbor_runtime_data_handler() always returns EN_UNHANDLED if any local_datapaths are added or removed from rt_data->local_datapaths. This means that neighbor_sb_fdb_handler() will only ever be called if the set of local_datapaths has not changed since the last engine run. I think that rather than trying to change the code here, you could add a comment that makes it clear that neighbor_sb_fdb_handler() assumes that it will not be called if there are new or removed local_datapaths. It may also be worth updating the test in this patch to handle the case of a new datapath being added during the same transaction when FDB is updated. The test should pass with the current code, but that scenario can act as a canary if someone updates neighbor_handle_runtime_data() without also updating neighbor_handle_sb_fdb(). What do you think? > + return EN_UNHANDLED; > + } > + } > + > + return EN_HANDLED_UNCHANGED; > +} > + > static int > if_index_cmp(const void *a_, const void *b_) > { > @@ -7212,6 +7250,8 @@ inc_proc_ovn_controller_init( > neighbor_sb_datapath_binding_handler); > engine_add_input(&en_neighbor, &en_sb_port_binding, > neighbor_sb_port_binding_handler); > + engine_add_input(&en_neighbor, &en_sb_fdb, > + neighbor_sb_fdb_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); > diff --git a/ovn-nb.xml b/ovn-nb.xml > index 1901bbf26..9ad3b35fc 100644 > --- a/ovn-nb.xml > +++ b/ovn-nb.xml > @@ -1041,9 +1041,13 @@ > > <p> > If <code>fdb</code> is specified then ovn-controller will advertise > - all workloads that are local to the chassis. The applies to VIFs, > + all workloads that are local to the chassis. This applies to VIFs, > container ports, virtual ports, connected DGPs and connected GW > - routers. > + routers. Additionally, dynamically learned FDB entries (from ports > + with <code>unknown</code> addresses) are also advertised. For VIF > + ports, only entries learned on the local chassis are advertised; > + for localnet ports, entries are advertised on all chassis where the > + datapath is local. > </p> > > <p> > diff --git a/tests/ovn-inc-proc-graph-dump.at > b/tests/ovn-inc-proc-graph-dump.at > index 9a1483ee0..52bccf06d 100644 > --- a/tests/ovn-inc-proc-graph-dump.at > +++ b/tests/ovn-inc-proc-graph-dump.at > @@ -395,6 +395,7 @@ digraph "Incremental-Processing-Engine" { > runtime_data -> neighbor [[label="neighbor_runtime_data_handler"]]; > SB_datapath_binding -> neighbor > [[label="neighbor_sb_datapath_binding_handler"]]; > SB_port_binding -> neighbor > [[label="neighbor_sb_port_binding_handler"]]; > + SB_fdb -> neighbor [[label="neighbor_sb_fdb_handler"]]; > host_if_monitor [[style=filled, shape=box, fillcolor=white, > label="host_if_monitor"]]; > neighbor_table_notify [[style=filled, shape=box, fillcolor=white, > label="neighbor_table_notify"]]; > neighbor_exchange_status [[style=filled, shape=box, fillcolor=white, > label="neighbor_exchange_status"]]; > diff --git a/tests/system-ovn.at b/tests/system-ovn.at > index 17b4dcb3d..3cfb78585 100644 > --- a/tests/system-ovn.at > +++ b/tests/system-ovn.at > @@ -18388,6 +18388,68 @@ f0:00:0f:16:01:20 dev $LO_NAME master $BR_NAME static > f0:00:0f:16:01:20 dev $LO_NAME vlan 1 master $BR_NAME static > ]) > > +AS_BOX([L2 EVPN learned FDB advertising]) > + > +# Add a port with "unknown" addresses and FDB learning enabled. > +# redistribute=fdb is already set from the previous section. > +check ovn-nbctl --wait=hv \ > + -- lsp-add ls-evpn wl-unknown \ > + -- lsp-set-addresses wl-unknown "unknown" > + > +ADD_NAMESPACES(wl-unknown) > +ADD_VETH(wl-unknown, wl-unknown, br-int, "172.16.1.30/24", > "f0:00:0f:16:01:30") > +check ovn-nbctl --wait=hv sync > +wait_for_ports_up wl-unknown > + > +# Trigger FDB learning by sending traffic from the namespace. > +NS_CHECK_EXEC([wl-unknown], [ping -c 1 -W 1 172.16.1.10 > /dev/null 2>&1 || > true]) > + > +# Wait for the FDB entry to appear in the SB database. > +learned_mac=f0:00:0f:16:01:30 > +wait_row_count fdb 1 mac='"'$learned_mac'"' > + > +# The dynamically learned FDB MAC should be advertised on the loopback. > +OVS_WAIT_FOR_OUTPUT_UNQUOTED([bridge fdb show | grep $LO_NAME | grep > "$learned_mac" | sort], [0], [dnl > +$learned_mac dev $LO_NAME master $BR_NAME static > +$learned_mac dev $LO_NAME vlan 1 master $BR_NAME static > +]) > + > +# Disabling dynamic-routing-advertise on the port should remove it. > +check ovn-nbctl --wait=hv set Logical_Switch_Port wl-unknown \ > + options:dynamic-routing-advertise=false > +OVS_WAIT_FOR_OUTPUT_UNQUOTED([bridge fdb show | grep $LO_NAME | grep > "$learned_mac" | sort], [0], [dnl > +]) > + > +# Re-enabling should bring it back. > +check ovn-nbctl --wait=hv set Logical_Switch_Port wl-unknown \ > + options:dynamic-routing-advertise=true > +OVS_WAIT_FOR_OUTPUT_UNQUOTED([bridge fdb show | grep $LO_NAME | grep > "$learned_mac" | sort], [0], [dnl > +$learned_mac dev $LO_NAME master $BR_NAME static > +$learned_mac dev $LO_NAME vlan 1 master $BR_NAME static > +]) > + > +# Also verify that FDB entries learned on localnet ports are advertised. > +# The localnet port already has addresses="unknown" from > lsp-add-localnet-port. > +check ovn-nbctl --wait=hv set Logical_Switch_Port ln_port \ > + options:localnet_learn_fdb=true > + > +# Send traffic from the physical side to trigger localnet FDB learning. > +ADD_NAMESPACES(ext-host) > +ADD_VETH(ext-host, ext-host, br-ext, "172.16.1.40/24", "aa:bb:cc:dd:ee:ff") > +NS_CHECK_EXEC([ext-host], [ping -c 1 -W 1 172.16.1.10 > /dev/null 2>&1 || > true]) > + > +ln_learned_mac=aa:bb:cc:dd:ee:ff > +wait_row_count fdb 1 mac='"'$ln_learned_mac'"' > +OVS_WAIT_FOR_OUTPUT_UNQUOTED([bridge fdb show | grep $LO_NAME | grep > "$ln_learned_mac" | sort], [0], [dnl > +$ln_learned_mac dev $LO_NAME master $BR_NAME static > +$ln_learned_mac dev $LO_NAME vlan 1 master $BR_NAME static > +]) > + > +# Cleanup. > +check ovn-sbctl --all destroy FDB > +check ovn-nbctl --wait=hv lsp-del wl-unknown \ > + -- remove Logical_Switch_Port ln_port options localnet_learn_fdb > + > check ovn-nbctl --wait=hv lsp-del workload2 > OVS_WAIT_FOR_OUTPUT_UNQUOTED([bridge fdb show | grep $LO_NAME | grep > "f0:00:0f:16:01" | sort], [0], [dnl > f0:00:0f:16:01:10 dev $LO_NAME master $BR_NAME static > -- > 2.55.0 > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
