Fix online upgrades for Bios/Traits

... And tags, but nobody uses tags since it is not available
via the API.

Anyhow, the online upgrade code was written under the assumption
that *all* tables had an "id" column. This is not always true
in the ironic data model for tables which started as pure extensions
of the Nodes table, and fails in particular when:

1) A database row has data stored in an ealier version of the object
2) That same object gets a version upgrade.

In the case which discovered this, BIOSSetting was added at version
1.0, and later updated to include additional fields which incremented
the version to 1.1. When the upgrade went to evaluate and iterate
through the fields, the command failed because the table was designed
around "node_id" instead of "id".

Story: 2010632
Task: 47590

Change-Id: I7bec6cfacb9d1558bc514c07386583436759f4df
(cherry picked from commit cbe5f86ce7c100f834701cf43f7b9b1bfdfb9ea5)

3 files changed, 63 insertions, 13 deletions

diff --git a/ironic/db/sqlalchemy/api.py b/ironic/db/sqlalchemy/api.py
index c14719af8..2618f106c 100644
--- a/ironic/db/sqlalchemy/api.py
+++ b/ironic/db/sqlalchemy/api.py
@@ -1716,9 +1716,12 @@ class Connection(api.Connection):
         max_to_migrate = max_count or total_to_migrate
 
         for model in sql_models:
+            use_node_id = False
+            if (not hasattr(model, 'id') and hasattr(model, 'node_id')):
+                use_node_id = True
             version = mapping[model.__name__][0]
             num_migrated = 0
-            with _session_for_write():
+            with _session_for_write() as session:
                 query = model_query(model).filter(model.version != version)
                 # NOTE(rloo) Caution here; after doing query.count(), it is
                 #            possible that the value is different in the
@@ -1729,13 +1732,27 @@ class Connection(api.Connection):
                     # max_to_migrate objects.
                     ids = []
                     for obj in query.slice(0, max_to_migrate):
-                        ids.append(obj['id'])
-                    num_migrated = (
-                        model_query(model).
-                        filter(sql.and_(model.id.in_(ids),
-                                        model.version != version)).
-                        update({model.version: version},
-                               synchronize_session=False))
+                        if not use_node_id:
+                            ids.append(obj['id'])
+                        else:
+                            # BIOSSettings, NodeTrait, NodeTag do not have id
+                            # columns, fallback to node_id as they both have
+                            # it.
+                            ids.append(obj['node_id'])
+                    if not use_node_id:
+                        num_migrated = (
+                            session.query(model).
+                            filter(sql.and_(model.id.in_(ids),
+                                            model.version != version)).
+                            update({model.version: version},
+                                   synchronize_session=False))
+                    else:
+                        num_migrated = (
+                            session.query(model).
+                            filter(sql.and_(model.node_id.in_(ids),
+                                            model.version != version)).
+                            update({model.version: version},
+                                   synchronize_session=False))
                 else:
                     num_migrated = (
                         model_query(model).
diff --git a/ironic/tests/unit/db/test_api.py b/ironic/tests/unit/db/test_api.py
index 6142fdfae..2396b1253 100644
--- a/ironic/tests/unit/db/test_api.py
+++ b/ironic/tests/unit/db/test_api.py
@@ -226,6 +226,11 @@ class UpdateToLatestVersionsTestCase(base.DbTestCase):
         for i in range(0, num_nodes):
             node = utils.create_test_node(version=version,
                                           uuid=uuidutils.generate_uuid())
+            # Create entries on the tables so we force field upgrades
+            utils.create_test_node_trait(node_id=node.id, trait='foo',
+                                         version='0.0')
+            utils.create_test_bios_setting(node_id=node.id, version='1.0')
+
             nodes.append(node.uuid)
         for uuid in nodes:
             node = self.dbapi.get_node_by_uuid(uuid)
@@ -238,10 +243,15 @@ class UpdateToLatestVersionsTestCase(base.DbTestCase):
             return
 
         nodes = self._create_nodes(5)
+        # Check/migrate 2, 10 remain.
+        self.assertEqual(
+            (10, 2), self.dbapi.update_to_latest_versions(self.context, 2))
+        # Check/migrate 10, 8 migrated, 8 remain.
         self.assertEqual(
-            (5, 2), self.dbapi.update_to_latest_versions(self.context, 2))
+            (8, 8), self.dbapi.update_to_latest_versions(self.context, 10))
+        # Just make sure it is still 0, 0 in case more things are added.
         self.assertEqual(
-            (3, 3), self.dbapi.update_to_latest_versions(self.context, 10))
+            (0, 0), self.dbapi.update_to_latest_versions(self.context, 10))
         for uuid in nodes:
             node = self.dbapi.get_node_by_uuid(uuid)
             self.assertEqual(self.node_ver, node.version)
@@ -250,10 +260,19 @@ class UpdateToLatestVersionsTestCase(base.DbTestCase):
         if self.node_version_same:
             # can't test if we don't have diff versions of the node
             return
-
-        nodes = self._create_nodes(5)
+        vm_count = 5
+        nodes = self._create_nodes(vm_count)
+        # NOTE(TheJulia): Under current testing, 5 node will result in 10
+        # records implicitly needing to be migrated.
+        migrate_count = vm_count * 2
+        self.assertEqual(
+            (migrate_count, migrate_count),
+            self.dbapi.update_to_latest_versions(self.context,
+                                                 migrate_count))
         self.assertEqual(
-            (5, 5), self.dbapi.update_to_latest_versions(self.context, 5))
+            (0, 0), self.dbapi.update_to_latest_versions(self.context,
+                                                         migrate_count))
+
         for uuid in nodes:
             node = self.dbapi.get_node_by_uuid(uuid)
             self.assertEqual(self.node_ver, node.version)
diff --git a/releasenotes/notes/fix-online-version-migration-db432a7b239647fa.yaml b/releasenotes/notes/fix-online-version-migration-db432a7b239647fa.yaml
new file mode 100644
index 000000000..824185aab
--- /dev/null
+++ b/releasenotes/notes/fix-online-version-migration-db432a7b239647fa.yaml
@@ -0,0 +1,14 @@
+---
+fixes:
+  - |
+    Fixes an issue in the online upgrade logic where database models for
+    Node Traits and BIOS Settings resulted in an error when performing
+    the online data migration. This was because these tables were originally
+    created as extensions of the Nodes database table, and the schema
+    of the database was slightly different enough to result in an error
+    if there was data to migrate in these tables upon upgrade,
+    which would have occured if an early BIOS Setting adopter had
+    data in the database prior to upgrading to the Yoga release of Ironic.
+
+    The online upgrade parameter now subsitutes an alternate primary key name
+    name when applicable.