Merge "ironic-dbsync: check object versions"

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.
 
-..
+Before this ``upgrade`` is invoked, the command
-  TODO(rloo): add this in Queens; doesn't make sense to add in Pike
+:command:`ironic-dbsync online_data_migrations` must have been successfully run
-  Before this ``upgrade`` is invoked, the command
+using the previous version of ironic (if you are doing an upgrade as opposed to
-  `ironic db-sync online_data_migrations` must have been successfully run using
+a new installation of ironic). If it wasn't run, the database will not be
-  the previous version of ironic (if you are doing an upgrade as opposed to a
+compatible with this recent version of ironic, and this command will return
-  new installation of ironic). If it wasn't run, the database will not be
+2 (error).
-  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
+        self._check_versions()
-        # 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()
         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.
+        self._check_versions()
-        # 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._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``.