Re: [Kimchi-devel] [PATCH v3 3/4] CDROM Management: Devices management model implementation

On 02/12/2014 07:08 AM, Royce Lv wrote: > On 2014?02?12? 10:38, Daniel Barboza wrote: >> From: Rodrigo Trujillo&lt;rodrigo.trujillo(a)linux.vnet.ibm.com&gt; >> >> This patch adds the CREATE, LOOKUP, UPDATE and DELETE functionalities >> to guest storage devices support. >> >> Signed-off-by: Rodrigo Trujillo&lt;rodrigo.trujillo(a)linux.vnet.ibm.com&gt; >> >> Using lxml and objectify to parse the XML info. >> Pep8 adjustments in model/vms.py >> >> Signed-off-by: Daniel Henrique Barboza&lt;danielhb(a)linux.vnet.ibm.com&gt; >> --- >> src/kimchi/model/vms.py | 184 ++++++++++++++++++++++++++++++++++++++++++++++-- >> src/kimchi/xmlutils.py | 5 ++ >> 2 files changed, 185 insertions(+), 4 deletions(-) >> >> diff --git a/src/kimchi/model/vms.py b/src/kimchi/model/vms.py >> index 1244eeb..3c2a661 100644 >> --- a/src/kimchi/model/vms.py >> +++ b/src/kimchi/model/vms.py >> @@ -20,13 +20,17 @@ >> # 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 string >> import time >> +import urlparse >> import uuid >> -from xml.etree import ElementTree >> >> -import libvirt >> from cherrypy.process.plugins import BackgroundTask >> +from lxml import etree, objectify >> +from xml.etree import ElementTree >> >> from kimchi import vnc >> from kimchi import xmlutils >> @@ -36,7 +40,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 +52,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 = {} >> @@ -222,7 +231,7 @@ class VMModel(object): >> dom = self.get_vm(name, self.conn) >> dom = self._static_vm_update(dom, params) >> self._live_vm_update(dom, params) >> - return dom.name().decode('utf-8') >> + return dom.name() >> >> def _static_vm_update(self, dom, params): >> state = DOM_STATE_MAP[dom.info()[0]] >> @@ -453,3 +462,170 @@ 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> >> + """ > To keep consistency, may I suggest we use lxml.builder and lxml etree. >> + 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 > Here too. I tried, but couldn't came up with a code using lxml.builder and/or objectify which is simpler and more readable than this one. If anyone wants to jump in and provide such code, please do :)

>> + 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 valid" >> + 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' > REF: > http://wiki.qemu.org/download/qemu-doc.html#disk_005fimages_005ffat_005fi... > http://en.wikibooks.org/wiki/QEMU/Devices/Storage > 'dir' type is used to share directory between host(linux) and guest(windows) without using samba or nfs, or create a boot floppy. > So I don't think its related to CDROM management and will cause unexpected result. Let's get rid of it temporarily until we start to support this feature. >> + else: >> + params['src_type'] = 'block' > Does it mean other type is classified to block? I think only block > device should be used. Also I suggest we only suggest the same > usecase with template cdrom, that is file or valid file link, > The use of host block device may be out of our scope. >> + else: >> + params['src_type'] = 'network' > Will it be better if we validate the source file as we did in osinfo.py? > response = urllib2.urlopen(request) > data = response.read() >> + >> + # Use device name passed or pick next >> + dev_name = params.get('dev') >> + if not dev_name: >> + params['dev'] = self._get_storage_device_name(vm_name) >> + >> + # 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_storage_device_name(self, vm_name): >> + dev_list = [dev for dev in self.get_list(vm_name) >> + if dev.startswith('hd')] >> + if len(dev_list) == 0: >> + return 'hda' >> + dev_list.sort() >> + last_dev = dev_list.pop() >> + # TODO: Improve to device names "greater then" hdz >> + next_dev_letter_pos = string.ascii_lowercase.index(last_dev[2]) + 1 >> + return 'hd' + string.ascii_lowercase[next_dev_letter_pos] >> + >> + def get_list(self, vm_name): >> + dom = VMModel.get_vm(vm_name, self.conn) >> + xml = dom.XMLDesc(0) >> + devices = objectify.fromstring(xml).devices >> + storages = [disk.target.attrib['dev'] >> + for disk in devices.xpath("./disk[@device='disk']")] >> + storages += [disk.target.attrib['dev'] >> + for disk in devices.xpath("./disk[@device='cdrom']")] >> + return storages >> + >> + >> +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) >> + xml = dom.XMLDesc(0) >> + devices = objectify.fromstring(xml).devices >> + disk = devices.xpath("./disk/target[@dev='%s']/.." % dev_name) >> + return disk[0] >> + >> + 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 None: >> + raise NotFoundError('The storage device "%s" does not exist in ' >> + 'the guest "%s"' % (dev_name, vm_name)) >> + source = disk.source >> + path = "" >> + if source is not None: >> + src_type = disk.attrib['type'] >> + if src_type == 'network': >> + host = source.host >> + path = (source.attrib['protocol'] + '://' + >> + host.attrib['name'] + ':' + >> + host.attrib['port'] + source.attrib['name']) >> + else: >> + path = source.attrib[DEV_TYPE_SRC_ATTR_MAP[src_type]] >> + dev_type = disk.attrib['device'] >> + return {'dev': dev_name, >> + 'type': dev_type, >> + 'path': path} >> + >> + def delete(self, vm_name, dev_name): >> + # Get storage device xml >> + disk = self._get_device_xml(vm_name, dev_name) >> + if disk is None: >> + raise NotFoundError('The storage device "%s" does not exist in ' >> + 'the guest "%s"' % (dev_name, vm_name)) >> + try: >> + conn = self.conn.get() >> + dom = conn.lookupByName(vm_name) >> + dom.detachDeviceFlags(etree.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): >> + info = self.lookup(vm_name, dev_name) >> + backup_params = info.copy() >> + try: >> + self.delete(vm_name, dev_name) >> + except: >> + msg = 'Was not possible to update storage device: %s' % e.message >> + kimchi_log.error(msg) >> + raise OperationFailed(e.message) >> + >> + info.update(params) >> + kargs = {'conn': self.conn} >> + stgModel = StoragesModel(**kargs) >> + try: >> + dev_name = stgModel.create(vm_name, info) >> + return dev_name >> + except Exception as e: >> + # Restoring previous device >> + dev_name = stgModel.create(vm_name, backup_params) >> + msg = 'Was not possible to update storage device: %s' % e.message >> + kimchi_log.error(msg) >> + raise OperationFailed(e.message) >> 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