From 165b03e02102e7ab760ccb7f9bb24ff771d4b408 Mon Sep 17 00:00:00 2001 From: Rodrigo Barbieri Date: Wed, 23 Mar 2016 15:43:03 -0300 Subject: [PATCH] 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 --- manila/api/v1/share_manage.py | 2 +- manila/db/api.py | 4 +- manila/db/sqlalchemy/api.py | 3 +- manila/scheduler/rpcapi.py | 2 +- manila/share/api.py | 65 ++++++++--------- manila/tests/api/v1/test_share_manage.py | 4 +- manila/tests/api/v2/test_shares.py | 4 +- manila/tests/scheduler/test_rpcapi.py | 2 +- manila/tests/share/test_api.py | 69 +++++++++---------- .../tests/api/admin/test_share_manage.py | 29 +++++--- 10 files changed, 99 insertions(+), 85 deletions(-) diff --git a/manila/api/v1/share_manage.py b/manila/api/v1/share_manage.py index faac2f7403..8909b8bc40 100644 --- a/manila/api/v1/share_manage.py +++ b/manila/api/v1/share_manage.py @@ -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) diff --git a/manila/db/api.py b/manila/db/api.py index 2f72c9ab8f..f5d7096b0c 100644 --- a/manila/db/api.py +++ b/manila/db/api.py @@ -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) diff --git a/manila/db/sqlalchemy/api.py b/manila/db/sqlalchemy/api.py index cf0f3db08c..473a0cbddd 100644 --- a/manila/db/sqlalchemy/api.py +++ b/manila/db/sqlalchemy/api.py @@ -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) diff --git a/manila/scheduler/rpcapi.py b/manila/scheduler/rpcapi.py index a80f29d63a..a79e9499bd 100644 --- a/manila/scheduler/rpcapi.py +++ b/manila/scheduler/rpcapi.py @@ -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, diff --git a/manila/share/api.py b/manila/share/api.py index 65d92eb0b4..d6bdf98275 100644 --- a/manila/share/api.py +++ b/manila/share/api.py @@ -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 diff --git a/manila/tests/api/v1/test_share_manage.py b/manila/tests/api/v1/test_share_manage.py index 2c87bceaa0..4183caf155 100644 --- a/manila/tests/api/v1/test_share_manage.py +++ b/manila/tests/api/v1/test_share_manage.py @@ -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, diff --git a/manila/tests/api/v2/test_shares.py b/manila/tests/api/v2/test_shares.py index 862359c38d..d24efca2fd 100644 --- a/manila/tests/api/v2/test_shares.py +++ b/manila/tests/api/v2/test_shares.py @@ -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, diff --git a/manila/tests/scheduler/test_rpcapi.py b/manila/tests/scheduler/test_rpcapi.py index 7029193745..883a00ac3f 100644 --- a/manila/tests/scheduler/test_rpcapi.py +++ b/manila/tests/scheduler/test_rpcapi.py @@ -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', diff --git a/manila/tests/share/test_api.py b/manila/tests/share/test_api.py index a0da552de9..f5ac78b651 100644 --- a/manila/tests/share/test_api.py +++ b/manila/tests/share/test_api.py @@ -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 diff --git a/manila_tempest_tests/tests/api/admin/test_share_manage.py b/manila_tempest_tests/tests/api/admin/test_share_manage.py index 1afecf3a0b..a9ad4a6c8c 100644 --- a/manila_tempest_tests/tests/api/admin/test_share_manage.py +++ b/manila_tempest_tests/tests/api/admin/test_share_manage.py @@ -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'])