Re: [ovs-dev] [PATCH v1 11/18] tests: wrap ovs-ofctl calls to test python parser

[Adrian Moreno]

On 12/23/21 15:01, Eelco Chaudron wrote:
Some small remarks.

On 22 Nov 2021, at 12:22, Adrian Moreno wrote:

Some ovs-ofctl commands are used to parse or dump openflow flows,
specially in ofp-actions.at

Use a wrapper around ovs-ofctl, called ovs-test-ofparse.py that, apart
from calling ovs-ofctl, also parses its output (or input, depending on
the command) to make sure the python flow parsing library can also parse
the flows.

Signed-off-by: Adrian Moreno <amore...@redhat.com>
---
  tests/automake.mk         |   4 +-
  tests/ofp-actions.at      |  46 ++++++++---------
  tests/ovs-test-ofparse.py | 103 ++++++++++++++++++++++++++++++++++++++
  3 files changed, 129 insertions(+), 24 deletions(-)
  create mode 100755 tests/ovs-test-ofparse.py

diff --git a/tests/automake.mk b/tests/automake.mk
index 43731d097..5dc805ef4 100644
--- a/tests/automake.mk
+++ b/tests/automake.mk
@@ -20,7 +20,9 @@ EXTRA_DIST += \
        tests/atlocal.in \
        $(srcdir)/package.m4 \
        $(srcdir)/tests/testsuite \
-       $(srcdir)/tests/testsuite.patch
+       $(srcdir)/tests/testsuite.patch \
+       $(srcdir)/tests/ovs-test-ofparse.py

Add in alphabetical order

And maybe also add it to CHECK_PYFILES to capture flake8 errors.


OK. Thanks.

+

  COMMON_MACROS_AT = \
        tests/ovsdb-macros.at \
diff --git a/tests/ofp-actions.at b/tests/ofp-actions.at
index ef4898abb..0b79742be 100644
--- a/tests/ofp-actions.at
+++ b/tests/ofp-actions.at
@@ -327,7 +327,7 @@ AT_CAPTURE_FILE([input.txt])
  AT_CAPTURE_FILE([expout])
  AT_CAPTURE_FILE([experr])
  AT_CHECK(
-  [ovs-ofctl '-vPATTERN:console:%c|%p|%m' parse-actions OpenFlow10 < 
input.txt],
+  [ovs-test-ofparse.py '-vPATTERN:console:%c|%p|%m' parse-actions OpenFlow10 < 
input.txt],
    [0], [expout], [experr])
  AT_CLEANUP

@@ -500,7 +500,7 @@ AT_CAPTURE_FILE([input.txt])
  AT_CAPTURE_FILE([expout])
  AT_CAPTURE_FILE([experr])
  AT_CHECK(
-  [ovs-ofctl '-vPATTERN:console:%c|%p|%m' parse-actions OpenFlow11 < 
input.txt],
+  [ovs-test-ofparse.py '-vPATTERN:console:%c|%p|%m' parse-actions OpenFlow11 < 
input.txt],
    [0], [expout], [experr])
  AT_CLEANUP

@@ -735,7 +735,7 @@ AT_CAPTURE_FILE([input.txt])
  AT_CAPTURE_FILE([expout])
  AT_CAPTURE_FILE([experr])
  AT_CHECK(
-  [ovs-ofctl '-vPATTERN:console:%c|%p|%m' parse-actions OpenFlow12 < 
input.txt],
+  [ovs-test-ofparse.py '-vPATTERN:console:%c|%p|%m' parse-actions OpenFlow12 < 
input.txt],
    [0], [expout], [experr])
  AT_CLEANUP

@@ -788,7 +788,7 @@ AT_CAPTURE_FILE([input.txt])
  AT_CAPTURE_FILE([expout])
  AT_CAPTURE_FILE([experr])
  AT_CHECK(
-  [ovs-ofctl '-vPATTERN:console:%c|%p|%m' parse-actions OpenFlow13 < 
input.txt],
+  [ovs-test-ofparse.py '-vPATTERN:console:%c|%p|%m' parse-actions OpenFlow13 < 
input.txt],
    [0], [expout], [experr])
  AT_CLEANUP

@@ -817,16 +817,16 @@ AT_CAPTURE_FILE([input.txt])
  AT_CAPTURE_FILE([expout])
  AT_CAPTURE_FILE([experr])
  AT_CHECK(
-  [ovs-ofctl '-vPATTERN:console:%c|%p|%m' parse-actions OpenFlow15 < 
input.txt],
+  [ovs-test-ofparse.py '-vPATTERN:console:%c|%p|%m' parse-actions OpenFlow15 < 
input.txt],
    [0], [expout], [experr])
  AT_CLEANUP

  AT_SETUP([ofp-actions - inconsistent MPLS actions])
  OVS_VSWITCHD_START
  dnl OK: Use fin_timeout action on TCP flow
-AT_CHECK([ovs-ofctl -O OpenFlow11 -vwarn add-flow br0 'tcp 
actions=fin_timeout(idle_timeout=1)'])
+AT_CHECK([ovs-test-ofparse.py -O OpenFlow11 -vwarn add-flow br0 'tcp 
actions=fin_timeout(idle_timeout=1)'])
  dnl Bad: Use fin_timeout action on TCP flow that has been converted to MPLS
-AT_CHECK([ovs-ofctl -O OpenFlow11 -vwarn add-flow br0 'tcp 
actions=push_mpls:0x8847,fin_timeout(idle_timeout=1)'],
+AT_CHECK([ovs-test-ofparse.py -O OpenFlow11 -vwarn add-flow br0 'tcp 
actions=push_mpls:0x8847,fin_timeout(idle_timeout=1)'],
           [1], [], [dnl
  ovs-ofctl: none of the usable flow formats (OpenFlow10,NXM) is among the 
allowed flow formats (OpenFlow11)
  ])
@@ -840,8 +840,8 @@ dnl In OpenFlow 1.3, set_field always sets all the bits in 
the field,
  dnl but when we translate to NXAST_LOAD we need to only set the bits that
  dnl actually exist (e.g. mpls_label only has 20 bits) otherwise OVS rejects
  dnl the "load" action as invalid.  Check that we do this correctly.
-AT_CHECK([ovs-ofctl -O OpenFlow13 add-flow br0 
mpls,actions=set_field:10-\>mpls_label])
-AT_CHECK([ovs-ofctl -O OpenFlow10 dump-flows br0 | ofctl_strip], [0], [dnl
+AT_CHECK([ovs-test-ofparse.py -O OpenFlow13 add-flow br0 
mpls,actions=set_field:10-\>mpls_label])
+AT_CHECK([ovs-test-ofparse.py -O OpenFlow10 dump-flows br0 | ofctl_strip], 
[0], [dnl
  NXST_FLOW reply:
   mpls actions=load:0xa->OXM_OF_MPLS_LABEL[[]]
  ])
@@ -853,12 +853,12 @@ AT_KEYWORDS([ofp-actions])
  OVS_VSWITCHD_START
  dnl OpenFlow 1.0 has an "enqueue" action.  For OpenFlow 1.1+, we translate
  dnl it to a series of actions that accomplish the same thing.
-AT_CHECK([ovs-ofctl -O OpenFlow10 add-flow br0 'actions=enqueue(123,456)'])
-AT_CHECK([ovs-ofctl -O OpenFlow10 dump-flows br0 | ofctl_strip], [0], [dnl
+AT_CHECK([ovs-test-ofparse.py -O OpenFlow10 add-flow br0 
'actions=enqueue(123,456)'])
+AT_CHECK([ovs-test-ofparse.py -O OpenFlow10 dump-flows br0 | ofctl_strip], 
[0], [dnl
  NXST_FLOW reply:
   actions=enqueue:123:456
  ])
-AT_CHECK([ovs-ofctl -O OpenFlow13 dump-flows br0 | ofctl_strip], [0], [dnl
+AT_CHECK([ovs-test-ofparse.py -O OpenFlow13 dump-flows br0 | ofctl_strip], 
[0], [dnl
  OFPST_FLOW reply (OF1.3):
   reset_counts actions=set_queue:456,output:123,pop_queue
  ])
@@ -870,12 +870,12 @@ AT_KEYWORDS([ofp-actions])
  OVS_VSWITCHD_START
  dnl OpenFlow 1.1+ have a mod_nw_ttl action.  For OpenFlow 1.0, we translate
  dnl it to an Open vSwitch extension.
-AT_CHECK([ovs-ofctl -O OpenFlow11 add-flow br0 'ip,actions=mod_nw_ttl:123'])
-AT_CHECK([ovs-ofctl -O OpenFlow10 dump-flows br0 | ofctl_strip], [0], [dnl
+AT_CHECK([ovs-test-ofparse.py -O OpenFlow11 add-flow br0 
'ip,actions=mod_nw_ttl:123'])
+AT_CHECK([ovs-test-ofparse.py -O OpenFlow10 dump-flows br0 | ofctl_strip], 
[0], [dnl
  NXST_FLOW reply:
   ip actions=load:0x7b->NXM_NX_IP_TTL[[]]
  ])
-AT_CHECK([ovs-ofctl -O OpenFlow11 dump-flows br0 | ofctl_strip], [0], [dnl
+AT_CHECK([ovs-test-ofparse.py -O OpenFlow11 dump-flows br0 | ofctl_strip], 
[0], [dnl
  OFPST_FLOW reply (OF1.1):
   ip actions=mod_nw_ttl:123
  ])
@@ -889,16 +889,16 @@ OVS_VSWITCHD_START
  dnl OpenFlow 1.1, but no other version, has a "mod_nw_ecn" action.
  dnl Check that we translate it properly for OF1.0 and OF1.2.
  dnl (OF1.3+ should be the same as OF1.2.)
-AT_CHECK([ovs-ofctl -O OpenFlow11 add-flow br0 'ip,actions=mod_nw_ecn:2'])
-AT_CHECK([ovs-ofctl -O OpenFlow10 dump-flows br0 | ofctl_strip], [0], [dnl
+AT_CHECK([ovs-test-ofparse.py -O OpenFlow11 add-flow br0 
'ip,actions=mod_nw_ecn:2'])
+AT_CHECK([ovs-test-ofparse.py -O OpenFlow10 dump-flows br0 | ofctl_strip], 
[0], [dnl
  NXST_FLOW reply:
   ip actions=load:0x2->NXM_NX_IP_ECN[[]]
  ])
-AT_CHECK([ovs-ofctl -O OpenFlow11 dump-flows br0 | ofctl_strip], [0], [dnl
+AT_CHECK([ovs-test-ofparse.py -O OpenFlow11 dump-flows br0 | ofctl_strip], 
[0], [dnl
  OFPST_FLOW reply (OF1.1):
   ip actions=mod_nw_ecn:2
  ])
-AT_CHECK([ovs-ofctl -O OpenFlow12 dump-flows br0 | ofctl_strip], [0], [dnl
+AT_CHECK([ovs-test-ofparse.py -O OpenFlow12 dump-flows br0 | ofctl_strip], 
[0], [dnl
  OFPST_FLOW reply (OF1.2):
   ip actions=set_field:2->nw_ecn
  ])
@@ -911,8 +911,8 @@ dnl that anything that comes in as reg_load gets translated 
back to reg_load
  dnl on output.  Perhaps this is somewhat inconsistent but it's what OVS
  dnl has done for multiple versions.
  AT_CHECK([ovs-ofctl del-flows br0])
-AT_CHECK([ovs-ofctl -O OpenFlow12 add-flow br0 
'ip,actions=set_field:2->ip_ecn'])
-AT_CHECK([ovs-ofctl -O OpenFlow11 dump-flows br0 | ofctl_strip], [0], [dnl
+AT_CHECK([ovs-test-ofparse.py -O OpenFlow12 add-flow br0 
'ip,actions=set_field:2->ip_ecn'])
+AT_CHECK([ovs-test-ofparse.py -O OpenFlow11 dump-flows br0 | ofctl_strip], 
[0], [dnl
  OFPST_FLOW reply (OF1.1):
   ip actions=mod_nw_ecn:2
  ])
@@ -920,9 +920,9 @@ OFPST_FLOW reply (OF1.1):
  dnl Check that OF1.2+ set_field to set ECN is translated for earlier OF
  dnl versions.
  AT_CHECK([ovs-ofctl del-flows br0])
-AT_CHECK([ovs-ofctl -O OpenFlow10 add-flow br0 
'ip,actions=set_field:2->ip_ecn'])
+AT_CHECK([ovs-test-ofparse.py -O OpenFlow10 add-flow br0 
'ip,actions=set_field:2->ip_ecn'])
  AT_CHECK([ovs-ofctl del-flows br0])
-AT_CHECK([ovs-ofctl -O OpenFlow11 add-flow br0 
'ip,actions=set_field:2->ip_ecn'])
+AT_CHECK([ovs-test-ofparse.py -O OpenFlow11 add-flow br0 
'ip,actions=set_field:2->ip_ecn'])

  OVS_VSWITCHD_STOP
  AT_CLEANUP
diff --git a/tests/ovs-test-ofparse.py b/tests/ovs-test-ofparse.py
new file mode 100755
index 000000000..89a0f3cf5
--- /dev/null
+++ b/tests/ovs-test-ofparse.py
@@ -0,0 +1,103 @@
+#!/usr/bin/env python3
+# Copyright (c) 2021 Red Hat, Inc.
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at:
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+# Breaks lines read from stdin into groups using blank lines as
+# group separators, then sorts lines within the groups for
+# reproducibility.

Is this above text a leftover from the header cut/paste?


Right, I'll removed it in the next version.

+
+
+# ovs-test-ofparse is just a wrapper around ovs-ofctl
+# that also runs the python flow parsing utility to check that flows are
+# parseable

Add dot.


Man I really have to stop writing python, it's lack of line termination 
character is creeping up on me ;).

+
+import subprocess
+import sys
+import re
+
+from ovs.flows.ofp import OFPFlowFactory
+
+diff_regexp = re.compile(r"\d{2}: (\d{2}|\(none\)) -> (\d{2}|\(none\))$")
+
+
+def run_ofctl(with_stdin):
+    cmd = sys.argv
+    cmd[0] = "ovs-ofctl"
+    if with_stdin:
+        p = subprocess.Popen(
+            cmd, stdin=subprocess.PIPE, stdout=subprocess.PIPE, 
stderr=subprocess.PIPE

Truncate to <=79 chars

+        )
+        input_data = sys.stdin.read()
+        out, err = p.communicate(input_data.encode("utf-8"))
+
+        print(out.decode("utf-8"), file=sys.stdout, end="")
+        print(err.decode("utf-8"), file=sys.stderr, end="")
+        return p.returncode, out, err
+    else:
+        p = subprocess.Popen(cmd, stdout=subprocess.PIPE, 
stderr=subprocess.PIPE)

Truncate to <=79 chars

+        out, err = p.communicate()
+
+        print(out.decode("utf-8"), file=sys.stdout, end="")
+        print(err.decode("utf-8"), file=sys.stderr, end="")
+        return p.returncode, out, err
+
+
+def main():
+    flows = list()
+    return_code = 0
+
+    if "parse-actions" in sys.argv:
+        return_code, out, err = run_ofctl(True)
+
+        out_lines = out.decode("utf-8").split("\n")
+        for line in out_lines:
+            if not (
+                "bad" in line  # skip "bad action at..."
+                or line.strip() == ""  # skip empty lines
+                or diff_regexp.match(line)  # skip differences
+            ):
+                flows.append(line)
+
+    elif "add-flow" in sys.argv:
+        return_code, out, err = run_ofctl(False)
+        flows.append(sys.argv[-1])
+
+    elif "dump-flows" in sys.argv:
+        return_code, out, err = run_ofctl(False)
+        out_lines = out.decode("utf-8").split("\n")
+
+        for line in out_lines:
+            if not (
+                "reply" in line  # skip NXST_FLOW reply:
+                or line.strip() == ""  # skip empty lines
+            ):
+                flows.append(line)
+    else:
+        print("Unsupported command: {}".format(sys.argv))
+        sys.exit(1)
+
+    if return_code == 0:
+        factory = OFPFlowFactory()
+        for flow in flows:
+            try:
+                _ = factory.from_string(flow)
+            except Exception as e:
+                print(e)
+                return 1
+
+    return return_code
+
+
+if __name__ == "__main__":
+    sys.exit(main())
--
2.31.1


--
Adrián Moreno

_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev