[Kimchi-devel] [PATCH v3] [Kimchi] Issue #920: Template is removed if an ISO doesn't exist

Aline Manera alinefm at linux.vnet.ibm.com
Mon Apr 18 16:04:44 UTC 2016



On 04/15/2016 05:48 PM, Jose Ricardo Ziviani wrote:
>   - This commit changes the way the template update works. Now, it
>     really updates the existing template instead of creating a new
>     one (and deleting the old one)
>
> Signed-off-by: Jose Ricardo Ziviani <joserz at linux.vnet.ibm.com>
> ---
>   i18n.py                |  3 +++
>   model/templates.py     | 70 ++++++++++++++++++++++++++++++++------------------
>   tests/test_template.py |  6 ++++-
>   3 files changed, 53 insertions(+), 26 deletions(-)
>
> diff --git a/i18n.py b/i18n.py
> index 7cce796..e4656cb 100644
> --- a/i18n.py
> +++ b/i18n.py
> @@ -155,8 +155,10 @@ messages = {
>       "KCHVMIF0011E": _("Cannot change MAC address of a running virtual machine"),
>
>       "KCHTMPL0001E": _("Template %(name)s already exists"),
> +    "KCHTMPL0002E": _("Source media %(path)s not found"),
>       "KCHTMPL0003E": _("Network '%(network)s' specified for template %(template)s does not exist"),
>       "KCHTMPL0004E": _("Storage pool %(pool)s specified for template %(template)s does not exist"),
> +    "KCHTMPL0005E": _("Template %(name)s does not exist."),
>       "KCHTMPL0006E": _("Invalid parameter '%(param)s' specified for CDROM."),
>       "KCHTMPL0007E": _("Network %(network)s specified for template %(template)s is not active"),
>       "KCHTMPL0008E": _("Template name must be a string"),
> @@ -181,6 +183,7 @@ messages = {
>       "KCHTMPL0029E": _("Disk format must be 'raw', for logical, iscsi, and scsi pools."),
>       "KCHTMPL0030E": _("Memory expects an object with one or both parameters: 'current' and 'maxmemory'"),
>       "KCHTMPL0031E": _("Memory value (%(mem)sMiB) must be equal or lesser than maximum memory value (%(maxmem)sMiB)"),
> +    "KCHTMPL0032E": _("Unable to update template due error: %(err)s"),
>
>       "KCHPOOL0001E": _("Storage pool %(name)s already exists"),
>       "KCHPOOL0002E": _("Storage pool %(name)s does not exist"),
> diff --git a/model/templates.py b/model/templates.py
> index a02099c..d0742ca 100644
> --- a/model/templates.py
> +++ b/model/templates.py
> @@ -51,7 +51,7 @@ class TemplatesModel(object):
>           self.objstore = kargs['objstore']
>           self.conn = kargs['conn']
>
> -    def create(self, params):
> +    def _validate_template_params(self, params):
>           name = params.get('name', '').strip()
>
>           conn = self.conn.get()
> @@ -72,8 +72,7 @@ class TemplatesModel(object):
>
>           # image does not exists: raise error
>           elif not os.path.exists(path):
> -            raise InvalidParameter("Unable to find file %(path)s" %
> -                                   {"path": path})
> +            raise InvalidParameter("KCHTMPL0002E", {'path': path})
>
>           # create magic object to discover file type
>           file_type = magic.open(magic.MAGIC_NONE)
> @@ -99,10 +98,6 @@ class TemplatesModel(object):
>               params["disks"] = params.get('disks', [])
>               params["disks"].append({"base": path})
>
> -        return self.save_template(params)
> -
> -    def save_template(self, params):
> -
>           # Creates the template class with necessary information
>           t = LibvirtVMTemplate(params, scan=True, conn=self.conn)
>
> @@ -119,16 +114,18 @@ class TemplatesModel(object):
>               if 'volume' in disk.keys():
>                   self.template_volume_validate(volume, disk['pool'])
>
> -        # template with the same name already exists: raise exception
> -        name = params['name']
> -        with self.objstore as session:
> -            if name in session.get_list('template'):
> -                raise InvalidOperation("KCHTMPL0001E", {'name': name})
> +        return t
> +
> +    def create(self, params):
> +        template = self._validate_template_params(params)
>
>           # Store template on objectstore
> +        name = params['name']
>           try:
>               with self.objstore as session:
> -                session.store('template', name, t.info,
> +                if name in session.get_list('template'):
> +                    raise InvalidOperation("KCHTMPL0001E", {'name': name})
> +                session.store('template', name, template.info,
>                                 get_kimchi_version())
>           except InvalidOperation:
>               raise
> @@ -191,7 +188,8 @@ class TemplateModel(object):
>
>           temp = self.lookup(name)
>           temp['name'] = clone_name
> -        ident = self.templates.save_template(temp)

> +        temp['source_media'] = temp.get('base', temp['cdrom'])

Not sure I understood this block.
To create a template the user must to pass a value for source_media and 
cdrom or disks[base] are not allowed. Said that, I don't see reasons to 
have it here.

> +        ident = self.templates.create(temp)
>           return ident
>
>       def delete(self, name):
> @@ -204,31 +202,36 @@ class TemplateModel(object):
>               raise OperationFailed('KCHTMPL0021E', {'err': e.message})
>
>       def update(self, name, params):
> -        old_t = self.lookup(name)
> -        new_t = copy.copy(old_t)
> +        edit_template = self.lookup(name)
> +

> +        edit_template['source_media'] = edit_template.get(
> +            'base', edit_template['cdrom'])
> +
> +        if 'cdrom' in params:
> +            edit_template['source_media'] = params['cdrom']
>

source_media is not a valid parameter for template update. It is only 
required to create a new Template.
To update it, you need to use disks[base] or cdrom. The same it was 
before source_media being added.


>           # Merge graphics settings
>           graph_args = params.get('graphics')
>           if graph_args:
> -            graphics = dict(new_t['graphics'])
> +            graphics = dict(edit_template['graphics'])
>               graphics.update(graph_args)
>               params['graphics'] = graphics
>
>           # Merge cpu_info settings
>           new_cpu_info = params.get('cpu_info')
>           if new_cpu_info:
> -            cpu_info = dict(new_t['cpu_info'])
> +            cpu_info = dict(edit_template['cpu_info'])
>               cpu_info.update(new_cpu_info)
>               params['cpu_info'] = cpu_info
>
>           # Fix memory values, because method update does not work recursively
>           new_mem = params.get('memory')
>           if new_mem is not None:
> -            params['memory'] = copy.copy(old_t.get('memory'))
> +            params['memory'] = copy.copy(edit_template.get('memory'))
>               params['memory'].update(new_mem)
>               validate_memory(params['memory'])
>
> -        new_t.update(params)
> +        edit_template.update(params)
>
>           for net_name in params.get(u'networks', []):
>               try:
> @@ -238,13 +241,30 @@ class TemplateModel(object):
>                   raise InvalidParameter("KCHTMPL0003E", {'network': net_name,
>                                                           'template': name})
>
> -        self.delete(name)
> +        template = self.templates._validate_template_params(edit_template)
> +
>           try:
> -            ident = self.templates.save_template(new_t)
> -        except:
> -            ident = self.templates.save_template(old_t)
> +            with self.objstore as session:
> +                if name not in session.get_list('template'):
> +                    raise InvalidOperation("KCHTMPL0005E", {'name': name})
> +
> +                if template.info['name'] != name and \
> +                   template.info['name'] in session.get_list('template'):
> +                    raise InvalidOperation("KCHTMPL0001E",
> +                                           {'name': template.info['name']})
> +
> +                if template.info['name'] != name:
> +                    session.delete('template', name)
> +
> +                session.store('template', template.info['name'],
> +                              template.info, get_kimchi_version())
> +
> +        except InvalidOperation:
>               raise
> -        return ident
> +        except Exception, e:
> +            raise OperationFailed('KCHTMPL0032E', {'err': e.message})
> +
> +        return template.info['name']
>
>
>   def validate_memory(memory):
> diff --git a/tests/test_template.py b/tests/test_template.py
> index 158bbeb..f0ef030 100644
> --- a/tests/test_template.py
> +++ b/tests/test_template.py
> @@ -256,7 +256,11 @@ class TemplateTests(unittest.TestCase):
>           self.assertEquals(1024, update_tmpl['memory']['maxmemory'])
>
>           # Update cdrom
> -        cdrom_data = {'cdrom': '/tmp/mock2.iso'}
> +        cdrom_data = {'cdrom': 'inexistent.iso'}
> +        resp = self.request(new_tmpl_uri, json.dumps(cdrom_data), 'PUT')
> +        self.assertEquals(400, resp.status)
> +
> +        cdrom_data = {'cdrom': '/tmp/existent.iso'}
>           resp = self.request(new_tmpl_uri, json.dumps(cdrom_data), 'PUT')
>           self.assertEquals(200, resp.status)
>           update_tmpl = json.loads(resp.read())




More information about the Kimchi-devel mailing list