diff --git a/nova/compute/provider_tree.py b/nova/compute/provider_tree.py index 323faaa4551c..f2f1f29d9482 100644 --- a/nova/compute/provider_tree.py +++ b/nova/compute/provider_tree.py @@ -146,7 +146,7 @@ class ProviderTree(object): self.roots.remove(found) def new_root(self, name, uuid, generation): - """Adds a new root provider to the tree.""" + """Adds a new root provider to the tree, returning its UUID.""" with self.lock: exists = True @@ -159,9 +159,9 @@ class ProviderTree(object): err = _("Provider %s already exists as a root.") raise ValueError(err % uuid) - p = _Provider(name, uuid, generation) + p = _Provider(name, uuid=uuid, generation=generation) self.roots.append(p) - return p + return p.uuid def _find_with_lock(self, name_or_uuid): for root in self.roots: @@ -170,31 +170,22 @@ class ProviderTree(object): return found raise ValueError(_("No such provider %s") % name_or_uuid) - def find(self, name_or_uuid): - """Search for a provider with the given name or UUID. - - :raises ValueError if name_or_uuid points to a non-existing provider. - :param name_or_uuid: Either name or UUID of the resource provider to - search for. - """ - with self.lock: - return self._find_with_lock(name_or_uuid) - def exists(self, name_or_uuid): """Given either a name or a UUID, return True if the tree contains the provider, False otherwise. """ - try: - self.find(name_or_uuid) - return True - except ValueError: - return False + with self.lock: + try: + self._find_with_lock(name_or_uuid) + return True + except ValueError: + return False def new_child(self, name, parent_uuid, uuid=None, generation=None): """Creates a new child provider with the given name and uuid under the given parent. - :returns: the new provider + :returns: the UUID of the new provider :raises ValueError if parent_uuid points to a non-existing provider. """ @@ -202,7 +193,7 @@ class ProviderTree(object): parent = self._find_with_lock(parent_uuid) p = _Provider(name, uuid, generation, parent_uuid) parent.add_child(p) - return p + return p.uuid def has_inventory(self, name_or_uuid): """Returns True if the provider identified by name_or_uuid has any diff --git a/nova/scheduler/client/report.py b/nova/scheduler/client/report.py index bf1e72f4e12c..ad09c45ae6ea 100644 --- a/nova/scheduler/client/report.py +++ b/nova/scheduler/client/report.py @@ -489,10 +489,10 @@ class SchedulerReportClient(object): the placement API for the supplied UUID, passing in a name for the resource provider. - If found or created, an object representing the provider (and potential - child providers) is returned from this method. If the resource provider - 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 found or created, the provider's UUID is returned from this method. + If the resource provider 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 tree @@ -503,12 +503,9 @@ class SchedulerReportClient(object): does not exist. If empty, the name is set to the UUID value """ - try: - rp = self._provider_tree.find(uuid) + if self._provider_tree.exists(uuid): self._refresh_aggregate_map(uuid) - return rp - except ValueError: - pass + return uuid # No local information about the resource provider in our tree. Check # the placement API. diff --git a/nova/tests/unit/compute/test_provider_tree.py b/nova/tests/unit/compute/test_provider_tree.py index 2dc40b2d18d2..81535bf415e7 100644 --- a/nova/tests/unit/compute/test_provider_tree.py +++ b/nova/tests/unit/compute/test_provider_tree.py @@ -50,20 +50,17 @@ class TestProviderTree(test.NoDBTestCase): self.assertFalse(pt.exists(uuids.non_existing_rp)) self.assertFalse(pt.exists('noexist')) - numa_cell0 = pt.new_child('numa_cell0', cn1.uuid) - numa_cell1 = pt.new_child('numa_cell1', cn1.uuid) + numa_cell0_uuid = pt.new_child('numa_cell0', cn1.uuid) + numa_cell1_uuid = pt.new_child('numa_cell1', cn1.uuid) - self.assertEqual(numa_cell0, pt.find('numa_cell0')) - self.assertEqual(numa_cell0, pt.find(numa_cell0.uuid)) - - self.assertTrue(pt.exists(numa_cell0.uuid)) + self.assertTrue(pt.exists(numa_cell0_uuid)) self.assertTrue(pt.exists('numa_cell0')) - self.assertTrue(pt.exists(numa_cell1.uuid)) + self.assertTrue(pt.exists(numa_cell1_uuid)) self.assertTrue(pt.exists('numa_cell1')) - pf1_cell0 = pt.new_child('pf1_cell0', numa_cell0.uuid) - self.assertTrue(pt.exists(pf1_cell0.uuid)) + pf1_cell0_uuid = pt.new_child('pf1_cell0', numa_cell0_uuid) + self.assertTrue(pt.exists(pf1_cell0_uuid)) self.assertTrue(pt.exists('pf1_cell0')) self.assertRaises( @@ -98,21 +95,16 @@ class TestProviderTree(test.NoDBTestCase): uuids.non_existing_rp, ) - # Save numa cell1 uuid since removing will invalidate the object - cell0_uuid = numa_cell0.uuid - cell1_uuid = numa_cell1.uuid - pf1_uuid = pf1_cell0.uuid - - pt.remove(cell1_uuid) - self.assertFalse(pt.exists(cell1_uuid)) - self.assertTrue(pt.exists(pf1_uuid)) - self.assertTrue(pt.exists(cell0_uuid)) + pt.remove(numa_cell1_uuid) + self.assertFalse(pt.exists(numa_cell1_uuid)) + self.assertTrue(pt.exists(pf1_cell0_uuid)) + self.assertTrue(pt.exists(numa_cell0_uuid)) self.assertTrue(pt.exists(uuids.cn1)) # Now remove the root and check that children no longer exist pt.remove(uuids.cn1) - self.assertFalse(pt.exists(pf1_uuid)) - self.assertFalse(pt.exists(cell0_uuid)) + self.assertFalse(pt.exists(pf1_cell0_uuid)) + self.assertFalse(pt.exists(numa_cell0_uuid)) self.assertFalse(pt.exists(uuids.cn1)) def test_has_inventory_changed_no_existing_rp(self): diff --git a/nova/tests/unit/scheduler/client/test_report.py b/nova/tests/unit/scheduler/client/test_report.py index 378a17a3b12c..6db119aa86d7 100644 --- a/nova/tests/unit/scheduler/client/test_report.py +++ b/nova/tests/unit/scheduler/client/test_report.py @@ -230,12 +230,41 @@ class SchedulerReportClientTestCase(test.NoDBTestCase): }, } generation = generation_override or 1 - rp = self.client._provider_tree.new_root( + rp_uuid = self.client._provider_tree.new_root( cn.hypervisor_hostname, cn.uuid, generation, ) - rp.update_inventory(resources, generation) + self.client._provider_tree.update_inventory(rp_uuid, resources, + generation) + + def _validate_provider(self, name_or_uuid, **kwargs): + """Validates existence and values of a provider in this client's + _provider_tree. + + _Provider is deliberately hidden, so this method has to cheat, using + private methods to access the _Provider directly. + + :param name_or_uuid: The name or UUID of the provider to validate. + :param kwargs: Optional keyword arguments of internal _Provider + attributes whose values are to be validated. + """ + pt = self.client._provider_tree + # This locking is overkill, but good form + with pt.lock: + try: + found = pt._find_with_lock(name_or_uuid) + except ValueError: + self.fail("Provider with name or UUID %s doesn't exist" % + name_or_uuid) + # If kwargs provided, their names indicate _Provider attributes + for attr, expected in kwargs.items(): + try: + self.assertEqual(getattr(found, attr), expected) + except AttributeError: + self.fail("Provider with name or UUID %s doesn't have " + "attribute %s (expected value: %s)" % + (name_or_uuid, attr, expected)) class TestPutAllocations(SchedulerReportClientTestCase): @@ -1117,7 +1146,6 @@ class TestProviderOperations(SchedulerReportClientTestCase): # No resource provider exists in the client's cache, and # _create_provider raises, 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) @@ -1688,8 +1716,7 @@ class TestInventory(SchedulerReportClientTestCase): self.assertIsNone(result) self.assertFalse(mock_delete.called) self.assertFalse(mock_extract.called) - rp = self.client._provider_tree.find(cn.uuid) - self.assertEqual(1, rp.generation) + self._validate_provider(cn.uuid, generation=1) @mock.patch.object(report.LOG, 'info') @mock.patch('nova.scheduler.client.report.SchedulerReportClient.' @@ -1727,8 +1754,7 @@ class TestInventory(SchedulerReportClientTestCase): self.assertTrue(mock_info.called) self.assertEqual(uuids.request_id, mock_info.call_args[0][1]['placement_req_id']) - rp = self.client._provider_tree.find(cn.uuid) - self.assertEqual(44, rp.generation) + self._validate_provider(cn.uuid, generation=44) @mock.patch('nova.scheduler.client.report.SchedulerReportClient.' 'put') @@ -1795,10 +1821,9 @@ class TestInventory(SchedulerReportClientTestCase): uuids.request_id} self.client._delete_inventory(cn.uuid) self.assertTrue(mock_debug.called) - rp = self.client._provider_tree.find(cn.uuid) exp_url = '/resource_providers/%s/inventories' % cn.uuid payload = { - 'resource_provider_generation': rp.generation, + 'resource_provider_generation': 42, 'inventories': {}, } mock_put.assert_called_once_with(exp_url, payload) @@ -1882,7 +1907,7 @@ There was a conflict when trying to complete your request. uuids.request_id} result = self.client._delete_inventory(cn.uuid) self.assertIsNone(result) - self.assertRaises(ValueError, self.client._provider_tree.find, cn.uuid) + self.assertFalse(self.client._provider_tree.exists(cn.uuid)) self.assertTrue(mock_debug.called) self.assertIn('deleted by another thread', mock_debug.call_args[0][0]) self.assertEqual(uuids.request_id, @@ -1965,8 +1990,7 @@ There was a conflict when trying to complete your request. exp_url = '/resource_providers/%s/inventories' % uuid mock_get.assert_called_once_with(exp_url) # Updated with the new inventory from the PUT call - rp = self.client._provider_tree.find(uuid) - self.assertEqual(44, rp.generation) + self._validate_provider(uuid, generation=44) expected = { # Called with the newly-found generation from the existing # inventory @@ -2042,8 +2066,7 @@ There was a conflict when trying to complete your request. exp_url = '/resource_providers/%s/inventories' % uuid mock_get.assert_called_once_with(exp_url) # Updated with the new inventory from the PUT call - rp = self.client._provider_tree.find(uuid) - self.assertEqual(44, rp.generation) + self._validate_provider(uuid, generation=44) expected = { # Called with the newly-found generation from the existing # inventory @@ -2130,8 +2153,7 @@ There was a conflict when trying to complete your request. # No update so put should not be called self.assertFalse(mock_put.called) # Make sure we updated the generation from the inventory records - rp = self.client._provider_tree.find(compute_node.uuid) - self.assertEqual(43, rp.generation) + self._validate_provider(uuid, generation=43) @mock.patch.object(report.LOG, 'info') @mock.patch('nova.scheduler.client.report.SchedulerReportClient.' @@ -2211,7 +2233,7 @@ There was a conflict when trying to complete your request. ) # Did NOT invalidate the cache - self.assertIsNotNone(self.client._provider_tree.find(uuid)) + self.assertTrue(self.client._provider_tree.exists(uuid)) @mock.patch.object(report.LOG, 'info') @mock.patch('nova.scheduler.client.report.SchedulerReportClient.'