This is the first patchset which fixes the issue #544. After applying these patches, the user can now download image files to a logical storage pool. However, they cannot create a template with it because (1) the UI filters the storage volumes and only displays the ones with format='iso' and storage volumes stored in a logical pool always have format='raw', and (2) the template classes fail to identify logical volumes as files because they're symlinks. Editing an existing VM's XML to use the storage volume downloaded to a logical pool works, though. Crístian Viana (3): Change "_get_storagevolume" to static Use 'bytes' as volume capacity and allocation unit issue #544: Refactor storage volume download docs/API.md | 4 +- src/kimchi/model/storagevolumes.py | 75 +++++++++++++++++++++++++++++++------- tests/test_model.py | 19 +++++----- tests/test_rest.py | 12 +++--- 4 files changed, 78 insertions(+), 32 deletions(-) -- 2.1.0
The class StorageVolumeModel is instantiated during the creation of StorageVolumesModel. Thus, the class StorageVolumesModel cannot have an instance of StorageVolumeModel initialized in its constructor as well, otherwise it will cause an infinite loop (i.e. StorageVolumeModel creates an instance of StorageVolumesModel during its initialization, which creates an instance of StorageVolumeModel...). On future patches, we will need to use the function "_get_storagevolume" (StorageVolumeModel) from inside StorageVolumesModel, and due to the problem described above, it isn't possible to have an instance of StorageVolumeModel inside StorageVolumesModel. Change the function "_get_storagevolume" to static, and rename it to "get_storagevolume". Signed-off-by: Crístian Viana <vianac@linux.vnet.ibm.com> --- src/kimchi/model/storagevolumes.py | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/src/kimchi/model/storagevolumes.py b/src/kimchi/model/storagevolumes.py index 48f715e..56c54fa 100644 --- a/src/kimchi/model/storagevolumes.py +++ b/src/kimchi/model/storagevolumes.py @@ -249,8 +249,9 @@ class StorageVolumeModel(object): self.task = TaskModel(**kargs) self.storagevolumes = StorageVolumesModel(**kargs) - def _get_storagevolume(self, poolname, name): - pool = StoragePoolModel.get_storagepool(poolname, self.conn) + @staticmethod + def get_storagevolume(poolname, name, conn): + pool = StoragePoolModel.get_storagepool(poolname, conn) if not pool.isActive(): raise InvalidOperation("KCHVOL0006E", {'name': pool}) try: @@ -263,7 +264,7 @@ class StorageVolumeModel(object): raise def lookup(self, pool, name): - vol = self._get_storagevolume(pool, name) + vol = StorageVolumeModel.get_storagevolume(pool, name, self.conn) path = vol.path() info = vol.info() xml = vol.XMLDesc(0) @@ -297,7 +298,7 @@ class StorageVolumeModel(object): return res def wipe(self, pool, name): - volume = self._get_storagevolume(pool, name) + volume = StorageVolumeModel.get_storagevolume(pool, name, self.conn) try: volume.wipePattern(libvirt.VIR_STORAGE_VOL_WIPE_ALG_ZERO, 0) except libvirt.libvirtError as e: @@ -310,7 +311,7 @@ class StorageVolumeModel(object): if pool_info['type'] in READONLY_POOL_TYPE: raise InvalidParameter("KCHVOL0012E", {'type': pool_info['type']}) - volume = self._get_storagevolume(pool, name) + volume = StorageVolumeModel.get_storagevolume(pool, name, self.conn) try: volume.delete(0) except libvirt.libvirtError as e: @@ -319,7 +320,7 @@ class StorageVolumeModel(object): def resize(self, pool, name, size): size = size << 20 - volume = self._get_storagevolume(pool, name) + volume = StorageVolumeModel.get_storagevolume(pool, name, self.conn) try: volume.resize(size, 0) except libvirt.libvirtError as e: @@ -385,8 +386,9 @@ class StorageVolumeModel(object): try: cb('setting up volume cloning') - orig_vir_vol = self._get_storagevolume(orig_pool_name, - orig_vol_name) + orig_vir_vol = StorageVolumeModel.get_storagevolume(orig_pool_name, + orig_vol_name, + self.conn) orig_vol = self.lookup(orig_pool_name, orig_vol_name) new_vir_pool = StoragePoolModel.get_storagepool(new_pool_name, self.conn) -- 2.1.0
When creating a storage volume using the REST API (i.e. "POST /storagepools/<pool>/storagevolumes {'capacity': 5}", the parameters 'capacity' and 'allocation' must be specified in megabytes (actually, mebibytes). This approach is not consistent with the REST command "GET /storagepools/<pool>/storagevolumes/<vol>", which returns the same values in bytes, and it also doesn't allow creating a volume with a size not multiple of 1 MiB. Update the REST command "POST /storagepools/<pool>/storagevolumes" so it receives size parameters in bytes. Signed-off-by: Crístian Viana <vianac@linux.vnet.ibm.com> --- docs/API.md | 4 ++-- src/kimchi/model/storagevolumes.py | 5 ++--- tests/test_model.py | 19 +++++++++---------- tests/test_rest.py | 12 ++++++------ 4 files changed, 19 insertions(+), 21 deletions(-) diff --git a/docs/API.md b/docs/API.md index 210a036..1d20f0b 100644 --- a/docs/API.md +++ b/docs/API.md @@ -475,7 +475,7 @@ A interface represents available network interface on VM. * 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 + The unit is bytes * format: The format of the defined Storage Volume * file: File to be uploaded, passed through form data * url: URL to be downloaded @@ -510,7 +510,7 @@ A interface represents available network interface on VM. * resize: Resize a Storage Volume * size: resize the total space which can be used to store data - The unit is MBytes + The unit is bytes * wipe: Wipe a Storage Volume * clone: Clone a Storage Volume. * pool: The name of the destination pool (optional). diff --git a/src/kimchi/model/storagevolumes.py b/src/kimchi/model/storagevolumes.py index 56c54fa..4bf029a 100644 --- a/src/kimchi/model/storagevolumes.py +++ b/src/kimchi/model/storagevolumes.py @@ -154,8 +154,8 @@ class StorageVolumesModel(object): vol_xml = """ <volume> <name>%(name)s</name> - <allocation unit="MiB">%(allocation)s</allocation> - <capacity unit="MiB">%(capacity)s</capacity> + <allocation unit='bytes'>%(allocation)s</allocation> + <capacity unit='bytes'>%(capacity)s</capacity> <source> </source> <target> @@ -319,7 +319,6 @@ class StorageVolumeModel(object): {'name': name, 'err': e.get_error_message()}) def resize(self, pool, name, size): - size = size << 20 volume = StorageVolumeModel.get_storagevolume(pool, name, self.conn) try: volume.resize(size, 0) diff --git a/tests/test_model.py b/tests/test_model.py index 5984906..2c44893 100644 --- a/tests/test_model.py +++ b/tests/test_model.py @@ -186,8 +186,8 @@ class ModelTests(unittest.TestCase): with RollbackContext() as rollback: vol = 'base-vol.img' params = {'name': vol, - 'capacity': 1024, - 'allocation': 1, + 'capacity': 1073741824, # 1 GiB + 'allocation': 1048576, # 1 MiB 'format': 'qcow2'} task_id = inst.storagevolumes_create('default', params)['id'] rollback.prependDefer(inst.storagevolume_delete, 'default', vol) @@ -357,8 +357,8 @@ class ModelTests(unittest.TestCase): rollback.prependDefer(inst.storagepool_deactivate, pool) params = {'name': vol, - 'capacity': 1024, - 'allocation': 512, + 'capacity': 1073741824, # 1 GiB + 'allocation': 536870912, # 512 MiB 'format': 'qcow2'} task_id = inst.storagevolumes_create(pool, params)['id'] rollback.prependDefer(inst.storagevolume_delete, pool, vol) @@ -635,8 +635,8 @@ class ModelTests(unittest.TestCase): vols = inst.storagevolumes_get_list(pool) num = len(vols) + 2 - params = {'capacity': 1024, - 'allocation': 512, + params = {'capacity': 1073741824, # 1 GiB + 'allocation': 536870912, # 512 MiB 'format': 'raw'} # 'name' is required for this type of volume self.assertRaises(InvalidParameter, inst.storagevolumes_create, @@ -660,13 +660,12 @@ class ModelTests(unittest.TestCase): self.assertEquals(0, volinfo['ref_cnt']) volinfo = inst.storagevolume_lookup(pool, vol) - # Define the size = capacity + 16M - capacity = volinfo['capacity'] >> 20 - size = capacity + 16 + # Define the size = capacity + 16 MiB + size = volinfo['capacity'] + 16777216 inst.storagevolume_resize(pool, vol, size) volinfo = inst.storagevolume_lookup(pool, vol) - self.assertEquals((1024 + 16) << 20, volinfo['capacity']) + self.assertEquals(size, volinfo['capacity']) poolinfo = inst.storagepool_lookup(pool) self.assertEquals(len(vols), poolinfo['nr_volumes']) diff --git a/tests/test_rest.py b/tests/test_rest.py index fa15ef6..e61cfcf 100644 --- a/tests/test_rest.py +++ b/tests/test_rest.py @@ -1189,8 +1189,8 @@ class RestTests(unittest.TestCase): # Create a storage volume can only be successful for active pool req = json.dumps({'name': 'test-volume', - 'capacity': 1024, - 'allocation': 512, + 'capacity': 1073741824, # 1 GiB + 'allocation': 536870912, # 512 MiB 'type': 'disk', 'format': 'raw'}) resp = self.request('/storagepools/pool-2/storagevolumes/', @@ -1212,12 +1212,12 @@ class RestTests(unittest.TestCase): self.assertEquals('raw', storagevolume['format']) # Resize the storage volume - req = json.dumps({'size': 768}) + req = json.dumps({'size': 805306368}) # 768 MiB uri = '/storagepools/pool-2/storagevolumes/test-volume/resize' resp = self.request(uri, req, 'POST') uri = '/storagepools/pool-2/storagevolumes/test-volume' storagevolume = json.loads(self.request(uri).read()) - self.assertEquals(768 << 20, storagevolume['capacity']) + self.assertEquals(805306368, storagevolume['capacity']) # 768 MiB # Wipe the storage volume uri = '/storagepools/pool-2/storagevolumes/test-volume/wipe' @@ -1489,7 +1489,7 @@ class RestTests(unittest.TestCase): self._create_pool('pool-3') self.request('/storagepools/pool-3/activate', '{}', 'POST') params = {'name': 'fedora.iso', - 'capacity': 1024, + 'capacity': 1073741824, # 1 GiB 'type': 'file', 'format': 'iso'} model.storagevolumes_create('pool-3', params) @@ -1500,7 +1500,7 @@ class RestTests(unittest.TestCase): self.assertEquals('iso', storagevolume['format']) self.assertEquals('/var/lib/libvirt/images/fedora.iso', storagevolume['path']) - self.assertEquals(1024 << 20, storagevolume['capacity']) + self.assertEquals(1073741824, storagevolume['capacity']) # 1 GiB self.assertEquals(0, storagevolume['allocation']) self.assertEquals('unknown', storagevolume['os_version']) self.assertEquals('unknown', storagevolume['os_distro']) -- 2.1.0
The current storage volume download implementation relies on writing the new volume as a file. But that's not the case for some storage pool types (e.g. logical), which don't treat their volumes as plain files. Use the libvirt API to write data to a storage volume instead of writing it directly to a file. The libvirt API requires an existing volume to write the data to it, and a volume requires a capacity to be created. As some remote URLs don't provide the header 'Content-length', sometimes we need to download a file without knowing its size in advance. Due to those cases, instead of downloading and writing the data directly to the storage volume, this patch makes it so a temporary file is always created - so we can get its size - and then that file is transfered to a storage volume. Fix issue #544 ("Download to logical storage pool fails"). Signed-off-by: Crístian Viana <vianac@linux.vnet.ibm.com> --- src/kimchi/model/storagevolumes.py | 52 +++++++++++++++++++++++++++++++++++--- 1 file changed, 49 insertions(+), 3 deletions(-) diff --git a/src/kimchi/model/storagevolumes.py b/src/kimchi/model/storagevolumes.py index 4bf029a..67fcaf0 100644 --- a/src/kimchi/model/storagevolumes.py +++ b/src/kimchi/model/storagevolumes.py @@ -20,6 +20,7 @@ import contextlib import lxml.etree as ET import os +import tempfile import time import urllib2 from lxml.builder import E @@ -203,7 +204,11 @@ class StorageVolumesModel(object): pool_model = StoragePoolModel(conn=self.conn, objstore=self.objstore) pool = pool_model.lookup(pool_name) - file_path = os.path.join(pool['path'], name) + + if pool['type'] in ['dir', 'netfs']: + file_path = os.path.join(pool['path'], name) + else: + file_path = tempfile.mkstemp(prefix=name)[1] with contextlib.closing(urllib2.urlopen(url)) as response: with open(file_path, 'w') as volume_file: @@ -219,14 +224,55 @@ class StorageVolumesModel(object): volume_file.write(chunk_data) downloaded_size += len(chunk_data) cb('%s/%s' % (downloaded_size, remote_size)) - except Exception, e: + except (IOError, libvirt.libvirtError) as e: if os.path.isfile(file_path): os.remove(file_path) + raise OperationFailed('KCHVOL0007E', {'name': name, 'pool': pool_name, 'err': e.message}) - StoragePoolModel.get_storagepool(pool_name, self.conn).refresh(0) + if pool['type'] in ['dir', 'netfs']: + virt_pool = StoragePoolModel.get_storagepool(pool_name, self.conn) + virt_pool.refresh(0) + else: + def _stream_handler(stream, nbytes, fd): + return fd.read(nbytes) + + virt_stream = virt_vol = None + + try: + task = self.create(pool_name, {'name': name, + 'format': 'raw', + 'capacity': downloaded_size, + 'allocation': downloaded_size}) + self.task.wait(task['id']) + virt_vol = StorageVolumeModel.get_storagevolume(pool_name, + name, + self.conn) + + virt_stream = self.conn.get().newStream(0) + virt_vol.upload(virt_stream, 0, downloaded_size, 0) + + with open(file_path) as fd: + virt_stream.sendAll(_stream_handler, fd) + + virt_stream.finish() + except (IOError, libvirt.libvirtError) as e: + try: + if virt_stream: + virt_stream.abort() + if virt_vol: + virt_vol.delete(0) + except libvirt.libvirtError, virt_e: + kimchi_log.error(virt_e.message) + finally: + raise OperationFailed('KCHVOL0007E', {'name': name, + 'pool': pool_name, + 'err': e.message}) + finally: + os.remove(file_path) + cb('OK', True) def get_list(self, pool_name): -- 2.1.0
Reviewed-by: Daniel Barboza <dhbarboza82@gmail.com> On 12/30/2014 11:24 AM, Crístian Viana wrote:
This is the first patchset which fixes the issue #544. After applying these patches, the user can now download image files to a logical storage pool. However, they cannot create a template with it because (1) the UI filters the storage volumes and only displays the ones with format='iso' and storage volumes stored in a logical pool always have format='raw', and (2) the template classes fail to identify logical volumes as files because they're symlinks.
Editing an existing VM's XML to use the storage volume downloaded to a logical pool works, though.
Crístian Viana (3): Change "_get_storagevolume" to static Use 'bytes' as volume capacity and allocation unit issue #544: Refactor storage volume download
docs/API.md | 4 +- src/kimchi/model/storagevolumes.py | 75 +++++++++++++++++++++++++++++++------- tests/test_model.py | 19 +++++----- tests/test_rest.py | 12 +++--- 4 files changed, 78 insertions(+), 32 deletions(-)
On 30/12/2014 11:24, Crístian Viana wrote:
This is the first patchset which fixes the issue #544. After applying these patches, the user can now download image files to a logical storage pool.
However, they cannot create a template with it because (1) the UI filters the storage volumes and only displays the ones with format='iso' and storage volumes stored in a logical pool always have format='raw', and (2) the template classes fail to identify logical volumes as files because they're symlinks.
Cristian, please open issues to track those problems you found. I hope to get them fixed for 1.4.1 release too.
Editing an existing VM's XML to use the storage volume downloaded to a logical pool works, though.
Crístian Viana (3): Change "_get_storagevolume" to static Use 'bytes' as volume capacity and allocation unit issue #544: Refactor storage volume download
docs/API.md | 4 +- src/kimchi/model/storagevolumes.py | 75 +++++++++++++++++++++++++++++++------- tests/test_model.py | 19 +++++----- tests/test_rest.py | 12 +++--- 4 files changed, 78 insertions(+), 32 deletions(-)
On 08/01/2015 17:06, Crístian Viana wrote:
On 08-01-2015 12:40, Aline Manera wrote:
Cristian, please open issues to track those problems you found. I hope to get them fixed for 1.4.1 release too.
Issues #564 and #565 have been filed regarding the problems I described here.
Thanks!
participants (3)
-
Aline Manera -
Crístian Viana -
Daniel Henrique Barboza