Make _Provider really private
By design, nova.compute.provider_tree._Provider is intended to be accessed only through ProviderTree interfaces to ensure thread safety. However, certain ProviderTree methods were returning _Provider objects, allowing callers to think it was okay to access their internals directly. With this change, we truly hide _Provider behind ProviderTree. As long as callers are well-behaved (accessing only public methods of public classes), there is no longer any way to access _Provider internals. Summary of the changes: - ProviderTree.find is removed. - ProviderTree.new_root and .new_child now return the UUID of the newly-created _Provider, rather than the _Provider itself. (We return the UUID because it may have been generated; and there is currently no other way for the caller to discover it.) Note: It is possible that future changes will be necessary to provide (read-only) access to some _Provider attributes. No such accessors are implemented herein. Change-Id: If5eed34cb5a2cd94a6305a7626adfe7129119e44 blueprint: nested-resource-providers
This commit is contained in:
parent
6ee125ca81
commit
1f41326ee6
@ -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
|
||||
|
@ -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.
|
||||
|
@ -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):
|
||||
|
@ -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.'
|
||||
|
Loading…
Reference in New Issue
Block a user