From cc97cfe8e02716be33d3c6c3980a2bf1bce73dac Mon Sep 17 00:00:00 2001 From: Igor Malinovskiy Date: Mon, 20 Jul 2015 18:11:08 +0300 Subject: [PATCH] Implement shrink_share() method in Generic driver - Add implementation for shrink_share() method - Add methods to generic driver helpers: - disable_access_for_maintenance() - restore_access_after_maintenance() - Add appropriate unit tests - Add appropriate tempest tests - Update generic driver documentation Partially implements bp shrink-share-in-generic-driver Change-Id: I256db947b65f66dfe514e0c0b350c1949da7f7be --- .../tempest/api/share/test_shares_actions.py | 16 ++ .../api/share/test_shares_actions_negative.py | 44 ++++ contrib/tempest/tempest/config_share.py | 15 +- .../services/share/json/shares_client.py | 11 + doc/source/devref/generic_driver.rst | 4 + manila/share/drivers/generic.py | 144 +++++++++++- manila/tests/share/drivers/test_generic.py | 222 +++++++++++++++++- 7 files changed, 444 insertions(+), 12 deletions(-) diff --git a/contrib/tempest/tempest/api/share/test_shares_actions.py b/contrib/tempest/tempest/api/share/test_shares_actions.py index a4806a64fd..923c7d81ce 100644 --- a/contrib/tempest/tempest/api/share/test_shares_actions.py +++ b/contrib/tempest/tempest/api/share/test_shares_actions.py @@ -394,6 +394,22 @@ class SharesActionsTest(base.BaseSharesTest): share = self.shares_client.get_share(share['id']) self.assertEqual(new_size, share['size']) + @test.attr(type=["gate", ]) + @testtools.skipUnless( + CONF.share.run_shrink_tests, + "Share shrink tests are disabled.") + def test_shrink_share(self): + share = self.create_share(size=2, cleanup_in_class=False) + new_size = 1 + + # shrink share and wait for active status + self.shares_client.shrink_share(share['id'], new_size) + self.shares_client.wait_for_share_status(share['id'], 'available') + + # check state and new size + share = self.shares_client.get_share(share['id']) + self.assertEqual(new_size, share['size']) + class SharesRenameTest(base.BaseSharesTest): diff --git a/contrib/tempest/tempest/api/share/test_shares_actions_negative.py b/contrib/tempest/tempest/api/share/test_shares_actions_negative.py index cdd664e4dd..512849cc86 100644 --- a/contrib/tempest/tempest/api/share/test_shares_actions_negative.py +++ b/contrib/tempest/tempest/api/share/test_shares_actions_negative.py @@ -89,4 +89,48 @@ class SharesActionsNegativeTest(base.BaseSharesTest): self.assertRaises(lib_exc.BadRequest, self.shares_client.extend_share, share['id'], + new_size) + + @test.attr(type=["negative", ]) + @testtools.skipUnless( + CONF.share.run_shrink_tests, + "Share shrink tests are disabled.") + def test_share_shrink_with_greater_size(self): + new_size = int(self.share['size']) + 1 + + # shrink share with invalid size and check result + self.assertRaises(lib_exc.BadRequest, + self.shares_client.shrink_share, + self.share['id'], + new_size) + + @test.attr(type=["negative", ]) + @testtools.skipUnless( + CONF.share.run_shrink_tests, + "Share shrink tests are disabled.") + def test_share_shrink_with_same_size(self): + new_size = int(self.share['size']) + + # shrink share with invalid size and check result + self.assertRaises(lib_exc.BadRequest, + self.shares_client.shrink_share, + self.share['id'], + new_size) + + @test.attr(type=["negative", ]) + @testtools.skipUnless( + CONF.share.run_shrink_tests, + "Share shrink tests are disabled.") + def test_share_shrink_with_invalid_share_state(self): + share = self.create_share(size=2, cleanup_in_class=False) + new_size = int(share['size']) - 1 + + # set "error" state + admin_client = clients.AdminManager().shares_client + admin_client.reset_state(share['id']) + + # run shrink operation on same share and check result + self.assertRaises(lib_exc.BadRequest, + self.shares_client.shrink_share, + share['id'], new_size) \ No newline at end of file diff --git a/contrib/tempest/tempest/config_share.py b/contrib/tempest/tempest/config_share.py index 5d916d8a41..d419210329 100644 --- a/contrib/tempest/tempest/config_share.py +++ b/contrib/tempest/tempest/config_share.py @@ -113,6 +113,16 @@ ShareGroup = [ help="Defines whether to run manage/unmanage tests or not. " "These test may leave orphaned resources, so be careful " "enabling this opt."), + cfg.BoolOpt("run_extend_tests", + default=True, + help="Defines whether to run share extend tests or not. " + "Disable this feature if used driver doesn't " + "support it."), + cfg.BoolOpt("run_shrink_tests", + default=True, + help="Defines whether to run share shrink tests or not. " + "Disable this feature if used driver doesn't " + "support it."), cfg.StrOpt("image_with_share_tools", default="manila-service-image", help="Image name for vm booting with nfs/smb clients tool."), @@ -125,11 +135,6 @@ ShareGroup = [ cfg.StrOpt("client_vm_flavor_ref", default="100", help="Flavor used for client vm in scenario tests."), - cfg.BoolOpt("run_extend_tests", - default=True, - help="Defines whether to run share extend tests or not." - "Disable this feature if used driver doesn't " - "support it."), ] diff --git a/contrib/tempest/tempest/services/share/json/shares_client.py b/contrib/tempest/tempest/services/share/json/shares_client.py index 0509325f5a..5ada329835 100644 --- a/contrib/tempest/tempest/services/share/json/shares_client.py +++ b/contrib/tempest/tempest/services/share/json/shares_client.py @@ -168,6 +168,17 @@ class SharesClient(rest_client.RestClient): self.expected_success(202, resp.status) return body + def shrink_share(self, share_id, new_size): + post_body = { + "os-shrink": { + "new_size": new_size, + } + } + body = json.dumps(post_body) + resp, body = self.post("shares/%s/action" % share_id, body) + self.expected_success(202, resp.status) + return body + def create_snapshot(self, share_id, name=None, description=None, force=False): if name is None: diff --git a/doc/source/devref/generic_driver.rst b/doc/source/devref/generic_driver.rst index fee2a4b5ee..0b94deefee 100644 --- a/doc/source/devref/generic_driver.rst +++ b/doc/source/devref/generic_driver.rst @@ -86,6 +86,10 @@ Known restrictions - Liberty version adds a share extend capability. Share access will be briefly interrupted during an extend operation. +- Liberty version adds a share shrink capability, but this capability is not + effective because generic driver shrinks only filesystem size and doesn't + shrink the size of Cinder volume. + The :mod:`manila.share.drivers.generic` Module ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ diff --git a/manila/share/drivers/generic.py b/manila/share/drivers/generic.py index 38ef81d6fa..638a90ee45 100644 --- a/manila/share/drivers/generic.py +++ b/manila/share/drivers/generic.py @@ -25,6 +25,7 @@ from oslo_log import log from oslo_utils import excutils from oslo_utils import importutils from oslo_utils import strutils +from oslo_utils import units import retrying import six @@ -88,6 +89,10 @@ share_opts = [ CONF = cfg.CONF CONF.register_opts(share_opts) +# NOTE(u_glide): These constants refer to the column number in the "df" output +BLOCK_DEVICE_SIZE_INDEX = 1 +USED_SPACE_INDEX = 2 + def ensure_server(f): @@ -578,12 +583,60 @@ class GenericShareDriver(driver.ExecuteMixin, driver.ShareDriver): msg_error=msg_error, msg_timeout=msg_timeout ) - def _resize_filesystem(self, server_details, volume): + @ensure_server + def shrink_share(self, share, new_size, share_server=None): + server_details = share_server['backend_details'] + + helper = self._get_helper(share) + export_location = share['export_locations'][0]['path'] + mount_path = helper.get_share_path_by_export_location( + server_details, export_location) + + consumed_space = self._get_consumed_space(mount_path, server_details) + + LOG.debug("Consumed space on share: %s", consumed_space) + + if consumed_space >= new_size: + raise exception.ShareShrinkingPossibleDataLoss( + share_id=share['id']) + + volume = self._get_volume(self.admin_context, share['id']) + + helper.disable_access_for_maintenance(server_details, share['name']) + self._unmount_device(share, server_details) + + try: + self._resize_filesystem(server_details, volume, new_size=new_size) + except exception.Invalid: + raise exception.ShareShrinkingPossibleDataLoss( + share_id=share['id']) + except Exception as e: + msg = _("Cannot shrink share: %s") % six.text_type(e) + raise exception.Invalid(msg) + finally: + self._mount_device(share, server_details, volume) + helper.restore_access_after_maintenance(server_details, + share['name']) + + def _resize_filesystem(self, server_details, volume, new_size=None): """Resize filesystem of provided volume.""" check_command = ['sudo', 'fsck', '-pf', volume['mountpoint']] self._ssh_exec(server_details, check_command) command = ['sudo', 'resize2fs', volume['mountpoint']] - self._ssh_exec(server_details, command) + + if new_size: + command.append("%sG" % six.text_type(new_size)) + + try: + self._ssh_exec(server_details, command) + except processutils.ProcessExecutionError as e: + if e.stderr.find('New size smaller than minimum') != -1: + msg = (_("Invalid 'new_size' provided: %s") + % six.text_type(new_size)) + raise exception.Invalid(msg) + else: + msg = _("Cannot resize file-system: %s") % six.text_type(e) + raise exception.ManilaException(msg) def _is_share_server_active(self, context, share_server): """Check if the share server is active.""" @@ -818,13 +871,27 @@ class GenericShareDriver(driver.ExecuteMixin, driver.ShareDriver): server_details, old_export_location) return {'size': share_size, 'export_locations': export_locations} - def _get_mounted_share_size(self, mount_path, server_details): - share_size_cmd = ['df', '-PBG', mount_path] + def _get_mount_stats_by_index(self, mount_path, server_details, index, + block_size='G'): + """Get mount stats using df shell command. + + :param mount_path: Share path on share server + :param server_details: Share server connection details + :param index: Data index in df command output: + BLOCK_DEVICE_SIZE_INDEX - Size of block device + USED_SPACE_INDEX - Used space + :param block_size: size of block (example: G, M, Mib, etc) + :returns: value of provided index + """ + share_size_cmd = ['df', '-PB%s' % block_size, mount_path] output, __ = self._ssh_exec(server_details, share_size_cmd) lines = output.split('\n') + return int(lines[1].split()[index][:-1]) + def _get_mounted_share_size(self, mount_path, server_details): try: - size = int(lines[1].split()[1][:-1]) + size = self._get_mount_stats_by_index( + mount_path, server_details, BLOCK_DEVICE_SIZE_INDEX) except Exception as e: msg = _("Cannot calculate size of share %(path)s : %(error)s") % { 'path': mount_path, @@ -834,6 +901,21 @@ class GenericShareDriver(driver.ExecuteMixin, driver.ShareDriver): return size + def _get_consumed_space(self, mount_path, server_details): + try: + size = self._get_mount_stats_by_index( + mount_path, server_details, USED_SPACE_INDEX, block_size='M') + size /= float(units.Ki) + except Exception as e: + msg = _("Cannot calculate consumed space on share " + "%(path)s : %(error)s") % { + 'path': mount_path, + 'error': six.text_type(e) + } + raise exception.InvalidShare(reason=msg) + + return size + class NASHelperBase(object): """Interface to work with share.""" @@ -877,6 +959,16 @@ class NASHelperBase(object): """Returns share path by its export location.""" raise NotImplementedError() + def disable_access_for_maintenance(self, server, share_name): + """Disables access to share to perform maintenance operations.""" + + def restore_access_after_maintenance(self, server, share_name): + """Enables access to share after maintenance operations were done.""" + + def _get_maintenance_file_path(self, share_name): + return os.path.join(self.configuration.share_mount_path, + "%s.maintenance" % share_name) + def nfs_synchronized(f): @@ -968,6 +1060,32 @@ class NFSHelper(NASHelperBase): def get_share_path_by_export_location(self, server, export_location): return export_location.split(':')[-1] + @nfs_synchronized + def disable_access_for_maintenance(self, server, share_name): + maintenance_file = self._get_maintenance_file_path(share_name) + backup_exports = [ + 'cat', const.NFS_EXPORTS_FILE, + '| grep', share_name, + '| sudo tee', maintenance_file + ] + self._ssh_exec(server, backup_exports) + + local_path = os.path.join(self.configuration.share_mount_path, + share_name) + self._ssh_exec(server, ['sudo', 'exportfs', '-u', local_path]) + self._sync_nfs_temp_and_perm_files(server) + + @nfs_synchronized + def restore_access_after_maintenance(self, server, share_name): + maintenance_file = self._get_maintenance_file_path(share_name) + restore_exports = [ + 'cat', maintenance_file, + '| sudo tee -a', const.NFS_EXPORTS_FILE, + '&& sudo exportfs -r', + '&& sudo rm -f', maintenance_file + ] + self._ssh_exec(server, restore_exports) + class CIFSHelper(NASHelperBase): """Manage shares in samba server by net conf tool. @@ -1109,3 +1227,19 @@ class CIFSHelper(NASHelperBase): # Remove special symbols from response and return path return out.strip() + + def disable_access_for_maintenance(self, server, share_name): + maintenance_file = self._get_maintenance_file_path(share_name) + allowed_hosts = " ".join(self._get_allow_hosts(server, share_name)) + + backup_exports = [ + 'echo', "'%s'" % allowed_hosts, '| sudo tee', maintenance_file + ] + self._ssh_exec(server, backup_exports) + self._set_allow_hosts(server, [], share_name) + + def restore_access_after_maintenance(self, server, share_name): + maintenance_file = self._get_maintenance_file_path(share_name) + (exports, __) = self._ssh_exec(server, ['cat', maintenance_file]) + self._set_allow_hosts(server, exports.split(), share_name) + self._ssh_exec(server, ['sudo rm -f', maintenance_file]) diff --git a/manila/tests/share/drivers/test_generic.py b/manila/tests/share/drivers/test_generic.py index 4e5afb6ca1..fbdf0384ab 100644 --- a/manila/tests/share/drivers/test_generic.py +++ b/manila/tests/share/drivers/test_generic.py @@ -1318,6 +1318,35 @@ class GenericShareDriverTestCase(test.TestCase): self._driver._get_mounted_share_size, '/fake/path', {}) + def test_get_consumed_space(self): + mount_path = "fake_path" + server_details = {} + index = 2 + valid_result = 1 + self.mock_object(self._driver, '_get_mount_stats_by_index', + mock.Mock(return_value=valid_result * 1024)) + + actual_result = self._driver._get_consumed_space( + mount_path, server_details) + + self.assertEqual(valid_result, actual_result) + self._driver._get_mount_stats_by_index.assert_called_once_with( + mount_path, server_details, index, block_size='M' + ) + + def test_get_consumed_space_invalid(self): + self.mock_object( + self._driver, + '_get_mount_stats_by_index', + mock.Mock(side_effect=exception.ManilaException("fake")) + ) + + self.assertRaises( + exception.InvalidShare, + self._driver._get_consumed_space, + "fake", "fake" + ) + def test_extend_share(self): fake_volume = "fake" fake_share = {'id': 'fake'} @@ -1376,14 +1405,122 @@ class GenericShareDriverTestCase(test.TestCase): fake_volume = {'mountpoint': '/dev/fake'} self.mock_object(self._driver, '_ssh_exec') - self._driver._resize_filesystem(fake_server_details, fake_volume) + self._driver._resize_filesystem( + fake_server_details, fake_volume, new_size=123) self._driver._ssh_exec.assert_any_call( fake_server_details, ['sudo', 'fsck', '-pf', '/dev/fake']) self._driver._ssh_exec.assert_any_call( - fake_server_details, ['sudo', 'resize2fs', '/dev/fake']) + fake_server_details, + ['sudo', 'resize2fs', '/dev/fake', "%sG" % 123] + ) self.assertEqual(2, self._driver._ssh_exec.call_count) + @ddt.data( + { + 'source': processutils.ProcessExecutionError( + stderr="resize2fs: New size smaller than minimum (123456)"), + 'target': exception.Invalid + }, + { + 'source': processutils.ProcessExecutionError(stderr="fake_error"), + 'target': exception.ManilaException + } + ) + @ddt.unpack + def test_resize_filesystem_invalid_new_size(self, source, target): + fake_server_details = {'fake': 'fake'} + fake_volume = {'mountpoint': '/dev/fake'} + ssh_mock = mock.Mock(side_effect=["fake", source]) + self.mock_object(self._driver, '_ssh_exec', ssh_mock) + + self.assertRaises( + target, + self._driver._resize_filesystem, + fake_server_details, fake_volume, new_size=123 + ) + + def test_shrink_share_invalid_size(self): + fake_share = {'id': 'fake', 'export_locations': [{'path': 'test'}]} + new_size = 123 + self.mock_object( + self._driver.service_instance_manager, + 'get_common_server', + mock.Mock(return_value=self.server) + ) + self.mock_object(self._driver, '_get_helper') + self.mock_object(self._driver, '_get_consumed_space', + mock.Mock(return_value=200)) + CONF.set_default('driver_handles_share_servers', False) + + self.assertRaises( + exception.ShareShrinkingPossibleDataLoss, + self._driver.shrink_share, + fake_share, + new_size + ) + + self._driver._get_helper.assert_called_once_with(fake_share) + self._driver._get_consumed_space.assert_called_once_with( + mock.ANY, self.server['backend_details']) + + def _setup_shrink_mocks(self): + share = {'id': 'fake', 'export_locations': [{'path': 'test'}], + 'name': 'fake'} + volume = {'id': 'fake'} + new_size = 123 + server_details = self.server['backend_details'] + self.mock_object( + self._driver.service_instance_manager, + 'get_common_server', + mock.Mock(return_value=self.server) + ) + helper = mock.Mock() + self.mock_object(self._driver, '_get_helper', + mock.Mock(return_value=helper)) + self.mock_object(self._driver, '_get_consumed_space', + mock.Mock(return_value=100)) + self.mock_object(self._driver, '_get_volume', + mock.Mock(return_value=volume)) + self.mock_object(self._driver, '_unmount_device') + self.mock_object(self._driver, '_mount_device') + CONF.set_default('driver_handles_share_servers', False) + + return share, volume, new_size, server_details, helper + + @ddt.data({'source': exception.Invalid("fake"), + 'target': exception.ShareShrinkingPossibleDataLoss}, + {'source': exception.ManilaException("fake"), + 'target': exception.Invalid}) + @ddt.unpack + def test_shrink_share_error_on_resize_fs(self, source, target): + share, vol, size, server_details, _ = self._setup_shrink_mocks() + resize_mock = mock.Mock(side_effect=source) + self.mock_object(self._driver, '_resize_filesystem', resize_mock) + + self.assertRaises(target, self._driver.shrink_share, share, size) + + resize_mock.assert_called_once_with(server_details, vol, + new_size=size) + + def test_shrink_share(self): + share, vol, size, server_details, helper = self._setup_shrink_mocks() + self.mock_object(self._driver, '_resize_filesystem') + + self._driver.shrink_share(share, size) + + self._driver._get_helper.assert_called_once_with(share) + self._driver._get_consumed_space.assert_called_once_with( + mock.ANY, server_details) + self._driver._get_volume.assert_called_once_with(mock.ANY, share['id']) + self._driver._unmount_device.assert_called_once_with(share, + server_details) + self._driver._resize_filesystem( + server_details, vol, new_size=size) + self._driver._mount_device(share, server_details, vol) + self.assertTrue(helper.disable_access_for_maintenance.called) + self.assertTrue(helper.restore_access_after_maintenance.called) + @ddt.data({'share_servers': [], 'result': None}, {'share_servers': None, 'result': None}, {'share_servers': ['fake'], 'result': 'fake'}, @@ -1554,6 +1691,49 @@ class NFSHelperTestCase(test.TestCase): self.assertEqual('/foo/bar', result) + def test_disable_access_for_maintenance(self): + fake_maintenance_path = "fake.path" + share_mount_path = os.path.join( + self._helper.configuration.share_mount_path, self.share_name) + self.mock_object(self._helper, '_ssh_exec') + self.mock_object(self._helper, '_sync_nfs_temp_and_perm_files') + self.mock_object(self._helper, '_get_maintenance_file_path', + mock.Mock(return_value=fake_maintenance_path)) + + self._helper.disable_access_for_maintenance( + self.server, self.share_name) + + self._helper._ssh_exec.assert_any_call( + self.server, + ['cat', const.NFS_EXPORTS_FILE, + '| grep', self.share_name, + '| sudo tee', fake_maintenance_path] + ) + self._helper._ssh_exec.assert_any_call( + self.server, + ['sudo', 'exportfs', '-u', share_mount_path] + ) + self._helper._sync_nfs_temp_and_perm_files.assert_called_once_with( + self.server + ) + + def test_restore_access_after_maintenance(self): + fake_maintenance_path = "fake.path" + self.mock_object(self._helper, '_get_maintenance_file_path', + mock.Mock(return_value=fake_maintenance_path)) + self.mock_object(self._helper, '_ssh_exec') + + self._helper.restore_access_after_maintenance( + self.server, self.share_name) + + self._helper._ssh_exec.assert_called_once_with( + self.server, + ['cat', fake_maintenance_path, + '| sudo tee -a', const.NFS_EXPORTS_FILE, + '&& sudo exportfs -r', '&& sudo rm -f', + fake_maintenance_path] + ) + @ddt.ddt class CIFSHelperTestCase(test.TestCase): @@ -1862,3 +2042,41 @@ class CIFSHelperTestCase(test.TestCase): fake_server, ['sudo', 'net', 'conf', 'getparm', 'foo', 'path']) self._helper._get_share_group_name_from_export_location.\ assert_called_once_with(export_location) + + def test_disable_access_for_maintenance(self): + allowed_hosts = ['test', 'test2'] + maintenance_path = os.path.join( + self._helper.configuration.share_mount_path, + "%s.maintenance" % self.share_name) + self.mock_object(self._helper, '_set_allow_hosts') + self.mock_object(self._helper, '_get_allow_hosts', + mock.Mock(return_value=allowed_hosts)) + + self._helper.disable_access_for_maintenance( + self.server_details, self.share_name) + + self._helper._get_allow_hosts.assert_called_once_with( + self.server_details, self.share_name) + self._helper._set_allow_hosts.assert_called_once_with( + self.server_details, [], self.share_name) + valid_cmd = ['echo', "'test test2'", '| sudo tee', maintenance_path] + self._helper._ssh_exec.assert_called_once_with( + self.server_details, valid_cmd) + + def test_restore_access_after_maintenance(self): + fake_maintenance_path = "test.path" + self.mock_object(self._helper, '_set_allow_hosts') + self.mock_object(self._helper, '_get_maintenance_file_path', + mock.Mock(return_value=fake_maintenance_path)) + self.mock_object(self._helper, '_ssh_exec', + mock.Mock(side_effect=[("fake fake2", 0), "fake"])) + + self._helper.restore_access_after_maintenance( + self.server_details, self.share_name) + + self._helper._set_allow_hosts.assert_called_once_with( + self.server_details, ['fake', 'fake2'], self.share_name) + self._helper._ssh_exec.assert_any_call( + self.server_details, ['cat', fake_maintenance_path]) + self._helper._ssh_exec.assert_any_call( + self.server_details, ['sudo rm -f', fake_maintenance_path]) \ No newline at end of file