[PATCH 0/3 V6] Storage volume upload
V5 - V6: - Enclose offset information by the lock - Update test case to store file content in a variable to avoid IO v4 -> v5: - Reduce number of locks - Update MockModel and test cases accordingly Aline Manera (2): Upload storage volume Remove storage volume creation from file Royce Lv (1): Update controller to make update accept formdata params docs/API.md | 8 +++- src/kimchi/API.json | 22 ++++++++++ src/kimchi/control/base.py | 6 +-- src/kimchi/i18n.py | 6 +++ src/kimchi/mockmodel.py | 34 +++++++++------ src/kimchi/model/storagevolumes.py | 86 ++++++++++++++++++++----------------- tests/test_model_storagevolume.py | 88 +++++++++++++++++++++++++++++++------- tests/test_rest.py | 45 +------------------ 8 files changed, 180 insertions(+), 115 deletions(-) -- 2.1.0
From: Royce Lv <lvroyce@linux.vnet.ibm.com> When update does not accept params in base class, cherrypy will raise error that extra params are provided in body. So allow update function to accept params. Signed-off-by: Royce Lv <lvroyce@linux.vnet.ibm.com> --- src/kimchi/control/base.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/kimchi/control/base.py b/src/kimchi/control/base.py index b50ea5c..b9520c0 100644 --- a/src/kimchi/control/base.py +++ b/src/kimchi/control/base.py @@ -156,7 +156,7 @@ class Resource(object): raise cherrypy.HTTPError(400, e.message) @cherrypy.expose - def index(self): + def index(self, *args, **kargs): method = validate_method(('GET', 'DELETE', 'PUT'), self.role_key, self.admin_methods) @@ -167,7 +167,7 @@ class Resource(object): return {'GET': self.get, 'DELETE': self.delete, - 'PUT': self.update}[method]() + 'PUT': self.update}[method](*args, **kargs) except InvalidOperation, e: raise cherrypy.HTTPError(400, e.message) except InvalidParameter, e: @@ -194,7 +194,7 @@ class Resource(object): return user_name in users or len(set(user_groups) & set(groups)) > 0 - def update(self): + def update(self, *args, **kargs): try: update = getattr(self.model, model_fn(self, 'update')) except AttributeError: -- 2.1.0
Storage volume upload will use the same REST API as 'capacity' to create the storage volume for upload. To do that, the 'upload' parameter must be set accordingly. POST /storagepools/<pool-name>/storagevolumes/ {"capacity": 1000000, "format": "raw", "name": "volume-1", "upload": true} After storage volume is created, the data transfer will be done through several PUT requests: PUT /storagepools/<pool-name>/storagevolumes/volume-1 {"chunk_size": "1024", "chunk": form-data} PUT /storagepools/<pool-name>/storagevolumes/volume-1 {"chunk_size": "1024", "chunk": form-data} ... PUT /storagepools/<pool-name>/storagevolumes/volume-1 {"chunk_size": "1024", "chunk": form-data} Kimchi will keep the file offset internally to avoid data corruption. So all the PUT requests must be sequential. This patch also updates MockModel to manually does the upload process as the virStorageVol.upload() function is not supported by the libvirt Test driver. The test cases were also updated accordindly to those changes. Signed-off-by: Aline Manera <alinefm@linux.vnet.ibm.com> Signed-off-by: Royce Lv <lvroyce@linux.vnet.ibm.com> --- docs/API.md | 5 +++ src/kimchi/API.json | 22 ++++++++++ src/kimchi/i18n.py | 6 +++ src/kimchi/mockmodel.py | 18 +++++++- src/kimchi/model/storagevolumes.py | 50 +++++++++++++++++++--- tests/test_model_storagevolume.py | 88 +++++++++++++++++++++++++++++++------- tests/test_rest.py | 45 +------------------ 7 files changed, 169 insertions(+), 65 deletions(-) diff --git a/docs/API.md b/docs/API.md index 62e4063..88c5fec 100644 --- a/docs/API.md +++ b/docs/API.md @@ -487,6 +487,8 @@ A interface represents available network interface on VM. The unit is bytes * format: The format of the defined Storage Volume. Only used when creating a storage volume with 'capacity'. + * upload: True to start an upload process. False, otherwise. + Only used when creating a storage volume 'capacity' parameter. * file: File to be uploaded, passed through form data * url: URL to be downloaded @@ -515,6 +517,9 @@ A interface represents available network interface on VM. * **DELETE**: Remove the Storage Volume * **POST**: *See Storage Volume Actions* +* **PUT**: Upload storage volume chunk + * chunk_size: Chunk size of the slice in Bytes. + * chunk: Actual data of uploaded file **Actions (POST):** diff --git a/src/kimchi/API.json b/src/kimchi/API.json index 474661c..a6330ae 100644 --- a/src/kimchi/API.json +++ b/src/kimchi/API.json @@ -202,6 +202,11 @@ "minimum": 1, "error": "KCHVOL0020E" }, + "upload": { + "description": "When the storage volume will be uploaded", + "type": "boolean", + "error": "KCHVOL0025E" + }, "allocation": { "description": "The size(MiB) of allocation when create the storage volume", "type": "number", @@ -222,6 +227,23 @@ } } }, + "storagevolume_update": { + "type": "object", + "properties": { + "chunk": { + "description": "Upload storage volume chunk", + "error": "KCHVOL0024E", + "required": true + }, + "chunk_size": { + "description": "Chunk size of uploaded storage volume", + "type": "string", + "error": "KCHVOL0024E", + "required": true + } + }, + "additionalProperties": false + }, "vms_create": { "type": "object", "error": "KCHVM0016E", diff --git a/src/kimchi/i18n.py b/src/kimchi/i18n.py index 18e84bc..9f169ab 100644 --- a/src/kimchi/i18n.py +++ b/src/kimchi/i18n.py @@ -214,6 +214,12 @@ messages = { "KCHVOL0021E": _("Storage volume URL must be http://, https://, ftp:// or ftps://."), "KCHVOL0022E": _("Unable to access file %(url)s. Please, check it."), "KCHVOL0023E": _("Unable to clone storage volume '%(name)s' in pool '%(pool)s'. Details: %(err)s"), + "KCHVOL0024E": _("Specify chunk data and its size to upload a file."), + "KCHVOL0025E": _("In order to upload a storage volume, specify the 'upload' parameter."), + "KCHVOL0026E": _("Unable to upload chunk data as it does not match with requested chunk size."), + "KCHVOL0027E": _("The storage volume %(vol)s is not under an upload process."), + "KCHVOL0028E": _("The upload chunk data will exceed the storage volume size."), + "KCHVOL0029E": _("Unable to upload chunk data to storage volume. Details: %(err)s."), "KCHIFACE0001E": _("Interface %(name)s does not exist"), diff --git a/src/kimchi/mockmodel.py b/src/kimchi/mockmodel.py index ae97354..4395883 100644 --- a/src/kimchi/mockmodel.py +++ b/src/kimchi/mockmodel.py @@ -38,7 +38,7 @@ from kimchi.model.libvirtstoragepool import IscsiPoolDef, NetfsPoolDef from kimchi.model.libvirtstoragepool import StoragePoolDef from kimchi.model.model import Model from kimchi.model.storagepools import StoragePoolModel -from kimchi.model.storagevolumes import StorageVolumesModel +from kimchi.model.storagevolumes import StorageVolumeModel, StorageVolumesModel from kimchi.model.templates import LibvirtVMTemplate from kimchi.model.users import PAMUsersModel from kimchi.model.groups import PAMGroupsModel @@ -112,6 +112,7 @@ class MockModel(Model): DeviceModel.lookup = self._mock_device_lookup StoragePoolModel._update_lvm_disks = self._update_lvm_disks StorageVolumesModel.get_list = self._mock_storagevolumes_get_list + StorageVolumeModel.doUpload = self._mock_storagevolume_doUpload DebugReportsModel._gen_debugreport_file = self._gen_debugreport_file LibvirtVMTemplate._get_volume_path = self._get_volume_path VMTemplate.get_iso_info = self._probe_image @@ -313,6 +314,21 @@ class MockModel(Model): return self._model_storagevolume_lookup(pool, vol) + def _mock_storagevolume_doUpload(self, vol, offset, data, data_size): + vol_path = vol.path() + + # MockModel does not create the storage volume as a file + # So create it to do the file upload + if offset == 0: + dirname = os.path.dirname(vol_path) + if not os.path.exists(dirname): + os.makedirs(dirname) + open(vol_path, 'w').close() + + with open(vol_path, 'a') as fd: + fd.seek(offset) + fd.write(data) + def _mock_partitions_get_list(self): return self._mock_partitions.partitions.keys() diff --git a/src/kimchi/model/storagevolumes.py b/src/kimchi/model/storagevolumes.py index cf4611d..8ca71a9 100644 --- a/src/kimchi/model/storagevolumes.py +++ b/src/kimchi/model/storagevolumes.py @@ -21,6 +21,7 @@ import contextlib import lxml.etree as ET import os import tempfile +import threading import time import urllib2 from lxml.builder import E @@ -44,12 +45,11 @@ VOLUME_TYPE_MAP = {0: 'file', 2: 'directory', 3: 'network'} - READ_CHUNK_SIZE = 1048576 # 1 MiB - - REQUIRE_NAME_PARAMS = ['capacity'] +upload_volumes = dict() + class StorageVolumesModel(object): def __init__(self, **kargs): @@ -186,10 +186,13 @@ class StorageVolumesModel(object): objstore=self.objstore).lookup(pool_name, name) + vol_path = vol_info['path'] + if params.get('upload', False): + upload_volumes[vol_path] = {'lock': threading.Lock(), 'offset': 0} + try: with self.objstore as session: - session.store('storagevolume', vol_info['path'], - {'ref_cnt': 0}) + session.store('storagevolume', vol_path, {'ref_cnt': 0}) except Exception as e: # If the storage volume was created flawlessly, then lets hide this # error to avoid more error in the VM creation process @@ -491,6 +494,43 @@ class StorageVolumeModel(object): cb('OK', True) + def doUpload(self, vol, offset, data, data_size): + try: + st = self.conn.get().newStream(0) + vol.upload(st, offset, data_size) + st.send(data) + st.finish() + except Exception as e: + st and st.abort() + raise OperationFailed("KCHVOL0029E", {"err": e.message}) + + def update(self, pool, name, params): + chunk_data = params['chunk'].fullvalue() + chunk_size = int(params['chunk_size']) + + if len(chunk_data) != chunk_size: + raise OperationFailed("KCHVOL0026E") + + vol = StorageVolumeModel.get_storagevolume(pool, name, self.conn) + vol_path = vol.path() + vol_capacity = vol.info()[1] + + vol_data = upload_volumes.get(vol_path) + if vol_data is None: + raise OperationFailed("KCHVOL0027E", {"vol": vol_path}) + + lock = vol_data['lock'] + with lock: + offset = vol_data['offset'] + if (offset + chunk_size) > vol_capacity: + raise OperationFailed("KCHVOL0028E") + + self.doUpload(vol, offset, chunk_data, chunk_size) + + vol_data['offset'] += chunk_size + if vol_data['offset'] == vol_capacity: + del upload_volumes[vol_path] + class IsoVolumesModel(object): def __init__(self, **kargs): diff --git a/tests/test_model_storagevolume.py b/tests/test_model_storagevolume.py index a3c3ce3..fea1de1 100644 --- a/tests/test_model_storagevolume.py +++ b/tests/test_model_storagevolume.py @@ -149,24 +149,82 @@ def _do_volume_test(self, model, host, ssl_port, pool_name): resp = self.request(vol_uri) self.assertEquals(404, resp.status) - # Create storage volume with 'file' - filepath = os.path.join(paths.get_prefix(), 'COPYING.LGPL') - url = 'https://%s:%s' % (host, ssl_port) + uri - with open(filepath, 'rb') as fd: - r = requests.post(url, files={'file': fd}, - verify=False, - headers=fake_auth_header()) - + # Storage volume upload + # It is done through a sequence of POST and several PUT requests + filename = 'COPYING.LGPL' + filepath = os.path.join(paths.get_prefix(), filename) + filesize = os.stat(filepath).st_size + + # Create storage volume for upload + req = json.dumps({'name': filename, 'format': 'raw', + 'capacity': filesize, 'upload': True}) + resp = self.request(uri, req, 'POST') if pool_info['type'] in READONLY_POOL_TYPE: - self.assertEquals(r.status_code, 400) + self.assertEquals(400, resp.status) else: - rollback.prependDefer(model.storagevolume_delete, pool_name, - 'COPYING.LGPL') - self.assertEquals(r.status_code, 202) - task = r.json() - wait_task(_task_lookup, task['id']) - resp = self.request(uri + '/COPYING.LGPL') + rollback.prependDefer(rollback_wrapper, model.storagevolume_delete, + pool_name, filename) + self.assertEquals(202, resp.status) + task_id = json.loads(resp.read())['id'] + wait_task(_task_lookup, task_id) + status = json.loads(self.request('/tasks/%s' % task_id).read()) + self.assertEquals('finished', status['status']) + + # Upload volume content + url = 'https://%s:%s' % (host, ssl_port) + uri + '/' + filename + + # Create a file with 5M to upload + # Max body size is set to 4M so the upload will fail with 413 + newfile = '/tmp/5m-file' + with open(newfile, 'wb') as fd: + fd.seek(5*1024*1024-1) + fd.write("\0") + rollback.prependDefer(os.remove, newfile) + + with open(newfile, 'rb') as fd: + with open(newfile + '.tmp', 'wb') as tmp_fd: + data = fd.read() + tmp_fd.write(data) + + with open(newfile + '.tmp', 'rb') as tmp_fd: + r = requests.put(url, data={'chunk_size': len(data)}, + files={'chunk': tmp_fd}, + verify=False, + headers=fake_auth_header()) + self.assertEquals(r.status_code, 413) + + # Do upload + index = 0 + chunk_size = 2 * 1024 + content = '' + + with open(filepath, 'rb') as fd: + while True: + with open(filepath + '.tmp', 'wb') as tmp_fd: + fd.seek(index*chunk_size) + data = fd.read(chunk_size) + tmp_fd.write(data) + + with open(filepath + '.tmp', 'rb') as tmp_fd: + r = requests.put(url, data={'chunk_size': len(data)}, + files={'chunk': tmp_fd}, + verify=False, + headers=fake_auth_header()) + self.assertEquals(r.status_code, 200) + content += data + index = index + 1 + + if len(data) < chunk_size: + break + + rollback.prependDefer(os.remove, filepath + '.tmp') + resp = self.request(uri + '/' + filename) self.assertEquals(200, resp.status) + uploaded_path = json.loads(resp.read())['path'] + with open(uploaded_path) as fd: + uploaded_content = fd.read() + + self.assertEquals(content, uploaded_content) # Create storage volume with 'url' url = 'https://github.com/kimchi-project/kimchi/raw/master/COPYING' diff --git a/tests/test_rest.py b/tests/test_rest.py index c2ebd88..914b602 100644 --- a/tests/test_rest.py +++ b/tests/test_rest.py @@ -21,7 +21,6 @@ import json import os import re -import requests import time import unittest import urllib2 @@ -35,7 +34,7 @@ import kimchi.server from kimchi.osinfo import get_template_default from kimchi.rollbackcontext import RollbackContext from kimchi.utils import add_task -from utils import fake_auth_header, get_free_port, patch_auth, request +from utils import get_free_port, patch_auth, request from utils import run_server, wait_task @@ -1188,48 +1187,6 @@ class RestTests(unittest.TestCase): resp = self.request('%s/fedora-fake' % base_uri, '{}', 'DELETE') self.assertEquals(204, resp.status) - def test_upload(self): - with RollbackContext() as rollback: - url = "https://%s:%s/storagepools/default-pool/storagevolumes" % \ - (host, ssl_port) - - # Create a file with 3M to upload - vol_path = '/tmp/3m-file' - with open(vol_path, 'wb') as fd: - fd.seek(3*1024*1024-1) - fd.write("\0") - rollback.prependDefer(os.remove, vol_path) - - with open(vol_path, 'rb') as fd: - r = requests.post(url, - files={'file': fd}, - verify=False, - headers=fake_auth_header()) - - self.assertEquals(r.status_code, 202) - task = r.json() - wait_task(self._task_lookup, task['id'], 15) - uri = '/storagepools/default-pool/storagevolumes/%s' - resp = self.request(uri % 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_path = '/tmp/5m-file' - with open(vol_path, 'wb') as fd: - fd.seek(5*1024*1024-1) - fd.write("\0") - rollback.prependDefer(os.remove, vol_path) - - with open(vol_path, 'rb') as fd: - r = requests.post(url, - files={'file': fd}, - verify=False, - headers=fake_auth_header()) - - self.assertEquals(r.status_code, 413) - class HttpsRestTests(RestTests): """ -- 2.1.0
Now the upload process will be a sequence of POST and several PUT requests to the storage volume to avoid browser crashing. So remove the former way to upload a file to Kimchi server which used 'file' parameter to POST request. Signed-off-by: Aline Manera <alinefm@linux.vnet.ibm.com> --- docs/API.md | 3 +-- src/kimchi/mockmodel.py | 16 +++++----------- src/kimchi/model/storagevolumes.py | 36 ++---------------------------------- 3 files changed, 8 insertions(+), 47 deletions(-) diff --git a/docs/API.md b/docs/API.md index 88c5fec..71f2539 100644 --- a/docs/API.md +++ b/docs/API.md @@ -481,7 +481,7 @@ A interface represents available network interface on VM. in the defined Storage Pool * **POST**: Create a new Storage Volume in the Storage Pool The return resource is a task resource * See Resource: Task * - Only one of 'file', 'capacity', 'url' can be specified. + Only one of 'capacity', 'url' can be specified. * name: The name of the Storage Volume * capacity: The total space which can be used to store volumes The unit is bytes @@ -490,7 +490,6 @@ A interface represents available network interface on VM. * upload: True to start an upload process. False, otherwise. Only used when creating a storage volume 'capacity' parameter. * file: File to be uploaded, passed through form data - * url: URL to be downloaded ### Resource: Storage Volume diff --git a/src/kimchi/mockmodel.py b/src/kimchi/mockmodel.py index 4395883..b205608 100644 --- a/src/kimchi/mockmodel.py +++ b/src/kimchi/mockmodel.py @@ -282,21 +282,15 @@ class MockModel(Model): kimchi_log.info("The host system will be rebooted") def _mock_storagevolumes_create(self, pool, params): - vol_source = ['file', 'url', 'capacity'] + vol_source = ['url', 'capacity'] index_list = list(i for i in range(len(vol_source)) if vol_source[i] in params) create_param = vol_source[index_list[0]] name = params.get('name') - if name is None: - if create_param == 'file': - name = os.path.basename(params['file'].filename) - del params['file'] - params['capacity'] = 1024 - elif create_param == 'url': - name = os.path.basename(params['url']) - del params['url'] - params['capacity'] = 1024 - params['name'] = name + if name is None and create_param == 'url': + params['name'] = os.path.basename(params['url']) + del params['url'] + params['capacity'] = 1024 return self._model_storagevolumes_create(pool, params) diff --git a/src/kimchi/model/storagevolumes.py b/src/kimchi/model/storagevolumes.py index 8ca71a9..e92946e 100644 --- a/src/kimchi/model/storagevolumes.py +++ b/src/kimchi/model/storagevolumes.py @@ -58,7 +58,7 @@ class StorageVolumesModel(object): self.task = TaskModel(**kargs) def create(self, pool_name, params): - vol_source = ['file', 'url', 'capacity'] + vol_source = ['url', 'capacity'] name = params.get('name') @@ -89,9 +89,7 @@ class StorageVolumesModel(object): # 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': + if create_param == 'url': name = os.path.basename(params['url']) else: name = 'upload-%s' % int(time.time()) @@ -120,36 +118,6 @@ class StorageVolumesModel(object): taskid = add_task(targeturi, create_func, self.objstore, params) return self.task.lookup(taskid) - def _create_volume_with_file(self, cb, params): - pool_name = params.pop('pool') - dir_path = StoragePoolModel( - conn=self.conn, objstore=self.objstore).lookup(pool_name)['path'] - file_path = os.path.join(dir_path, params['name']) - if os.path.exists(file_path): - raise InvalidParameter('KCHVOL0001E', {'name': params['name']}) - - upload_file = params['file'] - f_len = upload_file.fp.length - try: - size = 0 - with open(file_path, 'wb') as f: - while True: - data = upload_file.file.read(READ_CHUNK_SIZE) - if not data: - break - size += len(data) - f.write(data) - cb('%s/%s' % (size, f_len)) - except Exception as e: - raise OperationFailed('KCHVOL0007E', - {'name': params['name'], - 'pool': pool_name, - 'err': e.message}) - - # Refresh to make sure volume can be found in following lookup - StoragePoolModel.get_storagepool(pool_name, self.conn).refresh(0) - cb('OK', True) - def _create_volume_with_capacity(self, cb, params): pool_name = params.pop('pool') vol_xml = """ -- 2.1.0
Reviewed-by: Crístian Deives <cristiandeives@gmail.com> On 15-05-2015 17:31, Aline Manera wrote:
V5 - V6: - Enclose offset information by the lock - Update test case to store file content in a variable to avoid IO
v4 -> v5: - Reduce number of locks - Update MockModel and test cases accordingly
Aline Manera (2): Upload storage volume Remove storage volume creation from file
Royce Lv (1): Update controller to make update accept formdata params
docs/API.md | 8 +++- src/kimchi/API.json | 22 ++++++++++ src/kimchi/control/base.py | 6 +-- src/kimchi/i18n.py | 6 +++ src/kimchi/mockmodel.py | 34 +++++++++------ src/kimchi/model/storagevolumes.py | 86 ++++++++++++++++++++----------------- tests/test_model_storagevolume.py | 88 +++++++++++++++++++++++++++++++------- tests/test_rest.py | 45 +------------------ 8 files changed, 180 insertions(+), 115 deletions(-)
participants (2)
-
Aline Manera -
Crístian Deives