On Thu, May 16, 2024 at 10:09 PM Indrajitt Valsaraj <
[email protected]> wrote:
>
> Issue:
> In case of a Logical_Router without mac_binding_age_threshold set or a
> Logical_Router with an incorrectly formatted mac_binding_threshold
> option, entries were not being purged from the Mac Binding table in
> SouthBound.
>
> This was because in the function `en_mac_binding_aging_run` in case of
> an invalid mac_binding_threshold entry or if mac_binding_threshold is
> not set we are returning from the loop instead of iterating through the
> remaining LRs. As a result, subsequent runs of the aging_waker node are
> also not scehduled and we end up not purging any MAC Bindings.
>
> Fix:
> This patch fixes this issue by changing the return to a continue so that
> we skip the current LR but continue processing for the remaining LRs.
>
> Fixes: 78851b6ffb58 ("Support CIDR-based MAC binding aging threshold.")
> Signed-off-by: Indrajitt Valsaraj <[email protected]>
> Acked-by: Naveen Yerramneni <[email protected]>
>
Hi Indrajitt,
Thanks for reporting and fixing the issue. Please see my comments below on
the test case.
> ---
> v1:
> - Addressed review comment from Ales
> v2:
> - Fix test failure
> ---
> northd/aging.c | 2 +-
> tests/ovn.at | 107 +++++++++++++++++++++++++++++++++++--------------
> 2 files changed, 77 insertions(+), 32 deletions(-)
>
> diff --git a/northd/aging.c b/northd/aging.c
> index b76963a2d..9685044e7 100644
> --- a/northd/aging.c
> +++ b/northd/aging.c
> @@ -421,7 +421,7 @@ en_mac_binding_aging_run(struct engine_node *node,
void *data OVS_UNUSED)
> if (!parse_aging_threshold(smap_get(&od->nbr->options,
> "mac_binding_age_threshold"),
> &threshold_config)) {
> - return;
> + continue;
> }
>
> aging_context_set_threshold(&ctx, &threshold_config);
> diff --git a/tests/ovn.at b/tests/ovn.at
> index 486680649..5ab64ae9b 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -34414,10 +34414,15 @@ AT_CHECK([ovn-nbctl lsp-set-addresses ln_port
unknown])
> AT_CHECK([ovn-nbctl lsp-set-type ln_port localnet])
> AT_CHECK([ovn-nbctl lsp-set-options ln_port network_name=physnet1])
>
> -AT_CHECK([ovn-nbctl lsp-add public public-gw])
> -AT_CHECK([ovn-nbctl lsp-set-type public-gw router])
> -AT_CHECK([ovn-nbctl lsp-set-addresses public-gw 00:00:00:00:10:00
router])
> -AT_CHECK([ovn-nbctl lsp-set-options public-gw router-port=gw-public])
> +AT_CHECK([ovn-nbctl lsp-add public public-gw-1])
> +AT_CHECK([ovn-nbctl lsp-set-type public-gw-1 router])
> +AT_CHECK([ovn-nbctl lsp-set-addresses public-gw-1 00:00:00:00:10:00
router])
> +AT_CHECK([ovn-nbctl lsp-set-options public-gw-1 router-port=gw-1-public])
> +
> +AT_CHECK([ovn-nbctl lsp-add public public-gw-2])
> +AT_CHECK([ovn-nbctl lsp-set-type public-gw-2 router])
> +AT_CHECK([ovn-nbctl lsp-set-addresses public-gw-2 00:00:00:00:30:00
router])
> +AT_CHECK([ovn-nbctl lsp-set-options public-gw-2 router-port=gw-2-public])
>
> AT_CHECK([ovn-nbctl lsp-add internal internal-gw])
> AT_CHECK([ovn-nbctl lsp-set-type internal-gw router])
> @@ -34430,9 +34435,12 @@ AT_CHECK([ovn-nbctl lsp-set-addresses vif1
"00:00:00:00:20:10 192.168.20.10"])
> AT_CHECK([ovn-nbctl lsp-add internal vif2])
> AT_CHECK([ovn-nbctl lsp-set-addresses vif2 "00:00:00:00:20:20
192.168.20.20"])
>
> -AT_CHECK([ovn-nbctl lr-add gw])
> -AT_CHECK([ovn-nbctl lrp-add gw gw-public 00:00:00:00:10:00
192.168.10.1/24])
> -AT_CHECK([ovn-nbctl lrp-add gw gw-internal 00:00:00:00:20:00
192.168.20.1/24])
> +AT_CHECK([ovn-nbctl lr-add gw-1])
> +AT_CHECK([ovn-nbctl lrp-add gw-1 gw-1-public 00:00:00:00:10:00
192.168.10.1/24])
> +AT_CHECK([ovn-nbctl lrp-add gw-1 gw-internal 00:00:00:00:20:00
192.168.20.1/24])
> +
> +AT_CHECK([ovn-nbctl lr-add gw-2])
> +AT_CHECK([ovn-nbctl lrp-add gw-2 gw-2-public 00:00:00:00:30:00
192.168.10.2/24])
>
> sim_add hv1
> as hv1
> @@ -34500,21 +34508,27 @@ send_udp() {
> as $hv ovs-appctl netdev-dummy/receive $dev $packet
> }
> # Check if the option is not present by default
> -AT_CHECK([fetch_column nb:logical_router options name="gw" | grep -q
mac_binding_age_threshold], [1])
> +AT_CHECK([fetch_column nb:logical_router options name="gw-1" | grep -q
mac_binding_age_threshold], [1])
> +AT_CHECK([fetch_column nb:logical_router options name="gw-2" | grep -q
mac_binding_age_threshold], [1])
>
> # Send GARP to populate MAC binding table records
> send_garp hv1 ext1 10
> send_garp hv2 ext2 20
>
> -wait_row_count mac_binding 1 ip="192.168.10.10"
> -wait_row_count mac_binding 1 ip="192.168.10.20"
> +# Two rows present for each IP, one corresponding to each logical_port
> +wait_row_count mac_binding 2 ip="192.168.10.10"
> +wait_row_count mac_binding 2 ip="192.168.10.20"
>
> -dp_key=$(printf "0x%x" $(as hv1 fetch_column datapath tunnel_key
external_ids:name=gw))
> -port_key=$(printf "0x%x" $(as hv1 fetch_column port_binding tunnel_key
logical_port=gw-public))
> +dp_key_1=$(printf "0x%x" $(as hv1 fetch_column datapath tunnel_key
external_ids:name=gw-1))
> +port_key_1=$(printf "0x%x" $(as hv1 fetch_column port_binding tunnel_key
logical_port=gw-1-public))
> +dp_key_2=$(printf "0x%x" $(as hv1 fetch_column datapath tunnel_key
external_ids:name=gw-2))
> +port_key_2=$(printf "0x%x" $(as hv1 fetch_column port_binding tunnel_key
logical_port=gw-2-public))
>
> AT_CHECK_UNQUOTED([as hv1 ovs-ofctl dump-flows br-int
table=OFTABLE_MAC_CACHE_USE --no-stats | strip_cookie | sort], [0], [dnl
> - table=OFTABLE_MAC_CACHE_USE,
priority=100,ip,reg14=${port_key},metadata=${dp_key},dl_src=00:00:00:00:10:10,nw_src=192.168.10.10
actions=drop
> - table=OFTABLE_MAC_CACHE_USE,
priority=100,ip,reg14=${port_key},metadata=${dp_key},dl_src=00:00:00:00:10:20,nw_src=192.168.10.20
actions=drop
> + table=OFTABLE_MAC_CACHE_USE,
priority=100,ip,reg14=${port_key_1},metadata=${dp_key_1},dl_src=00:00:00:00:10:10,nw_src=192.168.10.10
actions=drop
> + table=OFTABLE_MAC_CACHE_USE,
priority=100,ip,reg14=${port_key_1},metadata=${dp_key_1},dl_src=00:00:00:00:10:20,nw_src=192.168.10.20
actions=drop
> + table=OFTABLE_MAC_CACHE_USE,
priority=100,ip,reg14=${port_key_2},metadata=${dp_key_2},dl_src=00:00:00:00:10:10,nw_src=192.168.10.10
actions=drop
> + table=OFTABLE_MAC_CACHE_USE,
priority=100,ip,reg14=${port_key_2},metadata=${dp_key_2},dl_src=00:00:00:00:10:20,nw_src=192.168.10.20
actions=drop
> ])
>
> timestamp=$(fetch_column mac_binding timestamp ip="192.168.10.20")
> @@ -34525,8 +34539,8 @@ send_udp hv2 ext2 20
> OVS_WAIT_UNTIL([as hv1 ovs-ofctl dump-flows br-int
table=OFTABLE_MAC_CACHE_USE | grep "192.168.10.10" | grep -q "n_packets=1"])
> OVS_WAIT_UNTIL([as hv2 ovs-ofctl dump-flows br-int
table=OFTABLE_MAC_CACHE_USE | grep "192.168.10.20" | grep -q "n_packets=1"])
>
> -# Set the MAC binding aging threshold
> -AT_CHECK([ovn-nbctl set logical_router gw
options:mac_binding_age_threshold=5])
> +# Set the MAC binding aging threshold for gw-1 router. No option for
gw-2 router.
> +AT_CHECK([ovn-nbctl set logical_router gw-1
options:mac_binding_age_threshold=5])
> AT_CHECK([fetch_column nb:logical_router options | grep -q
mac_binding_age_threshold=5])
> AT_CHECK([ovn-nbctl --wait=sb sync])
>
> @@ -34542,21 +34556,24 @@ OVS_WAIT_UNTIL([
> ])
> check test "$(fetch_column mac_binding timestamp ip='192.168.10.20')" !=
""
>
> -# Check if the records are removed after some inactivity
> +# Check if the records are removed after some inactivity for gw-1. Only
1 entry should be present for gw-2.
> OVS_WAIT_UNTIL([
> - test "0" = "$(ovn-sbctl list mac_binding | grep -c '192.168.10.10')"
> + test "1" = "$(ovn-sbctl list mac_binding | grep -c '192.168.10.10')"
> ])
> # The second one takes longer because it got refreshed
> OVS_WAIT_UNTIL([
> - test "0" = "$(ovn-sbctl list mac_binding | grep -c '192.168.10.20')"
> + test "1" = "$(ovn-sbctl list mac_binding | grep -c '192.168.10.20')"
> ])
>
> -AT_CHECK([as hv1 ovs-ofctl dump-flows br-int table=OFTABLE_MAC_CACHE_USE
--no-stats | strip_cookie], [0], [])
> +AT_CHECK_UNQUOTED([as hv1 ovs-ofctl dump-flows br-int
table=OFTABLE_MAC_CACHE_USE --no-stats | strip_cookie | sort], [0], [dnl
> + table=OFTABLE_MAC_CACHE_USE,
priority=100,ip,reg14=${port_key_2},metadata=${dp_key_2},dl_src=00:00:00:00:10:10,nw_src=192.168.10.10
actions=drop
> + table=OFTABLE_MAC_CACHE_USE,
priority=100,ip,reg14=${port_key_2},metadata=${dp_key_2},dl_src=00:00:00:00:10:20,nw_src=192.168.10.20
actions=drop
> +])
>
> # Test CIDR-based threshold configuration
> -check ovn-nbctl set logical_router gw options:mac_binding_age_threshold="
192.168.10.0/255.255.255.0:2;192.168.10.64/26:0;192.168.10.20:0"
> +check ovn-nbctl set logical_router gw-1
options:mac_binding_age_threshold="
192.168.10.0/255.255.255.0:2;192.168.10.64/26:0;192.168.10.20:0"
> check ovn-nbctl --wait=sb sync
> -uuid=$(fetch_column datapath _uuid external_ids:name=gw)
> +uuid=$(fetch_column datapath _uuid external_ids:name=gw-1)
> AT_CHECK([ovn-sbctl get datapath $uuid
external_ids:mac_binding_age_threshold], [0], [dnl
> "2"
> ])
> @@ -34566,12 +34583,12 @@ send_garp hv1 ext1 10 # belong to
192.168.10.0/24
> send_garp hv2 ext2 20 # belong to 192.168.10.20/32
> send_garp hv2 ext2 65 # belong to 192.168.10.64/26
>
> -OVS_WAIT_UNTIL([ovn-sbctl list mac_binding | grep -q "192.168.10.10"])
> -OVS_WAIT_UNTIL([ovn-sbctl list mac_binding | grep -q "192.168.10.20"])
> -OVS_WAIT_UNTIL([ovn-sbctl list mac_binding | grep -q "192.168.10.65"])
> +wait_row_count mac_binding 2 ip="192.168.10.10"
> +wait_row_count mac_binding 2 ip="192.168.10.20"
> +wait_row_count mac_binding 2 ip="192.168.10.65"
This is good. However, several places below are missing similar changes,
such as:
AT_CHECK([ovn-sbctl list mac_binding | grep -q "192.168.10.20"])
...
or
OVS_WAIT_UNTIL([ovn-sbctl list mac_binding | grep -q "192.168.10.10"])
...
We should check if the count is 2 instead of just checking if the row
exists.
However, it is not very straightforward to check the count. Since here we
only focus on the gw-1, maybe it is more clear just to change "list
mac_binding" to "find mac_binding logical_port=gw-1-public". Of course, if
we want to make it look more concise, we can use
wait_row_count/check_row_count to replace OVS_WAIT_UNTIL/AT_CHECK + grep.
What do you think?
>
> OVS_WAIT_UNTIL([
> - test "0" = "$(ovn-sbctl list mac_binding | grep -c '192.168.10.10')"
> + test "1" = "$(ovn-sbctl list mac_binding | grep -c '192.168.10.10')"
> ])
> # The other two should remain because the corresponding prefixes have
threshold 0
> AT_CHECK([ovn-sbctl list mac_binding | grep -q "192.168.10.20"])
As mentioned above, here we should change to:
AT_CHECK([ovn-sbctl find mac_binding logical_port=gw-1-public | grep -q
"192.168.10.20"])
or
check_row_count mac_binding 1 ip="192.168.10.20" logical_port=gw-1-public
> @@ -34579,9 +34596,9 @@ AT_CHECK([ovn-sbctl list mac_binding | grep -q
"192.168.10.65"])
> check ovn-sbctl --all destroy mac_binding
>
> # Set the aging threshold mixed with IPv6 prefixes and default threshold
> -check ovn-nbctl set logical_router gw
options:mac_binding_age_threshold="2;
192.168.10.64/26:0;ff00:1234::/32:888;ff00::abcd:1"
> +check ovn-nbctl set logical_router gw-1
options:mac_binding_age_threshold="2;
192.168.10.64/26:0;ff00:1234::/32:888;ff00::abcd:1"
> check ovn-nbctl --wait=sb sync
> -uuid=$(fetch_column datapath _uuid external_ids:name=gw)
> +uuid=$(fetch_column datapath _uuid external_ids:name=gw-1)
> AT_CHECK([ovn-sbctl get datapath $uuid
external_ids:mac_binding_age_threshold], [0], [dnl
> "1"
> ])
> @@ -34594,15 +34611,15 @@ OVS_WAIT_UNTIL([ovn-sbctl list mac_binding |
grep -q "192.168.10.10"])
> OVS_WAIT_UNTIL([ovn-sbctl list mac_binding | grep -q "192.168.10.65"])
Similar here. Should be:
OVS_WAIT_UNTIL([ovn-sbctl list mac_binding logical_port=gw-1-public | grep
-q "192.168.10.65"])
or
wait_row_count mac_binding 1 ip="192.168.10.65" logical_port=gw-1-public
>
> OVS_WAIT_UNTIL([
> - test "0" = "$(ovn-sbctl list mac_binding | grep -c '192.168.10.10')"
> + test "1" = "$(ovn-sbctl list mac_binding | grep -c '192.168.10.10')"
> ])
> AT_CHECK([ovn-sbctl list mac_binding | grep -q "192.168.10.65"])
Here, too.
> check ovn-sbctl --all destroy mac_binding
>
> # Set the aging threshold with invalid format
> -check ovn-nbctl set logical_router gw
options:mac_binding_age_threshold="1;abc/26:0"
> +check ovn-nbctl set logical_router gw-1
options:mac_binding_age_threshold="1;abc/26:0"
> check ovn-nbctl --wait=sb sync
> -uuid=$(fetch_column datapath _uuid external_ids:name=gw)
> +uuid=$(fetch_column datapath _uuid external_ids:name=gw-1)
> AT_CHECK([ovn-sbctl get datapath $uuid
external_ids:mac_binding_age_threshold], [1], [ignore], [ignore])
>
> # Send GARP to populate MAC binding table records
> @@ -34612,6 +34629,34 @@ OVS_WAIT_UNTIL([ovn-sbctl list mac_binding |
grep -q "192.168.10.10"])
Same here.
> # The record is not deleted
> sleep 5
> AT_CHECK([ovn-sbctl list mac_binding | grep -q "192.168.10.10"])
Same here.
Thanks,
Han
> +check ovn-sbctl --all destroy mac_binding
> +
> +# Set the aging threshold on both routers and ensure that they are aged
out of both the routers
> +AT_CHECK([ovn-nbctl set logical_router gw-1
options:mac_binding_age_threshold=5])
> +AT_CHECK([ovn-nbctl set logical_router gw-2
options:mac_binding_age_threshold=5])
> +check ovn-nbctl --wait=sb sync
> +uuid=$(fetch_column datapath _uuid external_ids:name=gw-1)
> +AT_CHECK([ovn-sbctl get datapath $uuid
external_ids:mac_binding_age_threshold], [0], [dnl
> +"5"
> +])
> +uuid=$(fetch_column datapath _uuid external_ids:name=gw-2)
> +AT_CHECK([ovn-sbctl get datapath $uuid
external_ids:mac_binding_age_threshold], [0], [dnl
> +"5"
> +])
> +
> +# Send GARP to populate MAC binding table records
> +send_garp hv1 ext1 10 # belong to 192.168.10.0/24
> +send_garp hv2 ext2 20 # belong to 192.168.10.20/32
> +
> +wait_row_count mac_binding 2 ip="192.168.10.10"
> +wait_row_count mac_binding 2 ip="192.168.10.20"
> +
> +OVS_WAIT_UNTIL([
> + test "0" = "$(ovn-sbctl list mac_binding | grep -c '192.168.10.10')"
> +])
> +OVS_WAIT_UNTIL([
> + test "0" = "$(ovn-sbctl list mac_binding | grep -c '192.168.10.20')"
> +])
>
> OVN_CLEANUP([hv1], [hv2])
> AT_CLEANUP
> --
> 2.22.3
> _______________________________________________
> 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
