On Wed, Jul 29, 2026 at 8:44 AM Ales Musil <[email protected]> wrote:
> > > On Tue, Jul 28, 2026 at 7:13 PM Mark Michelson <[email protected]> > wrote: > >> 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 >> > > Hi Mark, > > thank you for the review. > > >> >> 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? >> > > Yeah that makes sense. I'll add a comment and try to adjust > the test. > After giving it more thought I'm not sure how to trigger additon of datapath at the same time as an fdb update. We probably don't have race free way of doing that. In the meantime, I have posted v2. If I'll figure something out I can adjust it or add it as follow up. > > >> >> > + 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 >> > >> >> > Regards, > Ales > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
