From 8dc6235b5f2c063c446a9e99c4bfe9048742a25a Mon Sep 17 00:00:00 2001 From: Felipe Rodrigues Date: Thu, 16 Jun 2022 12:11:27 -0300 Subject: [PATCH] Fix available share servers to be reused The change [1] modified the function `share_server_get_all_by_host_and_share_subnet_valid` adding the status option. However, it removed the actual filter from the query wrongly. As result, the method is returning all share servers without taking the status in account. This patch reverts that change and also creating a new one for collecting all share servers no matter their states. [1] https://review.opendev.org/c/openstack/manila/+/825110 Closes-Bug: #1978962 Change-Id: I8d9437eafde67407ba5e337dd495fdb74eefec70 --- manila/db/api.py | 9 ++- manila/db/sqlalchemy/api.py | 48 +++++++++----- manila/share/manager.py | 5 +- manila/tests/db/sqlalchemy/test_api.py | 63 ++++++++++++++----- manila/tests/share/test_manager.py | 10 ++- ...nd-available-servers-2dec3a4f3f0ef7e4.yaml | 8 +++ 6 files changed, 103 insertions(+), 40 deletions(-) create mode 100644 releasenotes/notes/bug-1978962-fix-find-available-servers-2dec3a4f3f0ef7e4.yaml diff --git a/manila/db/api.py b/manila/db/api.py index 441431176a..c5f667e2f0 100644 --- a/manila/db/api.py +++ b/manila/db/api.py @@ -1095,13 +1095,20 @@ def share_server_search_by_identifier(context, identifier, session=None): def share_server_get_all_by_host_and_share_subnet_valid(context, host, share_subnet_id, - server_status=None, session=None): """Get share server DB records by host and share net not error.""" return IMPL.share_server_get_all_by_host_and_share_subnet_valid( context, host, share_subnet_id, session=session) +def share_server_get_all_by_host_and_share_subnet(context, host, + share_subnet_id, + session=None): + """Get share server DB records by host and share net.""" + return IMPL.share_server_get_all_by_host_and_share_subnet( + context, host, share_subnet_id, session=session) + + def share_server_get_all(context): """Get all share server DB records.""" return IMPL.share_server_get_all(context) diff --git a/manila/db/sqlalchemy/api.py b/manila/db/sqlalchemy/api.py index 46458948bd..5d1e1baab7 100644 --- a/manila/db/sqlalchemy/api.py +++ b/manila/db/sqlalchemy/api.py @@ -4445,24 +4445,21 @@ def share_server_search_by_identifier(context, identifier, session=None): @require_context def share_server_get_all_by_host_and_share_subnet_valid(context, host, share_subnet_id, - server_status=None, session=None): - query = (_server_get_query(context, session) - .filter_by(host=host) - .filter(models.ShareServer.share_network_subnets.any( - id=share_subnet_id))) - if server_status: - query.filter_by(status=server_status) - else: - query.filter(models.ShareServer.status.in_( - (constants.STATUS_CREATING, constants.STATUS_ACTIVE))) + result = (_server_get_query(context, session) + .filter_by(host=host) + .filter(models.ShareServer.share_network_subnets.any( + id=share_subnet_id)) + .filter(models.ShareServer.status.in_( + (constants.STATUS_CREATING, + constants.STATUS_ACTIVE))).all()) - result = query.all() if not result: - filters_description = ('share_network_subnet_id is "%(share_net_id)s",' - ' host is "%(host)s" and status in' - ' "%(status_cr)s" or "%(status_act)s"') % { - 'share_net_id': share_subnet_id, + filters_description = ('share_network_subnet_id is ' + '"%(share_subnet_id)s", host is "%(host)s" and ' + 'status in "%(status_cr)s" or ' + '"%(status_act)s"') % { + 'share_subnet_id': share_subnet_id, 'host': host, 'status_cr': constants.STATUS_CREATING, 'status_act': constants.STATUS_ACTIVE, @@ -4472,6 +4469,27 @@ def share_server_get_all_by_host_and_share_subnet_valid(context, host, return result +@require_context +def share_server_get_all_by_host_and_share_subnet(context, host, + share_subnet_id, + session=None): + result = (_server_get_query(context, session) + .filter_by(host=host) + .filter(models.ShareServer.share_network_subnets.any( + id=share_subnet_id)).all()) + + if not result: + filters_description = ('share_network_subnet_id is ' + '"%(share_subnet_id)s" and host is ' + '"%(host)s".') % { + 'share_subnet_id': share_subnet_id, + 'host': host, + } + raise exception.ShareServerNotFoundByFilters( + filters_description=filters_description) + return result + + @require_context def share_server_get_all(context): return _server_get_query(context).all() diff --git a/manila/share/manager.py b/manila/share/manager.py index 00cea77ad9..b9c33697bb 100644 --- a/manila/share/manager.py +++ b/manila/share/manager.py @@ -6147,9 +6147,8 @@ class ShareManager(manager.SchedulerDependentManager): current_subnets = [subnet for subnet in current_subnets if subnet['id'] != new_share_network_subnet_id] share_servers = ( - self.db.share_server_get_all_by_host_and_share_subnet_valid( - context, self.host, new_share_network_subnet_id, - server_status=constants.STATUS_SERVER_NETWORK_CHANGE)) + self.db.share_server_get_all_by_host_and_share_subnet( + context, self.host, new_share_network_subnet_id)) for share_server in share_servers: share_server_id = share_server['id'] diff --git a/manila/tests/db/sqlalchemy/test_api.py b/manila/tests/db/sqlalchemy/test_api.py index e2207c0b09..6529e0fd4d 100644 --- a/manila/tests/db/sqlalchemy/test_api.py +++ b/manila/tests/db/sqlalchemy/test_api.py @@ -3314,8 +3314,7 @@ class ShareServerDatabaseAPITestCase(test.TestCase): db_api.share_server_update, self.ctxt, fake_id, {}) - @ddt.data(None, constants.STATUS_SERVER_NETWORK_CHANGE) - def test_get_all_by_host_and_share_net_valid(self, server_status): + def test_get_all_by_host_and_share_subnet_valid(self): subnet_1 = { 'id': '1', 'share_network_id': '1', @@ -3326,16 +3325,11 @@ class ShareServerDatabaseAPITestCase(test.TestCase): } share_net_subnets1 = db_utils.create_share_network_subnet(**subnet_1) share_net_subnets2 = db_utils.create_share_network_subnet(**subnet_2) - valid_no_status = { + valid = { 'share_network_subnets': [share_net_subnets1], 'host': 'host1', 'status': constants.STATUS_ACTIVE, } - valid_with_status = { - 'share_network_subnets': [share_net_subnets1], - 'host': 'host1', - 'status': constants.STATUS_SERVER_NETWORK_CHANGE, - } invalid = { 'share_network_subnets': [share_net_subnets2], 'host': 'host1', @@ -3346,28 +3340,67 @@ class ShareServerDatabaseAPITestCase(test.TestCase): 'host': 'host2', 'status': constants.STATUS_ACTIVE, } - if server_status: - valid = db_utils.create_share_server(**valid_with_status) - else: - valid = db_utils.create_share_server(**valid_no_status) + valid = db_utils.create_share_server(**valid) db_utils.create_share_server(**invalid) db_utils.create_share_server(**other) servers = db_api.share_server_get_all_by_host_and_share_subnet_valid( self.ctxt, host='host1', - share_subnet_id='1', - server_status=server_status) + share_subnet_id='1') self.assertEqual(valid['id'], servers[0]['id']) - def test_get_all_by_host_and_share_net_not_found(self): + def test_get_all_by_host_and_share_subnet_valid_not_found(self): self.assertRaises( exception.ShareServerNotFound, db_api.share_server_get_all_by_host_and_share_subnet_valid, self.ctxt, host='fake', share_subnet_id='fake' ) + def test_get_all_by_host_and_share_subnet(self): + subnet_1 = { + 'id': '1', + 'share_network_id': '1', + } + share_net_subnets1 = db_utils.create_share_network_subnet(**subnet_1) + valid = { + 'share_network_subnets': [share_net_subnets1], + 'host': 'host1', + 'status': constants.STATUS_SERVER_NETWORK_CHANGE, + } + other = { + 'share_network_subnets': [share_net_subnets1], + 'host': 'host1', + 'status': constants.STATUS_ERROR, + } + invalid = { + 'share_network_subnets': [share_net_subnets1], + 'host': 'host2', + 'status': constants.STATUS_ACTIVE, + } + valid = db_utils.create_share_server(**valid) + invalid = db_utils.create_share_server(**invalid) + other = db_utils.create_share_server(**other) + + servers = db_api.share_server_get_all_by_host_and_share_subnet( + self.ctxt, + host='host1', + share_subnet_id='1') + + self.assertEqual(2, len(servers)) + ids = [s['id'] for s in servers] + self.assertTrue(valid['id'] in ids) + self.assertTrue(other['id'] in ids) + self.assertFalse(invalid['id'] in ids) + + def test_get_all_by_host_and_share_subnet_not_found(self): + self.assertRaises( + exception.ShareServerNotFound, + db_api.share_server_get_all_by_host_and_share_subnet, + self.ctxt, host='fake', share_subnet_id='fake' + ) + def test_get_all(self): srv1 = { 'host': 'host1', diff --git a/manila/tests/share/test_manager.py b/manila/tests/share/test_manager.py index 6b91a281b4..c23918c0a7 100644 --- a/manila/tests/share/test_manager.py +++ b/manila/tests/share/test_manager.py @@ -10011,7 +10011,7 @@ class ShareManagerTestCase(test.TestCase): server = {'id': server_id} mock_servers_get = self.mock_object( self.share_manager.db, - 'share_server_get_all_by_host_and_share_subnet_valid', + 'share_server_get_all_by_host_and_share_subnet', mock.Mock(return_value=[server])) current_network_allocations = 'fake_current_net_allocations' mock_form_net_allocations = self.mock_object( @@ -10047,8 +10047,7 @@ class ShareManagerTestCase(test.TestCase): self.context, net_id, new_subnet['availability_zone_id'], fallback_to_default=False) mock_servers_get.assert_called_once_with( - self.context, self.share_manager.host, new_share_network_subnet_id, - server_status=constants.STATUS_SERVER_NETWORK_CHANGE) + self.context, self.share_manager.host, new_share_network_subnet_id) mock_form_net_allocations.assert_called_once_with( self.context, server['id'], subnets) mock_instances_get.assert_called_once_with( @@ -10082,7 +10081,7 @@ class ShareManagerTestCase(test.TestCase): server = {'id': server_id} mock_servers_get = self.mock_object( self.share_manager.db, - 'share_server_get_all_by_host_and_share_subnet_valid', + 'share_server_get_all_by_host_and_share_subnet', mock.Mock(return_value=[server])) current_network_allocations = 'fake_current_net_allocations' mock_form_net_allocations = self.mock_object( @@ -10123,8 +10122,7 @@ class ShareManagerTestCase(test.TestCase): self.context, net_id, new_subnet['availability_zone_id'], fallback_to_default=False) mock_servers_get.assert_called_once_with( - self.context, self.share_manager.host, new_share_network_subnet_id, - server_status=constants.STATUS_SERVER_NETWORK_CHANGE) + self.context, self.share_manager.host, new_share_network_subnet_id) mock_form_net_allocations.assert_called_once_with( self.context, server['id'], subnets) mock_instances_get.assert_called_once_with( diff --git a/releasenotes/notes/bug-1978962-fix-find-available-servers-2dec3a4f3f0ef7e4.yaml b/releasenotes/notes/bug-1978962-fix-find-available-servers-2dec3a4f3f0ef7e4.yaml new file mode 100644 index 0000000000..268042c042 --- /dev/null +++ b/releasenotes/notes/bug-1978962-fix-find-available-servers-2dec3a4f3f0ef7e4.yaml @@ -0,0 +1,8 @@ +--- +fixes: + - | + Drivers using DHSS True mode has the server creation phase. This phase + tries to reuse one of available share servers, however, the Manila code + is considering all share servers states as available, rather than + considering only the active or creating ones. Now, only the correct share + servers are passed to drivers as available to be reused.