From 351677d3b83f83a5d203642c750133a58e695570 Mon Sep 17 00:00:00 2001 From: Vladyslav Drok Date: Fri, 1 Dec 2017 15:30:38 -0800 Subject: [PATCH] Do not access dbapi attributes on dbsync import This may cause troubles as some config options may not be registered yet on the import of dbsync module. Accessing dbapi attributes, which is lazy-loaded, will force the object to initialize, and it may fail at such an early stage. Change-Id: I3b37498325a191f7c42b3b856b1b2cf326d57492 --- ironic/cmd/dbsync.py | 9 +++- ironic/tests/unit/cmd/test_dbsync.py | 79 ++++++++++++++++++---------- 2 files changed, 58 insertions(+), 30 deletions(-) diff --git a/ironic/cmd/dbsync.py b/ironic/cmd/dbsync.py index 6c34bee219..0450ffede0 100644 --- a/ironic/cmd/dbsync.py +++ b/ironic/cmd/dbsync.py @@ -57,10 +57,14 @@ dbapi = db_api.get_instance() # migrated (at the beginning of this call) and the number # of migrated objects. # """ +# NOTE(vdrok): Do not access objects' attributes, instead only provide object +# and attribute name tuples, so that not to trigger the load of the whole +# object, in case it is lazy loaded. The attribute will be accessed when needed +# by doing getattr on the object ONLINE_MIGRATIONS = ( # Added in Pike, modified in Queens # TODO(rloo): remove in Rocky - dbapi.backfill_version_column, + (dbapi, 'backfill_version_column'), ) @@ -129,7 +133,8 @@ class DBCommand(object): """ total_migrated = 0 - for migration_func in ONLINE_MIGRATIONS: + for migration_func_obj, migration_func_name in ONLINE_MIGRATIONS: + migration_func = getattr(migration_func_obj, migration_func_name) num_to_migrate = max_count - total_migrated try: total_to_do, num_migrated = migration_func(context, diff --git a/ironic/tests/unit/cmd/test_dbsync.py b/ironic/tests/unit/cmd/test_dbsync.py index d8a459b25b..0b02990175 100644 --- a/ironic/tests/unit/cmd/test_dbsync.py +++ b/ironic/tests/unit/cmd/test_dbsync.py @@ -53,11 +53,12 @@ class OnlineMigrationTestCase(db_base.DbTestCase): @mock.patch.object(dbsync, 'ONLINE_MIGRATIONS', autospec=True) def test__run_migration_functions(self, mock_migrations): + mock_migrations.__iter__.return_value = ((self.dbapi, 'foo'),) mock_func = mock.MagicMock(side_effect=((15, 15),), __name__='foo') - mock_migrations.__iter__.return_value = (mock_func,) - self.assertTrue( - self.db_cmds._run_migration_functions(self.context, 50)) - mock_func.assert_called_once_with(self.context, 50) + with mock.patch.object(self.dbapi, 'foo', mock_func, create=True): + self.assertTrue( + 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) def test__run_migration_functions_none(self, mock_migrations): @@ -68,74 +69,96 @@ class OnlineMigrationTestCase(db_base.DbTestCase): @mock.patch.object(dbsync, 'ONLINE_MIGRATIONS', autospec=True) def test__run_migration_functions_exception(self, mock_migrations): + mock_migrations.__iter__.return_value = ((self.dbapi, 'foo'),) # Migration function raises exception mock_func = mock.MagicMock(side_effect=TypeError("bar"), __name__='foo') - mock_migrations.__iter__.return_value = (mock_func,) - self.assertRaises(TypeError, self.db_cmds._run_migration_functions, - self.context, 50) + with mock.patch.object(self.dbapi, 'foo', mock_func, create=True): + self.assertRaises( + TypeError, 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) def test__run_migration_functions_2(self, mock_migrations): # 2 migration functions, migration completed + mock_migrations.__iter__.return_value = ((self.dbapi, 'func1'), + (self.dbapi, 'func2')) mock_func1 = mock.MagicMock(side_effect=((15, 15),), __name__='func1') mock_func2 = mock.MagicMock(side_effect=((20, 20),), __name__='func2') - mock_migrations.__iter__.return_value = (mock_func1, mock_func2) - self.assertTrue( - self.db_cmds._run_migration_functions(self.context, 50)) + 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) @mock.patch.object(dbsync, 'ONLINE_MIGRATIONS', autospec=True) def test__run_migration_functions_2_notdone(self, mock_migrations): # 2 migration functions; only first function was run but not completed + mock_migrations.__iter__.return_value = ((self.dbapi, 'func1'), + (self.dbapi, 'func2')) mock_func1 = mock.MagicMock(side_effect=((15, 10),), __name__='func1') mock_func2 = mock.MagicMock(side_effect=((20, 0),), __name__='func2') - mock_migrations.__iter__.return_value = (mock_func1, mock_func2) - self.assertFalse( - self.db_cmds._run_migration_functions(self.context, 10)) + 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)) mock_func1.assert_called_once_with(self.context, 10) self.assertFalse(mock_func2.called) @mock.patch.object(dbsync, 'ONLINE_MIGRATIONS', autospec=True) def test__run_migration_functions_2_onedone(self, mock_migrations): # 2 migration functions; only first function was run and completed + mock_migrations.__iter__.return_value = ((self.dbapi, 'func1'), + (self.dbapi, 'func2')) mock_func1 = mock.MagicMock(side_effect=((10, 10),), __name__='func1') mock_func2 = mock.MagicMock(side_effect=((20, 0),), __name__='func2') - mock_migrations.__iter__.return_value = (mock_func1, mock_func2) - self.assertFalse( - self.db_cmds._run_migration_functions(self.context, 10)) + 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)) mock_func1.assert_called_once_with(self.context, 10) self.assertFalse(mock_func2.called) @mock.patch.object(dbsync, 'ONLINE_MIGRATIONS', autospec=True) def test__run_migration_functions_2_done(self, mock_migrations): # 2 migration functions; migrations completed + mock_migrations.__iter__.return_value = ((self.dbapi, 'func1'), + (self.dbapi, 'func2')) mock_func1 = mock.MagicMock(side_effect=((10, 10),), __name__='func1') mock_func2 = mock.MagicMock(side_effect=((0, 0),), __name__='func2') - mock_migrations.__iter__.return_value = (mock_func1, mock_func2) - self.assertTrue( - self.db_cmds._run_migration_functions(self.context, 15)) + 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)) mock_func1.assert_called_once_with(self.context, 15) mock_func2.assert_called_once_with(self.context, 5) @mock.patch.object(dbsync, 'ONLINE_MIGRATIONS', autospec=True) def test__run_migration_functions_two_calls_done(self, mock_migrations): # 2 migration functions; migrations completed after calling twice + mock_migrations.__iter__.return_value = ((self.dbapi, 'func1'), + (self.dbapi, 'func2')) mock_func1 = mock.MagicMock(side_effect=((10, 10), (0, 0)), __name__='func1') mock_func2 = mock.MagicMock(side_effect=((0, 0), (0, 0)), __name__='func2') - mock_migrations.__iter__.return_value = (mock_func1, mock_func2) - 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)) - mock_func1.assert_has_calls((mock.call(self.context, 10),) * 2) - mock_func2.assert_called_once_with(self.context, 10) + 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)) + 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)) + mock_func1.assert_has_calls((mock.call(self.context, 10),) * 2) + mock_func2.assert_called_once_with(self.context, 10) @mock.patch.object(dbsync.DBCommand, '_run_migration_functions', autospec=True)