Merge "Make get_provider_by_name public and remove safe_connect"

Author: Zuul

nova/scheduler/client/report.py

@@ -28,6 +28,7 @@
 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 @@ def delete_resource_provider(self, context, compute_node, cascade=False):
             # 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 @@ def _get_provider_by_name(self, context, name):
                  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 @@ def aggregate_add_host(self, context, agg_uuid, host_name=None,
                  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 @@ def aggregate_remove_host(self, context, agg_uuid, host_name):
                  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

nova/tests/unit/scheduler/client/test_report.py

@@ -3792,7 +3792,7 @@ def test_get_provider_by_name_success(self):
         }
         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 @@ def test_get_provider_by_name_multiple_results(self, mock_log):
         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 @@ def test_get_provider_by_name_500(self, mock_log):
         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,15 +3839,15 @@ def test_get_provider_by_name_404(self, mock_log):
         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.'
                 'set_aggregates_for_provider')
     @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 @@ def test_aggregate_add_host_success_no_existing(
     @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 @@ def test_aggregate_add_host_rp_uuid(self, mock_get_aggs, mock_set_aggs):
     @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 @@ def test_aggregate_add_host_success_already_existing(
             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 @@ def test_aggregate_add_host_no_placement(self, mock_get_by_name):
     @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 @@ def test_aggregate_add_host_retry_success(
     @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 @@ def test_aggregate_add_host_no_host_name_or_rp_uuid(self):
             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 @@ def test_aggregate_remove_host_no_placement(self, mock_get_by_name):
     @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 @@ def test_aggregate_remove_host_success_already_existing(
     @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 @@ def test_aggregate_remove_host_success_no_existing(
     @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 @@ def test_aggregate_remove_host_retry_success(
     @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 = {