[libvirt] [test-API PATCH 5/6] Cleanup and fix of domain/define test
Martin Kletzander
mkletzan at redhat.com
Tue Mar 27 07:57:40 UTC 2012
On 03/26/2012 03:02 PM, Guannan Ren wrote:
> On 03/25/2012 01:42 AM, Martin Kletzander wrote:
>> I added support for 'uri' parameter and moved some functions into
>> util/Python/utils.py in order not to lose them from the code and keep
>> them accessible for other tests.
>> ---
>> repos/domain/define.py | 132
>> ++++++++----------------------------------------
>> utils/Python/utils.py | 45 ++++++++++++++++-
>> 2 files changed, 65 insertions(+), 112 deletions(-)
>>
>> diff --git a/repos/domain/define.py b/repos/domain/define.py
>> index 8f0095a..25630c5 100644
>> --- a/repos/domain/define.py
>> +++ b/repos/domain/define.py
>> @@ -19,7 +19,7 @@
>> __author__ = 'Alex Jia: ajia at redhat.com'
>> __date__ = 'Mon Jan 28, 2010'
>> __version__ = '0.1.0'
>> -__credits__ = 'Copyright (C) 2009 Red Hat, Inc.'
>> +__credits__ = 'Copyright (C) 2009, 2012 Red Hat, Inc.'
>> __all__ = ['usage', 'check_define_domain', 'define']
>>
>> import os
>> @@ -63,9 +63,6 @@ def usage():
>> macaddr
>> ifacetype
>> source
>> - target_machine
>> - username
>> - password
>> '''
>>
>> def check_params(params):
>> @@ -107,58 +104,17 @@ def ssh_keygen(logger):
>>
>> return 0
>>
>> -def ssh_tunnel(hostname, username, password, logger):
>> - """setup a tunnel to a give host"""
>> - logger.info("setup ssh tunnel with host %s" % hostname)
>> - user_host = "%s@%s" % (username, hostname)
>> - child = pexpect.spawn(SSH_COPY_ID, [ user_host])
>> - while True:
>> - index = child.expect(['yes\/no', 'password: ',
>> - pexpect.EOF,
>> - pexpect.TIMEOUT])
>> - if index == 0:
>> - child.sendline("yes")
>> - elif index == 1:
>> - child.sendline(password)
>> - elif index == 2:
>> - logger.debug(string.strip(child.before))
>> - child.close()
>> - return 0
>> - elif index == 3:
>> - logger.error("setup tunnel timeout")
>> - logger.debug(string.strip(child.before))
>> - child.close()
>> - return 1
>> -
>> - return 0
>> -
>> -def check_define_domain(guestname, guesttype, target_machine,
>> username, \
>> - password, util, logger):
>> - """Check define domain result, if define domain is successful,
>> - guestname.xml will exist under /etc/libvirt/qemu/
>> - and can use virt-xml-validate tool to check the file validity
>> +def check_define_domain(conn, guestname, logger):
>> + """Check define domain result. To make this work on all
>> + hypervisors and with all configuration posibilities, use the
>> + default way through libvirt to check if the guest was defined
>> """
>> - if "kvm" in guesttype:
>> - path = "/etc/libvirt/qemu/%s.xml" % guestname
>> - elif "xen" in guesttype:
>> - path = "/etc/xen/%s" % guestname
>> - else:
>> - logger.error("unknown guest type")
>> -
>> - if target_machine:
>> - cmd = "ls %s" % path
>> - ret, output = util.remote_exec_pexpect(target_machine,
>> username, \
>> - password, cmd)
>> - if ret:
>> - logger.error("guest %s xml file doesn't exsits" % guestname)
>> - return False
>> - else:
>> - return True
>> - else:
>> - if os.access(path, os.R_OK):
>> - return True
>> - else:
>> - return False
>> + try:
>> + conn.lookupByName(guestname + 'asdf')
>
> the testing code?
>
Yes, sorry =) I needed to test if this fails etc. and didn't remove it.
>> + return True
>> + except libvirtError, e:
>> + logger.error(e.message())
>> + return False
>
> It's better not to use libvirt API to check the result of one
> another API.
> We should use native approach to do the checking. So I insist
> on the original checking.
>
There was no lookupByName function, but I agree it's better to use the
same approach, so I'll add this method to the connection API.
One more question, whilst on this subject, though. I still didn't get
why we encapsulate libvirt API into one more class where we don't make
use of any persistence, inheritance nor any other OOP approach. It would
help me to understand so I don't make future mistakes.
>>
>> def define(params):
>> """Define a domain from xml"""
>> @@ -169,41 +125,10 @@ def define(params):
>> logger = params['logger']
>> guestname = params['guestname']
>> guesttype = params['guesttype']
>> + uri = params['uri']
>
> If uri is not None, we need to get the IP or hostname of
> target machine from the uri
> Use that IP or hostname to perform ssh operation.
>
I dropped the part of the code that connected to the machine by ssh in
order to check for the domain to be created. It was very limited and in
case any other tests needs the IP of the target machine, there are 2
ways it can get it:
1) have another parameter for the address (as the number of tests that
could make use of the target IP address will be minimal, I'd prefer this
option)
2) implement URI parsing the same way as it is done in libvirt code
(libxml2.xmlParseURI or something like that)
I think this is still better than having hardcoded '127.0.0.1' in the
tests =)
>> test_result = False
>>
>> - if params.has_key('target_machine'):
>> - logger.info("define domain on remote host")
>> - target_machine = params['target_machine']
>> - username = params['username']
>> - password = params['password']
>> - else:
>> - logger.info("define domain on local host")
>> - target_machine = None
>> - username = None
>> - password = None
>> -
>> # Connect to hypervisor connection URI
>> - util = utils.Utils()
>> - if target_machine:
>> - uri = util.get_uri(target_machine)
>> -
>> - #generate ssh key pair
>> - ret = ssh_keygen(logger)
>> - if ret:
>> - logger.error("failed to generate RSA key")
>> - return 1
>> - #setup ssh tunnel with target machine
>> - ret = ssh_tunnel(target_machine, username, password, logger)
>> - if ret:
>> - logger.error("faild to setup ssh tunnel with target
>> machine %s" % \
>> - target_machine)
>> - return 1
>> -
>> - commands.getstatusoutput("ssh-add")
>> -
>> - else:
>> - uri = util.get_uri('127.0.0.1')
>> -
>> conn = connectAPI.ConnectAPI()
>> virconn = conn.open(uri)
>>
>> @@ -222,35 +147,20 @@ def define(params):
>>
>> # Define domain from xml
>> try:
>> - try:
>> - dom_obj.define(dom_xml)
>> - if check_define_domain(guestname, guesttype,
>> target_machine, \
>> - username, password, util, logger):
>> - logger.info("define a domain form xml is successful")
>> - test_result = True
>> - else:
>> - logger.error("fail to check define domain")
>> - test_result = False
>> - except LibvirtAPI, e:
>> - logger.error("fail to define a domain from xml")
>> + dom_obj.define(dom_xml)
>> + if check_define_domain(virconn, guestname, logger):
>> + logger.info("define a domain form xml is successful")
>> + test_result = True
>> + else:
>> + logger.error("failed to check define domain")
>> test_result = False
>> + except LibvirtAPI, e:
>> + logger.error("failed to define a domain from xml")
>> + test_result = False
>> finally:
>> conn.close()
>> logger.info("closed hypervisor connection")
>
> try ... except ...finally is new syntax since 2.5,
> But we need to support 2.4, so should use
> try:
> try:
> except:
> finally
>
>
>>
>> - if target_machine:
>> - REMOVE_SSH = "ssh %s \"rm -rf /root/.ssh/*\"" % (target_machine)
>> - logger.info("remove ssh key on remote machine")
>> - status, ret = util.exec_cmd(REMOVE_SSH, shell=True)
>> - if status:
>> - logger.error("failed to remove ssh key")
>> -
>> - REMOVE_LOCAL_SSH = "rm -rf /root/.ssh/*"
>
> Never remove or change the local ssh directory like this.
> The autotest have ssh configuration file stored here.
>
>
>> - logger.info("remove local ssh key")
>> - status, ret = util.exec_cmd(REMOVE_LOCAL_SSH, shell=True)
>> - if status:
>> - logger.error("failed to remove local ssh key")
>> -
>> if test_result:
>> return 0
>> else:
>> diff --git a/utils/Python/utils.py b/utils/Python/utils.py
>> index 55c1cb5..7382abb 100644
>> --- a/utils/Python/utils.py
>> +++ b/utils/Python/utils.py
>> @@ -1,6 +1,6 @@
>> #!/usr/bin/env python
>> #
>> -# libvirt-test-API is copyright 2010 Red Hat, Inc.
>> +# libvirt-test-API is copyright 2010, 2012 Red Hat, Inc.
>> #
>> # libvirt-test-API is free software: you can redistribute it and/or
>> modify it
>> # under the terms of the GNU General Public License as published by
>> @@ -433,3 +433,46 @@ class Utils(object):
>> return 1
>>
>> return 0
>> +
>> +
>> + def ssh_keygen(logger):
>
> I agree with this, the migrate.py use this too.
> could you help clean migrate.py along with this together?
> If we put this in utils.py, It's better not to accept
> "logger" argument just like other utilities do
>
> If we create a ssh-keygen and ssh_tunnel as a standalone
> testcase. we will use the connect
> for all of following testcases rather than setup a ssh
> connection in each case. This will be good.
>
>> + """using pexpect to generate RSA"""
>> + SSH_KEYGEN = "ssh-keygen -t rsa"
>> + SSH_COPY_ID = "ssh-copy-id"
>> +
>> + logger.info("generate ssh RSA \"%s\"" % SSH_KEYGEN)
>> + child = pexpect.spawn(SSH_KEYGEN)
>> + while True:
>> + index = child.expect(['Enter file in which to save the
>> key ',
>> + 'Enter passphrase ',
>> + 'Enter same passphrase again: ',
>> + pexpect.EOF,
>> + pexpect.TIMEOUT])
>> + if index == 0:
>> + child.sendline("\r")
>> + elif index == 1:
>> + child.sendline("\r")
>> + elif index == 2:
>> + child.sendline("\r")
>> + elif index == 3:
>> + logger.debug(string.strip(child.before))
>> + child.close()
>> + return 0
>> + elif index == 4:
>> + logger.error("ssh_keygen timeout")
>> + logger.debug(string.strip(child.before))
>> + child.close()
>> + return 1
>> +
>> + return 0
>> +
>> + def ssh_remove_keys(host, logger):
>
> same as above
>
>> + """remove ssh keys on remote machine"""
>> + REMOVE_SSH = "ssh %s \"rm -rf /root/.ssh/*\"" % (target_machine)
>> + logger.info("remove ssh key on remote machine")
>> + ret, dummy = self.exec_cmd(REMOVE_SSH)
>> + if ret:
>> + logger.error("failed to remove ssh key")
>> + return 1
>> +
>> + return 0
>
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
More information about the libvir-list
mailing list