> > Hi Lorenzo, a few comments down below. Hi Mark,
thx for review. I will fix them in v2. Regards, Lorenzo > > On 2/3/21 10:54 AM, Lorenzo Bianconi wrote: > > In the current ovn-nbctl lr-route-add implementation is possible to add > > multiple duplicated routes adding --ecmp or --ecmp-symmetric-reply > > option, e.g: > > > > $ovn-nbctl --ecmp-symmetric-reply lr-route-add lr0 10.244.0.7/32 172.18.0.4 > > $ovn-nbctl --ecmp-symmetric-reply lr-route-add lr0 10.244.0.7/32 172.18.0.4 > > $ovn-nbctl --ecmp-symmetric-reply lr-route-add lr0 10.244.0.7/32 172.18.0.4 > > > > https://bugzilla.redhat.com/show_bug.cgi?id=1916842 > > > > Fix the issue checking nexthop for ECMP routes. > > > > Signed-off-by: Lorenzo Bianconi <[email protected]> > > --- > > tests/ovn-nbctl.at | 19 +++++----- > > utilities/ovn-nbctl.c | 81 +++++++++++++++++++++++++++---------------- > > 2 files changed, 63 insertions(+), 37 deletions(-) > > > > diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at > > index e4a2a9695..6d91aa4c5 100644 > > --- a/tests/ovn-nbctl.at > > +++ b/tests/ovn-nbctl.at > > @@ -1539,34 +1539,34 @@ AT_CHECK([ovn-nbctl lr-route-list lr0], [0], [dnl > > dnl Add ecmp routes > > AT_CHECK([ovn-nbctl lr-route-add lr0 10.0.0.0/24 11.0.0.1]) > > AT_CHECK([ovn-nbctl --ecmp lr-route-add lr0 10.0.0.0/24 11.0.0.2]) > > -AT_CHECK([ovn-nbctl --ecmp lr-route-add lr0 10.0.0.0/24 11.0.0.2]) > > AT_CHECK([ovn-nbctl --ecmp lr-route-add lr0 10.0.0.0/24 11.0.0.3]) > > -AT_CHECK([ovn-nbctl --ecmp lr-route-add lr0 10.0.0.0/24 11.0.0.3 lp0]) > > +AT_CHECK([ovn-nbctl --ecmp lr-route-add lr0 10.0.0.0/24 11.0.0.4 lp0]) > > AT_CHECK([ovn-nbctl lr-route-list lr0], [0], [dnl > > IPv4 Routes > > 10.0.0.0/24 11.0.0.1 dst-ip ecmp > > 10.0.0.0/24 11.0.0.2 dst-ip ecmp > > - 10.0.0.0/24 11.0.0.2 dst-ip ecmp > > 10.0.0.0/24 11.0.0.3 dst-ip ecmp > > - 10.0.0.0/24 11.0.0.3 dst-ip lp0 ecmp > > + 10.0.0.0/24 11.0.0.4 dst-ip lp0 ecmp > > +]) > > +AT_CHECK([ovn-nbctl --ecmp lr-route-add lr0 10.0.0.0/24 11.0.0.2], [1], [], > > + [ovn-nbctl: duplicate nexthop for the same ECMP route > > ]) > > > > dnl Delete ecmp routes > > AT_CHECK([ovn-nbctl lr-route-del lr0 10.0.0.0/24 11.0.0.1]) > > AT_CHECK([ovn-nbctl lr-route-list lr0], [0], [dnl > > IPv4 Routes > > - 10.0.0.0/24 11.0.0.2 dst-ip ecmp > > 10.0.0.0/24 11.0.0.2 dst-ip ecmp > > 10.0.0.0/24 11.0.0.3 dst-ip ecmp > > - 10.0.0.0/24 11.0.0.3 dst-ip lp0 ecmp > > + 10.0.0.0/24 11.0.0.4 dst-ip lp0 ecmp > > ]) > > AT_CHECK([ovn-nbctl lr-route-del lr0 10.0.0.0/24 11.0.0.2]) > > AT_CHECK([ovn-nbctl lr-route-list lr0], [0], [dnl > > IPv4 Routes > > 10.0.0.0/24 11.0.0.3 dst-ip ecmp > > - 10.0.0.0/24 11.0.0.3 dst-ip lp0 ecmp > > + 10.0.0.0/24 11.0.0.4 dst-ip lp0 ecmp > > ]) > > -AT_CHECK([ovn-nbctl lr-route-del lr0 10.0.0.0/24 11.0.0.3 lp0]) > > +AT_CHECK([ovn-nbctl lr-route-del lr0 10.0.0.0/24 11.0.0.4 lp0]) > > AT_CHECK([ovn-nbctl lr-route-list lr0], [0], [dnl > > IPv4 Routes > > 10.0.0.0/24 11.0.0.3 dst-ip > > @@ -1611,6 +1611,9 @@ AT_CHECK([ovn-nbctl --ecmp lr-route-add lr0 > > 2001:0db8:1::/64 2001:0db8:0:f103::3 > > AT_CHECK([ovn-nbctl --ecmp lr-route-add lr0 2001:0db8:1::/64 > > 2001:0db8:0:f103::4]) > > AT_CHECK([ovn-nbctl lr-route-add lr0 2002:0db8:1::/64 > > 2001:0db8:0:f103::5]) > > AT_CHECK([ovn-nbctl --ecmp-symmetric-reply lr-route-add lr0 > > 2003:0db8:1::/64 2001:0db8:0:f103::6]) > > +AT_CHECK([ovn-nbctl --ecmp-symmetric-reply lr-route-add lr0 > > 2003:0db8:1::/64 2001:0db8:0:f103::6], [1], [], > > + [ovn-nbctl: duplicate nexthop for the same ECMP route > > +]) > > > > AT_CHECK([ovn-nbctl lr-route-list lr0], [0], [dnl > > IPv4 Routes > > diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c > > index 3f28ba11a..07f2b008d 100644 > > --- a/utilities/ovn-nbctl.c > > +++ b/utilities/ovn-nbctl.c > > @@ -3917,6 +3917,48 @@ nbctl_lr_policy_list(struct ctl_context *ctx) > > } > > free(policies); > > } > > + > > +static struct nbrec_logical_router_static_route * > > +nbctl_lr_get_route_prefix(const struct nbrec_logical_router *lr, > > + char *prefix, char *next_hop, bool is_src_route, > > + bool ecmp) > > I think this function should be renamed to "nbctl_lr_get_route". The > return type is a static route, not a prefix. > > > +{ > > + for (int i = 0; i < lr->n_static_routes; i++) { > > + struct nbrec_logical_router_static_route *route = > > lr->static_routes[i]; > > + > > + /* Compare route policy. */ > > + char *nb_policy = route->policy; > > + bool nb_is_src_route = false; > > + if (nb_policy && !strcmp(nb_policy, "src-ip")) { > > + nb_is_src_route = true; > > + } > > + if (is_src_route != nb_is_src_route) { > > + continue; > > + } > > + > > + /* Compare route prefix. */ > > + char *rt_prefix = normalize_prefix_str(route->ip_prefix); > > + if (!rt_prefix) { > > + /* Ignore existing prefix we couldn't parse. */ > > + continue; > > + } > > + > > + if (strcmp(rt_prefix, prefix)) { > > + free(rt_prefix); > > + continue; > > + } > > + > > + if (ecmp && strcmp(next_hop, route->nexthop)) { > > + free(rt_prefix); > > + continue; > > + } > > + > > + free(rt_prefix); > > + return route; > > + } > > + return NULL; > > +} > > + > > > > static void > > nbctl_lr_route_add(struct ctl_context *ctx) > > @@ -3988,42 +4030,21 @@ nbctl_lr_route_add(struct ctl_context *ctx) > > "--ecmp-symmetric-reply") != > > NULL; > > bool ecmp = shash_find(&ctx->options, "--ecmp") != NULL || > > ecmp_symmetric_reply; > > + struct nbrec_logical_router_static_route *route = > > + nbctl_lr_get_route_prefix(lr, prefix, next_hop, is_src_route, > > ecmp); > > if (!ecmp) { > > - for (int i = 0; i < lr->n_static_routes; i++) { > > - const struct nbrec_logical_router_static_route *route > > - = lr->static_routes[i]; > > - char *rt_prefix; > > - > > - /* Compare route policy. */ > > - char *nb_policy = lr->static_routes[i]->policy; > > - bool nb_is_src_route = false; > > - if (nb_policy && !strcmp(nb_policy, "src-ip")) { > > - nb_is_src_route = true; > > - } > > - if (is_src_route != nb_is_src_route) { > > - continue; > > - } > > - > > - /* Compare route prefix. */ > > - rt_prefix = > > normalize_prefix_str(lr->static_routes[i]->ip_prefix); > > - if (!rt_prefix) { > > - /* Ignore existing prefix we couldn't parse. */ > > - continue; > > - } > > - > > - if (strcmp(rt_prefix, prefix)) { > > - free(rt_prefix); > > - continue; > > - } > > - > > + if (route) { > > if (!may_exist) { > > ctl_error(ctx, "duplicate prefix: %s (policy: %s). Use > > option" > > " --ecmp to allow this for ECMP routing.", > > prefix, is_src_route ? "src-ip" : "dst-ip"); > > - free(rt_prefix); > > goto cleanup; > > } > > > > + char *rt_prefix = normalize_prefix_str(route->ip_prefix); > > + if (!rt_prefix) { > > + goto cleanup; > > + } > > This can be removed: > 1) Since route is non-NULL, normalize_prefix_str(route->ip_prefix) is > guaranteed to return non-NULL as well. > 2) rt_prefix is not actually being used anywhere in this block. > > > /* Update the next hop for an existing route. */ > > nbrec_logical_router_verify_static_routes(lr); > > nbrec_logical_router_static_route_verify_ip_prefix(route); > > @@ -4052,9 +4073,11 @@ nbctl_lr_route_add(struct ctl_context *ctx) > > free(rt_prefix); > > goto cleanup; > > } > > + } else if (route && !strcmp(route->nexthop, next_hop)) { > > You can remove the strcmp() call. Since ecmp is true, route->nexthop and > next_hop must be the same in order for route to be non-NULL. > > > + ctl_error(ctx, "duplicate nexthop for the same ECMP route"); > > + goto cleanup; > > } > > > > - struct nbrec_logical_router_static_route *route; > > route = nbrec_logical_router_static_route_insert(ctx->txn); > > nbrec_logical_router_static_route_set_ip_prefix(route, prefix); > > nbrec_logical_router_static_route_set_nexthop(route, next_hop); > > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
