[PATCH 0/2] Template disk format fixes
From: Daniel Henrique Barboza <dhbarboza82@gmail.com> Kimchi does not obey the value set by the template, always creating a disk of format 'qcow2' even if the user set another format. This patchset fixes this issue by: - always set the disk format with the value the user set, unless there is a storagepool restrition - getting the default disk format type in template.conf. This value is optional and will be used in the case the user does not provide any other value. - if there is no user choice and no default set in template.conf, the default disk format will be 'qcow2'. See the code and unit tests for more info. Daniel Henrique Barboza (2): Getting default disk format from template.conf New unit tests to check disk creation behavior src/kimchi/vmtemplate.py | 19 +++++++-- tests/test_model.py | 106 ++++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 120 insertions(+), 5 deletions(-) -- 2.4.3
From: Daniel Henrique Barboza <dhbarboza82@gmail.com> If the user change disk format in template.conf, templates are going to be created correctly and vm xml as well. But, when the disk is created, the format is always qcow2. This patch considers the settings in the template.conf to define the default volume format when creating the disks. User defined settings will override any default (unless the storage type is logical, iscsi or scsi. In those cases the format will always be 'raw'). If the user does not provide any format and template.conf does not declare any default as well, the default value to be used as volume type will be 'qcow2' (considering the storage type restriction mentioned above). Signed-off-by: Daniel Henrique Barboza <dhbarboza82@gmail.com> --- src/kimchi/vmtemplate.py | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/src/kimchi/vmtemplate.py b/src/kimchi/vmtemplate.py index 3e42971..1ad9131 100644 --- a/src/kimchi/vmtemplate.py +++ b/src/kimchi/vmtemplate.py @@ -24,11 +24,13 @@ import urlparse import uuid import platform +from configobj import ConfigObj from lxml import etree from lxml.builder import E from kimchi import imageinfo from kimchi import osinfo +from kimchi.config import paths from kimchi.exception import InvalidParameter, IsoFormatError, MissingParameter from kimchi.exception import ImageFormatError, OperationFailed from kimchi.isoinfo import IsoImage @@ -158,6 +160,13 @@ class VMTemplate(object): dev, xml = get_disk_xml(params) return xml + def _get_default_disk0_format(self): + config_file = os.path.join(paths.conf_dir, 'template.conf') + config = ConfigObj(config_file) + + default_vol_format = config['storage']['disk.0'].get('format', 'qcow2') + return default_vol_format + def _get_disks_xml(self, vm_uuid): # Current implementation just allows to create disk in one single # storage pool, so we cannot mix the types (scsi volumes vs img file) @@ -165,7 +174,9 @@ class VMTemplate(object): storage_path = self._get_storage_path() base_disk_params = {'type': 'disk', 'disk': 'file', - 'bus': self.info['disk_bus'], 'format': 'qcow2'} + 'bus': self.info['disk_bus'], + 'format': self._get_default_disk0_format()} + logical_disk_params = {'format': 'raw'} iscsi_disk_params = {'disk': 'block', 'format': 'raw'} @@ -193,8 +204,8 @@ class VMTemplate(object): return unicode(disks_xml, 'utf-8') def to_volume_list(self, vm_uuid): + default_vol_format = self._get_default_disk0_format() storage_path = self._get_storage_path() - fmt = 'raw' if self._get_storage_type() in ['logical'] else 'qcow2' ret = [] for i, d in enumerate(self.info['disks']): index = d.get('index', i) @@ -202,11 +213,11 @@ class VMTemplate(object): info = {'name': volume, 'capacity': d['size'], - 'format': fmt, + 'format': d.get('format', default_vol_format), 'path': '%s/%s' % (storage_path, volume)} if 'logical' == self._get_storage_type() or \ - fmt not in ['qcow2', 'raw']: + info['format'] not in ['qcow2', 'raw']: info['allocation'] = info['capacity'] else: info['allocation'] = 0 -- 2.4.3
From: Daniel Henrique Barboza <dhbarboza82@gmail.com> Three new unit tests were added: - check that the vm template creates always the disk format set by the user, doesn't matter the default value from template.conf - check that the vm template creates a disk with format set by the default valuein the disk.0 entry of template.conf, if the user didn't provide any other. - check that the vm template creates a disk with format 'qcow2' if the user didn't provide any other and there is no default value in template.conf Signed-off-by: Daniel Henrique Barboza <dhbarboza82@gmail.com> --- tests/test_model.py | 106 +++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 105 insertions(+), 1 deletion(-) diff --git a/tests/test_model.py b/tests/test_model.py index 896e29b..ef8bb25 100644 --- a/tests/test_model.py +++ b/tests/test_model.py @@ -33,14 +33,16 @@ import kimchi.objectstore import utils from kimchi import netinfo from kimchi.basemodel import Singleton -from kimchi.config import config +from kimchi.config import config, paths from kimchi.exception import InvalidOperation from kimchi.exception import InvalidParameter, NotFoundError, OperationFailed from kimchi.osinfo import get_template_default from kimchi.model import model from kimchi.model.libvirtconnection import LibvirtConnection +from kimchi.model.vms import VMModel from kimchi.rollbackcontext import RollbackContext from kimchi.utils import add_task +from kimchi.xmlutils.utils import xpath_get_text invalid_repository_urls = ['www.fedora.org', # missing protocol @@ -600,6 +602,108 @@ class ModelTests(unittest.TestCase): self.assertTrue(os.access(disk_path, os.F_OK)) self.assertFalse(os.access(disk_path, os.F_OK)) + def _create_template_conf_with_disk_format(self, vol_format): + if vol_format is None: + conf_file_data = "[main]\n\n[storage]\n\n[[disk.0]]\n" \ + "#format = \n\n[graphics]\n\n[processor]\n" + else: + conf_file_data = "[main]\n\n[storage]\n\n[[disk.0]]\n" \ + "format = %s\n\n[graphics]\n\n[processor]\n"\ + % vol_format + + config_file = os.path.join(paths.conf_dir, 'template.conf') + config_bkp_file = \ + os.path.join(paths.conf_dir, 'template.conf-unit_test_bkp') + + os.rename(config_file, config_bkp_file) + + with open(config_file, 'w') as f: + f.write(conf_file_data) + + def _restore_template_conf_file(self): + config_file = os.path.join(paths.conf_dir, 'template.conf') + config_bkp_file = \ + os.path.join(paths.conf_dir, 'template.conf-unit_test_bkp') + os.rename(config_bkp_file, config_file) + + def _get_disk_format_from_vm(self, vm, conn): + dom = VMModel.get_vm(vm, conn) + xml = dom.XMLDesc(0) + xpath = "/domain/devices/disk[@device='disk']/driver/@type" + return xpath_get_text(xml, xpath)[0] + + @unittest.skipUnless(utils.running_as_root(), 'Must be run as root') + def test_template_get_default_vol_format_from_conf(self): + inst = model.Model(objstore_loc=self.tmp_store) + + with RollbackContext() as rollback: + self._create_template_conf_with_disk_format('vmdk') + rollback.prependDefer(self._restore_template_conf_file) + + params = {'name': 'test', 'disks': [{'size': 1}], + 'cdrom': UBUNTU_ISO} + inst.templates_create(params) + rollback.prependDefer(inst.template_delete, 'test') + + params = {'name': 'test-vm-1', 'template': '/templates/test'} + task = inst.vms_create(params) + inst.task_wait(task['id']) + rollback.prependDefer(inst.vm_delete, 'test-vm-1') + + created_disk_format = self._get_disk_format_from_vm( + 'test-vm-1', inst.conn + ) + self.assertEqual(created_disk_format, 'vmdk') + + @unittest.skipUnless(utils.running_as_root(), 'Must be run as root') + def test_template_creates_user_defined_vol_format_instead_default(self): + inst = model.Model(objstore_loc=self.tmp_store) + + default_vol = 'vmdk' + user_vol = 'raw' + with RollbackContext() as rollback: + self._create_template_conf_with_disk_format(default_vol) + rollback.prependDefer(self._restore_template_conf_file) + + params = {'name': 'test', + 'disks': [{'size': 1, 'format': user_vol}], + 'cdrom': UBUNTU_ISO} + inst.templates_create(params) + rollback.prependDefer(inst.template_delete, 'test') + + params = {'name': 'test-vm-1', 'template': '/templates/test'} + task = inst.vms_create(params) + inst.task_wait(task['id']) + rollback.prependDefer(inst.vm_delete, 'test-vm-1') + + created_disk_format = self._get_disk_format_from_vm( + 'test-vm-1', inst.conn + ) + self.assertEqual(created_disk_format, user_vol) + + @unittest.skipUnless(utils.running_as_root(), 'Must be run as root') + def test_template_uses_qcow2_format_if_no_user_or_default_defined(self): + inst = model.Model(objstore_loc=self.tmp_store) + + with RollbackContext() as rollback: + self._create_template_conf_with_disk_format(None) + rollback.prependDefer(self._restore_template_conf_file) + + params = {'name': 'test', + 'disks': [{'size': 1}], 'cdrom': UBUNTU_ISO} + inst.templates_create(params) + rollback.prependDefer(inst.template_delete, 'test') + + params = {'name': 'test-vm-1', 'template': '/templates/test'} + task = inst.vms_create(params) + inst.task_wait(task['id']) + rollback.prependDefer(inst.vm_delete, 'test-vm-1') + + created_disk_format = self._get_disk_format_from_vm( + 'test-vm-1', inst.conn + ) + self.assertEqual(created_disk_format, 'qcow2') + def test_vm_memory_hotplug(self): config.set("authentication", "method", "pam") inst = model.Model(None, objstore_loc=self.tmp_store) -- 2.4.3
Reviewed-by: Jose Ricardo Ziviani <joserz@linux.vnet.ibm.com> On 15-07-2015 18:35, dhbarboza82@gmail.com wrote:
From: Daniel Henrique Barboza <dhbarboza82@gmail.com>
Kimchi does not obey the value set by the template, always creating a disk of format 'qcow2' even if the user set another format.
This patchset fixes this issue by:
- always set the disk format with the value the user set, unless there is a storagepool restrition
- getting the default disk format type in template.conf. This value is optional and will be used in the case the user does not provide any other value.
- if there is no user choice and no default set in template.conf, the default disk format will be 'qcow2'.
See the code and unit tests for more info.
Daniel Henrique Barboza (2): Getting default disk format from template.conf New unit tests to check disk creation behavior
src/kimchi/vmtemplate.py | 19 +++++++-- tests/test_model.py | 106 ++++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 120 insertions(+), 5 deletions(-)
-- Jose Ricardo Ziviani ----------------------------- Software Engineer Linux Technology Center - IBM
Patch set applied to master branch of https://github.com/danielhb/kimchi Thanks for the review! On 07/16/2015 09:04 AM, Jose Ricardo Ziviani wrote:
Reviewed-by: Jose Ricardo Ziviani <joserz@linux.vnet.ibm.com>
On 15-07-2015 18:35, dhbarboza82@gmail.com wrote:
From: Daniel Henrique Barboza <dhbarboza82@gmail.com>
Kimchi does not obey the value set by the template, always creating a disk of format 'qcow2' even if the user set another format.
This patchset fixes this issue by:
- always set the disk format with the value the user set, unless there is a storagepool restrition
- getting the default disk format type in template.conf. This value is optional and will be used in the case the user does not provide any other value.
- if there is no user choice and no default set in template.conf, the default disk format will be 'qcow2'.
See the code and unit tests for more info.
Daniel Henrique Barboza (2): Getting default disk format from template.conf New unit tests to check disk creation behavior
src/kimchi/vmtemplate.py | 19 +++++++-- tests/test_model.py | 106 ++++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 120 insertions(+), 5 deletions(-)
participants (4)
-
Aline Manera -
Daniel Henrique Barboza -
dhbarboza82＠gmail.com
-
Jose Ricardo Ziviani