Re: [PATCH 0/3] Launch other test during migration

From: Michael Goldish <mgoldish@redhat.com>
To: Jason Wang <jasowang@redhat.com>
Cc: autotest@test.kernel.org, kvm@vger.kernel.org, mst@redhat.com
Subject: Re: [PATCH 0/3] Launch other test during migration
Date: Wed, 03 Nov 2010 11:08:11 +0200	[thread overview]
Message-ID: <4CD1267B.7070106@redhat.com> (raw)
In-Reply-To: <19664.52928.45775.15354@gargle.gargle.HOWL>

On 11/03/2010 04:53 AM, Jason Wang wrote:
> Michael Goldish writes:
>  > On 11/02/2010 07:34 AM, Jason Wang wrote:
>  > > Michael Goldish writes:
>  > >  > On 09/25/2010 11:36 AM, Jason Wang wrote:
>  > >  > > We could give a further test of migration by launch test during migartion. So
>  > >  > > the following series implements:
>  > >  > > 
>  > >  > > - A simple class to run a specified test in the background which could be used
>  > >  > > to launch other test during migartion. Its design is rather simple and its usage
>  > >  > > is a little tricky, but it work well.
>  > >  > > - Two sample tests which take advantages of the background class: Test reboot
>  > >  > > during guest migration and test file_transfer during guest migration.
>  > >  > > 
>  > >  > > In the future, we could even lauch autotest client test during guest migation.
>  > >  > > 
>  > >  > > ---
>  > >  > > 
>  > >  > > Jason Wang (3):
>  > >  > >       KVM Test: Introduce a helper class to run a test in the background
>  > >  > >       KVM test: Test reboot during migration
>  > >  > >       KVM test: Test the file transfer during migartion
>  > >  > > 
>  > >  > > 
>  > >  > >  client/tests/kvm/kvm_test_utils.py                 |   44 +++++++++++++++
>  > >  > >  .../kvm/tests/migration_with_file_transfer.py      |   59 ++++++++++++++++++++
>  > >  > >  client/tests/kvm/tests/migration_with_reboot.py    |   45 +++++++++++++++
>  > >  > >  client/tests/kvm/tests_base.cfg.sample             |   12 ++++
>  > >  > >  4 files changed, 159 insertions(+), 1 deletions(-)
>  > >  > >  create mode 100644 client/tests/kvm/tests/migration_with_file_transfer.py
>  > >  > >  create mode 100644 client/tests/kvm/tests/migration_with_reboot.py
>  > >  > 
>  > >  > It seems to me that this method is only applicable to tests/functions
>  > >  > that don't require a VM object (i.e. that require only a shell session
>  > >  > object).  kvm_test_utils.reboot() operates on a VM object, and the same
>  > >  > VM is destroyed by migrate() which runs in the background, so eventually
>  > >  > reboot() tries logging into a destroyed VM, which fails because
>  > >  > vm.get_address() fails.  Any monitor operation will also fail.  If the
>  > >  > autotest wrapper requires a VM object (currently it does) then it can't
>  > >  > be used either.
>  > >  > 
>  > > 
>  > > You are right and that's an issue when running test in parallel with
>  > > migration, but reboot through shell should work. The aim of this kind
>  > > of test is just to add some stress ( such as run_autotest) during
>  > > migartion, so most (probably all) of the tests only needs a
>  > > session. So I think it's not a very big issue in this situation.
>  > 
>  > Many tests need to be modified in order to require only a session.  I've
>  > tried reboot and it doesn't work, and I can see that run_autotest() also
>  > uses a VM.  Reboot needs the VM object in order to log in to make sure
>  > the VM goes back up, and run_autotest() needs it for SCP and probably
>  > is_alive().  I agree that some tests don't require a VM object, but the
>  > ones that do are also interesting.
>  > 
> 
> Looks like every test need a VM object and we could not expect the
> execution order among autotest threads, so if the thread created by
> BackgroundTest was executed after migration, it would always fail.
> 
> I know re-use the VM object after migration involves lots of
> modification, but is that possible or valuable?

I don't think it's worth it.  A qemu process waiting for migration has a
monitor of its own, so it needs to have an instance id of its own.  I
suppose we can refactor everything to get around that somehow, but it'll
be messy.

Another option is to pass env and the string 'vm1' to functions, instead
of passing the VM object itself.  migrate() replaces 'vm1' in env, so
whenever the test accesses env['vm1'] it'll get the right VM.  However,
it's still not safe to use the monitor of a VM that's going to be
destroyed and replaced, and if the monitor can't be used safely,
allowing access to the VM is a bit unclean IMO.

> And I think we can just focus on the tests which is useful to run in
> parallel with migration. Your suggestions looks good but it only works
> with the tests which have a step to wait for something like test
> results. For the tests who does not have such step, another method is
> needed.

I think most interesting tests have some sort of loop or waiting step in
them.  For run_autotest(), we should insert the calls to migrate() in a
loop that waits for the test to complete (i.e. waits for the shell
prompt to return).  The only case I can think of where another method
might be needed is the file transfer test, where the waiting loop is
deep inside some internal function.  However, even if we use
BackgroundTest for file transfer, it's still not nice to pass the VM
object (by calling vm.copy_files_to()) knowing that the same object will
be destroyed soon.

>  > >  > An alternative (somewhat ugly) way to migrate in the background is to
>  > >  > pass a boolean 'migrate' flag to various functions/tests, such as
>  > >  > reboot() and run_autotest().  If 'migrate' is True, these functions will
>  > >  > do something like
>  > >  > 
>  > >  > vm = kvm_test_utils.migrate(vm, ...)
>  > >  > 
>  > >  > in their waiting loops, where wait_for() is normally used.  This
>  > >  > guarantees that 'vm' is always a valid VM object.  For example:
>  > >  > 
>  > >  > # Log in after reboot
>  > >  > while time.time() < end_time:
>  > >  >     if migrate_in_bg:
>  > >  >         vm = kvm_test_utils.migrate(vm, ...)
>  > >  >     session = vm.remote_login()
>  > >  >     if session:
>  > >  >         break
>  > >  >     time.sleep(2)
>  > >  > 
>  > >  > This is much longer than the usual wait_for(...) but it does the job.
>  > >  > What do you think?
>  > > 
>  > > This makes sense but it would let testcases need to care about the
>  > > migration and it's also hard to put all related codes into a wrapper
>  > > which would complicate the codes.
>  > > 
>  > > Despite the issue of vm object, all tests that only depends on the
>  > > shell session should works well with my method and it's more easy to
>  > 
>  > We should also find a solution for tests that require a VM object.
>  > 
>  > Which other tests do you think it would be interesting to run during
>  > migration, in addition to reboot(), run_autotest() and
>  > file_transfer()?
> 
> The most important test is run_autotst and maybe run_ping() and other
> network related test.
> 
>  > 
>  > > be extended. Maybe we could just warn the user of its usage and adapt
>  > > my method?
>  > 
>  > I think it's cleaner to just avoid passing a VM object to BackgroundTest.
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html