[ovs-dev,v3,2/4] system-dpdk: Use dummy-pmd port for packet injection.

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.maximets@ovn.org>
Signed-off-by: David Marchand <david.marchand@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

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.maximets@ovn.org>
> Signed-off-by: David Marchand <david.marchand@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
> 

Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Thanks,
Maxime

On 30 Nov 2021, at 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.maximets@ovn.org>
> Signed-off-by: David Marchand <david.marchand@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,

Changes look good to me!

Acked-by: Eelco Chaudron <echaudro@redhat.com>

Hi David,

Please find my replies inline.

> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Tuesday, November 30, 2021 8:30 PM
> To: dev@openvswitch.org
> Cc: i.maximets@ovn.org; Stokes, Ian <ian.stokes@intel.com>;
> tredaelli@redhat.com; Amber, Kumar <kumar.amber@intel.com>;
> fbl@sysclose.org; echaudro@redhat.com; maxime.coquelin@redhat.com
> Subject: [PATCH v3 2/4] system-dpdk: Use dummy-pmd port for packet injection.
> 
> 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.maximets@ovn.org>
> Signed-off-by: David Marchand <david.marchand@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.
> 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)
> +    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.

> +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
> diff --git a/tests/mfex_fuzzy.py b/tests/mfex_fuzzy.py deleted file mode 100755
> index 3efe1152da..0000000000
> --- a/tests/mfex_fuzzy.py
> +++ /dev/null
> @@ -1,32 +0,0 @@
> -#!/usr/bin/python3
> -
> -import sys
> -
> -from scapy.all import RandMAC, RandIP, PcapWriter, RandIP6, RandShort, fuzz
> -from scapy.all import IPv6, Dot1Q, IP, Ether, UDP, TCP
> -
> -path = str(sys.argv[1]) + "/pcap/fuzzy.pcap"
> -pktdump = PcapWriter(path, append=False, sync=True)
> -
> -for i in range(0, 2000):
> -
> -    # 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
> -    pktdump.write(fuzz(eth / ipv4 / udp))
> -    pktdump.write(fuzz(eth / ipv4 / tcp))
> -    pktdump.write(fuzz(eth / vlan / ipv4 / udp))
> -    pktdump.write(fuzz(eth / vlan / ipv4 / tcp))
> -
> -    # IPv6 packets with fuzzing
> -    pktdump.write(fuzz(eth / ipv6 / udp))
> -    pktdump.write(fuzz(eth / ipv6 / tcp))
> -    pktdump.write(fuzz(eth / vlan / ipv6 / udp))
> -    pktdump.write(fuzz(eth / vlan / ipv6 / tcp))
> diff --git a/tests/pcap/mfex_test.pcap b/tests/pcap/mfex_test.pcap deleted file
> mode 100644 index
> 1aac67b8d643ecb016c758cba4cc32212a80f52a..0000000000000000000000000
> 000000000000000
> GIT binary patch
> literal 0
> HcmV?d00001
> 
> literal 416
> zcmca|c+)~A1{MYw`2U}Qff2}Q<eHVR>K`M68ITRa|G@yFii5$Gfk6YL%z>@uY&}o
> |
> z2s4N<1VH2&7y^V87$)XGOtD~MV$cFgfG~zBGGJ2#YtF$<F=a4i;9x8Q*<ZrSM6Uf
> z
> xK>KST_NTIwYriok6N4Vm)gX-
> Q@<yO<!C`>c^{cp<7_5LgK^UuU{2>VS0RZ!RQ+EIW
> 
> diff --git a/tests/system-dpdk-macros.at b/tests/system-dpdk-macros.at index
> ef0e84e939..5a6b3cbff9 100644
> --- a/tests/system-dpdk-macros.at
> +++ b/tests/system-dpdk-macros.at
> @@ -37,7 +37,7 @@ m4_define([OVS_DPDK_PRE_PHY_SKIP],
>  #
>  # Create an empty database and start ovsdb-server. Add special configuration  #
> dpdk-init to enable DPDK functionality. Start ovs-vswitchd connected to that -#
> database using system devices (no dummies).
> +# database using system devices.
>  #
>  m4_define([OVS_DPDK_START],
>    [dnl Create database.
> @@ -62,7 +62,7 @@ m4_define([OVS_DPDK_START],
>     AT_CHECK([ovs-vsctl --no-wait set Open_vSwitch . other_config:dpdk-extra=--
> log-level=pmd.*:error])
> 
>     dnl Start ovs-vswitchd.
> -   AT_CHECK([ovs-vswitchd --detach --no-chdir --pidfile --log-file -vvconn -
> vofproto_dpif -vunixctl], [0], [stdout], [stderr])
> +   AT_CHECK([ovs-vswitchd --enable-dummy --detach --no-chdir --pidfile
> + --log-file -vvconn -vofproto_dpif -vunixctl], [0], [stdout], [stderr])
>     AT_CAPTURE_FILE([ovs-vswitchd.log])
>     on_exit "kill_ovs_vswitchd `cat ovs-vswitchd.pid`"
>  ])
> diff --git a/tests/system-dpdk.at b/tests/system-dpdk.at index
> 9c8f4bb15a..73c58030b1 100644
> --- a/tests/system-dpdk.at
> +++ b/tests/system-dpdk.at
> @@ -224,22 +224,29 @@ dnl -------------------------------------------------------------
> -------------
>  dnl Add standard DPDK PHY port
>  AT_SETUP([OVS-DPDK - MFEX Autovalidator])
>  AT_KEYWORDS([dpdk])
> +AT_SKIP_IF([! $PYTHON3 -c "import scapy"], [], [])
> 
>  OVS_DPDK_START()
> 
>  dnl Add userspace bridge and attach it to OVS  AT_CHECK([ovs-vsctl add-br br0 -
> - set bridge br0 datapath_type=netdev]) -AT_CHECK([ovs-vsctl add-port br0 p1 --
> set Interface p1 type=dpdk options:dpdk-
> devargs=net_pcap1,rx_pcap=$srcdir/pcap/mfex_test.pcap,infinite_rx=1], [],
> [stdout], [stderr])
> +AT_CHECK([ovs-vsctl add-port br0 p1 -- set interface p1
> +type=dummy-pmd])
>  AT_CHECK([ovs-vsctl show], [], [stdout])
> 
>  AT_SKIP_IF([! ovs-appctl dpif-netdev/miniflow-parser-get | sed 1,4d | grep
> "True"], [], [dnl
>  ])
> 
> +on_exit "pkill -f -x -9 '$PYTHON3 $srcdir/genpkts.py -1'"
> +($PYTHON3 $srcdir/genpkts.py -1 | 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 1000])
> +OVS_WAIT_UNTIL([test `ovs-vsctl get interface p1 statistics:rx_packets`
> +-ge 1000]) pkill -f -x -9 '$PYTHON3 $srcdir/genpkts.py -1'
> 
>  dnl Clean up
>  AT_CHECK([ovs-vsctl del-port br0 p1], [], [stdout], [stderr]) @@ -252,22
> +259,27 @@ dnl Add standard DPDK PHY port  AT_SETUP([OVS-DPDK - MFEX
> Autovalidator Fuzzy])
>  AT_KEYWORDS([dpdk])
>  AT_SKIP_IF([! $PYTHON3 -c "import scapy"], [], [])
> -AT_CHECK([$PYTHON3 $srcdir/mfex_fuzzy.py $srcdir], [], [stdout])
>  OVS_DPDK_START()
> 
>  dnl Add userspace bridge and attach it to OVS  AT_CHECK([ovs-vsctl add-br br0 -
> - set bridge br0 datapath_type=netdev]) -AT_CHECK([ovs-vsctl add-port br0 p1 --
> set Interface p1 type=dpdk options:dpdk-
> devargs=net_pcap1,rx_pcap=$srcdir/pcap/fuzzy.pcap,infinite_rx=1], [], [stdout],
> [stderr])
> +AT_CHECK([ovs-vsctl add-port br0 p1 -- set interface p1
> +type=dummy-pmd])
>  AT_CHECK([ovs-vsctl show], [], [stdout])
> 
>  AT_SKIP_IF([! ovs-appctl dpif-netdev/miniflow-parser-get | sed 1,4d | grep
> "True"], [], [dnl
>  ])
> 
> +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.

Regards
Amber

> +OVS_WAIT_UNTIL([test `ovs-vsctl get interface p1 statistics:rx_packets`
> +-ge 1000]) pkill -f -x -9 '$PYTHON3 $srcdir/genpkts.py -1 fuzz'
> 
>  dnl Clean up
>  AT_CHECK([ovs-vsctl del-port br0 p1], [], [stdout], [stderr]) @@ -278,13
> +290,20 @@ dnl --------------------------------------------------------------------------
>  dnl --------------------------------------------------------------------------
>  AT_SETUP([OVS-DPDK - MFEX Configuration])
>  AT_KEYWORDS([dpdk])
> +AT_SKIP_IF([! $PYTHON3 -c "import scapy"], [], [])
> +
>  OVS_DPDK_START()
>  AT_CHECK([ovs-vsctl --no-wait set Open_vSwitch . other_config:pmd-cpu-
> mask=0xC])  dnl Add userspace bridge and attach it to OVS  AT_CHECK([ovs-vsctl
> add-br br0 -- set bridge br0 datapath_type=netdev]) -AT_CHECK([ovs-vsctl add-
> port br0 p1 -- set Interface p1 type=dpdk options:dpdk-
> devargs=net_pcap1,rx_pcap=$srcdir/pcap/mfex_test.pcap,infinite_rx=1], [],
> [stdout], [stderr])
> +AT_CHECK([ovs-vsctl add-port br0 p1 -- set interface p1
> +type=dummy-pmd])
>  AT_CHECK([ovs-vsctl show], [], [stdout])
> 
> +on_exit "pkill -f -x -9 '$PYTHON3 $srcdir/genpkts.py -1'"
> +($PYTHON3 $srcdir/genpkts.py -1 | while read pkt; do
> +     ovs-appctl netdev-dummy/receive p1 "$pkt" || break
> + done) &
> +
>  AT_CHECK([ovs-appctl dpif-netdev/miniflow-parser-set scalar 1], [2],  [], [dnl
>  Error: unknown argument 1.
> @@ -377,6 +396,8 @@ Error: invalid study_pkt_cnt value: -pmd.
>  ovs-appctl: ovs-vswitchd: server returned an error
>  ])
> 
> +pkill -f -x -9 '$PYTHON3 $srcdir/genpkts.py -1'
> +
>  dnl Clean up
>  AT_CHECK([ovs-vsctl del-port br0 p1], [], [stdout], [stderr])
> OVS_VSWITCHD_STOP("m4_join([], [SYSTEM_DPDK_ALLOWED_LOGS], [
> --
> 2.23.0

On Wed, Dec 1, 2021 at 3:52 PM Amber, Kumar <kumar.amber@intel.com> 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.


> > +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.


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.

> -----Original Message-----
> From: dev <ovs-dev-bounces@openvswitch.org> On Behalf Of David Marchand
> Sent: Thursday, December 2, 2021 12:21 PM
> To: Amber, Kumar <kumar.amber@intel.com>
> Cc: dev@openvswitch.org; i.maximets@ovn.org; fbl@sysclose.org;
> maxime.coquelin@redhat.com
> 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 <kumar.amber@intel.com>
> 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())

The same issue exists here with TCP sport/dport, which were previously randomly
generated, and are now being hard-coded to a small set of specific values;
  previously:    tcp = TCP(dport=RandShort(), sport=RandShort())

Amber correctly points out that hard-coding these values is a regression in testing.
Re-writing a PCAP file is easy, please refer to the current "mfex_fuzzy.py" script.
Updating hex ethernet addresses manually, or TCP ports manually instead of
automatically generating them is a step backwards.

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.


> > > +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.


(Side note; there are methods to get reproducible PRNG built into unit tests,
see this project for example, where exactly that is achieved by printing the "seed"
value of the PRNG before test, allowing developers to reproduce the exact run:
https://nemequ.github.io/munit/#prng)


> 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.

Regards, -Harry

On Thu, Dec 2, 2021 at 2:56 PM Van Haaren, Harry
<harry.van.haaren@intel.com> wrote:
>
> > -----Original Message-----
> > From: dev <ovs-dev-bounces@openvswitch.org> On Behalf Of David Marchand
> > Sent: Thursday, December 2, 2021 12:21 PM
> > To: Amber, Kumar <kumar.amber@intel.com>
> > Cc: dev@openvswitch.org; i.maximets@ovn.org; fbl@sysclose.org;
> > maxime.coquelin@redhat.com
> > 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 <kumar.amber@intel.com>
> > 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?

> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Thursday, December 2, 2021 3:18 PM
> To: Van Haaren, Harry <harry.van.haaren@intel.com>
> Cc: Amber, Kumar <kumar.amber@intel.com>; dev@openvswitch.org;
> i.maximets@ovn.org; fbl@sysclose.org; maxime.coquelin@redhat.com
> Subject: Re: [ovs-dev] [PATCH v3 2/4] system-dpdk: Use dummy-pmd port for
> packet injection.
> 
> On Thu, Dec 2, 2021 at 2:56 PM Van Haaren, Harry
> <harry.van.haaren@intel.com> wrote:
> >
> > > -----Original Message-----
> > > From: dev <ovs-dev-bounces@openvswitch.org> On Behalf Of David
> Marchand
> > > Sent: Thursday, December 2, 2021 12:21 PM
> > > To: Amber, Kumar <kumar.amber@intel.com>
> > > Cc: dev@openvswitch.org; i.maximets@ovn.org; fbl@sysclose.org;
> > > maxime.coquelin@redhat.com
> > > 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 <kumar.amber@intel.com>
> > > 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.

Apologies, my misunderstanding - I was under the impression the intent
was to replace all of them.

> 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.

Understood - gotcha.

> > 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.

Yes, agree.

> > > > > +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?

I guess this is a tradeoff, we have fuzzing quantity & validation vs time.

This patch reduces the performance of the fuzzing (due to dummy-inject instead
of PCAP PMD read with DPDK+PCAP dependency), and trades off the amount
of fuzzing that will be done in the CI as a result.

Given that the other MFEX fuzzing tests are still valuable for stress-testing using
the DPDK PCAP PMD, I wonder is there enough value to have an option of PCAP
or dummy-inject instead of removing the PCAP approach?

For example, can we runtime-detect if OVS is DPDK-PCAP enabled? And use the
high performance method in that case?

Regards, -Harry

On 12/3/21 17:47, Van Haaren, Harry wrote:
>> -----Original Message-----
>> From: David Marchand <david.marchand@redhat.com>
>> Sent: Thursday, December 2, 2021 3:18 PM
>> To: Van Haaren, Harry <harry.van.haaren@intel.com>
>> Cc: Amber, Kumar <kumar.amber@intel.com>; dev@openvswitch.org;
>> i.maximets@ovn.org; fbl@sysclose.org; maxime.coquelin@redhat.com
>> Subject: Re: [ovs-dev] [PATCH v3 2/4] system-dpdk: Use dummy-pmd port for
>> packet injection.
>>
>> On Thu, Dec 2, 2021 at 2:56 PM Van Haaren, Harry
>> <harry.van.haaren@intel.com> wrote:
>>>
>>>> -----Original Message-----
>>>> From: dev <ovs-dev-bounces@openvswitch.org> On Behalf Of David
>> Marchand
>>>> Sent: Thursday, December 2, 2021 12:21 PM
>>>> To: Amber, Kumar <kumar.amber@intel.com>
>>>> Cc: dev@openvswitch.org; i.maximets@ovn.org; fbl@sysclose.org;
>>>> maxime.coquelin@redhat.com
>>>> 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 <kumar.amber@intel.com>
>>>> 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.
> 
> Apologies, my misunderstanding - I was under the impression the intent
> was to replace all of them.
> 
>> 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.
> 
> Understood - gotcha.
> 
>>> 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.
> 
> Yes, agree.
> 
>>>>>> +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 agree that fuzzing is a great testing method and OVS actually uses it
via OSS-Fuzz project.  But I also agree that unit/system tests should be
as stable as possible, i.e. not be flaky.  Hence, fuzzing should not
really be part of them.

Also, fuzzing is not a replacement for unit tests, because it's unreliable.
For example, I tried to generate the pcap file with mfex_fuzz script and
didn't found a single valid TCP packet in it.  So, it doesn't cover even
very basic cases of valid traffic, and obviously any special cases of
valid traffic are not covered too.

Not in this patch set, but in general, we need at least pass all the
packets generated by tests/flowgen.py through the autovalidator.

Harry, Is that something you can work on?

>>
>> I did not remove the fuzzing part, just made it so it runs in a
>> reasonable amount of time.

This looks reasonable to me.  See below.

>>
>>
>>
>>>
>>>> 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?
> 
> I guess this is a tradeoff, we have fuzzing quantity & validation vs time.
> 
> This patch reduces the performance of the fuzzing (due to dummy-inject instead
> of PCAP PMD read with DPDK+PCAP dependency), and trades off the amount
> of fuzzing that will be done in the CI as a result.
> 
> Given that the other MFEX fuzzing tests are still valuable for stress-testing using
> the DPDK PCAP PMD, I wonder is there enough value to have an option of PCAP
> or dummy-inject instead of removing the PCAP approach?
> 
> For example, can we runtime-detect if OVS is DPDK-PCAP enabled? And use the
> high performance method in that case?

I think the end goal should be to remove the DPDK dependency entirely,
so tests can be moved out of system-dpdk (patch #3), because they are
not actual system tests (in OVS's meaning of that term).

This move will compensate the reduced number of fuzzy runs with the
increased number of testsuite runs in general, since people who actually
has the hardware will run these tests while running generic testsuite.
I don't think that a lot of people actually runs system tests.

For your in-house testing, Harry, I assume, you might just run mfex tests
as many times as you think you need (Assuming that packets are actually
different every time, otherwise the whole fuzzy testing concept is kind
of pointless) until the right solution for fuzzing is in place.  That
should not be hard to do.

The right solution for the fuzzing should be: implement a fuzzing target
for the OSS-Fuzz and let it do its job.  All the found issues could be
added as regression tests to the generic testsuite along with other unit
tests.  It's a current workflow for all fuzzing tests we have.
See tests/oss-fuzz/ and tests/fuzz-regression/ directories for details.

Harry, Amber, can you work on that?

Once this is done, we can remove all the fuzzing parts for autovalidator
from the current testsuite keeping only static non-random tests.

For the current patch: Taking into account all that I said, I don't think
that reduced number of fuzzy packets should be a blocker here.

Best regards, Ilya Maximets.

On 12/6/21 17:54, Ilya Maximets wrote:
> On 12/3/21 17:47, Van Haaren, Harry wrote:
>>> -----Original Message-----
>>> From: David Marchand <david.marchand@redhat.com>
>>> Sent: Thursday, December 2, 2021 3:18 PM
>>> To: Van Haaren, Harry <harry.van.haaren@intel.com>
>>> Cc: Amber, Kumar <kumar.amber@intel.com>; dev@openvswitch.org;
>>> i.maximets@ovn.org; fbl@sysclose.org; maxime.coquelin@redhat.com
>>> Subject: Re: [ovs-dev] [PATCH v3 2/4] system-dpdk: Use dummy-pmd port for
>>> packet injection.
>>>
>>> On Thu, Dec 2, 2021 at 2:56 PM Van Haaren, Harry
>>> <harry.van.haaren@intel.com> wrote:
>>>>
>>>>> -----Original Message-----
>>>>> From: dev <ovs-dev-bounces@openvswitch.org> On Behalf Of David
>>> Marchand
>>>>> Sent: Thursday, December 2, 2021 12:21 PM
>>>>> To: Amber, Kumar <kumar.amber@intel.com>
>>>>> Cc: dev@openvswitch.org; i.maximets@ovn.org; fbl@sysclose.org;
>>>>> maxime.coquelin@redhat.com
>>>>> 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 <kumar.amber@intel.com>
>>>>> 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.
>>
>> Apologies, my misunderstanding - I was under the impression the intent
>> was to replace all of them.
>>
>>> 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.
>>
>> Understood - gotcha.
>>
>>>> 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.
>>
>> Yes, agree.
>>
>>>>>>> +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 agree that fuzzing is a great testing method and OVS actually uses it
> via OSS-Fuzz project.  But I also agree that unit/system tests should be
> as stable as possible, i.e. not be flaky.  Hence, fuzzing should not
> really be part of them.
> 
> Also, fuzzing is not a replacement for unit tests, because it's unreliable.
> For example, I tried to generate the pcap file with mfex_fuzz script and
> didn't found a single valid TCP packet in it.  So, it doesn't cover even
> very basic cases of valid traffic, and obviously any special cases of
> valid traffic are not covered too.
> 
> Not in this patch set, but in general, we need at least pass all the
> packets generated by tests/flowgen.py through the autovalidator.
> 
> Harry, Is that something you can work on?
> 
>>>
>>> I did not remove the fuzzing part, just made it so it runs in a
>>> reasonable amount of time.
> 
> This looks reasonable to me.  See below.
> 
>>>
>>>
>>>
>>>>
>>>>> 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?
>>
>> I guess this is a tradeoff, we have fuzzing quantity & validation vs time.
>>
>> This patch reduces the performance of the fuzzing (due to dummy-inject instead
>> of PCAP PMD read with DPDK+PCAP dependency), and trades off the amount
>> of fuzzing that will be done in the CI as a result.
>>
>> Given that the other MFEX fuzzing tests are still valuable for stress-testing using
>> the DPDK PCAP PMD, I wonder is there enough value to have an option of PCAP
>> or dummy-inject instead of removing the PCAP approach?
>>
>> For example, can we runtime-detect if OVS is DPDK-PCAP enabled? And use the
>> high performance method in that case?
> 
> I think the end goal should be to remove the DPDK dependency entirely,
> so tests can be moved out of system-dpdk (patch #3), because they are

It should be "patch #4", of course.

> not actual system tests (in OVS's meaning of that term).
> 
> This move will compensate the reduced number of fuzzy runs with the
> increased number of testsuite runs in general, since people who actually
> has the hardware will run these tests while running generic testsuite.
> I don't think that a lot of people actually runs system tests.
> 
> For your in-house testing, Harry, I assume, you might just run mfex tests
> as many times as you think you need (Assuming that packets are actually
> different every time, otherwise the whole fuzzy testing concept is kind
> of pointless) until the right solution for fuzzing is in place.  That
> should not be hard to do.
> 
> The right solution for the fuzzing should be: implement a fuzzing target
> for the OSS-Fuzz and let it do its job.  All the found issues could be
> added as regression tests to the generic testsuite along with other unit
> tests.  It's a current workflow for all fuzzing tests we have.
> See tests/oss-fuzz/ and tests/fuzz-regression/ directories for details.
> 
> Harry, Amber, can you work on that?
> 
> Once this is done, we can remove all the fuzzing parts for autovalidator
> from the current testsuite keeping only static non-random tests.
> 
> For the current patch: Taking into account all that I said, I don't think
> that reduced number of fuzzy packets should be a blocker here.
> 
> Best regards, Ilya Maximets.
>

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.maximets@ovn.org>
> Signed-off-by: David Marchand <david.marchand@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.

> 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?

> +    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.

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)

---

What do you think?

Best regards, Ilya Maximets.

On Mon, Dec 6, 2021 at 11:16 PM Ilya Maximets <i.maximets@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.maximets@ovn.org>
> > Signed-off-by: David Marchand <david.marchand@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.

On 12/14/21 16:38, David Marchand wrote:
> On Mon, Dec 6, 2021 at 11:16 PM Ilya Maximets <i.maximets@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.maximets@ovn.org>
>>> Signed-off-by: David Marchand <david.marchand@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?

Yep.

> I'll still update this part in my patch (and rebase if there is a
> conflict once your doc patch is merged).

OK.  And we might not even need a rebase, as rebase would mean just
dropping the documentation changes, AFAICT.  And that's not hard to
do while applying the patch.

> 
> 
>>
>>> 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.
> 
>