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 bcd1b2eaa918e5d3fecec14f019451f83462894e..edbfc42bfc7167c157b19798232c1fdec3542313 100644 (file)
--- 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 981e57f00c3ca33b0cbaea5dcbd3debd92161c5b..c826e286038dd54cdf9b37093b3ab95f32798d6d 100644 (file)
--- 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 8dfcf5e3d613547d6c5e37b0dc4ffd75c93bb8a2..ac01271a3ea7fcac94217b7be276fbf6f2dd3240 100644 (file)
--- 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."""