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?
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
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
