openstack/manila
Code Issues Proposed changes

Merge "Fix logic that determines a share exists before manage"

Browse Source
This commit is contained in:
Zuul 2020-11-24 05:30:20 +00:00 committed by Gerrit Code Review
commit d8dab2d077
7 changed files with 106 additions and 56 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

11
manila/api/v1/share_manage.py
View File

@ -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)

13
manila/db/sqlalchemy/api.py
View File

@ -1581,6 +1581,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

37
manila/share/api.py
View File

@ -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'],

14
manila/tests/api/v1/test_share_manage.py
View File

@ -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',

2
manila/tests/api/v2/test_shares.py
View File

@ -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',

74
manila/tests/share/test_api.py
View File

@ -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,

11
releasenotes/notes/bug-1848608-1893718-fix-manage-api-for-shares-with-multiple-export-locations-32ade25e9d82535b.yaml Normal file
View File

@ -0,0 +1,11 @@
---
fixes:
- |
The `API to import shares into manila
<https://docs.openstack.org/api-ref/shared-file-system/#manage-share-since-api-v2-7>`_
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 <https://launchpad.net/bugs/1848608>`_ and `bug #1893718
<https://launchpad.net/bugs/1893718>`_ for more details.