Re: [PATCH 2/2] KVM test: Run client tests of autotest in parallel with migration

[Michael Goldish <mgoldish@redhat.com>]

On 01/11/2011 07:22 AM, Lucas Meneghel Rodrigues wrote:
> From: Jason Wang <jasowang@redhat.com>
> 
> Run some workload in the background and do the migration would be helpful to
> verfiy the correctness, so this patch use the Thread class to wait for the
> completion of autotest cmd -- "bin/autotest control" and do the migration in
> parallel.
> 
> Changes from v1:
> - Use the new super cool vm.migrate() method
> 
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  client/tests/kvm/kvm_test_utils.py     |   33 +++++++++++++++++++++++++++++--
>  client/tests/kvm/tests/autotest.py     |    5 +++-
>  client/tests/kvm/tests_base.cfg.sample |   11 ++++++++++
>  3 files changed, 45 insertions(+), 4 deletions(-)
> 
> diff --git a/client/tests/kvm/kvm_test_utils.py b/client/tests/kvm/kvm_test_utils.py
> index c1bff29..c9a9b42 100644
> --- a/client/tests/kvm/kvm_test_utils.py
> +++ b/client/tests/kvm/kvm_test_utils.py
> @@ -429,7 +429,7 @@ def get_memory_info(lvms):
>      return meminfo
>  
>  
> -def run_autotest(vm, session, control_path, timeout, outputdir):
> +def run_autotest(vm, session, control_path, timeout, outputdir, params):
>      """
>      Run an autotest control file inside a guest (linux only utility).
>  
> @@ -439,6 +439,9 @@ def run_autotest(vm, session, control_path, timeout, outputdir):
>      @param timeout: Timeout under which the autotest control file must complete.
>      @param outputdir: Path on host where we should copy the guest autotest
>              results to.
> +
> +    The following params is used by the migration
> +    @param params: Test params used in the migration test
>      """
>      def copy_if_hash_differs(vm, local_path, remote_path):
>          """
> @@ -515,6 +518,11 @@ def run_autotest(vm, session, control_path, timeout, outputdir):
>          raise error.TestError("Invalid path to autotest control file: %s" %
>                                control_path)
>  
> +    migrate_background = params.get("migrate_background") == "yes"
> +    if migrate_background:
> +        mig_timeout = float(params.get("mig_timeout", "3600"))
> +        mig_protocol = params.get("migration_protocol", "tcp")

Don't you think it's a cleaner to extract these parameters from params
outside this function (in the autotest wrapper test) and pass them to
this function instead of the params dict?  We only need mig_protocol and
mig_timeout.  Migration will take place if mig_protocol is a nonempty
string.

Alternatively, we can move the contents of this function back to the
test itself, because there's only 1 test using this function, and then
we can look at params directly like all tests do.

>      compressed_autotest_path = "/tmp/autotest.tar.bz2"
>  
>      # To avoid problems, let's make the test use the current AUTODIR
> @@ -551,12 +559,31 @@ def run_autotest(vm, session, control_path, timeout, outputdir):
>      except kvm_subprocess.ShellError:
>          pass
>      try:
> +        bg = None
>          try:
>              logging.info("---------------- Test output ----------------")
> -            session.cmd_output("bin/autotest control", timeout=timeout,
> -                               print_func=logging.info)
> +            if migrate_background:
> +                mig_timeout = float(params.get("mig_timeout", "3600"))
> +                mig_protocol = params.get("migration_protocol", "tcp")
> +
> +                bg = kvm_utils.Thread(session.cmd_output,
> +                                      kwargs={'cmd': "bin/autotest control",
> +                                              'timeout': timeout,
> +                                              'print_func': logging.info})
> +
> +                bg.start()
> +
> +                while bg.is_alive():
> +                    logging.info("Tests is not ended, start a round of"
> +                                 "migration ...")
> +                    vm.migrate(timeout=mig_timeout, protocol=mig_protocol)
> +            else:
> +                session.cmd_output("bin/autotest control", timeout=timeout,
> +                                   print_func=logging.info)
>          finally:
>              logging.info("------------- End of test output ------------")
> +            if migrate_background and bg:
> +                bg.join()
>      except kvm_subprocess.ShellTimeoutError:
>          if vm.is_alive():
>              get_results()
> diff --git a/client/tests/kvm/tests/autotest.py b/client/tests/kvm/tests/autotest.py
> index 0b97b03..37e1b00 100644
> --- a/client/tests/kvm/tests/autotest.py
> +++ b/client/tests/kvm/tests/autotest.py
> @@ -19,8 +19,11 @@ def run_autotest(test, params, env):
>  
>      # Collect test parameters
>      timeout = int(params.get("test_timeout", 300))
> +    migrate = params.get("migrate" , "no") == "yes"
>      control_path = os.path.join(test.bindir, "autotest_control",
>                                  params.get("test_control_file"))
>      outputdir = test.outputdir
>  
> -    kvm_test_utils.run_autotest(vm, session, control_path, timeout, outputdir)
> +    kvm_test_utils.run_autotest(vm, session, control_path, timeout, outputdir,
> +                                params)
> +
> diff --git a/client/tests/kvm/tests_base.cfg.sample b/client/tests/kvm/tests_base.cfg.sample
> index 047b0f3..c5d9ca3 100644
> --- a/client/tests/kvm/tests_base.cfg.sample
> +++ b/client/tests/kvm/tests_base.cfg.sample
> @@ -170,6 +170,17 @@ variants:
>              - with_file_transfer:
>                  iterations = 1
>                  type = migration_with_file_transfer
> +            - with_autotest:
> +                type = autotest
> +                migrate_background = yes
> +                test_timeout = 1800
> +                variants:
> +                    - dbench:
> +                        test_control_file = dbench.control
> +                    - stress:
> +                        test_control_file = stress.control
> +                    - monotonic_time:
> +                        test_control_file = monotonic_time.control
>  
>      - migrate_multi_host:      install setup unattended_install.cdrom
>          type = migration_multi_host