On Mon, Dec 6, 2021 at 11:16 PM Ilya Maximets <i.maxim...@ovn.org> wrote:
>
> On 11/30/21 16:00, David Marchand wrote:
> > net_pcap is not always available in DPDK (like, in a dev
> > environment when you forgot to install the libpcap-devel).
> > On the other hand, OVS already has its own way to inject packets into a
> > bridge. Let's make use of it.
> >
> > This solution is slower than net/pcap DPDK, so lower the number of
> > expected packets so that it runs in OVS_CTL_TIMEOUT seconds.
> >
> > While at it, convert "known" packets from pcap to scapy so that the
> > injected packets can be updated without having to read/write a pcap file.
> >
> > Note: this change also (avoids/) fixes a python exception in PcapWriter
> > with scapy 2.4.3 that comes from EPEL.
> >
> > Suggested-by: Ilya Maximets <i.maxim...@ovn.org>
> > Signed-off-by: David Marchand <david.march...@redhat.com>
> > ---
> > Changes since v2:
> > - updated documentation,
> > - cleaned tests/automake.mk,
> > - fixed shebang in python script,
> > - added missing check for scapy availability,
> >
> > Changes since v1:
> > - renamed generator script,
> > - decreased packet count for fuzzy test,
> > - simplified wait expression for packet count,
> >
> > ---
> > Documentation/topics/dpdk/bridge.rst | 7 ++--
> > tests/automake.mk | 7 +---
> > tests/genpkts.py | 56 +++++++++++++++++++++++++++
> > tests/mfex_fuzzy.py | 32 ---------------
> > tests/pcap/mfex_test.pcap | Bin 416 -> 0 bytes
> > tests/system-dpdk-macros.at | 4 +-
> > tests/system-dpdk.at | 33 +++++++++++++---
> > 7 files changed, 89 insertions(+), 50 deletions(-)
> > create mode 100755 tests/genpkts.py
> > delete mode 100755 tests/mfex_fuzzy.py
> > delete mode 100644 tests/pcap/mfex_test.pcap
> >
> > diff --git a/Documentation/topics/dpdk/bridge.rst
> > b/Documentation/topics/dpdk/bridge.rst
> > index f645b9ade5..648ce203eb 100644
> > --- a/Documentation/topics/dpdk/bridge.rst
> > +++ b/Documentation/topics/dpdk/bridge.rst
> > @@ -408,7 +408,7 @@ Fuzzy tests can also be done on miniflow extract with
> > the help of
> > auto-validator and Scapy. The steps below describes the steps to
> > reproduce the setup with IP being fuzzed to generate packets.
> >
> > -Scapy is used to create fuzzy IP packets and save them into a PCAP ::
> > +Scapy is used to create fuzzy IP packets (see tests/genpkts.py) ::
> >
> > pkt = fuzz(Ether()/IP()/TCP())
> >
> > @@ -418,9 +418,8 @@ Set the miniflow extract to autovalidator using ::
> >
> > OVS is configured to receive the generated packets ::
> >
> > - $ ovs-vsctl add-port br0 pcap0 -- \
> > - set Interface pcap0 type=dpdk options:dpdk-devargs=net_pcap0
> > - "rx_pcap=fuzzy.pcap"
> > + $ ovs-vsctl add-port br0 p1 -- \
> > + set Interface p1 type=dummy-pmd
> >
> > With this workflow, the autovalidator will ensure that all MFEX
> > implementations are classifying each packet in exactly the same way.
>
> I'm actually getting a hard time understanding a value of this section
> of the doc for the end user, who is the target audience for this doc.
>
> And the next section "Unit Fuzzy test with Autovalidator" is incorrectly
> formatted and also almost to the word equal to the "Unit Test Miniflow
> Extract" section.
>
> It doesn't make a lot of sense talking that functionality is covered by
> unit tests. Everything should be covered by unit tests by default.
> And this is definitely not something that should be part of an end-user
> high-level documentation.
>
> It's not a problem of a current patch set, but I'd suggest to just
> remove the last 3 sections of dpdk/bridge.rst. They should not be there.
I guess this comment is covered by your patch that refactors the
documentation, correct?
I'll still update this part in my patch (and rebase if there is a
conflict once your doc patch is merged).
>
> > diff --git a/tests/automake.mk b/tests/automake.mk
> > index 43731d0973..3bc74a5aff 100644
> > --- a/tests/automake.mk
> > +++ b/tests/automake.mk
> > @@ -143,11 +143,6 @@ $(srcdir)/tests/fuzz-regression-list.at:
> > tests/automake.mk
> > echo "TEST_FUZZ_REGRESSION([$$basename])"; \
> > done > $@.tmp && mv $@.tmp $@
> >
> > -EXTRA_DIST += $(MFEX_AUTOVALIDATOR_TESTS)
> > -MFEX_AUTOVALIDATOR_TESTS = \
> > - tests/pcap/mfex_test.pcap \
> > - tests/mfex_fuzzy.py
> > -
> > OVSDB_CLUSTER_TESTSUITE_AT = \
> > tests/ovsdb-cluster-testsuite.at \
> > tests/ovsdb-execution.at \
> > @@ -518,7 +513,7 @@ tests_test_type_props_SOURCES = tests/test-type-props.c
> > CHECK_PYFILES = \
> > tests/appctl.py \
> > tests/flowgen.py \
> > - tests/mfex_fuzzy.py \
> > + tests/genpkts.py \
> > tests/ovsdb-monitor-sort.py \
> > tests/test-daemon.py \
> > tests/test-json.py \
> > 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)
>
> We already have the same packet. Is it intended behavior?
That's what the unit test was doing before, I translated it in python.
I don't get the intention of always testing with the same packets and
I see little value in it.
We could even remove the loop on receiving 1k packets.
>
> > + 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()]
> > +elif sys.argv[2] == 'fuzz':
> > + # Generate random protocol bases, use a fuzz() over the combined packet
> > + # for full fuzzing.
> > + eth = Ether(src=RandMAC(), dst=RandMAC())
> > + vlan = Dot1Q()
> > + ipv4 = IP(src=RandIP(), dst=RandIP())
> > + ipv6 = IPv6(src=RandIP6(), dst=RandIP6())
> > + udp = UDP(dport=RandShort(), sport=RandShort())
> > + tcp = TCP(dport=RandShort(), sport=RandShort())
> > +
> > + # IPv4 packets with fuzzing
> > + tmpl += [fuzz(eth / ipv4 / udp).build().hex()]
> > + tmpl += [fuzz(eth / ipv4 / tcp).build().hex()]
> > + tmpl += [fuzz(eth / vlan / ipv4 / udp).build().hex()]
> > + tmpl += [fuzz(eth / vlan / ipv4 / tcp).build().hex()]
> > +
> > + # IPv6 packets with fuzzing
> > + tmpl += [fuzz(eth / ipv6 / udp).build().hex()]
> > + tmpl += [fuzz(eth / ipv6 / tcp).build().hex()]
> > + tmpl += [fuzz(eth / vlan / ipv6 / udp).build().hex()]
> > + tmpl += [fuzz(eth / vlan / ipv6 / tcp).build().hex()]
> > +
> > +i = 0
> > +count = int(sys.argv[1])
> > +while count != 0:
> > + print(tmpl[i % len(tmpl)])
> > + i += 1
> > + count -= 1
>
> While it's OK to reduce the number of packets for the fuzzy test
> in order to seed it up, we should still generate N different
> fuzzy packets. This script seems to print out the same 8 packets
> again and again.
Argh... yes this is a regression.
>
> Maybe something like this:
>
> def simple():
> tmpl = []
> <generate a few packets>
> return tmpl
>
> def fuzzy():
> tmpl = []
> <generate a few fuzzy packets>
> return tmpl
>
> while True:
> for packet in simple() if len(sys.argv) == 2 else fuzzy():
> print(packet)
> count -= 1
> if count == 0:
> exit(0)
I'll try with your suggestion.
Thanks Ilya.
--
David Marchand
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
