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

["Juraj Linkeš" <juraj.linkes@pantheon.tech>]

On 24. 9. 2024 15:57, Jeremy Spewock wrote:
> 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).
> 

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.