From: Lucian Petrut Date: Tue, 17 Feb 2015 16:28:07 +0000 (+0200) Subject: SMBFS: Fix retrieving total allocated size X-Git-Url: https://review.fuel-infra.org/gitweb?a=commitdiff_plain;h=667c1dc511f40854a8bc5ce62ea1461735ba8fc2;p=openstack-build%2Fcinder-build.git SMBFS: Fix retrieving total allocated size Currently, the Windows SMBFS driver parses all the VHD/X images present on the configured share in order to retrieve the allocated size. Not only that this can be slow when having many volumes, but it can also fail in certain environments when the images are in use. The Linux SMBFS driver uses 'du', which gives incorrect values in case of VHD/X images. In order to avoid this, the driver can keep track of the allocated size according to each share, the most basic solution being a JSON stored in a file. As the Windows SMBFS driver inherits the Linux SMBFS driver, only overwriting the os specific methods, the fix is applied to the base SMBFS driver. DocImpact Closes-Bug: #1425100 Change-Id: I14aa7b001605ae14fe0b6d0a45ee6b1abf8c4f60 --- diff --git a/cinder/tests/unit/test_smbfs.py b/cinder/tests/unit/test_smbfs.py index 92f0d3120..bb1a65a5c 100644 --- a/cinder/tests/unit/test_smbfs.py +++ b/cinder/tests/unit/test_smbfs.py @@ -13,19 +13,37 @@ # under the License. import copy +import functools import os import mock +from oslo_utils import fileutils from cinder import exception from cinder.image import image_utils from cinder import test +from cinder.volume.drivers import remotefs from cinder.volume.drivers import smbfs +def requires_allocation_data_update(expected_size): + def wrapper(func): + @functools.wraps(func) + def inner(inst, *args, **kwargs): + with mock.patch.object( + inst._smbfs_driver, + 'update_disk_allocation_data') as fake_update: + func(inst, *args, **kwargs) + fake_update.assert_called_once_with(inst._FAKE_VOLUME, + expected_size) + return inner + return wrapper + + class SmbFsTestCase(test.TestCase): _FAKE_SHARE = '//1.2.3.4/share1' + _FAKE_SHARE_HASH = 'db0bf952c1734092b83e8990bd321131' _FAKE_MNT_BASE = '/mnt' _FAKE_VOLUME_NAME = 'volume-4f711859-4928-4cb7-801a-a50c37ceaccc' _FAKE_TOTAL_SIZE = '2048' @@ -36,7 +54,7 @@ class SmbFsTestCase(test.TestCase): 'provider_location': _FAKE_SHARE, 'name': _FAKE_VOLUME_NAME, 'status': 'available'} - _FAKE_MNT_POINT = os.path.join(_FAKE_MNT_BASE, 'fake_hash') + _FAKE_MNT_POINT = os.path.join(_FAKE_MNT_BASE, _FAKE_SHARE_HASH) _FAKE_VOLUME_PATH = os.path.join(_FAKE_MNT_POINT, _FAKE_VOLUME_NAME) _FAKE_SNAPSHOT_ID = '5g811859-4928-4cb7-801a-a50c37ceacba' _FAKE_SNAPSHOT = {'id': _FAKE_SNAPSHOT_ID, @@ -48,9 +66,9 @@ class SmbFsTestCase(test.TestCase): _FAKE_SHARE_OPTS = '-o username=Administrator,password=12345' _FAKE_OPTIONS_DICT = {'username': 'Administrator', 'password': '12345'} + _FAKE_ALLOCATION_DATA_PATH = os.path.join('fake_dir', + 'fake_allocation_data') - _FAKE_LISTDIR = [_FAKE_VOLUME_NAME, _FAKE_VOLUME_NAME + '.vhd', - _FAKE_VOLUME_NAME + '.vhdx', 'fake_folder'] _FAKE_SMBFS_CONFIG = mock.MagicMock() _FAKE_SMBFS_CONFIG.smbfs_oversub_ratio = 2 _FAKE_SMBFS_CONFIG.smbfs_used_ratio = 0.5 @@ -67,7 +85,88 @@ class SmbFsTestCase(test.TestCase): return_value=self._FAKE_MNT_POINT) self._smbfs_driver._execute = mock.Mock() self._smbfs_driver.base = self._FAKE_MNT_BASE + self._smbfs_driver._alloc_info_file_path = ( + self._FAKE_ALLOCATION_DATA_PATH) + + def _get_fake_allocation_data(self): + return {self._FAKE_SHARE_HASH: { + 'total_allocated': self._FAKE_TOTAL_ALLOCATED}} + + @mock.patch.object(smbfs, 'open', create=True) + @mock.patch('os.path.exists') + @mock.patch.object(fileutils, 'ensure_tree') + @mock.patch('json.load') + def _test_setup_allocation_data(self, mock_json_load, mock_ensure_tree, + mock_exists, mock_open, + allocation_data_exists=False): + mock_exists.return_value = allocation_data_exists + self._smbfs_driver._update_allocation_data_file = mock.Mock() + + self._smbfs_driver._setup_allocation_data() + + if allocation_data_exists: + fd = mock_open.return_value.__enter__.return_value + mock_json_load.assert_called_once_with(fd) + self.assertEqual(mock_json_load.return_value, + self._smbfs_driver._allocation_data) + else: + mock_ensure_tree.assert_called_once_with( + os.path.dirname(self._FAKE_ALLOCATION_DATA_PATH)) + update_func = self._smbfs_driver._update_allocation_data_file + update_func.assert_called_once_with() + + def test_setup_allocation_data_file_unexisting(self): + self._test_setup_allocation_data() + + def test_setup_allocation_data_file_existing(self): + self._test_setup_allocation_data(allocation_data_exists=True) + + def _test_update_allocation_data(self, virtual_size_gb=None, + volume_exists=True): + self._smbfs_driver._update_allocation_data_file = mock.Mock() + update_func = self._smbfs_driver._update_allocation_data_file + + fake_alloc_data = self._get_fake_allocation_data() + if volume_exists: + fake_alloc_data[self._FAKE_SHARE_HASH][ + self._FAKE_VOLUME_NAME] = self._FAKE_VOLUME['size'] + + self._smbfs_driver._allocation_data = fake_alloc_data + self._smbfs_driver.update_disk_allocation_data(self._FAKE_VOLUME, + virtual_size_gb) + + vol_allocated_size = fake_alloc_data[self._FAKE_SHARE_HASH].get( + self._FAKE_VOLUME_NAME, None) + if not virtual_size_gb: + expected_total_allocated = (self._FAKE_TOTAL_ALLOCATED - + self._FAKE_VOLUME['size']) + + self.assertIsNone(vol_allocated_size) + else: + expected_total_allocated = (self._FAKE_TOTAL_ALLOCATED + + virtual_size_gb - + self._FAKE_VOLUME['size']) + self.assertEqual(virtual_size_gb, vol_allocated_size) + + update_func.assert_called_once_with() + + self.assertEqual( + expected_total_allocated, + fake_alloc_data[self._FAKE_SHARE_HASH]['total_allocated']) + + def test_update_allocation_data_volume_deleted(self): + self._test_update_allocation_data() + + def test_update_allocation_data_volume_extended(self): + self._test_update_allocation_data( + virtual_size_gb=self._FAKE_VOLUME['size'] + 1) + + def test_update_allocation_data_volume_created(self): + self._test_update_allocation_data( + virtual_size_gb=self._FAKE_VOLUME['size']) + + @requires_allocation_data_update(expected_size=None) def test_delete_volume(self): drv = self._smbfs_driver fake_vol_info = self._FAKE_VOLUME_PATH + '.info' @@ -208,9 +307,8 @@ class SmbFsTestCase(test.TestCase): self._smbfs_driver._mounted_shares = mounted_shares self._smbfs_driver._is_share_eligible = mock.Mock( return_value=eligible_shares) - fake_capacity_info = ((2, 1, 5), (2, 1, 4), (2, 1, 1)) - self._smbfs_driver._get_capacity_info = mock.Mock( - side_effect=fake_capacity_info) + self._smbfs_driver._get_total_allocated = mock.Mock( + side_effect=[3, 2, 1]) if not mounted_shares: self.assertRaises(exception.SmbfsNoSharesMounted, @@ -425,12 +523,11 @@ class SmbFsTestCase(test.TestCase): fake_convert: if extend_failed: self.assertRaises(exception.ExtendVolumeError, - drv._extend_volume, + drv.extend_volume, self._FAKE_VOLUME, mock.sentinel.new_size) else: - drv._extend_volume( - self._FAKE_VOLUME, - mock.sentinel.new_size) + drv.extend_volume(self._FAKE_VOLUME, mock.sentinel.new_size) + if image_format in (drv._DISK_FORMAT_VHDX, drv._DISK_FORMAT_VHD_LEGACY): fake_tmp_path = self._FAKE_VOLUME_PATH + '.tmp' @@ -445,12 +542,14 @@ class SmbFsTestCase(test.TestCase): fake_resize.assert_called_once_with( self._FAKE_VOLUME_PATH, mock.sentinel.new_size) + @requires_allocation_data_update(expected_size=mock.sentinel.new_size) def test_extend_volume(self): self._test_extend_volume() def test_extend_volume_failed(self): self._test_extend_volume(extend_failed=True) + @requires_allocation_data_update(expected_size=mock.sentinel.new_size) def test_extend_vhd_volume(self): self._test_extend_volume(image_format='vpc') @@ -492,6 +591,29 @@ class SmbFsTestCase(test.TestCase): def test_check_extend_volume_uneligible_share(self): self._test_check_extend_support(is_eligible=False) + @requires_allocation_data_update(expected_size=_FAKE_VOLUME['size']) + @mock.patch.object(remotefs.RemoteFSSnapDriver, 'create_volume') + def test_create_volume_base(self, mock_create_volume): + self._smbfs_driver.create_volume(self._FAKE_VOLUME) + mock_create_volume.assert_called_once_with(self._FAKE_VOLUME) + + @requires_allocation_data_update(expected_size=_FAKE_VOLUME['size']) + @mock.patch.object(smbfs.SmbfsDriver, + '_create_volume_from_snapshot') + def test_create_volume_from_snapshot(self, mock_create_volume): + self._smbfs_driver.create_volume_from_snapshot(self._FAKE_VOLUME, + self._FAKE_SNAPSHOT) + mock_create_volume.assert_called_once_with(self._FAKE_VOLUME, + self._FAKE_SNAPSHOT) + + @requires_allocation_data_update(expected_size=_FAKE_VOLUME['size']) + @mock.patch.object(smbfs.SmbfsDriver, '_create_cloned_volume') + def test_create_cloned_volume(self, mock_create_volume): + self._smbfs_driver.create_cloned_volume(self._FAKE_VOLUME, + mock.sentinel.src_vol) + mock_create_volume.assert_called_once_with(self._FAKE_VOLUME, + mock.sentinel.src_vol) + def test_create_volume_from_in_use_snapshot(self): fake_snapshot = {'status': 'in-use'} self.assertRaises( @@ -597,19 +719,18 @@ class SmbFsTestCase(test.TestCase): fake_block_size = 4096.0 fake_total_blocks = 1024 fake_avail_blocks = 512 - fake_total_allocated = fake_total_blocks * fake_block_size fake_df = ('%s %s %s' % (fake_block_size, fake_total_blocks, fake_avail_blocks), None) - fake_du = (str(fake_total_allocated), None) self._smbfs_driver._get_mount_point_for_share = mock.Mock( return_value=self._FAKE_MNT_POINT) - self._smbfs_driver._execute = mock.Mock( - side_effect=(fake_df, fake_du)) + self._smbfs_driver._get_total_allocated = mock.Mock( + return_value=self._FAKE_TOTAL_ALLOCATED) + self._smbfs_driver._execute.return_value = fake_df ret_val = self._smbfs_driver._get_capacity_info(self._FAKE_SHARE) expected = (fake_block_size * fake_total_blocks, fake_block_size * fake_avail_blocks, - fake_total_allocated) + self._FAKE_TOTAL_ALLOCATED) self.assertEqual(expected, ret_val) diff --git a/cinder/tests/unit/windows/test_smbfs.py b/cinder/tests/unit/windows/test_smbfs.py index b02bbf22a..caff008ed 100644 --- a/cinder/tests/unit/windows/test_smbfs.py +++ b/cinder/tests/unit/windows/test_smbfs.py @@ -30,8 +30,6 @@ class WindowsSmbFsTestCase(test.TestCase): _FAKE_MNT_POINT = os.path.join(_FAKE_MNT_BASE, 'fake_hash') _FAKE_VOLUME_NAME = 'volume-4f711859-4928-4cb7-801a-a50c37ceaccc' _FAKE_SNAPSHOT_NAME = _FAKE_VOLUME_NAME + '-snapshot.vhdx' - _FAKE_VOLUME_PATH = os.path.join(_FAKE_MNT_POINT, - _FAKE_VOLUME_NAME) _FAKE_SNAPSHOT_PATH = os.path.join(_FAKE_MNT_POINT, _FAKE_SNAPSHOT_NAME) _FAKE_TOTAL_SIZE = '2048' @@ -46,8 +44,6 @@ class WindowsSmbFsTestCase(test.TestCase): _FAKE_SHARE_OPTS = '-o username=Administrator,password=12345' _FAKE_VOLUME_PATH = os.path.join(_FAKE_MNT_POINT, _FAKE_VOLUME_NAME + '.vhdx') - _FAKE_LISTDIR = [_FAKE_VOLUME_NAME + '.vhd', - _FAKE_VOLUME_NAME + '.vhdx', 'fake_folder'] def setUp(self): super(WindowsSmbFsTestCase, self).setUp() @@ -105,24 +101,6 @@ class WindowsSmbFsTestCase(test.TestCase): self._FAKE_TOTAL_ALLOCATED]] self.assertEqual(expected_ret_val, ret_val) - def test_get_total_allocated(self): - fake_listdir = mock.Mock(side_effect=[self._FAKE_LISTDIR, - self._FAKE_LISTDIR[:-1]]) - fake_folder_path = os.path.join(self._FAKE_SHARE, 'fake_folder') - fake_isdir = lambda x: x == fake_folder_path - self._smbfs_driver._remotefsclient.is_symlink = mock.Mock( - return_value=False) - fake_getsize = mock.Mock(return_value=self._FAKE_VOLUME['size']) - self._smbfs_driver.vhdutils.get_vhd_size = mock.Mock( - return_value={'VirtualSize': 1}) - - with mock.patch.multiple('os.path', isdir=fake_isdir, - getsize=fake_getsize): - with mock.patch('os.listdir', fake_listdir): - ret_val = self._smbfs_driver._get_total_allocated( - self._FAKE_SHARE) - self.assertEqual(4, ret_val) - def _test_get_img_info(self, backing_file=None): self._smbfs_driver.vhdutils.get_vhd_parent_path.return_value = ( backing_file) diff --git a/cinder/volume/drivers/smbfs.py b/cinder/volume/drivers/smbfs.py index 312b922a8..596403883 100644 --- a/cinder/volume/drivers/smbfs.py +++ b/cinder/volume/drivers/smbfs.py @@ -13,12 +13,17 @@ # License for the specific language governing permissions and limitations # under the License. +import decorator + +import inspect +import json import os from os_brick.remotefs import remotefs from oslo_concurrency import processutils as putils from oslo_config import cfg from oslo_log import log as logging +from oslo_utils import fileutils from oslo_utils import units from cinder import exception @@ -36,6 +41,10 @@ volume_opts = [ cfg.StrOpt('smbfs_shares_config', default='/etc/cinder/smbfs_shares', help='File with the list of available smbfs shares.'), + cfg.StrOpt('smbfs_allocation_info_file_path', + default='$state_path/allocation_data', + help=('The path of the automatically generated file containing ' + 'information about volume disk space allocation.')), cfg.StrOpt('smbfs_default_volume_format', default='qcow2', choices=['raw', 'qcow2', 'vhd', 'vhdx'], @@ -69,6 +78,25 @@ CONF = cfg.CONF CONF.register_opts(volume_opts) +def update_allocation_data(delete=False): + @decorator.decorator + def wrapper(func, inst, *args, **kwargs): + ret_val = func(inst, *args, **kwargs) + + call_args = inspect.getcallargs(func, inst, *args, **kwargs) + volume = call_args['volume'] + requested_size = call_args.get('size_gb', None) + + if delete: + allocated_size_gb = None + else: + allocated_size_gb = requested_size or volume['size'] + + inst.update_disk_allocation_data(volume, allocated_size_gb) + return ret_val + return wrapper + + class SmbfsDriver(remotefs_drv.RemoteFSSnapDriver): """SMBFS based cinder volume driver.""" @@ -100,6 +128,7 @@ class SmbfsDriver(remotefs_drv.RemoteFSSnapDriver): smbfs_mount_point_base=self.base, smbfs_mount_options=opts) self.img_suffix = None + self._alloc_info_file_path = CONF.smbfs_allocation_info_file_path def _qemu_img_info(self, path, volume_name): return super(SmbfsDriver, self)._qemu_img_info_base( @@ -163,6 +192,51 @@ class SmbfsDriver(remotefs_drv.RemoteFSSnapDriver): self.shares = {} # address : options self._ensure_shares_mounted() + self._setup_allocation_data() + + def _setup_allocation_data(self): + if not os.path.exists(self._alloc_info_file_path): + fileutils.ensure_tree( + os.path.dirname(self._alloc_info_file_path)) + self._allocation_data = {} + self._update_allocation_data_file() + else: + with open(self._alloc_info_file_path, 'r') as f: + self._allocation_data = json.load(f) + + def update_disk_allocation_data(self, volume, virtual_size_gb=None): + volume_name = volume['name'] + smbfs_share = volume['provider_location'] + if smbfs_share: + share_hash = self._get_hash_str(smbfs_share) + else: + return + + share_alloc_data = self._allocation_data.get(share_hash, {}) + old_virtual_size = share_alloc_data.get(volume_name, 0) + total_allocated = share_alloc_data.get('total_allocated', 0) + + if virtual_size_gb: + share_alloc_data[volume_name] = virtual_size_gb + total_allocated += virtual_size_gb - old_virtual_size + elif share_alloc_data.get(volume_name): + # The volume is deleted. + del share_alloc_data[volume_name] + total_allocated -= old_virtual_size + + share_alloc_data['total_allocated'] = total_allocated + self._allocation_data[share_hash] = share_alloc_data + self._update_allocation_data_file() + + def _update_allocation_data_file(self): + with open(self._alloc_info_file_path, 'w') as f: + json.dump(self._allocation_data, f) + + def _get_total_allocated(self, smbfs_share): + share_hash = self._get_hash_str(smbfs_share) + share_alloc_data = self._allocation_data.get(share_hash, {}) + total_allocated = share_alloc_data.get('total_allocated', 0) << 30 + return float(total_allocated) def local_path(self, volume): """Get volume path (mounted locally fs path) for given volume. @@ -224,6 +298,7 @@ class SmbfsDriver(remotefs_drv.RemoteFSSnapDriver): return volume_format @remotefs_drv.locked_volume_id_operation + @update_allocation_data(delete=True) def delete_volume(self, volume): """Deletes a logical volume.""" if not volume['provider_location']: @@ -254,6 +329,11 @@ class SmbfsDriver(remotefs_drv.RemoteFSSnapDriver): volume_path, str(volume_size * units.Gi), run_as_root=True) + @remotefs_drv.locked_volume_id_operation + @update_allocation_data() + def create_volume(self, volume): + return super(SmbfsDriver, self).create_volume(volume) + def _do_create_volume(self, volume): """Create a volume on given smbfs_share. @@ -298,9 +378,7 @@ class SmbfsDriver(remotefs_drv.RemoteFSSnapDriver): total_available = block_size * blocks_avail total_size = block_size * blocks_total - du, _ = self._execute('du', '-sb', '--apparent-size', '--exclude', - '*snapshot*', mount_point, run_as_root=True) - total_allocated = float(du.split()[0]) + total_allocated = self._get_total_allocated(smbfs_share) return total_size, total_available, total_allocated def _find_share(self, volume_size_in_gib): @@ -321,7 +399,7 @@ class SmbfsDriver(remotefs_drv.RemoteFSSnapDriver): for smbfs_share in self._mounted_shares: if not self._is_share_eligible(smbfs_share, volume_size_in_gib): continue - total_allocated = self._get_capacity_info(smbfs_share)[2] + total_allocated = self._get_total_allocated(smbfs_share) if target_share is not None: if target_share_reserved > total_allocated: target_share = smbfs_share @@ -398,6 +476,7 @@ class SmbfsDriver(remotefs_drv.RemoteFSSnapDriver): raise exception.InvalidVolume(err_msg) @remotefs_drv.locked_volume_id_operation + @update_allocation_data() def extend_volume(self, volume, size_gb): LOG.info(_LI('Extending volume %s.'), volume['id']) self._extend_volume(volume, size_gb) @@ -449,6 +528,11 @@ class SmbfsDriver(remotefs_drv.RemoteFSSnapDriver): 'extend volume %s to %sG.' % (volume['id'], size_gb)) + @remotefs_drv.locked_volume_id_operation + @update_allocation_data() + def create_volume_from_snapshot(self, volume, snapshot): + return self._create_volume_from_snapshot(volume, snapshot) + def _copy_volume_from_snapshot(self, snapshot, volume, volume_size): """Copy data from snapshot to destination volume. @@ -506,6 +590,12 @@ class SmbfsDriver(remotefs_drv.RemoteFSSnapDriver): reason=(_("Expected volume size was %d") % volume['size']) + (_(" but size is now %d.") % virt_size)) + @remotefs_drv.locked_volume_id_operation + @update_allocation_data() + def create_cloned_volume(self, volume, src_vref): + """Creates a clone of the specified volume.""" + return self._create_cloned_volume(volume, src_vref) + def _ensure_share_mounted(self, smbfs_share): mnt_flags = [] if self.shares.get(smbfs_share) is not None: diff --git a/cinder/volume/drivers/windows/smbfs.py b/cinder/volume/drivers/windows/smbfs.py index eb31ab4c1..37844d0d1 100644 --- a/cinder/volume/drivers/windows/smbfs.py +++ b/cinder/volume/drivers/windows/smbfs.py @@ -15,7 +15,6 @@ import os -import re import sys from oslo_config import cfg @@ -26,7 +25,7 @@ from oslo_utils import units from cinder import exception from cinder.i18n import _, _LI from cinder.image import image_utils -from cinder import utils +from cinder.volume.drivers import remotefs as remotefs_drv from cinder.volume.drivers import smbfs from cinder.volume.drivers.windows import remotefs from cinder.volume.drivers.windows import vhdutils @@ -38,6 +37,8 @@ LOG = logging.getLogger(__name__) CONF = cfg.CONF CONF.set_default('smbfs_shares_config', r'C:\OpenStack\smbfs_shares.txt') +CONF.set_default('smbfs_allocation_info_file_path', + r'C:\OpenStack\allocation_data.txt') CONF.set_default('smbfs_mount_point_base', r'C:\OpenStack\_mnt') CONF.set_default('smbfs_default_volume_format', 'vhd') @@ -111,25 +112,6 @@ class WindowsSmbfsDriver(smbfs.SmbfsDriver): 'allocated': total_allocated}) return [float(x) for x in return_value] - def _get_total_allocated(self, smbfs_share): - elements = os.listdir(smbfs_share) - total_allocated = 0 - for element in elements: - element_path = os.path.join(smbfs_share, element) - if not self._remotefsclient.is_symlink(element_path): - if "snapshot" in element: - continue - if re.search(r'\.vhdx?$', element): - total_allocated += self.vhdutils.get_vhd_size( - element_path)['VirtualSize'] - continue - if os.path.isdir(element_path): - total_allocated += self._get_total_allocated(element_path) - continue - total_allocated += os.path.getsize(element_path) - - return total_allocated - def _img_commit(self, snapshot_path): self.vhdutils.merge_vhd(snapshot_path) self._delete(snapshot_path) @@ -172,7 +154,7 @@ class WindowsSmbfsDriver(smbfs.SmbfsDriver): def _do_extend_volume(self, volume_path, size_gb, volume_name=None): self.vhdutils.resize_vhd(volume_path, size_gb * units.Gi) - @utils.synchronized('smbfs', external=False) + @remotefs_drv.locked_volume_id_operation def copy_volume_to_image(self, context, volume, image_service, image_meta): """Copy the volume to the specified image."""