[Kimchi-devel] [PATCH v3] [Kimchi] Issue #920: Template is removed if an ISO doesn't exist
Jose Ricardo Ziviani
joserz at linux.vnet.ibm.com
Mon Apr 18 18:29:33 UTC 2016
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 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.
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
More information about the Kimchi-devel
mailing list