Ignore newly introduced tables in pre-upgrade versions check

The version check is run before tables are created, so it cannot
succeed for them.

Change-Id: Ibcf0728bc5d1b0cbdd78796526f9c93fc99e8c08
Story: #2004589
Task: #28467

doc/source/contributor/releasing.rst

@@ -127,6 +127,8 @@ We need to submit patches for changes on master to:
     are used to migrate from an old release to this latest release; they
     shouldn't be needed after that.)
 
+  * remove any model class names from ``ironic.cmd.dbsync.NEW_MODELS``.
+
 As **ironic-tempest-plugin** is branchless, we need to submit a patch adding
 stable jobs to its master branch. `Example for Queens
 <https://review.openstack.org/#/c/543555/>`_.

doc/source/contributor/rolling-upgrades.rst

@@ -372,7 +372,9 @@ following needs to be considered:
 - Any change of fields or change in signature of remotable methods needs a bump
   of the object version. The object versions are also maintained in
   ``ironic/common/release_mappings.py``.
-- New objects must be added to ``ironic/common/release_mappings.py``.
+- New objects must be added to ``ironic/common/release_mappings.py``. Also for
+  the first releases they should be excluded from the version check by adding
+  their class names to the ``NEW_MODELS`` list in ``ironic/cmd/dbsync.py``.
 - The arguments of remotable methods (methods which are remoted to the
   conductor via RPC) can only be added as optional. They cannot be removed or
   changed in an incompatible way (to the previous release).
@@ -500,3 +502,6 @@ This check is done by comparing the objects' ``version`` field in the database
 with the expected (or supported) versions of these objects. The supported
 versions are the versions specified in
 ``ironic.common.release_mappings.RELEASE_MAPPING``.
+The newly created tables cannot pass this check and thus have to be excluded by
+adding their object class names (e.g. ``Node``) to
+``ironic.cmd.dbsync.NEW_MODELS``.

ironic/cmd/dbsync.py

@@ -78,10 +78,15 @@ ONLINE_MIGRATIONS = (
     (dbapi, 'update_to_latest_versions'),
 )
 
+# These are the models added in supported releases. We skip the version check
+# for them since the tables do not exist when it happens.
+NEW_MODELS = [
+]
+
 
 class DBCommand(object):
 
-    def _check_versions(self):
+    def _check_versions(self, ignore_missing_tables=False):
         """Check the versions of objects.
 
         Check that the object versions are compatible with this release
@@ -94,8 +99,13 @@ class DBCommand(object):
             # no tables, nothing to check
             return
 
+        if ignore_missing_tables:
+            ignore_models = NEW_MODELS
+        else:
+            ignore_models = ()
+
         try:
-            if not dbapi.check_versions():
+            if not dbapi.check_versions(ignore_models=ignore_models):
                 sys.stderr.write(
                     _('The database is not compatible with this '
                       'release of ironic (%s). Please run '
@@ -119,7 +129,7 @@ class DBCommand(object):
             sys.exit(2)
 
     def upgrade(self):
-        self._check_versions()
+        self._check_versions(ignore_missing_tables=True)
         migration.upgrade(CONF.command.revision)
 
     def revision(self):

ironic/db/api.py

@@ -902,13 +902,14 @@ class Connection(object):
         """
 
     @abc.abstractmethod
-    def check_versions(self):
+    def check_versions(self, ignore_models=()):
         """Checks the whole database for incompatible objects.
 
         This scans all the tables in search of objects that are not supported;
         i.e., those that are not specified in
         `ironic.common.release_mappings.RELEASE_MAPPING`.
 
+        :param ignore_models: List of model names to skip.
         :returns: A Boolean. True if all the objects have supported versions;
                   False otherwise.
         """

ironic/db/sqlalchemy/api.py

@@ -1198,7 +1198,7 @@ class Connection(api.Connection):
                     model.version.notin_(versions)))
         return query.all()
 
-    def check_versions(self):
+    def check_versions(self, ignore_models=()):
         """Checks the whole database for incompatible objects.
 
         This scans all the tables in search of objects that are not supported;
@@ -1206,38 +1206,45 @@ class Connection(api.Connection):
         `ironic.common.release_mappings.RELEASE_MAPPING`. This includes objects
         that have null 'version' values.
 
+        :param ignore_models: List of model names to skip.
         :returns: A Boolean. True if all the objects have supported versions;
                   False otherwise.
         """
         object_versions = release_mappings.get_object_versions()
         for model in models.Base.__subclasses__():
-            if model.__name__ in object_versions:
-                supported_versions = object_versions[model.__name__]
-                if not supported_versions:
-                    continue
+            if model.__name__ not in object_versions:
+                continue
 
-                # NOTE(mgagne): Additional safety check to detect old database
-                # version which does not have the 'version' columns available.
-                # This usually means a skip version upgrade is attempted
-                # from a version earlier than Pike which added
-                # those columns required for the next check.
-                engine = enginefacade.reader.get_engine()
-                if not db_utils.column_exists(engine,
-                                              model.__tablename__,
-                                              model.version.name):
-                    raise exception.DatabaseVersionTooOld()
+            if model.__name__ in ignore_models:
+                continue
+
+            supported_versions = object_versions[model.__name__]
+            if not supported_versions:
+                continue
+
+            # NOTE(mgagne): Additional safety check to detect old database
+            # version which does not have the 'version' columns available.
+            # This usually means a skip version upgrade is attempted
+            # from a version earlier than Pike which added
+            # those columns required for the next check.
+            engine = enginefacade.reader.get_engine()
+            if not db_utils.column_exists(engine,
+                                          model.__tablename__,
+                                          model.version.name):
+                raise exception.DatabaseVersionTooOld()
+
+            # 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.version).filter(
+                sql.or_(model.version == sql.null(),
+                        model.version.notin_(supported_versions)))
+            if query.count():
+                return False
 
-                # 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.version).filter(
-                    sql.or_(model.version == sql.null(),
-                            model.version.notin_(supported_versions)))
-                if query.count():
-                    return False
         return True
 
     @oslo_db_api.retry_on_deadlock

ironic/tests/unit/cmd/test_dbsync.py

@@ -41,16 +41,24 @@ class OnlineMigrationTestCase(db_base.DbTestCase):
                                autospec=True) as mock_check_versions:
             mock_check_versions.return_value = True
             self.db_cmds._check_versions()
-            mock_check_versions.assert_called_once_with()
+            mock_check_versions.assert_called_once_with(ignore_models=())
 
     def test__check_versions_bad(self):
         with mock.patch.object(self.dbapi, 'check_versions',
                                autospec=True) as mock_check_versions:
             mock_check_versions.return_value = False
             exit = self.assertRaises(SystemExit, self.db_cmds._check_versions)
-            mock_check_versions.assert_called_once_with()
+            mock_check_versions.assert_called_once_with(ignore_models=())
             self.assertEqual(2, exit.code)
 
+    def test__check_versions_ignore_models(self):
+        with mock.patch.object(self.dbapi, 'check_versions',
+                               autospec=True) as mock_check_versions:
+            mock_check_versions.return_value = True
+            self.db_cmds._check_versions(True)
+            mock_check_versions.assert_called_once_with(
+                ignore_models=dbsync.NEW_MODELS)
+
     @mock.patch.object(dbsync, 'ONLINE_MIGRATIONS', autospec=True)
     def test__run_migration_functions(self, mock_migrations):
         mock_migrations.__iter__.return_value = ((self.dbapi, 'foo'),)

ironic/tests/unit/db/test_api.py

@@ -56,6 +56,12 @@ class UpgradingTestCase(base.DbTestCase):
         self.assertIsNone(node.version)
         self.assertFalse(self.dbapi.check_versions())
 
+    def test_check_versions_ignore_node(self):
+        node = utils.create_test_node(version=None)
+        node = self.dbapi.get_node_by_id(node.id)
+        self.assertIsNone(node.version)
+        self.assertTrue(self.dbapi.check_versions(ignore_models=['Node']))
+
     def test_check_versions_node_old(self):
         node = utils.create_test_node(version='1.0')
         node = self.dbapi.get_node_by_id(node.id)