From e88d3b2c8205a4865d64419623de10d9159eeea2 Mon Sep 17 00:00:00 2001 From: John Griffith Date: Fri, 6 Oct 2017 17:56:52 +0000 Subject: [PATCH] Fix migration 112 to use live_data_migration API In the 112 db migration I was being lazy and generating and updating the newly added UUID column. I also didn't update the Service object (left that for the follow on patch). Turns out we're going to want/need the online_data_migration pieces for some follow up work and we might as well do this at least a little bit more efficiently/correctly now. This patch modifies the db migration to NOT try and populate the newly added UUID fields in the Service table, and it updates the Service object properly including adding the gen UUID components to service.create() This also fixes up what we had in place for the online_data_migration code in cmd/manage.py; note that we had a framework there/started but it wasn't being used up to this point. Change-Id: I6696a15cd2c8fbf851a59b8d6d60ae1981bb1b89 Closes-Bug: #1721837 --- cinder/cmd/manage.py | 24 +++++++------- cinder/db/api.py | 12 +++++++ cinder/db/sqlalchemy/api.py | 33 +++++++++++++++++++ .../112_add_uuid_to_services_table.py | 9 ----- cinder/objects/base.py | 1 + cinder/objects/service.py | 29 +++++++++++++++- cinder/tests/unit/fake_service.py | 1 + cinder/tests/unit/objects/test_objects.py | 2 +- cinder/tests/unit/objects/test_service.py | 3 +- cinder/tests/unit/test_cmd.py | 22 ++++++------- 10 files changed, 100 insertions(+), 36 deletions(-) diff --git a/cinder/cmd/manage.py b/cinder/cmd/manage.py index 917974e690d..873e409028d 100644 --- a/cinder/cmd/manage.py +++ b/cinder/cmd/manage.py @@ -205,7 +205,10 @@ class HostCommands(object): class DbCommands(object): """Class for managing the database.""" - online_migrations = () + online_migrations = ( + # Added in Queens + db.service_uuids_online_data_migration, + ) def __init__(self): pass @@ -250,13 +253,13 @@ class DbCommands(object): "logs for more details.")) sys.exit(1) - def _run_migration(self, ctxt, max_count, ignore_state): + def _run_migration(self, ctxt, max_count): ran = 0 migrations = {} for migration_meth in self.online_migrations: count = max_count - ran try: - found, done = migration_meth(ctxt, count, ignore_state) + found, done = migration_meth(ctxt, count) except Exception: print(_("Error attempting to run %(method)s") % {'method': migration_meth.__name__}) @@ -283,11 +286,7 @@ class DbCommands(object): @args('--max_count', metavar='', dest='max_count', type=int, help='Maximum number of objects to consider.') - @args('--ignore_state', action='store_true', dest='ignore_state', - help='Force records to migrate even if another operation is ' - 'performed on them. This may be dangerous, please refer to ' - 'release notes for more information.') - def online_data_migrations(self, max_count=None, ignore_state=False): + def online_data_migrations(self, max_count=None): """Perform online data migrations for the release in batches.""" ctxt = context.get_admin_context() if max_count is not None: @@ -303,19 +302,18 @@ class DbCommands(object): ran = None migration_info = {} while ran is None or ran != 0: - migrations = self._run_migration(ctxt, max_count, ignore_state) + migrations = self._run_migration(ctxt, max_count) migration_info.update(migrations) ran = sum([done for found, done, remaining in migrations.values()]) if not unlimited: break headers = ["{}".format(_('Migration')), - "{}".format(_('Found')), - "{}".format(_('Done')), - "{}".format(_('Remaining'))] + "{}".format(_('Total Needed')), + "{}".format(_('Completed')), ] t = prettytable.PrettyTable(headers) for name in sorted(migration_info.keys()): info = migration_info[name] - t.add_row([name, info[0], info[1], info[2]]) + t.add_row([name, info[0], info[1]]) print(t) sys.exit(1 if ran else 0) diff --git a/cinder/db/api.py b/cinder/db/api.py index ac24e0c4cec..c6df6cbf4a0 100644 --- a/cinder/db/api.py +++ b/cinder/db/api.py @@ -93,6 +93,10 @@ def dispose_engine(): ################### +def service_uuids_online_data_migration(context, max_count): + return IMPL.service_uuids_online_data_migration(context, max_count) + + def service_destroy(context, service_id): """Destroy the service or raise if it does not exist.""" return IMPL.service_destroy(context, service_id) @@ -142,6 +146,14 @@ def service_update(context, service_id, values): return IMPL.service_update(context, service_id, values) +def service_get_by_uuid(context, service_uuid): + """Get a service by it's uuid. + + Return Service ref or raise if it does not exist. + """ + return IMPL.service_get_by_uuid(context, service_uuid) + + ############### diff --git a/cinder/db/sqlalchemy/api.py b/cinder/db/sqlalchemy/api.py index 4c35cfb569f..e1fdbcfb3a7 100644 --- a/cinder/db/sqlalchemy/api.py +++ b/cinder/db/sqlalchemy/api.py @@ -32,6 +32,7 @@ import uuid from oslo_config import cfg from oslo_db import exception as db_exc from oslo_db import options +from oslo_db.sqlalchemy import enginefacade from oslo_db.sqlalchemy import session as db_session from oslo_log import log as logging from oslo_utils import importutils @@ -555,6 +556,16 @@ def service_get_all(context, backend_match_level=None, **filters): return [] if not query else query.all() +@require_admin_context +def service_get_by_uuid(context, service_uuid): + query = model_query(context, models.Service).fitler_by(uuid=service_uuid) + result = query.first() + if not result: + raise exception.ServiceNotFound(service_id=service_uuid) + + return result + + @require_admin_context def service_create(context, values): if not values.get('uuid'): @@ -585,6 +596,27 @@ def service_update(context, service_id, values): raise exception.ServiceNotFound(service_id=service_id) +@enginefacade.writer +def service_uuids_online_data_migration(context, max_count): + from cinder.objects import service + + total = 0 + updated = 0 + + db_services = model_query(context, models.Service).filter_by( + uuid=None).limit(max_count) + for db_service in db_services: + total += 1 + + # The conversion in the Service object code + # will generate a UUID and save it for us. + service_obj = service.Service._from_db_object( + context, service.Service(), db_service) + if 'uuid' in service_obj: + updated += 1 + return total, updated + + ################### @@ -605,6 +637,7 @@ def is_backend_frozen(context, host, cluster_name): ################### + def _cluster_query(context, is_up=None, get_services=False, services_summary=False, read_deleted='no', name_match_level=None, name=None, session=None, **filters): diff --git a/cinder/db/sqlalchemy/migrate_repo/versions/112_add_uuid_to_services_table.py b/cinder/db/sqlalchemy/migrate_repo/versions/112_add_uuid_to_services_table.py index f51eea6f87c..30667dd61c5 100644 --- a/cinder/db/sqlalchemy/migrate_repo/versions/112_add_uuid_to_services_table.py +++ b/cinder/db/sqlalchemy/migrate_repo/versions/112_add_uuid_to_services_table.py @@ -10,9 +10,6 @@ # License for the specific language governing permissions and limitations # under the License. -import six -import uuid - from sqlalchemy import Column from sqlalchemy.engine.reflection import Inspector from sqlalchemy import Index @@ -33,9 +30,3 @@ def upgrade(migrate_engine): if uuid_index_name not in (i['name'] for i in indexes): services = Table('services', meta, autoload=True) Index(uuid_index_name, services.c.uuid, unique=True).create() - - service_list = list(services.select().execute()) - for s in service_list: - if not s.uuid: - services.update().where(services.c.id == s.id).\ - values(uuid=six.text_type(uuid.uuid4())).execute() diff --git a/cinder/objects/base.py b/cinder/objects/base.py index 7b89c17b978..c600c9dfc65 100644 --- a/cinder/objects/base.py +++ b/cinder/objects/base.py @@ -135,6 +135,7 @@ OBJ_VERSIONS.add('1.24', {'LogLevel': '1.0', 'LogLevelList': '1.0'}) OBJ_VERSIONS.add('1.25', {'Group': '1.2'}) OBJ_VERSIONS.add('1.26', {'Snapshot': '1.5'}) OBJ_VERSIONS.add('1.27', {'Backup': '1.5', 'BackupImport': '1.5'}) +OBJ_VERSIONS.add('1.28', {'Service': '1.5'}) class CinderObjectRegistry(base.VersionedObjectRegistry): diff --git a/cinder/objects/service.py b/cinder/objects/service.py index a1706184f3a..9182cef8f26 100644 --- a/cinder/objects/service.py +++ b/cinder/objects/service.py @@ -12,6 +12,8 @@ # License for the specific language governing permissions and limitations # under the License. +from oslo_log import log as logging +from oslo_utils import uuidutils from oslo_utils import versionutils from oslo_versionedobjects import fields @@ -24,6 +26,9 @@ from cinder.objects import fields as c_fields from cinder import utils +LOG = logging.getLogger(__name__) + + @base.CinderObjectRegistry.register class Service(base.CinderPersistentObject, base.CinderObject, base.CinderObjectDictCompat, base.CinderComparableObject, @@ -33,7 +38,8 @@ class Service(base.CinderPersistentObject, base.CinderObject, # Version 1.2: Add get_minimum_rpc_version() and get_minimum_obj_version() # Version 1.3: Add replication fields # Version 1.4: Add cluster fields - VERSION = '1.4' + # Version 1.5: Add UUID field + VERSION = '1.5' OPTIONAL_FIELDS = ('cluster',) @@ -59,6 +65,8 @@ class Service(base.CinderPersistentObject, base.CinderObject, 'replication_status': c_fields.ReplicationStatusField(nullable=True), 'frozen': fields.BooleanField(default=False), 'active_backend_id': fields.StringField(nullable=True), + + 'uuid': fields.StringField(nullable=True), } def obj_make_compatible(self, primitive, target_version): @@ -71,6 +79,8 @@ class Service(base.CinderPersistentObject, base.CinderObject, if target_version < (1, 4): for obj_field in ('cluster', 'cluster_name'): primitive.pop(obj_field, None) + if target_version < (1, 5) and 'uuid' in primitive: + del primitive['uuid'] @staticmethod def _from_db_object(context, service, db_service, expected_attrs=None): @@ -98,6 +108,14 @@ class Service(base.CinderPersistentObject, base.CinderObject, service.cluster = None service.obj_reset_changes() + + # TODO(jdg): Remove in S when we're sure all Services have UUID in db + if 'uuid' not in service: + service.uuid = uuidutils.generate_uuid() + LOG.debug('Generated UUID %(uuid)s for service %(id)i', + dict(uuid=service.uuid, id=service.id)) + service.save() + return service def obj_load_attr(self, attrname): @@ -131,6 +149,11 @@ class Service(base.CinderPersistentObject, base.CinderObject, db_service = db.service_get(context, host=host, binary=binary_key) return cls._from_db_object(context, cls(context), db_service) + @classmethod + def get_by_uuid(cls, context, service_uuid): + db_service = db.service_get_by_uuid(context, service_uuid) + return cls._from_db_object(context, cls(), db_service) + def create(self): if self.obj_attr_is_set('id'): raise exception.ObjectActionError(action='create', @@ -139,6 +162,10 @@ class Service(base.CinderPersistentObject, base.CinderObject, if 'cluster' in updates: raise exception.ObjectActionError( action='create', reason=_('cluster assigned')) + if 'uuid' not in updates: + updates['uuid'] = uuidutils.generate_uuid() + self.uuid = updates['uuid'] + db_service = db.service_create(self._context, updates) self._from_db_object(self._context, self, db_service) diff --git a/cinder/tests/unit/fake_service.py b/cinder/tests/unit/fake_service.py index 455db247c86..30dd71b37e7 100644 --- a/cinder/tests/unit/fake_service.py +++ b/cinder/tests/unit/fake_service.py @@ -34,6 +34,7 @@ def fake_db_service(**updates): 'deleted_at': None, 'deleted': False, 'id': 123, + 'uuid': 'ce59413f-4061-425c-9ad0-3479bd102ab2', 'host': 'fake-host', 'binary': 'fake-service', 'topic': 'fake-service-topic', diff --git a/cinder/tests/unit/objects/test_objects.py b/cinder/tests/unit/objects/test_objects.py index 62ae091a60f..29c78b54d6c 100644 --- a/cinder/tests/unit/objects/test_objects.py +++ b/cinder/tests/unit/objects/test_objects.py @@ -43,7 +43,7 @@ object_data = { 'QualityOfServiceSpecs': '1.0-0b212e0a86ee99092229874e03207fe8', 'QualityOfServiceSpecsList': '1.0-15ecf022a68ddbb8c2a6739cfc9f8f5e', 'RequestSpec': '1.1-b0bd1a28d191d75648901fa853e8a733', - 'Service': '1.4-a6727ccda6d4043f5e38e75c7c518c7f', + 'Service': '1.5-dfa465098dd9017bad825cab55eacc93', 'ServiceList': '1.1-15ecf022a68ddbb8c2a6739cfc9f8f5e', 'Snapshot': '1.5-ac1cdbd5b89588f6a8f44afdf6b8b201', 'SnapshotList': '1.0-15ecf022a68ddbb8c2a6739cfc9f8f5e', diff --git a/cinder/tests/unit/objects/test_service.py b/cinder/tests/unit/objects/test_service.py index 1babfb584a0..7a709aed159 100644 --- a/cinder/tests/unit/objects/test_service.py +++ b/cinder/tests/unit/objects/test_service.py @@ -62,7 +62,8 @@ class TestService(test_objects.BaseObjectsTestCase): service = objects.Service(context=self.context) service.create() self.assertEqual(db_service['id'], service.id) - service_create.assert_called_once_with(self.context, {}) + service_create.assert_called_once_with(self.context, + {'uuid': mock.ANY}) @mock.patch('cinder.db.service_update') def test_save(self, service_update): diff --git a/cinder/tests/unit/test_cmd.py b/cinder/tests/unit/test_cmd.py index 5907597a582..ca2006607d9 100644 --- a/cinder/tests/unit/test_cmd.py +++ b/cinder/tests/unit/test_cmd.py @@ -227,7 +227,7 @@ class TestCinderManageCmd(test.TestCase): exit = self.assertRaises(SystemExit, db_cmds.online_data_migrations) self.assertEqual(0, exit.code) cinder_manage.DbCommands.online_migrations[0].assert_has_calls( - (mock.call(mock.ANY, 50, False),) * 2) + (mock.call(mock.ANY, 50),) * 2) def _fake_db_command(self, migrations=None): if migrations is None: @@ -254,17 +254,17 @@ class TestCinderManageCmd(test.TestCase): expected = """\ 5 rows matched query mock_mig_1, 4 migrated, 1 remaining 6 rows matched query mock_mig_2, 6 migrated, 0 remaining -+------------+-------+------+-----------+ -| Migration | Found | Done | Remaining | -+------------+-------+------+-----------+ -| mock_mig_1 | 5 | 4 | 1 | -| mock_mig_2 | 6 | 6 | 0 | -+------------+-------+------+-----------+ ++------------+--------------+-----------+ +| Migration | Total Needed | Completed | ++------------+--------------+-----------+ +| mock_mig_1 | 5 | 4 | +| mock_mig_2 | 6 | 6 | ++------------+--------------+-----------+ """ command.online_migrations[0].assert_has_calls([mock.call(ctxt, - 10, False)]) + 10)]) command.online_migrations[1].assert_has_calls([mock.call(ctxt, - 6, False)]) + 6)]) self.assertEqual(expected, sys.stdout.getvalue()) @@ -273,10 +273,10 @@ class TestCinderManageCmd(test.TestCase): def test_db_commands_online_data_migrations_ignore_state_and_max(self): db_cmds = cinder_manage.DbCommands() exit = self.assertRaises(SystemExit, db_cmds.online_data_migrations, - 2, True) + 2) self.assertEqual(1, exit.code) cinder_manage.DbCommands.online_migrations[0].assert_called_once_with( - mock.ANY, 2, True) + mock.ANY, 2) @mock.patch('cinder.cmd.manage.DbCommands.online_migrations', (mock.Mock(side_effect=((2, 2), (0, 0)), __name__='foo'),))