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