From 78d6d95033fa8040157160d010377f571965f981 Mon Sep 17 00:00:00 2001 From: Dmitry Tantsur Date: Tue, 11 Dec 2018 18:17:30 +0100 Subject: [PATCH] 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 | 2 + doc/source/contributor/rolling-upgrades.rst | 7 ++- ironic/cmd/dbsync.py | 16 ++++-- ironic/db/api.py | 3 +- ironic/db/sqlalchemy/api.py | 59 ++++++++++++--------- ironic/tests/unit/cmd/test_dbsync.py | 12 ++++- ironic/tests/unit/db/test_api.py | 6 +++ 7 files changed, 72 insertions(+), 33 deletions(-) diff --git a/doc/source/contributor/releasing.rst b/doc/source/contributor/releasing.rst index 33c7c3d336..187a81468a 100644 --- a/doc/source/contributor/releasing.rst +++ b/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 `_. diff --git a/doc/source/contributor/rolling-upgrades.rst b/doc/source/contributor/rolling-upgrades.rst index 2c0895a01a..663b78cb05 100644 --- a/doc/source/contributor/rolling-upgrades.rst +++ b/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``. diff --git a/ironic/cmd/dbsync.py b/ironic/cmd/dbsync.py index 99badb4dfd..52012a2c31 100644 --- a/ironic/cmd/dbsync.py +++ b/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): diff --git a/ironic/db/api.py b/ironic/db/api.py index 0ca10c1db5..c98880c7f4 100644 --- a/ironic/db/api.py +++ b/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. """ diff --git a/ironic/db/sqlalchemy/api.py b/ironic/db/sqlalchemy/api.py index 8d32d63858..2c096f0028 100644 --- a/ironic/db/sqlalchemy/api.py +++ b/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 diff --git a/ironic/tests/unit/cmd/test_dbsync.py b/ironic/tests/unit/cmd/test_dbsync.py index 406215c1a8..335f95c5e5 100644 --- a/ironic/tests/unit/cmd/test_dbsync.py +++ b/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'),) diff --git a/ironic/tests/unit/db/test_api.py b/ironic/tests/unit/db/test_api.py index 555aba84b8..1d053a7d87 100644 --- a/ironic/tests/unit/db/test_api.py +++ b/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)