> On 2/26/25 17:21, Lorenzo Bianconi wrote:
> > Introduce the capability to actively refresh mac_binding entries available
> > in OFTABLE_MAC_BINDING table when they are inactive for more than the
> > configured threshold. Add the OFTABLE_MAC_BINDING table stats
> > monitoring. This feature avoids possible unnecessary mac_entry removals
> > if the destination is still alive but inactive for more than the configured
> > value.
> >
> > Acked-by: Mark Michelson <mmich...@redhat.com>
> > Reported-at: https://issues.redhat.com/browse/FDP-1135
> > Signed-off-by: Lorenzo Bianconi <lorenzo.bianc...@redhat.com>
> > ---
> > controller/mac-cache.c | 111 +++++++++++++++++-
> > controller/mac-cache.h | 26 ++++-
> > controller/ovn-controller.c | 40 +++++--
> > controller/pinctrl.c | 8 +-
> > controller/pinctrl.h | 7 ++
> > controller/statctrl.c | 25 +++-
> > tests/ovn.at | 221 ++++++++++++++++++++++++++++++++++++
> > tests/system-ovn.at | 108 ++++++++++++++++++
> > 8 files changed, 521 insertions(+), 25 deletions(-)
>
> Hi, Lorenzo. Not a full review, but see some comments for a system test
> below.
Hi Ilya,
thx for the review.
>
> > diff --git a/tests/system-ovn.at b/tests/system-ovn.at
> > index 9982da7fe..d5858bec7 100644
> > --- a/tests/system-ovn.at
> > +++ b/tests/system-ovn.at
> > @@ -17240,3 +17240,111 @@ OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port
> > patch-.*/d
> > /connection dropped.*/d"])
> > AT_CLEANUP
> > ])
> > +
> > +OVN_FOR_EACH_NORTHD([
> > +AT_SETUP([Mac binding aging - Probing])
> > +AT_KEYWORDS([mac_binding_probing])
> > +ovn_start
> > +OVS_TRAFFIC_VSWITCHD_START()
> > +
> > +ADD_BR([br-int])
> > +ADD_BR([br-ext])
> > +ADD_BR([br-gw])
> > +
> > +check ovs-ofctl add-flow br-ext action=normal
> > +check ovs-ofctl add-flow br-gw action=normal
> > +# Set external-ids in br-int needed for ovn-controller
> > +check ovs-vsctl \
> > + -- set Open_vSwitch . external-ids:system-id=hv1 \
> > + -- set Open_vSwitch .
> > external-ids:ovn-remote=unix:$ovs_base/ovn-sb/ovn-sb.sock \
> > + -- set Open_vSwitch . external-ids:ovn-encap-type=geneve \
> > + -- set Open_vSwitch . external-ids:ovn-encap-ip=169.0.0.1 \
> > + -- set bridge br-int fail-mode=secure
> > other-config:disable-in-band=true
> > +
> > +# Start ovn-controller
> > +start_daemon ovn-controller
> > +
> > +check ovn-appctl -t ovn-controller vlog/set mac_cache:file:dbg
> > pinctrl:file:dbg
> > +check ovn-nbctl lr-add lr
> > +check ovn-nbctl set logical_router lr options:chassis=hv1
> > +check ovn-nbctl set logical_router lr options:mac_binding_age_threshold=10
> > +
> > +check ovn-nbctl ls-add sw
> > +check ovn-nbctl ls-add public
> > +
> > +check ovn-nbctl lrp-add lr rp-sw 00:00:01:01:02:03 192.168.1.1/24
> > +check ovn-nbctl lrp-add lr rp-public 00:00:02:01:02:03 172.16.1.1/24
> > +
> > +check ovn-nbctl lsp-add sw sw-rp -- set Logical_Switch_Port sw-rp \
> > + type=router options:router-port=rp-sw \
> > + -- lsp-set-addresses sw-rp router
> > +
> > +check ovn-nbctl lsp-add public public-rp -- set Logical_Switch_Port
> > public-rp \
> > + type=router options:router-port=rp-public \
> > + -- lsp-set-addresses public-rp router
> > +
> > +ADD_NAMESPACES(alice)
> > +ADD_VETH(alice, alice, br-int, "192.168.1.2/24", "f0:00:00:01:02:03",
> > "192.168.1.1")
> > +check ovn-nbctl lsp-add sw alice -- lsp-set-addresses alice
> > "f0:00:00:01:02:03 192.168.1.2"
> > +
> > +check ovs-vsctl set Open_vSwitch .
> > external-ids:ovn-bridge-mappings=phynet:br-ext
> > +check ovn-nbctl lsp-add public public \
> > + -- lsp-set-addresses public unknown \
> > + -- lsp-set-type public localnet \
> > + -- lsp-set-options public network_name=phynet
> > +
> > +ADD_NAMESPACES(gw)
> > +ADD_VETH(gw0, gw, br-ext, "172.16.1.2/24", "f0:00:00:01:02:04",
> > "172.16.1.1")
> > +ADD_VETH(gw1, gw, br-gw, "172.16.2.2/24", "f0:00:00:01:03:04")
> > +
> > +NS_CHECK_EXEC([gw], [sysctl -w net.ipv4.conf.all.forwarding=1],[0], [dnl
> > +net.ipv4.conf.all.forwarding = 1
> > +])
> > +
> > +ADD_NAMESPACES(bob)
> > +ADD_VETH(bob, bob, br-gw, "172.16.2.10/24", "f0:00:00:01:02:06",
> > "172.16.2.2")
> > +
> > +check ovn-nbctl lr-route-add lr 0.0.0.0/0 172.16.1.2
> > +check ovn-nbctl --wait=hv sync
> > +
> > +NS_CHECK_EXEC([alice], [ping -q -c 3 -i 0.3 -w 2 172.16.2.10 |
> > FORMAT_PING], \
> > +[0], [dnl
> > +3 packets transmitted, 3 received, 0% packet loss, time 0ms
> > +])
> > +check_row_count mac_binding 1 mac=\"f0:00:00:01:02:04\"
> > +mac_binding_uuid=$(fetch_column mac_binding _uuid logical_port=rp-public
> > ip=172.16.1.2)
> > +
> > +NETNS_START_TCPDUMP([gw], [-n -c 2 -i gw0 arp[[6:2]] == 0x1 and
> > arp[[24:4]] == 0xac100102], [gw])
> > +NS_CHECK_EXEC([alice], [ping -q -c 20 -i 0.5 172.16.2.10 | FORMAT_PING], \
>
> Hmm. The aging threshold is 10, and we're sending 20 packets here with
> 0.5 second interval, so also 10 seconds. It's possible that the entry
> will just not age out on it's own, because we're on the edge of the allowed
> time. So, this test may actually pass even without re-ARP in some cases.
> northd is not precise in timing for entry removal and they can linger for
> quite some time after the threashold.
>
> Please, reduce the age threshold to some lower value, e.g. 4-5 seconds
> and ping for, let's say 12-15 seconds (3x the threshold). This would
> guarantee that we hit the threshold if something goes wrong with the
> probing.
ack, I will fix it in v5.
>
> > +[0], [dnl
> > +20 packets transmitted, 20 received, 0% packet loss, time 0ms
> > +])
> > +
> > +# Check mac binding entry is still active.
> > +check_row_count mac_binding 1 mac=\"f0:00:00:01:02:04\"
> > +OVS_WAIT_UNTIL([
> > + n_arp=$(cat gw.tcpdump | wc -l)
> > + test ${n_arp} -ge 2
> > +])
> > +
> > +AT_CHECK([test "$(fetch_column mac_binding _uuid logical_port=rp-public
> > ip=172.16.1.2)" = "$mac_binding_uuid"])
>
> This should be checked before waiting for tcpdump output to avoid
> potential flakes where the entry expires while we wait.
ack, I will fix it in v5.
>
> > +
> > +# Wait for the mac binding entry to expire.
> > +wait_row_count MAC_Binding 0
> > +
> > +OVS_APP_EXIT_AND_WAIT([ovn-controller])
> > +
> > +as ovn-sb
> > +OVS_APP_EXIT_AND_WAIT([ovsdb-server])
> > +
> > +as ovn-nb
> > +OVS_APP_EXIT_AND_WAIT([ovsdb-server])
> > +
> > +as northd
> > +OVS_APP_EXIT_AND_WAIT([ovn-northd])
> > +
> > +as
> > +OVS_TRAFFIC_VSWITCHD_STOP(["/.*error receiving.*/d
> > +/.*terminating with signal 15.*/d"])
>
> Are these actually needed in this test?
ack, I will fix it in v5.
Regards,
Lorenzo
>
> Best regards, Ilya Maximets.
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
