Deleting volume metadata keys with a single request

Deleting multiple volume metadata keys with a single request
to improve performance. To delete multiple metadata items without
affecting the remaining ones, just update the metadata items with
the updated complete list of ones (without items to delete)
in the body of the request. This patch uses etags to avoid the
lost update problem with volume metadata. The command isn't changed:

 $ cinder metadata volume_id unset k1 k2 k3

Co-Authored-By: Ivan Kolodyazhny <e0ne@e0ne.info>
Depends-On: I575635258c10f299181b8e4cdb51a7ad1f1be764
Implements: blueprint delete-multiple-metadata-keys
Change-Id: I8e18133ffee87c240a7af4b8177683ab99330d9e

cinderclient/api_versions.py

@@ -32,7 +32,7 @@ if not LOG.handlers:
 
 # key is a deprecated version and value is an alternative version.
 DEPRECATED_VERSIONS = {"1": "2"}
-MAX_VERSION = "3.14"
+MAX_VERSION = "3.15"
 
 _SUBSTITUTIONS = {}
 

cinderclient/base.py

@@ -328,7 +328,7 @@ class Manager(common_base.HookableMixin):
 
     def _update(self, url, body, response_key=None, **kwargs):
         self.run_hooks('modify_body_for_update', body, **kwargs)
-        resp, body = self.api.client.put(url, body=body)
+        resp, body = self.api.client.put(url, body=body, **kwargs)
         if response_key:
             return self.resource_class(self, body[response_key], loaded=True,
                                        resp=resp)

cinderclient/openstack/common/apiclient/base.py

@@ -467,6 +467,8 @@ class Resource(RequestIdMixin):
         self._info = info
         self._add_details(info)
         self._loaded = loaded
+        if resp and hasattr(resp, "headers"):
+            self._checksum = resp.headers.get("Etag")
         self.setup()
         self.append_request_ids(resp)
 

cinderclient/tests/unit/test_base.py

@@ -33,6 +33,8 @@ REQUEST_ID = 'req-test-request-id'
 def create_response_obj_with_header():
     resp = Response()
     resp.headers['x-openstack-request-id'] = REQUEST_ID
+    resp.headers['Etag'] = 'd5103bf7b26ff0310200d110da3ed186'
+    resp.status_code = 200
     return resp
 
 

cinderclient/tests/unit/v2/fakes.py

@@ -474,6 +474,10 @@ class FakeHTTPClient(base_client.HTTPClient):
         r = {'volume': self.get_volumes_detail(id=5678)[2]['volumes'][0]}
         return (200, {}, r)
 
+    def get_volumes_1234_metadata(self, **kw):
+        r = {"metadata": {'k1': 'v1', 'k2': 'v2', 'k3': 'v3'}}
+        return (200, {}, r)
+
     def get_volumes_1234_encryption(self, **kw):
         r = {'encryption_key_id': 'id'}
         return (200, {}, r)

cinderclient/tests/unit/v2/test_volumes.py

@@ -177,9 +177,12 @@ class VolumesTest(utils.TestCase):
         self._assert_request_id(vol)
 
     def test_delete_metadata(self):
-        keys = ['key1']
+        volume = Volume(self, {'id': '1234', 'metadata': {
-        vol = cs.volumes.delete_metadata(1234, keys)
+                        'k1': 'v1', 'k2': 'v2', 'k3': 'v3'}})
-        cs.assert_called('DELETE', '/volumes/1234/metadata/key1')
+        keys = ['k1', 'k3']
+        vol = cs.volumes.delete_metadata(volume, keys)
+        cs.assert_called('PUT', '/volumes/1234/metadata',
+                         {'metadata': {'k2': 'v2'}})
         self._assert_request_id(vol)
 
     def test_extend(self):

cinderclient/tests/unit/v3/test_shell.py

@@ -21,6 +21,7 @@ from requests_mock.contrib import fixture as requests_mock_fixture
 from cinderclient import client
 from cinderclient import exceptions
 from cinderclient import shell
+from cinderclient.v3 import volumes
 from cinderclient.tests.unit import utils
 from cinderclient.tests.unit.v3 import fakes
 from cinderclient.tests.unit.fixture_data import keystone_client
@@ -376,3 +377,17 @@ class ShellTest(utils.TestCase):
             '--os-volume-api-version 3.3 message-delete 1234 12345')
         self.assert_called_anytime('DELETE', '/messages/1234')
         self.assert_called_anytime('DELETE', '/messages/12345')
+
+    @mock.patch('cinderclient.utils.find_volume')
+    def test_delete_metadata(self, mock_find_volume):
+        mock_find_volume.return_value = volumes.Volume(self,
+                                                       {'id': '1234',
+                                                        'metadata':
+                                                            {'k1': 'v1',
+                                                             'k2': 'v2',
+                                                             'k3': 'v3'}},
+                                                       loaded = True)
+        expected = {'metadata': {'k2': 'v2'}}
+        self.run_command('--os-volume-api-version 3.15 '
+                         'metadata 1234 unset k1 k3')
+        self.assert_called('PUT', '/volumes/1234/metadata', body=expected)

cinderclient/v3/shell.py

@@ -617,8 +617,16 @@ def do_rename(cs, args):
            metavar='<key=value>',
            nargs='+',
            default=[],
+           end_version='3.14',
            help='Metadata key and value pair to set or unset. '
                 'For unset, specify only the key.')
+@utils.arg('metadata',
+           metavar='<key=value>',
+           nargs='+',
+           default=[],
+           start_version='3.15',
+           help='Metadata key and value pair to set or unset. '
+                'For unset, specify only the key(s): <key key>')
 @utils.service_type('volumev3')
 def do_metadata(cs, args):
     """Sets or deletes volume metadata."""

cinderclient/v3/volumes.py

@@ -435,6 +435,7 @@ class VolumeManager(base.ManagerWithFind):
         return self._create("/volumes/%s/metadata" % base.getid(volume),
                             body, "metadata")
 
+    @api_versions.wraps("2.0")
     def delete_metadata(self, volume, keys):
         """Delete specified keys from volumes metadata.
 
@@ -444,11 +445,28 @@ class VolumeManager(base.ManagerWithFind):
         response_list = []
         for k in keys:
             resp, body = self._delete("/volumes/%s/metadata/%s" %
-                                      (base.getid(volume), k))
+                                     (base.getid(volume), k))
             response_list.append(resp)
 
         return common_base.ListWithMeta([], response_list)
 
+    @api_versions.wraps("3.15")
+    def delete_metadata(self, volume, keys):
+        """Delete specified keys from volumes metadata.
+
+        :param volume: The :class:`Volume`.
+        :param keys: A list of keys to be removed.
+        """
+        data = self._get("/volumes/%s/metadata" % base.getid(volume))
+        metadata = data._info.get("metadata", {})
+        if set(keys).issubset(metadata.keys()):
+            for k in keys:
+                metadata.pop(k)
+            body = {'metadata': metadata}
+            kwargs = {'headers': {'If-Match': data._checksum}}
+            return self._update("/volumes/%s/metadata" % base.getid(volume),
+                                body, **kwargs)
+
     def set_image_metadata(self, volume, metadata):
         """Set a volume's image metadata.