On 9/28/26 4:01 PM, Michal Arbet wrote:
> When multiple logical router ports on the same router share a connected
> prefix and BFD is enabled on each port, northd generates BFD helper routes
> with identical matches but different actions.
>
> ovn-controller represents desired flows with the same OpenFlow match using
> a single installed flow, so only one of these routes becomes active. The
> selected route can also change after a full recompute. As a result, BFD
> traffic for one logical router port can be routed through another port and
> redirected to a different gateway chassis.
>
> BFD packets generated by pinctrl already carry the BFD logical router port
> as MFF_LOG_INPORT. Include that logical inport in the BFD helper route
> match so that each BFD session selects the route associated with its own
> logical router port.
>
> Avoid adding the inport twice for IPv6 link-local connected routes, which
> are already scoped to their logical router port.
>
> Add a northd regression test with two logical router ports in the same
> IPv4 subnet and ECMP+BFD routes to the same nexthop. Also add a packet
> test with two gateway chassis that captures controller-generated BFD
> traffic at the provider uplinks. Verify each session uses its own LRP
> before and after controller recompute, and after moving the LRPs onto
> the same chassis and back. The same-chassis check detects the wrong
> source MAC independently of conflicting flow ordering.
>
> All four BFD tests pass with the fix. Both variants of the new packet
> test fail when run with ovn-northd built without the fix. In one run,
> BFD traffic follows the correct path initially but is redirected to
> the wrong gateway after controller recompute.
>
> Reported-at: https://github.com/ovn-org/ovn/issues/330
> Submitted-at: https://github.com/ovn-org/ovn/pull/331
> Assisted-by: GPT-6, OpenAI Codex
> Signed-off-by: Michal Arbet <[email protected]>
> ---
> Changes in v2:
> - Add a packet-forwarding regression test for BFD sessions on LRPs
> sharing a connected subnet, including controller recompute and
> gateway placement changes.
> - Verify both packet-test variants fail without the fix and all four
> BFD tests pass with the fix.
> - Add Assisted-by and update the testing description.
>
Hi Michal,
Thanks for the fix!
> northd/northd.c | 7 +++
> tests/ovn-northd.at | 42 +++++++++++++-
> tests/ovn.at | 134 ++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 182 insertions(+), 1 deletion(-)
>
> diff --git a/northd/northd.c b/northd/northd.c
> index 4eb2ea44b..0ad7969ca 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -13422,6 +13422,13 @@ add_route(struct lflow_table *lflows, const struct
> ovn_datapath *od,
> ds_cstr(&match), ds_cstr(&actions), lflow_ref,
> WITH_HINT(stage_hint));
> if (op && bfd_is_port_running(bfd_ports, op->key)) {
> + /* BFD packets generated by ovn-controller are injected with their
> + * logical router port set as the logical inport. Scope this helper
> + * route to that port so LRPs sharing a connected prefix do not
> + * generate conflicting flows with identical matches. */
> + if (!op_inport) {
> + ds_put_format(&match, " && inport == %s", op->json_key);
> + }
> ds_put_format(&match, " && udp.dst == 3784");
> ovn_lflow_add(lflows, op->od, S_ROUTER_IN_IP_ROUTING, priority + 1,
> ds_cstr(&match), ds_cstr(&common_actions), lflow_ref,
> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> index 6572b1318..8f8aa8c54 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -4653,6 +4653,47 @@ OVN_CLEANUP_NORTHD
> AT_CLEANUP
> ])
>
> +OVN_FOR_EACH_NORTHD_NO_HV([
> +AT_SETUP([BFD routes on LRPs sharing a connected subnet])
> +AT_KEYWORDS([northd-bfd])
> +ovn_start
> +
> +check ovn-nbctl lr-add r0
> +check ovn-nbctl lrp-add r0 r0-ext-a 00:00:00:00:00:01 10.0.0.10/24
> +check ovn-nbctl lrp-add r0 r0-ext-b 00:00:00:00:00:02 10.0.0.20/24
> +check ovn-nbctl ls-add ext
> +check ovn-nbctl lsp-add-router-port ext ext-r0-a r0-ext-a
> +check ovn-nbctl lsp-add-router-port ext ext-r0-b r0-ext-b
> +
> +# Neutron creates these routes and BFD records directly in the NB database.
> +# Use the same approach here because lr-route-add rejects ECMP routes with a
> +# duplicate nexthop, even when they use different output ports.
> +check_uuid ovn-nbctl --wait=sb \
> + --id=@bfd_a create bfd logical_port=r0-ext-a dst_ip=10.0.0.1 -- \
> + --id=@route_a create logical_router_static_route ip_prefix=0.0.0.0/0 \
> + nexthop=10.0.0.1 output_port=r0-ext-a bfd=@bfd_a -- \
> + add logical_router r0 static_routes @route_a -- \
> + --id=@bfd_b create bfd logical_port=r0-ext-b dst_ip=10.0.0.1 -- \
> + --id=@route_b create logical_router_static_route ip_prefix=0.0.0.0/0 \
> + nexthop=10.0.0.1 output_port=r0-ext-b bfd=@bfd_b -- \
> + add logical_router r0 static_routes @route_b
> +
> +AT_CHECK([ovn-sbctl lflow-list | grep 'lr_in_ip_routing' | \
> + grep '10.0.0.0/24' | grep 'udp.dst == 3784' | wc -l], [0], [2
> +])
> +AT_CHECK([ovn-sbctl lflow-list | grep 'lr_in_ip_routing' | \
> + grep '10.0.0.0/24' | grep 'udp.dst == 3784' | \
> + grep -c 'inport == "r0-ext-a"'], [0], [1
> +])
> +AT_CHECK([ovn-sbctl lflow-list | grep 'lr_in_ip_routing' | \
> + grep '10.0.0.0/24' | grep 'udp.dst == 3784' | \
> + grep -c 'inport == "r0-ext-b"'], [0], [1
> +])
> +
> +OVN_CLEANUP_NORTHD
> +AT_CLEANUP
> +])
> +
> OVN_FOR_EACH_NORTHD_NO_HV([
> AT_SETUP([ovn -- check CoPP config])
> AT_KEYWORDS([northd-CoPP])
> @@ -24351,4 +24392,3 @@ CHECK_NO_CHANGE_AFTER_RECOMPUTE
> OVN_CLEANUP_NORTHD
> AT_CLEANUP
> ])
> -
Nit: unrelated removal.
> diff --git a/tests/ovn.at b/tests/ovn.at
> index 13e95f9db..f647c886d 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -12815,6 +12815,140 @@ OVN_CLEANUP([hv1])
> AT_CLEANUP
> ])
>
> +OVN_FOR_EACH_NORTHD([
> +AT_SETUP([BFD packets on LRPs sharing a connected subnet])
> +AT_KEYWORDS([ovn-bfd bfd-shared-subnet])
> +ovn_start
> +
> +net_add underlay
> +net_add provider
> +
> +for i in 1 2; do
> + sim_add gw$i
> + as gw$i
> + check ovs-vsctl add-br br-phys
> + ovn_attach underlay br-phys 192.168.0.$i
> + check ovs-vsctl add-br br-ex
> + net_attach provider br-ex
> + check ovs-vsctl set Open_vSwitch . \
> + external-ids:ovn-bridge-mappings=phys:br-ex
> +done
> +OVN_POPULATE_ARP
> +
> +check ovn-nbctl lr-add r0
> +check ovn-nbctl ls-add ext
> +check ovn-nbctl lsp-add-localnet-port ext ln-ext phys
> +for i in 1 2; do
> + check ovn-nbctl lrp-add r0 r0-ext$i 00:00:00:00:00:0$i 10.0.0.$i/24
> + check ovn-nbctl lsp-add-router-port ext ext-r0-$i r0-ext$i
> + check ovn-nbctl lrp-set-gateway-chassis r0-ext$i gw$i
> + # Avoid depending on ARP replies from an external BFD peer.
> + check ovn-nbctl static-mac-binding-add r0-ext$i 10.0.0.254 \
> + 00:00:00:00:00:fe
> +done
> +
> +# Like Neutron, create the routes directly: lr-route-add rejects duplicate
> +# ECMP nexthops even when the output ports differ.
> +check_uuid ovn-nbctl --wait=hv \
> + --id=@bfd1 create BFD logical_port=r0-ext1 dst_ip=10.0.0.254 -- \
> + --id=@route1 create Logical_Router_Static_Route ip_prefix=0.0.0.0/0 \
> + nexthop=10.0.0.254 output_port=r0-ext1 bfd=@bfd1 -- \
> + add Logical_Router r0 static_routes @route1 -- \
> + --id=@bfd2 create BFD logical_port=r0-ext2 dst_ip=10.0.0.254 -- \
> + --id=@route2 create Logical_Router_Static_Route ip_prefix=0.0.0.0/0 \
> + nexthop=10.0.0.254 output_port=r0-ext2 bfd=@bfd2 -- \
> + add Logical_Router r0 static_routes @route2
> +
> +for i in 1 2; do
> + chassis=$(fetch_column Chassis _uuid name=gw$i)
> + wait_column "$chassis" Port_Binding chassis logical_port=cr-r0-ext$i
> +
> + # Identify each controller-generated BFD session by its source port and
> + # discriminator, and check its Ethernet and IP addresses on the wire.
> + src_port=$(printf '%04x' $(fetch_column BFD src_port
> logical_port=r0-ext$i))
> + disc=$(printf '%08x' $(fetch_column BFD disc logical_port=r0-ext$i))
> + echo
> "0000000000fe00000000000${i}08000a00000${i}0a0000fe${src_port}0ec8${disc}" \
> + > bfd$i.expected
> +done
> +
> +OVN_WAIT_PATCH_PORT_FLOWS([ln-ext], [gw1 gw2])
> +check ovn-nbctl --wait=hv sync
> +
> +check_bfd_packets() {
> + local colocated=$1 gw
> +
> + for gw in gw1 gw2; do
> + as $gw reset_pcap_file br-ex_provider $gw/br-ex_provider
> + done
> + if test "$colocated" = yes; then
> + cat bfd1.expected bfd2.expected | sort > gw1.expected
> + : > gw2.expected
> + else
> + cp bfd1.expected gw1.expected
> + cp bfd2.expected gw2.expected
> + fi
> +
> + # These are packets emitted by pinctrl and forwarded by ovs-vswitchd,
> + # not traces or flow dumps. Ignore ARP and compare the set of BFD
> + # sessions at each provider uplink, including the source MAC selected
> + # by routing. Wait for several packets, and reject unexpected sessions.
> + OVS_WAIT_UNTIL([
> + for gw in gw1 gw2; do
> + $PYTHON "$ovs_srcdir/utilities/ovs-pcap.in" \
> + $gw/br-ex_provider-tx.pcap | \
> + grep -E '^.{24}080045.{16}11.{24}0ec8' | \
> + cut -c 1-28,53-76,93-100 > $gw.bfd
> + sort -u $gw.bfd > $gw.actual
> + done
> + test $(wc -l < gw1.bfd) -ge 3 &&
> + { test "$colocated" = yes || test $(wc -l < gw2.bfd) -ge 3; } &&
> + diff -u gw1.expected gw1.actual &&
> + diff -u gw2.expected gw2.actual
> + ])
> +}
> +
> +AT_CAPTURE_FILE([gw1.actual])
> +AT_CAPTURE_FILE([gw2.actual])
> +AT_CAPTURE_FILE([gw1.expected])
> +AT_CAPTURE_FILE([gw2.expected])
> +
> +AS_BOX([BFD sessions on separate gateways])
> +# Each gateway must send only its own session through its provider uplink.
> +check_bfd_packets no
> +
> +AS_BOX([BFD sessions after controller recompute])
> +# Recomputing the controllers must not change either session's egress path.
> +for gw in gw1 gw2; do
> + check as $gw ovn-appctl -t ovn-controller inc-engine/recompute
> +done
> +check ovn-nbctl --wait=hv sync
> +check_bfd_packets no
> +
> +AS_BOX([BFD sessions on the same gateway])
> +# Also check both LRPs on one chassis. Without the fix only one of the
> +# conflicting routes can win, so at least one session gets the wrong source
> +# MAC regardless of flow ordering. This makes the regression deterministic.
> +check ovn-nbctl --wait=hv \
> + lrp-del-gateway-chassis r0-ext2 gw2 -- \
> + lrp-set-gateway-chassis r0-ext2 gw1
> +chassis=$(fetch_column Chassis _uuid name=gw1)
> +wait_column "$chassis" Port_Binding chassis logical_port=cr-r0-ext2
> +check ovn-nbctl --wait=hv sync
> +check_bfd_packets yes
> +
> +AS_BOX([BFD sessions back on separate gateways])
> +check ovn-nbctl --wait=hv \
> + lrp-del-gateway-chassis r0-ext2 gw1 -- \
> + lrp-set-gateway-chassis r0-ext2 gw2
> +chassis=$(fetch_column Chassis _uuid name=gw2)
> +wait_column "$chassis" Port_Binding chassis logical_port=cr-r0-ext2
> +check ovn-nbctl --wait=hv sync
> +check_bfd_packets no
> +
> +OVN_CLEANUP([gw1], [gw2])
The cleanup checks fail occasionally (both in GitHub CI and locally)
because at this point the "ext" localnet switch has multiple "peer"
ports to logical router "r0".
In physical.c we generate conjunctive flows for each of the peers in:
https://github.com/ovn-org/ovn/blob/3bc96bd8b9fa52241ce2b3fbd2b39ee58a958fee/controller/physical.c#L1154
But with a recompute (which is what the OVN_CLEANUP implementation does)
the order of the peer ports in the vector may change, resulting in
slightly different (equivalent) conjunctive flows.
We should fix that in OVN but until then let's skip the check for your test.
I added:
diff --git a/tests/ovn.at b/tests/ovn.at
index f647c886dc..783c822a3e 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -12945,7 +12945,11 @@ wait_column "$chassis" Port_Binding chassis
logical_port=cr-r0-ext2
check ovn-nbctl --wait=hv sync
check_bfd_packets no
-OVN_CLEANUP([gw1], [gw2])
+OVN_CLEANUP([gw1
+ignored_tables=OFTABLE_PHY_TO_LOG
+], [gw2
+ignored_tables=OFTABLE_PHY_TO_LOG
+])
AT_CLEANUP
]
> +AT_CLEANUP
> +])
> +
> OVN_FOR_EACH_NORTHD([
> AT_SETUP([4 HV, 1 LS, 1 LR, packet test with HA distributed router gateway
> port])
> ovn_start
With this fixed up, I applied the patch to main, 26.09 and 26.03. I
also added you to the AUTHORS.rst list.
Regards,
Dumitru
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
