[PATCH v3] [Kimchi] Issue #920: Template is removed if an ISO doesn't exist
- 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@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']) + 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'] # 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()) -- 1.9.1
Reviewed-by: Daniel Barboza <dhbarboza82@gmail.com> 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@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']) + 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']
# 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())
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@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())
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@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.
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())
-- Jose Ricardo Ziviani ----------------------------- Software Engineer Linux Technology Center - IBM
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@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.
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?
+ 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())
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@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.
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())
-- Jose Ricardo Ziviani ----------------------------- Software Engineer Linux Technology Center - IBM
On 04/18/2016 04:38 PM, Jose Ricardo Ziviani wrote:
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@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.
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'?
This code is from template creation, right? The Template creation depends on source_media! But clone/update a Template will rely on cdrom and base[img]
+ 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())
participants (3)
-
Aline Manera -
Daniel Henrique Barboza -
Jose Ricardo Ziviani