On 10/24/25 3:16 PM, Lorenzo Bianconi wrote: > On Oct 24, Dumitru Ceara wrote: >> On 10/23/25 12:16 PM, Lorenzo Bianconi wrote: >>> Introduce the capability to ovn-controller to announce local ips/macs >>> bindings via EVPN route type2 messages in the EVPN domain. >>> >>> Reported-at: https://issues.redhat.com/browse/FDP-2019 >>> Signed-off-by: Lorenzo Bianconi <[email protected]> >>> --- >> >> Hi Lorenzo, >> >> Thanks for v3! > > Hi Dumitru, > > thx for the review. > >> >>> controller/neighbor.c | 68 ++++++++++++++++++++++++++++++++++--- >>> controller/neighbor.h | 2 ++ >>> controller/ovn-controller.c | 23 ++++++++++++- >>> tests/system-ovn.at | 35 +++++++++++++++++++ >>> 4 files changed, 122 insertions(+), 6 deletions(-) >>> >>> diff --git a/controller/neighbor.c b/controller/neighbor.c >>> index 1870159bc..6104af894 100644 >>> --- a/controller/neighbor.c >>> +++ b/controller/neighbor.c >>> @@ -20,6 +20,7 @@ >>> #include "lib/sset.h" >>> #include "local_data.h" >>> #include "lport.h" >>> +#include "openvswitch/ofp-parse.h" >>> #include "ovn-sb-idl.h" >>> >>> #include "neighbor.h" >>> @@ -42,6 +43,10 @@ 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_ip_mac_to_advertise( >>> + const struct neighbor_ctx_in *, >>> + struct hmap *neighbors_v4, struct hmap *neighbors_v6, >>> + struct sset *advertised_pbs, const struct sbrec_datapath_binding *); >>> static const struct sbrec_port_binding *neighbor_get_relevant_port_binding( >>> struct ovsdb_idl_index *sbrec_pb_by_name, >>> const struct sbrec_port_binding *); >>> @@ -114,13 +119,20 @@ neighbor_run(struct neighbor_ctx_in *n_ctx_in, >>> >>> const char *redistribute = smap_get(&ld->datapath->external_ids, >>> >>> "dynamic-routing-redistribute"); >>> - if (!redistribute || strcmp(redistribute, "fdb")) { >>> - continue; >>> + if (redistribute && !strcmp(redistribute, "fdb")) { >>> + neighbor_collect_mac_to_advertise(n_ctx_in, >>> + &lo->announced_neighbors, >>> + n_ctx_out->advertised_pbs, >>> + ld->datapath); >>> + } >>> + if (redistribute && !strcmp(redistribute, "ip")) { >>> + neighbor_collect_ip_mac_to_advertise(n_ctx_in, >>> + >>> &br_v4->announced_neighbors, >>> + >>> &br_v6->announced_neighbors, >>> + n_ctx_out->advertised_pbs, >>> + ld->datapath); >>> } >>> >>> - neighbor_collect_mac_to_advertise(n_ctx_in, >>> &lo->announced_neighbors, >>> - n_ctx_out->advertised_pbs, >>> - ld->datapath); >>> } >>> } >>> >>> @@ -236,6 +248,52 @@ neighbor_collect_mac_to_advertise(const struct >>> neighbor_ctx_in *n_ctx_in, >>> sbrec_port_binding_index_destroy_row(target); >>> } >>> >>> +static void >>> +neighbor_collect_ip_mac_to_advertise( >>> + const struct neighbor_ctx_in *n_ctx_in, >>> + struct hmap *neighbors_v4, >>> + struct hmap *neighbors_v6, >>> + struct sset *advertised_pbs, >>> + const struct sbrec_datapath_binding *dp) >>> +{ >>> + struct sbrec_advertised_mac_binding *target = >>> + sbrec_advertised_mac_binding_index_init_row( >>> + n_ctx_in->sbrec_amb_by_dp); >>> + sbrec_advertised_mac_binding_index_set_datapath(target, dp); >>> + >>> + const struct sbrec_advertised_mac_binding *adv_mb; >>> + SBREC_ADVERTISED_MAC_BINDING_FOR_EACH_EQUAL (adv_mb, target, >>> + >>> n_ctx_in->sbrec_amb_by_dp) { >>> + enum en_lport_type type = get_lport_type(adv_mb->logical_port); >>> + if (type != LP_PATCH && >>> + !lport_pb_is_chassis_resident(n_ctx_in->chassis, >>> + adv_mb->logical_port)) { >>> + continue; >>> + } >> >> I spent some time thinking some more about this and also chatted with >> Ales about it and it probably doesn't make sense to advertise IPs >> configured on "purely distributed" router ports. >> >> The code above is slightly incorrect because a LSP of type LP_PATCH >> may be connected to a pure LRP of type LP_PATCH OR to a LRP chassis >> redirect port. >> >> But given that advertising these "distributed IPs" would just confuse >> the fabric it probably doesn't make sense to do that at all. Also >> the final BGP routes advertised by FRR (or whatever control protocol >> is running) will set as next-hop an IP that's local to the chassis >> (maybe also configured on a Gateway Router) so there's no need to >> advertise these distributed ones. >> >> A reason for thinking that this is the correct approach is also >> the fact that the multinode test passes regardless of whether we >> advertise the distributed router port IP or not. >> >> I think we should change this and do the same thing we do for FDB >> records, i.e.: >> >> const struct sbrec_port_binding *pb = >> neighbor_get_relevant_port_binding(n_ctx_in->sbrec_pb_by_name, >> adv_mb->logical_port); >> if (!lport_pb_is_chassis_resident(n_ctx_in->chassis, pb)) { >> continue; >> } >> >>> + >>> + struct in6_addr ip; >>> + if (!ip46_parse(adv_mb->ip, &ip)) { >>> + continue; >>> + } >>> + >>> + struct eth_addr ea; >>> + char *err = str_to_mac(adv_mb->mac, &ea); >>> + if (err) { >>> + free(err); >>> + continue; >>> + } >>> + >>> + struct hmap *neighbors = IN6_IS_ADDR_V4MAPPED(&ip) >>> + ? neighbors_v4 : neighbors_v6; >>> + if (!advertise_neigh_find(neighbors, ea, &ip)) { >>> + advertise_neigh_add(neighbors, ea, ip); >>> + } >>> + sset_add(advertised_pbs, adv_mb->logical_port->logical_port); >> >> This would have to change to "pb->logical_port". >> >> Also we're missing code in the neighbor_runtime_data_handler() to >> handle changes to advertised_pbs for the case when IP redistribution >> is configured for the switch but FDB redistribution isn't. >> >>> + } >>> + >>> + sbrec_advertised_mac_binding_index_destroy_row(target); >>> +} >>> + >>> static const struct sbrec_port_binding * >>> neighbor_get_relevant_port_binding(struct ovsdb_idl_index >>> *sbrec_pb_by_name, >>> const struct sbrec_port_binding *pb) >>> diff --git a/controller/neighbor.h b/controller/neighbor.h >>> index ce73dd4c4..e3adc87d1 100644 >>> --- a/controller/neighbor.h >>> +++ b/controller/neighbor.h >>> @@ -44,6 +44,8 @@ struct neighbor_ctx_in { >>> const struct hmap *local_datapaths; >>> /* Index for Port Binding by Datapath. */ >>> struct ovsdb_idl_index *sbrec_pb_by_dp; >>> + /* Index for Advertised_Mac_Binding by Datapath. */ >>> + struct ovsdb_idl_index *sbrec_amb_by_dp; >>> /* Index for Port Binding by name. */ >>> struct ovsdb_idl_index *sbrec_pb_by_name; >>> const struct sbrec_chassis *chassis; >>> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c >>> index 3cc7ab340..45005b5c0 100644 >>> --- a/controller/ovn-controller.c >>> +++ b/controller/ovn-controller.c >>> @@ -267,6 +267,7 @@ update_sb_monitors(struct ovsdb_idl *ovnsb_idl, >>> struct ovsdb_idl_condition nh = OVSDB_IDL_CONDITION_INIT(&nh); >>> struct ovsdb_idl_condition ar = OVSDB_IDL_CONDITION_INIT(&ar); >>> struct ovsdb_idl_condition lr = OVSDB_IDL_CONDITION_INIT(&lr); >>> + struct ovsdb_idl_condition amb = OVSDB_IDL_CONDITION_INIT(&amb); >>> >>> /* Always monitor all logical datapath groups. Otherwise, DPG updates >>> may >>> * be received *after* the lflows using it are seen by ovn-controller. >>> @@ -301,6 +302,7 @@ update_sb_monitors(struct ovsdb_idl *ovnsb_idl, >>> ovsdb_idl_condition_add_clause_true(&tv); >>> ovsdb_idl_condition_add_clause_true(&nh); >>> ovsdb_idl_condition_add_clause_true(&ar); >>> + ovsdb_idl_condition_add_clause_true(&amb); >>> goto out; >>> } >>> >>> @@ -400,6 +402,8 @@ update_sb_monitors(struct ovsdb_idl *ovnsb_idl, >>> uuid); >>> sbrec_ecmp_nexthop_add_clause_datapath(&nh, OVSDB_F_EQ, uuid); >>> sbrec_advertised_route_add_clause_datapath(&ar, OVSDB_F_EQ, >>> uuid); >>> + sbrec_advertised_mac_binding_add_clause_datapath(&amb, >>> OVSDB_F_EQ, >>> + uuid); >>> } >>> >>> /* Datapath groups are immutable, which means a new group record is >>> @@ -430,6 +434,8 @@ out:; >>> sb_table_set_opt_mon_condition(ovnsb_idl, ecmp_nexthop, &nh), >>> sb_table_set_opt_mon_condition(ovnsb_idl, advertised_route, &ar), >>> sb_table_set_opt_mon_condition(ovnsb_idl, learned_route, &lr), >>> + sb_table_set_opt_mon_condition(ovnsb_idl, advertised_mac_binding, >>> + &amb), >>> }; >>> >>> unsigned int expected_cond_seqno = 0; >>> @@ -452,6 +458,7 @@ out:; >>> ovsdb_idl_condition_destroy(&nh); >>> ovsdb_idl_condition_destroy(&ar); >>> ovsdb_idl_condition_destroy(&lr); >>> + ovsdb_idl_condition_destroy(&amb); >>> return expected_cond_seqno; >>> } >>> >>> @@ -1014,7 +1021,8 @@ ctrl_register_ovs_idl(struct ovsdb_idl *ovs_idl) >>> SB_NODE(chassis_template_var) \ >>> SB_NODE(acl_id) \ >>> SB_NODE(advertised_route) \ >>> - SB_NODE(learned_route) >>> + SB_NODE(learned_route) \ >>> + SB_NODE(advertised_mac_binding) >>> >>> enum sb_engine_node { >>> #define SB_NODE(NAME) SB_##NAME, >>> @@ -5872,6 +5880,10 @@ en_neighbor_run(struct engine_node *node OVS_UNUSED, >>> void *data) >>> engine_ovsdb_node_get_index( >>> engine_get_input("SB_chassis", node), >>> "name"); >>> + struct ovsdb_idl_index *sbrec_advertised_mac_binding_by_datapath = >>> + engine_ovsdb_node_get_index( >>> + engine_get_input("SB_advertised_mac_binding", node), >>> + "datapath"); >>> >>> const char *chassis_id = get_ovs_chassis_id(ovs_table); >>> ovs_assert(chassis_id); >>> @@ -5882,6 +5894,7 @@ en_neighbor_run(struct engine_node *node OVS_UNUSED, >>> void *data) >>> struct neighbor_ctx_in n_ctx_in = { >>> .local_datapaths = &rt_data->local_datapaths, >>> .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, >>> .chassis = chassis, >>> }; >>> @@ -6684,6 +6697,9 @@ main(int argc, char *argv[]) >>> struct ovsdb_idl_index *sbrec_learned_route_index_by_datapath >>> = ovsdb_idl_index_create1(ovnsb_idl_loop.idl, >>> &sbrec_learned_route_col_datapath); >>> + struct ovsdb_idl_index *sbrec_advertised_mac_binding_index_by_dp >>> + = ovsdb_idl_index_create1(ovnsb_idl_loop.idl, >>> + >>> &sbrec_advertised_mac_binding_col_datapath); >>> >>> ovsdb_idl_track_add_all(ovnsb_idl_loop.idl); >>> ovsdb_idl_omit_alert(ovnsb_idl_loop.idl, >>> @@ -6718,6 +6734,8 @@ main(int argc, char *argv[]) >>> &sbrec_advertised_route_col_external_ids); >>> ovsdb_idl_omit(ovnsb_idl_loop.idl, >>> &sbrec_learned_route_col_external_ids); >>> + ovsdb_idl_omit(ovnsb_idl_loop.idl, >>> + &sbrec_advertised_mac_binding_col_external_ids); >>> >>> /* We don't want to monitor Connection table at all. So omit all the >>> * columns. */ >>> @@ -7019,6 +7037,7 @@ main(int argc, char *argv[]) >>> >>> engine_add_input(&en_neighbor, &en_ovs_open_vswitch, NULL); >>> engine_add_input(&en_neighbor, &en_sb_chassis, NULL); >>> + engine_add_input(&en_neighbor, &en_sb_advertised_mac_binding, NULL); >>> engine_add_input(&en_neighbor, &en_runtime_data, >>> neighbor_runtime_data_handler); >>> engine_add_input(&en_neighbor, &en_sb_datapath_binding, >>> @@ -7108,6 +7127,8 @@ main(int argc, char *argv[]) >>> >>> sbrec_chassis_template_var_index_by_chassis); >>> engine_ovsdb_node_add_index(&en_sb_learned_route, "datapath", >>> sbrec_learned_route_index_by_datapath); >>> + engine_ovsdb_node_add_index(&en_sb_advertised_mac_binding, "datapath", >>> + sbrec_advertised_mac_binding_index_by_dp); >>> engine_ovsdb_node_add_index(&en_sb_mac_binding, "lport_ip", >>> sbrec_mac_binding_by_lport_ip); >>> engine_ovsdb_node_add_index(&en_ovs_flow_sample_collector_set, "id", >>> diff --git a/tests/system-ovn.at b/tests/system-ovn.at >>> index 973d2404e..4f51e309a 100644 >>> --- a/tests/system-ovn.at >>> +++ b/tests/system-ovn.at >>> @@ -18759,6 +18759,41 @@ AT_CHECK_UNQUOTED([ovs-ofctl dump-flows br-int >>> table=OFTABLE_MAC_LOOKUP | grep p >>> awk '{print $7, $8}' | sort], [0], [dnl >>> ]) >>> >>> +AS_BOX([L2 EVPN ARP advertising]) >>> +# Add a router connected to the EVPN logical switch. >>> +check ovn-nbctl --wait=hv \ >>> + -- lr-add lr \ >>> + -- set Logical_Router lr options:chassis=hv1 \ >>> + -- lrp-add lr lr-ls-evpn f0:00:0f:16:01:01 172.16.1.1/24 >>> + >>> +ls_evpn_uuid=$(fetch_column Datapath_Binding _uuid >>> external_ids:name=ls-evpn) >>> +check ovn-nbctl --wait=hv set logical_switch ls-evpn >>> other_config:dynamic-routing-redistribute=ip >>> +check_row_count Advertised_MAC_Binding 1 ip=172.16.1.10 >>> mac='f0\:00\:0f\:16\:01\:10' datapath=$ls_evpn_uuid >>> +check_row_count Advertised_MAC_Binding 1 ip=172.16.1.1 >>> mac='f0\:00\:0f\:16\:01\:01' datapath=$ls_evpn_uuid >>> + >>> +AT_CHECK_UNQUOTED([ip neigh show dev br-10 nud noarp | grep -q >>> '172.16.1.10 lladdr f0:00:0f:16:01:10 NOARP']) >>> +AT_CHECK_UNQUOTED([ip neigh show dev br-10 nud noarp | grep -q '172.16.1.1 >>> lladdr f0:00:0f:16:01:01 NOARP']) >>> + >>> +check ovn-nbctl --wait=hv lsp-add ls-evpn workload2 \ >>> + -- lsp-set-addresses workload2 "f0:00:0f:16:01:20 172.16.1.20" >>> + >>> +check_row_count Advertised_MAC_Binding 1 ip=172.16.1.20 >>> mac='f0\:00\:0f\:16\:01\:20' datapath=$ls_evpn_uuid >>> +AT_CHECK_UNQUOTED([ip neigh show dev br-10 nud noarp | grep -q >>> '172.16.1.20 lladdr f0:00:0f:16:01:20 NOARP']) >>> + >>> +check ovn-nbctl --wait=hv lsp-del workload2 >>> +AT_CHECK_UNQUOTED([ip neigh show dev br-10 nud noarp | grep -q >>> '172.16.1.20 lladdr f0:00:0f:16:01:20 NOARP'], [1]) >>> + >>> +check ovn-nbctl --wait=hv lsp-del workload1 >>> +check_row_count Advertised_MAC_Binding 1 >>> +AT_CHECK_UNQUOTED([ip neigh show dev br-10 nud noarp | grep -q >>> '172.16.1.10 lladdr f0:00:0f:16:01:10 NOARP'], [1]) >>> + >>> +check ovn-nbctl --wait=hv lrp-del lr-ls-evpn >>> +check_row_count Advertised_MAC_Binding 0 >>> +AT_CHECK_UNQUOTED([ip neigh show dev br-10 nud noarp | grep -q '172.16.1.1 >>> lladdr f0:00:0f:16:01:01 NOARP'], [1]) >> >> All AT_CHECK_UNQUOTED above can be AT_CHECK. >> >>> + >>> +check ovn-nbctl --wait=hv ls-del ls-evpn >>> +check ovn-nbctl --wait=hv lr-del lr >>> + >>> OVN_CLEANUP_CONTROLLER([hv1]) >>> >>> as ovn-sb >> >> I'm thinking of squashing in the following incremental change before >> applying the patch to the main branch, please let me know if that's OK >> for you: >> >> diff --git a/controller/neighbor.c b/controller/neighbor.c >> index 6104af894e..0bad7e0191 100644 >> --- a/controller/neighbor.c >> +++ b/controller/neighbor.c >> @@ -264,10 +264,10 @@ neighbor_collect_ip_mac_to_advertise( >> const struct sbrec_advertised_mac_binding *adv_mb; >> SBREC_ADVERTISED_MAC_BINDING_FOR_EACH_EQUAL (adv_mb, target, >> n_ctx_in->sbrec_amb_by_dp) >> { >> - enum en_lport_type type = get_lport_type(adv_mb->logical_port); >> - if (type != LP_PATCH && >> - !lport_pb_is_chassis_resident(n_ctx_in->chassis, >> - adv_mb->logical_port)) { >> + const struct sbrec_port_binding *pb = >> + neighbor_get_relevant_port_binding(n_ctx_in->sbrec_pb_by_name, >> + adv_mb->logical_port); >> + if (!lport_pb_is_chassis_resident(n_ctx_in->chassis, pb)) { >> continue; >> } >> >> @@ -288,7 +288,7 @@ neighbor_collect_ip_mac_to_advertise( >> if (!advertise_neigh_find(neighbors, ea, &ip)) { >> advertise_neigh_add(neighbors, ea, ip); >> } >> - sset_add(advertised_pbs, adv_mb->logical_port->logical_port); >> + sset_add(advertised_pbs, pb->logical_port); >> } >> >> sbrec_advertised_mac_binding_index_destroy_row(target); >> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c >> index 45005b5c00..ea65d3a3e8 100644 >> --- a/controller/ovn-controller.c >> +++ b/controller/ovn-controller.c >> @@ -5960,7 +5960,10 @@ neighbor_runtime_data_handler(struct engine_node >> *node, void *data) >> >> const char *redistribute = smap_get(&ld->datapath->external_ids, >> "dynamic-routing-redistribute"); >> - if (!redistribute || strcmp(redistribute, "fdb")) { >> + if (!redistribute) { >> + continue; >> + } >> + if (strcmp(redistribute, "fdb") && strcmp(redistribute, "ip")) { >> continue; >> } >> >> diff --git a/tests/system-ovn.at b/tests/system-ovn.at >> index 3832ba0b4a..2b880ec378 100644 >> --- a/tests/system-ovn.at >> +++ b/tests/system-ovn.at >> @@ -18448,25 +18448,25 @@ check ovn-nbctl --wait=hv set logical_switch >> ls-evpn other_config:dynamic-routin >> check_row_count Advertised_MAC_Binding 1 ip=172.16.1.10 >> mac='f0\:00\:0f\:16\:01\:10' datapath=$ls_evpn_uuid >> check_row_count Advertised_MAC_Binding 1 ip=172.16.1.1 >> mac='f0\:00\:0f\:16\:01\:01' datapath=$ls_evpn_uuid >> >> -AT_CHECK_UNQUOTED([ip neigh show dev br-10 nud noarp | grep -q '172.16.1.10 >> lladdr f0:00:0f:16:01:10 NOARP']) >> -AT_CHECK_UNQUOTED([ip neigh show dev br-10 nud noarp | grep -q '172.16.1.1 >> lladdr f0:00:0f:16:01:01 NOARP']) >> +AT_CHECK([ip neigh show dev br-10 nud noarp | grep -q '172.16.1.10 lladdr >> f0:00:0f:16:01:10 NOARP']) >> +AT_CHECK([ip neigh show dev br-10 nud noarp | grep -q '172.16.1.1 lladdr >> f0:00:0f:16:01:01 NOARP']) >> >> check ovn-nbctl --wait=hv lsp-add ls-evpn workload2 \ >> -- lsp-set-addresses workload2 "f0:00:0f:16:01:20 172.16.1.20" >> >> check_row_count Advertised_MAC_Binding 1 ip=172.16.1.20 >> mac='f0\:00\:0f\:16\:01\:20' datapath=$ls_evpn_uuid >> -AT_CHECK_UNQUOTED([ip neigh show dev br-10 nud noarp | grep -q '172.16.1.20 >> lladdr f0:00:0f:16:01:20 NOARP']) >> +AT_CHECK([ip neigh show dev br-10 nud noarp | grep -q '172.16.1.20 lladdr >> f0:00:0f:16:01:20 NOARP']) >> >> check ovn-nbctl --wait=hv lsp-del workload2 >> -AT_CHECK_UNQUOTED([ip neigh show dev br-10 nud noarp | grep -q '172.16.1.20 >> lladdr f0:00:0f:16:01:20 NOARP'], [1]) >> +AT_CHECK([ip neigh show dev br-10 nud noarp | grep -q '172.16.1.20 lladdr >> f0:00:0f:16:01:20 NOARP'], [1]) >> >> check ovn-nbctl --wait=hv lsp-del workload1 >> check_row_count Advertised_MAC_Binding 1 >> -AT_CHECK_UNQUOTED([ip neigh show dev br-10 nud noarp | grep -q '172.16.1.10 >> lladdr f0:00:0f:16:01:10 NOARP'], [1]) >> +AT_CHECK([ip neigh show dev br-10 nud noarp | grep -q '172.16.1.10 lladdr >> f0:00:0f:16:01:10 NOARP'], [1]) >> >> check ovn-nbctl --wait=hv lrp-del lr-ls-evpn >> check_row_count Advertised_MAC_Binding 0 >> -AT_CHECK_UNQUOTED([ip neigh show dev br-10 nud noarp | grep -q '172.16.1.1 >> lladdr f0:00:0f:16:01:01 NOARP'], [1]) >> +AT_CHECK([ip neigh show dev br-10 nud noarp | grep -q '172.16.1.1 lladdr >> f0:00:0f:16:01:01 NOARP'], [1]) >> >> check ovn-nbctl --wait=hv ls-del ls-evpn >> check ovn-nbctl --wait=hv lr-del lr > > ack, I am fine with that. > > Regards, > Lorenzo >
Cool, thanks! I squashed the change in and pushed this patch to main. Regards, Dumitru _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
