From 4b58b8a823cbd5ea823759be890ad3d95faa2863 Mon Sep 17 00:00:00 2001 From: Jay Faulkner Date: Wed, 10 Dec 2025 13:59:48 -0800 Subject: [PATCH] Support filtering portgroups by shard This was somehow missed during initial implementation. Adding ability to filter portgroup by shard. This was mostly vibe coded with claude, with me interupting to suggest better implementations when it did something silly. Tested manually by a human using fake drivers :). Closes-bug: #2134566 Generated-by: Claude code (claude) Signed-off-by: Jay Faulkner Change-Id: Ic67c02763c2d832f616dc4526e4be891d639b976 --- .../contributor/webapi-version-history.rst | 5 ++ ironic/api/controllers/v1/portgroup.py | 42 +++++++++--- ironic/api/controllers/v1/utils.py | 14 ++++ ironic/api/controllers/v1/versions.py | 5 +- ironic/common/release_mappings.py | 2 +- ironic/db/api.py | 15 ++++ ironic/db/sqlalchemy/api.py | 10 +++ ironic/objects/portgroup.py | 21 ++++++ .../unit/api/controllers/v1/test_portgroup.py | 68 +++++++++++++++++++ ironic/tests/unit/db/test_portgroups.py | 32 +++++++++ ...roup-filter-by-shard-941960e6e49fc638.yaml | 7 ++ 11 files changed, 207 insertions(+), 14 deletions(-) create mode 100644 releasenotes/notes/portgroup-filter-by-shard-941960e6e49fc638.yaml diff --git a/doc/source/contributor/webapi-version-history.rst b/doc/source/contributor/webapi-version-history.rst index b52f3fdb54..e8e1def63d 100644 --- a/doc/source/contributor/webapi-version-history.rst +++ b/doc/source/contributor/webapi-version-history.rst @@ -2,6 +2,11 @@ REST API Version History ======================== +1.106 (Gazpacho) +---------------------- + +Add ability to filter on a node shard when listing portgroups. + 1.105 (Gazpacho) ---------------------- diff --git a/ironic/api/controllers/v1/portgroup.py b/ironic/api/controllers/v1/portgroup.py index 47da30e483..75d62bfd24 100644 --- a/ironic/api/controllers/v1/portgroup.py +++ b/ironic/api/controllers/v1/portgroup.py @@ -195,7 +195,7 @@ class PortgroupsController(pecan.rest.RestController): super(PortgroupsController, self).__init__() self.parent_node_ident = node_ident - def _get_portgroups_collection(self, node_ident, address, + def _get_portgroups_collection(self, node_ident, address, shard, marker, limit, sort_key, sort_dir, resource_url=None, fields=None, detail=None, project=None, @@ -204,6 +204,8 @@ class PortgroupsController(pecan.rest.RestController): :param node_ident: UUID or name of a node. :param address: MAC address of a portgroup. + :param shard: A list of shards, to get only portgroups for nodes + in those shards. :param marker: Pagination marker for large data sets. :param limit: Maximum number of resources to return in a single result. :param sort_key: Column to sort results by. Default: id. @@ -229,10 +231,14 @@ class PortgroupsController(pecan.rest.RestController): node_ident = self.parent_node_ident or node_ident - if conductor_groups and (node_ident or address): - raise exception.Invalid( - _("Filtering by conductor_groups and node_ident/address " - "simultaneously is not supported.")) + exclusive_filters = 0 + for i in [node_ident, address, shard, conductor_groups]: + if i: + exclusive_filters += 1 + if exclusive_filters > 1: + raise exception.Invalid( + _("Filtering by node, address, or shard " + "simultaneously is not supported.")) if node_ident: # FIXME: Since all we need is the node ID, we can @@ -247,6 +253,10 @@ class PortgroupsController(pecan.rest.RestController): elif address: portgroups = self._get_portgroups_by_address(address, project=project) + elif shard: + portgroups = objects.Portgroup.list_by_node_shards( + api.request.context, shard, limit, + marker_obj, sort_key=sort_key, sort_dir=sort_dir) else: portgroups = objects.Portgroup.list( api.request.context, limit, @@ -285,10 +295,11 @@ class PortgroupsController(pecan.rest.RestController): @args.validate(node=args.uuid_or_name, address=args.mac_address, marker=args.uuid, limit=args.integer, sort_key=args.string, sort_dir=args.string, fields=args.string_list, - detail=args.boolean, conductor_groups=args.string_list) + detail=args.boolean, shard=args.string_list, + conductor_groups=args.string_list) def get_all(self, node=None, address=None, marker=None, limit=None, sort_key='id', sort_dir='asc', fields=None, - detail=None, conductor_groups=None): + detail=None, shard=None, conductor_groups=None): """Retrieve a list of portgroups. :param node: UUID or name of a node, to get only portgroups for that @@ -304,6 +315,9 @@ class PortgroupsController(pecan.rest.RestController): :param sort_dir: direction to sort. "asc" or "desc". Default: asc. :param fields: Optional, a list with a specified set of fields of the resource to be returned. + :param shard: Optional, a list of shard ids to filter by, only + portgroups associated with nodes in these shards will + be returned. :param conductor_groups: conductor groups, to filter the request by. """ if not api_utils.allow_portgroups(): @@ -318,6 +332,7 @@ class PortgroupsController(pecan.rest.RestController): portgroup=True, parent_node=self.parent_node_ident) + api_utils.check_allow_portgroup_filter_by_shard(shard) api_utils.check_allowed_portgroup_fields(fields) api_utils.check_allowed_portgroup_fields([sort_key]) @@ -325,7 +340,7 @@ class PortgroupsController(pecan.rest.RestController): _DEFAULT_RETURN_FIELDS) return self._get_portgroups_collection( - node, address, marker, limit, sort_key, sort_dir, + node, address, shard, marker, limit, sort_key, sort_dir, fields=fields, resource_url='portgroups', detail=detail, @@ -336,10 +351,11 @@ class PortgroupsController(pecan.rest.RestController): @method.expose() @args.validate(node=args.uuid_or_name, address=args.mac_address, marker=args.uuid, limit=args.integer, sort_key=args.string, - sort_dir=args.string, conductor_groups=args.string_list) + sort_dir=args.string, shard=args.string_list, + conductor_groups=args.string_list) def detail(self, node=None, address=None, marker=None, limit=None, sort_key='id', sort_dir='asc', - conductor_groups=None): + shard=None, conductor_groups=None): """Retrieve a list of portgroups with detail. :param node: UUID or name of a node, to get only portgroups for that @@ -353,6 +369,9 @@ class PortgroupsController(pecan.rest.RestController): max_limit resources will be returned. :param sort_key: column to sort results by. Default: id. :param sort_dir: direction to sort. "asc" or "desc". Default: asc. + :param shard: Optional, a list of shard ids to filter by, only + portgroups associated with nodes in these shards will + be returned. :param conductor_groups: conductor groups, to filter the request by. """ if not api_utils.allow_portgroups(): @@ -367,6 +386,7 @@ class PortgroupsController(pecan.rest.RestController): portgroup=True, parent_node=self.parent_node_ident) + api_utils.check_allow_portgroup_filter_by_shard(shard) api_utils.check_allowed_portgroup_fields([sort_key]) # NOTE: /detail should only work against collections @@ -375,7 +395,7 @@ class PortgroupsController(pecan.rest.RestController): raise exception.HTTPNotFound() return self._get_portgroups_collection( - node, address, marker, limit, sort_key, sort_dir, + node, address, shard, marker, limit, sort_key, sort_dir, resource_url='portgroups/detail', project=project, conductor_groups=conductor_groups) diff --git a/ironic/api/controllers/v1/utils.py b/ironic/api/controllers/v1/utils.py index 5784a916c8..16a706a1a7 100644 --- a/ironic/api/controllers/v1/utils.py +++ b/ironic/api/controllers/v1/utils.py @@ -1174,6 +1174,20 @@ def check_allow_filter_by_shard(shard): 'opr': versions.MINOR_82_NODE_SHARD}) +def check_allow_portgroup_filter_by_shard(shard): + """Check if filtering portgroups by shard is allowed. + + Version 1.106 of the API allows filtering portgroups by shard. + """ + if (shard is not None and api.request.version.minor + < versions.MINOR_106_PORTGROUP_SHARD): + raise exception.NotAcceptable(_( + "Request not acceptable. The minimal required API version " + "should be %(base)s.%(opr)s") % + {'base': versions.BASE_VERSION, + 'opr': versions.MINOR_106_PORTGROUP_SHARD}) + + def check_allow_child_node_params(include_children=None, parent_node=None): if ((include_children is not None diff --git a/ironic/api/controllers/v1/versions.py b/ironic/api/controllers/v1/versions.py index 278fe425bf..3e5652567f 100644 --- a/ironic/api/controllers/v1/versions.py +++ b/ironic/api/controllers/v1/versions.py @@ -142,8 +142,8 @@ BASE_VERSION = 1 # v1.102: Add physical_network field to portgroup. # v1.103: Add category field to portgroup # v1.104: Add instance_name to node -# v1.103: Add category field to portgroup. # v1.105: Remove broken ovn vtep metadata support +# v1.106: Add shard filtering support for portgroups MINOR_0_JUNO = 0 MINOR_1_INITIAL_VERSION = 1 @@ -251,6 +251,7 @@ MINOR_102_PORTGROUP_PHYSICAL_NETWORK = 102 MINOR_103_PORTGROUP_CATEGORY = 103 MINOR_104_NODE_INSTANCE_NAME = 104 MINOR_105_REMOVE_OVN_VTEP = 105 +MINOR_106_PORTGROUP_SHARD = 106 # When adding another version, update: # - MINOR_MAX_VERSION @@ -260,7 +261,7 @@ MINOR_105_REMOVE_OVN_VTEP = 105 # - Add a comment describing the change above the list of consts -MINOR_MAX_VERSION = MINOR_105_REMOVE_OVN_VTEP +MINOR_MAX_VERSION = MINOR_106_PORTGROUP_SHARD # String representations of the minor and maximum versions _MIN_VERSION_STRING = '{}.{}'.format(BASE_VERSION, MINOR_1_INITIAL_VERSION) diff --git a/ironic/common/release_mappings.py b/ironic/common/release_mappings.py index 5dc0b6ab0b..8759595bf5 100644 --- a/ironic/common/release_mappings.py +++ b/ironic/common/release_mappings.py @@ -944,7 +944,7 @@ RELEASE_MAPPING = { # make it below. To release, we will preserve a version matching # the release as a separate block of text, like above. 'master': { - 'api': '1.105', + 'api': '1.106', 'rpc': '1.62', 'networking_rpc': '1.0', 'objects': { diff --git a/ironic/db/api.py b/ironic/db/api.py index a7a54cd1e8..7492c62dbe 100644 --- a/ironic/db/api.py +++ b/ironic/db/api.py @@ -438,6 +438,21 @@ class Connection(object, metaclass=abc.ABCMeta): :returns: A list of portgroups. """ + @abc.abstractmethod + def get_portgroups_by_shards(self, shards, limit=None, marker=None, + sort_key=None, sort_dir=None): + """Return a list of portgroups contained in the provided shards. + + :param shards: A list of shards to filter portgroups by. + :param limit: Maximum number of portgroups to return. + :param marker: The last item of the previous page; we return the next + result set. + :param sort_key: Attribute by which results should be sorted + :param sort_dir: Direction in which results should be sorted + (asc, desc) + :returns: A list of portgroups. + """ + @abc.abstractmethod def create_portgroup(self, values): """Create a new portgroup. diff --git a/ironic/db/sqlalchemy/api.py b/ironic/db/sqlalchemy/api.py index 708a63d45b..cc17b70d92 100644 --- a/ironic/db/sqlalchemy/api.py +++ b/ironic/db/sqlalchemy/api.py @@ -1263,6 +1263,16 @@ class Connection(api.Connection): return _paginate_query(models.Portgroup, limit, marker, sort_key, sort_dir, query) + def get_portgroups_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) + query = sa.select(models.Portgroup) \ + .where(models.Portgroup.node_id.in_(shard_node_ids)) + return _paginate_query( + models.Portgroup, limit, marker, sort_key, sort_dir, query) + @oslo_db_api.retry_on_deadlock def create_portgroup(self, values): if not values.get('uuid'): diff --git a/ironic/objects/portgroup.py b/ironic/objects/portgroup.py index 7d3dbbe4f9..9036443c33 100644 --- a/ironic/objects/portgroup.py +++ b/ironic/objects/portgroup.py @@ -281,6 +281,27 @@ class Portgroup(base.IronicObject, object_base.VersionedObjectDictCompat): project=project) return cls._from_db_object_list(context, db_portgroups) + @classmethod + def list_by_node_shards(cls, context, shards, limit=None, marker=None, + sort_key=None, sort_dir=None): + """Return a list of Portgroup 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". + :returns: A list of :class:`Portgroup` object. + + """ + db_portgroups = cls.dbapi.get_portgroups_by_shards(shards, + limit=limit, + marker=marker, + sort_key=sort_key, + sort_dir=sort_dir) + return cls._from_db_object_list(context, db_portgroups) + @object_base.remotable def create(self, context=None): """Create a Portgroup record in the DB. diff --git a/ironic/tests/unit/api/controllers/v1/test_portgroup.py b/ironic/tests/unit/api/controllers/v1/test_portgroup.py index 760123e323..ecb3aff417 100644 --- a/ironic/tests/unit/api/controllers/v1/test_portgroup.py +++ b/ironic/tests/unit/api/controllers/v1/test_portgroup.py @@ -27,6 +27,7 @@ from ironic.api.controllers import base as api_base from ironic.api.controllers import v1 as api_v1 from ironic.api.controllers.v1 import notification_utils from ironic.api.controllers.v1 import utils as api_utils +from ironic.api.controllers.v1 import versions from ironic.common import exception from ironic.common import states from ironic.conductor import rpcapi @@ -640,6 +641,73 @@ class TestListPortgroups(test_api_base.BaseApiTest): self.assertEqual([], data['portgroups']) +class TestListPortgroupsByShard(test_api_base.BaseApiTest): + + def setUp(self): + super(TestListPortgroupsByShard, self).setUp() + self.headers = { + api_base.Version.string: '1.%s' % + versions.MINOR_106_PORTGROUP_SHARD + } + + def _create_portgroup_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_portgroup( + self.context, name='portgroup_%s' % shard, + node_id=node.id, address=address, + uuid=uuidutils.generate_uuid()) + + def test_get_by_shard_single_fail_api_version(self): + self._create_portgroup_with_shard('test_shard', 'aa:bb:cc:dd:ee:ff') + # Use a version that supports portgroups but not shard filtering + old_headers = { + api_base.Version.string: '1.%s' % + versions.MINOR_104_NODE_INSTANCE_NAME + } + data = self.get_json('/portgroups?shard=test_shard', + headers=old_headers, expect_errors=True) + self.assertEqual(406, data.status_int) + + def test_get_by_shard_single(self): + portgroup = self._create_portgroup_with_shard('test_shard', + 'aa:bb:cc:dd:ee:ff') + data = self.get_json('/portgroups?shard=test_shard', + headers=self.headers) + self.assertEqual(portgroup.uuid, data['portgroups'][0]["uuid"]) + + def test_get_by_shard_multi(self): + bad_shard_address = 'ee:ee:ee:ee:ee:ee' + self._create_portgroup_with_shard('shard1', 'aa:bb:cc:dd:ee:ff') + self._create_portgroup_with_shard('shard2', 'ab:bb:cc:dd:ee:ff') + self._create_portgroup_with_shard('shard3', bad_shard_address) + + res = self.get_json('/portgroups?shard=shard1,shard2', + headers=self.headers) + self.assertEqual(2, len(res['portgroups'])) + self.assertNotEqual(res['portgroups'][0]['address'], bad_shard_address) + self.assertNotEqual(res['portgroups'][1]['address'], bad_shard_address) + + def test_get_by_shard_detail(self): + portgroup = self._create_portgroup_with_shard('test_shard', + 'aa:bb:cc:dd:ee:ff') + data = self.get_json('/portgroups/detail?shard=test_shard', + headers=self.headers) + self.assertEqual(portgroup.uuid, data['portgroups'][0]["uuid"]) + self.assertIn('extra', data['portgroups'][0]) + self.assertIn('node_uuid', data['portgroups'][0]) + + def test_get_by_shard_and_node_fails(self): + portgroup = self._create_portgroup_with_shard('test_shard', + 'aa:bb:cc:dd:ee:ff') + node_uuid = portgroup.node_uuid + data = self.get_json( + '/portgroups?shard=test_shard&node=%s' % node_uuid, + headers=self.headers, expect_errors=True) + self.assertEqual(400, data.status_int) + + @mock.patch.object(rpcapi.ConductorAPI, 'update_portgroup', autospec=True) class TestPatch(test_api_base.BaseApiTest): headers = {api_base.Version.string: str(api_v1.max_version())} diff --git a/ironic/tests/unit/db/test_portgroups.py b/ironic/tests/unit/db/test_portgroups.py index ad1e1cf099..ffb4f97290 100644 --- a/ironic/tests/unit/db/test_portgroups.py +++ b/ironic/tests/unit/db/test_portgroups.py @@ -146,6 +146,38 @@ class DbportgroupTestCase(base.DbTestCase): res_uuids = [r.uuid for r in res] self.assertJsonEqual(group_a_uuids + group_b_uuids, res_uuids) + def _create_test_portgroup_with_shard(self, shard, address): + node = db_utils.create_test_node( + uuid=uuidutils.generate_uuid(), + owner='12345', lessee='54321', shard=shard) + return db_utils.create_test_portgroup( + name='portgroup-%s' % shard, + uuid=uuidutils.generate_uuid(), + node_id=node.id, + address=address) + + def test_get_portgroups_by_shard_no_match(self): + res = self.dbapi.get_portgroups_by_shards(['shard1', 'shard2']) + self.assertEqual([], res) + + def test_get_portgroups_by_shard_with_match_single(self): + self._create_test_portgroup_with_shard('shard1', 'aa:bb:cc:dd:ee:ff') + + res = self.dbapi.get_portgroups_by_shards(['shard1']) + self.assertEqual(1, len(res)) + self.assertEqual('portgroup-shard1', res[0].name) + + def test_get_portgroups_by_shard_with_match_multi(self): + self._create_test_portgroup_with_shard('shard1', 'aa:bb:cc:dd:ee:ff') + self._create_test_portgroup_with_shard('shard2', 'ab:bb:cc:dd:ee:ff') + self._create_test_portgroup_with_shard('shard3', 'ac:bb:cc:dd:ee:ff') + + res = self.dbapi.get_portgroups_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('portgroup-shard3', res[0].name) + self.assertNotEqual('portgroup-shard3', res[1].name) + def test_destroy_portgroup(self): self.dbapi.destroy_portgroup(self.portgroup.id) self.assertRaises(exception.PortgroupNotFound, diff --git a/releasenotes/notes/portgroup-filter-by-shard-941960e6e49fc638.yaml b/releasenotes/notes/portgroup-filter-by-shard-941960e6e49fc638.yaml new file mode 100644 index 0000000000..26bb35025a --- /dev/null +++ b/releasenotes/notes/portgroup-filter-by-shard-941960e6e49fc638.yaml @@ -0,0 +1,7 @@ +--- +features: + - | + The ``/v1/portgroups`` API endpoint now accepts filtering by shard. + In API version 1.106, requests with the ``shard`` parameter will only + return portgroups associated with nodes assigned to that shard. +