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

On 18-04-2016 13:04, Aline Manera wrote: > > > 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. This code was added in the clone() method, that is because I created the method _validate_template_params(), that checks if all parameters are ok before creating the template. So why do I pass temp['base'] or temp['cdrom'] to temp['source_media']? The reason is that I thought it's worth to validate the path when cloning a template.

> >> + 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. Basically it's the same case explained above. Bellow I call "_validate_template_params()", that will verify if the path to the cdrom/image is right and will inform the user about it. > > >> # 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()) >