[ovs-dev] [PATCH 3/3] tests: Ignore options order in dhcpv4 ovn test.

[Balazs Nemeth]

>On 14/12/2016 06:38, "Balazs Nemeth" <balazs.nemeth at ericsson.com> wrote:
>
>>>On 12/12/2016 13:14, "Ben Pfaff" <blp at ovn.org> wrote:
>>>
>>>>On Thu, Dec 08, 2016 at 06:50:32PM -0800, Daniele Di Proietto wrote:
>>>>> The order of the options in the packet generated by ovs-controller
>>>>> depends on the hash function.  I believe that murmur hash (our default)
>>>>> produces different outputs depending on the endianness of the system.
>>>>>
>>>>> This commit fixes the test by reordering the options in the packet
>>>>> before checking them.
>>>>>
>>>>> This was reported before as a failure of the test on x86 with sse42
>>>>> enabled, I'm fixing it now because it affects other targets build by
>>>>> distros by default.
>>>>>
>>>>> Reported-at: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=840770
>>>>> Signed-off-by: Daniele Di Proietto <diproiettod at vmware.com>
>>>>
>>>>I agree that this would solve the problem.
>>>>
>>>>I think that it might be nicer to instead sort the options in
>>>>build_dhcpv4_action() and build_dhcpv6_action() in ovn-northd.c.  Then
>>>>the generated logical actions won't differ from one architecture to
>>>>another, so that if we one day push a test to a higher level we won't
>>>>have an underlying source of differences.  There's even a helper
>>>>smap_sort() that could do most of the work.
>>>>
>>>>Are you willing to make that change?
>>>
>>>Sure, it's better than parsing TLVs with awk.  I'll send a v2.
>>>
>>>Thanks,
>>>
>>>Daniele
>>>
>>>>
>>>>Thanks,
>>>>
>>>>Ben.
>>
>>
>>Hi Daniele,
>>
>>I applied this patch on top of branch-2.6 (which is including 1/3 and 2/3 patch). I built OVS with SSE4.2 flag. UT 2246 is still failing
>> due to only 571st char of 'packets' and 'expected' files are compared and those are different in case of CRC32 hash. The following patch will fix the failing UT in case of build without any extra flag or with SSE4.2 as well:
>>
>>diff --git a/tests/ovn.at b/tests/ovn.at
>>index 8b1228f..3f4c6cf 100644
>>--- a/tests/ovn.at
>>+++ b/tests/ovn.at
>>@@ -3724,8 +3724,8 @@ AT_CHECK([cat 1.packets | cut -c 53-570], [0], [expout])
>># The ordering of the options in the packet depends on the hash function used
>># and possibly on the endianness of the system.  We parse and sort both the
>># expected and the generated packet.
>>-cat 1.expected | cut -c 571 | sort_dhcpv4opts_tlv > expout
>>-AT_CHECK([cat 1.packets | cut -c 571 | sort_dhcpv4opts_tlv], [0], [expout])
>>+cat 1.expected | cut -c 571- | sort_dhcpv4opts_tlv > expout
>>+AT_CHECK([cat 1.packets | cut -c 571- | sort_dhcpv4opts_tlv], [0], [expout])
>>
>># ovs-ofctl also resumes the packets and this causes other ports to receive
>># the DHCP request packet. So reset the pcap files so that its easier to test.
>>@@ -3752,8 +3752,8 @@ AT_CHECK([cat 2.packets | cut -c 53-570], [0], [expout])
>># The ordering of the options in the packet depends on the hash function used
>># and possibly on the endianness of the system.  We parse and sort both the
>># expected and the generated packet.
>>-cat 2.expected | cut -c 571 | sort_dhcpv4opts_tlv > expout
>>-AT_CHECK([cat 2.packets | cut -c 571 | sort_dhcpv4opts_tlv], [0], [expout])
>>+cat 2.expected | cut -c 571- | sort_dhcpv4opts_tlv > expout
>>+AT_CHECK([cat 2.packets | cut -c 571- | sort_dhcpv4opts_tlv], [0], [expout])
>>
>>reset_pcap_file hv1-vif1 hv1/vif1
>>reset_pcap_file hv1-vif2 hv1/vif2
>
>Hi Balazs,
>
>I think you're right, I was only cutting the 571st character, thanks for spotting the error.
>
>Anyway, we decided to drop this patch, go with another approach and sort the options in the program, not in the test:
>
>https://mail.openvswitch.org/pipermail/ovs-dev/2016-December/326133.html
>
>I already merged this to master:
>
>7c76bf4e0e27 ("ovn-northd: Sort options in put_dhcp(v6)_opts.")
>
>and branch-2.6:
>
>cbd39073245e ("ovn-northd: Sort options in put_dhcp(v6)_opts.")
>
>Can you try latest master and branch-2.6 without any other patch applied?
>
>It seems to pass for me on x86_64 x86_64+sse4.2 and mips.
>
>Thanks,
>
>Daniele

Hi Daniele,

Yes, I tried with the latest master and branch-2.6 including the mentioned patch. All TC is successful with/without SSE 4.2 support as well.
Thank you. Also my question on discuss list with topic "Failing UT 2246 in case of build with SSE4.2 support" is closed now.

BR,
Balazs