On Mon, Feb 03, 2025 at 02:48:24PM +0100, Dumitru Ceara wrote: > 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>
Hi Dumitru, thanks a lot for the review. The changes will be in the next version. Thanks a lot, Felix > > > 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
