| Message ID | 20210415215141.1865467-5-crosa@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Tests: introduce custom jobs | expand |
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.
On Thu, Apr 15, 2021 at 6:52 PM Cleber Rosa <crosa@redhat.com> 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) I agree with Phil that a generic function would be reusable when needed, but as it is not needed yet, this looks good to me: Reviewed-by: Willian Rampazzo <willianr@redhat.com>
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.
Hi, On 4/15/21 6: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) My concern is about that cancellation actually covering up a real bug. Cleber, have you seen the test failing on CI? - Wainer > self.assert_migration(source_vm, dest_vm) > > def _get_free_port(self):
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) def _get_free_port(self):
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(-)