On Mon, Feb 03, 2025 at 04:29:25PM +0100, Dumitru Ceara wrote:
> On 1/29/25 12:16 PM, Felix Huettner via dev wrote:
> > Previously vrf names where generated using "ovnvrf" + datapath id.
> > Now the controller supports the dynamic-routing-vrf-name setting to
> > overwrite this to whatever value the user desires.
> > Note that the vrf ID is still the datapath id.
> >
> > Signed-off-by: Felix Huettner <felix.huettner@stackit.cloud>
> > ---
>
> Hi Felix,
Hi Dumitru,
thanks a lot for the review.
The smaller stuff will all be fixed in the next version.
>
> > controller/route-exchange.c | 14 ++++++--------
> > controller/route.c | 15 +++++++++++++++
> > controller/route.h | 2 ++
> > tests/system-ovn.at | 15 ++++++++++++++-
> > 4 files changed, 37 insertions(+), 9 deletions(-)
> >
> > diff --git a/controller/route-exchange.c b/controller/route-exchange.c
> > index 47ff2350e..79e65280b 100644
> > --- a/controller/route-exchange.c
> > +++ b/controller/route-exchange.c
> > @@ -221,27 +221,25 @@ route_exchange_run(struct route_exchange_ctx_in
> > *r_ctx_in,
> > const struct advertise_datapath_entry *ad;
> > HMAP_FOR_EACH (ad, node, r_ctx_in->announce_routes) {
> > uint32_t table_id = ad->db->tunnel_key;
> > - char vrf_name[IFNAMSIZ + 1];
> > - snprintf(vrf_name, sizeof vrf_name, "ovnvrf%"PRIi32, table_id);
> >
> > if (ad->maintain_vrf) {
> > - if (!sset_contains(&old_maintained_vrfs, vrf_name)) {
> > - int error = re_nl_create_vrf(vrf_name, table_id);
> > + if (!sset_contains(&old_maintained_vrfs, ad->vrf_name)) {
> > + int error = re_nl_create_vrf(ad->vrf_name, table_id);
> > if (error && error != EEXIST) {
> > VLOG_WARN_RL(&rl,
> > "Unable to create VRF %s for datapath "
> > "%"PRIi32": %s.",
> > - vrf_name, table_id,
> > + ad->vrf_name, table_id,
> > ovs_strerror(error));
> > continue;
> > }
> > }
> > - sset_add(&_maintained_vrfs, vrf_name);
> > + sset_add(&_maintained_vrfs, ad->vrf_name);
> > } else {
> > /* a previous maintain-vrf flag was removed. We should therfor
> > * also not delete it even if we created it previously. */
> > - sset_find_and_delete(&_maintained_vrfs, vrf_name);
> > - sset_find_and_delete(&old_maintained_vrfs, vrf_name);
> > + sset_find_and_delete(&_maintained_vrfs, ad->vrf_name);
> > + sset_find_and_delete(&old_maintained_vrfs, ad->vrf_name);
> > }
> >
> > maintained_route_table_add(table_id);
> > diff --git a/controller/route.c b/controller/route.c
> > index 2d86edc97..d513b449e 100644
> > --- a/controller/route.c
> > +++ b/controller/route.c
> > @@ -143,6 +143,21 @@ route_run(struct route_ctx_in *r_ctx_in,
> > char *ifname = nullable_xstrdup(
> > smap_get(&repb->options,
> > "dynamic-routing-ifname"));
> > +
> > + const char *vrf_name = smap_get(&repb->options,
> > + "dynamic-routing-vrf-name");
>
> Indentation is a bit off.
>
> > + if (vrf_name && strlen(vrf_name) >= sizeof ad->vrf_name) {
> > + VLOG_WARN("Ignoring vrf name %s, since it is too long",
> > + vrf_name);
>
> Maybe we should log the maximum here? Also, please rate limit this log.
>
> > + vrf_name = NULL;
> > + }
> > + 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);
> > + }
> > +
> > smap_add_nocopy(&ad->bound_ports,
> > xstrdup(local_peer->logical_port), ifname);
> > }
> > diff --git a/controller/route.h b/controller/route.h
> > index 986c35ec0..1afaf0ef6 100644
> > --- a/controller/route.h
> > +++ b/controller/route.h
> > @@ -20,6 +20,7 @@
> >
> > #include <stdbool.h>
> > #include <netinet/in.h>
> > +#include <net/if.h>
> > #include "openvswitch/hmap.h"
> > #include "sset.h"
> > #include "smap.h"
> > @@ -55,6 +56,7 @@ struct advertise_datapath_entry {
> > struct hmap_node node;
> > const struct sbrec_datapath_binding *db;
> > bool maintain_vrf;
> > + char vrf_name[IFNAMSIZ + 1];
> > struct hmap routes;
> >
> > /* The name of the port bindings locally bound for this datapath and
> > diff --git a/tests/system-ovn.at b/tests/system-ovn.at
> > index 963a67e0c..12985edd2 100644
> > --- a/tests/system-ovn.at
> > +++ b/tests/system-ovn.at
> > @@ -14867,6 +14867,7 @@ OVN_FOR_EACH_NORTHD([
> > AT_SETUP([dynamic-routing - DGP])
> >
> > VRF_RESERVE([1337])
> > +VRF_RESERVE([1338])
> >
> > # This test uses dynamic routing on a simulated multi-tenant internet
> > # connection.
> > @@ -15113,10 +15114,22 @@ blackhole 192.0.2.10 proto 84 metric 100
> > blackhole 198.51.100.0/24 proto 84 metric 1000
> > 233.252.0.0/24 via 192.168.10.10 dev lo onlink])
> >
> > +# Changing the vrf name will switch to the new one.
> > +check ovn-nbctl --wait=hv set Logical_Router internet
> > options:dynamic-routing-vrf-name=ovnvrf1338
>
> I had mentioned the misconfig of the dynamic-routing-vrf-name in my
> reply to this patch last week, mentioning it again so the review is
> consolidated. :)
Ah yea i missed that when sending the series. I already changed it
locally, but did not send a new northd version.
I'll send the next patchset as one large one. That should solve the
issue.
Thanks a lot,
Felix
>
> > +check ovn-nbctl --wait=hv set Logical_Router_Port internet-phys \
> > + options:dynamic-routing-maintain-vrf=true
> > +OVS_WAIT_UNTIL_EQUAL([ip route list vrf ovnvrf1338 | awk '{$1=$1};1'], [dnl
> > +blackhole 192.0.2.1 proto 84 metric 1000
> > +blackhole 192.0.2.2 proto 84 metric 100
> > +blackhole 192.0.2.3 proto 84 metric 100
> > +blackhole 192.0.2.10 proto 84 metric 100
> > +blackhole 198.51.100.0/24 proto 84 metric 1000
> > +233.252.0.0/24 via 192.168.10.10 dev lo onlink])
> > +
> > # Stoping with --restart will not touch the routes.
> > check ovn-appctl -t ovn-controller exit --restart
> > OVS_WAIT_UNTIL([test "$(ovn-appctl -t ovn-controller debug/status)" !=
> > "running"])
> > -OVS_WAIT_UNTIL_EQUAL([ip route list vrf ovnvrf1337 | awk '{$1=$1};1'], [dnl
> > +OVS_WAIT_UNTIL_EQUAL([ip route list vrf ovnvrf1338 | awk '{$1=$1};1'], [dnl
> > blackhole 192.0.2.1 proto 84 metric 1000
> > blackhole 192.0.2.2 proto 84 metric 100
> > blackhole 192.0.2.3 proto 84 metric 100
>
> Regards,
> Dumitru
>
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
