From 28167f18f85823d97a12460b6ba4a724b5e27486 Mon Sep 17 00:00:00 2001 From: Jay Faulkner Date: Mon, 30 Jan 2023 15:25:42 -0800 Subject: [PATCH] Allow port queries by shard list Adds ability to query ports by shard. Example usage: /v1/ports?shard=lol,cats Change-Id: Icacef7f71414d30f492a6d144bcc89dff4891784 --- api-ref/source/baremetal-api-v1-ports.inc | 5 ++ api-ref/source/parameters.yaml | 7 +++ ironic/api/controllers/v1/port.py | 46 ++++++++++++++----- ironic/api/controllers/v1/utils.py | 2 +- ironic/db/api.py | 8 ++++ ironic/db/sqlalchemy/api.py | 12 +++++ ironic/objects/port.py | 21 +++++++++ .../unit/api/controllers/v1/test_port.py | 40 +++++++++++++++- ironic/tests/unit/db/test_ports.py | 39 ++++++++++++++++ 9 files changed, 166 insertions(+), 14 deletions(-) diff --git a/api-ref/source/baremetal-api-v1-ports.inc b/api-ref/source/baremetal-api-v1-ports.inc index f40d133915..3fa7e9d474 100644 --- a/api-ref/source/baremetal-api-v1-ports.inc +++ b/api-ref/source/baremetal-api-v1-ports.inc @@ -49,6 +49,10 @@ By default, this query will return the uuid and address for each Port. .. versionadded:: 1.53 Added the ``is_smartnic`` field. +.. versionadded:: 1.82 + Added the ability to filter ports based on the shard of the node they are + associated with. + Normal response code: 200 Request @@ -60,6 +64,7 @@ Request - node_uuid: r_port_node_uuid - portgroup: r_port_portgroup_ident - address: r_port_address + - shard: r_port_shard - fields: fields - limit: limit - marker: marker diff --git a/api-ref/source/parameters.yaml b/api-ref/source/parameters.yaml index 930d85f1e6..13eb45d7d0 100644 --- a/api-ref/source/parameters.yaml +++ b/api-ref/source/parameters.yaml @@ -366,6 +366,13 @@ r_port_portgroup_ident: in: query required: false type: string +r_port_shard: + description: | + Filter the list of returned Ports, and only return the ones associated + with nodes in this specific shard(s), or an empty set if not found. + in: query + required: false + type: array r_portgroup_address: description: | Filter the list of returned Portgroups, and only return the ones with the diff --git a/ironic/api/controllers/v1/port.py b/ironic/api/controllers/v1/port.py index d70c13f27e..00dddf2287 100644 --- a/ironic/api/controllers/v1/port.py +++ b/ironic/api/controllers/v1/port.py @@ -208,7 +208,7 @@ class PortsController(rest.RestController): self.parent_portgroup_ident = portgroup_ident def _get_ports_collection(self, node_ident, address, portgroup_ident, - marker, limit, sort_key, sort_dir, + shard, marker, limit, sort_key, sort_dir, resource_url=None, fields=None, detail=None, project=None): """Retrieve a collection of ports. @@ -219,6 +219,8 @@ class PortsController(rest.RestController): this MAC address. :param portgroup_ident: UUID or name of a portgroup, to get only ports for that portgroup. + :param shard: A comma-separated shard list, to get only ports for those + shards :param marker: pagination marker for large data sets. :param limit: maximum number of resources to return in a single result. This value cannot be larger than the value of max_limit @@ -251,8 +253,12 @@ class PortsController(rest.RestController): node_ident = self.parent_node_ident or node_ident portgroup_ident = self.parent_portgroup_ident or portgroup_ident - if node_ident and portgroup_ident: - raise exception.OperationNotPermitted() + exclusive_filters = 0 + for i in [node_ident, portgroup_ident, shard]: + if i: + exclusive_filters += 1 + if exclusive_filters > 1: + raise exception.OperationNotPermitted() if portgroup_ident: # FIXME: Since all we need is the portgroup ID, we can @@ -279,6 +285,11 @@ class PortsController(rest.RestController): project=project) elif address: ports = self._get_ports_by_address(address, project=project) + elif shard: + ports = objects.Port.list_by_node_shards(api.request.context, + shard, limit, + marker_obj, sort_key, + sort_dir, project=project) else: ports = objects.Port.list(api.request.context, limit, marker_obj, sort_key=sort_key, @@ -349,10 +360,11 @@ class PortsController(rest.RestController): address=args.mac_address, marker=args.uuid, limit=args.integer, sort_key=args.string, sort_dir=args.string, fields=args.string_list, - portgroup=args.uuid_or_name, detail=args.boolean) + portgroup=args.uuid_or_name, detail=args.boolean, + shard=args.string_list) def get_all(self, node=None, node_uuid=None, address=None, marker=None, limit=None, sort_key='id', sort_dir='asc', fields=None, - portgroup=None, detail=None): + portgroup=None, detail=None, shard=None): """Retrieve a list of ports. Note that the 'node_uuid' interface is deprecated in favour @@ -375,6 +387,8 @@ class PortsController(rest.RestController): of the resource to be returned. :param portgroup: UUID or name of a portgroup, to get only ports for that portgroup. + :param shard: Optional, a list of shard ids to filter by, only ports + associated with nodes in these shards will be returned. :raises: NotAcceptable, HTTPNotFound """ project = api_utils.check_port_list_policy( @@ -394,6 +408,8 @@ class PortsController(rest.RestController): if portgroup and not api_utils.allow_portgroups_subcontrollers(): raise exception.NotAcceptable() + api_utils.check_allow_filter_by_shard(shard) + fields = api_utils.get_request_return_fields(fields, detail, _DEFAULT_RETURN_FIELDS) @@ -406,8 +422,9 @@ class PortsController(rest.RestController): raise exception.NotAcceptable() return self._get_ports_collection(node_uuid or node, address, - portgroup, marker, limit, sort_key, - sort_dir, resource_url='ports', + portgroup, shard, marker, limit, + sort_key, sort_dir, + resource_url='ports', fields=fields, detail=detail, project=project) @@ -416,10 +433,11 @@ class PortsController(rest.RestController): @args.validate(node=args.uuid_or_name, node_uuid=args.uuid, address=args.mac_address, marker=args.uuid, limit=args.integer, sort_key=args.string, - sort_dir=args.string, - portgroup=args.uuid_or_name) + sort_dir=args.string, portgroup=args.uuid_or_name, + shard=args.string_list) def detail(self, node=None, node_uuid=None, address=None, marker=None, - limit=None, sort_key='id', sort_dir='asc', portgroup=None): + limit=None, sort_key='id', sort_dir='asc', portgroup=None, + shard=None): """Retrieve a list of ports with detail. Note that the 'node_uuid' interface is deprecated in favour @@ -433,6 +451,8 @@ class PortsController(rest.RestController): this MAC address. :param portgroup: UUID or name of a portgroup, to get only ports for that portgroup. + :param shard: comma separated list of shards, to only get ports + associated with nodes in those shards. :param marker: pagination marker for large data sets. :param limit: maximum number of resources to return in a single result. This value cannot be larger than the value of max_limit @@ -450,6 +470,8 @@ class PortsController(rest.RestController): if portgroup and not api_utils.allow_portgroups_subcontrollers(): raise exception.NotAcceptable() + api_utils.check_allow_filter_by_shard(shard) + if not node_uuid and node: # We're invoking this interface using positional notation, or # explicitly using 'node'. Try and determine which one. @@ -464,8 +486,8 @@ class PortsController(rest.RestController): raise exception.HTTPNotFound() return self._get_ports_collection(node_uuid or node, address, - portgroup, marker, limit, sort_key, - sort_dir, + portgroup, shard, marker, limit, + sort_key, sort_dir, resource_url='ports/detail', project=project) diff --git a/ironic/api/controllers/v1/utils.py b/ironic/api/controllers/v1/utils.py index 1f92b89bca..12cba1a4a3 100644 --- a/ironic/api/controllers/v1/utils.py +++ b/ironic/api/controllers/v1/utils.py @@ -1069,7 +1069,7 @@ def check_allow_filter_by_lessee(lessee): def check_allow_filter_by_shard(shard): """Check if filtering nodes by shard is allowed. - Version 1.81 of the API allows filtering nodes by shard. + Version 1.82 of the API allows filtering nodes by shard. """ if (shard is not None and api.request.version.minor < versions.MINOR_82_NODE_SHARD): diff --git a/ironic/db/api.py b/ironic/db/api.py index a9024f66bc..9e16f142a0 100644 --- a/ironic/db/api.py +++ b/ironic/db/api.py @@ -296,6 +296,14 @@ class Connection(object, metaclass=abc.ABCMeta): (asc, desc) """ + @abc.abstractmethod + def get_ports_by_shards(self, shards, limit=None, marker=None, + sort_key=None, sort_dir=None): + """Return a list of ports contained in the provided shards. + + :param shard_ids: A list of shards to filter ports by. + """ + @abc.abstractmethod def get_ports_by_node_id(self, node_id, limit=None, marker=None, sort_key=None, sort_dir=None): diff --git a/ironic/db/sqlalchemy/api.py b/ironic/db/sqlalchemy/api.py index bfc03b9a70..474a49ec51 100644 --- a/ironic/db/sqlalchemy/api.py +++ b/ironic/db/sqlalchemy/api.py @@ -965,6 +965,18 @@ class Connection(api.Connection): return _paginate_query(models.Port, limit, marker, sort_key, sort_dir, query) + def get_ports_by_shards(self, shards, limit=None, marker=None, + sort_key=None, sort_dir=None): + shard_node_ids = sa.select(models.Node) \ + .where(models.Node.shard.in_(shards)) \ + .with_only_columns(models.Node.id) + with _session_for_read() as session: + query = session.query(models.Port).filter( + models.Port.node_id.in_(shard_node_ids)) + ports = _paginate_query( + models.Port, limit, marker, sort_key, sort_dir, query) + return ports + def get_ports_by_node_id(self, node_id, limit=None, marker=None, sort_key=None, sort_dir=None, owner=None, project=None): diff --git a/ironic/objects/port.py b/ironic/objects/port.py index d45bee4b5a..8f6f7ddf0a 100644 --- a/ironic/objects/port.py +++ b/ironic/objects/port.py @@ -299,6 +299,27 @@ class Port(base.IronicObject, object_base.VersionedObjectDictCompat): project=project) return cls._from_db_object_list(context, db_ports) + @classmethod + def list_by_node_shards(cls, context, shards, limit=None, marker=None, + sort_key=None, sort_dir=None, project=None): + """Return a list of Port objects associated with nodes in shards + + :param context: Security context. + :param shards: a list of shards + :param limit: maximum number of resources to return in a single result. + :param marker: pagination marker for large data sets. + :param sort_key: column to sort results by. + :param sort_dir: direction to sort. "asc" or "desc". + :param project: a node owner or lessee to match against + :returns: a list of :class:`Port` object. + + """ + db_ports = cls.dbapi.get_ports_by_shards(shards, limit=limit, + marker=marker, + sort_key=sort_key, + sort_dir=sort_dir) + return cls._from_db_object_list(context, db_ports) + # 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. diff --git a/ironic/tests/unit/api/controllers/v1/test_port.py b/ironic/tests/unit/api/controllers/v1/test_port.py index 7092a4012d..4e2cf51b9b 100644 --- a/ironic/tests/unit/api/controllers/v1/test_port.py +++ b/ironic/tests/unit/api/controllers/v1/test_port.py @@ -194,7 +194,7 @@ class TestPortsController__GetPortsCollection(base.TestCase): mock_request.context = 'fake-context' mock_list.return_value = [] self.controller._get_ports_collection(None, None, None, None, None, - None, 'asc', + None, None, 'asc', resource_url='ports') mock_list.assert_called_once_with('fake-context', 1000, None, project=None, sort_dir='asc', @@ -1100,6 +1100,44 @@ class TestListPorts(test_api_base.BaseApiTest): response.json['error_message']) +class TestListPortsByShard(test_api_base.BaseApiTest): + def setUp(self): + super(TestListPortsByShard, self).setUp() + self.headers = { + api_base.Version.string: '1.%s' % versions.MINOR_82_NODE_SHARD + } + + def _create_port_with_shard(self, shard, address): + node = obj_utils.create_test_node(self.context, owner='12345', + shard=shard, + uuid=uuidutils.generate_uuid()) + return obj_utils.create_test_port(self.context, name='port_%s' % shard, + node_id=node.id, address=address, + uuid=uuidutils.generate_uuid()) + + def test_get_by_shard_single_fail_api_version(self): + self._create_port_with_shard('test_shard', 'aa:bb:cc:dd:ee:ff') + data = self.get_json('/ports?shard=test_shard', expect_errors=True) + self.assertEqual(406, data.status_int) + + def test_get_by_shard_single(self): + port = self._create_port_with_shard('test_shard', 'aa:bb:cc:dd:ee:ff') + data = self.get_json('/ports?shard=test_shard', headers=self.headers) + self.assertEqual(port.uuid, data['ports'][0]["uuid"]) + + def test_get_by_shard_multi(self): + bad_shard_address = 'ee:ee:ee:ee:ee:ee' + self._create_port_with_shard('shard1', 'aa:bb:cc:dd:ee:ff') + self._create_port_with_shard('shard2', 'ab:bb:cc:dd:ee:ff') + self._create_port_with_shard('shard3', bad_shard_address) + + res = self.get_json('/ports?shard=shard1,shard2', headers=self.headers) + self.assertEqual(2, len(res['ports'])) + print(res['ports'][0]) + self.assertNotEqual(res['ports'][0]['address'], bad_shard_address) + self.assertNotEqual(res['ports'][1]['address'], bad_shard_address) + + @mock.patch.object(rpcapi.ConductorAPI, 'update_port', autospec=True, side_effect=_rpcapi_update_port) class TestPatch(test_api_base.BaseApiTest): diff --git a/ironic/tests/unit/db/test_ports.py b/ironic/tests/unit/db/test_ports.py index 97d1e98a75..0284ee0d00 100644 --- a/ironic/tests/unit/db/test_ports.py +++ b/ironic/tests/unit/db/test_ports.py @@ -22,6 +22,23 @@ from ironic.tests.unit.db import base from ironic.tests.unit.db import utils as db_utils +def _create_test_port_with_shard(shard, address): + node = db_utils.create_test_node( + uuid=uuidutils.generate_uuid(), + owner='12345', lessee='54321', shard=shard) + pg = db_utils.create_test_portgroup( + name='pg-%s' % shard, + uuid=uuidutils.generate_uuid(), + node_id=node.id, + address=address) + return db_utils.create_test_port( + name='port-%s' % shard, + uuid=uuidutils.generate_uuid(), + node_id=node.id, + address=address, + portgroup_id=pg.id) + + class DbPortTestCase(base.DbTestCase): def setUp(self): @@ -202,6 +219,28 @@ class DbPortTestCase(base.DbTestCase): def test_get_ports_by_portgroup_id_that_does_not_exist(self): self.assertEqual([], self.dbapi.get_ports_by_portgroup_id(99)) + def test_get_ports_by_shard_no_match(self): + res = self.dbapi.get_ports_by_shards(['shard1', 'shard2']) + self.assertEqual([], res) + + def test_get_ports_by_shard_with_match_single(self): + _create_test_port_with_shard('shard1', 'aa:bb:cc:dd:ee:ff') + + res = self.dbapi.get_ports_by_shards(['shard1']) + self.assertEqual(1, len(res)) + self.assertEqual('port-shard1', res[0].name) + + def test_get_ports_by_shard_with_match_multi(self): + _create_test_port_with_shard('shard1', 'aa:bb:cc:dd:ee:ff') + _create_test_port_with_shard('shard2', 'ab:bb:cc:dd:ee:ff') + _create_test_port_with_shard('shard3', 'ac:bb:cc:dd:ee:ff') + + res = self.dbapi.get_ports_by_shards(['shard1', 'shard2']) + self.assertEqual(2, len(res)) + # note(JayF): We do not query for shard3; ensure we don't get it. + self.assertNotEqual('port-shard3', res[0].name) + self.assertNotEqual('port-shard3', res[1].name) + def test_destroy_port(self): self.dbapi.destroy_port(self.port.id) self.assertRaises(exception.PortNotFound,