From 8b00932e485813f1ed6013de6c7b7e7f180491f8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Harald=20Jens=C3=A5s?= Date: Fri, 28 Oct 2022 16:40:05 +0200 Subject: [PATCH] Use association_proxy for ports node_uuid This change adds 'node_uuid' to ironic.objects.port.Port and adds a relationship using association_proxy in models.Port. Using the association_proxy removes the need to do the node lookup to populate node uuid for ports in the api controller. NOTE: On port create a read is added to read the port from the database, this ensures node_uuid is loaded and solves the DetachedInstanceError which is otherwise raised. Bumps Port object version to 1.11 With patch: 1. Returned 20000 ports in python 2.7768702507019043 seconds from the DB. 2. Took 0.433107852935791 seconds to iterate through 20000 port objects. Ports table is roughly 12800000 bytes of JSON. 3. Took 5.662816762924194 seconds to return all 20000 ports via ports API call pattern. Without patch: 1. Returned 20000 ports in python 1.0273635387420654 seconds from the DB. 2. Took 0.4772777557373047 seconds to iterate through 20000 port objects. Ports table is roughly 12800000 bytes of JSON. 3. Took 147.8800814151764 seconds to return all 20000 ports via ports API call pattern. Conclusion: Test #1 plain dbapi.get_port_list() test is ~3 times slower, but Test #3 doing the API call pattern test is ~2500% better. Story: 2007789 Task: 40035 Change-Id: Iff204b3056f3058f795f05dc1d240f494d60672a --- ironic/api/controllers/v1/port.py | 8 ++--- ironic/common/release_mappings.py | 2 +- ironic/db/sqlalchemy/api.py | 4 +-- ironic/db/sqlalchemy/models.py | 10 +++++++ ironic/objects/port.py | 8 ++++- .../unit/api/controllers/v1/test_port.py | 29 ------------------- ironic/tests/unit/db/utils.py | 2 ++ ironic/tests/unit/objects/test_objects.py | 2 +- ironic/tests/unit/objects/test_port.py | 12 +++++--- 9 files changed, 33 insertions(+), 44 deletions(-) diff --git a/ironic/api/controllers/v1/port.py b/ironic/api/controllers/v1/port.py index adc21ebc2e..d70c13f27e 100644 --- a/ironic/api/controllers/v1/port.py +++ b/ironic/api/controllers/v1/port.py @@ -123,9 +123,9 @@ def convert_with_links(rpc_port, fields=None, sanitize=True): 'local_link_connection', 'physical_network', 'pxe_enabled', + 'node_uuid', ) ) - api_utils.populate_node_uuid(rpc_port, port) if rpc_port.portgroup_id: pg = objects.Portgroup.get(api.request.context, rpc_port.portgroup_id) port['portgroup_uuid'] = pg.uuid @@ -166,12 +166,10 @@ def list_convert_with_links(rpc_ports, limit, url, fields=None, **kwargs): try: port = convert_with_links(rpc_port, fields=fields, sanitize=False) - except exception.NodeNotFound: # NOTE(dtantsur): node was deleted after we fetched the port # list, meaning that the port was also deleted. Skip it. - LOG.debug('Skipping port %s as its node was deleted', - rpc_port.uuid) - continue + if port['node_uuid'] is None: + continue except exception.PortgroupNotFound: # NOTE(dtantsur): port group was deleted after we fetched the # port list, it may mean that the port was deleted too, but diff --git a/ironic/common/release_mappings.py b/ironic/common/release_mappings.py index 555ff164d1..76c40fc2fd 100644 --- a/ironic/common/release_mappings.py +++ b/ironic/common/release_mappings.py @@ -523,7 +523,7 @@ RELEASE_MAPPING = { 'Chassis': ['1.3'], 'Deployment': ['1.0'], 'DeployTemplate': ['1.1'], - 'Port': ['1.10'], + 'Port': ['1.11'], 'Portgroup': ['1.4'], 'Trait': ['1.0'], 'TraitList': ['1.0'], diff --git a/ironic/db/sqlalchemy/api.py b/ironic/db/sqlalchemy/api.py index a0cefea35b..7a6e6862e3 100644 --- a/ironic/db/sqlalchemy/api.py +++ b/ironic/db/sqlalchemy/api.py @@ -980,9 +980,7 @@ class Connection(api.Connection): sort_key=None, sort_dir=None, owner=None, project=None): query = sa.select(models.Port).where( - models.Port.portgroup_id == portgroup_id - ) - + models.Port.portgroup_id == portgroup_id) if owner: query = add_port_filter_by_node_owner(query, owner) elif project: diff --git a/ironic/db/sqlalchemy/models.py b/ironic/db/sqlalchemy/models.py index 34de1b364a..6fc8e21ab1 100644 --- a/ironic/db/sqlalchemy/models.py +++ b/ironic/db/sqlalchemy/models.py @@ -25,6 +25,7 @@ from urllib import parse as urlparse from oslo_db import options as db_options from oslo_db.sqlalchemy import models from oslo_db.sqlalchemy import types as db_types +from sqlalchemy.ext.associationproxy import association_proxy from sqlalchemy import Boolean, Column, DateTime, false, Index from sqlalchemy import ForeignKey, Integer from sqlalchemy import schema, String, Text @@ -262,6 +263,15 @@ class Port(Base): is_smartnic = Column(Boolean, nullable=True, default=False) name = Column(String(255), nullable=True) + _node_uuid = orm.relationship( + "Node", + viewonly=True, + primaryjoin="(Node.id == Port.node_id)", + lazy="selectin", + ) + node_uuid = association_proxy( + "_node_uuid", "uuid", creator=lambda _i: Node(uuid=_i)) + class Portgroup(Base): """Represents a group of network ports of a bare metal node.""" diff --git a/ironic/objects/port.py b/ironic/objects/port.py index b4d0e78eb1..d45bee4b5a 100644 --- a/ironic/objects/port.py +++ b/ironic/objects/port.py @@ -44,7 +44,8 @@ class Port(base.IronicObject, object_base.VersionedObjectDictCompat): # change) # Version 1.9: Add support for Smart NIC port # Version 1.10: Add name field - VERSION = '1.10' + # Version 1.11: Add node_uuid field + VERSION = '1.11' dbapi = dbapi.get_instance() @@ -52,6 +53,7 @@ class Port(base.IronicObject, object_base.VersionedObjectDictCompat): 'id': object_fields.IntegerField(), 'uuid': object_fields.UUIDField(nullable=True), 'node_id': object_fields.IntegerField(nullable=True), + 'node_uuid': object_fields.UUIDField(nullable=True), 'address': object_fields.MACAddressField(nullable=True), 'extra': object_fields.FlexibleDictField(nullable=True), 'local_link_connection': object_fields.FlexibleDictField( @@ -377,6 +379,10 @@ class Port(base.IronicObject, object_base.VersionedObjectDictCompat): """ values = self.do_version_changes_for_db() db_port = self.dbapi.create_port(values) + # NOTE(hjensas): To avoid lazy load issue (DetachedInstanceError) in + # sqlalchemy, get new port the port from the DB to ensure the node_uuid + # via association_proxy relationship is loaded. + db_port = self.dbapi.get_port_by_id(db_port['id']) self._from_db_object(self._context, self, db_port) # NOTE(xek): We don't want to enable RPC on this call just yet. Remotable diff --git a/ironic/tests/unit/api/controllers/v1/test_port.py b/ironic/tests/unit/api/controllers/v1/test_port.py index 6823c3b518..7092a4012d 100644 --- a/ironic/tests/unit/api/controllers/v1/test_port.py +++ b/ironic/tests/unit/api/controllers/v1/test_port.py @@ -236,35 +236,6 @@ class TestListPorts(test_api_base.BaseApiTest): # never expose the node_id self.assertNotIn('node_id', data['ports'][0]) - # NOTE(jlvillal): autospec=True doesn't work on staticmethods: - # https://bugs.python.org/issue23078 - @mock.patch.object(objects.Node, 'get_by_id', spec_set=types.FunctionType) - def test_list_with_deleted_node(self, mock_get_node): - # check that we don't end up with HTTP 400 when node deletion races - # with listing ports - see https://launchpad.net/bugs/1748893 - obj_utils.create_test_port(self.context, node_id=self.node.id) - mock_get_node.side_effect = exception.NodeNotFound('boom') - data = self.get_json('/ports') - self.assertEqual([], data['ports']) - - # NOTE(jlvillal): autospec=True doesn't work on staticmethods: - # https://bugs.python.org/issue23078 - @mock.patch.object(objects.Node, 'get_by_id', - spec_set=types.FunctionType) - def test_list_detailed_with_deleted_node(self, mock_get_node): - # check that we don't end up with HTTP 400 when node deletion races - # with listing ports - see https://launchpad.net/bugs/1748893 - port = obj_utils.create_test_port(self.context, node_id=self.node.id) - port2 = obj_utils.create_test_port(self.context, node_id=self.node.id, - uuid=uuidutils.generate_uuid(), - address='66:44:55:33:11:22') - mock_get_node.side_effect = [exception.NodeNotFound('boom'), self.node] - data = self.get_json('/ports/detail') - # The "correct" port is still returned - self.assertEqual(1, len(data['ports'])) - self.assertIn(data['ports'][0]['uuid'], {port.uuid, port2.uuid}) - self.assertEqual(self.node.uuid, data['ports'][0]['node_uuid']) - # NOTE(jlvillal): autospec=True doesn't work on staticmethods: # https://bugs.python.org/issue23078 @mock.patch.object(objects.Portgroup, 'get', spec_set=types.FunctionType) diff --git a/ironic/tests/unit/db/utils.py b/ironic/tests/unit/db/utils.py index c59ee1cc42..87ba4b5147 100644 --- a/ironic/tests/unit/db/utils.py +++ b/ironic/tests/unit/db/utils.py @@ -272,6 +272,8 @@ def get_test_port(**kw): 'version': kw.get('version', port.Port.VERSION), 'uuid': kw.get('uuid', '1be26c0b-03f2-4d2e-ae87-c02d7f33c781'), 'node_id': kw.get('node_id', 123), + 'node_uuid': kw.get('node_uuid', + '59d102f7-5840-4299-8ec8-80c0ebae9de1'), 'address': kw.get('address', '52:54:00:cf:2d:31'), 'extra': kw.get('extra', {}), 'created_at': kw.get('created_at'), diff --git a/ironic/tests/unit/objects/test_objects.py b/ironic/tests/unit/objects/test_objects.py index 2e45ee0436..017a0d2103 100644 --- a/ironic/tests/unit/objects/test_objects.py +++ b/ironic/tests/unit/objects/test_objects.py @@ -679,7 +679,7 @@ expected_object_fingerprints = { 'Node': '1.36-8a080e31ba89ca5f09e859bd259b54dc', 'MyObj': '1.5-9459d30d6954bffc7a9afd347a807ca6', 'Chassis': '1.3-d656e039fd8ae9f34efc232ab3980905', - 'Port': '1.10-67381b065c597c8d3a13c5dbc6243c33', + 'Port': '1.11-97bf15b61224f26c65e90f007d78bfd2', 'Portgroup': '1.4-71923a81a86743b313b190f5c675e258', 'Conductor': '1.3-d3f53e853b4d58cae5bfbd9a8341af4a', 'EventType': '1.1-aa2ba1afd38553e3880c267404e8d370', diff --git a/ironic/tests/unit/objects/test_port.py b/ironic/tests/unit/objects/test_port.py index cf76338086..4c7280216f 100644 --- a/ironic/tests/unit/objects/test_port.py +++ b/ironic/tests/unit/objects/test_port.py @@ -88,12 +88,16 @@ class TestPortObject(db_base.DbTestCase, obj_utils.SchemasTestMixIn): port = objects.Port(self.context, **self.fake_port) with mock.patch.object(self.dbapi, 'create_port', autospec=True) as mock_create_port: - mock_create_port.return_value = db_utils.get_test_port() + with mock.patch.object(self.dbapi, 'get_port_by_id', + autospec=True) as mock_get_port: + test_port = db_utils.get_test_port() + mock_create_port.return_value = test_port + mock_get_port.return_value = test_port - port.create() + port.create() - args, _kwargs = mock_create_port.call_args - self.assertEqual(objects.Port.VERSION, args[0]['version']) + args, _kwargs = mock_create_port.call_args + self.assertEqual(objects.Port.VERSION, args[0]['version']) def test_save(self): uuid = self.fake_port['uuid']