From 6bab551cd8a523332b7c387c36c701cb90fd96bd Mon Sep 17 00:00:00 2001 From: Richard Avelar Date: Thu, 23 Feb 2017 15:35:21 +0000 Subject: [PATCH] Validate rolling upgrade is run in order This patch addresses a bug that allows rolling upgrades to be run out of order and without first checking if the previous command has been run to a higher version before hand. Change-Id: I55fa4f600d89f3a2fb14868f6886b52fd1ef6c6b Closes-Bug: 1615014 --- keystone/common/sql/upgrades.py | 56 +++++++++++++++ keystone/tests/unit/test_sql_upgrade.py | 72 +++++++++++++------ .../notes/bug-1615014-b30f606a2d202428.yaml | 7 ++ 3 files changed, 112 insertions(+), 23 deletions(-) create mode 100644 releasenotes/notes/bug-1615014-b30f606a2d202428.yaml diff --git a/keystone/common/sql/upgrades.py b/keystone/common/sql/upgrades.py index 86d1c0a33f..4e668ef4fe 100644 --- a/keystone/common/sql/upgrades.py +++ b/keystone/common/sql/upgrades.py @@ -19,6 +19,7 @@ import os import migrate from migrate import exceptions from migrate.versioning import api as versioning_api +from oslo_db import exception as db_exception from oslo_db.sqlalchemy import migration import six import sqlalchemy @@ -53,6 +54,7 @@ class Repository(object): upgrade = True version = versioning_api._migrate_version( self.schema_, version, upgrade, err) + validate_upgrade_order(self.repo_name, target_repo_version=version) if not current_schema: current_schema = self.schema_ changeset = current_schema.changeset(version) @@ -256,6 +258,57 @@ def get_db_version(repo=LEGACY_REPO): session.get_bind(), find_repo(repo), get_init_version()) +def validate_upgrade_order(repo_name, target_repo_version=None): + """Validate the state of the migration repositories. + + This is run before allowing the db_sync command to execute. Ensure the + upgrade step and version specified by the operator remains consistent with + the upgrade process. I.e. expand's version is greater or equal to + migrate's, migrate's version is greater or equal to contract's. + + :param repo_name: The name of the repository that the user is trying to + upgrade. + :param target_repo_version: The version to upgrade the repo. Otherwise, the + version will be upgraded to the latest version + available. + """ + # Initialize a dict to have each key assigned a repo with their value being + # the repo that comes before. + db_sync_order = {DATA_MIGRATION_REPO: EXPAND_REPO, + CONTRACT_REPO: DATA_MIGRATION_REPO} + + if repo_name == LEGACY_REPO: + return + # If expand is being run, we validate that Legacy repo is at the maximum + # version before running the additional schema expansions. + elif repo_name == EXPAND_REPO: + abs_path = find_repo(LEGACY_REPO) + repo = migrate.versioning.repository.Repository(abs_path) + if int(repo.latest) != get_db_version(): + raise db_exception.DBMigrationError( + 'Your Legacy repo version is not up to date. Please refer to ' + 'https://docs.openstack.org/developer/keystone/upgrading.html ' + 'to see the proper steps for rolling upgrades.') + return + + # find the latest version that the current command will upgrade to if there + # wasn't a version specified for upgrade. + if not target_repo_version: + abs_path = find_repo(repo_name) + repo = migrate.versioning.repository.Repository(abs_path) + target_repo_version = int(repo.latest) + + # get current version of the command that runs before the current command. + dependency_repo_version = get_db_version(repo=db_sync_order[repo_name]) + + if dependency_repo_version < target_repo_version: + raise db_exception.DBMigrationError( + 'You are attempting to upgrade %s ahead of %s. Please refer to ' + 'https://docs.openstack.org/developer/keystone/upgrading.html ' + 'to see the proper steps for rolling upgrades.' % ( + repo_name, db_sync_order[repo_name])) + + def expand_schema(): """Expand the database schema ahead of data migration. @@ -266,6 +319,7 @@ def expand_schema(): # Make sure all the legacy migrations are run before we run any new # expand migrations. _sync_common_repo(version=None) + validate_upgrade_order(EXPAND_REPO) _sync_repo(repo_name=EXPAND_REPO) @@ -276,6 +330,7 @@ def migrate_data(): schema has been expanded for the new release. """ + validate_upgrade_order(DATA_MIGRATION_REPO) _sync_repo(repo_name=DATA_MIGRATION_REPO) @@ -287,4 +342,5 @@ def contract_schema(): tables/columns that are no longer required. """ + validate_upgrade_order(CONTRACT_REPO) _sync_repo(repo_name=CONTRACT_REPO) diff --git a/keystone/tests/unit/test_sql_upgrade.py b/keystone/tests/unit/test_sql_upgrade.py index 61d0364958..bfa6dfb937 100644 --- a/keystone/tests/unit/test_sql_upgrade.py +++ b/keystone/tests/unit/test_sql_upgrade.py @@ -1683,6 +1683,50 @@ class VersionTests(SqlMigrateBase): self.assertRegexpMatches(file_name, pattern, msg) +class MigrationValidation(SqlMigrateBase, unit.TestCase): + """Test validation of database between database phases.""" + + def setUp(self): + super(MigrationValidation, self).setUp() + + def _set_db_sync_command_versions(self): + self.expand(1) + self.migrate(1) + self.contract(1) + self.assertEqual(upgrades.get_db_version('expand_repo'), 1) + self.assertEqual(upgrades.get_db_version('data_migration_repo'), 1) + self.assertEqual(upgrades.get_db_version('contract_repo'), 1) + + def test_running_db_sync_expand_without_up_to_date_legacy_fails(self): + # Set Legacy version and then test that running expand fails if Legacy + # isn't at the latest version. + self.upgrade(67) + latest_version = self.repos[EXPAND_REPO].max_version + self.assertRaises( + db_exception.DBMigrationError, + self.expand, + latest_version, + "You are attempting to upgrade migrate ahead of expand") + + def test_running_db_sync_migrate_ahead_of_expand_fails(self): + self.upgrade() + self._set_db_sync_command_versions() + self.assertRaises( + db_exception.DBMigrationError, + self.migrate, + 2, + "You are attempting to upgrade migrate ahead of expand") + + def test_running_db_sync_contract_ahead_of_migrate_fails(self): + self.upgrade() + self._set_db_sync_command_versions() + self.assertRaises( + db_exception.DBMigrationError, + self.contract, + 2, + "You are attempting to upgrade contract ahead of migrate") + + class FullMigration(SqlMigrateBase, unit.TestCase): """Test complete orchestration between all database phases.""" @@ -1691,21 +1735,6 @@ class FullMigration(SqlMigrateBase, unit.TestCase): # Upgrade the legacy repository self.upgrade() - def test_that_running_upgrades_out_of_order_passes(self): - # Currently, an operator can run db_sync operations out of order - # without an error or exception being raised. If this gets fixed in - # the future to force expand, migrate, contract to be run in order, - # then you may change this test to reflect that. - self.contract(2) - self.expand(1) - self.migrate(1) - expand = upgrades.get_db_version('expand_repo') - migrate = upgrades.get_db_version('data_migration_repo') - contract = upgrades.get_db_version('contract_repo') - self.assertTrue(2, contract) - self.assertTrue(1, expand) - self.assertTrue(1, migrate) - def test_db_sync_check(self): checker = cli.DbSync() latest_version = self.repos[EXPAND_REPO].max_version @@ -1742,16 +1771,13 @@ class FullMigration(SqlMigrateBase, unit.TestCase): self.assertIn("All db_sync commands are upgraded", log_info.output) self.assertEqual(status, 0) - def test_db_sync_check_out_of_sync(self): - checker = cli.DbSync() - # Assert we alert operator upgrades are out of sync + def test_out_of_sync_db_migration_fails(self): + # We shouldn't allow for operators to accidentally run migration out of + # order. This test ensures we fail if we attempt to upgrade the + # contract repository ahead of the expand or migrate repositories. self.expand(3) self.migrate(3) - self.contract(4) - log_info = self.useFixture(fixtures.FakeLogger(level=log.INFO)) - status = checker.check_db_sync_status() - self.assertIn("Your database is out of sync", log_info.output) - self.assertEqual(status, 1) + self.assertRaises(db_exception.DBMigrationError, self.contract, 4) def test_migration_002_password_created_at_not_nullable(self): # upgrade each repository to 001 diff --git a/releasenotes/notes/bug-1615014-b30f606a2d202428.yaml b/releasenotes/notes/bug-1615014-b30f606a2d202428.yaml new file mode 100644 index 0000000000..b56d2ed109 --- /dev/null +++ b/releasenotes/notes/bug-1615014-b30f606a2d202428.yaml @@ -0,0 +1,7 @@ +--- +fixes: + - | + [`bug 1615014 `_] + Migration order is now strictly enforced. The ensure upgrade process is + done in the order it is officially documented and support, starting with + `expand`, then `migrate`, and finishing with `contract`.