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!
> 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
--
Regards,
Dumitru
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
