Date:   Tue, 7 Mar 2023 17:02:19 -0800
From:   Jakub Kicinski <kuba@kernel.org>
To:     Po-Hsu Lin <po-hsu.lin@canonical.com>
Cc:     linux-kernel@vger.kernel.org, linux-kselftest@vger.kernel.org,
        netdev@vger.kernel.org, idosch@mellanox.com,
        danieller@mellanox.com, petrm@mellanox.com, shuah@kernel.org,
        pabeni@redhat.com, edumazet@google.com, davem@davemloft.net
Subject: Re: [PATCHv2] selftests: net: devlink_port_split.py: skip test if
 no suitable device available
Message-ID: <20230307170219.4699af9b@kernel.org>
In-Reply-To: <20230307150030.527726-1-po-hsu.lin@canonical.com>
References: <20230307150030.527726-1-po-hsu.lin@canonical.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

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__":