From 36ef217fdb3a63ad70a567a94f1229922409964b Mon Sep 17 00:00:00 2001 From: Jay Faulkner Date: Thu, 10 Nov 2022 15:54:44 -0800 Subject: [PATCH] DB & Object layer for node.shard DB and object implementations for new node.shard key. Story: 2010768 Task: 46624 Change-Id: Ia7ef3cffc321c93501b1cc5185972a4ac1dcb212 --- ironic/api/controllers/v1/node.py | 2 + ironic/common/release_mappings.py | 2 +- ironic/db/api.py | 9 ++++ .../4dbec778866e_create_node_shard.py | 31 +++++++++++++ ironic/db/sqlalchemy/api.py | 28 +++++++++++ ironic/db/sqlalchemy/models.py | 3 ++ ironic/objects/node.py | 8 +++- .../unit/db/sqlalchemy/test_migrations.py | 4 ++ ironic/tests/unit/db/test_shard.py | 46 +++++++++++++++++++ ironic/tests/unit/db/utils.py | 1 + ironic/tests/unit/objects/test_objects.py | 2 +- 11 files changed, 132 insertions(+), 4 deletions(-) create mode 100644 ironic/db/sqlalchemy/alembic/versions/4dbec778866e_create_node_shard.py create mode 100644 ironic/tests/unit/db/test_shard.py diff --git a/ironic/api/controllers/v1/node.py b/ironic/api/controllers/v1/node.py index 312143500c..4b2f61b1cc 100644 --- a/ironic/api/controllers/v1/node.py +++ b/ironic/api/controllers/v1/node.py @@ -180,6 +180,7 @@ def node_schema(): 'retired': {'type': ['string', 'boolean', 'null']}, 'retired_reason': {'type': ['string', 'null']}, 'secure_boot': {'type': ['string', 'boolean', 'null']}, + 'shard': {'type': ['string', 'null']}, 'storage_interface': {'type': ['string', 'null']}, 'uuid': {'type': ['string', 'null']}, 'vendor_interface': {'type': ['string', 'null']}, @@ -1383,6 +1384,7 @@ def _get_fields_for_node_query(fields=None): 'retired', 'retired_reason', 'secure_boot', + 'shard', 'storage_interface', 'target_power_state', 'target_provision_state', diff --git a/ironic/common/release_mappings.py b/ironic/common/release_mappings.py index 79636e30ea..6d96bf20b7 100644 --- a/ironic/common/release_mappings.py +++ b/ironic/common/release_mappings.py @@ -516,7 +516,7 @@ RELEASE_MAPPING = { 'objects': { 'Allocation': ['1.1'], 'BIOSSetting': ['1.1'], - 'Node': ['1.36'], + 'Node': ['1.37'], 'NodeHistory': ['1.0'], 'NodeInventory': ['1.0'], 'Conductor': ['1.3'], diff --git a/ironic/db/api.py b/ironic/db/api.py index dc723a5fc8..a9024f66bc 100644 --- a/ironic/db/api.py +++ b/ironic/db/api.py @@ -72,6 +72,7 @@ class Connection(object, metaclass=abc.ABCMeta): :reserved_by_any_of: [conductor1, conductor2] :resource_class: resource class name :retired: True | False + :shard_in: shard (multiple possibilities) :provision_state: provision state of node :provision_state_in: provision state of node (multiple possibilities) @@ -106,6 +107,7 @@ class Connection(object, metaclass=abc.ABCMeta): :provisioned_before: nodes with provision_updated_at field before this interval in seconds + :shard: nodes with the given shard :param limit: Maximum number of nodes to return. :param marker: the last item of the previous page; we return the next result set. @@ -1455,3 +1457,10 @@ class Connection(object, metaclass=abc.ABCMeta): :param node_id: The integer node ID. :returns: An inventory of a node. """ + + @abc.abstractmethod + def get_shard_list(self): + """Retrieve a list of shards. + + :returns: list of dicts containing shard names and count + """ diff --git a/ironic/db/sqlalchemy/alembic/versions/4dbec778866e_create_node_shard.py b/ironic/db/sqlalchemy/alembic/versions/4dbec778866e_create_node_shard.py new file mode 100644 index 0000000000..a446da7012 --- /dev/null +++ b/ironic/db/sqlalchemy/alembic/versions/4dbec778866e_create_node_shard.py @@ -0,0 +1,31 @@ +# 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. +"""create node.shard + +Revision ID: 4dbec778866e +Revises: 0ac0f39bc5aa +Create Date: 2022-11-10 14:20:59.175355 + +""" +from alembic import op +import sqlalchemy as sa + + +# revision identifiers, used by Alembic. +revision = '4dbec778866e' +down_revision = '0ac0f39bc5aa' + + +def upgrade(): + op.add_column('nodes', sa.Column('shard', sa.String(length=255), + nullable=True)) + op.create_index('shard_idx', 'nodes', ['shard'], unique=False) diff --git a/ironic/db/sqlalchemy/api.py b/ironic/db/sqlalchemy/api.py index 7a6e6862e3..7e9254b052 100644 --- a/ironic/db/sqlalchemy/api.py +++ b/ironic/db/sqlalchemy/api.py @@ -2588,3 +2588,31 @@ class Connection(api.Connection): return query.one() except NoResultFound: raise exception.NodeInventoryNotFound(node_id=node_id) + + def get_shard_list(self): + """Return a list of shards. + + :returns: A list of dicts containing the keys name and count. + """ + # Note(JayF): This should never be a large enough list to require + # pagination. Furthermore, it wouldn't really be a sensible + # thing to paginate as the data it's fetching can mutate. + # So we just aren't even going to try. + shard_list = [] + with _session_for_read() as session: + res = session.execute( + # Note(JayF): SQLAlchemy counts are notoriously slow because + # sometimes they will use a subquery. Be careful + # before changing this to use any magic. + sa.text( + "SELECT count(id), shard from nodes group by shard;" + )).fetchall() + + if res: + res.sort(key=lambda x: x[0], reverse=True) + for shard in res: + shard_list.append( + {"name": str(shard[1]), "count": shard[0]} + ) + + return shard_list diff --git a/ironic/db/sqlalchemy/models.py b/ironic/db/sqlalchemy/models.py index 6fc8e21ab1..29863603cd 100644 --- a/ironic/db/sqlalchemy/models.py +++ b/ironic/db/sqlalchemy/models.py @@ -134,6 +134,7 @@ class NodeBase(Base): Index('reservation_idx', 'reservation'), Index('conductor_group_idx', 'conductor_group'), Index('resource_class_idx', 'resource_class'), + Index('shard_idx', 'shard'), table_args()) id = Column(Integer, primary_key=True) uuid = Column(String(36)) @@ -214,6 +215,8 @@ class NodeBase(Base): boot_mode = Column(String(16), nullable=True) secure_boot = Column(Boolean, nullable=True) + shard = Column(String(255), nullable=True) + class Node(NodeBase): """Represents a bare metal node.""" diff --git a/ironic/objects/node.py b/ironic/objects/node.py index 7c6c8bc1ae..93df5b3c10 100644 --- a/ironic/objects/node.py +++ b/ironic/objects/node.py @@ -78,7 +78,8 @@ class Node(base.IronicObject, object_base.VersionedObjectDictCompat): # Version 1.34: Add lessee field # Version 1.35: Add network_data field # Version 1.36: Add boot_mode and secure_boot fields - VERSION = '1.36' + # Version 1.37: Add shard field + VERSION = '1.37' dbapi = db_api.get_instance() @@ -170,6 +171,7 @@ class Node(base.IronicObject, object_base.VersionedObjectDictCompat): 'network_data': object_fields.FlexibleDictField(nullable=True), 'boot_mode': object_fields.StringField(nullable=True), 'secure_boot': object_fields.BooleanField(nullable=True), + 'shard': object_fields.StringField(nullable=True), } def as_dict(self, secure=False, mask_configdrive=True): @@ -656,6 +658,8 @@ class Node(base.IronicObject, object_base.VersionedObjectDictCompat): should be set to empty dict (or removed). Version 1.36: boot_mode, secure_boot were was added. Defaults are None. For versions prior to this, it should be set to None or removed. + Version 1.37: shard was added. Default is None. For versions prior to + this, it should be set to None or removed. :param target_version: the desired version of the object :param remove_unavailable_fields: True to remove fields that are @@ -671,7 +675,7 @@ class Node(base.IronicObject, object_base.VersionedObjectDictCompat): ('automated_clean', 28), ('protected_reason', 29), ('owner', 30), ('allocation_id', 31), ('description', 32), ('retired_reason', 33), ('lessee', 34), ('boot_mode', 36), - ('secure_boot', 36)] + ('secure_boot', 36), ('shard', 37)] for name, minor in fields: self._adjust_field_to_version(name, None, target_version, diff --git a/ironic/tests/unit/db/sqlalchemy/test_migrations.py b/ironic/tests/unit/db/sqlalchemy/test_migrations.py index f9081a6dff..4abd1cbc4a 100644 --- a/ironic/tests/unit/db/sqlalchemy/test_migrations.py +++ b/ironic/tests/unit/db/sqlalchemy/test_migrations.py @@ -1257,6 +1257,10 @@ class MigrationCheckersMixin(object): self.assertIsInstance(node_inventory.c.node_id.type, sqlalchemy.types.Integer) + def _check_4dbec778866e(self, engine, data): + nodes = db_utils.get_table(engine, 'nodes') + self.assertIsInstance(nodes.c.shard.type, sqlalchemy.types.String) + 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_shard.py b/ironic/tests/unit/db/test_shard.py new file mode 100644 index 0000000000..b4b7b129f3 --- /dev/null +++ b/ironic/tests/unit/db/test_shard.py @@ -0,0 +1,46 @@ +# 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. + +"""Tests for fetching shards via the DB API""" +import uuid + +from oslo_db.sqlalchemy import enginefacade + +from ironic.tests.unit.db import base +from ironic.tests.unit.db import utils + + +class ShardTestCase(base.DbTestCase): + def setUp(self): + super(ShardTestCase, self).setUp() + self.engine = enginefacade.writer.get_engine() + + def test_get_shard_list(self): + """Validate shard list is returned, and with correct sorting.""" + for i in range(1, 2): + utils.create_test_node(uuid=str(uuid.uuid4())) + for i in range(1, 3): + utils.create_test_node(uuid=str(uuid.uuid4()), shard="shard1") + for i in range(1, 4): + utils.create_test_node(uuid=str(uuid.uuid4()), shard="shard2") + + res = self.dbapi.get_shard_list() + self.assertEqual(res, [ + {"name": "shard2", "count": 3}, + {"name": "shard1", "count": 2}, + {"name": "None", "count": 1}, + ]) + + def test_get_shard_empty_list(self): + """Validate empty list is returned if no assigned shards.""" + res = self.dbapi.get_shard_list() + self.assertEqual(res, []) diff --git a/ironic/tests/unit/db/utils.py b/ironic/tests/unit/db/utils.py index 87ba4b5147..bc2b1244e2 100644 --- a/ironic/tests/unit/db/utils.py +++ b/ironic/tests/unit/db/utils.py @@ -237,6 +237,7 @@ def get_test_node(**kw): 'network_data': kw.get('network_data'), 'boot_mode': kw.get('boot_mode', None), 'secure_boot': kw.get('secure_boot', None), + 'shard': kw.get('shard', None) } for iface in drivers_base.ALL_INTERFACES: diff --git a/ironic/tests/unit/objects/test_objects.py b/ironic/tests/unit/objects/test_objects.py index 017a0d2103..8e903eed7f 100644 --- a/ironic/tests/unit/objects/test_objects.py +++ b/ironic/tests/unit/objects/test_objects.py @@ -676,7 +676,7 @@ class TestObject(_LocalTest, _TestObject): # version bump. It is an MD5 hash of the object fields and remotable methods. # The fingerprint values should only be changed if there is a version bump. expected_object_fingerprints = { - 'Node': '1.36-8a080e31ba89ca5f09e859bd259b54dc', + 'Node': '1.37-6b38eb91aec57532547ea8607f95675a', 'MyObj': '1.5-9459d30d6954bffc7a9afd347a807ca6', 'Chassis': '1.3-d656e039fd8ae9f34efc232ab3980905', 'Port': '1.11-97bf15b61224f26c65e90f007d78bfd2',