Re: [ovs-dev] [PATCH ovn 4/4] tests: Add missing sync calls

Mon, 14 Aug 2023 13:03:18 -0700 

Hi Ales, I have a couple of notes below.

On 8/14/23 05:21, Ales Musil wrote:

Add various missing sync calls which caused the
tests to be flaky due to sometimes missed on various
checks or packets.

Signed-off-by: Ales Musil <amu...@redhat.com>
---
  tests/ovn-controller.at | 10 +++++-----
  tests/ovn-ic.at         |  7 ++++---
  tests/ovn-northd.at     | 14 +++++++++-----
  tests/ovn.at            | 20 ++++++++++++++------
  tests/system-ovn.at     |  5 +++--
  5 files changed, 35 insertions(+), 21 deletions(-)

diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at
index f2216d245..6a4bcedab 100644
--- a/tests/ovn-controller.at
+++ b/tests/ovn-controller.at
@@ -433,10 +433,10 @@ check ovn-nbctl --wait=hv sync
  check ovn-nbctl --wait=hv sync
# nb_cfg should be set to 1 in the chassis_private/nb_global/sb_global table 
-check_column 1 chassis_private nb_cfg
-check_column 1 sb_global nb_cfg
-check_column 1 nb:nb_global nb_cfg
-check_column 0 chassis nb_cfg
+wait_row_count nb:nb_global 1 nb_cfg=1
+wait_row_count chassis_private 1 nb_cfg=1
+wait_row_count sb_global 1 nb_cfg=1
+wait_row_count chassis 1 nb_cfg=0
Why is this change necessary? We can see that there is an `ovn-nbctl --wait=hv sync` call above. Shouldn't we be able to check the values directly instead of having to wait?
OVN_CLEANUP([hv]) 
  AT_CLEANUP
@@ -562,7 +562,7 @@ primary lport : [[lsp1]]
  ])
# Set the port type to localport 
-check ovn-nbctl lsp-set-type lsp1 localport
+check ovn-nbctl --wait=hv lsp-set-type lsp1 localport
  check as hv1 ovs-vsctl set open . external_ids:ovn-cms-options=localport
  OVS_WAIT_UNTIL([test localport = $(ovn-sbctl get chassis . 
other_config:ovn-cms-options)])
diff --git a/tests/ovn-ic.at b/tests/ovn-ic.at 
index 09eac860c..a654e59fe 100644
--- a/tests/ovn-ic.at
+++ b/tests/ovn-ic.at
@@ -543,6 +543,7 @@ done
  # Create directly-connected routes
  ovn_as az2 ovn-nbctl lrp-add lr12 lrp-lr12-ls2 aa:aa:aa:aa:bb:01 
"192.168.0.1/24"
  ovn_as az2 ovn-nbctl lr-route-add lr12 10.10.10.0/24 192.168.0.10
+ovn_as az1 ovn-nbctl --wait=sb sync
echo az1 
  ovn_as az1 ovn-nbctl show
@@ -951,7 +952,7 @@ ovn_as az2 ovn-nbctl --route-table=rtb2 lr-route-add lr12 
10.10.10.0/24 192.168.
  ovn_as az2 ovn-nbctl --route-table=rtb3 lr-route-add lr12 10.10.10.0/24 
192.168.0.12
# Create directly-connected route in VPC2 
-ovn_as az2 ovn-nbctl lrp-add lr22 lrp-lr22 aa:aa:aa:aa:bb:01 "192.168.0.1/24"
+ovn_as az2 ovn-nbctl --wait=sb lrp-add lr22 lrp-lr22 aa:aa:aa:aa:bb:01 
"192.168.0.1/24"
# Test direct routes from lr12 were learned to lr11 
  AT_CHECK([ovn_as az1 ovn-nbctl lr-route-list lr11 | grep 192.168 |
@@ -1077,7 +1078,7 @@ ovn_as az2 ovn-nbctl --route-table=rtb2 lr-route-add lr12 
2001:db8:aaaa::/64 200
  ovn_as az2 ovn-nbctl --route-table=rtb3 lr-route-add lr12 2001:db8:aaaa::/64 
2001:db8:200::12
# Create directly-connected route in VPC2 
-ovn_as az2 ovn-nbctl lrp-add lr22 lrp-lr22 aa:aa:aa:aa:bb:01 
"2001:db8:200::1/64"
+ovn_as az2 ovn-nbctl --wait=sb lrp-add lr22 lrp-lr22 aa:aa:aa:aa:bb:01 
"2001:db8:200::1/64"
# Test direct routes from lr12 were learned to lr11 
  AT_CHECK([ovn_as az1 ovn-nbctl lr-route-list lr11 | grep 2001:db8:200 |
@@ -1146,7 +1147,7 @@ for i in 1 2; do
      ovn-nbctl lrp-add $lr lrp-local-subnet 00:00:00:00:00:0$i 192.168.$i.1/24
      ovn-nbctl list logical-router-static-route
      check ovn-nbctl lr-route-add $lr 10.0.0.0/24 192.168.$i.10
-    check ovn-nbctl lr-route-add $lr 0.0.0.0/0 192.168.$i.11
+    check ovn-nbctl --wait=sb lr-route-add $lr 0.0.0.0/0 192.168.$i.11
  done
AT_CHECK([ovn_as az1 ovn-nbctl lr-route-list lr11 | grep dst-ip | sort], [0], [dnl 
diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
index d5be3be75..d1ea892ec 100644
--- a/tests/ovn-northd.at
+++ b/tests/ovn-northd.at
@@ -1156,7 +1156,7 @@ ovn-nbctl --stateless lr-nat-add DR dnat_and_snat  
172.16.1.2 50.0.0.11
  ovn-nbctl --stateless lr-nat-add CR dnat_and_snat  172.16.1.2 50.0.0.11
ovn-nbctl lr-nat-update-ext-ip DR dnat_and_snat 172.16.1.2 allowed_range 
-ovn-nbctl lr-nat-update-ext-ip CR dnat_and_snat 172.16.1.2 allowed_range
+check ovn-nbctl --wait=sb lr-nat-update-ext-ip CR dnat_and_snat 172.16.1.2 
allowed_range
ovn-sbctl dump-flows DR > drflows5 
  AT_CAPTURE_FILE([drflows2])
@@ -4815,7 +4815,7 @@ AS_BOX([Checking that routable NAT flows are installed 
when gateway chassis exis
  check ovn-nbctl lr-nat-del ro1
  check ovn-nbctl lr-nat-del ro2
  check ovn-nbctl --add-route lr-nat-add ro1 dnat 10.0.0.100 192.168.1.100
-check ovn-nbctl --add-route lr-nat-add ro2 dnat 20.0.0.100 192.168.2.100
+check ovn-nbctl --wait=sb --add-route lr-nat-add ro2 dnat 20.0.0.100 
192.168.2.100
check_lflows 1 @@ -4846,7 +4846,7 @@ check ovn-nbctl lr-nat-del ro1 
  check ovn-nbctl lr-nat-del ro2
check ovn-nbctl lr-nat-add ro1 dnat_and_snat 10.0.0.100 192.168.1.2 vm1 00:00:00:00:00:01 
-check ovn-nbctl lr-nat-add ro2 dnat_and_snat 20.0.0.100 192.168.2.2 vm2 
00:00:00:00:00:02
+check ovn-nbctl --wait=sb lr-nat-add ro2 dnat_and_snat 20.0.0.100 192.168.2.2 
vm2 00:00:00:00:00:02
check_lflows 0 @@ -4991,7 +4991,7 @@ check ovn-nbctl lsp-add ls2 ls2-ro2 
  check ovn-nbctl lsp-set-type ls2-ro2 router
  check ovn-nbctl lsp-set-addresses ls2-ro2 router
  check ovn-nbctl lsp-set-options ls2-ro2 router-port=ro2-ls2
-
+check ovn-nbctl --wait=sb sync
ovn-sbctl lflow-list ls1 > ls1_lflows 
  AT_CHECK([grep "ls_in_l2_lkup" ls1_lflows | sed 's/table=../table=??/' | 
sort], [0], [dnl
@@ -5858,6 +5858,7 @@ sm_vip2=$(fetch_column Service_Monitor _uuid 
logical_port=vip2)
ovn-sbctl set service_monitor $sm_vip1 status=offline 
  ovn-sbctl set service_monitor $sm_vip2 status=offline
+check ovn-nbctl --wait=sb sync
AT_CHECK([ovn-sbctl dump-flows lr0 | grep "lr_in_dnat" | sort], [0], [dnl 
    table=7 (lr_in_dnat         ), priority=0    , match=(1), action=(next;)
@@ -6689,6 +6690,7 @@ ovn-nbctl lb-add lb0 172.16.1.10:80 10.0.0.1:80
  ovn-nbctl lr-lb-add R1 lb0
  ovn-nbctl lb-add lb1 172.16.1.10:8080 10.0.0.1:8080
  ovn-nbctl lr-lb-add R1 lb1
+ovn-nbctl --wait=sb sync
AT_CHECK([ovn-sbctl get Port_Binding S1-R1 nat_addresses |grep -q 172.16.1.10], [0]) @@ -8593,6 +8595,7 @@ lb1_uuid=$(fetch_column nb:load_balancer _uuid name=lb1) 
  lbg=$(ovn-nbctl create load_balancer_group name=lbg -- \
      add load_balancer_group lbg load_balancer $lb1_uuid)
  ovn-nbctl add logical_router R1 load_balancer_group $lbg
+ovn-nbctl --wait=sb sync
ovn-sbctl dump-flows S0 > S0flows 
  ovn-sbctl dump-flows S1 > S1flows
@@ -8719,6 +8722,7 @@ ovn-nbctl lsp-set-options S1-R1 router-port=R1-S1
  ovn-nbctl lb-add lb0 172.16.0.10:80 10.0.0.2:80,20.0.0.2:80 tcp
  ovn-nbctl lr-lb-add R1 lb0
  ovn-nbctl ls-lb-add S0 lb0
+ovn-nbctl --wait=sb sync
ovn-sbctl dump-flows S0 > S0flows 
  ovn-sbctl dump-flows R1 > R1flows
@@ -9438,7 +9442,7 @@ acl_test() {
  ])
# Remove the ACL with the "pass" verdict. Ensure that no eval flows are present. 
-    check ovn-nbctl acl-del $thing
+    check ovn-nbctl --wait=sb acl-del $thing
      ovn-sbctl lflow-list ls > lflows
      AT_CHECK([ACL_FLOWS([$eval_stage], [priority=2000])], [0], [])
  }
diff --git a/tests/ovn.at b/tests/ovn.at
index 56ac17261..29fddf365 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -20805,6 +20805,7 @@ ovn-nbctl lsp-set-options public-lr1 
router-port=lr1-public
ovn-nbctl lr-nat-add lr1 snat 172.168.0.100 10.0.0.0/24 
  ovn-nbctl lr-nat-add lr1 snat 2000::1 1000::/64
+ovn-nbctl --wait=sb sync
dp_uuid=$(ovn-sbctl find datapath_binding | grep sw0 -B2 | grep _uuid | \ 
  awk '{print $3}')
@@ -21024,6 +21025,7 @@ AT_CAPTURE_FILE([sbflows2])
  check ovn-nbctl lr-policy-add lr0 2000 "ip4.src == 192.168.1.3" allow
  policy=$(fetch_column nb:Logical_Router_Policy _uuid priority=2000)
  check ovn-nbctl set logical_router_policy $policy options:pkt_mark=100
+check ovn-nbctl --wait=hv sync
  as hv2
  # add a flow in egress pipeline to check pkt marking
  ovs-ofctl --protocols=OpenFlow13 add-flow br-int 
"table=37,priority=200,ip,nw_src=172.16.1.2,pkt_mark=0x64 actions=resubmit(,38)"
@@ -21156,6 +21158,7 @@ check ovn-nbctl --wait=hv sync
  # Recreate two floating IPs, one for each VIF.
  check ovn-nbctl lr-nat-del lr0 dnat_and_snat 172.24.4.100
  check ovn-nbctl lr-nat-del lr1 dnat_and_snat 172.24.4.200
+check ovn-nbctl --wait=hv sync
check ovn-sbctl --all destroy mac_binding @@ -23276,6 +23279,9 @@ send_mld_v2_report hv1-vif1 hv1 \ 
  # Check IGMP_Group table on both HV.
  wait_row_count IGMP_Group 1 address='"ff0a:dead:beef::1"'
+# This gives the ovn-controller nodes a chance to see the updated IGMP_Group. 
+check ovn-nbctl --wait=hv sync
+
  # Send traffic and make sure it gets forwarded only on the port that joined.
  as hv1 reset_pcap_file hv1-vif1 hv1/vif1
  as hv2 reset_pcap_file hv2-vif1 hv2/vif1
@@ -23307,6 +23313,9 @@ OVS_WAIT_UNTIL(
  check ovn-sbctl ip-multicast-flush sw1
  wait_row_count IGMP_Group 0 address='"ff0a:dead:beef::1"'
+# This gives the ovn-controller nodes a chance to see the updated IGMP_Group. 
+check ovn-nbctl --wait=hv sync
+
  # Check that traffic for "all-hosts" is flooded even if some hosts register
  # for it.
  # Inject MLD Join for ff02::1 on sw1-p11.
@@ -28146,7 +28155,7 @@ AT_CHECK([
      grep "n_packets=0" -c)
  ])
-ovn-nbctl set logical_router_policy $pol1 options:pkt_mark=2 
+ovn-nbctl --wait=hv set logical_router_policy $pol1 options:pkt_mark=2
  send_ipv4_pkt hv1 hv1-vif1 505400000003 00000000ff01 \
      $(ip_to_hex 10 0 0 3) $(ip_to_hex 172 168 0 120) \
      c3ad 83dc
@@ -35936,7 +35945,7 @@ ovn-nbctl --wait=hv sync
check ovn-nbctl lb-add lb1 "192.168.0.10" "192.168.10.10,192.168.10.20" \ 
      -- set load_balancer lb1 options:ct_flush="true"
-check ovn-nbctl ls-lb-add sw lb1
+check ovn-nbctl --wait=hv ls-lb-add sw lb1
# Remove a single backend 
  check ovn-nbctl set load_balancer lb1 vips='"192.168.0.10"="192.168.10.10"'
@@ -35960,9 +35969,8 @@ AT_CHECK([grep -q "Flushing CT for 5-tuple: 
vip=192.168.0.10:0, backend=192.168.
  # Check flush for LB with port and protocol
  check ovn-nbctl lb-add lb1 "192.168.30.10:80" 
"192.168.40.10:8080,192.168.40.20:8090" udp \
      -- set load_balancer lb1 options:ct_flush="true"
-check ovn-nbctl ls-lb-add sw lb1
-check ovn-nbctl lb-del lb1
-check ovn-nbctl --wait=hv sync
+check ovn-nbctl --wait=hv ls-lb-add sw lb1
+check ovn-nbctl --wait=hv lb-del lb1
I don't understand why this change was made. We already were waiting for the hv, but with this change, we're doing it two times instead. Is that on purpose?
AT_CHECK([grep -q "Flushing CT for 5-tuple: vip=192.168.30.10:80, backend=192.168.40.10:8080, protocol=17" hv1/ovn-controller.log], [0]) 
  AT_CHECK([grep -q "Flushing CT for 5-tuple: vip=192.168.30.10:80, 
backend=192.168.40.20:8090, protocol=17" hv1/ovn-controller.log], [0])
@@ -35983,7 +35991,7 @@ AT_CHECK([grep -q "Flushing CT for 5-tuple: 
vip=192.168.50.10:80, backend=192.16
  AT_CHECK([test "$(grep -c "Flushing CT for 5-tuple" hv1/ovn-controller.log)" = 
"6"], [0])
# Check if CT flush is disabled by default 
-check ovn-nbctl lb-del lb1
+check ovn-nbctl --wait=hv lb-del lb1
  check ovn-nbctl lb-add lb1 "192.168.70.10:80" 
"192.168.80.10:8080,192.168.90.10:8080"
  check ovn-nbctl ls-lb-add sw lb1
  check ovs-vsctl set interface p1 external_ids:iface-id=lsp1
diff --git a/tests/system-ovn.at b/tests/system-ovn.at
index bfa323b5c..55c5ddc19 100644
--- a/tests/system-ovn.at
+++ b/tests/system-ovn.at
@@ -7447,7 +7447,7 @@ AT_CHECK([ovn-nbctl copp-list copp3 |grep bfd], [0], [dnl
  bfd: bfd-meter
  ])
-check ovn-nbctl --bfd lr-route-add R1 240.0.0.0/8 172.16.1.50 rp-public 
+check ovn-nbctl --wait=hv --bfd lr-route-add R1 240.0.0.0/8 172.16.1.50 
rp-public
  printf "%08x" $(ovn-sbctl get bfd . disc) > /tmp/disc
  NS_EXEC([server], [tcpdump -l -nn -i s1 udp port 3784 and ip[[29]]==0x90 -Q in > 
bfd.pcap &])
  ip netns exec server scapy -H <<-EOF
@@ -8586,6 +8586,7 @@ ovn-nbctl lsp-set-addresses sw0-p1 "00:54:00:00:00:03 
10.0.0.3"
  ovn-nbctl ls-add sw0
  ovn-nbctl lsp-add sw0 sw0-p1.2 sw0-p1 2
  ovn-nbctl lsp-set-addresses sw0-p1.2 "00:54:00:00:00:04 10.0.0.4"
+check ovn-nbctl --wait=hv sync
ADD_NAMESPACES(sw0-p1) 
  ADD_VETH(sw0-p1, sw0-p1, br-int, "10.0.0.3/24", "00:54:00:00:00:03", \
@@ -11280,7 +11281,7 @@ ovn-nbctl lsp-add foo foo1 \
  ADD_NAMESPACES(bar1)
  ADD_VETH(bar1, bar1, br-int, "2002::2/64", "f0:00:00:01:02:05", \
           "2002::1", "nodad", "192.168.2.2/24", "192.168.2.1")
-ovn-nbctl lsp-add bar bar1 \
+ovn-nbctl --wait=hv lsp-add bar bar1 \
  -- lsp-set-addresses bar1 "f0:00:00:01:02:05 192.168.2.2 2002::2"
# Warm up the datapath (needed to make the DPDK datapth happy) 

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

Reply via email to