From 6a91b38ca48eea2892a5a82b4369f1f9b277d019 Mon Sep 17 00:00:00 2001
From: Ruby Loo <ruby.loo@intel.com>
Date: Thu, 24 Aug 2017 20:56:22 -0400
Subject: [PATCH] ironic-dbsync: check object versions
Now that we have rolling upgrades and the version column was
added and populated in the Pike release, we can add checks to
make sure the versions of objects in the DB are compatible
with this ironic release, before ironic-dbsync's upgrade or
online_data_migrations does its work. These ironic-dbsync
calls are made as part of upgrading to this (Queens) release.
Change-Id: I68758f8a29d483f5c0a7439fa2ea2962b2eb4124
Partial-Bug: #1708243
---
doc/source/cli/ironic-dbsync.rst | 14 ++++----
ironic/cmd/dbsync.py | 33 +++++++------------
ironic/db/sqlalchemy/api.py | 18 +++++++++-
ironic/tests/unit/db/test_api.py | 21 ++++++++++++
...dbsync-check-version-c71d5f4fd89ed117.yaml | 11 +++++++
5 files changed, 67 insertions(+), 30 deletions(-)
create mode 100644 releasenotes/notes/dbsync-check-version-c71d5f4fd89ed117.yaml
diff --git a/doc/source/cli/ironic-dbsync.rst b/doc/source/cli/ironic-dbsync.rst
index e30f8b351b..bba2633d8c 100644
--- a/doc/source/cli/ironic-dbsync.rst
+++ b/doc/source/cli/ironic-dbsync.rst
@@ -188,14 +188,12 @@ upgrade
This command will upgrade existing database tables to the most recent version,
or to the version specified with the :option:`--revision` option.
-..
- TODO(rloo): add this in Queens; doesn't make sense to add in Pike
- Before this ``upgrade`` is invoked, the command
- `ironic db-sync online_data_migrations` must have been successfully run using
- the previous version of ironic (if you are doing an upgrade as opposed to a
- new installation of ironic). If it wasn't run, the database will not be
- compatible with this recent version of ironic, and this command will return
- 2 (error).
+Before this ``upgrade`` is invoked, the command
+:command:`ironic-dbsync online_data_migrations` must have been successfully run
+using the previous version of ironic (if you are doing an upgrade as opposed to
+a new installation of ironic). If it wasn't run, the database will not be
+compatible with this recent version of ironic, and this command will return
+2 (error).
If there are no existing tables, then new tables are created, beginning
with the oldest known version, and successively upgraded using all of the
diff --git a/ironic/cmd/dbsync.py b/ironic/cmd/dbsync.py
index 93a1ffd467..6c34bee219 100644
--- a/ironic/cmd/dbsync.py
+++ b/ironic/cmd/dbsync.py
@@ -75,6 +75,10 @@ class DBCommand(object):
If it isn't compatible, we exit the program, returning 2.
"""
+ if migration.version() is None:
+ # no tables, nothing to check
+ return
+
if not dbapi.check_versions():
sys.stderr.write(_('The database is not compatible with this '
'release of ironic (%s). Please run '
@@ -86,13 +90,7 @@ class DBCommand(object):
sys.exit(2)
def upgrade(self):
- # TODO(rloo): enable this in Queens because we want the check done
- # before someone tries to upgrade to Queens.
- # It won't work now because the check looks at the value in the new
- # 'version' column for all objects. And this column doesn't exist until
- # *after* an upgrade to this Pike release (i.e., after this
- # 'ironic-dbsync upgrade' is run).
- # self._check_versions()
+ self._check_versions()
migration.upgrade(CONF.command.revision)
def revision(self):
@@ -108,12 +106,7 @@ class DBCommand(object):
migration.create_schema()
def online_data_migrations(self):
- # TODO(rloo): enable this in Queens.
- # It won't work now because the check looks at the value in the new
- # 'version' column for all objects, which cannot be null/None. In Pike,
- # only after running this 'ironic-dbsync online_data_migrations'
- # command, will the version column be populated.
- # self._check_versions()
+ self._check_versions()
self._run_online_data_migrations(max_count=CONF.command.max_count)
def _run_migration_functions(self, context, max_count):
@@ -217,16 +210,14 @@ def add_command_parsers(subparsers):
parser = subparsers.add_parser(
'upgrade',
- # TODO(rloo): Add this to the help string in Queens (because we need
- # to wait until online_data_migrations exists in older release first):
- # It returns 2 (error) if the database is "
- # "not compatible with this version. If this happens, the "
- # "'ironic-dbsync online_data_migrations' command should be run "
- # "using the previous version of ironic, before upgrading and "
- # "running this command."))
help=_("Upgrade the database schema to the latest version. "
"Optionally, use --revision to specify an alembic revision "
- "string to upgrade to."))
+ "string to upgrade to. It returns 2 (error) if the database is "
+ "not compatible with this version. If this happens, the "
+ "'ironic-dbsync online_data_migrations' command should be run "
+ "using the previous version of ironic, before upgrading and "
+ "running this command."))
+
parser.set_defaults(func=command_object.upgrade)
parser.add_argument('--revision', nargs='?')
diff --git a/ironic/db/sqlalchemy/api.py b/ironic/db/sqlalchemy/api.py
index 0b963cac63..122dd363fa 100644
--- a/ironic/db/sqlalchemy/api.py
+++ b/ironic/db/sqlalchemy/api.py
@@ -1149,9 +1149,25 @@ class Connection(api.Connection):
supported_versions = object_versions[model.__name__]
if not supported_versions:
continue
+
+ # TODO(rloo). Because we forgot to set the version of
+ # conductors in Pike, we allow the Conductor version to
+ # be null now (Queens). In Rocky, it cannot be null,
+ # and this 'if...' can be deleted then.
+ if model.__name__ == 'Conductor':
+ query = model_query(model.version).filter(
+ model.version.notin_(supported_versions))
+ if query.count():
+ return False
+ continue
+
+ # NOTE(rloo): we use model.version, not model, because we
+ # know that the object has a 'version' column
+ # but we don't know whether the entire object is
+ # compatible with its (old) DB representation.
# NOTE(rloo): .notin_ does not handle null:
# http://docs.sqlalchemy.org/en/latest/core/sqlelement.html#sqlalchemy.sql.operators.ColumnOperators.notin_
- query = model_query(model).filter(
+ query = model_query(model.version).filter(
sql.or_(model.version == sql.null(),
model.version.notin_(supported_versions)))
if query.count():
diff --git a/ironic/tests/unit/db/test_api.py b/ironic/tests/unit/db/test_api.py
index 86b2002343..19cf9f70d8 100644
--- a/ironic/tests/unit/db/test_api.py
+++ b/ironic/tests/unit/db/test_api.py
@@ -50,6 +50,27 @@ class UpgradingTestCase(base.DbTestCase):
self.assertEqual('1.0', node.version)
self.assertFalse(self.dbapi.check_versions())
+ def test_check_versions_conductor(self):
+ for v in self.object_versions['Conductor']:
+ conductor = utils.create_test_conductor(
+ uuid=uuidutils.generate_uuid(), version=v)
+ conductor = self.dbapi.get_conductor(conductor.hostname)
+ self.assertEqual(v, conductor.version)
+ self.assertTrue(self.dbapi.check_versions())
+
+ def test_check_versions_conductor_no_version(self):
+ # works in Queens only
+ conductor = utils.create_test_conductor(version=None)
+ conductor = self.dbapi.get_conductor(conductor.hostname)
+ self.assertIsNone(conductor.version)
+ self.assertTrue(self.dbapi.check_versions())
+
+ def test_check_versions_conductor_old(self):
+ conductor = utils.create_test_conductor(version='1.0')
+ conductor = self.dbapi.get_conductor(conductor.hostname)
+ self.assertEqual('1.0', conductor.version)
+ self.assertFalse(self.dbapi.check_versions())
+
class BackfillVersionTestCase(base.DbTestCase):
diff --git a/releasenotes/notes/dbsync-check-version-c71d5f4fd89ed117.yaml b/releasenotes/notes/dbsync-check-version-c71d5f4fd89ed117.yaml
new file mode 100644
index 0000000000..65ce108d95
--- /dev/null
+++ b/releasenotes/notes/dbsync-check-version-c71d5f4fd89ed117.yaml
@@ -0,0 +1,11 @@
+---
+upgrade:
+ - |
+ The ``ironic-dbsync`` command will check the object versions to make
+ sure they are compatible with the new ironic release, before doing the
+ ``upgrade`` or ``online_data_migrations``.
+other:
+ - |
+ The ``ironic-dbsync`` command will check the object versions to make
+ sure they are compatible with the new ironic release, before doing the
+ ``upgrade`` or ``online_data_migrations``.