Re: [PATCH][KVM-AUTOTEST] Add custom install option for kvm_install

[Mike Burns <mburns@redhat.com>]

Uri Lublin wrote:
> On 05/12/2009 06:34 PM, Mike Burns wrote:
>> From: Michael Burns<mburns@redhat.com>
>>
>>
>> Signed-off-by: Michael Burns<mburns@redhat.com>
>> ---
>>   client/tests/kvm_runtest_2/control        |   18 +++++++++++++++++-
>>   client/tests/kvm_runtest_2/kvm_install.py |   15 +++++++++++++++
>>   2 files changed, 32 insertions(+), 1 deletions(-)
>>
>> diff --git a/client/tests/kvm_runtest_2/control
>> b/client/tests/kvm_runtest_2/control
>> index fd68e94..d6e26bc 100644
>> --- a/client/tests/kvm_runtest_2/control
>> +++ b/client/tests/kvm_runtest_2/control
>> @@ -41,6 +41,19 @@ link_if_not_exist(pwd, qemu_img, 'qemu-img')
>>
>>   # ---------------------
>>   # Build and install kvm
>> +#
>> +# Details of Install options
>> +#   Mode: custom
>> +#   Description:  install from custom install script
>> +#   Parameters needed:
>> +#     install_script:
>> +#       location of script relative to the kvm-runtest_2 directory.
>> +#       Script will be executed from test.bindir (generally
>> kvm_runtest_2)
>> +#       parameters for the script can be passed either as
>> environment variables
>> +#       in the params array below or in the definition of
>> install_script.
>> +#       If they are passed as part of params, then they will be
>> accessible as
>> +#       KVM_INSTALL_<s>  in the OS Environment when your script runs.
>> +#
>
> I think this, being the only explanation about kvm_install, can be
> confusing to the user. We can add a link to the wiki instead:
> http://kvm.et.redhat.com/page/KVM-Autotest/ControlFile
That is a good point.  I was debating whether to put them all there or
not, but didn't want to clutter up my patch too much.  I decided
afterwards to add the page to the wiki, so I'll link there instead when
I repost.
>
>>   # ---------------------
>>   params = {
>>       "name": "kvm_install",
>> @@ -57,7 +70,10 @@ params = {
>>
>>       ## Install from git
>>       "git_repo":
>> 'git://git.kernel.org/pub/scm/linux/kernel/git/avi/kvm.git',
>> -    "user_git_repo":
>> 'git://git.kernel.org/pub/scm/linux/kernel/git/avi/kvm-userspace.git'
>> +    "user_git_repo":
>> 'git://git.kernel.org/pub/scm/linux/kernel/git/avi/kvm-userspace.git',
>> +
>> +    ## Custom install
>> +    "install_script": 'custom_kvm_install.sh param1'
>>   }
>>
>>   # Comment the job.run_test line if you do not want to install kvm
>> on the host.
>> diff --git a/client/tests/kvm_runtest_2/kvm_install.py
>> b/client/tests/kvm_runtest_2/kvm_install.py
>> index 8be5a93..234c77a 100755
>> --- a/client/tests/kvm_runtest_2/kvm_install.py
>> +++ b/client/tests/kvm_runtest_2/kvm_install.py
>> @@ -77,6 +77,21 @@ 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(test.bindir,install_script)
>
> This line (script = ..) should be located below the following if
> statement.
> if "install_script" is not in params (and install_script is None),
> os.path.join fails.
>
Ok, will fix
>> +        if not install_script:
>> +            message = "Custom script filename not specified"
>> +            kvm_log.error(message)
>> +            raise error.TestError, message
>> +        for k in params.keys():
>
> Fix white-space.
>
>> +      kvm_log.info("Adding KVM_INSTALL_%s to Environment" % (k))
>
> kvm_log.debug
>
>> +          os.putenv("KVM_INSTALL_%s" % (k), str(params[k]))
>> +    kvm_log.info("Running " + script + " to install kvm")
>> +        os.system("cd %s; %s" % (test.bindir, script))
>
> if the script fails, quit (raise).
>
we're going to change this to utils.system instead to use the built-in
error handling.
>> +    kvm_log.info("Completed %s" % (script))
>> +
>>       # invalid installation mode
>>       else:
>>           message = "Invalid installation mode: '%s'" % install_mode
>
>
>
> Sorry for the late reply,
>     Uri.