Re: [PATCH v4 00/10] dts: add hello world testcase

[Bruce Richardson <bruce.richardson@intel.com>]

On Mon, Feb 20, 2023 at 11:13:45AM +0100, Juraj Linkeš wrote:
> Thanks for the comments, Bruce.
> 
> On Fri, Feb 17, 2023 at 6:26 PM Bruce Richardson
> <bruce.richardson@intel.com> wrote:
> >
> > On Mon, Feb 13, 2023 at 04:28:36PM +0100, Juraj Linkeš wrote:
> > > Add code needed to run the HelloWorld testcase which just runs the hello
> > > world dpdk application.
> > >
> > > The patchset currently heavily refactors this original DTS code needed
> > > to run the testcase:
> > > * The whole architecture has been redone into more sensible class
> > >   hierarchy
> > > * DPDK build on the System under Test
> > > * DPDK eal args construction, app running and shutting down
> > > * Optional SUT hugepage memory configuration
> > > * Test runner
> > > * Test results
> > > * TestSuite class
> > > * Test runner parts interfacing with TestSuite
> > > * The HelloWorld testsuite itself
> > >
> > > The code is divided into sub-packages, some of which are divided
> > > further.
> > >
> > > There patch may need to be divided into smaller chunks. If so, proposals
> > > on where exactly to split it would be very helpful.
> > >
> > > v3:
> > > Finished refactoring everything in this patch, with test suite and test
> > > results being the last parts.
> > > Also changed the directory structure. It's now simplified and the
> > > imports look much better.
> > > I've also many many minor changes such as renaming variables here and
> > > there.
> > >
> > > v4:
> > > Made hugepage config optional, users may now specify that in the main
> > > config file.
> > > Removed HelloWorld test plan and incorporated parts of it into the test
> > > suite python file.
> > > Updated documentation.
> > >
> > Hi,
> >
> > just trying this out by reading the docs and trying to follow along. Couple
> > of high-level comments thus far without getting into the patches:
> >
> > * In the "configuring DTS" section, I think it would be good to:
> >    - say that the config file should be named conf.yaml by default. It's in
> >      the next section, but I think it should be called out earlier.
> >    - say that there is a template conf.yaml file in the dts directory
> >      already. On my first reading I actually thought that the sample config
> >      file was dts/framework/config/conf_yaml_schema.json (and I was going
> >      to comment on the name being weird! :-)). Only when I opened it did I
> >      realise my mistake. Therefore, downplan the schema, and put more
> >      emphasis on where to find the simple conf example to start with.
> 
> Good points, I'll rewrite that a bit.
> 
> >    - if hugepage config is now optional, as you say above, remove that from
> >      the sample and the docs.
> >
> 
> The optional part is just that users may choose between DTS either
> configuring the hugepages or not, but hugepages still must be
> configured (if not by DTS, then beforehand). I'll document this a bit
> more, but I'd like to leave it in the sample config with a note saying
> it's optional.
> 
> > * The code thus far seems to imply that you are always going to use root.
> >   When I configured it to log on to bruce@localhost, it timed out waiting
> >   for a prompt, I believe because it was looking for "#" which is the
> >   default only for root prompts.
> >
> 
> True, I'll add this to docs. This will not be a requirement in the
> future though - we want to do passwordless sudo.
> 
> > * When running as root, things progressed further but I hit an error when
> >   DTS was trying to get the CPU config. No idea what is happening here,
> >   because running the same commands manually over ssh seemed to work fine.
> >   Below is the error. Any hints as to what is the problem appreciated.
> >
> 
> I remember running into the same issue as well. I think it's related
> to the bracketed paste feature of some terminal emulators:
> https://askubuntu.com/questions/662222/why-bracketed-paste-mode-is-enabled-sporadically-in-my-terminal-screen
> Please try disabling it and see whether that helps.
> I haven't gone to great lengths to harden this part of SSH
> implementation as we'll be moving to Fabric (from pexpect) after this
> patch (which uses a mature Python SSH implementation instead of
> expect).
> 
Ok, thanks for the explanation, I'll try that out.

/Bruce