Re: [PATCH v2 1/1] dts: add binding to different drivers to TG node

[Jeremy Spewock <jspewock@iol.unh.edu>]

On Tue, Sep 24, 2024 at 5:12 AM Juraj Linkeš <juraj.linkes@pantheon.tech> wrote:
>
> I have some thoughts for the future:
> 1a. The traffic generator is specified per-node, so maybe we could also
> change the binding to be for the whole lifetime of the TG node,
> 1b. But the same is true for the SUT node as well, right? After we do
> the port update (with kernel driver), we can just bind to DPDK driver.
> With SUT in the mix, this looks like a change for a different patch,

Right, these are good points. A good observation too that we only
really need the kernel driver at the start in both cases. You had
mentioned in your previous comments as well that we should only be
binding on the TG once per lifetime, but I ended up not adding it for
that very reason of I still wanted the binding to be in Node, but I
didn't want to change the process for the SUT.

> 2. We could add a symlink to the devbind script with the target being in
> the dts directory. This way we don't have to go outside the dts
> directory and if DTS ever become a python package, we could just copy
> the script to the appropriate place. This is also something we don't
> really need to do.

I like this idea a lot actually. It feels very weird to me having to
step out of the DTS directory and I like the idea of keeping it
together like it were a package (even if it isn't yet).

>
> And also two minor comments. A lot of suggestions for separate patches
> overall. :-)
>
> On 19. 9. 2024 20:16, jspewock@iol.unh.edu wrote:
> > From: Jeremy Spewock <jspewock@iol.unh.edu>
> >
> > The DTS framework in its current state supports binding ports to
> > different drivers on the SUT node but not the TG node. The TG node
> > already has the information that it needs about the different drivers
> > that it has available in the configuration file, but it did not
> > previously have access to the devbind script, so it did not use that
> > information for anything.
> >
> > This patch moves the location of the tmp directory as well as the method
> > for binding ports into the node class rather than the SUT node class and
> > adds an abstract method for getting the path to the devbind script into
> > the node class. Then, binding ports to the correct drivers is moved into
> > the build target setup and run on both nodes.
> >
> > Bugzilla ID: 1420
> >
> > Signed-off-by: Jeremy Spewock <jspewock@iol.unh.edu>
> > ---
>
> With the two minor comments,
> Reviewed-by: Juraj Linkeš <juraj.linkes@pantheon.tech>
>
> > diff --git a/dts/framework/testbed_model/node.py b/dts/framework/testbed_model/node.py
>
> > @@ -58,8 +65,10 @@ class Node(ABC):
> >       lcores: list[LogicalCore]
> >       ports: list[Port]
> >       _logger: DTSLogger
> > +    _remote_tmp_dir: PurePath
> >       _other_sessions: list[OSSession]
> >       _test_run_config: TestRunConfiguration
> > +    _path_to_devbind_script: PurePath | None
>
> A note on the naming. We have _remote_tmp_dir and
> _path_to_devbind_script. Both are pointing to a remote file/dir, but
> only one has the _remote prefix. They should probably be unified.

I didn't think of this but you're right, the two are very similar but
named differently.

>
> I've thought a bit about what the right name is. Dropping the prefix
> makes sense; sut_node.tmp_dir should mean the tmp dir on the SUT node
> (which would make it remote from the execution host's point of view, but
> not the node's view; the file is local to SUT node). This could be a
> good separate patch (improving the remote/local naming scheme to make it
> consistent and as sensible as possible).

I also like the sound of it without the prefix and how it actually has
a more fitting meaning from the two perspectives. I agree that there
is probably some other work to be done on this in another patch, but
since I am moving the _remote_tmp_dir variable around anyway I think
it wouldn't hurt for me to rename it.

>
>
> > diff --git a/dts/framework/utils.py b/dts/framework/utils.py
>
> > @@ -29,6 +29,8 @@
> >   from .exception import ConfigurationError, InternalError
> >
> >   REGEX_FOR_PCI_ADDRESS: str = "/[0-9a-fA-F]{4}:[0-9a-fA-F]{2}:[0-9a-fA-F]{2}.[0-9]{1}/"
> > +#: Path to DPDK directory on the host where DTS is being run.
> > +LOCAL_DPDK_DIR: PurePath = PurePath(__file__).parents[2]
>
> Local paths can be just pathlib.Path. PurePaths are for path
> manipulations only (useful for remote paths in RemoteSessions,
> OSSessions and Nodes), but for local existing paths we should use Path.
>
> The OSSession and subclasses need a bit of an update in this regard -
> use Path for local paths and PurePaths for remote ones. We added this
> into our pre-built DPDK patch.

Ack. I tried to search for the differences between the two actually
and saw a similar answer that PurePaths are used for manipulation, but
I didn't fully understand as I am still manipulated this path and it
isn't referencing a real file on host where I am defining the variable
(what ever host is running DTS that is). This is a good distinction
though to use Paths when it is used as a local path somewhere.

>
> >
> >
> >   def expand_range(range_str: str) -> list[int]:
>