On Wed, Aug 6, 2025 at 12:54 PM Dumitru Ceara <dce...@redhat.com> wrote:
> On 7/25/25 8:03 AM, Ales Musil wrote: > > Add an option to LS that is inline with existing LR option called > > "dynamic-routing-redistribute". That option currently accepts only > > "fdb" which will lead to advertisment of workloads connected to > > the specified switch. This doesn't work without the switch having > > "dynamic-routing-vni" option set. > > > > Currently ovn-controller will advertise VIFs, virtual ports, > > container ports, DGPs and GW router ports bound to that chassis. > > In other words only local workloads are advertised to avoid > > the potential confusion of defining the same MAC multiple times > > as the same EVPN might be connected to more than one chassis. > > > > Reported-at: https://issues.redhat.com/browse/FDP-1389 > > Signed-off-by: Ales Musil <amu...@redhat.com> > > --- > > Hi Ales, > > I only have a few minor comments for now, otherwise this looks OK to me. > Hi Dumitru, thank you for the review. > > > NEWS | 2 + > > controller/neighbor.c | 137 ++++++++++++++++++++++++++++++++++++ > > controller/neighbor.h | 13 ++++ > > controller/ovn-controller.c | 92 +++++++++++++++++++++++- > > northd/northd.c | 13 ++++ > > northd/northd.h | 3 +- > > ovn-nb.xml | 24 +++++++ > > 7 files changed, 282 insertions(+), 2 deletions(-) > > > > diff --git a/NEWS b/NEWS > > index 2659e2b70..f3278628d 100644 > > --- a/NEWS > > +++ b/NEWS > > @@ -47,6 +47,8 @@ Post v25.03.0 > > for the EVPN traffic egressing through the tunnel. > > * Add the "other_config:dynamic-routing-vni" to Logical Switches. > If set > > to valid integer the LS is considered to be connected to EVPN L2 > domain. > > + * Allow the "other_config:dynamic-routing-redistribute" to Logical > > Maybe 'Add the "other_config:dynamic-routing-redistribute" ...'? > > > + Switches. The only accept value at the moment is "fdb". > > Typo: s/accept/accepted/ > > > > > OVN v25.03.0 - 07 Mar 2025 > > -------------------------- > > diff --git a/controller/neighbor.c b/controller/neighbor.c > > index ee77d02d6..9fe9ee405 100644 > > --- a/controller/neighbor.c > > +++ b/controller/neighbor.c > > @@ -19,6 +19,7 @@ > > #include "lib/packets.h" > > #include "lib/sset.h" > > #include "local_data.h" > > +#include "lport.h" > > #include "ovn-sb-idl.h" > > > > #include "neighbor.h" > > @@ -38,6 +39,17 @@ static struct neighbor_interface_monitor * > > neighbor_interface_monitor_alloc(enum neighbor_family family, > > enum neighbor_interface_type type, > > uint32_t vni); > > +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 const struct sbrec_port_binding > *neighbor_get_relevant_port_binding( > > + struct ovsdb_idl_index *sbrec_pb_by_name, > > + const struct sbrec_port_binding *); > > +static void advertise_neigh_add(struct hmap *neighbors, struct eth_addr > mac, > > + struct in6_addr ip); > > +static struct advertise_neighbor_entry *advertise_neigh_find( > > + const struct hmap *neighbors, struct eth_addr mac, > > + const struct in6_addr *ip); > > > > uint32_t > > advertise_neigh_hash(const struct eth_addr *eth, const struct in6_addr > *ip) > > @@ -85,6 +97,16 @@ neighbor_run(struct neighbor_ctx_in *n_ctx_in, > > neighbor_interface_monitor_alloc(NEIGH_AF_INET6, > > NEIGH_IFACE_BRIDGE, vni); > > vector_push(n_ctx_out->monitored_interfaces, &br_v6); > > + > > + > > Nit: too many empty lines. > > > + if (!smap_get_bool(&ld->datapath->external_ids, > > + "dynamic-routing-advertise-fdb", false)) { > > + continue; > > + } > > + > > + neighbor_collect_mac_to_advertise(n_ctx_in, > &lo->announced_neighbors, > > + n_ctx_out->advertised_pbs, > > + ld->datapath); > > } > > } > > > > @@ -98,6 +120,26 @@ neighbor_cleanup(struct vector *monitored_interfaces) > > vector_clear(monitored_interfaces); > > } > > > > +bool > > +neighbor_is_relevant_port_updated(struct ovsdb_idl_index > *sbrec_pb_by_name, > > + const struct sbrec_chassis *chassis, > > + struct sset *advertised_pbs, > > + const struct tracked_lport *lport) > > +{ > > + if (lport->tracked_type == TRACKED_RESOURCE_REMOVED && > > + sset_contains(advertised_pbs, lport->pb->logical_port)) { > > + return true; > > + } > > + > > + if (lport->tracked_type != TRACKED_RESOURCE_NEW) { > > + return false; > > + } > > + > > + const struct sbrec_port_binding *pb = > > + neighbor_get_relevant_port_binding(sbrec_pb_by_name, lport->pb); > > + return pb && lport_pb_is_chassis_resident(chassis, pb); > > Nit: too many spaces > > > +} > > + > > static void > > neighbor_interface_monitor_destroy(struct neighbor_interface_monitor > *nim) > > { > > @@ -139,3 +181,98 @@ neighbor_interface_monitor_alloc(enum > neighbor_family family, > > neighbor_interface_prefixes[type], vni); > > return nim; > > } > > + > > +static void > > +neighbor_collect_mac_to_advertise(const struct neighbor_ctx_in > *n_ctx_in, > > + struct hmap *neighbors, > > + struct sset *advertised_pbs, > > + const struct sbrec_datapath_binding > *dp) > > +{ > > + struct sbrec_port_binding *target = > > + sbrec_port_binding_index_init_row(n_ctx_in->sbrec_pb_by_dp); > > + sbrec_port_binding_index_set_datapath(target, dp); > > + > > + const struct sbrec_port_binding *dp_pb; > > + SBREC_PORT_BINDING_FOR_EACH_EQUAL (dp_pb, target, > > + n_ctx_in->sbrec_pb_by_dp) { > > + const struct sbrec_port_binding *pb = > > + > neighbor_get_relevant_port_binding(n_ctx_in->sbrec_pb_by_name, > > + dp_pb); > > + if (!(pb && lport_pb_is_chassis_resident(n_ctx_in->chassis, > pb))) { > > + continue; > > + } > > + > > + for (size_t i = 0; i < pb->n_mac; i++) { > > + struct lport_addresses addresses; > > + if (!extract_lsp_addresses(pb->mac[i], &addresses)) { > > + continue; > > + } > > It's a bit annoying we parse lsp addresses so many times in > ovn-controller (even before this call). I'm just ranting, we should > maybe try to improve this in general in ovn-controller in the future. > > > + > > + if (!advertise_neigh_find(neighbors, addresses.ea, > &in6addr_any)) { > > + advertise_neigh_add(neighbors, addresses.ea, > in6addr_any); > > + } > > + > > + destroy_lport_addresses(&addresses); > > + } > > + > > + sset_add(advertised_pbs, pb->logical_port); > > + } > > + > > + sbrec_port_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) > > +{ > > + enum en_lport_type type = get_lport_type(pb); > > + if (type == LP_VIF || type == LP_VIRTUAL || type == LP_CONTAINER) { > > + return pb; > > + } > > + > > + if (type == LP_L3GATEWAY) { > > + return lport_get_peer(pb, sbrec_pb_by_name); > > + } > > + > > + if (type == LP_PATCH) { > > + const struct sbrec_port_binding *peer = > > + lport_get_peer(pb, sbrec_pb_by_name); > > + if (!peer) { > > + return NULL; > > + } > > + > > + return lport_get_cr_port(sbrec_pb_by_name, peer, NULL); > > + } > > + > > + return NULL; > > +} > > + > > +static void > > +advertise_neigh_add(struct hmap *neighbors, struct eth_addr mac, > > + struct in6_addr ip) > > +{ > > + struct advertise_neighbor_entry *ne = xmalloc(sizeof *ne); > > + *ne = (struct advertise_neighbor_entry) { > > + .lladdr = mac, > > + .addr = ip, > > + }; > > + > > + hmap_insert(neighbors, &ne->node, advertise_neigh_hash(&mac, &ip)); > > +} > > + > > +static struct advertise_neighbor_entry * > > +advertise_neigh_find(const struct hmap *neighbors, struct eth_addr mac, > > + const struct in6_addr *ip) > > +{ > > + uint32_t hash = advertise_neigh_hash(&mac, ip); > > + > > + struct advertise_neighbor_entry *ne; > > + HMAP_FOR_EACH_WITH_HASH (ne, node, hash, neighbors) { > > + if (eth_addr_equals(ne->lladdr, mac) && > > + ipv6_addr_equals(&ne->addr, ip)) { > > + return ne; > > + } > > + } > > + > > + return NULL; > > +} > > diff --git a/controller/neighbor.h b/controller/neighbor.h > > index 3dc21938b..c5d063ad5 100644 > > --- a/controller/neighbor.h > > +++ b/controller/neighbor.h > > @@ -33,6 +33,8 @@ > > #endif > > #endif /* __APPLE__ */ > > > > +struct tracked_lport; > > + > > enum neighbor_family { > > NEIGH_AF_INET = AF_INET, > > NEIGH_AF_INET6 = AF_INET6, > > @@ -42,11 +44,18 @@ enum neighbor_family { > > struct neighbor_ctx_in { > > /* Contains 'struct local_datapath'. */ > > const struct hmap *local_datapaths; > > + /* Index for Port Binding by Datapath. */ > > + struct ovsdb_idl_index *sbrec_pb_by_dp; > > + /* Index for Port Binding by name. */ > > + struct ovsdb_idl_index *sbrec_pb_by_name; > > + const struct sbrec_chassis *chassis; > > }; > > > > struct neighbor_ctx_out { > > /* Contains struct neighbor_interface_monitor pointers. */ > > struct vector *monitored_interfaces; > > + /* Contains set of PB names that are currently advertised. */ > > Nit: I think I'd expand "PB" into "port binding". > > > + struct sset *advertised_pbs; > > }; > > > > enum neighbor_interface_type { > > @@ -79,5 +88,9 @@ uint32_t advertise_neigh_hash(const struct eth_addr *, > > const struct in6_addr *); > > void neighbor_run(struct neighbor_ctx_in *, struct neighbor_ctx_out *); > > void neighbor_cleanup(struct vector *monitored_interfaces); > > +bool neighbor_is_relevant_port_updated( > > + struct ovsdb_idl_index *sbrec_pb_by_name, > > + const struct sbrec_chassis *chassis, struct sset *advertised_pbs, > > + const struct tracked_lport *lport); > > > > #endif /* NEIGHBOR_H */ > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > > index a3d4f28c6..5786542f6 100644 > > --- a/controller/ovn-controller.c > > +++ b/controller/ovn-controller.c > > @@ -5772,6 +5772,8 @@ en_host_if_monitor_run(struct engine_node *node > OVS_UNUSED, void *data) > > struct ed_type_neighbor { > > /* Contains struct neighbor_interface_monitor pointers. */ > > struct vector monitored_interfaces; > > + /* Contains set of PB names that are currently advertised. */ > > + struct sset advertised_pbs; > > }; > > > > static void * > > @@ -5783,6 +5785,7 @@ en_neighbor_init(struct engine_node *node > OVS_UNUSED, > > *data = (struct ed_type_neighbor) { > > .monitored_interfaces = > > VECTOR_EMPTY_INITIALIZER(struct neighbor_interface_monitor > *), > > + .advertised_pbs = SSET_INITIALIZER(&data->advertised_pbs), > > }; > > return data; > > } > > @@ -5794,6 +5797,7 @@ en_neighbor_cleanup(void *data) > > > > neighbor_cleanup(&ne_data->monitored_interfaces); > > vector_destroy(&ne_data->monitored_interfaces); > > + sset_destroy(&ne_data->advertised_pbs); > > } > > > > static enum engine_node_state > > @@ -5802,25 +5806,68 @@ en_neighbor_run(struct engine_node *node > OVS_UNUSED, void *data) > > struct ed_type_neighbor *ne_data = data; > > struct ed_type_runtime_data *rt_data = > > engine_get_input_data("runtime_data", node); > > + struct ovsdb_idl_index *sbrec_port_binding_by_datapath = > > + engine_ovsdb_node_get_index( > > + engine_get_input("SB_port_binding", node), > > + "datapath"); > > + struct ovsdb_idl_index *sbrec_port_binding_by_name = > > + engine_ovsdb_node_get_index( > > + engine_get_input("SB_port_binding", node), > > + "name"); > > + const struct ovsrec_open_vswitch_table *ovs_table = > > + EN_OVSDB_GET(engine_get_input("OVS_open_vswitch", node)); > > + struct ovsdb_idl_index *sbrec_chassis_by_name = > > + engine_ovsdb_node_get_index( > > + engine_get_input("SB_chassis", node), > > + "name"); > > + > > + const char *chassis_id = get_ovs_chassis_id(ovs_table); > > + ovs_assert(chassis_id); > > + const struct sbrec_chassis *chassis = > > + chassis_lookup_by_name(sbrec_chassis_by_name, chassis_id); > > + ovs_assert(chassis); > > + > > struct neighbor_ctx_in n_ctx_in = { > > .local_datapaths = &rt_data->local_datapaths, > > + .sbrec_pb_by_dp = sbrec_port_binding_by_datapath, > > + .sbrec_pb_by_name = sbrec_port_binding_by_name, > > + .chassis = chassis, > > }; > > > > struct neighbor_ctx_out n_ctx_out = { > > .monitored_interfaces = &ne_data->monitored_interfaces, > > + .advertised_pbs = &ne_data->advertised_pbs, > > }; > > > > neighbor_cleanup(&ne_data->monitored_interfaces); > > + sset_clear(&ne_data->advertised_pbs); > > neighbor_run(&n_ctx_in, &n_ctx_out); > > > > return EN_UPDATED; > > } > > > > static enum engine_input_handler_result > > -neighbor_runtime_data_handler(struct engine_node *node, void *data > OVS_UNUSED) > > +neighbor_runtime_data_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); > > + struct ovsdb_idl_index *sbrec_port_binding_by_name = > > + engine_ovsdb_node_get_index( > > + engine_get_input("SB_port_binding", node), > > + "name"); > > + const struct ovsrec_open_vswitch_table *ovs_table = > > + EN_OVSDB_GET(engine_get_input("OVS_open_vswitch", node)); > > + struct ovsdb_idl_index *sbrec_chassis_by_name = > > + engine_ovsdb_node_get_index( > > + engine_get_input("SB_chassis", node), > > + "name"); > > + > > + const char *chassis_id = get_ovs_chassis_id(ovs_table); > > + ovs_assert(chassis_id); > > + const struct sbrec_chassis *chassis = > > + chassis_lookup_by_name(sbrec_chassis_by_name, chassis_id); > > + ovs_assert(chassis); > > > > /* There are no tracked data. Fall back to full recompute. */ > > if (!rt_data->tracked) { > > @@ -5845,6 +5892,21 @@ neighbor_runtime_data_handler(struct engine_node > *node, void *data OVS_UNUSED) > > tdp->tracked_type == TRACKED_RESOURCE_REMOVED) { > > return EN_UNHANDLED; > > } > > + > > + if (!smap_get_bool(&ld->datapath->external_ids, > > + "dynamic-routing-advertise-fdb", false)) { > > + continue; > > + } > > + > > + const struct shash_node *shash_node; > > + SHASH_FOR_EACH (shash_node, &tdp->lports) { > > + if > (neighbor_is_relevant_port_updated(sbrec_port_binding_by_name, > > + chassis, > > + > &ne_data->advertised_pbs, > > + shash_node->data)) { > > + return EN_UNHANDLED; > > + } > > + } > > } > > > > return EN_HANDLED_UNCHANGED; > > @@ -5882,6 +5944,30 @@ neighbor_sb_datapath_binding_handler(struct > engine_node *node, > > return EN_HANDLED_UNCHANGED; > > } > > > > +static enum engine_input_handler_result > > +neighbor_sb_port_binding_handler(struct engine_node *node, void *data) > > +{ > > + struct ed_type_neighbor *ne_data = data; > > + const struct sbrec_port_binding_table *pb_table = > > + EN_OVSDB_GET(engine_get_input("SB_port_binding", node)); > > + > > + const struct sbrec_port_binding *pb; > > + SBREC_PORT_BINDING_TABLE_FOR_EACH_TRACKED (pb, pb_table) { > > + if (sbrec_port_binding_is_new(pb) || > > + sbrec_port_binding_is_deleted(pb)) { > > + /* The removal and addition is handled via runtime_data. */ > > + return EN_HANDLED_UNCHANGED; > > + } > > + > > + if (sbrec_port_binding_is_updated(pb, > SBREC_PORT_BINDING_COL_MAC) && > > + sset_contains(&ne_data->advertised_pbs, pb->logical_port)) { > > + return EN_UNHANDLED; > > + } > > + } > > + > > + return EN_HANDLED_UNCHANGED; > > +} > > + > > struct ed_type_neighbor_table_notify { > > /* For incremental processing this could be tracked per interface in > > * the future. */ > > @@ -6775,10 +6861,14 @@ main(int argc, char *argv[]) > > engine_add_input(&en_garp_rarp, &en_runtime_data, > > garp_rarp_runtime_data_handler); > > > > + 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_runtime_data, > > neighbor_runtime_data_handler); > > engine_add_input(&en_neighbor, &en_sb_datapath_binding, > > neighbor_sb_datapath_binding_handler); > > + engine_add_input(&en_neighbor, &en_sb_port_binding, > > + neighbor_sb_port_binding_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/northd/northd.c b/northd/northd.c > > index b45cac3cf..cd9409023 100644 > > --- a/northd/northd.c > > +++ b/northd/northd.c > > @@ -830,6 +830,11 @@ ovn_datapath_update_external_ids(struct > ovn_datapath *od) > > "dynamic-routing-vni"); > > smap_add(&ids, "dynamic-routing-vni", vni); > > } > > + > > + smap_add(&ids, "dynamic-routing-advertise-fdb", > > + drr_mode_FDB_is_set(od->dynamic_routing_redistribute) > > + ? "true" > > + : "false"); > > } > > > > /* Set snat-ct-zone */ > > @@ -906,6 +911,10 @@ parse_dynamic_routing_redistribute( > > out |= DRRM_LB; > > continue; > > } > > + if (!strcmp(token, "fdb")) { > > + out |= DRRM_FDB; > > + continue; > > + } > > As parse_dynamic_routing_redistribute() is now used for both switches > and routers (router ports) should we validate the parsed values in the > callers? > I have added an extra argument to the function. > > > static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1); > > VLOG_WARN_RL(&rl, > > "unknown dynamic-routing-redistribute option '%s' > on %s", > > @@ -991,6 +1000,10 @@ join_datapaths(const struct > nbrec_logical_switch_table *nbrec_ls_table, > > if (ovn_is_valid_vni(vni)) { > > od->has_evpn_vni = true; > > } > > + > > + od->dynamic_routing_redistribute = > > + parse_dynamic_routing_redistribute(&od->nbs->other_config, > > + DRRM_NONE, > od->nbs->name); > > } > > > > const struct nbrec_logical_router *nbr; > > diff --git a/northd/northd.h b/northd/northd.h > > index fbd76704d..08730d4f5 100644 > > --- a/northd/northd.h > > +++ b/northd/northd.h > > @@ -322,7 +322,8 @@ struct mcast_port_info { > > DRR_MODE(CONNECTED_AS_HOST, 1) \ > > DRR_MODE(STATIC, 2) \ > > DRR_MODE(NAT, 3) \ > > - DRR_MODE(LB, 4) > > + DRR_MODE(LB, 4) \ > > + DRR_MODE(FDB, 5) > > > > enum dynamic_routing_redistribute_mode_bits { > > #define DRR_MODE(PROTOCOL, BIT) DRRM_##PROTOCOL##_BIT = BIT, > > diff --git a/ovn-nb.xml b/ovn-nb.xml > > index 065bffd3f..a3e8da0fb 100644 > > --- a/ovn-nb.xml > > +++ b/ovn-nb.xml > > @@ -889,6 +889,30 @@ > > removal/change in the future. > > </p> > > </column> > > + > > + <column name="other_config" key="dynamic-routing-redistribute" > > + type='{"type": "string"}'> > > + <p> > > + Only relevant if <ref column="other_config" > key="dynamic-routing-vni" > > + table="Logical_switch"/> is set to valid VNI. > > + </p> > > + > > + <p> > > + This is a list of elements separated by <code>,</code>. > > This sounds a bit weird as we only allow one value. :) > True. I have updated that in v2. > > > + </p> > > + > > + <p> > > + If <code>fdb</code> is in the list then ovn-controller will > > + all workloads that are local to the chassis. The applies to > VIFs, > > + container ports, virtual ports, connected DGPs and connected > GW > > + routers. > > + </p> > > + > > + <p> > > + NOTE: this feature is experimental and may be subject to > > + removal/change in the future. > > + </p> > > + </column> > > </group> > > > > <group title="IP Multicast Snooping Options"> > > Regards, > Dumitru > > Thanks, Ales _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
