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. 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. Best regards, Ilya Maximets. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
