[Kimchi-devel] [PATCH v2] 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 should 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. If an unknown volume creation method is used (i.e. if another method is added in the future and this function is not updated accordingly), the default name will be based on the current time. Signed-off-by: Crístian Viana <vianac(a)linux.vnet.ibm.com&gt; --- src/kimchi/API.json | 2 -- src/kimchi/mockmodel.py | 24 ++++++++++++++++++----- src/kimchi/model/storagevolumes.py | 32 ++++++++++++++++++++++++++----- tests/test_model.py | 18 ++++++++++++------ tests/test_rest.py | 39 +++++++++++++++++++++----------------- 5 files changed, 80 insertions(+), 35 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..8b58c36 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,25 @@ 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') + + if create_param == 'file': + name = os.path.basename(params['file'].filename) + elif create_param == 'url': + name = os.path.basename(params['url']) + else: + name = 'upload-%s' % int(time.time()) + 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 4d0c231..8d5d697 100644 --- a/src/kimchi/model/storagevolumes.py +++ b/src/kimchi/model/storagevolumes.py @@ -19,6 +19,7 @@ import contextlib import os +import time import urllib2 import libvirt @@ -44,6 +45,9 @@ VOLUME_TYPE_MAP = {0: 'file', READ_CHUNK_SIZE = 1048576 # 1 MiB +REQUIRE_NAME_PARAMS = ['capacity'] + + class StorageVolumesModel(object): def __init__(self, **kargs): self.conn = kargs['conn'] @@ -53,7 +57,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 +65,30 @@ 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. + if create_param == 'file': + name = os.path.basename(params['file'].filename) + elif create_param == 'url': + name = os.path.basename(params['url']) + else: + name = 'upload-%s' % int(time.time()) + 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..5774c82 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,23 @@ 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.assertEquals('COPYING', vol_name) 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 b63e990..c34b4d5 100644 --- a/tests/test_rest.py +++ b/tests/test_rest.py @@ -490,6 +490,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, @@ -1028,16 +1036,17 @@ 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()) + vol_name = task['target_uri'].split('/')[-1] + self.assertEquals('anyurl.wor.kz', vol_name) 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') + resp = self.request('/storagepools/pool-1/storagevolumes/%s' % + vol_name, '{}', 'GET') self.assertEquals(200, resp.status) # Now remove the StoragePool from mock model @@ -1908,27 +1917,25 @@ class RestTests(unittest.TestCase): return headers with RollbackContext() as rollback: - vol_name = 'COPYING' - vol_path = os.path.join(paths.get_prefix(), vol_name) + vol_path = os.path.join(paths.get_prefix(), 'COPYING') url = "https://%s:%s/storagepools/default/storagevolumes" % \ (host, ssl_port) with open(vol_path, 'rb') as fd: r = requests.post(url, files={'file': fd}, - data={'name': vol_name}, verify=False, headers=fake_auth_header()) self.assertEquals(r.status_code, 202) - self._wait_task(r.json()['id']) + task = r.json() + self._wait_task(task['id']) resp = self.request('/storagepools/default/storagevolumes/%s' % - vol_name) + task['target_uri'].split('/')[-1]) self.assertEquals(200, resp.status) # Create a file with 3M to upload - vol_name = '3m-file' - vol_path = os.path.join('/tmp', vol_name) + vol_path = '/tmp/3m-file' with open(vol_path, 'wb') as fd: fd.seek(3*1024*1024-1) fd.write("\0") @@ -1937,20 +1944,19 @@ class RestTests(unittest.TestCase): with open(vol_path, 'rb') as fd: r = requests.post(url, files={'file': fd}, - data={'name': vol_name}, verify=False, headers=fake_auth_header()) self.assertEquals(r.status_code, 202) - self._wait_task(r.json()['id']) + task = r.json() + self._wait_task(task['id']) resp = self.request('/storagepools/default/storagevolumes/%s' % - vol_name) + task['target_uri'].split('/')[-1]) self.assertEquals(200, resp.status) # Create a file with 5M to upload # Max body size is set to 4M so the upload will fail with 413 - vol_name = '5m-file' - vol_path = os.path.join('/tmp', vol_name) + vol_path = '/tmp/5m-file' with open(vol_path, 'wb') as fd: fd.seek(5*1024*1024-1) fd.write("\0") @@ -1959,7 +1965,6 @@ class RestTests(unittest.TestCase): with open(vol_path, 'rb') as fd: r = requests.post(url, files={'file': fd}, - data={'name': vol_name}, verify=False, headers=fake_auth_header()) -- 1.9.3