On 04/06/2016 01:18 AM, Ramon Medeiros wrote:

Signed-off-by: Ramon Medeiros <ramonn@linux.vnet.ibm.com> --- API.json | 9 ++++----- i18n.py | 2 +- 2 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/API.json b/API.json index 6f0b484..fe10848 100644 --- a/API.json +++ b/API.json @@ -468,11 +468,10 @@ "error": "KCHTMPL0011E" }, "memory": { "$ref": "#/kimchitype/memory" }, - "cdrom": { - "description": "Path for cdrom", - "type": "string", - "pattern": "^((/)|(http)[s]?:|[t]?(ftp)[s]?:)+.*$", - "error": "KCHTMPL0014E"

PLEASE, also remove the 'base' parameter on 'disks'

+ "source_media": { + "description": "Path for installation media (ISO, disk, remote ISO)", + "type" : "string", + "pattern" : "^[^ ]+( +[^ ]+)*$"

Also improve the regex to only allow a string starting with /, http, https, tftp, ftp or ftps. And mark source_media as a required parameter.

}, "disks": { "description": "List of disks", diff --git a/i18n.py b/i18n.py index 6214687..57b033d 100644 --- a/i18n.py +++ b/i18n.py @@ -168,7 +168,7 @@ messages = { "KCHTMPL0013E": _("Amount of memory and maximum 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 or a base image to create a template"), + "KCHTMPL0016E": _("Specify a path to source media (ISO, disk or remote ISO) to create a template"), "KCHTMPL0017E": _("All networks for the template must be specified in a list."), "KCHTMPL0018E": _("Specify a volume to a template when storage pool is iSCSI or SCSI"), "KCHTMPL0019E": _("The volume %(volume)s is not in storage pool %(pool)s"),

The same comments I did in the previous patch version. http://lists.ovirt.org/pipermail/kimchi-devel/2016-April/014338.html On 04/06/2016 01:18 AM, Ramon Medeiros wrote:

Template was being check if duplicated after calling for VMTemplate, which can make unecessary work. Check it before creating the class.

Signed-off-by: Ramon Medeiros <ramonn@linux.vnet.ibm.com> --- model/templates.py | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/model/templates.py b/model/templates.py index 92705b6..28c8a08 100644 --- a/model/templates.py +++ b/model/templates.py @@ -52,6 +52,12 @@ class TemplatesModel(object): def create(self, params): name = params.get('name', '').strip() iso = params.get('cdrom') + + # template with the same name already exists: raise exception + with self.objstore as session: + if name in session.get_list('template'): + raise InvalidOperation("KCHTMPL0001E", {'name': name}) + # check search permission if iso and iso.startswith('/') and os.path.exists(iso): st_mode = os.stat(iso).st_mode @@ -72,8 +78,7 @@ class TemplatesModel(object): raise InvalidParameter("KCHTMPL0003E", {'network': net_name, 'template': name}) # Creates the template class with necessary information - # Checkings will be done while creating this class, so any exception - # will be raised here + t = LibvirtVMTemplate(params, scan=True, conn=self.conn)

# Validate cpu info @@ -93,8 +98,6 @@ class TemplatesModel(object): name = params['name'] try: with self.objstore as session: - if name in session.get_list('template'): - raise InvalidOperation("KCHTMPL0001E", {'name': name}) session.store('template', name, t.info, get_kimchi_version()) except InvalidOperation:

On 04/06/2016 01:18 AM, Ramon Medeiros wrote:

First, improve functions inside class. This will improve code reading, also, make more functions to split procedures like: validating disks, graphics xml and distro checking.

Add separate function to identify the installation media.

Signed-off-by: Ramon Medeiros <ramonn@linux.vnet.ibm.com> --- model/templates.py | 17 +++------ vmtemplate.py | 104 ++++++++++++++++++++++++++++++++++++++++++++--------- 2 files changed, 92 insertions(+), 29 deletions(-)

diff --git a/model/templates.py b/model/templates.py index 28c8a08..ca540cd 100644 --- a/model/templates.py +++ b/model/templates.py @@ -51,25 +51,12 @@ class TemplatesModel(object):

def create(self, params): name = params.get('name', '').strip() - iso = params.get('cdrom')

Join this patch with 2/4

# template with the same name already exists: raise exception with self.objstore as session: if name in session.get_list('template'): raise InvalidOperation("KCHTMPL0001E", {'name': name})

- # check search permission - if iso and iso.startswith('/') and os.path.exists(iso): - st_mode = os.stat(iso).st_mode - if stat.S_ISREG(st_mode) or stat.S_ISBLK(st_mode): - user = UserTests().probe_user() - run_setfacl_set_attr(iso, user=user) - ret, excp = probe_file_permission_as_user(iso, user) - if ret is False: - raise InvalidParameter('KCHISO0008E', - {'filename': iso, 'user': user, - 'err': excp}) - conn = self.conn.get() for net_name in params.get(u'networks', []): try: @@ -198,6 +185,10 @@ class TemplateModel(object): params['memory'].update(new_mem) validate_memory(params['memory'])

+ # params are cdrom or disk: add it to source_media + if "cdrom" in params: + params["source_media"] = params.get("cdrom") +

WHY? cdrom will not be on params! Now the only way to create a Template is through source_media!

new_t.update(params)

for net_name in params.get(u'networks', []): diff --git a/vmtemplate.py b/vmtemplate.py index 653ad02..7073d3a 100644 --- a/vmtemplate.py +++ b/vmtemplate.py @@ -17,6 +17,7 @@ # License along with this library; if not, write to the Free Software # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA

+import magic import os import platform import stat @@ -40,6 +41,10 @@ from wok.plugins.kimchi.xmlutils.interface import get_iface_xml from wok.plugins.kimchi.xmlutils.qemucmdline import get_qemucmdline_xml from wok.plugins.kimchi.xmlutils.serial import get_serial_xml

+DISK_TYPE = {"QEMU QCOW Image": "qcow2", + "data": "raw"} +ISO_TYPE = "ISO 9660 CD-ROM" +

class VMTemplate(object): def __init__(self, args, scan=False): @@ -54,20 +59,13 @@ class VMTemplate(object): self.info = {} self.fc_host_support = args.get('fc_host_support')

- # Fetch defaults based on the os distro and version - try: - distro, version = self._get_os_info(args, scan) - except ImageFormatError as e: - raise OperationFailed('KCHTMPL0020E', {'err': e.message}) - os_distro = args.get('os_distro', distro) - os_version = args.get('os_version', version) - entry = osinfo.lookup(os_distro, os_version) - self.info.update(entry)

+ # no record found: create template + if scan:

The 'scan' is not related to new records! It is only to identify if the source_media will be scanned to get its OS and OS version. All the validation should continue to happen in all cases.

+ self._create_template(args, scan)

- # Auto-generate a template name if no one is passed - if 'name' not in args or args['name'] == '': - args['name'] = self._gen_name(distro, version) - self.name = args['name']

Why did you remove the above lines? Where the template name is generated if no one is passed?

+ # template already exists: load it + else: + self.info.update(args)

# Merge graphics settings graph_args = args.get('graphics') @@ -76,8 +74,6 @@ class VMTemplate(object): graphics.update(graph_args) args['graphics'] = graphics

- default_disk = self.info['disks'][0] - # Complete memory args, because dict method update is not recursive if 'memory' in args: if 'current' not in args['memory']: @@ -87,6 +83,42 @@ class VMTemplate(object):

# Override template values according to 'args' self.info.update(args) + + if self.info.get("disks") is not None and len(self.info["disks"]) >= 1: + default_disk = self.info['disks'][0] + + # validate disks + self._validate_disks(default_disk) + + def _create_template(self, args, scan=False): + """ + Creates a new template + """ + # no source_media argument: raise error + if args.get("source_media") is None: + raise MissingParameter('KCHTMPL0016E') + + # identify source media + self._identify_installation_media(args) + + # Fetch defaults based on the os distro and version + try: + distro, version = self._get_os_info(args, scan) + except ImageFormatError as e: + raise OperationFailed('KCHTMPL0020E', {'err': e.message}) + os_distro = args.get('os_distro', distro) + os_version = args.get('os_version', version) + entry = osinfo.lookup(os_distro, os_version) + + # update info + self.info.update(entry) + + # Auto-generate a template name if no one is passed + if 'name' not in args or args['name'] == '': + args['name'] = self._gen_name(distro, version) + self.name = args['name'] + + def _validate_disks(self, default_disk): disks = self.info.get('disks')

basic_disk = ['index', 'format', 'pool', 'size'] @@ -95,7 +127,6 @@ class VMTemplate(object):

for index, disk in enumerate(disks): disk_info = dict(default_disk) - pool_type = self._get_storage_type(disk['pool']['name']) if pool_type in ['iscsi', 'scsi']: disk_info = {'index': 0, 'format': 'raw', 'volume': None} @@ -118,6 +149,47 @@ class VMTemplate(object): disk_info['index'] = disk_info.get('index', index) self.info['disks'][index] = disk_info

+ def _identify_installation_media(self, args): + path = args.get("source_media") +

+ # user did not passed source media: return + if path is None: + return + + # create magic object to discover file type + file_type = magic.open(magic.MAGIC_NONE) + file_type.load() + + # file is local and exists: check if it's disk or cdrom + if path.startswith("/") and os.path.exists(path): + ftype = file_type.file(path)

It is better to check for http/https/tftp/ftp/ftps and already add it to cdrom before starting using magic. Example: if path.startswith(http/https/tftp/ftp/ftps): args[cdrom] = path return if not os.path.exists(path): raise InvalidParameter(Unable to find file %(path)s) # Otherwise, use magic to identify the file # create magic object to discover file type file_type = magic.open(magic.MAGIC_NONE) file_type.load() ftype = file_type.file(path)

+ + # cdrom + if ISO_TYPE in ftype: + args["cdrom"] = path + return +

# if it is not a cdrom, assume it is an image else: disks[0][base] = path

+ # disk + for types in DISK_TYPE.keys(): + if types in ftype: +

We don't need to do that as it is an easy point of failure due the amount of disk format we have.

+ # no disk list: create it + if args.get("disks") is None: + args["disks"] = [] +

+ osinfo_disk = osinfo.lookup("unknow", "unknow")["disks"] + + # os_info has default pool: get it + if osinfo_disk is not None: + args["disks"] = osinfo_disk +

Do not do this! The osinfo information will be get in other place. Only set disk[base] value as you did below.

+ args["disks"][0]["base"] = path

+ + # not local image: set as remote ISO + elif check_url_path(path): + args["cdrom"] = path + return +

isoinfo will do that verification! Here you can check the path starts with some patterns as I described below.

def _get_os_info(self, args, scan): distro = version = 'unknown'

On 04/06/2016 01:18 AM, Ramon Medeiros wrote:

Update all tests to use source_media instead of disks and cdrom

Signed-off-by: Ramon Medeiros <ramonn@linux.vnet.ibm.com> --- tests/test_authorization.py | 4 +- tests/test_livemigration.py | 7 ++-- tests/test_mockmodel.py | 13 ++++--- tests/test_model.py | 94 ++++++++++++++++++++++++--------------------- tests/test_rest.py | 36 ++++++++--------- tests/test_template.py | 47 ++++++++++------------- tests/test_vmtemplate.py | 40 ++++++++++--------- 7 files changed, 124 insertions(+), 117 deletions(-)

diff --git a/tests/test_authorization.py b/tests/test_authorization.py index d88f763..7cf8da2 100644 --- a/tests/test_authorization.py +++ b/tests/test_authorization.py @@ -91,7 +91,7 @@ class AuthorizationTests(unittest.TestCase): # but he can get and create a new one resp = self.request('/plugins/kimchi/templates', '{}', 'GET') self.assertEquals(403, resp.status) - req = json.dumps({'name': 'test', 'cdrom': fake_iso}) + req = json.dumps({'name': 'test', 'source_media': fake_iso}) resp = self.request('/plugins/kimchi/templates', req, 'POST') self.assertEquals(403, resp.status) resp = self.request('/plugins/kimchi/templates/test', '{}', 'PUT') @@ -100,7 +100,7 @@ class AuthorizationTests(unittest.TestCase): self.assertEquals(403, resp.status)

# Non-root users can only get vms authorized to them - model.templates_create({'name': u'test', 'cdrom': fake_iso}) + model.templates_create({'name': u'test', 'source_media': fake_iso})

task_info = model.vms_create({ 'name': u'test-me', diff --git a/tests/test_livemigration.py b/tests/test_livemigration.py index aa8d1bd..34fe853 100644 --- a/tests/test_livemigration.py +++ b/tests/test_livemigration.py @@ -97,12 +97,13 @@ class LiveMigrationTests(unittest.TestCase): ) params = {'name': u'template_test_vm_migrate', 'disks': [], - 'cdrom': UBUNTU_ISO, - 'memory': {'current': 2048, 'maxmemory': 4096 << 10}} + 'source_media': UBUNTU_ISO, + 'memory': {'current': 2048, 'max_memory': 4096 << 10}} + self.inst.templates_create(params) params = {'name': u'template_test_vm_migrate_nonshared', 'disks': [{'name': 'test_vm_migrate.img', 'size': 1}], - 'cdrom': UBUNTU_ISO, + 'source_media': UBUNTU_ISO, 'memory': {'current': 2048, 'maxmemory': 4096*1024}} self.inst.templates_create(params)

diff --git a/tests/test_mockmodel.py b/tests/test_mockmodel.py index 0668191..41156f9 100644 --- a/tests/test_mockmodel.py +++ b/tests/test_mockmodel.py @@ -30,6 +30,7 @@ from wok.exception import InvalidOperation from wok.plugins.kimchi import mockmodel from wok.plugins.kimchi.osinfo import get_template_default

+import iso_gen

test_server = None model = None @@ -50,7 +51,7 @@ def setUpModule(): test_server = run_server(host, port, ssl_port, test_mode=True, model=model) fake_iso = '/tmp/fake.iso' - open(fake_iso, 'w').close() + iso_gen.construct_fake_iso(fake_iso, True, '12.04', 'ubuntu')

def tearDown(): @@ -65,7 +66,7 @@ class MockModelTests(unittest.TestCase):

def test_screenshot_refresh(self): # Create a VM - req = json.dumps({'name': 'test', 'cdrom': fake_iso}) + req = json.dumps({'name': 'test', 'source_media': fake_iso}) request(host, ssl_port, '/plugins/kimchi/templates', req, 'POST') req = json.dumps({'name': 'test-vm', 'template': '/plugins/kimchi/templates/test'}) @@ -95,7 +96,7 @@ class MockModelTests(unittest.TestCase): resp.getheader('last-modified'))

def test_vm_list_sorted(self): - req = json.dumps({'name': 'test', 'cdrom': fake_iso}) + req = json.dumps({'name': 'test', 'source_media': fake_iso}) request(host, ssl_port, '/plugins/kimchi/templates', req, 'POST')

def add_vm(name): @@ -115,7 +116,7 @@ class MockModelTests(unittest.TestCase):

def test_memory_window_changes(self): model.templates_create({'name': u'test', - 'cdrom': fake_iso}) + 'source_media': fake_iso}) task = model.vms_create({'name': u'test-vm', 'template': '/plugins/kimchi/templates/test'}) wait_task(model.task_lookup, task['id']) @@ -127,7 +128,7 @@ class MockModelTests(unittest.TestCase):

def test_hotplug_3D_card(self): model.templates_create({'name': u'test', - 'cdrom': fake_iso}) + 'source_media': fake_iso}) task = model.vms_create({'name': u'test-vm', 'template': '/plugins/kimchi/templates/test'}) wait_task(model.task_lookup, task['id']) @@ -147,7 +148,7 @@ class MockModelTests(unittest.TestCase):

def test_vm_info(self): model.templates_create({'name': u'test', - 'cdrom': fake_iso}) + 'source_media': fake_iso}) task = model.vms_create({'name': u'test-vm', 'template': '/plugins/kimchi/templates/test'}) wait_task(model.task_lookup, task['id']) diff --git a/tests/test_model.py b/tests/test_model.py index c91ba8f..52fdced 100644 --- a/tests/test_model.py +++ b/tests/test_model.py @@ -138,9 +138,10 @@ class ModelTests(unittest.TestCase): vol = inst.storagevolume_lookup(u'default', vol_params['name'])

params = {'name': 'test', 'disks': - [{'base': vol['path'], 'size': 1, 'pool': { - 'name': '/plugins/kimchi/storagepools/default'}}], - 'cdrom': UBUNTU_ISO} + [{'index': 0, 'base': vol['path'], 'size': 1, 'pool': {

Remove the 'base' information.

+ 'name': '/plugins/kimchi/storagepools/default'}, + 'format': 'qcow2'}], 'source_media': UBUNTU_ISO} + inst.templates_create(params) rollback.prependDefer(inst.template_delete, 'test')

@@ -162,7 +163,6 @@ class ModelTests(unittest.TestCase):

self.assertRaises(InvalidOperation, inst.vmsnapshots_create, u'kimchi-vm') - inst.vm_poweroff(u'kimchi-vm') vm = inst.vm_lookup(u'kimchi-vm')

@@ -171,6 +171,9 @@ class ModelTests(unittest.TestCase):

# this snapshot should be deleted when its VM is deleted params = {'name': u'mysnap'} + + vm = inst.vm_lookup(u'kimchi-vm') + task = inst.vmsnapshots_create(u'kimchi-vm', params) inst.task_wait(task['id']) task = inst.task_lookup(task['id']) @@ -208,7 +211,7 @@ class ModelTests(unittest.TestCase): self.assertEquals(snap, current_snap)

# update vm name - inst.vm_update('kimchi-vm', {'name': u'kimchi-vm-new'}) + inst.vm_update(u'kimchi-vm', {'name': u'kimchi-vm-new'})

# Look up the first created snapshot from the renamed vm snap = inst.vmsnapshot_lookup(u'kimchi-vm-new', params['name']) @@ -216,7 +219,6 @@ class ModelTests(unittest.TestCase): # snapshot revert to the first created vm result = inst.vmsnapshot_revert(u'kimchi-vm-new', params['name']) self.assertEquals(result, [u'kimchi-vm', snap['name']]) - vm = inst.vm_lookup(u'kimchi-vm') self.assertEquals(vm['state'], snap['state'])

@@ -247,7 +249,7 @@ class ModelTests(unittest.TestCase): inst.vm_suspend(u'kimchi-vm')

vms = inst.vms_get_list() - self.assertFalse('kimchi-vm' in vms) + self.assertFalse(u'kimchi-vm' in vms)

@unittest.skipUnless(utils.running_as_root(), 'Must be run as root') def test_image_based_template(self): @@ -259,10 +261,14 @@ class ModelTests(unittest.TestCase): 'capacity': 1073741824, # 1 GiB 'allocation': 1048576, # 1 MiB 'format': 'qcow2'} - task_id = inst.storagevolumes_create('default', params)['id'] - rollback.prependDefer(inst.storagevolume_delete, 'default', vol) - inst.task_wait(task_id) - self.assertEquals('finished', inst.task_lookup(task_id)['status']) + + if "base-vol.img" not in inst.storagevolumes_get_list("default"):

Same I commented before. base-vol.img is only based during the tests and should not exist at this point. So why are you adding this condition here?

+ task_id = inst.storagevolumes_create('default', params)['id'] + rollback.prependDefer(inst.storagevolume_delete, 'default', + vol) + inst.task_wait(task_id) + self.assertEquals('finished', + inst.task_lookup(task_id)['status']) vol_path = inst.storagevolume_lookup('default', vol)['path']

# Create template based on IMG file @@ -272,9 +278,9 @@ class ModelTests(unittest.TestCase): "graphics": {"type": "vnc", "listen": "127.0.0.1"}, "networks": ["default"], "memory": {'current': 1024}, "folder": [], "icon": "images/icon-vm.png", - "cdrom": "", "os_distro": "unknown", - "os_version": "unknown", - "disks": [{"base": vol_path, "size": 10, + "os_distro": "unknown", "os_version": "unknown", + "source_media": vol_path, + "disks": [{"size": 10, "format": "qcow2", "pool": {"name": pool_uri}}]}

@@ -299,7 +305,7 @@ class ModelTests(unittest.TestCase): @unittest.skipUnless(utils.running_as_root(), 'Must be run as root') def test_vm_graphics(self): inst = model.Model(objstore_loc=self.tmp_store) - params = {'name': 'test', 'disks': [], 'cdrom': UBUNTU_ISO} + params = {'name': 'test', 'disks': [], 'source_media': UBUNTU_ISO} inst.templates_create(params) with RollbackContext() as rollback: params = {'name': 'kimchi-vnc', @@ -329,7 +335,7 @@ class ModelTests(unittest.TestCase): @unittest.skipUnless(utils.running_as_root(), "Must be run as root") def test_vm_serial(self): inst = model.Model(objstore_loc=self.tmp_store) - params = {'name': 'test', 'disks': [], 'cdrom': UBUNTU_ISO} + params = {'name': 'test', 'disks': [], 'source_media': UBUNTU_ISO} inst.templates_create(params) with RollbackContext() as rollback: params = {'name': 'kimchi-serial', @@ -350,7 +356,7 @@ class ModelTests(unittest.TestCase): def test_vm_ifaces(self): inst = model.Model(objstore_loc=self.tmp_store) with RollbackContext() as rollback: - params = {'name': 'test', 'disks': [], 'cdrom': UBUNTU_ISO} + params = {'name': 'test', 'disks': [], 'source_media': UBUNTU_ISO} inst.templates_create(params) rollback.prependDefer(inst.template_delete, 'test')

@@ -466,7 +472,7 @@ class ModelTests(unittest.TestCase): inst.task_wait(task_id)

vm_name = 'kimchi-cdrom' - params = {'name': 'test', 'disks': [], 'cdrom': UBUNTU_ISO} + params = {'name': 'test', 'disks': [], 'source_media': UBUNTU_ISO} inst.templates_create(params) rollback.prependDefer(inst.template_delete, 'test') params = {'name': vm_name, @@ -510,7 +516,7 @@ class ModelTests(unittest.TestCase):

vm_name = 'kimchi-ide-bus-vm' params = {'name': 'old_distro_template', 'disks': [], - 'cdrom': old_distro_iso} + 'source_media': old_distro_iso} inst.templates_create(params) rollback.prependDefer(inst.template_delete, 'old_distro_template') params = { @@ -535,7 +541,7 @@ class ModelTests(unittest.TestCase): inst = model.Model(objstore_loc=self.tmp_store) with RollbackContext() as rollback: vm_name = 'kimchi-cdrom' - params = {'name': 'test', 'disks': [], 'cdrom': UBUNTU_ISO} + params = {'name': 'test', 'disks': [], 'source_media': UBUNTU_ISO} inst.templates_create(params) rollback.prependDefer(inst.template_delete, 'test') params = {'name': vm_name, @@ -624,9 +630,10 @@ class ModelTests(unittest.TestCase): inst = model.Model(objstore_loc=self.tmp_store)

with RollbackContext() as rollback: - params = {'name': 'test', 'disks': [{'size': 1, 'pool': { - 'name': '/plugins/kimchi/storagepools/default'}}], - 'cdrom': UBUNTU_ISO} + params = {'name': 'test', 'disks': [{'size': 1, 'index': 0, + 'pool': {'name': '/plugins/kimchi/storagepools/default'},

+ 'format': 'qcow2'}], 'source_media': UBUNTU_ISO}

Why did you add format in the parameters?

+ inst.templates_create(params) rollback.prependDefer(inst.template_delete, 'test')

@@ -683,9 +690,9 @@ class ModelTests(unittest.TestCase): self._create_template_conf_with_disk_format('vmdk') rollback.prependDefer(self._restore_template_conf_file)

- params = {'name': 'test', 'disks': [{'size': 1, 'pool': { - 'name': '/plugins/kimchi/storagepools/default'}}], - 'cdrom': UBUNTU_ISO} + params = {'name': 'test', 'disks': [{'index': 0, 'size': 1, + 'pool': {'name': '/plugins/kimchi/storagepools/default'},

+ 'format': 'vmdk'}], 'source_media': UBUNTU_ISO}

Same here.

inst.templates_create(params) rollback.prependDefer(inst.template_delete, 'test')

@@ -710,10 +717,10 @@ class ModelTests(unittest.TestCase): self._create_template_conf_with_disk_format(default_vol) rollback.prependDefer(self._restore_template_conf_file)

- params = {'name': 'test', 'disks': [{ - 'size': 1, 'format': user_vol, - 'pool': {'name': '/plugins/kimchi/storagepools/default'}}], - 'cdrom': UBUNTU_ISO} + params = {'name': 'test', 'disks': [{'index': 0, 'size': 1, + 'pool': {'name': '/plugins/kimchi/storagepools/default'}, + 'format': 'raw'}], 'source_media': UBUNTU_ISO} +

Why did you change the format value here?

inst.templates_create(params) rollback.prependDefer(inst.template_delete, 'test')

@@ -736,9 +743,9 @@ class ModelTests(unittest.TestCase): self._create_template_conf_with_disk_format(None) rollback.prependDefer(self._restore_template_conf_file)

- params = {'name': 'test', 'disks': [{'size': 1, 'pool': { - 'name': '/plugins/kimchi/storagepools/default'}}], - 'cdrom': UBUNTU_ISO} + params = {'name': 'test', 'disks': [{'index': 0, 'size': 1, + 'pool': {'name': '/plugins/kimchi/storagepools/default'}, + 'format': 'qcow2'}], 'source_media': UBUNTU_ISO}

And added here.

inst.templates_create(params) rollback.prependDefer(inst.template_delete, 'test')

@@ -758,7 +765,7 @@ class ModelTests(unittest.TestCase): inst = model.Model(None, objstore_loc=self.tmp_store) orig_params = {'name': 'test', 'memory': {'current': 1024, 'maxmemory': 3072}, - 'cdrom': UBUNTU_ISO} + 'source_media': UBUNTU_ISO} inst.templates_create(orig_params)

with RollbackContext() as rollback: @@ -793,11 +800,13 @@ class ModelTests(unittest.TestCase):

# template disk format must be qcow2 because vmsnapshot # only supports this format - orig_params = {'name': 'test', 'memory': {'current': 1024}, - 'cpu_info': {'vcpus': 1}, - 'cdrom': UBUNTU_ISO, - 'disks': [{'size': 1, 'format': 'qcow2', 'pool': { - 'name': '/plugins/kimchi/storagepools/default'}}]} + orig_params = { + 'name': 'test', 'memory': {'current': 1024}, + 'cpu_info': {'vcpus': 1}, + 'disks': [{ + 'index': 0, 'size': 1, 'format': 'qcow2', + 'pool': {'name': '/plugins/kimchi/storagepools/default'}}], + 'source_media': UBUNTU_ISO} inst.templates_create(orig_params)

with RollbackContext() as rollback: @@ -1034,7 +1043,7 @@ class ModelTests(unittest.TestCase): inst = model.Model(objstore_loc=self.tmp_store)

with RollbackContext() as rollback: - params = {'name': u'test', 'disks': [], 'cdrom': UBUNTU_ISO} + params = {'name': u'test', 'source_media': UBUNTU_ISO}

You can continue to pass 'disks: []'

inst.templates_create(params) rollback.prependDefer(inst.template_delete, 'test')

@@ -1060,7 +1069,7 @@ class ModelTests(unittest.TestCase): inst = model.Model(objstore_loc=self.tmp_store)

with RollbackContext() as rollback: - params = {'name': 'test', 'disks': [], 'cdrom': UBUNTU_ISO} + params = {'name': 'test', 'source_media': UBUNTU_ISO}

Same here.

inst.templates_create(params) rollback.prependDefer(inst.template_delete, 'test')

@@ -1131,8 +1140,7 @@ class ModelTests(unittest.TestCase): with RollbackContext() as rollback: params = { 'name': 'test', - 'disks': [],

And here.

- 'cdrom': UBUNTU_ISO, + 'source_media': UBUNTU_ISO, 'domain': 'test', 'arch': 'i686' } diff --git a/tests/test_rest.py b/tests/test_rest.py index d0d2fcf..a30b0b1 100644 --- a/tests/test_rest.py +++ b/tests/test_rest.py @@ -18,7 +18,6 @@ # License along with this library; if not, write to the Free Software # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA

-import copy import json import os import re @@ -107,7 +106,7 @@ class RestTests(unittest.TestCase): self.assertEquals(1, len(vms))

# Create a template as a base for our VMs - req = json.dumps({'name': 'test', 'cdrom': fake_iso}) + req = json.dumps({'name': 'test', 'source_media': fake_iso}) resp = self.request('/plugins/kimchi/templates', req, 'POST') self.assertEquals(201, resp.status)

@@ -134,7 +133,7 @@ class RestTests(unittest.TestCase): self.assertEquals([], vm['groups'])

def test_edit_vm(self): - req = json.dumps({'name': 'test', 'cdrom': fake_iso}) + req = json.dumps({'name': 'test', 'source_media': fake_iso}) resp = self.request('/plugins/kimchi/templates', req, 'POST') self.assertEquals(201, resp.status)

@@ -291,9 +290,9 @@ class RestTests(unittest.TestCase):

def test_vm_lifecycle(self): # Create a Template - req = json.dumps({'name': 'test', 'disks': DISKS, + req = json.dumps({'name': 'test', 'source_media': fake_iso, 'icon': 'plugins/kimchi/images/icon-debian.png', - 'cdrom': fake_iso}) + 'disks': DISKS}) resp = self.request('/plugins/kimchi/templates', req, 'POST') self.assertEquals(201, resp.status)

@@ -518,7 +517,7 @@ class RestTests(unittest.TestCase):

def test_vm_graphics(self): # Create a Template - req = json.dumps({'name': 'test', 'cdrom': fake_iso}) + req = json.dumps({'name': 'test', 'source_media': fake_iso}) resp = self.request('/plugins/kimchi/templates', req, 'POST') self.assertEquals(201, resp.status)

@@ -612,7 +611,7 @@ class RestTests(unittest.TestCase):

with RollbackContext() as rollback: # Create a template as a base for our VMs - req = json.dumps({'name': 'test', 'cdrom': fake_iso}) + req = json.dumps({'name': 'test', 'source_media': fake_iso}) resp = self.request('/plugins/kimchi/templates', req, 'POST') self.assertEquals(201, resp.status) # Delete the template @@ -785,7 +784,7 @@ class RestTests(unittest.TestCase):

with RollbackContext() as rollback: # Create a template as a base for our VMs - req = json.dumps({'name': 'test', 'cdrom': fake_iso}) + req = json.dumps({'name': 'test', 'source_media': fake_iso}) resp = self.request('/plugins/kimchi/templates', req, 'POST') self.assertEquals(201, resp.status) # Delete the template @@ -891,8 +890,8 @@ class RestTests(unittest.TestCase):

def test_vm_customise_storage(self): # Create a Template - req = json.dumps({'name': 'test', 'cdrom': fake_iso, - 'disks': DISKS}) + req = json.dumps({'name': 'test', 'source_media': fake_iso, + 'disks': DISKS}) resp = self.request('/plugins/kimchi/templates', req, 'POST') self.assertEquals(201, resp.status)

@@ -954,8 +953,9 @@ class RestTests(unittest.TestCase):

# Create template fails because SCSI volume is missing tmpl_params = { - 'name': 'test_fc_pool', 'cdrom': fake_iso, 'disks': [{'pool': { - 'name': '/plugins/kimchi/storagepools/scsi_fc_pool'}}]} + 'name': 'test_fc_pool', 'source_media': fake_iso, + 'disks': [{'pool': {'name': + '/plugins/kimchi/storagepools/scsi_fc_pool'}}]}

req = json.dumps(tmpl_params) resp = self.request('/plugins/kimchi/templates', req, 'POST') @@ -998,7 +998,7 @@ class RestTests(unittest.TestCase):

def test_unnamed_vms(self): # Create a Template - req = json.dumps({'name': 'test', 'cdrom': fake_iso}) + req = json.dumps({'name': 'test', 'source_media': fake_iso}) resp = self.request('/plugins/kimchi/templates', req, 'POST') self.assertEquals(201, resp.status)

@@ -1039,10 +1039,8 @@ class RestTests(unittest.TestCase):

# Create a Template mock_base = '/tmp/mock.img' - open(mock_base, 'w').close() - disks = copy.deepcopy(DISKS) - disks[0]['base'] = mock_base - req = json.dumps({'name': 'test', 'disks': disks}) + os.system("qemu-img create -f qcow2 %s 10M" % mock_base) + req = json.dumps({'name': 'test', 'source_media': mock_base}) resp = self.request('/plugins/kimchi/templates', req, 'POST') self.assertEquals(201, resp.status)

@@ -1112,7 +1110,7 @@ class RestTests(unittest.TestCase): # In real model os distro/version can be omitted # as we will scan the iso req = json.dumps({'name': 'test', - 'cdrom': storagevolume['path'], + 'source_media': storagevolume['path'], 'os_distro': storagevolume['os_distro'], 'os_version': storagevolume['os_version']}) resp = self.request('/plugins/kimchi/templates', req, 'POST') @@ -1150,7 +1148,7 @@ class RestTests(unittest.TestCase):

def test_screenshot_refresh(self): # Create a VM - req = json.dumps({'name': 'test', 'cdrom': fake_iso}) + req = json.dumps({'name': 'test', 'source_media': fake_iso}) resp = self.request('/plugins/kimchi/templates', req, 'POST') req = json.dumps({'name': 'test-vm', 'template': '/plugins/kimchi/templates/test'}) diff --git a/tests/test_template.py b/tests/test_template.py index fcb2e46..d9e7f36 100644 --- a/tests/test_template.py +++ b/tests/test_template.py @@ -18,6 +18,7 @@ # License along with this library; if not, write to the Free Software # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA

+import iso_gen import json import os import psutil @@ -38,7 +39,7 @@ host = None port = None ssl_port = None cherrypy_port = None - +MOCK_ISO = "/tmp/mock.iso" DEFAULT_POOL = u'/plugins/kimchi/storagepools/default-pool'

@@ -53,6 +54,7 @@ def setUpModule(): cherrypy_port = get_free_port('cherrypy_port') test_server = run_server(host, port, ssl_port, test_mode=True, cherrypy_port=cherrypy_port, model=model) + iso_gen.construct_fake_iso(MOCK_ISO, True, '14.04', 'ubuntu')

def tearDownModule(): @@ -79,7 +81,7 @@ class TemplateTests(unittest.TestCase): self.assertEquals(400, resp.status)

# Create a template - t = {'name': 'test', 'cdrom': '/tmp/mock.iso'} + t = {'name': 'test', 'source_media': MOCK_ISO} req = json.dumps(t) resp = self.request('/plugins/kimchi/templates', req, 'POST') self.assertEquals(201, resp.status) @@ -117,22 +119,20 @@ class TemplateTests(unittest.TestCase): self.assertEquals(204, resp.status)

# Create a template with same name fails with 400 - req = json.dumps({'name': 'test', 'cdrom': '/tmp/mock.iso'}) + req = json.dumps({'name': 'test', 'source_media': MOCK_ISO}) resp = self.request('/plugins/kimchi/templates', req, 'POST') self.assertEquals(400, resp.status)

# Create an image based template - open('/tmp/mock.img', 'w').close() - t = {'name': 'test_img_template', 'disks': [{ - 'base': '/tmp/mock.img', 'format': 'qcow2', - 'pool': {'name': DEFAULT_POOL}, 'size': 1}]} + os.system("qemu-img create -f qcow2 %s 10G" % '/tmp/mock.img') + t = {'name': 'test_img_template', 'source_media': '/tmp/mock.img'} req = json.dumps(t) resp = self.request('/plugins/kimchi/templates', req, 'POST') self.assertEquals(201, resp.status) os.remove('/tmp/mock.img')

# Test disk format - t = {'name': 'test-format', 'cdrom': '/tmp/mock.iso', 'disks': [{ + t = {'name': 'test-format', 'source_media': MOCK_ISO, 'disks': [{ 'index': 0, 'size': 10, 'format': 'vmdk', 'pool': { 'name': DEFAULT_POOL}}]} req = json.dumps(t) @@ -149,7 +149,7 @@ class TemplateTests(unittest.TestCase): else: max_mem = (psutil.TOTAL_PHYMEM >> 10 >> 10) memory = max_mem + 1024 - t = {'name': 'test-maxmem', 'cdrom': '/tmp/mock.iso', + t = {'name': 'test-maxmem', 'source_media': MOCK_ISO, 'memory': {'current': memory}} req = json.dumps(t) resp = self.request('/plugins/kimchi/templates', req, 'POST') @@ -158,7 +158,7 @@ class TemplateTests(unittest.TestCase):

def test_customized_tmpl(self): # Create a template - t = {'name': 'test', 'cdrom': '/tmp/mock.iso'} + t = {'name': 'test', 'source_media': MOCK_ISO} req = json.dumps(t) resp = self.request('/plugins/kimchi/templates', req, 'POST') self.assertEquals(201, resp.status) @@ -328,7 +328,7 @@ class TemplateTests(unittest.TestCase):

def test_customized_network(self): # Create a template - t = {'name': 'test', 'cdrom': '/tmp/mock.iso'} + t = {'name': 'test', 'source_media': MOCK_ISO} req = json.dumps(t) resp = self.request('/plugins/kimchi/templates', req, 'POST') self.assertEquals(201, resp.status) @@ -364,7 +364,7 @@ class TemplateTests(unittest.TestCase):

def test_customized_storagepool(self): # Create a template - t = {'name': 'test', 'cdrom': '/tmp/mock.iso'} + t = {'name': 'test', 'source_media': MOCK_ISO} req = json.dumps(t) resp = self.request('/plugins/kimchi/templates', req, 'POST') self.assertEquals(201, resp.status)

@@ -410,13 +410,16 @@ class TemplateTests(unittest.TestCase): vol = vols[0]['name'] req = json.dumps({'disks': [{'volume': vol, 'pool': {'name': pool_uri}, - 'format': 'raw'}]}) + 'format': 'raw', + 'index': 1}]}) elif pool['type'] == 'logical': req = json.dumps({'disks': [{'pool': {'name': pool_uri}, - 'format': 'raw', 'size': 10}]}) + 'format': 'raw', 'size': 10, + 'index': 1}]}) else: req = json.dumps({'disks': [{'pool': {'name': pool_uri}, - 'format': 'qcow2', 'size': 10}]}) + 'format': 'qcow2', 'size': 10, + 'index': 1}]})

Why index is needed?

if req is not None: resp = self.request('/plugins/kimchi/templates/test', req, @@ -425,7 +428,7 @@ class TemplateTests(unittest.TestCase):

# Test disk template update with different pool pool_uri = u'/plugins/kimchi/storagepools/kīмсhīUnitTestDirPool' - disk_data = {'disks': [{'size': 5, 'format': 'qcow2', + disk_data = {'disks': [{'size': 5, 'format': 'qcow2', 'index': 0, 'pool': {'name': pool_uri}}]}

Same here.

req = json.dumps(disk_data) resp = self.request('/plugins/kimchi/templates/test', req, 'PUT') @@ -453,12 +456,8 @@ class TemplateTests(unittest.TestCase): self.request(pool_uri + '/activate', '{}', 'POST')

# Create a template using the custom network and pool - t = {'name': 'test', 'cdrom': '/tmp/mock.iso', - 'networks': ['nat-network'], - 'disks': [{'pool': { - 'name': '/plugins/kimchi/storagepools/dir-pool'}, - 'size': 2, - 'format': 'qcow2'}]} + t = {'name': 'test', 'source_media': MOCK_ISO, + 'networks': ['nat-network']}

Why did you remove disks parameter?

req = json.dumps(t) resp = self.request('/plugins/kimchi/templates', req, 'POST') self.assertEquals(201, resp.status) @@ -482,7 +481,3 @@ class TemplateTests(unittest.TestCase): resp = self.request('/plugins/kimchi/storagepools/dir-pool', '{}', 'DELETE') self.assertEquals(400, resp.status) -

- # Verify the template - res = json.loads(self.request('/plugins/kimchi/templates/test').read()) - self.assertEquals(res['invalid']['cdrom'], ['/tmp/mock.iso'])

Why did you remove it? It confirms the source_media was properly used to set the cdrom value.

diff --git a/tests/test_vmtemplate.py b/tests/test_vmtemplate.py index eed58b0..d50dbd2 100644 --- a/tests/test_vmtemplate.py +++ b/tests/test_vmtemplate.py

As we discussed offline, vmtemplate will keep the same but all those changes will not be needed anymore.

@@ -17,6 +17,7 @@ # License along with this library; if not, write to the Free Software # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA

+import iso_gen import os import psutil import unittest @@ -36,7 +37,7 @@ DISKS = [{'size': 10, 'format': 'raw', 'index': 0, 'pool': {'name': class VMTemplateTests(unittest.TestCase): def setUp(self): self.iso = '/tmp/mock.iso' - open(self.iso, 'w').close() + iso_gen.construct_fake_iso(self.iso, True, '12.04', 'ubuntu')

def tearDown(self): os.unlink(self.iso) @@ -45,51 +46,52 @@ class VMTemplateTests(unittest.TestCase): disk_bus = get_template_default('old', 'disk_bus') memory = get_template_default('old', 'memory') nic_model = get_template_default('old', 'nic_model') - fields = (('name', 'test'), ('cdrom', self.iso), + fields = (('name', 'test'), ('source_media', self.iso), ('os_distro', 'unknown'), ('os_version', 'unknown'), ('cpu_info', {'vcpus': 1, 'maxvcpus': 1}), ('memory', memory), ('networks', ['default']), ('disk_bus', disk_bus), ('nic_model', nic_model), ('graphics', {'type': 'vnc', 'listen': '127.0.0.1'}))

- args = {'name': 'test', 'cdrom': self.iso} - t = VMTemplate(args) + args = {'name': 'test', 'source_media': self.iso} + t = VMTemplate(args, True) for name, val in fields: self.assertEquals(val, t.info.get(name))

def test_construct_overrides(self): graphics = {'type': 'spice', 'listen': '127.0.0.1'} - args = {'name': 'test', 'disks': DISKS, - 'graphics': graphics, "cdrom": self.iso} - t = VMTemplate(args) + args = {'name': 'test', 'graphics': graphics, 'disks': DISKS, + 'source_media': self.iso} + t = VMTemplate(args, True) self.assertEquals(2, len(t.info['disks'])) self.assertEquals(graphics, t.info['graphics'])

def test_specified_graphics(self): # Test specified listen graphics = {'type': 'vnc', 'listen': '127.0.0.1'} - args = {'name': 'test', 'disks': DISKS, - 'graphics': graphics, 'cdrom': self.iso} - t = VMTemplate(args) + args = {'name': 'test', 'graphics': graphics, 'disks': DISKS, + 'source_media': self.iso} + t = VMTemplate(args, True) self.assertEquals(graphics, t.info['graphics'])

# Test specified type graphics = {'type': 'spice', 'listen': '127.0.0.1'} args['graphics'] = graphics - t = VMTemplate(args) + t = VMTemplate(args, True) self.assertEquals(graphics, t.info['graphics'])

# If no listen specified, test the default listen graphics = {'type': 'vnc'} args['graphics'] = graphics - t = VMTemplate(args) + t = VMTemplate(args, True) self.assertEquals(graphics['type'], t.info['graphics']['type']) self.assertEquals('127.0.0.1', t.info['graphics']['listen'])

def test_to_xml(self): graphics = {'type': 'spice', 'listen': '127.0.0.1'} vm_uuid = str(uuid.uuid4()).replace('-', '') - t = VMTemplate({'name': 'test-template', 'cdrom': self.iso}) + t = VMTemplate({'name': 'test-template', 'source_media': self.iso}, + True) xml = t.to_vm_xml('test-vm', vm_uuid, graphics=graphics) self.assertEquals(vm_uuid, xpath_get_text(xml, "/domain/uuid")[0]) self.assertEquals('test-vm', xpath_get_text(xml, "/domain/name")[0]) @@ -108,8 +110,9 @@ class VMTemplateTests(unittest.TestCase): host_memory = psutil.virtual_memory().total >> 10 else: host_memory = psutil.TOTAL_PHYMEM >> 10 - t = VMTemplate({'name': 'test-template', 'cdrom': self.iso, - 'memory': {'current': (host_memory >> 10) - 512}}) + t = VMTemplate({'name': 'test-template', 'source_media': self.iso, + 'memory': {'current': (host_memory >> 10) - 512}}, + True) try: xml = t.to_vm_xml('test-vm', vm_uuid, graphics=graphics) except Exception as e: @@ -125,12 +128,13 @@ class VMTemplateTests(unittest.TestCase): args = {'name': 'test', 'os_distro': 'opensuse', 'os_version': '12.3', 'cpu_info': {'vcpus': 2, 'maxvcpus': 4}, 'memory': {'current': 2048, 'maxmemory': 3072}, - 'networks': ['foo'], 'cdrom': self.iso, 'graphics': graphics} - t = VMTemplate(args) + 'networks': ['foo'], 'source_media': self.iso, + 'graphics': graphics} + t = VMTemplate(args, True) self.assertEquals(2, t.info.get('cpu_info', {}).get('vcpus')) self.assertEquals(4, t.info.get('cpu_info', {}).get('maxvcpus')) self.assertEquals(2048, t.info.get('memory').get('current')) self.assertEquals(3072, t.info.get('memory').get('maxmemory')) self.assertEquals(['foo'], t.info.get('networks')) - self.assertEquals(self.iso, t.info.get('cdrom')) + self.assertEquals(self.iso, t.info.get('source_media')) self.assertEquals(graphics, t.info.get('graphics'))

Please, also add new tests to verify the source_media works! Create a template with an ISO file and verify cdrom parameter is filled with the right value. Same for an IMG file.