From c2ba0ef21e60474d48733b4869b54327bcb413e3 Mon Sep 17 00:00:00 2001 From: Dan Smith Date: Fri, 10 Jan 2020 13:55:25 -0800 Subject: [PATCH] Add NovaEphemeralObject class for non-persistent objects This adds a NovaEphemeralObject class, which inherits from NovaObject and also from EphemeralObject. The latter is proposed against o.vo, and is copied here for expediency. In the past, all of our objects were representations of rows of columns in the database and thus it was very important to not default fields in objects when working on a record remotely that may have values in the database for those fields which we did not wish to overwrite. Lately, we have a lot of objects that are used purely over RPC, or which are stored in the database as a blob and for which the explicit defaulting nature is annoying. This is an attempt to classify such objects in a single hierarchy and allow them to behave differently. We already have some objects in the tree which do their own defaulting in __init__, against what should be our policy. Some of those can be made to inherit from the new class and continue, while others need to be fixed. This patch adds a test to ensure that things comply with the policy henceforth. Objects that had to be changed in the same patch as the test are fixed here. Specific object changes are: - Diagnostics: Not persisted, can be converted to ephemeral - InstanceNUMACell: Stored as a blob, can be converted to ephemeral. Removes a test that asserted no dirty fields left after init, but for no reason and not compatible with the base class. - PCIDevice: Persited row-based, so cannot be ephemeral. Converts it to set default for the one field on lazy-load - Quotas, QuotasNoOp: Stored row-based, so cannot be ephemeral. Converts it to set default on lazy-load, and persists the resetting of dirty fields for compatibility with what is there (even though it is probably not needed). - RequestGroup: Not persisted, can be converted to ephemeral. - Service: Does defaulting in init, but specifically because we *want* to always overwrite what is in the database on save. This is part of how service version works to always know the version of compute (et al) services once they start up. For the objects that were converted to Ephemeral, some hashes changed but only because defaults were added, so no need to bump the version, just update the hash. The hash checker is trying to make sure we bump the version when necessary, but in this case, a default does not affect RPC or object behavior in an incompatible way, so no bump is needed. Change-Id: Ica9f217d0318fc7c2db4bcdea12d00aad749c30c --- nova/objects/base.py | 43 +++++++++++++ nova/objects/diagnostics.py | 3 +- nova/objects/instance_numa.py | 34 ++++------- nova/objects/pci_device.py | 19 +++++- nova/objects/quotas.py | 25 ++++---- nova/objects/request_spec.py | 6 +- nova/tests/unit/objects/test_instance_numa.py | 4 -- nova/tests/unit/objects/test_objects.py | 60 ++++++++++++++++--- 8 files changed, 138 insertions(+), 56 deletions(-) diff --git a/nova/objects/base.py b/nova/objects/base.py index fe00a96d7365..4d108829e8f5 100644 --- a/nova/objects/base.py +++ b/nova/objects/base.py @@ -145,6 +145,49 @@ class NovaPersistentObject(object): } +# NOTE(danms): This is copied from oslo.versionedobjects ahead of +# a release. Do not use it directly or modify it. +# TODO(danms): Remove this when we can get it from oslo.versionedobjects +class EphemeralObject(object): + """Mix-in to provide more recognizable field defaulting. + + If an object should have all fields with a default= set to + those values during instantiation, inherit from this class. + + The base VersionedObject class is designed in such a way that all + fields are optional, which makes sense when representing a remote + database row where not all columns are transported across RPC and + not all columns should be set during an update operation. This is + why fields with default= are not set implicitly during object + instantiation, to avoid clobbering existing fields in the + database. However, objects based on VersionedObject are also used + to represent all-or-nothing blobs stored in the database, or even + used purely in RPC to represent things that are not ever stored in + the database. Thus, this mix-in is provided for these latter + object use cases where the desired behavior is to always have + default= fields be set at __init__ time. + """ + + def __init__(self, *args, **kwargs): + super(EphemeralObject, self).__init__(*args, **kwargs) + # Not specifying any fields causes all defaulted fields to be set + self.obj_set_defaults() + + +class NovaEphemeralObject(EphemeralObject, + NovaObject): + """Base class for objects that are not row-column in the DB. + + Objects that are used purely over RPC (i.e. not persisted) or are + written to the database in blob form or otherwise do not represent + rows directly as fields should inherit from this object. + + The principal difference is that fields with a default value will + be set at __init__ time instead of requiring manual intervention. + """ + pass + + class ObjectListBase(ovoo_base.ObjectListBase): # NOTE(danms): These are for transition to using the oslo # base object and can be removed when we move to it. diff --git a/nova/objects/diagnostics.py b/nova/objects/diagnostics.py index e64cae78cb82..0f2395793a9b 100644 --- a/nova/objects/diagnostics.py +++ b/nova/objects/diagnostics.py @@ -79,7 +79,7 @@ class MemoryDiagnostics(base.NovaObject): @base.NovaObjectRegistry.register -class Diagnostics(base.NovaObject): +class Diagnostics(base.NovaEphemeralObject): # Version 1.0: Initial version VERSION = '1.0' @@ -104,7 +104,6 @@ class Diagnostics(base.NovaObject): def __init__(self, *args, **kwargs): super(Diagnostics, self).__init__(*args, **kwargs) - self.obj_set_defaults() self.num_cpus = len(self.cpu_details) self.num_nics = len(self.nic_details) diff --git a/nova/objects/instance_numa.py b/nova/objects/instance_numa.py index 806919e59928..492ccb06d0ca 100644 --- a/nova/objects/instance_numa.py +++ b/nova/objects/instance_numa.py @@ -26,7 +26,7 @@ from nova.virt import hardware # TODO(berrange): Remove NovaObjectDictCompat @base.NovaObjectRegistry.register -class InstanceNUMACell(base.NovaObject, +class InstanceNUMACell(base.NovaEphemeralObject, base.NovaObjectDictCompat): # Version 1.0: Initial version # Version 1.1: Add pagesize field @@ -50,37 +50,23 @@ class InstanceNUMACell(base.NovaObject, 'id': obj_fields.IntegerField(), 'cpuset': obj_fields.SetOfIntegersField(), 'memory': obj_fields.IntegerField(), - 'pagesize': obj_fields.IntegerField(nullable=True), + 'pagesize': obj_fields.IntegerField(nullable=True, + default=None), 'cpu_topology': obj_fields.ObjectField('VirtCPUTopology', nullable=True), - 'cpu_pinning_raw': obj_fields.DictOfIntegersField(nullable=True), - 'cpu_policy': obj_fields.CPUAllocationPolicyField(nullable=True), + 'cpu_pinning_raw': obj_fields.DictOfIntegersField(nullable=True, + default=None), + 'cpu_policy': obj_fields.CPUAllocationPolicyField(nullable=True, + default=None), 'cpu_thread_policy': obj_fields.CPUThreadAllocationPolicyField( - nullable=True), + nullable=True, default=None), # These physical CPUs are reserved for use by the hypervisor - 'cpuset_reserved': obj_fields.SetOfIntegersField(nullable=True), + 'cpuset_reserved': obj_fields.SetOfIntegersField(nullable=True, + default=None), } cpu_pinning = obj_fields.DictProxyField('cpu_pinning_raw') - def __init__(self, **kwargs): - super(InstanceNUMACell, self).__init__(**kwargs) - if 'pagesize' not in kwargs: - self.pagesize = None - self.obj_reset_changes(['pagesize']) - if 'cpu_pinning' not in kwargs: - self.cpu_pinning = None - self.obj_reset_changes(['cpu_pinning_raw']) - if 'cpu_policy' not in kwargs: - self.cpu_policy = None - self.obj_reset_changes(['cpu_policy']) - if 'cpu_thread_policy' not in kwargs: - self.cpu_thread_policy = None - self.obj_reset_changes(['cpu_thread_policy']) - if 'cpuset_reserved' not in kwargs: - self.cpuset_reserved = None - self.obj_reset_changes(['cpuset_reserved']) - def __len__(self): return len(self.cpuset) diff --git a/nova/objects/pci_device.py b/nova/objects/pci_device.py index 56492a1d3db2..0efb1980d4e4 100644 --- a/nova/objects/pci_device.py +++ b/nova/objects/pci_device.py @@ -112,7 +112,7 @@ class PciDevice(base.NovaPersistentObject, base.NovaObject): 'label': fields.StringField(nullable=True), 'instance_uuid': fields.StringField(nullable=True), 'request_id': fields.StringField(nullable=True), - 'extra_info': fields.DictOfStringsField(), + 'extra_info': fields.DictOfStringsField(default={}), 'numa_node': fields.IntegerField(nullable=True), 'parent_addr': fields.StringField(nullable=True), } @@ -173,14 +173,23 @@ class PciDevice(base.NovaPersistentObject, base.NovaObject): def __init__(self, *args, **kwargs): super(PciDevice, self).__init__(*args, **kwargs) - self.obj_reset_changes() - self.extra_info = {} + # NOTE(ndipanov): These are required to build an in-memory device tree # but don't need to be proper fields (and can't easily be as they would # hold circular references) self.parent_device = None self.child_devices = [] + def obj_load_attr(self, attr): + if attr in ['extra_info']: + # NOTE(danms): extra_info used to be defaulted during init, + # so make sure any bare instantiations of this object can + # rely on the expectation that referencing that field will + # not fail. + self.obj_set_defaults(attr) + else: + super(PciDevice, self).obj_load_attr(attr) + def __eq__(self, other): return compare_pci_device_attributes(self, other) @@ -231,6 +240,10 @@ class PciDevice(base.NovaPersistentObject, base.NovaObject): thus we should not reset changes here for fields from dict. """ pci_device = cls() + # NOTE(danms): extra_info used to always be defaulted during init, + # so make sure we replicate that behavior outside of init here + # for compatibility reasons. + pci_device.obj_set_defaults('extra_info') pci_device.update_device(dev_dict) pci_device.status = fields.PciDeviceStatus.AVAILABLE pci_device.uuid = uuidutils.generate_uuid() diff --git a/nova/objects/quotas.py b/nova/objects/quotas.py index 574107c1a495..19693969ce70 100644 --- a/nova/objects/quotas.py +++ b/nova/objects/quotas.py @@ -64,18 +64,23 @@ class Quotas(base.NovaObject): fields = { # TODO(melwitt): Remove this field in version 2.0 of the object. - 'reservations': fields.ListOfStringsField(nullable=True), - 'project_id': fields.StringField(nullable=True), - 'user_id': fields.StringField(nullable=True), + 'reservations': fields.ListOfStringsField(nullable=True, + default=[]), + 'project_id': fields.StringField(nullable=True, + default=None), + 'user_id': fields.StringField(nullable=True, + default=None), } - def __init__(self, *args, **kwargs): - super(Quotas, self).__init__(*args, **kwargs) - # Set up defaults. - self.reservations = [] - self.project_id = None - self.user_id = None - self.obj_reset_changes() + def obj_load_attr(self, attr): + self.obj_set_defaults(attr) + # NOTE(danms): This is strange because resetting these would cause + # them not to be saved to the database. I would imagine this is + # from overzealous defaulting and that all three fields ultimately + # get set all the time. However, quotas are weird, so replicate the + # longstanding behavior of setting defaults and clearing their + # dirty bit. + self.obj_reset_changes(fields=[attr]) @staticmethod @db_api.api_context_manager.reader diff --git a/nova/objects/request_spec.py b/nova/objects/request_spec.py index bf0a7a5c4aeb..e4699ac1b18e 100644 --- a/nova/objects/request_spec.py +++ b/nova/objects/request_spec.py @@ -1072,7 +1072,7 @@ class SchedulerLimits(base.NovaObject): @base.NovaObjectRegistry.register -class RequestGroup(base.NovaObject): +class RequestGroup(base.NovaEphemeralObject): """Versioned object based on the unversioned nova.api.openstack.placement.lib.RequestGroup object. """ @@ -1109,10 +1109,6 @@ class RequestGroup(base.NovaObject): 'in_tree': fields.UUIDField(nullable=True, default=None), } - def __init__(self, context=None, **kwargs): - super(RequestGroup, self).__init__(context=context, **kwargs) - self.obj_set_defaults() - @classmethod def from_port_request(cls, context, port_uuid, port_resource_request): """Init the group from the resource request of a neutron port diff --git a/nova/tests/unit/objects/test_instance_numa.py b/nova/tests/unit/objects/test_instance_numa.py index 33a679fb6aeb..f7dd878e147b 100644 --- a/nova/tests/unit/objects/test_instance_numa.py +++ b/nova/tests/unit/objects/test_instance_numa.py @@ -126,10 +126,6 @@ class _TestInstanceNUMATopology(object): inst_cell.pin_vcpus((0, 14), (1, 15), (2, 16), (3, 17)) self.assertEqual({0: 14, 1: 15, 2: 16, 3: 17}, inst_cell.cpu_pinning) - def test_default_behavior(self): - inst_cell = objects.InstanceNUMACell() - self.assertEqual(0, len(inst_cell.obj_get_changes())) - def test_cpu_pinning_requested_cell(self): inst_cell = objects.InstanceNUMACell(cpuset=set([0, 1, 2, 3]), cpu_pinning=None) diff --git a/nova/tests/unit/objects/test_objects.py b/nova/tests/unit/objects/test_objects.py index f22a74b4e124..a1b88f2c3622 100644 --- a/nova/tests/unit/objects/test_objects.py +++ b/nova/tests/unit/objects/test_objects.py @@ -12,6 +12,7 @@ # License for the specific language governing permissions and limitations # under the License. +import collections import contextlib import copy import datetime @@ -1085,7 +1086,7 @@ object_data = { 'InstanceList': '2.6-238f125650c25d6d12722340d726f723', 'InstanceMapping': '1.2-3bd375e65c8eb9c45498d2f87b882e03', 'InstanceMappingList': '1.3-d34b6ebb076d542ae0f8b440534118da', - 'InstanceNUMACell': '1.4-7c1eb9a198dee076b4de0840e45f4f55', + 'InstanceNUMACell': '1.4-b68e13eacba363ae8f196abf0ffffb5b', 'InstanceNUMATopology': '1.3-ec0030cb0402a49c96da7051c037082a', 'InstancePCIRequest': '1.3-f6d324f1c337fad4f34892ed5f484c9a', 'InstancePCIRequests': '1.1-65e38083177726d806684cb1cc0136d2', @@ -1112,13 +1113,13 @@ object_data = { 'NetworkRequestList': '1.1-15ecf022a68ddbb8c2a6739cfc9f8f5e', 'NicDiagnostics': '1.0-895e9ad50e0f56d5258585e3e066aea5', 'PCIDeviceBus': '1.0-2b891cb77e42961044689f3dc2718995', - 'PciDevice': '1.6-2a2612baaa1786679e52084e82ca7e66', + 'PciDevice': '1.6-25ca0542a22bc25386a72c0065a79c01', 'PciDeviceList': '1.3-52ff14355491c8c580bdc0ba34c26210', 'PciDevicePool': '1.1-3f5ddc3ff7bfa14da7f6c7e9904cc000', 'PciDevicePoolList': '1.1-15ecf022a68ddbb8c2a6739cfc9f8f5e', 'PowerVMLiveMigrateData': '1.4-a745f4eda16b45e1bc5686a0c498f27e', - 'Quotas': '1.3-40fcefe522111dddd3e5e6155702cf4e', - 'QuotasNoOp': '1.3-347a039fc7cfee7b225b68b5181e0733', + 'Quotas': '1.3-3b2b91371f60e788035778fc5f87797d', + 'QuotasNoOp': '1.3-d1593cf969c81846bc8192255ea95cce', 'RequestGroup': '1.3-0458d350a8ec9d0673f9be5640a990ce', 'RequestLevelParams': '1.0-1e5c8c18bd44cd233c8b32509c99d06f', 'RequestSpec': '1.13-e1aa38b2bf3f8547474ee9e4c0aa2745', @@ -1171,10 +1172,16 @@ def get_nova_objects(): nova_classes = {} for name in all_classes: objclasses = all_classes[name] - if (objclasses[0].OBJ_PROJECT_NAMESPACE != - base.NovaObject.OBJ_PROJECT_NAMESPACE): - continue - nova_classes[name] = objclasses + # NOTE(danms): All object registries that inherit from the + # base VersionedObjectRegistry share a common list of classes. + # That means even things like os_vif objects will be in our + # registry, and for any of them that share the same name + # (i.e. Network), we need to keep ours and exclude theirs. + our_ns = [cls for cls in objclasses + if (cls.OBJ_PROJECT_NAMESPACE == + base.NovaObject.OBJ_PROJECT_NAMESPACE)] + if our_ns: + nova_classes[name] = our_ns return nova_classes @@ -1319,3 +1326,40 @@ class TestObjMethodOverrides(test.NoDBTestCase): obj_class = obj_classes[obj_name][0] self.assertEqual(args, utils.getargspec(obj_class.obj_reset_changes)) + + +class TestObjectsDefaultingOnInit(test.NoDBTestCase): + def test_init_behavior_policy(self): + all_objects = get_nova_objects() + + violations = collections.defaultdict(list) + + # NOTE(danms): Do not add things to this list! + # + # There is one known exception to this init policy, and that + # is the Service object because of the special behavior of the + # version field. We *want* to counteract the usual non-clobber + # behavior of that field specifically. See the comments in + # Service.__init__ for more details. This will likely never + # apply to any other non-ephemeral object, so this list should + # never grow. + exceptions = [objects.Service] + + for name, objclasses in all_objects.items(): + for objcls in objclasses: + if objcls in exceptions: + continue + + key = '%s-%s' % (name, objcls.VERSION) + obj = objcls() + if isinstance(obj, base.NovaEphemeralObject): + # Skip ephemeral objects, which are allowed to + # set fields at init time + continue + for field in objcls.fields: + if field in obj: + violations[key].append(field) + + self.assertEqual({}, violations, + 'Some non-ephemeral objects set fields during ' + 'initialization; This is not allowed.')