On 2/16/24 21:40, Mike Pattrick wrote: > On Fri, Feb 16, 2024 at 3:23 PM Ilya Maximets <[email protected]> wrote: >> >> On 2/15/24 23:53, Mike Pattrick wrote: >>> Previously a gap existed in the tunnel system tests where only ICMP and >>> TCP traffic was tested. However, the code paths using for UDP traffic is >>> different then either of those and should also be tested. >>> >>> Some of the modified tests had previously checked for TCP with ncat but >>> didn't include an appropriate check for ncat support. That check was >>> added to these tests. >>> >>> Signed-off-by: Mike Pattrick <[email protected]> >>> --- >>> tests/system-traffic.at | 118 +++++++++++++++++++++++++++++++++++++--- >>> 1 file changed, 111 insertions(+), 7 deletions(-) >>> >> >> Hi, Mike. Thanks for the fixes. They look mostly fine to me and I >> tested them also with David's triple-tunnel test and it seem to work. >> >> The code becomes excessively hard to read though. I think the dp-packet >> API has to be re-done before userspace-tso can be declared not >> experimental. I don't have any good ideas on how to actually do that >> though. > > I strongly agree, I can send a proposal later.
Thanks! > >> But this should not be addressed in this patch set. >> >> See a few comments for the tests below. >> >>> diff --git a/tests/system-traffic.at b/tests/system-traffic.at >>> index e68fe7e18..ca54a0f73 100644 >>> --- a/tests/system-traffic.at >>> +++ b/tests/system-traffic.at >>> @@ -292,6 +292,7 @@ OVS_TRAFFIC_VSWITCHD_STOP >>> AT_CLEANUP >>> >>> AT_SETUP([datapath - ping over vxlan tunnel]) >>> +AT_SKIP_IF([test $HAVE_NC = no]) >>> OVS_CHECK_VXLAN() >>> >>> OVS_TRAFFIC_VSWITCHD_START() >>> @@ -318,6 +319,10 @@ NS_CHECK_EXEC([at_ns0], [ping -q -c 3 -i 0.3 -W 2 >>> 172.31.1.100 | FORMAT_PING], [ >>> 3 packets transmitted, 3 received, 0% packet loss, time 0ms >>> ]) >>> >>> +dnl Start ncat listeners. >>> +OVS_DAEMONIZE([nc -l 10.1.1.100 1234 > tcp_data], [nc.pid]) >>> +NETNS_DAEMONIZE([at_ns0], [nc -l -u 10.1.1.1 4321 > udp_data], [nc2.pid]) >>> + >> >> This should not be here. We should start them below, close to their usage. >> >>> dnl Okay, now check the overlay with different packet sizes >>> NS_CHECK_EXEC([at_ns0], [ping -q -c 3 -i 0.3 -W 2 10.1.1.100 | >>> FORMAT_PING], [0], [dnl >>> 3 packets transmitted, 3 received, 0% packet loss, time 0ms >>> @@ -329,15 +334,29 @@ NS_CHECK_EXEC([at_ns0], [ping -s 3200 -q -c 3 -i 0.3 >>> -W 2 10.1.1.100 | FORMAT_PI >>> 3 packets transmitted, 3 received, 0% packet loss, time 0ms >>> ]) >>> >>> +dnl Verify that ncat is ready. >>> +OVS_WAIT_UNTIL([netstat -ln | grep :1234]) >>> +OVS_WAIT_UNTIL([NS_EXEC([at_ns0], [netstat -ln | grep :4321])]) >>> + >>> dnl Check large bidirectional TCP. >>> AT_CHECK([dd if=/dev/urandom of=payload.bin bs=60000 count=1 2> /dev/null]) >>> -OVS_DAEMONIZE([nc -l 10.1.1.100 1234 > data], [nc.pid]) >>> NS_CHECK_EXEC([at_ns0], [nc $NC_EOF_OPT 10.1.1.100 1234 < payload.bin]) >>> >>> dnl Wait until transfer completes before checking. >>> OVS_WAIT_WHILE([kill -0 $(cat nc.pid)]) >>> -AT_CHECK([diff -q payload.bin data], [0]) >>> +AT_CHECK([diff -q payload.bin tcp_data], [0]) >>> + >>> +dnl Check UDP >> >> Period at the end. >> >>> +AT_CHECK([dd if=/dev/urandom of=payload.bin bs=600 count=1 2> /dev/null]) >>> +AT_CHECK([nc $NC_EOF_OPT -u 10.1.1.1 4321 < payload.bin]) >>> >>> +dnl The UDP listener will just listen forever if not terminated. >>> +OVS_WAIT_UNTIL([kill -0 $(cat nc2.pid)]) >>> +AT_CHECK([kill $(cat nc2.pid)]) >>> + >>> +dnl Wait until transfer completes before checking. >>> +OVS_WAIT_WHILE([kill -0 $(cat nc2.pid)]) >> >> I don't think 6 lines above do anything useful. >> We check that the process is running, kill it, wait until it dies. >> But if it didn't finish writing the data, we will kill it and end >> up with incomplete data... >> >>> +AT_CHECK([diff -q payload.bin udp_data], [0]) >> >> ... so instead of all these manipulations we can just use >> OVS_WAIT_UNTIL here and wait until the file is fully transferred. >> >> The server will be stopped by the on_exit hook registered by the >> NETNS_DAEMONIZE. >> >> We can also apply the same strategy to the TCP check, we can just >> wait on a content and not wait for the process to exit. >> >> The same applies to all other tests here. >> >> I already tried these changes here: >> >> https://github.com/igsilya/ovs/commit/f544d397250714b7ba8c1a3f81eada29ffd59142 >> >> I did several iterations of GHA runs with these changes and they >> work fine in CI. >> >> If you agree, I can fold those in while applying the set. >> >> Please, let me know as soon as possible as we need to proceed with >> the release. > > I agree with those changes. Thanks! I adjusted the subject of this patch a little, folded discussed changes in and applied the set. Also backported to 3.3. Best regards, Ilya Maximets. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
