Hi Rosemarie, Thanks for your comments.
Em qui., 1 de out. de 2026 às 14:12, Rosemarie O'Riorden < [email protected]> escreveu: > 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! > > I just changed the commit title in the current version. > > 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. > > I agree > > + > > +/* 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 :) > I'll add a comment > > > +} > > + > > +/* 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] > > Regards, Lucas -- _‘Esta mensagem é direcionada apenas para os endereços constantes no cabeçalho inicial. Se você não está listado nos endereços constantes no cabeçalho, pedimos-lhe que desconsidere completamente o conteúdo dessa mensagem e cuja cópia, encaminhamento e/ou execução das ações citadas estão imediatamente anuladas e proibidas’._ * **‘Apesar do Magazine Luiza tomar todas as precauções razoáveis para assegurar que nenhum vírus esteja presente nesse e-mail, a empresa não poderá aceitar a responsabilidade por quaisquer perdas ou danos causados por esse e-mail ou por seus anexos’.* _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
