Re: [ovs-dev] [PATCH ovn 2/2] ovn-ic: support learning routes in same AZ
Bleep bloop. Greetings Maxim Korezkij, I am a robot and I have tried out your patch. Thanks for your contribution. I encountered some error that I wasn't expecting. See the details below. checkpatch: ERROR: Author should not be also be co-author. Lines checked: 228, Warnings: 0, Errors: 1 Please check this out. If you feel there has been an error, please email acon...@redhat.com Thanks, 0-day Robot ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn 2/2] ovn-ic: support learning routes in same AZ
Bleep bloop. Greetings maximkorezkij, I am a robot and I have tried out your patch. Thanks for your contribution. I encountered some error that I wasn't expecting. See the details below. checkpatch: ERROR: Author maximkorezkij needs to sign off. WARNING: Unexpected sign-offs from developers who are not authors or co-authors or committers: Maxim Korezkij , Felix Huettner Lines checked: 225, Warnings: 1, Errors: 1 Please check this out. If you feel there has been an error, please email acon...@redhat.com Thanks, 0-day Robot ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn 2/2] ovn-ic: support learning routes in same AZ
Looks good to me.
Acked-by: Mark Michelson
Having said that, I'd like for some OVN devs that are more familiar with
the ins and outs of ovn-ic to make the determination if this is behavior
that we want or not. For non-IC OVN, routers learn routes to their
neighbor routers automatically, unless the "dynamic_neigh_routers"
option is enabled. For ovn-ic, do we have a way to ensure that routers
do not learn neighbor routes if they do not want to?
On 8/1/23 03:22, maximkorezkij via dev wrote:
when connecting multiple logical routers to a transit switch per az then
previously the routers in the same az would not learn each others
routes while the routers in the others az would learn all of them.
As this is confusing and would require each user to have additional
logical that configures static routing within each az.
Signed-off-by: Maxim Korezkij
Signed-off-by: Felix Huettner
---
ic/ovn-ic.c | 52 -
tests/ovn-ic.at | 2 ++
2 files changed, 40 insertions(+), 14 deletions(-)
diff --git a/ic/ovn-ic.c b/ic/ovn-ic.c
index 49850954f..cea14d008 100644
--- a/ic/ovn-ic.c
+++ b/ic/ovn-ic.c
@@ -861,6 +861,8 @@ struct ic_route_info {
const char *origin;
const char *route_table;
+const struct nbrec_logical_router *nb_lr;
+
/* Either nb_route or nb_lrp is set and the other one must be NULL.
* - For a route that is learned from IC-SB, or a static route that is
* generated from a route that is configured in NB, the "nb_route"
@@ -937,7 +939,8 @@ parse_route(const char *s_prefix, const char *s_nexthop,
/* Return false if can't be added due to bad format. */
static bool
add_to_routes_learned(struct hmap *routes_learned,
- const struct nbrec_logical_router_static_route *nb_route)
+ const struct nbrec_logical_router_static_route *nb_route,
+ const struct nbrec_logical_router *nb_lr)
{
struct in6_addr prefix, nexthop;
unsigned int plen;
@@ -959,6 +962,7 @@ add_to_routes_learned(struct hmap *routes_learned,
ic_route->nb_route = nb_route;
ic_route->origin = origin;
ic_route->route_table = nb_route->route_table;
+ic_route->nb_lr = nb_lr;
hmap_insert(routes_learned, &ic_route->node,
ic_route_hash(&prefix, plen, &nexthop, origin,
nb_route->route_table));
@@ -1099,7 +1103,8 @@ add_to_routes_ad(struct hmap *routes_ad, const struct
in6_addr prefix,
unsigned int plen, const struct in6_addr nexthop,
const char *origin, const char *route_table,
const struct nbrec_logical_router_port *nb_lrp,
- const struct nbrec_logical_router_static_route *nb_route)
+ const struct nbrec_logical_router_static_route *nb_route,
+ const struct nbrec_logical_router *nb_lr)
{
if (route_table == NULL) {
route_table = "";
@@ -1117,6 +1122,7 @@ add_to_routes_ad(struct hmap *routes_ad, const struct
in6_addr prefix,
ic_route->origin = origin;
ic_route->route_table = route_table;
ic_route->nb_lrp = nb_lrp;
+ic_route->nb_lr = nb_lr;
hmap_insert(routes_ad, &ic_route->node, hash);
} else {
static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
@@ -1130,6 +1136,7 @@ static void
add_static_to_routes_ad(
struct hmap *routes_ad,
const struct nbrec_logical_router_static_route *nb_route,
+const struct nbrec_logical_router *nb_lr,
const struct lport_addresses *nexthop_addresses,
const struct smap *nb_options)
{
@@ -1172,14 +1179,15 @@ add_static_to_routes_ad(
}
add_to_routes_ad(routes_ad, prefix, plen, nexthop, ROUTE_ORIGIN_STATIC,
- nb_route->route_table, NULL, nb_route);
+ nb_route->route_table, NULL, nb_route, nb_lr);
}
static void
add_network_to_routes_ad(struct hmap *routes_ad, const char *network,
const struct nbrec_logical_router_port *nb_lrp,
const struct lport_addresses *nexthop_addresses,
- const struct smap *nb_options)
+ const struct smap *nb_options,
+ const struct nbrec_logical_router *nb_lr)
{
struct in6_addr prefix, nexthop;
unsigned int plen;
@@ -1218,7 +1226,7 @@ add_network_to_routes_ad(struct hmap *routes_ad, const
char *network,
/* directly-connected routes go to route table */
add_to_routes_ad(routes_ad, prefix, plen, nexthop, ROUTE_ORIGIN_CONNECTED,
- NULL, nb_lrp, NULL);
+ NULL, nb_lrp, NULL, nb_lr);
}
static bool
@@ -1322,7 +1330,6 @@ lrp_is_ts_port(struct ic_context *ctx, struct
ic_router_info *ic_lr,
static void
sync_learned_routes(struct ic_context *ctx,
-const struct icsbrec_avai
Re: [ovs-dev] [PATCH ovn 2/2] ovn-ic: support learning routes in same AZ
Thank you for the feedback,
the route learning feature needs to be explicitly enabled [1]. This is
currently a global setting, so if we want to limit this to individual
routers i would rather make this a general ovn-ic feature than to
include this here.
Thanks
Felix
[1]
https://docs.ovn.org/en/latest/tutorials/ovn-interconnection.html#route-advertisement
On Wed, Aug 02, 2023 at 03:13:01PM -0400, Mark Michelson wrote:
> Looks good to me.
>
> Acked-by: Mark Michelson
>
> Having said that, I'd like for some OVN devs that are more familiar with the
> ins and outs of ovn-ic to make the determination if this is behavior that we
> want or not. For non-IC OVN, routers learn routes to their neighbor routers
> automatically, unless the "dynamic_neigh_routers" option is enabled. For
> ovn-ic, do we have a way to ensure that routers do not learn neighbor routes
> if they do not want to?
>
> On 8/1/23 03:22, maximkorezkij via dev wrote:
> > when connecting multiple logical routers to a transit switch per az then
> > previously the routers in the same az would not learn each others
> > routes while the routers in the others az would learn all of them.
> >
> > As this is confusing and would require each user to have additional
> > logical that configures static routing within each az.
> >
> > Signed-off-by: Maxim Korezkij
> > Signed-off-by: Felix Huettner
> > ---
> > ic/ovn-ic.c | 52 -
> > tests/ovn-ic.at | 2 ++
> > 2 files changed, 40 insertions(+), 14 deletions(-)
> >
> > diff --git a/ic/ovn-ic.c b/ic/ovn-ic.c
> > index 49850954f..cea14d008 100644
> > --- a/ic/ovn-ic.c
> > +++ b/ic/ovn-ic.c
> > @@ -861,6 +861,8 @@ struct ic_route_info {
> > const char *origin;
> > const char *route_table;
> >
> > +const struct nbrec_logical_router *nb_lr;
> > +
> > /* Either nb_route or nb_lrp is set and the other one must be NULL.
> >* - For a route that is learned from IC-SB, or a static route that is
> >* generated from a route that is configured in NB, the "nb_route"
> > @@ -937,7 +939,8 @@ parse_route(const char *s_prefix, const char *s_nexthop,
> > /* Return false if can't be added due to bad format. */
> > static bool
> > add_to_routes_learned(struct hmap *routes_learned,
> > - const struct nbrec_logical_router_static_route
> > *nb_route)
> > + const struct nbrec_logical_router_static_route
> > *nb_route,
> > + const struct nbrec_logical_router *nb_lr)
> > {
> > struct in6_addr prefix, nexthop;
> > unsigned int plen;
> > @@ -959,6 +962,7 @@ add_to_routes_learned(struct hmap *routes_learned,
> > ic_route->nb_route = nb_route;
> > ic_route->origin = origin;
> > ic_route->route_table = nb_route->route_table;
> > +ic_route->nb_lr = nb_lr;
> > hmap_insert(routes_learned, &ic_route->node,
> > ic_route_hash(&prefix, plen, &nexthop, origin,
> > nb_route->route_table));
> > @@ -1099,7 +1103,8 @@ add_to_routes_ad(struct hmap *routes_ad, const struct
> > in6_addr prefix,
> >unsigned int plen, const struct in6_addr nexthop,
> >const char *origin, const char *route_table,
> >const struct nbrec_logical_router_port *nb_lrp,
> > - const struct nbrec_logical_router_static_route *nb_route)
> > + const struct nbrec_logical_router_static_route *nb_route,
> > + const struct nbrec_logical_router *nb_lr)
> > {
> > if (route_table == NULL) {
> > route_table = "";
> > @@ -1117,6 +1122,7 @@ add_to_routes_ad(struct hmap *routes_ad, const struct
> > in6_addr prefix,
> > ic_route->origin = origin;
> > ic_route->route_table = route_table;
> > ic_route->nb_lrp = nb_lrp;
> > +ic_route->nb_lr = nb_lr;
> > hmap_insert(routes_ad, &ic_route->node, hash);
> > } else {
> > static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
> > @@ -1130,6 +1136,7 @@ static void
> > add_static_to_routes_ad(
> > struct hmap *routes_ad,
> > const struct nbrec_logical_router_static_route *nb_route,
> > +const struct nbrec_logical_router *nb_lr,
> > const struct lport_addresses *nexthop_addresses,
> > const struct smap *nb_options)
> > {
> > @@ -1172,14 +1179,15 @@ add_static_to_routes_ad(
> > }
> >
> > add_to_routes_ad(routes_ad, prefix, plen, nexthop,
> > ROUTE_ORIGIN_STATIC,
> > - nb_route->route_table, NULL, nb_route);
> > + nb_route->route_table, NULL, nb_route, nb_lr);
> > }
> >
> > static void
> > add_network_to_routes_ad(struct hmap *routes_ad, const char *network,
> >const struct nbrec_logical_router_port *nb_lrp,
> >const struct lport_addresses *nexthop_addresses,
> > -
