Hi Lucas, thanks for the patch! See some minor comments below.
BTW this isn't really a full review so I'd appreciate another set of
eyes if anyone else can take a look.
On 9/30/26 9:24 AM, Lucas Vargas Dias via dev wrote:
> The ECMP group id and the member ids are written into the
> lr_in_ip_routing and lr_in_ip_routing_ecmp logical flows, but they
> were assigned from the order in which routes reached
> en_group_ecmp_route: the group id was hmap_count() at creation time
> and the member id was the insertion position. On a full recompute
> the routes are walked in hmap order, while incremental processing
> appends them in event order, so the same set of routes could get a
> different numbering. Every forced or fallback recompute could then
> delete and re-insert Logical_Flow rows in the Southbound DB even
> though nothing had changed.
>
> Fixes: 6979a96138a2 ("northd: Support I+P for group_ecmp_route engine.")
> Fixes: 5395e12f247c ("northd: Drop traffic for ECMP group with "discard"
> route.")
> Assisted-by: Claude Opus 5.5, Claude Code
> Signed-off-by: Lucas Vargas Dias <[email protected]>
> ---
Nit: Including a changelog would help!
> northd/en-group-ecmp-route.c | 175 +++++++++++++++++++++++++++++------
> tests/ovn-northd.at | 66 +++++++++++++
> 2 files changed, 215 insertions(+), 26 deletions(-)
>
> diff --git a/northd/en-group-ecmp-route.c b/northd/en-group-ecmp-route.c
> index aca197318..43a85a9cc 100644
> --- a/northd/en-group-ecmp-route.c
> +++ b/northd/en-group-ecmp-route.c
> @@ -228,14 +228,12 @@ ecmp_groups_add_route(struct ecmp_groups_node *group,
> const struct parsed_route *route)
> {
> static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
> - if (group->route_count == UINT16_MAX) {
> + if (vector_len(&group->route_list) == UINT16_MAX) {
> VLOG_WARN_RL(&rl, "too many routes in a single ecmp group.");
> return;
> }
>
> if (route->is_discard_route) {
> - group->has_discard_route = true;
> -
> char *prefix = normalize_v46_prefix(&route->prefix, route->plen);
> VLOG_WARN_RL(&rl, "The ECMP route \"%s\" contains \"discard\" "
> "route, the whole group will drop traffic.", prefix);
> @@ -244,22 +242,14 @@ ecmp_groups_add_route(struct ecmp_groups_node *group,
>
> struct ecmp_route_list_node er = (struct ecmp_route_list_node) {
> .route = route,
> - .id = ++group->route_count,
> };
>
> - if (group->route_count == 1) {
> - sset_clone(&group->selection_fields, &route->ecmp_selection_fields);
> - } else {
> - sset_intersect(&group->selection_fields,
> - &route->ecmp_selection_fields);
> - }
> -
> vector_push(&group->route_list, &er);
> + group->route_count = vector_len(&group->route_list);
> }
>
> -/* Removes a route from an ecmp group. If the ecmp group should persist
> - * afterwards you must call ecmp_groups_update_ids before any further
> - * insertions. */
> +/* Removes a route from an ecmp group. The ids of the group and of its
> + * members are refreshed by group_ecmp_datapath_finalize(). */
> static const struct parsed_route *
> ecmp_groups_remove_route(struct ecmp_groups_node *group,
> const struct parsed_route *pr)
> @@ -278,15 +268,147 @@ ecmp_groups_remove_route(struct ecmp_groups_node
> *group,
> return NULL;
> }
>
> +static int
> +nullable_strcmp(const char *a, const char *b)
> +{
> + if (!a || !b) {
> + return !!a - !!b;
> + }
> + return strcmp(a, b);
> +}
Btw, OVS already has nullable_string_is_equal() but it returns opposite
poliarify from your nullable_strcmp(). I think your function is more
robust anyways because it handles the order of args when just one is
NULL, by returning a signed int rather than a bool, as the preexisting
function does. And your function's behavior aligns more closely with
strcmp() in that way as well.
Could it make sense to replace calls to nullable_string_is_equal() with
!nullable_strcmp() in a new patch, and to put nullable_strcmp() in
util.h instead?
In any case, I think adding a brief comment would be a good idea.
Especially if both functions remain it would be good to differentiate.
But I think the other one could be removed altogether. And it would be
good I think to explain that this just handles NULL input and treats two
NULLs as equal.
> +
> +/* Orders the members of an ecmp group by their content only, so that the
> + * member ids do not depend on the order in which the routes were added. */
> +static int
> +ecmp_route_list_node_cmp(const void *a_, const void *b_)
> +{
> + const struct parsed_route *a =
> + ((const struct ecmp_route_list_node *) a_)->route;
> + const struct parsed_route *b =
> + ((const struct ecmp_route_list_node *) b_)->route;
> + int cmp;
> +
> + if (a->source != b->source) {
> + return a->source < b->source ? -1 : 1;
> + }
> + if (a->is_discard_route != b->is_discard_route) {
> + return a->is_discard_route ? -1 : 1;
> + }
> + if (!a->nexthop || !b->nexthop) {
> + cmp = !!a->nexthop - !!b->nexthop;
> + } else {
> + cmp = memcmp(a->nexthop, b->nexthop, sizeof *a->nexthop);
> + }
> + if (cmp) {
> + return cmp;
> + }
> + cmp = nullable_strcmp(a->out_port ? a->out_port->key : NULL,
> + b->out_port ? b->out_port->key : NULL);
> + if (cmp) {
> + return cmp;
> + }
> + cmp = nullable_strcmp(a->lrp_addr_s, b->lrp_addr_s);
> + if (cmp) {
> + return cmp;
> + }
> + if (a->ecmp_symmetric_reply != b->ecmp_symmetric_reply) {
> + return a->ecmp_symmetric_reply ? -1 : 1;
> + }
> + if (!a->source_hint || !b->source_hint) {
> + return !!a->source_hint - !!b->source_hint;
> + }
> + return uuid_compare_3way(&a->source_hint->uuid, &b->source_hint->uuid);
> +}
> +
> +/* NAT and LB routes for the same prefix share a group, see
> + * route_sources_ecmp_compatible(). */
> +static int
> +ecmp_group_source_class(enum route_source source)
> +{
> + return source == ROUTE_SOURCE_LB ? ROUTE_SOURCE_NAT : source;
> +}
> +
> +/* Orders the ecmp groups of a datapath by their key only. The key is unique
> + * within a datapath, see ecmp_groups_find(). */
> +static int
> +ecmp_groups_node_cmp(const void *a_, const void *b_)
> +{
> + const struct ecmp_groups_node *a =
> + *(const struct ecmp_groups_node *const *) a_;
> + const struct ecmp_groups_node *b =
> + *(const struct ecmp_groups_node *const *) b_;
> +
> + if (a->route_table_id != b->route_table_id) {
> + return a->route_table_id < b->route_table_id ? -1 : 1;
> + }
> + if (a->is_src_route != b->is_src_route) {
> + return a->is_src_route ? 1 : -1;
> + }
> + if (a->plen != b->plen) {
> + return a->plen < b->plen ? -1 : 1;
> + }
> + int cmp = memcmp(&a->prefix, &b->prefix, sizeof a->prefix);
> + if (cmp) {
> + return cmp;
> + }
> + int a_class = ecmp_group_source_class(a->source);
> + int b_class = ecmp_group_source_class(b->source);
> + return a_class < b_class ? -1 : a_class > b_class;
Nit: It took me a second to realize that if a_class and b_class are
equal here, the nodes are identical so they're part of the same group.
Maybe the comment could say that more explicitly or a comment could be
added before the very last block where returning 0 is possible ? That
could just be me though :)
> +}
> +
> +/* Recomputes everything in 'group' that is derived from its members, so that
> + * the result only depends on the set of members. */
> static void
> -ecmp_group_update_ids(struct ecmp_groups_node *group)
> +ecmp_group_finalize(struct ecmp_groups_node *group)
> {
> + vector_qsort(&group->route_list, ecmp_route_list_node_cmp);
> +
> struct ecmp_route_list_node *er;
> - size_t i = 0;
> + uint16_t id = 0;
> + group->has_discard_route = false;
> VECTOR_FOR_EACH_PTR (&group->route_list, er) {
> - er->id = i++;
> + er->id = ++id;
> + if (er->route->is_discard_route) {
> + group->has_discard_route = true;
> + }
> + if (id == 1) {
> + sset_destroy(&group->selection_fields);
> + sset_clone(&group->selection_fields,
> + &er->route->ecmp_selection_fields);
> + group->source = er->route->source;
> + } else {
> + sset_intersect(&group->selection_fields,
> + &er->route->ecmp_selection_fields);
> + }
> + }
> + group->route_count = id;
> +}
> +
> +/* Assigns the group and member ids of all ecmp groups of 'gn'. Must be
> + * called after the ecmp groups of 'gn' changed, both on full recompute and
> on
> + * incremental processing, so that both yield the same ids for the same set
> + * of routes and the generated logical flows do not change on recompute. */
> +static void
> +group_ecmp_datapath_finalize(struct group_ecmp_datapath *gn)
> +{
> + size_t n = hmap_count(&gn->ecmp_groups);
> + if (!n) {
> + return;
> }
> - group->route_count = i;
> +
> + struct ecmp_groups_node **groups = xmalloc(n * sizeof *groups);
> + struct ecmp_groups_node *eg;
> + size_t i = 0;
> + HMAP_FOR_EACH (eg, hmap_node, &gn->ecmp_groups) {
> + ecmp_group_finalize(eg);
> + groups[i++] = eg;
> + }
> +
> + qsort(groups, n, sizeof *groups, ecmp_groups_node_cmp);
> + for (i = 0; i < n; i++) {
> + groups[i]->id = i + 1;
> + }
> + free(groups);
> }
>
> static struct ecmp_groups_node *
> @@ -302,7 +424,6 @@ ecmp_groups_add(struct group_ecmp_datapath *gn,
> struct ecmp_groups_node *eg = xzalloc(sizeof *eg);
> hmap_insert(&gn->ecmp_groups, &eg->hmap_node, route->hash);
>
> - eg->id = hmap_count(&gn->ecmp_groups);
> eg->prefix = route->prefix;
> eg->plen = route->plen;
> eg->is_src_route = route->is_src_route;
> @@ -396,6 +517,10 @@ group_ecmp_route(struct group_ecmp_route_data *data,
> gn = group_ecmp_datapath_lookup_or_add(data, pr->od);
> add_route(gn, pr);
> }
> +
> + HMAP_FOR_EACH (gn, hmap_node, &data->datapaths) {
> + group_ecmp_datapath_finalize(gn);
> + }
> }
>
> enum engine_node_state
> @@ -472,9 +597,7 @@ handle_deleted_route(struct group_ecmp_route_data *data,
> * unique route. Otherwise it stays an ecmp group with just one
> * member. */
> ecmp_groups_remove_route(eg, pr);
> - if (ecmp_group_has_symmetric_reply(eg)) {
> - ecmp_group_update_ids(eg);
> - } else {
> + if (!ecmp_group_has_symmetric_reply(eg)) {
> const struct ecmp_route_list_node *er =
> vector_get_ptr(&eg->route_list, 0);
> unique_routes_add(node, er->route);
> @@ -482,11 +605,8 @@ handle_deleted_route(struct group_ecmp_route_data *data,
> ecmp_groups_node_free(eg);
> }
> } else {
> - /* We can just remove the member from the group. We need to
> update
> - * the indices of all routes so that future insertions directly
> - * have a new index. */
> + /* We can just remove the member from the group. */
> ecmp_groups_remove_route(eg, pr);
> - ecmp_group_update_ids(eg);
> }
> }
>
> @@ -537,6 +657,7 @@ group_ecmp_route_learned_route_change_handler(struct
> engine_node *eng_node,
> hmapx_add(&data->trk_data.deleted_datapath_routes, node);
> hmap_remove(&data->datapaths, &node->hmap_node);
> } else {
> + group_ecmp_datapath_finalize(node);
> hmapx_add(&data->trk_data.crupdated_datapath_routes, node);
> }
> }
> @@ -589,6 +710,7 @@ group_ecmp_route_routes_change_handler(struct engine_node
> *eng_node,
> hmapx_add(&data->trk_data.deleted_datapath_routes, node);
> hmap_remove(&data->datapaths, &node->hmap_node);
> } else {
> + group_ecmp_datapath_finalize(node);
> hmapx_add(&data->trk_data.crupdated_datapath_routes, node);
> }
> }
> @@ -645,6 +767,7 @@ group_ecmp_route_dynamic_routes_change_handler(struct
> engine_node *eng_node,
> hmapx_add(&gdata->trk_data.deleted_datapath_routes, node);
> hmap_remove(&gdata->datapaths, &node->hmap_node);
> } else {
> + group_ecmp_datapath_finalize(node);
> hmapx_add(&gdata->trk_data.crupdated_datapath_routes, node);
> }
> }
> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> index 695ffbdc1..3787fc3ba 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -23755,6 +23755,72 @@ done
> OVN_CLEANUP_NORTHD
> AT_CLEANUP
>
> +OVN_FOR_EACH_NORTHD_NO_HV([
> +AT_SETUP([Static routes - ECMP ids stable across recompute])
> +AT_KEYWORDS([ecmp])
> +ovn_start
> +
> +check ovn-nbctl lr-add lr0
> +for i in 1 2 3 4; do
> + check ovn-nbctl lrp-add lr0 lr0-sw$i 00:00:00:00:0$i:01 10.0.$i.1/24
> +done
> +check ovn-nbctl --wait=sb sync
> +
> +dnl Add the routes in "reverse" order, the ids must not depend on it.
> +for p in 192.168.30.0/24 192.168.20.0/24 192.168.10.0/24; do
> + for i in 3 2 1; do
> + check ovn-nbctl --wait=sb --ecmp lr-route-add lr0 $p 10.0.$i.10
> + done
> +done
> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> +
> +AT_CHECK([ovn-sbctl dump-flows lr0 | grep -w lr_in_ip_routing | grep select
> | ovn_strip_lflows], [0], [dnl
> + table=??(lr_in_ip_routing ), priority=1840 , match=(reg7 == 0 && ip4.dst
> == 192.168.10.0/24), action=(ip.ttl--; flags.loopback = 1; reg8[[0..15]] = 1;
> reg8[[16..31]] = select(1, 2, 3);)
> + table=??(lr_in_ip_routing ), priority=1840 , match=(reg7 == 0 && ip4.dst
> == 192.168.20.0/24), action=(ip.ttl--; flags.loopback = 1; reg8[[0..15]] = 2;
> reg8[[16..31]] = select(1, 2, 3);)
> + table=??(lr_in_ip_routing ), priority=1840 , match=(reg7 == 0 && ip4.dst
> == 192.168.30.0/24), action=(ip.ttl--; flags.loopback = 1; reg8[[0..15]] = 3;
> reg8[[16..31]] = select(1, 2, 3);)
> +])
> +AT_CHECK([ovn-sbctl dump-flows lr0 | grep lr_in_ip_routing_ecmp | grep -F
> "reg8[[0..15]] == 1 &&" | ovn_strip_lflows], [0], [dnl
> + table=??(lr_in_ip_routing_ecmp), priority=100 , match=(reg8[[0..15]] == 1
> && reg8[[16..31]] == 1), action=(reg0 = 10.0.1.10; reg5 = 10.0.1.1; eth.src =
> 00:00:00:00:01:01; outport = "lr0-sw1"; reg9[[9]] = 1; next;)
> + table=??(lr_in_ip_routing_ecmp), priority=100 , match=(reg8[[0..15]] == 1
> && reg8[[16..31]] == 2), action=(reg0 = 10.0.2.10; reg5 = 10.0.2.1; eth.src =
> 00:00:00:00:02:01; outport = "lr0-sw2"; reg9[[9]] = 1; next;)
> + table=??(lr_in_ip_routing_ecmp), priority=100 , match=(reg8[[0..15]] == 1
> && reg8[[16..31]] == 3), action=(reg0 = 10.0.3.10; reg5 = 10.0.3.1; eth.src =
> 00:00:00:00:03:01; outport = "lr0-sw3"; reg9[[9]] = 1; next;)
> +])
> +
> +dnl Remove a member from the middle of a group with more than 2 members.
> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> +check ovn-nbctl --wait=sb lr-route-del lr0 192.168.10.0/24 10.0.2.10
> +check_engine_compute group_ecmp_route incremental
> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> +
> +dnl Remove a whole group and then add a new one, group ids must stay unique.
> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> +check ovn-nbctl --wait=sb lr-route-del lr0 192.168.20.0/24
> +check ovn-nbctl --wait=sb --ecmp lr-route-add lr0 192.168.40.0/24 10.0.1.10
> +check ovn-nbctl --wait=sb --ecmp lr-route-add lr0 192.168.40.0/24 10.0.4.10
> +check_engine_compute group_ecmp_route incremental
> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> +AT_CHECK([ovn-sbctl dump-flows lr0 | grep -w lr_in_ip_routing | grep select
> | ovn_strip_lflows], [0], [dnl
> + table=??(lr_in_ip_routing ), priority=1840 , match=(reg7 == 0 && ip4.dst
> == 192.168.10.0/24), action=(ip.ttl--; flags.loopback = 1; reg8[[0..15]] = 1;
> reg8[[16..31]] = select(1, 2);)
> + table=??(lr_in_ip_routing ), priority=1840 , match=(reg7 == 0 && ip4.dst
> == 192.168.30.0/24), action=(ip.ttl--; flags.loopback = 1; reg8[[0..15]] = 2;
> reg8[[16..31]] = select(1, 2, 3);)
> + table=??(lr_in_ip_routing ), priority=1840 , match=(reg7 == 0 && ip4.dst
> == 192.168.40.0/24), action=(ip.ttl--; flags.loopback = 1; reg8[[0..15]] = 3;
> reg8[[16..31]] = select(1, 2);)
> +])
> +
> +dnl Once no member is a discard route anymore the group must stop dropping.
> +route=$(ovn-nbctl --bare --columns _uuid find Logical_Router_Static_Route \
> + ip_prefix="192.168.30.0/24" nexthop="10.0.3.10")
> +check ovn-nbctl --wait=sb set Logical_Router_Static_Route $route
> nexthop=discard
> +AT_CHECK([ovn-sbctl dump-flows lr0 | grep lr_in_ip_routing_ecmp | grep -c
> "drop;"], [0], [4
> +])
> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> +check ovn-nbctl --wait=sb set Logical_Router_Static_Route $route
> nexthop=10.0.3.10
> +check_engine_compute group_ecmp_route incremental
> +AT_CHECK([ovn-sbctl dump-flows lr0 | grep lr_in_ip_routing_ecmp | grep -c
> "drop;"], [0], [1
> +])
> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> +
> +OVN_CLEANUP_NORTHD
> +AT_CLEANUP
> +])
> +
> OVN_FOR_EACH_NORTHD_NO_HV([
> AT_SETUP([Static routes - ECMP with discard])
> ovn_start
--
Rosemarie O'Riorden
Boston & Lowell, MA, USA
[email protected]
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
