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

On 18-04-2016 16:20, Aline Manera wrote: > > > On 04/18/2016 03:29 PM, Jose Ricardo Ziviani wrote: >> >> >> 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. >> > > The source_media parameter is not used for validation, it is used to > identify the media type. So once you already have the information as > cdrom or base disk there is no need to check it again. > > In other words, when user inputs source_media kimchi needs to identify > if it is a cdrom or base image. It does not depend on cdrom or base > image validation itself. It is the second step. > > Does that make sense? ok, so today we have: # get source_media path = params.pop("source_media") # not local image: set as remote ISO if urlparse.urlparse(path).scheme in ["http", "https", "tftp", "ftp", "ftps"]: params["cdrom"] = path # image does not exists: raise error elif not os.path.exists(path): raise InvalidParameter("Unable to find file %(path)s" % {"path": path}) it's better to change that code to doesn't count on 'source_media'?

> >>> >>>> + 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()) >>> >> >