From cbe5f86ce7c100f834701cf43f7b9b1bfdfb9ea5 Mon Sep 17 00:00:00 2001 From: Julia Kreger Date: Mon, 6 Mar 2023 10:48:07 -0800 Subject: [PATCH] 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 --- ironic/db/sqlalchemy/api.py | 31 ++++++++++++++----- ironic/tests/unit/db/test_api.py | 29 ++++++++++++++--- ...ne-version-migration-db432a7b239647fa.yaml | 14 +++++++++ 3 files changed, 62 insertions(+), 12 deletions(-) create mode 100644 releasenotes/notes/fix-online-version-migration-db432a7b239647fa.yaml diff --git a/ironic/db/sqlalchemy/api.py b/ironic/db/sqlalchemy/api.py index d5f4a9d650..93a211fc3c 100644 --- a/ironic/db/sqlalchemy/api.py +++ b/ironic/db/sqlalchemy/api.py @@ -1834,6 +1834,9 @@ 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() as session: @@ -1847,13 +1850,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 = ( - session.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 = ( session.query(model). diff --git a/ironic/tests/unit/db/test_api.py b/ironic/tests/unit/db/test_api.py index 6142fdfae6..2396b12532 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( - (5, 2), self.dbapi.update_to_latest_versions(self.context, 2)) + (10, 2), self.dbapi.update_to_latest_versions(self.context, 2)) + # Check/migrate 10, 8 migrated, 8 remain. self.assertEqual( - (3, 3), self.dbapi.update_to_latest_versions(self.context, 10)) + (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( + (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( - (5, 5), self.dbapi.update_to_latest_versions(self.context, 5)) + (migrate_count, migrate_count), + self.dbapi.update_to_latest_versions(self.context, + migrate_count)) + self.assertEqual( + (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 0000000000..824185aab5 --- /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.