From fd492fc11dc14f39c31f2451cb75e811802cc708 Mon Sep 17 00:00:00 2001 From: Clinton Knight Date: Wed, 9 Sep 2015 11:27:40 -0400 Subject: [PATCH] Missing check in ShareManager::manage_existing() In ShareManager::manage_existing(), there is a check for the DHSS mode of the driver but there is no check for the DHSS mode in the specified share type. This omission requires redundant checks in the drivers. This patch adds the missing check to the share manager, removes the redundant checks from the drivers, and updates all relevant unit tests. Also, fix a couple minor tempest resource cleanup issues discovered while debugging this issue. Change-Id: Ib579fd0558e59c28777342bb9d36def12f6bf4da Closes-Bug: #1493869 --- manila/share/drivers/generic.py | 17 -------- manila/share/drivers/hitachi/hds_hnas.py | 20 --------- manila/share/drivers/huawei/v3/connection.py | 12 ------ manila/share/manager.py | 14 ++++++- .../share/drivers/hitachi/test_hds_hnas.py | 29 ------------- .../share/drivers/huawei/test_huawei_nas.py | 27 ------------ manila/tests/share/drivers/test_generic.py | 18 -------- manila/tests/share/test_manager.py | 42 +++++++++++++++++-- .../tests/api/admin/test_share_manage.py | 4 +- 9 files changed, 53 insertions(+), 130 deletions(-) diff --git a/manila/share/drivers/generic.py b/manila/share/drivers/generic.py index f798075df3..97b1e731d8 100644 --- a/manila/share/drivers/generic.py +++ b/manila/share/drivers/generic.py @@ -24,7 +24,6 @@ from oslo_config import cfg 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 @@ -39,7 +38,6 @@ from manila.i18n import _LI from manila.i18n import _LW from manila.share import driver from manila.share.drivers import service_instance -from manila.share import share_types from manila import utils from manila import volume @@ -831,22 +829,7 @@ class GenericShareDriver(driver.ExecuteMixin, driver.ShareDriver): :param driver_options: Empty dict or dict with 'volume_id' option. :return: dict with share size, example: {'size': 1} """ - if self.driver_handles_share_servers: - msg = _('Operation "manage" for shares is supported only when ' - 'driver does not handle share servers.') - raise exception.InvalidDriverMode(msg) - helper = self._get_helper(share) - driver_mode = share_types.get_share_type_extra_specs( - share['share_type_id'], - const.ExtraSpecs.DRIVER_HANDLES_SHARE_SERVERS) - - if strutils.bool_from_string(driver_mode): - msg = _("%(mode)s != False") % { - 'mode': const.ExtraSpecs.DRIVER_HANDLES_SHARE_SERVERS - } - raise exception.ManageExistingShareTypeMismatch(reason=msg) - share_server = self.service_instance_manager.get_common_server() server_details = share_server['backend_details'] diff --git a/manila/share/drivers/hitachi/hds_hnas.py b/manila/share/drivers/hitachi/hds_hnas.py index b55d04d49d..c495b513c0 100644 --- a/manila/share/drivers/hitachi/hds_hnas.py +++ b/manila/share/drivers/hitachi/hds_hnas.py @@ -15,17 +15,13 @@ from oslo_config import cfg from oslo_log import log -from oslo_utils import strutils import six -from manila.common import constants as const from manila import exception from manila.i18n import _ -from manila.i18n import _LE from manila.i18n import _LI from manila.share import driver from manila.share.drivers.hitachi import ssh -from manila.share import share_types LOG = log.getLogger(__name__) @@ -379,22 +375,6 @@ class HDSHNASDriver(driver.ShareDriver): :returns: Returns a dict with size of share managed and its location (your path in file-system). """ - if self.driver_handles_share_servers: - msg = (_("DHSS = %s") % self.driver_handles_share_servers) - LOG.error(_LE("Operation 'manage' for shares is supported only " - "when driver does not handle share servers.")) - raise exception.InvalidDriverMode(driver_mode=msg) - - driver_mode = share_types.get_share_type_extra_specs( - share['share_type_id'], - const.ExtraSpecs.DRIVER_HANDLES_SHARE_SERVERS) - - if strutils.bool_from_string(driver_mode): - msg = _("%(mode)s != False.") % { - 'mode': const.ExtraSpecs.DRIVER_HANDLES_SHARE_SERVERS - } - raise exception.ManageExistingShareTypeMismatch(reason=msg) - share_id = self._get_hnas_share_id(share['id']) LOG.info(_LI("Share %(shr_path)s will be managed with ID %(shr_id)s."), diff --git a/manila/share/drivers/huawei/v3/connection.py b/manila/share/drivers/huawei/v3/connection.py index 4e00f1ef22..9c4b7ee9d3 100644 --- a/manila/share/drivers/huawei/v3/connection.py +++ b/manila/share/drivers/huawei/v3/connection.py @@ -16,7 +16,6 @@ import time from oslo_log import log -from oslo_utils import strutils from oslo_utils import units from manila.common import constants as common_constants @@ -29,7 +28,6 @@ from manila.share.drivers.huawei import constants from manila.share.drivers.huawei import huawei_utils from manila.share.drivers.huawei.v3 import helper from manila.share.drivers.huawei.v3 import smartx -from manila.share import share_types from manila.share import utils as share_utils @@ -495,16 +493,6 @@ class V3StorageConnection(driver.HuaweiBase): def manage_existing(self, share, driver_options): """Manage existing share.""" - driver_mode = share_types.get_share_type_extra_specs( - share['share_type_id'], - common_constants.ExtraSpecs.DRIVER_HANDLES_SHARE_SERVERS) - - if strutils.bool_from_string(driver_mode): - msg = _("%(mode)s != False") % { - 'mode': - common_constants.ExtraSpecs.DRIVER_HANDLES_SHARE_SERVERS - } - raise exception.ManageExistingShareTypeMismatch(reason=msg) share_proto = share['share_proto'] share_name = share['name'] diff --git a/manila/share/manager.py b/manila/share/manager.py index 9766b8c8fe..71d100945f 100644 --- a/manila/share/manager.py +++ b/manila/share/manager.py @@ -28,6 +28,7 @@ from oslo_serialization import jsonutils from oslo_service import periodic_task from oslo_utils import excutils from oslo_utils import importutils +from oslo_utils import strutils from oslo_utils import timeutils import six @@ -44,6 +45,7 @@ import manila.share.configuration from manila.share import drivers_private_data from manila.share import migration from manila.share import rpcapi as share_rpcapi +from manila.share import share_types from manila.share import utils as share_utils from manila import utils @@ -783,7 +785,17 @@ class ShareManager(manager.SchedulerDependentManager): if self.driver.driver_handles_share_servers: msg = _("Manage share is not supported for " "driver_handles_share_servers=True mode.") - raise exception.InvalidShare(reason=msg) + raise exception.InvalidDriverMode(driver_mode=msg) + + driver_mode = share_types.get_share_type_extra_specs( + share_instance['share_type_id'], + constants.ExtraSpecs.DRIVER_HANDLES_SHARE_SERVERS) + + if strutils.bool_from_string(driver_mode): + msg = _("%(mode)s != False") % { + 'mode': constants.ExtraSpecs.DRIVER_HANDLES_SHARE_SERVERS + } + raise exception.ManageExistingShareTypeMismatch(reason=msg) share_update = ( self.driver.manage_existing(share_instance, driver_options) diff --git a/manila/tests/share/drivers/hitachi/test_hds_hnas.py b/manila/tests/share/drivers/hitachi/test_hds_hnas.py index be0b04d77a..1254ea6372 100644 --- a/manila/tests/share/drivers/hitachi/test_hds_hnas.py +++ b/manila/tests/share/drivers/hitachi/test_hds_hnas.py @@ -327,29 +327,6 @@ class HDSHNASTestCase(test.TestCase): CONF._unset_defaults_and_overrides() - def test_manage_share_type_dhss_true(self): - driver_op = 'fake' - - self.mock_object(share_types, 'get_share_type_extra_specs', - mock.Mock(return_value='True')) - - self.assertRaises(exception.ManageExistingShareTypeMismatch, - self._driver.manage_existing, - self.share, driver_op) - share_types.get_share_type_extra_specs.assert_called_once_with( - self.share['share_type_id'], self.const_dhss) - - def test_manage_conf_dhss_true(self): - driver_op = 'fake' - - CONF.set_default('driver_handles_share_servers', True) - self.mock_object(share_types, 'get_share_type_extra_specs', - mock.Mock(return_value='True')) - - self.assertRaises(exception.InvalidDriverMode, - self._driver.manage_existing, - self.share, driver_op) - def test_manage_invalid_host(self): driver_op = 'fake' self.share_invalid_host = { @@ -369,8 +346,6 @@ class HDSHNASTestCase(test.TestCase): self.assertRaises(exception.ShareBackendException, self._driver.manage_existing, self.share_invalid_host, driver_op) - share_types.get_share_type_extra_specs.assert_called_once_with( - self.share_invalid_host['share_type_id'], self.const_dhss) def test_manage_invalid_path(self): driver_op = 'fake' @@ -391,8 +366,6 @@ class HDSHNASTestCase(test.TestCase): self.assertRaises(exception.ShareBackendException, self._driver.manage_existing, self.share_invalid_path, driver_op) - share_types.get_share_type_extra_specs.assert_called_once_with( - self.share_invalid_path['share_type_id'], self.const_dhss) def test_manage_invalid_evs_ip(self): driver_op = 'fake' @@ -413,8 +386,6 @@ class HDSHNASTestCase(test.TestCase): self.assertRaises(exception.ShareBackendException, self._driver.manage_existing, self.share_invalid_ip, driver_op) - share_types.get_share_type_extra_specs.assert_called_once_with( - self.share_invalid_ip['share_type_id'], self.const_dhss) def test_unmanage(self): self._driver.unmanage(self.share) diff --git a/manila/tests/share/drivers/huawei/test_huawei_nas.py b/manila/tests/share/drivers/huawei/test_huawei_nas.py index e75809b059..e71088d349 100644 --- a/manila/tests/share/drivers/huawei/test_huawei_nas.py +++ b/manila/tests/share/drivers/huawei/test_huawei_nas.py @@ -1483,33 +1483,6 @@ class HuaweiShareDriverTestCase(test.TestCase): self.share_nfs, self.driver_options) - def test_manage_existing_share_type_mismatch(self): - fake_extra_specs = { - 'driver_handles_share_servers': 'True', - } - fake_share_type_id = 'fake_id' - fake_type_mismatch_extra = { - 'test_with_extra': { - 'created_at': 'fake_time', - 'deleted': '0', - 'deleted_at': None, - 'extra_specs': fake_extra_specs, - 'required_extra_specs': {}, - 'id': fake_share_type_id, - 'name': 'test_with_extra', - 'updated_at': None - } - } - share_type = fake_type_mismatch_extra['test_with_extra'] - self.mock_object(db, - 'share_type_get', - mock.Mock(return_value=share_type)) - self.driver.plugin.helper.login() - self.assertRaises(exception.ManageExistingShareTypeMismatch, - self.driver.manage_existing, - self.share_nfs, - self.driver_options) - def test_get_pool_success(self): self.driver.plugin.helper.login() pool_name = self.driver.get_pool(self.share_nfs_host_not_exist) diff --git a/manila/tests/share/drivers/test_generic.py b/manila/tests/share/drivers/test_generic.py index e46189033c..39a9e322d3 100644 --- a/manila/tests/share/drivers/test_generic.py +++ b/manila/tests/share/drivers/test_generic.py @@ -1309,12 +1309,6 @@ class GenericShareDriverTestCase(test.TestCase): self.assertEqual(True, result['driver_handles_share_servers']) self.assertEqual('Open Source', result['vendor_name']) - def test_manage_invalid_driver_mode(self): - CONF.set_default('driver_handles_share_servers', True) - - self.assertRaises(exception.InvalidDriverMode, - self._driver.manage_existing, 'fake', {}) - def _setup_manage_mocks(self, get_share_type_extra_specs='False', is_device_mounted=True, @@ -1340,18 +1334,6 @@ class GenericShareDriverTestCase(test.TestCase): self.assertRaises(exception.InvalidShare, self._driver.manage_existing, share, {}) - def test_manage_share_type_mismatch(self): - share = {'share_proto': 'NFS', 'share_type_id': 'fake'} - self._setup_manage_mocks(get_share_type_extra_specs='True') - - self.assertRaises(exception.ManageExistingShareTypeMismatch, - self._driver.manage_existing, share, {}) - - share_types.get_share_type_extra_specs.assert_called_once_with( - share['share_type_id'], - const.ExtraSpecs.DRIVER_HANDLES_SHARE_SERVERS - ) - def test_manage_not_mounted_share(self): share = get_fake_manage_share() fake_path = '/foo/bar' diff --git a/manila/tests/share/test_manager.py b/manila/tests/share/test_manager.py index 3eca468b70..64fc8bc81d 100644 --- a/manila/tests/share/test_manager.py +++ b/manila/tests/share/test_manager.py @@ -33,6 +33,7 @@ from manila.share import drivers_private_data from manila.share import manager from manila.share import migration from manila.share import rpcapi +from manila.share import share_types from manila import test from manila.tests import db_utils from manila.tests import utils as test_utils @@ -833,12 +834,33 @@ class ShareManagerTestCase(test.TestCase): def test_manage_share_invalid_driver(self): self.mock_object(self.share_manager, 'driver', mock.Mock()) self.share_manager.driver.driver_handles_share_servers = True + self.mock_object(share_types, + 'get_share_type_extra_specs', + mock.Mock(return_value='False')) self.mock_object(self.share_manager.db, 'share_update', mock.Mock()) share = db_utils.create_share() share_id = share['id'] self.assertRaises( - exception.InvalidShare, + exception.InvalidDriverMode, + self.share_manager.manage_share, self.context, share_id, {}) + + self.share_manager.db.share_update.assert_called_once_with( + utils.IsAMatcher(context.RequestContext), share_id, + {'status': constants.STATUS_MANAGE_ERROR, 'size': 1}) + + def test_manage_share_invalid_share_type(self): + self.mock_object(self.share_manager, 'driver', mock.Mock()) + self.share_manager.driver.driver_handles_share_servers = False + self.mock_object(share_types, + 'get_share_type_extra_specs', + mock.Mock(return_value='True')) + self.mock_object(self.share_manager.db, 'share_update', mock.Mock()) + share = db_utils.create_share() + share_id = share['id'] + + self.assertRaises( + exception.ManageExistingShareTypeMismatch, self.share_manager.manage_share, self.context, share_id, {}) self.share_manager.db.share_update.assert_called_once_with( @@ -849,9 +871,12 @@ class ShareManagerTestCase(test.TestCase): CustomException = type('CustomException', (Exception,), dict()) self.mock_object(self.share_manager, 'driver', mock.Mock()) self.share_manager.driver.driver_handles_share_servers = False - self.mock_object( - self.share_manager.driver, - "manage_existing", mock.Mock(side_effect=CustomException)) + self.mock_object(self.share_manager.driver, + 'manage_existing', + mock.Mock(side_effect=CustomException)) + self.mock_object(share_types, + 'get_share_type_extra_specs', + mock.Mock(return_value='False')) self.mock_object(self.share_manager.db, 'share_update', mock.Mock()) share = db_utils.create_share() share_id = share['id'] @@ -872,6 +897,9 @@ class ShareManagerTestCase(test.TestCase): def test_manage_share_invalid_size(self): self.mock_object(self.share_manager, 'driver') self.share_manager.driver.driver_handles_share_servers = False + self.mock_object(share_types, + 'get_share_type_extra_specs', + mock.Mock(return_value='False')) self.mock_object(self.share_manager.driver, "manage_existing", mock.Mock(return_value=None)) @@ -894,6 +922,9 @@ class ShareManagerTestCase(test.TestCase): def test_manage_share_quota_error(self): self.mock_object(self.share_manager, 'driver') self.share_manager.driver.driver_handles_share_servers = False + self.mock_object(share_types, + 'get_share_type_extra_specs', + mock.Mock(return_value='False')) self.mock_object(self.share_manager.driver, "manage_existing", mock.Mock(return_value={'size': 3})) @@ -934,6 +965,9 @@ class ShareManagerTestCase(test.TestCase): mock.Mock(side_effect=( self.share_manager.db.share_export_locations_update))) self.share_manager.driver.driver_handles_share_servers = False + self.mock_object(share_types, + 'get_share_type_extra_specs', + mock.Mock(return_value='False')) self.mock_object(self.share_manager.driver, "manage_existing", mock.Mock(return_value=driver_data)) diff --git a/manila_tempest_tests/tests/api/admin/test_share_manage.py b/manila_tempest_tests/tests/api/admin/test_share_manage.py index df3896ecc9..286f501ce2 100644 --- a/manila_tempest_tests/tests/api/admin/test_share_manage.py +++ b/manila_tempest_tests/tests/api/admin/test_share_manage.py @@ -100,7 +100,7 @@ class ManageNFSShareTest(base.BaseSharesAdminTest): # Add managed share to cleanup queue self.method_resources.insert( - 0, {'type': 'share_type', 'id': share['id'], + 0, {'type': 'share', 'id': share['id'], 'client': self.shares_client}) # Wait for success @@ -137,7 +137,7 @@ class ManageNFSShareTest(base.BaseSharesAdminTest): # Add managed share to cleanup queue self.method_resources.insert( - 0, {'type': 'share_type', 'id': share['id'], + 0, {'type': 'share', 'id': share['id'], 'client': self.shares_client}) # Wait for success