[Libvirt-cim] [PATCH] [TEST] Add tc to verify VSMS.RemoveResourceSettings() with correct resource

[Kaitlin Rupert]

yunguol at cn.ibm.com wrote:
> # HG changeset patch
> # User Guolian Yun <yunguol at cn.ibm.com>
> # Date 1236759196 25200
> # Node ID 0e350cf7bbab15c07d948976435ff4fe1b4ab597
> # Parent  a78b6f23ebaa8a38fa591e420d742aa03cd9e515
> [TEST] Add tc to verify VSMS.RemoveResourceSettings() with correct resource
> 
> 
> This tc verify disk and network devices now, follow up patch will verify input and graphics devices
> 
> Tested for KVM/Xen/XenFV with current sources and rpm
> 
> Signed-off-by: Guolian Yun<yunguol at cn.ibm.com>
> 
> diff -r a78b6f23ebaa -r 0e350cf7bbab suites/libvirt-cim/cimtest/VirtualSystemManagementService/16_removeresource.py
> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> +++ b/suites/libvirt-cim/cimtest/VirtualSystemManagementService/16_removeresource.py	Wed Mar 11 01:13:16 2009 -0700
> @@ -0,0 +1,109 @@
> +#!/usr/bin/python
> +#
> +# Copyright 2008 IBM Corp.

Change date to 2009.

> +
> +import sys
> +import pywbem

Redundant import - this can be removed.

> +import random
> +from pywbem.cim_obj import CIMInstanceName
> +from XenKvmLib import vsms

Instead of importing the whole module, just import get_vsms_class().

> +from XenKvmLib import vxml

Instead of importing the whole module, just import get_class().

> +from XenKvmLib.classes import get_typed_class
> +from CimTest.Globals import logger
> +from XenKvmLib.const import do_main, get_provider_version, default_network_name
> +from CimTest.ReturnCodes import FAIL, PASS
> +from XenKvmLib.common_util import create_netpool_conf, destroy_netpool
> +from XenKvmLib.xm_virt_util import get_bridge_from_network_xml
> +
> +sup_types = ['Xen', 'KVM', 'XenFV']
> +default_dom = 'domain'
> +libvirt_cim_dev_xml = 828

This variable is a bit oddly named.  Can you use something like 
rem_res_err_rev?  Or something that is more descriptive of the actual 
changeset?

> +ntype = 'network'
> +nmac = '00:11:22:33:44:55'
> +npool_name = default_network_name + str(random.randint(1, 100))
> +
> + at do_main(sup_types)
> +def main():
> +    options = main.options
> +
> +    if options.virt == 'KVM':
> +        nddev = 'hdb'
> +    else:
> +        nddev = 'xvdb'
> +
> +    cxml = vxml.get_class(options.virt)(default_dom, disk=nddev, mac=nmac)
> +    ret = cxml.cim_define(options.ip)
> +    if not ret:
> +        logger.error("Failed to define the dom: %s", default_dom)
> +        return FAIL
> +
> +    drasd = get_typed_class(options.virt, 'DiskResourceAllocationSettingData')
> +    drasd_id = '%s/%s' % (default_dom, nddev)
> +    dkeys = {'InstanceID' : drasd_id}
> +   
> +    nrasd = get_typed_class(options.virt, 'NetResourceAllocationSettingData')
> +    nrasd_id = '%s/%s' % (default_dom, nmac)
> +    nkeys = {'InstanceID' : nrasd_id}

Instead of hand building the instances for the resources, use the proper 
association calls to get the RASD instances.   You know the guest, so 
you can use SystemDevice to get the resource instance, and then 
SettingsDefineState to get the RASD instances.

> +
> +    status, net_name = create_netpool_conf(options.ip, options.virt,
> +                                           use_existing=False,
> +                                           net_name=npool_name)

Why are you creating a network?  The guest is created using the default 
network that already exists.

> +    if status != PASS:
> +        logger.error('Unable to create network pool')
> +        return FAIL
> +
> +    dresource = CIMInstanceName(drasd, keybindings = dkeys)
> +    nresource = CIMInstanceName(nrasd, keybindings = nkeys)
> +
> +    service = vsms.get_vsms_class(options.virt)(options.ip) 
> +    try:
> +        service.RemoveResourceSettings(ResourceSettings=[dresource, nresource])
> +    except Exception, details:
> +        logger.error('Failed to remove % or %s', dresource, nresource)
> +        logger.error(details)
> +        cxml.undefine(options.ip)
> +        destroy_netpool(options.ip, options.virt, net_name)
> +        return FAIL
> +    
> +    cxml.dumpxml(options.ip) 
> +    disk = cxml.get_value_xpath('/domain/devices/disk/target/@dev[. = "%s"]'
> +                                % nddev)
> +    if options.virt == 'KVM':
> +        net = cxml.get_value_xpath(
> +              '/domain/devices/interface/source/@network[. = "%s"]' % nmac)
> +    else:
> +        br = get_bridge_from_network_xml(net_name, options.ip, options.virt)
> +        net = cxml.get_value_xpath(
> +              '/domain/devices/interface/source/@bridge[. = "%s"]' % br)

You are checking for the name of the pool you created - this isn't the 
same pool that was used to define the guest.

> +
> +    curr_cim_rev, changeset = get_provider_version(options.virt, options.ip) 
> +    if disk != None and net != None and curr_cim_rev >= libvirt_cim_dev_xml:

This is actually a regression - the RemoveResources() call did work 
prior to changeset 779.

So, you'll want to return an XFAIL if the revision is: 779 <= rev <= 828.

Instead of checking disk and net in one if, break it up so that you can 
return an error message for each resource that fails:

     if disk != None:
         logger.error('The disk device was not removed successfully')
     if net != None:
         logger.error('The network device was not removed successfully')

> +        logger.error('The resource is not removerd successfully')
> +        cxml.undefine(options.ip)
> +        destroy_netpool(options.ip, options.virt, net_name)
> +        return FAIL
> +
> +    cxml.undefine(options.ip)
> +    destroy_netpool(options.ip, options.virt, net_name)
> +    return PASS
> +

I would use exceptions so that you only need to call undefine() once.

-- 
Kaitlin Rupert
IBM Linux Technology Center
kaitlin at linux.vnet.ibm.com