| Message ID | 1c1360b80d91b46babe5c1d66f77254a55abf6fa.1712398898.git.lorenzo.bianconi@redhat.com |
|---|---|
| State | Accepted |
| Delegated to: | Dumitru Ceara |
| Headers | show |
| Series | [ovs-dev] northd: Fix BFD for policy routing. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
Hi Lorenzo, This looks good to me. Thanks for fixing this! Acked-by: Mark Michelson <mmichels@redhat.com> On 4/6/24 06:32, Lorenzo Bianconi wrote: > Fix the following ovn-controller error when bfd policy routing has just one > available next-hop. > > lflow|WARN|error parsing actions "reg8[0..15] = 1; reg8[16..31] = select(1);": Syntax error at `;' expecting at least 2 group members. > > Fixes: 62d5491c0155 ("northd: Add BFD support for ECMP route policy.") > Reported-at: https://issues.redhat.com/browse/FDP-543 > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> > --- > northd/northd.c | 29 ++++++++++++++++++++--------- > tests/system-ovn.at | 13 +++++++++---- > 2 files changed, 29 insertions(+), 13 deletions(-) > > diff --git a/northd/northd.c b/northd/northd.c > index 02cf5b234..5b6bd90c8 100644 > --- a/northd/northd.c > +++ b/northd/northd.c > @@ -10087,19 +10087,30 @@ build_ecmp_routing_policy_flows(struct lflow_table *lflows, > lflow_ref); > } > > + if (!n_valid_nexthops) { > + goto cleanup; > + } > + > ds_clear(&actions); > - ds_put_format(&actions, "%s = %"PRIu16 > - "; %s = select(", REG_ECMP_GROUP_ID, ecmp_group_id, > - REG_ECMP_MEMBER_ID); > + if (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 < n_valid_nexthops; i++) { > + if (i > 0) { > + ds_put_cstr(&actions, ", "); > + } > > - for (size_t i = 0; i < n_valid_nexthops; i++) { > - if (i > 0) { > - ds_put_cstr(&actions, ", "); > + ds_put_format(&actions, "%"PRIuSIZE, valid_nexthops[i]); > } > - > - ds_put_format(&actions, "%"PRIuSIZE, valid_nexthops[i]); > + ds_put_cstr(&actions, ");"); > + } else { > + ds_put_format(&actions, "%s = %"PRIu16 > + "; %s = %"PRIuSIZE"; next;", REG_ECMP_GROUP_ID, > + ecmp_group_id, REG_ECMP_MEMBER_ID, > + valid_nexthops[0]); > } > - ds_put_cstr(&actions, ");"); > ovn_lflow_add_with_hint(lflows, od, S_ROUTER_IN_POLICY, > rule->priority, rule->match, > ds_cstr(&actions), &rule->header_, > diff --git a/tests/system-ovn.at b/tests/system-ovn.at > index c699f7960..085edd81d 100644 > --- a/tests/system-ovn.at > +++ b/tests/system-ovn.at > @@ -6834,12 +6834,13 @@ check ovn-nbctl clear logical_router_static_route $route_uuid bfd > wait_column "admin_down" nb:bfd status logical_port=rp-public > 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 > -wait_column "up" nb:bfd status logical_port=rp-public > -OVS_WAIT_UNTIL([ovn-sbctl dump-flows R1 |grep lr_in_policy |grep 'ip4.src == 200.0.0.0/8' |grep -q 172.16.1.50]) > +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]) > > check ovn-nbctl lr-policy-del R1 > -wait_column "admin_down" nb:bfd status logical_port=rp-public > +wait_column "admin_down" nb:bfd status dst_ip=172.16.1.50 > +wait_column "admin_down" nb:bfd status dst_ip=172.16.1.60 > OVS_WAIT_UNTIL([ip netns exec server bfdd-control status | grep -qi state=Down]) > > NETNS_START_TCPDUMP([server], [-nni s1 udp port 3784 -Q in], [bfd]) > @@ -6847,6 +6848,10 @@ sleep 5 > kill $(pidof tcpdump) > AT_CHECK([grep -qi bfd bfd.tcpdump],[1]) > > +uuid=$(fetch_column nb:bfd _uuid dst_ip="172.16.1.60") > +check ovn-nbctl destroy bfd $uuid > +uuid=$(fetch_column nb:bfd _uuid logical_port="rp-public") > + > # restart the connection > check ovn-nbctl set logical_router_static_route $route_uuid bfd=$uuid > check ovn-nbctl --bfd lr-policy-add R1 100 "ip4.src == 200.0.0.0/8" reroute 172.16.1.50
On 4/9/24 20:42, Mark Michelson wrote: > Hi Lorenzo, > > This looks good to me. Thanks for fixing this! > > Acked-by: Mark Michelson <mmichels@redhat.com> > Thanks, Lorenzo and Mark! Applied to main and backported to 24.03. Regards, Dumitru
diff --git a/northd/northd.c b/northd/northd.c index 02cf5b234..5b6bd90c8 100644 --- a/northd/northd.c +++ b/northd/northd.c @@ -10087,19 +10087,30 @@ build_ecmp_routing_policy_flows(struct lflow_table *lflows, lflow_ref); } + if (!n_valid_nexthops) { + goto cleanup; + } + ds_clear(&actions); - ds_put_format(&actions, "%s = %"PRIu16 - "; %s = select(", REG_ECMP_GROUP_ID, ecmp_group_id, - REG_ECMP_MEMBER_ID); + if (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 < n_valid_nexthops; i++) { + if (i > 0) { + ds_put_cstr(&actions, ", "); + } - for (size_t i = 0; i < n_valid_nexthops; i++) { - if (i > 0) { - ds_put_cstr(&actions, ", "); + ds_put_format(&actions, "%"PRIuSIZE, valid_nexthops[i]); } - - ds_put_format(&actions, "%"PRIuSIZE, valid_nexthops[i]); + ds_put_cstr(&actions, ");"); + } else { + ds_put_format(&actions, "%s = %"PRIu16 + "; %s = %"PRIuSIZE"; next;", REG_ECMP_GROUP_ID, + ecmp_group_id, REG_ECMP_MEMBER_ID, + valid_nexthops[0]); } - ds_put_cstr(&actions, ");"); ovn_lflow_add_with_hint(lflows, od, S_ROUTER_IN_POLICY, rule->priority, rule->match, ds_cstr(&actions), &rule->header_, diff --git a/tests/system-ovn.at b/tests/system-ovn.at index c699f7960..085edd81d 100644 --- a/tests/system-ovn.at +++ b/tests/system-ovn.at @@ -6834,12 +6834,13 @@ check ovn-nbctl clear logical_router_static_route $route_uuid bfd wait_column "admin_down" nb:bfd status logical_port=rp-public 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 -wait_column "up" nb:bfd status logical_port=rp-public -OVS_WAIT_UNTIL([ovn-sbctl dump-flows R1 |grep lr_in_policy |grep 'ip4.src == 200.0.0.0/8' |grep -q 172.16.1.50]) +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]) check ovn-nbctl lr-policy-del R1 -wait_column "admin_down" nb:bfd status logical_port=rp-public +wait_column "admin_down" nb:bfd status dst_ip=172.16.1.50 +wait_column "admin_down" nb:bfd status dst_ip=172.16.1.60 OVS_WAIT_UNTIL([ip netns exec server bfdd-control status | grep -qi state=Down]) NETNS_START_TCPDUMP([server], [-nni s1 udp port 3784 -Q in], [bfd]) @@ -6847,6 +6848,10 @@ sleep 5 kill $(pidof tcpdump) AT_CHECK([grep -qi bfd bfd.tcpdump],[1]) +uuid=$(fetch_column nb:bfd _uuid dst_ip="172.16.1.60") +check ovn-nbctl destroy bfd $uuid +uuid=$(fetch_column nb:bfd _uuid logical_port="rp-public") + # restart the connection check ovn-nbctl set logical_router_static_route $route_uuid bfd=$uuid check ovn-nbctl --bfd lr-policy-add R1 100 "ip4.src == 200.0.0.0/8" reroute 172.16.1.50
Fix the following ovn-controller error when bfd policy routing has just one available next-hop. lflow|WARN|error parsing actions "reg8[0..15] = 1; reg8[16..31] = select(1);": Syntax error at `;' expecting at least 2 group members. Fixes: 62d5491c0155 ("northd: Add BFD support for ECMP route policy.") Reported-at: https://issues.redhat.com/browse/FDP-543 Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> --- northd/northd.c | 29 ++++++++++++++++++++--------- tests/system-ovn.at | 13 +++++++++---- 2 files changed, 29 insertions(+), 13 deletions(-)