From 1b5771ef15da3be52f54f2177c181020d2479a8b Mon Sep 17 00:00:00 2001 From: haixin Date: Tue, 1 Sep 2020 11:03:12 +0800 Subject: [PATCH] Fix logic that determines a share exists before manage The share "manage" API checks whether an existing/known share is being imported by matching the export path provided to existing shares. This lookup does not consider the fact that shares may have multiple export locations, because it relies on an old/deprecated "export_location" property on shares which was added to provide backwards compatibility to the API that presented only one export location per share. Further, it's possible to get a "ERROR: Invalid share: Share already exists" exception even when no such share exists in the database. Fix the lookup by using the "export_location_path" based lookup which is faster, since it performs a meaningful join on the export locations table; and remove the parameters "protocol" and "share_type_id" - these things make no difference when there's a duplicated export location. We'll consider "host" as a lookup parameter since we can't be sure that export locations are unique in a deployment - but they ought to be unique for a given host. Closes-Bug: #1848608 Closes-Bug: #1893718 Change-Id: I1d1aef0c2b48764789b43b91b258def6464b389f Co-Authored-By: Goutham Pacha Ravi Signed-off-by: Goutham Pacha Ravi --- manila/api/v1/share_manage.py | 11 ++- manila/db/sqlalchemy/api.py | 13 ++++ manila/share/api.py | 37 +++++----- manila/tests/api/v1/test_share_manage.py | 14 +++- manila/tests/api/v2/test_shares.py | 2 +- manila/tests/share/test_api.py | 74 ++++++++++--------- ...ple-export-locations-32ade25e9d82535b.yaml | 11 +++ 7 files changed, 106 insertions(+), 56 deletions(-) create mode 100644 releasenotes/notes/bug-1848608-1893718-fix-manage-api-for-shares-with-multiple-export-locations-32ade25e9d82535b.yaml diff --git a/manila/api/v1/share_manage.py b/manila/api/v1/share_manage.py index cd0872bf5e..9b4fed567f 100644 --- a/manila/api/v1/share_manage.py +++ b/manila/api/v1/share_manage.py @@ -43,7 +43,7 @@ class ShareManageMixin(object): share = { 'host': share_data['service_host'], - 'export_location': share_data['export_path'], + 'export_location_path': share_data['export_path'], 'share_proto': share_data['protocol'].upper(), 'share_type_id': share_data['share_type_id'], 'display_name': name, @@ -86,6 +86,15 @@ class ShareManageMixin(object): msg = _("Required parameter %s is empty") % parameter raise exc.HTTPUnprocessableEntity(explanation=msg) + if isinstance(data['export_path'], dict): + # the path may be inside this dictionary + try: + data['export_path'] = data['export_path']['path'] + except KeyError: + msg = ("Export path must be a string, or a dictionary " + "with a 'path' item") + raise exc.HTTPUnprocessableEntity(explanation=msg) + if not share_utils.extract_host(data['service_host'], 'pool'): msg = _("service_host parameter should contain pool.") raise exc.HTTPBadRequest(explanation=msg) diff --git a/manila/db/sqlalchemy/api.py b/manila/db/sqlalchemy/api.py index ab3b89753f..f54a059672 100644 --- a/manila/db/sqlalchemy/api.py +++ b/manila/db/sqlalchemy/api.py @@ -1549,6 +1549,19 @@ def share_instances_get_all(context, filters=None, session=None): if instance_ids: query = query.filter(models.ShareInstance.id.in_(instance_ids)) + # TODO(gouthamr): This DB API method needs to be generalized for all + # share instance fields. + host = filters.get('host') + if host: + query = query.filter( + or_(models.ShareInstance.host == host, + models.ShareInstance.host.like("{0}#%".format(host))) + ) + share_server_id = filters.get('share_server_id') + if share_server_id: + query = query.filter( + models.ShareInstance.share_server_id == share_server_id) + # Returns list of share instances that satisfy filters. query = query.all() return query diff --git a/manila/share/api.py b/manila/share/api.py index e0464a89cb..3886b4bace 100644 --- a/manila/share/api.py +++ b/manila/share/api.py @@ -769,18 +769,28 @@ class API(base.Base): self.share_rpcapi.update_share_replica(context, share_replica) def manage(self, context, share_data, driver_options): - shares = self.get_all(context, { + + # Check whether there's a share already with the provided options: + filters = { + 'export_location_path': share_data['export_location_path'], 'host': share_data['host'], - 'export_location': share_data['export_location'], - 'share_proto': share_data['share_proto'], - 'share_type_id': share_data['share_type_id'] - }) + } + share_server_id = share_data.get('share_server_id') + if share_server_id: + filters['share_server_id'] = share_data['share_server_id'] + + already_managed = self.db.share_instances_get_all(context, + filters=filters) + + if already_managed: + LOG.error("Found an existing share with export location %s!", + share_data['export_location_path']) + msg = _("A share already exists with the export path specified.") + raise exception.InvalidShare(reason=msg) share_type_id = share_data['share_type_id'] share_type = share_types.get_share_type(context, share_type_id) - share_server_id = share_data.get('share_server_id') - dhss = share_types.parse_boolean_extra_spec( 'driver_handles_share_servers', share_type['extra_specs']['driver_handles_share_servers']) @@ -820,18 +830,11 @@ class API(base.Base): share_data.update( self.get_share_attributes_from_share_type(share_type)) - LOG.debug("Manage: Found shares %s.", len(shares)) - - export_location = share_data.pop('export_location') - - if len(shares) == 0: - share = self.db.share_create(context, share_data) - else: - msg = _("Share already exists.") - raise exception.InvalidShare(reason=msg) + share = self.db.share_create(context, share_data) + export_location_path = share_data.pop('export_location_path') self.db.share_export_locations_update(context, share.instance['id'], - export_location) + export_location_path) request_spec = self._get_request_spec_dict( share, share_type, size=0, share_proto=share_data['share_proto'], diff --git a/manila/tests/api/v1/test_share_manage.py b/manila/tests/api/v1/test_share_manage.py index de7d83496a..fedfb61628 100644 --- a/manila/tests/api/v1/test_share_manage.py +++ b/manila/tests/api/v1/test_share_manage.py @@ -60,7 +60,9 @@ class ShareManageTest(test.TestCase): @ddt.data({}, {'shares': {}}, - {'share': get_fake_manage_body('', None, None)}) + {'share': get_fake_manage_body('', None, None)}, + {'share': get_fake_manage_body( + export_path={'not_path': '/fake'})}) def test_share_manage_invalid_body(self, body): self.assertRaises(webob.exc.HTTPUnprocessableEntity, self.controller.create, @@ -192,6 +194,8 @@ class ShareManageTest(test.TestCase): get_fake_manage_body(display_name='foo', display_description='bar'), get_fake_manage_body(display_name='foo', display_description='bar', driver_options=dict(volume_id='quuz')), + get_fake_manage_body(display_name='foo', display_description='bar', + export_path={'path': '/fake'}), ) def test_share_manage(self, data): self._setup_manage_mocks() @@ -200,7 +204,7 @@ class ShareManageTest(test.TestCase): share_api.API, 'manage', mock.Mock(return_value=return_share)) share = { 'host': data['share']['service_host'], - 'export_location': data['share']['export_path'], + 'export_location_path': data['share']['export_path'], 'share_proto': data['share']['protocol'].upper(), 'share_type_id': 'fake', 'display_name': 'foo', @@ -208,6 +212,10 @@ class ShareManageTest(test.TestCase): } data['share']['is_public'] = 'foo' driver_options = data['share'].get('driver_options', {}) + if isinstance(share['export_location_path'], dict): + share['export_location_path'] = ( + share['export_location_path']['path'] + ) actual_result = self.controller.create(self.request, data) @@ -225,7 +233,7 @@ class ShareManageTest(test.TestCase): share_api.API, 'manage', mock.Mock(return_value=return_share)) share = { 'host': data['share']['service_host'], - 'export_location': data['share']['export_path'], + 'export_location_path': data['share']['export_path'], 'share_proto': data['share']['protocol'].upper(), 'share_type_id': 'fake', 'display_name': 'foo', diff --git a/manila/tests/api/v2/test_shares.py b/manila/tests/api/v2/test_shares.py index eb354063c9..67608180be 100644 --- a/manila/tests/api/v2/test_shares.py +++ b/manila/tests/api/v2/test_shares.py @@ -2892,7 +2892,7 @@ class ShareManageTest(test.TestCase): mock.Mock(side_effect=lambda *args, **kwargs: args[1])) share = { 'host': data['share']['service_host'], - 'export_location': data['share']['export_path'], + 'export_location_path': data['share']['export_path'], 'share_proto': data['share']['protocol'].upper(), 'share_type_id': 'fake', 'display_name': 'foo', diff --git a/manila/tests/share/test_api.py b/manila/tests/share/test_api.py index 847ac7f483..d9509ebf91 100644 --- a/manila/tests/share/test_api.py +++ b/manila/tests/share/test_api.py @@ -1019,7 +1019,7 @@ class ShareAPITestCase(test.TestCase): def test_manage_new(self, replication_type, dhss, share_server_id): share_data = { 'host': 'fake', - 'export_location': 'fake', + 'export_location_path': 'fake', 'share_proto': 'fake', 'share_type_id': 'fake', } @@ -1063,7 +1063,8 @@ class ShareAPITestCase(test.TestCase): mock.Mock(return_value=share_server)) self.mock_object(db_api, 'share_network_subnet_get', mock.Mock(return_value=fake_subnet)) - self.mock_object(self.api, 'get_all', mock.Mock(return_value=[])) + self.mock_object(db_api, 'share_instances_get_all', + mock.Mock(return_value=[])) self.api.manage(self.context, copy.deepcopy(share_data), driver_options) @@ -1090,13 +1091,18 @@ class ShareAPITestCase(test.TestCase): if dhss: share_data.update({ 'share_network_id': fake_subnet['share_network_id']}) - export_location = share_data.pop('export_location') - self.api.get_all.assert_called_once_with(self.context, mock.ANY) + export_location = share_data.pop('export_location_path') + filters = {'export_location_path': export_location, + 'host': share_data['host'] + } + if share_server_id: + filters['share_server_id'] = share_server_id + db_api.share_instances_get_all.assert_called_once_with( + self.context, filters=filters) db_api.share_create.assert_called_once_with(self.context, share_data) db_api.share_get.assert_called_once_with(self.context, share['id']) db_api.share_export_locations_update.assert_called_once_with( - self.context, share.instance['id'], export_location - ) + self.context, share.instance['id'], export_location) self.scheduler_rpcapi.manage_share.assert_called_once_with( self.context, share['id'], driver_options, expected_request_spec) if dhss: @@ -1114,7 +1120,7 @@ class ShareAPITestCase(test.TestCase): has_share_server_id): share_data = { 'host': 'fake', - 'export_location': 'fake', + 'export_location_path': 'fake', 'share_proto': 'fake', 'share_type_id': 'fake', } @@ -1137,7 +1143,8 @@ class ShareAPITestCase(test.TestCase): self.mock_object(share_types, 'get_share_type', mock.Mock(return_value=fake_type)) - self.mock_object(self.api, 'get_all', mock.Mock(return_value=[])) + self.mock_object(db_api, 'share_instances_get_all', + mock.Mock(return_value=[])) self.assertRaises(exception_type, self.api.manage, @@ -1148,19 +1155,18 @@ class ShareAPITestCase(test.TestCase): share_types.get_share_type.assert_called_once_with( self.context, share_data['share_type_id'] ) - self.api.get_all.assert_called_once_with( - self.context, { - 'host': share_data['host'], - 'export_location': share_data['export_location'], - 'share_proto': share_data['share_proto'], - 'share_type_id': share_data['share_type_id'] - } - ) + filters = {'export_location_path': share_data['export_location_path'], + 'host': share_data['host'] + } + if has_share_server_id: + filters['share_server_id'] = 'fake' + db_api.share_instances_get_all.assert_called_once_with( + self.context, filters=filters) def test_manage_new_share_server_not_found(self): share_data = { 'host': 'fake', - 'export_location': 'fake', + 'export_location_path': 'fake', 'share_proto': 'fake', 'share_type_id': 'fake', 'share_server_id': 'fake' @@ -1184,7 +1190,8 @@ class ShareAPITestCase(test.TestCase): self.mock_object(share_types, 'get_share_type', mock.Mock(return_value=fake_type)) - self.mock_object(self.api, 'get_all', mock.Mock(return_value=[])) + self.mock_object(db_api, 'share_instances_get_all', + mock.Mock(return_value=[])) self.assertRaises(exception.InvalidInput, self.api.manage, @@ -1195,19 +1202,18 @@ class ShareAPITestCase(test.TestCase): share_types.get_share_type.assert_called_once_with( self.context, share_data['share_type_id'] ) - self.api.get_all.assert_called_once_with( - self.context, { + db_api.share_instances_get_all.assert_called_once_with( + self.context, filters={ + 'export_location_path': share_data['export_location_path'], 'host': share_data['host'], - 'export_location': share_data['export_location'], - 'share_proto': share_data['share_proto'], - 'share_type_id': share_data['share_type_id'] + 'share_server_id': share_data['share_server_id'] } ) def test_manage_new_share_server_not_active(self): share_data = { 'host': 'fake', - 'export_location': 'fake', + 'export_location_path': 'fake', 'share_proto': 'fake', 'share_type_id': 'fake', 'share_server_id': 'fake' @@ -1237,7 +1243,8 @@ class ShareAPITestCase(test.TestCase): self.mock_object(share_types, 'get_share_type', mock.Mock(return_value=fake_type)) - self.mock_object(self.api, 'get_all', mock.Mock(return_value=[])) + self.mock_object(db_api, 'share_instances_get_all', + mock.Mock(return_value=[])) self.mock_object(db_api, 'share_server_get', mock.Mock(return_value=share)) @@ -1250,12 +1257,11 @@ class ShareAPITestCase(test.TestCase): share_types.get_share_type.assert_called_once_with( self.context, share_data['share_type_id'] ) - self.api.get_all.assert_called_once_with( - self.context, { + db_api.share_instances_get_all.assert_called_once_with( + self.context, filters={ + 'export_location_path': share_data['export_location_path'], 'host': share_data['host'], - 'export_location': share_data['export_location'], - 'share_proto': share_data['share_proto'], - 'share_type_id': share_data['share_type_id'] + 'share_server_id': share_data['share_server_id'] } ) db_api.share_server_get.assert_called_once_with( @@ -1266,7 +1272,7 @@ class ShareAPITestCase(test.TestCase): def test_manage_duplicate(self, status): share_data = { 'host': 'fake', - 'export_location': 'fake', + 'export_location_path': 'fake', 'share_proto': 'fake', 'share_type_id': 'fake', } @@ -1279,9 +1285,9 @@ class ShareAPITestCase(test.TestCase): 'driver_handles_share_servers': False, }, } - shares = [{'id': 'fake', 'status': status}] - self.mock_object(self.api, 'get_all', - mock.Mock(return_value=shares)) + already_managed = [{'id': 'fake', 'status': status}] + self.mock_object(db_api, 'share_instances_get_all', + mock.Mock(return_value=already_managed)) self.mock_object(share_types, 'get_share_type', mock.Mock(return_value=fake_type)) self.assertRaises(exception.InvalidShare, self.api.manage, diff --git a/releasenotes/notes/bug-1848608-1893718-fix-manage-api-for-shares-with-multiple-export-locations-32ade25e9d82535b.yaml b/releasenotes/notes/bug-1848608-1893718-fix-manage-api-for-shares-with-multiple-export-locations-32ade25e9d82535b.yaml new file mode 100644 index 0000000000..a8dcff8363 --- /dev/null +++ b/releasenotes/notes/bug-1848608-1893718-fix-manage-api-for-shares-with-multiple-export-locations-32ade25e9d82535b.yaml @@ -0,0 +1,11 @@ +--- +fixes: + - | + The `API to import shares into manila + `_ + could sometimes allow a share to be "managed" into manila multiple times + via different export paths. This API could also incorrectly + disallow a manage operation citing a new share in question was already + managed. Both issues have now been fixed. See `bug + #1848608 `_ and `bug #1893718 + `_ for more details.