On Thu, Mar 13, 2025 at 9:33 AM Xavier Simonart <xsimo...@redhat.com> wrote:
> - Add ability to ignore some specific datapath or tables when comparing
> flows.
> This is necessary as, as of today, ovn does not properly remove datapath
> from list of local datapaths when some prots are removed.
> - Properly check ovn-nb/ovn-nb.sock when AZ is present.
> - Ignore flow differences in OFTABLE_MAC_BINDING, OFTABLE_MAC_LOOKUP
> and OFTABLE_MAC_CACHE_USE.
>
> Signed-off-by: Xavier Simonart <xsimo...@redhat.com>
>
> ---
> v2: - avoid using bash arrays.
> - use diff instead of comm -3 for flow diff as more readable.
> - avoid cat file | grep ... > file.
> - ignore flow differences in OFTABLE_MAC_BINDING, OFTABLE_MAC_LOOKUP
> and OFTABLE_MAC_CACHE_USE.
> v3: - Properly check ovn-nb/ovn-nb.sock when AZ is present in
> CHECK_RELATED_PORTS.
> - Add possibility to ignore some tables when comparing I+P and
> recompute flows.
> - When multiple hv were cleaned, v1 & v2 resulted in
> checking_recompute for
> hv1, cleanup hv1, checking_recompute for hv2, cleanup hv2.
> However, cleaning up hv1 usually results in chassis removal in hv2
> and
> multiple node recomputes in hv2. We now first check_recomputes on
> all hv,
> then clean the hv. This highlighted a few more tests or ovn
> I+P/recompute
> differences on hv2, hv3...
> v4: - Fix those I+P/recompute differences in this patch (was erroneoulsy
> fixed in
> next patch of the serie).
> ---
>
Hi Xavier,
thank you for the patch.
I'm slightly afraid that we are increasing complexity of tests in general,
now everyone will need to understand what is ok to ignore and why.
Not sure what to do about it though so I won't block the series, but
any idea how to improve it is welcome I suppose.
tests/ovn-macros.at | 95 ++++++++++++++++++++++++++++++++++-----------
> tests/ovn.at | 42 +++++++++++++++-----
> 2 files changed, 106 insertions(+), 31 deletions(-)
>
> diff --git a/tests/ovn-macros.at b/tests/ovn-macros.at
> index 6a568e743..cdfa503da 100644
> --- a/tests/ovn-macros.at
> +++ b/tests/ovn-macros.at
> @@ -196,7 +196,7 @@ m4_define([DUMP_FLOWS], [
> sbox=$1
> output_file=$2
> as $sbox
> - ovs-ofctl dump-flows br-int |
> + ovs-ofctl dump-flows br-int | grep -v "NXST_FLOW reply" |
> sed 's/cookie=0x[[^,]]*/cookie=xx/g' |
> sed 's/duration=[[^,]]*/duration=xx/g' |
> sed 's/idle_age=[[^,]]*/idle_age=xx/g' |
> @@ -221,7 +221,7 @@ m4_define([CHECK_FLOWS_AFTER_RECOMPUTE], [
> sbox=$2
> # Make sure I+P has finalized his job before getting flows and
> comparing them after recompte.
> # Some tests have northd and ovn-nb ovsdb stopped, so avoid ovn-nbctl
> for those.
> - if [[ -e ovn-nb/ovn-nb.sock ]] && [[ -e northd/ovn-northd.pid ]]; then
> + if [[ -e ${AZ_DIR}ovn-nb/ovn-nb.sock ]] && [[ -e
> ${AZ_DIR}northd/ovn-northd.pid ]]; then
> # Do wait twice to handle some potential race conditions
> check ovn-nbctl --wait=hv sync
> check ovn-nbctl --wait=hv sync
> @@ -230,15 +230,15 @@ m4_define([CHECK_FLOWS_AFTER_RECOMPUTE], [
> as $sbox
> if test "$hv" != "vtep"; then
> # Get flows before and after recompute
> - DUMP_FLOWS([$sbox], [flows-$hv-1])
> + DUMP_FLOWS([$sbox], [flows-$hv-before])
>
> check ovn-appctl -t ovn-controller recompute
> # The recompute might cause some sb changes. Let controller catch
> up.
> - if [[ -e ovn-nb/ovn-nb.sock ]] && [[ -e northd/ovn-northd.pid ]];
> then
> + if [[ -e ${AZ_DIR}ovn-nb/ovn-nb.sock ]] && [[ -e
> ${AZ_DIR}northd/ovn-northd.pid ]]; then
> check ovn-nbctl --wait=hv sync
> fi
> - DUMP_FLOWS([$sbox], [flows-$hv-2])
> - diff flows-$hv-1 flows-$hv-2 > flow-diff
> + DUMP_FLOWS([$sbox], [flows-$hv-after])
> + diff flows-$hv-before flows-$hv-after > flow-diff
> AT_CHECK([wc -l < flow-diff], [0], [0
> ])
> fi
> @@ -251,7 +251,7 @@ m4_define([CHECK_RELATED_PORTS_AFTER_RECOMPUTE], [
> AT_CAPTURE_FILE([related-ports-diff])
> # Make sure I+P has finalized the job before getting flows and
> comparing them after recompte.
> # Some tests have northd and ovn-nb ovsdb stopped, so avoid ovn-nbctl
> for those.
> - if [[ -e ovn-nb/ovn-nb.sock ]] && [[ -e northd/ovn-northd.pid ]]; then
> + if [[ -e ${AZ_DIR}ovn-nb/ovn-nb.sock ]] && [[ -e
> ${AZ_DIR}northd/ovn-northd.pid ]]; then
> # Do wait twice to handle some potential race conditions
> check ovn-nbctl --wait=hv sync
> check ovn-nbctl --wait=hv sync
> @@ -262,7 +262,7 @@ m4_define([CHECK_RELATED_PORTS_AFTER_RECOMPUTE], [
> DUMP_RELATED_PORTS([$sbox], [related-ports-$hv-before])
> check ovn-appctl -t ovn-controller recompute
> # The recompute might cause some sb changes. Let controller catch
> up.
> - if [[ -e ovn-nb/ovn-nb.sock ]] && [[ -e northd/ovn-northd.pid ]];
> then
> + if [[ -e ${AZ_DIR}ovn-nb/ovn-nb.sock ]] && [[ -e
> ${AZ_DIR}northd/ovn-northd.pid ]]; then
> check ovn-nbctl --wait=hv sync
> fi
> DUMP_RELATED_PORTS([$sbox], [related-ports-$hv-after])
> @@ -275,14 +275,20 @@ m4_define([CHECK_RELATED_PORTS_AFTER_RECOMPUTE], [
> fi
> ])
>
> +# CHECK_AFTER_RECOMPUTE(hv, sbox, related_ports, ignored_dp)
> +# Check related ports (ignoring $related_ports) and FLOWS (ignoring flows
> from $ignored_dp and from $ignored_tables)
> +# by comparing IP flows and flows after recompute.
> +# We also ignore Flows from tables OFTABLE_MAC_BINDING,
> OFTABLE_MAC_LOOKUP, OFTABLE_MAC_CACHE_USE.
> m4_define([CHECK_AFTER_RECOMPUTE], [
> hv=$1
> sbox=$2
> related_ports=$3
> + ignored_dp=$4
> + ignored_tables=$5
> AT_CAPTURE_FILE([related-ports-diff])
> # Make sure I+P has finalized his job before getting flows and
> comparing them after recompte.
> # Some tests have northd and ovn-nb ovsdb stopped, so avoid ovn-nbctl
> for those.
> - if [[ -e ovn-nb/ovn-nb.sock ]] && [[ -e northd/ovn-northd.pid ]]; then
> + if [[ -e ${AZ_DIR}ovn-nb/ovn-nb.sock ]] && [[ -e
> ${AZ_DIR}northd/ovn-northd.pid ]]; then
> # Do wait twice to handle some potential race conditions
> check ovn-nbctl --wait=hv sync
> check ovn-nbctl --wait=hv sync
> @@ -290,25 +296,59 @@ m4_define([CHECK_AFTER_RECOMPUTE], [
>
> as $sbox
> if test "$hv" != "vtep"; then
> - DUMP_RELATED_PORTS([$sbox], [related-ports-$hv-1])
> - DUMP_FLOWS([$sbox], [flows-$hv-1])
> + DUMP_RELATED_PORTS([$sbox], [related-ports-$hv-before])
> + DUMP_FLOWS([$sbox], [flows-$hv-before])
> check ovn-appctl -t ovn-controller recompute
> + echo "Checking after recompute for $sbox"
> # The recompute might cause some sb changes. Let controller catch
> up.
> - if [[ -e ovn-nb/ovn-nb.sock ]] && [[ -e northd/ovn-northd.pid ]];
> then
> + if [[ -e ${AZ_DIR}ovn-nb/ovn-nb.sock ]] && [[ -e
> ${AZ_DIR}northd/ovn-northd.pid ]]; then
> check ovn-nbctl --wait=hv sync
> fi
> - DUMP_RELATED_PORTS([$sbox], [related-ports-$hv-2])
> - DUMP_FLOWS([$sbox], [flows-$hv-2])
> - # Compare and store differences before and after recompute
> - comm -3 related-ports-$hv-1 related-ports-$hv-2 >
> related-ports-diff-$hv
> + DUMP_RELATED_PORTS([$sbox], [related-ports-$hv-after])
> + DUMP_FLOWS([$sbox], [flows-$hv-after])
> +
> + # Compare and store related_ports differences before and after
> recompute
> + comm -3 related-ports-$hv-before related-ports-$hv-after >
> related-ports-diff-$hv
> # Ignore some differences.
> + if [[ -n "$related_ports" ]]; then
> + echo "Ignoring related ports $related_ports"
> + fi
> echo "$related_ports" | comm -2 -3 related-ports-diff-$hv - >
> related-ports-diff
> AT_CHECK([wc -l < related-ports-diff], [0], [0
> ])
> - diff flows-$hv-1 flows-$hv-2 > flow-diff
> - AT_CHECK([wc -l < flow-diff], [0], [0
> +
> + # Compare and store flow differences before and after recompute
> + diff -u flows-$hv-before flows-$hv-after | grep "^+ \|^- " >
> flow-diff-$hv
> + if [[ -n "$related_ports" ]]; then
> + tag=$(ovn-sbctl --bare --columns tag list port_binding
> $related_ports)
> + if [[ -n "$tag" ]]; then
> + echo "Ignoring tag $tag"
> + cat flow-diff-$hv | grep -v "dl_vlan=$tag" > tmp; mv tmp
> flow-diff-$hv
> + fi
> + fi
> +
> + dps=$(echo "$ignored_dp" | tr ',' ' ')
> + for dp in $dps
> + do
> + if [[ -n "${dp}" ]]; then
> + key=$(printf '0x%x' $(ovn-sbctl --bare --columns tunnel_key
> list datapath $dp))
> + if [[ -n "${key}" ]]; then
> + echo "Ignoring $dp i.e. tunnel_key $key"
> + cat flow-diff-$hv | grep -v "metadata=$key" | grep -v
> "load:${key}->OXM_OF_METADATA" > tmp; mv tmp flow-diff-$hv
> + fi
> + fi
> + done
> +
> + tables=$(echo "$ignored_tables" | tr ',' ' ')
> + for table in $tables
> + do
> + echo "Ignoring table $table"
> + cat flow-diff-$hv | grep -v "table=$table" > tmp; mv tmp
> flow-diff-$hv
> + done
> +
> + cat flow-diff-$hv | grep -v "table=OFTABLE_MAC_BINDING" | grep -v
> "table=OFTABLE_MAC_LOOKUP" | grep -v "table=OFTABLE_MAC_CACHE_USE" > tmp;
> mv tmp flow-diff-$hv
> + AT_CHECK([wc -l < flow-diff-$hv], [0], [0
> ])
> - diff flows-$hv-1 flows-$hv-2 > flow-diff
> fi
> ])
>
> @@ -322,8 +362,11 @@ m4_define([OVN_CLEANUP_CONTROLLER],[
> hv=$1
> sbox=$2
> related_ports=$3
> + no_recompute_check=$4
> + if test "$no_recompute_check" != "True"; then
> + CHECK_RELATED_PORTS_AFTER_RECOMPUTE([$hv], [$sbox],
> [$related_ports])
> + fi
>
> - CHECK_RELATED_PORTS_AFTER_RECOMPUTE([$hv], [$sbox], [$related_ports])
> if test "$hv" = "vtep"; then
> OVS_APP_EXIT_AND_WAIT([ovn-controller-vtep])
> OVS_APP_EXIT_AND_WAIT([ovs-vtep])
> @@ -342,8 +385,10 @@ m4_define([OVN_CLEANUP_SBOX],[
> sbox=$1
> error=$2
> related_ports=$3
> + no_recompute_check=$4
> echo "$sbox: clean up sandbox"
> - OVN_CLEANUP_CONTROLLER([$sbox], [$sbox], [$related_ports])
> + as $sbox
> + OVN_CLEANUP_CONTROLLER([$sbox], [$sbox], [$related_ports],
> [$no_recompute_check])
> OVN_CLEANUP_VSWITCH([$sbox])
>
> # Check for errors in logs. Ignore following errors:
> @@ -372,7 +417,12 @@ m4_define([OVN_CLEANUP],[
> sbox=$(echo "sbox_and_error" |sed -n '1p')
> error=$(echo "sbox_and_error" | grep '/')
> related_ports=$(echo "sbox_and_error" | sed -n '2,$p' | grep -v
> '/')
> - OVN_CLEANUP_SBOX([$sbox], [$error], [$related_ports])
> + CHECK_RELATED_PORTS_AFTER_RECOMPUTE([$sbox], [$sbox],
> [$related_ports])
> + ])
> + m4_foreach([sbox_and_error], [$@], [
> + sbox=$(echo "sbox_and_error" |sed -n '1p')
> + error=$(echo "sbox_and_error" | grep '/')
> + OVN_CLEANUP_SBOX([$sbox], [$error], [], ["True"])
> ])
>
> echo
> @@ -753,6 +803,7 @@ start_virtual_controller() {
> # ovn_setenv AZ
> ovn_setenv () {
> local d=$ovs_base/$1
> + AS_VAR_SET([AZ_DIR], ["$d"/]); export $var
> AS_VAR_SET([OVN_NB_DB], [unix:"$d"/ovn-nb/ovn-nb.sock]); export $var
> AS_VAR_SET([OVN_SB_DB], [unix:"$d"/ovn-sb/ovn-sb.sock]); export $var
> }
> diff --git a/tests/ovn.at b/tests/ovn.at
> index f4abfb345..3d159d02b 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -9925,7 +9925,10 @@ n_arp=$(tcpdump -c 10 -ner hv1/snoopvif-tx.pcap arp
> | wc -l)
> test "$n_arp" = 10
> ])
>
> -OVN_CLEANUP([hv1],[hv2])
> +# lr0 & ls0 should not be local datapath on hv2 anymore, and hence
> +# ln_port should not be a related_port on hv2.
> +OVN_CLEANUP([hv1],[hv2
> +ln_port])
>
> AT_CLEANUP
> ])
> @@ -25162,12 +25165,13 @@ m4_define([DVR_N_S_ARP_HANDLING],
> as hv4 ovs-appctl fdb/show br-phys
>
> # Expect some potential transaction errors for MAC_Binding
> - # Since router-to-underlay moved to hv3, ls-underlay is not local to
> hv1 anymore and ln3 should not be a related_port.
> + # Since router-to-underlay moved to hv3, ls-underlay is not local to
> hv1 or hv2 anymore.
> + # As ls-underlay is not local, ln3 should not be a related_port.
> OVN_CLEANUP([hv1
> /transaction error/d
> ln3],[hv2
> /transaction error/d
> - ],[hv3
> +ln3],[hv3
> /transaction error/d
> ],[hv4])
>
> @@ -26352,7 +26356,10 @@ wait_row_count Service_Monitor 1
> wait_row_count Service_Monitor 0 status=offline
> wait_row_count Service_Monitor 0 status=online
>
> -OVN_CLEANUP([hv1], [hv2])
> +# lr0_public is on hv1. Hence lr0 & public should not be local datapaths
> on hv2, and ln_public not a related port for hv2.
> +OVN_CLEANUP([hv1], [hv2
> +ln-public])
> +
> AT_CLEANUP
> ])
>
> @@ -26550,7 +26557,10 @@ wait_row_count Service_Monitor 1
> wait_row_count Service_Monitor 0 status=offline
> wait_row_count Service_Monitor 0 status=online
>
> -OVN_CLEANUP([hv1], [hv2])
> +# lr0_public is on hv1. Hence lr0 & public should not be local datapaths
> on hv2, and ln_public not a related port for hv2.
> +OVN_CLEANUP([hv1], [hv2
> +ln-public])
> +
> AT_CLEANUP
> ])
>
> @@ -26669,7 +26679,10 @@ check_row_count Service_Monitor 0
>
> AT_CHECK([test 1 = `grep -c "SCTP load balancers do not currently support
> health checks" northd/ovn-northd.log`])
>
> -OVN_CLEANUP([hv1],[hv2])
> +# lr0_public is on hv1. Hence lr0 & public should not be local datapaths
> on hv2, and ln_public not a related port for hv2.
> +OVN_CLEANUP([hv1], [hv2
> +ln-public])
> +
> AT_CLEANUP
> ])
>
> @@ -34368,13 +34381,22 @@ test_ip vif11 f00000000010 06ac10010001 $sip
> $dip vif-north3
> OVN_CHECK_PACKETS_REMOVE_BROADCAST([hv5/vif-north3-tx.pcap],
> [vif-north3.expected])
>
> # Since DR-S1, DR-S2 and DR-S3 are gw-chassis on resp. hv2, hv3 and hv4,
> -# S1, S2 and S3 should not be local datapaths on hv1, and their localnets
> +# S1, S2 and S3 should not be local datapaths on the other hvs, and their
> localnets
> # should not be related ports.
> OVN_CLEANUP([hv1
> ln1
> ln2
> ln3
> -],[hv2],[hv3],[hv4],[hv5])
> +],[hv2
> +ln2
> +ln3],[hv3
> +ln1
> +ln3],[hv4
> +ln1
> +ln2],[hv5
> +ln1
> +ln2
> +ln3])
> AT_CLEANUP
> ])
>
> @@ -40499,7 +40521,9 @@ OVS_WAIT_UNTIL([test $(as hv2 ovs-ofctl dump-flows
> br-int table=OFTABLE_OUTPUT_L
> check ovn-nbctl --wait=hv lsp-del multi
> OVS_WAIT_UNTIL([test $(as hv2 ovs-ofctl dump-flows br-int
> table=OFTABLE_OUTPUT_LARGE_PKT_DETECT | grep -c check_pkt_larger) -eq 0])
>
> -OVN_CLEANUP([hv1],[hv2])
> +# ls (and it's localnet ln) should not be local datapath to hv2 as multi
> was deleted. Still local datapath to hv1 due to lsp1.
> +OVN_CLEANUP([hv1],[hv2
> +ln])
>
> AT_CLEANUP
> ])
> --
> 2.47.1
>
> _______________________________________________
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
Acked-by: Ales Musil <amu...@redhat.com>
Thanks,
Ales
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
