From fa2d6685f373168a9dfe4cd1f7d83af9d87a025c Mon Sep 17 00:00:00 2001 From: Mahnoor Asghar Date: Mon, 5 Jun 2023 01:33:32 -0400 Subject: [PATCH] Handle duplicate node inventory entries per node When a node is inspected more than one time and the database is configured as a storage backend, a new entry is made in the database for each inspection result (node inventory). This patch handles this behaviour as follows: By deleting previous inventory entries for the same node before adding a new entry in the database. By retrieving the most recent node inventory from the database when the database is queried. Change-Id: Ic3df86f395601742d2fea2bcde62f7547067d8e4 --- ironic/db/sqlalchemy/api.py | 39 +++++++++++++------ .../unit/api/controllers/v1/test_node.py | 4 +- ironic/tests/unit/db/test_nodes.py | 26 +++++++++++++ ironic/tests/unit/db/utils.py | 2 +- ...ate-node-inventories-fc41a7decbb6da76.yaml | 15 +++++++ 5 files changed, 72 insertions(+), 14 deletions(-) create mode 100644 releasenotes/notes/handle-duplicate-node-inventories-fc41a7decbb6da76.yaml diff --git a/ironic/db/sqlalchemy/api.py b/ironic/db/sqlalchemy/api.py index 38bc6950cd..e0a36e291f 100644 --- a/ironic/db/sqlalchemy/api.py +++ b/ironic/db/sqlalchemy/api.py @@ -2651,12 +2651,13 @@ class Connection(api.Connection): inventory = models.NodeInventory() inventory.update(values) with _session_for_write() as session: - try: - session.add(inventory) - session.flush() - except db_exc.DBDuplicateEntry: - raise exception.NodeInventoryAlreadyExists( - id=values['id']) + session.query( + models.NodeInventory + ).filter( + models.NodeInventory.node_id == values['node_id'] + ).delete() + session.add(inventory) + session.flush() return inventory @oslo_db_api.retry_on_deadlock @@ -2670,12 +2671,26 @@ class Connection(api.Connection): node=node_id) def get_node_inventory_by_node_id(self, node_id): - try: - with _session_for_read() as session: - query = session.query(models.NodeInventory).filter_by( - node_id=node_id) - res = query.one() - except NoResultFound: + with _session_for_read() as session: + # Note(masghar): The most recent node inventory is extracted + # (as per the created_at field). This is because previously, it was + # possible to add more than one inventory per node into the + # database, due to there being no unique constraint on the node_id + # column in the node_inventory table. Now, all previous node + # inventories are deleted before a new one is added using + # create_node_inventory. However, some databases would already + # contain multiple node inventories due to the prior + # implementation. Hence the most recent one is being retrieved. + query = session.query( + models.NodeInventory + ).filter_by( + node_id=node_id + ).order_by( + models.NodeInventory.created_at.desc() + ) + res = query.first() + + if res is None: raise exception.NodeInventoryNotFound(node=node_id) return res diff --git a/ironic/tests/unit/api/controllers/v1/test_node.py b/ironic/tests/unit/api/controllers/v1/test_node.py index c21fd42b0f..6513107792 100644 --- a/ironic/tests/unit/api/controllers/v1/test_node.py +++ b/ironic/tests/unit/api/controllers/v1/test_node.py @@ -7976,7 +7976,9 @@ class TestNodeHistory(test_api_base.BaseApiTest): class TestNodeInventory(test_api_base.BaseApiTest): - fake_inventory_data = {"cpu": "amd"} + fake_inventory_data = {'cpu': {'count': 1, + 'model_name': 'qemu64', + 'architecture': 'x86_64'}} fake_plugin_data = {"disks": [{"name": "/dev/vda"}]} def setUp(self): diff --git a/ironic/tests/unit/db/test_nodes.py b/ironic/tests/unit/db/test_nodes.py index e7053d6f58..2e4ada5b6f 100644 --- a/ironic/tests/unit/db/test_nodes.py +++ b/ironic/tests/unit/db/test_nodes.py @@ -24,6 +24,7 @@ from sqlalchemy.orm import exc as sa_exc from ironic.common import exception from ironic.common import states +from ironic.db.sqlalchemy.models import NodeInventory from ironic.tests.unit.db import base from ironic.tests.unit.db import utils @@ -763,6 +764,31 @@ class DbNodeTestCase(base.DbTestCase): self.assertRaises(exception.NodeHistoryNotFound, self.dbapi.get_node_history_by_id, history.id) + def test_inventory_updated_for_node(self): + node = utils.create_test_node() + + first_timestamp = datetime.datetime(2000, 1, 1, 0, 0) + second_timestamp = first_timestamp + datetime.timedelta(minutes=8) + utils.create_test_inventory(node_id=node.id, + id=1, + created_at=first_timestamp) + utils.create_test_inventory(node_id=node.id, + id=2, + inventory={"inventory": "test2"}, + created_at=second_timestamp) + + node_inventory = self.dbapi.get_node_inventory_by_node_id( + node_id=node.id) + expected_inventory = NodeInventory(node_id=node.id, + id=2, + inventory_data={"inventory": + "test2"}, + created_at=second_timestamp, + plugin_data={"pdata": + {"plugin": "data"}}, + version='1.0') + self.assertJsonEqual(expected_inventory, node_inventory) + def test_inventory_get_destroyed_after_destroying_a_node_by_uuid(self): node = utils.create_test_node() diff --git a/ironic/tests/unit/db/utils.py b/ironic/tests/unit/db/utils.py index d01c384996..4bcbbf8c26 100644 --- a/ironic/tests/unit/db/utils.py +++ b/ironic/tests/unit/db/utils.py @@ -743,7 +743,7 @@ def get_test_inventory(**kw): def create_test_inventory(**kw): """Create test inventory entry in DB and return NodeInventory DB object. - :param kw: kwargs with overriding values for port's attributes. + :param kw: kwargs with overriding values for inventory attributes. :returns: Test NodeInventory DB object. """ inventory = get_test_inventory(**kw) diff --git a/releasenotes/notes/handle-duplicate-node-inventories-fc41a7decbb6da76.yaml b/releasenotes/notes/handle-duplicate-node-inventories-fc41a7decbb6da76.yaml new file mode 100644 index 0000000000..6c0a29b85b --- /dev/null +++ b/releasenotes/notes/handle-duplicate-node-inventories-fc41a7decbb6da76.yaml @@ -0,0 +1,15 @@ +--- +fixes: + - | + Fixes a bug that occurs when a node is inspected more than once and the + database is configured as a storage backend: a new node inventory entry + is added in the database for each inspection result, causing more than one + inventory to exist for the node in the node_inventory table. + + This is handled by: + + * Deleting any previous inventory entries for a node before adding a new + entry in the database. + * Retrieving the most recent node inventory from the database when the + database is queried. (To cater for databases that already contain + duplicate node inventories due to the bug.)