[Kimchi-devel] [PATCH 2/5 - v4] Enable create guests with multiple disks from different pools
Aline Manera
alinefm at linux.vnet.ibm.com
Mon Nov 30 14:09:26 UTC 2015
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 at linux.vnet.ibm.com>
>>> ---
>>> 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 at ovirt.org
>> http://lists.ovirt.org/mailman/listinfo/kimchi-devel
>>
>
> _______________________________________________
> Kimchi-devel mailing list
> Kimchi-devel at ovirt.org
> http://lists.ovirt.org/mailman/listinfo/kimchi-devel
>
More information about the Kimchi-devel
mailing list