[PATCH 0/7] Use the common get_disk_xml() to generate VMTemplate disks
Aline Manera (7): Make disk type an optional parameter on get_disk_xml() Set guest disk cache to none to support live migration Change VMTemplate._get_scsi_disks_xml() to use the common get_disk_xml() Change VMTemplate._get_iscsi_disks_xml() to use the common get_disk_xml() Change VMTemplate._get_disks_xml() to use the common get_disk_xml() Change VMTemplate._get_disks_xml() to handle all type of disks Remove VMTemplate._get_scsi_disks_xml() and VMTemplate._get_iscsi_disks_xml() src/kimchi/model/vmstorages.py | 1 + src/kimchi/vmtemplate.py | 108 ++++++++++++----------------------------- src/kimchi/xmlutils/disk.py | 10 +++- tests/test_model.py | 3 +- 4 files changed, 42 insertions(+), 80 deletions(-) -- 1.9.3
There are cases in which the disk file does not exist but we know prior to it what is the disk type so in this case the disk type should be read from a parameter. So only when no value for disk type is passed, we get the disk type according to source path. Signed-off-by: Aline Manera <alinefm@linux.vnet.ibm.com> --- src/kimchi/model/vmstorages.py | 1 + src/kimchi/xmlutils/disk.py | 4 +++- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/src/kimchi/model/vmstorages.py b/src/kimchi/model/vmstorages.py index 72f6bfd..955a20a 100644 --- a/src/kimchi/model/vmstorages.py +++ b/src/kimchi/model/vmstorages.py @@ -120,6 +120,7 @@ class VMStoragesModel(object): params['format'] = vol_info['format'] params['path'] = vol_info['path'] + params['disk'] = vol_info['type'] params.update(self._get_available_bus_address(params['bus'], vm_name)) params['path'] = check_remote_disk_path(params['path']) diff --git a/src/kimchi/xmlutils/disk.py b/src/kimchi/xmlutils/disk.py index f066dbe..2520929 100644 --- a/src/kimchi/xmlutils/disk.py +++ b/src/kimchi/xmlutils/disk.py @@ -46,7 +46,9 @@ def get_disk_xml(params): </disk> """ path = params['path'] - disk_type = _get_disk_type(path) if len(path) > 0 else 'file' + disk_type = params.get('disk', None) + if disk_type is None: + 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'])) -- 1.9.3
According to commit 707aee09 the configuration "cache=none" must be set to allow live migration. So do it on get_disk_xml() to make sure all guest disks will have this configuration. The test needed to be updated because libvirt uses O_DIRECT flag when cache=none to read the disk file and it is not supported on tmpfs. For reference: https://www.redhat.com/archives/libvir-list/2013-August/msg00783.html Signed-off-by: Aline Manera <alinefm@linux.vnet.ibm.com> --- src/kimchi/xmlutils/disk.py | 6 +++++- tests/test_model.py | 3 ++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/src/kimchi/xmlutils/disk.py b/src/kimchi/xmlutils/disk.py index 2520929..64e243a 100644 --- a/src/kimchi/xmlutils/disk.py +++ b/src/kimchi/xmlutils/disk.py @@ -50,7 +50,11 @@ def get_disk_xml(params): if disk_type is None: 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'])) + driver = E.driver(name='qemu', type=params['format']) + if params['type'] != 'cdrom': + driver.set('cache', 'none') + + disk.append(driver) # Get device name according to bus and index values dev = params.get('dev', (BUS_TO_DEV_MAP[params['bus']] + diff --git a/tests/test_model.py b/tests/test_model.py index 563e80a..8fbb642 100644 --- a/tests/test_model.py +++ b/tests/test_model.py @@ -267,12 +267,13 @@ class ModelTests(unittest.TestCase): inst = model.Model(objstore_loc=self.tmp_store) with RollbackContext() as rollback: - path = '/tmp/kimchi-images' + path = os.path.join(os.getcwd(), 'kimchi-images') pool = 'test-pool' vol = 'test-volume.img' vol_path = "%s/%s" % (path, vol) if not os.path.exists(path): os.mkdir(path) + rollback.prependDefer(shutil.rmtree, path) args = {'name': pool, 'path': path, -- 1.9.3
Signed-off-by: Aline Manera <alinefm@linux.vnet.ibm.com> --- src/kimchi/vmtemplate.py | 28 +++++++++++----------------- 1 file changed, 11 insertions(+), 17 deletions(-) diff --git a/src/kimchi/vmtemplate.py b/src/kimchi/vmtemplate.py index 0541106..04b2cc2 100644 --- a/src/kimchi/vmtemplate.py +++ b/src/kimchi/vmtemplate.py @@ -193,27 +193,21 @@ class VMTemplate(object): return graphics_xml def _get_scsi_disks_xml(self): + ret = "" luns = [disk['volume'] for disk in self.info.get('disks', {}) if 'volume' in disk] - ret = "" - # Passthrough configuration - disk_xml = """ - <disk type='volume' device='lun'> - <driver name='qemu' type='raw' cache='none'/> - <source dev='%(src)s'/> - <target dev='%(dev)s' bus='scsi'/> - </disk>""" - if not self.fc_host_support: - disk_xml = disk_xml.replace('volume', 'block') - - pool = self._storage_validate() - # Creating disk xml for each lun passed + pool_name = pool_name_from_uri(self.info['storagepool']) for index, lun in enumerate(luns): - path = pool.storageVolLookupByName(lun).path() - dev = "sd%s" % string.lowercase[index] - params = {'src': path, 'dev': dev} - ret = ret + disk_xml % params + params = {} + params['disk'] = 'volume' if self.fc_host_support else 'block' + params['type'] = 'lun' + params['format'] = 'raw' + params['bus'] = 'scsi' + params['index'] = index + params['path'] = self._get_volume_path(pool_name, lun) + ret += get_disk_xml(params)[1] + return ret def _get_iscsi_disks_xml(self): -- 1.9.3
Signed-off-by: Aline Manera <alinefm@linux.vnet.ibm.com> --- src/kimchi/vmtemplate.py | 24 ++++++++---------------- 1 file changed, 8 insertions(+), 16 deletions(-) diff --git a/src/kimchi/vmtemplate.py b/src/kimchi/vmtemplate.py index 04b2cc2..915f826 100644 --- a/src/kimchi/vmtemplate.py +++ b/src/kimchi/vmtemplate.py @@ -18,7 +18,6 @@ # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA import os -import string import socket import time import urlparse @@ -211,24 +210,17 @@ class VMTemplate(object): return ret def _get_iscsi_disks_xml(self): - def build_disk_xml(children=[]): - disk = E.disk(type='block', device='disk') - disk.extend(children) - return etree.tostring(disk) - ret = "" - children = [] - children.append(E.driver(name='qemu', type='raw')) - disk_bus = self.info['disk_bus'] - dev_prefix = self._bus_to_dev[disk_bus] pool_name = pool_name_from_uri(self.info['storagepool']) for i, d in enumerate(self.info['disks']): - source = E.source(dev=self._get_volume_path(pool_name, - d.get('volume'))) - # FIXME if more than 26 disks - target = E.target(dev=dev_prefix + string.lowercase[i], - bus=disk_bus) - ret += build_disk_xml(children+[source, target]) + params = {} + params['disk'] = 'block' + params['type'] = 'disk' + params['format'] = 'raw' + params['bus'] = self.info['disk_bus'] + params['index'] = i + params['path'] = self._get_volume_path(pool_name, d.get('volume')) + ret += get_disk_xml(params) return ret -- 1.9.3
Signed-off-by: Aline Manera <alinefm@linux.vnet.ibm.com> --- src/kimchi/vmtemplate.py | 30 ++++++++++++++---------------- 1 file changed, 14 insertions(+), 16 deletions(-) diff --git a/src/kimchi/vmtemplate.py b/src/kimchi/vmtemplate.py index 915f826..45f49c7 100644 --- a/src/kimchi/vmtemplate.py +++ b/src/kimchi/vmtemplate.py @@ -153,24 +153,22 @@ class VMTemplate(object): return xml def _get_disks_xml(self, vm_uuid): - storage_path = self._get_storage_path() ret = "" + storage_path = self._get_storage_path() + storage_type = self._get_storage_type() + for i, disk in enumerate(self.info['disks']): - index = disk.get('index', i) - volume = "%s-%s.img" % (vm_uuid, index) - src = os.path.join(storage_path, volume) - dev = "%s%s" % (self._bus_to_dev[self.info['disk_bus']], - string.lowercase[index]) - fmt = 'raw' if self._get_storage_type() in ['logical'] else 'qcow2' - params = {'src': src, 'dev': dev, 'bus': self.info['disk_bus'], - 'type': fmt} - ret += """ - <disk type='file' device='disk'> - <driver name='qemu' type='%(type)s' cache='none'/> - <source file='%(src)s' /> - <target dev='%(dev)s' bus='%(bus)s' /> - </disk> - """ % params + params = {} + params['type'] = 'disk' + params['disk'] = 'file' + params['index'] = disk.get('index', i) + params['bus'] = self.info['disk_bus'] + volume = "%s-%s.img" % (vm_uuid, params['index']) + params['path'] = os.path.join(storage_path, volume) + params['format'] = 'raw' if storage_type in ['logical'] \ + else 'qcow2' + ret += get_disk_xml(params)[1] + return ret def _get_graphics_xml(self, params): -- 1.9.3
Signed-off-by: Aline Manera <alinefm@linux.vnet.ibm.com> --- src/kimchi/vmtemplate.py | 53 ++++++++++++++++++++++++++---------------------- 1 file changed, 29 insertions(+), 24 deletions(-) diff --git a/src/kimchi/vmtemplate.py b/src/kimchi/vmtemplate.py index 45f49c7..7f7a907 100644 --- a/src/kimchi/vmtemplate.py +++ b/src/kimchi/vmtemplate.py @@ -153,23 +153,37 @@ class VMTemplate(object): return xml def _get_disks_xml(self, vm_uuid): - ret = "" - storage_path = self._get_storage_path() + # Current implementation just allows to create disk in one single + # storage pool, so we cannot mix the types (scsi volumes vs img file) storage_type = self._get_storage_type() + storage_path = self._get_storage_path() - for i, disk in enumerate(self.info['disks']): - params = {} - params['type'] = 'disk' - params['disk'] = 'file' - params['index'] = disk.get('index', i) - params['bus'] = self.info['disk_bus'] - volume = "%s-%s.img" % (vm_uuid, params['index']) - params['path'] = os.path.join(storage_path, volume) - params['format'] = 'raw' if storage_type in ['logical'] \ - else 'qcow2' - ret += get_disk_xml(params)[1] + base_disk_params = {'type': 'disk', 'disk': 'file', + 'bus': self.info['disk_bus'], 'format': 'qcow2'} + logical_disk_params = {'format': 'raw'} + iscsi_disk_params = {'disk': 'block', 'format': 'raw'} - return ret + scsi_disk = 'volume' if self.fc_host_support else 'block' + scsi_disk_params = {'disk': scsi_disk, 'type': 'lun', + 'format': 'raw', 'bus': 'scsi'} + + disks_xml = '' + pool_name = pool_name_from_uri(self.info['storagepool']) + for index, disk in enumerate(self.info['disks']): + params = dict(base_disk_params) + params.update(locals().get('%s_disk_params' % storage_type, {})) + params['index'] = index + + volume = disk.get('volume') + if volume is not None: + params['path'] = self._get_volume_path(pool_name, volume) + else: + volume = "%s-%s.img" % (vm_uuid, params['index']) + params['path'] = os.path.join(storage_path, volume) + + disks_xml += get_disk_xml(params)[1] + + return disks_xml def _get_graphics_xml(self, params): graphics_xml = """ @@ -334,16 +348,7 @@ class VMTemplate(object): graphics = kwargs.get('graphics') params['graphics'] = self._get_graphics_xml(graphics) params['cpu_info'] = self._get_cpu_xml() - - # Current implementation just allows to create disk in one single - # storage pool, so we cannot mix the types (scsi volumes vs img file) - storage_type = self._get_storage_type() - if storage_type == "iscsi": - params['disks'] = self._get_iscsi_disks_xml() - elif storage_type == "scsi": - params['disks'] = self._get_scsi_disks_xml() - else: - params['disks'] = self._get_disks_xml(vm_uuid) + params['disks'] = self._get_disks_xml(vm_uuid) qemu_stream_dns = kwargs.get('qemu_stream_dns', False) libvirt_stream_protocols = kwargs.get('libvirt_stream_protocols', []) -- 1.9.3
VMTemplate._get_disks_xml() handles all type of disks so those functions are not needed anymore. Signed-off-by: Aline Manera <alinefm@linux.vnet.ibm.com> --- src/kimchi/vmtemplate.py | 35 ----------------------------------- 1 file changed, 35 deletions(-) diff --git a/src/kimchi/vmtemplate.py b/src/kimchi/vmtemplate.py index 7f7a907..f9b2b42 100644 --- a/src/kimchi/vmtemplate.py +++ b/src/kimchi/vmtemplate.py @@ -37,8 +37,6 @@ from kimchi.xmlutils.qemucmdline import get_qemucmdline_xml class VMTemplate(object): - _bus_to_dev = {'ide': 'hd', 'virtio': 'vd', 'scsi': 'sd'} - def __init__(self, args, scan=False): """ Construct a VM Template from a widely variable amount of information. @@ -203,39 +201,6 @@ class VMTemplate(object): graphics_xml = graphics_xml + spicevmc_xml return graphics_xml - def _get_scsi_disks_xml(self): - ret = "" - luns = [disk['volume'] for disk in self.info.get('disks', {}) - if 'volume' in disk] - - pool_name = pool_name_from_uri(self.info['storagepool']) - for index, lun in enumerate(luns): - params = {} - params['disk'] = 'volume' if self.fc_host_support else 'block' - params['type'] = 'lun' - params['format'] = 'raw' - params['bus'] = 'scsi' - params['index'] = index - params['path'] = self._get_volume_path(pool_name, lun) - ret += get_disk_xml(params)[1] - - return ret - - def _get_iscsi_disks_xml(self): - ret = "" - pool_name = pool_name_from_uri(self.info['storagepool']) - for i, d in enumerate(self.info['disks']): - params = {} - params['disk'] = 'block' - params['type'] = 'disk' - params['format'] = 'raw' - params['bus'] = self.info['disk_bus'] - params['index'] = i - params['path'] = self._get_volume_path(pool_name, d.get('volume')) - ret += get_disk_xml(params) - - return ret - def to_volume_list(self, vm_uuid): storage_path = self._get_storage_path() fmt = 'raw' if self._get_storage_type() in ['logical'] else 'qcow2' -- 1.9.3
Reviewed-by: Daniel Barboza <danielhb@linux.vnet.ibm.com> Just a comment: this patch [PATCH 5/7] Change VMTemplate._get_disks_xml() to use the common get_disk_xml() will need to be rebased with the changes of the disk format in template. On 10/31/2014 03:22 PM, Aline Manera wrote:
Aline Manera (7): Make disk type an optional parameter on get_disk_xml() Set guest disk cache to none to support live migration Change VMTemplate._get_scsi_disks_xml() to use the common get_disk_xml() Change VMTemplate._get_iscsi_disks_xml() to use the common get_disk_xml() Change VMTemplate._get_disks_xml() to use the common get_disk_xml() Change VMTemplate._get_disks_xml() to handle all type of disks Remove VMTemplate._get_scsi_disks_xml() and VMTemplate._get_iscsi_disks_xml()
src/kimchi/model/vmstorages.py | 1 + src/kimchi/vmtemplate.py | 108 ++++++++++++----------------------------- src/kimchi/xmlutils/disk.py | 10 +++- tests/test_model.py | 3 +- 4 files changed, 42 insertions(+), 80 deletions(-)
On 10/31/2014 06:09 PM, Daniel H Barboza wrote:
Reviewed-by: Daniel Barboza <danielhb@linux.vnet.ibm.com>
Just a comment: this patch
[PATCH 5/7] Change VMTemplate._get_disks_xml() to use the common get_disk_xml()
will need to be rebased with the changes of the disk format in template.
Thanks, Daniel! The rebased patch set can be found on my branch if you want to test it. Repository; https://github.com/alinefm/kimchi Branch: alinefm/vmtemplate-refactor
On 10/31/2014 03:22 PM, Aline Manera wrote:
Aline Manera (7): Make disk type an optional parameter on get_disk_xml() Set guest disk cache to none to support live migration Change VMTemplate._get_scsi_disks_xml() to use the common get_disk_xml() Change VMTemplate._get_iscsi_disks_xml() to use the common get_disk_xml() Change VMTemplate._get_disks_xml() to use the common get_disk_xml() Change VMTemplate._get_disks_xml() to handle all type of disks Remove VMTemplate._get_scsi_disks_xml() and VMTemplate._get_iscsi_disks_xml()
src/kimchi/model/vmstorages.py | 1 + src/kimchi/vmtemplate.py | 108 ++++++++++++----------------------------- src/kimchi/xmlutils/disk.py | 10 +++- tests/test_model.py | 3 +- 4 files changed, 42 insertions(+), 80 deletions(-)
_______________________________________________ Kimchi-devel mailing list Kimchi-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/kimchi-devel
On 11/02/2014 11:53 AM, Aline Manera wrote:
The rebased patch set can be found on my branch if you want to test it.
Repository; https://github.com/alinefm/kimchi Branch: alinefm/vmtemplate-refactor
Reviewed-by: Daniel Barboza <danielhb@linux.vnet.ibm.com> Tested-by: Daniel Barboza <danielhb@linux.vnet.ibm.com>
Applied. Thanks. Regards, Aline Manera
participants (2)
-
Aline Manera -
Daniel H Barboza