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

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(a)linux.vnet.ibm.com wrote: >>> From: ShaoHe Feng <shaohef(a)linux.vnet.ibm.com&gt; >>> >>> 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(a)linux.vnet.ibm.com&gt; >>> --- >>> 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, >> >> >