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 3454158516..84215f4ecb 100644 --- a/manila/db/sqlalchemy/api.py +++ b/manila/db/sqlalchemy/api.py @@ -1454,6 +1454,19 @@ def share_instances_get_all(context, filters=None): models.ShareInstanceExportLocations.uuid == export_location_id) + # 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 6223113536..81708d80c4 100644 --- a/manila/share/api.py +++ b/manila/share/api.py @@ -666,18 +666,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']) @@ -717,18 +727,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 1dcad16d20..265f179f0c 100644 --- a/manila/tests/api/v1/test_share_manage.py +++ b/manila/tests/api/v1/test_share_manage.py @@ -59,7 +59,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 d460f9c99f..f02d13bf5a 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 981bb6bc34..7c173a6764 100644 --- a/manila/tests/share/test_api.py +++ b/manila/tests/share/test_api.py @@ -1001,7 +1001,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', } @@ -1045,7 +1045,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) @@ -1072,13 +1073,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: @@ -1096,7 +1102,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', } @@ -1119,7 +1125,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, @@ -1130,19 +1137,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' @@ -1166,7 +1172,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, @@ -1177,19 +1184,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' @@ -1219,7 +1225,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)) @@ -1232,12 +1239,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( @@ -1248,7 +1254,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', } @@ -1261,9 +1267,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.