Re: [Kimchi-devel] [PATCH] Improve code to get default disk format for VMTemplate

The default template configuration is defined in the template.conf file. This file is parsed on server start up and the result is always returned in the osinfo.lookup() method. That means when creating a new VMTemplate instance it will already contain all the default values to use for the Template creation. The disks are in a dict format like below: [{u'index': 0, u'size': 10, u'format': u'qcow2'}] Each disk has its own size and format and we just need to use them. We don't need to parse the template.conf file again in order to get the right values. Also adjust the tests according to those changes. Signed-off-by: Aline Manera <alinefm(a)linux.vnet.ibm.com&gt; --- src/kimchi/vmtemplate.py | 40 ++++++++++++++++++---------------------- tests/test_model.py | 9 ++++++--- tests/test_template.py | 8 ++++---- 3 files changed, 28 insertions(+), 29 deletions(-) diff --git a/src/kimchi/vmtemplate.py b/src/kimchi/vmtemplate.py index a165a5d..56a0de8 100644 --- a/src/kimchi/vmtemplate.py +++ b/src/kimchi/vmtemplate.py @@ -23,13 +23,11 @@ import time import urlparse import uuid -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 @@ -64,23 +62,31 @@ class VMTemplate(object): entry = osinfo.lookup(os_distro, os_version) self.info.update(entry) - # Auto-generate a template name and no one is passed + # Auto-generate a template name if no one is passed if 'name' not in args or args['name'] == '': args['name'] = self._gen_name(distro, version) self.name = args['name'] - # Override with the passed in parameters + # Merge graphics settings graph_args = args.get('graphics') if graph_args: graphics = dict(self.info['graphics']) graphics.update(graph_args) args['graphics'] = graphics - self.info.update(args) - # Assign right disk format to logical and [i]scsi storagepools - if self._get_storage_type() in ['logical', 'iscsi', 'scsi']: - for i, disk in enumerate(self.info['disks']): - self.info['disks'][i]['format'] = 'raw' + # Merge disks dict + default_disk = self.info['disks'][0] + for i, d in enumerate(args.get('disks', [])): + disk = dict(default_disk) + disk.update(d) + + # Assign right disk format to logical and [i]scsi storagepools + if self._get_storage_type() in ['logical', 'iscsi', 'scsi']: + disk['format'] = 'raw' + args['disks'][i] = disk + + # Override template values according to 'args' + self.info.update(args) def _get_os_info(self, args, scan): distro = version = 'unknown' @@ -159,14 +165,6 @@ class VMTemplate(object): dev, xml = get_disk_xml(params) return xml - @staticmethod - def get_default_disk0_format(): - 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) @@ -174,8 +172,7 @@ class VMTemplate(object): storage_path = self._get_storage_path() base_disk_params = {'type': 'disk', 'disk': 'file', - 'bus': self.info['disk_bus'], - 'format': self.get_default_disk0_format()} + 'bus': self.info['disk_bus']} logical_disk_params = {'format': 'raw'} iscsi_disk_params = {'disk': 'block', 'format': 'raw'} @@ -187,7 +184,7 @@ class VMTemplate(object): pool_name = pool_name_from_uri(self.info['storagepool']) for index, disk in enumerate(self.info['disks']): params = dict(base_disk_params) - params['format'] = disk.get('format', params['format']) + params['format'] = disk['format'] params.update(locals().get('%s_disk_params' % storage_type, {})) params['index'] = index @@ -203,7 +200,6 @@ 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() ret = [] for i, d in enumerate(self.info['disks']): @@ -212,7 +208,7 @@ class VMTemplate(object): info = {'name': volume, 'capacity': d['size'], - 'format': d.get('format', default_vol_format), + 'format': d['format'], 'path': '%s/%s' % (storage_path, volume)} if 'logical' == self._get_storage_type() or \ diff --git a/tests/test_model.py b/tests/test_model.py index 717b9bc..7ca823a 100644 --- a/tests/test_model.py +++ b/tests/test_model.py @@ -32,11 +32,11 @@ import iso_gen import kimchi.objectstore import utils from kimchi import netinfo +from kimchi import osinfo from kimchi.basemodel import Singleton 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 @@ -386,7 +386,7 @@ class ModelTests(unittest.TestCase): def test_vm_disk(self): disk_path = os.path.join(TMP_DIR, 'existent2.iso') open(disk_path, 'w').close() - modern_disk_bus = get_template_default('modern', 'disk_bus') + modern_disk_bus = osinfo.get_template_default('modern', 'disk_bus') def _attach_disk(expect_bus=modern_disk_bus): disk_args = {"type": "disk", @@ -485,7 +485,7 @@ class ModelTests(unittest.TestCase): rollback.prependDefer(inst.vm_delete, vm_name) # Need to check the right disk_bus for old distro - disk = _attach_disk(get_template_default('old', 'disk_bus')) + disk = _attach_disk(osinfo.get_template_default('old', 'disk_bus')) inst.vmstorage_delete('kimchi-ide-bus-vm', disk) # Hot plug IDE bus disk does not work @@ -620,11 +620,14 @@ class ModelTests(unittest.TestCase): with open(config_file, 'w') as f: f.write(conf_file_data) + osinfo.defaults = osinfo._get_tmpl_defaults() + 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) + osinfo.defaults = osinfo._get_tmpl_defaults() def _get_disk_format_from_vm(self, vm, conn): dom = VMModel.get_vm(vm, conn) diff --git a/tests/test_template.py b/tests/test_template.py index 83561c1..48ef229 100644 --- a/tests/test_template.py +++ b/tests/test_template.py @@ -24,7 +24,7 @@ import unittest from functools import partial -from kimchi.vmtemplate import VMTemplate +from kimchi import osinfo from kimchi.config import READONLY_POOL_TYPE from kimchi.mockmodel import MockModel from utils import get_free_port, patch_auth, request, run_server @@ -88,7 +88,7 @@ class TemplateTests(unittest.TestCase): self.assertEquals(sorted(tmpl.keys()), sorted(keys)) # Verify if default disk format was configured - default_disk_format = VMTemplate.get_default_disk0_format() + default_disk_format = osinfo.defaults['disks'][0]['format'] self.assertEquals(tmpl['disks'][0]['format'], default_disk_format) # Clone a template @@ -199,8 +199,8 @@ class TemplateTests(unittest.TestCase): self.assertEquals(update_tmpl['cdrom'], cdrom_data['cdrom']) # Update disks - disk_data = {'disks': [{'index': 0, 'size': 10}, - {'index': 1, 'size': 20}]} + disk_data = {'disks': [{'index': 0, 'size': 10, 'format': 'raw'}, + {'index': 1, 'size': 20, 'format': 'raw'}]} resp = self.request(new_tmpl_uri, json.dumps(disk_data), 'PUT') self.assertEquals(200, resp.status) resp = self.request(new_tmpl_uri)