openstack
/
manila
Code Issues Proposed changes

Fix Manage API synchronous call 

RPCAPI call to Scheduler to perform share_type and host
validation should be asynchronous so share can be
created in DB with status "manage_error" if validation
is not successful.

This change also addresses incorrect exception type in API
and DB popping fields from supplied parameter.

APIImpact

Closes-bug: #1561139
Change-Id: I928f1d6b5657098f9d2b7917e2e334a1f08903f8
This commit is contained in:
Rodrigo Barbieri 2016-03-23 15:43:03 -03:00
parent 8f7df3fc53
commit 165b03e021
10 changed files with 99 additions and 85 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

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

@ -58,7 +58,7 @@ class ShareManageMixin(object):
share_ref = self.share_api.manage(context, share, driver_options)
except exception.PolicyNotAuthorized as e:
raise exc.HTTPForbidden(explanation=six.text_type(e))
except exception.ManilaException as e:
except exception.InvalidShare as e:
raise exc.HTTPConflict(explanation=six.text_type(e))
return self._view_builder.detail(req, share_ref)

4
manila/db/api.py
View File

@ -348,9 +348,9 @@ def share_instances_get_all_by_consistency_group_id(context, cg_id):
###################
def share_create(context, values, create_share_instance=True):
def share_create(context, share_values, create_share_instance=True):
"""Create new share."""
return IMPL.share_create(context, values,
return IMPL.share_create(context, share_values,
create_share_instance=create_share_instance)

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

@ -1473,7 +1473,8 @@ def _metadata_refs(metadata_dict, meta_class):
@require_context
def share_create(context, values, create_share_instance=True):
def share_create(context, share_values, create_share_instance=True):
values = copy.deepcopy(share_values)
values = ensure_model_dict_has_id(values)
values['share_metadata'] = _metadata_refs(values.get('metadata'),
models.ShareMetadata)

2
manila/scheduler/rpcapi.py
View File

@ -108,7 +108,7 @@ class SchedulerAPI(object):
request_spec=None, filter_properties=None):
call_context = self.client.prepare(version='1.6')
return call_context.call(context, 'manage_share',
return call_context.cast(context, 'manage_share',
share_id=share_id,
driver_options=driver_options,
request_spec=request_spec,

65
manila/share/api.py
View File

@ -537,63 +537,66 @@ class API(base.Base):
share = self.db.share_update(context, shares[0]['id'], share_data)
else:
msg = _("Share already exists.")
raise exception.ManilaException(msg)
raise exception.InvalidShare(reason=msg)
self.db.share_export_locations_update(context, share.instance['id'],
export_location)
request_spec = self._get_request_spec_dict(share, share_type, size=0)
request_spec = self._get_request_spec_dict(
share, share_type, size=0, share_proto=share_data['share_proto'],
host=share_data['host'])
# NOTE(ganso): Scheduler is called to validate if share type
# provided can fit in host provided. It will invoke manage upon
# successful validation.
self.scheduler_rpcapi.manage_share(context, share['id'],
driver_options, request_spec)
try:
self.scheduler_rpcapi.manage_share(context, share['id'],
driver_options, request_spec)
except Exception:
msg = _('Host %(host)s did not pass validation for managing of '
'share %(share)s with type %(type)s.') % {
'host': share['host'],
'share': share['id'],
'type': share['share_type_id']}
raise exception.InvalidHost(reason=msg)
return self.db.share_get(context, share['id'])
def _get_request_spec_dict(self, share, share_type, **kwargs):
if share is None:
share = {'instance': {}}
share_instance = share['instance']
share_properties = {
'size': kwargs.get('size', share['size']),
'user_id': kwargs.get('user_id', share['user_id']),
'project_id': kwargs.get('project_id', share['project_id']),
'size': kwargs.get('size', share.get('size')),
'user_id': kwargs.get('user_id', share.get('user_id')),
'project_id': kwargs.get('project_id', share.get('project_id')),
'snapshot_support': kwargs.get(
'snapshot_support',
share_type['extra_specs']['snapshot_support']),
'share_proto': kwargs.get('share_proto', share['share_proto']),
'share_proto': kwargs.get('share_proto', share.get('share_proto')),
'share_type_id': kwargs.get('share_type_id',
share['share_type_id']),
'is_public': kwargs.get('is_public', share['is_public']),
'consistency_group_id': kwargs.get('consistency_group_id',
share['consistency_group_id']),
share.get('share_type_id')),
'is_public': kwargs.get('is_public', share.get('is_public')),
'consistency_group_id': kwargs.get(
'consistency_group_id', share.get('consistency_group_id')),
'source_cgsnapshot_member_id': kwargs.get(
'source_cgsnapshot_member_id',
share['source_cgsnapshot_member_id']),
'snapshot_id': kwargs.get('snapshot_id', share['snapshot_id']),
share.get('source_cgsnapshot_member_id')),
'snapshot_id': kwargs.get('snapshot_id', share.get('snapshot_id')),
}
share_instance_properties = {
'availability_zone_id': kwargs.get(
'availability_zone_id',
share_instance['availability_zone_id']),
'share_network_id': kwargs.get('share_network_id',
share_instance['share_network_id']),
'share_server_id': kwargs.get('share_server_id',
share_instance['share_server_id']),
'share_id': kwargs.get('share_id', share_instance['share_id']),
'host': kwargs.get('host', share_instance['host']),
'status': kwargs.get('status', share_instance['status']),
share_instance.get('availability_zone_id')),
'share_network_id': kwargs.get(
'share_network_id', share_instance.get('share_network_id')),
'share_server_id': kwargs.get(
'share_server_id', share_instance.get('share_server_id')),
'share_id': kwargs.get('share_id', share_instance.get('share_id')),
'host': kwargs.get('host', share_instance.get('host')),
'status': kwargs.get('status', share_instance.get('status')),
}
request_spec = {
'share_properties': share_properties,
'share_instance_properties': share_instance_properties,
'share_type': share_type,
'share_id': share['id']
'share_id': share.get('id'),
}
return request_spec

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

@ -114,9 +114,9 @@ class ShareManageTest(test.TestCase):
def test_share_manage_duplicate_share(self):
body = get_fake_manage_body()
exc = exception.InvalidShare(reason="fake")
self._setup_manage_mocks()
self.mock_object(share_api.API, 'manage',
mock.Mock(side_effect=exception.ManilaException()))
self.mock_object(share_api.API, 'manage', mock.Mock(side_effect=exc))
self.assertRaises(webob.exc.HTTPConflict,
self.controller.create,

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

@ -1815,9 +1815,9 @@ class ShareManageTest(test.TestCase):
def test_share_manage_duplicate_share(self):
body = get_fake_manage_body()
exc = exception.InvalidShare(reason="fake")
self._setup_manage_mocks()
self.mock_object(share_api.API, 'manage',
mock.Mock(side_effect=exception.ManilaException()))
self.mock_object(share_api.API, 'manage', mock.Mock(side_effect=exc))
self.assertRaises(webob.exc.HTTPConflict,
self.controller.manage,

2
manila/tests/scheduler/test_rpcapi.py
View File

@ -121,7 +121,7 @@ class SchedulerRpcAPITestCase(test.TestCase):
def test_manage_share(self):
self._test_scheduler_api('manage_share',
rpc_method='call',
rpc_method='cast',
share_id='share_id',
driver_options='fake_driver_options',
request_spec='fake_request_spec',

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

@ -728,8 +728,7 @@ class ShareAPITestCase(test.TestCase):
self.context, request_spec=mock.ANY, filter_properties={})
self.assertFalse(self.api.share_rpcapi.create_share_instance.called)
@ddt.data('no_valid_host', None)
def test_manage_new(self, exc):
def test_manage_new(self):
share_data = {
'host': 'fake',
'export_location': 'fake',
@ -752,9 +751,7 @@ class ShareAPITestCase(test.TestCase):
share = db_api.share_create(self.context, fake_share_data)
if exc:
self.mock_object(self.scheduler_rpcapi, 'manage_share',
mock.Mock(side_effect=exception.NoValidHost))
self.mock_object(self.scheduler_rpcapi, 'manage_share')
self.mock_object(db_api, 'share_create',
mock.Mock(return_value=share))
self.mock_object(db_api, 'share_export_locations_update')
@ -764,14 +761,8 @@ class ShareAPITestCase(test.TestCase):
mock.Mock(return_value=fake_type))
self.mock_object(self.api, 'get_all', mock.Mock(return_value=[]))
if exc:
self.assertRaises(exception.InvalidHost, self.api.manage,
self.context, copy.deepcopy(share_data),
driver_options)
else:
self.api.manage(self.context,
copy.deepcopy(share_data),
driver_options)
self.api.manage(self.context, copy.deepcopy(share_data),
driver_options)
share_data.update({
'user_id': self.context.user_id,
@ -782,13 +773,13 @@ class ShareAPITestCase(test.TestCase):
})
expected_request_spec = self._get_request_spec_dict(
share, fake_type, size=0)
share, fake_type, size=0, share_proto=share_data['share_proto'],
host=share_data['host'])
export_location = share_data.pop('export_location')
self.api.get_all.assert_called_once_with(self.context, mock.ANY)
db_api.share_create.assert_called_once_with(self.context, share_data)
if not exc:
db_api.share_get.assert_called_once_with(self.context, share['id'])
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
)
@ -829,7 +820,8 @@ class ShareAPITestCase(test.TestCase):
driver_options)
expected_request_spec = self._get_request_spec_dict(
share, fake_type, size=0)
share, fake_type, size=0, share_proto=share_data['share_proto'],
host=share_data['host'])
db_api.share_update.assert_called_once_with(
self.context, 'fake', mock.ANY)
@ -863,43 +855,48 @@ class ShareAPITestCase(test.TestCase):
self.context, share_data, driver_options)
def _get_request_spec_dict(self, share, share_type, **kwargs):
if share is None:
share = {'instance': {}}
share_instance = share['instance']
share_properties = {
'size': kwargs.get('size', share['size']),
'user_id': kwargs.get('user_id', share['user_id']),
'project_id': kwargs.get('project_id', share['project_id']),
'size': kwargs.get('size', share.get('size')),
'user_id': kwargs.get('user_id', share.get('user_id')),
'project_id': kwargs.get('project_id', share.get('project_id')),
'snapshot_support': kwargs.get(
'snapshot_support',
share_type['extra_specs']['snapshot_support']),
'share_proto': kwargs.get('share_proto', share['share_proto']),
'share_proto': kwargs.get('share_proto', share.get('share_proto')),
'share_type_id': kwargs.get('share_type_id',
share['share_type_id']),
'is_public': kwargs.get('is_public', share['is_public']),
'consistency_group_id': kwargs.get('consistency_group_id',
share['consistency_group_id']),
share.get('share_type_id')),
'is_public': kwargs.get('is_public', share.get('is_public')),
'consistency_group_id': kwargs.get(
'consistency_group_id', share.get('consistency_group_id')),
'source_cgsnapshot_member_id': kwargs.get(
'source_cgsnapshot_member_id',
share['source_cgsnapshot_member_id']),
'snapshot_id': kwargs.get('snapshot_id', share['snapshot_id']),
share.get('source_cgsnapshot_member_id')),
'snapshot_id': kwargs.get('snapshot_id', share.get('snapshot_id')),
}
share_instance_properties = {
'availability_zone_id': kwargs.get(
'availability_zone_id',
share_instance['availability_zone_id']),
'share_network_id': kwargs.get('share_network_id',
share_instance['share_network_id']),
'share_server_id': kwargs.get('share_server_id',
share_instance['share_server_id']),
'share_id': kwargs.get('share_id', share_instance['share_id']),
'host': kwargs.get('host', share_instance['host']),
'status': kwargs.get('status', share_instance['status']),
share_instance.get('availability_zone_id')),
'share_network_id': kwargs.get(
'share_network_id', share_instance.get('share_network_id')),
'share_server_id': kwargs.get(
'share_server_id', share_instance.get('share_server_id')),
'share_id': kwargs.get('share_id', share_instance.get('share_id')),
'host': kwargs.get('host', share_instance.get('host')),
'status': kwargs.get('status', share_instance.get('status')),
}
request_spec = {
'share_properties': share_properties,
'share_instance_properties': share_instance_properties,
'share_type': share_type,
'share_id': share['id']
'share_id': share.get('id'),
}
return request_spec

29
manila_tempest_tests/tests/api/admin/test_share_manage.py
View File

@ -156,31 +156,44 @@ class ManageNFSShareTest(base.BaseSharesAdminTest):
self._test_manage(share=self.shares[0])
@test.attr(type=["gate", "smoke"])
def test_manage_retry(self):
# Manage share with invalid parameters
@test.attr(type=["gate", "smoke", "negative", ])
def test_manage_with_type_invalid(self):
# Manage share with invalid type
self.assertRaises(
lib_exc.Conflict,
self.shares_v2_client.manage_share,
share = self.shares_v2_client.manage_share(
service_host=self.shares[1]['host'],
export_path=self.shares[1]['export_locations'][0],
protocol=self.shares[1]['share_proto'],
share_type_id=self.st_invalid['share_type']['id'])
# Wait for failure
self.shares_v2_client.wait_for_share_status(share['id'],
'manage_error')
# Delete share
self.shares_v2_client.reset_state(share['id'])
self.shares_v2_client.delete_share(share['id'])
self.shares_v2_client.wait_for_resource_deletion(share_id=share['id'])
self.assertRaises(lib_exc.NotFound, self.shares_v2_client.get_share,
share['id'])
@test.attr(type=["gate", "smoke", ])
def test_manage_with_type(self):
# Manage share with type
share = self.shares_v2_client.manage_share(
service_host=self.shares[1]['host'],
export_path=self.shares[1]['export_locations'][0],
protocol=self.shares[1]['share_proto'],
share_type_id=self.st['share_type']['id'])
# Wait for success
self.shares_v2_client.wait_for_share_status(share['id'], 'available')
# Delete share
self.shares_v2_client.delete_share(share['id'])
self.shares_v2_client.wait_for_resource_deletion(share_id=share['id'])
self.assertRaises(lib_exc.NotFound,
self.shares_v2_client.get_share,
self.assertRaises(lib_exc.NotFound, self.shares_v2_client.get_share,
share['id'])