On Mon, Oct 19, 2026 at 11:22 PM Mark Michelson via dev < [email protected]> wrote:
> When building our route policies, we evaluate whether the configured > nexthops are valid based on whether we can find a legitimate output port > for the policy, and whether the BFD state for that nexthop is up. By the > time we are writing logical flows, the northbound database's configured > nexthops are irrelevant since we only want to use the routing policy's > valid_nexthops for evaluation. > > When determining whether to evaluate a policy as ECMP or not, we should > use the number of valid nexthops as the determiner, not the northbound > rule's number of nexthops. It's possible that the northbound rule > intends for the policy to be an ECMP reroute to a number of nexthops. > However, if only one configured nexthop is valid, then we can just > program the logical flow as if this were a non-ECMP reroute. We can > skip the need for the OpenFlow select since there is only one valid > nexthop. > > Signed-off-by: Mark Michelson <[email protected]> > --- > northd/northd.c | 30 ++++++++++++------------------ > tests/system-ovn.at | 5 ++++- > 2 files changed, 16 insertions(+), 19 deletions(-) > > diff --git a/northd/northd.c b/northd/northd.c > index eaf8f0b8f..734d39a3a 100644 > --- a/northd/northd.c > +++ b/northd/northd.c > @@ -12428,7 +12428,7 @@ build_routing_policy_flow(struct lflow_table > *lflows, struct ovn_datapath *od, > struct ds actions = DS_EMPTY_INITIALIZER; > > if (!strcmp(rule->action, "reroute")) { > - ovs_assert(rule->n_nexthops <= 1); > + ovs_assert(rp->n_valid_nexthops <= 1); > > if (!rp->n_valid_nexthops) { > return; > @@ -12502,7 +12502,7 @@ build_ecmp_routing_policy_flows(struct lflow_table > *lflows, > { > bool nexthops_is_ipv4 = true; > const struct nbrec_logical_router_policy *rule = rp->rule; > - ovs_assert(rule->n_nexthops > 1); > + ovs_assert(rp->n_valid_nexthops > 1); > > /* Check that all the nexthops belong to the same addr family before > * adding logical flows. */ > @@ -12568,24 +12568,18 @@ build_ecmp_routing_policy_flows(struct > lflow_table *lflows, > } > > ds_clear(&actions); > - if (rp->n_valid_nexthops > 1) { > - ds_put_format(&actions, "%s = %"PRIu16 > - "; %s = select(", REG_ECMP_GROUP_ID, ecmp_group_id, > - REG_ECMP_MEMBER_ID); > - > - for (size_t i = 0; i < rp->n_valid_nexthops; i++) { > - if (i > 0) { > - ds_put_cstr(&actions, ", "); > - } > + ds_put_format(&actions, "%s = %"PRIu16 > + "; %s = select(", REG_ECMP_GROUP_ID, ecmp_group_id, > + REG_ECMP_MEMBER_ID); > > - ds_put_format(&actions, "%"PRIuSIZE, i + 1); > + for (size_t i = 0; i < rp->n_valid_nexthops; i++) { > + if (i > 0) { > + ds_put_cstr(&actions, ", "); > } > - ds_put_cstr(&actions, ");"); > - } else { > - ds_put_format(&actions, "%s = %"PRIu16 > - "; %s = 1; next;", REG_ECMP_GROUP_ID, > - ecmp_group_id, REG_ECMP_MEMBER_ID); > + > + ds_put_format(&actions, "%"PRIuSIZE, i + 1); > } > + ds_put_cstr(&actions, ");"); > ovn_lflow_add(lflows, od, S_ROUTER_IN_POLICY, rule->priority, > rule->match, > ds_cstr(&actions), lflow_ref, > WITH_HINT(&rule->header_)); > cleanup: > @@ -15937,7 +15931,7 @@ build_ingress_policy_flows_for_lrouter( > route_policies) { > const struct nbrec_logical_router_policy *rule = rp->rule; > bool is_ecmp_reroute = > - (!strcmp(rule->action, "reroute") && rule->n_nexthops > 1); > + (!strcmp(rule->action, "reroute") && rp->n_valid_nexthops > > 1); > > if (is_ecmp_reroute) { > build_ecmp_routing_policy_flows(lflows, od, rp, ecmp_group_id, > diff --git a/tests/system-ovn.at b/tests/system-ovn.at > index 3f43132cc..0f516527e 100644 > --- a/tests/system-ovn.at > +++ b/tests/system-ovn.at > @@ -7349,7 +7349,10 @@ OVS_WAIT_UNTIL([ip netns exec server bfdd-control > status | grep -qi state=Down]) > > check ovn-nbctl --bfd lr-policy-add R1 100 "ip4.src == 200.0.0.0/8" > reroute 172.16.1.50,172.16.1.60 > wait_column "up" nb:bfd status dst_ip=172.16.1.50 > -OVS_WAIT_UNTIL([ovn-sbctl dump-flows R1 | grep lr_in_policy_ecmp | grep > -q 172.16.1.50]) > + > +# Even though the policy is an ECMP policy, only one route is actually > valid. > +# Therefore, the policy is installed in lr_in_policy instead of > lr_in_policy_ecmp. > +OVS_WAIT_UNTIL([ovn-sbctl dump-flows R1 | grep 'lr_in_policy[[^_]]' | > grep -q 172.16.1.50]) > > check ovn-nbctl lr-policy-del R1 > wait_column "admin_down" nb:bfd status dst_ip=172.16.1.50 > -- > 2.55.0 > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > Looks good to me, thanks. Acked-by: Ales Musil <[email protected]> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
