From: Royce Lv <lvroyce(a)linux.vnet.ibm.com&gt; 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(a)linux.vnet.ibm.com&gt; --- 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

From: Royce Lv <lvroyce(a)linux.vnet.ibm.com&gt; Storage update abandoned async task because mem copy is not long lasting task and does not need to use task to track its status. So update just do the data upload, and following storage volume lookup will take care of current upload progress. Signed-off-by: Royce Lv <lvroyce(a)linux.vnet.ibm.com&gt; --- src/kimchi/i18n.py | 1 + src/kimchi/model/storagevolumes.py | 61 +++++++++++++++++--------------------- 2 files changed, 28 insertions(+), 34 deletions(-) diff --git a/src/kimchi/i18n.py b/src/kimchi/i18n.py index b712c16..13ac55b 100644 --- a/src/kimchi/i18n.py +++ b/src/kimchi/i18n.py @@ -216,6 +216,7 @@ messages = { "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": _("Upload volume chunk index, size and total size must be integer"), + "KCHVOL0025E": _("Upload volume fails. Details: %(err)s"), "KCHVOL0026E": _("Inconsistent upload count"), "KCHIFACE0001E": _("Interface %(name)s does not exist"), diff --git a/src/kimchi/model/storagevolumes.py b/src/kimchi/model/storagevolumes.py index f02efca..8121324 100644 --- a/src/kimchi/model/storagevolumes.py +++ b/src/kimchi/model/storagevolumes.py @@ -34,6 +34,7 @@ from kimchi.isoinfo import IsoImage from kimchi.model.diskutils import get_disk_ref_cnt from kimchi.model.storagepools import StoragePoolModel from kimchi.model.tasks import TaskModel +from kimchi.model.utils import get_vol_update_lock, release_vol_update_lock from kimchi.utils import add_task, get_next_clone_name, get_unique_file_name from kimchi.utils import kimchi_log from kimchi.xmlutils.utils import xpath_get_text @@ -58,7 +59,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 +90,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 +119,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 = """ @@ -489,6 +458,30 @@ class StorageVolumeModel(object): cb('OK', True) + def update(self, pool, name, params): + vol = StorageVolumeModel.get_storagevolume(pool, name, self.conn) + vol_path = vol.path() + + chunk_size = int(params['chunk_size']) + index = int(params['index']) + pos = chunk_size * index + lock = get_vol_update_lock(vol_path) + try: + with lock: + st = self.conn.get().newStream(0) + vol.upload(st, pos, chunk_size) + st.send(params['chunk'].fullvalue()) + st.finish() + except Exception as e: + st and st.abort() + raise OperationFailed('KCHVOL0025E', + {'err': e.message}) + finally: + release_vol_update_lock(vol_path) + + # Refresh to make sure volume can be found in following lookup + StoragePoolModel.get_storagepool(pool, self.conn).refresh(0) + class IsoVolumesModel(object): def __init__(self, **kargs): -- 2.1.0

From: Royce Lv <lvroyce(a)linux.vnet.ibm.com&gt; Signed-off-by: Royce Lv <lvroyce(a)linux.vnet.ibm.com&gt; --- tests/test_model_storagevolume.py | 95 +++++++++++++++++++++++++++++---------- 1 file changed, 72 insertions(+), 23 deletions(-) diff --git a/tests/test_model_storagevolume.py b/tests/test_model_storagevolume.py index a3c3ce3..6433a1f 100644 --- a/tests/test_model_storagevolume.py +++ b/tests/test_model_storagevolume.py @@ -20,17 +20,18 @@ import json import os -import requests +import tempfile import unittest +from cherrypy._cpreqbody import Entity from functools import partial -from kimchi.config import paths, READONLY_POOL_TYPE +from kimchi.config import READONLY_POOL_TYPE from kimchi.model.model import Model from kimchi.mockmodel import MockModel from kimchi.rollbackcontext import RollbackContext -from utils import fake_auth_header, get_free_port, patch_auth, request -from utils import rollback_wrapper, run_server, wait_task +from utils import get_free_port, patch_auth, request +from utils import rollback_wrapper, run_server, wait_task, running_as_root model = None @@ -149,25 +150,6 @@ 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()) - - if pool_info['type'] in READONLY_POOL_TYPE: - self.assertEquals(r.status_code, 400) - 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') - self.assertEquals(200, resp.status) - # Create storage volume with 'url' url = 'https://github.com/kimchi-project/kimchi/raw/master/COPYING' req = json.dumps({'url': url}) @@ -189,6 +171,73 @@ class StorageVolumeTests(unittest.TestCase): def setUp(self): self.request = partial(request, host, ssl_port) + @unittest.skipUnless(running_as_root(), 'Must be run as root') + def test_volume_upload(self): + class fake_header(object): + def elements(self, key): + return None + + def __getattr__(self, key): + try: + return self.__getattribute__(key) + except AttributeError: + return lambda x, y: None + + #inst = Model(objstore_loc=self.tmp_store) + vol_path = os.path.abspath('./run_tests.sh') + + def do_upload(tmp_file, vol_path, url): + index = 0 + chunk_size = 2 * 1000 + with open(vol_path, 'rb') as fd: + while True: + with open(tmp_file, 'wb') as tmp_fd: + fd.seek(index * chunk_size) + data = fd.read(chunk_size) + tmp_fd.write(data) + + # only file open for read can be handled by cherrypy + with open(tmp_file, 'rb') as tmp_fd: + # Hack cherrypy entity object and pass it to volume upload + entity = Entity(None, fake_header()) + entity.file = tmp_fd + param = {'index': str(index), + 'chunk_size': str(chunk_size), + 'chunk': entity} + model.storagevolume_update(pool, params['name'], param) + vol_info = model.storagevolume_lookup(pool, params['name']) + index = index + 1 + if len(data) < chunk_size: + return vol_info + + # Create a volume with raw format first, following upload will override it. + params = {'capacity': os.path.getsize(vol_path), + 'format': 'raw', + 'name': os.path.basename(vol_path)} + pool = 'default' + + with RollbackContext() as rollback: + task_response = model.storagevolumes_create(pool, params) + rollback.prependDefer(model.storagevolume_delete, pool, + params['name']) + taskid = task_response['id'] + vol_uri = task_response['target_uri'] + model.task_wait(taskid) + self.assertEquals('finished', model.task_lookup(taskid)['status']) + + f = tempfile.NamedTemporaryFile(delete=False) + rollback.prependDefer(os.remove, f.name) + resp = do_upload(f.name, vol_path, vol_uri) + + with open(vol_path) as vol_file: + vol_content = vol_file.read() + + with open(resp['path']) as copy_file: + cp_content = copy_file.read() + + self.assertEquals(vol_content, cp_content) + + def test_get_storagevolume(self): uri = '/storagepools/default/storagevolumes' resp = self.request(uri) -- 2.1.0

Reviewed-by: Aline Manera <alinefm(a)linux.vnet.ibm.com&gt; On 22/04/2015 05:02, lvroyce(a)linux.vnet.ibm.com wrote:

On 22/04/2015 05:02, lvroyce(a)linux.vnet.ibm.com wrote: It should be joined with the first patch.

On 22/04/2015 05:02, lvroyce(a)linux.vnet.ibm.com wrote: This should be added to the first patch of this series. For what is this for? You can remove this comment. We have agreed to always do the tests using the REST API instead of the model instances. So please, change it to request the data using the REST API Same I commented above related to use the REST API instead of the model instance.