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

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(a)linux.vnet.ibm.com&gt; > --- > 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())