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 <[email protected]> > --- > 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. > + > > 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? > + > + > +# 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. > + > +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 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
