| Message ID | 20230307150030.527726-1-po-hsu.lin@canonical.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [PATCHv2] selftests: net: devlink_port_split.py: skip test if no suitable device available | expand |
On Tue, 7 Mar 2023 23:00:30 +0800 Po-Hsu Lin wrote: > The `devlink -j port show` command output may not contain the "flavour" > key, an example from s390x LPAR with Ubuntu 22.10 (5.19.0-37-generic), > iproute2-5.15.0: > {"port":{"pci/0001:00:00.0/1":{"type":"eth","netdev":"ens301"}, > "pci/0001:00:00.0/2":{"type":"eth","netdev":"ens301d1"}, > "pci/0002:00:00.0/1":{"type":"eth","netdev":"ens317"}, > "pci/0002:00:00.0/2":{"type":"eth","netdev":"ens317d1"}}} > > This will cause a KeyError exception. I looked closer and I don't understand why the key is not there. Both 5.19 kernel should always put this argument out, and 5.15 iproute2 should always interpret it. Am I looking wrong? Do you see how we can get a dump with no flavor? I worry that this is some endianness problem, and we just misreport stuff on big-endian. > Create a validate_devlink_output() to check for this "flavour" from > devlink command output to avoid this KeyError exception. Also let > it handle the check for `devlink -j dev show` output in main(). > > Apart from this, if the test was not started because of any reason > (e.g. "lanes" does not exist, max lanes is 0 or the flavour of the > designated device is not "physical" and etc.) The script will still > return 0 and thus causing a false-negative test result. > > Use a test_ran flag to determine if these tests were skipped and > return KSFT_SKIP to make it more clear. > > V2: factor out the skip logic from main(), update commit message and > skip reasons accordingly. > Link: https://bugs.launchpad.net/bugs/1937133 > Fixes: f3348a82e727 ("selftests: net: Add port split test") > Signed-off-by: Po-Hsu Lin <po-hsu.lin@canonical.com> > --- > tools/testing/selftests/net/devlink_port_split.py | 36 +++++++++++++++++++---- > 1 file changed, 31 insertions(+), 5 deletions(-) > > diff --git a/tools/testing/selftests/net/devlink_port_split.py b/tools/testing/selftests/net/devlink_port_split.py > index 2b5d6ff..749606c 100755 > --- a/tools/testing/selftests/net/devlink_port_split.py > +++ b/tools/testing/selftests/net/devlink_port_split.py > @@ -59,6 +59,8 @@ class devlink_ports(object): > assert stderr == "" > ports = json.loads(stdout)['port'] > > + validate_devlink_output(ports, 'flavour') If it's just a matter of kernel/iproute2 version we shouldn't need to check here again? > for port in ports: > if dev in port: > if ports[port]['flavour'] == 'physical': > @@ -220,6 +222,27 @@ def split_splittable_port(port, k, lanes, dev): > unsplit(port.bus_info) > > > +def validate_devlink_output(devlink_data, target_property=None): > + """ > + Determine if test should be skipped by checking: > + 1. devlink_data contains values > + 2. The target_property exist in devlink_data > + """ > + skip_reason = None > + if any(devlink_data.values()): > + if target_property: > + skip_reason = "{} not found in devlink output, test skipped".format(target_property) > + for key in devlink_data: > + if target_property in devlink_data[key]: > + skip_reason = None > + else: > + skip_reason = 'devlink output is empty, test skipped' > + > + if skip_reason: > + print(skip_reason) > + sys.exit(KSFT_SKIP) Looks good, so.. > def make_parser(): > parser = argparse.ArgumentParser(description='A test for port splitting.') > parser.add_argument('--dev', > @@ -231,6 +254,7 @@ def make_parser(): > > > def main(cmdline=None): > + test_ran = False I don't think we need the test_ran tracking any more? > parser = make_parser() > args = parser.parse_args(cmdline) > > @@ -240,12 +264,9 @@ def main(cmdline=None): > stdout, stderr = run_command(cmd) > assert stderr == "" > > + validate_devlink_output(json.loads(stdout)) > devs = json.loads(stdout)['dev'] > - if devs: > - dev = list(devs.keys())[0] > - else: > - print("no devlink device was found, test skipped") > - sys.exit(KSFT_SKIP) > + dev = list(devs.keys())[0] > > cmd = "devlink dev show %s" % dev > stdout, stderr = run_command(cmd) > @@ -277,6 +298,11 @@ def main(cmdline=None): > split_splittable_port(port, lane, max_lanes, dev) > > lane //= 2 > + test_ran = True > + > + if not test_ran: > + print("Test not started, no suitable device for the test") > + sys.exit(KSFT_SKIP) > > > if __name__ == "__main__":
Wed, Mar 08, 2023 at 03:37:41PM CET, po-hsu.lin@canonical.com wrote: >On Wed, Mar 8, 2023 at 7:41 PM Jiri Pirko <jiri@resnulli.us> wrote: >> >> Wed, Mar 08, 2023 at 11:21:57AM CET, po-hsu.lin@canonical.com wrote: >> >On Wed, Mar 8, 2023 at 5:31 PM Jiri Pirko <jiri@resnulli.us> wrote: >> >> >> >> Tue, Mar 07, 2023 at 04:00:30PM CET, po-hsu.lin@canonical.com wrote: >> >> >The `devlink -j port show` command output may not contain the "flavour" >> >> >key, an example from s390x LPAR with Ubuntu 22.10 (5.19.0-37-generic), >> >> >iproute2-5.15.0: >> >> > {"port":{"pci/0001:00:00.0/1":{"type":"eth","netdev":"ens301"}, >> >> > "pci/0001:00:00.0/2":{"type":"eth","netdev":"ens301d1"}, >> >> > "pci/0002:00:00.0/1":{"type":"eth","netdev":"ens317"}, >> >> > "pci/0002:00:00.0/2":{"type":"eth","netdev":"ens317d1"}}} >> >> >> >> As Jakub wrote, this is odd. Could you debug if kernel sends the flavour >> >> attr and if not why? Also, could you try with most recent kernel? >> > >> >I did a quick check on another s390x LPAR instance which is running >> >with Ubuntu 23.04 (6.1.0-16-generic) iproute2-6.1.0, there is still no >> >"flavour" attribute. >> >$ devlink port show >> >pci/0001:00:00.0/1: type eth netdev ens301 >> >pci/0001:00:00.0/2: type eth netdev ens301d1 >> >pci/0002:00:00.0/1: type eth netdev ens317 >> >pci/0002:00:00.0/2: type eth netdev ens317d1 >> > >> >The behaviour didn't change with iproute2 built from source [1] >> >> Could you paste output of "devlink dev info"? >> Looks like something might be wrong in the kernel devlink/driver code. >> >The `devlink dev info` output is empty. The following output is from >that Ubuntu 23.04 s390x LPAR, run as root: ># devlink dev show >pci/0001:00:00.0 >pci/0002:00:00.0 ># devlink dev show pci/0001:00:00.0 >pci/0001:00:00.0 ># devlink dev info ># devlink dev info pci/0001:00:00.0 Interesting, could you try ethtool -i to get the driver name? >kernel answers: Operation not supported > >>
Thu, Mar 09, 2023 at 04:44:10PM CET, po-hsu.lin@canonical.com wrote: >On Thu, Mar 9, 2023 at 2:24 AM Jiri Pirko <jiri@resnulli.us> wrote: >> >> Wed, Mar 08, 2023 at 03:37:41PM CET, po-hsu.lin@canonical.com wrote: >> >On Wed, Mar 8, 2023 at 7:41 PM Jiri Pirko <jiri@resnulli.us> wrote: >> >> >> >> Wed, Mar 08, 2023 at 11:21:57AM CET, po-hsu.lin@canonical.com wrote: >> >> >On Wed, Mar 8, 2023 at 5:31 PM Jiri Pirko <jiri@resnulli.us> wrote: >> >> >> >> >> >> Tue, Mar 07, 2023 at 04:00:30PM CET, po-hsu.lin@canonical.com wrote: >> >> >> >The `devlink -j port show` command output may not contain the "flavour" >> >> >> >key, an example from s390x LPAR with Ubuntu 22.10 (5.19.0-37-generic), >> >> >> >iproute2-5.15.0: >> >> >> > {"port":{"pci/0001:00:00.0/1":{"type":"eth","netdev":"ens301"}, >> >> >> > "pci/0001:00:00.0/2":{"type":"eth","netdev":"ens301d1"}, >> >> >> > "pci/0002:00:00.0/1":{"type":"eth","netdev":"ens317"}, >> >> >> > "pci/0002:00:00.0/2":{"type":"eth","netdev":"ens317d1"}}} >> >> >> >> >> >> As Jakub wrote, this is odd. Could you debug if kernel sends the flavour >> >> >> attr and if not why? Also, could you try with most recent kernel? >> >> > >> >> >I did a quick check on another s390x LPAR instance which is running >> >> >with Ubuntu 23.04 (6.1.0-16-generic) iproute2-6.1.0, there is still no >> >> >"flavour" attribute. >> >> >$ devlink port show >> >> >pci/0001:00:00.0/1: type eth netdev ens301 >> >> >pci/0001:00:00.0/2: type eth netdev ens301d1 >> >> >pci/0002:00:00.0/1: type eth netdev ens317 >> >> >pci/0002:00:00.0/2: type eth netdev ens317d1 >> >> > >> >> >The behaviour didn't change with iproute2 built from source [1] >> >> >> >> Could you paste output of "devlink dev info"? >> >> Looks like something might be wrong in the kernel devlink/driver code. >> >> >> >The `devlink dev info` output is empty. The following output is from >> >that Ubuntu 23.04 s390x LPAR, run as root: >> ># devlink dev show >> >pci/0001:00:00.0 >> >pci/0002:00:00.0 >> ># devlink dev show pci/0001:00:00.0 >> >pci/0001:00:00.0 >> ># devlink dev info >> ># devlink dev info pci/0001:00:00.0 >> >> Interesting, could you try ethtool -i to get the driver name? >> >Hi, > >Here you go: >$ ethtool -i ens301 >driver: mlx4_en >version: 4.0-0 >firmware-version: 2.35.5100 >expansion-rom-version: >bus-info: 0001:00:00.0 >supports-statistics: yes >supports-test: yes >supports-eeprom-access: no >supports-register-dump: no >supports-priv-flags: yes > >$ ethtool -i ens317 >driver: mlx4_en mlx4 is indeed not setting attrs. So you patch is needed. >version: 4.0-0 >firmware-version: 2.35.5100 >expansion-rom-version: >bus-info: 0002:00:00.0 >supports-statistics: yes >supports-test: yes >supports-eeprom-access: no >supports-register-dump: no >supports-priv-flags: yes > >HTH > > >> >> >kernel answers: Operation not supported >> > >> >>
On Tue, 7 Mar 2023 23:00:30 +0800 Po-Hsu Lin wrote: > def main(cmdline=None): > + test_ran = False Could you move this variable init right before the for port in ports.if_names: line, and call it something like found_max_lanes ? > parser = make_parser() > args = parser.parse_args(cmdline) > > @@ -240,12 +264,9 @@ def main(cmdline=None): > stdout, stderr = run_command(cmd) > assert stderr == "" > > + validate_devlink_output(json.loads(stdout)) > devs = json.loads(stdout)['dev'] > - if devs: > - dev = list(devs.keys())[0] > - else: > - print("no devlink device was found, test skipped") > - sys.exit(KSFT_SKIP) > + dev = list(devs.keys())[0] > > cmd = "devlink dev show %s" % dev > stdout, stderr = run_command(cmd) > @@ -277,6 +298,11 @@ def main(cmdline=None): > split_splittable_port(port, lane, max_lanes, dev) > > lane //= 2 > + test_ran = True > + > + if not test_ran: > + print("Test not started, no suitable device for the test") Then change the message to f"Test not started, no port of device {dev} reports max_lanes" > + sys.exit(KSFT_SKIP) >
On Sat, Mar 11, 2023 at 8:05 AM Jakub Kicinski <kuba@kernel.org> wrote: > > On Tue, 7 Mar 2023 23:00:30 +0800 Po-Hsu Lin wrote: > > def main(cmdline=None): > > + test_ran = False > > Could you move this variable init right before the > > for port in ports.if_names: > > line, and call it something like found_max_lanes ? > > > parser = make_parser() > > args = parser.parse_args(cmdline) > > > > @@ -240,12 +264,9 @@ def main(cmdline=None): > > stdout, stderr = run_command(cmd) > > assert stderr == "" > > > > + validate_devlink_output(json.loads(stdout)) > > devs = json.loads(stdout)['dev'] > > - if devs: > > - dev = list(devs.keys())[0] > > - else: > > - print("no devlink device was found, test skipped") > > - sys.exit(KSFT_SKIP) > > + dev = list(devs.keys())[0] > > > > cmd = "devlink dev show %s" % dev > > stdout, stderr = run_command(cmd) > > @@ -277,6 +298,11 @@ def main(cmdline=None): > > split_splittable_port(port, lane, max_lanes, dev) > > > > lane //= 2 > > + test_ran = True > > + > > + if not test_ran: > > + print("Test not started, no suitable device for the test") > > Then change the message to > > f"Test not started, no port of device {dev} reports max_lanes" > Sure, will update this in V3 Thanks for the feedback! > > + sys.exit(KSFT_SKIP) > >
diff --git a/tools/testing/selftests/net/devlink_port_split.py b/tools/testing/selftests/net/devlink_port_split.py index 2b5d6ff..749606c 100755 --- a/tools/testing/selftests/net/devlink_port_split.py +++ b/tools/testing/selftests/net/devlink_port_split.py @@ -59,6 +59,8 @@ class devlink_ports(object): assert stderr == "" ports = json.loads(stdout)['port'] + validate_devlink_output(ports, 'flavour') + for port in ports: if dev in port: if ports[port]['flavour'] == 'physical': @@ -220,6 +222,27 @@ def split_splittable_port(port, k, lanes, dev): unsplit(port.bus_info) +def validate_devlink_output(devlink_data, target_property=None): + """ + Determine if test should be skipped by checking: + 1. devlink_data contains values + 2. The target_property exist in devlink_data + """ + skip_reason = None + if any(devlink_data.values()): + if target_property: + skip_reason = "{} not found in devlink output, test skipped".format(target_property) + for key in devlink_data: + if target_property in devlink_data[key]: + skip_reason = None + else: + skip_reason = 'devlink output is empty, test skipped' + + if skip_reason: + print(skip_reason) + sys.exit(KSFT_SKIP) + + def make_parser(): parser = argparse.ArgumentParser(description='A test for port splitting.') parser.add_argument('--dev', @@ -231,6 +254,7 @@ def make_parser(): def main(cmdline=None): + test_ran = False parser = make_parser() args = parser.parse_args(cmdline) @@ -240,12 +264,9 @@ def main(cmdline=None): stdout, stderr = run_command(cmd) assert stderr == "" + validate_devlink_output(json.loads(stdout)) devs = json.loads(stdout)['dev'] - if devs: - dev = list(devs.keys())[0] - else: - print("no devlink device was found, test skipped") - sys.exit(KSFT_SKIP) + dev = list(devs.keys())[0] cmd = "devlink dev show %s" % dev stdout, stderr = run_command(cmd) @@ -277,6 +298,11 @@ def main(cmdline=None): split_splittable_port(port, lane, max_lanes, dev) lane //= 2 + test_ran = True + + if not test_ran: + print("Test not started, no suitable device for the test") + sys.exit(KSFT_SKIP) if __name__ == "__main__":
The `devlink -j port show` command output may not contain the "flavour" key, an example from s390x LPAR with Ubuntu 22.10 (5.19.0-37-generic), iproute2-5.15.0: {"port":{"pci/0001:00:00.0/1":{"type":"eth","netdev":"ens301"}, "pci/0001:00:00.0/2":{"type":"eth","netdev":"ens301d1"}, "pci/0002:00:00.0/1":{"type":"eth","netdev":"ens317"}, "pci/0002:00:00.0/2":{"type":"eth","netdev":"ens317d1"}}} This will cause a KeyError exception. Create a validate_devlink_output() to check for this "flavour" from devlink command output to avoid this KeyError exception. Also let it handle the check for `devlink -j dev show` output in main(). Apart from this, if the test was not started because of any reason (e.g. "lanes" does not exist, max lanes is 0 or the flavour of the designated device is not "physical" and etc.) The script will still return 0 and thus causing a false-negative test result. Use a test_ran flag to determine if these tests were skipped and return KSFT_SKIP to make it more clear. V2: factor out the skip logic from main(), update commit message and skip reasons accordingly. Link: https://bugs.launchpad.net/bugs/1937133 Fixes: f3348a82e727 ("selftests: net: Add port split test") Signed-off-by: Po-Hsu Lin <po-hsu.lin@canonical.com> --- tools/testing/selftests/net/devlink_port_split.py | 36 +++++++++++++++++++---- 1 file changed, 31 insertions(+), 5 deletions(-)