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

@@ -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

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='?')
 

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

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

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