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

5 files changed, 102 insertions, 10 deletions

diff --git a/ironic/api/controllers/v1/allocation.py b/ironic/api/controllers/v1/allocation.py
index ab157d698..504a488eb 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 35d299737..73e47dae8 100644
--- a/ironic/db/api.py
+++ b/ironic/db/api.py
@@ -98,6 +98,20 @@ class Connection(object):
         """
 
     @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 af403cc56..6ca3f1504 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 0637903ae..91124965f 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 79002dabc..11568b963 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))