On Mon, Aug 14, 2023 at 10:02 PM Mark Michelson <mmich...@redhat.com> wrote:
> 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?
>
You are right, we should be able to check that directly. Looking through
the code again there is a bug during overflow I'll post v2 with proper fix.
>
> >
> > 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?
Yeah that is on purpose, and I understand why it can be confusing. The
problem is that if for some reason we will process add and del in a single
northd run, from controller perspective nothing happens and the flush isn't
called. So we need to wait for ovn-controller to pick up the new LB first
and only then delete it to really ensure that the flush will happen.
>
> >
> > 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)
>
>
Thanks,
Ales
--
Ales Musil
Senior Software Engineer - OVN Core
Red Hat EMEA <https://www.redhat.com>
amu...@redhat.com IM: amusil
<https://red.ht/sig>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
