[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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

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
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
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

logger.info("creating a new folder for customizing custom.iso file in

if os.path.exists(new_dir):
logger.info("the folder exists, remove it")

+ 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

+ 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.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

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

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.


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
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")

- 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):
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
currently, the TESTCASE_check is the name supported by framework to check
something before starting test, if just "check" is better than
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)
- 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.


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]