4:30 p.m.
On 02/10/2014 06:11 PM, Aline Manera wrote:
On 02/10/2014 10:52 AM, Rodrigo Trujillo wrote:
> This patch adds the CREATE, LOOKUP, UPDATE and DELETE functionalities
> to guest storage devices support.
>
> Signed-off-by: Rodrigo Trujillo <rodrigo.trujillo(a)linux.vnet.ibm.com>
> ---
> src/kimchi/model/vms.py | 167
> +++++++++++++++++++++++++++++++++++++++++++++++-
> src/kimchi/xmlutils.py | 5 ++
> 2 files changed, 171 insertions(+), 1 deletion(-)
>
> diff --git a/src/kimchi/model/vms.py b/src/kimchi/model/vms.py
> index 4623e28..069ab7c 100644
> --- a/src/kimchi/model/vms.py
> +++ b/src/kimchi/model/vms.py
> @@ -20,8 +20,11 @@
> # License along with this library; if not, write to the Free Software
> # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
> 02110-1301 USA
>
> +import libvirt
> import os
> +import socket
> import time
> +import urlparse
> import uuid
> from xml.etree import ElementTree
>
> @@ -36,7 +39,8 @@ from kimchi.model.config import CapabilitiesModel
> from kimchi.model.templates import TemplateModel
> from kimchi.model.utils import get_vm_name
> from kimchi.screenshot import VMScreenshot
> -from kimchi.utils import run_setfacl_set_attr, template_name_from_uri
> +from kimchi.utils import kimchi_log, run_setfacl_set_attr
> +from kimchi.utils import template_name_from_uri
>
>
> DOM_STATE_MAP = {0: 'nostate',
> @@ -47,6 +51,10 @@ DOM_STATE_MAP = {0: 'nostate',
> 5: 'shutoff',
> 6: 'crashed'}
>
> +DEV_TYPE_SRC_ATTR_MAP = {'file': 'file',
> + 'block': 'dev',
> + 'dir': 'dir'}
> +
> GUESTS_STATS_INTERVAL = 5
> VM_STATIC_UPDATE_PARAMS = {'name': './name'}
> VM_LIVE_UPDATE_PARAMS = {}
> @@ -474,3 +482,160 @@ class LibvirtVMScreenshot(VMScreenshot):
> stream.finish()
> finally:
> os.close(fd)
> +
> +
> +class StoragesModel(object):
> + def __init__(self, **kargs):
> + self.conn = kargs['conn']
> +
> + def _get_storage_xml(self, params):
> + storage = """
> + <disk type='%(src_type)s' device='%(dev_type)s'>
> + <driver name='qemu' type='raw'/>%(source)s
> + <target dev='%(dev)s' bus='ide'/>
> + </disk>
> + """
You can use lxml to build the xml entry
I think there are some code examples upstream
> + info = {}
> + info['dev'] = params.get('dev')
> + info['dev_type'] = params.get('type')
> + info['src_type'] = params.get('src_type')
> +
> + # Working with url paths
> + if info['src_type'] == 'network':
> + net = {}
> + output = urlparse.urlparse(params.get('path'))
> + net['protocol'] = output.scheme
> + net['port'] = output.port or
> socket.getservbyname(output.scheme)
> + net['hostname'] = output.hostname
> + net['url_path'] = output.path
> + info['source'] = """
> + <source protocol='%(protocol)s'
name='%(url_path)s'>
> + <host name='%(hostname)s' port='%(port)s'/>
> + </source>""" % net
> + else:
> + # Fixing source attribute
> + info['source'] = """
> + <source %s='%s'/>""" %
> (DEV_TYPE_SRC_ATTR_MAP[info['src_type']],
> + params.get('path'))
> + return storage % info
> +
> + def create(self, vm_name, params):
> + #TODO: Check if device name is already use
> + path = params.get('path')
> +
> + # Checking if path exist, if not url
> + if path.startswith('/'):
> + if not os.path.exists(path):
> + msg = "Path specified for device is not valide"
> + raise InvalidParameter(msg)
> + elif path.endswith('.iso') or os.path.isfile(path):
> + params['src_type'] = 'file'
> + elif os.path.isdir(path):
> + params['src_type'] = 'dir'
> + else:
> + params['src_type'] = 'block'
> + else:
> + params['src_type'] = 'network'
> +
> + # Add device to VM
> + dev_xml = self._get_storage_xml(params)
> + try:
> + conn = self.conn.get()
> + dom = conn.lookupByName(vm_name)
> + dom.attachDeviceFlags(dev_xml,
> libvirt.VIR_DOMAIN_AFFECT_CURRENT)
> + except Exception as e:
> + msg = 'Was not possible to attach storage device: %s' %
> e.message
> + kimchi_log.error(msg)
> + raise OperationFailed(e.message)
> + return params['dev']
> +
> +
> + def get_list(self, dev):
> + dom = VMModel.get_vm(dev, self.conn)
> + xml = dom.XMLDesc(0)
> + device_xml = xmlutils.xml_get_child(xml, './devices')
> + storages = self._parse_vm_disks(device_xml, ['disk',
'cdrom'])
> + return storages
> +
> + def _parse_vm_disks(self, xml_str, filter_list):
> + # xml_str: xml_str of device with all devices
> + # filter_list: List of which device type to retrieve
> + root = ElementTree.fromstring(xml_str)
> + ret = []
> + for opt in filter_list:
> + xpath = ".disk/[@device='%s']" % opt
> + for disk in root.findall(xpath):
> + name = disk.find('./target').attrib['dev']
> + ret.append(name)
> + return ret
> +
> +
> +class StorageModel(object):
> + def __init__(self, **kargs):
> + self.conn = kargs['conn']
> + self.kargs = kargs
> +
> + def _get_device_xml(self, vm_name, dev_name):
> + # Get VM xml and then devices xml
> + dom = VMModel.get_vm(vm_name, self.conn)
> + device_xml = xmlutils.xml_get_child(dom.XMLDesc(0),
> './devices')
> + dev_root = ElementTree.fromstring(device_xml)
> + xpath = "./disk/target[@dev='%s']/.." % dev_name
> + return dev_root.find(xpath)
why did you do in 2 steps?
xpath = "./devices/disk/target[@dev=%s]" % de
> +
> + def lookup(self, vm_name, dev_name):
> + # Retrieve disk xml and format return dict
> + disk = self._get_device_xml(vm_name, dev_name)
> + if disk is not None:
> + source = disk.find('./source')
> + path = ""
> + if source is not None:
> + src_type = disk.attrib.get('type')
> + if src_type == 'network':
> + host = source.find('./host')
> + path = source.attrib.get('protocol') + '://' +\
> + host.attrib.get('name') + ':' +\
> + host.attrib.get('port') +
> source.attrib.get('name')
> + else:
> + path = source.attrib.get(
> + DEV_TYPE_SRC_ATTR_MAP[src_type])
> + dev_type = disk.attrib['device']
> + return { 'dev': dev_name,
> + 'type': dev_type,
> + 'path': path}
> + else:
> + msg = 'The storage device "%s" does not exist in the
> guest "%s"' % (
> + dev_name,vm_name)
> + raise NotFoundError(msg)
> +
Just a suggestion:
In a if/else block statement, try to put first the smaller block and
return after it
That way we avoid one level indentation in the bigger block
Example:
if disk is None:
reutrn NotFoundError()
source = disk.find('./source')
...
Agree
> + def delete(self, vm_name, dev_name):
> + # Get storage device xml
> + disk = self._get_device_xml(vm_name, dev_name)
> + if disk is None:
> + msg = 'The storage device "%s" does not exist in the
> guest "%s"' % (
> + dev_name,vm_name)
> + raise NotFoundError(msg)
> + try:
> + conn = self.conn.get()
> + dom = conn.lookupByName(vm_name)
> + dom.detachDeviceFlags(ElementTree.tostring(disk),
> + libvirt.VIR_DOMAIN_AFFECT_CURRENT)
> + except Exception as e:
> + msg = 'Was not possible to detach storage device: %s' %
> e.message
> + kimchi_log.error(msg)
> + raise OperationFailed(e.message)
> +
> + def update(self, vm_name, dev_name, params):
> + try:
> + info = self.lookup(vm_name, dev_name)
> + self.delete(vm_name, dev_name)
> + info.update(params)
> + kargs = {'conn': self.conn}
> + stgModel = StoragesModel(**kargs)
> + dev_name = stgModel.create(vm_name, info)
> + return dev_name
> + except Exception as e:
> + raise
> + msg = 'Was not possible to update storage device: %s' %
> e.message
> + kimchi_log.error(msg)
> + raise OperationFailed(e.message)
In case of failure you should recreate the old instance. Otherwise,
the vm will loose a cdrom/disk
I suggest:
try:
# create new storage
except:
# raise error
# delete old storage
I cannot create before delete, because storage devices are
going to have
the same name, and then conflict.
I am going to improve this code.
> diff --git a/src/kimchi/xmlutils.py b/src/kimchi/xmlutils.py
> index 51ff0ec..176ceb1 100644
> --- a/src/kimchi/xmlutils.py
> +++ b/src/kimchi/xmlutils.py
> @@ -40,3 +40,8 @@ def xml_item_update(xml, xpath, value):
> item = root.find(xpath)
> item.text = value
> return ElementTree.tostring(root, encoding="utf-8")
> +
> +def xml_get_child(xml, xpath):
> + root = ElementTree.fromstring(xml)
> + item = root.find(xpath)
> + return ElementTree.tostring(item, encoding="utf-8")
_______________________________________________
Kimchi-devel mailing list
Kimchi-devel(a)ovirt.org
http://lists.ovirt.org/mailman/listinfo/kimchi-devel