From mboxrd@z Thu Jan 1 00:00:00 1970 From: Michael Goldish Subject: Re: [PATCH] [KVM_Autotest] Added functionality to the preprocessor to run scripts Date: Fri, 22 May 2009 07:58:12 -0400 (EDT) Message-ID: <1844402991.226901242993492364.JavaMail.root@zmail05.collab.prod.int.phx2.redhat.com> References: <1222268607.226581242993156722.JavaMail.root@zmail05.collab.prod.int.phx2.redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Cc: kvm@vger.kernel.org To: David Huff Return-path: Received: from mx1.redhat.com ([66.187.233.31]:38963 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753848AbZEVL6M (ORCPT ); Fri, 22 May 2009 07:58:12 -0400 Received: from int-mx1.corp.redhat.com (int-mx1.corp.redhat.com [172.16.52.254]) by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id n4MBwEQK006654 for ; Fri, 22 May 2009 07:58:14 -0400 In-Reply-To: <1222268607.226581242993156722.JavaMail.root@zmail05.collab.prod.int.phx2.redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: Looks good to me. See some comments below. ----- "David Huff" wrote: > This patch will run pre and post scripts > defined in config file with the parameter pre_command > and post_command post_command. > > Also exports all the prameters in preprocess for passing > arguments to the script. Why not do this for post_command as well? > --- > client/tests/kvm_runtest_2/kvm_preprocessing.py | 31 > +++++++++++++++++++++- > 1 files changed, 29 insertions(+), 2 deletions(-) > > diff --git a/client/tests/kvm_runtest_2/kvm_preprocessing.py > b/client/tests/kvm_runtest_2/kvm_preprocessing.py > index c9eb35d..02df615 100644 > --- a/client/tests/kvm_runtest_2/kvm_preprocessing.py > +++ b/client/tests/kvm_runtest_2/kvm_preprocessing.py > @@ -135,8 +135,7 @@ def postprocess_vm(test, params, env, name): > "Waiting for VM to kill itself..."): > kvm_log.debug("'kill_vm' specified; killing VM...") > vm.destroy(gracefully = params.get("kill_vm_gracefully") == > "yes") > - > - > + I hate to be petty, but we usually keep two blank lines between top level functions. Also, you have some trailing whitespace there... > def process(test, params, env, image_func, vm_func): > """Pre- or post-process VMs and images according to the > instructions in params. > > @@ -169,6 +168,7 @@ def preprocess(test, params, env): > params -- a dict containing all VM and image parameters > env -- the environment (a dict-like object) > > + Also, runs any setup command defined in the parameter > pre_command > Also, collect some host information, such as the KVM version. > """ > # Verify the identities of all living VMs > @@ -192,6 +192,22 @@ def preprocess(test, params, env): > vm.destroy() > del env[key] > > + #execute any pre_commands > + pre_command = params.get("pre_command") > + if pre_command: > + # export environment vars > + for k in params.keys(): > + kvm_log.info("Adding KVM_TEST_%s to Environment" % (k)) > + os.putenv("KVM_TEST_%s" % (k), str(params[k])) > + > + # execute command > + kvm_log.info("Executing command '%s'..." % pre_command) > + (status, pid, output) = kvm_utils.run_bg("cd %s; %s" % > (test.bindir, pre_command), > + None, kvm_log.debug, > "(pre_command) ", timeout=600) It wouldn't hurt to make this timeout user-configurable with a default value of 600 or so: timeout = int(params.get("pre_commmand_timeout", "600")) (status, pid, output) = kvm_utils.run_bg(..., timeout=timeout) We can also do that in a separate patch. > + if status != 0: > + kvm_utils.safe_kill(pid, signal.SIGTERM) > + raise error.TestError, "Custom processing pre_command > failed" > + > # Preprocess all VMs and images > process(test, params, env, preprocess_image, preprocess_vm) > > @@ -232,6 +248,8 @@ def postprocess(test, params, env): > test -- an Autotest test object > params -- a dict containing all VM and image parameters > env -- the environment (a dict-like object) > + > + Also, runs any command defined in the parameter post_command > """ > process(test, params, env, postprocess_image, postprocess_vm) > > @@ -241,6 +259,15 @@ def postprocess(test, params, env): > kvm_log.debug("'keep_ppm_files' not specified; removing all > PPM files from results dir...") > kvm_utils.run_bg("rm -vf %s" % os.path.join(test.debugdir, > "*.ppm"), None, kvm_log.debug, "(rm) ", timeout=5.0) > > + #execute any post_commands > + post_command = params.get("post_command") > + if post_command: > + kvm_log.info("Executing command '%s'..." % post_command) > + (status, pid, output) = kvm_utils.run_bg("cd %s; %s" % > (test.bindir, post_command), > + None, kvm_log.debug, > "(post_command) ", timeout=600) > + if status != 0: > + kvm_utils.safe_kill(pid, signal.SIGTERM) > + raise error.TestError, "Custom processing command > failed" Same for post_command. > def postprocess_on_error(test, params, env): > """Perform postprocessing operations required only if the test > failed. Thanks, Michael