Date: Sun, 14 Apr 2024 13:04:46 -0400
From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
To: Jakub Kicinski <kuba@...nel.org>, 
 davem@...emloft.net
Cc: netdev@...r.kernel.org, 
 edumazet@...gle.com, 
 pabeni@...hat.com, 
 shuah@...nel.org, 
 petrm@...dia.com, 
 linux-kselftest@...r.kernel.org, 
 willemb@...gle.com, 
 Jakub Kicinski <kuba@...nel.org>
Subject: Re: [PATCH net-next 1/5] selftests: drv-net: define endpoint
 structures

Jakub Kicinski wrote:
> Define the endpoint "model". To execute most meaningful device
> driver tests we need to be able to communicate with a remote system,
> and have it send traffic to the device under test.
> 
> Various test environments will have different requirements.
> 
> 0) "Local" netdevsim-based testing can simply use net namespaces.
> netdevsim supports connecting two devices now, to form a veth-like
> construct.
> 
> 1) Similarly on hosts with multiple NICs, the NICs may be connected
> together with a loopback cable or internal device loopback.
> One interface may be placed into separate netns, and tests
> would proceed much like in the netdevsim case. Note that
> the loopback config or the moving of one interface
> into a netns is not expected to be part of selftest code.
> 
> 2) Some systems may need to communicate with the endpoint via SSH.
> 
> 3) Last but not least environment may have its own custom communication
> method.
> 
> Fundamentally we only need two operations:
>  - run a command remotely
>  - deploy a binary (if some tool we need is built as part of kselftests)
> 
> Wrap these two in a class. Use dynamic loading to load the Endpoint
> class. This will allow very easy definition of other communication
> methods without bothering upstream code base.
> 
> Stick to the "simple" / "no unnecessary abstractions" model for
> referring to the endpoints. The host / endpoint object are passed
> as an argument to the usual cmd() or ip() invocation. For example:
> 
>  ip("link show", json=True, host=endpoint)
> 
> Signed-off-by: Jakub Kicinski <kuba@...nel.org>
> ---
>  .../selftests/drivers/net/lib/py/__init__.py  |  1 +
>  .../selftests/drivers/net/lib/py/endpoint.py  | 13 +++++++
>  .../selftests/drivers/net/lib/py/ep_netns.py  | 15 ++++++++
>  .../selftests/drivers/net/lib/py/ep_ssh.py    | 34 +++++++++++++++++++
>  tools/testing/selftests/net/lib/py/utils.py   | 19 ++++++-----
>  5 files changed, 73 insertions(+), 9 deletions(-)
>  create mode 100644 tools/testing/selftests/drivers/net/lib/py/endpoint.py
>  create mode 100644 tools/testing/selftests/drivers/net/lib/py/ep_netns.py
>  create mode 100644 tools/testing/selftests/drivers/net/lib/py/ep_ssh.py
> 
> diff --git a/tools/testing/selftests/drivers/net/lib/py/__init__.py b/tools/testing/selftests/drivers/net/lib/py/__init__.py
> index 4653dffcd962..0d71ec83135b 100644
> --- a/tools/testing/selftests/drivers/net/lib/py/__init__.py
> +++ b/tools/testing/selftests/drivers/net/lib/py/__init__.py
> @@ -15,3 +15,4 @@ KSFT_DIR = (Path(__file__).parent / "../../../..").resolve()
>      sys.exit(4)
>  
>  from .env import *
> +from .endpoint import Endpoint
> diff --git a/tools/testing/selftests/drivers/net/lib/py/endpoint.py b/tools/testing/selftests/drivers/net/lib/py/endpoint.py
> new file mode 100644
> index 000000000000..9272fdc47a06
> --- /dev/null
> +++ b/tools/testing/selftests/drivers/net/lib/py/endpoint.py
> @@ -0,0 +1,13 @@
> +# SPDX-License-Identifier: GPL-2.0
> +
> +import importlib
> +
> +_modules = {}
> +
> +def Endpoint(ep_type, ep_args):
> +    global _modules
> +
> +    if ep_type not in _modules:
> +        _modules[ep_type] = importlib.import_module("..ep_" + ep_type, __name__)
> +
> +    return getattr(_modules[ep_type], "Endpoint")(ep_args)
> diff --git a/tools/testing/selftests/drivers/net/lib/py/ep_netns.py b/tools/testing/selftests/drivers/net/lib/py/ep_netns.py
> new file mode 100644
> index 000000000000..f5c588bb31f0
> --- /dev/null
> +++ b/tools/testing/selftests/drivers/net/lib/py/ep_netns.py
> @@ -0,0 +1,15 @@
> +# SPDX-License-Identifier: GPL-2.0
> +
> +from lib.py import cmd
> +
> +
> +class Endpoint:
> +    def __init__(self, name):
> +        self.name = name
> +
> +    def cmd(self, *args):
> +        c = cmd(*args, ns=self.name)
> +        return c.stdout, c.stderr, c.ret
> +
> +    def deploy(self, what):
> +        return what
> diff --git a/tools/testing/selftests/drivers/net/lib/py/ep_ssh.py b/tools/testing/selftests/drivers/net/lib/py/ep_ssh.py
> new file mode 100644
> index 000000000000..620df0dd8c07
> --- /dev/null
> +++ b/tools/testing/selftests/drivers/net/lib/py/ep_ssh.py
> @@ -0,0 +1,34 @@
> +# SPDX-License-Identifier: GPL-2.0
> +
> +import os
> +import shlex
> +import string
> +import random
> +
> +from lib.py import cmd
> +
> +
> +class Endpoint:
> +    def __init__(self, name):
> +        self.name = name
> +        self._tmpdir = None
> +
> +    def __del__(self):
> +        if self._tmpdir:
> +            self.cmd("rm -rf " + self._tmpdir)
> +            self._tmpdir = None
> +
> +    def cmd(self, comm, *args):
> +        c = cmd("ssh " + self.name + " " + shlex.quote(comm), *args)
> +        return c.stdout, c.stderr, c.ret
> +
> +    def _mktmp(self):
> +        return ''.join(random.choice(string.ascii_lowercase) for _ in range(8))
> +
> +    def deploy(self, what):
> +        if not self._tmpdir:
> +            self._tmpdir = "/tmp/" + self._mktmp()
> +            self.cmd("mkdir " + self._tmpdir)
> +        file_name = self._tmpdir + "/" + self._mktmp() + os.path.basename(what)
> +        cmd(f"scp {what} {self.name}:{file_name}")
> +        return file_name
> diff --git a/tools/testing/selftests/net/lib/py/utils.py b/tools/testing/selftests/net/lib/py/utils.py
> index f0d425731fd4..eff50ddd9a9d 100644
> --- a/tools/testing/selftests/net/lib/py/utils.py
> +++ b/tools/testing/selftests/net/lib/py/utils.py
> @@ -4,10 +4,8 @@ import json as _json
>  import subprocess
>  
>  class cmd:
> -    def __init__(self, comm, shell=True, fail=True, ns=None, background=False):
> +    def __init__(self, comm, shell=True, fail=True, ns=None, background=False, host=None):
>          if ns:
> -            if isinstance(ns, NetNS):
> -                ns = ns.name
>              comm = f'ip netns exec {ns} ' + comm
>  
>          self.stdout = None
> @@ -15,10 +13,13 @@ import subprocess
>          self.ret = None
>  
>          self.comm = comm
> -        self.proc = subprocess.Popen(comm, shell=shell, stdout=subprocess.PIPE,
> -                                     stderr=subprocess.PIPE)
> -        if not background:
> -            self.process(terminate=False, fail=fail)
> +        if host:
> +            self.stdout, self.stderr, self.ret = host.cmd(comm)
> +        else:
> +            self.proc = subprocess.Popen(comm, shell=shell, stdout=subprocess.PIPE,
> +                                         stderr=subprocess.PIPE)
> +            if not background:
> +                self.process(terminate=False, fail=fail)
>  
>      def process(self, terminate=True, fail=None):
>          if terminate:

Perhaps superfluous / putting the cart before the horse, but a few
thorny issues I've repeatedly run into with similar infra is

1. Cleaning up remote state in all conditions, including timeout/kill.

   Some tests require a setup phase before the test, and a matching
   cleanup phase. If any of the configured state is variable (even
   just a randomized filepath) this needs to be communicated to the
   cleanup phase. The remote filepath is handled well here. But if
   a test needs per-test setup? Say, change MTU or an Ethtool feature.
   Multiple related tests may want to share a setup/cleanup.

   Related: some tests may need benefit from a lightweight stateless
   check phase to detect preconditions before committing to any setup.
   Again, say an Ethtool feature like rx-gro-hw, or AF_XDP metadata rx.

2. Synchronizing peers. Often both peers need to be started at the
   same time, but then the client may need to wait until the server
   is listening. Paolo added a nice local script to detect a listening
   socket with sockstat. Less of a problem with TCP tests than UDP or
   raw packet tests.

Powered by blists - more mailing lists