| Message ID | 20210611062452.362848-4-hzhou@ovn.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev,1/4] ovn-northd.at: Fix test "northd ssl file change -- ovn-northd-ddlog". | expand |
I noticed that there is still some other potential for this test to fail in slow environments. Just above your changes are some lines like this: as hv1 ovs-vsctl set interface hv1-vif3 external-ids:iface-id=sw0-vir AT_CHECK([test x$(ovn-sbctl --bare --columns chassis find port_binding \ logical_port=sw0-vir) = x], [0], []) It seems like that AT_CHECK should be changed to OVS_WAIT_UNTIL since there could be a delay before the SB port_binding is updated. Similar patterns are used a few times in this test. If you're going to fix this test, then you probably should fix those up too. On 6/11/21 2:24 AM, Han Zhou wrote: > The test case fails quite often for northd-ddlog because of the tunnel > keys mismatch when comparing OpenFlow rules. Keys can change in > different runs. This patch fixes it by extracting the expected keys from > SB DB before comparison instead of hardcoding. > > Signed-off-by: Han Zhou <hzhou@ovn.org> > --- > tests/ovn.at | 14 +++++++++----- > 1 file changed, 9 insertions(+), 5 deletions(-) > > diff --git a/tests/ovn.at b/tests/ovn.at > index b440f5517..67cf75719 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -17317,14 +17317,18 @@ logical_port=sw0-vir) = x], [0], []) > check_virtual_offlows_present() { > hv=$1 > > - AT_CHECK([as $hv ovs-ofctl dump-flows br-int table=44 | ofctl_strip_all | grep "priority=2000"], [0], [dnl > - table=44, priority=2000,ip,metadata=0x1 actions=resubmit(,45) > - table=44, priority=2000,ipv6,metadata=0x1 actions=resubmit(,45) > + sw0_dp_key=$(fetch_column Datapath_Binding tunnel_key external_ids:name=sw0) > + lr0_dp_key=$(fetch_column Datapath_Binding tunnel_key external_ids:name=lr0) > + lr0_public_dp_key=$(fetch_column Port_Binding tunnel_key logical_port=lr0-public) > + > + AT_CHECK_UNQUOTED([as $hv ovs-ofctl dump-flows br-int table=44 | ofctl_strip_all | grep "priority=2000"], [0], [dnl > + table=44, priority=2000,ip,metadata=0x$sw0_dp_key actions=resubmit(,45) > + table=44, priority=2000,ipv6,metadata=0x$sw0_dp_key actions=resubmit(,45) > ]) > > - AT_CHECK([as $hv ovs-ofctl dump-flows br-int table=11 | ofctl_strip_all | \ > + AT_CHECK_UNQUOTED([as $hv ovs-ofctl dump-flows br-int table=11 | ofctl_strip_all | \ > grep "priority=92" | grep 172.168.0.50], [0], [dnl > - table=11, priority=92,arp,reg14=0x3,metadata=0x3,arp_tpa=172.168.0.50,arp_op=1 actions=move:NXM_OF_ETH_SRC[[]]->NXM_OF_ETH_DST[[]],mod_dl_src:10:54:00:00:00:10,load:0x2->NXM_OF_ARP_OP[[]],move:NXM_NX_ARP_SHA[[]]->NXM_NX_ARP_THA[[]],load:0x105400000010->NXM_NX_ARP_SHA[[]],push:NXM_OF_ARP_SPA[[]],push:NXM_OF_ARP_TPA[[]],pop:NXM_OF_ARP_SPA[[]],pop:NXM_OF_ARP_TPA[[]],move:NXM_NX_REG14[[]]->NXM_NX_REG15[[]],load:0x1->NXM_NX_REG10[[0]],resubmit(,37) > + table=11, priority=92,arp,reg14=0x$lr0_public_dp_key,metadata=0x$lr0_dp_key,arp_tpa=172.168.0.50,arp_op=1 actions=move:NXM_OF_ETH_SRC[[]]->NXM_OF_ETH_DST[[]],mod_dl_src:10:54:00:00:00:10,load:0x2->NXM_OF_ARP_OP[[]],move:NXM_NX_ARP_SHA[[]]->NXM_NX_ARP_THA[[]],load:0x105400000010->NXM_NX_ARP_SHA[[]],push:NXM_OF_ARP_SPA[[]],push:NXM_OF_ARP_TPA[[]],pop:NXM_OF_ARP_SPA[[]],pop:NXM_OF_ARP_TPA[[]],move:NXM_NX_REG14[[]]->NXM_NX_REG15[[]],load:0x1->NXM_NX_REG10[[0]],resubmit(,37) > ]) > } > >
On Fri, Jun 11, 2021 at 11:24 AM Mark Michelson <mmichels@redhat.com> wrote: > > I noticed that there is still some other potential for this test to fail > in slow environments. Just above your changes are some lines like this: > > as hv1 > ovs-vsctl set interface hv1-vif3 external-ids:iface-id=sw0-vir > > AT_CHECK([test x$(ovn-sbctl --bare --columns chassis find port_binding \ > logical_port=sw0-vir) = x], [0], []) > > It seems like that AT_CHECK should be changed to OVS_WAIT_UNTIL since > there could be a delay before the SB port_binding is updated. > > Similar patterns are used a few times in this test. > > If you're going to fix this test, then you probably should fix those up too. Thanks for spotting this. I think you are right and I added the fixes in v2 for the issue you mentioned along with one immediately below it. I quickly looked and it seems other places in this test case when any interface is updated there is some kind of *wait* mechanism used, so it looks ok for me now. Let me know if you see more places that need similar fixes. https://patchwork.ozlabs.org/project/ovn/patch/20210611224852.3267068-1-hzhou@ovn.org/ Thanks, Han > > On 6/11/21 2:24 AM, Han Zhou wrote: > > The test case fails quite often for northd-ddlog because of the tunnel > > keys mismatch when comparing OpenFlow rules. Keys can change in > > different runs. This patch fixes it by extracting the expected keys from > > SB DB before comparison instead of hardcoding. > > > > Signed-off-by: Han Zhou <hzhou@ovn.org> > > --- > > tests/ovn.at | 14 +++++++++----- > > 1 file changed, 9 insertions(+), 5 deletions(-) > > > > diff --git a/tests/ovn.at b/tests/ovn.at > > index b440f5517..67cf75719 100644 > > --- a/tests/ovn.at > > +++ b/tests/ovn.at > > @@ -17317,14 +17317,18 @@ logical_port=sw0-vir) = x], [0], []) > > check_virtual_offlows_present() { > > hv=$1 > > > > - AT_CHECK([as $hv ovs-ofctl dump-flows br-int table=44 | ofctl_strip_all | grep "priority=2000"], [0], [dnl > > - table=44, priority=2000,ip,metadata=0x1 actions=resubmit(,45) > > - table=44, priority=2000,ipv6,metadata=0x1 actions=resubmit(,45) > > + sw0_dp_key=$(fetch_column Datapath_Binding tunnel_key external_ids:name=sw0) > > + lr0_dp_key=$(fetch_column Datapath_Binding tunnel_key external_ids:name=lr0) > > + lr0_public_dp_key=$(fetch_column Port_Binding tunnel_key logical_port=lr0-public) > > + > > + AT_CHECK_UNQUOTED([as $hv ovs-ofctl dump-flows br-int table=44 | ofctl_strip_all | grep "priority=2000"], [0], [dnl > > + table=44, priority=2000,ip,metadata=0x$sw0_dp_key actions=resubmit(,45) > > + table=44, priority=2000,ipv6,metadata=0x$sw0_dp_key actions=resubmit(,45) > > ]) > > > > - AT_CHECK([as $hv ovs-ofctl dump-flows br-int table=11 | ofctl_strip_all | \ > > + AT_CHECK_UNQUOTED([as $hv ovs-ofctl dump-flows br-int table=11 | ofctl_strip_all | \ > > grep "priority=92" | grep 172.168.0.50], [0], [dnl > > - table=11, priority=92,arp,reg14=0x3,metadata=0x3,arp_tpa=172.168.0.50,arp_op=1 actions=move:NXM_OF_ETH_SRC[[]]->NXM_OF_ETH_DST[[]],mod_dl_src:10:54:00:00:00:10,load:0x2->NXM_OF_ARP_OP[[]],move:NXM_NX_ARP_SHA[[]]->NXM_NX_ARP_THA[[]],load:0x105400000010->NXM_NX_ARP_SHA[[]],push:NXM_OF_ARP_SPA[[]],push:NXM_OF_ARP_TPA[[]],pop:NXM_OF_ARP_SPA[[]],pop:NXM_OF_ARP_TPA[[]],move:NXM_NX_REG14[[]]->NXM_NX_REG15[[]],load:0x1->NXM_NX_REG10[[0]],resubmit(,37) > > + table=11, priority=92,arp,reg14=0x$lr0_public_dp_key,metadata=0x$lr0_dp_key,arp_tpa=172.168.0.50,arp_op=1 actions=move:NXM_OF_ETH_SRC[[]]->NXM_OF_ETH_DST[[]],mod_dl_src:10:54:00:00:00:10,load:0x2->NXM_OF_ARP_OP[[]],move:NXM_NX_ARP_SHA[[]]->NXM_NX_ARP_THA[[]],load:0x105400000010->NXM_NX_ARP_SHA[[]],push:NXM_OF_ARP_SPA[[]],push:NXM_OF_ARP_TPA[[]],pop:NXM_OF_ARP_SPA[[]],pop:NXM_OF_ARP_TPA[[]],move:NXM_NX_REG14[[]]->NXM_NX_REG15[[]],load:0x1->NXM_NX_REG10[[0]],resubmit(,37) > > ]) > > } > > > > >
diff --git a/tests/ovn.at b/tests/ovn.at index b440f5517..67cf75719 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -17317,14 +17317,18 @@ logical_port=sw0-vir) = x], [0], []) check_virtual_offlows_present() { hv=$1 - AT_CHECK([as $hv ovs-ofctl dump-flows br-int table=44 | ofctl_strip_all | grep "priority=2000"], [0], [dnl - table=44, priority=2000,ip,metadata=0x1 actions=resubmit(,45) - table=44, priority=2000,ipv6,metadata=0x1 actions=resubmit(,45) + sw0_dp_key=$(fetch_column Datapath_Binding tunnel_key external_ids:name=sw0) + lr0_dp_key=$(fetch_column Datapath_Binding tunnel_key external_ids:name=lr0) + lr0_public_dp_key=$(fetch_column Port_Binding tunnel_key logical_port=lr0-public) + + AT_CHECK_UNQUOTED([as $hv ovs-ofctl dump-flows br-int table=44 | ofctl_strip_all | grep "priority=2000"], [0], [dnl + table=44, priority=2000,ip,metadata=0x$sw0_dp_key actions=resubmit(,45) + table=44, priority=2000,ipv6,metadata=0x$sw0_dp_key actions=resubmit(,45) ]) - AT_CHECK([as $hv ovs-ofctl dump-flows br-int table=11 | ofctl_strip_all | \ + AT_CHECK_UNQUOTED([as $hv ovs-ofctl dump-flows br-int table=11 | ofctl_strip_all | \ grep "priority=92" | grep 172.168.0.50], [0], [dnl - table=11, priority=92,arp,reg14=0x3,metadata=0x3,arp_tpa=172.168.0.50,arp_op=1 actions=move:NXM_OF_ETH_SRC[[]]->NXM_OF_ETH_DST[[]],mod_dl_src:10:54:00:00:00:10,load:0x2->NXM_OF_ARP_OP[[]],move:NXM_NX_ARP_SHA[[]]->NXM_NX_ARP_THA[[]],load:0x105400000010->NXM_NX_ARP_SHA[[]],push:NXM_OF_ARP_SPA[[]],push:NXM_OF_ARP_TPA[[]],pop:NXM_OF_ARP_SPA[[]],pop:NXM_OF_ARP_TPA[[]],move:NXM_NX_REG14[[]]->NXM_NX_REG15[[]],load:0x1->NXM_NX_REG10[[0]],resubmit(,37) + table=11, priority=92,arp,reg14=0x$lr0_public_dp_key,metadata=0x$lr0_dp_key,arp_tpa=172.168.0.50,arp_op=1 actions=move:NXM_OF_ETH_SRC[[]]->NXM_OF_ETH_DST[[]],mod_dl_src:10:54:00:00:00:10,load:0x2->NXM_OF_ARP_OP[[]],move:NXM_NX_ARP_SHA[[]]->NXM_NX_ARP_THA[[]],load:0x105400000010->NXM_NX_ARP_SHA[[]],push:NXM_OF_ARP_SPA[[]],push:NXM_OF_ARP_TPA[[]],pop:NXM_OF_ARP_SPA[[]],pop:NXM_OF_ARP_TPA[[]],move:NXM_NX_REG14[[]]->NXM_NX_REG15[[]],load:0x1->NXM_NX_REG10[[0]],resubmit(,37) ]) }
The test case fails quite often for northd-ddlog because of the tunnel keys mismatch when comparing OpenFlow rules. Keys can change in different runs. This patch fixes it by extracting the expected keys from SB DB before comparison instead of hardcoding. Signed-off-by: Han Zhou <hzhou@ovn.org> --- tests/ovn.at | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-)