From 6bdeb63c3b99a247ee5762f05b2c98334c1fc011 Mon Sep 17 00:00:00 2001 From: Valeriy Ponomaryov Date: Fri, 6 Mar 2015 15:02:46 +0200 Subject: [PATCH] Always allow delete share-network when no shares exist Currently tenants are unable to delete share_networks if there are existing share_servers. This doesn't make sense, because the share_server exists only for the share_network, and if the tenant is done with the share_network, then the share_server can go away safely. Change-Id: I8925f3deba64da6b011516163e5554b8134c8c0a Closes-Bug: #1429123 --- .../api/share/admin/test_share_servers.py | 24 ++++- .../api/share/test_share_networks_negative.py | 21 ++++ manila/api/v1/share_networks.py | 13 ++- manila/db/api.py | 8 ++ manila/db/sqlalchemy/api.py | 17 +++- manila/share/manager.py | 69 +++++++++---- manila/tests/api/v1/test_share_networks.py | 75 ++++++++++---- manila/tests/share/test_manager.py | 99 ++++++++++++------- 8 files changed, 250 insertions(+), 76 deletions(-) diff --git a/contrib/tempest/tempest/api/share/admin/test_share_servers.py b/contrib/tempest/tempest/api/share/admin/test_share_servers.py index c82af7a3de..4acb111167 100644 --- a/contrib/tempest/tempest/api/share/admin/test_share_servers.py +++ b/contrib/tempest/tempest/api/share/admin/test_share_servers.py @@ -212,7 +212,7 @@ class ShareServersAdminTest(base.BaseSharesAdminTest): self.assertTrue(isinstance(v, six.string_types)) @test.attr(type=["gate", "smoke", ]) - def test_delete_share_server(self): + def _delete_share_server(self, delete_share_network): # Get network and subnet from existing share_network and reuse it # to be able to delete share_server after test ends. # TODO(vponomaryov): attach security-services too. If any exist from @@ -258,9 +258,27 @@ class ShareServersAdminTest(base.BaseSharesAdminTest): self.assertIn(int(resp["status"]), self.HTTP_SUCCESS) self.assertEqual(len(empty), 0) - # Delete share server - resp, __ = self.shares_client.delete_share_server(serv["id"]) + if delete_share_network: + # Delete share network, it should trigger share server deletion + resp, __ = self.shares_client.delete_share_network( + new_sn["id"]) + else: + # Delete share server + resp, __ = self.shares_client.delete_share_server(serv["id"]) + self.assertIn(int(resp["status"]), self.HTTP_SUCCESS) # Wait for share server deletion self.shares_client.wait_for_resource_deletion(server_id=serv["id"]) + + if delete_share_network: + self.shares_client.wait_for_resource_deletion( + sn_id=new_sn["id"]) + + @test.attr(type=["gate", "smoke", ]) + def test_delete_share_server(self): + self._delete_share_server(False) + + @test.attr(type=["gate", "smoke", ]) + def test_delete_share_server_by_deletion_of_share_network(self): + self._delete_share_server(True) diff --git a/contrib/tempest/tempest/api/share/test_share_networks_negative.py b/contrib/tempest/tempest/api/share/test_share_networks_negative.py index 20ccc09944..a4890bf4a3 100644 --- a/contrib/tempest/tempest/api/share/test_share_networks_negative.py +++ b/contrib/tempest/tempest/api/share/test_share_networks_negative.py @@ -110,3 +110,24 @@ class ShareNetworksNegativeTest(base.BaseSharesTest): exceptions.BadRequest, self.shares_client.list_share_networks_with_detail, params={'created_before': '2014-10-23T08:31:58.000000'}) + + @test.attr(type=["gate", "smoke", "negative"]) + @testtools.skipIf(not CONF.share.multitenancy_enabled, + 'Can run only with drivers that do handle share servers ' + 'creation. Skipping.') + def test_try_delete_share_network_with_existing_shares(self): + # Get valid network data for successful share creation + __, share_network = self.shares_client.get_share_network( + self.shares_client.share_network_id) + __, new_sn = self.create_share_network( + neutron_net_id=share_network['neutron_net_id'], + neutron_subnet_id=share_network['neutron_subnet_id'], + nova_net_id=share_network['nova_net_id']) + + # Create share with share network + __, share = self.create_share(share_network_id=new_sn['id']) + + # Try delete share network + self.assertRaises( + lib_exc.Conflict, + self.shares_client.delete_share_network, new_sn['id']) diff --git a/manila/api/v1/share_networks.py b/manila/api/v1/share_networks.py index 8fe46b215f..e2a9d7f9ca 100644 --- a/manila/api/v1/share_networks.py +++ b/manila/api/v1/share_networks.py @@ -109,10 +109,15 @@ class ShareNetworkController(wsgi.Controller): share_network = db_api.share_network_get(context, id) except exception.ShareNetworkNotFound as e: raise exc.HTTPNotFound(explanation=six.text_type(e)) - if share_network['share_servers']: - msg = _("Cannot delete share network %s. " - "There are share servers using it") % id - raise exc.HTTPForbidden(explanation=msg) + + shares = db_api.share_get_all_by_share_network(context, id) + if shares: + msg = _("Can not delete share network %(id)s, it has " + "%(len)s share(s).") % {'id': id, 'len': len(shares)} + LOG.error(msg) + raise exc.HTTPConflict(explanation=msg) + for share_server in share_network['share_servers']: + self.share_rpcapi.delete_share_server(context, share_server) db_api.share_network_delete(context, id) try: diff --git a/manila/db/api.py b/manila/db/api.py index 08f3b2fba4..e577afc659 100644 --- a/manila/db/api.py +++ b/manila/db/api.py @@ -318,6 +318,14 @@ def share_get_all_by_project(context, project_id, filters=None, sort_key=None, ) +def share_get_all_by_share_network(context, share_network_id, filters=None, + sort_key=None, sort_dir=None): + """Returns list of shares that belong to given share network.""" + return IMPL.share_get_all_by_share_network( + context, share_network_id, filters=filters, sort_key=sort_key, + sort_dir=sort_dir) + + def share_get_all_by_share_server(context, share_server_id, filters=None, sort_key=None, sort_dir=None): """Returns all shares with given share server ID.""" diff --git a/manila/db/sqlalchemy/api.py b/manila/db/sqlalchemy/api.py index ddbf2d0b38..3da9e429c5 100644 --- a/manila/db/sqlalchemy/api.py +++ b/manila/db/sqlalchemy/api.py @@ -1170,13 +1170,14 @@ def share_get(context, share_id, session=None): @require_context def _share_get_all_with_filters(context, project_id=None, share_server_id=None, - host=None, filters=None, + share_network_id=None, host=None, filters=None, sort_key=None, sort_dir=None): """Returns sorted list of shares that satisfies filters. :param context: context to query under :param project_id: project id that owns shares :param share_server_id: share server that hosts shares + :param share_network_id: share network that was used for shares :param host: host name where shares [and share servers] are located :param filters: dict of filters to specify share selection :param sort_key: key of models.Share to be used for sorting @@ -1193,6 +1194,8 @@ def _share_get_all_with_filters(context, project_id=None, share_server_id=None, query = query.filter_by(project_id=project_id) if share_server_id: query = query.filter_by(share_server_id=share_server_id) + if share_network_id: + query = query.filter_by(share_network_id=share_network_id) if host and isinstance(host, six.string_types): session = get_session() with session.begin(): @@ -1268,6 +1271,16 @@ def share_get_all_by_project(context, project_id, filters=None, return query +@require_context +def share_get_all_by_share_network(context, share_network_id, filters=None, + sort_key=None, sort_dir=None): + """Returns list of shares that belong to given share network.""" + query = _share_get_all_with_filters( + context, share_network_id=share_network_id, filters=filters, + sort_key=sort_key, sort_dir=sort_dir) + return query + + @require_context def share_get_all_by_share_server(context, share_server_id, filters=None, sort_key=None, sort_dir=None): @@ -1948,6 +1961,8 @@ def share_server_backend_details_delete(context, share_server_id, @require_context def share_server_backend_details_get(context, share_server_id, session=None): + if not session: + session = get_session() query = model_query(context, models.ShareServerBackendDetails, session=session)\ .filter_by(share_server_id=share_server_id).all() diff --git a/manila/share/manager.py b/manila/share/manager.py index 7307f70ba9..085908b59e 100644 --- a/manila/share/manager.py +++ b/manila/share/manager.py @@ -21,6 +21,7 @@ from oslo_config import cfg from oslo_log import log +from oslo_serialization import jsonutils from oslo_utils import excutils from oslo_utils import importutils from oslo_utils import timeutils @@ -484,11 +485,31 @@ class ShareManager(manager.SchedulerDependentManager): # Get share_network again in case it was updated. share_network = self.db.share_network_get( context, share_server['share_network_id']) - network_info = self._form_server_setup_info( context, share_server, share_network) + + # NOTE(vponomaryov): Save security services data to share server + # details table to remove dependency from share network after + # creation operation. It will allow us to delete share server and + # share network separately without dependency on each other. + for security_service in network_info['security_services']: + ss_type = security_service['type'] + data = { + 'name': security_service['name'], + 'domain': security_service['domain'], + 'server': security_service['server'], + 'dns_ip': security_service['dns_ip'], + 'user': security_service['user'], + 'type': ss_type, + 'password': security_service['password'], + } + self.db.share_server_backend_details_set( + context, share_server['id'], + {'security_service_' + ss_type: jsonutils.dumps(data)}) + server_info = self.driver.setup_server( network_info, metadata=metadata) + if server_info and isinstance(server_info, dict): self.db.share_server_backend_details_set( context, share_server['id'], server_info) @@ -528,31 +549,43 @@ class ShareManager(manager.SchedulerDependentManager): # of share_server_id field for share. If so, after lock realese # this method starts executing when amount of dependent shares # has been changed. - shares = self.db.share_get_all_by_share_server( - context, share_server['id']) - if shares: - raise exception.ShareServerInUse( - share_server_id=share_server['id']) + server_id = share_server['id'] + shares = self.db.share_get_all_by_share_server(context, server_id) - self.db.share_server_update(context, share_server['id'], + if shares: + raise exception.ShareServerInUse(share_server_id=server_id) + + if 'backend_details' not in share_server: + server_details = self.db.share_server_backend_details_get( + context, server_id) + else: + server_details = share_server['backend_details'] + + self.db.share_server_update(context, server_id, {'status': constants.STATUS_DELETING}) - sec_services = self.db.share_network_get( - context, - share_server['share_network_id'])['security_services'] try: - LOG.debug("Deleting share server") - self.driver.teardown_server(share_server['backend_details'], - security_services=sec_services) + LOG.debug("Deleting share server '%s'", server_id) + security_services = [] + for ss_name in constants.SECURITY_SERVICES_ALLOWED_TYPES: + ss = server_details.get('security_service_' + ss_name) + if ss: + security_services.append(jsonutils.loads(ss)) + + self.driver.teardown_server( + server_details=server_details, + security_services=security_services) except Exception: with excutils.save_and_reraise_exception(): - LOG.error(_LE("Share server %s failed on deletion."), - share_server['id']) + LOG.error( + _LE("Share server '%s' failed on deletion."), + server_id) self.db.share_server_update( - context, share_server['id'], - {'status': constants.STATUS_ERROR}) + context, server_id, {'status': constants.STATUS_ERROR}) else: self.db.share_server_delete(context, share_server['id']) _teardown_server() - LOG.info(_LI("Share server deleted successfully.")) + LOG.info( + _LI("Share server '%s' has been deleted successfully."), + share_server['id']) self.driver.deallocate_network(context, share_server['id']) diff --git a/manila/tests/api/v1/test_share_networks.py b/manila/tests/api/v1/test_share_networks.py index 6d2cf6d37a..86ba6c967d 100644 --- a/manila/tests/api/v1/test_share_networks.py +++ b/manila/tests/api/v1/test_share_networks.py @@ -201,42 +201,83 @@ class ShareNetworkAPITest(test.TestCase): self.req, body) - @mock.patch.object(db_api, 'share_network_get', mock.Mock()) def test_delete_nominal(self): share_nw = fake_share_network.copy() - share_nw['share_servers'] = [] - db_api.share_network_get.return_value = share_nw + share_nw['share_servers'] = ['foo', 'bar'] + self.mock_object(db_api, 'share_network_get', + mock.Mock(return_value=share_nw)) + self.mock_object(db_api, 'share_get_all_by_share_network', + mock.Mock(return_value=[])) + self.mock_object(self.controller.share_rpcapi, 'delete_share_server') + self.mock_object(db_api, 'share_network_delete') - with mock.patch.object(db_api, 'share_network_delete'): - self.controller.delete(self.req, share_nw) - db_api.share_network_delete.assert_called_once_with( - self.req.environ['manila.context'], - share_nw) + self.controller.delete(self.req, share_nw['id']) + + db_api.share_network_get.assert_called_once_with( + self.req.environ['manila.context'], share_nw['id']) + db_api.share_get_all_by_share_network.assert_called_once_with( + self.req.environ['manila.context'], share_nw['id']) + self.controller.share_rpcapi.delete_share_server.assert_has_calls([ + mock.call(self.req.environ['manila.context'], 'foo'), + mock.call(self.req.environ['manila.context'], 'bar')]) + db_api.share_network_delete.assert_called_once_with( + self.req.environ['manila.context'], share_nw['id']) - @mock.patch.object(db_api, 'share_network_get', mock.Mock()) def test_delete_not_found(self): share_nw = 'fake network id' - db_api.share_network_get.side_effect = exception.ShareNetworkNotFound( - share_network_id=share_nw) + self.mock_object(db_api, 'share_network_get', + mock.Mock(side_effect=exception.ShareNetworkNotFound( + share_network_id=share_nw))) self.assertRaises(webob_exc.HTTPNotFound, self.controller.delete, self.req, share_nw) - @mock.patch.object(db_api, 'share_network_get', mock.Mock()) + def test_quota_delete_reservation_failed(self): + share_nw = fake_share_network.copy() + share_nw['share_servers'] = ['foo', 'bar'] + self.mock_object(db_api, 'share_network_get', + mock.Mock(return_value=share_nw)) + self.mock_object(db_api, 'share_get_all_by_share_network', + mock.Mock(return_value=[])) + self.mock_object(self.controller.share_rpcapi, 'delete_share_server') + self.mock_object(db_api, 'share_network_delete') + self.mock_object(share_networks.QUOTAS, 'reserve', + mock.Mock(side_effect=Exception)) + self.mock_object(share_networks.QUOTAS, 'commit') + + self.controller.delete(self.req, share_nw['id']) + + db_api.share_network_get.assert_called_once_with( + self.req.environ['manila.context'], share_nw['id']) + db_api.share_get_all_by_share_network.assert_called_once_with( + self.req.environ['manila.context'], share_nw['id']) + self.controller.share_rpcapi.delete_share_server.assert_has_calls([ + mock.call(self.req.environ['manila.context'], 'foo'), + mock.call(self.req.environ['manila.context'], 'bar')]) + db_api.share_network_delete.assert_called_once_with( + self.req.environ['manila.context'], share_nw['id']) + self.assertTrue(share_networks.QUOTAS.reserve.called) + self.assertFalse(share_networks.QUOTAS.commit.called) + def test_delete_in_use(self): share_nw = fake_share_network.copy() - share_servers = [{'id': 1}] - share_nw['share_servers'] = share_servers + self.mock_object(db_api, 'share_network_get', + mock.Mock(return_value=share_nw)) + self.mock_object(db_api, 'share_get_all_by_share_network', + mock.Mock(return_value=['foo', 'bar'])) - db_api.share_network_get.return_value = share_nw - - self.assertRaises(webob_exc.HTTPForbidden, + self.assertRaises(webob_exc.HTTPConflict, self.controller.delete, self.req, share_nw['id']) + db_api.share_network_get.assert_called_once_with( + self.req.environ['manila.context'], share_nw['id']) + db_api.share_get_all_by_share_network.assert_called_once_with( + self.req.environ['manila.context'], share_nw['id']) + def test_show_nominal(self): share_nw = 'fake network id' with mock.patch.object(db_api, diff --git a/manila/tests/share/test_manager.py b/manila/tests/share/test_manager.py index 291a79a391..3f187b3cfb 100644 --- a/manila/tests/share/test_manager.py +++ b/manila/tests/share/test_manager.py @@ -15,7 +15,9 @@ """Test of Share Manager for Manila.""" +import ddt import mock +from oslo_serialization import jsonutils from oslo_utils import importutils from manila.common import constants @@ -43,6 +45,7 @@ class FakeAccessRule(object): return getattr(self, item) +@ddt.ddt class ShareManagerTestCase(test.TestCase): def setUp(self): @@ -97,17 +100,17 @@ class ShareManagerTestCase(test.TestCase): return db.share_access_create(context.get_admin_context(), access) @staticmethod - def _create_share_server(state='ACTIVE', share_network_id=None, host=None): + def _create_share_server(state='ACTIVE', share_network_id=None, host=None, + backend_details=None): """Create a share server object.""" srv = {} srv['host'] = host srv['share_network_id'] = share_network_id srv['status'] = state share_srv = db.share_server_create(context.get_admin_context(), srv) - backend_details = {'fake': 'fake'} - db.share_server_backend_details_set(context.get_admin_context(), - share_srv['id'], - backend_details) + if backend_details: + db.share_server_backend_details_set( + context.get_admin_context(), share_srv['id'], backend_details) return db.share_server_get(context.get_admin_context(), share_srv['id']) @@ -334,8 +337,9 @@ class ShareManagerTestCase(test.TestCase): def test_create_share_from_snapshot_with_server(self): """Test share can be created from snapshot if server exists.""" network = self._create_share_network() - server = self._create_share_server(share_network_id=network['id'], - host='fake_host') + server = self._create_share_server( + share_network_id=network['id'], host='fake_host', + backend_details=dict(fake='fake')) parent_share = self._create_share(share_network_id='net-id', share_server_id=server['id']) share = self._create_share() @@ -696,33 +700,35 @@ class ShareManagerTestCase(test.TestCase): share_id ) - def test_delete_share_last_on_server_with_sec_services(self): + @ddt.data(True, False) + def test_delete_share_last_on_server_with_sec_services(self, with_details): share_net = self._create_share_network() sec_service = self._create_security_service(share_net['id']) - share_srv = self._create_share_server( - share_network_id=share_net['id'], - host=self.share_manager.host - ) + backend_details = dict( + security_service_ldap=jsonutils.dumps(sec_service)) + if with_details: + share_srv = self._create_share_server( + share_network_id=share_net['id'], + host=self.share_manager.host, + backend_details=backend_details) + else: + share_srv = self._create_share_server( + share_network_id=share_net['id'], + host=self.share_manager.host) + db.share_server_backend_details_set( + context.get_admin_context(), share_srv['id'], backend_details) share = self._create_share(share_network_id=share_net['id'], share_server_id=share_srv['id']) - share_id = share['id'] - self.share_manager.driver = mock.Mock() manager.CONF.delete_share_server_with_last_share = True - self.share_manager.delete_share(self.context, share_id) - self.assertTrue(self.share_manager.driver.teardown_server.called) - call_args = self.share_manager.driver.teardown_server.call_args[0] - call_kwargs = self.share_manager.driver.teardown_server.call_args[1] - self.assertEqual( - call_args[0], - share_srv.get('backend_details')) - self.assertEqual( - len(call_kwargs['security_services']), 1) - self.assertTrue( - call_kwargs['security_services'][0]['id'], - sec_service['id']) + self.share_manager.delete_share(self.context, share_id) + + self.share_manager.driver.teardown_server.assert_called_once_with( + server_details=backend_details, + security_services=[jsonutils.loads( + backend_details['security_service_ldap'])]) def test_delete_share_last_on_server(self): share_net = self._create_share_network() @@ -739,8 +745,8 @@ class ShareManagerTestCase(test.TestCase): manager.CONF.delete_share_server_with_last_share = True self.share_manager.delete_share(self.context, share_id) self.share_manager.driver.teardown_server.assert_called_once_with( - share_srv.get('backend_details'), security_services=[] - ) + server_details=share_srv.get('backend_details'), + security_services=[]) def test_delete_share_last_on_server_deletion_disabled(self): share_net = self._create_share_network() @@ -833,7 +839,18 @@ class ShareManagerTestCase(test.TestCase): } metadata = {'fake_metadata_key': 'fake_metadata_value'} share_network = {'id': 'fake_sn_id'} - network_info = {'fake_network_info_key': 'fake_network_info_value'} + network_info = {'security_services': []} + for ss_type in constants.SECURITY_SERVICES_ALLOWED_TYPES: + network_info['security_services'].append({ + 'name': 'fake_name' + ss_type, + 'domain': 'fake_domain' + ss_type, + 'server': 'fake_server' + ss_type, + 'dns_ip': 'fake_dns_ip' + ss_type, + 'user': 'fake_user' + ss_type, + 'type': ss_type, + 'password': 'fake_password' + ss_type, + }) + sec_services = network_info['security_services'] server_info = {'fake_server_info_key': 'fake_server_info_value'} # mock required stuff @@ -866,8 +883,18 @@ class ShareManagerTestCase(test.TestCase): self.share_manager.driver.setup_server.assert_called_once_with( network_info, metadata=metadata) self.share_manager.db.share_server_backend_details_set.\ - assert_called_once_with( - self.context, share_server['id'], server_info) + assert_has_calls([ + mock.call(self.context, share_server['id'], + {'security_service_' + sec_services[0]['type']: + jsonutils.dumps(sec_services[0])}), + mock.call(self.context, share_server['id'], + {'security_service_' + sec_services[1]['type']: + jsonutils.dumps(sec_services[1])}), + mock.call(self.context, share_server['id'], + {'security_service_' + sec_services[2]['type']: + jsonutils.dumps(sec_services[2])}), + mock.call(self.context, share_server['id'], server_info), + ]) self.share_manager.db.share_server_update.assert_called_once_with( self.context, share_server['id'], {'status': constants.STATUS_ACTIVE}) @@ -880,7 +907,10 @@ class ShareManagerTestCase(test.TestCase): } metadata = {'fake_metadata_key': 'fake_metadata_value'} share_network = {'id': 'fake_sn_id'} - network_info = {'fake_network_info_key': 'fake_network_info_value'} + network_info = { + 'fake_network_info_key': 'fake_network_info_value', + 'security_services': [], + } server_info = {} # mock required stuff @@ -921,7 +951,10 @@ class ShareManagerTestCase(test.TestCase): } server_info = {'details_key': 'value'} share_network = {'id': 'fake_sn_id'} - network_info = {'fake_network_info_key': 'fake_network_info_value'} + network_info = { + 'fake_network_info_key': 'fake_network_info_value', + 'security_services': [], + } if detail_data_proper: detail_data = {'server_details': server_info} self.mock_object(self.share_manager.db,