From 5254e52aeecb21f72a2580fa7412abaf319257dd Mon Sep 17 00:00:00 2001 From: Igor Malinovskiy Date: Mon, 22 Jun 2015 11:11:31 +0300 Subject: [PATCH] Allow drivers to ask for additional share_servers Add new method choose_share_server_compatible_for_share() to Share Driver interface. This method is executed in Share Manager on share creation step to filter existing share servers. Driver should choose one share server from the list or return None. If this method returns None, then Share Manager starts the creation of new share server. Also, this commit adds a possibility to create share from a snapshot on share server which differs from original share server where the snapshot is placed. Implements bp allow-additional-share-servers Change-Id: Ia85fcf69b953c199808d7e7d3cb0652c2425fd9d --- manila/db/api.py | 12 +- manila/db/sqlalchemy/api.py | 9 +- manila/exception.py | 4 + manila/share/driver.py | 14 +++ manila/share/manager.py | 125 +++++++++++++++------ manila/tests/share/drivers/test_generic.py | 15 +++ manila/tests/share/test_manager.py | 84 +++++++++++++- manila/tests/test_db.py | 10 +- 8 files changed, 218 insertions(+), 55 deletions(-) diff --git a/manila/db/api.py b/manila/db/api.py index a9d384a82e..f9de0265ed 100644 --- a/manila/db/api.py +++ b/manila/db/api.py @@ -625,14 +625,12 @@ def share_server_get_by_host_and_share_net(context, host, share_net_id, session=session) -def share_server_get_by_host_and_share_net_valid(context, host, - share_net_id, - session=None): +def share_server_get_all_by_host_and_share_net_valid(context, host, + share_net_id, + session=None): """Get share server DB records by host and share net not error.""" - return IMPL.share_server_get_by_host_and_share_net_valid(context, - host, - share_net_id, - session=session) + return IMPL.share_server_get_all_by_host_and_share_net_valid( + context, host, share_net_id, session=session) def share_server_get_all(context): diff --git a/manila/db/sqlalchemy/api.py b/manila/db/sqlalchemy/api.py index b6cfae73a8..f025c0e31b 100644 --- a/manila/db/sqlalchemy/api.py +++ b/manila/db/sqlalchemy/api.py @@ -2003,13 +2003,14 @@ def share_server_get(context, server_id, session=None): @require_context -def share_server_get_by_host_and_share_net_valid(context, host, share_net_id, - session=None): +def share_server_get_all_by_host_and_share_net_valid(context, host, + share_net_id, + session=None): result = _server_get_query(context, session).filter_by(host=host)\ .filter_by(share_network_id=share_net_id)\ .filter(models.ShareServer.status.in_( - (constants.STATUS_CREATING, constants.STATUS_ACTIVE))).first() - if result is None: + (constants.STATUS_CREATING, constants.STATUS_ACTIVE))).all() + if not result: filters_description = ('share_network_id is "%(share_net_id)s",' ' host is "%(host)s" and status in' ' "%(status_cr)s" or "%(status_act)s"') % { diff --git a/manila/exception.py b/manila/exception.py index 6ec78c4b76..b3d024e6e7 100644 --- a/manila/exception.py +++ b/manila/exception.py @@ -194,6 +194,10 @@ class ShareServerInUse(InUse): message = _("Share server %(share_server_id)s is in use.") +class InvalidShareServer(Invalid): + message = _("Share server %(share_server_id)s is not valid.") + + class ShareServerNotCreated(ManilaException): message = _("Share server %(share_server_id)s failed on creation.") diff --git a/manila/share/driver.py b/manila/share/driver.py index e3345d66a1..d1a7ae30af 100644 --- a/manila/share/driver.py +++ b/manila/share/driver.py @@ -327,6 +327,20 @@ class ShareDriver(object): if self.get_network_allocations_number(): self.network_api.deallocate_network(context, share_server_id) + def choose_share_server_compatible_with_share(self, context, share_servers, + share, snapshot=None): + """Method that allows driver to choose share server for provided share. + + If compatible share-server is not found, method should return None. + + :param context: Current context + :param share_servers: list with share-server models + :param share: share model + :param snapshot: snapshot model + :returns: share-server or None + """ + return share_servers[0] if share_servers else None + def setup_server(self, *args, **kwargs): if self.driver_handles_share_servers: return self._setup_server(*args, **kwargs) diff --git a/manila/share/manager.py b/manila/share/manager.py index 115ec2f4b8..19a0e2ff42 100644 --- a/manila/share/manager.py +++ b/manila/share/manager.py @@ -206,7 +206,7 @@ class ShareManager(manager.SchedulerDependentManager): self.publish_service_capabilities(ctxt) def _provide_share_server_for_share(self, context, share_network_id, - share_id): + share, snapshot=None): """Gets or creates share_server and updates share with its id. Active share_server can be deleted if there are no dependent shares @@ -218,21 +218,87 @@ class ShareManager(manager.SchedulerDependentManager): For this purpose used shared lock between this method and the one with deletion of share_server. + :param context: Current context + :param share_network_id: Share network where existing share server + should be found or created. If + share_network_id is None method use + share_network_id from provided snapshot. + :param share: Share model + :param snapshot: Optional -- Snapshot model + :returns: dict, dict -- first value is share_server, that has been chosen for share schedule. Second value is share updated with share_server_id. """ + if not (share_network_id or snapshot): + msg = _("'share_network_id' parameter or 'snapshot'" + " should be provided. ") + raise ValueError(msg) + + parent_share_server = None + + def error(msg, *args): + LOG.error(msg, *args) + self.db.share_update(context, share['id'], + {'status': constants.STATUS_ERROR}) + + if snapshot: + parent_share_server_id = snapshot['share']['share_server_id'] + try: + parent_share_server = self.db.share_server_get( + context, parent_share_server_id) + except exception.ShareServerNotFound: + with excutils.save_and_reraise_exception(): + error(_LE("Parent share server %s does not exist."), + parent_share_server_id) + + if parent_share_server['status'] != constants.STATUS_ACTIVE: + error_params = { + 'id': parent_share_server_id, + 'status': parent_share_server['status'], + } + error(_LE("Parent share server %(id)s has invalid status " + "'%(status)s'."), error_params) + raise exception.InvalidShareServer( + share_server_id=parent_share_server + ) + + if parent_share_server and not share_network_id: + share_network_id = parent_share_server['share_network_id'] + + def get_available_share_servers(): + if parent_share_server: + return [parent_share_server] + else: + return ( + self.db.share_server_get_all_by_host_and_share_net_valid( + context, self.host, share_network_id) + ) @utils.synchronized("share_manager_%s" % share_network_id) def _provide_share_server_for_share(): - exist = False try: - share_server = \ - self.db.share_server_get_by_host_and_share_net_valid( - context, self.host, share_network_id) - exist = True + available_share_servers = get_available_share_servers() except exception.ShareServerNotFound: - share_server = self.db.share_server_create( + available_share_servers = None + + compatible_share_server = None + + if available_share_servers: + try: + compatible_share_server = ( + self.driver.choose_share_server_compatible_with_share( + context, available_share_servers, share, + snapshot=snapshot + ) + ) + except Exception as e: + with excutils.save_and_reraise_exception(): + error(_LE("Cannot choose compatible share-server: %s"), + e) + + if not compatible_share_server: + compatible_share_server = self.db.share_server_create( context, { 'host': self.host, @@ -241,23 +307,28 @@ class ShareManager(manager.SchedulerDependentManager): } ) - LOG.debug("Using share_server %s for share %s" % ( - share_server['id'], share_id)) + msg = "Using share_server %(share_server)s for share %(share_id)s" + LOG.debug(msg, { + 'share_server': compatible_share_server['id'], + 'share_id': share['id'] + }) + share_ref = self.db.share_update( context, - share_id, - {'share_server_id': share_server['id']}, + share['id'], + {'share_server_id': compatible_share_server['id']}, ) - if not exist: - # Create share server on backend with data from db - share_server = self._setup_server(context, share_server) + if compatible_share_server['status'] == constants.STATUS_CREATING: + # Create share server on backend with data from db. + compatible_share_server = self._setup_server( + context, compatible_share_server) LOG.info(_LI("Share server created successfully.")) else: - LOG.info(_LI("Used already existed share server " + LOG.info(_LI("Used preexisting share server " "'%(share_server_id)s'"), - {'share_server_id': share_server['id']}) - return share_server, share_ref + {'share_server_id': compatible_share_server['id']}) + return compatible_share_server, share_ref return _provide_share_server_for_share() @@ -292,24 +363,12 @@ class ShareManager(manager.SchedulerDependentManager): snapshot_ref = None parent_share_server_id = None - if parent_share_server_id: - try: - share_server = self.db.share_server_get(context, - parent_share_server_id) - LOG.debug("Using share_server " - "%s for share %s" % (share_server['id'], share_id)) - share_ref = self.db.share_update( - context, share_id, {'share_server_id': share_server['id']}) - except exception.ShareServerNotFound: - with excutils.save_and_reraise_exception(): - LOG.error(_LE("Share server %s does not exist."), - parent_share_server_id) - self.db.share_update(context, share_id, - {'status': constants.STATUS_ERROR}) - elif share_network_id: + if share_network_id or parent_share_server_id: try: share_server, share_ref = self._provide_share_server_for_share( - context, share_network_id, share_id) + context, share_network_id, share_ref, + snapshot=snapshot_ref + ) except Exception: with excutils.save_and_reraise_exception(): LOG.error(_LE("Failed to get share server" diff --git a/manila/tests/share/drivers/test_generic.py b/manila/tests/share/drivers/test_generic.py index 2cb75233eb..7161e08e90 100644 --- a/manila/tests/share/drivers/test_generic.py +++ b/manila/tests/share/drivers/test_generic.py @@ -1360,6 +1360,21 @@ class GenericShareDriverTestCase(test.TestCase): fake_server_details, ['sudo', 'resize2fs', '/dev/fake']) self.assertEqual(2, self._driver._ssh_exec.call_count) + @ddt.data({'share_servers': [], 'result': None}, + {'share_servers': None, 'result': None}, + {'share_servers': ['fake'], 'result': 'fake'}, + {'share_servers': ['fake', 'test'], 'result': 'fake'}) + @ddt.unpack + def tests_choose_share_server_compatible_with_share(self, share_servers, + result): + fake_share = "fake" + + actual_result = self._driver.choose_share_server_compatible_with_share( + self._context, share_servers, fake_share + ) + + self.assertEqual(result, actual_result) + @generic.ensure_server def fake(driver_instance, context, share_server=None): diff --git a/manila/tests/share/test_manager.py b/manila/tests/share/test_manager.py index ea71e1e576..4103fa3374 100644 --- a/manila/tests/share/test_manager.py +++ b/manila/tests/share/test_manager.py @@ -570,7 +570,10 @@ class ShareManagerTestCase(test.TestCase): def test_create_share_with_share_network_server_creation_failed(self): fake_share = {'id': 'fake_share_id', 'share_network_id': 'fake_sn_id'} - fake_server = {'id': 'fake_srv_id'} + fake_server = { + 'id': 'fake_srv_id', + 'status': constants.STATUS_CREATING, + } self.mock_object(db, 'share_server_create', mock.Mock(return_value=fake_server)) self.mock_object(db, 'share_update', @@ -585,7 +588,8 @@ class ShareManagerTestCase(test.TestCase): def raise_manila_exception(*args, **kwargs): raise exception.ManilaException() - self.mock_object(db, 'share_server_get_by_host_and_share_net_valid', + self.mock_object(db, + 'share_server_get_all_by_host_and_share_net_valid', mock.Mock(side_effect=raise_share_server_not_found)) self.mock_object(self.share_manager, '_setup_server', mock.Mock(side_effect=raise_manila_exception)) @@ -596,7 +600,7 @@ class ShareManagerTestCase(test.TestCase): self.context, fake_share['id'], ) - db.share_server_get_by_host_and_share_net_valid.\ + db.share_server_get_all_by_host_and_share_net_valid.\ assert_called_once_with( utils.IsAMatcher(context.RequestContext), self.share_manager.host, @@ -642,8 +646,12 @@ class ShareManagerTestCase(test.TestCase): share_id = share['id'] - self.share_manager.driver = mock.Mock() - self.share_manager.driver.create_share.return_value = "fake_location" + driver_mock = mock.Mock() + driver_mock.create_share.return_value = "fake_location" + driver_mock.choose_share_server_compatible_with_share.return_value = ( + share_srv + ) + self.share_manager.driver = driver_mock self.share_manager.create_share(self.context, share_id) self.assertFalse(self.share_manager.driver.setup_network.called) self.assertEqual(share_id, db.share_get(context.get_admin_context(), @@ -682,7 +690,10 @@ class ShareManagerTestCase(test.TestCase): share_network_id=share_net['id'], host=self.share_manager.host, state=constants.STATUS_ERROR) share_id = share['id'] - fake_server = {'id': 'fake_srv_id'} + fake_server = { + 'id': 'fake_srv_id', + 'status': constants.STATUS_CREATING, + } self.mock_object(db, 'share_server_create', mock.Mock(return_value=fake_server)) self.mock_object(self.share_manager, '_setup_server', @@ -736,6 +747,67 @@ class ShareManagerTestCase(test.TestCase): utils.IsAMatcher(models.Share), share_server=None) + def test_provide_share_server_for_share_incompatible_servers(self): + fake_exception = exception.ManilaException("fake") + fake_share_server = {'id': 'fake'} + share = self._create_share() + + self.mock_object(db, + 'share_server_get_all_by_host_and_share_net_valid', + mock.Mock(return_value=[fake_share_server])) + self.mock_object( + self.share_manager.driver, + "choose_share_server_compatible_with_share", + mock.Mock(side_effect=fake_exception) + ) + + self.assertRaises(exception.ManilaException, + self.share_manager._provide_share_server_for_share, + self.context, "fake_id", share) + driver_mock = self.share_manager.driver + driver_method_mock = ( + driver_mock.choose_share_server_compatible_with_share + ) + driver_method_mock.assert_called_once_with( + self.context, [fake_share_server], share, snapshot=None) + + def test_provide_share_server_for_share_invalid_arguments(self): + self.assertRaises(ValueError, + self.share_manager._provide_share_server_for_share, + self.context, None, None) + + def test_provide_share_server_for_share_parent_ss_not_found(self): + fake_parent_id = "fake_server_id" + fake_exception = exception.ShareServerNotFound("fake") + share = self._create_share() + fake_snapshot = {'share': {'share_server_id': fake_parent_id}} + self.mock_object(db, 'share_server_get', + mock.Mock(side_effect=fake_exception)) + + self.assertRaises(exception.ShareServerNotFound, + self.share_manager._provide_share_server_for_share, + self.context, "fake_id", share, + snapshot=fake_snapshot) + + db.share_server_get.assert_called_once_with( + self.context, fake_parent_id) + + def test_provide_share_server_for_share_parent_ss_invalid(self): + fake_parent_id = "fake_server_id" + share = self._create_share() + fake_snapshot = {'share': {'share_server_id': fake_parent_id}} + fake_parent_share_server = {'status': 'fake'} + self.mock_object(db, 'share_server_get', + mock.Mock(return_value=fake_parent_share_server)) + + self.assertRaises(exception.InvalidShareServer, + self.share_manager._provide_share_server_for_share, + self.context, "fake_id", share, + snapshot=fake_snapshot) + + db.share_server_get.assert_called_once_with( + self.context, fake_parent_id) + 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 diff --git a/manila/tests/test_db.py b/manila/tests/test_db.py index dbed6300cb..0fd9e0b0e2 100644 --- a/manila/tests/test_db.py +++ b/manila/tests/test_db.py @@ -105,7 +105,7 @@ class ShareServerTableTestCase(test.TestCase): db.share_server_update, self.ctxt, fake_id, {}) - def test_share_server_get_by_host_and_share_net_valid(self): + def test_share_server_get_all_by_host_and_share_net_valid(self): valid = { 'share_network_id': '1', 'host': 'host1', @@ -125,15 +125,15 @@ class ShareServerTableTestCase(test.TestCase): self._create_share_server(invalid) self._create_share_server(other) - servers = db.share_server_get_by_host_and_share_net_valid( + servers = db.share_server_get_all_by_host_and_share_net_valid( self.ctxt, host='host1', share_net_id='1') - self.assertEqual(servers['id'], valid['id']) + self.assertEqual(servers[0]['id'], valid['id']) - def test_share_server_get_by_host_and_share_net_not_found(self): + def test_share_server_get_all_by_host_and_share_net_not_found(self): self.assertRaises(exception.ShareServerNotFound, - db.share_server_get_by_host_and_share_net_valid, + db.share_server_get_all_by_host_and_share_net_valid, self.ctxt, host='fake', share_net_id='fake') def test_share_server_get_all(self):