Re: [PATCH 2/3] KVM Test: Add function run_autotest_background and wait_autotest_background.

[Michael Goldish <mgoldish@redhat.com>]

On 04/07/2010 11:49 AM, Feng Yang wrote:
> Add function run_autotest_background and wait_autotest_background to
> kvm_test_utils.py.  This two functions is used in ioquit test script.
> 
> Signed-off-by: Feng Yang <fyang@redhat.com>
> ---
>  client/tests/kvm/kvm_test_utils.py |   68 +++++++++++++++++++++++++++++++++++-
>  1 files changed, 67 insertions(+), 1 deletions(-)
> 
> diff --git a/client/tests/kvm/kvm_test_utils.py b/client/tests/kvm/kvm_test_utils.py
> index f512044..2a1054e 100644
> --- a/client/tests/kvm/kvm_test_utils.py
> +++ b/client/tests/kvm/kvm_test_utils.py
> @@ -21,7 +21,7 @@ More specifically:
>  @copyright: 2008-2009 Red Hat Inc.
>  """
>  
> -import time, os, logging, re, commands
> +import time, os, logging, re, commands, sys
>  from autotest_lib.client.common_lib import error
>  from autotest_lib.client.bin import utils
>  import kvm_utils, kvm_vm, kvm_subprocess, scan_results
> @@ -402,3 +402,69 @@ def run_autotest(vm, session, control_path, timeout, test_name, outputdir):
>          result = bad_results[0]
>          raise error.TestFail("Test '%s' ended with %s (reason: '%s')"
>                               % (result[0], result[1], result[3]))
> +
> +
> +def run_autotest_background(vm, session, control_path, timeout, test_name,
> +                                outputdir):
> +    """
> +    Wrapper of run_autotest() and make it run in the background through fork()
> +    and let it run in the child process.
> +    1) Flush the stdio.
> +    2) Build test params which is recevied from arguments and used by
> +       run_autotest()
> +    3) Fork the process and let the run_autotest() run in the child
> +    4) Catch the exception raise by run_autotest() and exit the child with
> +       non-zero return code.
> +    5) If no exception catched, reutrn 0
> +
> +    @param vm: VM object.
> +    @param session: A shell session on the VM provided.
> +    @param control: An autotest control file.
> +    @param timeout: Timeout under which the autotest test must complete.
> +    @param test_name: Autotest client test name.
> +    @param outputdir: Path on host where we should copy the guest autotest
> +            results to.
> +    """
> +
> +    def flush():
> +        sys.stdout.flush()
> +        sys.stderr.flush()
> +
> +    logging.info("Running autotest background ...")
> +    flush()
> +    pid = os.fork()
> +    if pid:
> +        # Parent process
> +        return pid
> +
> +    try:
> +        # Launch autotest
> +        logging.info("child process of run_autotest_background")
> +        run_autotest(vm, session, control_path, timeout, test_name, outputdir)
> +    except error.TestFail, message_fail:
> +        logging.info("[Autotest Background FAIL] %s" % message_fail)
> +        os._exit(1)
> +    except error.TestError, message_error:
> +        logging.info("[Autotest Background ERROR] %s" % message_error)
> +        os._exit(2)
> +    except:
> +        os._exit(3)
> +
> +    logging.info("[Auototest Background GOOD]")
> +    os._exit(0)
> +
> +
> +def wait_autotest_background(pid):
> +    """
> +    Wait for background autotest finish.
> +
> +    @param pid: Pid of the child process executing background autotest
> +    """
> +    logging.info("Waiting for background autotest to finish ...")
> +
> +    (pid, s) = os.waitpid(pid,0)
> +    status = os.WEXITSTATUS(s)
> +    if status != 0:
> +        return False
> +    return True
> +

I think these functions are unnecessary.  IMO forking is not the clean
way of running autotest in the background.  The kvm_shell_session
object, used to run autotest in the guest, by default runs things in the
background (e.g. session.sendline() returns immediately).
run_autotest(), which uses kvm_shell_session, blocks until the autotest
test is done.  So in order to run autotest in the background, we should
modify run_autotest(), or break it up into smaller parts, to make it
nonblocking.  There's no need to implement yet another wrapper.