Deleting volume metadata keys with a single request1.9.0

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

9 files changed, 58 insertions, 6 deletions

diff --git a/cinderclient/api_versions.py b/cinderclient/api_versions.py
index bdd975f..383d10f 100644
--- a/cinderclient/api_versions.py
+++ b/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 = {}
 
diff --git a/cinderclient/base.py b/cinderclient/base.py
index 613a066..d30a08e 100644
--- a/cinderclient/base.py
+++ b/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)
diff --git a/cinderclient/openstack/common/apiclient/base.py b/cinderclient/openstack/common/apiclient/base.py
index bea1520..95ae8b9 100644
--- a/cinderclient/openstack/common/apiclient/base.py
+++ b/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)
 
diff --git a/cinderclient/tests/unit/test_base.py b/cinderclient/tests/unit/test_base.py
index 613d09b..587925a 100644
--- a/cinderclient/tests/unit/test_base.py
+++ b/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
 
 
diff --git a/cinderclient/tests/unit/v2/fakes.py b/cinderclient/tests/unit/v2/fakes.py
index a70d63c..3c9d028 100644
--- a/cinderclient/tests/unit/v2/fakes.py
+++ b/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)
diff --git a/cinderclient/tests/unit/v2/test_volumes.py b/cinderclient/tests/unit/v2/test_volumes.py
index 0fb54ce..fbc85aa 100644
--- a/cinderclient/tests/unit/v2/test_volumes.py
+++ b/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']
-        vol = cs.volumes.delete_metadata(1234, keys)
-        cs.assert_called('DELETE', '/volumes/1234/metadata/key1')
+        volume = Volume(self, {'id': '1234', 'metadata': {
+                        'k1': 'v1', 'k2': 'v2', 'k3': 'v3'}})
+        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):
diff --git a/cinderclient/tests/unit/v3/test_shell.py b/cinderclient/tests/unit/v3/test_shell.py
index a3718fa..e6a1eca 100644
--- a/cinderclient/tests/unit/v3/test_shell.py
+++ b/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)
diff --git a/cinderclient/v3/shell.py b/cinderclient/v3/shell.py
index 13097d2..d0ed94c 100644
--- a/cinderclient/v3/shell.py
+++ b/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."""
diff --git a/cinderclient/v3/volumes.py b/cinderclient/v3/volumes.py
index 6862a18..6dd0f9c 100644
--- a/cinderclient/v3/volumes.py
+++ b/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.