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

[Zhou Zheng Sheng]

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.

>>
>>> + 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!

Zhou Zheng Sheng / 周征晟
E-mail: zhshzhou at linux.vnet.ibm.com
Telephone: 86-10-82454397