On Thu, Dec 2, 2021 at 2:56 PM Van Haaren, Harry <[email protected]> wrote: > > > -----Original Message----- > > From: dev <[email protected]> On Behalf Of David Marchand > > Sent: Thursday, December 2, 2021 12:21 PM > > To: Amber, Kumar <[email protected]> > > Cc: [email protected]; [email protected]; [email protected]; > > [email protected] > > Subject: Re: [ovs-dev] [PATCH v3 2/4] system-dpdk: Use dummy-pmd port for > > packet injection. > > > > On Wed, Dec 1, 2021 at 3:52 PM Amber, Kumar <[email protected]> > > wrote: > > > > diff --git a/tests/genpkts.py b/tests/genpkts.py new file mode 100755 > > > > index > > > > 0000000000..f64f786ccb > > > > --- /dev/null > > > > +++ b/tests/genpkts.py > > > > @@ -0,0 +1,56 @@ > > > > +#!/usr/bin/env python3 > > > > + > > > > +import sys > > > > + > > > > +from scapy.all import RandMAC, RandIP, RandIP6, RandShort, fuzz from > > > > +scapy.all import IPv6, Dot1Q, IP, Ether, UDP, TCP > > > > + > > > > +if len(sys.argv) < 2: > > > > + print('usage: {} packets_count [fuzz]'.format(sys.argv[0])) > > > > + sys.exit(1) > > > > + > > > > +tmpl = [] > > > > + > > > > +if len(sys.argv) == 2: > > > > + eth = Ether(dst='ff:ff:ff:ff:ff:ff') > > > > + vlan = eth / Dot1Q(vlan=1) > > > > + p = eth / IP() / TCP(sport=20, dport=80, flags='SA', window=8192) > > > > + tmpl += [p.build().hex()] > > > > + p = eth / IP() / UDP(sport=53, dport=53) > > > > + tmpl += [p.build().hex()] > > > > + p = eth / IP() / TCP(sport=20, dport=80, flags='S', window=8192) > > > > + tmpl += [p.build().hex()] > > > > + p = eth / IP() / UDP(sport=53, dport=53) > > > > + tmpl += [p.build().hex()] > > > > + p = vlan / IP() / UDP(sport=53, dport=53) > > > > + tmpl += [p.build().hex()] > > > > + p = vlan / IP() / TCP(sport=20, dport=80, flags='S', window=8192) > > > > + tmpl += [p.build().hex()] > > > > > > Hardcoding the values here is not preferable as we wanted to test the > > optimized implementations > > > with various values contained inside the header. > > > > Those hardcoded values comes from the pcap file that was previously used. > > If you want to add more protocols, it is easier with this patch as you > > only need to update some python script rather than rewrite a pcap > > file. > > The PCAP was written by a script, which generated random MAC addresses; > previously: eth = Ether(src=RandMAC(), dst=RandMAC())
We have 3 MFEX tests. OVS-DPDK - MFEX Autovalidator OVS-DPDK - MFEX Autovalidator fuzzy OVS-DPDK - MFEX Configuration In the first and 3rd ones, a pcap file tests/pcap/mfex_test.pcap (committed to OVS sources) was used before this patch. The hunk above from my patch is about replacing this "hardcoded" pcap binary file with a python script that generates hardcoded packets. For the 2nd test, if you look at the patch, you'll notice that the fuzzy part is handled like before and generate random packets. > > I do not see the value add here, and I'm concerned that when regressions in > these methods are pointed > out that these are not acted on, but instead we are told "this hardcoded > method is better". It is not. Read what I wrote as: "hardcoded python" is better than "hardcoded pcap" binary file. > > > > +on_exit "pkill -f -x -9 '$PYTHON3 $srcdir/genpkts.py -1 fuzz'" > > > > +($PYTHON3 $srcdir/genpkts.py -1 fuzz | while read pkt; do > > > > + ovs-appctl netdev-dummy/receive p1 "$pkt" || break > > > > + done) & > > > > + > > > > AT_CHECK([ovs-appctl dpif-netdev/miniflow-parser-set autovalidator], > > > > [0], > > [dnl > > > > Miniflow extract implementation set to autovalidator. > > > > ]) > > > > > > > > -OVS_WAIT_UNTIL([test `ovs-vsctl get interface p1 statistics | grep -oP > > > > 'rx_packets=\s*\K\d+'` -ge 100000]) > > > > > > We should increase the packet count to at-least 10x the current number to > > have a proper fuzzy testing and we have measured it would only take 10~ to > > 15~ > > sec more. The current runtime did not catch issues when we purposely broke > > the > > implementation and by allowing to run for 10000 packets, it did catch the > > induced > > error. > > > > For me, the fuzzy testing does not have its place in a CI, because it > > is not reproducible. > > I let it in place and just made sure it would not reach the timeout. > > Disagree here, Fuzzing is *ideal* for CI, as it tests different inputs > continuously, > and each CI run improves the confidence in the system. A large number of open > source projects are actively doing large-scale fuzzing in CI instances. > e.g.; https://google.github.io/clusterfuzz/ > > If the fuzzing autotests fails in CI, it still flags that there is *an > issue*. In our > case with AutoValidators for DPCLS and MFEX, it even prints a whole debug log > of "good" miniflow, as well as "bad" results of the optimized implementation. > These VLOG_ERR results would be hugely helpful in identifying & debugging. > > I do not understand the motivation for disabling/limiting fuzzing in CI. Fuzzing is sure a cool thing when run on an identified version to stress it. But those unit tests will be used for gating patches sent on the mailing list. Having non reproducible input means the test may flag a patch as failing because of some previously merged change that did introduce the regression. I did not remove the fuzzing part, just made it so it runs in a reasonable amount of time. > > > On the system I used, this test takes 5s with 1k and timeouts with 10k. > > So I guess the 10s/15s evaluation is dependent on the system. > > > > I prefer to stick to current value. > > > > > > Thanks. > > > > -- > > David Marchand > > Removing automated randomness from Fuzzy testing, and removing/limiting > fuzz-testing in CI runs > are both big steps backwards in my view, please reconsider what this patch is > actually trying to achieve. This patch removes dependency on DPDK pcap to inject traffic in OVS. OVS has its own mechanism and this dependency is unjustified. That's what this patch is about. What do you propose so that we can move forward? -- David Marchand _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
