From: Bruce Richardson <bruce.richardson@intel.com>
To: "Juraj Linkeš" <juraj.linkes@pantheon.tech>
Cc: Stanislaw Kardach <kda@semihalf.com>,
"thomas@monjalon.net" <thomas@monjalon.net>,
"david.marchand@redhat.com" <david.marchand@redhat.com>,
"ronan.randles@intel.com" <ronan.randles@intel.com>,
"Honnappa.Nagarahalli@arm.com" <Honnappa.Nagarahalli@arm.com>,
"ohilyard@iol.unh.edu" <ohilyard@iol.unh.edu>,
"lijuan.tu@intel.com" <lijuan.tu@intel.com>,
"dev@dpdk.org" <dev@dpdk.org>
Subject: Re: [PATCH v4 4/9] dts: add ssh pexpect library
Date: Fri, 23 Sep 2022 09:15:07 +0100 [thread overview]
Message-ID: <Yy1rC2doQEMTJZH+@bricha3-MOBL.ger.corp.intel.com> (raw)
In-Reply-To: <115446c94a7c4eda877c8a968adce100@pantheon.tech>
On Fri, Sep 23, 2022 at 07:22:26AM +0000, Juraj Linkeš wrote:
>
>
> > -----Original Message-----
> > From: Stanislaw Kardach <kda@semihalf.com>
> > Sent: Thursday, September 22, 2022 4:32 PM
> > To: Juraj Linkeš <juraj.linkes@pantheon.tech>
> > Cc: thomas@monjalon.net; david.marchand@redhat.com;
> > ronan.randles@intel.com; Honnappa.Nagarahalli@arm.com;
> > ohilyard@iol.unh.edu; lijuan.tu@intel.com; dev@dpdk.org
> > Subject: Re: [PATCH v4 4/9] dts: add ssh pexpect library
> >
> > On Thu, Sep 22, 2022 at 09:41:40AM +0000, Juraj Linkeš wrote:
> > > Hi Stanislaw,
> > First a preface. I understand that the DTS rework is sponsored by someone and
> > there may be people waiting with their labs for this job to be done.
> > Everything that I'll write below is from a point of view of a developer who'd like
> > to utilize DTS as a test suite for DPDK when adding support for new PMDs or
> > architectures/boards. This might be in conflict with time-to-market metric at this
> > point in time but I'm more focused on the state of DPDK+DTS in the long run.
> > So feel free to disregard my comments if there are higher priorities.
> > >
> > > Neither of the current DTS maintainer nor me are the author of the code, so
> > we can only speculate as to why certain parts were implemented the way they
> > are.
> > >
> > > We've thought a lot about replacing pexpect with something else, such as
> > Fabric. Fabric is supposedly faster, which is the biggest draw and instead of
> > fixing/reworking pexpect code, it makes sense to switch our focus on Fabric. For
> > this PoC version though, we'd like to stay with this pexpect code and work on
> > other appoaches in the next release cycle. The code is well tested so there's not
> > much point in poking in it if it's to be replaced.
> > I have a nasty experience of code staying without re-factoring for long "because
> > it works". When it comes to DTS my experience is that it works only if used
> > exactly on the setups it was meant for. Adapting it to a new setup, new PMD or
> > "even" running in the cloud shows that parts of it are held together with a string.
> > I'm not blaming DTS devs here. Such approach is often needed for various
> > reasons (usually time-to-market) and it's hard to be forward-compatible.
> >
> > That said I would suggest to use this opportunity to refactor DTS while it's still
> > not merged. Otherwise we'll be left with code that we're uncertain why it works.
> > That's not a quality-first approach and it'll bite us in the backside in the future.
> >
> > Let's do things right, not fast.
>
> Absolutely, but effective time use is also something to consider. Our current plan doesn't won't really have to contend with problems in the future, as we want to add the Farbic implementation in the next release cycle. I'm also working on refactoring the code a bit - I'm adding an abstraction that would allow us to easily replace the pexpect implementation with Fabric (with no impact on DTS behavior - the same APIs will need to be implemented). Also, we'll remove the pexpect implementation once Fabric is in place (unless we can think of a reason for pexpect to stay, in which case we'll need to refactor it). I think that instead of focusing on pexpect we could focus on making sure the replacement won't cause any issues. What do you think?
>
Personally, I would be very keen to get the move of DTS to the main repo
underway, and so I wouldn't look to have too many massive changes required
before we start seeing patches merged in. Basic code cleanup and
refactoring is fine, but I would think that requiring massive changes like
replacing expect with fabric may be too big an ask. After all, the rest of
DPDK is moving on, meaning more and more DTS content is being added to the
separate DTS repo every release, making the job bigger each time. :-(
Tl;dr - I'm ok to leave fabric replacement for a release next year.
/Bruce
next prev parent reply other threads:[~2022-09-23 8:15 UTC|newest]
Thread overview: 105+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-06-22 12:14 [PATCH v1 0/8] dts: ssh connection to a node Juraj Linkeš
2022-06-22 12:14 ` [PATCH v1 1/8] dts: add ssh pexpect library Juraj Linkeš
2022-06-22 12:14 ` [PATCH v1 2/8] dts: add locks for parallel node connections Juraj Linkeš
2022-06-22 12:14 ` [PATCH v1 3/8] dts: add ssh connection extension Juraj Linkeš
2022-06-22 12:14 ` [PATCH v1 4/8] dts: add basic logging facility Juraj Linkeš
2022-06-22 12:14 ` [PATCH v1 5/8] dts: add Node base class Juraj Linkeš
2022-06-22 12:14 ` [PATCH v1 6/8] dts: add config parser module Juraj Linkeš
2022-06-22 12:14 ` [PATCH v1 7/8] dts: add dts runtime workflow module Juraj Linkeš
2022-06-22 12:14 ` [PATCH v1 8/8] dts: add main script for running dts Juraj Linkeš
2022-07-11 14:51 ` [PATCH v2 0/8] ssh connection to a node Juraj Linkeš
2022-07-11 14:51 ` [PATCH v2 1/8] dts: add basic logging facility Juraj Linkeš
2022-07-11 14:51 ` [PATCH v2 2/8] dts: add ssh pexpect library Juraj Linkeš
2022-07-11 14:51 ` [PATCH v2 3/8] dts: add locks for parallel node connections Juraj Linkeš
2022-07-11 14:51 ` [PATCH v2 4/8] dts: add ssh connection extension Juraj Linkeš
2022-07-11 14:51 ` [PATCH v2 5/8] dts: add config parser module Juraj Linkeš
2022-07-11 14:51 ` [PATCH v2 6/8] dts: add Node base class Juraj Linkeš
2022-07-11 14:51 ` [PATCH v2 7/8] dts: add dts workflow module Juraj Linkeš
2022-07-11 14:51 ` [PATCH v2 8/8] dts: add dts executable script Juraj Linkeš
2022-07-28 10:00 ` [PATCH v3 0/9] dts: ssh connection to a node Juraj Linkeš
2022-07-28 10:00 ` [PATCH v3 1/9] dts: add project tools config Juraj Linkeš
2022-07-28 10:00 ` [PATCH v3 2/9] dts: add developer tools Juraj Linkeš
2022-07-28 10:00 ` [PATCH v3 3/9] dts: add basic logging facility Juraj Linkeš
2022-07-28 10:00 ` [PATCH v3 4/9] dts: add ssh pexpect library Juraj Linkeš
2022-07-28 10:00 ` [PATCH v3 5/9] dts: add ssh connection extension Juraj Linkeš
2022-07-28 10:00 ` [PATCH v3 6/9] dts: add config parser module Juraj Linkeš
2022-07-28 10:00 ` [PATCH v3 7/9] dts: add Node base class Juraj Linkeš
2022-07-28 10:00 ` [PATCH v3 8/9] dts: add dts workflow module Juraj Linkeš
2022-07-28 10:00 ` [PATCH v3 9/9] dts: add dts executable script Juraj Linkeš
2022-07-29 10:55 ` [PATCH v4 0/9] dts: ssh connection to a node Juraj Linkeš
2022-07-29 10:55 ` [PATCH v4 1/9] dts: add project tools config Juraj Linkeš
2022-08-10 6:30 ` Tu, Lijuan
2022-09-07 16:16 ` Bruce Richardson
2022-09-09 13:38 ` Juraj Linkeš
2022-09-09 13:52 ` Bruce Richardson
2022-09-09 14:13 ` Juraj Linkeš
2022-09-12 14:06 ` Owen Hilyard
2022-09-12 15:15 ` Bruce Richardson
2022-09-13 12:08 ` Juraj Linkeš
2022-09-13 14:18 ` Bruce Richardson
2022-09-13 19:03 ` Honnappa Nagarahalli
2022-09-13 19:19 ` Honnappa Nagarahalli
2022-09-14 9:37 ` Thomas Monjalon
2022-09-14 12:55 ` Juraj Linkeš
2022-09-14 13:11 ` Bruce Richardson
2022-09-14 14:28 ` Thomas Monjalon
2022-09-21 10:49 ` Juraj Linkeš
2022-09-13 19:11 ` Honnappa Nagarahalli
2022-07-29 10:55 ` [PATCH v4 2/9] dts: add developer tools Juraj Linkeš
2022-08-10 6:30 ` Tu, Lijuan
2022-09-07 16:37 ` Bruce Richardson
2022-09-13 12:38 ` Juraj Linkeš
2022-09-13 20:38 ` Honnappa Nagarahalli
2022-09-14 7:37 ` Bruce Richardson
2022-09-14 12:45 ` Juraj Linkeš
2022-09-14 13:13 ` Bruce Richardson
2022-09-14 14:26 ` Thomas Monjalon
2022-09-14 19:08 ` Honnappa Nagarahalli
2022-09-20 12:14 ` Juraj Linkeš
2022-09-20 12:22 ` Tu, Lijuan
2022-07-29 10:55 ` [PATCH v4 3/9] dts: add basic logging facility Juraj Linkeš
2022-08-10 6:31 ` Tu, Lijuan
2022-09-08 8:31 ` Bruce Richardson
2022-09-13 12:52 ` Juraj Linkeš
2022-09-13 23:31 ` Honnappa Nagarahalli
2022-09-14 12:51 ` Juraj Linkeš
2022-07-29 10:55 ` [PATCH v4 4/9] dts: add ssh pexpect library Juraj Linkeš
2022-08-10 6:31 ` Tu, Lijuan
2022-09-08 9:53 ` Bruce Richardson
2022-09-13 13:36 ` Juraj Linkeš
2022-09-13 14:23 ` Bruce Richardson
2022-09-13 14:59 ` Stanislaw Kardach
2022-09-13 17:23 ` Owen Hilyard
2022-09-14 0:03 ` Honnappa Nagarahalli
2022-09-14 7:42 ` Bruce Richardson
2022-09-14 7:58 ` Stanislaw Kardach
2022-09-14 19:57 ` Honnappa Nagarahalli
2022-09-19 14:21 ` Owen Hilyard
2022-09-20 17:54 ` Honnappa Nagarahalli
2022-09-21 1:01 ` Tu, Lijuan
2022-09-21 5:37 ` Jerin Jacob
2022-09-22 9:03 ` Juraj Linkeš
2022-09-14 9:42 ` Stanislaw Kardach
2022-09-22 9:41 ` Juraj Linkeš
2022-09-22 14:32 ` Stanislaw Kardach
2022-09-23 7:22 ` Juraj Linkeš
2022-09-23 8:15 ` Bruce Richardson [this message]
2022-09-23 10:18 ` Stanislaw Kardach
2022-07-29 10:55 ` [PATCH v4 5/9] dts: add ssh connection extension Juraj Linkeš
2022-08-10 6:32 ` Tu, Lijuan
2022-09-13 17:04 ` Bruce Richardson
2022-09-13 17:32 ` Owen Hilyard
2022-09-14 7:46 ` Bruce Richardson
2022-09-14 12:02 ` Owen Hilyard
2022-09-14 13:15 ` Bruce Richardson
2022-07-29 10:55 ` [PATCH v4 6/9] dts: add config parser module Juraj Linkeš
2022-08-10 6:33 ` Tu, Lijuan
2022-09-13 17:19 ` Bruce Richardson
2022-09-13 17:47 ` Owen Hilyard
2022-09-14 7:48 ` Bruce Richardson
2022-07-29 10:55 ` [PATCH v4 7/9] dts: add Node base class Juraj Linkeš
2022-08-10 6:33 ` Tu, Lijuan
2022-07-29 10:55 ` [PATCH v4 8/9] dts: add dts workflow module Juraj Linkeš
2022-08-10 6:34 ` Tu, Lijuan
2022-07-29 10:55 ` [PATCH v4 9/9] dts: add dts executable script Juraj Linkeš
2022-08-10 6:35 ` Tu, Lijuan
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Yy1rC2doQEMTJZH+@bricha3-MOBL.ger.corp.intel.com \
--to=bruce.richardson@intel.com \
--cc=Honnappa.Nagarahalli@arm.com \
--cc=david.marchand@redhat.com \
--cc=dev@dpdk.org \
--cc=juraj.linkes@pantheon.tech \
--cc=kda@semihalf.com \
--cc=lijuan.tu@intel.com \
--cc=ohilyard@iol.unh.edu \
--cc=ronan.randles@intel.com \
--cc=thomas@monjalon.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).