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

[Het Gala]

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 ?)

> > 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'
      2. 'simple' does not fit perfect here IMO.
> > 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

> > 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