From 33383c9c73955f4a41b0ad31f80dcee1f797773d Mon Sep 17 00:00:00 2001 From: Ruby Loo Date: Tue, 12 Feb 2019 22:37:36 +0000 Subject: [PATCH] Add check for object versions Adds a check for compatibility of the object versions (in the DB) with the release of ironic. This check is used by the 'ironic-status upgrade check' command. The ironic-status command is added to devstack and grenade (when upgrading). Change-Id: I2043dc01856106d50356637db327a2817db90366 Story: 2004990 Task: 29459 --- devstack/lib/ironic | 4 ++ devstack/upgrade/upgrade.sh | 5 ++ doc/source/cli/ironic-status.rst | 3 +- ironic/cmd/dbsync.py | 49 +++++++++++-------- ironic/cmd/status.py | 33 ++++++++----- ironic/tests/unit/cmd/test_dbsync.py | 24 ++++++--- ironic/tests/unit/cmd/test_status.py | 17 +++++-- .../check_obj_versions-e86d897df673e833.yaml | 5 ++ 8 files changed, 96 insertions(+), 44 deletions(-) create mode 100644 releasenotes/notes/check_obj_versions-e86d897df673e833.yaml diff --git a/devstack/lib/ironic b/devstack/lib/ironic index e10ef7e714..46250f1252 100644 --- a/devstack/lib/ironic +++ b/devstack/lib/ironic @@ -1417,6 +1417,10 @@ function init_ironic { $IRONIC_BIN_DIR/ironic-dbsync --config-file=$IRONIC_CONF_FILE fi create_ironic_cache_dir + + # NOTE(rloo): We're not upgrading but want to make sure this command works, + # even though we're not parsing the output of this command. + $IRONIC_BIN_DIR/ironic-status upgrade check } # _ironic_bm_vm_names() - Generates list of names for baremetal VMs. diff --git a/devstack/upgrade/upgrade.sh b/devstack/upgrade/upgrade.sh index f4cb426e17..eee38cac7b 100755 --- a/devstack/upgrade/upgrade.sh +++ b/devstack/upgrade/upgrade.sh @@ -71,6 +71,11 @@ stack_install_service ironic # calls upgrade-ironic for specific release upgrade_project ironic $RUN_DIR $BASE_DEVSTACK_BRANCH $TARGET_DEVSTACK_BRANCH +# NOTE(rloo): make sure it is OK to do an upgrade. Except that we aren't +# parsing/checking the output of this command because the output could change +# based on the checks it makes. +$IRONIC_BIN_DIR/ironic-status upgrade check + $IRONIC_BIN_DIR/ironic-dbsync --config-file=$IRONIC_CONF_FILE # NOTE(vsaienko) pin_release only on multinode job, for cold upgrade (single node) diff --git a/doc/source/cli/ironic-status.rst b/doc/source/cli/ironic-status.rst index ea7c2b2aed..877518492a 100644 --- a/doc/source/cli/ironic-status.rst +++ b/doc/source/cli/ironic-status.rst @@ -75,4 +75,5 @@ Upgrade **12.0.0 (Stein)** - * Placeholder to be filled in with checks as they are added in Stein. + * Adds a check for compatibility of the object versions with the release + of ironic. diff --git a/ironic/cmd/dbsync.py b/ironic/cmd/dbsync.py index f554cbdfc1..b140028791 100644 --- a/ironic/cmd/dbsync.py +++ b/ironic/cmd/dbsync.py @@ -88,14 +88,14 @@ NEW_MODELS = [ class DBCommand(object): - def _check_versions(self, ignore_missing_tables=False): + def check_obj_versions(self, ignore_missing_tables=False): """Check the versions of objects. Check that the object versions are compatible with this release of ironic. It does this by comparing the objects' .version field in the database, with the expected versions of these objects. - If it isn't compatible, we exit the program, returning 2. + Returns None if compatible; a string describing the issue otherwise. """ if migration.version() is None: # no tables, nothing to check @@ -106,28 +106,35 @@ class DBCommand(object): else: ignore_models = () + msg = None try: 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 ' - '"ironic-dbsync online_data_migrations" using ' - 'the previous release.\n') - % version.version_info.release_string()) - # NOTE(rloo): We return 1 in online_data_migrations() to - # indicate that there are more objects to migrate, - # so don't use 1 here. - sys.exit(2) + msg = (_('The database is not compatible with this ' + 'release of ironic (%s). Please run ' + '"ironic-dbsync online_data_migrations" using ' + 'the previous release.\n') + % version.version_info.release_string()) except exception.DatabaseVersionTooOld: - sys.stderr.write( - _('The database version is not compatible with this ' - 'release of ironic (%s). This can happen if you are ' - 'attempting to upgrade from a version older than ' - 'the previous release (skip versions upgrade). ' - 'This is an unsupported upgrade method. ' - 'Please run "ironic-dbsync upgrade" using the previous ' - 'releases for a fast-forward upgrade.\n') - % version.version_info.release_string()) + msg = (_('The database version is not compatible with this ' + 'release of ironic (%s). This can happen if you are ' + 'attempting to upgrade from a version older than ' + 'the previous release (skip versions upgrade). ' + 'This is an unsupported upgrade method. ' + 'Please run "ironic-dbsync upgrade" using the previous ' + 'releases for a fast-forward upgrade.\n') + % version.version_info.release_string()) + + return msg + + def _check_versions(self, ignore_missing_tables=False): + msg = self.check_obj_versions( + ignore_missing_tables=ignore_missing_tables) + if not msg: + return + else: + sys.stderr.write(msg) + # NOTE(rloo): We return 1 in online_data_migrations() to indicate + # that there are more objects to migrate, so don't use 1 here. sys.exit(2) def upgrade(self): diff --git a/ironic/cmd/status.py b/ironic/cmd/status.py index c1366bb4c8..a1a1b5502c 100644 --- a/ironic/cmd/status.py +++ b/ironic/cmd/status.py @@ -17,6 +17,7 @@ import sys from oslo_config import cfg from oslo_upgradecheck import upgradecheck +from ironic.cmd import dbsync from ironic.common.i18n import _ @@ -28,21 +29,31 @@ class Checks(upgradecheck.UpgradeCommands): and added to _upgrade_checks tuple. """ - def _check_placeholder(self): - # This is just a placeholder for upgrade checks, it should be - # removed when the actual checks are added - return upgradecheck.Result(upgradecheck.Code.SUCCESS) + def _check_obj_versions(self): + """Check that the DB versions of objects are compatible. - # The format of the check functions is to return an - # oslo_upgradecheck.upgradecheck.Result - # object with the appropriate - # oslo_upgradecheck.upgradecheck.Code and details set. - # If the check hits warnings or failures then those should be stored + Checks that the object versions are compatible with this + release of ironic. It does this by comparing the objects' + .version field in the database, with the expected versions + of these objects. + """ + msg = dbsync.DBCommand().check_obj_versions(ignore_missing_tables=True) + + if not msg: + return upgradecheck.Result(upgradecheck.Code.SUCCESS) + else: + return upgradecheck.Result(upgradecheck.Code.FAILURE, details=msg) + + # A tuple of check tuples of (, ). + # The name of the check will be used in the output of this command. + # The check function takes no arguments and returns an + # oslo_upgradecheck.upgradecheck.Result object with the appropriate + # oslo_upgradecheck.upgradecheck.Code and details set. If the + # check function hits warnings or failures then those should be stored # in the returned Result's "details" attribute. The # summary will be rolled up at the end of the check() method. _upgrade_checks = ( - # In the future there should be some real checks added here - (_('Placeholder'), _check_placeholder), + (_('Object versions'), _check_obj_versions), ) diff --git a/ironic/tests/unit/cmd/test_dbsync.py b/ironic/tests/unit/cmd/test_dbsync.py index 335f95c5e5..f1f9e05b93 100644 --- a/ironic/tests/unit/cmd/test_dbsync.py +++ b/ironic/tests/unit/cmd/test_dbsync.py @@ -36,29 +36,39 @@ class OnlineMigrationTestCase(db_base.DbTestCase): self.context = context.get_admin_context() self.db_cmds = dbsync.DBCommand() - def test__check_versions(self): + def test_check_obj_versions(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() + msg = self.db_cmds.check_obj_versions() + self.assertIsNone(msg) mock_check_versions.assert_called_once_with(ignore_models=()) - def test__check_versions_bad(self): + def test_check_obj_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) + msg = self.db_cmds.check_obj_versions() + self.assertIsNotNone(msg) mock_check_versions.assert_called_once_with(ignore_models=()) - self.assertEqual(2, exit.code) - def test__check_versions_ignore_models(self): + def test_check_obj_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) + msg = self.db_cmds.check_obj_versions(ignore_missing_tables=True) + self.assertIsNone(msg) mock_check_versions.assert_called_once_with( ignore_models=dbsync.NEW_MODELS) + @mock.patch.object(dbsync.DBCommand, 'check_obj_versions', autospec=True) + def test_check_versions_bad(self, mock_check_versions): + mock_check_versions.return_value = 'This is bad' + exit = self.assertRaises(SystemExit, self.db_cmds._check_versions) + mock_check_versions.assert_called_once_with( + mock.ANY, ignore_missing_tables=False) + self.assertEqual(2, exit.code) + @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/cmd/test_status.py b/ironic/tests/unit/cmd/test_status.py index 49ce00b114..aa4a5258bb 100644 --- a/ironic/tests/unit/cmd/test_status.py +++ b/ironic/tests/unit/cmd/test_status.py @@ -12,8 +12,10 @@ # License for the specific language governing permissions and limitations # under the License. +import mock from oslo_upgradecheck.upgradecheck import Code +from ironic.cmd import dbsync from ironic.cmd import status from ironic.tests.unit.db import base as db_base @@ -24,7 +26,14 @@ class TestUpgradeChecks(db_base.DbTestCase): super(TestUpgradeChecks, self).setUp() self.cmd = status.Checks() - def test__check_placeholder(self): - check_result = self.cmd._check_placeholder() - self.assertEqual( - Code.SUCCESS, check_result.code) + def test__check_obj_versions(self): + check_result = self.cmd._check_obj_versions() + self.assertEqual(Code.SUCCESS, check_result.code) + + @mock.patch.object(dbsync.DBCommand, 'check_obj_versions', autospec=True) + def test__check_obj_versions_bad(self, mock_check): + msg = 'This is bad' + mock_check.return_value = msg + check_result = self.cmd._check_obj_versions() + self.assertEqual(Code.FAILURE, check_result.code) + self.assertEqual(msg, check_result.details) diff --git a/releasenotes/notes/check_obj_versions-e86d897df673e833.yaml b/releasenotes/notes/check_obj_versions-e86d897df673e833.yaml new file mode 100644 index 0000000000..b08bf2fb50 --- /dev/null +++ b/releasenotes/notes/check_obj_versions-e86d897df673e833.yaml @@ -0,0 +1,5 @@ +--- +upgrade: + - | + Adds a check to the ``ironic-status upgrade check`` command, to check for + compatibility of the object versions with the release of ironic.