On 24. 9. 2024 15:57, Jeremy Spewock wrote:
On Tue, Sep 24, 2024 at 5:12 AM Juraj Linkeš <juraj.lin...@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).
Ok, you can add that to the next version.
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.
Yes, we should pick one naming convention to be consistent in this patch
and we can have a broader (framework-wide) look at this in a separate patch.
