On Thu, Oct 30, 2025 at 1:43 PM Felix Huettner <[email protected]> wrote:
> On Wed, Oct 29, 2025 at 05:35:20PM +0100, Ales Musil via dev wrote: > > On Wed, Oct 29, 2025 at 4:32 PM Felix Huettner > <[email protected]> > > wrote: > > > > > On Wed, Oct 29, 2025 at 01:30:45PM +0100, Ales Musil via dev wrote: > > > > Add an LR option, called "dynamic-routing-vrf-id", that allows CMS to > > > > specify vrf table_id. The table_id will be used instead of tunnel_key > > > > for given datapath corresponding to the LR. This also allows CMS to > > > > share one vrf between multiple LRs. > > > > > > > > Fixes: cb7ba9e9057c ("controller: Support user defined vrf names.") > > > > Fixes: faf4df563f1d ("controller: Announce routes via > route-exchange.") > > > > Reported-at: https://issues.redhat.com/browse/FDP-1886 > > > > Signed-off-by: Ales Musil <[email protected]> > > > > > > Hi Ales, > > > > > > thanks for that patch. > > > > > > > Hi Felix, > > > > thank you for the review. > > > > > > > > > > > --- > > > > NEWS | 2 ++ > > > > controller/route-exchange.c | 9 +++++---- > > > > controller/route.c | 12 ++++++++++-- > > > > controller/route.h | 2 ++ > > > > northd/en-datapath-logical-router.c | 7 +++++++ > > > > ovn-nb.xml | 29 > +++++++++++++++++++++++++++-- > > > > tests/multinode-bgp-macros.at | 2 +- > > > > tests/system-ovn.at | 22 +++++++++++----------- > > > > 8 files changed, 65 insertions(+), 20 deletions(-) > > > > > > > > diff --git a/NEWS b/NEWS > > > > index 06cc18b41..1e2abfa1c 100644 > > > > --- a/NEWS > > > > +++ b/NEWS > > > > @@ -17,6 +17,8 @@ Post v25.09.0 > > > > * Add the "other_config:dynamic-routing-redistribute=ip" to > Logical > > > > Switches to announce local IPs/MAC bindings belonging to the > same > > > > EVPN domain. > > > > + * Add the option "dynamic-routing-vrf-id" to Logical Routers > which > > > allows > > > > + CMS to specify the table_id for given vrf. > > > > - Add support for Network Function insertion in OVN with stateful > > > traffic > > > > redirection capability in Logical Switch datapath. The feature > > > introduces > > > > three new NB database tables: > > > > diff --git a/controller/route-exchange.c > b/controller/route-exchange.c > > > > index 161385aa4..7eb9c8c31 100644 > > > > --- a/controller/route-exchange.c > > > > +++ b/controller/route-exchange.c > > > > @@ -19,6 +19,7 @@ > > > > > > > > #include <errno.h> > > > > #include <net/if.h> > > > > +#include <linux/rtnetlink.h> > > > > #include <stdbool.h> > > > > > > > > #include "openvswitch/poll-loop.h" > > > > @@ -242,7 +243,7 @@ route_exchange_run(const struct > > > route_exchange_ctx_in *r_ctx_in, > > > > CLEAR_ROUTE_EXCHANGE_NL_STATUS(); > > > > const struct advertise_datapath_entry *ad; > > > > HMAP_FOR_EACH (ad, node, r_ctx_in->announce_routes) { > > > > - uint32_t table_id = ad->db->tunnel_key; > > > > + uint32_t table_id = route_get_table_id(ad->db); > > > > > > We previously could rely on the tunnel_key of the datapath_binding to > be > > > globally unique and therefor we would have a unique table_id. > > > However once users can configure these ids we might have duplicates. > > > > > > As the VRF is in the end just backed by a routing table with this ID > > > having a duplicate would mean we will fight against ourselfes to > > > synchronize these VRFs. Routes added/removed in one of them would be > > > added or removed in another vrf with the same ID as well. > > > > > > I would propose we validate that the table_id is unique here and skip > > > all cases where it occurs multiple times. > > > > > > > Yes the uniqueness is causing issues when you want to monitor the > > same vrf table_id from multiple LRs at once. It's arguable that setting > > router to the same id when those routers are on the same chassis is > > misconfiguration. But I wonder if we should enforce it, afterall it's > just > > optional configuration. It will still use tunnel_key by default. I > probably > > don't have a strong opinion and if you feel like we should forbid two > > routers with the table_id on single chassis I can add some sort of a > check > > here. > > Hi Ales, > > it definately is a misconfiguration if that ever happens. > > I am also uncertain of what to do with it. Even if we detect the problem > we can not easily choose which of the multiple users is the "correct" > one. The most ideal result we could probably hope for is to keep the > previously already used table, so that at least what had worked > previously would continue to work. > > My feeling is that this might be more complex than it would need to be. > Especially since in such a situation probably the outside frr or so > might have issues with this as well, so we would not be the only one > that needs to do something. > > I guess the most helpful thing would be to detect the situation and just > create some kind of log message. Then at least if there is some issue > that arrises then a user can easily see that something is wrong. > > Would that be fine for you? > Please check out v2 if that works for you. But is essentially logging and skipping the "second", that is random, I don't know if we can do better than that without extra code. But considering we log it the user should notice something is wrong and fix that anyway. > > Thanks, > Felix > > > > > > > > > > > Thanks, > > > Felix > > > > > > > > > > > if (ad->maintain_vrf) { > > > > if (!sset_contains(&old_maintained_vrfs, ad->vrf_name)) > { > > > > @@ -250,8 +251,8 @@ route_exchange_run(const struct > > > route_exchange_ctx_in *r_ctx_in, > > > > if (error && error != EEXIST) { > > > > VLOG_WARN_RL(&rl, > > > > "Unable to create VRF %s for > datapath " > > > > - "%"PRIi32": %s.", > > > > - ad->vrf_name, table_id, > > > > + UUID_FMT": %s.", ad->vrf_name, > > > > + UUID_ARGS(&ad->db->header_.uuid), > > > > ovs_strerror(error)); > > > > SET_ROUTE_EXCHANGE_NL_STATUS(error); > > > > continue; > > > > @@ -270,7 +271,7 @@ route_exchange_run(const struct > > > route_exchange_ctx_in *r_ctx_in, > > > > struct vector received_routes = > > > > VECTOR_EMPTY_INITIALIZER(struct > re_nl_received_route_node); > > > > > > > > - error = re_nl_sync_routes(ad->db->tunnel_key, &ad->routes, > > > > + error = re_nl_sync_routes(table_id, &ad->routes, > > > > &received_routes, ad->db); > > > > SET_ROUTE_EXCHANGE_NL_STATUS(error); > > > > > > > > diff --git a/controller/route.c b/controller/route.c > > > > index db4aa4122..1a4d18f48 100644 > > > > --- a/controller/route.c > > > > +++ b/controller/route.c > > > > @@ -218,8 +218,8 @@ route_run(struct route_ctx_in *r_ctx_in, > > > > if (vrf_name) { > > > > memcpy(ad->vrf_name, vrf_name, strlen(vrf_name) + > 1); > > > > } else { > > > > - snprintf(ad->vrf_name, sizeof ad->vrf_name, > > > "ovnvrf%"PRIi64, > > > > - ad->db->tunnel_key); > > > > + snprintf(ad->vrf_name, sizeof ad->vrf_name, > > > "ovnvrf%"PRIu32, > > > > + route_get_table_id(ad->db)); > > > > } > > > > > > > > if (!port_name) { > > > > @@ -323,3 +323,11 @@ route_cleanup(struct hmap *announce_routes) > > > > advertise_datapath_cleanup(ad); > > > > } > > > > } > > > > + > > > > +uint32_t > > > > +route_get_table_id(const struct sbrec_datapath_binding *dp) > > > > +{ > > > > + int64_t vrf_id = ovn_smap_get_llong(&dp->external_ids, > > > > + "dynamic-routing-vrf-id", -1); > > > > + return (vrf_id >= 1 && vrf_id <= UINT32_MAX) ? vrf_id : > > > dp->tunnel_key; > > > > +} > > > > diff --git a/controller/route.h b/controller/route.h > > > > index c2c92e70b..f6cb5993c 100644 > > > > --- a/controller/route.h > > > > +++ b/controller/route.h > > > > @@ -29,6 +29,7 @@ struct hmap; > > > > struct ovsdb_idl_index; > > > > struct sbrec_chassis; > > > > struct sbrec_port_binding; > > > > +struct sbrec_datapath_binding; > > > > > > > > struct route_ctx_in { > > > > const struct sbrec_advertised_route_table > *advertised_route_table; > > > > @@ -85,5 +86,6 @@ const struct sbrec_port_binding > > > *route_exchange_find_port( > > > > uint32_t advertise_route_hash(const struct in6_addr *dst, unsigned > int > > > plen); > > > > void route_run(struct route_ctx_in *, struct route_ctx_out *); > > > > void route_cleanup(struct hmap *announce_routes); > > > > +uint32_t route_get_table_id(const struct sbrec_datapath_binding > *dp); > > > > > > > > #endif /* ROUTE_H */ > > > > diff --git a/northd/en-datapath-logical-router.c > > > b/northd/en-datapath-logical-router.c > > > > index f6f49422f..4625edfd0 100644 > > > > --- a/northd/en-datapath-logical-router.c > > > > +++ b/northd/en-datapath-logical-router.c > > > > @@ -88,6 +88,13 @@ gather_external_ids(const struct > nbrec_logical_router > > > *nbr, > > > > smap_add_format(external_ids, "disable_garp_rarp", > > > > disable_garp_rarp ? "true" : "false"); > > > > > > > > + int64_t vrf_id = ovn_smap_get_llong(&nbr->options, > > > > + "dynamic-routing-vrf-id", > -1); > > > > + if (vrf_id > 0) { > > > > + smap_add_format(external_ids, "dynamic-routing-vrf-id", > > > "%"PRId64, > > > > + vrf_id); > > > > + } > > > > + > > > > /* For backwards-compatibility, also store the NB UUID in > > > > * external-ids:logical-router. This is useful if ovn-controller > > > > * has not updated and expects this to be where to find the > > > > diff --git a/ovn-nb.xml b/ovn-nb.xml > > > > index 36d7accaf..f4b47f213 100644 > > > > --- a/ovn-nb.xml > > > > +++ b/ovn-nb.xml > > > > @@ -3383,7 +3383,9 @@ or > > > > <p> > > > > This defines the name of the vrf the ovn-controller will > use > > > to > > > > advertise and learn routes. If not set the vrf will be > named > > > "ovnvrf" > > > > - with the datapath id of the Logical Router appended to it. > > > > + with the <ref column="options" > key="dynamic-routing-vrf-id" > > > > + table="Logical_Router"/> or datapath id of the Logical > Router > > > > + appended to it. > > > > </p> > > > > > > > > <p> > > > > @@ -3398,6 +3400,27 @@ or > > > > </p> > > > > </column> > > > > > > > > + <column name="options" key="dynamic-routing-vrf-id" > > > > + type='{"type": "integer", "minInteger": 1, > > > > + "maxInteger": 4294967295}'> > > > > + <p> > > > > + Only relevant if <ref column="options" > key="dynamic-routing"/> > > > > + is set to <code>true</code>. > > > > + </p> > > > > + > > > > + <p> > > > > + This defines the vrf table id used by the specified vrf. > When > > > not > > > > + set or set to invalid value datapath id of the Logical > Router > > > > + will be used instead. > > > > + </p> > > > > + > > > > + <p> > > > > + This can also affect the vrf name. For details see > > > > + <ref column="options" key="dynamic-routing-vrf-name" > > > > + table="Logical_Router"/> on the Logical_Router. > > > > + </p> > > > > + </column> > > > > + > > > > <column name="options" key="ct-commit-all" type='{"type": > > > "boolean"}'> > > > > When enabled the LR will commit traffic in a zone that is > > > stateful. > > > > The traffic is not commited to both zones but it is > selective > > > based > > > > @@ -4416,7 +4439,9 @@ or > > > > this LRP. > > > > Unless <ref column="options" > key="dynamic-routing-vrf-name" > > > > table="Logical_Router"/> is set the vrf will be named > > > "ovnvrf" with > > > > - the datapath id of the Logical Router appended to it. > > > > + the <ref column="options" key="dynamic-routing-vrf-id" > > > > + table="Logical_Router"/> or datapath id of the Logical > Router > > > > + appended to it. > > > > </p> > > > > > > > > <p> > > > > diff --git a/tests/multinode-bgp-macros.at b/tests/ > > > multinode-bgp-macros.at > > > > index fb0906005..2ea9136c7 100644 > > > > --- a/tests/multinode-bgp-macros.at > > > > +++ b/tests/multinode-bgp-macros.at > > > > @@ -275,7 +275,7 @@ m_setup_ovn_frr_router() { > > > > > > > > check multinode_nbctl lr-add $lr > > > > check multinode_nbctl set Logical_Router $lr > options:chassis=$node \ > > > > - options:dynamic-routing=true options:requested-tnl-key=$vni > > > > + options:dynamic-routing=true > options:dynamic-routing-vrf-id=$vni > > > > > > > > check multinode_nbctl lrp-add $lr $lrp $bgp_mac > > > > check multinode_nbctl lrp-set-options $lrp \ > > > > diff --git a/tests/system-ovn.at b/tests/system-ovn.at > > > > index 2b880ec37..98dcc60e9 100644 > > > > --- a/tests/system-ovn.at > > > > +++ b/tests/system-ovn.at > > > > @@ -15821,7 +15821,7 @@ check ovn-nbctl ls-add p2 > > > > > > > > check ovn-nbctl lr-add internet \ > > > > -- set Logical_Router internet options:dynamic-routing=true \ > > > > - options:requested-tnl-key=1337 > > > > + > options:dynamic-routing-vrf-id=1337 > > > > > > > > check ovn-nbctl lrp-add internet internet-public \ > > > > 00:00:02:01:02:03 192.0.2.1/24 \ > > > > @@ -15845,7 +15845,7 @@ check ovn-nbctl lsp-add phys phys-internet \ > > > > # LR pr1 setup. > > > > > > > > check ovn-nbctl lr-add pr1 \ > > > > - -- set Logical_Router pr1 options:requested-tnl-key=1338 > > > > + -- set Logical_Router pr1 options:dynamic-routing-vrf-id=1338 > > > > > > > > check ovn-nbctl lrp-add pr1 pr1-public \ > > > > 00:00:02:01:02:04 192.0.2.2/24 > > > > @@ -15867,7 +15867,7 @@ check ovn-nbctl lr-route-add pr1 0.0.0.0/0 > > > 192.0.2.1 > > > > # LR pr2 setup. > > > > > > > > check ovn-nbctl lr-add pr2 \ > > > > - -- set Logical_Router pr2 options:requested-tnl-key=1339 > > > > + -- set Logical_Router pr2 options:dynamic-routing-vrf-id=1339 > > > > > > > > check ovn-nbctl lrp-add pr2 pr2-public \ > > > > 00:00:02:01:02:05 192.0.2.3/24 > > > > @@ -16177,7 +16177,7 @@ check ovn-nbctl ls-add p2 > > > > > > > > check ovn-nbctl lr-add internet \ > > > > -- set Logical_Router internet options:dynamic-routing=true \ > > > > - options:requested-tnl-key=1337 \ > > > > + > options:dynamic-routing-vrf-id=1337 \ > > > > options:chassis=hv1 > > > > > > > > check ovn-nbctl lrp-add internet internet-public \ > > > > @@ -16201,7 +16201,7 @@ check ovn-nbctl lsp-add phys phys-internet \ > > > > # LR pr1 setup. > > > > > > > > check ovn-nbctl lr-add pr1 \ > > > > - -- set Logical_Router pr1 options:requested-tnl-key=1338 > > > > + -- set Logical_Router pr1 options:dynamic-routing-vrf-id=1338 > > > > > > > > check ovn-nbctl lrp-add pr1 pr1-public \ > > > > 00:00:02:01:02:04 192.0.2.2/24 > > > > @@ -16223,7 +16223,7 @@ check ovn-nbctl lr-route-add pr1 0.0.0.0/0 > > > 192.0.2.1 > > > > # LR pr2 setup. > > > > > > > > check ovn-nbctl lr-add pr2 \ > > > > - -- set Logical_Router pr2 options:requested-tnl-key=1339 > > > > + -- set Logical_Router pr2 options:dynamic-routing-vrf-id=1339 > > > > > > > > check ovn-nbctl lrp-add pr2 pr2-public \ > > > > 00:00:02:01:02:05 192.0.2.3/24 > > > > @@ -16484,7 +16484,7 @@ OVS_WAIT_WHILE([ip link | grep -q > > > ovnvrf$vrf:.*UP]) > > > > check ovn-nbctl \ > > > > -- lr-add R1 \ > > > > -- set Logical_Router R1 \ > > > > - options:requested-tnl-key=$vrf \ > > > > + options:dynamic-routing-vrf-id=$vrf \ > > > > options:dynamic-routing=true \ > > > > -- ls-add sw0 \ > > > > -- ls-add public \ > > > > @@ -16642,7 +16642,7 @@ OVS_WAIT_WHILE([ip link | grep -q > > > ovnvrf$vrf:.*UP]) > > > > check ovn-nbctl \ > > > > -- lr-add R1 \ > > > > -- set Logical_Router R1 \ > > > > - options:requested-tnl-key=$vrf \ > > > > + options:dynamic-routing-vrf-id=$vrf \ > > > > options:dynamic-routing=true \ > > > > -- ls-add sw0 \ > > > > -- ls-add public \ > > > > @@ -16802,7 +16802,7 @@ OVS_WAIT_WHILE([ip link | grep -q > > > ovnvrf$vrf:.*UP]) > > > > check ovn-nbctl \ > > > > -- lr-add R1 \ > > > > -- set Logical_Router R1 \ > > > > - options:requested-tnl-key=$vrf \ > > > > + options:dynamic-routing-vrf-id=$vrf \ > > > > options:dynamic-routing=true \ > > > > -- ls-add sw0 \ > > > > -- ls-add public \ > > > > @@ -17032,7 +17032,7 @@ OVS_WAIT_WHILE([ip link | grep -q > > > ovnvrf$vrf:.*UP]) > > > > check ovn-nbctl \ > > > > -- lr-add R1 \ > > > > -- set Logical_Router R1 \ > > > > - options:requested-tnl-key=$vrf \ > > > > + options:dynamic-routing-vrf-id=$vrf \ > > > > options:dynamic-routing=true \ > > > > -- ls-add sw0 \ > > > > -- ls-add public \ > > > > @@ -17297,7 +17297,7 @@ check ovn-nbctl > > > \ > > > > check ovn-nbctl \ > > > > -- lr-add R2 \ > > > > -- set Logical_Router R2 \ > > > > - options:requested-tnl-key=$vrf \ > > > > + options:dynamic-routing-vrf-id=$vrf \ > > > > options:dynamic-routing=true \ > > > > options:dynamic-routing-redistribute=lb,nat \ > > > > -- lrp-add R2 R2-PUB1 00:02:02:01:02:03 172.16.1.2/24 \ > > > > -- > > > > 2.51.0 > > > > > > > > _______________________________________________ > > > > dev mailing list > > > > [email protected] > > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > > > > > Regards, > > Ales > > _______________________________________________ > > dev mailing list > > [email protected] > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > Regards, Ales _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
