[Kimchi-devel] [PATCH v3] [Kimchi] Issue #920: Template is removed if an ISO doesn't exist
Aline Manera
alinefm at linux.vnet.ibm.com
Mon Apr 18 19:20:33 UTC 2016
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 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.
>
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())
>>
>
More information about the Kimchi-devel
mailing list