[PATCHv4 0/7] Support image upload
From: Royce Lv <lvroyce@linux.vnet.ibm.com> v2>v3, Add mockmodel and tests v3>v4, Update progress presentation, update max_body_size control Royce Lv (7): Storage volume upload: Update API.md Fix mockmodel reset for objectstore Storage volume upload: Parse params for upload formdata Storage volume upload: Control request body size of kimchi Storage volume upload: Support file based upload Storage volume upload: Adding progress to task message Storage volume upload: Change mockmodel and test docs/API.md | 2 ++ src/kimchi.conf.in | 3 +++ src/kimchi/config.py.in | 1 + src/kimchi/control/utils.py | 4 +++- src/kimchi/mockmodel.py | 26 +++++++++++++++++++++-- src/kimchi/model/storagevolumes.py | 32 ++++++++++++++++++++++++++++ src/kimchi/server.py | 1 + src/kimchid.in | 1 + tests/test_rest.py | 43 +++++++++++++++++++++++++++++++++++++- tests/utils.py | 2 +- 10 files changed, 110 insertions(+), 5 deletions(-) -- 1.8.3.2
From: Royce Lv <lvroyce@linux.vnet.ibm.com> Update API.md adding upload option to storagevolume creation. Signed-off-by: Royce Lv <lvroyce@linux.vnet.ibm.com> --- docs/API.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/docs/API.md b/docs/API.md index 298441f..ca64af6 100644 --- a/docs/API.md +++ b/docs/API.md @@ -413,11 +413,13 @@ 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. * name: The name of the Storage Volume * type: The type of the defined Storage Volume * capacity: The total space which can be used to store volumes The unit is MBytes * format: The format of the defined Storage Volume + * file: File to be uploaded, passed through form data ### Resource: Storage Volume -- 1.8.3.2
On 04-09-2014 06:25, lvroyce@linux.vnet.ibm.com wrote:
From: Royce Lv <lvroyce@linux.vnet.ibm.com>
Update API.md adding upload option to storagevolume creation.
Signed-off-by: Royce Lv <lvroyce@linux.vnet.ibm.com>
You should also update the file "src/kimchi/API.json" and add the new parameter "file". I just sent a new revision of my patch with this change as well. I discussed with Aline today about this, and it seems your case is a little different than mine: what should be the type of the parameter "file"? AFAIU, that parameter sends the file's binary data (not its name or its path) so we can't use "string". If you can find the correct data type for that (e.g. something like blob), please make this change.
From: Royce Lv <lvroyce@linux.vnet.ibm.com> In mockmodel reset, we only reset model fields dict but objectstore remains unchanged, tasks retained in objecstore making mockmodel task related tests fail. Reset objectstore to fix this. Signed-off-by: Royce Lv <lvroyce@linux.vnet.ibm.com> --- src/kimchi/mockmodel.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/kimchi/mockmodel.py b/src/kimchi/mockmodel.py index 05e3fa4..d7a21ed 100644 --- a/src/kimchi/mockmodel.py +++ b/src/kimchi/mockmodel.py @@ -64,6 +64,7 @@ class MockModel(object): def __init__(self, objstore_loc=None): self.reset() self.objstore = ObjectStore(objstore_loc) + self.objstore_loc = objstore_loc self.distros = self._get_distros() def capabilities_lookup(self, *ident): @@ -78,6 +79,8 @@ class MockModel(object): 'federation': 'off'} def reset(self): + if hasattr(self, 'objstore'): + self.objstore = ObjectStore(self.objstore_loc) self._mock_vms = {} self._mock_screenshots = {} self._mock_templates = {} -- 1.8.3.2
From: Royce Lv <lvroyce@linux.vnet.ibm.com> When pass form to cherrypy, it is stored in cherrypy.request.params, so adjust parse_request for this change. Also avoid body parse so that file content will not be loaded into mem. Signed-off-by: Royce Lv <lvroyce@linux.vnet.ibm.com> --- src/kimchi/control/utils.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/kimchi/control/utils.py b/src/kimchi/control/utils.py index fbd5177..64e0ac5 100644 --- a/src/kimchi/control/utils.py +++ b/src/kimchi/control/utils.py @@ -72,14 +72,16 @@ def mime_in_header(header, mime): def parse_request(): if 'Content-Length' not in cherrypy.request.headers: return {} - rawbody = cherrypy.request.body.read() if mime_in_header('Content-Type', 'application/json'): + rawbody = cherrypy.request.body.read() try: return json.loads(rawbody) except ValueError: e = OperationFailed('KCHAPI0006E') raise cherrypy.HTTPError(400, e.message) + elif mime_in_header('Content-Type', 'multipart/form-data'): + return cherrypy.request.params else: e = OperationFailed('KCHAPI0007E') raise cherrypy.HTTPError(415, e.message) -- 1.8.3.2
From: Royce Lv <lvroyce@linux.vnet.ibm.com> This patch control request body size to be 4G, when request size exceed, kimchi will response with 413. Signed-off-by: Royce Lv <lvroyce@linux.vnet.ibm.com> --- src/kimchi.conf.in | 3 +++ src/kimchi/config.py.in | 1 + src/kimchi/server.py | 1 + src/kimchid.in | 1 + 4 files changed, 6 insertions(+) diff --git a/src/kimchi.conf.in b/src/kimchi.conf.in index 2be1a0e..ea39292 100644 --- a/src/kimchi.conf.in +++ b/src/kimchi.conf.in @@ -30,6 +30,9 @@ # at the same network #federation = off +# Max request body size in KB, default value is 4GB +#max_body_size = 4 * 1024 * 1024 + [logging] # Log directory #log_dir = @localstatedir@/log/kimchi diff --git a/src/kimchi/config.py.in b/src/kimchi/config.py.in index d403827..48b43d7 100644 --- a/src/kimchi/config.py.in +++ b/src/kimchi/config.py.in @@ -255,6 +255,7 @@ def _get_config(): config.set("server", "ssl_key", "") config.set("server", "environment", "production") config.set("server", "federation", "off") + config.set("server", "max_body_size", "4*1024*1024*1024") config.add_section("logging") config.set("logging", "log_dir", paths.log_dir) config.set("logging", "log_level", DEFAULT_LOG_LEVEL) diff --git a/src/kimchi/server.py b/src/kimchi/server.py index 10f5dff..1c3b360 100644 --- a/src/kimchi/server.py +++ b/src/kimchi/server.py @@ -87,6 +87,7 @@ class Server(object): # directly. You must go through the proxy. cherrypy.server.socket_host = '127.0.0.1' cherrypy.server.socket_port = options.cherrypy_port + cherrypy.server.max_request_body_size = eval(options.max_body_size) cherrypy.log.screen = True cherrypy.log.access_file = options.access_log diff --git a/src/kimchid.in b/src/kimchid.in index 3ed087f..6e2d9b7 100644 --- a/src/kimchid.in +++ b/src/kimchid.in @@ -86,6 +86,7 @@ def main(options): # Add non-option arguments setattr(options, 'ssl_cert', config.config.get('server', 'ssl_cert')) setattr(options, 'ssl_key', config.config.get('server', 'ssl_key')) + setattr(options, 'max_body_size', config.config.get('server', 'max_body_size')*1024) kimchi.server.main(options) -- 1.8.3.2
On 09/04/2014 06:25 AM, lvroyce@linux.vnet.ibm.com wrote:
From: Royce Lv <lvroyce@linux.vnet.ibm.com>
This patch control request body size to be 4G, when request size exceed, kimchi will response with 413.
Signed-off-by: Royce Lv <lvroyce@linux.vnet.ibm.com> --- src/kimchi.conf.in | 3 +++ src/kimchi/config.py.in | 1 + src/kimchi/server.py | 1 + src/kimchid.in | 1 + 4 files changed, 6 insertions(+)
diff --git a/src/kimchi.conf.in b/src/kimchi.conf.in index 2be1a0e..ea39292 100644 --- a/src/kimchi.conf.in +++ b/src/kimchi.conf.in @@ -30,6 +30,9 @@ # at the same network #federation = off
+# Max request body size in KB, default value is 4GB +#max_body_size = 4 * 1024 * 1024 + [logging] # Log directory #log_dir = @localstatedir@/log/kimchi diff --git a/src/kimchi/config.py.in b/src/kimchi/config.py.in index d403827..48b43d7 100644 --- a/src/kimchi/config.py.in +++ b/src/kimchi/config.py.in @@ -255,6 +255,7 @@ def _get_config(): config.set("server", "ssl_key", "") config.set("server", "environment", "production") config.set("server", "federation", "off")
+ config.set("server", "max_body_size", "4*1024*1024*1024")
You should use the same value from config file, ie, 4 * 1024 * 1024. Otherwise, if user remove the "max_body_value" from config file you will use this value as default and then multiply by 1024 again which would result in a big number.
config.add_section("logging") config.set("logging", "log_dir", paths.log_dir) config.set("logging", "log_level", DEFAULT_LOG_LEVEL) diff --git a/src/kimchi/server.py b/src/kimchi/server.py index 10f5dff..1c3b360 100644 --- a/src/kimchi/server.py +++ b/src/kimchi/server.py @@ -87,6 +87,7 @@ class Server(object): # directly. You must go through the proxy. cherrypy.server.socket_host = '127.0.0.1' cherrypy.server.socket_port = options.cherrypy_port + cherrypy.server.max_request_body_size = eval(options.max_body_size)
cherrypy.log.screen = True cherrypy.log.access_file = options.access_log diff --git a/src/kimchid.in b/src/kimchid.in index 3ed087f..6e2d9b7 100644 --- a/src/kimchid.in +++ b/src/kimchid.in @@ -86,6 +86,7 @@ def main(options): # Add non-option arguments setattr(options, 'ssl_cert', config.config.get('server', 'ssl_cert')) setattr(options, 'ssl_key', config.config.get('server', 'ssl_key')) + setattr(options, 'max_body_size', config.config.get('server', 'max_body_size')*1024)
kimchi.server.main(options)
Please, run "make check-local" to fix formatting issues with this patch. On 04-09-2014 06:25, lvroyce@linux.vnet.ibm.com wrote:
From: Royce Lv <lvroyce@linux.vnet.ibm.com>
This patch control request body size to be 4G, when request size exceed, kimchi will response with 413.
Signed-off-by: Royce Lv <lvroyce@linux.vnet.ibm.com>
From: Royce Lv <lvroyce@linux.vnet.ibm.com> Start a task to upload storage volume. Signed-off-by: Royce Lv <lvroyce@linux.vnet.ibm.com> --- src/kimchi/model/storagevolumes.py | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/src/kimchi/model/storagevolumes.py b/src/kimchi/model/storagevolumes.py index 6b001f7..91abc81 100644 --- a/src/kimchi/model/storagevolumes.py +++ b/src/kimchi/model/storagevolumes.py @@ -64,6 +64,34 @@ class StorageVolumesModel(object): taskid = add_task('', 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'] + try: + f = open(file_path, 'wb') + while True: + data = upload_file.file.read(8192) + if not data: + break + f.write(data) + f.close() + 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 + pool = StoragePoolModel.get_storagepool(pool_name, self.conn) + pool.refresh() + cb('', True) + def _create_volume_with_capacity(self, cb, params): pool_name = params.pop('pool') vol_xml = """ -- 1.8.3.2
On 04-09-2014 06:25, lvroyce@linux.vnet.ibm.com wrote:
+ f = open(file_path, 'wb') + while True: + data = upload_file.file.read(8192) + if not data: + break + f.write(data) + f.close()
The code above may leak the file "f" if an exception is raised while reading its contents. You must make sure the method "close" will be executed. I suggest the following pattern: with open(file_path, 'wb') as f: # handle file f When the "with" block ends, the file be closed whether an exception was raised or not. You don't need to call "close" explicitly.
From: Royce Lv <lvroyce@linux.vnet.ibm.com> Periodically update current written bytes to task message so that UI can query the latest progress of volume uploading. Signed-off-by: Royce Lv <lvroyce@linux.vnet.ibm.com> --- src/kimchi/model/storagevolumes.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/kimchi/model/storagevolumes.py b/src/kimchi/model/storagevolumes.py index 91abc81..a6c7c9f 100644 --- a/src/kimchi/model/storagevolumes.py +++ b/src/kimchi/model/storagevolumes.py @@ -73,13 +73,19 @@ class StorageVolumesModel(object): raise InvalidParameter('KCHVOL0001E', {'name': params['name']}) upload_file = params['file'] + f_len = upload_file.fp.length + pool = StoragePoolModel.get_storagepool(pool_name, self.conn) try: + size = 0 f = open(file_path, 'wb') + pool.refresh() while True: data = upload_file.file.read(8192) if not data: break + size += len(data) f.write(data) + cb('%s/%s' % (f_len, size), True) f.close() except Exception as e: raise OperationFailed('KCHVOL0007E', @@ -88,9 +94,7 @@ class StorageVolumesModel(object): 'err': e.message}) # Refresh to make sure volume can be found in following lookup - pool = StoragePoolModel.get_storagepool(pool_name, self.conn) pool.refresh() - cb('', True) def _create_volume_with_capacity(self, cb, params): pool_name = params.pop('pool') -- 1.8.3.2
On 09/04/2014 06:25 AM, lvroyce@linux.vnet.ibm.com wrote:
From: Royce Lv <lvroyce@linux.vnet.ibm.com>
Periodically update current written bytes to task message so that UI can query the latest progress of volume uploading.
Signed-off-by: Royce Lv <lvroyce@linux.vnet.ibm.com> --- src/kimchi/model/storagevolumes.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/src/kimchi/model/storagevolumes.py b/src/kimchi/model/storagevolumes.py index 91abc81..a6c7c9f 100644 --- a/src/kimchi/model/storagevolumes.py +++ b/src/kimchi/model/storagevolumes.py @@ -73,13 +73,19 @@ class StorageVolumesModel(object): raise InvalidParameter('KCHVOL0001E', {'name': params['name']})
upload_file = params['file'] + f_len = upload_file.fp.length + pool = StoragePoolModel.get_storagepool(pool_name, self.conn) try: + size = 0 f = open(file_path, 'wb') + pool.refresh() while True: data = upload_file.file.read(8192) if not data: break + size += len(data) f.write(data)
+ cb('%s/%s' % (f_len, size), True)
It should be (size, f_len), right? =) As size represents the partial size and f_len the total file size
f.close() except Exception as e: raise OperationFailed('KCHVOL0007E', @@ -88,9 +94,7 @@ class StorageVolumesModel(object): 'err': e.message})
# Refresh to make sure volume can be found in following lookup - pool = StoragePoolModel.get_storagepool(pool_name, self.conn) pool.refresh() - cb('', True)
def _create_volume_with_capacity(self, cb, params): pool_name = params.pop('pool')
On 04-09-2014 06:25, lvroyce@linux.vnet.ibm.com wrote:
try: + size = 0 f = open(file_path, 'wb') + pool.refresh() while True: data = upload_file.file.read(8192) if not data: break + size += len(data) f.write(data) + cb('%s/%s' % (f_len, size), True) f.close()
Is there any specific reason the method "pool.refresh()" is called from inside the loop? I mean, is it necessary for the upload operation / partial data retrieval? If not, I'd suggest to keep that command outside the loop (as it is now). That loop will run for many iterations (e.g. ~ 90,000 for a 700 MB ISO file), so the less we execute inside that loop, the better.
From: Royce Lv <lvroyce@linux.vnet.ibm.com> Change mockmodel and tests to support storagevolume upload. Signed-off-by: Royce Lv <lvroyce@linux.vnet.ibm.com> --- src/kimchi/mockmodel.py | 23 +++++++++++++++++++++-- tests/test_rest.py | 43 ++++++++++++++++++++++++++++++++++++++++++- tests/utils.py | 2 +- 3 files changed, 64 insertions(+), 4 deletions(-) diff --git a/src/kimchi/mockmodel.py b/src/kimchi/mockmodel.py index d7a21ed..a4f41da 100644 --- a/src/kimchi/mockmodel.py +++ b/src/kimchi/mockmodel.py @@ -482,7 +482,6 @@ class MockModel(object): def storagevolumes_create(self, pool_name, params): vol_source = ['file', 'url', 'capacity'] - index_list = list(i for i in range(len(vol_source)) if vol_source[i] in params) if len(index_list) != 1: @@ -499,6 +498,27 @@ class MockModel(object): taskid = self.add_task('', create_func, params) return self.task_lookup(taskid) + def _create_volume_with_file(self, cb, params): + upload_file = params['file'] + params['name'] = params['name'] + params['format'] = 'raw' + params['capacity'] = upload_file.fp.length + size = 0 + try: + while True: + data = upload_file.file.read(8192) + if not data: + break + size += len(data) + cb('%s/%s' % (size, params['capacity']), True) + except Exception as e: + raise OperationFailed('KCHVOL0007E', + {'name': params['name'], + 'pool': params['pool'], + 'err': e.message}) + self._create_volume_with_capacity(cb, params) + cb('%s/%s' % (size, params['capacity']), True) + def _create_volume_with_capacity(self, cb, params): pool_name = params.pop('pool') pool = self._get_storagepool(pool_name) @@ -506,7 +526,6 @@ class MockModel(object): raise InvalidOperation("KCHVOL0003E", {'pool': pool_name, 'volume': params['name']}) - try: name = params['name'] volume = MockStorageVolume(pool, name, params) diff --git a/tests/test_rest.py b/tests/test_rest.py index 9e14b6d..abc519f 100644 --- a/tests/test_rest.py +++ b/tests/test_rest.py @@ -22,6 +22,7 @@ import base64 import json import os import random +import requests import shutil import time import unittest @@ -1041,12 +1042,20 @@ class RestTests(unittest.TestCase): resp = self.request('/storagepools/pool-2/storagevolumes/', req, 'POST') self.assertEquals(202, resp.status) + task_id = json.loads(resp.read())['id'] + self._wait_task(task_id) + status = json.loads(self.request('/tasks/%s' % task_id).read()) + self.assertEquals('failed', status['status']) + resp = self.request('/storagepools/pool-2/activate', '{}', 'POST') self.assertEquals(200, resp.status) resp = self.request('/storagepools/pool-2/storagevolumes/', req, 'POST') self.assertEquals(202, resp.status) - time.sleep(1) + task_id = json.loads(resp.read())['id'] + self._wait_task(task_id) + status = json.loads(self.request('/tasks/%s' % task_id).read()) + self.assertEquals('finished', status['status']) # Verify the storage volume resp = self.request('/storagepools/pool-2/storagevolumes/test-volume') @@ -1872,6 +1881,38 @@ class RestTests(unittest.TestCase): resp = self.request('%s/fedora-fake' % base_uri, '{}', 'DELETE') self.assertEquals(204, resp.status) + def test_upload(self): + # If we use self.request, we may encode multipart formdata by ourselves + # requests lib take care of encode part, so use this lib instead + def fake_auth_header(): + headers = {'Accept': 'application/json'} + user, pw = kimchi.mockmodel.fake_user.items()[0] + hdr = "Basic " + base64.b64encode("%s:%s" % (user, pw)) + headers['AUTHORIZATION'] = hdr + return headers + + with RollbackContext() as rollback: + upload_path = '/tmp/vol' + f = open(upload_path, 'w') + f.write('abcd') + f.close() + rollback.prependDefer(os.remove, upload_path) + + vol = open(upload_path, 'rb') + url = "https://%s:%s/storagepools/default/storagevolumes" %\ + (host, ssl_port) + + r = requests.post(url, + files={'file': vol}, + data={'name': 'new_vol2'}, + verify=False, + headers=fake_auth_header()) + self.assertEquals(r.status_code, 202) + time.sleep(1) + vol_list = json.loads( + self.request('/storagepools/default/storagevolumes').read()) + self.assertEquals('new_vol2', vol_list[0]['name']) + class HttpsRestTests(RestTests): """ diff --git a/tests/utils.py b/tests/utils.py index 7fccae1..140bb1d 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -104,7 +104,7 @@ def run_server(host, port, ssl_port, test_mode, cherrypy_port=None, args = type('_', (object,), {'host': host, 'port': port, 'ssl_port': ssl_port, - 'cherrypy_port': cherrypy_port, + 'cherrypy_port': cherrypy_port, 'max_body_size': '4*1024', 'ssl_cert': '', 'ssl_key': '', 'test': test_mode, 'access_log': '/dev/null', 'error_log': '/dev/null', 'environment': environment, -- 1.8.3.2
This patch breaks the tests. Here is the error output: ====================================================================== ERROR: test_rest (unittest.loader.ModuleImportFailure) ---------------------------------------------------------------------- ImportError: Failed to import test module: test_rest Traceback (most recent call last): File "/usr/lib64/python2.7/unittest/loader.py", line 252, in _find_tests module = self._get_module_from_name(name) File "/usr/lib64/python2.7/unittest/loader.py", line 230, in _get_module_from_name __import__(name) File "/home/vianac/LTC/kimchi/tests/test_rest.py", line 25, in <module> import requests ImportError: No module named requests Please, run "sudo make check" to check for test errors before submitting patches. On 04-09-2014 06:25, lvroyce@linux.vnet.ibm.com wrote:
From: Royce Lv <lvroyce@linux.vnet.ibm.com>
Change mockmodel and tests to support storagevolume upload.
Signed-off-by: Royce Lv <lvroyce@linux.vnet.ibm.com>
On 04-09-2014 12:02, Crístian Viana wrote:
This patch breaks the tests. Here is the error output:
====================================================================== ERROR: test_rest (unittest.loader.ModuleImportFailure) ---------------------------------------------------------------------- ImportError: Failed to import test module: test_rest Traceback (most recent call last): File "/usr/lib64/python2.7/unittest/loader.py", line 252, in _find_tests module = self._get_module_from_name(name) File "/usr/lib64/python2.7/unittest/loader.py", line 230, in _get_module_from_name __import__(name) File "/home/vianac/LTC/kimchi/tests/test_rest.py", line 25, in <module> import requests ImportError: No module named requests
Please, run "sudo make check" to check for test errors before submitting patches.
BTW, I'm running Kimchi on Fedora 20.
On 09/04/2014 12:02 PM, Crístian Viana wrote:
This patch breaks the tests. Here is the error output:
====================================================================== ERROR: test_rest (unittest.loader.ModuleImportFailure) ---------------------------------------------------------------------- ImportError: Failed to import test module: test_rest Traceback (most recent call last): File "/usr/lib64/python2.7/unittest/loader.py", line 252, in _find_tests module = self._get_module_from_name(name) File "/usr/lib64/python2.7/unittest/loader.py", line 230, in _get_module_from_name __import__(name) File "/home/vianac/LTC/kimchi/tests/test_rest.py", line 25, in <module> import requests ImportError: No module named requests
Please, run "sudo make check" to check for test errors before submitting patches.
I got the same error on Fedora 20. We need to install python-requests on Fedora 20, openSUSE 13.1 and RHEL7 to get this module.
On 04-09-2014 06:25, lvroyce@linux.vnet.ibm.com wrote:
From: Royce Lv <lvroyce@linux.vnet.ibm.com>
Change mockmodel and tests to support storagevolume upload.
Signed-off-by: Royce Lv <lvroyce@linux.vnet.ibm.com>
_______________________________________________ Kimchi-devel mailing list Kimchi-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/kimchi-devel
participants (3)
-
Aline Manera -
Crístian Viana -
lvroyce＠linux.vnet.ibm.com