[PATCH net 0/4] selftests: openvswitch: Minor fixes for some systems

[PATCH net 0/4] selftests: openvswitch: Minor fixes for some systems

[Aaron Conole]

A number of corner cases were caught when trying to run the selftests on
older systems.  Missed skip conditions, some error cases, and outdated
python setups would all report failures but the issue would actually be
related to some other condition rather than the selftest suite.

Address these individual cases.

Aaron Conole (4):
  selftests: openvswitch: Add version check for pyroute2
  selftests: openvswitch: Catch cases where the tests are killed
  selftests: openvswitch: Skip drop testing on older kernels
  selftests: openvswitch: Fix the ct_tuple for v4

 .../selftests/net/openvswitch/openvswitch.sh  | 21 ++++++++-
 .../selftests/net/openvswitch/ovs-dpctl.py    | 46 ++++++++++++++++++-
 2 files changed, 65 insertions(+), 2 deletions(-)

-- 
2.40.1

[PATCH net 1/4] selftests: openvswitch: Add version check for pyroute2

[Aaron Conole]

Paolo Abeni reports that on some systems the pyroute2 version isn't
new enough to run the test suite.  Ensure that we support a minimum
version of 0.6 for all cases (which does include the existing ones).
The 0.6.1 version was released in May of 2021, so should be
propagated to most installations at this point.

The alternative that Paolo proposed was to only skip when the
add-flow is being run.  This would be okay for most cases, except
if a future test case is added that needs to do flow dump without
an associated add (just guessing).  In that case, it could also be
broken and we would need additional skip logic anyway.  Just draw
a line in the sand now.

Fixes: 25f16c873fb1 ("selftests: add openvswitch selftest suite")
Reported-by: Paolo Abeni <pabeni@redhat.com>
Closes: https://lore.kernel.org/lkml/8470c431e0930d2ea204a9363a60937289b7fdbe.camel@redhat.com/
Signed-off-by: Aaron Conole <aconole@redhat.com>
---
 tools/testing/selftests/net/openvswitch/openvswitch.sh | 2 +-
 tools/testing/selftests/net/openvswitch/ovs-dpctl.py   | 8 ++++++++
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/net/openvswitch/openvswitch.sh b/tools/testing/selftests/net/openvswitch/openvswitch.sh
index 9c2012d70b08e..220c3356901ef 100755
--- a/tools/testing/selftests/net/openvswitch/openvswitch.sh
+++ b/tools/testing/selftests/net/openvswitch/openvswitch.sh
@@ -525,7 +525,7 @@ run_test() {
 	fi
 
 	if python3 ovs-dpctl.py -h 2>&1 | \
-	     grep "Need to install the python" >/dev/null 2>&1; then
+	     grep -E "Need to (install|upgrade) the python" >/dev/null 2>&1; then
 		stdbuf -o0 printf "TEST: %-60s  [PYLIB]\n" "${tdesc}"
 		return $ksft_skip
 	fi
diff --git a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
index 912dc8c490858..9686ca30d516d 100644
--- a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
+++ b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
@@ -28,6 +28,8 @@ try:
     from pyroute2.netlink import nlmsg_atoms
     from pyroute2.netlink.exceptions import NetlinkError
     from pyroute2.netlink.generic import GenericNetlinkSocket
+    import pyroute2
+
 except ModuleNotFoundError:
     print("Need to install the python pyroute2 package.")
     sys.exit(0)
@@ -1998,6 +2000,12 @@ def main(argv):
     nlmsg_atoms.ovskey = ovskey
     nlmsg_atoms.ovsactions = ovsactions
 
+    # version check for pyroute2
+    prverscheck = pyroute2.__version__.split(".")
+    if int(prverscheck[0]) == 0 and int(prverscheck[1]) < 6:
+        print("Need to upgrade the python pyroute2 package.")
+        sys.exit(0)
+
     parser = argparse.ArgumentParser()
     parser.add_argument(
         "-v",
-- 
2.40.1

[PATCH net 2/4] selftests: openvswitch: Catch cases where the tests are killed

[Aaron Conole]

In case of fatal signal, or early abort at least cleanup the current
test case.

Fixes: 25f16c873fb1 ("selftests: add openvswitch selftest suite")
Signed-off-by: Aaron Conole <aconole@redhat.com>
---
 tools/testing/selftests/net/openvswitch/openvswitch.sh | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/testing/selftests/net/openvswitch/openvswitch.sh b/tools/testing/selftests/net/openvswitch/openvswitch.sh
index 220c3356901ef..2a0112be7ead5 100755
--- a/tools/testing/selftests/net/openvswitch/openvswitch.sh
+++ b/tools/testing/selftests/net/openvswitch/openvswitch.sh
@@ -3,6 +3,8 @@
 #
 # OVS kernel module self tests
 
+trap ovs_exit_sig EXIT TERM INT ERR
+
 # Kselftest framework requirement - SKIP code is 4.
 ksft_skip=4
 
-- 
2.40.1

[PATCH net 3/4] selftests: openvswitch: Skip drop testing on older kernels

[Aaron Conole]

Kernels that don't have support for openvswitch drop reasons also
won't have the drop counter reasons, so we should skip the test
completely.  It previously wasn't possible to build a test case
for this without polluting the datapath, so we introduce a mechanism
to clear all the flows from a datapath allowing us to test for
explicit drop actions, and then clear the flows to build the
original test case.

Fixes: 4242029164d6 ("selftests: openvswitch: add explicit drop testcase")
Signed-off-by: Aaron Conole <aconole@redhat.com>
---
 .../selftests/net/openvswitch/openvswitch.sh  | 17 ++++++++++
 .../selftests/net/openvswitch/ovs-dpctl.py    | 34 +++++++++++++++++++
 2 files changed, 51 insertions(+)

diff --git a/tools/testing/selftests/net/openvswitch/openvswitch.sh b/tools/testing/selftests/net/openvswitch/openvswitch.sh
index 2a0112be7ead5..ca7090e71bff2 100755
--- a/tools/testing/selftests/net/openvswitch/openvswitch.sh
+++ b/tools/testing/selftests/net/openvswitch/openvswitch.sh
@@ -144,6 +144,12 @@ ovs_add_flow () {
 	return 0
 }
 
+ovs_del_flows () {
+	info "Deleting all flows from DP: sbx:$1 br:$2"
+	ovs_sbx "$1" python3 $ovs_base/ovs-dpctl.py del-flows "$2"
+        return 0
+}
+
 ovs_drop_record_and_run () {
 	local sbx=$1
 	shift
@@ -200,6 +206,17 @@ test_drop_reason() {
 	ip netns exec server ip addr add 172.31.110.20/24 dev s1
 	ip netns exec server ip link set s1 up
 
+	# Check if drop reasons can be sent
+	ovs_add_flow "test_drop_reason" dropreason \
+		'in_port(1),eth(),eth_type(0x0806),arp()' 'drop(10)' 2>/dev/null
+	if [ $? == 1 ]; then
+		info "no support for drop reasons - skipping"
+		ovs_exit_sig
+		return $ksft_skip
+	fi
+
+	ovs_del_flows "test_drop_reason" dropreason
+
 	# Allow ARP
 	ovs_add_flow "test_drop_reason" dropreason \
 		'in_port(1),eth(),eth_type(0x0806),arp()' '2' || return 1
diff --git a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
index 9686ca30d516d..153042c1e8c13 100644
--- a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
+++ b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
@@ -1906,6 +1906,32 @@ class OvsFlow(GenericNetlinkSocket):
             raise ne
         return reply
 
+    def del_flows(self, dpifindex):
+        """
+        Send a del message to the kernel that will drop all flows.
+
+        dpifindex should be a valid datapath obtained by calling
+        into the OvsDatapath lookup
+        """
+
+        flowmsg = OvsFlow.ovs_flow_msg()
+        flowmsg["cmd"] = OVS_FLOW_CMD_DEL
+        flowmsg["version"] = OVS_DATAPATH_VERSION
+        flowmsg["reserved"] = 0
+        flowmsg["dpifindex"] = dpifindex
+
+        try:
+            reply = self.nlm_request(
+                flowmsg,
+                msg_type=self.prid,
+                msg_flags=NLM_F_REQUEST | NLM_F_ACK,
+            )
+            reply = reply[0]
+        except NetlinkError as ne:
+            print(flowmsg)
+            raise ne
+        return reply
+
     def dump(self, dpifindex, flowspec=None):
         """
         Returns a list of messages containing flows.
@@ -2068,6 +2094,9 @@ def main(argv):
     addflcmd.add_argument("flow", help="Flow specification")
     addflcmd.add_argument("acts", help="Flow actions")
 
+    delfscmd = subparsers.add_parser("del-flows")
+    delfscmd.add_argument("flsbr", help="Datapath name")
+
     args = parser.parse_args()
 
     if args.verbose > 0:
@@ -2151,6 +2180,11 @@ def main(argv):
         flow = OvsFlow.ovs_flow_msg()
         flow.parse(args.flow, args.acts, rep["dpifindex"])
         ovsflow.add_flow(rep["dpifindex"], flow)
+    elif hasattr(args, "flsbr"):
+        rep = ovsdp.info(args.flsbr, 0)
+        if rep is None:
+            print("DP '%s' not found." % args.flsbr)
+        ovsflow.del_flows(rep["dpifindex"])
 
     return 0
 
-- 
2.40.1

[PATCH net 4/4] selftests: openvswitch: Fix the ct_tuple for v4

[Aaron Conole]

Caught during code review.

Fixes: e52b07aa1a54 ("selftests: openvswitch: add flow dump support")
Signed-off-by: Aaron Conole <aconole@redhat.com>
---
 tools/testing/selftests/net/openvswitch/ovs-dpctl.py | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
index 153042c1e8c13..ed7bef7ca6883 100644
--- a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
+++ b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
@@ -1119,12 +1119,14 @@ class ovskey(nla):
                 "src",
                 lambda x: str(ipaddress.IPv4Address(x)),
                 int,
+                convert_ipv4,
             ),
             (
                 "dst",
                 "dst",
-                lambda x: str(ipaddress.IPv6Address(x)),
+                lambda x: str(ipaddress.IPv4Address(x)),
                 int,
+                convert_ipv4,
             ),
             ("tp_src", "tp_src", "%d", int),
             ("tp_dst", "tp_dst", "%d", int),
-- 
2.40.1

Re: [PATCH net 1/4] selftests: openvswitch: Add version check for pyroute2

[Paolo Abeni]

On Fri, 2023-10-06 at 11:12 -0400, Aaron Conole wrote:
> Paolo Abeni reports that on some systems the pyroute2 version isn't
> new enough to run the test suite.  Ensure that we support a minimum
> version of 0.6 for all cases (which does include the existing ones).
> The 0.6.1 version was released in May of 2021, so should be
> propagated to most installations at this point.
> 
> The alternative that Paolo proposed was to only skip when the
> add-flow is being run.  This would be okay for most cases, except
> if a future test case is added that needs to do flow dump without
> an associated add (just guessing).  In that case, it could also be
> broken and we would need additional skip logic anyway.  Just draw
> a line in the sand now.
> 
> Fixes: 25f16c873fb1 ("selftests: add openvswitch selftest suite")
> Reported-by: Paolo Abeni <pabeni@redhat.com>
> Closes: https://lore.kernel.org/lkml/8470c431e0930d2ea204a9363a60937289b7fdbe.camel@redhat.com/
> Signed-off-by: Aaron Conole <aconole@redhat.com>
> ---
>  tools/testing/selftests/net/openvswitch/openvswitch.sh | 2 +-
>  tools/testing/selftests/net/openvswitch/ovs-dpctl.py   | 8 ++++++++
>  2 files changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/net/openvswitch/openvswitch.sh b/tools/testing/selftests/net/openvswitch/openvswitch.sh
> index 9c2012d70b08e..220c3356901ef 100755
> --- a/tools/testing/selftests/net/openvswitch/openvswitch.sh
> +++ b/tools/testing/selftests/net/openvswitch/openvswitch.sh
> @@ -525,7 +525,7 @@ run_test() {
>  	fi
>  
>  	if python3 ovs-dpctl.py -h 2>&1 | \
> -	     grep "Need to install the python" >/dev/null 2>&1; then
> +	     grep -E "Need to (install|upgrade) the python" >/dev/null 2>&1; then
>  		stdbuf -o0 printf "TEST: %-60s  [PYLIB]\n" "${tdesc}"
>  		return $ksft_skip
>  	fi
> diff --git a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
> index 912dc8c490858..9686ca30d516d 100644
> --- a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
> +++ b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
> @@ -28,6 +28,8 @@ try:
>      from pyroute2.netlink import nlmsg_atoms
>      from pyroute2.netlink.exceptions import NetlinkError
>      from pyroute2.netlink.generic import GenericNetlinkSocket
> +    import pyroute2
> +
>  except ModuleNotFoundError:
>      print("Need to install the python pyroute2 package.")
>      sys.exit(0)
> @@ -1998,6 +2000,12 @@ def main(argv):
>      nlmsg_atoms.ovskey = ovskey
>      nlmsg_atoms.ovsactions = ovsactions
>  
> +    # version check for pyroute2
> +    prverscheck = pyroute2.__version__.split(".")
> +    if int(prverscheck[0]) == 0 and int(prverscheck[1]) < 6:
> +        print("Need to upgrade the python pyroute2 package.")

I think it would be better to propagate/print also the minimum version
required, so that the user should not have to resort looking at the
self-test sources to learn the required minimum version.

Cheers,

Paolo

Re: [PATCH net 3/4] selftests: openvswitch: Skip drop testing on older kernels

[Paolo Abeni]

On Fri, 2023-10-06 at 11:12 -0400, Aaron Conole wrote:
> Kernels that don't have support for openvswitch drop reasons also
> won't have the drop counter reasons, so we should skip the test
> completely.  It previously wasn't possible to build a test case
> for this without polluting the datapath, so we introduce a mechanism
> to clear all the flows from a datapath allowing us to test for
> explicit drop actions, and then clear the flows to build the
> original test case.
> 
> Fixes: 4242029164d6 ("selftests: openvswitch: add explicit drop testcase")
> Signed-off-by: Aaron Conole <aconole@redhat.com>
> ---
>  .../selftests/net/openvswitch/openvswitch.sh  | 17 ++++++++++
>  .../selftests/net/openvswitch/ovs-dpctl.py    | 34 +++++++++++++++++++
>  2 files changed, 51 insertions(+)
> 
> diff --git a/tools/testing/selftests/net/openvswitch/openvswitch.sh b/tools/testing/selftests/net/openvswitch/openvswitch.sh
> index 2a0112be7ead5..ca7090e71bff2 100755
> --- a/tools/testing/selftests/net/openvswitch/openvswitch.sh
> +++ b/tools/testing/selftests/net/openvswitch/openvswitch.sh
> @@ -144,6 +144,12 @@ ovs_add_flow () {
>  	return 0
>  }
>  
> +ovs_del_flows () {
> +	info "Deleting all flows from DP: sbx:$1 br:$2"
> +	ovs_sbx "$1" python3 $ovs_base/ovs-dpctl.py del-flows "$2"
> +        return 0

The chunk above mixes whitespaces and tabs for indenting, please be
consistent.


Thanks!

Paolo

Re: [PATCH net 4/4] selftests: openvswitch: Fix the ct_tuple for v4

[Paolo Abeni]

On Fri, 2023-10-06 at 11:12 -0400, Aaron Conole wrote:
> Caught during code review.

Since there are a few other small things, please additionally expand
this changelog briefly describing the addressed problem and it's
consequences.

Thanks,

Paolo

Re: [PATCH net 1/4] selftests: openvswitch: Add version check for pyroute2

[Aaron Conole]

Paolo Abeni <pabeni@redhat.com> writes:

> On Fri, 2023-10-06 at 11:12 -0400, Aaron Conole wrote:
>> Paolo Abeni reports that on some systems the pyroute2 version isn't
>> new enough to run the test suite.  Ensure that we support a minimum
>> version of 0.6 for all cases (which does include the existing ones).
>> The 0.6.1 version was released in May of 2021, so should be
>> propagated to most installations at this point.
>> 
>> The alternative that Paolo proposed was to only skip when the
>> add-flow is being run.  This would be okay for most cases, except
>> if a future test case is added that needs to do flow dump without
>> an associated add (just guessing).  In that case, it could also be
>> broken and we would need additional skip logic anyway.  Just draw
>> a line in the sand now.
>> 
>> Fixes: 25f16c873fb1 ("selftests: add openvswitch selftest suite")
>> Reported-by: Paolo Abeni <pabeni@redhat.com>
>> Closes: https://lore.kernel.org/lkml/8470c431e0930d2ea204a9363a60937289b7fdbe.camel@redhat.com/
>> Signed-off-by: Aaron Conole <aconole@redhat.com>
>> ---
>>  tools/testing/selftests/net/openvswitch/openvswitch.sh | 2 +-
>>  tools/testing/selftests/net/openvswitch/ovs-dpctl.py   | 8 ++++++++
>>  2 files changed, 9 insertions(+), 1 deletion(-)
>> 
>> diff --git a/tools/testing/selftests/net/openvswitch/openvswitch.sh b/tools/testing/selftests/net/openvswitch/openvswitch.sh
>> index 9c2012d70b08e..220c3356901ef 100755
>> --- a/tools/testing/selftests/net/openvswitch/openvswitch.sh
>> +++ b/tools/testing/selftests/net/openvswitch/openvswitch.sh
>> @@ -525,7 +525,7 @@ run_test() {
>>  	fi
>>  
>>  	if python3 ovs-dpctl.py -h 2>&1 | \
>> -	     grep "Need to install the python" >/dev/null 2>&1; then
>> +	     grep -E "Need to (install|upgrade) the python" >/dev/null 2>&1; then
>>  		stdbuf -o0 printf "TEST: %-60s  [PYLIB]\n" "${tdesc}"
>>  		return $ksft_skip
>>  	fi
>> diff --git a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
>> index 912dc8c490858..9686ca30d516d 100644
>> --- a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
>> +++ b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
>> @@ -28,6 +28,8 @@ try:
>>      from pyroute2.netlink import nlmsg_atoms
>>      from pyroute2.netlink.exceptions import NetlinkError
>>      from pyroute2.netlink.generic import GenericNetlinkSocket
>> +    import pyroute2
>> +
>>  except ModuleNotFoundError:
>>      print("Need to install the python pyroute2 package.")
>>      sys.exit(0)
>> @@ -1998,6 +2000,12 @@ def main(argv):
>>      nlmsg_atoms.ovskey = ovskey
>>      nlmsg_atoms.ovsactions = ovsactions
>>  
>> +    # version check for pyroute2
>> +    prverscheck = pyroute2.__version__.split(".")
>> +    if int(prverscheck[0]) == 0 and int(prverscheck[1]) < 6:
>> +        print("Need to upgrade the python pyroute2 package.")
>
> I think it would be better to propagate/print also the minimum version
> required, so that the user should not have to resort looking at the
> self-test sources to learn the required minimum version.

ACK - makes sense to me.

> Cheers,
>
> Paolo

Re: [PATCH net 3/4] selftests: openvswitch: Skip drop testing on older kernels

[Aaron Conole]

Paolo Abeni <pabeni@redhat.com> writes:

> On Fri, 2023-10-06 at 11:12 -0400, Aaron Conole wrote:
>> Kernels that don't have support for openvswitch drop reasons also
>> won't have the drop counter reasons, so we should skip the test
>> completely.  It previously wasn't possible to build a test case
>> for this without polluting the datapath, so we introduce a mechanism
>> to clear all the flows from a datapath allowing us to test for
>> explicit drop actions, and then clear the flows to build the
>> original test case.
>> 
>> Fixes: 4242029164d6 ("selftests: openvswitch: add explicit drop testcase")
>> Signed-off-by: Aaron Conole <aconole@redhat.com>
>> ---
>>  .../selftests/net/openvswitch/openvswitch.sh  | 17 ++++++++++
>>  .../selftests/net/openvswitch/ovs-dpctl.py    | 34 +++++++++++++++++++
>>  2 files changed, 51 insertions(+)
>> 
>> diff --git a/tools/testing/selftests/net/openvswitch/openvswitch.sh b/tools/testing/selftests/net/openvswitch/openvswitch.sh
>> index 2a0112be7ead5..ca7090e71bff2 100755
>> --- a/tools/testing/selftests/net/openvswitch/openvswitch.sh
>> +++ b/tools/testing/selftests/net/openvswitch/openvswitch.sh
>> @@ -144,6 +144,12 @@ ovs_add_flow () {
>>  	return 0
>>  }
>>  
>> +ovs_del_flows () {
>> +	info "Deleting all flows from DP: sbx:$1 br:$2"
>> +	ovs_sbx "$1" python3 $ovs_base/ovs-dpctl.py del-flows "$2"
>> +        return 0
>
> The chunk above mixes whitespaces and tabs for indenting, please be
> consistent.

Thanks.  Will fix in v2

> Thanks!
>
> Paolo

Re: [PATCH net 4/4] selftests: openvswitch: Fix the ct_tuple for v4

[Aaron Conole]

Paolo Abeni <pabeni@redhat.com> writes:

> On Fri, 2023-10-06 at 11:12 -0400, Aaron Conole wrote:
>> Caught during code review.
>
> Since there are a few other small things, please additionally expand
> this changelog briefly describing the addressed problem and it's
> consequences.

ACK.  will fix in v2.  Thanks Paolo!

> Thanks,
>
> Paolo