On 1/29/25 12:15 PM, Felix Huettner via dev wrote: > Previously we just assumed that if a LR had multiple LRPs bound to the > local chassis that all routes where valid for all of these LRPs. > This commit handles a previous option in ovn-nb that allowes the user to > specify the interface name that a route needs to use to be acceptable > for a given LRP. > The users can then have a 1:1 relationship between LRPs and interfaces > to allow ovn-controller to determine which route belongs to which LRP. > > This can e.g. be used if a chassis has multiple uplinks. Each of these > uplinks could be assigned to a different LRP of the same LR connected to > the same localnet. > The option allows then to map e.g. a learned default route to each > individual LRP. If one of the links would go down just one of the > learned routes would be removed, causing ECMP across multiple chassis to > be correctly weighted. > > Signed-off-by: Felix Huettner <felix.huettner@stackit.cloud> > ---
Hi Felix, I only have two minor comments below. If addressing those is the only diff in v6, feel free to also add my ack: Acked-by: Dumitru Ceara <dce...@redhat.com> > v3->v4: > - addressed review comments. > - updated commit message to be more descriptive. > > controller/route-exchange-netlink.c | 1 + > controller/route-exchange-netlink.h | 3 +++ > controller/route-exchange.c | 20 ++++++++++++++------ > controller/route.c | 14 +++++++++----- > controller/route.h | 6 ++++-- > tests/system-ovn.at | 23 ++++++++++++++++++++++- > 6 files changed, 53 insertions(+), 14 deletions(-) > > diff --git a/controller/route-exchange-netlink.c > b/controller/route-exchange-netlink.c > index 74741a3fd..c5a77efe4 100644 > --- a/controller/route-exchange-netlink.c > +++ b/controller/route-exchange-netlink.c > @@ -237,6 +237,7 @@ handle_route_msg(const struct route_table_msg *msg, void > *data) > rr->addr = rd->rta_dst; > rr->plen = rd->rtm_dst_len; > rr->nexthop = nexthop->addr; > + memcpy(rr->ifname, nexthop->ifname, IFNAMSIZ); As mentioned on patch 6/11 I think I'd just set rr->ifname[IFNAMSIZ] = 0 and use xmalloc() instead of xzalloc(). > } > return; > } > diff --git a/controller/route-exchange-netlink.h > b/controller/route-exchange-netlink.h > index bc77504ae..e4fe81977 100644 > --- a/controller/route-exchange-netlink.h > +++ b/controller/route-exchange-netlink.h > @@ -21,6 +21,7 @@ > #include <stdint.h> > #include "openvswitch/list.h" > #include <netinet/in.h> > +#include <net/if.h> > > /* This value is arbitrary but currently unused. > * See https://github.com/iproute2/iproute2/blob/main/etc/iproute2/rt_protos > */ > @@ -35,6 +36,8 @@ struct re_nl_received_route_node { > struct in6_addr addr; > unsigned int plen; > struct in6_addr nexthop; > + /* Adding 1 to this to be sure we actually have a terminating '\0' */ > + char ifname[IFNAMSIZ + 1]; > }; > > int re_nl_create_vrf(const char *ifname, uint32_t table_id); > diff --git a/controller/route-exchange.c b/controller/route-exchange.c > index a163968a7..dc3e5657c 100644 > --- a/controller/route-exchange.c > +++ b/controller/route-exchange.c > @@ -97,7 +97,7 @@ route_lookup(struct hmap *route_map, > static void > sb_sync_learned_routes(const struct ovs_list *learned_routes, > const struct sbrec_datapath_binding *datapath, > - const struct sset *bound_ports, > + const struct smap *bound_ports, > struct ovsdb_idl_txn *ovnsb_idl_txn, > struct ovsdb_idl_index *sbrec_port_binding_by_name, > struct ovsdb_idl_index > *sbrec_learned_route_by_datapath) > @@ -112,8 +112,9 @@ sb_sync_learned_routes(const struct ovs_list > *learned_routes, > SBREC_LEARNED_ROUTE_FOR_EACH_EQUAL (sb_route, filter, > sbrec_learned_route_by_datapath) { > /* If the port is not local we don't care about it. > - * Some other ovn-controller will handle it. */ > - if (!sset_contains(bound_ports, > + * Some other ovn-controller will handle it. > + * We may not use smap_get since the value might be validly NULL. */ > + if (!smap_get_node(bound_ports, > sb_route->logical_port->logical_port)) { > continue; > } > @@ -127,11 +128,18 @@ sb_sync_learned_routes(const struct ovs_list > *learned_routes, > learned_route->plen); > char *nexthop = normalize_v46(&learned_route->nexthop); > > - const char *logical_port_name; > - SSET_FOR_EACH (logical_port_name, bound_ports) { > + struct smap_node *port_node; > + SMAP_FOR_EACH (port_node, bound_ports) { > + /* The user specified an ifname, but we learned it on a different > + * port. */ > + if (port_node->value && strcmp(port_node->value, > + learned_route->ifname)) { > + continue; > + } > + > const struct sbrec_port_binding *logical_port = > lport_lookup_by_name(sbrec_port_binding_by_name, > - logical_port_name); > + port_node->key); > if (!logical_port) { > continue; > } > diff --git a/controller/route.c b/controller/route.c > index 49e76ee73..22f38976b 100644 > --- a/controller/route.c > +++ b/controller/route.c > @@ -87,7 +87,7 @@ advertise_datapath_cleanup(struct advertise_datapath_entry > *ad) > free(ar); > } > hmap_destroy(&ad->routes); > - sset_destroy(&ad->bound_ports); > + smap_destroy(&ad->bound_ports); > free(ad); > } > > @@ -108,8 +108,8 @@ void > route_run(struct route_ctx_in *r_ctx_in, > struct route_ctx_out *r_ctx_out) > { > - struct advertise_datapath_entry *ad; > const struct local_datapath *ld; > + struct advertise_datapath_entry *ad; I actually think it looks better in the other order (reverse xmas tree). :) > > HMAP_FOR_EACH (ld, hmap_node, r_ctx_in->local_datapaths) { > if (!ld->n_peer_ports || ld->is_switch) { > @@ -119,7 +119,7 @@ route_run(struct route_ctx_in *r_ctx_in, > ad = xzalloc(sizeof(*ad)); > ad->db = ld->datapath; > hmap_init(&ad->routes); > - sset_init(&ad->bound_ports); > + smap_init(&ad->bound_ports); > > /* This is a LR datapath, find LRPs with route exchange options > * that are bound locally. */ > @@ -137,10 +137,14 @@ route_run(struct route_ctx_in *r_ctx_in, > > ad->maintain_vrf |= smap_get_bool( > &repb->options, "dynamic-routing-maintain-vrf", false); > - sset_add(&ad->bound_ports, local_peer->logical_port); > + char *ifname = nullable_xstrdup( > + smap_get(&repb->options, > + "dynamic-routing-ifname")); > + smap_add_nocopy(&ad->bound_ports, > + xstrdup(local_peer->logical_port), ifname); > } > > - if (sset_is_empty(&ad->bound_ports)) { > + if (smap_is_empty(&ad->bound_ports)) { > advertise_datapath_cleanup(ad); > continue; > } > diff --git a/controller/route.h b/controller/route.h > index 6fc5963a5..f23115abb 100644 > --- a/controller/route.h > +++ b/controller/route.h > @@ -22,6 +22,7 @@ > #include <netinet/in.h> > #include "openvswitch/hmap.h" > #include "sset.h" > +#include "smap.h" > > struct hmap; > struct ovsdb_idl_index; > @@ -50,8 +51,9 @@ struct advertise_datapath_entry { > struct hmap routes; > > /* The name of the port bindings locally bound for this datapath and > - * running route exchange logic. */ > - struct sset bound_ports; > + * running route exchange logic. > + * The key is the port name and the value is the ifname if set. */ > + struct smap bound_ports; > }; > > struct advertise_route_entry { > diff --git a/tests/system-ovn.at b/tests/system-ovn.at > index dc99d4c57..e17d9534e 100644 > --- a/tests/system-ovn.at > +++ b/tests/system-ovn.at > @@ -15058,6 +15058,17 @@ check_row_count Learned_Route 1 > lp=$(fetch_column port_binding _uuid logical_port=internet-phys) > check_row_count Learned_Route 1 logical_port=$lp ip_prefix=233.252.0.0/24 > nexthop=192.168.10.10 > > +# By setting a learning interface filter we will now forget about this route. > +check ovn-nbctl --wait=hv set Logical_Router_Port internet-phys \ > + options:dynamic-routing-ifname=thisdoesnotexist > +check_row_count Learned_Route 0 > + > +# Changing it to "lo" will allow us to learn the route again. > +check ovn-nbctl --wait=hv set Logical_Router_Port internet-phys \ > + options:dynamic-routing-ifname=lo > +check_row_count Learned_Route 1 > + > + > OVS_APP_EXIT_AND_WAIT([ovn-controller]) > > as ovn-sb > @@ -15268,10 +15279,20 @@ check ip route add 233.252.0.0/24 via 192.168.10.10 > dev lo onlink vrf ovnvrf1337 > # For now we trigger a recompute as route watching is not yet implemented. > check ovn-appctl -t ovn-controller inc-engine/recompute > check ovn-nbctl --wait=hv sync > -check_row_count Learned_Route 2 > +check_row_count Learned_Route 1 > lp=$(fetch_column port_binding _uuid logical_port=internet-phys) > check_row_count Learned_Route 1 logical_port=$lp ip_prefix=233.252.0.0/24 > nexthop=192.168.10.10 > > +# By setting a learning interface filter we will now forget about this route. > +check ovn-nbctl --wait=hv set Logical_Router_Port internet-phys \ > + options:dynamic-routing-ifname=thisdoesnotexist > +check_row_count Learned_Route 0 > + > +# Changing it to "lo" will allow us to learn the route again. > +check ovn-nbctl --wait=hv set Logical_Router_Port internet-phys \ > + options:dynamic-routing-ifname=lo > +check_row_count Learned_Route 1 > + > as ovn-sb > OVS_APP_EXIT_AND_WAIT([ovsdb-server]) > Thanks, Dumitru _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
