Re: [PATCH v6 00/10] dts: ssh connection to a node

[Thomas Monjalon <thomas@monjalon.net>]

02/11/2022 13:58, Owen Hilyard:
> On Mon, Oct 31, 2022 at 3:01 PM Thomas Monjalon <thomas@monjalon.net> wrote:
> 
> > I was about to merge this series,
> > and after long thoughts, it deserves a bit more changes.
> > I would like to work with you for a merge in 22.11-rc3.
> >
> > 13/10/2022 12:35, Juraj Linkeš:
> > > All the necessary code needed to connect to a node in a topology with
> > > a bit more, such as basic logging and some extra useful methods.
> >
> > There is also some developer tooling,
> > and some documentation.
> >
> > [...]
> > > There are configuration files with a README that help with setting up
> > > the execution/development environment.
> >
> > I don't want to merge some doc which is not integrated
> > in the doc/ directory.
> > It should be in RST format in doc/guides/dts/
> > I can help with this conversion.
> >
> > > The code only connects to a node. You'll see logs emitted to console
> > > saying where DTS connected.
> > >
> > > There's only a bit of documentation, as there's not much to document.
> > > We'll add some real docs when there's enough functionality to document,
> > > when the HelloWorld testcases is in (point 4 in our roadmap below). What
> > > will be documented later is runtime dependencies and how to set up the
> > DTS
> > > control node environment.
> > >
> > [...]
> > >  .editorconfig                                 |   2 +-
> > >  .gitignore                                    |   9 +-
> >
> > Updating general Python guidelines in these files
> > should be done separately to get broader agreement.
> >
> > >  MAINTAINERS                                   |   5 +
> >
> > You can update this file in the first patch.
> >
> > >  devtools/python-checkpatch.sh                 |  39 ++
> >
> > Let's postpone the integration of checkpatch.
> > It should be integrated with the existing checkpatch.
> >
> 
> We wanted to specifically ensure that all code met the quality requirements
> for DTS from the start. The current formatting requirements are "whatever
> these tools run in this order with these settings results in", since the
> working group decided that fully automated rectification of minor issues
> would help new contributors focus on more important things. I agree that
> integrating it into existing checkpatch would be good, but I think that to
> do that we would first need to run the tool over existing DPDK python code.
> python-checkpatch does do linting like the main checkpatch, but it will
> also perform source code changes to automatically fix many formatting
> issues. Do we want to keep checkpatch as a read-only script and introduce
> another one which makes source-code changes, or merge them together? This
> would also mean that all DPDK developers would need to run checkpatch
> inside of a python virtual environment since DTS is currently very specific
> about what versions are used (via both version number and cryptographic
> hash of the source archive). These versions are newer than what are shipped
> in many stable linux distributions (Ubuntu, Debian, etc), because we want
> the additional capabilities that come with the newer versions and the
> version in the Debian Buster repos is old enough that it does not support
> the version of python that we are using. We were trying to avoid forcing
> all DPDK developers to install a full suite of python tools.

So who is running the script?
My wish is to make it automatically run when calling checkpatch.sh.

> >  devtools/python-format.sh                     |  54 +++
> > >  devtools/python-lint.sh                       |  26 ++
> >
> > Let's postpone the integration of these tools.
> > We need to discuss what is specific to DTS or not.
> >
> > >  doc/guides/contributing/coding_style.rst      |   4 +-
> >
> > It is not specific to DTS.
> >
> > >  dts/.devcontainer/devcontainer.json           |  30 ++
> > >  dts/Dockerfile                                |  39 ++
> >
> > Not sure about Docker tied to some personal choices.
> 
> The reason we are using docker is that it provides a canonical environment
> to run the test harness in, which can then connect to the SUT and traffic
> generator. It enforces isolation between these three components, and
> ensures that everyone is using the exact same environment to test and
> deploy the test harness. Although devcontainer started in Visual Studio
> Code, it is supported by many IDEs at this point and we wanted to encourage
> developers along a path which avoids needing to set up a development
> environment before they can start working. The very recent version of
> python (3.10) we choose to use for additional type system verification
> makes not using containers much harder. It also means that new
> dependencies, new dependency versions or additional system dependencies can
> be easily handled by the normal patch process and rolled out to everyone in
> a mostly automated fashion.
> 
> Additionally, although it is named Dockerfile, it is fully compatible with
> podman.

In general we avoid enforcing specific versions in DPDK.
The idea is to make it usable from any system with good compatibility.
Using a container is more convenient in general, I agree.
I just would like to have the choices here discussed specifically in a separate patch.

> > >  dts/README.md                                 | 154 ++++++++
> >
> > As said above, it should in RST format in doc/guides/dts/
> >
> > >  dts/conf.yaml                                 |   6 +
> > >  dts/framework/__init__.py                     |   4 +
> > >  dts/framework/config/__init__.py              | 100 +++++
> > >  dts/framework/config/conf_yaml_schema.json    |  65 ++++
> > >  dts/framework/dts.py                          |  68 ++++
> > >  dts/framework/exception.py                    |  57 +++
> > >  dts/framework/logger.py                       | 114 ++++++
> > >  dts/framework/remote_session/__init__.py      |  15 +
> > >  .../remote_session/remote_session.py          | 100 +++++
> > >  dts/framework/remote_session/ssh_session.py   | 185 +++++++++
> > >  dts/framework/settings.py                     | 119 ++++++
> > >  dts/framework/testbed_model/__init__.py       |   8 +
> > >  dts/framework/testbed_model/node.py           |  63 ++++
> > >  dts/framework/utils.py                        |  31 ++
> > >  dts/main.py                                   |  24 ++
> > >  dts/poetry.lock                               | 351 ++++++++++++++++++
> >
> > A lot of dependencies look not useful in this first series for SSH
> > connection.
> 
> I've put the actual dependency list below. The lock file contains the
> entire dependency tree, of which Scapy is responsible for a substantial
> portion. pexpect is currently used because of the large amount of code from
> the old dts that was moved over, but it will be replaced by a better
> solution soon with a rewrite of the dts/framework/remote_session module.
> Warlock is used to handle json schema checking, which is how we provide
> instant feedback about an incorrect config file to users (as opposed to old
> DTS, where you would be informed by a stack trace at an arbitrary point in
> the program). PyYAML is used to parse the yaml config file, and
> types-PyYAML is used to provide information to the python type checker
> about pyyaml. Scapy provides all of the packet manipulation capabilities in
> DTS, and I was not able to find a better library for doing packet
> manipulation in python, so in my eyes we can excuse its dependency tree due
> to how much time it will save.
> 
> [tool.poetry.dependencies]
> python = "^3.10"
> pexpect = "^4.8.0"
> warlock = "^2.0.1"
> PyYAML = "^6.0"
> types-PyYAML = "^6.0.8"
> scapy = "^2.4.5"

Scapy is not needed for SSH connection.
Is warlock used in this series?