[kimchi-devel][PATCH 0/5] Storagevolume upload enhancement
From: Royce Lv <lvroyce@linux.vnet.ibm.com> Royce Lv (5): Improve PUT param checking Update docs and json schema of storage volume upload Add utils to support file lock Update controller to make update accept formdata params Update model for storage volume update docs/API.md | 7 +++-- src/kimchi/API.json | 31 +++++++++++++++++--- src/kimchi/control/base.py | 16 ++--------- src/kimchi/control/debugreports.py | 1 - src/kimchi/control/host.py | 1 - src/kimchi/control/storagepools.py | 1 - src/kimchi/control/templates.py | 4 --- src/kimchi/control/vm/ifaces.py | 1 - src/kimchi/control/vm/storages.py | 1 - src/kimchi/control/vms.py | 2 -- src/kimchi/i18n.py | 1 + src/kimchi/model/storagevolumes.py | 58 ++++++++++++++++---------------------- src/kimchi/model/utils.py | 16 +++++++++++ tests/test_rest.py | 6 +++- 14 files changed, 81 insertions(+), 65 deletions(-) -- 1.9.3
From: Royce Lv <lvroyce@linux.vnet.ibm.com> Delete self-defined update param validation, and use additionalProperties in json schema validator instead. Signed-off-by: Royce Lv <lvroyce@linux.vnet.ibm.com> --- src/kimchi/API.json | 12 ++++++++---- src/kimchi/control/base.py | 11 +---------- src/kimchi/control/debugreports.py | 1 - src/kimchi/control/host.py | 1 - src/kimchi/control/storagepools.py | 1 - src/kimchi/control/templates.py | 4 ---- src/kimchi/control/vm/ifaces.py | 1 - src/kimchi/control/vm/storages.py | 1 - src/kimchi/control/vms.py | 2 -- tests/test_rest.py | 6 +++++- 10 files changed, 14 insertions(+), 26 deletions(-) diff --git a/src/kimchi/API.json b/src/kimchi/API.json index c5d7bdb..fd4c901 100644 --- a/src/kimchi/API.json +++ b/src/kimchi/API.json @@ -80,7 +80,8 @@ "pattern": "^[_A-Za-z0-9-]*$", "error": "KCHDR0007E" } - } + }, + "additionalProperties": false }, "storagepools_create": { "type": "object", @@ -182,7 +183,8 @@ "minItems": 1, "uniqueItems": true } - } + }, + "additionalProperties": false }, "storagevolumes_create": { "type": "object", @@ -303,7 +305,8 @@ "minimum": 512, "error": "KCHTMPL0013E" } - } + }, + "additionalProperties": false }, "networks_create": { "type": "object", @@ -554,7 +557,8 @@ "required": true, "error": "KCHVMSTOR0003E" } - } + }, + "additionalProperties": false }, "template_update": { "type": "object", diff --git a/src/kimchi/control/base.py b/src/kimchi/control/base.py index 60db1df..f1449ea 100644 --- a/src/kimchi/control/base.py +++ b/src/kimchi/control/base.py @@ -54,7 +54,6 @@ class Resource(object): self.model = model self.ident = ident self.model_args = (ident,) - self.update_params = [] self.role_key = None self.admin_methods = [] @@ -163,6 +162,7 @@ class Resource(object): except UnauthorizedError, e: raise cherrypy.HTTPError(403, e.message) except NotFoundError, e: + raise raise cherrypy.HTTPError(404, e.message) except OperationFailed, e: raise cherrypy.HTTPError(500, e.message) @@ -193,15 +193,6 @@ class Resource(object): params = parse_request() validate_params(params, self, 'update') - if self.update_params is not None: - invalids = [v for v in params.keys() if - v not in self.update_params] - if invalids: - msg_args = {'params': ", ".join(invalids), - 'resource': get_class_name(self)} - e = InvalidOperation('KCHAPI0004E', msg_args) - raise cherrypy.HTTPError(405, e.message) - args = list(self.model_args) + [params] ident = update(*args) self._redirect(ident) diff --git a/src/kimchi/control/debugreports.py b/src/kimchi/control/debugreports.py index 1d9ce59..359fe5e 100644 --- a/src/kimchi/control/debugreports.py +++ b/src/kimchi/control/debugreports.py @@ -40,7 +40,6 @@ class DebugReport(Resource): super(DebugReport, self).__init__(model, ident) self.role_key = 'host' self.admin_methods = ['GET', 'PUT', 'POST'] - self.update_params = ["name"] self.uri_fmt = '/debugreports/%s' self.content = DebugReportContent(model, ident) diff --git a/src/kimchi/control/host.py b/src/kimchi/control/host.py index c690945..2c037ac 100644 --- a/src/kimchi/control/host.py +++ b/src/kimchi/control/host.py @@ -147,7 +147,6 @@ class Repository(Resource): super(Repository, self).__init__(model, id) self.role_key = 'host' self.admin_methods = ['GET', 'PUT', 'POST', 'DELETE'] - self.update_params = ["config", "baseurl"] self.uri_fmt = "/host/repositories/%s" self.enable = self.generate_action_handler('enable') self.disable = self.generate_action_handler('disable') diff --git a/src/kimchi/control/storagepools.py b/src/kimchi/control/storagepools.py index 460beb1..4f455b0 100644 --- a/src/kimchi/control/storagepools.py +++ b/src/kimchi/control/storagepools.py @@ -77,7 +77,6 @@ class StoragePool(Resource): super(StoragePool, self).__init__(model, ident) self.role_key = 'storage' self.admin_methods = ['PUT', 'POST', 'DELETE'] - self.update_params = ["autostart", "disks"] self.uri_fmt = "/storagepools/%s" self.activate = self.generate_action_handler('activate') self.deactivate = self.generate_action_handler('deactivate') diff --git a/src/kimchi/control/templates.py b/src/kimchi/control/templates.py index 70c9457..98b949f 100644 --- a/src/kimchi/control/templates.py +++ b/src/kimchi/control/templates.py @@ -35,10 +35,6 @@ class Template(Resource): super(Template, self).__init__(model, ident) self.role_key = 'templates' self.admin_methods = ['PUT', 'POST', 'DELETE'] - self.update_params = ["name", "folder", "icon", "os_distro", - "storagepool", "os_version", "cpus", - "memory", "cdrom", "disks", "networks", - "graphics", "cpu_info"] self.uri_fmt = "/templates/%s" self.clone = self.generate_action_handler('clone') diff --git a/src/kimchi/control/vm/ifaces.py b/src/kimchi/control/vm/ifaces.py index ebcb044..f761660 100644 --- a/src/kimchi/control/vm/ifaces.py +++ b/src/kimchi/control/vm/ifaces.py @@ -34,7 +34,6 @@ class VMIfaces(Collection): class VMIface(Resource): def __init__(self, model, vm, ident): super(VMIface, self).__init__(model, ident) - self.update_params = ["model", "network"] self.vm = vm self.ident = ident self.info = {} diff --git a/src/kimchi/control/vm/storages.py b/src/kimchi/control/vm/storages.py index 984c4d2..58b0771 100644 --- a/src/kimchi/control/vm/storages.py +++ b/src/kimchi/control/vm/storages.py @@ -39,7 +39,6 @@ class VMStorage(Resource): self.info = {} self.model_args = [self.vm, self.ident] self.uri_fmt = '/vms/%s/storages/%s' - self.update_params = ['path'] @property def data(self): diff --git a/src/kimchi/control/vms.py b/src/kimchi/control/vms.py index a1589ef..0d41e14 100644 --- a/src/kimchi/control/vms.py +++ b/src/kimchi/control/vms.py @@ -35,8 +35,6 @@ class VM(Resource): def __init__(self, model, ident): super(VM, self).__init__(model, ident) self.role_key = 'guests' - self.update_params = ["name", "users", "groups", "cpus", "memory", - "graphics"] self.screenshot = VMScreenShot(model, ident) self.uri_fmt = '/vms/%s' for ident, node in sub_nodes.items(): diff --git a/tests/test_rest.py b/tests/test_rest.py index fa15ef6..1ddbd86 100644 --- a/tests/test_rest.py +++ b/tests/test_rest.py @@ -217,6 +217,10 @@ class RestTests(unittest.TestCase): resp = self.request('/vms/vm-1/start', '{}', 'POST') self.assertEquals(200, resp.status) + req = json.dumps({'unsupported-attr': 'attr'}) + resp = self.request('/vms/vm-1', req, 'PUT') + self.assertEquals(400, resp.status) + req = json.dumps({'name': 'new-vm'}) resp = self.request('/vms/vm-1', req, 'PUT') self.assertEquals(400, resp.status) @@ -271,7 +275,7 @@ class RestTests(unittest.TestCase): req = json.dumps({'name': 'new-name', 'cpus': 5, 'UUID': 'notallowed'}) resp = self.request('/vms/vm-1', req, 'PUT') - self.assertEquals(405, resp.status) + self.assertEquals(400, resp.status) params = {'name': u'∨м-црdαtеd', 'cpus': 5, 'memory': 4096} req = json.dumps(params) -- 1.9.3
From: Royce Lv <lvroyce@linux.vnet.ibm.com> File upload will use the same REST api as 'capacity' type when creating storage volume. Uploading will be implemented by following storage volume update. Signed-off-by: Royce Lv <lvroyce@linux.vnet.ibm.com> --- docs/API.md | 7 +++++-- src/kimchi/API.json | 19 +++++++++++++++++++ src/kimchi/i18n.py | 1 + 3 files changed, 25 insertions(+), 2 deletions(-) diff --git a/docs/API.md b/docs/API.md index 210a036..b160405 100644 --- a/docs/API.md +++ b/docs/API.md @@ -471,13 +471,12 @@ 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 * 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 * url: URL to be downloaded ### Resource: Storage Volume @@ -505,6 +504,10 @@ A interface represents available network interface on VM. * **DELETE**: Remove the Storage Volume * **POST**: *See Storage Volume Actions* +* **PUT**: Upload storage volume chunk + * index: Chunk index of the slice in file. + * 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 fd4c901..a64ec38 100644 --- a/src/kimchi/API.json +++ b/src/kimchi/API.json @@ -221,6 +221,25 @@ } } }, + "storagevolume_update": { + "type": "object", + "properties": { + "chunk": { + "description": "Upload storage volume chunk" + }, + "index": { + "description": "Chunk index of uploaded storage volume", + "type": "string", + "error": "KCHVOL0024E" + }, + "chunk_size": { + "description": "Chunk size of uploaded storage volume", + "type": "string", + "error": "KCHVOL0024E" + } + }, + "additionalProperties": false + }, "vms_create": { "type": "object", "error": "KCHVM0016E", diff --git a/src/kimchi/i18n.py b/src/kimchi/i18n.py index e3a051c..b1eaee9 100644 --- a/src/kimchi/i18n.py +++ b/src/kimchi/i18n.py @@ -211,6 +211,7 @@ 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": _("Upload volume chunk index, size and total size must be integer"), "KCHIFACE0001E": _("Interface %(name)s does not exist"), -- 1.9.3
From: Royce Lv <lvroyce@linux.vnet.ibm.com> Wrap posix file lock with contextlib so that file sections can be protected by lock. Signed-off-by: Royce Lv <lvroyce@linux.vnet.ibm.com> --- src/kimchi/model/utils.py | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/src/kimchi/model/utils.py b/src/kimchi/model/utils.py index 9896289..1188b19 100644 --- a/src/kimchi/model/utils.py +++ b/src/kimchi/model/utils.py @@ -18,8 +18,10 @@ # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA import libvirt +import posixfile import socket import urlparse +from contextlib import contextmanager from lxml import etree, objectify from lxml.builder import E, ElementMaker @@ -161,3 +163,17 @@ def get_metadata_node(dom, tag, metadata_support, mode="current"): if node is not None: return etree.tostring(node) return "" + + +@contextmanager +def flock(fp, pos, len): + fp.lock('w|', len, pos) + yield + fp.lock('u', len, pos) + + +@contextmanager +def f_open(path, mod): + file = posixfile.open(path, mod) + yield file + file.close() -- 1.9.3
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 | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/kimchi/control/base.py b/src/kimchi/control/base.py index f1449ea..5ec8dc8 100644 --- a/src/kimchi/control/base.py +++ b/src/kimchi/control/base.py @@ -143,7 +143,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) @@ -154,7 +154,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: @@ -162,7 +162,6 @@ class Resource(object): except UnauthorizedError, e: raise cherrypy.HTTPError(403, e.message) except NotFoundError, e: - raise raise cherrypy.HTTPError(404, e.message) except OperationFailed, e: raise cherrypy.HTTPError(500, e.message) @@ -182,7 +181,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: -- 1.9.3
From: Royce Lv <lvroyce@linux.vnet.ibm.com> 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@linux.vnet.ibm.com> --- src/kimchi/model/storagevolumes.py | 58 ++++++++++++++++---------------------- 1 file changed, 24 insertions(+), 34 deletions(-) diff --git a/src/kimchi/model/storagevolumes.py b/src/kimchi/model/storagevolumes.py index 48f715e..b6a17da 100644 --- a/src/kimchi/model/storagevolumes.py +++ b/src/kimchi/model/storagevolumes.py @@ -33,6 +33,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 f_open, flock 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 @@ -57,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') @@ -88,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()) @@ -119,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 = """ @@ -417,6 +386,27 @@ class StorageVolumeModel(object): cb('OK', True) + def update(self, pool, name, params): + vol = self._get_storagevolume(pool, name) + path = vol.path() + + size = int(params['chunk_size']) + index = int(params['index']) + pos = size * index + try: + with f_open(path, 'a+') as fp: + with flock(fp, pos, size): + fp.seek(pos) + fp.write(params['chunk'].fullvalue()) + except Exception as e: + raise OperationFailed('KCHVOL0007E', + {'name': name, + 'pool': pool, + 'err': e.message}) + + # 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): -- 1.9.3
On 07-01-2015 07:01, lvroyce@linux.vnet.ibm.com wrote:
+ def update(self, pool, name, params): + vol = self._get_storagevolume(pool, name) + path = vol.path() + + size = int(params['chunk_size']) + index = int(params['index']) + pos = size * index + try: + with f_open(path, 'a+') as fp: + with flock(fp, pos, size): + fp.seek(pos) + fp.write(params['chunk'].fullvalue()) + except Exception as e: + raise OperationFailed('KCHVOL0007E', + {'name': name, + 'pool': pool, + 'err': e.message}) + + # Refresh to make sure volume can be found in following lookup + StoragePoolModel.get_storagepool(pool, self.conn).refresh(0)
We should not treat the storage volumes as regular files, that depends on the underlying storage pool type (e.g. it won't work with logical pools and other types). We should use the function "<storageVolume>.upload" so libvirt can put data to a storage volume in a way that works for every storage pool type.
On 01/07/2015 02:44 PM, Crístian Viana wrote:
On 07-01-2015 07:01, lvroyce@linux.vnet.ibm.com wrote:
+ def update(self, pool, name, params): + vol = self._get_storagevolume(pool, name) + path = vol.path() + + size = int(params['chunk_size']) + index = int(params['index']) + pos = size * index + try: + with f_open(path, 'a+') as fp: + with flock(fp, pos, size): + fp.seek(pos) + fp.write(params['chunk'].fullvalue()) + except Exception as e: + raise OperationFailed('KCHVOL0007E', + {'name': name, + 'pool': pool, + 'err': e.message}) + + # Refresh to make sure volume can be found in following lookup + StoragePoolModel.get_storagepool(pool, self.conn).refresh(0)
We should not treat the storage volumes as regular files, that depends on the underlying storage pool type (e.g. it won't work with logical pools and other types). We should use the function "<storageVolume>.upload" so libvirt can put data to a storage volume in a way that works for every storage pool type. Good point, haven't tried this api, but will take a look at it .
Ops.. wrong notification This patch set was not applied! On 13/01/2015 11:31, Aline Manera wrote:
Applied. Thanks.
Regards,
Aline Manera
_______________________________________________ Kimchi-devel mailing list Kimchi-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/kimchi-devel
participants (4)
-
Aline Manera -
Crístian Viana -
lvroyce＠linux.vnet.ibm.com -
Royce Lv