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

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.

> + 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")