Re: [Kimchi-devel] [PATCH] storagevolume: Use default value for param 'name' when appropriate

The parameter 'name' is only required when creating storage volumes with the parameter 'capacity' (i.e. an empty volume). When creating volumes with 'url' or 'file', the volume name will have a default value if 'name' is not specified. Use a default name when creating storage volumes from a file or from a URL. The default name will be the file's or the URL's basename. Signed-off-by: Crístian Viana <vianac(a)linux.vnet.ibm.com&gt; --- src/kimchi/API.json | 2 -- src/kimchi/mockmodel.py | 19 ++++++++++++++----- src/kimchi/model/storagevolumes.py | 26 +++++++++++++++++++++----- tests/test_model.py | 17 +++++++++++------ tests/test_rest.py | 16 ++++++++++++---- 5 files changed, 58 insertions(+), 22 deletions(-) diff --git a/src/kimchi/API.json b/src/kimchi/API.json index 8a95804..1319531 100644 --- a/src/kimchi/API.json +++ b/src/kimchi/API.json @@ -156,13 +156,11 @@ }, "storagevolumes_create": { "type": "object", - "error": "KCHVOL0016E", "properties": { "name": { "description": "The name of the Storage Volume", "type": "string", "minLength": 1, - "required": true, "error": "KCHVOL0013E" }, "capacity": { diff --git a/src/kimchi/mockmodel.py b/src/kimchi/mockmodel.py index 15a75af..ef09a5b 100644 --- a/src/kimchi/mockmodel.py +++ b/src/kimchi/mockmodel.py @@ -482,8 +482,9 @@ class MockModel(object): def storagevolumes_create(self, pool_name, params): vol_source = ['file', 'url', 'capacity'] + require_name_params = ['capacity'] - name = params['name'] + name = params.get('name') index_list = list(i for i in range(len(vol_source)) if vol_source[i] in params) @@ -491,12 +492,20 @@ class MockModel(object): raise InvalidParameter("KCHVOL0018E", {'param': ",".join(vol_source)}) + create_param = vol_source[index_list[0]] + + if name is None: + if create_param in require_name_params: + raise InvalidParameter('KCHVOL0016E') + + name = os.path.basename(create_param) + params['name'] = name + try: - create_func = getattr(self, "_create_volume_with_" + - vol_source[index_list[0]]) + create_func = getattr(self, '_create_volume_with_%s' % + create_param) except AttributeError: - raise InvalidParameter("KCHVOL0019E", - {'param': vol_source[index_list[0]]}) + raise InvalidParameter("KCHVOL0019E", {'param': create_param}) pool = self._get_storagepool(pool_name) if pool.info['type'] in READONLY_POOL_TYPE: diff --git a/src/kimchi/model/storagevolumes.py b/src/kimchi/model/storagevolumes.py index ef8e684..ec610ac 100644 --- a/src/kimchi/model/storagevolumes.py +++ b/src/kimchi/model/storagevolumes.py @@ -44,6 +44,9 @@ VOLUME_TYPE_MAP = {0: 'file', DOWNLOAD_CHUNK_SIZE = 1048576 # 1 MiB +REQUIRE_NAME_PARAMS = ['capacity'] + + class StorageVolumesModel(object): def __init__(self, **kargs): self.conn = kargs['conn'] @@ -53,7 +56,7 @@ class StorageVolumesModel(object): def create(self, pool_name, params): vol_source = ['file', 'url', 'capacity'] - name = params['name'] + name = params.get('name') index_list = list(i for i in range(len(vol_source)) if vol_source[i] in params) @@ -61,12 +64,25 @@ class StorageVolumesModel(object): raise InvalidParameter("KCHVOL0018E", {'param': ",".join(vol_source)}) + create_param = vol_source[index_list[0]] + + if name is None: + # the methods listed in 'REQUIRE_NAME_PARAMS' cannot have + # 'name' == None + if create_param in REQUIRE_NAME_PARAMS: + raise InvalidParameter('KCHVOL0016E') + + # if 'name' is omitted - except for the methods listed in + # 'REQUIRE_NAME_PARAMS' - the default volume name will be the + # file/URL basename. + name = os.path.basename(create_param) + params['name'] = name + try: - create_func = getattr(self, "_create_volume_with_" + - vol_source[index_list[0]]) + create_func = getattr(self, '_create_volume_with_%s' % + create_param) except AttributeError: - raise InvalidParameter("KCHVOL0019E", - {'param': vol_source[index_list[0]]}) + raise InvalidParameter("KCHVOL0019E", {'param': create_param}) pool_info = StoragePoolModel(conn=self.conn, objstore=self.objstore).lookup(pool_name) diff --git a/tests/test_model.py b/tests/test_model.py index 4e9ba97..ecb0a8a 100644 --- a/tests/test_model.py +++ b/tests/test_model.py @@ -526,10 +526,13 @@ class ModelTests(unittest.TestCase): vols = inst.storagevolumes_get_list(pool) num = len(vols) + 2 - params = {'name': vol, - 'capacity': 1024, + params = {'capacity': 1024, 'allocation': 512, 'format': 'raw'} + # 'name' is required for this type of volume + self.assertRaises(InvalidParameter, inst.storagevolumes_create, + pool, params) + params['name'] = vol task_id = inst.storagevolumes_create(pool, params)['id'] self._wait_task(inst, task_id) self.assertEquals('finished', inst.task_lookup(task_id)['status']) @@ -560,20 +563,22 @@ class ModelTests(unittest.TestCase): # download remote volume # 1) try an invalid URL - params = {'name': 'foo', 'url': 'http://www.invalid.url'} + params = {'url': 'http://www.invalid.url'} taskid = inst.storagevolumes_create(pool, params)['id'] self._wait_task(inst, taskid) self.assertEquals('failed', inst.task_lookup(taskid)['status']) # 2) download Kimchi's "COPYING" from Github and compare its # content to the corresponding local file's url = 'https://github.com/kimchi-project/kimchi/raw/master/COPYING' - params = {'name': 'copying', 'url': url} - taskid = inst.storagevolumes_create(pool, params)['id'] + params = {'url': url} + task_response = inst.storagevolumes_create(pool, params) + taskid = task_response['id'] + vol_name = task_response['target_uri'].split('/')[-1] self._wait_task(inst, taskid) self.assertEquals('finished', inst.task_lookup(taskid)['status']) rollback.prependDefer(inst.storagevolume_delete, pool, params['name']) - vol_path = os.path.join(args['path'], params['name']) + vol_path = os.path.join(args['path'], vol_name) self.assertTrue(os.path.isfile(vol_path)) with open(vol_path) as vol_file: vol_content = vol_file.read() diff --git a/tests/test_rest.py b/tests/test_rest.py index ae1c971..a3dcf87 100644 --- a/tests/test_rest.py +++ b/tests/test_rest.py @@ -489,6 +489,14 @@ class RestTests(unittest.TestCase): resp = self.request('/storagepools/tmp/activate', req, 'POST') self.assertEquals(200, resp.status) + # 'name' is required for this type of volume + req = json.dumps({'capacity': 1024, + 'allocation': 512, + 'type': 'disk', + 'format': 'raw'}) + resp = self.request('/storagepools/tmp/storagevolumes', + req, 'POST') + self.assertEquals(400, resp.status) req = json.dumps({'name': "attach-volume", 'capacity': 1024, 'allocation': 512, @@ -1027,16 +1035,16 @@ class RestTests(unittest.TestCase): self.assertEquals('/var/lib/libvirt/images/volume-1', storagevolume['path']) - req = json.dumps({'name': 'downloaded', - 'url': 'https://anyurl.wor.kz'}) + req = json.dumps({'url': 'https://anyurl.wor.kz'}) resp = self.request('/storagepools/pool-1/storagevolumes', req, 'POST') self.assertEquals(202, resp.status) task = json.loads(resp.read()) self._wait_task(task['id']) task = json.loads(self.request('/tasks/%s' % task['id']).read()) self.assertEquals('finished', task['status']) - resp = self.request('/storagepools/pool-1/storagevolumes/downloaded', - '{}', 'GET') + vol_name = task['target_uri'].split('/')[-1] + resp = self.request('/storagepools/pool-1/storagevolumes/%s' % + vol_name, '{}', 'GET') self.assertEquals(200, resp.status) # Now remove the StoragePool from mock model