[PATCHv3 0/7] Support volume upload
From: Royce Lv <lvroyce@linux.vnet.ibm.com> v2>v3, Add mockmodel and testcases. 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 | 25 +++++++++++++++++++++-- src/kimchi/model/storagevolumes.py | 31 ++++++++++++++++++++++++++++ src/kimchi/server.py | 1 + src/kimchid.in | 3 +++ tests/test_rest.py | 41 +++++++++++++++++++++++++++++++++++++- tests/utils.py | 2 +- 10 files changed, 108 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
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 | 3 +++ 4 files changed, 8 insertions(+) diff --git a/src/kimchi.conf.in b/src/kimchi.conf.in index 2be1a0e..82baa0c 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 bytes +#max_body_size = 4 * 1024 * 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..08a9be7 100644 --- a/src/kimchid.in +++ b/src/kimchid.in @@ -47,6 +47,7 @@ def main(options): cherrypy_port = config.config.get("server", "cherrypy_port") runningEnv = config.config.get("server", "environment") federation = config.config.get("server", "federation") + max_body_size = config.config.get("server", "max_body_size") logDir = config.config.get("logging", "log_dir") logLevel = config.config.get("logging", "log_level") @@ -72,6 +73,8 @@ def main(options): parser.add_option('--federation', default=federation, help="Register and discover Kimchi peers at the same " "network using openSLP") + parser.add_option('--max_body_size', default=max_body_size, + help="Max request body size accept by kimchi") parser.add_option('--test', action='store_true', help="Run server in mock model") (options, args) = parser.parse_args() -- 1.8.3.2
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
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 | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/kimchi/model/storagevolumes.py b/src/kimchi/model/storagevolumes.py index 91abc81..7794b32 100644 --- a/src/kimchi/model/storagevolumes.py +++ b/src/kimchi/model/storagevolumes.py @@ -73,13 +73,18 @@ class StorageVolumesModel(object): raise InvalidParameter('KCHVOL0001E', {'name': params['name']}) upload_file = params['file'] + 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(size, True) f.close() except Exception as e: raise OperationFailed('KCHVOL0007E', @@ -88,9 +93,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
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 | 22 ++++++++++++++++++++-- tests/test_rest.py | 41 ++++++++++++++++++++++++++++++++++++++++- tests/utils.py | 2 +- 3 files changed, 61 insertions(+), 4 deletions(-) diff --git a/src/kimchi/mockmodel.py b/src/kimchi/mockmodel.py index d7a21ed..d416b90 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,26 @@ 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' + try: + size = 0 + while True: + data = upload_file.file.read(8192) + if not data: + break + size += len(data) + except Exception as e: + raise OperationFailed('KCHVOL0007E', + {'name': params['name'], + 'pool': params['pool'], + 'err': e.message}) + params['capacity'] = size + self._create_volume_with_capacity(cb, params) + cb(size, True) + def _create_volume_with_capacity(self, cb, params): pool_name = params.pop('pool') pool = self._get_storagepool(pool_name) @@ -506,7 +525,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..c67a23c 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,36 @@ class RestTests(unittest.TestCase): resp = self.request('%s/fedora-fake' % base_uri, '{}', 'DELETE') self.assertEquals(204, resp.status) + def test_upload(self): + 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
On 09/03/2014 08:58 AM, lvroyce0210@gmail.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 | 3 +++ 4 files changed, 8 insertions(+)
diff --git a/src/kimchi.conf.in b/src/kimchi.conf.in index 2be1a0e..82baa0c 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 bytes +#max_body_size = 4 * 1024 * 1024 * 1024 +
What about use MB or GB and do the conversion to bytes internally? It will be easier to user just specify a number. # Max request body size in GB #max_body_size = 4
[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..08a9be7 100644 --- a/src/kimchid.in +++ b/src/kimchid.in @@ -47,6 +47,7 @@ def main(options): cherrypy_port = config.config.get("server", "cherrypy_port") runningEnv = config.config.get("server", "environment") federation = config.config.get("server", "federation") + max_body_size = config.config.get("server", "max_body_size") logDir = config.config.get("logging", "log_dir") logLevel = config.config.get("logging", "log_level")
@@ -72,6 +73,8 @@ def main(options): parser.add_option('--federation', default=federation, help="Register and discover Kimchi peers at the same " "network using openSLP")
+ parser.add_option('--max_body_size', default=max_body_size, + help="Max request body size accept by kimchi")
I think we don't need to allow it on command line. Only the config file should be enough in this case.
parser.add_option('--test', action='store_true', help="Run server in mock model") (options, args) = parser.parse_args()
On 09/03/2014 08:58 AM, lvroyce0210@gmail.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> --- src/kimchi/mockmodel.py | 22 ++++++++++++++++++++-- tests/test_rest.py | 41 ++++++++++++++++++++++++++++++++++++++++- tests/utils.py | 2 +- 3 files changed, 61 insertions(+), 4 deletions(-)
diff --git a/src/kimchi/mockmodel.py b/src/kimchi/mockmodel.py index d7a21ed..d416b90 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,26 @@ 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'
+ try: + size = 0 + while True: + data = upload_file.file.read(8192) + if not data: + break + size += len(data)
From what I understood, you just want the size file to create an empty volume to it, right? So do you need really to read the file to get its length? Isn't there a function upload_file.file.length or something like that?
+ except Exception as e: + raise OperationFailed('KCHVOL0007E', + {'name': params['name'], + 'pool': params['pool'], + 'err': e.message}) + params['capacity'] = size + self._create_volume_with_capacity(cb, params) + cb(size, True) + def _create_volume_with_capacity(self, cb, params): pool_name = params.pop('pool') pool = self._get_storagepool(pool_name) @@ -506,7 +525,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..c67a23c 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,36 @@ class RestTests(unittest.TestCase): resp = self.request('%s/fedora-fake' % base_uri, '{}', 'DELETE') self.assertEquals(204, resp.status)
+ def test_upload(self): + 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())
Why can't you use: self.request("/storagepools/default/storagevolumes", "POST", {"name": 'new_vol2', 'file': vol}) ?
+ 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,
On 09/03/2014 08:58 AM, lvroyce0210@gmail.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 | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/src/kimchi/model/storagevolumes.py b/src/kimchi/model/storagevolumes.py index 91abc81..7794b32 100644 --- a/src/kimchi/model/storagevolumes.py +++ b/src/kimchi/model/storagevolumes.py @@ -73,13 +73,18 @@ class StorageVolumesModel(object): raise InvalidParameter('KCHVOL0001E', {'name': params['name']})
upload_file = params['file'] + 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(size, True)
Just to make download/upload consistent. In download patches, Cristian is recording <downloaded_size>/<remote_size> Is it possible to get the total file size prior to the upload starts? So we can record in the same way as Cristian did
f.close() except Exception as e: raise OperationFailed('KCHVOL0007E', @@ -88,9 +93,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 2014年09月04日 09:24, Aline Manera wrote:
On 09/03/2014 08:58 AM, lvroyce0210@gmail.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 | 3 +++ 4 files changed, 8 insertions(+)
diff --git a/src/kimchi.conf.in b/src/kimchi.conf.in index 2be1a0e..82baa0c 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 bytes +#max_body_size = 4 * 1024 * 1024 * 1024 +
What about use MB or GB and do the conversion to bytes internally? It will be easier to user just specify a number.
# Max request body size in GB #max_body_size = 4
ACK
[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..08a9be7 100644 --- a/src/kimchid.in +++ b/src/kimchid.in @@ -47,6 +47,7 @@ def main(options): cherrypy_port = config.config.get("server", "cherrypy_port") runningEnv = config.config.get("server", "environment") federation = config.config.get("server", "federation") + max_body_size = config.config.get("server", "max_body_size") logDir = config.config.get("logging", "log_dir") logLevel = config.config.get("logging", "log_level")
@@ -72,6 +73,8 @@ def main(options): parser.add_option('--federation', default=federation, help="Register and discover Kimchi peers at the same " "network using openSLP")
+ parser.add_option('--max_body_size', default=max_body_size, + help="Max request body size accept by kimchi")
I think we don't need to allow it on command line. Only the config file should be enough in this case.
ACK
parser.add_option('--test', action='store_true', help="Run server in mock model") (options, args) = parser.parse_args()
_______________________________________________ Kimchi-devel mailing list Kimchi-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/kimchi-devel
On 2014年09月04日 09:42, Aline Manera wrote:
On 09/03/2014 08:58 AM, lvroyce0210@gmail.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 | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/src/kimchi/model/storagevolumes.py b/src/kimchi/model/storagevolumes.py index 91abc81..7794b32 100644 --- a/src/kimchi/model/storagevolumes.py +++ b/src/kimchi/model/storagevolumes.py @@ -73,13 +73,18 @@ class StorageVolumesModel(object): raise InvalidParameter('KCHVOL0001E', {'name': params['name']})
upload_file = params['file'] + 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(size, True)
Just to make download/upload consistent. In download patches, Cristian is recording <downloaded_size>/<remote_size> Is it possible to get the total file size prior to the upload starts? So we can record in the same way as Cristian did
ack
f.close() except Exception as e: raise OperationFailed('KCHVOL0007E', @@ -88,9 +93,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')
_______________________________________________ Kimchi-devel mailing list Kimchi-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/kimchi-devel
On 2014年09月04日 09:32, Aline Manera wrote:
On 09/03/2014 08:58 AM, lvroyce0210@gmail.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> --- src/kimchi/mockmodel.py | 22 ++++++++++++++++++++-- tests/test_rest.py | 41 ++++++++++++++++++++++++++++++++++++++++- tests/utils.py | 2 +- 3 files changed, 61 insertions(+), 4 deletions(-)
diff --git a/src/kimchi/mockmodel.py b/src/kimchi/mockmodel.py index d7a21ed..d416b90 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,26 @@ 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'
+ try: + size = 0 + while True: + data = upload_file.file.read(8192) + if not data: + break + size += len(data)
From what I understood, you just want the size file to create an empty volume to it, right? So do you need really to read the file to get its length? Isn't there a function upload_file.file.length or something like that?
ack, found that.
+ except Exception as e: + raise OperationFailed('KCHVOL0007E', + {'name': params['name'], + 'pool': params['pool'], + 'err': e.message}) + params['capacity'] = size + self._create_volume_with_capacity(cb, params) + cb(size, True) + def _create_volume_with_capacity(self, cb, params): pool_name = params.pop('pool') pool = self._get_storagepool(pool_name) @@ -506,7 +525,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..c67a23c 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,36 @@ class RestTests(unittest.TestCase): resp = self.request('%s/fedora-fake' % base_uri, '{}', 'DELETE') self.assertEquals(204, resp.status)
+ def test_upload(self): + 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())
Why can't you use:
self.request("/storagepools/default/storagevolumes", "POST", {"name": 'new_vol2', 'file': vol})
?
If we are using httplib, we'll need to encode multipart for ourselves, lib requests take care of this part, so leverage this lib instead.
+ 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,
_______________________________________________ Kimchi-devel mailing list Kimchi-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/kimchi-devel
On 2014年09月04日 09:42, Aline Manera wrote:
On 09/03/2014 08:58 AM, lvroyce0210@gmail.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 | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/src/kimchi/model/storagevolumes.py b/src/kimchi/model/storagevolumes.py index 91abc81..7794b32 100644 --- a/src/kimchi/model/storagevolumes.py +++ b/src/kimchi/model/storagevolumes.py @@ -73,13 +73,18 @@ class StorageVolumesModel(object): raise InvalidParameter('KCHVOL0001E', {'name': params['name']})
upload_file = params['file'] + 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(size, True)
Just to make download/upload consistent. In download patches, Cristian is recording <downloaded_size>/<remote_size> Is it possible to get the total file size prior to the upload starts? So we can record in the same way as Cristian did
ACK
f.close() except Exception as e: raise OperationFailed('KCHVOL0007E', @@ -88,9 +93,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')
_______________________________________________ Kimchi-devel mailing list Kimchi-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/kimchi-devel
On 2014年09月04日 09:24, Aline Manera wrote:
On 09/03/2014 08:58 AM, lvroyce0210@gmail.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 | 3 +++ 4 files changed, 8 insertions(+)
diff --git a/src/kimchi.conf.in b/src/kimchi.conf.in index 2be1a0e..82baa0c 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 bytes +#max_body_size = 4 * 1024 * 1024 * 1024 +
What about use MB or GB and do the conversion to bytes internally? It will be easier to user just specify a number.
# Max request body size in GB #max_body_size = 4
ACK After a second thought I think GB maybe too large unit for http body control, We use 4GB is because what we upload is a disk image file. KB seems more reasonable unit if we want to control body size for other
On 2014年09月04日 11:03, Royce Lv wrote: purpose in the future, does that make sense?
[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..08a9be7 100644 --- a/src/kimchid.in +++ b/src/kimchid.in @@ -47,6 +47,7 @@ def main(options): cherrypy_port = config.config.get("server", "cherrypy_port") runningEnv = config.config.get("server", "environment") federation = config.config.get("server", "federation") + max_body_size = config.config.get("server", "max_body_size") logDir = config.config.get("logging", "log_dir") logLevel = config.config.get("logging", "log_level")
@@ -72,6 +73,8 @@ def main(options): parser.add_option('--federation', default=federation, help="Register and discover Kimchi peers at the same " "network using openSLP")
+ parser.add_option('--max_body_size', default=max_body_size, + help="Max request body size accept by kimchi")
I think we don't need to allow it on command line. Only the config file should be enough in this case.
ACK
parser.add_option('--test', action='store_true', help="Run server in mock model") (options, args) = parser.parse_args()
_______________________________________________ Kimchi-devel mailing list Kimchi-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/kimchi-devel
_______________________________________________ Kimchi-devel mailing list Kimchi-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/kimchi-devel
participants (3)
-
Aline Manera -
lvroyce0210＠gmail.com -
Royce Lv