Revert "Proper error handling by _ensure_resource_provider"

This reverts commit 44912b5fefd94d3cbff1e308fecb0589e7928932.

Change-Id: I92a9b97be2afea06a2ea6f05749e9b32d7514b15

8 files changed, 37 insertions, 75 deletions

diff --git a/doc/api_samples/os-hypervisors/v2.33/hypervisors-detail-resp.json b/doc/api_samples/os-hypervisors/v2.33/hypervisors-detail-resp.json
index cc1962c197..42adb643fe 100644
--- a/doc/api_samples/os-hypervisors/v2.33/hypervisors-detail-resp.json
+++ b/doc/api_samples/os-hypervisors/v2.33/hypervisors-detail-resp.json
@@ -22,7 +22,7 @@
             "host_ip": "1.1.1.1",
             "free_disk_gb": 1028,
             "free_ram_mb": 7680,
-            "hypervisor_hostname": "host1",
+            "hypervisor_hostname": "fake-mini",
             "hypervisor_type": "fake",
             "hypervisor_version": 1000,
             "id": 2,
diff --git a/doc/api_samples/os-hypervisors/v2.33/hypervisors-list-resp.json b/doc/api_samples/os-hypervisors/v2.33/hypervisors-list-resp.json
index 9a5771df02..3f221f12a7 100644
--- a/doc/api_samples/os-hypervisors/v2.33/hypervisors-list-resp.json
+++ b/doc/api_samples/os-hypervisors/v2.33/hypervisors-list-resp.json
@@ -1,7 +1,7 @@
 {
     "hypervisors": [
         {
-            "hypervisor_hostname": "host1",
+            "hypervisor_hostname": "fake-mini",
             "id": 2,
             "state": "up",
             "status": "enabled"
diff --git a/nova/exception.py b/nova/exception.py
index 85bc9ecbda..84cbcb6941 100644
--- a/nova/exception.py
+++ b/nova/exception.py
@@ -2114,14 +2114,6 @@ class ResourceProviderInUse(NovaException):
     msg_fmt = _("Resource provider has allocations.")
 
 
-class ResourceProviderRetrievalFailed(NovaException):
-    msg_fmt = _("Failed to get resource provider with UUID %(uuid)s")
-
-
-class ResourceProviderCreationFailed(NovaException):
-    msg_fmt = _("Failed to create resource provider %(name)s")
-
-
 class InventoryWithResourceClassNotFound(NotFound):
     msg_fmt = _("No inventory of class %(resource_class)s found.")
 
diff --git a/nova/scheduler/client/report.py b/nova/scheduler/client/report.py
index 0d64a6267b..b496b93bfa 100644
--- a/nova/scheduler/client/report.py
+++ b/nova/scheduler/client/report.py
@@ -301,10 +301,10 @@ class SchedulerReportClient(object):
         """Queries the placement API for a resource provider record with the
         supplied UUID.
 
+        Returns an `objects.ResourceProvider` object if found or None if no
+        such resource provider could be found.
+
         :param uuid: UUID identifier for the resource provider to look up
-        :return: An `objects.ResourceProvider` object if found or None if no
-                 such resource provider could be found.
-        :raise: ResourceProviderRetrievalFailed on error.
         """
         resp = self.get("/resource_providers/%s" % uuid)
         if resp.status_code == 200:
@@ -326,18 +326,16 @@ class SchedulerReportClient(object):
                 'err_text': resp.text,
             }
             LOG.error(msg, args)
-            raise exception.ResourceProviderRetrievalFailed(uuid=uuid)
 
     @safe_connect
     def _create_resource_provider(self, uuid, name):
         """Calls the placement API to create a new resource provider record.
 
+        Returns an `objects.ResourceProvider` object representing the
+        newly-created resource provider object.
+
         :param uuid: UUID of the new resource provider
         :param name: Name of the resource provider
-        :return: An `objects.ResourceProvider` object representing the
-                 newly-created resource provider object.
-        :raise: ResourceProviderCreationFailed or
-                ResourceProviderRetrievalFailed on error.
         """
         url = "/resource_providers"
         payload = {
@@ -355,10 +353,7 @@ class SchedulerReportClient(object):
                     name=name,
                     generation=0,
             )
-
-        # TODO(efried): Push error codes from placement, and use 'em.
-        name_conflict = 'Conflicting resource provider name:'
-        if resp.status_code == 409 and name_conflict not in resp.text:
+        elif resp.status_code == 409:
             # Another thread concurrently created a resource provider with the
             # same UUID. Log a warning and then just return the resource
             # provider object from _get_resource_provider()
@@ -368,17 +363,16 @@ class SchedulerReportClient(object):
             msg = msg.format(uuid)
             LOG.info(msg)
             return self._get_resource_provider(uuid)
-
-        # A provider with the same *name* already exists, or some other error.
-        msg = _LE("Failed to create resource provider record in placement API "
-                  "for UUID %(uuid)s. Got %(status_code)d: %(err_text)s.")
-        args = {
-            'uuid': uuid,
-            'status_code': resp.status_code,
-            'err_text': resp.text,
-        }
-        LOG.error(msg, args)
-        raise exception.ResourceProviderCreationFailed(name=name)
+        else:
+            msg = _LE("Failed to create resource provider record in "
+                      "placement API for UUID %(uuid)s. "
+                      "Got %(status_code)d: %(err_text)s.")
+            args = {
+                'uuid': uuid,
+                'status_code': resp.status_code,
+                'err_text': resp.text,
+            }
+            LOG.error(msg, args)
 
     def _ensure_resource_provider(self, uuid, name=None):
         """Ensures that the placement API has a record of a resource provider
@@ -389,11 +383,7 @@ class SchedulerReportClient(object):
         The found or created resource provider object is returned from this
         method. If the resource provider object for the supplied uuid was not
         found and the resource provider record could not be created in the
-        placement API, an exception is raised.
-
-        If this method returns successfully, callers are assured both that
-        the placement API contains a record of the provider and the local cache
-        of resource provider information contains a record of the provider.
+        placement API, we return None.
 
         :param uuid: UUID identifier for the resource provider to ensure exists
         :param name: Optional name for the resource provider if the record
@@ -416,8 +406,10 @@ class SchedulerReportClient(object):
 
         rp = self._get_resource_provider(uuid)
         if rp is None:
-            rp = self._create_resource_provider(uuid, name or uuid)
-
+            name = name or uuid
+            rp = self._create_resource_provider(uuid, name)
+            if rp is None:
+                return
         msg = "Grabbing aggregate associations for resource provider %s"
         LOG.debug(msg, uuid)
         aggs = self._get_provider_aggregates(uuid)
diff --git a/nova/tests/functional/api_sample_tests/api_samples/os-hypervisors/v2.33/hypervisors-detail-resp.json.tpl b/nova/tests/functional/api_sample_tests/api_samples/os-hypervisors/v2.33/hypervisors-detail-resp.json.tpl
index c15737123a..a45e6a2a07 100644
--- a/nova/tests/functional/api_sample_tests/api_samples/os-hypervisors/v2.33/hypervisors-detail-resp.json.tpl
+++ b/nova/tests/functional/api_sample_tests/api_samples/os-hypervisors/v2.33/hypervisors-detail-resp.json.tpl
@@ -22,7 +22,7 @@
             "host_ip": "%(ip)s",
             "free_disk_gb": 1028,
             "free_ram_mb": 7680,
-            "hypervisor_hostname": "host1",
+            "hypervisor_hostname": "fake-mini",
             "hypervisor_type": "fake",
             "hypervisor_version": 1000,
             "id": %(hypervisor_id)s,
diff --git a/nova/tests/functional/api_sample_tests/api_samples/os-hypervisors/v2.33/hypervisors-list-resp.json.tpl b/nova/tests/functional/api_sample_tests/api_samples/os-hypervisors/v2.33/hypervisors-list-resp.json.tpl
index 9a5771df02..3f221f12a7 100644
--- a/nova/tests/functional/api_sample_tests/api_samples/os-hypervisors/v2.33/hypervisors-list-resp.json.tpl
+++ b/nova/tests/functional/api_sample_tests/api_samples/os-hypervisors/v2.33/hypervisors-list-resp.json.tpl
@@ -1,7 +1,7 @@
 {
     "hypervisors": [
         {
-            "hypervisor_hostname": "host1",
+            "hypervisor_hostname": "fake-mini",
             "id": 2,
             "state": "up",
             "status": "enabled"
diff --git a/nova/tests/functional/api_sample_tests/test_hypervisors.py b/nova/tests/functional/api_sample_tests/test_hypervisors.py
index c56a38bd69..0b14343a70 100644
--- a/nova/tests/functional/api_sample_tests/test_hypervisors.py
+++ b/nova/tests/functional/api_sample_tests/test_hypervisors.py
@@ -18,7 +18,6 @@ import mock
 from nova.cells import utils as cells_utils
 from nova import objects
 from nova.tests.functional.api_sample_tests import api_sample_base
-from nova.virt import fake
 
 
 class HypervisorsSampleJsonTests(api_sample_base.ApiSampleTestBaseV21):
@@ -156,10 +155,7 @@ class HypervisorsSampleJson233Tests(api_sample_base.ApiSampleTestBaseV21):
         self.api.microversion = self.microversion
         # Start a new compute service to fake a record with hypervisor id=2
         # for pagination test.
-        host = 'host1'
-        fake.set_nodes([host])
-        self.addCleanup(fake.restore_nodes)
-        self.start_service('compute', host=host)
+        self.start_service('compute', host='host1')
 
     def test_hypervisors_list(self):
         response = self._do_get('os-hypervisors?limit=1&marker=1')
diff --git a/nova/tests/unit/scheduler/client/test_report.py b/nova/tests/unit/scheduler/client/test_report.py
index 5cb9d5ec89..29ee1ec7f1 100644
--- a/nova/tests/unit/scheduler/client/test_report.py
+++ b/nova/tests/unit/scheduler/client/test_report.py
@@ -238,19 +238,16 @@ class TestProviderOperations(SchedulerReportClientTestCase):
                 '_get_provider_aggregates')
     @mock.patch('nova.scheduler.client.report.SchedulerReportClient.'
                 '_get_resource_provider')
-    def test_ensure_resource_provider_create_fail(self, get_rp_mock,
+    def test_ensure_resource_provider_create_none(self, get_rp_mock,
             get_agg_mock, create_rp_mock):
         # No resource provider exists in the client's cache, and
-        # _create_provider raises, indicating there was an error with the
+        # _create_provider returns None, indicating there was an error with the
         # create call. Ensure we don't populate the resource provider cache
         # with a None value.
         get_rp_mock.return_value = None
-        create_rp_mock.side_effect = exception.ResourceProviderCreationFailed(
-            name=uuids.compute_node)
+        create_rp_mock.return_value = None
 
-        self.assertRaises(
-            exception.ResourceProviderCreationFailed,
-            self.client._ensure_resource_provider, uuids.compute_node)
+        self.client._ensure_resource_provider(uuids.compute_node)
 
         get_rp_mock.assert_called_once_with(uuids.compute_node)
         create_rp_mock.assert_called_once_with(uuids.compute_node,
@@ -394,9 +391,7 @@ class TestProviderOperations(SchedulerReportClientTestCase):
         self.ks_sess_mock.get.return_value = resp_mock
 
         uuid = uuids.compute_node
-        self.assertRaises(
-            exception.ResourceProviderRetrievalFailed,
-            self.client._get_resource_provider, uuid)
+        result = self.client._get_resource_provider(uuid)
 
         expected_url = '/resource_providers/' + uuid
         self.ks_sess_mock.get.assert_called_once_with(expected_url,
@@ -405,6 +400,7 @@ class TestProviderOperations(SchedulerReportClientTestCase):
         # A 503 Service Unavailable should trigger an error logged and
         # return None from _get_resource_provider()
         self.assertTrue(logging_mock.called)
+        self.assertIsNone(result)
 
     def test_create_resource_provider(self):
         # Ensure _create_resource_provider() returns a ResourceProvider object
@@ -445,9 +441,8 @@ class TestProviderOperations(SchedulerReportClientTestCase):
         # record.
         uuid = uuids.compute_node
         name = 'computehost'
-        self.ks_sess_mock.post.return_value = mock.Mock(
-            status_code=409,
-            text='not a name conflict')
+        resp_mock = mock.Mock(status_code=409)
+        self.ks_sess_mock.post.return_value = resp_mock
 
         get_rp_mock.return_value = mock.sentinel.get_rp
 
@@ -465,18 +460,6 @@ class TestProviderOperations(SchedulerReportClientTestCase):
                 raise_exc=False)
         self.assertEqual(mock.sentinel.get_rp, result)
 
-    def test_create_resource_provider_name_conflict(self):
-        # When the API call to create the resource provider fails 409 with a
-        # name conflict, we raise an exception.
-        self.ks_sess_mock.post.return_value = mock.Mock(
-            status_code=409,
-            text='<stuff>Conflicting resource provider name: foo already '
-                 'exists.</stuff>')
-
-        self.assertRaises(
-            exception.ResourceProviderCreationFailed,
-            self.client._create_resource_provider, uuids.compute_node, 'foo')
-
     @mock.patch.object(report.LOG, 'error')
     def test_create_resource_provider_error(self, logging_mock):
         # Ensure _create_resource_provider() sets the error flag when trying to
@@ -487,9 +470,7 @@ class TestProviderOperations(SchedulerReportClientTestCase):
         resp_mock = mock.Mock(status_code=503)
         self.ks_sess_mock.post.return_value = resp_mock
 
-        self.assertRaises(
-            exception.ResourceProviderCreationFailed,
-            self.client._create_resource_provider, uuid, name)
+        result = self.client._create_resource_provider(uuid, name)
 
         expected_payload = {
             'uuid': uuid,
@@ -504,6 +485,7 @@ class TestProviderOperations(SchedulerReportClientTestCase):
         # A 503 Service Unavailable should log an error and
         # _create_resource_provider() should return None
         self.assertTrue(logging_mock.called)
+        self.assertFalse(result)
 
 
 class TestAggregates(SchedulerReportClientTestCase):