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