From 668e509dc00642461c19bf8e95831ce9267c9e39 Mon Sep 17 00:00:00 2001 From: Diem Tran Date: Wed, 30 Sep 2015 14:04:47 -0400 Subject: [PATCH] Fix ZFSSA drivers' local cache bugs - Use image_utils to retrieve an image's virtual_size on the fly. - Fix race conditions happened when multiple bootable volumes are created and deleted simultaneously. - Handle a situation in iSCSI driver when custom schema are deleted. Change-Id: I588fb6c668f2a891ab24d89fea26536dd0cd5742 Closes-Bug: #1498965 --- cinder/tests/unit/test_zfssa.py | 62 +++++++++++++++-------- cinder/volume/drivers/zfssa/zfssaiscsi.py | 46 +++++++++++------ cinder/volume/drivers/zfssa/zfssanfs.py | 32 ++++++++---- 3 files changed, 92 insertions(+), 48 deletions(-) diff --git a/cinder/tests/unit/test_zfssa.py b/cinder/tests/unit/test_zfssa.py index 4e35c2e53..83709c493 100644 --- a/cinder/tests/unit/test_zfssa.py +++ b/cinder/tests/unit/test_zfssa.py @@ -23,8 +23,10 @@ import six from cinder import context from cinder import exception +from cinder.image import image_utils from cinder import test from cinder.tests.unit import fake_utils +from cinder.tests.unit import utils from cinder.volume import configuration as conf from cinder.volume import driver from cinder.volume.drivers import remotefs @@ -48,14 +50,14 @@ no_virtsize_img = { small_img = { 'id': 'small_id1234', 'size': 654321, - 'properties': {'virtual_size': 2361393152}, + 'virtual_size': 2361393152, 'updated_at': date(2015, 1, 1), } large_img = { 'id': 'large_id5678', 'size': 50000000, - 'properties': {'virtual_size': 11806965760}, + 'virtual_size': 11806965760, 'updated_at': date(2015, 2, 2), } @@ -81,6 +83,11 @@ img_service = 'fakeimgservice' img_location = 'fakeimglocation' +class ImgInfo(object): + def __init__(self, vsize): + self.virtual_size = vsize + + class FakeResponse(object): def __init__(self, statuscode, data='data'): self.status = statuscode @@ -512,11 +519,15 @@ class TestZFSSAISCSIDriver(test.TestCase): val = None return val + @mock.patch.object(image_utils, 'qemu_img_info') + @mock.patch.object(image_utils.TemporaryImages, 'fetch') @mock.patch.object(iscsi.ZFSSAISCSIDriver, '_verify_cache_volume') - def test_clone_image_negative(self, _verify_cache_volume): + def test_clone_image_negative(self, _verify_cache_volume, _fetch, _info): # Disabling local cache feature: self.configuration.zfssa_enable_local_cache = False + _fetch.return_value = mock.MagicMock(spec=utils.get_file_spec()) + _info.return_value = ImgInfo(small_img['virtual_size']) self.assertEqual((None, False), self.drv.clone_image(fakecontext, self.test_vol, img_location, @@ -525,20 +536,15 @@ class TestZFSSAISCSIDriver(test.TestCase): self.configuration.zfssa_enable_local_cache = True # Creating a volume smaller than image: + _info.return_value = ImgInfo(large_img['virtual_size']) self.assertEqual((None, False), self.drv.clone_image(fakecontext, self.test_vol, img_location, large_img, img_service)) - # The image does not have virtual_size property: - self.assertEqual((None, False), - self.drv.clone_image(fakecontext, self.test_vol, - img_location, - no_virtsize_img, - img_service)) - # Exception raised in _verify_cache_image + _info.return_value = ImgInfo(small_img['virtual_size']) self.drv._verify_cache_volume.side_effect = ( exception.VolumeBackendAPIException('fakeerror')) self.assertEqual((None, False), @@ -547,15 +553,21 @@ class TestZFSSAISCSIDriver(test.TestCase): small_img, img_service)) + @mock.patch.object(image_utils, 'qemu_img_info') + @mock.patch.object(image_utils.TemporaryImages, 'fetch') @mock.patch.object(iscsi.ZFSSAISCSIDriver, '_get_voltype_specs') @mock.patch.object(iscsi.ZFSSAISCSIDriver, '_verify_cache_volume') @mock.patch.object(iscsi.ZFSSAISCSIDriver, 'extend_volume') - def test_clone_image(self, _extend_vol, _verify_cache, _get_specs): + def test_clone_image(self, _extend_vol, _verify_cache, _get_specs, + _fetch, _info): lcfg = self.configuration cache_vol = 'os-cache-vol-%s' % small_img['id'] cache_snap = 'image-%s' % small_img['id'] self.drv._get_voltype_specs.return_value = fakespecs.copy() self.drv._verify_cache_volume.return_value = cache_vol, cache_snap + _fetch.return_value = mock.MagicMock(spec=utils.get_file_spec()) + _info.return_value = ImgInfo(small_img['virtual_size']) + model, cloned = self.drv.clone_image(fakecontext, self.test_vol2, img_location, small_img, @@ -636,7 +648,7 @@ class TestZFSSAISCSIDriver(test.TestCase): @mock.patch.object(driver.BaseVD, 'copy_image_to_volume') def test_create_cache_volume(self, _copy_image): lcfg = self.configuration - virtual_size = int(small_img['properties'].get('virtual_size')) + virtual_size = int(small_img['virtual_size']) volsize = math.ceil(float(virtual_size) / units.Gi) lunsize = "%sg" % six.text_type(int(volsize)) volname = 'os-cache-vol-%s' % small_img['id'] @@ -882,9 +894,15 @@ class TestZFSSANFSDriver(test.TestCase): self.drv.zfssa.delete_file.assert_called_once_with( img_props_nfs['name']) + @mock.patch.object(image_utils, 'qemu_img_info') + @mock.patch.object(image_utils.TemporaryImages, 'fetch') @mock.patch.object(zfssanfs.ZFSSANFSDriver, '_verify_cache_volume') @mock.patch.object(zfssanfs.ZFSSANFSDriver, 'create_cloned_volume') - def test_clone_image_negative(self, _create_clone, _verify_cache_volume): + def test_clone_image_negative(self, _create_clone, _verify_cache_volume, + _fetch, _info): + _fetch.return_value = mock.MagicMock(spec=utils.get_file_spec()) + _info.return_value = ImgInfo(small_img['virtual_size']) + # Disabling local cache feature: self.configuration.zfssa_enable_local_cache = False self.assertEqual((None, False), @@ -896,20 +914,15 @@ class TestZFSSANFSDriver(test.TestCase): self.configuration.zfssa_enable_local_cache = True # Creating a volume smaller than image: + _info.return_value = ImgInfo(large_img['virtual_size']) self.assertEqual((None, False), self.drv.clone_image(fakecontext, self.test_vol, img_location, large_img, img_service)) - # The image does not have virtual_size property: - self.assertEqual((None, False), - self.drv.clone_image(fakecontext, self.test_vol, - img_location, - no_virtsize_img, - img_service)) - # Exception raised in _verify_cache_image + _info.return_value = ImgInfo(small_img['virtual_size']) self.drv._verify_cache_volume.side_effect = ( exception.VolumeBackendAPIException('fakeerror')) self.assertEqual((None, False), @@ -918,10 +931,15 @@ class TestZFSSANFSDriver(test.TestCase): small_img, img_service)) + @mock.patch.object(image_utils, 'qemu_img_info') + @mock.patch.object(image_utils.TemporaryImages, 'fetch') @mock.patch.object(zfssanfs.ZFSSANFSDriver, 'create_cloned_volume') @mock.patch.object(zfssanfs.ZFSSANFSDriver, '_verify_cache_volume') @mock.patch.object(zfssanfs.ZFSSANFSDriver, 'extend_volume') - def test_clone_image(self, _extend_vol, _verify_cache, _create_clone): + def test_clone_image(self, _extend_vol, _verify_cache, _create_clone, + _fetch, _info): + _fetch.return_value = mock.MagicMock(spec=utils.get_file_spec()) + _info.return_value = ImgInfo(small_img['virtual_size']) self.drv._verify_cache_volume.return_value = img_props_nfs['name'] prov_loc = {'provider_location': self.test_vol['provider_location']} self.drv.create_cloned_volume.return_value = prov_loc @@ -990,7 +1008,7 @@ class TestZFSSANFSDriver(test.TestCase): @mock.patch.object(remotefs.RemoteFSDriver, 'copy_image_to_volume') @mock.patch.object(remotefs.RemoteFSDriver, 'create_volume') def test_create_cache_volume(self, _create_vol, _copy_image): - virtual_size = int(small_img['properties'].get('virtual_size')) + virtual_size = int(small_img['virtual_size']) volsize = math.ceil(float(virtual_size) / units.Gi) cache_vol = { 'name': img_props_nfs['name'], diff --git a/cinder/volume/drivers/zfssa/zfssaiscsi.py b/cinder/volume/drivers/zfssa/zfssaiscsi.py index 389182b0b..587798b4d 100644 --- a/cinder/volume/drivers/zfssa/zfssaiscsi.py +++ b/cinder/volume/drivers/zfssa/zfssaiscsi.py @@ -26,6 +26,7 @@ import six from cinder import exception from cinder import utils from cinder.i18n import _, _LE, _LI, _LW +from cinder.image import image_utils from cinder.volume import driver from cinder.volume.drivers.san import san from cinder.volume.drivers.zfssa import zfssarest @@ -461,6 +462,7 @@ class ZFSSAISCSIDriver(driver.ISCSIDriver): # Cleanup snapshot self.delete_snapshot(zfssa_snapshot) + @utils.synchronized('zfssaiscsi', external=True) def clone_image(self, context, volume, image_location, image_meta, image_service): @@ -476,21 +478,28 @@ class ZFSSAISCSIDriver(driver.ISCSIDriver): Create a new cache volume as in (1). Clone a volume from the cache volume and returns it to Cinder. + + A file lock is placed on this method to prevent: + + (a) a race condition when a cache volume has been verified, but then + gets deleted before it is cloned. + + (b) failure of subsequent clone_image requests if the first request is + still pending. """ LOG.debug('Cloning image %(image)s to volume %(volume)s', {'image': image_meta['id'], 'volume': volume['name']}) lcfg = self.configuration + cachevol_size = 0 if not lcfg.zfssa_enable_local_cache: return None, False - # virtual_size is the image's actual size when stored in a volume - # virtual_size is expected to be updated manually through glance - try: - virtual_size = int(image_meta['properties'].get('virtual_size')) - except Exception: - LOG.error(_LE('virtual_size property is not set for the image.')) - return None, False - cachevol_size = int(math.ceil(float(virtual_size) / units.Gi)) + with image_utils.TemporaryImages.fetch(image_service, + context, + image_meta['id']) as tmp_image: + info = image_utils.qemu_img_info(tmp_image) + cachevol_size = int(math.ceil(float(info.virtual_size) / units.Gi)) + if cachevol_size > volume['size']: exception_msg = (_LE('Image size %(img_size)dGB is larger ' 'than volume size %(vol_size)dGB.'), @@ -529,7 +538,6 @@ class ZFSSAISCSIDriver(driver.ISCSIDriver): return None, True - @utils.synchronized('zfssaiscsi', external=True) def _verify_cache_volume(self, context, img_meta, img_service, specs, cachevol_props): """Verify if we have a cache volume that we want. @@ -542,9 +550,6 @@ class ZFSSAISCSIDriver(driver.ISCSIDriver): If it's out of date, delete it and create a new one. After the function returns, there should be a cache volume available, ready for cloning. - - There needs to be a file lock here, otherwise subsequent clone_image - requests will fail if the first request is still pending. """ lcfg = self.configuration cachevol_name = 'os-cache-vol-%s' % img_meta['id'] @@ -562,6 +567,13 @@ class ZFSSAISCSIDriver(driver.ISCSIDriver): cache_vol = self.zfssa.get_lun(lcfg.zfssa_pool, lcfg.zfssa_cache_project, cachevol_name) + if (not cache_vol.get('updated_at', None) or + not cache_vol.get('image_id', None)): + exc_msg = (_('Cache volume %s does not have required ' + 'properties') % cachevol_name) + LOG.error(exc_msg) + raise exception.VolumeBackendAPIException(data=exc_msg) + cache_snap = self.zfssa.get_lun_snapshot(lcfg.zfssa_pool, lcfg.zfssa_cache_project, cachevol_name, @@ -950,9 +962,13 @@ class ZFSSAISCSIDriver(driver.ISCSIDriver): numclones = 0 if numclones == 0: - self.zfssa.delete_lun(lcfg.zfssa_pool, - lcfg.zfssa_cache_project, - cache['share']) + try: + self.zfssa.delete_lun(lcfg.zfssa_pool, + lcfg.zfssa_cache_project, + cache['share']) + except exception.VolumeBackendAPIException: + LOG.warning(_LW("Volume %s exists but can't be deleted"), + cache['share']) class MigrateVolumeInit(task.Task): diff --git a/cinder/volume/drivers/zfssa/zfssanfs.py b/cinder/volume/drivers/zfssa/zfssanfs.py index 71dcc32b6..2b25c6a95 100644 --- a/cinder/volume/drivers/zfssa/zfssanfs.py +++ b/cinder/volume/drivers/zfssa/zfssanfs.py @@ -28,6 +28,7 @@ import six from cinder import exception from cinder import utils from cinder.i18n import _, _LE, _LI +from cinder.image import image_utils from cinder.volume.drivers import nfs from cinder.volume.drivers.san import san from cinder.volume.drivers.zfssa import zfssarest @@ -297,6 +298,7 @@ class ZFSSANFSDriver(nfs.NfsDriver): 'volume': volume['name']}) self._check_origin(vol_props['origin']) + @utils.synchronized('zfssanfs', external=True) def clone_image(self, context, volume, image_location, image_meta, image_service): @@ -312,21 +314,26 @@ class ZFSSANFSDriver(nfs.NfsDriver): Create a new cache volume as in (1). Clone a volume from the cache volume and returns it to Cinder. + + A file lock is placed on this method to prevent: + (a) a race condition when a cache volume has been verified, but then + gets deleted before it is cloned. + + (b) failure of subsequent clone_image requests if the first request is + still pending. """ LOG.debug('Cloning image %(image)s to volume %(volume)s', {'image': image_meta['id'], 'volume': volume['name']}) lcfg = self.configuration + cachevol_size = 0 if not lcfg.zfssa_enable_local_cache: return None, False - # virtual_size is the image's actual size when stored in a volume - # virtual_size is expected to be updated manually through glance - try: - virtual_size = int(image_meta['properties'].get('virtual_size')) - except Exception: - LOG.error(_LE('virtual_size property is not set for the image.')) - return None, False - cachevol_size = int(math.ceil(float(virtual_size) / units.Gi)) + with image_utils.TemporaryImages.fetch( + image_service, context, image_meta['id']) as tmp_image: + info = image_utils.qemu_img_info(tmp_image) + cachevol_size = int(math.ceil(float(info.virtual_size) / units.Gi)) + if cachevol_size > volume['size']: exception_msg = (_LE('Image size %(img_size)dGB is larger ' 'than volume size %(vol_size)dGB.'), @@ -370,7 +377,6 @@ class ZFSSANFSDriver(nfs.NfsDriver): return clone_vol, True - @utils.synchronized('zfssanfs', external=True) def _verify_cache_volume(self, context, img_meta, img_service, cachevol_props): """Verify if we have a cache volume that we want. @@ -490,7 +496,12 @@ class ZFSSANFSDriver(nfs.NfsDriver): If the cache no longer has clone, it will be deleted. """ - cachevol_props = self.zfssa.get_volume(origin) + try: + cachevol_props = self.zfssa.get_volume(origin) + except exception.VolumeNotFound: + LOG.debug('Origin %s does not exist', origin) + return + numclones = cachevol_props['numclones'] LOG.debug('Number of clones: %d', numclones) if numclones <= 1: @@ -500,7 +511,6 @@ class ZFSSANFSDriver(nfs.NfsDriver): cachevol_props = {'numclones': six.text_type(numclones - 1)} self.zfssa.set_file_props(origin, cachevol_props) - @utils.synchronized('zfssanfs', external=True) def _update_origin(self, vol_name, cachevol_name): """Update WebDAV property of a volume. -- 2.45.2