[ovs-dev] [PATCH ovn v2] ovn.at: Fix test "virtual ports -- ovn-northd-ddlog".

Han Zhou hzhou at ovn.org
Tue Jun 22 01:58:37 UTC 2021


On Thu, Jun 17, 2021 at 12:59 PM Mark Michelson <mmichels at redhat.com> wrote:
>
> On 6/14/21 2:44 PM, Ben Pfaff wrote:
> > On Fri, Jun 11, 2021 at 03:48:52PM -0700, 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.
> >>
> >> There are some other potential timing issues in this test and this
> >> patch fixes them as well by replacing AT_CHECK with OVS_WAIT_UNTIL.
> >>
> >> Signed-off-by: Han Zhou <hzhou at ovn.org>
> >
> > Awesome!  Thank you.
> >
> >> -AT_CHECK([test x$(ovn-sbctl --bare --columns chassis find
port_binding \
> >> +OVS_WAIT_UNTIL([test x$(ovn-sbctl --bare --columns chassis find
port_binding \
> >>   logical_port=sw0-vir) = x], [0], [])
> >
> > I think the above can be better written:
> >      wait_row_count Port_Binding 0 logical_port=sw0-vir
>
> I don't think this is correct. The test is not attempting to wait for
> the Port_Binding record to be deleted. It's waiting for the chassis
> column in the Port_Binding to contain an empty string. I think
> wait_column() could work:
>
> wait_column "" Port_Binding chassis logical_port=sw0-vir
>
> (assuming that testing for an empty string works)
>

Thanks Ben and Mark! I used wait_column in v4:
https://patchwork.ozlabs.org/project/ovn/patch/20210622015529.2005615-1-hzhou@ovn.org/

> >
> >
> >>   # Cleanup hv1-vif3.
> >>   as hv1
> >>   ovs-vsctl del-port hv1-vif3
> >>
> >> -AT_CHECK([test x$(ovn-sbctl --bare --columns chassis find
port_binding \
> >> +OVS_WAIT_UNTIL([test x$(ovn-sbctl --bare --columns chassis find
port_binding \
> >>   logical_port=sw0-vir) = x], [0], [])
> >
> > Ditto?
> >
> >> +    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)
> >
> > I think that the above will retrieve tunnel keys in decimal...
> >
> >> +    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)
> >>   ])
> >
> > ...therefore I think that the above 0x should not be there.  (I guess
> > the test passes because the numbers in the test are all under 10.)
>
> Yeah, it should probably be fine for this test to not worry about this.
>
I changed it to hex format in v4.

Thanks,
Han


More information about the dev mailing list