[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:40:54 UTC 2016
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 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'?
>
>
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())
>>>>
>>>
>>
>
More information about the Kimchi-devel
mailing list