Hi Juraj, Thank you for your review!
On 12/01/2024 13:24, Juraj Linkeš wrote:
I have two extra suggestions apart from the comments below: There's a typo inside the "How To Write a Test Suite" section: In that case, nothing will happen when they're is executed. And Mypy is missing from the list of linters in the "DTS Developer Tools" section, could you please add it?
Ack.
Just out of curiosity, is this generated from the schema? It's a pretty neat format, but maintaining it could be a nightmare without a script that would always produce the same format.
I originally found only one tool that would generate rst. The generated output was quite messy though. Only after hacking its few configuration options and the schema, it produced something decent. But as it is only available on pip and it would have to become a DPDK docs requirement to generate them, it wouldn't be acceptable.
Unless someone comes up with a good tool that could match our needs, unfortunately manual work is the only solution for the time being...and I won't deny it took me a bit of time to format it. The only major problem is all the extra whitespaces and the alignment of the columns needed to make sphinx happy. Once most of the work is done though – as it is in this case, changing it from there shouldn't be too bad.
The section names look to be taken from the schema and all of the terminology is taken from the schema. Would it make sense to use YAML terminology here, since people will try to use this information in YAML files? Or maybe explain what we mean by definitions, properties, objects, arrays or maybe some other things which YAML doesn't specify.
I understand your point of view. I had a quick look at the YAML glossary and I think it may be a lot more confusing than using generic software engineering terminology. Also we may not want to be constrained to YAML.
The YAML glossary refers to what we would call dictionary in Python and objects in JSON as mappings. And arrays and lists as sequences. Maybe changing array to list could be a good idea. Objects instead is something I don't personally like either. I took it from the JSON schema to rst generator tool, as I am not sure what fits best here.
I welcome suggestions to change specific terms. On the other hand, I am not so sure about explaining them as it is out of scope. Moreover, the configuration template should cover all of the scenarios, so one can also infer usage from there.
We should update "amount" (uncountable) to something that's countable, such as count or number.
Ack.
Of note here is that some traffic generators (to which the port config also applies) won't be using os_driver_for_dpdk (such as Scapy), but rather os_driver, so the use is broader.
Ack. Thanks for the explanation, makes sense.
The last newline is missing.
Ack.
Let's keep the note that the skip_smoke_tests flag is optional
Ack.
The traffic generator may need this core configuration. However, since it's not required for all traffic generators (such as Scapy), we could just move to the traffic_generator section. That would require some code modifications though, but even the removal of lcores and use_first_core should be addressed in the configuration classes as well. Have you tried running DTS with these changes?
Please correct me if I'm wrong. At the current state of DTS it seems that these two properties are only used with DPDK. If this is correct, it may be misleading to the user to add them for the traffic generator node, hinting that they may make a difference when they don't.
It would definitely makes sense to have dedicated properties for DPDK and for the traffic generators, instead of common ones. But this is possibly more of a concern when and if support for other traffic generators will be added in the future.
The removal of `lcores` and `use_first_core` does not affect the execution as both have default values they fallback to, as seen in
`dts/framework/config/__init__.py#NodeConfiguration.from_dict`. Yes, I tried running DTS and wasn't met by anything unusual. Best, Luca
