Re: [Kimchi-devel] [PATCH 2/5 - v4] Enable create guests with multiple disks from different pools

On 30/11/2015 11:42, Rodrigo Trujillo wrote: > > > On 11/30/2015 09:08 AM, Aline Manera wrote: >> >> >> On 30/11/2015 01:29, Rodrigo Trujillo wrote: >>> This patch changes the Kimchi backend in order to support creation of >>> guests from Templates that have multiple disks from different storage >>> pools. >>> Before, it was only possible to create a guest with one, two or more >>> disks from only one storagepool. In order to add a disk from another >>> pool, user would have to edit the guest and add it. >>> Now users can do this in creation time, using pre-configured Template. >>> >>> Signed-off-by: Rodrigo Trujillo <rodrigo.trujillo(a)linux.vnet.ibm.com&gt; >>> --- >>> src/wok/plugins/kimchi/i18n.py | 1 + >>> src/wok/plugins/kimchi/model/templates.py | 21 +++++++++++++++------ >>> src/wok/plugins/kimchi/model/vms.py | 18 ++++++------------ >>> src/wok/plugins/kimchi/vmtemplate.py | 30 >>> ++++++++++++++++-------------- >>> 4 files changed, 38 insertions(+), 32 deletions(-) >>> >>> diff --git a/src/wok/plugins/kimchi/i18n.py >>> b/src/wok/plugins/kimchi/i18n.py >>> index de7962a..b18048c 100644 >>> --- a/src/wok/plugins/kimchi/i18n.py >>> +++ b/src/wok/plugins/kimchi/i18n.py >>> @@ -128,6 +128,7 @@ messages = { >>> "KCHVM0069E": _("Password field must be a string."), >>> "KCHVM0070E": _("Error creating local host ssh rsa key of >>> user 'root'."), >>> "KCHVM0071E": _("Memory value %(mem)s must be aligned to >>> %(alignment)sMiB."), >> >>> + "KCHVM0072E": _("Template given has multiple disks assigned to >>> different types of storage pools, conflicting with storage pool >>> provided."), >> >> As each volume will have its own pool information we should remove >> the former storage pool information. Otherwise, it will be an >> useless information. > > I agree, and disagree. I is still used, I decided to keep it in > order to have compatibility with older templates version. > Otherwise, I would have to find a way to update information in > objectstore. > So, storagepool is used in the code for 2 reason now: > - It holds the DEFAULT pool from templates.conf. If user > does not pass pool when create a template, then , it is used > - And, for compatibility > OK. But the default pool does not make sense anymore. It is default for each volume. In the code, you did a compatibility logic to update the objectstore. Why can not you do the same in that case? Otherwise, we will have a monster objectstore only for 'compatibility'

>> >>> "KCHVMHDEV0001E": _("VM %(vmid)s does not contain directly >>> assigned host device %(dev_name)s."), >>> "KCHVMHDEV0002E": _("The host device %(dev_name)s is not >>> allowed to directly assign to VM."), >>> diff --git a/src/wok/plugins/kimchi/model/templates.py >>> b/src/wok/plugins/kimchi/model/templates.py >>> index bac6e47..eae2714 100644 >>> --- a/src/wok/plugins/kimchi/model/templates.py >>> +++ b/src/wok/plugins/kimchi/model/templates.py >>> @@ -147,6 +147,16 @@ class TemplateModel(object): >>> params = session.get('template', name) >>> if overrides: >>> params.update(overrides) >> >>> + if 'storagepool' in params: >>> + libvVMT = LibvirtVMTemplate(params, conn=conn) >>> + poolType = >>> libvVMT._get_storage_type(params['storagepool']) >>> + for i, disk in enumerate(params['disks']): >>> + if disk['pool']['type'] != poolType: >>> + raise InvalidOperation('KCHVM0072E') >>> + else: >>> + params['disks'][i]['pool']['name'] = \ >>> + params['storagepool'] >>> + >> >> The pool is defined for each volume. We should remove the former >> storage pool information >> > > User is not forced to give each disk 'pool", so, kimchi uses the > "default" storagepool configured. > This information must be recorded somewhere. > > >>> return LibvirtVMTemplate(params, False, conn) >>> >>> def lookup(self, name): >>> @@ -274,8 +284,8 @@ class LibvirtVMTemplate(VMTemplate): >>> raise InvalidParameter("KCHTMPL0007E", >>> {'network': name, >>> 'template': self.name}) >>> >>> - def _get_storage_path(self): >>> - pool = self._storage_validate() >>> + def _get_storage_path(self, pool_uri=None): >>> + pool = self._storage_validate(pool_uri) >>> xml = pool.XMLDesc(0) >>> return xpath_get_text(xml, "/pool/target/path")[0] >>> >>> @@ -285,7 +295,7 @@ class LibvirtVMTemplate(VMTemplate): >>> return xpath_get_text(xml, "/pool/@type")[0] >>> >>> def _get_volume_path(self, pool, vol): >>> - pool = self._storage_validate() >>> + pool = self._storage_validate(pool) >>> try: >>> return pool.storageVolLookupByName(vol).path() >>> except: >>> @@ -293,12 +303,11 @@ class LibvirtVMTemplate(VMTemplate): >>> 'pool': pool}) >>> >>> def fork_vm_storage(self, vm_uuid): >>> - # Provision storage: >>> - # TODO: Rebase on the storage API once upstream >>> - pool = self._storage_validate() >>> + # Provision storages: >>> vol_list = self.to_volume_list(vm_uuid) >>> try: >>> for v in vol_list: >>> + pool = self._storage_validate(v['pool']) >>> # outgoing text to libvirt, encode('utf-8') >>> pool.createXML(v['xml'].encode('utf-8'), 0) >>> except libvirt.libvirtError as e: >>> diff --git a/src/wok/plugins/kimchi/model/vms.py >>> b/src/wok/plugins/kimchi/model/vms.py >>> index 366f1b4..ebe716e 100644 >>> --- a/src/wok/plugins/kimchi/model/vms.py >>> +++ b/src/wok/plugins/kimchi/model/vms.py >>> @@ -151,28 +151,22 @@ class VMsModel(object): >>> wok_log.error('Error trying to update database >>> with guest ' >>> 'icon information due error: %s', >>> e.message) >>> >>> - # If storagepool is SCSI, volumes will be LUNs and must be >>> passed by >>> - # the user from UI or manually. >>> - cb('Provisioning storage for new VM') >>> - vol_list = [] >>> - if t._get_storage_type() not in ["iscsi", "scsi"]: >>> - vol_list = t.fork_vm_storage(vm_uuid) >> >>> + cb('Provisioning storages for new VM') >>> + vol_list = t.fork_vm_storage(vm_uuid) >> >> >> >>> graphics = params.get('graphics', {}) >>> stream_protocols = self.caps.libvirt_stream_protocols >>> xml = t.to_vm_xml(name, vm_uuid, >>> libvirt_stream_protocols=stream_protocols, >>> - graphics=graphics, >>> - volumes=vol_list) >>> + graphics=graphics) >>> >>> cb('Defining new VM') >>> try: >>> conn.defineXML(xml.encode('utf-8')) >>> except libvirt.libvirtError as e: >>> - if t._get_storage_type() not in READONLY_POOL_TYPE: >>> - for v in vol_list: >>> - vol = conn.storageVolLookupByPath(v['path']) >>> - vol.delete(0) >> >>> + for v in vol_list: >>> + vol = conn.storageVolLookupByPath(v['path']) >>> + vol.delete(0) >>> raise OperationFailed("KCHVM0007E", {'name': name, >>> 'err': >>> e.get_error_message()}) >> >> Why are not you considering the read-only pools? > > Notice above that the old code says: > > - if t._get_storage_type() not in ["iscsi", "scsi"]: > - vol_list = t.fork_vm_storage(vm_uuid) > > fork_vm_storage call "to_volume_list", where I moved this condition > (see below): > > + # Create only .img. If storagepool is (i)SCSI, volumes > will be LUNs > + if d['pool']['type'] in ["iscsi", "scsi"]: > + continue > > OK > So vol_list will is always a list of physical ".img"s. > Maybe the confusion is in the "volume" name associated. "Volumes" > related to SCSI and iSCSI > (read-only) pools does not need the path to be discovered here. > >> >>> diff --git a/src/wok/plugins/kimchi/vmtemplate.py >>> b/src/wok/plugins/kimchi/vmtemplate.py >>> index c7635b0..69cc9b5 100644 >>> --- a/src/wok/plugins/kimchi/vmtemplate.py >>> +++ b/src/wok/plugins/kimchi/vmtemplate.py >>> @@ -184,11 +184,6 @@ class VMTemplate(object): >>> return xml >>> >>> 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) >>> - storage_type = self._get_storage_type() >>> - storage_path = self._get_storage_path() >>> - >>> base_disk_params = {'type': 'disk', 'disk': 'file', >>> 'bus': self.info['disk_bus']} >>> logical_disk_params = {'format': 'raw'} >>> @@ -199,37 +194,44 @@ class VMTemplate(object): >>> '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['format'] = disk['format'] >>> - params.update(locals().get('%s_disk_params' % >>> storage_type, {})) >>> + params.update(locals().get('%s_disk_params' % >>> + disk['pool']['type'], {})) >>> params['index'] = index >>> >>> volume = disk.get('volume') >>> if volume is not None: >>> - params['path'] = self._get_volume_path(pool_name, >>> volume) >>> + params['path'] = >>> self._get_volume_path(disk['pool']['name'], >>> + volume) >>> else: >>> - volume = "%s-%s.img" % (vm_uuid, params['index']) >>> - params['path'] = os.path.join(storage_path, volume) >>> + img = "%s-%s.img" % (vm_uuid, params['index']) >>> + storage_path = >>> self._get_storage_path(disk['pool']['name']) >>> + params['path'] = os.path.join(storage_path, img) >>> >>> disks_xml += get_disk_xml(params)[1] >>> >>> return unicode(disks_xml, 'utf-8') >>> >>> def to_volume_list(self, vm_uuid): >>> - storage_path = self._get_storage_path() >>> ret = [] >>> for i, d in enumerate(self.info['disks']): >>> + # Create only .img. If storagepool is (i)SCSI, volumes >>> will be LUNs >>> + if d['pool']['type'] in ["iscsi", "scsi"]: >>> + continue >>> + >>> index = d.get('index', i) >>> volume = "%s-%s.img" % (vm_uuid, index) >>> >>> + storage_path = self._get_storage_path(d['pool']['name']) >>> info = {'name': volume, >>> 'capacity': d['size'], >>> 'format': d['format'], >>> - 'path': '%s/%s' % (storage_path, volume)} >>> + 'path': '%s/%s' % (storage_path, volume), >>> + 'pool': d['pool']['name']} >>> >>> - if 'logical' == self._get_storage_type() or \ >>> + if 'logical' == d['pool']['type'] or \ >>> info['format'] not in ['qcow2', 'raw']: >>> info['allocation'] = info['capacity'] >>> else: >>> @@ -414,7 +416,7 @@ class VMTemplate(object): >>> def fork_vm_storage(self, vm_uuid): >>> pass >>> >>> - def _get_storage_path(self): >>> + def _get_storage_path(self, pool_uri=None): >>> return '' >>> >>> def _get_storage_type(self, pool=None): >> >> _______________________________________________ >> Kimchi-devel mailing list >> Kimchi-devel(a)ovirt.org >> http://lists.ovirt.org/mailman/listinfo/kimchi-devel >> > > _______________________________________________ > Kimchi-devel mailing list > Kimchi-devel(a)ovirt.org > http://lists.ovirt.org/mailman/listinfo/kimchi-devel > _______________________________________________ Kimchi-devel mailing list Kimchi-devel(a)ovirt.org http://lists.ovirt.org/mailman/listinfo/kimchi-devel