On Thu, Jun 18, 2020 at 05:15:53PM +0200, Adrian Moreno wrote:
Thanks for your feedback Ondrej.
Comments below.
On 6/18/20 12:21 PM, Ondrej Lichtner wrote:
> On Mon, Jun 15, 2020 at 06:22:53PM +0200, Adrian Moreno wrote:
For now, I would not like to pay the maintenance price of another project to be
honest :)
Same here, hence why I said it's an "issue" :).
> Also, at this point the lnst.Tests.TRex module doesn't have
a very big
> dependency on LNST - it uses a couple of parameters for type checking
> and defines a module interface API, which is just that there's a
"run"
> method, but you still kept that in the standalone library. I guess this
> is your main point for possible future extensions?
>
Right, the more that can be added to the standalone library, the better because
the development is much easier. However, I must admit it's my lack of expertise
in LNST talking here.
Could we implement tperf using the existing "Tests/Trex.py" without having to
run a full blown lnst-slave? If it's possible without doing too much
LNST-specific things, that would save us the refactoring, the weird
"placement"
and reduce the temptations of creating another project. What do you think?
So... without context I think the _current_ implementation of tperf
could be done without the refactor, the lnst.Test.TRex really doesn't
require a running lnst-slave, it's actually a "reverse dependency" - if
you want to run trex on a slave then you need the lnst.Test.TRex module
- the slave depends on the module, not the other way around.
BUT, with context I think a refactor and split into a "generic" lib is
better for future development.
>
>>
>> Finally, the stream can be modularized into TRex compatible modules so that:
>> - New stream generators can be easily implemented
>> - TRex tools (e.g: stl-sym) can be used for stream generator development
>
> This took me a bit to understand... I was confused by the module
> implementation with a "register" function... I now see that it's
another
> one of TRex weirdnes, I would personally prefer a class based approach
> but I guess this is fine since it follows some "standard"
>
Right. It's a trex thing. We can have another class-based API for LNST but keep
the trex stl one for prototyping. But we might break the latter without knowing.
Nah, I think this is good, reimplementing the lnst stream creation via
the simple TRex module is IMO better as long as it integrates well (and
it looks like it does) with how we want to run if from actual recipes.
And refactoring the lnst.trex module so that it supports the generic
trex modules is good as it just expands the possibilities for lnst.
My original implementation is just flawed because I had a hard time
getting into TRex and this was the easiest way _for me at the time_ to
get it running...
>
>>
>> The current stream generation is modularized as an example: udp_simple
>
> So does this patchset require any other changes to the recipes that
> currently use TRex? or is it drop in? Haven't tested this since I don't
> have a pre-setup dpdk environment...
>
Right now, no change is needed. The default module implements the current
functionality so we're good.
When we add more modules, we can have an optional parameter that can be set by
new tests pointing to the new modules.
cool
>
>>
>> I'm only a sporadic contributor to LNST so sending this RFC to get some
feedback.
>
> I think the only thing I would complain about is the placement of the
> "lnst.TRex" module... I'm not sure I want it at this top level, I
think
> it also contributed to my slower understanding of the RFC. Not exactly
> sure where to put it though. I'll keep thinking about this
>
I agree. I dislike the placement as well.
If you agree with the approach described above (tperf depend on Tests/Trex.py
directly) we could have:
lnst/Tests/Trex.py
lnst/Tests/TRexModules/udp_simple.py
lnst/Tests/TRexModules/...(otherModules).py
test-tools/tperf/tperf
What do you think?
So if keeping the refactor I'm thinking something like:
lnst/Tests/TRex.py for the test module
lnst/Tests/TRexWrap/*.py for the refactored trex wrapper code
lnst/Tests/TRexModules/udp_simple.py sounds good for additional stream
generation modules
Another option could be:
lnst/Tests/TRex/TestModule.py for the test module
lnst/Tests/TRex/*.py for the wrapped stuff, can be multiple
modules or submodules
lnst/Tests/TRex/__init__.py here you can just export the
Client/Server test modules and whatever
else is important - I think that will
still work with the current imports in
Recipes
test-tools/tperf/tperf looks ok in both situations I think.
-Ondrej