On 5/20/26 4:03 PM, Dumitru Ceara wrote:
> Hi Ales, Mark,
>
> On 5/19/26 9:39 PM, Mark Michelson wrote:
>> Thanks Ales,
>>
>> Acked-by: Mark Michelson <[email protected]>
>>
>
> Thanks for the patch and review, I have a few (minor) comments myself:
>
>> On Wed, May 13, 2026 at 3:04 AM Ales Musil via dev
>> <[email protected]> wrote:
>>>
>>> When LRP is configured with multiple IPs we could have MAC Bindings
>>> with IP addresses corresponding to any of the LRP subnets. However,
>>> the refresh mechanism would just choose the first IP as source.
>>> That could cause confusion for the switches along the way and it
>>> could also be dropped along the way. Make sure we always select
>>> the right IP. The linear lookup shouldn't cause any performance
>>> issues as in the practice there isn't too many IPs per LRP.
>>>
>>> Reported-at: https://redhat.atlassian.net/browse/FDP-3784
>>> Fixes: 1e4d4409f391 ("controller: Send ARP/ND for stale mac_bindings
>>> entries.")
>>> Assisted-by: Claude Opus 4.6, Claude Code
>>> Signed-off-by: Ales Musil <[email protected]>
>>> ---
>>> controller/mac-cache.c | 28 +++++++---
>>> tests/ovn.at | 113 +++++++++++++++++++++++++++++++++++++++++
>>> 2 files changed, 135 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/controller/mac-cache.c b/controller/mac-cache.c
>>> index bdf35eeb7..108d93f97 100644
>>> --- a/controller/mac-cache.c
>>> +++ b/controller/mac-cache.c
>>> @@ -920,13 +920,29 @@ mac_binding_probe_stats_run(struct vector *stats_vec,
>>> uint64_t *req_delay,
>>> continue;
>>> }
>>>
>>> - bool is_mb_v4 = IN6_IS_ADDR_V4MAPPED(&mb->data.ip);
>>> - if ((is_mb_v4 && laddr.n_ipv4_addrs)
>>> - || (!is_mb_v4 && laddr.n_ipv6_addrs)) {
>>> - struct in6_addr local =
>>> - is_mb_v4 ? in6_addr_mapped_ipv4(laddr.ipv4_addrs[0].addr)
>>> - : laddr.ipv6_addrs[0].addr;
>>> + struct in6_addr local = in6addr_any;
>>> + if (IN6_IS_ADDR_V4MAPPED(&mb->data.ip)) {
>>> + ovs_be32 ip4 = in6_addr_get_mapped_ipv4(&mb->data.ip);
>>> + for (size_t i = 0; i < laddr.n_ipv4_addrs; i++) {
>>> + struct ipv4_netaddr address = laddr.ipv4_addrs[i];
>>> + if (address.network == (ip4 & address.mask)) {
>>> + local = in6_addr_mapped_ipv4(address.addr);
>>> + break;
>>> + }
>>> + }
>>> + } else {
>>> + for (size_t i = 0; i < laddr.n_ipv6_addrs; i++) {
>>> + struct ipv6_netaddr address = laddr.ipv6_addrs[i];
>>> + struct in6_addr neigh_prefix =
>>> + ipv6_addr_bitand(&mb->data.ip, &address.mask);
>>> + if (ipv6_addr_equals(&address.network, &neigh_prefix)) {
>>> + local = address.addr;
>>> + break;
>>> + }
>>> + }
>>> + }
>>>
>>> + if (!ipv6_addr_equals(&local, &in6addr_any)) {
>>> mac_binding_update_log("Sending ARP/ND request for active",
>>> &mb->data, true, threshold,
>>> stats->idle_age_ms, since_updated_ms);
>>> diff --git a/tests/ovn.at b/tests/ovn.at
>>> index 2914d0e64..cd849f4db 100644
>>> --- a/tests/ovn.at
>>> +++ b/tests/ovn.at
>>> @@ -37258,6 +37258,119 @@ OVN_CLEANUP([hv1])
>>> AT_CLEANUP
>>> ])
>>>
>>> +OVN_FOR_EACH_NORTHD([
>>> +AT_SETUP([MAC binding aging - probing multi-subnet source IP])
>>> +AT_SKIP_IF([test $HAVE_SCAPY = no])
>>> +ovn_start
>>> +
>>> +aging_th=10
>>> +net_add n1
>>> +sim_add hv1
>>> +as hv1
>>> +check ovs-vsctl add-br br-phys
>>> +ovn_attach n1 br-phys 192.168.0.1
>>> +ovn-appctl -t ovn-controller vlog/set mac_cache:file:dbg pinctrl:file:dbg
>>> +
>>> +check ovn-nbctl
>>> \
>>> + -- ls-add ls1
>>> \
>>> + -- lr-add lr
>>> \
>>> + -- set logical_router lr options:mac_binding_age_threshold=$aging_th
>>> \
>>> + -- lrp-add lr lr-ls1 00:00:00:00:10:00 10.10.10.1/24 42.42.42.1/24
>>> \
>>> + fd11::1/64 fd12::1/64
>>> \
>>> + -- lsp-add-router-port ls1 ls1-lr lr-ls1
>>> \
>>> + -- lsp-add ls1 vif1
>>> \
>>> + -- lsp-set-addresses vif1 "unknown"
>>> +
>>> +check ovs-vsctl
>>> \
>>> + -- add-port br-int vif1
>>> \
>>> + -- set interface vif1 external-ids:iface-id=vif1
>>> \
>>> + options:tx_pcap=hv1/vif1-tx.pcap options:rxq_pcap=hv1/vif1-rx.pcap
>>> +
>>> +OVN_POPULATE_ARP
>>> +wait_for_ports_up
>>> +check ovn-nbctl --wait=hv sync
>>> +
>>> +# Wait for pinctrl thread to be connected.
>>> +OVS_WAIT_UNTIL([grep pinctrl hv1/ovn-controller.log | grep -q connected])
>>> +
>>> +# Create MAC bindings in both IPv4 subnets.
>>> +send_garp hv1 vif1 2 00:00:00:00:10:1a ff:ff:ff:ff:ff:ff 10.10.10.100
>>> 10.10.10.100
>>> +wait_row_count mac_binding 1 ip="10.10.10.100" logical_port="lr-ls1"
>>> +
>>> +send_garp hv1 vif1 2 00:00:00:00:10:1b ff:ff:ff:ff:ff:ff 42.42.42.253
>>> 42.42.42.253
>>> +wait_row_count mac_binding 1 ip="42.42.42.253" logical_port="lr-ls1"
>>> +
>>> +# Create MAC bindings in both IPv6 subnets.
>>> +send_na hv1 vif1 00:00:00:00:10:1a 00:00:00:00:10:00 fd11::64 fd11::1
>>> +wait_row_count mac_binding 1 ip=\"fd11::64\" logical_port=\"lr-ls1\"
>>> +
>>> +send_na hv1 vif1 00:00:00:00:10:1b 00:00:00:00:10:00 fd12::64 fd12::1
>>> +wait_row_count mac_binding 1 ip=\"fd12::64\" logical_port=\"lr-ls1\"
>>> +
>>> +# Record UUIDs for all MAC bindings.
>>> +uuid_v4_1=$(fetch_column Mac_Binding _uuid ip=10.10.10.100)
>>> +uuid_v4_2=$(fetch_column Mac_Binding _uuid ip=42.42.42.253)
>>> +uuid_v6_1=$(fetch_column Mac_Binding _uuid ip=\"fd11::64\")
>>> +uuid_v6_2=$(fetch_column Mac_Binding _uuid ip=\"fd12::64\")
>>> +
>>> +# Send IPv4 and IPv6 UDP traffic to refresh entries in OFTABLE_MAC_BINDING.
>>> +send_udp hv1 vif1 00:00:00:00:10:00 00:00:00:00:10:1a 42.42.42.253
>>> 10.10.10.100
>>> +send_udp hv1 vif1 00:00:00:00:10:00 00:00:00:00:10:1b 10.10.10.100
>>> 42.42.42.253
>>> +send_udp6 hv1 vif1 00:00:00:00:10:00 00:00:00:00:10:1a fd12::64 fd11::64
>>> +send_udp6 hv1 vif1 00:00:00:00:10:00 00:00:00:00:10:1b fd11::64 fd12::64
>>> +
>>> +OVS_WAIT_UNTIL([$(ovs-ofctl dump-flows br-int table=OFTABLE_MAC_BINDING | \
>>> + sed
>>> 's/reg15=0x.,metadata=0x./reg15=<cleared>,metadata=<cleared>/g' | \
>>> + grep -q
>>> "reg0=0xa0a0a64,reg15=<cleared>,metadata=<cleared>
>>> actions=mod_dl_dst:00:00:00:00:10:1a")])
>>> +OVS_WAIT_UNTIL([$(ovs-ofctl dump-flows br-int table=OFTABLE_MAC_BINDING | \
>>> + sed
>>> 's/reg15=0x.,metadata=0x./reg15=<cleared>,metadata=<cleared>/g' | \
>>> + grep -q
>>> "reg0=0x2a2a2afd,reg15=<cleared>,metadata=<cleared>
>>> actions=mod_dl_dst:00:00:00:00:10:1b")])
>
> Spotted by my "tiny little AI helper", quoting:
>
> Nit (test): The OVS_WAIT_UNTIL calls use OVS_WAIT_UNTIL([$(... | grep -q
> ...)]). The $() command substitution captures empty stdout (since grep
> -q is used) and evaluates the empty result as a command, which always
> succeeds. The correct pattern would be OVS_WAIT_UNTIL([ovs-ofctl
> dump-flows ... | sed ... | grep -q ...]) without the $() wrapper. Same
> issue exists in the adjacent test (pre-existing).
>
>>> +
>>> +# Wait until all entries in OFTABLE_MAC_CACHE_USE are stale.
>>> +OVS_WAIT_UNTIL([test $(ovs-ofctl dump-flows br-int
>>> table=OFTABLE_MAC_CACHE_USE | \
>>> + awk '/nw_src=10.10.10.100/{print substr($6,10,1)}')
>>> -ge $((aging_th/2))])
>>> +OVS_WAIT_UNTIL([test $(ovs-ofctl dump-flows br-int
>>> table=OFTABLE_MAC_CACHE_USE | \
>>> + awk '/nw_src=42.42.42.253/{print substr($6,10,1)}')
>>> -ge $((aging_th/2))])
>>> +OVS_WAIT_UNTIL([test $(ovs-ofctl dump-flows br-int
>>> table=OFTABLE_MAC_CACHE_USE | \
>>> + awk '/ipv6_src=fd11::64/{print substr($6,10,1)}')
>>> -ge $((aging_th/2))])
>>> +OVS_WAIT_UNTIL([test $(ovs-ofctl dump-flows br-int
>>> table=OFTABLE_MAC_CACHE_USE | \
>>> + awk '/ipv6_src=fd12::64/{print substr($6,10,1)}')
>>> -ge $((aging_th/2))])
>>> +
>>> +# Send traffic to trigger probing for all entries.
>>> +send_udp hv1 vif1 00:00:00:00:10:00 00:00:00:00:10:1a 42.42.42.253
>>> 10.10.10.100
>>> +send_udp hv1 vif1 00:00:00:00:10:00 00:00:00:00:10:1b 10.10.10.100
>>> 42.42.42.253
>>> +send_udp6 hv1 vif1 00:00:00:00:10:00 00:00:00:00:10:1a fd12::64 fd11::64
>>> +send_udp6 hv1 vif1 00:00:00:00:10:00 00:00:00:00:10:1b fd11::64 fd12::64
>>> +
>>> +# Verify ARP probes use the correct source IPs from matching subnets.
>>> +# ARP for 10.10.10.100 must use source IP 10.10.10.1 (first subnet).
>>> +dump_arp 1 00:00:00:00:10:00 ff:ff:ff:ff:ff:ff 10.10.10.1 10.10.10.100
>>> 00:00:00:00:00:00 > expected
>>> +# ARP for 42.42.42.253 must use source IP 42.42.42.1 (second subnet).
>>> +dump_arp 1 00:00:00:00:10:00 ff:ff:ff:ff:ff:ff 42.42.42.1 42.42.42.253
>>> 00:00:00:00:00:00 >> expected
>>> +OVN_CHECK_PACKETS_CONTAIN([hv1/vif1-tx.pcap], [expected])
>>> +
>>> +# Verify NS probes use the correct source IPs from matching subnets.
>>> +# NS for fd11::64 must use source IP fd11::1 (first IPv6 subnet).
>>> +dump_ns 33:33:ff:00:00:64 00:00:00:00:10:00 ff02::1:ff00:64 fd11::1
>>> fd11::64 > expected_v6
>>> +# NS for fd12::64 must use source IP fd12::1 (second IPv6 subnet).
>>> +dump_ns 33:33:ff:00:00:64 00:00:00:00:10:00 ff02::1:ff00:64 fd12::1
>>> fd12::64 >> expected_v6
>>> +OVN_CHECK_PACKETS_CONTAIN([hv1/vif1-tx.pcap], [expected_v6])
>
> There's a race here, I think, this failed in CI in my fork:
> https://github.com/dceara/ovn/actions/runs/26164612950/job/76965765667#step:12:5323
>
>>> +
>>> +# Send ARP/NA replies and check MAC_Binding UUIDs remain consistent.
>>> +send_garp hv1 vif1 2 00:00:00:00:10:1a 00:00:00:00:10:00 10.10.10.100
>>> 10.10.10.1
>>> +send_garp hv1 vif1 2 00:00:00:00:10:1b 00:00:00:00:10:00 42.42.42.253
>>> 42.42.42.1
>>> +send_na hv1 vif1 00:00:00:00:10:1a 00:00:00:00:10:00 fd11::64 fd11::1
>>> +send_na hv1 vif1 00:00:00:00:10:1b 00:00:00:00:10:00 fd12::64 fd12::1
>>> +
>>> +AT_CHECK([test "$(fetch_column Mac_Binding _uuid ip=10.10.10.100)" =
>>> "$uuid_v4_1"])
>>> +AT_CHECK([test "$(fetch_column Mac_Binding _uuid ip=42.42.42.253)" =
>>> "$uuid_v4_2"])
>>> +AT_CHECK([test "$(fetch_column Mac_Binding _uuid ip=\"fd11::64\")" =
>>> "$uuid_v6_1"])
>>> +AT_CHECK([test "$(fetch_column Mac_Binding _uuid ip=\"fd12::64\")" =
>>> "$uuid_v6_2"])
>>> +
>>> +OVN_CLEANUP([hv1])
>>> +AT_CLEANUP
>>> +])
>>> +
FWIW, my "tiny little AI helper" suggested the following incremental
which makes sense to me (it's also what we do in the other mac binding
probing test):
diff --git a/tests/ovn.at b/tests/ovn.at
index c1d3a7b80d..135b38750d 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -37312,17 +37312,20 @@ uuid_v6_1=$(fetch_column Mac_Binding _uuid
ip=\"fd11::64\")
uuid_v6_2=$(fetch_column Mac_Binding _uuid ip=\"fd12::64\")
# Send IPv4 and IPv6 UDP traffic to refresh entries in OFTABLE_MAC_BINDING.
-send_udp hv1 vif1 00:00:00:00:10:00 00:00:00:00:10:1a 42.42.42.253 10.10.10.100
-send_udp hv1 vif1 00:00:00:00:10:00 00:00:00:00:10:1b 10.10.10.100 42.42.42.253
-send_udp6 hv1 vif1 00:00:00:00:10:00 00:00:00:00:10:1a fd12::64 fd11::64
-send_udp6 hv1 vif1 00:00:00:00:10:00 00:00:00:00:10:1b fd11::64 fd12::64
+# Use different src MACs than the ones from GARP/NA to avoid resetting
+# OFTABLE_MAC_CACHE_USE idle_age (which would keep the timestamp fresh
+# and suppress probing via the cooldown mechanism).
+send_udp hv1 vif1 00:00:00:00:10:00 00:00:00:00:10:2a 42.42.42.253 10.10.10.100
+send_udp hv1 vif1 00:00:00:00:10:00 00:00:00:00:10:2b 10.10.10.100 42.42.42.253
+send_udp6 hv1 vif1 00:00:00:00:10:00 00:00:00:00:10:2a fd12::64 fd11::64
+send_udp6 hv1 vif1 00:00:00:00:10:00 00:00:00:00:10:2b fd11::64 fd12::64
-OVS_WAIT_UNTIL([$(ovs-ofctl dump-flows br-int table=OFTABLE_MAC_BINDING | \
+OVS_WAIT_UNTIL([ovs-ofctl dump-flows br-int table=OFTABLE_MAC_BINDING | \
sed
's/reg15=0x.,metadata=0x./reg15=<cleared>,metadata=<cleared>/g' | \
- grep -q "reg0=0xa0a0a64,reg15=<cleared>,metadata=<cleared>
actions=mod_dl_dst:00:00:00:00:10:1a")])
-OVS_WAIT_UNTIL([$(ovs-ofctl dump-flows br-int table=OFTABLE_MAC_BINDING | \
+ grep -q "reg0=0xa0a0a64,reg15=<cleared>,metadata=<cleared>
actions=mod_dl_dst:00:00:00:00:10:1a"])
+OVS_WAIT_UNTIL([ovs-ofctl dump-flows br-int table=OFTABLE_MAC_BINDING | \
sed
's/reg15=0x.,metadata=0x./reg15=<cleared>,metadata=<cleared>/g' | \
- grep -q "reg0=0x2a2a2afd,reg15=<cleared>,metadata=<cleared>
actions=mod_dl_dst:00:00:00:00:10:1b")])
+ grep -q "reg0=0x2a2a2afd,reg15=<cleared>,metadata=<cleared>
actions=mod_dl_dst:00:00:00:00:10:1b"])
# Wait until all entries in OFTABLE_MAC_CACHE_USE are stale.
OVS_WAIT_UNTIL([test $(ovs-ofctl dump-flows br-int table=OFTABLE_MAC_CACHE_USE
| \
@@ -37335,10 +37338,11 @@ OVS_WAIT_UNTIL([test $(ovs-ofctl dump-flows br-int
table=OFTABLE_MAC_CACHE_USE |
awk '/ipv6_src=fd12::64/{print substr($6,10,1)}') -ge
$((aging_th/2))])
# Send traffic to trigger probing for all entries.
-send_udp hv1 vif1 00:00:00:00:10:00 00:00:00:00:10:1a 42.42.42.253 10.10.10.100
-send_udp hv1 vif1 00:00:00:00:10:00 00:00:00:00:10:1b 10.10.10.100 42.42.42.253
-send_udp6 hv1 vif1 00:00:00:00:10:00 00:00:00:00:10:1a fd12::64 fd11::64
-send_udp6 hv1 vif1 00:00:00:00:10:00 00:00:00:00:10:1b fd11::64 fd12::64
+# Again use different src MACs to only hit OFTABLE_MAC_BINDING (not
MAC_CACHE_USE).
+send_udp hv1 vif1 00:00:00:00:10:00 00:00:00:00:10:2a 42.42.42.253 10.10.10.100
+send_udp hv1 vif1 00:00:00:00:10:00 00:00:00:00:10:2b 10.10.10.100 42.42.42.253
+send_udp6 hv1 vif1 00:00:00:00:10:00 00:00:00:00:10:2a fd12::64 fd11::64
+send_udp6 hv1 vif1 00:00:00:00:10:00 00:00:00:00:10:2b fd11::64 fd12::64
# Verify ARP probes use the correct source IPs from matching subnets.
# ARP for 10.10.10.100 must use source IP 10.10.10.1 (first subnet).
---
Regards,
Dumitru
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
