[libvirt] [test-API][PATCH 2/2] Add case for testing spice compression options

[Osier Yang]

On 2011年09月29日 17:30, Nan Zhang wrote:
> * repos/domain/spice_options.py: add this test for BZ#682237
> ---
>   repos/domain/spice_options.py |  138 +++++++++++++++++++++++++++++++++++++++++
>   1 files changed, 138 insertions(+), 0 deletions(-)
>   create mode 100644 repos/domain/spice_options.py
>
> diff --git a/repos/domain/spice_options.py b/repos/domain/spice_options.py
> new file mode 100644
> index 0000000..7e0b027
> --- /dev/null
> +++ b/repos/domain/spice_options.py
> @@ -0,0 +1,138 @@
> +#!/usr/bin/env python
> +"""For configuring spice compression options testing
> +   domain:spice_options
> +       guestname
> +           xxx
> +       image
> +           auto_glz|auto_lz|quic|glz|lz|off
> +       jpeg
> +           auto|never|always
> +       zlib
> +           auto|never|always
> +       playback
> +           on|off
> +"""

Wondering if the spice graphic testing should be packed with
vnc graphic testing together, i.e. A single API for graphics
testing, so we don't have to expose more and more APIs for
testing conf in future.

> +
> +__author__ = 'Nan Zhang: nzhang at redhat.com'
> +__date__ = 'Thu Sep 8, 2011'
> +__version__ = '0.1.0'
> +__credits__ = 'Copyright (C) 2011 Red Hat, Inc.'
> +__all__ = ['usage', 'spice_config']
> +
> +import os
> +import re
> +import sys
> +from xml.dom import minidom
> +
> +def append_path(path):
> +    """Append root path of package"""
> +    if path in sys.path:
> +        pass
> +    else:
> +        sys.path.append(path)
> +
> +pwd = os.getcwd()
> +result = re.search('(.*)libvirt-test-API', pwd)
> +append_path(result.group(0))
> +
> +from lib import connectAPI
> +from lib import domainAPI
> +from utils.Python import utils
> +from utils.Python import xmlbuilder
> +from exception import LibvirtAPI
> +
> +def check_params(params):
> +    """Verify inputing parameter dictionary"""
> +    logger = params['logger']
> +    keys = ['guestname', 'image', 'jpeg', 'zlib', 'playback']
> +    for key in keys:
> +        if key not in params:
> +            logger.error("%s is required" %key)
> +            return 1
> +    return 0
> +
> +def spice_options(params):
> +    """check spice options"""
> +    # Initiate and check parameters
> +    params_check_result = check_params(params)
> +    if params_check_result:
> +        return 1
> +    logger = params['logger']
> +    guestname = params['guestname']
> +    image = params['image']
> +    jpeg = params['jpeg']
> +    zlib = params['zlib']
> +    playback = params['playback']
> +
> +    # Connect to local hypervisor connection URI
> +    util = utils.Utils()
> +    uri = util.get_uri('127.0.0.1')
> +    conn = connectAPI.ConnectAPI()
> +    virconn = conn.open(uri)
> +
> +    caps = conn.get_caps()
> +    logger.debug(caps)
> +
> +    # Get domain xml
> +    domobj = domainAPI.DomainAPI(virconn)
> +    try:
> +        if guestname not in domobj.get_defined_list():
> +            logger.error("%s doesn't exist or in running state." % guestname)
> +            return 1
> +    except LibvirtAPI, e:
> +        logger.error("API error message: %s, error code is %s" %
> +                     (e.response()['message'], e.response()['code']))
> +        return 1
> +
> +    guestxml = domobj.get_xml_desc(guestname)
> +    guestobj = minidom.parseString(guestxml)
> +    xmlobj = xmlbuilder.XmlBuilder()
> +    xmlobj.add_graphics(params, guestobj)
> +    guestxml = xmlobj.build_domain(guestobj)
> +
> +    try:
> +        domobj.undefine(guestname)

You don't need the undefine() here actually, a define on an existed
domain will just re-define the domain config.

> +        domobj.define(guestxml)

I even think you shouldn't invoke define(), start() here, what you
need to do is just generating the graphic XML and insert it into
the domain xml. Other left work should be done by other public APIs
we already have. And after that, a specific checking API (if
it's need) should be exposed as public API. and thus you use that
in testing conf.

Writing cases like so is destroying the design pricinple of test-API.

> +        domobj.start(guestname)
> +    except LibvirtAPI, e:
> +        logger.error("API error message: %s, error code is %s" %
> +                     (e.response()['message'], e.response()['code']))
> +        return 1
> +
> +    xmlobj = domobj.get_xml_desc(guestname)
> +    prexml = xmlobj.split('\n')
> +    postxml = ''
> +    for i in range(len(prexml)):
> +        postxml = postxml + prexml[i].lstrip()
> +    domxml = minidom.parseString(postxml)
> +
> +    # Check spice options in 'graphcis' tag
> +    graphTag = domxml.getElementsByTagName("graphics")
> +    try:
> +        try:
> +            for graphTag in domxml.getElementsByTagName("graphics"):
> +                assert len(graphTag.childNodes) == 4
> +                assert graphTag.childNodes[0].getAttribute("compression") \
> +                                                 == params['image']
> +                assert graphTag.childNodes[1].getAttribute("compression") \
> +                                                 == params['jpeg']
> +                assert graphTag.childNodes[2].getAttribute("compression") \
> +                                                 == params['zlib']
> +                assert graphTag.childNodes[3].getAttribute("compression") \
> +                                                 == params['playback']

This is an aditional constraints which is not in libvirt's domain XML.
With this codes, the first child node must be image? and likewise for
other child nodes. It just assumes libvirt will always outputs in the
fixed sequence for this child nodes, though indeed libvirt unlikely
change the sequence for campatibility, it still not a good idea.

> +        except AssertionError:
> +            logger.error("Wrong checks happend on spice options.")
> +            conn.close()
> +            logger.info("closed hypervisor connection")
> +            return 1
> +    finally:
> +        logger.info("spice options were checked successfully.")
> +        conn.close()
> +        logger.info("closed hypervisor connection")
> +
> +    return 0
> +
> +def spice_options_clean():
> +    """Clean testing environment"""
> +    pass
> +

So, IMHO the right way to go is:

1) A single public API for graphic testing, regardless of what the
graphic type is. This allow us have a unified API without considering
future graphic types.

2) A public API for accepting the configurations from user, and generate
the graphics XML.

3) A public API for inserting XML into the domain XML. As far as I
known, there is still no API for inserting XML. Per in the testing
world, you will want to insert XML from time to time, a common API
will definitely save the life.

4) your checking method as a public API, without introducing the
additional constraints.

In other words, the pricinple is to keep every API's work independant
enough. And thus you don't have to care about how the user will write
the testing conf.

This is what we do for virsh-rail too. Though it might be big effort
to do like so in test-API currently, it already went some distance
away from the original design.

Regards,
Osier