From 674b70d8c7fe30a967d607667358c03c6814f19f Mon Sep 17 00:00:00 2001 From: Matt Riedemann Date: Fri, 7 Jun 2019 16:45:09 -0400 Subject: [PATCH] Make get_provider_by_name public and remove safe_connect This makes _get_provider_by_name public to expose access for callers outside of SchedulerReportClient. The @safe_connect decorator is removed and the get() is wrapped in a try/except to handle KSA ClientException errors and raise PlacementAPIConnectFailure. The two existing calls for adding/removing providers to/from aggregates are updated to use the public method so they don't need to individually do the safe_connect handling. Change-Id: I504c374d3863a2a956d5c0156a43be2d2a2bc712 --- nova/scheduler/client/report.py | 39 ++++++++-------- .../unit/scheduler/client/test_report.py | 46 +++++++++---------- 2 files changed, 39 insertions(+), 46 deletions(-) diff --git a/nova/scheduler/client/report.py b/nova/scheduler/client/report.py index fed4d75fd9ca..1bd74c8f2a41 100644 --- a/nova/scheduler/client/report.py +++ b/nova/scheduler/client/report.py @@ -28,6 +28,7 @@ from oslo_middleware import request_id from oslo_utils import excutils from oslo_utils import versionutils import retrying +import six from nova.compute import provider_tree import nova.conf @@ -2182,8 +2183,7 @@ class SchedulerReportClient(object): # for backward compatibility. pass - @safe_connect - def _get_provider_by_name(self, context, name): + def get_provider_by_name(self, context, name): """Queries the placement API for resource provider information matching a supplied name. @@ -2193,9 +2193,17 @@ class SchedulerReportClient(object): provider's UUID and generation :raises: `exception.ResourceProviderNotFound` when no such provider was found + :raises: PlacementAPIConnectFailure if there was an issue making the + API call to placement. """ - resp = self.get("/resource_providers?name=%s" % name, - global_request_id=context.global_id) + try: + resp = self.get("/resource_providers?name=%s" % name, + global_request_id=context.global_id) + except ks_exc.ClientException as ex: + LOG.error('Failed to get resource provider by name: %s. Error: %s', + name, six.text_type(ex)) + raise exception.PlacementAPIConnectFailure() + if resp.status_code == 200: data = resp.json() records = data['resource_providers'] @@ -2244,18 +2252,13 @@ class SchedulerReportClient(object): failure attempting to save the provider aggregates :raises: `exception.ResourceProviderUpdateConflict` if a concurrent update to the provider was detected. + :raises: PlacementAPIConnectFailure if there was an issue making an + API call to placement. """ if host_name is None and rp_uuid is None: raise ValueError(_("Either host_name or rp_uuid is required")) if rp_uuid is None: - rp = self._get_provider_by_name(context, host_name) - # NOTE(jaypipes): Unfortunately, due to @safe_connect, - # _get_provider_by_name() can return None. If that happens, raise - # an error so we can trap for it in the Nova API code and ignore in - # Rocky, blow up in Stein. - if rp is None: - raise exception.PlacementAPIConnectFailure() - rp_uuid = rp['uuid'] + rp_uuid = self.get_provider_by_name(context, host_name)['uuid'] # Now attempt to add the aggregate to the resource provider. We don't # want to overwrite any other aggregates the provider may be associated @@ -2297,16 +2300,10 @@ class SchedulerReportClient(object): failure attempting to save the provider aggregates :raises: `exception.ResourceProviderUpdateConflict` if a concurrent update to the provider was detected. + :raises: PlacementAPIConnectFailure if there was an issue making an + API call to placement. """ - rp = self._get_provider_by_name(context, host_name) - # NOTE(jaypipes): Unfortunately, due to @safe_connect, - # _get_provider_by_name() can return None. If that happens, raise an - # error so we can trap for it in the Nova API code and ignore in Rocky, - # blow up in Stein. - if rp is None: - raise exception.PlacementAPIConnectFailure() - rp_uuid = rp['uuid'] - + rp_uuid = self.get_provider_by_name(context, host_name)['uuid'] # Now attempt to remove the aggregate from the resource provider. We # don't want to overwrite any other aggregates the provider may be # associated with, however, so we first grab the list of aggregates for diff --git a/nova/tests/unit/scheduler/client/test_report.py b/nova/tests/unit/scheduler/client/test_report.py index 2cf5c1bc9796..303c11d6d31a 100644 --- a/nova/tests/unit/scheduler/client/test_report.py +++ b/nova/tests/unit/scheduler/client/test_report.py @@ -3792,7 +3792,7 @@ class TestAggregateAddRemoveHost(SchedulerReportClientTestCase): } self.mock_get.return_value = get_resp name = 'cn1' - res = self.client._get_provider_by_name(self.context, name) + res = self.client.get_provider_by_name(self.context, name) exp_url = "/resource_providers?name=%s" % name self.mock_get.assert_called_once_with( @@ -3817,7 +3817,7 @@ class TestAggregateAddRemoveHost(SchedulerReportClientTestCase): name = 'cn1' self.assertRaises( exception.ResourceProviderNotFound, - self.client._get_provider_by_name, self.context, name) + self.client.get_provider_by_name, self.context, name) mock_log.assert_called_once() @mock.patch.object(report.LOG, 'warning') @@ -3828,7 +3828,7 @@ class TestAggregateAddRemoveHost(SchedulerReportClientTestCase): name = 'cn1' self.assertRaises( exception.ResourceProviderNotFound, - self.client._get_provider_by_name, self.context, name) + self.client.get_provider_by_name, self.context, name) mock_log.assert_called_once() @mock.patch.object(report.LOG, 'warning') @@ -3839,7 +3839,7 @@ class TestAggregateAddRemoveHost(SchedulerReportClientTestCase): name = 'cn1' self.assertRaises( exception.ResourceProviderNotFound, - self.client._get_provider_by_name, self.context, name) + self.client.get_provider_by_name, self.context, name) mock_log.assert_not_called() @mock.patch('nova.scheduler.client.report.SchedulerReportClient.' @@ -3847,7 +3847,7 @@ class TestAggregateAddRemoveHost(SchedulerReportClientTestCase): @mock.patch('nova.scheduler.client.report.SchedulerReportClient.' '_get_provider_aggregates') @mock.patch('nova.scheduler.client.report.SchedulerReportClient.' - '_get_provider_by_name') + 'get_provider_by_name') def test_aggregate_add_host_success_no_existing( self, mock_get_by_name, mock_get_aggs, mock_set_aggs): mock_get_by_name.return_value = { @@ -3868,7 +3868,7 @@ class TestAggregateAddRemoveHost(SchedulerReportClientTestCase): @mock.patch('nova.scheduler.client.report.SchedulerReportClient.' '_get_provider_aggregates') @mock.patch('nova.scheduler.client.report.SchedulerReportClient.' - '_get_provider_by_name', new=mock.NonCallableMock()) + 'get_provider_by_name', new=mock.NonCallableMock()) def test_aggregate_add_host_rp_uuid(self, mock_get_aggs, mock_set_aggs): mock_get_aggs.return_value = report.AggInfo( aggregates=set([]), generation=42) @@ -3883,7 +3883,7 @@ class TestAggregateAddRemoveHost(SchedulerReportClientTestCase): @mock.patch('nova.scheduler.client.report.SchedulerReportClient.' '_get_provider_aggregates') @mock.patch('nova.scheduler.client.report.SchedulerReportClient.' - '_get_provider_by_name') + 'get_provider_by_name') def test_aggregate_add_host_success_already_existing( self, mock_get_by_name, mock_get_aggs, mock_set_aggs): mock_get_by_name.return_value = { @@ -3908,14 +3908,12 @@ class TestAggregateAddRemoveHost(SchedulerReportClientTestCase): use_cache=False, generation=43) @mock.patch('nova.scheduler.client.report.SchedulerReportClient.' - '_get_provider_by_name') + 'get_provider_by_name', + side_effect=exception.PlacementAPIConnectFailure) def test_aggregate_add_host_no_placement(self, mock_get_by_name): - """In Rocky, we allow nova-api to not be able to communicate with - placement, so the @safe_connect decorator will return None. Check that - an appropriate exception is raised back to the nova-api code in this - case. + """Tests that PlacementAPIConnectFailure will be raised up from + aggregate_add_host if get_provider_by_name raises that error. """ - mock_get_by_name.return_value = None # emulate @safe_connect... name = 'cn1' agg_uuid = uuids.agg1 self.assertRaises( @@ -3929,7 +3927,7 @@ class TestAggregateAddRemoveHost(SchedulerReportClientTestCase): @mock.patch('nova.scheduler.client.report.SchedulerReportClient.' '_get_provider_aggregates') @mock.patch('nova.scheduler.client.report.SchedulerReportClient.' - '_get_provider_by_name') + 'get_provider_by_name') def test_aggregate_add_host_retry_success( self, mock_get_by_name, mock_get_aggs, mock_set_aggs): mock_get_by_name.return_value = { @@ -3957,7 +3955,7 @@ class TestAggregateAddRemoveHost(SchedulerReportClientTestCase): @mock.patch('nova.scheduler.client.report.SchedulerReportClient.' '_get_provider_aggregates') @mock.patch('nova.scheduler.client.report.SchedulerReportClient.' - '_get_provider_by_name') + 'get_provider_by_name') def test_aggregate_add_host_retry_raises( self, mock_get_by_name, mock_get_aggs, mock_set_aggs): mock_get_by_name.return_value = { @@ -3984,14 +3982,12 @@ class TestAggregateAddRemoveHost(SchedulerReportClientTestCase): self.client.aggregate_add_host, self.context, uuids.agg1) @mock.patch('nova.scheduler.client.report.SchedulerReportClient.' - '_get_provider_by_name') + 'get_provider_by_name', + side_effect=exception.PlacementAPIConnectFailure) def test_aggregate_remove_host_no_placement(self, mock_get_by_name): - """In Rocky, we allow nova-api to not be able to communicate with - placement, so the @safe_connect decorator will return None. Check that - an appropriate exception is raised back to the nova-api code in this - case. + """Tests that PlacementAPIConnectFailure will be raised up from + aggregate_remove_host if get_provider_by_name raises that error. """ - mock_get_by_name.return_value = None # emulate @safe_connect... name = 'cn1' agg_uuid = uuids.agg1 self.assertRaises( @@ -4004,7 +4000,7 @@ class TestAggregateAddRemoveHost(SchedulerReportClientTestCase): @mock.patch('nova.scheduler.client.report.SchedulerReportClient.' '_get_provider_aggregates') @mock.patch('nova.scheduler.client.report.SchedulerReportClient.' - '_get_provider_by_name') + 'get_provider_by_name') def test_aggregate_remove_host_success_already_existing( self, mock_get_by_name, mock_get_aggs, mock_set_aggs): mock_get_by_name.return_value = { @@ -4024,7 +4020,7 @@ class TestAggregateAddRemoveHost(SchedulerReportClientTestCase): @mock.patch('nova.scheduler.client.report.SchedulerReportClient.' '_get_provider_aggregates') @mock.patch('nova.scheduler.client.report.SchedulerReportClient.' - '_get_provider_by_name') + 'get_provider_by_name') def test_aggregate_remove_host_success_no_existing( self, mock_get_by_name, mock_get_aggs, mock_set_aggs): mock_get_by_name.return_value = { @@ -4053,7 +4049,7 @@ class TestAggregateAddRemoveHost(SchedulerReportClientTestCase): @mock.patch('nova.scheduler.client.report.SchedulerReportClient.' '_get_provider_aggregates') @mock.patch('nova.scheduler.client.report.SchedulerReportClient.' - '_get_provider_by_name') + 'get_provider_by_name') def test_aggregate_remove_host_retry_success( self, mock_get_by_name, mock_get_aggs, mock_set_aggs): mock_get_by_name.return_value = { @@ -4081,7 +4077,7 @@ class TestAggregateAddRemoveHost(SchedulerReportClientTestCase): @mock.patch('nova.scheduler.client.report.SchedulerReportClient.' '_get_provider_aggregates') @mock.patch('nova.scheduler.client.report.SchedulerReportClient.' - '_get_provider_by_name') + 'get_provider_by_name') def test_aggregate_remove_host_retry_raises( self, mock_get_by_name, mock_get_aggs, mock_set_aggs): mock_get_by_name.return_value = {