Re: [PATCH 4/8] tests/acceptance/migration.py: cancel test if migration is not supported

[Cleber Rosa]

On Fri, Apr 16, 2021 at 07:11:04AM +0200, Philippe Mathieu-Daudé wrote:
> On 4/15/21 11:51 PM, Cleber Rosa wrote:
> > FIXME: check if there's a way to query migration support before
> > actually requesting migration.
> > 
> > Some targets/machines contain devices that do not support migration.
> > Let's acknowledge that and cancel the test as early as possible.
> > 
> > Signed-off-by: Cleber Rosa <crosa@redhat.com>
> > ---
> >  tests/acceptance/migration.py | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tests/acceptance/migration.py b/tests/acceptance/migration.py
> > index 792639cb69..25ee55f36a 100644
> > --- a/tests/acceptance/migration.py
> > +++ b/tests/acceptance/migration.py
> > @@ -53,7 +53,11 @@ def do_migrate(self, dest_uri, src_uri=None):
> >          source_vm = self.get_vm()
> >          source_vm.add_args('-nodefaults')
> >          source_vm.launch()
> > -        source_vm.qmp('migrate', uri=src_uri)
> > +        response = source_vm.qmp('migrate', uri=src_uri)
> > +        if 'error' in response:
> > +            if 'desc' in response['error']:
> > +                msg = response['error']['desc']
> > +            self.cancel('Migration does not seem to be supported: %s' % 
> > msg)
> >          self.assert_migration(source_vm, dest_vm)
> 
> It would be better to have this done as a generic check_requisites()
> method. First because we could reuse it (also at the class level),
> second because we could account the time spent for checking separately
> from the time spent for the actual testing.
> 

With regards to separating the time, you suggest that we should
perform the check at the setUp(), and I absolutely agree with the
principle.  But, I wonder if any characteristic of the "vm",
configured during the test (and not available earlier), could affect
its migration capabilities.

Right now we are proposing some "require_*()" methods, such as
require_accelerator("kvm"), because they are checks that, to the best
of my knowlege, do not depend on any further configuration for the vm
instance.

But, your second point, about this being in a method for common use,
is very sound.  IMO the place to put something like you suggest would
be QEMUMachine.  Something like:

   try:
      source_vm.require_migrate()
   except RequirementError as e:
      self.cancel(e)

Ideally, though, one instance of the QEMUMachine used for the checks,
would not be re-used during the test.  The ideal implementation of
QEMUMachine.require_*(), would create a fresh QEMUMachine instance
with user defined characteristics and verify the requirement, leaving
the original instance untouched.

IMO we can pursue that discussion further, while handling this error
condition locally for now.

Thanks,
- Cleber.

signature.asc
Description: PGP signature