Re: [Autotest] [Autotest PATCH] KVM-test: Simple stop/continue test

[Lucas Meneghel Rodrigues <lmr@redhat.com>]

On Thu, Apr 21, 2011 at 3:21 AM, Amos Kong <akong@redhat.com> wrote:
> Change guest state by monitor cmd, verify guest status,
> and try to login guest by network.

I don't like the way you're handling human monitor and QMP monitors in
this tests...  comments below:

> Signed-off-by: Jason Wang <jasowang@redhat.com>
> Signed-off-by: Amos Kong <akong@redhat.com>
> ---
>  client/tests/kvm/tests/stop_continue.py |   52 +++++++++++++++++++++++++++++++
>  client/tests/kvm/tests_base.cfg.sample  |    4 ++
>  2 files changed, 56 insertions(+), 0 deletions(-)
>  create mode 100644 client/tests/kvm/tests/stop_continue.py
>
> diff --git a/client/tests/kvm/tests/stop_continue.py b/client/tests/kvm/tests/stop_continue.py
> new file mode 100644
> index 0000000..c7d8025
> --- /dev/null
> +++ b/client/tests/kvm/tests/stop_continue.py
> @@ -0,0 +1,52 @@
> +import logging
> +from autotest_lib.client.common_lib import error
> +
> +
> +def run_stop_continue(test, params, env):
> +    """
> +    Suspend a running Virtual Machine and verify its state.
> +
> +    1) Boot the vm
> +    2) Suspend the vm through stop command
> +    3) Verify the state through info status command
> +    4) Check is the ssh session to guest is still responsive,
> +       if succeed, fail the test.
> +
> +    @param test: Kvm test object
> +    @param params: Dictionary with the test parameters
> +    @param env: Dictionary with test environment.
> +    """
> +    vm = env.get_vm(params["main_vm"])
> +    vm.verify_alive()
> +    timeout = float(params.get("login_timeout", 240))
> +    session = vm.wait_for_login(timeout=timeout)
> +
> +    try:
> +        logging.info("Suspend the virtual machine")
> +        vm.monitor.cmd("stop")
> +
> +        logging.info("Verifying the status of virtual machine through monitor")
> +        o = vm.monitor.info("status")
> +        if 'paused' not in o and ( "u'running': False" not in str(o)):

^ Here, it's not clear what means a paused qmp monitor or a hmp
monitor. this statement is unnecessarily confusing. Here

'paused' not in o -> Is what would define a 'not paused hmp monitor'

"u'running': False" not in str(o) -> This defines a 'not paused qmp monitor'

why we are checking one _and_ the other, as one monitor can't be hmp
and qmp at the same time? It would be at least _or_. And like I said,
it's non trivial to flow this assertion made.

It seems to me that a better (although involving more code changes) approach is:

1) Introduce VM methods is_paused and verify_paused, which would
internally for the kvm vm class, call monitor methods also called
is_paused and verify_paused, with implementations for both hmp and
qmp. verify_paused would throw an exception in case of a failure,
while is_paused would return a boolean.

> +            logging.error(o)
> +            raise error.TestFail("Fail to suspend through monitor command line")
> +
> +        logging.info("Check the session responsiveness")
> +        if session.is_responsive():
> +            raise error.TestFail("Session is still responsive after stop")
> +
> +        logging.info("Try to resume the guest")
> +        vm.monitor.cmd("cont")
> +
> +        o = vm.monitor.info("status")
> +        m_type = params.get("monitor_type", "human")
> +        if ('human' in m_type and 'running' not in o) or\
> +           ('qmp' in m_type and "u'running': True" not in str(o)):

^ same here, we should have is_running and verify_running methods on
VM that would call monitor methods with the same names.

Now, of course I might be really mistaken here, would like to hear
your opinion on that subject.

-- 
Lucas