From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 50F97A00C2; Wed, 2 Nov 2022 13:59:19 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id F0D3540223; Wed, 2 Nov 2022 13:59:18 +0100 (CET) Received: from mail-pj1-f48.google.com (mail-pj1-f48.google.com [209.85.216.48]) by mails.dpdk.org (Postfix) with ESMTP id DF89840041 for ; Wed, 2 Nov 2022 13:59:17 +0100 (CET) Received: by mail-pj1-f48.google.com with SMTP id q1-20020a17090a750100b002139ec1e999so1848820pjk.1 for ; Wed, 02 Nov 2022 05:59:17 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=iol.unh.edu; s=unh-iol; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=uzKH/I6m3cz3qv/1dcvDupLalpKGBukf/j5tladVsVA=; b=P5l5fx34r5cUPfGe8XG4Fc7HHeD2qfVKO7ujdm9NMQfQQHAO7O2xmWRQIGu7d8oL6l ZJ/baXiQ9nMl6cU7Y5A1vNjGeg+SLaa6Hz45puA+U/dU+tWZ4KVVQ95Cu5/mbLxtpBEx dpWQSnil2E+amxRROCVvEFTqnamZj4/s7dZAk= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=uzKH/I6m3cz3qv/1dcvDupLalpKGBukf/j5tladVsVA=; b=u3uRCjLqWJRe7ILgvRJAiKe5Aua0uYvDjsmmytCBzjDnAGFhSS0PpRTJxFuv8GdoCY RnFTZqptriH8rb2v5mBQR7cc5Jd4HZkvhjy9VGcktRzzsbw05wvYeLGO40Eu9XxJDgIq KFs10zdzrK2r+DVxfatg4WvA+F9amDYbPvqW1iNrvIMyKOFDCICdyBPIwNTwkWMnZmkL 51Z5wadv0Z4jdwFFMM+REY0N3CdTXsuq0QGFTbqqE0aYc/+FhTiM4McLH0S2xSoNZT4L jkj2ENjvEZr1/D2B0BOUHtKU51V9B42DQjVywxtI84KMpKltlpDqJj+oC5x4+3qRNqX2 udSg== X-Gm-Message-State: ACrzQf2zjvT6E6kxWequlDZJLPk4OstWyuc2JenE19+EnyUDVCU0zROk tQfTFXa4Wb7SX6kCuav7Ih38OpKhb2EuW5fKoJx6Bg== X-Google-Smtp-Source: AMsMyM54ZShLNTDk0B2x3ny3iGMaXqkibfwdVEYWzRw5bIJIKy68i+js0XTYJlmmZ6mSTlgrrP5rLdx0CuyoNz7JvDs= X-Received: by 2002:a17:902:e1ca:b0:186:878e:3b03 with SMTP id t10-20020a170902e1ca00b00186878e3b03mr25149392pla.95.1667393957004; Wed, 02 Nov 2022 05:59:17 -0700 (PDT) MIME-Version: 1.0 References: <20221013103517.3443997-1-juraj.linkes@pantheon.tech> <5711158.zQ0Gbyo6oJ@thomas> In-Reply-To: <5711158.zQ0Gbyo6oJ@thomas> From: Owen Hilyard Date: Wed, 2 Nov 2022 08:58:41 -0400 Message-ID: Subject: Re: [PATCH v6 00/10] dts: ssh connection to a node To: Thomas Monjalon Cc: =?UTF-8?Q?Juraj_Linke=C5=A1?= , Honnappa.Nagarahalli@arm.com, lijuan.tu@intel.com, kda@semihalf.com, bruce.richardson@intel.com, dev@dpdk.org Content-Type: multipart/alternative; boundary="000000000000821c8305ec7c6815" X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org --000000000000821c8305ec7c6815 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Mon, Oct 31, 2022 at 3:01 PM Thomas Monjalon 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=C5=A1: > > 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). Wha= t > > 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. > 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. > > 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 =3D "^3.10" pexpect =3D "^4.8.0" warlock =3D "^2.0.1" PyYAML =3D "^6.0" types-PyYAML =3D "^6.0.8" scapy =3D "^2.4.5" > > > dts/pyproject.toml | 55 +++ > > 27 files changed, 1723 insertions(+), 4 deletions(-) > > > > --000000000000821c8305ec7c6815 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
On Mon, Oct 31, 2022 at 3:01 PM Thomas Mo= njalon <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=C5=A1:
> All the necessary code needed to connect to a node in a topology with<= br> > 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<= br> > 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 conso= le
> saying where DTS connected.
>
> There's only a bit of documentation, as there's not much to do= cument.
> 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). Wh= at
> will be documented later is runtime dependencies and how to set up the= DTS
> control node environment.
>
[...]
>=C2=A0 .editorconfig=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0|=C2= =A0 =C2=A02 +-
>=C2=A0 .gitignore=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 |= =C2=A0 =C2=A09 +-

Updating general Python guidelines in these files
should be done separately to get broader agreement.

>=C2=A0 MAINTAINERS=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0|= =C2=A0 =C2=A05 +

You can update this file in the first patch.

>=C2=A0 devtools/python-checkpatch.sh=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0|=C2=A0 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 quali= ty requirements for DTS from the start. The current formatting requirements= are "whatever these tools=C2=A0run in this order with these settings = results in", since the working group decided that fully automated rect= ification of minor issues would help new contributors focus on more importa= nt things. I agree that integrating it into existing checkpatch would be go= od, but I think that to do that we would first need to run the tool over ex= isting DPDK python code. python-checkpatch does do linting like the main ch= eckpatch, but it will also perform source code changes to automatically fix= many formatting issues. Do we want to keep checkpatch as a read-only scrip= t 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 c= heckpatch inside of a python virtual environment since DTS is currently ver= y specific about what versions are used (via both version number and crypto= graphic 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 t= he version in the Debian Buster repos is old enough that it does not suppor= t 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.

=
>=C2=A0 devtools= /python-format.sh=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0|=C2=A0 54 +++
>=C2=A0 devtools/python-lint.sh=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0|=C2=A0 26 ++

Let's postpone the integration of these tools.
We need to discuss what is specific to DTS or not.

>=C2=A0 doc/guides/contributing/coding_style.rst=C2=A0 =C2=A0 =C2=A0 |= =C2=A0 =C2=A04 +-

It is not specific to DTS.

>=C2=A0 dts/.devcontainer/devcontainer.json=C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0|=C2=A0 30 ++
>=C2=A0 dts/Dockerfile=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 |=C2=A0 39 += +

Not sure about Docker tied to some personal choices.
<= br>
The reason we are using docker is that it provides a canonica= l environment to run the test harness in, which can then connect to the SUT= and traffic generator. It enforces isolation between these three component= s, and ensures that everyone is using the exact same environment to test an= d deploy the test harness. Although devcontainer started in Visual Studio C= ode, 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 enviro= nment before they can start working. The very recent version of python (3.1= 0)=C2=A0we choose to use for additional type system verification makes not = using containers much harder. It also means that new dependencies, new depe= ndency versions or additional system dependencies can be easily handled by = the normal patch process and rolled out to everyone in a mostly automated f= ashion.=C2=A0

Additionally, although it is named Dockerfile, it is f= ully compatible with podman.=C2=A0
=C2=A0
>=C2=A0 dts/README.md=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0| 154 = ++++++++

As said above, it should in RST format in doc/guides/dts/

>=C2=A0 dts/conf.yaml=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0|=C2= =A0 =C2=A06 +
>=C2=A0 dts/framework/__init__.py=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0|=C2=A0 =C2=A04 +
>=C2=A0 dts/framework/config/__init__.py=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 | 100 +++++
>=C2=A0 dts/framework/config/conf_yaml_schema.json=C2=A0 =C2=A0 |=C2=A0 = 65 ++++
>=C2=A0 dts/framework/dts.py=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 |=C2=A0 68 ++++
>=C2=A0 dts/framework/exception.py=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 |=C2=A0 57 +++
>=C2=A0 dts/framework/logger.py=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0| 114 ++++++
>=C2=A0 dts/framework/remote_session/__init__.py=C2=A0 =C2=A0 =C2=A0 |= =C2=A0 15 +
>=C2=A0 .../remote_session/remote_session.py=C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 | 100 +++++
>=C2=A0 dts/framework/remote_session/ssh_session.py=C2=A0 =C2=A0| 185 ++= +++++++
>=C2=A0 dts/framework/settings.py=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0| 119 ++++++
>=C2=A0 dts/framework/testbed_model/__init__.py=C2=A0 =C2=A0 =C2=A0 =C2= =A0|=C2=A0 =C2=A08 +
>=C2=A0 dts/framework/testbed_model/node.py=C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0|=C2=A0 63 ++++
>=C2=A0 dts/framework/utils.py=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 |=C2=A0 31 ++
>=C2=A0 dts/main.py=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0|= =C2=A0 24 ++
>=C2=A0 dts/poetry.lock=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0| 351 +++++++= +++++++++++

A lot of dependencies look not useful in this first series for SSH connecti= on.

I've put the actual dependency = list below. The lock file contains the entire dependency tree, of which Sca= py is responsible for a substantial portion. pexpect is currently used beca= use of the large amount of code from the old dts that was moved over, but i= t will be replaced by a better solution soon with a rewrite of the dts/fram= ework/remote_session module. Warlock is used to handle json schema checking= , which is how we provide instant feedback about an incorrect config file t= o users (as opposed to old DTS, where you would be informed by a stack trac= e at an arbitrary point in the program). PyYAML is used to parse the yaml c= onfig file, and types-PyYAML is used to provide information to the python t= ype checker about pyyaml. Scapy provides all of the packet manipulation cap= abilities in DTS, and I was not able to find a better library for doing pac= ket manipulation in python, so in my eyes we can excuse its dependency tree= due to how much time it will save.=C2=A0

[too= l.poetry.dependencies]
python =3D "^3.10"
pexpect =3D "= ;^4.8.0"
warlock =3D "^2.0.1"
PyYAML =3D "^6.0&qu= ot;
types-PyYAML =3D "^6.0.8"
scapy =3D "^2.4.5"<= br>
=C2=A0

>=C2=A0 dts/pyproject.toml=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 |=C2=A0 55 +++
>=C2=A0 27 files changed, 1723 insertions(+), 4 deletions(-)



--000000000000821c8305ec7c6815--