From 8b083605fbe0aa4e70e900e76520784c39c06515 Mon Sep 17 00:00:00 2001 From: Valeriy Ponomaryov Date: Fri, 5 Dec 2014 19:34:11 +0200 Subject: [PATCH] Release network resources properly If we try to create share with backend that uses share-server, its network resources won't be released because of improper param provided. Method "deallocate_network" of module "neutron_network_plugin" expects "share_server" as second param but "share_network" is provided indeed. Make it be called only when share_server deleted and errored after network resources provided and with proper params. Change-Id: I8f79724dacb3dd7efc75dc459529a0c3185f999f Closes-Bug: #1399732 --- manila/network/__init__.py | 2 +- .../network/neutron/neutron_network_plugin.py | 8 +- manila/share/manager.py | 20 +++- manila/tests/fake_driver.py | 4 +- manila/tests/share/test_manager.py | 108 ++++++++---------- 5 files changed, 69 insertions(+), 73 deletions(-) diff --git a/manila/network/__init__.py b/manila/network/__init__.py index 706449792d..b57cb6a7c3 100644 --- a/manila/network/__init__.py +++ b/manila/network/__init__.py @@ -41,7 +41,7 @@ class NetworkBaseAPI(object): pass @abc.abstractmethod - def deallocate_network(self, context, share_network): + def deallocate_network(self, context, share_server_id): pass @abc.abstractmethod diff --git a/manila/network/neutron/neutron_network_plugin.py b/manila/network/neutron/neutron_network_plugin.py index a3dec9e95e..d2db8c2574 100644 --- a/manila/network/neutron/neutron_network_plugin.py +++ b/manila/network/neutron/neutron_network_plugin.py @@ -64,18 +64,18 @@ class NeutronNetworkPlugin(manila_network.NetworkBaseAPI, db_base.Base): return ports - def deallocate_network(self, context, share_server): - """Deallocate neutron network resources for the given network info. + def deallocate_network(self, context, share_server_id): + """Deallocate neutron network resources for the given share server. Delete previously allocated neutron ports, delete manila db records for deleted ports. :param context: RequestContext object - :param share_network: share network data + :param share_server_id: id of share server :rtype: None """ ports = self.db.network_allocations_get_for_share_server( - context, share_server['id']) + context, share_server_id) for port in ports: self._delete_port(context, port) diff --git a/manila/share/manager.py b/manila/share/manager.py index bd84d5d3a0..e323058f61 100644 --- a/manila/share/manager.py +++ b/manila/share/manager.py @@ -436,9 +436,14 @@ class ShareManager(manager.SchedulerDependentManager): return network_info def _setup_server(self, context, share_server, metadata=None): - share_network = self.db.share_network_get( - context, share_server['share_network_id']) + # NOTE(vponomaryov): set network_allocations to 0 before 'try' block + # for case we get exception calling appropriate method. This value will + # be used in exception handling and for case 'setup_server' method was + # not called we won't make redundant actions. + allocation_number = 0 try: + share_network = self.db.share_network_get( + context, share_server['share_network_id']) allocation_number = self.driver.get_network_allocations_number() if allocation_number: self.network_api.allocate_network( @@ -478,7 +483,9 @@ class ShareManager(manager.SchedulerDependentManager): self.db.share_server_update(context, share_server['id'], {'status': constants.STATUS_ERROR}) - self.network_api.deallocate_network(context, share_network) + if allocation_number: + self.network_api.deallocate_network( + context, share_server['id']) def delete_share_server(self, context, share_server): @@ -519,4 +526,9 @@ class ShareManager(manager.SchedulerDependentManager): _teardown_server() LOG.info(_LI("Share server deleted successfully.")) - self.network_api.deallocate_network(context, share_server) + # NOTE(vponomaryov): share servers created by Nova do not need + # explicit network allocations release. It is done by Nova itself. + # So, all drivers that use 'service_instance' module do not need + # following operation. + if self.driver.get_network_allocations_number(): + self.network_api.deallocate_network(context, share_server['id']) diff --git a/manila/tests/fake_driver.py b/manila/tests/fake_driver.py index 8f7a2e9ccf..70308a467f 100644 --- a/manila/tests/fake_driver.py +++ b/manila/tests/fake_driver.py @@ -76,7 +76,9 @@ class FakeShareDriver(driver.ShareDriver): pass def get_network_allocations_number(self): - pass + # NOTE(vponomaryov): Simulate drivers that use share servers and + # do not use 'service_instance' module. + return 2 @staticmethod def fake_execute(cmd, *_args, **_kwargs): diff --git a/manila/tests/share/test_manager.py b/manila/tests/share/test_manager.py index 6c4d062009..19bb75d0f2 100644 --- a/manila/tests/share/test_manager.py +++ b/manila/tests/share/test_manager.py @@ -952,8 +952,6 @@ class ShareManagerTestCase(test.TestCase): mock.Mock(side_effect=exception.ManilaException())) self.stubs.Set(self.share_manager.db, 'share_server_update', mock.Mock()) - self.stubs.Set(self.share_manager.network_api, 'deallocate_network', - mock.Mock()) # execute method _setup_server self.assertRaises( @@ -968,70 +966,25 @@ class ShareManagerTestCase(test.TestCase): assert_called_once_with() self.share_manager.db.share_server_update.assert_called_once_with( context, share_server['id'], {'status': constants.STATUS_ERROR}) - self.share_manager.network_api.deallocate_network.\ - assert_called_once_with(context, share_network) - def test_setup_server_exception_in_driver(self): + def setup_server_raise_exception(self, allocation_number, + detail_data_proper): # Setup required test data context = "fake_context" share_server = { 'id': 'fake_id', 'share_network_id': 'fake_sn_id', } - share_network = {'id': 'fake_sn_id'} server_info = {'details_key': 'value'} - network_info = {'fake_network_info_key': 'fake_network_info_value'} - allocation_number = 0 - - # Mock required parameters - self.stubs.Set(self.share_manager.db, 'share_network_get', - mock.Mock(return_value=share_network)) - self.stubs.Set(self.share_manager.driver, - 'get_network_allocations_number', - mock.Mock(return_value=allocation_number)) - self.stubs.Set(self.share_manager.db, 'share_server_update', - mock.Mock()) - self.stubs.Set(self.share_manager.network_api, 'deallocate_network', - mock.Mock()) - self.stubs.Set(self.share_manager, '_form_server_setup_info', - mock.Mock(return_value=network_info)) - self.stubs.Set(self.share_manager.db, - 'share_server_backend_details_set', - mock.Mock()) - self.stubs.Set(self.share_manager.driver, 'setup_server', - mock.Mock(side_effect=exception.ManilaException( - detail_data={'server_details': server_info}))) - - # execute method _setup_server - self.assertRaises( - exception.ManilaException, - self.share_manager._setup_server, - context, - share_server, - ) - self.share_manager.db.share_network_get.assert_called_once_with( - context, share_server['share_network_id']) - self.share_manager.driver.get_network_allocations_number.\ - assert_called_once_with() - self.share_manager._form_server_setup_info.assert_called_once_with( - context, share_server, share_network) - self.share_manager.db.share_server_backend_details_set.\ - assert_called_once_with(context, share_server['id'], server_info) - self.share_manager.db.share_server_update.assert_called_once_with( - context, share_server['id'], {'status': constants.STATUS_ERROR}) - self.share_manager.network_api.deallocate_network.\ - assert_called_once_with(context, share_network) - - def test_setup_server_incorrect_detail_data(self): - # Setup required test data - context = "fake_context" - share_server = { - 'id': 'fake_id', - 'share_network_id': 'fake_sn_id', - } share_network = {'id': 'fake_sn_id'} network_info = {'fake_network_info_key': 'fake_network_info_value'} - allocation_number = 0 + if detail_data_proper: + detail_data = {'server_details': server_info} + self.stubs.Set(self.share_manager.db, + 'share_server_backend_details_set', + mock.Mock()) + else: + detail_data = 'not dictionary detail data' # Mock required parameters self.stubs.Set(self.share_manager.db, 'share_network_get', @@ -1041,8 +994,9 @@ class ShareManagerTestCase(test.TestCase): mock.Mock(return_value=allocation_number)) self.stubs.Set(self.share_manager.db, 'share_server_update', mock.Mock()) - self.stubs.Set(self.share_manager.network_api, 'deallocate_network', - mock.Mock()) + if allocation_number: + for m in ['deallocate_network', 'allocate_network']: + self.stubs.Set(self.share_manager.network_api, m, mock.Mock()) self.stubs.Set(self.share_manager, '_form_server_setup_info', mock.Mock(return_value=network_info)) self.stubs.Set(self.share_manager.db, @@ -1050,7 +1004,7 @@ class ShareManagerTestCase(test.TestCase): mock.Mock()) self.stubs.Set(self.share_manager.driver, 'setup_server', mock.Mock(side_effect=exception.ManilaException( - detail_data='not dictionary detail data'))) + detail_data=detail_data))) # execute method _setup_server self.assertRaises( @@ -1059,13 +1013,41 @@ class ShareManagerTestCase(test.TestCase): context, share_server, ) - self.share_manager.db.share_network_get.assert_called_once_with( - context, share_server['share_network_id']) + if detail_data_proper: + self.share_manager.db.share_server_backend_details_set.\ + assert_called_once_with( + context, share_server['id'], server_info) self.share_manager.driver.get_network_allocations_number.\ assert_called_once_with() self.share_manager._form_server_setup_info.assert_called_once_with( context, share_server, share_network) self.share_manager.db.share_server_update.assert_called_once_with( context, share_server['id'], {'status': constants.STATUS_ERROR}) - self.share_manager.network_api.deallocate_network.\ - assert_called_once_with(context, share_network) + if allocation_number: + self.share_manager.db.share_network_get.assert_has_calls([ + mock.call(context, share_server['share_network_id']), + mock.call(context, share_server['share_network_id'])]) + self.share_manager.network_api.allocate_network.assert_has_calls([ + mock.call(context, share_server, share_network, + count=allocation_number)]) + self.share_manager.network_api.deallocate_network.assert_has_calls( + mock.call(context, share_server['id'])) + else: + self.share_manager.db.share_network_get.assert_called_once_with( + context, share_server['share_network_id']) + + def test_setup_server_incorrect_detail_data_an2(self): + # an2 - allocation number has value -> 2 + self.setup_server_raise_exception(2, detail_data_proper=False) + + def test_setup_server_incorrect_detail_data_an0(self): + # an0 - allocation number has value -> 0 + self.setup_server_raise_exception(0, detail_data_proper=False) + + def test_setup_server_exception_in_driver_an2(self): + # an2 - allocation number has value -> 2 + self.setup_server_raise_exception(2, detail_data_proper=True) + + def test_setup_server_exception_in_driver_an0(self): + # an0 - allocation number has value -> 0 + self.setup_server_raise_exception(2, detail_data_proper=True)