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.

> 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.

> +[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.

> +
> +# 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?

Best regards, Ilya Maximets.

_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to