openstack/ironic
Code Issues Proposed changes

Use association_proxy for ports node_uuid

Browse Source 
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
This commit is contained in:
Harald Jensås 2022-10-28 16:40:05 +02:00
parent f96b258709
commit 8b00932e48
9 changed files with 33 additions and 44 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

8
ironic/api/controllers/v1/port.py
View File

@ -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

2
ironic/common/release_mappings.py
View File

@ -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'],

4
ironic/db/sqlalchemy/api.py
View File

@ -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:

10
ironic/db/sqlalchemy/models.py
View File

@ -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."""

8
ironic/objects/port.py
View File

@ -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

29
ironic/tests/unit/api/controllers/v1/test_port.py
View File

@ -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)

2
ironic/tests/unit/db/utils.py
View File

@ -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'),

2
ironic/tests/unit/objects/test_objects.py
View File

@ -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',

12
ironic/tests/unit/objects/test_port.py
View File

@ -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']