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