On Wed, May 20, 2026 at 11:03 PM Dumitru Ceara <[email protected]> wrote:
> On 5/20/26 4:03 PM, Dumitru Ceara wrote: > > Hi Ales, Mark, > Hi Mark and Dumitru, thank you for the review. > > > > 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 > <http://10.10.10.100/%7Bprint> 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 > <http://42.42.42.253/%7Bprint> 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). > I have applied to diff in v2. It seems to work as I didn't see a single failure while running it in a loop. > --- > > Regards, > Dumitru > > Regards, Ales _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
