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

Osier Yang jyang at redhat.com
Wed Apr 25 03:35:24 UTC 2012


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?

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

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

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




More information about the libvir-list mailing list