From cdb6cdcc961a5553cf9f81cbb49994d98180f947 Mon Sep 17 00:00:00 2001 From: John Griffith Date: Sat, 11 Nov 2017 15:31:02 +0000 Subject: [PATCH] Add service_uuid FK to volumes This patch adds a service_uuid FK to the volumes table. Up until now we've just done some host name parsing and to match up service node with where the volume is being serviced from. With this, we now have a unique identifier that's user visible to indicate what node the volume-service for a particular volume is being serviced by. We'll use this for things like share-target locks going forward. Change-Id: Ia5d1e988256246e3552e3a770146503ea7f7bf73 --- cinder/cmd/manage.py | 9 +++ cinder/db/api.py | 4 ++ cinder/db/sqlalchemy/api.py | 32 +++++++++++ .../114_add_service_uuid_fk_to_volumes.py | 37 +++++++++++++ cinder/db/sqlalchemy/models.py | 10 ++++ cinder/objects/base.py | 1 + cinder/objects/volume.py | 7 ++- cinder/tests/unit/objects/test_objects.py | 2 +- cinder/tests/unit/test_db_api.py | 55 +++++++++++++++++++ cinder/tests/unit/test_migrations.py | 12 ++++ cinder/tests/unit/volume/test_volume.py | 2 +- cinder/volume/manager.py | 24 ++++++++ 12 files changed, 191 insertions(+), 4 deletions(-) create mode 100644 cinder/db/sqlalchemy/migrate_repo/versions/114_add_service_uuid_fk_to_volumes.py diff --git a/cinder/cmd/manage.py b/cinder/cmd/manage.py index a32d3fbd215..6a832e06b13 100644 --- a/cinder/cmd/manage.py +++ b/cinder/cmd/manage.py @@ -208,6 +208,7 @@ class DbCommands(object): # Added in Queens db.service_uuids_online_data_migration, db.backup_service_online_migration, + db.volume_service_uuids_online_data_migration, ) def __init__(self): @@ -299,6 +300,14 @@ class DbCommands(object): max_count = 50 print(_('Running batches of %i until complete.') % max_count) + # FIXME(jdg): So this is annoying and confusing, + # we iterate through in batches until there are no + # more updates, that's AWESOME!! BUT we only print + # out a table reporting found/done AFTER the loop + # here, so that means the response the user sees is + # always a table of "needed 0" and "completed 0". + # So it's an indication of "all done" but it seems like + # some feedback as we go would be nice to have here. ran = None migration_info = {} while ran is None or ran != 0: diff --git a/cinder/db/api.py b/cinder/db/api.py index d07f10a9b14..a0f1ca8f7e6 100644 --- a/cinder/db/api.py +++ b/cinder/db/api.py @@ -101,6 +101,10 @@ def backup_service_online_migration(context, max_count): return IMPL.backup_service_online_migration(context, max_count) +def volume_service_uuids_online_data_migration(context, max_count): + return IMPL.volume_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) diff --git a/cinder/db/sqlalchemy/api.py b/cinder/db/sqlalchemy/api.py index f3fe6f5dd4c..0e9a15aa579 100644 --- a/cinder/db/sqlalchemy/api.py +++ b/cinder/db/sqlalchemy/api.py @@ -632,6 +632,38 @@ def backup_service_online_migration(context, max_count): return total, updated + +@enginefacade.writer +def volume_service_uuids_online_data_migration(context, max_count): + """Update volume service_uuid columns.""" + + updated = 0 + query = model_query(context, + models.Volume).filter_by(service_uuid=None) + total = query.count() + vol_refs = query.limit(max_count).all() + + service_refs = model_query(context, models.Service).filter_by( + topic="cinder-volume").limit(max_count).all() + + # build a map to access the service uuid by host + svc_map = {} + for svc in service_refs: + svc_map[svc.host] = svc.uuid + + # update our volumes appropriately + for v in vol_refs: + host = v.host.split('#') + v['service_uuid'] = svc_map[host[0]] + # re-use the session we already have associated with the + # volumes here (from the query above) + session = query.session + with session.begin(): + v.save(session) + updated += 1 + return total, updated + + ################### diff --git a/cinder/db/sqlalchemy/migrate_repo/versions/114_add_service_uuid_fk_to_volumes.py b/cinder/db/sqlalchemy/migrate_repo/versions/114_add_service_uuid_fk_to_volumes.py new file mode 100644 index 00000000000..dead23a4900 --- /dev/null +++ b/cinder/db/sqlalchemy/migrate_repo/versions/114_add_service_uuid_fk_to_volumes.py @@ -0,0 +1,37 @@ +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. + +from sqlalchemy import Column +from sqlalchemy.engine.reflection import Inspector +from sqlalchemy import ForeignKey +from sqlalchemy import Index +from sqlalchemy import MetaData +from sqlalchemy import String +from sqlalchemy import Table + + +def upgrade(migrate_engine): + """Add service_uuid column to volumes.""" + meta = MetaData(bind=migrate_engine) + + Table('services', meta, autoload=True) + volumes = Table('volumes', meta, autoload=True) + if not hasattr(volumes.c, 'service_uuid'): + volumes.create_column(Column('service_uuid', String(36), + ForeignKey('services.uuid'), + nullable=True)) + + index_name = 'volumes_service_uuid_idx' + indexes = Inspector(migrate_engine).get_indexes('volumes') + if index_name not in (i['name'] for i in indexes): + volumes = Table('volumes', meta, autoload=True) + Index(index_name, volumes.c.service_uuid, volumes.c.deleted).create() diff --git a/cinder/db/sqlalchemy/models.py b/cinder/db/sqlalchemy/models.py index eed697cfcdb..15a736745fb 100644 --- a/cinder/db/sqlalchemy/models.py +++ b/cinder/db/sqlalchemy/models.py @@ -242,6 +242,10 @@ class GroupSnapshot(BASE, CinderBase): class Volume(BASE, CinderBase): """Represents a block storage device that can be attached to a vm.""" __tablename__ = 'volumes' + __table_args__ = (Index('volumes_service_uuid_idx', + 'deleted', 'service_uuid'), + CinderBase.__table_args__) + id = Column(String(36), primary_key=True) _name_id = Column(String(36)) # Don't access/modify this directly! @@ -311,6 +315,12 @@ class Volume(BASE, CinderBase): foreign_keys=group_id, primaryjoin='Volume.group_id == Group.id') + service_uuid = Column(String(36), index=True) + service = relationship(Service, + backref="volumes", + foreign_keys=service_uuid, + primaryjoin='Volume.service_uuid == Service.uuid') + class VolumeMetadata(BASE, CinderBase): """Represents a metadata key/value pair for a volume.""" diff --git a/cinder/objects/base.py b/cinder/objects/base.py index fe4195b7648..d71cdbb879e 100644 --- a/cinder/objects/base.py +++ b/cinder/objects/base.py @@ -138,6 +138,7 @@ OBJ_VERSIONS.add('1.27', {'Backup': '1.5', 'BackupImport': '1.5'}) OBJ_VERSIONS.add('1.28', {'Service': '1.5'}) OBJ_VERSIONS.add('1.29', {'Service': '1.6'}) OBJ_VERSIONS.add('1.30', {'RequestSpec': '1.2'}) +OBJ_VERSIONS.add('1.31', {'Volume': '1.7'}) class CinderObjectRegistry(base.VersionedObjectRegistry): diff --git a/cinder/objects/volume.py b/cinder/objects/volume.py index 50be64e2aba..b0d25334f3d 100644 --- a/cinder/objects/volume.py +++ b/cinder/objects/volume.py @@ -60,7 +60,8 @@ class Volume(cleanable.CinderCleanableObject, base.CinderObject, # Version 1.4: Added cluster fields # Version 1.5: Added group # Version 1.6: This object is now cleanable (adds rows to workers table) - VERSION = '1.6' + # Version 1.7: Added service_uuid + VERSION = '1.7' OPTIONAL_FIELDS = ('metadata', 'admin_metadata', 'glance_metadata', 'volume_type', 'volume_attachment', 'consistencygroup', @@ -124,6 +125,7 @@ class Volume(cleanable.CinderCleanableObject, base.CinderObject, nullable=True), 'snapshots': fields.ObjectField('SnapshotList', nullable=True), 'group': fields.ObjectField('Group', nullable=True), + 'service_uuid': fields.StringField(nullable=True), } # NOTE(thangp): obj_extra_fields is used to hold properties that are not @@ -233,7 +235,8 @@ class Volume(cleanable.CinderCleanableObject, base.CinderObject, def obj_make_compatible(self, primitive, target_version): """Make a Volume representation compatible with a target version.""" added_fields = (((1, 4), ('cluster', 'cluster_name')), - ((1, 5), ('group', 'group_id'))) + ((1, 5), ('group', 'group_id')), + ((1, 7), ('service_uuid'))) # Convert all related objects super(Volume, self).obj_make_compatible(primitive, target_version) diff --git a/cinder/tests/unit/objects/test_objects.py b/cinder/tests/unit/objects/test_objects.py index 3dd40e478c0..f8c9cf1ea67 100644 --- a/cinder/tests/unit/objects/test_objects.py +++ b/cinder/tests/unit/objects/test_objects.py @@ -47,7 +47,7 @@ object_data = { 'ServiceList': '1.1-15ecf022a68ddbb8c2a6739cfc9f8f5e', 'Snapshot': '1.5-ac1cdbd5b89588f6a8f44afdf6b8b201', 'SnapshotList': '1.0-15ecf022a68ddbb8c2a6739cfc9f8f5e', - 'Volume': '1.6-7d3bc8577839d5725670d55e480fe95f', + 'Volume': '1.7-0845c5b7b826a4e9019f3684c2f9b132', 'VolumeList': '1.1-15ecf022a68ddbb8c2a6739cfc9f8f5e', 'VolumeAttachment': '1.2-b68b357a1756582b706006ea9de40c9a', 'VolumeAttachmentList': '1.1-15ecf022a68ddbb8c2a6739cfc9f8f5e', diff --git a/cinder/tests/unit/test_db_api.py b/cinder/tests/unit/test_db_api.py index 891f0d8695a..2c2b8109c8e 100644 --- a/cinder/tests/unit/test_db_api.py +++ b/cinder/tests/unit/test_db_api.py @@ -460,6 +460,61 @@ class DBAPIServiceTestCase(BaseTest): self.assertIsInstance(binary_op, sqlalchemy_api.sql.functions.Function) self.assertEqual('binary', binary_op.name) + def test_volume_service_uuid_migrations(self): + # Force create one entry with no UUID + sqlalchemy_api.volume_create(self.ctxt, + {'host': 'host1@lvm-driver1#lvm-driver1'}) + + # Create another one with a valid UUID + sqlalchemy_api.volume_create( + self.ctxt, + {'host': 'host1@lvm-driver1#lvm-driver1', + 'service_uuid': 'a3a593da-7f8d-4bb7-8b4c-f2bc1e0b4824'}) + + # Need a service to query + values = { + 'host': 'host1@lvm-driver1', + 'binary': 'cinder-volume', + 'topic': 'cinder-volume'} + utils.create_service(self.ctxt, values) + + # Run the migration and verify that we updated 1 entry + total, updated = db.volume_service_uuids_online_data_migration( + self.ctxt, 10) + + self.assertEqual(1, total) + self.assertEqual(1, updated) + + def test_volume_service_uuid_migrations_with_limit(self): + """Test db migrate of volumes in batches.""" + db.volume_create( + self.ctxt, {'host': 'host1@lvm-driver1#lvm-driver1'}) + db.volume_create( + self.ctxt, {'host': 'host1@lvm-driver1#lvm-driver1'}) + db.volume_create( + self.ctxt, {'host': 'host1@lvm-driver1#lvm-driver1'}) + + values = { + 'host': 'host1@lvm-driver1', + 'binary': 'cinder-volume', + 'topic': 'cinder-volume', + 'uuid': 'a3a593da-7f8d-4bb7-8b4c-f2bc1e0b4824'} + utils.create_service(self.ctxt, values) + + # Run the migration and verify that we updated 2 entries + total, updated = db.volume_service_uuids_online_data_migration( + self.ctxt, 2) + + self.assertEqual(3, total) + self.assertEqual(2, updated) + + # Now get the ,last one (intentionally setting max > expected) + total, updated = db.volume_service_uuids_online_data_migration( + self.ctxt, 2) + + self.assertEqual(1, total) + self.assertEqual(1, updated) + @ddt.ddt class DBAPIVolumeTestCase(BaseTest): diff --git a/cinder/tests/unit/test_migrations.py b/cinder/tests/unit/test_migrations.py index f83fa2757cf..1536c7fb731 100644 --- a/cinder/tests/unit/test_migrations.py +++ b/cinder/tests/unit/test_migrations.py @@ -372,6 +372,18 @@ class MigrationsMixin(test_migrations.WalkVersionsMixin): self.assertEqual(sorted(['deleted', 'uuid']), sorted(index_columns)) + def _check_114(self, engine, data): + volumes = db_utils.get_table(engine, 'volumes') + self.assertIsInstance(volumes.c.service_uuid.type, + self.VARCHAR_TYPE) + index_columns = [] + for idx in volumes.indexes: + if idx.name == 'volumes_service_uuid_idx': + index_columns = idx.columns.keys() + break + self.assertEqual(sorted(['deleted', 'service_uuid']), + sorted(index_columns)) + def test_walk_versions(self): self.walk_versions(False, False) self.assert_each_foreign_key_is_part_of_an_index() diff --git a/cinder/tests/unit/volume/test_volume.py b/cinder/tests/unit/volume/test_volume.py index d77e48defbb..13e1b323478 100644 --- a/cinder/tests/unit/volume/test_volume.py +++ b/cinder/tests/unit/volume/test_volume.py @@ -168,7 +168,7 @@ class VolumeTestCase(base.BaseVolumeTestCase): m_get_stats.return_value = {'name': 'cinder-volumes'} m_get_filter.return_value = myfilterfunction m_get_goodness.return_value = mygoodnessfunction - manager._report_driver_status(1) + manager._report_driver_status(context.get_admin_context()) self.assertTrue(m_get_stats.called) mock_update.assert_called_once_with(expected) diff --git a/cinder/volume/manager.py b/cinder/volume/manager.py index 491e7cb4ccf..0a2046cacb5 100644 --- a/cinder/volume/manager.py +++ b/cinder/volume/manager.py @@ -206,6 +206,7 @@ class VolumeManager(manager.CleanableManager, self.configuration = config.Configuration(volume_backend_opts, config_group=service_name) self.stats = {} + self.service_uuid = None if not volume_driver: # Get from configuration, which will get the default @@ -236,6 +237,7 @@ class VolumeManager(manager.CleanableManager, "for driver init.") else: curr_active_backend_id = service.active_backend_id + self.service_uuid = service.uuid if self.configuration.suppress_requests_ssl_warnings: LOG.warning("Suppressing requests library SSL Warnings") @@ -678,6 +680,10 @@ class VolumeManager(manager.CleanableManager, # volume stats as these are decremented on delete. self._update_allocated_capacity(volume) + updates = {'service_uuid': self.service_uuid} + volume.update(updates) + volume.save() + LOG.info("Created volume successfully.", resource=volume) return volume.id @@ -2358,6 +2364,24 @@ class VolumeManager(manager.CleanableManager, @periodic_task.periodic_task def _report_driver_status(self, context): + # It's possible during live db migration that the self.service_uuid + # value isn't set (we didn't restart services), so we'll go ahead + # and make this a part of the service periodic + if not self.service_uuid: + svc_host = vol_utils.extract_host(self.host, 'backend') + # We hack this with a try/except for unit tests temporarily + try: + service = objects.Service.get_by_args( + context, + svc_host, + constants.VOLUME_BINARY) + self.service_uuid = service.uuid + except exception.ServiceNotFound: + LOG.warning("Attempt to update service_uuid " + "resulted in a Service NotFound " + "exception, service_uuid field on " + "volumes will be NULL.") + if not self.driver.initialized: if self.driver.configuration.config_group is None: config_group = ''