From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mike Burns Subject: Re: [PATCH][KVM-AUTOTEST] Add custom install option for kvm_install Date: Mon, 11 May 2009 13:52:19 -0400 Message-ID: <4A0865D3.7020500@redhat.com> References: <443364434.369371242063386949.JavaMail.root@zmail05.collab.prod.int.phx2.redhat.com> Reply-To: mburns@redhat.com Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Cc: kvm@vger.kernel.org, Uri Lublin , Eduardo Habkost To: Michael Goldish Return-path: Received: from mx2.redhat.com ([66.187.237.31]:51938 "EHLO mx2.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751409AbZEKRwV (ORCPT ); Mon, 11 May 2009 13:52:21 -0400 Received: from int-mx2.corp.redhat.com (int-mx2.corp.redhat.com [172.16.27.26]) by mx2.redhat.com (8.13.8/8.13.8) with ESMTP id n4BHqMek015667 for ; Mon, 11 May 2009 13:52:22 -0400 In-Reply-To: <443364434.369371242063386949.JavaMail.root@zmail05.collab.prod.int.phx2.redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: Michael Goldish wrote: > ----- "Mike Burns" wrote: > > >> Eduardo Habkost wrote: >> >>> Hi, >>> >>> Excerpts from Michael Burns's message of Fri May 08 15:55:30 -0300 >>> >> 2009: >> >>> >>> >>>> --- a/client/tests/kvm_runtest_2/kvm_install.py >>>> +++ b/client/tests/kvm_runtest_2/kvm_install.py >>>> @@ -77,6 +77,17 @@ def run_kvm_install(test, params, env): >>>> elif install_mode == "localsrc": >>>> __install_kvm(test, srcdir) >>>> >>>> + # install from custom script >>>> + elif install_mode == "custom": >>>> + install_script = params.get("install_script") >>>> + script = >>>> >> os.path.join(os.environ['AUTODIR'],install_script) >> >>>> + if not install_script: >>>> + message = "Custom script filename not specified" >>>> + kvm_log.error(message) >>>> + raise error.TestError, message >>>> + kvm_log.info("Running " + script + " to install kvm") >>>> + os.system(script) >>>> >>>> >>> What if we had some way to pass the other parameters from 'params' >>> >> to >> >>> the custom script? >>> >>> Maybe something like (untested): >>> >>> for k in params.keys(): >>> os.putenv("KVM_INSTALL_%s" % (k), params[k]) >>> >>> Are all values on 'params' guaranteed to be strings, or they can be >>> >> set >> >>> to any python value? In the latter case, we could use >>> >> str(params[k]), or >> >>> export only the string parameters. >>> >>> >> That's a good idea. I'm not sure about whether the params are all >> strings. I'll try it out and respin the patch after. >> > > 1. Not all params are strings -- 'depend' is a list of strings, so str(params[k]) is a good idea. > I put this in the latest post of the patch. I figured it didn't hurt anything being there. > 2. Why not just pass the parameters via the command line, e.g. > install_command = my_script.sh param1 param2 > That would work as well. It could be done either way, but if someone wanted to use something other than bash, then this way might be easier. > 3. Why do you use os.environ['AUTODIR'] instead of test.bindir? As far as I know, $AUTODIR is the 'client' dir, while test.bindir takes you directly to kvm_runtest_2. > A couple reasons... 1. I didn't know about test.bindir 2. I wanted something within the client directory since that is what gets copied when running in server mode. I can see the point in forcing it to be in kvm_runtest_2, but if someone wanted a single location for all custom scripts that do not apply to just kvm_runtest_2, then i could see them creating a directory at the highest level possible. It is easily switched if that is what we want to do instead, but that was my thought. > 4. It may be useful to run the script in the kvm_runtest_2 dir, in case the script wants to create symlinks or anything like that. So the last line could be something like > os.system("cd %s; %s" % (test.bindir, install_script)) > I agree. The script should probably be run from the kvm_runtest_2 directory. In addition, I'm going to be adding a page to the wiki about the control file and what the different things that can be set there are. I'm going to concentrate on the params section first, but eventually we should expand it to include everything. Mike