Re: [PATCH] tests/qtest: Standardize qtest function caller strings.

[Het Gala]

On 05/04/24 7:58 pm, Fabiano Rosas wrote:

> !-------------------------------------------------------------------|
>   CAUTION: External Email
>
> |-------------------------------------------------------------------!
>
> Het Gala <het.gala@nutanix.com> writes:
>
> > On 27/03/24 2:37 am, Fabiano Rosas wrote:
> > > Het Gala<het.gala@nutanix.com>  writes:
> > >
> > > Some comments, mostly just thinking out loud...
> > >
> > > > For <test-type> --> migrate
> > > > /<test-type>/<migration-mode>/<method>/<transport>/<invocation>/
> > > > <compression>/<encryption>/O:<others>/...
> > > >
> > > > For <test-type> --> validate
> > > > /<test-type>/<validate-variable>/O:<transport>/O:<invocation>/
> > > > <validate-test-result>/O:<test-reason>/O:<others>/...
> > > >           
> > > Do we need an optional 'capability' element? I'm not sure how practical
> > > is to leave that as 'others', because that puts it at the end of the
> > > string. We'd want the element that's more important/with more variants
> > > to be towards the start of the string so we can run all tests of the
> > > same kind with the -r option.
> > >         
> > While also looking at different functions for figuring out the transport
> > and invocation, my observation was that, there might be many capabilities
> > added to the same test, while it might not be important also.
> > Ex: /migrate/multifd/tcp/plain
> > 1. multifd is defined as a migration mode.
> > 2. It is also a capability, and comes in 2 parts [multifd, multifd-channels]
> >     though one is a capability and another is parameter
> > Similarly in other examples of compression, there are many capabilities
> > and parameters added, but it might be not important to mention that ?
> >
> > Secondly, there are multiple migration capabilities IIRC (> 15). And a test
> > requiring multiple capabilities, the overall string would be too long, and
> > not that important also to mention all capabilities.
> >
> > Just thinking out of mind - Can we have selective list of capabilities ? 
> > 1. multifd 2. compress (again, there might be confusion with multifd 
> > compression methods like zstd, zlib and just 'compress') 3. zero-page 
> > (This will have sub capabilities ?)
> >       
> I was thinking of keeping that part more open-ended. So not specifying
> capabilities one by one, but more like "if you're testing a capability,
> it comes here".
>
> About multifd, it's a bit special since it cannot be seen as just a
> "feature" anymore. It's a core part of the migration code. I wouldn't
> classify it as capability for the purposes of the tests.
>     
    Ack, got it.

> > > > test-type            :: migrate | validate
> > > >           
> > > We could alternatively drop migration|migrate|validate. They are kind of
> > > superfluous.
> > >         
> > I agree with the above comment. 'migrate' and 'validate' have a different
> > set of variables required, some necessary, while other optional. IMO this
> > will help is in streamlining the design further.
> > > > migration-mode
> > > >    a. migrate -->     :: precopy | postcopy | multifd
> > > >    b. validate -->    :: (what to validate)
> > > > methods              :: preempt | recovery | reboot | suspend | simple
> > > >           
> > >         
> > I want some inputs here.
> > 1. is there a better variable name rather than 'methods'
> >       
> Does this fall into the "mode" terminology that Steven introduced?
>     
    Yes, as we decided that we don't want
      'migration-mode' key-value pair,
      naming 'mode' would be a better term.
      
      In cases, where multiple modes are to be used ex:
      postcopy_preempt_recovery
      I feel it might be a good idea to separate multiple modes by '-'
      For example - .../preempty-recovery/...
    Similarly for other keys too if required 


> > 2. 'simple' does not fit perfect here IMO.
> >       
> Can we go without it?
>     
    You mean omit the key itself in case of a
      no-op ?

> > > > transport            :: tcp | fd | unix | file
> > > > invocation           :: uri | channels | both
> > > > CompressionType      :: zlib | zstd | none
> > > >           
> > > s/none/nocomp/ ? We're already familiar with that.
> > >         
> > Ack. Will change that.
> > > > encryptionType       :: tls | plain
> > > >           
> > > s/plain/notls/ ?
> > >         
> > What if there is another encryption technique in future ?
> > > Or maybe we simply omit the noop options. It would make the string way
> > > shorter in most cases.
> > >         
> > This might be a better approach. Can have some keys/variables as optional
> > while some necessary. For ex: for 'migrate' - transport and invocation
> > might be necessary while it might not be necessary for 'validate' qtests
> >       
> Yep
>     
    Ack, will do that!

> > > > validate-test-result :: success | failure
> > > > others               :: other comments/capability that needs to be
> > > >                          addressed. Can be multiple
> > > >
> > > > (more than one applicable, separated by using '-' in between)
> > > > O: optional
> > > >
> > > > Signed-off-by: Het Gala<het.gala@nutanix.com>
> > > > Suggested-by: Fabiano Rosas<farosas@suse.de>
> > > > ---
> > > >   tests/qtest/migration-test.c | 143 ++++++++++++++++++-----------------
> > > >   1 file changed, 72 insertions(+), 71 deletions(-)
> > > >
> > > > diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
> > > > index bd9f4b9dbb..bf4d000b76 100644
> > > > --- a/tests/qtest/migration-test.c
> > > > +++ b/tests/qtest/migration-test.c
> > > >           
> > >         
> > Regards,
> > Het Gala
> >       
> I'm wondering whether we should leave the existing tests untouched and
> require the new format only for new tests. Going through a git bisection
> with a change in the middle that alters test names would be infuriating.
>     
    Hmm yup. I had this doubt on, how would we be
      enforcing the new design
      for any new qtests that gets added from now on ?
      Can we have this design started for validation tests maybe for
      now, the
      number is low and might get some feedback to improve this ?
      
      
      Regards,
      Het Gala