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

Mark Michelson mmichels at redhat.com
Thu Jun 17 19:58:58 UTC 2021


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)

> 
> 
>>   # 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.

> 
> Thanks,
> 
> Ben.
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> 



More information about the dev mailing list