From d8f10c01ae4105a88e8caf58e89041d4c81f1670 Mon Sep 17 00:00:00 2001
From: Dmitry Tantsur <divius.inside@gmail.com>
Date: Fri, 19 Jan 2018 12:59:44 +0100
Subject: [PATCH] Allow data migrations to accept options

This allows migration to be tuned to a specific deployment. For example,
when we will migrate nodes to hardware types, an option will be used
to specify what to do with missing interfaces.

Change-Id: Ie5045b20b7420fc9b5d864bfb18258a4d8b93334
Related-Bug: #1690185
---
 doc/source/cli/ironic-dbsync.rst     |  7 ++-
 ironic/cmd/dbsync.py                 | 39 +++++++++++---
 ironic/tests/unit/cmd/test_dbsync.py | 78 ++++++++++++++++++++--------
 3 files changed, 95 insertions(+), 29 deletions(-)

diff --git a/doc/source/cli/ironic-dbsync.rst b/doc/source/cli/ironic-dbsync.rst
index bba2633d8c..e45f22166e 100644
--- a/doc/source/cli/ironic-dbsync.rst
+++ b/doc/source/cli/ironic-dbsync.rst
@@ -112,6 +112,11 @@ online_data_migrations
   If not specified, all the objects will be migrated (in batches of 50 to
   avoid locking the database for long periods of time).
 
+.. option:: --option <MIGRATION.KEY=VALUE>
+
+  If a migration accepts additional parameters, they can be passed via this
+  argument. It can be specified several times.
+
 This command will migrate objects in the database to their most recent versions.
 This command must be successfully run (return code 0) before upgrading to a
 future release.
@@ -124,7 +129,7 @@ It returns:
 
 * 0 (success) after migrations are finished or there are no data to migrate
 
-* 127 (error) if max-count is not a positive value
+* 127 (error) if max-count is not a positive value or an option is invalid
 
 * 2 (error) if the database is not compatible with this release. This command
   needs to be run using the previous release of ironic, before upgrading and
diff --git a/ironic/cmd/dbsync.py b/ironic/cmd/dbsync.py
index 0450ffede0..30b484df38 100644
--- a/ironic/cmd/dbsync.py
+++ b/ironic/cmd/dbsync.py
@@ -111,9 +111,10 @@ class DBCommand(object):
 
     def online_data_migrations(self):
         self._check_versions()
-        self._run_online_data_migrations(max_count=CONF.command.max_count)
+        self._run_online_data_migrations(max_count=CONF.command.max_count,
+                                         options=CONF.command.options)
 
-    def _run_migration_functions(self, context, max_count):
+    def _run_migration_functions(self, context, max_count, options):
         """Runs the migration functions.
 
         Runs the data migration functions in the ONLINE_MIGRATIONS list.
@@ -124,6 +125,8 @@ class DBCommand(object):
         :param: context: an admin context
         :param: max_count: the maximum number of objects (rows) to migrate;
             a value >= 1.
+        :param: options: migration options - dict mapping migration name
+            to a dictionary of options for this migration.
         :raises: Exception from the migration function
         :returns: Boolean value indicating whether migrations are done. Returns
             False if max_count objects have been migrated (since at that
@@ -135,10 +138,12 @@ class DBCommand(object):
 
         for migration_func_obj, migration_func_name in ONLINE_MIGRATIONS:
             migration_func = getattr(migration_func_obj, migration_func_name)
+            migration_opts = options.get(migration_func_name, {})
             num_to_migrate = max_count - total_migrated
             try:
                 total_to_do, num_migrated = migration_func(context,
-                                                           num_to_migrate)
+                                                           num_to_migrate,
+                                                           **migration_opts)
             except Exception as e:
                 print(_("Error while running %(migration)s: %(err)s.")
                       % {'migration': migration_func.__name__, 'err': e},
@@ -165,7 +170,7 @@ class DBCommand(object):
 
         return True
 
-    def _run_online_data_migrations(self, max_count=None):
+    def _run_online_data_migrations(self, max_count=None, options=None):
         """Perform online data migrations for the release.
 
         Online data migrations are done by running all the data migration
@@ -177,13 +182,27 @@ class DBCommand(object):
         :param: max_count: the maximum number of individual object migrations
             or modified rows, a value >= 1. If None, migrations are run in a
             loop in batches of 50, until completion.
+        :param: options: options to pass to migrations. List of values in the
+            form of <migration name>.<option>=<value>
         :raises: SystemExit. With exit code of:
             0: when all migrations are complete.
             1: when objects were migrated and the command needs to be
                re-run (because there might be more objects to be migrated)
-            127: if max_count is < 1
+            127: if max_count is < 1 or any option is invalid
         :raises: Exception from a migration function
         """
+        parsed_options = {}
+        if options:
+            for option in options:
+                try:
+                    migration, key_value = option.split('.', 1)
+                    key, value = key_value.split('=', 1)
+                except ValueError:
+                    print(_("Malformed option %s") % option)
+                    sys.exit(127)
+                else:
+                    parsed_options.setdefault(migration, {})[key] = value
+
         admin_context = context.get_admin_context()
         finished_migrating = False
         if max_count is None:
@@ -192,7 +211,7 @@ class DBCommand(object):
                     'completed.') % max_count)
             while not finished_migrating:
                 finished_migrating = self._run_migration_functions(
-                    admin_context, max_count)
+                    admin_context, max_count, parsed_options)
             print(_('Data migrations have completed.'))
             sys.exit(0)
 
@@ -201,7 +220,8 @@ class DBCommand(object):
             sys.exit(127)
 
         finished_migrating = self._run_migration_functions(admin_context,
-                                                           max_count)
+                                                           max_count,
+                                                           parsed_options)
         if finished_migrating:
             print(_('Data migrations have completed.'))
             sys.exit(0)
@@ -269,6 +289,11 @@ def add_command_parsers(subparsers):
         '--max-count', metavar='<number>', dest='max_count', type=int,
         help=_("Maximum number of objects to migrate. If unspecified, all "
                "objects are migrated."))
+    parser.add_argument(
+        '--option', metavar='<migration.opt=val>', action='append',
+        dest='options', default=[],
+        help=_("Options to pass to the migrations in the form of "
+               "<migration name>.<option>=<value>"))
     parser.set_defaults(func=command_object.online_data_migrations)
 
 
diff --git a/ironic/tests/unit/cmd/test_dbsync.py b/ironic/tests/unit/cmd/test_dbsync.py
index 0b02990175..406215c1a8 100644
--- a/ironic/tests/unit/cmd/test_dbsync.py
+++ b/ironic/tests/unit/cmd/test_dbsync.py
@@ -57,7 +57,7 @@ class OnlineMigrationTestCase(db_base.DbTestCase):
         mock_func = mock.MagicMock(side_effect=((15, 15),), __name__='foo')
         with mock.patch.object(self.dbapi, 'foo', mock_func, create=True):
             self.assertTrue(
-                self.db_cmds._run_migration_functions(self.context, 50))
+                self.db_cmds._run_migration_functions(self.context, 50, {}))
             mock_func.assert_called_once_with(self.context, 50)
 
     @mock.patch.object(dbsync, 'ONLINE_MIGRATIONS', autospec=True)
@@ -65,7 +65,7 @@ class OnlineMigrationTestCase(db_base.DbTestCase):
         # No migration functions to run
         mock_migrations.__iter__.return_value = ()
         self.assertTrue(
-            self.db_cmds._run_migration_functions(self.context, 50))
+            self.db_cmds._run_migration_functions(self.context, 50, {}))
 
     @mock.patch.object(dbsync, 'ONLINE_MIGRATIONS', autospec=True)
     def test__run_migration_functions_exception(self, mock_migrations):
@@ -76,7 +76,7 @@ class OnlineMigrationTestCase(db_base.DbTestCase):
         with mock.patch.object(self.dbapi, 'foo', mock_func, create=True):
             self.assertRaises(
                 TypeError, self.db_cmds._run_migration_functions,
-                self.context, 50)
+                self.context, 50, {})
         mock_func.assert_called_once_with(self.context, 50)
 
     @mock.patch.object(dbsync, 'ONLINE_MIGRATIONS', autospec=True)
@@ -89,10 +89,12 @@ class OnlineMigrationTestCase(db_base.DbTestCase):
         with mock.patch.object(self.dbapi, 'func1', mock_func1, create=True):
             with mock.patch.object(self.dbapi, 'func2', mock_func2,
                                    create=True):
-                self.assertTrue(
-                    self.db_cmds._run_migration_functions(self.context, 50))
-        mock_func1.assert_called_once_with(self.context, 50)
-        mock_func2.assert_called_once_with(self.context, 35)
+                options = {'func1': {'key': 'value'},
+                           'func2': {'x': 1, 'y': 2}}
+                self.assertTrue(self.db_cmds._run_migration_functions(
+                    self.context, 50, options))
+        mock_func1.assert_called_once_with(self.context, 50, key='value')
+        mock_func2.assert_called_once_with(self.context, 35, x=1, y=2)
 
     @mock.patch.object(dbsync, 'ONLINE_MIGRATIONS', autospec=True)
     def test__run_migration_functions_2_notdone(self, mock_migrations):
@@ -104,8 +106,8 @@ class OnlineMigrationTestCase(db_base.DbTestCase):
         with mock.patch.object(self.dbapi, 'func1', mock_func1, create=True):
             with mock.patch.object(self.dbapi, 'func2', mock_func2,
                                    create=True):
-                self.assertFalse(
-                    self.db_cmds._run_migration_functions(self.context, 10))
+                self.assertFalse(self.db_cmds._run_migration_functions(
+                    self.context, 10, {}))
         mock_func1.assert_called_once_with(self.context, 10)
         self.assertFalse(mock_func2.called)
 
@@ -119,8 +121,8 @@ class OnlineMigrationTestCase(db_base.DbTestCase):
         with mock.patch.object(self.dbapi, 'func1', mock_func1, create=True):
             with mock.patch.object(self.dbapi, 'func2', mock_func2,
                                    create=True):
-                self.assertFalse(
-                    self.db_cmds._run_migration_functions(self.context, 10))
+                self.assertFalse(self.db_cmds._run_migration_functions(
+                    self.context, 10, {}))
         mock_func1.assert_called_once_with(self.context, 10)
         self.assertFalse(mock_func2.called)
 
@@ -134,8 +136,8 @@ class OnlineMigrationTestCase(db_base.DbTestCase):
         with mock.patch.object(self.dbapi, 'func1', mock_func1, create=True):
             with mock.patch.object(self.dbapi, 'func2', mock_func2,
                                    create=True):
-                self.assertTrue(
-                    self.db_cmds._run_migration_functions(self.context, 15))
+                self.assertTrue(self.db_cmds._run_migration_functions(
+                    self.context, 15, {}))
         mock_func1.assert_called_once_with(self.context, 15)
         mock_func2.assert_called_once_with(self.context, 5)
 
@@ -151,12 +153,12 @@ class OnlineMigrationTestCase(db_base.DbTestCase):
         with mock.patch.object(self.dbapi, 'func1', mock_func1, create=True):
             with mock.patch.object(self.dbapi, 'func2', mock_func2,
                                    create=True):
-                self.assertFalse(
-                    self.db_cmds._run_migration_functions(self.context, 10))
+                self.assertFalse(self.db_cmds._run_migration_functions(
+                    self.context, 10, {}))
                 mock_func1.assert_called_once_with(self.context, 10)
                 self.assertFalse(mock_func2.called)
-                self.assertTrue(
-                    self.db_cmds._run_migration_functions(self.context, 10))
+                self.assertTrue(self.db_cmds._run_migration_functions(
+                    self.context, 10, {}))
                 mock_func1.assert_has_calls((mock.call(self.context, 10),) * 2)
                 mock_func2.assert_called_once_with(self.context, 10)
 
@@ -167,7 +169,41 @@ class OnlineMigrationTestCase(db_base.DbTestCase):
         exit = self.assertRaises(SystemExit,
                                  self.db_cmds._run_online_data_migrations)
         self.assertEqual(0, exit.code)
-        mock_functions.assert_called_once_with(self.db_cmds, mock.ANY, 50)
+        mock_functions.assert_called_once_with(self.db_cmds, mock.ANY, 50, {})
+
+    @mock.patch.object(dbsync.DBCommand, '_run_migration_functions',
+                       autospec=True)
+    def test__run_online_data_migrations_with_options(self, mock_functions):
+        mock_functions.return_value = True
+        exit = self.assertRaises(SystemExit,
+                                 self.db_cmds._run_online_data_migrations,
+                                 options=["m1.key1=value1", "m1.key2=value2",
+                                          "m2.key3=value3"])
+        self.assertEqual(0, exit.code)
+        mock_functions.assert_called_once_with(self.db_cmds, mock.ANY, 50,
+                                               {'m1': {'key1': 'value1',
+                                                       'key2': 'value2'},
+                                                'm2': {'key3': 'value3'}})
+
+    @mock.patch.object(dbsync.DBCommand, '_run_migration_functions',
+                       autospec=True)
+    def test__run_online_data_migrations_invalid_option1(self, mock_functions):
+        mock_functions.return_value = True
+        exit = self.assertRaises(SystemExit,
+                                 self.db_cmds._run_online_data_migrations,
+                                 options=["m1key1=value1"])
+        self.assertEqual(127, exit.code)
+        self.assertFalse(mock_functions.called)
+
+    @mock.patch.object(dbsync.DBCommand, '_run_migration_functions',
+                       autospec=True)
+    def test__run_online_data_migrations_invalid_option2(self, mock_functions):
+        mock_functions.return_value = True
+        exit = self.assertRaises(SystemExit,
+                                 self.db_cmds._run_online_data_migrations,
+                                 options=["m1.key1value1"])
+        self.assertEqual(127, exit.code)
+        self.assertFalse(mock_functions.called)
 
     @mock.patch.object(dbsync.DBCommand, '_run_migration_functions',
                        autospec=True)
@@ -177,7 +213,7 @@ class OnlineMigrationTestCase(db_base.DbTestCase):
                                  self.db_cmds._run_online_data_migrations)
         self.assertEqual(0, exit.code)
         mock_functions.assert_has_calls(
-            (mock.call(self.db_cmds, mock.ANY, 50),) * 2)
+            (mock.call(self.db_cmds, mock.ANY, 50, {}),) * 2)
 
     @mock.patch.object(dbsync.DBCommand, '_run_migration_functions',
                        autospec=True)
@@ -187,7 +223,7 @@ class OnlineMigrationTestCase(db_base.DbTestCase):
                                  self.db_cmds._run_online_data_migrations,
                                  max_count=30)
         self.assertEqual(1, exit.code)
-        mock_functions.assert_called_once_with(self.db_cmds, mock.ANY, 30)
+        mock_functions.assert_called_once_with(self.db_cmds, mock.ANY, 30, {})
 
     @mock.patch.object(dbsync.DBCommand, '_run_migration_functions',
                        autospec=True)
@@ -204,4 +240,4 @@ class OnlineMigrationTestCase(db_base.DbTestCase):
     def test__run_online_data_migrations_exception(self, mock_functions):
         mock_functions.side_effect = TypeError("yuck")
         self.assertRaises(TypeError, self.db_cmds._run_online_data_migrations)
-        mock_functions.assert_called_once_with(self.db_cmds, mock.ANY, 50)
+        mock_functions.assert_called_once_with(self.db_cmds, mock.ANY, 50, {})