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

@@ -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):

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")

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

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(),

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 <https://storyboard.openstack.org/#!/story/2002558>`_]