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

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.

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

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