[Kimchi-devel] [PATCH] validate the volume parameter when the pool of template is iscsi or scsi

[Sheldon]

On 03/26/2014 01:27 PM, Zhou Zheng Sheng wrote:
> on 2014/03/26 12:07, Sheldon wrote:
>> On 03/26/2014 03:33 AM, Aline Manera wrote:
>>> On 03/25/2014 09:49 AM, shaohef at linux.vnet.ibm.com wrote:
>>>> From: ShaoHe Feng <shaohef at linux.vnet.ibm.com>
>>>>
>>>> When the pool of template is iscsi or scsi, the volume parameter can not
>>>> be NULL.
>>>> And as we discussion, we do not mix disks from 2 different types of
>>>> storage pools, for instance: we do not create a template with 2
>>>> disks, where one comes from a SCSI pool and other is a .img in
>>>> a DIR pool.
>>>>
>>>> Signed-off-by: ShaoHe Feng <shaohef at linux.vnet.ibm.com>
>>>> ---
>>>> src/kimchi/i18n.py | 2 ++
>>>> src/kimchi/model/templates.py | 50
>>>> +++++++++++++++++++++++++++++++++----------
>>>> 2 files changed, 41 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/src/kimchi/i18n.py b/src/kimchi/i18n.py
>>>> index 1f84034..bcc15b7 100644
>>>> --- a/src/kimchi/i18n.py
>>>> +++ b/src/kimchi/i18n.py
>>>> @@ -107,6 +107,8 @@ messages = {
>>>> "KCHTMPL0015E": _("Invalid storage pool URI %(value)s specified for
>>>> template"),
>>>> "KCHTMPL0016E": _("Specify an ISO image as CDROM to create a template"),
>>>> "KCHTMPL0017E": _("All networks for the template must be specified in
>>>> a list."),
>>>> + "KCHTMPL0018E": _("Must specify a volume to a template, when
>>>> storage pool is iscsi or scsi"),
>>>> + "KCHTMPL0019E": _("The volume: %(volume)s in not in storage pool
>>>> %(pool)"),
>>>>
>>>> "KCHPOOL0001E": _("Storage pool %(name)s already exists"),
>>>> "KCHPOOL0002E": _("Storage pool %(name)s does not exist"),
>>>> diff --git a/src/kimchi/model/templates.py
>>>> b/src/kimchi/model/templates.py
>>>> index d49632e..0e5c2b2 100644
>>>> --- a/src/kimchi/model/templates.py
>>>> +++ b/src/kimchi/model/templates.py
>>>> @@ -55,15 +55,17 @@ class TemplatesModel(object):
>>>> params['name'] = name
>>>>
>>>> conn = self.conn.get()
>>>> -
>>>> pool_uri = params.get(u'storagepool', '')
>>>> if pool_uri:
>>>> - pool_name = pool_name_from_uri(pool_uri)
>>>> try:
>>>> - conn.storagePoolLookupByName(pool_name.encode("utf-8"))
>>>> + pool_name = pool_name_from_uri(pool_uri)
>>>> + pool = conn.storagePoolLookupByName(pool_name.encode("utf-8"))
>>>> except Exception:
>>>> raise InvalidParameter("KCHTMPL0004E", {'pool': pool_name,
>>>> 'template': name})
>>>> + tmp_volumes = [disk['volume'] for disk in params.get('disks', [])
>>>> + if 'volume' in disk]
>>>> + self.template_volume_validate(tmp_volumes, pool)
>>>>
>>>> for net_name in params.get(u'networks', []):
>>>> try:
>>>> @@ -83,6 +85,28 @@ class TemplatesModel(object):
>>>> with self.objstore as session:
>>>> return session.get_list('template')
>>>>
>>>> + def template_volume_validate(self, tmp_volumes, pool):
>>>> + kwargs = {'conn': self.conn, 'objstore': self.objstore}
>>>> + pool_type = xmlutils.xpath_get_text(pool.XMLDesc(0), "/pool/@type")[0]
>>>> + pool_name = pool.name()
>>>> +
>>>> + # as we discussion, we do not mix disks from 2 different types of
>>>> + # storage pools, for instance: we do not create a template with 2
>>>> + # disks, where one comes from a SCSI pool and other is a .img in
>>>> + # a DIR pool.
>>>> + if pool_type in ['iscsi', 'scsi']:
>>>> + if not tmp_volumes:
>>>> + raise InvalidParameter("KCHTMPL0018E")
>>>> +
>>>> + storagevolumes = __import__("kimchi.model.storagevolumes",
>>>> + fromlist=[''])
>>>> + pool_volumes = storagevolumes.StorageVolumesModel(
>>>> + **kwargs).get_list(pool_name)
>>> Why are you importing it that way?
>> this way can also works.
>> I get the StorageVolumesModel from globals()
>>
>> $ git diff
>> diff --git a/src/kimchi/model/templates.py b/src/kimchi/model/templates.py
>> index 0e5c2b2..9f45dc1 100644
>> --- a/src/kimchi/model/templates.py
>> +++ b/src/kimchi/model/templates.py
>> @@ -98,10 +98,8 @@ class TemplatesModel(object):
>> if not tmp_volumes:
>> raise InvalidParameter("KCHTMPL0018E")
>>
>> - storagevolumes = __import__("kimchi.model.storagevolumes",
>> - fromlist=[''])
>> - pool_volumes = storagevolumes.StorageVolumesModel(
>> - **kwargs).get_list(pool_name)
>> + StorageVolumesModel = globals()["StorageVolumesModel"]
>> + pool_volumes = StorageVolumesModel(**kwargs).get_list(pool_name)
>> vols = set(tmp_volumes) - set(pool_volumes)
>>
> I think using globals is not a good idea. Maybe a proper refactor to
> untie the circular import is better.
sure,  This is not the issue of this patch.

we will do a better refactor later.

>
>>>> + vols = set(tmp_volumes) - set(pool_volumes)
>>>> + if vols:
>>>> + raise InvalidParameter("KCHTMPL0019E", {'pool': pool_name,
>>>> + 'volume': vols})
>>>> +
>>>>
>>>> class TemplateModel(object):
>>>> def __init__(self, **kargs):
>>>> @@ -128,18 +152,22 @@ class TemplateModel(object):
>>>> new_t.update(params)
>>>> ident = name
>>>>
>>>> + conn = self.conn.get()
>>>> pool_uri = new_t.get(u'storagepool', '')
>>>> - pool_name = pool_name_from_uri(pool_uri)
>>>> - try:
>>>> - conn = self.conn.get()
>>>> - conn.storagePoolLookupByName(pool_name.encode("utf-8"))
>>>> - except Exception:
>>>> - raise InvalidParameter("KCHTMPL0004E", {'pool': pool_name,
>>>> - 'template': name})
>>>> +
>>>> + if pool_uri:
>>>> + try:
>>>> + pool_name = pool_name_from_uri(pool_uri)
>>>> + pool = conn.storagePoolLookupByName(pool_name.encode("utf-8"))
>>>> + except Exception:
>>>> + raise InvalidParameter("KCHTMPL0004E", {'pool': pool_name,
>>>> + 'template': name})
>>>> + tmp_volumes = [disk['volume'] for disk in new_t.get('disks', [])
>>>> + if 'volume' in disk]
>>>> + self.templates.template_volume_validate(tmp_volumes, pool)
>>>>
>>>> for net_name in params.get(u'networks', []):
>>>> try:
>>>> - conn = self.conn.get()
>>>> conn.networkLookupByName(net_name)
>>>> except Exception:
>>>> raise InvalidParameter("KCHTMPL0003E", {'network': net_name,
>>>
>>>
>>
>


-- 
Thanks and best regards!

Sheldon Feng(冯少合)<shaohef at linux.vnet.ibm.com>
IBM Linux Technology Center