[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 19:38:07 UTC 2016
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 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?
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
More information about the Kimchi-devel
mailing list