Merge "Handle duplicate node inventory entries per node"

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
 

ironic/tests/unit/api/controllers/v1/test_node.py

@@ -8025,7 +8025,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):

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()
 

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)

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.)