From b7ddeb314d98961b5d169f5356a451eec78c8cc7 Mon Sep 17 00:00:00 2001 From: Kaifeng Wang Date: Sat, 28 Nov 2020 01:07:18 +0800 Subject: Support port name MAC address is not user friendly for port management, having a name field is also a feature parity with other resources. This patch implements db related change. Change-Id: Ibad9a1b6bbfddc0af1950def4e27db3757904cb1 Story: 2003091 Task: 23180 --- ironic/common/exception.py | 4 ++ ironic/common/release_mappings.py | 2 +- ironic/db/api.py | 8 +++ .../alembic/versions/c0455649680c_port_name.py | 33 ++++++++++ ironic/db/sqlalchemy/api.py | 14 ++++- ironic/db/sqlalchemy/models.py | 2 + ironic/objects/port.py | 60 ++++++++++++++++-- ironic/tests/unit/db/sqlalchemy/test_migrations.py | 5 ++ ironic/tests/unit/db/test_ports.py | 7 ++- ironic/tests/unit/db/utils.py | 1 + ironic/tests/unit/objects/test_objects.py | 4 +- ironic/tests/unit/objects/test_port.py | 73 +++++++++++++++++++++- 12 files changed, 199 insertions(+), 14 deletions(-) create mode 100644 ironic/db/sqlalchemy/alembic/versions/c0455649680c_port_name.py diff --git a/ironic/common/exception.py b/ironic/common/exception.py index c2e5030e8..e017efa81 100644 --- a/ironic/common/exception.py +++ b/ironic/common/exception.py @@ -77,6 +77,10 @@ class PortAlreadyExists(Conflict): _msg_fmt = _("A port with UUID %(uuid)s already exists.") +class PortDuplicateName(Conflict): + _msg_fmt = _("A port with name %(name)s already exists.") + + class PortgroupAlreadyExists(Conflict): _msg_fmt = _("A portgroup with UUID %(uuid)s already exists.") diff --git a/ironic/common/release_mappings.py b/ironic/common/release_mappings.py index 624e004ed..995ae8070 100644 --- a/ironic/common/release_mappings.py +++ b/ironic/common/release_mappings.py @@ -275,7 +275,7 @@ RELEASE_MAPPING = { 'Chassis': ['1.3'], 'Deployment': ['1.0'], 'DeployTemplate': ['1.1'], - 'Port': ['1.9'], + 'Port': ['1.10'], 'Portgroup': ['1.4'], 'Trait': ['1.0'], 'TraitList': ['1.0'], diff --git a/ironic/db/api.py b/ironic/db/api.py index 48228773f..bee805f5c 100644 --- a/ironic/db/api.py +++ b/ironic/db/api.py @@ -251,6 +251,14 @@ class Connection(object, metaclass=abc.ABCMeta): :returns: A port. """ + @abc.abstractmethod + def get_port_by_name(self, port_name): + """Return a network port representation. + + :param port_name: The name of a port. + :returns: A port. + """ + @abc.abstractmethod def get_port_list(self, limit=None, marker=None, sort_key=None, sort_dir=None): diff --git a/ironic/db/sqlalchemy/alembic/versions/c0455649680c_port_name.py b/ironic/db/sqlalchemy/alembic/versions/c0455649680c_port_name.py new file mode 100644 index 000000000..80023664b --- /dev/null +++ b/ironic/db/sqlalchemy/alembic/versions/c0455649680c_port_name.py @@ -0,0 +1,33 @@ +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. + +"""port-name + +Revision ID: c0455649680c +Revises: cf1a80fdb352 +Create Date: 2020-11-27 20:12:24.752897 + +""" + +from alembic import op +import sqlalchemy as sa + + +# revision identifiers, used by Alembic. +revision = 'c0455649680c' +down_revision = 'cf1a80fdb352' + + +def upgrade(): + op.add_column('ports', sa.Column('name', sa.String(length=255), + nullable=True)) + op.create_unique_constraint('uniq_ports0name', 'ports', ['name']) diff --git a/ironic/db/sqlalchemy/api.py b/ironic/db/sqlalchemy/api.py index 7c478b009..7891122ce 100644 --- a/ironic/db/sqlalchemy/api.py +++ b/ironic/db/sqlalchemy/api.py @@ -705,6 +705,13 @@ class Connection(api.Connection): except NoResultFound: raise exception.PortNotFound(port=address) + def get_port_by_name(self, port_name): + query = model_query(models.Port).filter_by(name=port_name) + try: + return query.one() + except NoResultFound: + raise exception.PortNotFound(port=port_name) + def get_port_list(self, limit=None, marker=None, sort_key=None, sort_dir=None, owner=None, project=None): @@ -773,8 +780,11 @@ class Connection(api.Connection): session.flush() except NoResultFound: raise exception.PortNotFound(port=port_id) - except db_exc.DBDuplicateEntry: - raise exception.MACAlreadyExists(mac=values['address']) + except db_exc.DBDuplicateEntry as exc: + if 'name' in exc.columns: + raise exception.PortDuplicateName(name=values['name']) + else: + raise exception.MACAlreadyExists(mac=values['address']) return ref @oslo_db_api.retry_on_deadlock diff --git a/ironic/db/sqlalchemy/models.py b/ironic/db/sqlalchemy/models.py index 68b366f21..2072153e2 100644 --- a/ironic/db/sqlalchemy/models.py +++ b/ironic/db/sqlalchemy/models.py @@ -210,6 +210,7 @@ class Port(Base): __table_args__ = ( schema.UniqueConstraint('address', name='uniq_ports0address'), schema.UniqueConstraint('uuid', name='uniq_ports0uuid'), + schema.UniqueConstraint('name', name='uniq_ports0name'), table_args()) id = Column(Integer, primary_key=True) uuid = Column(String(36)) @@ -222,6 +223,7 @@ class Port(Base): internal_info = Column(db_types.JsonEncodedDict) physical_network = Column(String(64), nullable=True) is_smartnic = Column(Boolean, nullable=True, default=False) + name = Column(String(255), nullable=True) class Portgroup(Base): diff --git a/ironic/objects/port.py b/ironic/objects/port.py index 85690b162..b4d0e78eb 100644 --- a/ironic/objects/port.py +++ b/ironic/objects/port.py @@ -20,6 +20,7 @@ from oslo_utils import versionutils from oslo_versionedobjects import base as object_base from ironic.common import exception +from ironic.common import utils from ironic.db import api as dbapi from ironic.objects import base from ironic.objects import fields as object_fields @@ -42,7 +43,8 @@ class Port(base.IronicObject, object_base.VersionedObjectDictCompat): # internal_info['tenant_vif_port_id'] (not an explicit db # change) # Version 1.9: Add support for Smart NIC port - VERSION = '1.9' + # Version 1.10: Add name field + VERSION = '1.10' dbapi = dbapi.get_instance() @@ -60,8 +62,26 @@ class Port(base.IronicObject, object_base.VersionedObjectDictCompat): 'physical_network': object_fields.StringField(nullable=True), 'is_smartnic': object_fields.BooleanField(nullable=True, default=False), + 'name': object_fields.StringField(nullable=True), } + def _convert_name_field(self, target_version, + remove_unavailable_fields=True): + name_is_set = self.obj_attr_is_set('name') + if target_version >= (1, 10): + # Target version supports name. Set it to its default + # value if it is not set. + if not name_is_set: + self.name = None + elif name_is_set: + # Target version does not support name, and it is set. + if remove_unavailable_fields: + # (De)serialising: remove unavailable fields. + delattr(self, 'name') + elif self.name is not None: + # DB: set unavailable fields to their default. + self.name = None + def _convert_to_version(self, target_version, remove_unavailable_fields=True): """Convert to the target version. @@ -82,6 +102,9 @@ class Port(base.IronicObject, object_base.VersionedObjectDictCompat): Version 1.9: remove is_smartnic field for unsupported versions if remove_unavailable_fields is True. + Version 1.10: remove name field for unsupported versions if + remove_unavailable_fields is True. + :param target_version: the desired version of the object :param remove_unavailable_fields: True to remove fields that are unavailable in the target version; set this to True when @@ -134,6 +157,9 @@ class Port(base.IronicObject, object_base.VersionedObjectDictCompat): # DB: set unavailable fields to their default. self.is_smartnic = False + # Convert the name field. + self._convert_name_field(target_version, remove_unavailable_fields) + # NOTE(xek): We don't want to enable RPC on this call just yet. Remotable # methods can be used in the future to replace current explicit RPC calls. # Implications of calling new remote procedures should be thought through. @@ -142,11 +168,11 @@ class Port(base.IronicObject, object_base.VersionedObjectDictCompat): def get(cls, context, port_id): """Find a port. - Find a port based on its id or uuid or MAC address and return a Port - object. + Find a port based on its id or uuid or name or MAC address and return + a Port object. :param context: Security context - :param port_id: the id *or* uuid *or* MAC address of a port. + :param port_id: the id *or* uuid *or* name *or* MAC address of a port. :returns: a :class:`Port` object. :raises: InvalidIdentity @@ -157,6 +183,8 @@ class Port(base.IronicObject, object_base.VersionedObjectDictCompat): return cls.get_by_uuid(context, port_id) elif netutils.is_valid_mac(port_id): return cls.get_by_address(context, port_id) + elif utils.is_valid_logical_name(port_id): + return cls.get_by_name(context, port_id) else: raise exception.InvalidIdentity(identity=port_id) @@ -221,6 +249,25 @@ class Port(base.IronicObject, object_base.VersionedObjectDictCompat): port = cls._from_db_object(context, cls(), db_port) return port + # NOTE(xek): We don't want to enable RPC on this call just yet. Remotable + # methods can be used in the future to replace current explicit RPC calls. + # Implications of calling new remote procedures should be thought through. + # @object_base.remotable_classmethod + @classmethod + def get_by_name(cls, context, name): + """Find a port based on name and return a :class:`Port` object. + + :param cls: the :class:`Port` + :param context: Security context + :param name: the name of a port. + :returns: a :class:`Port` object. + :raises: PortNotFound + + """ + db_port = cls.dbapi.get_port_by_name(name) + port = cls._from_db_object(context, cls(), db_port) + return port + # NOTE(xek): We don't want to enable RPC on this call just yet. Remotable # methods can be used in the future to replace current explicit RPC calls. # Implications of calling new remote procedures should be thought through. @@ -435,7 +482,8 @@ class PortCRUDPayload(notification.NotificationPayloadBase): # Version 1.1: Add "portgroup_uuid" field # Version 1.2: Add "physical_network" field # Version 1.3: Add "is_smartnic" field - VERSION = '1.3' + # Version 1.4: Add "name" field + VERSION = '1.4' SCHEMA = { 'address': ('port', 'address'), @@ -447,6 +495,7 @@ class PortCRUDPayload(notification.NotificationPayloadBase): 'updated_at': ('port', 'updated_at'), 'uuid': ('port', 'uuid'), 'is_smartnic': ('port', 'is_smartnic'), + 'name': ('port', 'name'), } fields = { @@ -463,6 +512,7 @@ class PortCRUDPayload(notification.NotificationPayloadBase): 'uuid': object_fields.UUIDField(), 'is_smartnic': object_fields.BooleanField(nullable=True, default=False), + 'name': object_fields.StringField(nullable=True), } def __init__(self, port, node_uuid, portgroup_uuid): diff --git a/ironic/tests/unit/db/sqlalchemy/test_migrations.py b/ironic/tests/unit/db/sqlalchemy/test_migrations.py index 39293c6ac..7a2641323 100644 --- a/ironic/tests/unit/db/sqlalchemy/test_migrations.py +++ b/ironic/tests/unit/db/sqlalchemy/test_migrations.py @@ -1002,6 +1002,11 @@ class MigrationCheckersMixin(object): col_names = [column.name for column in nodes.c] self.assertIn('lessee', col_names) + def _check_c0455649680c(self, engine, data): + ports = db_utils.get_table(engine, 'ports') + col_names = [column.name for column in ports.c] + self.assertIn('name', col_names) + def test_upgrade_and_version(self): with patch_with_engine(self.engine): self.migration_api.upgrade('head') diff --git a/ironic/tests/unit/db/test_ports.py b/ironic/tests/unit/db/test_ports.py index d2434d603..18b8a9032 100644 --- a/ironic/tests/unit/db/test_ports.py +++ b/ironic/tests/unit/db/test_ports.py @@ -32,7 +32,8 @@ class DbPortTestCase(base.DbTestCase): lessee='54321') self.portgroup = db_utils.create_test_portgroup(node_id=self.node.id) self.port = db_utils.create_test_port(node_id=self.node.id, - portgroup_id=self.portgroup.id) + portgroup_id=self.portgroup.id, + name='port-name') def test_get_port_by_id(self): res = self.dbapi.get_port_by_id(self.port.id) @@ -68,6 +69,10 @@ class DbPortTestCase(base.DbTestCase): self.port.address, project='55555') + def test_get_port_by_name(self): + res = self.dbapi.get_port_by_name(self.port.name) + self.assertEqual(self.port.id, res.id) + def test_get_port_list(self): uuids = [] for i in range(1, 6): diff --git a/ironic/tests/unit/db/utils.py b/ironic/tests/unit/db/utils.py index 96254889d..436849054 100644 --- a/ironic/tests/unit/db/utils.py +++ b/ironic/tests/unit/db/utils.py @@ -277,6 +277,7 @@ def get_test_port(**kw): 'internal_info': kw.get('internal_info', {"bar": "buzz"}), 'physical_network': kw.get('physical_network'), 'is_smartnic': kw.get('is_smartnic', False), + 'name': kw.get('name'), } diff --git a/ironic/tests/unit/objects/test_objects.py b/ironic/tests/unit/objects/test_objects.py index 3852a9abd..7b7788933 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.35-aee8ecf5c4d0ed590eb484762aee7fca', 'MyObj': '1.5-9459d30d6954bffc7a9afd347a807ca6', 'Chassis': '1.3-d656e039fd8ae9f34efc232ab3980905', - 'Port': '1.9-0cb9202a4ec442e8c0d87a324155eaaf', + 'Port': '1.10-67381b065c597c8d3a13c5dbc6243c33', 'Portgroup': '1.4-71923a81a86743b313b190f5c675e258', 'Conductor': '1.3-d3f53e853b4d58cae5bfbd9a8341af4a', 'EventType': '1.1-aa2ba1afd38553e3880c267404e8d370', @@ -700,7 +700,7 @@ expected_object_fingerprints = { 'NodeCRUDNotification': '1.0-59acc533c11d306f149846f922739c15', 'NodeCRUDPayload': '1.13-8f673253ff8d7389897a6a80d224ac33', 'PortCRUDNotification': '1.0-59acc533c11d306f149846f922739c15', - 'PortCRUDPayload': '1.3-21235916ed54a91b2a122f59571194e7', + 'PortCRUDPayload': '1.4-9411a1701077ae9dc0aea27d6bf586fc', 'NodeMaintenanceNotification': '1.0-59acc533c11d306f149846f922739c15', 'NodeConsoleNotification': '1.0-59acc533c11d306f149846f922739c15', 'PortgroupCRUDNotification': '1.0-59acc533c11d306f149846f922739c15', diff --git a/ironic/tests/unit/objects/test_port.py b/ironic/tests/unit/objects/test_port.py index 43c58876e..43459c958 100644 --- a/ironic/tests/unit/objects/test_port.py +++ b/ironic/tests/unit/objects/test_port.py @@ -34,7 +34,7 @@ class TestPortObject(db_base.DbTestCase, obj_utils.SchemasTestMixIn): def setUp(self): super(TestPortObject, self).setUp() - self.fake_port = db_utils.get_test_port() + self.fake_port = db_utils.get_test_port(name='port-name') def test_get_by_id(self): port_id = self.fake_port['id'] @@ -69,9 +69,20 @@ class TestPortObject(db_base.DbTestCase, obj_utils.SchemasTestMixIn): mock_get_port.assert_called_once_with(address, project=None) self.assertEqual(self.context, port._context) - def test_get_bad_id_and_uuid_and_address(self): + def test_get_by_name(self): + name = self.fake_port['name'] + with mock.patch.object(self.dbapi, 'get_port_by_name', + autospec=True) as mock_get_port: + mock_get_port.return_value = self.fake_port + + port = objects.Port.get(self.context, name) + + mock_get_port.assert_called_once_with(name) + self.assertEqual(self.context, port._context) + + def test_get_bad_id_and_uuid_and_name_and_address(self): self.assertRaises(exception.InvalidIdentity, - objects.Port.get, self.context, 'not-a-uuid') + objects.Port.get, self.context, '#not-valid') def test_create(self): port = objects.Port(self.context, **self.fake_port) @@ -349,3 +360,59 @@ class TestConvertToVersion(db_base.DbTestCase): port._convert_to_version("1.8", False) self.assertFalse(port.is_smartnic) self.assertNotIn('is_smartnic', port.obj_get_changes()) + + def test_name_supported_missing(self): + # name not set, should be set to default. + port = objects.Port(self.context, **self.fake_port) + delattr(port, 'name') + port.obj_reset_changes() + port._convert_to_version("1.10") + self.assertIsNone(port.name) + self.assertIn('name', port.obj_get_changes()) + self.assertIsNone(port.obj_get_changes()['name']) + + def test_name_supported_set(self): + # Physical network set, no change required. + port = objects.Port(self.context, **self.fake_port) + port.name = 'meow' + port.obj_reset_changes() + port._convert_to_version("1.10") + self.assertEqual('meow', port.name) + self.assertNotIn('name', port.obj_get_changes()) + + def test_name_unsupported_missing(self): + # name not set, no change required. + port = objects.Port(self.context, **self.fake_port) + delattr(port, 'name') + port.obj_reset_changes() + port._convert_to_version("1.9") + self.assertNotIn('name', port) + self.assertNotIn('name', port.obj_get_changes()) + + def test_name_unsupported_set_remove(self): + # name set, should be removed. + port = objects.Port(self.context, **self.fake_port) + port.name = 'meow' + port.obj_reset_changes() + port._convert_to_version("1.9") + self.assertNotIn('name', port) + self.assertNotIn('name', port.obj_get_changes()) + + def test_name_unsupported_set_no_remove_non_default(self): + # name set, should be set to default. + port = objects.Port(self.context, **self.fake_port) + port.name = 'meow' + port.obj_reset_changes() + port._convert_to_version("1.9", False) + self.assertIsNone(port.name) + self.assertIn('name', port.obj_get_changes()) + self.assertIsNone(port.obj_get_changes()['name']) + + def test_name_unsupported_set_no_remove_default(self): + # name set, no change required. + port = objects.Port(self.context, **self.fake_port) + port.name = None + port.obj_reset_changes() + port._convert_to_version("1.9", False) + self.assertIsNone(port.name) + self.assertNotIn('name', port.obj_get_changes()) -- cgit v1.2.1