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. 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 -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list