From: Gorka Eguileor Date: Fri, 21 Aug 2015 17:06:07 +0000 (+0200) Subject: Improve metadata update operations X-Git-Url: https://review.fuel-infra.org/gitweb?a=commitdiff_plain;h=9bcf96230e4fd6d7998782c0c62cc8188df518ac;p=openstack-build%2Fcinder-build.git Improve metadata update operations Currently our metadata operations in the DB and the API are less than optimal, main issues are: - To update metadata first we get all metadata in the API and add requested update metadata, then on the DB we retrieve each of the metadata keys and update one by one contents. - When deletion of existing keys is requested in the DB we retrieve all metadata contents and for those that are not in the new metadata we retrieve the contents from the DB (again) and delete the entry. This patch changes this so we no longer retrieve metadata in the API, deletion does not retrieve any metadata from the DB and just makes 1 deletion request to the DB, and for changes we retrieve metadata once and then issue a bulk change to create new entries and update existing ones. Partial-Bug: #1238093 Change-Id: I12506541cca61282122e319e3504560f68df225b --- diff --git a/cinder/db/sqlalchemy/api.py b/cinder/db/sqlalchemy/api.py index bcd1b2eaa..edbfc42bf 100644 --- a/cinder/db/sqlalchemy/api.py +++ b/cinder/db/sqlalchemy/api.py @@ -1885,47 +1885,49 @@ def _volume_x_metadata_get_item(context, volume_id, key, model, notfound_exec, return result -def _volume_x_metadata_update(context, volume_id, metadata, delete, - model, notfound_exec, session=None): - if not session: - session = get_session() +def _volume_x_metadata_update(context, volume_id, metadata, delete, model, + session=None): + session = session or get_session() + metadata = metadata.copy() with session.begin(subtransactions=True): - # Set existing metadata to deleted if delete argument is True + # Set existing metadata to deleted if delete argument is True. This is + # commited immediately to the DB if delete: - original_metadata = _volume_x_metadata_get(context, volume_id, - model, session=session) - for meta_key, meta_value in original_metadata.items(): - if meta_key not in metadata: - meta_ref = _volume_x_metadata_get_item(context, volume_id, - meta_key, model, - notfound_exec, - session=session) - meta_ref.update({'deleted': True}) - meta_ref.save(session=session) - - meta_ref = None - - # Now update all existing items with new values, or create new meta - # objects - for meta_key, meta_value in metadata.items(): - - # update the value whether it exists or not - item = {"value": meta_value} - - try: - meta_ref = _volume_x_metadata_get_item(context, volume_id, - meta_key, model, - notfound_exec, - session=session) - except notfound_exec: - meta_ref = model() - item.update({"key": meta_key, "volume_id": volume_id}) - - meta_ref.update(item) - meta_ref.save(session=session) - - return _volume_x_metadata_get(context, volume_id, model) + expected_values = {'volume_id': volume_id} + # We don't want to delete keys we are going to update + if metadata: + expected_values['key'] = db.Not(metadata.keys()) + conditional_update(context, model, {'deleted': True}, + expected_values) + + # Get existing metadata + db_meta = _volume_x_metadata_get_query(context, volume_id, model).all() + save = [] + skip = [] + + # We only want to send changed metadata. + for row in db_meta: + if row.key in metadata: + value = metadata.pop(row.key) + if row.value != value: + # ORM objects will not be saved until we do the bulk save + row.value = value + save.append(row) + continue + skip.append(row) + + # We also want to save non-existent metadata + save.extend(model(key=key, value=value, volume_id=volume_id) + for key, value in metadata.items()) + # Do a bulk save + if save: + session.bulk_save_objects(save, update_changed_only=True) + + # Construct result dictionary with current metadata + save.extend(skip) + result = {row['key']: row['value'] for row in save} + return result def _volume_user_metadata_get_query(context, volume_id, session=None): @@ -1960,7 +1962,6 @@ def _volume_user_metadata_update(context, volume_id, metadata, delete, session=None): return _volume_x_metadata_update(context, volume_id, metadata, delete, models.VolumeMetadata, - exception.VolumeMetadataNotFound, session=session) @@ -1970,7 +1971,6 @@ def _volume_image_metadata_update(context, volume_id, metadata, delete, session=None): return _volume_x_metadata_update(context, volume_id, metadata, delete, models.VolumeGlanceMetadata, - exception.GlanceMetadataNotFound, session=session) @@ -2048,7 +2048,6 @@ def _volume_admin_metadata_update(context, volume_id, metadata, delete, session=None): return _volume_x_metadata_update(context, volume_id, metadata, delete, models.VolumeAdminMetadata, - exception.VolumeAdminMetadataNotFound, session=session) diff --git a/cinder/tests/unit/test_db_api.py b/cinder/tests/unit/test_db_api.py index 981e57f00..c826e2860 100644 --- a/cinder/tests/unit/test_db_api.py +++ b/cinder/tests/unit/test_db_api.py @@ -937,7 +937,12 @@ class DBAPIVolumeTestCase(BaseTest): 'metadata': {'m1': 'v1'}}) volume = db.volume_get(self.ctxt, volume['id']) self.assertEqual('h2', volume['host']) - self.assertEqual(dict(ref_a), dict(volume)) + expected = dict(ref_a) + expected['volume_metadata'] = list(map(dict, + expected['volume_metadata'])) + result = dict(volume) + result['volume_metadata'] = list(map(dict, result['volume_metadata'])) + self.assertEqual(expected, result) def test_volume_update_nonexistent(self): self.assertRaises(exception.VolumeNotFound, db.volume_update, diff --git a/cinder/volume/api.py b/cinder/volume/api.py index 8dfcf5e3d..ac01271a3 100644 --- a/cinder/volume/api.py +++ b/cinder/volume/api.py @@ -1047,26 +1047,9 @@ class API(base.Base): msg = _("The volume metadata cannot be updated when the volume " "is in maintenance mode.") raise exception.InvalidVolume(reason=msg) - if delete: - _metadata = metadata - else: - if meta_type == common.METADATA_TYPES.user: - orig_meta = self.get_volume_metadata(context, volume) - elif meta_type == common.METADATA_TYPES.image: - try: - orig_meta = self.get_volume_image_metadata(context, - volume) - except exception.GlanceMetadataNotFound: - orig_meta = {} - else: - raise exception.InvalidMetadataType(metadata_type=meta_type, - id=volume['id']) - _metadata = orig_meta.copy() - _metadata.update(metadata) - - self._check_metadata_properties(_metadata) + self._check_metadata_properties(metadata) db_meta = self.db.volume_metadata_update(context, volume['id'], - _metadata, + metadata, delete, meta_type) @@ -1076,17 +1059,6 @@ class API(base.Base): resource=volume) return db_meta - def get_volume_metadata_value(self, volume, key): - """Get value of particular metadata key.""" - metadata = volume.get('volume_metadata') - if metadata: - for i in volume['volume_metadata']: - if i['key'] == key: - return i['value'] - LOG.info(_LI("Get volume metadata key completed successfully."), - resource=volume) - return None - @wrap_check_policy def get_volume_admin_metadata(self, context, volume): """Get all administration metadata associated with a volume.""" @@ -1104,23 +1076,15 @@ class API(base.Base): `metadata` argument will be deleted. """ - if delete: - _metadata = metadata - else: - orig_meta = self.get_volume_admin_metadata(context, volume) - _metadata = orig_meta.copy() - _metadata.update(metadata) - - self._check_metadata_properties(_metadata) - - self.db.volume_admin_metadata_update(context, volume['id'], - _metadata, delete) + self._check_metadata_properties(metadata) + db_meta = self.db.volume_admin_metadata_update(context, volume['id'], + metadata, delete) # TODO(jdg): Implement an RPC call for drivers that may use this info LOG.info(_LI("Update volume admin metadata completed successfully."), resource=volume) - return _metadata + return db_meta def get_snapshot_metadata(self, context, snapshot): """Get all metadata associated with a snapshot."""