From 792da5dbbf854a3f23414cf4c53babd44db033cf Mon Sep 17 00:00:00 2001 From: Lucian Petrut Date: Thu, 11 May 2017 14:22:19 +0300 Subject: [PATCH] SMBFS: drop JSON file storing allocation data Due to the fact that we cannot query in-use images, some time ago we've started storing the total disk image allocated size within a JSON file. At the same time, we're caching those values within the service. As we're aiming to support A-A deployments, also considering the risk of having this file corrupted, we're now dropping it. Instead of using a separate DB, we're just going to rely on the Cinder DB when retrieving the allocated space for a specific share. One reason why we did not go with this approach from the beginning is the fact that we needed support for reporting each share as a pool, which is accomplished by the other patches from this chain. Change-Id: I2085c040d7140f8ba685f6a36c4d3b5b30b14103 --- cinder/tests/unit/windows/test_smbfs.py | 135 +++--------------- cinder/volume/drivers/windows/smbfs.py | 95 ++---------- ...drop-alloc-data-file-8b94da952a3b1548.yaml | 6 + 3 files changed, 40 insertions(+), 196 deletions(-) create mode 100644 releasenotes/notes/smbfs-drop-alloc-data-file-8b94da952a3b1548.yaml diff --git a/cinder/tests/unit/windows/test_smbfs.py b/cinder/tests/unit/windows/test_smbfs.py index 93b449f90aa..4837e6b95fb 100644 --- a/cinder/tests/unit/windows/test_smbfs.py +++ b/cinder/tests/unit/windows/test_smbfs.py @@ -13,7 +13,6 @@ # under the License. import copy -import functools import os import ddt @@ -31,20 +30,6 @@ from cinder.volume.drivers import remotefs from cinder.volume.drivers.windows 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.volume, - expected_size) - return inner - return wrapper - - @ddt.ddt class WindowsSmbFsTestCase(test.TestCase): @@ -66,8 +51,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) - _FAKE_ALLOCATION_DATA_PATH = os.path.join('fake_dir', - 'fake_allocation_data') _FAKE_SHARE_OPTS = '-o username=Administrator,password=12345' @mock.patch.object(smbfs, 'utilsfactory') @@ -90,8 +73,6 @@ class WindowsSmbFsTestCase(test.TestCase): self._smbfs_driver._local_volume_dir = mock.Mock( return_value=self._FAKE_MNT_POINT) self._smbfs_driver.base = self._FAKE_MNT_BASE - self._smbfs_driver._alloc_info_file_path = ( - self._FAKE_ALLOCATION_DATA_PATH) self._vhdutils = self._smbfs_driver._vhdutils @@ -118,7 +99,6 @@ class WindowsSmbFsTestCase(test.TestCase): return snapshot @mock.patch.object(smbfs.WindowsSmbfsDriver, '_check_os_platform') - @mock.patch.object(smbfs.WindowsSmbfsDriver, '_setup_allocation_data') @mock.patch.object(remotefs.RemoteFSSnapDriver, 'do_setup') @mock.patch('os.path.exists') @mock.patch('os.path.isabs') @@ -126,7 +106,7 @@ class WindowsSmbFsTestCase(test.TestCase): def _test_setup(self, mock_check_qemu_img_version, mock_is_abs, mock_exists, mock_remotefs_do_setup, - mock_setup_alloc_data, mock_check_os_platform, + mock_check_os_platform, config, share_config_exists=True): mock_exists.return_value = share_config_exists fake_ensure_mounted = mock.MagicMock() @@ -148,7 +128,6 @@ class WindowsSmbFsTestCase(test.TestCase): mock_is_abs.assert_called_once_with(self._smbfs_driver.base) self.assertEqual({}, self._smbfs_driver.shares) fake_ensure_mounted.assert_called_once_with() - mock_setup_alloc_data.assert_called_once_with() self._smbfs_driver._setup_pool_mappings.assert_called_once_with() mock_check_os_platform.assert_called_once_with() @@ -230,83 +209,32 @@ class WindowsSmbFsTestCase(test.TestCase): fake_config.smbfs_used_ratio = 1.1 self._test_setup(config=fake_config) - @mock.patch.object(smbfs, 'open', create=True) - @mock.patch('os.path.exists') - @mock.patch.object(smbfs.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() + @mock.patch.object(smbfs, 'context') + @mock.patch.object(smbfs.WindowsSmbfsDriver, + '_get_pool_name_from_share') + def test_get_total_allocated(self, mock_get_pool_name, mock_ctxt): + fake_pool_name = 'pool0' + fake_host_name = 'fake_host@fake_backend' + fake_vol_sz_sum = 5 - self._smbfs_driver._setup_allocation_data() + mock_db = mock.Mock() + mock_db.volume_data_get_for_host.return_value = [ + mock.sentinel.vol_count, fake_vol_sz_sum] - 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() + self._smbfs_driver.host = fake_host_name + self._smbfs_driver.db = mock_db - def test_setup_allocation_data_file_unexisting(self): - self._test_setup_allocation_data() + mock_get_pool_name.return_value = fake_pool_name - def test_setup_allocation_data_file_existing(self): - self._test_setup_allocation_data(allocation_data_exists=True) + allocated = self._smbfs_driver._get_total_allocated( + mock.sentinel.share) + self.assertEqual(fake_vol_sz_sum << 30, + allocated) - 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._FAKE_SHARE_HASH: { - 'total_allocated': self._FAKE_TOTAL_ALLOCATED}} - if volume_exists: - fake_alloc_data[self._FAKE_SHARE_HASH][ - self.volume.name] = self.volume.size - - self._smbfs_driver._allocation_data = fake_alloc_data - - self._smbfs_driver.update_disk_allocation_data(self.volume, - virtual_size_gb) - - vol_allocated_size = fake_alloc_data[self._FAKE_SHARE_HASH].get( - self.volume.name, None) - if not virtual_size_gb: - expected_total_allocated = (self._FAKE_TOTAL_ALLOCATED - - self.volume.size) - - self.assertIsNone(vol_allocated_size) - else: - exp_added = (self.volume.size if not volume_exists - else virtual_size_gb - self.volume.size) - expected_total_allocated = (self._FAKE_TOTAL_ALLOCATED + - exp_added) - 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.volume.size + 1) - - def test_update_allocation_data_volume_created(self): - self._test_update_allocation_data( - virtual_size_gb=self.volume.size, - volume_exists=False) + mock_get_pool_name.assert_called_once_with(mock.sentinel.share) + mock_db.volume_data_get_for_host.assert_called_once_with( + context=mock_ctxt.get_admin_context.return_value, + host='fake_host@fake_backend#pool0') def _test_is_share_eligible(self, capacity_info, volume_size): self._smbfs_driver._get_capacity_info = mock.Mock( @@ -494,7 +422,6 @@ class WindowsSmbFsTestCase(test.TestCase): self.assertEqual(expected_fmt, resulted_fmt) - @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.volume) @@ -527,7 +454,6 @@ class WindowsSmbFsTestCase(test.TestCase): def test_create_volume_invalid_volume(self): self._test_create_volume(volume_format="qcow") - @requires_allocation_data_update(expected_size=None) def test_delete_volume(self): drv = self._smbfs_driver fake_vol_info = self._FAKE_VOLUME_PATH + '.info' @@ -684,23 +610,6 @@ class WindowsSmbFsTestCase(test.TestCase): mock_nova_assisted_snap_del.assert_not_called() mock_write_info_file.assert_not_called() - @requires_allocation_data_update(expected_size=_FAKE_VOLUME_SIZE) - @mock.patch.object(smbfs.WindowsSmbfsDriver, - '_create_volume_from_snapshot') - def test_create_volume_from_snapshot(self, mock_create_volume): - self._smbfs_driver.create_volume_from_snapshot(self.volume, - self.snapshot) - mock_create_volume.assert_called_once_with(self.volume, - self.snapshot) - - @requires_allocation_data_update(expected_size=_FAKE_VOLUME_SIZE) - @mock.patch.object(smbfs.WindowsSmbfsDriver, '_create_cloned_volume') - def test_create_cloned_volume(self, mock_create_volume): - self._smbfs_driver.create_cloned_volume(self.volume, - mock.sentinel.src_vol) - mock_create_volume.assert_called_once_with(self.volume, - mock.sentinel.src_vol) - def test_create_volume_from_unavailable_snapshot(self): self.snapshot.status = fields.SnapshotStatus.ERROR self.assertRaises( diff --git a/cinder/volume/drivers/windows/smbfs.py b/cinder/volume/drivers/windows/smbfs.py index 32d2da15bbc..3da2f020d5a 100644 --- a/cinder/volume/drivers/windows/smbfs.py +++ b/cinder/volume/drivers/windows/smbfs.py @@ -13,12 +13,9 @@ # License for the specific language governing permissions and limitations # under the License. -import inspect -import json import os import sys -import decorator from os_brick.remotefs import windows_remotefs as remotefs_brick from os_win import utilsfactory from oslo_config import cfg @@ -26,6 +23,7 @@ from oslo_log import log as logging from oslo_utils import fileutils from oslo_utils import units +from cinder import context from cinder import exception from cinder.i18n import _ from cinder.image import image_utils @@ -43,7 +41,10 @@ volume_opts = [ cfg.StrOpt('smbfs_allocation_info_file_path', default=r'C:\OpenStack\allocation_data.txt', help=('The path of the automatically generated file containing ' - 'information about volume disk space allocation.')), + 'information about volume disk space allocation.'), + deprecated_for_removal=True, + deprecated_since="11.0.0", + deprecated_reason="This allocation file is no longer used."), cfg.StrOpt('smbfs_default_volume_format', default='vhd', choices=['vhd', 'vhdx'], @@ -79,25 +80,6 @@ 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 - - @interface.volumedriver class WindowsSmbfsDriver(remotefs_drv.RemoteFSPoolMixin, remotefs_drv.RemoteFSSnapDriver): @@ -138,9 +120,6 @@ class WindowsSmbfsDriver(remotefs_drv.RemoteFSPoolMixin, self._pathutils = utilsfactory.get_pathutils() self._smbutils = utilsfactory.get_smbutils() - self._alloc_info_file_path = ( - self.configuration.smbfs_allocation_info_file_path) - def do_setup(self, context): self._check_os_platform() super(WindowsSmbfsDriver, self).do_setup(context) @@ -177,7 +156,6 @@ class WindowsSmbfsDriver(remotefs_drv.RemoteFSPoolMixin, self.shares = {} # address : options self._ensure_shares_mounted() - self._setup_allocation_data() self._setup_pool_mappings() def _setup_pool_mappings(self): @@ -230,49 +208,14 @@ class WindowsSmbfsDriver(remotefs_drv.RemoteFSPoolMixin, "driver supports only Win32 platforms.") % sys.platform raise exception.SmbfsException(_msg) - 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) + pool_name = self._get_pool_name_from_share(smbfs_share) + host = "#".join([self.host, pool_name]) + + vol_sz_sum = self.db.volume_data_get_for_host( + context=context.get_admin_context(), + host=host)[1] + return float(vol_sz_sum * units.Gi) def _is_share_eligible(self, smbfs_share, volume_size_in_gib): """Verifies SMBFS share is eligible to host volume with given size. @@ -382,7 +325,6 @@ class WindowsSmbfsDriver(remotefs_drv.RemoteFSPoolMixin, self.configuration.smbfs_default_volume_format) @remotefs_drv.locked_volume_id_operation - @update_allocation_data() def create_volume(self, volume): return super(WindowsSmbfsDriver, self).create_volume(volume) @@ -408,7 +350,6 @@ class WindowsSmbfsDriver(remotefs_drv.RemoteFSPoolMixin, self._remotefsclient.mount(smbfs_share, mnt_flags) @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: @@ -491,7 +432,6 @@ class WindowsSmbfsDriver(remotefs_drv.RemoteFSPoolMixin, backing_file_full_path) @remotefs_drv.locked_volume_id_operation - @update_allocation_data() def extend_volume(self, volume, size_gb): LOG.info('Extending volume %s.', volume.id) @@ -616,17 +556,6 @@ class WindowsSmbfsDriver(remotefs_drv.RemoteFSPoolMixin, volume.size * units.Gi, is_file_max_size=False) - @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) - - @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 _copy_volume_from_snapshot(self, snapshot, volume, volume_size): """Copy data from snapshot to destination volume.""" diff --git a/releasenotes/notes/smbfs-drop-alloc-data-file-8b94da952a3b1548.yaml b/releasenotes/notes/smbfs-drop-alloc-data-file-8b94da952a3b1548.yaml new file mode 100644 index 00000000000..5f1b570c3e5 --- /dev/null +++ b/releasenotes/notes/smbfs-drop-alloc-data-file-8b94da952a3b1548.yaml @@ -0,0 +1,6 @@ +--- +deprecations: + - | + The 'smbfs_allocation_info_file_path' SMBFS driver config option is now + deprecated as we're no longer using a JSON file to store volume allocation + data. This file had a considerable chance of getting corrupted.