[Kimchi-devel] [PATCH 2/2] Change 'cdrom' to a optional param

[Aline Manera]

On 07/11/2014 03:56 AM, Royce Lv wrote:
> On 2014年07月10日 20:36, Aline Manera wrote:
>>
>>
>> On 07/09/2014 07:03 AM, lvroyce at linux.vnet.ibm.com wrote:
>>> From: Royce Lv <lvroyce at linux.vnet.ibm.com>
>>>
>>> Multiple files modification for change 'cdrom' to optional param.
>>>
>>> Signed-off-by: Royce Lv <lvroyce at linux.vnet.ibm.com>
>>> ---
>>> src/kimchi/control/templates.py | 2 +-
>>> src/kimchi/i18n.py | 2 +-
>>> src/kimchi/mockmodel.py | 26 ++++++++++++++++++++------
>>> src/kimchi/model/templates.py | 13 ++++++++-----
>>> src/kimchi/vmtemplate.py | 12 ++++++++++--
>>> tests/test_rest.py | 2 +-
>>> 6 files changed, 41 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/src/kimchi/control/templates.py
>>> b/src/kimchi/control/templates.py
>>> index a535960..e16cc1a 100644
>>> --- a/src/kimchi/control/templates.py
>>> +++ b/src/kimchi/control/templates.py
>>> @@ -47,7 +47,7 @@ class Template(Resource):
>>> 'os_version': self.info['os_version'],
>>> 'cpus': self.info['cpus'],
>>> 'memory': self.info['memory'],
>>> - 'cdrom': self.info['cdrom'],
>>> + 'cdrom': self.info.get('cdrom', None),
>>> 'disks': self.info['disks'],
>>> 'storagepool': self.info['storagepool'],
>>> 'networks': self.info['networks'],
>>> diff --git a/src/kimchi/i18n.py b/src/kimchi/i18n.py
>>> index 7a7b675..3ba1e23 100644
>>> --- a/src/kimchi/i18n.py
>>> +++ b/src/kimchi/i18n.py
>>> @@ -118,7 +118,7 @@ messages = {
>>> "KCHTMPL0013E": _("Amount of memory (MB) must be an integer greater
>>> than 512"),
>>> "KCHTMPL0014E": _("Template CDROM must be a local or remote ISO file"),
>>> "KCHTMPL0015E": _("Invalid storage pool URI %(value)s specified for
>>> template"),
>>> - "KCHTMPL0016E": _("Specify an ISO image as CDROM to create a
>>> template"),
>>> + "KCHTMPL0016E": _("Specify an ISO image as CDROM or a base image to
>>> create a template"),
>>> "KCHTMPL0017E": _("All networks for the template must be specified in
>>> a list."),
>>> "KCHTMPL0018E": _("Must specify a volume to a template, when storage
>>> pool is iscsi or scsi"),
>>> "KCHTMPL0019E": _("The volume: %(volume)s in not in storage pool
>>> %(pool)s"),
>>> diff --git a/src/kimchi/mockmodel.py b/src/kimchi/mockmodel.py
>>> index a0920e0..e14990e 100644
>>> --- a/src/kimchi/mockmodel.py
>>> +++ b/src/kimchi/mockmodel.py
>>> @@ -200,8 +200,9 @@ class MockModel(object):
>>> index += 1
>>>
>>> cdrom = "hd" + string.ascii_lowercase[index + 1]
>>> - cdrom_params = {'dev': cdrom, 'path': t_info['cdrom'], 'type':
>>> 'cdrom'}
>>> - vm.storagedevices[cdrom] = MockVMStorageDevice(cdrom_params)
>>> + if t_info.get('cdrom'):
>>> + cdrom_params = {'dev': cdrom, 'path': t_info['cdrom'], 'type':
>>> 'cdrom'}
>>> + vm.storagedevices[cdrom] = MockVMStorageDevice(cdrom_params)
>>>
>>> self._mock_vms[name] = vm
>>> return name
>>> @@ -237,11 +238,24 @@ class MockModel(object):
>>>
>>> def templates_create(self, params):
>>> name = params.get('name', '').strip()
>>> + iso = params.get('cdrom')
>>> if not name:
>>> - iso = params['cdrom']
>>> - iso_name = os.path.splitext(iso[iso.rfind('/') + 1:])[0]
>>> - name = iso_name + str(int(time.time() * 1000))
>>> - params['name'] = name
>>> + if iso:
>>> + iso_name = os.path.splitext(iso[iso.rfind('/') + 1:])[0]
>>> + name = iso_name + str(int(time.time() * 1000))
>>> + params['name'] = name
>>> + else:
>>> + raise InvalidParameter("KCHTMPL0008E")
>>> +
>>
>>
>>> + base_tmpl = False
>>> + if not iso:
>>> + if 'disks' in params:
>>> + for d in args['disks']:
>>> + if 'base' in d:
>>> + base_tmpl = True
>>> + break
>>> + if not base_tmpl:
>>> + raise InvalidParameter("KCHTMPL0016E")
>>
>> Why should that code be added here?
> No iso and no base disks, we can't create template under this situation.
> One of the two requirements need to be satisfied.
>>

I understand but this patch set is only to make cdrom optional, right?
Otherwise, update your commit message

>>>
>>> if name in self._mock_templates:
>>> raise InvalidOperation("KCHTMPL0001E", {'name': name})
>>> diff --git a/src/kimchi/model/templates.py
>>> b/src/kimchi/model/templates.py
>>> index 60f4de5..0e07a78 100644
>>> --- a/src/kimchi/model/templates.py
>>> +++ b/src/kimchi/model/templates.py
>>> @@ -40,9 +40,9 @@ class TemplatesModel(object):
>>>
>>> def create(self, params):
>>> name = params.get('name', '').strip()
>>> - iso = params['cdrom']
>>> + iso = params.get('cdrom')
>>> # check search permission
>>> - if iso.startswith('/') and os.path.isfile(iso):
>>> + if iso and iso.startswith('/') and os.path.isfile(iso):
>>> user = UserTests().probe_user()
>>> ret, excp = probe_file_permission_as_user(iso, user)
>>> if ret is False:
>>> @@ -51,9 +51,12 @@ class TemplatesModel(object):
>>> 'err': excp})
>>>
>>> if not name:
>>> - iso_name = os.path.splitext(iso[iso.rfind('/') + 1:])[0]
>>> - name = iso_name + str(int(time.time() * 1000))
>>> - params['name'] = name
>>> + if iso:
>>> + iso_name = os.path.splitext(iso[iso.rfind('/') + 1:])[0]
>>> + name = iso_name + str(int(time.time() * 1000))
>>> + params['name'] = name
>>> + else:
>>> + raise InvalidParameter('KCHTMPL0008E')
>>>
>>> conn = self.conn.get()
>>> pool_uri = params.get(u'storagepool', '')
>>> diff --git a/src/kimchi/vmtemplate.py b/src/kimchi/vmtemplate.py
>>> index 8d5217a..91617d9 100644
>>> --- a/src/kimchi/vmtemplate.py
>>> +++ b/src/kimchi/vmtemplate.py
>>> @@ -54,6 +54,14 @@ class VMTemplate(object):
>>> iso_distro = iso_version = 'unknown'
>>> iso = args.get('cdrom', '')
>>>
>>
>>
>>> + # if ISO not specified and base disk image specified,
>>> + # prevent cdrom from filling automatically
>>> + if len(iso) == 0 and 'disks' in args:
>>> + for d in args['disks']:
>>> + if 'base' in d:
>>> + args['cdrom'] = ''
>>> + break
>>> +
>>
>>
>> Why is that code needed?
>> If the ISO was not specified the "cdrom" parameter won't exist in the
>> dict - so we should not add a cdrom automatically
>>
>> We should only verify if a ISO or a base disk is specified to create a
>> template and raise a error if none of them exists
> This is because I planned to add probe os information for image(in my
> other patch), but after distro/version probed, osinfo.py will
> automatically fill a cdrom link if we do not specify it as empty
> string--finally will use iso streaming.
> So I can only specify it to empty, or I will need to delete cdrom after
> template is created.
>>

Some questions came up to my mind:

1) Why osinfo.py is changing cdrom value?

    I checked the code and see;

    params['cdrom'] = isolinks.get(distro, {}).get(version, '')

    And isolinks is a dict of remote ISOs

    The pre-configured remote ISOs should come through JSON files under 
/distros

    Seems this code is old and should be removed from osinfo.py

>>
>>
>>> if scan and len(iso) > 0:
>>> iso_distro, iso_version = self.get_iso_info(iso)
>>> if not iso.startswith('/'):
>>> @@ -414,8 +422,8 @@ drive=drive-%(bus)s0-1-0,id=%(bus)s0-1-0'/>
>>>
>>> # validate iso integrity
>>> # FIXME when we support multiples cdrom devices
>>> - iso = self.info['cdrom']
>>> - if not (os.path.isfile(iso) or check_url_path(iso)):
>>> + iso = self.info.get('cdrom')
>>> + if iso and not (os.path.isfile(iso) or check_url_path(iso)):
>>> invalid['cdrom'] = [iso]
>>>
>>> self.info['invalid'] = invalid
>>> diff --git a/tests/test_rest.py b/tests/test_rest.py
>>> index ad8fc72..5d84e85 100644
>>> --- a/tests/test_rest.py
>>> +++ b/tests/test_rest.py
>>> @@ -1068,7 +1068,7 @@ class RestTests(unittest.TestCase):
>>> self.assertEquals(200, resp.status)
>>> self.assertEquals(0, len(json.loads(resp.read())))
>>>
>>> - # Create a template without cdrom fails with 400
>>> + # Create a template without cdrom and disk specified fails with 400
>>> t = {'name': 'test', 'os_distro': 'ImagineOS',
>>> 'os_version': '1.0', 'memory': 1024, 'cpus': 1,
>>> 'storagepool': '/storagepools/alt'}
>>>
>