[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:14:46 UTC 2015
On 30/11/2015 12:09, Aline Manera wrote:
>
>
> 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'
>
You can also update the compatibility script created by Paulo to update
the objectstore once when running Kimchi 2.0
>>>
>>>> "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
>>
>
> _______________________________________________
> 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