openstack/placement
Code Issues Proposed changes

Don't use OVO with ResourceProvider and ResourceProviderList

Browse Source 
Turn ResourceProvider and ResourceProviderList into
classical Python objects.

There are two changes here which deserve particular
attention because they are more changing than the other
OVO removals:

* There were two deepcopy calls in the allocation request
  handling. When using OVO, the __deepcopy__ handling
  built into it prevents deep recursion. I changed them
  to copy and things still work as expected. This will
  be because using the nested objects by reference is
  acceptable.

* The removal of OVO removes the detection of changed fields.
  This was being used when creating and saving resource
  providers (at the object level) to automagically detect
  and prevent writing fields we don't want to.

  This change removes that functionality. Instead if bad
  data has made it as far as the create or save calls, we
  simply don't write it. The HTTP layer continues to
  maintain the guards it already had in place to prevent
  badness. Tests which were testing the object layer
  are removed.

  The create_provider function in functional/db/test_base
  allowed (trying to) create a provider with a root
  but that functionality was only called from one place.
  That place and the functionality in create_provider
  is removed.

Other things to note:

* As with the other changes, where context is actually used
  by the object it is required in the constructor. This
  cascades some changes to tests.

* A test that checks to see if adding traits resets the
  changes on a rp has been removed, because we don't
  track that any more if we haven't got OVO.

* The last_modified handling in placement/util no longer
  need NotImplemented handling, that was an artifact of
  OVO.

oslo.versionedobjects is removed from requirements. This
doesn't have a huge impact on the size of the virtualenv:
114M -> 107M [1] but it does take us from 132 python
dependencies (via pip list) to 119, removing plenty of
stuff that was only being called in because stuff that
we don't use depended on it.

lower-constraints.txt is updated to reflect the removal
of dependencies that are no longer needed.

[1] Of note, 26M of that is babel. Do we need to translate
exceptions? See email disussion at
http://lists.openstack.org/pipermail/openstack-discuss/2019-February/thread.html#3002

Change-Id: Ie0a9351e0d7c762c9c96c45cbe50132a0fbd1486
This commit is contained in:
Chris Dent
2019-02-14 01:01:15 +00:00
parent 75555e7e2c
commit bed7808b59
9 changed files with 78 additions and 132 deletions
Download Patch File Download Diff File Expand all files Collapse all files

17
lower-constraints.txt
View File

@@ -1,26 +1,20 @@
alembic==0.9.8
amqp==2.2.2
appdirs==1.4.3
attrs==17.4.0
Babel==2.3.4
bandit==1.1.0
cachetools==2.0.1
colorama==0.3.9
contextlib2==0.5.5
coverage==4.0
debtcollector==1.19.0
decorator==3.4.0
eventlet==0.18.2
extras==1.0.0
fasteners==0.14.1
fixtures==3.0.0
flake8==2.5.5
future==0.16.0
futurist==1.6.0
gabbi==1.35.0
gitdb2==2.0.3
GitPython==2.1.8
greenlet==0.4.10
hacking==0.12.0
iso8601==0.1.11
Jinja2==2.10
@@ -29,7 +23,6 @@ jsonpath-rw-ext==1.1.3
jsonschema==2.6.0
keystoneauth1==3.9.0
keystonemiddleware==4.18.0
kombu==4.1.0
linecache2==1.0.0
Mako==1.0.7
MarkupSafe==1.0
@@ -41,8 +34,8 @@ mox3==0.20.0
msgpack-python==0.5.6
netaddr==0.7.18
netifaces==0.10.4
os-resource-classes==0.2.0
os-client-config==1.29.0
os-resource-classes==0.2.0
os-service-types==1.2.0
os-traits==0.4.0
oslo.concurrency==3.26.0
@@ -51,17 +44,12 @@ oslo.context==2.19.2
oslo.db==4.40.0
oslo.i18n==3.15.3
oslo.log==3.36.0
oslo.messaging==6.3.0
oslo.middleware==3.31.0
oslo.policy==1.35.0
oslo.serialization==2.18.0
oslo.service==1.24.0
oslo.upgradecheck==0.2.0
oslo.utils==3.37.0
oslo.versionedobjects==1.31.2
oslotest==3.4.0
Paste==2.0.2
PasteDeploy==1.5.2
pbr==2.0.0
pep8==1.5.7
pluggy==0.6.0
@@ -71,7 +59,6 @@ psycopg2==2.7
py==1.5.2
pycadf==2.7.0
pyflakes==0.8.1
pyinotify==0.9.6
PyMySQL==0.7.6
pyparsing==2.2.0
pytest==3.4.2
@@ -95,7 +82,6 @@ statsd==3.2.2
stestr==1.0.0
stevedore==1.20.0
Tempita==0.5.2
tenacity==4.9.0
testrepository==0.0.20
testresources==2.0.0
testscenarios==0.4
@@ -103,7 +89,6 @@ testtools==2.2.0
traceback2==1.4.0
unittest2==1.1.0
urllib3==1.22
vine==1.1.4
WebOb==1.8.2
wrapt==1.10.11
wsgi-intercept==1.7.0

102
placement/objects/resource_provider.py
View File

@@ -28,8 +28,6 @@ from oslo_db import api as oslo_db_api
from oslo_db import exception as db_exc
from oslo_log import log as logging
from oslo_utils import encodeutils
from oslo_versionedobjects import base
from oslo_versionedobjects import fields
import six
import sqlalchemy as sa
from sqlalchemy import exc as sqla_exc
@@ -975,15 +973,17 @@ def _delete_rp_record(context, _id):
delete(synchronize_session=False)
@base.VersionedObjectRegistry.register_if(False)
class ResourceProvider(base.VersionedObject, base.TimestampedObject):
class ResourceProvider(object):
SETTABLE_FIELDS = ('name', 'parent_provider_uuid')
fields = {
'id': fields.IntegerField(read_only=True),
'uuid': fields.UUIDField(nullable=False),
'name': fields.StringField(nullable=False),
'generation': fields.IntegerField(nullable=False),
def __init__(self, context, id=None, uuid=None, name=None,
generation=None, parent_provider_uuid=None,
root_provider_uuid=None, updated_at=None, created_at=None):
self._context = context
self.id = id
self.uuid = uuid
self.name = name
self.generation = generation
# UUID of the root provider in a hierarchy of providers. Will be equal
# to the uuid field if this provider is the root provider of a
# hierarchy. This field is never manually set by the user. Instead, it
@@ -992,44 +992,44 @@ class ResourceProvider(base.VersionedObject, base.TimestampedObject):
# is an optimization field that allows us to very quickly query for all
# providers within a particular tree without doing any recursive
# querying.
'root_provider_uuid': fields.UUIDField(nullable=False),
self.root_provider_uuid = root_provider_uuid
# UUID of the direct parent provider, or None if this provider is a
# "root" provider.
'parent_provider_uuid': fields.UUIDField(nullable=True, default=None),
}
self.parent_provider_uuid = parent_provider_uuid
self.updated_at = updated_at
self.created_at = created_at
def create(self):
if 'id' in self:
if self.id is not None:
raise exception.ObjectActionError(action='create',
reason='already created')
if 'uuid' not in self:
if self.uuid is None:
raise exception.ObjectActionError(action='create',
reason='uuid is required')
if 'name' not in self:
if not self.name:
raise exception.ObjectActionError(action='create',
reason='name is required')
if 'root_provider_uuid' in self:
raise exception.ObjectActionError(
action='create',
reason=_('root provider UUID cannot be manually set.'))
self.obj_set_defaults()
updates = self.obj_get_changes()
# These are the only fields we are willing to create with.
# If there are others, ignore them.
updates = {
'name': self.name,
'uuid': self.uuid,
'parent_provider_uuid': self.parent_provider_uuid,
}
self._create_in_db(self._context, updates)
self.obj_reset_changes()
def destroy(self):
self._delete(self._context, self.id)
def save(self):
updates = self.obj_get_changes()
if updates and any(k not in self.SETTABLE_FIELDS
for k in updates.keys()):
raise exception.ObjectActionError(
action='save',
reason='Immutable fields changed')
# These are the only fields we are willing to save with.
# If there are others, ignore them.
updates = {
'name': self.name,
'parent_provider_uuid': self.parent_provider_uuid,
}
self._update_in_db(self._context, self.id, updates)
self.obj_reset_changes()
@classmethod
def get_by_uuid(cls, context, uuid):
@@ -1039,7 +1039,7 @@ class ResourceProvider(base.VersionedObject, base.TimestampedObject):
:param uuid: UUID of the provider to search for
"""
rp_rec = _get_provider_by_uuid(context, uuid)
return cls._from_db_object(context, cls(), rp_rec)
return cls._from_db_object(context, cls(context), rp_rec)
def add_inventory(self, inventory):
"""Add one new Inventory to the resource provider.
@@ -1048,12 +1048,10 @@ class ResourceProvider(base.VersionedObject, base.TimestampedObject):
already present.
"""
_add_inventory(self._context, self, inventory)
self.obj_reset_changes()
def delete_inventory(self, resource_class):
"""Delete Inventory of provided resource_class."""
_delete_inventory(self._context, self, resource_class)
self.obj_reset_changes()
def set_inventory(self, inv_list):
"""Set all resource provider Inventory to be the provided list."""
@@ -1062,7 +1060,6 @@ class ResourceProvider(base.VersionedObject, base.TimestampedObject):
LOG.warning('Resource provider %(uuid)s is now over-'
'capacity for %(resource)s',
{'uuid': uuid, 'resource': rclass})
self.obj_reset_changes()
def update_inventory(self, inventory):
"""Update one existing Inventory of the same resource class.
@@ -1074,7 +1071,6 @@ class ResourceProvider(base.VersionedObject, base.TimestampedObject):
LOG.warning('Resource provider %(uuid)s is now over-'
'capacity for %(resource)s',
{'uuid': uuid, 'resource': rclass})
self.obj_reset_changes()
def get_aggregates(self):
"""Get the aggregate uuids associated with this resource provider."""
@@ -1101,7 +1097,6 @@ class ResourceProvider(base.VersionedObject, base.TimestampedObject):
associate with the provider.
"""
_set_traits(self._context, self, traits)
self.obj_reset_changes()
@db_api.placement_context_manager.writer
def _create_in_db(self, context, updates):
@@ -1291,10 +1286,10 @@ class ResourceProvider(base.VersionedObject, base.TimestampedObject):
uuid = db_resource_provider['uuid']
db_resource_provider['root_provider_uuid'] = uuid
_set_root_provider_id(context, rp_id, rp_id)
for field in resource_provider.fields:
for field in ['id', 'uuid', 'name', 'generation',
'root_provider_uuid', 'parent_provider_uuid',
'updated_at', 'created_at']:
setattr(resource_provider, field, db_resource_provider[field])
resource_provider._context = context
resource_provider.obj_reset_changes()
return resource_provider
@@ -1425,12 +1420,25 @@ def _get_providers_with_shared_capacity(ctx, rc_id, amount, member_of=None):
return [r[0] for r in ctx.session.execute(sel)]
@base.VersionedObjectRegistry.register_if(False)
class ResourceProviderList(base.ObjectListBase, base.VersionedObject):
class ResourceProviderList(object):
fields = {
'objects': fields.ListOfObjectsField('ResourceProvider'),
}
def __init__(self, objects=None):
self.objects = objects or []
def __len__(self):
"""List length is a proxy for truthiness."""
return len(self.objects)
def __getitem__(self, index):
return self.objects[index]
# FIXME(cdent): There are versions of this that need context
# and versions that don't. Unify into a super class.
@staticmethod
def _set_objects(context, list_obj, item_cls, db_list):
for db_item in db_list:
list_obj.objects.append(item_cls(context, **db_item))
return list_obj
@staticmethod
@db_api.placement_context_manager.reader
@@ -1623,8 +1631,8 @@ class ResourceProviderList(base.ObjectListBase, base.VersionedObject):
:type filters: dict
"""
resource_providers = cls._get_all_by_filters_from_db(context, filters)
return base.obj_make_list(context, cls(context),
ResourceProvider, resource_providers)
return cls._set_objects(context, cls(), ResourceProvider,
resource_providers)
class Inventory(object):
@@ -3636,7 +3644,7 @@ def _alloc_candidates_single_provider(ctx, requested_resources, rp_tuples):
# We already added self
if anchor == rp_summary.resource_provider.root_provider_uuid:
continue
req_obj = copy.deepcopy(req_obj)
req_obj = copy.copy(req_obj)
req_obj.anchor_root_provider_uuid = anchor
alloc_requests.append(req_obj)
return alloc_requests, list(summaries.values())
@@ -3838,7 +3846,7 @@ def _consolidate_allocation_requests(areqs):
for arr in areq.resource_requests:
key = _rp_rc_key(arr.resource_provider, arr.resource_class)
if key not in arrs_by_rp_rc:
arrs_by_rp_rc[key] = copy.deepcopy(arr)
arrs_by_rp_rc[key] = copy.copy(arr)
else:
arrs_by_rp_rc[key].amount += arr.amount
return AllocationRequest(

3
placement/tests/functional/db/test_base.py
View File

@@ -26,13 +26,10 @@ from placement.tests.functional import base
def create_provider(context, name, *aggs, **kwargs):
parent = kwargs.get('parent')
root = kwargs.get('root')
uuid = kwargs.get('uuid', getattr(uuids, name))
rp = rp_obj.ResourceProvider(context, name=name, uuid=uuid)
if parent:
rp.parent_provider_uuid = parent
if root:
rp.root_provider_uuid = root
rp.create()
if aggs:
rp.set_aggregates(aggs)

9
placement/tests/functional/db/test_resource_provider.py
View File

@@ -240,15 +240,6 @@ class ResourceProviderTestCase(tb.PlacementDbBaseTestCase):
self.assertEqual(uuidsentinel.parent, rp1.root_provider_uuid)
def test_save_root_provider_failed(self):
"""Test that if we provide a root_provider_uuid value that points to
a resource provider that doesn't exist, we get an ObjectActionError if
we save the object.
"""
self.assertRaises(
exception.ObjectActionError,
self._create_provider, 'rp1', root=uuidsentinel.noexists)
def test_save_unknown_parent_provider(self):
"""Test that if we provide a parent_provider_uuid value that points to
a resource provider that doesn't exist, that we get an

5
placement/tests/unit/handlers/test_aggregate.py
View File

@@ -16,6 +16,7 @@ import six
import testtools
import webob
from placement import context
from placement import exception
from placement.handlers import aggregate
from placement.objects import resource_provider
@@ -27,7 +28,9 @@ class TestAggregateHandlerErrors(testtools.TestCase):
"""
def test_concurrent_exception_causes_409(self):
rp = resource_provider.ResourceProvider()
fake_context = context.RequestContext(
user_id='fake', project_id='fake')
rp = resource_provider.ResourceProvider(fake_context)
expected_message = ('Update conflict: Another thread concurrently '
'updated the data')
with mock.patch.object(rp, "set_aggregates",

52
placement/tests/unit/objects/test_resource_provider.py
View File

@@ -147,29 +147,6 @@ class TestResourceProviderNoDB(_TestCase):
self.assertRaises(exception.ObjectActionError,
obj.create)
def test_create_with_root_provider_uuid_fail(self):
obj = resource_provider.ResourceProvider(
context=self.context,
uuid=_RESOURCE_PROVIDER_UUID,
name=_RESOURCE_PROVIDER_NAME,
root_provider_uuid=_RESOURCE_PROVIDER_UUID,
)
exc = self.assertRaises(exception.ObjectActionError, obj.create)
self.assertIn('root provider UUID cannot be manually set', str(exc))
def test_save_immutable(self):
fields = {
'id': 1,
'uuid': _RESOURCE_PROVIDER_UUID,
'generation': 1,
'root_provider_uuid': _RESOURCE_PROVIDER_UUID,
}
for field in fields:
rp = resource_provider.ResourceProvider(context=self.context)
setattr(rp, field, fields[field])
self.assertRaises(exception.ObjectActionError, rp.save)
class TestProviderSummaryNoDB(_TestCase):
@@ -198,7 +175,8 @@ class TestInventoryNoDB(_TestCase):
id=_INVENTORY_DB['id'] + 1,
resource_provider_id=_RESOURCE_PROVIDER_ID)]
mock_get.return_value = expected
rp = resource_provider.ResourceProvider(id=_RESOURCE_PROVIDER_ID,
rp = resource_provider.ResourceProvider(self.context,
id=_RESOURCE_PROVIDER_ID,
uuid=_RESOURCE_PROVIDER_UUID)
objs = resource_provider.InventoryList.get_all_by_resource_provider(
self.context, rp)
@@ -208,7 +186,8 @@ class TestInventoryNoDB(_TestCase):
self.assertEqual(_RESOURCE_PROVIDER_ID, objs[0].resource_provider.id)
def test_set_defaults(self):
rp = resource_provider.ResourceProvider(id=_RESOURCE_PROVIDER_ID,
rp = resource_provider.ResourceProvider(self.context,
id=_RESOURCE_PROVIDER_ID,
uuid=_RESOURCE_PROVIDER_UUID)
kwargs = dict(resource_provider=rp,
resource_class=_RESOURCE_CLASS_NAME,
@@ -222,7 +201,8 @@ class TestInventoryNoDB(_TestCase):
self.assertEqual(1.0, inv.allocation_ratio)
def test_capacity(self):
rp = resource_provider.ResourceProvider(id=_RESOURCE_PROVIDER_ID,
rp = resource_provider.ResourceProvider(self.context,
id=_RESOURCE_PROVIDER_ID,
uuid=_RESOURCE_PROVIDER_UUID)
kwargs = dict(resource_provider=rp,
resource_class=_RESOURCE_CLASS_NAME,
@@ -240,7 +220,8 @@ class TestInventoryNoDB(_TestCase):
class TestInventoryList(_TestCase):
def test_find(self):
rp = resource_provider.ResourceProvider(uuid=uuids.rp_uuid)
rp = resource_provider.ResourceProvider(
self.context, uuid=uuids.rp_uuid)
inv_list = resource_provider.InventoryList(objects=[
resource_provider.Inventory(
resource_provider=rp,
@@ -283,7 +264,8 @@ class TestAllocationListNoDB(_TestCase):
return_value=[_ALLOCATION_DB])
def test_get_all_by_resource_provider(self, mock_get_allocations_from_db,
mock_create_consumers):
rp = resource_provider.ResourceProvider(id=_RESOURCE_PROVIDER_ID,
rp = resource_provider.ResourceProvider(self.context,
id=_RESOURCE_PROVIDER_ID,
uuid=uuids.resource_provider)
rp_alloc_list = resource_provider.AllocationList
allocations = rp_alloc_list.get_all_by_resource_provider(
@@ -350,20 +332,6 @@ class TestTraits(_TestCase):
synced = resource_provider._TRAITS_SYNCED
self.assertTrue(synced)
@mock.patch('placement.objects.resource_provider.'
'ResourceProvider.obj_reset_changes')
@mock.patch('placement.objects.resource_provider.'
'_set_traits')
def test_set_traits_resets_changes(self, mock_set_traits, mock_reset):
trait = resource_provider.Trait(self.context, name="HW_CPU_X86_AVX2")
traits = resource_provider.TraitList(objects=[trait])
rp = resource_provider.ResourceProvider(self.context, name='cn1',
uuid=uuids.cn1)
rp.set_traits(traits)
mock_set_traits.assert_called_once_with(self.context, rp, traits)
mock_reset.assert_called_once_with()
class TestAllocationCandidatesNoDB(_TestCase):
def test_limit_results(self):

11
placement/tests/unit/test_util.py
View File

@@ -286,11 +286,12 @@ class TestPlacementURLs(testtools.TestCase):
def setUp(self):
super(TestPlacementURLs, self).setUp()
self.resource_provider = rp_obj.ResourceProvider(
name=uuidsentinel.rp_name,
uuid=uuidsentinel.rp_uuid)
fake_context = context.RequestContext(
user_id='fake', project_id='fake')
self.resource_provider = rp_obj.ResourceProvider(
fake_context,
name=uuidsentinel.rp_name,
uuid=uuidsentinel.rp_uuid)
self.resource_class = rp_obj.ResourceClass(
fake_context,
name='CUSTOM_BAREMETAL_GOLD',
@@ -824,8 +825,10 @@ class TestPickLastModified(testtools.TestCase):
def setUp(self):
super(TestPickLastModified, self).setUp()
fake_context = context.RequestContext(
user_id='fake', project_id='fake')
self.resource_provider = rp_obj.ResourceProvider(
name=uuidsentinel.rp_name, uuid=uuidsentinel.rp_uuid)
fake_context, name=uuidsentinel.rp_name, uuid=uuidsentinel.rp_uuid)
def test_updated_versus_none(self):
now = timeutils.utcnow(with_timezone=True)

10
placement/util.py
View File

@@ -138,15 +138,7 @@ def pick_last_modified(last_modified, obj):
If updated_at is not implemented in `obj` use the current time in UTC.
"""
try:
current_modified = (obj.updated_at or obj.created_at)
# TODO(cdent): NotImplementedError catching can go away when all of
# OVO is gone.
except NotImplementedError:
# If updated_at is not implemented, we are looking at objects that
# have not come from the database, so "now" is the right modified
# time.
current_modified = timeutils.utcnow(with_timezone=True)
current_modified = (obj.updated_at or obj.created_at)
if current_modified is None:
# The object was not loaded from the DB, it was created in
# the current context.

1
requirements.txt
View File

@@ -22,7 +22,6 @@ oslo.policy>=1.35.0 # Apache-2.0
oslo.i18n>=3.15.3 # Apache-2.0
oslo.middleware>=3.31.0 # Apache-2.0
oslo.upgradecheck>=0.2.0 # Apache-2.0
oslo.versionedobjects>=1.31.2 # Apache-2.0
os-resource-classes>=0.2.0 # Apache-2.0
os-traits>=0.4.0 # Apache-2.0
microversion-parse>=0.2.1 # Apache-2.0