From 692f289a3298244ca4f007a0d0602b4113541b00 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mathieu=20Gagne=CC=81?= Date: Thu, 14 Jun 2018 17:56:11 -0400 Subject: [PATCH] Detect skip version upgrades from version earlier than Pike When performing a skip version upgrade from a release earlier than Pike, Ironic will crash when check_versions cannot find the 'version' columns in the database. This change adds a safety check which detects old database version missing the 'version' columns. Instead of crashing, it will inform the user that skip version upgrades are not supported and that database migrations need to be run for each skipped versions instead. Story: 2002558 Task: 22122 Change-Id: Ifa100c6fd168fc59b56bba0c41836958b10f2d47 --- ironic/cmd/dbsync.py | 31 ++++++++++++++----- ironic/common/exception.py | 4 +++ ironic/db/sqlalchemy/api.py | 11 +++++++ ironic/tests/unit/db/test_api.py | 8 +++++ ...-handle-skip-upgrade-3b6f06ac24937aa4.yaml | 11 +++++++ 5 files changed, 57 insertions(+), 8 deletions(-) create mode 100644 releasenotes/notes/better-handle-skip-upgrade-3b6f06ac24937aa4.yaml diff --git a/ironic/cmd/dbsync.py b/ironic/cmd/dbsync.py index 223cb7eafa..e75a9fb154 100644 --- a/ironic/cmd/dbsync.py +++ b/ironic/cmd/dbsync.py @@ -24,6 +24,7 @@ import sys from oslo_config import cfg from ironic.common import context +from ironic.common import exception from ironic.common.i18n import _ from ironic.common import service from ironic.conf import CONF @@ -88,14 +89,28 @@ class DBCommand(object): # 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 ' - '"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. + try: + if not dbapi.check_versions(): + 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) + 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()) sys.exit(2) def upgrade(self): diff --git a/ironic/common/exception.py b/ironic/common/exception.py index b08bb22190..78970d0d61 100644 --- a/ironic/common/exception.py +++ b/ironic/common/exception.py @@ -783,3 +783,7 @@ class BIOSSettingNotFound(NotFound): class BIOSSettingListNotFound(NotFound): _msg_fmt = _("Node %(node)s doesn't have BIOS settings '%(names)s'") + + +class DatabaseVersionTooOld(IronicException): + _msg_fmt = _("Database version is too old") diff --git a/ironic/db/sqlalchemy/api.py b/ironic/db/sqlalchemy/api.py index a4db43be2b..bcc22a5271 100644 --- a/ironic/db/sqlalchemy/api.py +++ b/ironic/db/sqlalchemy/api.py @@ -1204,6 +1204,17 @@ class Connection(api.Connection): 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 diff --git a/ironic/tests/unit/db/test_api.py b/ironic/tests/unit/db/test_api.py index 7265cc480a..2fd8742b39 100644 --- a/ironic/tests/unit/db/test_api.py +++ b/ironic/tests/unit/db/test_api.py @@ -10,6 +10,8 @@ # License for the specific language governing permissions and limitations # under the License. +import mock +from oslo_db.sqlalchemy import utils as db_utils from oslo_utils import uuidutils from testtools import matchers @@ -31,6 +33,12 @@ class UpgradingTestCase(base.DbTestCase): # nothing in the DB self.assertTrue(self.dbapi.check_versions()) + @mock.patch.object(db_utils, 'column_exists', autospec=True) + def test_check_versions_missing_version_columns(self, column_exists): + column_exists.return_value = False + self.assertRaises(exception.DatabaseVersionTooOld, + self.dbapi.check_versions) + def test_check_versions(self): for v in self.object_versions['Node']: node = utils.create_test_node(uuid=uuidutils.generate_uuid(), diff --git a/releasenotes/notes/better-handle-skip-upgrade-3b6f06ac24937aa4.yaml b/releasenotes/notes/better-handle-skip-upgrade-3b6f06ac24937aa4.yaml new file mode 100644 index 0000000000..cbed51c4a1 --- /dev/null +++ b/releasenotes/notes/better-handle-skip-upgrade-3b6f06ac24937aa4.yaml @@ -0,0 +1,11 @@ +--- +fixes: + - | + Better handles the case when an operator attempts to perform an upgrade + from a release older than Pike, directly to a release newer than Pike, + skipping one or more releases in between (i.e. a "skip version upgrade"). + Instead of crashing, the operator will be informed that + upgrading from a version older than the previous release is not supported + (skip version upgrades) and that (as of Pike) all database migrations need + to be performed using the previous releases for a fast-forward upgrade. + [Bug `2002558 `_]