Hi Ales, I have only one issue below, with the test. I think this can be fixed during merge, so with that addressed,
Acked-by: Mark Michelson <[email protected]> On Mon, Aug 31, 2026 at 2:21 AM Ales Musil via dev <[email protected]> wrote: > > The re-ARP probes were using unicast to check if the host is still > alive to prevent the entry from aging out. The unicast works fine > and prevents unnecessary floods, however, there is a case when the > MAC address could have been changed without OVN learning about that. > In that case the unicast won't be ever responded to and the only way > to refresh that entry is to wait for it to age out. Send broadcast > after two unicast attempts with the timing that gives us usually > 2 unicast probes and 2 broadcast, we will age out if neither of them > is responded to. > > Fixes: 59c7361e2613 ("pinctrl: Use unicast for MAC binding ARP probe.") > Reported-at: https://redhat.atlassian.net/browse/FDP-4224 > Assisted-by: Claude Opus 4.6, OpenCode > Signed-off-by: Ales Musil <[email protected]> > --- > v2: Rebase on top of latest main. > Update wrong numbers in comments. > Change the threshold name. > --- > controller/mac-cache.c | 11 ++++- > controller/mac-cache.h | 2 + > tests/ovn.at | 102 +++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 114 insertions(+), 1 deletion(-) > > diff --git a/controller/mac-cache.c b/controller/mac-cache.c > index 8f100a29d..359d4c18f 100644 > --- a/controller/mac-cache.c > +++ b/controller/mac-cache.c > @@ -34,6 +34,7 @@ VLOG_DEFINE_THIS_MODULE(mac_cache); > #define BUFFER_QUEUE_DEPTH 4 > #define BUFFERED_PACKETS_TIMEOUT_MS 10000 > #define BUFFERED_PACKETS_LOOKUP_MS 100 > +#define PROBE_MULICAST_THRESHOLD 2 > > static uint32_t > mac_binding_data_hash(const struct mac_binding_data *mb_data); > @@ -178,6 +179,7 @@ mac_binding_add(struct hmap *map, struct mac_binding_data > mb_data, > mb->data = mb_data; > mb->sbrec = smb; > mb->timestamp = timestamp; > + mb->arp_attempts = 0; > mac_binding_update_log("Added", &mb_data, false, NULL, 0, 0); > } > > @@ -908,6 +910,7 @@ mac_binding_probe_stats_run(struct vector *stats_vec, > uint64_t *req_delay, > "Not sending ARP/ND request for recently updated", > &mb->data, true, threshold, stats->idle_age_ms, > since_updated_ms); > + mb->arp_attempts = 0; > continue; > } > > @@ -954,6 +957,11 @@ mac_binding_probe_stats_run(struct vector *stats_vec, > uint64_t *req_delay, > } > > if (!ipv6_addr_equals(&local, &in6addr_any)) { > + struct eth_addr eth_dst = > + mb->arp_attempts < PROBE_MULICAST_THRESHOLD > + ? mb->data.mac > + : eth_addr_zero; > + > mac_binding_update_log("Sending ARP/ND request for active", > &mb->data, true, threshold, > stats->idle_age_ms, since_updated_ms); > @@ -961,9 +969,10 @@ mac_binding_probe_stats_run(struct vector *stats_vec, > uint64_t *req_delay, > send_self_originated_neigh_packet(probe_data->swconn, > sbrec->datapath->tunnel_key, > pb->tunnel_key, laddr.ea, > - mb->data.mac, &local, > + eth_dst, &local, > &mb->data.ip, > OFTABLE_LOCAL_OUTPUT); > + mb->arp_attempts++; > } > > destroy_lport_addresses(&laddr); > diff --git a/controller/mac-cache.h b/controller/mac-cache.h > index 8abea60c7..bf9afaf3a 100644 > --- a/controller/mac-cache.h > +++ b/controller/mac-cache.h > @@ -78,6 +78,8 @@ struct mac_binding { > const struct sbrec_mac_binding *sbrec; > /* User specified timestamp (in ms) */ > long long timestamp; > + /* Number of re-ARP attempts for given entry. */ > + size_t arp_attempts; > }; > > struct fdb_data { > diff --git a/tests/ovn.at b/tests/ovn.at > index a88a077c6..94e4927eb 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -37563,6 +37563,108 @@ OVN_CLEANUP([hv1]) > AT_CLEANUP > ]) > > +OVN_FOR_EACH_NORTHD([ > +AT_SETUP([MAC binding aging - probing unicast to broadcast transition]) > +CHECK_SCAPY > +ovn_start > + > +aging_th=5 > +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 one IPv4 and one IPv6 MAC binding. > +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_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\" > + > +# The first 2 probes are unicast (arp_attempts 0-1); the entry then falls > +# back to broadcast ARP / multicast NS. > +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_bcast > +OVN_CHECK_PACKETS_CONTAIN([hv1/vif1-tx.pcap], [expected_bcast]) > + > +dump_ns 33:33:ff:00:00:64 00:00:00:00:10:00 ff02::1:ff00:64 fd11::1 fd11::64 > > expected_mcast > +OVN_CHECK_PACKETS_CONTAIN([hv1/vif1-tx.pcap], [expected_mcast]) > + > +dump_arp 1 00:00:00:00:10:00 00:00:00:00:10:1a 10.10.10.1 10.10.10.100 > 00:00:00:00:10:1a > expected_ucast_v4 > +OVN_CHECK_PACKETS_CONTAIN([hv1/vif1-tx.pcap], [expected_ucast_v4]) > + > +dump_ns 00:00:00:00:10:1a 00:00:00:00:10:00 fd11::64 fd11::1 fd11::64 > > expected_ucast_v6 > +OVN_CHECK_PACKETS_CONTAIN([hv1/vif1-tx.pcap], [expected_ucast_v6]) > + > +# Verify the reset-to-zero mechanism using a distinct pair of neighbours > +# kept alive (never re-created) for the whole check. A single entry emits > +# at most ARP_BROADCAST_THRESHOLD (2) unicast probes before falling back to > +# broadcast, so a 4th unicast probe proves arp_attempts was reset. > +send_garp hv1 vif1 2 00:00:00:00:10:1b ff:ff:ff:ff:ff:ff 10.10.10.101 > 10.10.10.101 > +wait_row_count mac_binding 1 ip="10.10.10.101" logical_port="lr-ls1" > +v4_uuid=$(fetch_column Mac_Binding _uuid ip=10.10.10.101) > + > +send_na hv1 vif1 00:00:00:00:10:1b 00:00:00:00:10:00 fd11::65 fd11::1 > +wait_row_count mac_binding 1 ip=\"fd11::65\" logical_port=\"lr-ls1\" > +v6_uuid=$(fetch_column Mac_Binding _uuid ip=\"fd11::65\") > + > +# Keep the entries active (Tx towards them) and let a couple of unicast > +# probes go out (arp_attempts reaches 1, below the broadcast threshold). > +send_udp hv1 vif1 00:00:00:00:10:00 00:00:00:00:10:2a 10.10.10.101 > 42.42.42.100 > +send_udp6 hv1 vif1 00:00:00:00:10:00 00:00:00:00:10:2a fd11::65 fd12::100 > +OVS_WAIT_UNTIL([test $(grep -c "Sending ARP/ND.*ip: 10.10.10.101" > hv1/ovn-controller.log) -ge 2]) > +OVS_WAIT_UNTIL([test $(grep -c "Sending ARP/ND.*ip: fd11::65" > hv1/ovn-controller.log) -ge 2]) I think it would make more sense to check the pcap file for these unicast ARPs. Just because a log message says it has sent an ARP/ND, it doesn't mean we can trust it. This also allows us to change log messages without having to worry about tests failing as a result. > + > +# The neighbours answer, refreshing the rows in place (resetting > +# arp_attempts). Confirm the timestamps advanced. > +v4_ts=$(fetch_column Mac_Binding timestamp ip=10.10.10.101) > +v6_ts=$(fetch_column Mac_Binding timestamp ip=\"fd11::65\") > +send_garp hv1 vif1 2 00:00:00:00:10:1b 00:00:00:00:10:00 10.10.10.101 > 10.10.10.1 > +send_na hv1 vif1 00:00:00:00:10:1b 00:00:00:00:10:00 fd11::65 fd11::1 > +OVS_WAIT_UNTIL([test $(fetch_column Mac_Binding timestamp ip=10.10.10.101) > -gt $v4_ts]) > +OVS_WAIT_UNTIL([test $(fetch_column Mac_Binding timestamp ip=\"fd11::65\") > -gt $v6_ts]) > + > +# Probing must restart from unicast: wait for a 4th unicast ARP/NS probe > +# while the rows are still the original ones (same UUID, never re-created). > +dump_arp 1 00:00:00:00:10:00 00:00:00:00:10:1b 10.10.10.1 10.10.10.101 > 00:00:00:00:10:1b > ucast_v4.pkt > +dump_ns 00:00:00:00:10:1b 00:00:00:00:10:00 fd11::65 fd11::1 fd11::65 > > ucast_v6.pkt > +OVS_WAIT_UNTIL([ > + send_udp hv1 vif1 00:00:00:00:10:00 00:00:00:00:10:2a 10.10.10.101 > 42.42.42.100 > + send_udp6 hv1 vif1 00:00:00:00:10:00 00:00:00:00:10:2a fd11::65 fd12::100 > + test "$(fetch_column Mac_Binding _uuid ip=10.10.10.101)" = "$v4_uuid" && > \ > + test "$(fetch_column Mac_Binding _uuid ip=\"fd11::65\")" = "$v6_uuid" && > \ > + test $($PYTHON "$ovs_srcdir/utilities/ovs-pcap.in" hv1/vif1-tx.pcap | \ > + grep -Fc "$(cat ucast_v4.pkt)") -ge 4 && \ > + test $($PYTHON "$ovs_srcdir/utilities/ovs-pcap.in" hv1/vif1-tx.pcap | \ > + grep -Fc "$(cat ucast_v6.pkt)") -ge 4]) > + > +OVN_CLEANUP([hv1]) > +AT_CLEANUP > +]) > + > OVN_FOR_EACH_NORTHD([ > AT_SETUP([MAC binding aging - probing distributed GW router]) > CHECK_SCAPY > -- > 2.55.0 > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
