From 752c33e92b13758acfae56657b90e5dfbe73df62 Mon Sep 17 00:00:00 2001 From: Ihar Hrachyshka Date: Mon, 30 Apr 2018 22:59:48 +0000 Subject: [PATCH] objects: don't refetch a non-list object field if it's None When an object field has the value of corresponding model attribute set to None, it means that it's indeed unset, and there is no need to refetch it from the database (it will still be None). This becomes a more noticeable problem when we introduce security field to Network (part of I57395d0f646ffa3089c1ac6c5a68d952ccd0b42c) that is exactly this type of field, because with the auto-expiry patch included (I0d65d19204da8ce30addfa5faff68544534b7853) those redundant fetches trigger actual SELECT statements that affect performance and break test_get_objects_queries_constant regression test case. The patch also changes the type of 'distributed_binding' Port object field from ObjectField to ListOfObjectFields, and also renames the field into 'distributed_bindings' to reflect actual possibilities allowed by database schema. Note this patch is NOT expected to enable actual support for multiple binding values, which would belong to a separate patch, but just attempts to apply the minimal needed changes required because of the other changes included while making sure that consequent changes for the field don't need to change object definition. Partially-Implements: blueprint adopt-oslo-versioned-objects-for-db Change-Id: I833b07fdbb4a57bdf8bd4255e61098ec512d1a5b --- neutron/objects/base.py | 36 ++++++++++++---------- neutron/objects/ports.py | 20 +++++++----- neutron/tests/unit/objects/test_objects.py | 2 +- neutron/tests/unit/objects/test_ports.py | 25 +++++++++++++++ 4 files changed, 59 insertions(+), 24 deletions(-) diff --git a/neutron/objects/base.py b/neutron/objects/base.py index e835a689b17..cd39b11d0a7 100644 --- a/neutron/objects/base.py +++ b/neutron/objects/base.py @@ -713,8 +713,9 @@ class NeutronDbObject(NeutronObject): # subclasses=True for field in self.synthetic_fields: try: + field_def = self.fields[field] objclasses = NeutronObjectRegistry.obj_classes( - ).get(self.fields[field].objname) + ).get(field_def.objname) except AttributeError: # NOTE(rossella_s) this is probably because this field is not # an ObjectField @@ -735,25 +736,28 @@ class NeutronDbObject(NeutronObject): synthetic_field_db_name = ( self.fields_need_translation.get(field, field)) - synth_db_objs = (db_obj.get(synthetic_field_db_name, None) - if db_obj else None) # synth_db_objs can be list, empty list or None, that is why # we need 'is not None', because [] is valid case for 'True' - if synth_db_objs is not None: - if not isinstance(synth_db_objs, list): - synth_db_objs = [synth_db_objs] - synth_objs = [objclass._load_object(self.obj_context, obj) - for obj in synth_db_objs] - else: - synth_objs = objclass.get_objects( - self.obj_context, **{ - k: getattr(self, v) if v in self else db_obj.get(v) - for k, v in foreign_keys.items()}) - if isinstance(self.fields[field], obj_fields.ObjectField): - setattr(self, field, synth_objs[0] if synth_objs else None) - else: + if isinstance(field_def, obj_fields.ListOfObjectsField): + synth_db_objs = (db_obj.get(synthetic_field_db_name, None) + if db_obj else None) + if synth_db_objs is not None: + synth_objs = [objclass._load_object(self.obj_context, obj) + for obj in synth_db_objs] + else: + synth_objs = objclass.get_objects( + self.obj_context, **{ + k: getattr(self, v) if v in self else db_obj.get(v) + for k, v in foreign_keys.items()}) setattr(self, field, synth_objs) + else: + synth_db_obj = (db_obj.get(synthetic_field_db_name, None) + if db_obj else None) + if synth_db_obj: + synth_db_obj = objclass._load_object(self.obj_context, + synth_db_obj) + setattr(self, field, synth_db_obj) self.obj_reset_changes([field]) def create(self): diff --git a/neutron/objects/ports.py b/neutron/objects/ports.py index ea73e32a4e8..94741058732 100644 --- a/neutron/objects/ports.py +++ b/neutron/objects/ports.py @@ -263,7 +263,8 @@ class Port(base.NeutronDbObject): # Version 1.0: Initial version # Version 1.1: Add data_plane_status field # Version 1.2: Added segment_id to binding_levels - VERSION = '1.2' + # Version 1.3: distributed_binding -> distributed_bindings + VERSION = '1.3' db_model = models_v2.Port @@ -290,7 +291,7 @@ class Port(base.NeutronDbObject): 'dhcp_options': obj_fields.ListOfObjectsField( 'ExtraDhcpOpt', nullable=True ), - 'distributed_binding': obj_fields.ObjectField( + 'distributed_bindings': obj_fields.ListOfObjectsField( 'DistributedPortBinding', nullable=True ), 'dns': obj_fields.ObjectField('PortDNS', nullable=True), @@ -326,7 +327,7 @@ class Port(base.NeutronDbObject): 'binding_levels', 'data_plane_status', 'dhcp_options', - 'distributed_binding', + 'distributed_bindings', 'dns', 'fixed_ips', 'qos_policy_id', @@ -337,7 +338,7 @@ class Port(base.NeutronDbObject): fields_need_translation = { 'binding': 'port_binding', 'dhcp_options': 'dhcp_opts', - 'distributed_binding': 'distributed_port_binding', + 'distributed_bindings': 'distributed_port_binding', 'security': 'port_security', } @@ -430,11 +431,12 @@ class Port(base.NeutronDbObject): if 'mac_address' in fields: fields['mac_address'] = utils.AuthenticEUI(fields['mac_address']) - distributed_port_binding = fields.get('distributed_binding') + distributed_port_binding = fields.get('distributed_bindings') if distributed_port_binding: - fields['distributed_binding'] = fields['distributed_binding'][0] + # TODO(ihrachys) support multiple bindings + fields['distributed_bindings'] = fields['distributed_bindings'][0] else: - fields['distributed_binding'] = None + fields['distributed_bindings'] = [] return fields def from_db_object(self, db_obj): @@ -467,6 +469,10 @@ class Port(base.NeutronDbObject): for lvl in binding_levels: lvl['versioned_object.version'] = '1.0' lvl['versioned_object.data'].pop('segment_id', None) + if _target_version < (1, 3): + bindings = primitive.pop('distributed_bindings', []) + primitive['distributed_binding'] = (bindings[0] + if bindings else None) @classmethod def get_ports_by_router(cls, context, router_id, owner, subnet): diff --git a/neutron/tests/unit/objects/test_objects.py b/neutron/tests/unit/objects/test_objects.py index afcf7975f23..b4cea51f75c 100644 --- a/neutron/tests/unit/objects/test_objects.py +++ b/neutron/tests/unit/objects/test_objects.py @@ -62,7 +62,7 @@ object_data = { 'NetworkPortSecurity': '1.0-b30802391a87945ee9c07582b4ff95e3', 'NetworkRBAC': '1.0-c8a67f39809c5a3c8c7f26f2f2c620b2', 'NetworkSegment': '1.0-57b7f2960971e3b95ded20cbc59244a8', - 'Port': '1.2-5bf48d12a7bf7f5b7a319e8003b437a5', + 'Port': '1.3-4cb798ffc8b08f2657c0bd8afa708e9e', 'PortBinding': '1.0-3306deeaa6deb01e33af06777d48d578', 'PortBindingLevel': '1.1-50d47f63218f87581b6cd9a62db574e5', 'PortDataPlaneStatus': '1.0-25be74bda46c749653a10357676c0ab2', diff --git a/neutron/tests/unit/objects/test_ports.py b/neutron/tests/unit/objects/test_ports.py index 13f36d4d944..bf32c3563a0 100644 --- a/neutron/tests/unit/objects/test_ports.py +++ b/neutron/tests/unit/objects/test_ports.py @@ -393,3 +393,28 @@ class PortDbObjectTestCase(obj_test_base.BaseDbObjectTestCase, # downgrade of the binding object binding_v1_0 = binding.obj_to_primitive(target_version='1.0') self.assertEqual(binding_v1_0, lvl) + + def test_v1_3_to_v1_2_unlists_distributed_bindings(self): + port_new = self._create_test_port() + + # empty list transforms into None + port_v1_2 = port_new.obj_to_primitive(target_version='1.2') + port_data = port_v1_2['versioned_object.data'] + self.assertIsNone(port_data['distributed_binding']) + + # now insert a distributed binding + binding = ports.DistributedPortBinding( + self.context, + host='host1', port_id=port_new.id, status='ACTIVE', + vnic_type='vnic_type1', vif_type='vif_type1') + binding.create() + + # refetch port object to include binding + port_new = ports.Port.get_object(self.context, id=port_new.id) + + # new primitive should contain the binding data + port_v1_2 = port_new.obj_to_primitive(target_version='1.2') + port_data = port_v1_2['versioned_object.data'] + binding_data = ( + port_data['distributed_binding']['versioned_object.data']) + self.assertEqual(binding.host, binding_data['host'])