Re: [PATCH 3/3] KVM Test: Add ioquit test case

[Michael Goldish <mgoldish@redhat.com>]

On 04/07/2010 11:49 AM, Feng Yang wrote:
> Signed-off-by: Feng Yang <fyang@redhat.com>
> ---
>  client/tests/kvm/tests/ioquit.py       |   54 ++++++++++++++++++++++++++++++++
>  client/tests/kvm/tests_base.cfg.sample |    4 ++
>  2 files changed, 58 insertions(+), 0 deletions(-)
>  create mode 100644 client/tests/kvm/tests/ioquit.py
> 
> diff --git a/client/tests/kvm/tests/ioquit.py b/client/tests/kvm/tests/ioquit.py
> new file mode 100644
> index 0000000..c75a0e3
> --- /dev/null
> +++ b/client/tests/kvm/tests/ioquit.py
> @@ -0,0 +1,54 @@
> +import logging, time, random, signal, os
> +from autotest_lib.client.common_lib import error
> +import kvm_test_utils, kvm_utils
> +
> +
> +def run_ioquit(test, params, env):
> +    """
> +    Emulate the poweroff under IO workload(dbench so far) using monitor
> +    command 'quit'.
> +
> +    @param test: Kvm test object
> +    @param params: Dictionary with the test parameters.
> +    @param env: Dictionary with test environment.
> +    """

- Can you explain the goal of this test?  Why quit a VM under IO
workload?  What results do you expect?  How can the test ever fail?

- Why is dbench any better for this purpose than dd or some other simple
command?  Using dbench isn't necessarily bad, I'm just curious.

> +    vm = kvm_test_utils.get_living_vm(env, params.get("main_vm"))
> +    session = kvm_test_utils.wait_for_login(vm,
> +                  timeout=int(params.get("login_timeout", 360)))
> +    session2 = kvm_test_utils.wait_for_login(vm,
> +                  timeout=int(params.get("login_timeout", 360)))
> +    def is_autotest_launched():
> +        if session.get_command_status("pgrep autotest") != 0:
> +            logging.debug("Autotest process not found")
> +            return False
> +        return True
> +
> +    test_name = params.get("background_test", "dbench")
> +    control_file = params.get("control_file", "dbench.control")
> +    timeout = int(params.get("test_timeout", 300))
> +    control_path = os.path.join(test.bindir, "autotest_control",
> +                                control_file)
> +    outputdir = test.outputdir
> +
> +    pid = kvm_test_utils.run_autotest_background(vm, session2, control_path,
> +                                                 timeout, test_name,
> +                                                 outputdir)

As mentioned in the other message, I don't think it's necessary to fork
and use run_autotest() in a separate process.  Instead we should modify
run_autotest() to support non-blocking operation (if we need that at all).

> +    if pid < 0:
> +        raise error.TestError("Could not create child process to execute "
> +                              "autotest background")
> +
> +    if kvm_utils.wait_for(is_autotest_launched, 240, 0, 2):
> +        logging.debug("Background autotest successfully")
> +    else:
> +        logging.debug("Background autotest failed, start the test anyway")
> +
> +    time.sleep(100 + random.randrange(0,100))
> +    logging.info("Kill the virtual machine")
> +    vm.process.close()

This will do a 'kill -9' on the qemu process.  Didn't you intend to use
a 'quit'?  To do that, you should use vm.destroy(gracefully=False).

> +    logging.info("Kill the tracking process")
> +    kvm_utils.safe_kill(pid, signal.SIGKILL)
> +    kvm_test_utils.wait_autotest_background(pid)
> +    session.close()
> +    session2.close()
> +
> diff --git a/client/tests/kvm/tests_base.cfg.sample b/client/tests/kvm/tests_base.cfg.sample
> index 9b12fc2..d8530f6 100644
> --- a/client/tests/kvm/tests_base.cfg.sample
> +++ b/client/tests/kvm/tests_base.cfg.sample
> @@ -305,6 +305,10 @@ variants:
>              - ksm_parallel:
>                  ksm_mode = "parallel"
>  
> +    - ioquit:
> +        type = ioquit
> +        control_file = dbench.control.200
> +        background_test = dbench

You should probably add extra_params += " -snapshot" because this test
can break the filesystem.

>      # system_powerdown, system_reset and shutdown *must* be the last ones
>      # defined (in this order), since the effect of such tests can leave
>      # the VM on a bad state.