On 12/17/25 8:31 PM, Mark Michelson wrote: > Hi Lorenzo, thanks for the patch. Should this patch have:
Hi Lorenzo, Mark, Thanks for the patch and review! > > Reported-at: https://issues.redhat.com/browse/FDP-2730 > > ? Yes, it should, indeed. > > Aside from that, > > Acked-by: Mark Michelson <[email protected]> > On Wed, Dec 17, 2025 at 8:40 AM Lorenzo Bianconi via dev > <[email protected]> wrote: >> >> We need to be able to monitor two vxlan interfaces (one for IPv4 and one >> for IPv6) for dual stack EVPN logical switches since we can't configure a >> single vxlan interface with multiple local IPs (one for v4 one for v6). >> Fix the probelm allowing the CMS to specify a list of vxlan device names >> using the following syntax: >> >> dynamic-routing-vxlan-ifname=vxlan-if1,vxlan-if2,..,vxlan-ifn >> >> Fixes: 36737306806e ("Add the capability to specify EVPN device names.") >> Signed-off-by: Lorenzo Bianconi <[email protected]> >> --- >> controller/neighbor.c | 78 ++++++++++++++++++++--------- >> northd/en-datapath-logical-switch.c | 57 ++++++++++----------- >> ovn-nb.xml | 4 +- >> 3 files changed, 85 insertions(+), 54 deletions(-) >> >> diff --git a/controller/neighbor.c b/controller/neighbor.c >> index 075e672fa..6022cec82 100644 >> --- a/controller/neighbor.c >> +++ b/controller/neighbor.c >> @@ -21,10 +21,13 @@ >> #include "local_data.h" >> #include "lport.h" >> #include "openvswitch/ofp-parse.h" >> +#include "openvswitch/vlog.h" >> #include "ovn-sb-idl.h" >> >> #include "neighbor.h" >> >> +VLOG_DEFINE_THIS_MODULE(neighbor); >> + >> static const char *neighbor_interface_prefixes[] = { >> [NEIGH_IFACE_BRIDGE] = "br-", >> [NEIGH_IFACE_VXLAN] = "vxlan-", >> @@ -43,10 +46,9 @@ static bool neighbor_interface_with_vni_exists( >> struct vector *monitored_interfaces, >> uint32_t vni); >> static struct neighbor_interface_monitor * >> -neighbor_interface_monitor_alloc(struct local_datapath *ld, >> - enum neighbor_family family, >> +neighbor_interface_monitor_alloc(enum neighbor_family family, >> enum neighbor_interface_type type, >> - uint32_t vni); >> + uint32_t vni, const char *if_name); >> static void neighbor_collect_mac_to_advertise( >> const struct neighbor_ctx_in *, struct hmap *neighbors, >> struct sset *advertised_pbs, const struct sbrec_datapath_binding *); >> @@ -83,6 +85,23 @@ advertise_neigh_find(const struct hmap *neighbors, struct >> eth_addr mac, >> return NULL; >> } >> >> +static void >> +neigh_parse_device_name(struct sset *device_names, struct local_datapath >> *ld, >> + enum neighbor_interface_type type, uint32_t vni) >> +{ >> + const char *names = smap_get_def(&ld->datapath->external_ids, >> + neighbor_opt_name[type], ""); >> + sset_clear(device_names); >> + sset_from_delimited_string(device_names, names, ","); >> + if (sset_is_empty(device_names)) { >> + /* Default device name if not specified. */ >> + char if_name[IFNAMSIZ + 1]; >> + snprintf(if_name, sizeof if_name, "%s%"PRIu32, >> + neighbor_interface_prefixes[type], vni); >> + sset_add(device_names, if_name); >> + } >> +} >> + >> void >> neighbor_run(struct neighbor_ctx_in *n_ctx_in, >> struct neighbor_ctx_out *n_ctx_out) >> @@ -104,25 +123,44 @@ neighbor_run(struct neighbor_ctx_in *n_ctx_in, >> continue; >> } >> >> - struct neighbor_interface_monitor *vxlan = >> - neighbor_interface_monitor_alloc(ld, NEIGH_AF_BRIDGE, >> - NEIGH_IFACE_VXLAN, vni); >> - vector_push(n_ctx_out->monitored_interfaces, &vxlan); >> + struct sset device_names = SSET_INITIALIZER(&device_names); >> + neigh_parse_device_name(&device_names, ld, NEIGH_IFACE_VXLAN, vni); >> + const char *name; >> + SSET_FOR_EACH (name, &device_names) { >> + struct neighbor_interface_monitor *vxlan = >> + neighbor_interface_monitor_alloc(NEIGH_AF_BRIDGE, >> + NEIGH_IFACE_VXLAN, vni, >> name); >> + vector_push(n_ctx_out->monitored_interfaces, &vxlan); >> + } >> >> + neigh_parse_device_name(&device_names, ld, NEIGH_IFACE_LOOPBACK, >> vni); >> + if (sset_count(&device_names) > 1) { >> + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5); >> + VLOG_WARN_RL(&rl, "too many names provided for loopback >> device"); Nit: it might be useful to log the datapath UUID here. >> + } >> struct neighbor_interface_monitor *lo = >> - neighbor_interface_monitor_alloc(ld, NEIGH_AF_BRIDGE, >> - NEIGH_IFACE_LOOPBACK, vni); >> + neighbor_interface_monitor_alloc(NEIGH_AF_BRIDGE, >> + NEIGH_IFACE_LOOPBACK, vni, >> + SSET_FIRST(&device_names)); >> vector_push(n_ctx_out->monitored_interfaces, &lo); >> >> + neigh_parse_device_name(&device_names, ld, NEIGH_IFACE_BRIDGE, vni); >> + if (sset_count(&device_names) > 1) { >> + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5); >> + VLOG_WARN_RL(&rl, "too many names provided for bridge device"); Here too. >> + } >> struct neighbor_interface_monitor *br_v4 = >> - neighbor_interface_monitor_alloc(ld, NEIGH_AF_INET, >> - NEIGH_IFACE_BRIDGE, vni); >> + neighbor_interface_monitor_alloc(NEIGH_AF_INET, >> + NEIGH_IFACE_BRIDGE, vni, >> + SSET_FIRST(&device_names)); >> vector_push(n_ctx_out->monitored_interfaces, &br_v4); >> >> struct neighbor_interface_monitor *br_v6 = >> - neighbor_interface_monitor_alloc(ld, NEIGH_AF_INET6, >> - NEIGH_IFACE_BRIDGE, vni); >> + neighbor_interface_monitor_alloc(NEIGH_AF_INET6, >> + NEIGH_IFACE_BRIDGE, vni, >> + SSET_FIRST(&device_names)); >> vector_push(n_ctx_out->monitored_interfaces, &br_v6); >> + sset_destroy(&device_names); >> >> enum neigh_redistribute_mode mode = >> parse_neigh_dynamic_redistribute(&ld->datapath->external_ids); >> @@ -200,10 +238,9 @@ neighbor_interface_with_vni_exists(struct vector >> *monitored_interfaces, >> } >> >> static struct neighbor_interface_monitor * >> -neighbor_interface_monitor_alloc(struct local_datapath *ld, >> - enum neighbor_family family, >> +neighbor_interface_monitor_alloc(enum neighbor_family family, >> enum neighbor_interface_type type, >> - uint32_t vni) >> + uint32_t vni, const char *if_name) >> { >> struct neighbor_interface_monitor *nim = xmalloc(sizeof *nim); >> *nim = (struct neighbor_interface_monitor) { >> @@ -212,15 +249,8 @@ neighbor_interface_monitor_alloc(struct local_datapath >> *ld, >> .type = type, >> .vni = vni, >> }; >> + snprintf(nim->if_name, sizeof nim->if_name, "%s", if_name); >> >> - const char *if_name = smap_get(&ld->datapath->external_ids, >> - neighbor_opt_name[type]); >> - if (if_name) { >> - snprintf(nim->if_name, sizeof nim->if_name, "%s", if_name); >> - } else { >> - snprintf(nim->if_name, sizeof nim->if_name, "%s%"PRIu32, >> - neighbor_interface_prefixes[type], vni); >> - } >> return nim; >> } >> >> diff --git a/northd/en-datapath-logical-switch.c >> b/northd/en-datapath-logical-switch.c >> index ed0385a48..0ad5ad261 100644 >> --- a/northd/en-datapath-logical-switch.c >> +++ b/northd/en-datapath-logical-switch.c >> @@ -58,6 +58,26 @@ get_requested_tunnel_key(const struct >> nbrec_logical_switch *nbs, >> return requested_tunnel_key; >> } >> >> +static bool >> +check_dynamic_device_name(const char *if_names) >> +{ >> + struct sset device_names; >> + sset_from_delimited_string(&device_names, if_names, ","); >> + >> + bool ret = true; >> + const char *name; >> + SSET_FOR_EACH (name, &device_names) { >> + if (strlen(name) > IFNAMSIZ) { >> + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5); >> + VLOG_WARN_RL(&rl, "dynamic-routing-ifname %s is too long", >> name); >> + ret = false; >> + break; >> + } >> + } >> + sset_destroy(&device_names); >> + return ret; >> +} >> + >> static void >> gather_external_ids(const struct nbrec_logical_switch *nbs, >> struct smap *external_ids) >> @@ -95,42 +115,23 @@ gather_external_ids(const struct nbrec_logical_switch >> *nbs, >> >> const char *bridge_ifname = smap_get(&nbs->other_config, >> "dynamic-routing-bridge-ifname"); >> - if (bridge_ifname) { >> - if (strlen(bridge_ifname) > IFNAMSIZ) { >> - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5); >> - VLOG_WARN_RL(&rl, "dynamic-routing-bridge-ifname %s is too >> long", >> - bridge_ifname); >> - } else { >> - smap_add(external_ids, "dynamic-routing-bridge-ifname", >> - bridge_ifname); >> - } >> + if (bridge_ifname && check_dynamic_device_name(bridge_ifname)) { >> + smap_add(external_ids, "dynamic-routing-bridge-ifname", >> + bridge_ifname); >> } This is not exactly correct anymore. That's because bridge and advertise ifnames cannot be lists. So if the user provides a really long string however containing ',' more often than IFNAMESIZ characters then we incorrectly allow it. The same stands for adv_ifname below. I know we also check in ovn-controller but it might be good to log the warning in ovn-northd too. >> >> const char *vxlan_ifname = smap_get(&nbs->other_config, >> "dynamic-routing-vxlan-ifname"); >> - if (vxlan_ifname) { >> - if (strlen(vxlan_ifname) > IFNAMSIZ) { >> - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5); >> - VLOG_WARN_RL(&rl, "dynamic-routing-vxlan-ifname %s is too long", >> - vxlan_ifname); >> - } else { >> - smap_add(external_ids, "dynamic-routing-vxlan-ifname", >> - vxlan_ifname); >> - } >> + if (vxlan_ifname && check_dynamic_device_name(vxlan_ifname)) { >> + smap_add(external_ids, "dynamic-routing-vxlan-ifname", >> + vxlan_ifname); >> } >> >> const char *adv_ifname = smap_get(&nbs->other_config, >> "dynamic-routing-advertise-ifname"); >> - if (adv_ifname) { >> - if (strlen(adv_ifname) > IFNAMSIZ) { >> - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5); >> - VLOG_WARN_RL(&rl, >> - "dynamic-routing-advertise-ifname %s is too long", >> - adv_ifname); >> - } else { >> - smap_add(external_ids, "dynamic-routing-advertise-ifname", >> - adv_ifname); >> - } >> + if (adv_ifname && check_dynamic_device_name(adv_ifname)) { >> + smap_add(external_ids, "dynamic-routing-advertise-ifname", >> + adv_ifname); >> } >> >> const char *redistribute = >> diff --git a/ovn-nb.xml b/ovn-nb.xml >> index 19ccfc7ba..a1edd8d35 100644 >> --- a/ovn-nb.xml >> +++ b/ovn-nb.xml >> @@ -804,8 +804,8 @@ >> </column> >> >> <column name="other_config" key="dynamic-routing-vxlan-ifname"> >> - Set the interface name for the vxlan device used for EVPN >> integration. >> - The default name is <code>vxlan-$vni</code>. >> + List of interface names used for the vxlan devices used for EVPN >> + integration. The default name is <code>vxlan-$vni</code>. >> Only relevant if <ref column="other_config" key="dynamic-routing-vni" >> table="Logical_switch"/> is set to valid VNI. >> </column> >> -- >> 2.52.0 To simplify things I went ahead and made the straightforward validation change, squashed in the following incremental and applied the patch to main. Thanks, Dumitru diff --git a/controller/neighbor.c b/controller/neighbor.c index 6022cec825..9aeb1e36b6 100644 --- a/controller/neighbor.c +++ b/controller/neighbor.c @@ -136,7 +136,9 @@ neighbor_run(struct neighbor_ctx_in *n_ctx_in, neigh_parse_device_name(&device_names, ld, NEIGH_IFACE_LOOPBACK, vni); if (sset_count(&device_names) > 1) { static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5); - VLOG_WARN_RL(&rl, "too many names provided for loopback device"); + VLOG_WARN_RL(&rl, "Datapath "UUID_FMT" too many names provided " + "for loopback device", + UUID_ARGS(&ld->datapath->header_.uuid)); } struct neighbor_interface_monitor *lo = neighbor_interface_monitor_alloc(NEIGH_AF_BRIDGE, @@ -147,7 +149,9 @@ neighbor_run(struct neighbor_ctx_in *n_ctx_in, neigh_parse_device_name(&device_names, ld, NEIGH_IFACE_BRIDGE, vni); if (sset_count(&device_names) > 1) { static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5); - VLOG_WARN_RL(&rl, "too many names provided for bridge device"); + VLOG_WARN_RL(&rl, "Datapath "UUID_FMT" too many names provided " + "for bridge device", + UUID_ARGS(&ld->datapath->header_.uuid)); } struct neighbor_interface_monitor *br_v4 = neighbor_interface_monitor_alloc(NEIGH_AF_INET, diff --git a/northd/en-datapath-logical-switch.c b/northd/en-datapath-logical-switch.c index 0ad5ad2612..68925cb204 100644 --- a/northd/en-datapath-logical-switch.c +++ b/northd/en-datapath-logical-switch.c @@ -59,7 +59,22 @@ get_requested_tunnel_key(const struct nbrec_logical_switch *nbs, } static bool -check_dynamic_device_name(const char *if_names) +check_dynamic_device_name(const char *dynamic_routing_option, + const char *if_name) +{ + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5); + + if (strlen(if_name) > IFNAMSIZ) { + VLOG_WARN_RL(&rl, "%s %s is too long", dynamic_routing_option, + if_name); + return false; + } + return true; +} + +static bool +check_dynamic_device_names(const char *dynamic_routing_option, + const char *if_names) { struct sset device_names; sset_from_delimited_string(&device_names, if_names, ","); @@ -67,9 +82,7 @@ check_dynamic_device_name(const char *if_names) bool ret = true; const char *name; SSET_FOR_EACH (name, &device_names) { - if (strlen(name) > IFNAMSIZ) { - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5); - VLOG_WARN_RL(&rl, "dynamic-routing-ifname %s is too long", name); + if (!check_dynamic_device_name(dynamic_routing_option, name)) { ret = false; break; } @@ -115,21 +128,27 @@ gather_external_ids(const struct nbrec_logical_switch *nbs, const char *bridge_ifname = smap_get(&nbs->other_config, "dynamic-routing-bridge-ifname"); - if (bridge_ifname && check_dynamic_device_name(bridge_ifname)) { + if (bridge_ifname && + check_dynamic_device_name("dynamic-routing-bridge-ifname", + bridge_ifname)) { smap_add(external_ids, "dynamic-routing-bridge-ifname", bridge_ifname); } - const char *vxlan_ifname = smap_get(&nbs->other_config, - "dynamic-routing-vxlan-ifname"); - if (vxlan_ifname && check_dynamic_device_name(vxlan_ifname)) { + const char *vxlan_ifnames = smap_get(&nbs->other_config, + "dynamic-routing-vxlan-ifname"); + if (vxlan_ifnames && + check_dynamic_device_names("dynamic-routing-vxlan-ifname", + vxlan_ifnames)) { smap_add(external_ids, "dynamic-routing-vxlan-ifname", - vxlan_ifname); + vxlan_ifnames); } const char *adv_ifname = smap_get(&nbs->other_config, "dynamic-routing-advertise-ifname"); - if (adv_ifname && check_dynamic_device_name(adv_ifname)) { + if (adv_ifname && + check_dynamic_device_name("dynamic-routing-advertise-ifname", + adv_ifname)) { smap_add(external_ids, "dynamic-routing-advertise-ifname", adv_ifname); } _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
