From 885bfbda969f2cceeb6f5ac2c05461c86c6b5dbf Mon Sep 17 00:00:00 2001 From: Dmitry Tantsur Date: Mon, 4 Mar 2019 14:52:14 +0100 Subject: [PATCH] Allocation API: optimize check on candidate nodes Currently we fetch all nodes one by one when validating the provided candidate_nodes. This is not overly efficient, so this change introduces a new database API to specifically validate a list of names/uuids and convert it to uuids. As a nice side effect, no database query is done for identities that cannot be a valid node name or uuid. Change-Id: I7adfe897200b1b05d9e632c68d0d3d046c06a921 Story: #2004341 --- ironic/api/controllers/v1/allocation.py | 20 +++++------ ironic/db/api.py | 14 ++++++++ ironic/db/sqlalchemy/api.py | 34 ++++++++++++++++++ .../api/controllers/v1/test_allocation.py | 9 +++++ ironic/tests/unit/db/test_nodes.py | 35 +++++++++++++++++++ 5 files changed, 102 insertions(+), 10 deletions(-) diff --git a/ironic/api/controllers/v1/allocation.py b/ironic/api/controllers/v1/allocation.py index ab157d698c..504a488eb7 100644 --- a/ironic/api/controllers/v1/allocation.py +++ b/ironic/api/controllers/v1/allocation.py @@ -343,16 +343,16 @@ class AllocationsController(pecan.rest.RestController): if allocation.candidate_nodes: # Convert nodes from names to UUIDs and check their validity - converted = [] - for node in allocation.candidate_nodes: - try: - node = api_utils.get_rpc_node(node) - except exception.NodeNotFound as exc: - exc.code = http_client.BAD_REQUEST - raise - else: - converted.append(node.uuid) - allocation.candidate_nodes = converted + try: + converted = pecan.request.dbapi.check_node_list( + allocation.candidate_nodes) + except exception.NodeNotFound as exc: + exc.code = http_client.BAD_REQUEST + raise + else: + # Make sure we keep the ordering of candidate nodes. + allocation.candidate_nodes = [ + converted[ident] for ident in allocation.candidate_nodes] all_dict = allocation.as_dict() diff --git a/ironic/db/api.py b/ironic/db/api.py index 35d2997372..73e47dae87 100644 --- a/ironic/db/api.py +++ b/ironic/db/api.py @@ -97,6 +97,20 @@ class Connection(object): (asc, desc) """ + @abc.abstractmethod + def check_node_list(self, idents): + """Check a list of node identities and map it to UUIDs. + + This call takes a list of node names and/or UUIDs and tries to convert + them to UUIDs. It fails early if any identities cannot possible be used + as names or UUIDs. + + :param idents: List of identities. + :returns: A mapping from requests identities to node UUIDs. + :raises: NodeNotFound if some identities were not found or cannot be + valid names or UUIDs. + """ + @abc.abstractmethod def reserve_node(self, tag, node_id): """Reserve a node. diff --git a/ironic/db/sqlalchemy/api.py b/ironic/db/sqlalchemy/api.py index af403cc566..6ca3f15048 100644 --- a/ironic/db/sqlalchemy/api.py +++ b/ironic/db/sqlalchemy/api.py @@ -39,6 +39,7 @@ from ironic.common.i18n import _ from ironic.common import profiler from ironic.common import release_mappings from ironic.common import states +from ironic.common import utils from ironic.conf import CONF from ironic.db import api from ironic.db.sqlalchemy import models @@ -398,6 +399,39 @@ class Connection(api.Connection): return _paginate_query(models.Node, limit, marker, sort_key, sort_dir, query) + def check_node_list(self, idents): + mapping = {} + if idents: + idents = set(idents) + else: + return mapping + + uuids = {i for i in idents if uuidutils.is_uuid_like(i)} + names = {i for i in idents if not uuidutils.is_uuid_like(i) + and utils.is_valid_logical_name(i)} + missing = idents - set(uuids) - set(names) + if missing: + # Such nodes cannot exist, bailing out early + raise exception.NodeNotFound( + _("Nodes cannot be found: %s") % ', '.join(missing)) + + query = model_query(models.Node.uuid, models.Node.name).filter( + sql.or_(models.Node.uuid.in_(uuids), + models.Node.name.in_(names)) + ) + for row in query: + if row[0] in idents: + mapping[row[0]] = row[0] + if row[1] and row[1] in idents: + mapping[row[1]] = row[0] + + missing = idents - set(mapping) + if missing: + raise exception.NodeNotFound( + _("Nodes cannot be found: %s") % ', '.join(missing)) + + return mapping + @oslo_db_api.retry_on_deadlock def reserve_node(self, tag, node_id): with _session_for_write(): diff --git a/ironic/tests/unit/api/controllers/v1/test_allocation.py b/ironic/tests/unit/api/controllers/v1/test_allocation.py index 0637903aef..91124965fd 100644 --- a/ironic/tests/unit/api/controllers/v1/test_allocation.py +++ b/ironic/tests/unit/api/controllers/v1/test_allocation.py @@ -566,6 +566,15 @@ class TestPost(test_api_base.BaseApiTest): self.assertEqual(http_client.BAD_REQUEST, response.status_int) self.assertTrue(response.json['error_message']) + def test_create_allocation_candidate_node_invalid(self): + adict = apiutils.allocation_post_data( + candidate_nodes=['this/is/not a/node/name']) + response = self.post_json('/allocations', adict, expect_errors=True, + headers=self.headers) + self.assertEqual('application/json', response.content_type) + self.assertEqual(http_client.BAD_REQUEST, response.status_int) + self.assertTrue(response.json['error_message']) + def test_create_allocation_name_ok(self): name = 'foo' adict = apiutils.allocation_post_data(name=name) diff --git a/ironic/tests/unit/db/test_nodes.py b/ironic/tests/unit/db/test_nodes.py index 79002dabc6..11568b963d 100644 --- a/ironic/tests/unit/db/test_nodes.py +++ b/ironic/tests/unit/db/test_nodes.py @@ -853,3 +853,38 @@ class DbNodeTestCase(base.DbTestCase): 'Multiple nodes', self.dbapi.get_node_by_port_addresses, addresses) + + def test_check_node_list(self): + node1 = utils.create_test_node(uuid=uuidutils.generate_uuid()) + node2 = utils.create_test_node(uuid=uuidutils.generate_uuid(), + name='node_2') + node3 = utils.create_test_node(uuid=uuidutils.generate_uuid(), + name='node_3') + + mapping = self.dbapi.check_node_list([node1.uuid, node2.name, + node3.uuid]) + self.assertEqual({node1.uuid: node1.uuid, + node2.name: node2.uuid, + node3.uuid: node3.uuid}, + mapping) + + def test_check_node_list_non_existing(self): + node1 = utils.create_test_node(uuid=uuidutils.generate_uuid()) + node2 = utils.create_test_node(uuid=uuidutils.generate_uuid(), + name='node_2') + uuid = uuidutils.generate_uuid() + + exc = self.assertRaises(exception.NodeNotFound, + self.dbapi.check_node_list, + [node1.uuid, uuid, 'could-be-a-name', + node2.name]) + self.assertIn(uuid, str(exc)) + self.assertIn('could-be-a-name', str(exc)) + + def test_check_node_list_impossible(self): + node1 = utils.create_test_node(uuid=uuidutils.generate_uuid()) + + exc = self.assertRaises(exception.NodeNotFound, + self.dbapi.check_node_list, + [node1.uuid, 'this/cannot/be/a/name']) + self.assertIn('this/cannot/be/a/name', str(exc))