* [PATCH net-next 1/5] selftests: drv-net: define endpoint structures
2024-04-12 23:37 [PATCH net-next 0/5] selftests: drv-net: support testing with a remote system Jakub Kicinski
@ 2024-04-12 23:37 ` Jakub Kicinski
2024-04-14 17:04 ` Willem de Bruijn
2024-04-15 8:57 ` Paolo Abeni
2024-04-12 23:37 ` [PATCH net-next 2/5] selftests: drv-net: add stdout to the command failed exception Jakub Kicinski
` (4 subsequent siblings)
5 siblings, 2 replies; 23+ messages in thread
From: Jakub Kicinski @ 2024-04-12 23:37 UTC (permalink / raw)
To: davem
Cc: netdev, edumazet, pabeni, shuah, petrm, linux-kselftest, willemb,
Jakub Kicinski
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@kernel.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:
@@ -36,12 +37,12 @@ import subprocess
raise Exception("Command failed: %s\n%s" % (self.proc.args, stderr))
-def ip(args, json=None, ns=None):
+def ip(args, json=None, ns=None, host=None):
cmd_str = "ip "
if json:
cmd_str += '-j '
cmd_str += args
- cmd_obj = cmd(cmd_str, ns=ns)
+ cmd_obj = cmd(cmd_str, ns=ns, host=host)
if json:
return _json.loads(cmd_obj.stdout)
return cmd_obj
--
2.44.0
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH net-next 1/5] selftests: drv-net: define endpoint structures
2024-04-12 23:37 ` [PATCH net-next 1/5] selftests: drv-net: define endpoint structures Jakub Kicinski
@ 2024-04-14 17:04 ` Willem de Bruijn
2024-04-15 14:16 ` Jakub Kicinski
2024-04-15 19:39 ` Petr Machata
2024-04-15 8:57 ` Paolo Abeni
1 sibling, 2 replies; 23+ messages in thread
From: Willem de Bruijn @ 2024-04-14 17:04 UTC (permalink / raw)
To: Jakub Kicinski, davem
Cc: netdev, edumazet, pabeni, shuah, petrm, linux-kselftest, willemb,
Jakub Kicinski
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@kernel.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.
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH net-next 1/5] selftests: drv-net: define endpoint structures
2024-04-14 17:04 ` Willem de Bruijn
@ 2024-04-15 14:16 ` Jakub Kicinski
2024-04-15 15:23 ` Willem de Bruijn
2024-04-15 19:39 ` Petr Machata
1 sibling, 1 reply; 23+ messages in thread
From: Jakub Kicinski @ 2024-04-15 14:16 UTC (permalink / raw)
To: Willem de Bruijn
Cc: davem, netdev, edumazet, pabeni, shuah, petrm, linux-kselftest,
willemb
On Sun, 14 Apr 2024 13:04:46 -0400 Willem de Bruijn wrote:
> 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.
I think this falls into the "frameworking debate" we were having with
Petr. The consensus seems to be to keep things as simple as possible.
If we see that tests are poorly written and would benefit from extra
structure we should try impose some, but every local custom is
something people will have to learn.
timeout/kill is provided to us already by the kselftest harness.
> 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.
Yes, definitely. We should probably add that with the first test that
needs it.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH net-next 1/5] selftests: drv-net: define endpoint structures
2024-04-15 14:16 ` Jakub Kicinski
@ 2024-04-15 15:23 ` Willem de Bruijn
0 siblings, 0 replies; 23+ messages in thread
From: Willem de Bruijn @ 2024-04-15 15:23 UTC (permalink / raw)
To: Jakub Kicinski, Willem de Bruijn
Cc: davem, netdev, edumazet, pabeni, shuah, petrm, linux-kselftest,
willemb
Jakub Kicinski wrote:
> On Sun, 14 Apr 2024 13:04:46 -0400 Willem de Bruijn wrote:
> > 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.
>
> I think this falls into the "frameworking debate" we were having with
> Petr. The consensus seems to be to keep things as simple as possible.
Makes sense. We can find the sticking points as we go along.
tools/testing/selftests/net already has a couple of hardware feature
tests, that probably see little use now that they require manual
testing (csum, gro, toeplitz, ..). Really excited to include them in
this infra to hopefully see more regular testing across more hardware.
> If we see that tests are poorly written and would benefit from extra
> structure we should try impose some, but every local custom is
> something people will have to learn.
The above were just observations from embedding tests like those
mentioned in our internal custom test framework. Especially with
heterogenous hardware, a lot of it is "can we run this test on this
platform", or "disable this feature as it interacts with the tested
feature" (e.g., HW-GRO and csum.c).
> timeout/kill is provided to us already by the kselftest harness.
>
> > 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.
>
> Yes, definitely. We should probably add that with the first test that
> needs it.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH net-next 1/5] selftests: drv-net: define endpoint structures
2024-04-14 17:04 ` Willem de Bruijn
2024-04-15 14:16 ` Jakub Kicinski
@ 2024-04-15 19:39 ` Petr Machata
1 sibling, 0 replies; 23+ messages in thread
From: Petr Machata @ 2024-04-15 19:39 UTC (permalink / raw)
To: Willem de Bruijn
Cc: davem, netdev, edumazet, pabeni, shuah, petrm, linux-kselftest,
willemb, Jakub Kicinski
Willem de Bruijn <willemdebruijn.kernel@gmail.com> writes:
> 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.
Personally I like to wrap responsibilities of this sort in context
managers, e.g. something along these lines:
class changed_mtu:
def __init__(self, dev, mtu):
self.dev = dev
self.mtu = mtu
def __enter__(self):
js = cmd(f"ip -j link show dev {self.dev}", json=True)
self.orig_mtu = something_something(js)
cmd(f"ip link set dev {self.dev} mtu {self.mtu}")
def __exit__(self, type, value, traceback):
cmd(f"ip link set dev {self.dev} mtu {self.orig_mtu}")
with changed_mtu(swp1, 10000):
# MTU is 10K here
# and back to 1500
A lot of this can be made generic, where some object is given a setup /
cleanup commands and just invokes those. But things like MTU, ethtool
speed, sysctls and what have you that need to save a previous state and
revert back to it will probably need a custom handler. Like we have them
in lib.sh as well.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH net-next 1/5] selftests: drv-net: define endpoint structures
2024-04-12 23:37 ` [PATCH net-next 1/5] selftests: drv-net: define endpoint structures Jakub Kicinski
2024-04-14 17:04 ` Willem de Bruijn
@ 2024-04-15 8:57 ` Paolo Abeni
2024-04-15 14:19 ` Jakub Kicinski
1 sibling, 1 reply; 23+ messages in thread
From: Paolo Abeni @ 2024-04-15 8:57 UTC (permalink / raw)
To: Jakub Kicinski, davem
Cc: netdev, edumazet, shuah, petrm, linux-kselftest, willemb
On Fri, 2024-04-12 at 16:37 -0700, Jakub Kicinski wrote:
> +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
If I read correctly the above will do a full ssh handshake for each
command. If the test script/setup is complex, I think/fear the overhead
could become a bit cumbersome.
Would using something alike Fabric to create a single connection at
endpoint instantiation time and re-using it for all the command be too
much?
Thanks,
Paolo
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH net-next 1/5] selftests: drv-net: define endpoint structures
2024-04-15 8:57 ` Paolo Abeni
@ 2024-04-15 14:19 ` Jakub Kicinski
2024-04-15 16:02 ` Paolo Abeni
2024-04-15 16:11 ` Paolo Abeni
0 siblings, 2 replies; 23+ messages in thread
From: Jakub Kicinski @ 2024-04-15 14:19 UTC (permalink / raw)
To: Paolo Abeni
Cc: davem, netdev, edumazet, shuah, petrm, linux-kselftest, willemb
On Mon, 15 Apr 2024 10:57:31 +0200 Paolo Abeni wrote:
> If I read correctly the above will do a full ssh handshake for each
> command. If the test script/setup is complex, I think/fear the overhead
> could become a bit cumbersome.
Connection reuse. I wasn't sure if I should add a hint to the README,
let me do so.
> Would using something alike Fabric to create a single connection at
> endpoint instantiation time and re-using it for all the command be too
> much?
IDK what "Fabric" is, if its commonly used we can add the option
in tree. If less commonly - I hope the dynamic loading scheme
will allow users to very easily drop in their own class that
integrates with Fabric, without dirtying the tree? :)
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH net-next 1/5] selftests: drv-net: define endpoint structures
2024-04-15 14:19 ` Jakub Kicinski
@ 2024-04-15 16:02 ` Paolo Abeni
2024-04-15 16:11 ` Paolo Abeni
1 sibling, 0 replies; 23+ messages in thread
From: Paolo Abeni @ 2024-04-15 16:02 UTC (permalink / raw)
To: Jakub Kicinski
Cc: davem, netdev, edumazet, shuah, petrm, linux-kselftest, willemb
On Mon, 2024-04-15 at 07:19 -0700, Jakub Kicinski wrote:
> On Mon, 15 Apr 2024 10:57:31 +0200 Paolo Abeni wrote:
> > If I read correctly the above will do a full ssh handshake for each
> > command. If the test script/setup is complex, I think/fear the overhead
> > could become a bit cumbersome.
>
> Connection reuse. I wasn't sure if I should add a hint to the README,
> let me do so.
>
> > Would using something alike Fabric to create a single connection at
> > endpoint instantiation time and re-using it for all the command be too
> > much?
>
> IDK what "Fabric" is, if its commonly used we can add the option
> in tree. If less commonly - I hope the dynamic loading scheme
> will allow users to very easily drop in their own class that
> integrates with Fabric, without dirtying the tree? :)
I'm really a python-expert. 'Fabric' a python library to execute
commands over ssh:
https://www.fabfile.org/
>
No idea how much commont it is.
I'm fine with ssh connection sharing.
Thanks,
Paolo
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH net-next 1/5] selftests: drv-net: define endpoint structures
2024-04-15 14:19 ` Jakub Kicinski
2024-04-15 16:02 ` Paolo Abeni
@ 2024-04-15 16:11 ` Paolo Abeni
1 sibling, 0 replies; 23+ messages in thread
From: Paolo Abeni @ 2024-04-15 16:11 UTC (permalink / raw)
To: Jakub Kicinski
Cc: davem, netdev, edumazet, shuah, petrm, linux-kselftest, willemb
On Mon, 2024-04-15 at 07:19 -0700, Jakub Kicinski wrote:
> On Mon, 15 Apr 2024 10:57:31 +0200 Paolo Abeni wrote:
> > If I read correctly the above will do a full ssh handshake for each
> > command. If the test script/setup is complex, I think/fear the overhead
> > could become a bit cumbersome.
>
> Connection reuse. I wasn't sure if I should add a hint to the README,
> let me do so.
I'm sorry for the multiple, incremental feedback. I think such hinto
the readme will be definitely useful, thanks!
Paolo
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH net-next 2/5] selftests: drv-net: add stdout to the command failed exception
2024-04-12 23:37 [PATCH net-next 0/5] selftests: drv-net: support testing with a remote system Jakub Kicinski
2024-04-12 23:37 ` [PATCH net-next 1/5] selftests: drv-net: define endpoint structures Jakub Kicinski
@ 2024-04-12 23:37 ` Jakub Kicinski
2024-04-12 23:37 ` [PATCH net-next 3/5] selftests: drv-net: factor out parsing of the env Jakub Kicinski
` (3 subsequent siblings)
5 siblings, 0 replies; 23+ messages in thread
From: Jakub Kicinski @ 2024-04-12 23:37 UTC (permalink / raw)
To: davem
Cc: netdev, edumazet, pabeni, shuah, petrm, linux-kselftest, willemb,
Jakub Kicinski
ping prints all the info to stdout. To make debug easier capture
stdout in the Exception raised when command unexpectedly fails.
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
tools/testing/selftests/net/lib/py/utils.py | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/net/lib/py/utils.py b/tools/testing/selftests/net/lib/py/utils.py
index eff50ddd9a9d..d47d684d9e02 100644
--- a/tools/testing/selftests/net/lib/py/utils.py
+++ b/tools/testing/selftests/net/lib/py/utils.py
@@ -34,7 +34,8 @@ import subprocess
if self.proc.returncode != 0 and fail:
if len(stderr) > 0 and stderr[-1] == "\n":
stderr = stderr[:-1]
- raise Exception("Command failed: %s\n%s" % (self.proc.args, stderr))
+ raise Exception("Command failed: %s\nSTDOUT: %s\nSTDERR: %s" %
+ (self.proc.args, stdout, stderr))
def ip(args, json=None, ns=None, host=None):
--
2.44.0
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH net-next 3/5] selftests: drv-net: factor out parsing of the env
2024-04-12 23:37 [PATCH net-next 0/5] selftests: drv-net: support testing with a remote system Jakub Kicinski
2024-04-12 23:37 ` [PATCH net-next 1/5] selftests: drv-net: define endpoint structures Jakub Kicinski
2024-04-12 23:37 ` [PATCH net-next 2/5] selftests: drv-net: add stdout to the command failed exception Jakub Kicinski
@ 2024-04-12 23:37 ` Jakub Kicinski
2024-04-12 23:37 ` [PATCH net-next 4/5] selftests: drv-net: construct environment for running tests which require an endpoint Jakub Kicinski
` (2 subsequent siblings)
5 siblings, 0 replies; 23+ messages in thread
From: Jakub Kicinski @ 2024-04-12 23:37 UTC (permalink / raw)
To: davem
Cc: netdev, edumazet, pabeni, shuah, petrm, linux-kselftest, willemb,
Jakub Kicinski
The tests with a remote end will use a different class,
for clarity, but will also need to parse the env.
So factor parsing the env out to a function.
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
.../selftests/drivers/net/lib/py/env.py | 43 +++++++++++--------
1 file changed, 26 insertions(+), 17 deletions(-)
diff --git a/tools/testing/selftests/drivers/net/lib/py/env.py b/tools/testing/selftests/drivers/net/lib/py/env.py
index e1abe9491daf..a081e168f3db 100644
--- a/tools/testing/selftests/drivers/net/lib/py/env.py
+++ b/tools/testing/selftests/drivers/net/lib/py/env.py
@@ -6,12 +6,36 @@ from pathlib import Path
from lib.py import ip
from lib.py import NetdevSimDev
+
+def _load_env_file(src_path):
+ env = os.environ.copy()
+
+ src_dir = Path(src_path).parent.resolve()
+ if not (src_dir / "net.config").exists():
+ return env
+
+ lexer = shlex.shlex(open((src_dir / "net.config").as_posix(), 'r').read())
+ k = None
+ for token in lexer:
+ if k is None:
+ k = token
+ env[k] = ""
+ elif token == "=":
+ pass
+ else:
+ env[k] = token
+ k = None
+ return env
+
+
class NetDrvEnv:
+ """
+ Class for a single NIC / host env, with no remote end
+ """
def __init__(self, src_path):
self._ns = None
- self.env = os.environ.copy()
- self._load_env_file(src_path)
+ self.env = _load_env_file(src_path)
if 'NETIF' in self.env:
self.dev = ip("link show dev " + self.env['NETIF'], json=True)[0]
@@ -34,19 +58,4 @@ from lib.py import NetdevSimDev
self._ns.remove()
self._ns = None
- def _load_env_file(self, src_path):
- src_dir = Path(src_path).parent.resolve()
- if not (src_dir / "net.config").exists():
- return
- lexer = shlex.shlex(open((src_dir / "net.config").as_posix(), 'r').read())
- k = None
- for token in lexer:
- if k is None:
- k = token
- self.env[k] = ""
- elif token == "=":
- pass
- else:
- self.env[k] = token
- k = None
--
2.44.0
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH net-next 4/5] selftests: drv-net: construct environment for running tests which require an endpoint
2024-04-12 23:37 [PATCH net-next 0/5] selftests: drv-net: support testing with a remote system Jakub Kicinski
` (2 preceding siblings ...)
2024-04-12 23:37 ` [PATCH net-next 3/5] selftests: drv-net: factor out parsing of the env Jakub Kicinski
@ 2024-04-12 23:37 ` Jakub Kicinski
2024-04-14 16:45 ` Willem de Bruijn
2024-04-15 9:28 ` Paolo Abeni
2024-04-12 23:37 ` [PATCH net-next 5/5] selftests: drv-net: add a trivial ping test Jakub Kicinski
2024-04-15 15:30 ` [PATCH net-next 0/5] selftests: drv-net: support testing with a remote system Willem de Bruijn
5 siblings, 2 replies; 23+ messages in thread
From: Jakub Kicinski @ 2024-04-12 23:37 UTC (permalink / raw)
To: davem
Cc: netdev, edumazet, pabeni, shuah, petrm, linux-kselftest, willemb,
Jakub Kicinski
Nothing surprising here, hopefully. Wrap the variables from
the environment into a class or spawn a netdevsim based env
and pass it to the tests.
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
.../testing/selftests/drivers/net/README.rst | 31 +++++++
.../selftests/drivers/net/lib/py/env.py | 93 ++++++++++++++++++-
.../testing/selftests/net/lib/py/__init__.py | 1 +
tools/testing/selftests/net/lib/py/netns.py | 31 +++++++
4 files changed, 155 insertions(+), 1 deletion(-)
create mode 100644 tools/testing/selftests/net/lib/py/netns.py
diff --git a/tools/testing/selftests/drivers/net/README.rst b/tools/testing/selftests/drivers/net/README.rst
index 5ef7c417d431..ffc15fe5d555 100644
--- a/tools/testing/selftests/drivers/net/README.rst
+++ b/tools/testing/selftests/drivers/net/README.rst
@@ -23,8 +23,39 @@ Variables can be set in the environment or by creating a net.config
# Variable set in a file
NETIF=eth0
+Please note that the config parser is very simple, if there are
+any non-alphanumeric characters in the value it needs to be in
+double quotes.
+
NETIF
~~~~~
Name of the netdevice against which the test should be executed.
When empty or not set software devices will be used.
+
+LOCAL_V4, LOCAL_V6, EP_V4, EP_V6
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+Local and remote (endpoint) IP addresses.
+
+EP_TYPE
+~~~~~~~
+
+Communication method used to run commands on the endpoint.
+Test framework supports using ``netns`` and ``ssh`` channels.
+``netns`` assumes the "remote" interface is part of the same
+host, just moved to the specified netns.
+``ssh`` communicates with remote endpoint over ``ssh`` and ``scp``.
+
+Communication methods are defined by classes in ``lib/py/ep_{name}.py``.
+It should be possible to add a new method without modifying any of
+the framework, by simply adding an appropriately named file to ``lib/py``.
+
+EP_ARGS
+~~~~~~~
+
+Arguments used to construct the communication channel.
+Communication channel dependent::
+
+ for netns - name of the "remote" namespace
+ for ssh - name/address of the remote host
diff --git a/tools/testing/selftests/drivers/net/lib/py/env.py b/tools/testing/selftests/drivers/net/lib/py/env.py
index a081e168f3db..f63be0a72a53 100644
--- a/tools/testing/selftests/drivers/net/lib/py/env.py
+++ b/tools/testing/selftests/drivers/net/lib/py/env.py
@@ -4,7 +4,8 @@ import os
import shlex
from pathlib import Path
from lib.py import ip
-from lib.py import NetdevSimDev
+from lib.py import NetNS, NetdevSimDev
+from .endpoint import Endpoint
def _load_env_file(src_path):
@@ -59,3 +60,93 @@ from lib.py import NetdevSimDev
self._ns = None
+class NetDrvEpEnv:
+ """
+ Class for an environment with a local device and "remote endpoint"
+ which can be used to send traffic in.
+
+ For local testing it creates two network namespaces and a pair
+ of netdevsim devices.
+ """
+ def __init__(self, src_path):
+
+ self.env = _load_env_file(src_path)
+
+ # Things we try to destroy
+ self.endpoint = None
+ # These are for local testing state
+ self._netns = None
+ self._ns = None
+ self._ns_peer = None
+
+ if "NETIF" in self.env:
+ self.dev = ip("link show dev " + self.env['NETIF'], json=True)[0]
+
+ self.v4 = self.env.get("LOCAL_V4")
+ self.v6 = self.env.get("LOCAL_V6")
+ self.ep_v4 = self.env.get("EP_V4")
+ self.ep_v6 = self.env.get("EP_V6")
+ ep_type = self.env["EP_TYPE"]
+ ep_args = self.env["EP_ARGS"]
+ else:
+ self.create_local()
+
+ self.dev = self._ns.nsims[0].dev
+
+ self.v4 = "192.0.2.1"
+ self.v6 ="0100::1"
+ self.ep_v4 = "192.0.2.2"
+ self.ep_v6 = "0100::2"
+ ep_type = "netns"
+ ep_args = self._netns.name
+
+ self.endpoint = Endpoint(ep_type, ep_args)
+
+ self.addr = self.v6 if self.v6 else self.v4
+ self.ep_addr = self.ep_v6 if self.ep_v6 else self.ep_v4
+
+ self.ifname = self.dev['ifname']
+ self.ifindex = self.dev['ifindex']
+
+ def create_local(self):
+ self._netns = NetNS()
+ self._ns = NetdevSimDev()
+ self._ns_peer = NetdevSimDev(ns=self._netns)
+
+ with open("/proc/self/ns/net") as nsfd0, \
+ open("/var/run/netns/" + self._netns.name) as nsfd1:
+ ifi0 = self._ns.nsims[0].ifindex
+ ifi1 = self._ns_peer.nsims[0].ifindex
+ NetdevSimDev.ctrl_write('link_device',
+ f'{nsfd0.fileno()}:{ifi0} {nsfd1.fileno()}:{ifi1}')
+
+ ip(f" addr add dev {self._ns.nsims[0].ifname} 192.0.2.1/24")
+ ip(f"-6 addr add dev {self._ns.nsims[0].ifname} 0100::1/64 nodad")
+ ip(f" link set dev {self._ns.nsims[0].ifname} up")
+
+ ip(f" addr add dev {self._ns_peer.nsims[0].ifname} 192.0.2.2/24", ns=self._netns)
+ ip(f"-6 addr add dev {self._ns_peer.nsims[0].ifname} 0100::2/64 nodad", ns=self._netns)
+ ip(f" link set dev {self._ns_peer.nsims[0].ifname} up", ns=self._netns)
+
+ def __enter__(self):
+ return self
+
+ def __exit__(self, ex_type, ex_value, ex_tb):
+ """
+ __exit__ gets called at the end of a "with" block.
+ """
+ self.__del__()
+
+ def __del__(self):
+ if self._ns:
+ self._ns.remove()
+ self._ns = None
+ if self._ns_peer:
+ self._ns_peer.remove()
+ self._ns_peer = None
+ if self._netns:
+ del self._netns
+ self._netns = None
+ if self.endpoint:
+ del self.endpoint
+ self.endpoint = None
diff --git a/tools/testing/selftests/net/lib/py/__init__.py b/tools/testing/selftests/net/lib/py/__init__.py
index ded7102df18a..b6d498d125fe 100644
--- a/tools/testing/selftests/net/lib/py/__init__.py
+++ b/tools/testing/selftests/net/lib/py/__init__.py
@@ -2,6 +2,7 @@
from .consts import KSRC
from .ksft import *
+from .netns import NetNS
from .nsim import *
from .utils import *
from .ynl import NlError, YnlFamily, EthtoolFamily, NetdevFamily, RtnlFamily
diff --git a/tools/testing/selftests/net/lib/py/netns.py b/tools/testing/selftests/net/lib/py/netns.py
new file mode 100644
index 000000000000..ecff85f9074f
--- /dev/null
+++ b/tools/testing/selftests/net/lib/py/netns.py
@@ -0,0 +1,31 @@
+# SPDX-License-Identifier: GPL-2.0
+
+from .utils import ip
+import random
+import string
+
+
+class NetNS:
+ def __init__(self, name=None):
+ if name:
+ self.name = name
+ else:
+ self.name = ''.join(random.choice(string.ascii_lowercase) for _ in range(8))
+ ip('netns add ' + self.name)
+
+ def __del__(self):
+ if self.name:
+ ip('netns del ' + self.name)
+ self.name = None
+
+ def __enter__(self):
+ return self
+
+ def __exit__(self, ex_type, ex_value, ex_tb):
+ self.__del__()
+
+ def __str__(self):
+ return self.name
+
+ def __repr__(self):
+ return f"NetNS({self.name})"
--
2.44.0
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH net-next 4/5] selftests: drv-net: construct environment for running tests which require an endpoint
2024-04-12 23:37 ` [PATCH net-next 4/5] selftests: drv-net: construct environment for running tests which require an endpoint Jakub Kicinski
@ 2024-04-14 16:45 ` Willem de Bruijn
2024-04-15 14:31 ` Jakub Kicinski
2024-04-15 9:28 ` Paolo Abeni
1 sibling, 1 reply; 23+ messages in thread
From: Willem de Bruijn @ 2024-04-14 16:45 UTC (permalink / raw)
To: Jakub Kicinski, davem
Cc: netdev, edumazet, pabeni, shuah, petrm, linux-kselftest, willemb,
Jakub Kicinski
Jakub Kicinski wrote:
> Nothing surprising here, hopefully. Wrap the variables from
> the environment into a class or spawn a netdevsim based env
> and pass it to the tests.
>
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
> .../testing/selftests/drivers/net/README.rst | 31 +++++++
> .../selftests/drivers/net/lib/py/env.py | 93 ++++++++++++++++++-
> .../testing/selftests/net/lib/py/__init__.py | 1 +
> tools/testing/selftests/net/lib/py/netns.py | 31 +++++++
> 4 files changed, 155 insertions(+), 1 deletion(-)
> create mode 100644 tools/testing/selftests/net/lib/py/netns.py
>
> diff --git a/tools/testing/selftests/drivers/net/README.rst b/tools/testing/selftests/drivers/net/README.rst
> index 5ef7c417d431..ffc15fe5d555 100644
> --- a/tools/testing/selftests/drivers/net/README.rst
> +++ b/tools/testing/selftests/drivers/net/README.rst
> @@ -23,8 +23,39 @@ Variables can be set in the environment or by creating a net.config
> # Variable set in a file
> NETIF=eth0
>
> +Please note that the config parser is very simple, if there are
> +any non-alphanumeric characters in the value it needs to be in
> +double quotes.
> +
> NETIF
> ~~~~~
>
> Name of the netdevice against which the test should be executed.
> When empty or not set software devices will be used.
> +
> +LOCAL_V4, LOCAL_V6, EP_V4, EP_V6
> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> +
> +Local and remote (endpoint) IP addresses.
Overall, this is really cool stuff (obviously)!
REMOTE instead of EP?
Apparently I missed the earlier discussion. Would it also be possible
to have both sides be remote. Where the test runner might run on the
build host, but the kernel under test is run on two test machines.
To a certain extent, same for having two equivalent child network
namespaces isolated from the runner's environment.
> +
> +EP_TYPE
> +~~~~~~~
> +
> +Communication method used to run commands on the endpoint.
> +Test framework supports using ``netns`` and ``ssh`` channels.
> +``netns`` assumes the "remote" interface is part of the same
> +host, just moved to the specified netns.
> +``ssh`` communicates with remote endpoint over ``ssh`` and ``scp``.
> +
> +Communication methods are defined by classes in ``lib/py/ep_{name}.py``.
> +It should be possible to add a new method without modifying any of
> +the framework, by simply adding an appropriately named file to ``lib/py``.
> +
> +EP_ARGS
> +~~~~~~~
> +
> +Arguments used to construct the communication channel.
> +Communication channel dependent::
> +
> + for netns - name of the "remote" namespace
> + for ssh - name/address of the remote host
> diff --git a/tools/testing/selftests/drivers/net/lib/py/env.py b/tools/testing/selftests/drivers/net/lib/py/env.py
> index a081e168f3db..f63be0a72a53 100644
> --- a/tools/testing/selftests/drivers/net/lib/py/env.py
> +++ b/tools/testing/selftests/drivers/net/lib/py/env.py
> @@ -4,7 +4,8 @@ import os
> import shlex
> from pathlib import Path
> from lib.py import ip
> -from lib.py import NetdevSimDev
> +from lib.py import NetNS, NetdevSimDev
> +from .endpoint import Endpoint
>
>
> def _load_env_file(src_path):
> @@ -59,3 +60,93 @@ from lib.py import NetdevSimDev
> self._ns = None
>
>
> +class NetDrvEpEnv:
> + """
> + Class for an environment with a local device and "remote endpoint"
> + which can be used to send traffic in.
> +
> + For local testing it creates two network namespaces and a pair
> + of netdevsim devices.
> + """
> + def __init__(self, src_path):
> +
> + self.env = _load_env_file(src_path)
> +
> + # Things we try to destroy
> + self.endpoint = None
> + # These are for local testing state
> + self._netns = None
> + self._ns = None
> + self._ns_peer = None
> +
> + if "NETIF" in self.env:
> + self.dev = ip("link show dev " + self.env['NETIF'], json=True)[0]
> +
> + self.v4 = self.env.get("LOCAL_V4")
> + self.v6 = self.env.get("LOCAL_V6")
> + self.ep_v4 = self.env.get("EP_V4")
> + self.ep_v6 = self.env.get("EP_V6")
> + ep_type = self.env["EP_TYPE"]
> + ep_args = self.env["EP_ARGS"]
> + else:
> + self.create_local()
> +
> + self.dev = self._ns.nsims[0].dev
> +
> + self.v4 = "192.0.2.1"
> + self.v6 ="0100::1"
> + self.ep_v4 = "192.0.2.2"
> + self.ep_v6 = "0100::2"
Use FC00::/7 ULA addresses?
> + ep_type = "netns"
> + ep_args = self._netns.name
> +
> + self.endpoint = Endpoint(ep_type, ep_args)
> +
> + self.addr = self.v6 if self.v6 else self.v4
> + self.ep_addr = self.ep_v6 if self.ep_v6 else self.ep_v4
> +
> + self.ifname = self.dev['ifname']
> + self.ifindex = self.dev['ifindex']
> +
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH net-next 4/5] selftests: drv-net: construct environment for running tests which require an endpoint
2024-04-14 16:45 ` Willem de Bruijn
@ 2024-04-15 14:31 ` Jakub Kicinski
2024-04-15 15:28 ` Willem de Bruijn
0 siblings, 1 reply; 23+ messages in thread
From: Jakub Kicinski @ 2024-04-15 14:31 UTC (permalink / raw)
To: Willem de Bruijn
Cc: davem, netdev, edumazet, pabeni, shuah, petrm, linux-kselftest,
willemb
On Sun, 14 Apr 2024 12:45:43 -0400 Willem de Bruijn wrote:
> Overall, this is really cool stuff (obviously)!
>
> REMOTE instead of EP?
If I have to (:
Endpoint isn't great.
But remote doesn't seem much better, and it doesn't have a nice
abbreviation :(
> Apparently I missed the earlier discussion. Would it also be possible
> to have both sides be remote. Where the test runner might run on the
> build host, but the kernel under test is run on two test machines.
>
> To a certain extent, same for having two equivalent child network
> namespaces isolated from the runner's environment.
I was thinking about it (and even wrote one large test which uses
2 namespaces [1]). But I could not convince myself that the added
complication is worth it.
[1] https://github.com/kuba-moo/linux/blob/psp/tools/net/ynl/psp.py
Local namespace testing is one thing, entering the namespace from
python and using the right process abstraction to make sure garbage
collector doesn't collect the namespace before the test exits it
(sigh) is all doable. But we lose the ability interact with the local
system directly when the endpoint is remote. No local FW access with
read/write, we have to "cat" and "echo" like in bash. No YNL access,
unless we ship specs and CLI over.
So I concluded that we're better off leaning on kselftest for
remote/remote. make install, copy the tests over, run them remotely.
I may be biased tho, I don't have much use for remote/remote in my
development env.
> Use FC00::/7 ULA addresses?
Doesn't ULA have some magic address selection rules which IETF
is just trying to fix now? IIUC 0100:: is the documentation prefix,
so shouldn't be too bad?
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH net-next 4/5] selftests: drv-net: construct environment for running tests which require an endpoint
2024-04-15 14:31 ` Jakub Kicinski
@ 2024-04-15 15:28 ` Willem de Bruijn
2024-04-15 17:36 ` Jakub Kicinski
0 siblings, 1 reply; 23+ messages in thread
From: Willem de Bruijn @ 2024-04-15 15:28 UTC (permalink / raw)
To: Jakub Kicinski, Willem de Bruijn
Cc: davem, netdev, edumazet, pabeni, shuah, petrm, linux-kselftest,
willemb
Jakub Kicinski wrote:
> On Sun, 14 Apr 2024 12:45:43 -0400 Willem de Bruijn wrote:
> > Overall, this is really cool stuff (obviously)!
> >
> > REMOTE instead of EP?
>
> If I have to (:
> Endpoint isn't great.
> But remote doesn't seem much better, and it doesn't have a nice
> abbreviation :(
It pairs well with local.
Since in some tests the (local) machine under test is the sender and
in others it is the receiver, we cannot use SERVER/CLIENT or so.
> > Apparently I missed the earlier discussion. Would it also be possible
> > to have both sides be remote. Where the test runner might run on the
> > build host, but the kernel under test is run on two test machines.
> >
> > To a certain extent, same for having two equivalent child network
> > namespaces isolated from the runner's environment.
>
> I was thinking about it (and even wrote one large test which uses
> 2 namespaces [1]). But I could not convince myself that the added
> complication is worth it.
>
> [1] https://github.com/kuba-moo/linux/blob/psp/tools/net/ynl/psp.py
>
> Local namespace testing is one thing, entering the namespace from
> python and using the right process abstraction to make sure garbage
> collector doesn't collect the namespace before the test exits it
> (sigh) is all doable. But we lose the ability interact with the local
> system directly when the endpoint is remote. No local FW access with
> read/write, we have to "cat" and "echo" like in bash. No YNL access,
> unless we ship specs and CLI over.
In cases like testing jumbo frames (or other MTU, like 4K),
configuration changes will have to be made on both the machine under
test and the remote traffic generator/sink. It seems to me
unavoidable. Most of the two-machine tests I require an equal amount
of setup on both sides. But again, cart before the horse. We can
always revisit this later if needed.
> So I concluded that we're better off leaning on kselftest for
> remote/remote. make install, copy the tests over, run them remotely.
> I may be biased tho, I don't have much use for remote/remote in my
> development env.
>
> > Use FC00::/7 ULA addresses?
>
> Doesn't ULA have some magic address selection rules which IETF
> is just trying to fix now? IIUC 0100:: is the documentation prefix,
> so shouldn't be too bad?
RFC 6666 defines this as the "Discard Prefix".
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH net-next 4/5] selftests: drv-net: construct environment for running tests which require an endpoint
2024-04-15 15:28 ` Willem de Bruijn
@ 2024-04-15 17:36 ` Jakub Kicinski
0 siblings, 0 replies; 23+ messages in thread
From: Jakub Kicinski @ 2024-04-15 17:36 UTC (permalink / raw)
To: Willem de Bruijn
Cc: davem, netdev, edumazet, pabeni, shuah, petrm, linux-kselftest,
willemb
On Mon, 15 Apr 2024 11:28:47 -0400 Willem de Bruijn wrote:
> > If I have to (:
> > Endpoint isn't great.
> > But remote doesn't seem much better, and it doesn't have a nice
> > abbreviation :(
>
> It pairs well with local.
>
> Since in some tests the (local) machine under test is the sender and
> in others it is the receiver, we cannot use SERVER/CLIENT or so.
Alright.
> > > Use FC00::/7 ULA addresses?
> >
> > Doesn't ULA have some magic address selection rules which IETF
> > is just trying to fix now? IIUC 0100:: is the documentation prefix,
> > so shouldn't be too bad?
>
> RFC 6666 defines this as the "Discard Prefix".
Alright, let me use Paolo's suggestion of 2001:db8:
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH net-next 4/5] selftests: drv-net: construct environment for running tests which require an endpoint
2024-04-12 23:37 ` [PATCH net-next 4/5] selftests: drv-net: construct environment for running tests which require an endpoint Jakub Kicinski
2024-04-14 16:45 ` Willem de Bruijn
@ 2024-04-15 9:28 ` Paolo Abeni
1 sibling, 0 replies; 23+ messages in thread
From: Paolo Abeni @ 2024-04-15 9:28 UTC (permalink / raw)
To: Jakub Kicinski, davem
Cc: netdev, edumazet, shuah, petrm, linux-kselftest, willemb
On Fri, 2024-04-12 at 16:37 -0700, Jakub Kicinski wrote:
> Nothing surprising here, hopefully. Wrap the variables from
> the environment into a class or spawn a netdevsim based env
> and pass it to the tests.
>
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
> .../testing/selftests/drivers/net/README.rst | 31 +++++++
> .../selftests/drivers/net/lib/py/env.py | 93 ++++++++++++++++++-
> .../testing/selftests/net/lib/py/__init__.py | 1 +
> tools/testing/selftests/net/lib/py/netns.py | 31 +++++++
> 4 files changed, 155 insertions(+), 1 deletion(-)
> create mode 100644 tools/testing/selftests/net/lib/py/netns.py
>
> diff --git a/tools/testing/selftests/drivers/net/README.rst b/tools/testing/selftests/drivers/net/README.rst
> index 5ef7c417d431..ffc15fe5d555 100644
> --- a/tools/testing/selftests/drivers/net/README.rst
> +++ b/tools/testing/selftests/drivers/net/README.rst
> @@ -23,8 +23,39 @@ Variables can be set in the environment or by creating a net.config
> # Variable set in a file
> NETIF=eth0
>
> +Please note that the config parser is very simple, if there are
> +any non-alphanumeric characters in the value it needs to be in
> +double quotes.
> +
> NETIF
> ~~~~~
>
> Name of the netdevice against which the test should be executed.
> When empty or not set software devices will be used.
> +
> +LOCAL_V4, LOCAL_V6, EP_V4, EP_V6
> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> +
> +Local and remote (endpoint) IP addresses.
> +
> +EP_TYPE
> +~~~~~~~
> +
> +Communication method used to run commands on the endpoint.
> +Test framework supports using ``netns`` and ``ssh`` channels.
> +``netns`` assumes the "remote" interface is part of the same
> +host, just moved to the specified netns.
> +``ssh`` communicates with remote endpoint over ``ssh`` and ``scp``.
> +
> +Communication methods are defined by classes in ``lib/py/ep_{name}.py``.
> +It should be possible to add a new method without modifying any of
> +the framework, by simply adding an appropriately named file to ``lib/py``.
> +
> +EP_ARGS
> +~~~~~~~
> +
> +Arguments used to construct the communication channel.
> +Communication channel dependent::
> +
> + for netns - name of the "remote" namespace
> + for ssh - name/address of the remote host
> diff --git a/tools/testing/selftests/drivers/net/lib/py/env.py b/tools/testing/selftests/drivers/net/lib/py/env.py
> index a081e168f3db..f63be0a72a53 100644
> --- a/tools/testing/selftests/drivers/net/lib/py/env.py
> +++ b/tools/testing/selftests/drivers/net/lib/py/env.py
> @@ -4,7 +4,8 @@ import os
> import shlex
> from pathlib import Path
> from lib.py import ip
> -from lib.py import NetdevSimDev
> +from lib.py import NetNS, NetdevSimDev
> +from .endpoint import Endpoint
>
>
> def _load_env_file(src_path):
> @@ -59,3 +60,93 @@ from lib.py import NetdevSimDev
> self._ns = None
>
>
> +class NetDrvEpEnv:
> + """
> + Class for an environment with a local device and "remote endpoint"
> + which can be used to send traffic in.
> +
> + For local testing it creates two network namespaces and a pair
> + of netdevsim devices.
> + """
> + def __init__(self, src_path):
> +
> + self.env = _load_env_file(src_path)
> +
> + # Things we try to destroy
> + self.endpoint = None
> + # These are for local testing state
> + self._netns = None
> + self._ns = None
> + self._ns_peer = None
> +
> + if "NETIF" in self.env:
> + self.dev = ip("link show dev " + self.env['NETIF'], json=True)[0]
> +
> + self.v4 = self.env.get("LOCAL_V4")
> + self.v6 = self.env.get("LOCAL_V6")
> + self.ep_v4 = self.env.get("EP_V4")
> + self.ep_v6 = self.env.get("EP_V6")
> + ep_type = self.env["EP_TYPE"]
> + ep_args = self.env["EP_ARGS"]
> + else:
> + self.create_local()
> +
> + self.dev = self._ns.nsims[0].dev
> +
> + self.v4 = "192.0.2.1"
> + self.v6 ="0100::1"
Minor nit, what about using 2001:db8:, for more consistency with
existing 'net' self-tests?
Also +1 on Willem suggestion to possibly have both endpoints remote.
(Very cool stuff, ça va sans dire ;)
Thanks,
Paolo
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH net-next 5/5] selftests: drv-net: add a trivial ping test
2024-04-12 23:37 [PATCH net-next 0/5] selftests: drv-net: support testing with a remote system Jakub Kicinski
` (3 preceding siblings ...)
2024-04-12 23:37 ` [PATCH net-next 4/5] selftests: drv-net: construct environment for running tests which require an endpoint Jakub Kicinski
@ 2024-04-12 23:37 ` Jakub Kicinski
2024-04-15 9:31 ` Paolo Abeni
2024-04-15 15:30 ` [PATCH net-next 0/5] selftests: drv-net: support testing with a remote system Willem de Bruijn
5 siblings, 1 reply; 23+ messages in thread
From: Jakub Kicinski @ 2024-04-12 23:37 UTC (permalink / raw)
To: davem
Cc: netdev, edumazet, pabeni, shuah, petrm, linux-kselftest, willemb,
Jakub Kicinski
Add a very simple test for testing with a remote system.
Both IPv4 and IPv6 connectivity is optional so tests
will XFail is env doesn't define an address for the given
family.
Using netdevsim:
$ ./run_kselftest.sh -t drivers/net:ping.py
TAP version 13
1..1
# timeout set to 45
# selftests: drivers/net: ping.py
# KTAP version 1
# 1..2
# ok 1 ping.ping_v4
# ok 2 ping.ping_v6
# # Totals: pass:2 fail:0 xfail:0 xpass:0 skip:0 error:0
ok 1 selftests: drivers/net: ping.py
Command line SSH:
$ NETIF=virbr0 EP_TYPE=ssh EP_ARGS=root@192.168.122.123 \
LOCAL_V4=192.168.122.1 EP_V4=192.168.122.123 \
./tools/testing/selftests/drivers/net/ping.py
KTAP version 1
1..2
ok 1 ping.ping_v4
ok 2 ping.ping_v6 # XFAIL
# Totals: pass:1 fail:0 xfail:1 xpass:0 skip:0 error:0
Existing devices placed in netns (and using net.config):
$ cat drivers/net/net.config
NETIF=veth0
EP_TYPE=netns
EP_ARGS=red
LOCAL_V4="192.168.1.1"
EP_V4="192.168.1.2"
$ ./run_kselftest.sh -t drivers/net:ping.py
TAP version 13
1..1
# timeout set to 45
# selftests: drivers/net: ping.py
# KTAP version 1
# 1..2
# ok 1 ping.ping_v4
# ok 2 ping.ping_v6 # XFAIL
# # Totals: pass:1 fail:0 xfail:1 xpass:0 skip:0 error:0
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
tools/testing/selftests/drivers/net/Makefile | 4 ++-
tools/testing/selftests/drivers/net/ping.py | 32 ++++++++++++++++++++
2 files changed, 35 insertions(+), 1 deletion(-)
create mode 100755 tools/testing/selftests/drivers/net/ping.py
diff --git a/tools/testing/selftests/drivers/net/Makefile b/tools/testing/selftests/drivers/net/Makefile
index 379cdb1960a7..713ab251cea9 100644
--- a/tools/testing/selftests/drivers/net/Makefile
+++ b/tools/testing/selftests/drivers/net/Makefile
@@ -2,6 +2,8 @@
TEST_INCLUDES := $(wildcard lib/py/*.py)
-TEST_PROGS := stats.py
+TEST_PROGS := \
+ ping.py \
+ stats.py \
include ../../lib.mk
diff --git a/tools/testing/selftests/drivers/net/ping.py b/tools/testing/selftests/drivers/net/ping.py
new file mode 100755
index 000000000000..df746543f5c3
--- /dev/null
+++ b/tools/testing/selftests/drivers/net/ping.py
@@ -0,0 +1,32 @@
+#!/usr/bin/env python3
+# SPDX-License-Identifier: GPL-2.0
+
+from lib.py import ksft_run, KsftXfailEx
+from lib.py import NetDrvEpEnv
+from lib.py import cmd
+
+
+def ping_v4(cfg) -> None:
+ if not cfg.v4:
+ raise KsftXfailEx()
+
+ cmd(f"ping -c 1 -W0.5 {cfg.ep_v4}")
+ cmd(f"ping -c 1 -W0.5 {cfg.v4}", host=cfg.endpoint)
+
+
+def ping_v6(cfg) -> None:
+ if not cfg.v6:
+ raise KsftXfailEx()
+
+ cmd(f"ping -c 1 -W0.5 {cfg.ep_v6}")
+ cmd(f"ping -c 1 -W0.5 {cfg.v6}", host=cfg.endpoint)
+
+
+def main() -> None:
+ with NetDrvEpEnv(__file__) as cfg:
+ ksft_run([ping_v4, ping_v6],
+ args=(cfg, ))
+
+
+if __name__ == "__main__":
+ main()
--
2.44.0
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH net-next 5/5] selftests: drv-net: add a trivial ping test
2024-04-12 23:37 ` [PATCH net-next 5/5] selftests: drv-net: add a trivial ping test Jakub Kicinski
@ 2024-04-15 9:31 ` Paolo Abeni
2024-04-15 14:33 ` Jakub Kicinski
0 siblings, 1 reply; 23+ messages in thread
From: Paolo Abeni @ 2024-04-15 9:31 UTC (permalink / raw)
To: Jakub Kicinski, davem
Cc: netdev, edumazet, shuah, petrm, linux-kselftest, willemb
On Fri, 2024-04-12 at 16:37 -0700, Jakub Kicinski wrote:
> +def ping_v4(cfg) -> None:
> + if not cfg.v4:
> + raise KsftXfailEx()
> +
> + cmd(f"ping -c 1 -W0.5 {cfg.ep_v4}")
> + cmd(f"ping -c 1 -W0.5 {cfg.v4}", host=cfg.endpoint)
Very minor nit, I personally find a bit more readable:
cfg.endpoint.cmd()
Which is already supported by the current infra, right?
With both endpoint possibly remote could be:
cfg.ep1.cmd()
cfg.ep2.cmd()
Thanks!
Paolo
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH net-next 5/5] selftests: drv-net: add a trivial ping test
2024-04-15 9:31 ` Paolo Abeni
@ 2024-04-15 14:33 ` Jakub Kicinski
2024-04-15 16:09 ` Paolo Abeni
0 siblings, 1 reply; 23+ messages in thread
From: Jakub Kicinski @ 2024-04-15 14:33 UTC (permalink / raw)
To: Paolo Abeni
Cc: davem, netdev, edumazet, shuah, petrm, linux-kselftest, willemb
On Mon, 15 Apr 2024 11:31:05 +0200 Paolo Abeni wrote:
> On Fri, 2024-04-12 at 16:37 -0700, Jakub Kicinski wrote:
> > +def ping_v4(cfg) -> None:
> > + if not cfg.v4:
> > + raise KsftXfailEx()
> > +
> > + cmd(f"ping -c 1 -W0.5 {cfg.ep_v4}")
> > + cmd(f"ping -c 1 -W0.5 {cfg.v4}", host=cfg.endpoint)
>
> Very minor nit, I personally find a bit more readable:
>
> cfg.endpoint.cmd()
>
> Which is already supported by the current infra, right?
>
> With both endpoint possibly remote could be:
>
> cfg.ep1.cmd()
> cfg.ep2.cmd()
As I said in the cover letter, I don't want to push us too much towards
classes. The argument format make local and local+remote tests look more
similar.
I could be wrong 🤷️
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH net-next 5/5] selftests: drv-net: add a trivial ping test
2024-04-15 14:33 ` Jakub Kicinski
@ 2024-04-15 16:09 ` Paolo Abeni
0 siblings, 0 replies; 23+ messages in thread
From: Paolo Abeni @ 2024-04-15 16:09 UTC (permalink / raw)
To: Jakub Kicinski
Cc: davem, netdev, edumazet, shuah, petrm, linux-kselftest, willemb
On Mon, 2024-04-15 at 07:33 -0700, Jakub Kicinski wrote:
> On Mon, 15 Apr 2024 11:31:05 +0200 Paolo Abeni wrote:
> > On Fri, 2024-04-12 at 16:37 -0700, Jakub Kicinski wrote:
> > > +def ping_v4(cfg) -> None:
> > > + if not cfg.v4:
> > > + raise KsftXfailEx()
> > > +
> > > + cmd(f"ping -c 1 -W0.5 {cfg.ep_v4}")
> > > + cmd(f"ping -c 1 -W0.5 {cfg.v4}", host=cfg.endpoint)
> >
> > Very minor nit, I personally find a bit more readable:
> >
> > cfg.endpoint.cmd()
> >
> > Which is already supported by the current infra, right?
> >
> > With both endpoint possibly remote could be:
> >
> > cfg.ep1.cmd()
> > cfg.ep2.cmd()
>
> As I said in the cover letter, I don't want to push us too much towards
> classes. The argument format make local and local+remote tests look more
> similar.
I guess it's a matter of personal preferences. I know mine are usually
quite twisted ;)
I'm fine with either syntax.
Cheers,
Paolo
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH net-next 0/5] selftests: drv-net: support testing with a remote system
2024-04-12 23:37 [PATCH net-next 0/5] selftests: drv-net: support testing with a remote system Jakub Kicinski
` (4 preceding siblings ...)
2024-04-12 23:37 ` [PATCH net-next 5/5] selftests: drv-net: add a trivial ping test Jakub Kicinski
@ 2024-04-15 15:30 ` Willem de Bruijn
5 siblings, 0 replies; 23+ messages in thread
From: Willem de Bruijn @ 2024-04-15 15:30 UTC (permalink / raw)
To: Jakub Kicinski, davem
Cc: netdev, edumazet, pabeni, shuah, petrm, linux-kselftest, willemb,
Jakub Kicinski
Jakub Kicinski wrote:
> Hi!
>
> Implement support for tests which require access to a remote system /
> endpoint which can generate traffic.
> This series concludes the "groundwork" for upstream driver tests.
>
> I wanted to support the three models which came up in discussions:
> - SW testing with netdevsim
> - "local" testing with two ports on the same system in a loopback
> - "remote" testing via SSH
> so there is a tiny bit of an abstraction which wraps up how "remote"
> commands are executed. Otherwise hopefully there's nothing surprising.
>
> I'm only adding a ping test. I had a bigger one written but I was
> worried we'll get into discussing the details of the test itself
> and how I chose to hack up netdevsim, instead of the test infra...
> So that test will be a follow up :)
>
> ---
>
> TBH, this series is on top of the one I posted in the morning:
> https://lore.kernel.org/all/20240412141436.828666-1-kuba@kernel.org/
> but it applies cleanly, and all it needs is the ifindex definition
> in netdevsim. Testing with real HW works fine even without the other
> series.
>
> Jakub Kicinski (5):
> selftests: drv-net: define endpoint structures
> selftests: drv-net: add stdout to the command failed exception
> selftests: drv-net: factor out parsing of the env
> selftests: drv-net: construct environment for running tests which
> require an endpoint
> selftests: drv-net: add a trivial ping test
For the series:
Reviewed-by: Willem de Bruijn <willemb@google.com>
I left some comments for discussion, but did not spell out the more
important part: series looks great to me. Thanks for building this!
^ permalink raw reply [flat|nested] 23+ messages in thread