[Kimchi-devel] [PATCH 3/3] (WIP) CDROM Management: Devices management model implementation
Rodrigo Trujillo
rodrigo.trujillo at linux.vnet.ibm.com
Mon Feb 10 22:30:21 UTC 2014
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 at 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 at ovirt.org
> http://lists.ovirt.org/mailman/listinfo/kimchi-devel
>
More information about the Kimchi-devel
mailing list