diff --git a/manila/db/api.py b/manila/db/api.py index eed5afe2a4..8bf44c9447 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 f6abe9750d..61aa18972b 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 3894d43e98..6de3321d01 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 24c1758bd0..4c2d25362f 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 fa95ec8e35..f8474cb60d 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 206a8932a9..4e5afb6ca1 100644 --- a/manila/tests/share/drivers/test_generic.py +++ b/manila/tests/share/drivers/test_generic.py @@ -1384,6 +1384,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 624972da4c..e64e7f38e4 100644 --- a/manila/tests/share/test_manager.py +++ b/manila/tests/share/test_manager.py @@ -571,7 +571,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', @@ -586,7 +589,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)) @@ -597,7 +601,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, @@ -643,8 +647,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(), @@ -683,7 +691,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', @@ -737,6 +748,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):