From fb93d4bc3cd8f500f937884574d1b3165e8e8887 Mon Sep 17 00:00:00 2001 From: Dmitry Tantsur Date: Thu, 3 Jan 2019 15:01:04 +0100 Subject: [PATCH] Allocation API: minor fixes to DB and RPC This is a follow-up to commit 36b047ff5e1b0c1cf64d4b58bf8a9ce3bcc87e85: * Do the join on the database level when filtering allocations by node UUID * Add a check that we do not touch instance_info in destroy_allocation to document it as an explicit decision. * Fix a few grammar nits and inprecise comments. Change-Id: I1fa7815ae3b6a0190bdb3bf0257ae79fb6a36671 Story: #2004341 --- ironic/db/sqlalchemy/api.py | 16 ++++++++++---- ironic/objects/allocation.py | 28 ++++++++++++------------ ironic/tests/unit/db/test_allocations.py | 6 ++++- 3 files changed, 31 insertions(+), 19 deletions(-) diff --git a/ironic/db/sqlalchemy/api.py b/ironic/db/sqlalchemy/api.py index beaca9339c..a6d110dc1e 100644 --- a/ironic/db/sqlalchemy/api.py +++ b/ironic/db/sqlalchemy/api.py @@ -182,6 +182,15 @@ def add_node_filter_by_chassis(query, value): return query.filter(models.Chassis.uuid == value) +def add_allocation_filter_by_node(query, value): + if strutils.is_int_like(value): + return query.filter_by(node_id=value) + else: + query = query.join(models.Node, + models.Allocation.node_id == models.Node.id) + return query.filter(models.Node.uuid == value) + + def _paginate_query(model, limit=None, marker=None, sort_key=None, sort_dir=None, query=None): if not query: @@ -289,8 +298,7 @@ class Connection(api.Connection): except KeyError: pass else: - node_obj = self.get_node_by_uuid(node_uuid) - filters['node_id'] = node_obj.id + query = add_allocation_filter_by_node(query, node_uuid) if filters: query = query.filter_by(**filters) @@ -1656,8 +1664,8 @@ class Connection(api.Connection): raise exception.AllocationDuplicateName( name=values['name']) elif 'instance_uuid' in exc.columns: - # Case when the referenced node is associated with an - # instance already. + # Case when the allocation UUID is already used on some + # node as instance_uuid. raise exception.InstanceAssociated( instance_uuid=instance_uuid, node=node_uuid) else: diff --git a/ironic/objects/allocation.py b/ironic/objects/allocation.py index 1bfe44a0a7..655b9ad1ec 100644 --- a/ironic/objects/allocation.py +++ b/ironic/objects/allocation.py @@ -64,11 +64,11 @@ class Allocation(base.IronicObject, object_base.VersionedObjectDictCompat): # @object_base.remotable_classmethod @classmethod def get(cls, context, allocation_ident): - """Find a allocation based on its id, uuid, name or address. + """Find an allocation by its ID, UUID or name. - :param allocation_ident: The id, uuid, name or address of a allocation. + :param allocation_ident: The ID, UUID or name of an allocation. :param context: Security context - :returns: A :class:`Allocation` object. + :returns: An :class:`Allocation` object. :raises: InvalidIdentity """ @@ -87,12 +87,12 @@ class Allocation(base.IronicObject, object_base.VersionedObjectDictCompat): # @object_base.remotable_classmethod @classmethod def get_by_id(cls, context, allocation_id): - """Find a allocation by its integer ID and return a Allocation object. + """Find an allocation by its integer ID. :param cls: the :class:`Allocation` :param context: Security context - :param allocation_id: The ID of a allocation. - :returns: A :class:`Allocation` object. + :param allocation_id: The ID of an allocation. + :returns: An :class:`Allocation` object. :raises: AllocationNotFound """ @@ -106,12 +106,12 @@ class Allocation(base.IronicObject, object_base.VersionedObjectDictCompat): # @object_base.remotable_classmethod @classmethod def get_by_uuid(cls, context, uuid): - """Find a allocation by UUID and return a :class:`Allocation` object. + """Find an allocation by its UUID. :param cls: the :class:`Allocation` :param context: Security context - :param uuid: The UUID of a allocation. - :returns: A :class:`Allocation` object. + :param uuid: The UUID of an allocation. + :returns: An :class:`Allocation` object. :raises: AllocationNotFound """ @@ -125,12 +125,12 @@ class Allocation(base.IronicObject, object_base.VersionedObjectDictCompat): # @object_base.remotable_classmethod @classmethod def get_by_name(cls, context, name): - """Find allocation based on name and return a :class:`Allocation` object. + """Find an allocation based by its name. :param cls: the :class:`Allocation` :param context: Security context - :param name: The name of a allocation. - :returns: A :class:`Allocation` object. + :param name: The name of an allocation. + :returns: An :class:`Allocation` object. :raises: AllocationNotFound """ @@ -234,7 +234,7 @@ class Allocation(base.IronicObject, object_base.VersionedObjectDictCompat): def refresh(self, context=None): """Loads updates for this Allocation. - Loads a allocation with the same uuid from the database and + Loads an allocation with the same uuid from the database and checks for updated attributes. Updates are applied from the loaded allocation column by column, if there are any updates. @@ -254,7 +254,7 @@ class Allocation(base.IronicObject, object_base.VersionedObjectDictCompat): @base.IronicObjectRegistry.register class AllocationCRUDNotification(notification.NotificationBase): - """Notification when ironic creates, updates or deletes a allocation.""" + """Notification when ironic creates, updates or deletes an allocation.""" # Version 1.0: Initial version VERSION = '1.0' diff --git a/ironic/tests/unit/db/test_allocations.py b/ironic/tests/unit/db/test_allocations.py index 451b087faf..8809c7a764 100644 --- a/ironic/tests/unit/db/test_allocations.py +++ b/ironic/tests/unit/db/test_allocations.py @@ -125,13 +125,17 @@ class AllocationsTestCase(base.DbTestCase): def test_destroy_allocation_with_node(self): self.dbapi.update_node(self.node.id, {'allocation_id': self.allocation.id, - 'instance_uuid': uuidutils.generate_uuid()}) + 'instance_uuid': uuidutils.generate_uuid(), + 'instance_info': {'traits': ['foo']}}) self.dbapi.destroy_allocation(self.allocation.id) self.assertRaises(exception.AllocationNotFound, self.dbapi.get_allocation_by_id, self.allocation.id) node = self.dbapi.get_node_by_id(self.node.id) self.assertIsNone(node.allocation_id) self.assertIsNone(node.instance_uuid) + # NOTE(dtantsur): currently we do not clean up instance_info contents + # on deallocation. It may be changed in the future. + self.assertEqual(node.instance_info, {'traits': ['foo']}) def test_destroy_allocation_that_does_not_exist(self): self.assertRaises(exception.AllocationNotFound,