[libvirt] [test-API PATCH 4/4] improve and fix testcases found during testcases run

[Osier Yang]

On 2012年04月25日 12:23, Guannan Ren wrote:
> On 04/25/2012 11:35 AM, Osier Yang wrote:
>> On 2012年04月24日 17:40, Guannan Ren wrote:
>>> install_linux_cdrom.py: qemu process couldn't visit custom.iso that
>>> resides in other user's home, so we copy
>>> custom.iso into /tmp for easy use.
>>
>> We need something like /var/cache/libvirt-test-API as a global
>> var in global.cfg.
>>
>>> install_linux_check.py: use hddriver and nicdriver name
>>
>> Why? I guess the old "hdmode" and "nicmode" is not correct to represent
>> the actual argument, but it should be explained.
>>
>>> network/create.py: in the case of the network with 'isolate' type,
>>> we need to remove '<forward mode="NETMODE"/>'line,
>>> The bug is caused by changes on using xml files
>>> network/define.py: the same as network/create.py
>>> repos/snapshot/delete.py: the TESTCASE_check is reserved fuction name
>>> for
>>> framework, so change the name of its internal
>>> 'check' function
>>> ---
>>> repos/domain/install_linux_cdrom.py | 14 +++++++++++---
>>> repos/domain/install_linux_check.py | 6 +++---
>>> repos/network/create.py | 4 ++++
>>> repos/network/define.py | 6 +++++-
>>> repos/snapshot/delete.py | 6 +++---
>>> 5 files changed, 26 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/repos/domain/install_linux_cdrom.py
>>> b/repos/domain/install_linux_cdrom.py
>>> index 60d12a7..17052d4 100644
>>> --- a/repos/domain/install_linux_cdrom.py
>>> +++ b/repos/domain/install_linux_cdrom.py
>>> @@ -47,13 +47,18 @@ def prepare_cdrom(*args):
>>> ks_name = os.path.basename(ks)
>>>
>>> new_dir = os.path.join(HOME_PATH, guestname)
>>> + customeiso_dir = os.path.join('/tmp/libvirt-test-API', guestname)
>>
>> typo, custom. And in anycase, it should use the new global var in
>> global.cfg.
>>
>>> logger.info("creating a new folder for customizing custom.iso file in
>>> it")
>>>
>>> if os.path.exists(new_dir):
>>> logger.info("the folder exists, remove it")
>>> shutil.rmtree(new_dir)
>>>
>>> + if os.path.exists(customeiso_dir):
>>> + shutil.rmtree(customeiso_dir)
>>> +
>>
>> Why the dir should be deleted if exists? shouldn't we reuse it?
>
>
> We download boot.iso first, and add kickstart file into it
> recreate a new iso file named custom.iso. We can not tell it is
> rhel6u1 or rhel6u2 from the custom.iso, so remove it before
> creating a new one.

I mean why not change the utils/ksiso.sh to accept a argument,
and create the custom.iso in the desired target dir, such as
/var/cache/libvirt-test-API/$guest/

>
>
>
>
>>
>>> os.makedirs(new_dir)
>>> + os.makedirs(customeiso_dir)
>>> logger.info("the directory is %s" % new_dir)
>>>
>>> boot_path = os.path.join(ostree, 'images/boot.iso')
>>> @@ -76,8 +81,11 @@ def prepare_cdrom(*args):
>>> (status, text) = commands.getstatusoutput(shell_cmd)
>>>
>>> logger.debug(text)
>>> - logger.info("make custom.iso file, change to original directory: %s" %
>>> - src_path)
>>> +
>>> + logger.info("copy custom.iso to %s" % customeiso_dir)
>>> + shutil.copy('custom.iso', customeiso_dir)
>>> +
>>
>> Unreasonable. Why not create the iso directly in the dir, instead of
>> creating it anoter place, and copying it to the desired place
>> afterwards?
>>
>
> Every time we install a new guest, we create a folder named after the
> name of the guest in root of libvirt-test-API. All of the related temporary
> files will be generated in this folder. The custom.iso is created in this
> folder.

I mean throw all of these stuffs into /var/cache/libvirt-test-API/$guest.

Probably user git clone libvirt-test-API testsuits in his/her home
> directory. The qemu process couldn't visit the custom.iso as the permission
> problem, the code is to copy the custom.iso into /tmp for qemu process.
>
>
>
>
>
>
>>> + logger.info("go back to original directory: %s" % src_path)
>>
>> We should cleanup the logging like this to logger.debug. Nobody will
>> care about how the scripts work in background I think.
>>
>>> os.chdir(src_path)
>>>
>>> def prepare_boot_guest(domobj, xmlstr, guestname, installtype, logger):
>>> @@ -196,7 +204,7 @@ def install_linux_cdrom(params):
>>> logger.info('prepare installation...')
>>>
>>> bootcd = '%s/custom.iso' % \
>>> - (os.path.join(HOME_PATH, guestname))
>>> + (os.path.join('/tmp/libvirt-test-API', guestname))
>>
>> Well, hardcode again.
>>
>>> logger.debug("the bootcd path is %s" % bootcd)
>>> logger.info("begin to customize the custom.iso file")
>>> prepare_cdrom(ostree, ks, guestname, logger)
>>> diff --git a/repos/domain/install_linux_check.py
>>> b/repos/domain/install_linux_check.py
>>> index d034aba..ab1e7db 100644
>>> --- a/repos/domain/install_linux_check.py
>>> +++ b/repos/domain/install_linux_check.py
>>> @@ -15,7 +15,7 @@ from src import sharedmod
>>> from src import env_parser
>>> from utils import utils
>>>
>>> -required_params = ('guestname', 'virt_type', 'hdmodel', 'nicmodel',)
>>> +required_params = ('guestname', 'virt_type', 'hddriver', 'nicdriver',)
>>
>> Finally I see why "hdmodel" and "nicmodel" are changed in 1/4,
>> 1/4 should come after this patch, or either you break the 1/4
>> into several patches, the one about paramas' name changing comes
>> after this patch.
>>
>>> optional_params = {}
>>>
>>> HOME_PATH = os.getcwd()
>>> @@ -71,8 +71,8 @@ def install_linux_check(params):
>>> logger.info("Now checking guest health after installation")
>>>
>>> domain_name=guestname
>>> - blk_type=params['hdmodel']
>>> - nic_type=params['nicmodel']
>>> + blk_type=params['hddriver']
>>> + nic_type=params['nicdriver']
>>> Test_Result = 0
>>>
>>> # Ping guest from host
>>> diff --git a/repos/network/create.py b/repos/network/create.py
>>> index 839e93b..399328c 100644
>>> --- a/repos/network/create.py
>>> +++ b/repos/network/create.py
>>> @@ -39,6 +39,7 @@ def create(params):
>>> """Create a network from xml"""
>>> logger = params['logger']
>>> networkname = params['networkname']
>>> + netmode = params['netmode']
>>> xmlstr = params['xml']
>>>
>>> conn = sharedmod.libvirtobj['conn']
>>> @@ -47,6 +48,9 @@ def create(params):
>>> logger.error("the %s network is running" % networkname)
>>> return 1
>>>
>>> + if netmode == 'isolate':
>>> + xmlstr = re.sub('<forward.*\n', '', xmlstr)
>>> +
>>
>> This looks fine.
>>
>>> logger.debug("%s network xml:\n%s" % (networkname, xmlstr))
>>>
>>> net_num1 = conn.numOfNetworks()
>>> diff --git a/repos/network/define.py b/repos/network/define.py
>>> index 923db29..0c38944 100644
>>> --- a/repos/network/define.py
>>> +++ b/repos/network/define.py
>>> @@ -42,14 +42,18 @@ def define(params):
>>> """Define a network from xml"""
>>> logger = params['logger']
>>> networkname = params['networkname']
>>> + netmode = params['netmode']
>>> xmlstr = params['xml']
>>>
>>> conn = sharedmod.libvirtobj['conn']
>>>
>>> if check_network_define(networkname, logger):
>>> - logger.error("%s network is defined" % networkname)
>>> + logger.error("%s network is defined already" % networkname)
>>
>> "network '%s' is already defined" % networkname
>>
>>> return 1
>>>
>>> + if netmode == 'isolate':
>>> + xmlstr = re.sub('<forward.*\n', '', xmlstr)
>>> +
>>> logger.debug("network xml:\n%s" % xmlstr)
>>>
>>> net_num1 = conn.numOfDefinedNetworks()
>>> diff --git a/repos/snapshot/delete.py b/repos/snapshot/delete.py
>>> index 19689b1..49ce4d9 100644
>>> --- a/repos/snapshot/delete.py
>>> +++ b/repos/snapshot/delete.py
>>> @@ -24,7 +24,7 @@ def check_domain_state(conn, guestname, logger):
>>> else:
>>> return True
>>>
>>> -def delete_check(guestname, snapshotname, expected_flag, logger):
>>> +def delete_checkout(guestname, snapshotname, expected_flag, logger):
>>
>> Not a good name. Still wondering why not use "check" in scripts
>> under repo, and in framework, using "$script.check".
>
>
> I don't think it is a good name either, in v2 it is changed to
> 'delete_checking'
> currently, the TESTCASE_check is the name supported by framework to check
> something before starting test, if just "check" is better than
> "TESTCASE_check"
> we can change it.
>
>
>
>
>>
>>> """ after deleting, check if appropriate xml file exists or not"""
>>> guest_snapshot_dir = os.path.join(SNAPSHOT_DIR, guestname)
>>> snapshot_entries = os.listdir(guest_snapshot_dir)
>>> @@ -54,7 +54,7 @@ def delete(params):
>>> logger.error("checking failed")
>>> return 1
>>>
>>> - if not delete_check(guestname, snapshotname, "exist", logger):
>>> + if not delete_checkout(guestname, snapshotname, "exist", logger):
>>> logger.error("no snapshot %s exists" % snapshotname)
>>> logger.debug("not corresponding xml file in %s" % SNAPSHOT_DIR)
>>> return 1
>>> @@ -64,7 +64,7 @@ def delete(params):
>>> domobj = conn.lookupByName(guestname)
>>> snapobj = domobj.snapshotLookupByName(snapshotname, 0)
>>> snapobj.delete(0)
>>> - if not delete_check(guestname, snapshotname, "noexist", logger):
>>> + if not delete_checkout(guestname, snapshotname, "noexist", logger):
>>> logger.error("after deleting, the corresponding \
>>> xmlfile still exists in %s" % SNAPSHOT_DIR)
>>> return 1
>>
>> V2 is needed.
>>
>> Osier
>