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 >> +]) >> + >> OVN_FOR_EACH_NORTHD([ >> AT_SETUP([MAC binding aging - probing distributed GW router]) >> AT_SKIP_IF([test $HAVE_SCAPY = no]) >> -- >> 2.54.0 >> Regards, Dumitru _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
