[PATCH 0/8 V3] Create a common function to generate guest disk XML

V2 -> V3: - Create check_remote_disk_path() on model/utils.py to verify the necessity to convert hostname to IP V1 -> V2: - Make xmlutils/disk.py independent of model instances Aline Manera (8): Move _get_storage_xml() to xmlutils/disk.py Move vmdisks.py functions to xmlutils/disk.py Remove 'bus' paramater from /vms/<name>/storages documentation Update get_disk_xml() to get the device same according to bus and index values Remove ignore_src parameter from get_disk_xml() Get disk type according to file path on get_disk_xml() Check QEMU stream DNS capability when attaching new disk to guest Update vmtemplate.py to use get_disk_xml() while generating CDROM XML docs/API.md | 1 - src/kimchi/model/storagevolumes.py | 8 +- src/kimchi/model/utils.py | 19 ++++- src/kimchi/model/vmstorages.py | 144 +++++++++------------------------ src/kimchi/vmdisks.py | 75 ------------------ src/kimchi/vmtemplate.py | 74 ++++++----------- src/kimchi/xmlutils/disk.py | 158 +++++++++++++++++++++++++++++++++++++ tests/test_model.py | 11 ++- 8 files changed, 248 insertions(+), 242 deletions(-) delete mode 100644 src/kimchi/vmdisks.py create mode 100644 src/kimchi/xmlutils/disk.py -- 1.9.3

It is the first step to have xmlutils/disk.py holding all the disk XML manipulation. So we can reuse the same function on vmtemplate.py to build the guest XML. Signed-off-by: Aline Manera <alinefm@linux.vnet.ibm.com> --- src/kimchi/model/vmstorages.py | 49 ++++------------------------ src/kimchi/xmlutils/disk.py | 73 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 79 insertions(+), 43 deletions(-) create mode 100644 src/kimchi/xmlutils/disk.py diff --git a/src/kimchi/model/vmstorages.py b/src/kimchi/model/vmstorages.py index 055aa50..808b3d7 100644 --- a/src/kimchi/model/vmstorages.py +++ b/src/kimchi/model/vmstorages.py @@ -18,14 +18,10 @@ # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA import os -import socket import stat import string -import urlparse -import lxml.etree as ET from lxml import etree -from lxml.builder import E from kimchi.exception import InvalidOperation, InvalidParameter, NotFoundError from kimchi.exception import OperationFailed @@ -35,7 +31,7 @@ from kimchi.model.utils import get_vm_config_flag from kimchi.utils import check_url_path from kimchi.osinfo import lookup from kimchi.vmdisks import get_device_xml, get_vm_disk, get_vm_disk_list -from kimchi.vmdisks import DEV_TYPE_SRC_ATTR_MAP +from kimchi.xmlutils.disk import get_disk_xml HOTPLUG_TYPE = ['scsi', 'virtio'] PREFIX_MAP = {'ide': 'hd', 'virtio': 'vd', 'scsi': 'sd'} @@ -49,39 +45,6 @@ def _get_device_bus(dev_type, dom): return lookup(distro, version)[dev_type+'_bus'] -def _get_storage_xml(params, ignore_source=False): - src_type = params.get('src_type') - disk = E.disk(type=src_type, device=params.get('type')) - disk.append(E.driver(name='qemu', type=params['format'])) - - disk.append(E.target(dev=params.get('dev'), bus=params['bus'])) - if params.get('address'): - # ide disk target id is always '0' - disk.append(E.address( - type='drive', controller=params['address']['controller'], - bus=params['address']['bus'], target='0', - unit=params['address']['unit'])) - - if ignore_source: - return ET.tostring(disk) - - # Working with url paths - if src_type == 'network': - output = urlparse.urlparse(params.get('path')) - port = str(output.port or socket.getservbyname(output.scheme)) - host = E.host(name=output.hostname, port=port) - source = E.source(protocol=output.scheme, name=output.path) - source.append(host) - disk.append(source) - else: - # Fixing source attribute - source = E.source() - source.set(DEV_TYPE_SRC_ATTR_MAP[src_type], params.get('path')) - disk.append(source) - - return ET.tostring(disk) - - def _check_path(path): if check_url_path(path): src_type = 'network' @@ -164,14 +127,14 @@ class VMStoragesModel(object): {"format": vol_info['format'], "type": params['type']}) params['path'] = vol_info['path'] - params['src_type'] = _check_path(params['path']) + if (params['bus'] not in HOTPLUG_TYPE and DOM_STATE_MAP[dom.info()[0]] != 'shutoff'): raise InvalidOperation('KCHVMSTOR0011E') params.update(self._get_available_bus_address(params['bus'], vm_name)) # Add device to VM - dev_xml = _get_storage_xml(params) + dev_xml = get_disk_xml(_check_path(params['path']), params) try: conn = self.conn.get() dom = conn.lookupByName(vm_name) @@ -234,10 +197,10 @@ class VMStorageModel(object): def update(self, vm_name, dev_name, params): path = params.get('path') if path and len(path) != 0: - params['src_type'] = _check_path(path) + src_type = _check_path(path) ignore_source = False else: - params['src_type'] = 'file' + src_type = 'file' ignore_source = True dom = VMModel.get_vm(vm_name, self.conn) @@ -245,8 +208,8 @@ class VMStorageModel(object): if dev_info['type'] != 'cdrom': raise InvalidOperation("KCHVMSTOR0006E") dev_info.update(params) - xml = _get_storage_xml(dev_info, ignore_source) + xml = get_disk_xml(src_type, dev_info, ignore_source) try: dom.updateDeviceFlags(xml, get_vm_config_flag(dom, 'all')) except Exception as e: diff --git a/src/kimchi/xmlutils/disk.py b/src/kimchi/xmlutils/disk.py new file mode 100644 index 0000000..8c64ff4 --- /dev/null +++ b/src/kimchi/xmlutils/disk.py @@ -0,0 +1,73 @@ +# +# Project Kimchi +# +# Copyright IBM, Corp. 2014 +# +# This library is free software; you can redistribute it and/or +# modify it under the terms of the GNU Lesser General Public +# License as published by the Free Software Foundation; either +# version 2.1 of the License, or (at your option) any later version. +# +# This library is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU +# Lesser General Public License for more details. +# +# You should have received a copy of the GNU Lesser General Public +# 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 lxml.etree as ET +import socket +import urlparse + +from lxml.builder import E + +DEV_TYPE_SRC_ATTR_MAP = {'file': 'file', 'block': 'dev'} + + +def get_disk_xml(src_type, params, ignore_src=False): + """ + <disk type='file' device='cdrom'> + <driver name='qemu' type='raw'/> + + [source XML according to src_type] + + <target dev='%(dev)s' bus='%(bus)s'/> + <readonly/> + </disk> + """ + disk = E.disk(type=src_type, device=params['type']) + disk.append(E.driver(name='qemu', type=params['format'])) + disk.append(E.target(dev=params['dev'], bus=params['bus'])) + + if params.get('address'): + # ide disk target id is always '0' + disk.append(E.address( + type='drive', controller=params['address']['controller'], + bus=params['address']['bus'], target='0', + unit=params['address']['unit'])) + + if ignore_src: + return ET.tostring(disk, encoding='utf-8', pretty_print=True) + + if src_type == 'network': + """ + <source protocol='%(protocol)s' name='%(url_path)s'> + <host name='%(hostname)s' port='%(port)s'/> + </source> + """ + output = urlparse.urlparse(params['path']) + port = str(output.port or socket.getservbyname(output.scheme)) + + source = E.source(protocol=output.scheme, name=output.path) + source.append(E.host(name=output.hostname, port=port)) + else: + """ + <source file='%(src)s' /> + """ + source = E.source() + source.set(DEV_TYPE_SRC_ATTR_MAP[src_type], params['path']) + + disk.append(source) + return ET.tostring(disk, encoding='utf-8', pretty_print=True) -- 1.9.3

vmdisks.py also had some functions to manipulate the guest disk XML. So move them to xmlutils/disk.py Also update the references to it on Kimchi code. Signed-off-by: Aline Manera <alinefm@linux.vnet.ibm.com> --- src/kimchi/model/storagevolumes.py | 4 +- src/kimchi/model/vmstorages.py | 10 ++--- src/kimchi/vmdisks.py | 75 -------------------------------------- src/kimchi/xmlutils/disk.py | 53 +++++++++++++++++++++++++++ 4 files changed, 60 insertions(+), 82 deletions(-) delete mode 100644 src/kimchi/vmdisks.py diff --git a/src/kimchi/model/storagevolumes.py b/src/kimchi/model/storagevolumes.py index 1ee8d0a..db596c3 100644 --- a/src/kimchi/model/storagevolumes.py +++ b/src/kimchi/model/storagevolumes.py @@ -32,7 +32,7 @@ from kimchi.model.storagepools import StoragePoolModel from kimchi.model.tasks import TaskModel from kimchi.model.vms import VMsModel, VMModel from kimchi.utils import add_task, kimchi_log -from kimchi.vmdisks import get_vm_disk, get_vm_disk_list +from kimchi.xmlutils.disk import get_vm_disk_info, get_vm_disk_list from kimchi.xmlutils.utils import xpath_get_text @@ -267,7 +267,7 @@ class StorageVolumeModel(object): dom = VMModel.get_vm(vm, self.conn) storages = get_vm_disk_list(dom) for disk in storages: - d_info = get_vm_disk(dom, disk) + d_info = get_vm_disk_info(dom, disk) if path == d_info['path']: ref_cnt = ref_cnt + 1 session.store('storagevolume', vol_id, diff --git a/src/kimchi/model/vmstorages.py b/src/kimchi/model/vmstorages.py index 808b3d7..3a8b6b3 100644 --- a/src/kimchi/model/vmstorages.py +++ b/src/kimchi/model/vmstorages.py @@ -30,8 +30,8 @@ from kimchi.model.storagevolumes import StorageVolumeModel from kimchi.model.utils import get_vm_config_flag from kimchi.utils import check_url_path from kimchi.osinfo import lookup -from kimchi.vmdisks import get_device_xml, get_vm_disk, get_vm_disk_list -from kimchi.xmlutils.disk import get_disk_xml +from kimchi.xmlutils.disk import get_device_node, get_disk_xml +from kimchi.xmlutils.disk import get_vm_disk_info, get_vm_disk_list HOTPLUG_TYPE = ['scsi', 'virtio'] PREFIX_MAP = {'ide': 'hd', 'virtio': 'vd', 'scsi': 'sd'} @@ -78,7 +78,7 @@ class VMStoragesModel(object): valid_id = [('0', '0'), ('0', '1'), ('1', '0'), ('1', '1')] controller_id = '0' for dev_name in disks: - disk = get_device_xml(dom, dev_name) + disk = get_device_node(dom, dev_name) if disk.target.attrib['bus'] == 'ide': controller_id = disk.address.attrib['controller'] bus_id = disk.address.attrib['bus'] @@ -170,7 +170,7 @@ class VMStorageModel(object): def lookup(self, vm_name, dev_name): # Retrieve disk xml and format return dict dom = VMModel.get_vm(vm_name, self.conn) - return get_vm_disk(dom, dev_name) + return get_vm_disk_info(dom, dev_name) def delete(self, vm_name, dev_name): # Get storage device xml @@ -188,7 +188,7 @@ class VMStorageModel(object): try: conn = self.conn.get() dom = conn.lookupByName(vm_name) - disk = get_device_xml(dom, dev_name) + disk = get_device_node(dom, dev_name) dom.detachDeviceFlags(etree.tostring(disk), get_vm_config_flag(dom, 'all')) except Exception as e: diff --git a/src/kimchi/vmdisks.py b/src/kimchi/vmdisks.py deleted file mode 100644 index f1c3f02..0000000 --- a/src/kimchi/vmdisks.py +++ /dev/null @@ -1,75 +0,0 @@ -# -# Project Kimchi -# -# Copyright IBM, Corp. 2014 -# -# This library is free software; you can redistribute it and/or -# modify it under the terms of the GNU Lesser General Public -# License as published by the Free Software Foundation; either -# version 2.1 of the License, or (at your option) any later version. -# -# This library is distributed in the hope that it will be useful, -# but WITHOUT ANY WARRANTY; without even the implied warranty of -# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU -# Lesser General Public License for more details. -# -# You should have received a copy of the GNU Lesser General Public -# License along with this library; if not, write to the Free Software -# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA - -from lxml import objectify - -from kimchi.exception import NotFoundError - -DEV_TYPE_SRC_ATTR_MAP = {'file': 'file', - 'block': 'dev'} - - -def get_device_xml(dom, dev_name): - # Get VM xml and then devices xml - xml = dom.XMLDesc(0) - devices = objectify.fromstring(xml).devices - disk = devices.xpath("./disk/target[@dev='%s']/.." % dev_name) - if not disk: - return None - return disk[0] - - -def get_vm_disk(dom, dev_name): - # Retrieve disk xml and format return dict - disk = get_device_xml(dom, dev_name) - if disk is None: - raise NotFoundError("KCHVMSTOR0007E", - {'dev_name': dev_name, - 'vm_name': dom.name()}) - path = "" - dev_bus = disk.target.attrib['bus'] - try: - source = disk.source - 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]] - except: - pass - dev_type = disk.attrib['device'] - return {'dev': dev_name, - 'type': dev_type, - 'path': path, - 'format': disk.driver.attrib['type'], - 'bus': dev_bus} - - -def get_vm_disk_list(dom): - 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 diff --git a/src/kimchi/xmlutils/disk.py b/src/kimchi/xmlutils/disk.py index 8c64ff4..800a64c 100644 --- a/src/kimchi/xmlutils/disk.py +++ b/src/kimchi/xmlutils/disk.py @@ -21,8 +21,11 @@ import lxml.etree as ET import socket import urlparse +from lxml import objectify from lxml.builder import E +from kimchi.exception import NotFoundError + DEV_TYPE_SRC_ATTR_MAP = {'file': 'file', 'block': 'dev'} @@ -71,3 +74,53 @@ def get_disk_xml(src_type, params, ignore_src=False): disk.append(source) return ET.tostring(disk, encoding='utf-8', pretty_print=True) + + +def get_device_node(dom, dev_name): + xml = dom.XMLDesc(0) + devices = objectify.fromstring(xml).devices + disk = devices.xpath("./disk/target[@dev='%s']/.." % dev_name) + + if not disk: + raise NotFoundError("KCHVMSTOR0007E", + {'dev_name': dev_name, + 'vm_name': dom.name()}) + + return disk[0] + + +def get_vm_disk_info(dom, dev_name): + # Retrieve disk xml and format return dict + disk = get_device_node(dom, dev_name) + if disk is None: + return None + + try: + source = disk.source + 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]] + except: + path = "" + + return {'dev': dev_name, + 'path': path, + 'type': disk.attrib['device'], + 'format': disk.driver.attrib['type'], + 'bus': disk.target.attrib['bus']} + + +def get_vm_disk_list(dom): + 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 -- 1.9.3

While attaching a new disk to a guest, Kimchi will automatically identify the best bus for it. So update the documentation to reflect it. Signed-off-by: Aline Manera <alinefm@linux.vnet.ibm.com> --- docs/API.md | 1 - 1 file changed, 1 deletion(-) diff --git a/docs/API.md b/docs/API.md index 6984649..d65e667 100644 --- a/docs/API.md +++ b/docs/API.md @@ -152,7 +152,6 @@ Represents a snapshot of the Virtual Machine's primary monitor. * path: Path of cdrom iso. * pool: Storage pool which disk image file locate in. * vol: Storage volume name of disk image. - * bus: Target bus type for disk to attach. ### Sub-resource: storage **URI:** /vms/*:name*/storages/*:dev* -- 1.9.3

When generating the disk XML, the most common information to generate the device name are: 1) the bus - to identify the right prefix, and 2) the disk index. Add this logic to get_disk_xml() so when no 'dev' name is specified the device name will be automatically generated according to bus and index value. Signed-off-by: Aline Manera <alinefm@linux.vnet.ibm.com> --- src/kimchi/model/storagevolumes.py | 6 ++-- src/kimchi/model/vmstorages.py | 67 ++++++++++++++++++-------------------- src/kimchi/xmlutils/disk.py | 25 +++++++++----- 3 files changed, 51 insertions(+), 47 deletions(-) diff --git a/src/kimchi/model/storagevolumes.py b/src/kimchi/model/storagevolumes.py index db596c3..9ff43e6 100644 --- a/src/kimchi/model/storagevolumes.py +++ b/src/kimchi/model/storagevolumes.py @@ -32,7 +32,7 @@ from kimchi.model.storagepools import StoragePoolModel from kimchi.model.tasks import TaskModel from kimchi.model.vms import VMsModel, VMModel from kimchi.utils import add_task, kimchi_log -from kimchi.xmlutils.disk import get_vm_disk_info, get_vm_disk_list +from kimchi.xmlutils.disk import get_vm_disk_info, get_vm_disks from kimchi.xmlutils.utils import xpath_get_text @@ -265,8 +265,8 @@ class StorageVolumeModel(object): vms = VMsModel.get_vms(self.conn) for vm in vms: dom = VMModel.get_vm(vm, self.conn) - storages = get_vm_disk_list(dom) - for disk in storages: + storages = get_vm_disks(dom) + for disk in storages.keys(): d_info = get_vm_disk_info(dom, disk) if path == d_info['path']: ref_cnt = ref_cnt + 1 diff --git a/src/kimchi/model/vmstorages.py b/src/kimchi/model/vmstorages.py index 3a8b6b3..b3b1514 100644 --- a/src/kimchi/model/vmstorages.py +++ b/src/kimchi/model/vmstorages.py @@ -31,10 +31,9 @@ from kimchi.model.utils import get_vm_config_flag from kimchi.utils import check_url_path from kimchi.osinfo import lookup from kimchi.xmlutils.disk import get_device_node, get_disk_xml -from kimchi.xmlutils.disk import get_vm_disk_info, get_vm_disk_list +from kimchi.xmlutils.disk import get_vm_disk_info, get_vm_disks HOTPLUG_TYPE = ['scsi', 'virtio'] -PREFIX_MAP = {'ide': 'hd', 'virtio': 'vd', 'scsi': 'sd'} def _get_device_bus(dev_type, dom): @@ -95,14 +94,28 @@ class VMStoragesModel(object): return dict(address=address) def create(self, vm_name, params): - dom = VMModel.get_vm(vm_name, self.conn) - params['bus'] = _get_device_bus(params['type'], dom) - self._get_storage_device_name(vm_name, params) # Path will never be blank due to API.json verification. # There is no need to cover this case here. - params['format'] = 'raw' if not ('vol' in params) ^ ('path' in params): raise InvalidParameter("KCHVMSTOR0017E") + + dom = VMModel.get_vm(vm_name, self.conn) + params['bus'] = _get_device_bus(params['type'], dom) + params['format'] = 'raw' + + dev_list = [dev for dev, bus in get_vm_disks(dom).iteritems() + if bus == params['bus']] + dev_list.sort() + if len(dev_list) == 0: + params['index'] = 0 + else: + char = dev_list.pop()[2] + params['index'] = string.ascii_lowercase.index(char) + 1 + + if (params['bus'] not in HOTPLUG_TYPE + and DOM_STATE_MAP[dom.info()[0]] != 'shutoff'): + raise InvalidOperation('KCHVMSTOR0011E') + if params.get('vol'): try: pool = params['pool'] @@ -119,48 +132,30 @@ class VMStoragesModel(object): supported_format = { "disk": ["raw", "bochs", "qcow", "qcow2", "qed", "vmdk"], "cdrom": "iso"} - if vol_info['format'] in supported_format[params['type']]: - if params['type'] == 'disk': - params['format'] = vol_info['format'] - else: + if vol_info['format'] not in supported_format[params['type']]: raise InvalidParameter("KCHVMSTOR0018E", {"format": vol_info['format'], "type": params['type']}) - params['path'] = vol_info['path'] - if (params['bus'] not in HOTPLUG_TYPE - and DOM_STATE_MAP[dom.info()[0]] != 'shutoff'): - raise InvalidOperation('KCHVMSTOR0011E') + if params['type'] == 'disk': + params['format'] = vol_info['format'] + + params['path'] = vol_info['path'] params.update(self._get_available_bus_address(params['bus'], vm_name)) # Add device to VM - dev_xml = get_disk_xml(_check_path(params['path']), params) + dev, xml = get_disk_xml(_check_path(params['path']), params) try: conn = self.conn.get() dom = conn.lookupByName(vm_name) - dom.attachDeviceFlags(dev_xml, get_vm_config_flag(dom, 'all')) + dom.attachDeviceFlags(xml, get_vm_config_flag(dom, 'all')) except Exception as e: raise OperationFailed("KCHVMSTOR0008E", {'error': e.message}) - return params['dev'] - - def _get_storage_device_name(self, vm_name, params): - bus_prefix = PREFIX_MAP[params['bus']] - dev_list = [dev for dev in self.get_list(vm_name) - if dev.startswith(bus_prefix)] - if len(dev_list) == 0: - params['dev'] = bus_prefix + 'a' - else: - 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 - params['dev'] =\ - bus_prefix + string.ascii_lowercase[next_dev_letter_pos] + return dev def get_list(self, vm_name): dom = VMModel.get_vm(vm_name, self.conn) - return get_vm_disk_list(dom) + return get_vm_disks(dom).keys() class VMStorageModel(object): @@ -207,11 +202,11 @@ class VMStorageModel(object): dev_info = self.lookup(vm_name, dev_name) if dev_info['type'] != 'cdrom': raise InvalidOperation("KCHVMSTOR0006E") - dev_info.update(params) - xml = get_disk_xml(src_type, dev_info, ignore_source) + dev_info.update(params) + dev, xml = get_disk_xml(src_type, dev_info, ignore_source) try: dom.updateDeviceFlags(xml, get_vm_config_flag(dom, 'all')) except Exception as e: raise OperationFailed("KCHVMSTOR0009E", {'error': e.message}) - return dev_name + return dev diff --git a/src/kimchi/xmlutils/disk.py b/src/kimchi/xmlutils/disk.py index 800a64c..aadbfb8 100644 --- a/src/kimchi/xmlutils/disk.py +++ b/src/kimchi/xmlutils/disk.py @@ -19,6 +19,7 @@ import lxml.etree as ET import socket +import string import urlparse from lxml import objectify @@ -26,6 +27,7 @@ from lxml.builder import E from kimchi.exception import NotFoundError +BUS_TO_DEV_MAP = {'ide': 'hd', 'virtio': 'vd', 'scsi': 'sd'} DEV_TYPE_SRC_ATTR_MAP = {'file': 'file', 'block': 'dev'} @@ -42,7 +44,11 @@ def get_disk_xml(src_type, params, ignore_src=False): """ disk = E.disk(type=src_type, device=params['type']) disk.append(E.driver(name='qemu', type=params['format'])) - disk.append(E.target(dev=params['dev'], bus=params['bus'])) + + # Get device name according to bus and index values + dev = params.get('dev', (BUS_TO_DEV_MAP[params['bus']] + + string.lowercase[params.get('index', 0)])) + disk.append(E.target(dev=dev, bus=params['bus'])) if params.get('address'): # ide disk target id is always '0' @@ -52,7 +58,7 @@ def get_disk_xml(src_type, params, ignore_src=False): unit=params['address']['unit'])) if ignore_src: - return ET.tostring(disk, encoding='utf-8', pretty_print=True) + return (dev, ET.tostring(disk, encoding='utf-8', pretty_print=True)) if src_type == 'network': """ @@ -73,7 +79,7 @@ def get_disk_xml(src_type, params, ignore_src=False): source.set(DEV_TYPE_SRC_ATTR_MAP[src_type], params['path']) disk.append(source) - return ET.tostring(disk, encoding='utf-8', pretty_print=True) + return (dev, ET.tostring(disk, encoding='utf-8', pretty_print=True)) def get_device_node(dom, dev_name): @@ -116,11 +122,14 @@ def get_vm_disk_info(dom, dev_name): 'bus': disk.target.attrib['bus']} -def get_vm_disk_list(dom): +def get_vm_disks(dom): 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']")] + + storages = {} + all_disks = devices.xpath("./disk[@device='disk']") + all_disks.extend(devices.xpath("./disk[@device='cdrom']")) + for disk in all_disks: + storages[disk.target.attrib['dev']] = disk.target.attrib['bus'] + return storages -- 1.9.3

The ignore_src parameter can be automatically identified by the disk path value, ie, when it is an empty string the source can be ignored. Signed-off-by: Aline Manera <alinefm@linux.vnet.ibm.com> --- src/kimchi/model/vmstorages.py | 7 +++---- src/kimchi/xmlutils/disk.py | 4 ++-- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/src/kimchi/model/vmstorages.py b/src/kimchi/model/vmstorages.py index b3b1514..4c4682c 100644 --- a/src/kimchi/model/vmstorages.py +++ b/src/kimchi/model/vmstorages.py @@ -190,13 +190,12 @@ class VMStorageModel(object): raise OperationFailed("KCHVMSTOR0010E", {'error': e.message}) def update(self, vm_name, dev_name, params): - path = params.get('path') - if path and len(path) != 0: + path = params.get('path', '') + params['path'] = path + if len(path) != 0: src_type = _check_path(path) - ignore_source = False else: src_type = 'file' - ignore_source = True dom = VMModel.get_vm(vm_name, self.conn) dev_info = self.lookup(vm_name, dev_name) diff --git a/src/kimchi/xmlutils/disk.py b/src/kimchi/xmlutils/disk.py index aadbfb8..f40f34f 100644 --- a/src/kimchi/xmlutils/disk.py +++ b/src/kimchi/xmlutils/disk.py @@ -31,7 +31,7 @@ BUS_TO_DEV_MAP = {'ide': 'hd', 'virtio': 'vd', 'scsi': 'sd'} DEV_TYPE_SRC_ATTR_MAP = {'file': 'file', 'block': 'dev'} -def get_disk_xml(src_type, params, ignore_src=False): +def get_disk_xml(src_type, params): """ <disk type='file' device='cdrom'> <driver name='qemu' type='raw'/> @@ -57,7 +57,7 @@ def get_disk_xml(src_type, params, ignore_src=False): bus=params['address']['bus'], target='0', unit=params['address']['unit'])) - if ignore_src: + if len(params['path']) == 0: return (dev, ET.tostring(disk, encoding='utf-8', pretty_print=True)) if src_type == 'network': -- 1.9.3

The disk type can be also be identified by the file path. So move it to the common place to handle guest disk XML manipulation. Signed-off-by: Aline Manera <alinefm@linux.vnet.ibm.com> --- src/kimchi/model/vmstorages.py | 33 ++++----------------------------- src/kimchi/xmlutils/disk.py | 33 ++++++++++++++++++++++++++++----- 2 files changed, 32 insertions(+), 34 deletions(-) diff --git a/src/kimchi/model/vmstorages.py b/src/kimchi/model/vmstorages.py index 4c4682c..70240b0 100644 --- a/src/kimchi/model/vmstorages.py +++ b/src/kimchi/model/vmstorages.py @@ -17,8 +17,6 @@ # 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 os -import stat import string from lxml import etree @@ -28,7 +26,6 @@ from kimchi.exception import OperationFailed from kimchi.model.vms import DOM_STATE_MAP, VMModel from kimchi.model.storagevolumes import StorageVolumeModel from kimchi.model.utils import get_vm_config_flag -from kimchi.utils import check_url_path from kimchi.osinfo import lookup from kimchi.xmlutils.disk import get_device_node, get_disk_xml from kimchi.xmlutils.disk import get_vm_disk_info, get_vm_disks @@ -44,24 +41,6 @@ def _get_device_bus(dev_type, dom): return lookup(distro, version)[dev_type+'_bus'] -def _check_path(path): - if check_url_path(path): - src_type = 'network' - # Check if path is a valid local path - elif os.path.exists(path): - if os.path.isfile(path): - src_type = 'file' - else: - r_path = os.path.realpath(path) - if not stat.S_ISBLK(os.stat(r_path).st_mode): - raise InvalidParameter("KCHVMSTOR0003E", {'value': path}) - - src_type = 'block' - else: - raise InvalidParameter("KCHVMSTOR0003E", {'value': path}) - return src_type - - class VMStoragesModel(object): def __init__(self, **kargs): self.conn = kargs['conn'] @@ -144,7 +123,7 @@ class VMStoragesModel(object): params.update(self._get_available_bus_address(params['bus'], vm_name)) # Add device to VM - dev, xml = get_disk_xml(_check_path(params['path']), params) + dev, xml = get_disk_xml(params) try: conn = self.conn.get() dom = conn.lookupByName(vm_name) @@ -190,20 +169,16 @@ class VMStorageModel(object): raise OperationFailed("KCHVMSTOR0010E", {'error': e.message}) def update(self, vm_name, dev_name, params): - path = params.get('path', '') - params['path'] = path - if len(path) != 0: - src_type = _check_path(path) - else: - src_type = 'file' dom = VMModel.get_vm(vm_name, self.conn) dev_info = self.lookup(vm_name, dev_name) if dev_info['type'] != 'cdrom': raise InvalidOperation("KCHVMSTOR0006E") + params['path'] = params.get('path', '') dev_info.update(params) - dev, xml = get_disk_xml(src_type, dev_info, ignore_source) + + dev, xml = get_disk_xml(dev_info) try: dom.updateDeviceFlags(xml, get_vm_config_flag(dom, 'all')) except Exception as e: diff --git a/src/kimchi/xmlutils/disk.py b/src/kimchi/xmlutils/disk.py index f40f34f..f066dbe 100644 --- a/src/kimchi/xmlutils/disk.py +++ b/src/kimchi/xmlutils/disk.py @@ -18,20 +18,23 @@ # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA import lxml.etree as ET +import os import socket +import stat import string import urlparse from lxml import objectify from lxml.builder import E -from kimchi.exception import NotFoundError +from kimchi.exception import InvalidParameter, NotFoundError +from kimchi.utils import check_url_path BUS_TO_DEV_MAP = {'ide': 'hd', 'virtio': 'vd', 'scsi': 'sd'} DEV_TYPE_SRC_ATTR_MAP = {'file': 'file', 'block': 'dev'} -def get_disk_xml(src_type, params): +def get_disk_xml(params): """ <disk type='file' device='cdrom'> <driver name='qemu' type='raw'/> @@ -42,7 +45,9 @@ def get_disk_xml(src_type, params): <readonly/> </disk> """ - disk = E.disk(type=src_type, device=params['type']) + path = params['path'] + disk_type = _get_disk_type(path) if len(path) > 0 else 'file' + disk = E.disk(type=disk_type, device=params['type']) disk.append(E.driver(name='qemu', type=params['format'])) # Get device name according to bus and index values @@ -60,7 +65,7 @@ def get_disk_xml(src_type, params): if len(params['path']) == 0: return (dev, ET.tostring(disk, encoding='utf-8', pretty_print=True)) - if src_type == 'network': + if disk_type == 'network': """ <source protocol='%(protocol)s' name='%(url_path)s'> <host name='%(hostname)s' port='%(port)s'/> @@ -76,12 +81,30 @@ def get_disk_xml(src_type, params): <source file='%(src)s' /> """ source = E.source() - source.set(DEV_TYPE_SRC_ATTR_MAP[src_type], params['path']) + source.set(DEV_TYPE_SRC_ATTR_MAP[disk_type], params['path']) disk.append(source) return (dev, ET.tostring(disk, encoding='utf-8', pretty_print=True)) +def _get_disk_type(path): + if check_url_path(path): + return 'network' + + if not os.path.exists(path): + raise InvalidParameter("KCHVMSTOR0003E", {'value': path}) + + # Check if path is a valid local path + if os.path.isfile(path): + return 'file' + + r_path = os.path.realpath(path) + if stat.S_ISBLK(os.stat(r_path).st_mode): + return 'block' + + raise InvalidParameter("KCHVMSTOR0003E", {'value': path}) + + def get_device_node(dom, dev_name): xml = dom.XMLDesc(0) devices = objectify.fromstring(xml).devices -- 1.9.3

When QEMU is not able to solve hostname for a remote CDROM path, we need to convert the hostname to IP to avoid problems. To make xmlutils/ independent of model instances, that verification must be done prior to the XML generation. Signed-off-by: Aline Manera <alinefm@linux.vnet.ibm.com> --- src/kimchi/model/utils.py | 19 +++++++++++++++---- src/kimchi/model/vmstorages.py | 8 +++++--- tests/test_model.py | 11 ++++++++++- 3 files changed, 30 insertions(+), 8 deletions(-) diff --git a/src/kimchi/model/utils.py b/src/kimchi/model/utils.py index 4d8e65a..5cc5075 100644 --- a/src/kimchi/model/utils.py +++ b/src/kimchi/model/utils.py @@ -17,14 +17,16 @@ # 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 re -from kimchi.exception import OperationFailed -from kimchi.featuretests import FeatureTests -from kimchi.model.config import CapabilitiesModel import libvirt +import re +import socket +import urlparse from lxml import etree from lxml.builder import E, ElementMaker +from kimchi.exception import OperationFailed +from kimchi.featuretests import FeatureTests +from kimchi.model.config import CapabilitiesModel KIMCHI_META_URL = "https://github.com/kimchi-project/kimchi" KIMCHI_NAMESPACE = "kimchi" @@ -40,6 +42,15 @@ def get_vm_name(vm_name, t_name, name_list): raise OperationFailed("KCHUTILS0003E") +def check_remote_disk_path(path): + hostname = urlparse.urlparse(path).hostname + if hostname is not None and not CapabilitiesModel().qemu_stream_dns: + ip = socket.gethostbyname(hostname) + return path.replace(hostname, ip) + + return path + + def get_vm_config_flag(dom, mode="persistent"): # libvirt.VIR_DOMAIN_AFFECT_CURRENT is 0 # VIR_DOMAIN_AFFECT_LIVE is 1, VIR_DOMAIN_AFFECT_CONFIG is 2 diff --git a/src/kimchi/model/vmstorages.py b/src/kimchi/model/vmstorages.py index 70240b0..72f6bfd 100644 --- a/src/kimchi/model/vmstorages.py +++ b/src/kimchi/model/vmstorages.py @@ -25,7 +25,7 @@ from kimchi.exception import InvalidOperation, InvalidParameter, NotFoundError from kimchi.exception import OperationFailed from kimchi.model.vms import DOM_STATE_MAP, VMModel from kimchi.model.storagevolumes import StorageVolumeModel -from kimchi.model.utils import get_vm_config_flag +from kimchi.model.utils import check_remote_disk_path, get_vm_config_flag from kimchi.osinfo import lookup from kimchi.xmlutils.disk import get_device_node, get_disk_xml from kimchi.xmlutils.disk import get_vm_disk_info, get_vm_disks @@ -122,6 +122,8 @@ class VMStoragesModel(object): params['path'] = vol_info['path'] params.update(self._get_available_bus_address(params['bus'], vm_name)) + params['path'] = check_remote_disk_path(params['path']) + # Add device to VM dev, xml = get_disk_xml(params) try: @@ -175,10 +177,10 @@ class VMStorageModel(object): if dev_info['type'] != 'cdrom': raise InvalidOperation("KCHVMSTOR0006E") - params['path'] = params.get('path', '') + params['path'] = check_remote_disk_path(params.get('path', '')) dev_info.update(params) - dev, xml = get_disk_xml(dev_info) + try: dom.updateDeviceFlags(xml, get_vm_config_flag(dom, 'all')) except Exception as e: diff --git a/tests/test_model.py b/tests/test_model.py index 896540d..3e43780 100644 --- a/tests/test_model.py +++ b/tests/test_model.py @@ -25,9 +25,11 @@ import psutil import pwd import re import shutil +import socket import tempfile import threading import time +import urlparse import unittest import uuid @@ -435,7 +437,14 @@ class ModelTests(unittest.TestCase): {'path': valid_remote_iso_path}) cdrom_info = inst.vmstorage_lookup(vm_name, cdrom_dev) cur_cdrom_path = re.sub(":80/", '/', cdrom_info['path']) - self.assertEquals(valid_remote_iso_path, cur_cdrom_path) + + # As Kimchi server is not running during this test case + # CapabilitiesModel.qemu_stream_dns will be always False + # so we need to convert the hostname to IP + output = urlparse.urlparse(valid_remote_iso_path) + hostname = socket.gethostbyname(output.hostname) + url = valid_remote_iso_path.replace(output.hostname, hostname) + self.assertEquals(url, cur_cdrom_path) @unittest.skipUnless(utils.running_as_root(), 'Must be run as root') def test_vm_storage_provisioning(self): -- 1.9.3

It is the first step to have vmtemplate.py using the same functions used by model/vmstorages.py The disk generation will also use get_disk_xml() but it will be done in a separated patch. Signed-off-by: Aline Manera <alinefm@linux.vnet.ibm.com> --- src/kimchi/vmtemplate.py | 74 ++++++++++++++---------------------------------- 1 file changed, 22 insertions(+), 52 deletions(-) diff --git a/src/kimchi/vmtemplate.py b/src/kimchi/vmtemplate.py index b48cdbd..cfc46f6 100644 --- a/src/kimchi/vmtemplate.py +++ b/src/kimchi/vmtemplate.py @@ -24,18 +24,16 @@ import time import urlparse import uuid - from distutils.version import LooseVersion - +from lxml import etree +from lxml.builder import E from kimchi import osinfo from kimchi.exception import InvalidParameter, IsoFormatError, MissingParameter from kimchi.imageinfo import probe_image, probe_img_info from kimchi.isoinfo import IsoImage from kimchi.utils import check_url_path, pool_name_from_uri -from lxml import etree -from lxml.builder import E - +from kimchi.xmlutils.disk import get_disk_xml QEMU_NAMESPACE = "xmlns:qemu='http://libvirt.org/schemas/domain/qemu/1.0'" @@ -125,66 +123,38 @@ class VMTemplate(object): def _get_cdrom_xml(self, libvirt_stream_protocols, qemu_stream_dns): if 'cdrom' not in self.info: return '' - bus = self.info['cdrom_bus'] - dev = "%s%s" % (self._bus_to_dev[bus], - string.lowercase[self.info['cdrom_index']]) - - local_file = """ - <disk type='file' device='cdrom'> - <driver name='qemu' type='raw'/> - <source file='%(src)s' /> - <target dev='%(dev)s' bus='%(bus)s'/> - <readonly/> - </disk> - """ - network_file = """ - <disk type='network' device='cdrom'> - <driver name='qemu' type='raw'/> - <source protocol='%(protocol)s' name='%(url_path)s'> - <host name='%(hostname)s' port='%(port)s'/> - </source> - <target dev='%(dev)s' bus='%(bus)s'/> - <readonly/> - </disk> - """ + params = {} + params['type'] = 'cdrom' + params['format'] = 'raw' + params['bus'] = self.info['cdrom_bus'] + params['index'] = self.info['cdrom_index'] + params['path'] = self.info['cdrom'] qemu_stream_cmdline = """ <qemu:commandline> <qemu:arg value='-drive'/> - <qemu:arg value='file=%(url)s,if=none,id=drive-%(bus)s0-1-0,\ -readonly=on,format=raw'/> + <qemu:arg value='file=%(path)s,if=none,id=drive-%(bus)s0-1-0,\ +readonly=on,format=%(format)s'/> <qemu:arg value='-device'/> <qemu:arg value='%(bus)s-cd,bus=%(bus)s.1,unit=0,\ drive=drive-%(bus)s0-1-0,id=%(bus)s0-1-0'/> </qemu:commandline> """ - if not self.info.get('iso_stream', False): - params = {'src': self.info['cdrom'], 'dev': dev, 'bus': bus} - return local_file % (params) - - output = urlparse.urlparse(self.info['cdrom']) - port = output.port - protocol = output.scheme - hostname = output.hostname - url_path = output.path + hostname = urlparse.urlparse(params['path']).hostname + if hostname is not None and not qemu_stream_dns: + ip = socket.gethostbyname(hostname) + params['path'] = params['path'].replace(hostname, ip) - if port is None: - port = socket.getservbyname(protocol) + if self.info.get('iso_stream', False): + protocol = urlparse.urlparse(params['path']).scheme + if protocol not in libvirt_stream_protocols: + # return qemucmdline XML + return qemu_stream_cmdline % params - url = self.info['cdrom'] - if not qemu_stream_dns: - hostname = socket.gethostbyname(hostname) - url = protocol + "://" + hostname + ":" + str(port) + url_path - - if protocol not in libvirt_stream_protocols: - return qemu_stream_cmdline % {'url': url, 'bus': bus} - - params = {'protocol': protocol, 'url_path': url_path, - 'hostname': hostname, 'port': port, 'dev': dev, 'bus': bus} - - return network_file % (params) + dev, xml = get_disk_xml(params) + return xml def _get_disks_xml(self, vm_uuid): storage_path = self._get_storage_path() -- 1.9.3

Tested-by: Paulo Vital <pvital@linux.vnet.ibm.com> Reviewed-by: Paulo Vital <pvital@linux.vnet.ibm.com> On Thu, 2014-10-30 at 10:26 -0200, Aline Manera wrote:
V2 -> V3: - Create check_remote_disk_path() on model/utils.py to verify the necessity to convert hostname to IP
V1 -> V2: - Make xmlutils/disk.py independent of model instances
Aline Manera (8): Move _get_storage_xml() to xmlutils/disk.py Move vmdisks.py functions to xmlutils/disk.py Remove 'bus' paramater from /vms/<name>/storages documentation Update get_disk_xml() to get the device same according to bus and index values Remove ignore_src parameter from get_disk_xml() Get disk type according to file path on get_disk_xml() Check QEMU stream DNS capability when attaching new disk to guest Update vmtemplate.py to use get_disk_xml() while generating CDROM XML
docs/API.md | 1 - src/kimchi/model/storagevolumes.py | 8 +- src/kimchi/model/utils.py | 19 ++++- src/kimchi/model/vmstorages.py | 144 +++++++++------------------------ src/kimchi/vmdisks.py | 75 ------------------ src/kimchi/vmtemplate.py | 74 ++++++----------- src/kimchi/xmlutils/disk.py | 158 +++++++++++++++++++++++++++++++++++++ tests/test_model.py | 11 ++- 8 files changed, 248 insertions(+), 242 deletions(-) delete mode 100644 src/kimchi/vmdisks.py create mode 100644 src/kimchi/xmlutils/disk.py
participants (2)
-
Aline Manera
-
Paulo Ricardo Paz Vital