[PATCH 0/2] Doc and testcases: Create template from image
From: Royce Lv <lvroyce@linux.vnet.ibm.com> This patch is to present the api definition and clean original cdrom based template creation implementation. Royce Lv (2): Change doc and api specification Change 'cdrom' to a optional param docs/API.md | 3 ++- src/kimchi/API.json | 8 +++++++- src/kimchi/control/templates.py | 2 +- src/kimchi/i18n.py | 3 ++- src/kimchi/mockmodel.py | 26 ++++++++++++++++++++------ src/kimchi/model/templates.py | 13 ++++++++----- src/kimchi/vmtemplate.py | 12 ++++++++++-- tests/test_rest.py | 2 +- 8 files changed, 51 insertions(+), 18 deletions(-) -- 1.8.3.2
From: Royce Lv <lvroyce@linux.vnet.ibm.com> Add 'base' to 'disks' param to create template, so that we can support create template from image. Signed-off-by: Royce Lv <lvroyce@linux.vnet.ibm.com> --- docs/API.md | 3 ++- src/kimchi/API.json | 8 +++++++- src/kimchi/i18n.py | 1 + 3 files changed, 10 insertions(+), 2 deletions(-) diff --git a/docs/API.md b/docs/API.md index 4f51dd0..a261762 100644 --- a/docs/API.md +++ b/docs/API.md @@ -174,7 +174,7 @@ Represents a snapshot of the Virtual Machine's primary monitor. * cpus *(optional)*: The number of CPUs assigned to the VM. Default is 1. * memory *(optional)*: The amount of memory assigned to the VM. Default is 1024M. - * cdrom *(required)*: A volume name or URI to an ISO image. + * cdrom *(optional)*: A volume name or URI to an ISO image. * storagepool *(optional)*: URI of the storagepool. Default is '/storagepools/default' * networks *(optional)*: list of networks will be assigned to the new VM. @@ -183,6 +183,7 @@ Represents a snapshot of the Virtual Machine's primary monitor. (either *size* or *volume* must be specified): * index: The device index * size: The device size in GB + * base: Base image of this disk * graphics *(optional)*: The graphics paramenters of this template * type: The type of graphics. It can be VNC or spice or None. diff --git a/src/kimchi/API.json b/src/kimchi/API.json index 6d1324c..60deeb4 100644 --- a/src/kimchi/API.json +++ b/src/kimchi/API.json @@ -362,7 +362,6 @@ "description": "Path for cdrom", "type": "string", "pattern": "^((/)|(http)[s]?:|[t]?(ftp)[s]?:)+.*$", - "required": true, "error": "KCHTMPL0014E" }, "disks": { @@ -381,7 +380,14 @@ "type": "number", "minimum": 1, "error": "KCHTMPL0022E" + }, + "base": { + "description": "Base image of the disk", + "type": "string", + "pattern": "^/.+$", + "error": "KCHTMPL0023E" } + } }, "minItems": 1, diff --git a/src/kimchi/i18n.py b/src/kimchi/i18n.py index daeeed8..7a7b675 100644 --- a/src/kimchi/i18n.py +++ b/src/kimchi/i18n.py @@ -125,6 +125,7 @@ messages = { "KCHTMPL0020E": _("Unable to create template due error: %(err)s"), "KCHTMPL0021E": _("Unable to delete template due error: %(err)s"), "KCHTMPL0022E": _("Disk size must be greater than 1GB."), + "KCHTMPL0023E": _("Template base image must be a valid local image file"), "KCHPOOL0001E": _("Storage pool %(name)s already exists"), "KCHPOOL0002E": _("Storage pool %(name)s does not exist"), -- 1.8.3.2
Reviewed-by: Aline Manera <alinefm@linux.vnet.ibm.com> On 07/09/2014 07:03 AM, lvroyce@linux.vnet.ibm.com wrote:
From: Royce Lv <lvroyce@linux.vnet.ibm.com>
Add 'base' to 'disks' param to create template, so that we can support create template from image.
Signed-off-by: Royce Lv <lvroyce@linux.vnet.ibm.com> --- docs/API.md | 3 ++- src/kimchi/API.json | 8 +++++++- src/kimchi/i18n.py | 1 + 3 files changed, 10 insertions(+), 2 deletions(-)
diff --git a/docs/API.md b/docs/API.md index 4f51dd0..a261762 100644 --- a/docs/API.md +++ b/docs/API.md @@ -174,7 +174,7 @@ Represents a snapshot of the Virtual Machine's primary monitor. * cpus *(optional)*: The number of CPUs assigned to the VM. Default is 1. * memory *(optional)*: The amount of memory assigned to the VM. Default is 1024M. - * cdrom *(required)*: A volume name or URI to an ISO image. + * cdrom *(optional)*: A volume name or URI to an ISO image. * storagepool *(optional)*: URI of the storagepool. Default is '/storagepools/default' * networks *(optional)*: list of networks will be assigned to the new VM. @@ -183,6 +183,7 @@ Represents a snapshot of the Virtual Machine's primary monitor. (either *size* or *volume* must be specified): * index: The device index * size: The device size in GB + * base: Base image of this disk
* graphics *(optional)*: The graphics paramenters of this template * type: The type of graphics. It can be VNC or spice or None. diff --git a/src/kimchi/API.json b/src/kimchi/API.json index 6d1324c..60deeb4 100644 --- a/src/kimchi/API.json +++ b/src/kimchi/API.json @@ -362,7 +362,6 @@ "description": "Path for cdrom", "type": "string", "pattern": "^((/)|(http)[s]?:|[t]?(ftp)[s]?:)+.*$", - "required": true, "error": "KCHTMPL0014E" }, "disks": { @@ -381,7 +380,14 @@ "type": "number", "minimum": 1, "error": "KCHTMPL0022E" + }, + "base": { + "description": "Base image of the disk", + "type": "string", + "pattern": "^/.+$", + "error": "KCHTMPL0023E" } + } }, "minItems": 1, diff --git a/src/kimchi/i18n.py b/src/kimchi/i18n.py index daeeed8..7a7b675 100644 --- a/src/kimchi/i18n.py +++ b/src/kimchi/i18n.py @@ -125,6 +125,7 @@ messages = { "KCHTMPL0020E": _("Unable to create template due error: %(err)s"), "KCHTMPL0021E": _("Unable to delete template due error: %(err)s"), "KCHTMPL0022E": _("Disk size must be greater than 1GB."), + "KCHTMPL0023E": _("Template base image must be a valid local image file"),
"KCHPOOL0001E": _("Storage pool %(name)s already exists"), "KCHPOOL0002E": _("Storage pool %(name)s does not exist"),
From: Royce Lv <lvroyce@linux.vnet.ibm.com> Multiple files modification for change 'cdrom' to optional param. Signed-off-by: Royce Lv <lvroyce@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") 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 + 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'} -- 1.8.3.2
On 07/09/2014 07:03 AM, lvroyce@linux.vnet.ibm.com wrote:
From: Royce Lv <lvroyce@linux.vnet.ibm.com>
Multiple files modification for change 'cdrom' to optional param.
Signed-off-by: Royce Lv <lvroyce@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?
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
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'}
On 2014年07月10日 20:36, Aline Manera wrote:
On 07/09/2014 07:03 AM, lvroyce@linux.vnet.ibm.com wrote:
From: Royce Lv <lvroyce@linux.vnet.ibm.com>
Multiple files modification for change 'cdrom' to optional param.
Signed-off-by: Royce Lv <lvroyce@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.
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.
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'}
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@linux.vnet.ibm.com wrote:
From: Royce Lv <lvroyce@linux.vnet.ibm.com>
Multiple files modification for change 'cdrom' to optional param.
Signed-off-by: Royce Lv <lvroyce@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'}
participants (3)
-
Aline Manera -
lvroyce＠linux.vnet.ibm.com -
Royce Lv